From f69e6be86127982f969d2df224b4bab4a251924b Mon Sep 17 00:00:00 2001 From: Ashfaq <105435085+Ashfaqbs@users.noreply.github.com> Date: Wed, 23 Sep 2026 22:31:50 +0530 Subject: [PATCH 1/4] feat(presets): runtime continuation dispatcher for script composition (#4551) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A "wrap" script's $CORE_SCRIPT reference previously had no runtime resolution mechanism at all: resolve_content("script") composes the full chain into a single spliced file, but nothing ever calls it in a real code path, so preset/extension script composition has been dead since it was added. This adds the runtime side, scoped to Bash for this first increment: - PresetResolver.resolve_script_chain(): returns the ordered file chain for a script name (highest priority down through the terminating "replace" layer), reusing collect_all_layers(). - `specify preset script-chain ` (hidden): prints that chain, one path per line, for the bash adapter to consume. - scripts/bash/continuation-runner.sh: advances the chain one hop, execing into the next layer and threading the remainder via SPECKIT_SCRIPT_CONTINUATION so a further "wrap" layer's own $CORE_SCRIPT call continues correctly. - PresetManager._reconcile_script_chain(): writes the project's canonical .specify/scripts/bash/.sh — a verbatim copy when there's nothing to compose, or a fixed dispatcher stub when there is. The stub carries no stack-specific data, so it resolves the live chain fresh on every invocation and never needs rewriting again: install/remove (which change which layers exist) call it, but enable/disable/set-priority (which only reorder existing layers) don't need to. Fixed a real bug while building the bash integration test: `specify`'s stdout carries CRLF line endings on Windows, and bash's $(...) only strips trailing newlines, not carriage returns, so an unstripped \r was corrupting the exec path. Stripped at the source with `tr -d '\r'` plus a defensive strip in the runner's own read loop. PowerShell and Python adapters, and extending the preset manifest schema so a `type: script` entry can declare per-language files (today `file:` only supports .sh), are left for follow-up commits pending confirmation on the schema shape. Disclosure per CONTRIBUTING.md: implemented with Claude Code, autonomous mode with human review of the diff and test results; the design (continuation representation, dispatcher/runner split, why scripts don't need commands' repeated reconciliation) follows directly from the approach mnriem and I settled on in this issue's discussion. --- scripts/bash/continuation-runner.sh | 45 +++++ src/specify_cli/presets/__init__.py | 166 +++++++++++++++ src/specify_cli/presets/command_resolve.py | 39 ++++ tests/test_presets.py | 223 +++++++++++++++++++++ tests/test_script_continuation_bash.py | 199 ++++++++++++++++++ 5 files changed, 672 insertions(+) create mode 100644 scripts/bash/continuation-runner.sh create mode 100644 tests/test_script_continuation_bash.py diff --git a/scripts/bash/continuation-runner.sh b/scripts/bash/continuation-runner.sh new file mode 100644 index 0000000000..79c60c4b44 --- /dev/null +++ b/scripts/bash/continuation-runner.sh @@ -0,0 +1,45 @@ +#!/usr/bin/env bash +# Advances a script continuation one hop (#4551). +# +# Consumes the head of SPECKIT_SCRIPT_CONTINUATION — a newline-delimited +# list of the remaining script layers below the one that just called +# $CORE_SCRIPT, highest priority first — and execs into it. If more +# layers remain after that hop, the reduced list is re-exported so that, +# if the next layer is itself a "wrap" script, its own $CORE_SCRIPT call +# (which always points back at this same file) continues the chain +# correctly. If nothing remains, the next layer is the terminating +# "replace" layer and SPECKIT_SCRIPT_CONTINUATION is unset for it, since +# a replace layer never references $CORE_SCRIPT. +# +# Deliberately portable to bash 3.2 (macOS's system bash): no mapfile, +# no ^^ case expansion, no associative arrays. +set -e + +if [[ -z "${SPECKIT_SCRIPT_CONTINUATION:-}" ]]; then + echo "ERROR: continuation-runner invoked with no remaining script layers (SPECKIT_SCRIPT_CONTINUATION is unset)" >&2 + exit 1 +fi + +__speckit_remaining=() +while IFS= read -r __speckit_line; do + # Strip a trailing \r defensively (CRLF content from a Windows + # producer), same as the dispatcher stub does at the source. + __speckit_line="${__speckit_line%$'\r'}" + [[ -n "$__speckit_line" ]] && __speckit_remaining+=("$__speckit_line") +done <<< "$SPECKIT_SCRIPT_CONTINUATION" + +if [[ ${#__speckit_remaining[@]} -eq 0 ]]; then + echo "ERROR: continuation-runner found no remaining script layers to run" >&2 + exit 1 +fi + +__speckit_next="${__speckit_remaining[0]}" + +if [[ ${#__speckit_remaining[@]} -gt 1 ]]; then + SPECKIT_SCRIPT_CONTINUATION=$(printf '%s\n' "${__speckit_remaining[@]:1}") + export SPECKIT_SCRIPT_CONTINUATION +else + unset SPECKIT_SCRIPT_CONTINUATION +fi + +exec "$__speckit_next" "$@" diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index eff1e68159..f440866074 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -273,6 +273,45 @@ class PresetCompatibilityError(PresetError): # Scripts only support replace and wrap (prepend/append don't make semantic sense for executable code) VALID_SCRIPT_STRATEGIES = {"replace", "wrap"} +# Fixed dispatcher stub written to a project's canonical +# scripts/bash/.sh when that script name's priority stack requires +# composition (a "wrap" top layer). It carries no stack-specific data — +# only the script's own name, which is stable across preset installs, +# removals, priority changes and enablement toggles — so it never needs +# rewriting once installed; it resolves the live chain fresh on every +# invocation via `specify preset script-chain` (#4551). +_SCRIPT_CONTINUATION_DISPATCHER_TEMPLATE = """#!/usr/bin/env bash +# Generated by specify: continuation dispatcher for the "{script_name}" +# script. Do not edit directly — customize via presets/overrides instead +# (see `specify preset add`, `.specify/templates/overrides/scripts/`). +set -e +set -o pipefail + +SCRIPT_DIR="$(CDPATH="" cd -- "$(dirname -- "${{BASH_SOURCE[0]}}")" && pwd)" +source "$SCRIPT_DIR/common.sh" + +REPO_ROOT=$(get_repo_root) +# Strip carriage returns immediately: on Windows, `specify`'s stdout +# carries CRLF line endings, and bash's $(...) only strips trailing +# newlines, not carriage returns — an unstripped one ends up embedded in +# TOP_LAYER below and corrupts the exec path with "No such file or +# directory". +CHAIN=$( (cd "$REPO_ROOT" && specify preset script-chain "{script_name}") | tr -d '\\r' ) || {{ + echo "ERROR: could not resolve the script chain for '{script_name}'" >&2 + exit 1 +}} + +TOP_LAYER=$(printf '%s\\n' "$CHAIN" | head -n 1) +REMAINING=$(printf '%s\\n' "$CHAIN" | tail -n +2) + +if [[ -n "$REMAINING" ]]; then + export SPECKIT_SCRIPT_CONTINUATION="$REMAINING" + export CORE_SCRIPT="$SCRIPT_DIR/continuation-runner.sh" +fi + +exec "$TOP_LAYER" "$@" +""" + class PresetManifest: """Represents and validates a preset manifest (preset.yml).""" @@ -2386,6 +2425,56 @@ def _merge_pack_registered_skills( if changed: self.registry.update(pack_id, {"registered_skills": merged_skills}) + def _reconcile_script_chain(self, script_name: str) -> None: + """Materialize the project's canonical bash script for ``script_name``. + + Unlike commands (see ``_reconcile_composed_commands``), a script's + canonical file is executed rather than merely read by an agent, so + composition doesn't need to be re-spliced every time the priority + stack changes: a small continuation dispatcher resolves the live + chain via ``specify preset script-chain`` at *invocation* time + (#4551), so later `specify preset enable/disable/set-priority` + calls take effect without touching this file again. Only install + and remove — which change *which* layers exist at all, not merely + their order — ever need to call this. + + Writes ``/scripts/bash/.sh``: + - A single-layer chain (no composition) is a verbatim copy of that + layer's file, so overrides/extensions take effect even without + a ``"wrap"`` script in the stack. + - A multi-layer chain is the fixed dispatcher stub below. It only + resolves the chain and execs the top layer, so growing or + shrinking the chain later (further installs/removes, or a + priority/enablement change) never requires rewriting it. + + Does nothing when the script name has no resolvable layers at all + (leaves whatever canonical file, if any, already exists alone). + """ + resolver = PresetResolver(self.project_root) + chain = resolver.resolve_script_chain(script_name) + if not chain: + return + + canonical = ( + self.project_root / ".specify" / "scripts" / "bash" / f"{script_name}.sh" + ) + canonical.parent.mkdir(parents=True, exist_ok=True) + + if len(chain) == 1: + content = chain[0].read_text(encoding="utf-8") + canonical.write_text(content, encoding="utf-8") + else: + canonical.write_text( + _SCRIPT_CONTINUATION_DISPATCHER_TEMPLATE.format( + script_name=script_name + ), + encoding="utf-8", + ) + + if os.name != "nt": + mode = canonical.stat().st_mode + canonical.chmod(mode | 0o111) + def _reconcile_skills( self, command_names: List[str], @@ -3955,6 +4044,29 @@ def install_from_directory( stacklevel=2, ) + # Scripts don't need the same repeated reconciliation as commands: + # once a script name's canonical file is a continuation dispatcher, + # it re-resolves the live stack on every invocation (#4551), so + # later priority/enablement changes take effect without touching + # this file again. Only install (here) and remove() ever need to + # write it. + script_names = [ + t["name"] + for t in manifest.templates + if t.get("type") == "script" + ] + for script_name in script_names: + try: + self._reconcile_script_chain(script_name) + except Exception as exc: + import warnings + warnings.warn( + f"Post-install script reconciliation failed for " + f"{manifest.id} script '{script_name}': {exc}. " + f"Run 'specify preset script-chain {script_name}' to diagnose.", + stacklevel=2, + ) + # Materialize constitution-template changes only for projects that opt # into the constitution-sync preset. The core /constitution command # resolves this template on demand; constitution-sync preserves the @@ -4173,6 +4285,7 @@ def remove(self, pack_id: str) -> bool: # entirely and _unregister_skills would restore core/extension # content instead of a surviving lower-priority preset's override. removed_cmd_names = set() + removed_script_names = set() removed_constitution = any( path.exists() for path in ( @@ -4211,6 +4324,10 @@ def remove(self, pack_id: str) -> bool: for alias in tmpl.get("aliases", []): if isinstance(alias, str): removed_cmd_names.add(alias) + if tmpl.get("type") == "script": + name = tmpl.get("name") + if isinstance(name, str): + removed_script_names.add(name) except PresetValidationError: # Invalid manifest — skip alias extraction; primary command # names from registered_commands are still unregistered. @@ -4352,6 +4469,19 @@ def remove(self, pack_id: str) -> bool: stacklevel=2, ) + if removed_script_names: + for script_name in removed_script_names: + try: + self._reconcile_script_chain(script_name) + except Exception as exc: + import warnings + warnings.warn( + f"Post-removal script reconciliation failed for " + f"{pack_id} script '{script_name}': {exc}. " + f"Run 'specify preset script-chain {script_name}' to diagnose.", + stacklevel=2, + ) + if removed_constitution: try: self._reconcile_constitution() @@ -5992,6 +6122,42 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: return layers + def resolve_script_chain(self, script_name: str) -> List[Path]: + """Return the ordered chain of files backing a script's continuation. + + Scripts are executed rather than merely read, so — unlike + templates and commands — their composition doesn't need to be + spliced into a single file ahead of time. A ``"wrap"`` script + contains a literal ``$CORE_SCRIPT`` reference that a runtime + continuation runner resolves hop by hop, so this returns the + stack of files that reference forms, in priority order, rather + than composed content. + + This walks the same priority stack as ``resolve_content()`` for + ``template_type="script"``: the highest-priority layer down + through the nearest layer with strategy ``"replace"`` + (inclusive), which terminates the chain — only ``"replace"`` and + ``"wrap"`` are valid script strategies, so a chain longer than + one entry always has a ``"wrap"`` top. Layers below the + terminating ``"replace"`` layer are never reachable and are + omitted, matching ``resolve_content()``. + + Returns an empty list when the script name has no layers, or + when none of them has strategy ``"replace"`` (composition has no + base to terminate on — the same condition under which + ``resolve_content()`` returns ``None``). + """ + layers = self.collect_all_layers(script_name, "script") + if not layers: + return [] + base_idx = next( + (i for i, layer in enumerate(layers) if layer["strategy"] == "replace"), + None, + ) + if base_idx is None: + return [] + return [layer["path"] for layer in layers[: base_idx + 1]] + def _find_bundled_core( self, template_name: str, diff --git a/src/specify_cli/presets/command_resolve.py b/src/specify_cli/presets/command_resolve.py index 7256307160..f3012ff4a8 100644 --- a/src/specify_cli/presets/command_resolve.py +++ b/src/specify_cli/presets/command_resolve.py @@ -11,6 +11,45 @@ from ._commands import preset_app +@preset_app.command("script-chain", hidden=True) +def preset_script_chain( + script_name: str = typer.Argument( + ..., help="Script name to resolve (e.g., setup-plan)" + ), +): + """Print the resolved script continuation chain, one path per line. + + Internal command consumed by the generated script continuation + dispatcher and runner (see ``PresetResolver.resolve_script_chain``); + not intended for interactive use. Output is the ordered chain of + file paths from highest priority to the terminating "replace" layer, + one absolute path per line, with no other output. + """ + from .. import _require_specify_project + from . import PresetResolver + + if re.fullmatch(r"[a-z0-9-]+", script_name) is None: + typer.echo( + f"Error: invalid script name '{script_name}'; " + "use lowercase letters, digits, and hyphens", + err=True, + ) + raise typer.Exit(1) + + project_root = _require_specify_project() + resolver = PresetResolver(project_root) + chain = resolver.resolve_script_chain(script_name) + if not chain: + typer.echo( + f"Error: could not resolve a script chain for '{script_name}'", + err=True, + ) + raise typer.Exit(1) + + for path in chain: + typer.echo(str(path)) + + @preset_app.command("resolve") def preset_resolve( template_name: str = typer.Argument( diff --git a/tests/test_presets.py b/tests/test_presets.py index a7e590b071..36cc911801 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -12346,6 +12346,229 @@ def test_layers_read_strategy_from_manifest(self, project_dir, temp_dir, valid_p assert layers[1]["strategy"] == "replace" +class TestResolveScriptChain: + """Test PresetResolver.resolve_script_chain() (#4551). + + Unlike resolve_content(), which splices script content together + ahead of time, this returns the ordered *files* a runtime + continuation dispatcher walks hop by hop, so priority/enablement + changes take effect without re-splicing. + """ + + def test_missing_script_returns_empty(self, project_dir): + resolver = PresetResolver(project_dir) + assert resolver.resolve_script_chain("does-not-exist") == [] + + def test_single_core_layer(self, project_dir): + """A script with no overrides resolves to a one-entry chain.""" + core_script = ( + project_dir / ".specify" / "templates" / "scripts" / "solo-script.sh" + ) + core_script.parent.mkdir(parents=True, exist_ok=True) + core_script.write_text("echo core\n") + + resolver = PresetResolver(project_dir) + chain = resolver.resolve_script_chain("solo-script") + assert chain == [core_script] + + def test_wrap_over_core_orders_top_first( + self, project_dir, temp_dir, valid_pack_data + ): + core_script = ( + project_dir / ".specify" / "templates" / "scripts" / "wrapped.sh" + ) + core_script.parent.mkdir(parents=True, exist_ok=True) + core_script.write_text("echo core\n") + + manager = PresetManager(project_dir) + pack_dir = _create_pack( + temp_dir, + valid_pack_data, + "wrap-pack", + "echo before\n$CORE_SCRIPT\necho after\n", + strategy="wrap", + template_type="script", + template_name="wrapped", + ) + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + chain = resolver.resolve_script_chain("wrapped") + assert len(chain) == 2 + assert chain[0].read_text().startswith("echo before") + assert chain[1] == core_script + + def test_replace_layer_terminates_chain( + self, project_dir, temp_dir, valid_pack_data + ): + """A "replace" layer wins outright; nothing below it is reachable.""" + core_script = ( + project_dir / ".specify" / "templates" / "scripts" / "overridden.sh" + ) + core_script.parent.mkdir(parents=True, exist_ok=True) + core_script.write_text("echo core\n") + + manager = PresetManager(project_dir) + pack_dir = _create_pack( + temp_dir, + valid_pack_data, + "replace-pack", + "echo replaced\n", + strategy="replace", + template_type="script", + template_name="overridden", + ) + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + chain = resolver.resolve_script_chain("overridden") + assert len(chain) == 1 + assert chain[0].read_text() == "echo replaced\n" + + def test_no_replace_base_returns_empty(self, project_dir, temp_dir, valid_pack_data): + """A wrap-only stack with no core/replace layer can't terminate.""" + manager = PresetManager(project_dir) + pack_dir = _create_pack( + temp_dir, + valid_pack_data, + "dangling-wrap", + "echo before\n$CORE_SCRIPT\n", + strategy="wrap", + template_type="script", + template_name="dangling", + ) + manager.install_from_directory(pack_dir, "0.1.5") + + resolver = PresetResolver(project_dir) + assert resolver.resolve_script_chain("dangling") == [] + + def test_priority_change_reorders_chain_without_reinstall( + self, project_dir, temp_dir, valid_pack_data + ): + """Changing priority alone (no reinstall) must reorder the next + resolve_script_chain() call — this is the property the runtime + dispatcher relies on to avoid re-materializing on every + enable/disable/set-priority change.""" + core_script = ( + project_dir / ".specify" / "templates" / "scripts" / "reorder-me.sh" + ) + core_script.parent.mkdir(parents=True, exist_ok=True) + core_script.write_text("echo core\n") + + manager = PresetManager(project_dir) + for pid, prio in [("layer-a", 5), ("layer-b", 10)]: + pack_dir = _create_pack( + temp_dir, + valid_pack_data, + pid, + f"echo {pid} before\n$CORE_SCRIPT\necho {pid} after\n", + strategy="wrap", + template_type="script", + template_name="reorder-me", + ) + manager.install_from_directory(pack_dir, "0.1.5", priority=prio) + + resolver = PresetResolver(project_dir) + chain_before = resolver.resolve_script_chain("reorder-me") + assert "layer-a" in str(chain_before[0]) + + manager.registry.update("layer-a", {"priority": 20}) + + chain_after = resolver.resolve_script_chain("reorder-me") + assert "layer-b" in str(chain_after[0]) + assert "layer-a" in str(chain_after[1]) + + +class TestScriptChainReconciliation: + """Test PresetManager._reconcile_script_chain() (#4551). + + Verifies the canonical ``.specify/scripts/bash/.sh`` file that + agents actually invoke: a plain copy when there's nothing to + compose, and a fixed continuation dispatcher stub when there is. + """ + + def _canonical(self, project_dir, name): + return project_dir / ".specify" / "scripts" / "bash" / f"{name}.sh" + + def test_install_replace_script_copies_content_verbatim( + self, project_dir, temp_dir, valid_pack_data + ): + manager = PresetManager(project_dir) + pack_dir = _create_pack( + temp_dir, + valid_pack_data, + "override-only", + "echo overridden\n", + strategy="replace", + template_type="script", + template_name="plain-override", + ) + manager.install_from_directory(pack_dir, "0.1.5") + + canonical = self._canonical(project_dir, "plain-override") + assert canonical.is_file() + assert canonical.read_text() == "echo overridden\n" + + def test_install_wrap_script_writes_dispatcher_stub( + self, project_dir, temp_dir, valid_pack_data + ): + core_script = ( + project_dir / ".specify" / "templates" / "scripts" / "stub-target.sh" + ) + core_script.parent.mkdir(parents=True, exist_ok=True) + core_script.write_text("echo core\n") + + manager = PresetManager(project_dir) + pack_dir = _create_pack( + temp_dir, + valid_pack_data, + "stub-pack", + "echo before\n$CORE_SCRIPT\necho after\n", + strategy="wrap", + template_type="script", + template_name="stub-target", + ) + manager.install_from_directory(pack_dir, "0.1.5") + + canonical = self._canonical(project_dir, "stub-target") + content = canonical.read_text() + assert "specify preset script-chain \"stub-target\"" in content + assert "SPECKIT_SCRIPT_CONTINUATION" in content + assert "CORE_SCRIPT" in content + # The dispatcher carries no stack-specific data (no reference to + # "stub-pack" or the resolved core path) — it resolves fresh at + # every invocation instead. + assert "stub-pack" not in content + + def test_remove_last_composing_preset_reverts_to_core_copy( + self, project_dir, temp_dir, valid_pack_data + ): + core_script = ( + project_dir / ".specify" / "templates" / "scripts" / "revert-me.sh" + ) + core_script.parent.mkdir(parents=True, exist_ok=True) + core_script.write_text("echo core\n") + + manager = PresetManager(project_dir) + pack_dir = _create_pack( + temp_dir, + valid_pack_data, + "revert-pack", + "echo before\n$CORE_SCRIPT\n", + strategy="wrap", + template_type="script", + template_name="revert-me", + ) + manager.install_from_directory(pack_dir, "0.1.5") + + canonical = self._canonical(project_dir, "revert-me") + assert "SPECKIT_SCRIPT_CONTINUATION" in canonical.read_text() + + manager.remove("revert-pack") + + assert canonical.read_text() == "echo core\n" + + class TestRemoveReconciliation: """Test that removing a preset re-registers the next layer's command.""" diff --git a/tests/test_script_continuation_bash.py b/tests/test_script_continuation_bash.py new file mode 100644 index 0000000000..2a0ac9862c --- /dev/null +++ b/tests/test_script_continuation_bash.py @@ -0,0 +1,199 @@ +"""End-to-end bash test for the script continuation dispatcher (#4551). + +Proves the runtime chain actually executes correctly, not just that the +resolver computes the right file list: installs two "wrap" script +presets over a core script, invokes the *canonical* materialized script +exactly as a coding agent would (via its fixed frontmatter path), and +checks the process actually ran outer-before -> inner-before -> core -> +inner-after -> outer-after, with args and exit status propagated. +""" + +import os +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest +import yaml + +from specify_cli.presets import PresetManager + +from tests.conftest import requires_bash + +PROJECT_ROOT = Path(__file__).resolve().parent.parent +COMMON_SH = PROJECT_ROOT / "scripts" / "bash" / "common.sh" +CONTINUATION_RUNNER_SH = PROJECT_ROOT / "scripts" / "bash" / "continuation-runner.sh" + +# The `specify` console script must be resolvable from the bash +# subprocess's PATH, since the dispatcher shells out to `specify preset +# script-chain`. Resolve it relative to the running interpreter rather +# than assuming a particular PATH setup, so this works the same way in a +# venv, in CI, or via `pip install -e .` locally. +_BIN_DIR = str(Path(sys.executable).parent) + + +def _clean_env() -> dict: + env = os.environ.copy() + for key in list(env): + if key.startswith("SPECIFY_"): + env.pop(key) + env["PATH"] = _BIN_DIR + os.pathsep + env.get("PATH", "") + return env + + +@pytest.fixture +def project_dir(tmp_path: Path) -> Path: + project = tmp_path / "project" + (project / ".specify" / "templates" / "scripts").mkdir(parents=True) + (project / ".specify" / "scripts" / "bash").mkdir(parents=True) + shutil.copy(COMMON_SH, project / ".specify" / "scripts" / "bash" / "common.sh") + shutil.copy( + CONTINUATION_RUNNER_SH, + project / ".specify" / "scripts" / "bash" / "continuation-runner.sh", + ) + return project + + +def _install_wrap_layer( + project_dir: Path, + temp_dir: Path, + pack_id: str, + priority: int, + script_name: str, + label: str, +) -> None: + pack_dir = temp_dir / pack_id + (pack_dir / "scripts").mkdir(parents=True) + (pack_dir / "scripts" / f"{script_name}.sh").write_text( + "#!/usr/bin/env bash\n" + "set -e\n" + f'echo "{label}-before $*"\n' + '"$CORE_SCRIPT" "$@"\n' + f'echo "{label}-after"\n' + ) + manifest = { + "schema_version": "1.0", + "preset": { + "id": pack_id, + "name": pack_id, + "version": "0.1.0", + "description": "test", + "author": "Test Author", + "repository": "https://github.com/test/test-pack", + "license": "MIT", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "script", + "name": script_name, + "file": f"scripts/{script_name}.sh", + "strategy": "wrap", + } + ] + }, + } + (pack_dir / "preset.yml").write_text(yaml.safe_dump(manifest), encoding="utf-8") + + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.0", priority=priority) + + +@requires_bash +def test_two_layer_continuation_runs_in_priority_order( + project_dir: Path, tmp_path: Path +) -> None: + core_script = project_dir / ".specify" / "templates" / "scripts" / "chained.sh" + core_script.write_text( + '#!/usr/bin/env bash\necho "core $*"\n' + ) + + temp_dir = tmp_path / "packs" + temp_dir.mkdir() + # priority 1 (outer, checked first) and priority 5 (inner) + _install_wrap_layer(project_dir, temp_dir, "outer-pack", 1, "chained", "outer") + _install_wrap_layer(project_dir, temp_dir, "inner-pack", 5, "chained", "inner") + + canonical = project_dir / ".specify" / "scripts" / "bash" / "chained.sh" + assert canonical.is_file(), "install should have written the dispatcher stub" + + result = subprocess.run( + ["bash", str(canonical), "arg1", "arg2"], + cwd=project_dir, + capture_output=True, + text=True, + check=False, + env=_clean_env(), + ) + assert result.returncode == 0, result.stderr + result.stdout + lines = [line for line in result.stdout.splitlines() if line.strip()] + assert lines == [ + "outer-before arg1 arg2", + "inner-before arg1 arg2", + "core arg1 arg2", + "inner-after", + "outer-after", + ] + + +@requires_bash +def test_priority_change_takes_effect_without_reinstall( + project_dir: Path, tmp_path: Path +) -> None: + """The whole point of the continuation model (#4551): swapping + priority must change execution order on the *next invocation*, + without touching the canonical file at all.""" + core_script = project_dir / ".specify" / "templates" / "scripts" / "reorder.sh" + core_script.write_text('#!/usr/bin/env bash\necho "core"\n') + + temp_dir = tmp_path / "packs" + temp_dir.mkdir() + _install_wrap_layer(project_dir, temp_dir, "layer-a", 1, "reorder", "a") + _install_wrap_layer(project_dir, temp_dir, "layer-b", 5, "reorder", "b") + + canonical = project_dir / ".specify" / "scripts" / "bash" / "reorder.sh" + before_bytes = canonical.read_bytes() + + manager = PresetManager(project_dir) + manager.registry.update("layer-a", {"priority": 20}) + + # The dispatcher file itself must be byte-for-byte unchanged — + # reordering must not require rewriting it. + assert canonical.read_bytes() == before_bytes + + result = subprocess.run( + ["bash", str(canonical)], + cwd=project_dir, + capture_output=True, + text=True, + check=False, + env=_clean_env(), + ) + assert result.returncode == 0, result.stderr + result.stdout + lines = [line for line in result.stdout.splitlines() if line.strip()] + assert lines == ["b-before ", "a-before ", "core", "a-after", "b-after"] + + +@requires_bash +def test_nonzero_exit_status_propagates_through_the_chain( + project_dir: Path, tmp_path: Path +) -> None: + core_script = project_dir / ".specify" / "templates" / "scripts" / "failing.sh" + core_script.write_text('#!/usr/bin/env bash\nexit 7\n') + + temp_dir = tmp_path / "packs" + temp_dir.mkdir() + _install_wrap_layer(project_dir, temp_dir, "wrap-pack", 1, "failing", "w") + + canonical = project_dir / ".specify" / "scripts" / "bash" / "failing.sh" + result = subprocess.run( + ["bash", str(canonical)], + cwd=project_dir, + capture_output=True, + text=True, + check=False, + env=_clean_env(), + ) + assert result.returncode == 7 From 73ad6044510af3a97804bf3a98533e988827b012 Mon Sep 17 00:00:00 2001 From: Ashfaq <105435085+Ashfaqbs@users.noreply.github.com> Date: Thu, 24 Sep 2026 06:59:28 +0530 Subject: [PATCH 2/4] fix(presets): address review feedback on script continuation dispatcher - Generate the runner next to the dispatcher instead of shipping a new scripts/bash file, so shared-infra inventories are unchanged and projects initialized earlier still get it. - Run layers through bash so preset/override scripts need no execute bit. - Always install the dispatcher while a preset provides the script, so enable/disable/set-priority changes that alter chain length stay valid. - Refuse to write through symlinked destinations; remove a stale generated dispatcher when its last provider is removed. - Resolve the built-in Bash core from scripts/bash and validate that every wrap layer contains $CORE_SCRIPT. - Fall back to python3 -m specify_cli when no specify executable is on PATH. - Register the hidden script-chain command in the stable-order test. Disclosure per CONTRIBUTING.md: implemented with Claude Code, autonomous mode, changes verified by the test suite and manual bash runs. --- scripts/bash/continuation-runner.sh | 45 ---- src/specify_cli/presets/__init__.py | 198 +++++++++++++----- src/specify_cli/presets/command_resolve.py | 8 +- .../specify_cli/presets/test_registration.py | 1 + tests/test_presets.py | 87 +++++++- tests/test_script_continuation_bash.py | 5 - 6 files changed, 235 insertions(+), 109 deletions(-) delete mode 100644 scripts/bash/continuation-runner.sh diff --git a/scripts/bash/continuation-runner.sh b/scripts/bash/continuation-runner.sh deleted file mode 100644 index 79c60c4b44..0000000000 --- a/scripts/bash/continuation-runner.sh +++ /dev/null @@ -1,45 +0,0 @@ -#!/usr/bin/env bash -# Advances a script continuation one hop (#4551). -# -# Consumes the head of SPECKIT_SCRIPT_CONTINUATION — a newline-delimited -# list of the remaining script layers below the one that just called -# $CORE_SCRIPT, highest priority first — and execs into it. If more -# layers remain after that hop, the reduced list is re-exported so that, -# if the next layer is itself a "wrap" script, its own $CORE_SCRIPT call -# (which always points back at this same file) continues the chain -# correctly. If nothing remains, the next layer is the terminating -# "replace" layer and SPECKIT_SCRIPT_CONTINUATION is unset for it, since -# a replace layer never references $CORE_SCRIPT. -# -# Deliberately portable to bash 3.2 (macOS's system bash): no mapfile, -# no ^^ case expansion, no associative arrays. -set -e - -if [[ -z "${SPECKIT_SCRIPT_CONTINUATION:-}" ]]; then - echo "ERROR: continuation-runner invoked with no remaining script layers (SPECKIT_SCRIPT_CONTINUATION is unset)" >&2 - exit 1 -fi - -__speckit_remaining=() -while IFS= read -r __speckit_line; do - # Strip a trailing \r defensively (CRLF content from a Windows - # producer), same as the dispatcher stub does at the source. - __speckit_line="${__speckit_line%$'\r'}" - [[ -n "$__speckit_line" ]] && __speckit_remaining+=("$__speckit_line") -done <<< "$SPECKIT_SCRIPT_CONTINUATION" - -if [[ ${#__speckit_remaining[@]} -eq 0 ]]; then - echo "ERROR: continuation-runner found no remaining script layers to run" >&2 - exit 1 -fi - -__speckit_next="${__speckit_remaining[0]}" - -if [[ ${#__speckit_remaining[@]} -gt 1 ]]; then - SPECKIT_SCRIPT_CONTINUATION=$(printf '%s\n' "${__speckit_remaining[@]:1}") - export SPECKIT_SCRIPT_CONTINUATION -else - unset SPECKIT_SCRIPT_CONTINUATION -fi - -exec "$__speckit_next" "$@" diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index f440866074..8b663d45d0 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -273,17 +273,20 @@ class PresetCompatibilityError(PresetError): # Scripts only support replace and wrap (prepend/append don't make semantic sense for executable code) VALID_SCRIPT_STRATEGIES = {"replace", "wrap"} -# Fixed dispatcher stub written to a project's canonical -# scripts/bash/.sh when that script name's priority stack requires -# composition (a "wrap" top layer). It carries no stack-specific data — -# only the script's own name, which is stable across preset installs, -# removals, priority changes and enablement toggles — so it never needs -# rewriting once installed; it resolves the live chain fresh on every -# invocation via `specify preset script-chain` (#4551). +# Generated files for script composition (#4551). Both carry no +# stack-specific data — only the script's own name — so they never need +# rewriting when priorities/enablement change; the dispatcher resolves the +# live chain on every invocation via `specify preset script-chain`. They are +# generated (not shipped under scripts/bash) so projects initialized before +# this feature get the runner too, and shared-infra inventories are unchanged. +_SCRIPT_DISPATCHER_MARKER = "# speckit-generated: script continuation dispatcher" +_SCRIPT_RUNNER_MARKER = "# speckit-generated: script continuation runner" +_SCRIPT_RUNNER_NAME = "continuation-runner.sh" + _SCRIPT_CONTINUATION_DISPATCHER_TEMPLATE = """#!/usr/bin/env bash -# Generated by specify: continuation dispatcher for the "{script_name}" -# script. Do not edit directly — customize via presets/overrides instead -# (see `specify preset add`, `.specify/templates/overrides/scripts/`). +# speckit-generated: script continuation dispatcher +# Generated by specify for the "{script_name}" script. Do not edit directly; +# customize via presets/overrides instead (`specify preset add`). set -e set -o pipefail @@ -291,12 +294,17 @@ class PresetCompatibilityError(PresetError): source "$SCRIPT_DIR/common.sh" REPO_ROOT=$(get_repo_root) -# Strip carriage returns immediately: on Windows, `specify`'s stdout -# carries CRLF line endings, and bash's $(...) only strips trailing -# newlines, not carriage returns — an unstripped one ends up embedded in -# TOP_LAYER below and corrupts the exec path with "No such file or -# directory". -CHAIN=$( (cd "$REPO_ROOT" && specify preset script-chain "{script_name}") | tr -d '\\r' ) || {{ +if command -v specify >/dev/null 2>&1; then + SPECIFY_CMD=(specify) +elif command -v python3 >/dev/null 2>&1; then + SPECIFY_CMD=(python3 -m specify_cli) +else + echo "ERROR: 'specify' is required to resolve the '{script_name}' script chain" >&2 + exit 1 +fi +# Strip carriage returns: on Windows the CLI emits CRLF, and $(...) only +# strips trailing newlines, so a stray CR would corrupt the exec path. +CHAIN=$( (cd "$REPO_ROOT" && "${{SPECIFY_CMD[@]}}" preset script-chain "{script_name}") | tr -d '\\r' ) || {{ echo "ERROR: could not resolve the script chain for '{script_name}'" >&2 exit 1 }} @@ -306,10 +314,48 @@ class PresetCompatibilityError(PresetError): if [[ -n "$REMAINING" ]]; then export SPECKIT_SCRIPT_CONTINUATION="$REMAINING" - export CORE_SCRIPT="$SCRIPT_DIR/continuation-runner.sh" + export CORE_SCRIPT="$SCRIPT_DIR/{runner_name}" +fi + +# Run via bash so layers need no execute bit (preset copies keep source modes). +exec bash "$TOP_LAYER" "$@" +""" + +_SCRIPT_CONTINUATION_RUNNER = """#!/usr/bin/env bash +# speckit-generated: script continuation runner +# Advances the script continuation one hop: consumes the head of +# SPECKIT_SCRIPT_CONTINUATION (newline-delimited remaining layers, highest +# priority first) and runs it, re-exporting the remainder so a further +# "wrap" layer's own $CORE_SCRIPT call continues correctly. +# Portable to bash 3.2: no mapfile or associative arrays. +set -e + +if [[ -z "${SPECKIT_SCRIPT_CONTINUATION:-}" ]]; then + echo "ERROR: continuation runner invoked with no remaining script layers" >&2 + exit 1 +fi + +__speckit_remaining=() +while IFS= read -r __speckit_line; do + __speckit_line="${__speckit_line%$'\\r'}" + [[ -n "$__speckit_line" ]] && __speckit_remaining+=("$__speckit_line") +done <<< "$SPECKIT_SCRIPT_CONTINUATION" + +if [[ ${#__speckit_remaining[@]} -eq 0 ]]; then + echo "ERROR: continuation runner found no remaining script layers" >&2 + exit 1 fi -exec "$TOP_LAYER" "$@" +__speckit_next="${__speckit_remaining[0]}" + +if [[ ${#__speckit_remaining[@]} -gt 1 ]]; then + SPECKIT_SCRIPT_CONTINUATION=$(printf '%s\\n' "${__speckit_remaining[@]:1}") + export SPECKIT_SCRIPT_CONTINUATION +else + unset SPECKIT_SCRIPT_CONTINUATION +fi + +exec bash "$__speckit_next" "$@" """ @@ -2428,52 +2474,61 @@ def _merge_pack_registered_skills( def _reconcile_script_chain(self, script_name: str) -> None: """Materialize the project's canonical bash script for ``script_name``. - Unlike commands (see ``_reconcile_composed_commands``), a script's - canonical file is executed rather than merely read by an agent, so - composition doesn't need to be re-spliced every time the priority - stack changes: a small continuation dispatcher resolves the live - chain via ``specify preset script-chain`` at *invocation* time - (#4551), so later `specify preset enable/disable/set-priority` - calls take effect without touching this file again. Only install - and remove — which change *which* layers exist at all, not merely - their order — ever need to call this. - - Writes ``/scripts/bash/.sh``: - - A single-layer chain (no composition) is a verbatim copy of that - layer's file, so overrides/extensions take effect even without - a ``"wrap"`` script in the stack. - - A multi-layer chain is the fixed dispatcher stub below. It only - resolves the chain and execs the top layer, so growing or - shrinking the chain later (further installs/removes, or a - priority/enablement change) never requires rewriting it. - - Does nothing when the script name has no resolvable layers at all - (leaves whatever canonical file, if any, already exists alone). + Scripts are executed rather than read, so composition is resolved at + *invocation* time (#4551): while any preset provides this script, the + canonical ``.specify/scripts/bash/.sh`` is a fixed dispatcher + that resolves the live chain via ``specify preset script-chain``. + Because it is used for every chain length (including one layer), later + enable/disable/set-priority changes never require rewriting it. When + no preset provides the script any more, the bundled core script is + restored (or the generated dispatcher removed if there is no core). """ resolver = PresetResolver(self.project_root) chain = resolver.resolve_script_chain(script_name) - if not chain: - return + scripts_dir = self.project_root / ".specify" / "scripts" / "bash" + canonical = scripts_dir / f"{script_name}.sh" - canonical = ( - self.project_root / ".specify" / "scripts" / "bash" / f"{script_name}.sh" + # Never write through symlinks: a link here could redirect the write + # outside the project. + for target in (self.project_root / ".specify" / "scripts", scripts_dir, canonical): + if target.is_symlink(): + raise PresetValidationError( + f"Refusing to write script through symlink: {target}" + ) + + provided_by_preset = any( + self.project_root / ".specify" / "presets" in path.parents + or (self.project_root / ".specify" / "templates" / "overrides") + in path.parents + for path in chain ) - canonical.parent.mkdir(parents=True, exist_ok=True) - if len(chain) == 1: - content = chain[0].read_text(encoding="utf-8") - canonical.write_text(content, encoding="utf-8") - else: + if provided_by_preset: + scripts_dir.mkdir(parents=True, exist_ok=True) + runner = scripts_dir / _SCRIPT_RUNNER_NAME + runner.write_text( + _SCRIPT_CONTINUATION_RUNNER, encoding="utf-8", newline="\n" + ) canonical.write_text( _SCRIPT_CONTINUATION_DISPATCHER_TEMPLATE.format( - script_name=script_name + script_name=script_name, runner_name=_SCRIPT_RUNNER_NAME ), encoding="utf-8", + newline="\n", ) + if os.name != "nt": + for generated in (runner, canonical): + generated.chmod(generated.stat().st_mode | 0o111) + return - if os.name != "nt": - mode = canonical.stat().st_mode - canonical.chmod(mode | 0o111) + # No preset layer left: restore core, or drop a stale generated stub. + if chain: + canonical.parent.mkdir(parents=True, exist_ok=True) + canonical.write_text(chain[-1].read_text(encoding="utf-8"), encoding="utf-8") + elif canonical.is_file(): + first_line = canonical.read_text(encoding="utf-8").splitlines()[:2] + if any(_SCRIPT_DISPATCHER_MARKER in line for line in first_line): + canonical.unlink() def _reconcile_skills( self, @@ -6147,7 +6202,15 @@ def resolve_script_chain(self, script_name: str) -> List[Path]: base to terminate on — the same condition under which ``resolve_content()`` returns ``None``). """ - layers = self.collect_all_layers(script_name, "script") + layers = list(self.collect_all_layers(script_name, "script")) + if not any(layer["strategy"] == "replace" for layer in layers): + # collect_all_layers() only knows scripts/.sh; the real + # built-in Bash assets live under scripts/bash/. + bundled = self._find_bundled_bash_script(script_name) + if bundled is not None: + layers.append( + {"path": bundled, "source": "core (bundled)", "strategy": "replace"} + ) if not layers: return [] base_idx = next( @@ -6156,7 +6219,34 @@ def resolve_script_chain(self, script_name: str) -> List[Path]: ) if base_idx is None: return [] - return [layer["path"] for layer in layers[: base_idx + 1]] + chain_layers = layers[: base_idx + 1] + for layer in chain_layers: + if layer["strategy"] != "wrap": + continue + try: + body = layer["path"].read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + raise PresetValidationError( + f"Cannot read wrap script '{layer['source']}': {exc}" + ) from exc + if "$CORE_SCRIPT" not in body: + raise PresetValidationError( + f"Wrap strategy in '{layer['source']}' is missing the " + f"$CORE_SCRIPT placeholder; executing it would silently " + f"drop every lower layer." + ) + return [layer["path"] for layer in chain_layers] + + def _find_bundled_bash_script(self, script_name: str) -> Optional[Path]: + """Locate the built-in Bash script from the core pack or source tree.""" + try: + from specify_cli import _locate_core_pack, _repo_root + except ImportError: + return None + core_pack = _locate_core_pack() + base = core_pack if core_pack is not None else _repo_root() + candidate = base / "scripts" / "bash" / f"{script_name}.sh" + return candidate if candidate.is_file() else None def _find_bundled_core( self, diff --git a/src/specify_cli/presets/command_resolve.py b/src/specify_cli/presets/command_resolve.py index f3012ff4a8..083cae7214 100644 --- a/src/specify_cli/presets/command_resolve.py +++ b/src/specify_cli/presets/command_resolve.py @@ -26,7 +26,7 @@ def preset_script_chain( one absolute path per line, with no other output. """ from .. import _require_specify_project - from . import PresetResolver + from . import PresetResolver, PresetValidationError if re.fullmatch(r"[a-z0-9-]+", script_name) is None: typer.echo( @@ -38,7 +38,11 @@ def preset_script_chain( project_root = _require_specify_project() resolver = PresetResolver(project_root) - chain = resolver.resolve_script_chain(script_name) + try: + chain = resolver.resolve_script_chain(script_name) + except PresetValidationError as exc: + typer.echo(f"Error: {exc}", err=True) + raise typer.Exit(1) if not chain: typer.echo( f"Error: could not resolve a script chain for '{script_name}'", diff --git a/tests/specify_cli/presets/test_registration.py b/tests/specify_cli/presets/test_registration.py index 0aa4d74b34..dc5110089a 100644 --- a/tests/specify_cli/presets/test_registration.py +++ b/tests/specify_cli/presets/test_registration.py @@ -13,6 +13,7 @@ def test_preset_commands_registered_once_in_stable_order(): "remove", "update", "search", + "script-chain", "resolve", "info", "set-priority", diff --git a/tests/test_presets.py b/tests/test_presets.py index 36cc911801..e15f4f712b 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -12479,6 +12479,36 @@ def test_priority_change_reorders_chain_without_reinstall( assert "layer-a" in str(chain_after[1]) + def test_wrap_missing_core_script_placeholder_is_rejected( + self, project_dir, temp_dir, valid_pack_data + ): + core = project_dir / ".specify" / "templates" / "scripts" / "bad-wrap.sh" + core.parent.mkdir(parents=True, exist_ok=True) + core.write_text("echo core\n") + pack_dir = _create_pack( + temp_dir, valid_pack_data, "bad-wrap-pack", "echo no placeholder\n", + strategy="wrap", template_type="script", template_name="bad-wrap", + ) + PresetManager(project_dir).install_from_directory(pack_dir, "0.1.5") + with pytest.raises(PresetValidationError, match="CORE_SCRIPT"): + PresetResolver(project_dir).resolve_script_chain("bad-wrap") + + def test_builtin_bash_script_is_found_as_core_base( + self, project_dir, temp_dir, valid_pack_data + ): + """A wrap over a real built-in (scripts/bash/setup-plan.sh) must + resolve without a fabricated .specify/templates/scripts core.""" + pack_dir = _create_pack( + temp_dir, valid_pack_data, "real-wrap", "echo a\n$CORE_SCRIPT\n", + strategy="wrap", template_type="script", template_name="setup-plan", + ) + PresetManager(project_dir).install_from_directory(pack_dir, "0.1.5") + chain = PresetResolver(project_dir).resolve_script_chain("setup-plan") + assert len(chain) == 2 + assert chain[1].name == "setup-plan.sh" + assert chain[1].parent.name == "bash" + + class TestScriptChainReconciliation: """Test PresetManager._reconcile_script_chain() (#4551). @@ -12490,9 +12520,12 @@ class TestScriptChainReconciliation: def _canonical(self, project_dir, name): return project_dir / ".specify" / "scripts" / "bash" / f"{name}.sh" - def test_install_replace_script_copies_content_verbatim( + def test_install_replace_script_uses_dispatcher_and_resolves_override( self, project_dir, temp_dir, valid_pack_data ): + """Even a single-layer override gets the dispatcher, so a later + priority/enable change that makes it a multi-layer chain needs no + rewrite of the canonical file.""" manager = PresetManager(project_dir) pack_dir = _create_pack( temp_dir, @@ -12506,8 +12539,56 @@ def test_install_replace_script_copies_content_verbatim( manager.install_from_directory(pack_dir, "0.1.5") canonical = self._canonical(project_dir, "plain-override") + assert "speckit-generated: script continuation dispatcher" in canonical.read_text() + chain = PresetResolver(project_dir).resolve_script_chain("plain-override") + assert [p.read_text() for p in chain] == ["echo overridden\n"] + + def test_install_writes_runner_next_to_dispatcher( + self, project_dir, temp_dir, valid_pack_data + ): + """The runner is generated, not shipped, so pre-existing projects get it.""" + manager = PresetManager(project_dir) + pack_dir = _create_pack( + temp_dir, valid_pack_data, "runner-pack", "echo x\n", + template_type="script", template_name="needs-runner", + ) + manager.install_from_directory(pack_dir, "0.1.5") + runner = project_dir / ".specify" / "scripts" / "bash" / "continuation-runner.sh" + assert "script continuation runner" in runner.read_text() + + def test_remove_only_provider_removes_generated_dispatcher( + self, project_dir, temp_dir, valid_pack_data + ): + manager = PresetManager(project_dir) + pack_dir = _create_pack( + temp_dir, valid_pack_data, "solo-pack", "echo x\n", + template_type="script", template_name="no-core-script", + ) + manager.install_from_directory(pack_dir, "0.1.5") + canonical = self._canonical(project_dir, "no-core-script") assert canonical.is_file() - assert canonical.read_text() == "echo overridden\n" + manager.remove("solo-pack") + assert not canonical.exists() + + def test_reconcile_refuses_symlinked_destination( + self, project_dir, temp_dir, valid_pack_data + ): + import os + outside = temp_dir / "outside" + outside.mkdir() + scripts = project_dir / ".specify" / "scripts" + scripts.mkdir(parents=True) + try: + os.symlink(outside, scripts / "bash", target_is_directory=True) + except (OSError, NotImplementedError): + pytest.skip("symlinks unavailable") + pack_dir = _create_pack( + temp_dir, valid_pack_data, "sym-pack", "echo x\n", + template_type="script", template_name="sym-script", + ) + with pytest.warns(UserWarning, match="symlink"): + PresetManager(project_dir).install_from_directory(pack_dir, "0.1.5") + assert list(outside.iterdir()) == [] def test_install_wrap_script_writes_dispatcher_stub( self, project_dir, temp_dir, valid_pack_data @@ -12532,7 +12613,7 @@ def test_install_wrap_script_writes_dispatcher_stub( canonical = self._canonical(project_dir, "stub-target") content = canonical.read_text() - assert "specify preset script-chain \"stub-target\"" in content + assert 'preset script-chain "stub-target"' in content assert "SPECKIT_SCRIPT_CONTINUATION" in content assert "CORE_SCRIPT" in content # The dispatcher carries no stack-specific data (no reference to diff --git a/tests/test_script_continuation_bash.py b/tests/test_script_continuation_bash.py index 2a0ac9862c..6471a8557e 100644 --- a/tests/test_script_continuation_bash.py +++ b/tests/test_script_continuation_bash.py @@ -23,7 +23,6 @@ PROJECT_ROOT = Path(__file__).resolve().parent.parent COMMON_SH = PROJECT_ROOT / "scripts" / "bash" / "common.sh" -CONTINUATION_RUNNER_SH = PROJECT_ROOT / "scripts" / "bash" / "continuation-runner.sh" # The `specify` console script must be resolvable from the bash # subprocess's PATH, since the dispatcher shells out to `specify preset @@ -48,10 +47,6 @@ def project_dir(tmp_path: Path) -> Path: (project / ".specify" / "templates" / "scripts").mkdir(parents=True) (project / ".specify" / "scripts" / "bash").mkdir(parents=True) shutil.copy(COMMON_SH, project / ".specify" / "scripts" / "bash" / "common.sh") - shutil.copy( - CONTINUATION_RUNNER_SH, - project / ".specify" / "scripts" / "bash" / "continuation-runner.sh", - ) return project From 4d92033828f6ee62cfe1834a97af357ec5da3284 Mon Sep 17 00:00:00 2001 From: Ashfaq <105435085+Ashfaqbs@users.noreply.github.com> Date: Thu, 24 Sep 2026 19:30:34 +0530 Subject: [PATCH 3/4] fix(presets): harden script dispatcher writes and chain detection - Write the dispatcher, runner and restored core script through the shared safe-destination helpers so a symlinked ancestor such as .specify cannot redirect writes or the cleanup unlink outside the project. - Reserve the `common` and `continuation-runner` script names; a dispatcher for either would overwrite the runtime helpers every dispatcher relies on. - Add specify_cli/__main__.py so the `python3 -m specify_cli` fallback works when no `specify` executable is on PATH. - Decide whether a preset provides a script from all active declarations, not the truncated chain, so an extension replace layer above a preset no longer leaves the preset without a dispatcher. Disclosure per CONTRIBUTING.md: implemented with Claude Code, autonomous mode, changes verified by the test suite (one pre-existing PowerShell failure unrelated to this change). --- src/specify_cli/__main__.py | 6 +++ src/specify_cli/presets/__init__.py | 59 +++++++++++++++---------- tests/test_presets.py | 67 +++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 23 deletions(-) create mode 100644 src/specify_cli/__main__.py diff --git a/src/specify_cli/__main__.py b/src/specify_cli/__main__.py new file mode 100644 index 0000000000..0ae7916d64 --- /dev/null +++ b/src/specify_cli/__main__.py @@ -0,0 +1,6 @@ +"""Allow ``python -m specify_cli`` to run the CLI.""" + +from specify_cli import main + +if __name__ == "__main__": + main() diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 8b663d45d0..5de07c2b89 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -282,6 +282,10 @@ class PresetCompatibilityError(PresetError): _SCRIPT_DISPATCHER_MARKER = "# speckit-generated: script continuation dispatcher" _SCRIPT_RUNNER_MARKER = "# speckit-generated: script continuation runner" _SCRIPT_RUNNER_NAME = "continuation-runner.sh" +# Names the generated runtime relies on: a dispatcher for ``common`` would +# overwrite the library every dispatcher sources, and one for +# ``continuation-runner`` would overwrite the runner itself. +_RESERVED_SCRIPT_NAMES = frozenset({"common", "continuation-runner"}) _SCRIPT_CONTINUATION_DISPATCHER_TEMPLATE = """#!/usr/bin/env bash # speckit-generated: script continuation dispatcher @@ -2483,38 +2487,46 @@ def _reconcile_script_chain(self, script_name: str) -> None: no preset provides the script any more, the bundled core script is restored (or the generated dispatcher removed if there is no core). """ + if script_name in _RESERVED_SCRIPT_NAMES: + raise PresetValidationError( + f"Script name '{script_name}' is reserved for the generated " + f"script continuation runtime and cannot be provided by a preset." + ) resolver = PresetResolver(self.project_root) chain = resolver.resolve_script_chain(script_name) scripts_dir = self.project_root / ".specify" / "scripts" / "bash" canonical = scripts_dir / f"{script_name}.sh" - # Never write through symlinks: a link here could redirect the write - # outside the project. - for target in (self.project_root / ".specify" / "scripts", scripts_dir, canonical): - if target.is_symlink(): - raise PresetValidationError( - f"Refusing to write script through symlink: {target}" - ) - - provided_by_preset = any( - self.project_root / ".specify" / "presets" in path.parents - or (self.project_root / ".specify" / "templates" / "overrides") - in path.parents - for path in chain + # Validate every ancestor (not just the leaf): a symlinked ``.specify`` + # would otherwise redirect the writes and the cleanup unlink outside + # the project. + _ensure_safe_shared_directory(self.project_root, scripts_dir) + _ensure_safe_shared_destination(self.project_root, canonical) + runner = scripts_dir / _SCRIPT_RUNNER_NAME + _ensure_safe_shared_destination(self.project_root, runner) + + # Judge "provided by a preset" from every active declaration, not the + # truncated chain: an extension's replace layer can end the chain + # above a lower-priority preset, and a later set-priority or enable + # change must still find the dispatcher already in place. + preset_roots = ( + self.project_root / ".specify" / "presets", + self.project_root / ".specify" / "templates" / "overrides", + ) + provided_by_preset = bool(chain) and any( + root in layer["path"].parents + for layer in resolver.collect_all_layers(script_name, "script") + for root in preset_roots ) if provided_by_preset: - scripts_dir.mkdir(parents=True, exist_ok=True) - runner = scripts_dir / _SCRIPT_RUNNER_NAME - runner.write_text( - _SCRIPT_CONTINUATION_RUNNER, encoding="utf-8", newline="\n" - ) - canonical.write_text( + _write_shared_text(self.project_root, runner, _SCRIPT_CONTINUATION_RUNNER) + _write_shared_text( + self.project_root, + canonical, _SCRIPT_CONTINUATION_DISPATCHER_TEMPLATE.format( script_name=script_name, runner_name=_SCRIPT_RUNNER_NAME ), - encoding="utf-8", - newline="\n", ) if os.name != "nt": for generated in (runner, canonical): @@ -2523,8 +2535,9 @@ def _reconcile_script_chain(self, script_name: str) -> None: # No preset layer left: restore core, or drop a stale generated stub. if chain: - canonical.parent.mkdir(parents=True, exist_ok=True) - canonical.write_text(chain[-1].read_text(encoding="utf-8"), encoding="utf-8") + _write_shared_text( + self.project_root, canonical, chain[-1].read_text(encoding="utf-8") + ) elif canonical.is_file(): first_line = canonical.read_text(encoding="utf-8").splitlines()[:2] if any(_SCRIPT_DISPATCHER_MARKER in line for line in first_line): diff --git a/tests/test_presets.py b/tests/test_presets.py index e15f4f712b..fbe500097d 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -12621,6 +12621,73 @@ def test_install_wrap_script_writes_dispatcher_stub( # every invocation instead. assert "stub-pack" not in content + @pytest.mark.parametrize("reserved", ["common", "continuation-runner"]) + def test_reserved_helper_names_are_refused(self, project_dir, reserved): + """A dispatcher named after a runtime helper would overwrite it.""" + manager = PresetManager(project_dir) + with pytest.raises(PresetValidationError, match="reserved"): + manager._reconcile_script_chain(reserved) + + def test_symlinked_specify_dir_is_not_written_through( + self, project_dir, temp_dir, valid_pack_data + ): + """A symlinked ancestor must not redirect the dispatcher writes.""" + outside = temp_dir / "outside" + (outside / "scripts" / "bash").mkdir(parents=True) + real_specify = project_dir / ".specify" + moved = temp_dir / "moved-specify" + real_specify.rename(moved) + try: + real_specify.symlink_to(outside, target_is_directory=True) + except (OSError, NotImplementedError): + real_specify.mkdir() + pytest.skip("symlinks unavailable on this platform") + manager = PresetManager(project_dir) + with pytest.raises(ValueError, match="symlink"): + manager._reconcile_script_chain("linked") + assert list((outside / "scripts" / "bash").iterdir()) == [] + + def test_dispatcher_written_when_extension_layer_ends_chain( + self, project_dir, temp_dir, valid_pack_data, monkeypatch + ): + """An extension replace layer above a preset truncates the chain, but + the preset is still an active declaration and needs the dispatcher so + a later priority change is not inert.""" + preset_file = ( + project_dir / ".specify" / "presets" / "p1" / "scripts" / "shadowed.sh" + ) + preset_file.parent.mkdir(parents=True) + preset_file.write_text("echo preset\n") + ext_file = temp_dir / "ext-shadowed.sh" + ext_file.write_text("echo ext\n") + monkeypatch.setattr( + PresetResolver, "resolve_script_chain", lambda self, name: [ext_file] + ) + monkeypatch.setattr( + PresetResolver, + "collect_all_layers", + lambda self, name, kind: [ + {"path": ext_file, "source": "extension", "strategy": "replace"}, + {"path": preset_file, "source": "preset", "strategy": "replace"}, + ], + ) + PresetManager(project_dir)._reconcile_script_chain("shadowed") + canonical = self._canonical(project_dir, "shadowed") + assert "script continuation dispatcher" in canonical.read_text() + + def test_python_module_entry_point_runs_cli(self): + """The dispatcher's ``python3 -m specify_cli`` fallback needs __main__.""" + import subprocess + import sys + + result = subprocess.run( + [sys.executable, "-m", "specify_cli", "preset", "script-chain", "--help"], + capture_output=True, + text=True, + encoding="utf-8", + ) + assert result.returncode == 0, result.stderr + def test_remove_last_composing_preset_reverts_to_core_copy( self, project_dir, temp_dir, valid_pack_data ): From e77742ae30ccd71fb77b4b15e5d2a68f46bb454e Mon Sep 17 00:00:00 2001 From: Ashfaqbs <105435085+Ashfaqbs@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:50:40 +0530 Subject: [PATCH 4/4] fix(presets): keep restored core script executable after last preset removal --- src/specify_cli/presets/__init__.py | 4 ++++ tests/test_presets.py | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 5de07c2b89..acb44f5982 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2538,6 +2538,10 @@ def _reconcile_script_chain(self, script_name: str) -> None: _write_shared_text( self.project_root, canonical, chain[-1].read_text(encoding="utf-8") ) + # Command frontmatter executes this path directly, and + # _write_shared_text writes 0644. + if os.name != "nt": + canonical.chmod(canonical.stat().st_mode | 0o111) elif canonical.is_file(): first_line = canonical.read_text(encoding="utf-8").splitlines()[:2] if any(_SCRIPT_DISPATCHER_MARKER in line for line in first_line): diff --git a/tests/test_presets.py b/tests/test_presets.py index fbe500097d..6d06fa50c3 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -15,6 +15,7 @@ import json import tarfile import shutil +import sys import zipfile from contextlib import contextmanager from pathlib import Path @@ -12570,6 +12571,29 @@ def test_remove_only_provider_removes_generated_dispatcher( manager.remove("solo-pack") assert not canonical.exists() + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX execute bits") + def test_remove_last_provider_restores_executable_core_script( + self, project_dir, temp_dir, valid_pack_data + ): + """Frontmatter runs the canonical path directly, so the restored core + script must stay executable after the dispatcher is replaced.""" + core_script = ( + project_dir / ".specify" / "templates" / "scripts" / "exec-restore.sh" + ) + core_script.parent.mkdir(parents=True, exist_ok=True) + core_script.write_text("echo core\n") + manager = PresetManager(project_dir) + pack_dir = _create_pack( + temp_dir, valid_pack_data, "exec-pack", "echo x\n", + strategy="wrap", template_type="script", template_name="exec-restore", + ) + manager.install_from_directory(pack_dir, "0.1.5") + manager.remove("exec-pack") + + canonical = self._canonical(project_dir, "exec-restore") + assert canonical.read_text() == "echo core\n" + assert canonical.stat().st_mode & 0o111 + def test_reconcile_refuses_symlinked_destination( self, project_dir, temp_dir, valid_pack_data ):