You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Continues #4551 — implementing the design @mnriem and I settled on there. This is the first increment, scoped to Bash, since the full evidence plan (all three runtimes) is a larger unit of work than I want to land in one shot without a checkpoint.
What this adds
PresetResolver.resolve_script_chain(name) — returns the ordered file chain for a script name (highest priority down through the terminating "replace" layer), reusing collect_all_layers() rather than a second resolution protocol.
specify preset script-chain <name> (hidden) — prints that chain, one path per line. This is what the runtime adapter shells out to.
scripts/bash/continuation-runner.sh — advances the chain one hop: execs into the next layer, and if more remain, re-exports the reduced list via SPECKIT_SCRIPT_CONTINUATION so a further "wrap" layer's own $CORE_SCRIPT call continues correctly. No identity, no position — just the list it was handed, matching what we agreed on.
PresetManager._reconcile_script_chain() — writes the project's canonical .specify/scripts/bash/<name>.sh: a verbatim copy when there's nothing to compose, or a fixed dispatcher stub when there is.
Why this needs less reconciliation than commands
The dispatcher stub carries no stack-specific data — only the script's own name — so once it's written it never needs rewriting again: it resolves the live chain fresh on every invocation, not at materialization time. That means install/remove (which change which layers exist) call _reconcile_script_chain, but enable/disable/set-priority (which only reorder existing layers) don't need to touch it at all — priority and enablement changes take effect on the next run for free. Commands can't do this because an agent reads the command file directly; a script is executed, so it can carry its own resolution logic.
Evidence
TestResolveScriptChain / TestScriptChainReconciliation in tests/test_presets.py: ordering, the "replace"-terminates-the-chain rule, a dangling-wrap-with-no-base case, install writing the stub vs. a verbatim copy, remove reverting it, and — importantly — a priority change via registry.update() alone (no reinstall) reordering resolve_script_chain()'s output.
tests/test_script_continuation_bash.py: a real bash subprocess test with two wrap layers over a core script, asserting the actual execution order (outer-before → inner-before → core → inner-after → outer-after), arg propagation, exit-status propagation, and — the core claim — that a priority swap reorders execution while the dispatcher file stays byte-for-byte unchanged. (These are marked requires_bash like the existing bash-dependent tests in this suite; I additionally hand-verified the full chain and the priority-swap case against the real bash.exe outside pytest, both passing, since bash wasn't resolvable via my sandbox's bare-bash PATH probe.)
Full existing suite (tests/test_presets.py, 605 pre-existing tests) still green — no changes to command/template resolution behavior.
Found and fixed a real bug while building the bash test: specify's stdout carries CRLF line endings on Windows, and bash's $(...) only strips trailing newlines, not carriage returns — an unstripped \r was corrupting the exec path with "No such file or directory". Fixed with tr -d '\r' at the source, plus a defensive strip in the runner's own read loop.
Left for follow-up, pending your input
PowerShell and Python adapters. Structurally the same idea (ContinuationRunner.ps1, a run_script_continuation() in scripts/python/common.py), but I wanted this checkpoint reviewed first rather than tripling the diff.
Manifest schema for cross-runtime scripts. Today a type: script entry's file: field only ever points at a .sh — there's no way for a preset to declare .ps1/.py counterparts of the same script name. That's a schema decision (separate fields? a per-language provides block?) I'd rather you weigh in on before I build the other two adapters against a shape I invented unilaterally.
Ran the full existing test suite plus the new tests locally; happy to adjust the chain representation or the reconcile-only-on-install/remove call sites if you see it differently.
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, the install/remove-only reconciliation argument) follows directly from what we worked out together on #4551.
…github#4551)
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 <name>` (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/<name>.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.
- 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.
Pushed 73ad604 addressing the Copilot review and the failing CI.
Changed:
The runner is now generated next to the dispatcher instead of shipped as a new scripts/bash file. That was what broke the ~40 file-inventory tests, and it also means projects initialized before this feature still get the runner.
Layers are run via bash <layer>, so preset copies and overrides need no execute bit.
While any preset provides a script, the canonical file is always the dispatcher (even for a one-layer chain), so enable/disable/set-priority changes that move the chain between one and many layers stay valid without a rewrite.
Symlinked .specify/scripts, bash or <name>.sh destinations are refused. Removing the last provider deletes the generated dispatcher, or restores the bundled core.
The built-in Bash core is now found under scripts/bash/, and every wrap layer is validated to contain $CORE_SCRIPT before the chain is returned.
Added the hidden script-chain command to the stable-order registration test.
Not addressed in this push, and I'd like your steer:
specify init --force / integration upgrades rewrite .specify/scripts/bash without reconciling already-installed script presets, so an enabled wrapper would be replaced by core until the next preset install/remove. I think the right fix is a hook in the shared-infra refresh path, but that touches code outside the presets module.
The dispatcher still needs the CLI at run time. It now falls back to python3 -m specify_cli, but a one-shot uvx project with neither on PATH fails with a clear error. Is a project-local resolver acceptable, or is requiring the CLI fine?
Verified: tests/test_presets.py, tests/specify_cli/presets, a sample of the integration inventory tests and ruff@0.15.0 pass locally. The three real-bash tests skip on my Windows machine, so I ran the two-layer chain and priority-swap scenarios by hand against Git Bash and both give the expected order.
Disclosure: this comment and the commit were produced with Claude Code in autonomous mode, on behalf of @Ashfaqbs.
- 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).
Pushed 4d92033 addressing the latest automated review round:
Symlinked ancestors: the dispatcher, runner and restored core script are now written through the shared safe-destination helpers, which validate every ancestor.
Helper name collisions: common and continuation-runner are reserved script names and are refused.
Python fallback: added specify_cli/__main__.py so python3 -m specify_cli runs the CLI.
Chain truncation: whether a preset provides a script is now decided from all active declarations rather than the truncated chain.
The registration-order test item was already covered in 73ad604. Still open and waiting on maintainer direction: reconciling script chains on init --force/upgrades, and resolving the chain without depending on a current specify on PATH.
Disclosure per CONTRIBUTING.md: this comment and the commit were produced with Claude Code in autonomous mode, on behalf of @Ashfaqbs. Tests pass except one PowerShell test that fails on unmodified main.
Restored core script lost its execute bit: fixed. When the last composing preset is removed, the core script is written back and its execute bits are restored, as the dispatcher branch already does. Regression test added (POSIX only, skipped on Windows).
Override-only scripts (.specify/templates/overrides/scripts/<name>.sh added by hand after init): not addressed in this PR. Nothing runs when a user drops a file there, so the fix needs a trigger, most likely generating dispatchers for canonical Bash scripts during shared-infrastructure setup. That changes the init file inventory for every project, so I would rather have your call on it than fold it into this increment. Happy to do it as a follow-up if you want it.
Still open and waiting on maintainer direction: reconciling script chains on init --force/upgrades, and resolving the chain without depending on a current specify on PATH.
Disclosure per CONTRIBUTING.md: this comment and the commit were produced with Claude Code in autonomous mode, on behalf of @Ashfaqbs. Ran tests/test_presets.py and tests/test_script_continuation_bash.py: 624 passed, 4 skipped.
Move script-chain command into its dedicated module
src/specify_cli/presets/command_resolve.py:15
The CLI architecture requires each real command to live in its matching command_<name>.py module (design/cli.md:32-58), with tests mirroring that structure. Defining script-chain inside command_resolve.py obscures command ownership; move it to command_script_chain.py and import it at the intended stable registration position.
Port the script continuation dispatcher onto the private presets modules
introduced by the domain split (github#4747), and move its tests into the
mirrored tests/specify_cli/presets layout.
Merged main (e30639f). #4747 split presets/__init__.py into private modules, so the dispatcher code now lives in _manager.py and _resolver.py and its tests moved into tests/specify_cli/presets/ (the shared pack helper is now create_pack in _helpers.py). No behavior change; the presets tests and the new bash continuation tests pass. The one failure in the full run, test_setup_tasks_ps_core_template_resolved (PowerShell JSON output on Windows), is in code this PR doesn't touch.
- tests/specify_cli/presets/test_manager.py: drop redundant local
`import os` / `import subprocess` that shadowed the already-imported
module-level names, fixing the ruff F401/F811 failures blocking CI.
- _manager.py / _resolver.py: fix mojibake (`—`) back to em dashes in
comments and docstrings.
- _manager.py: add `PresetManager.reconcile_all_script_chains()`, and
call it from command_init.py right after `install_shared_infra`.
`specify init --force` (and forced integration upgrades) rewrite
`.specify/scripts/bash/<name>.sh` from the bundled core, clobbering
any generated continuation dispatcher for an already-enabled script
preset. Unlike commands/skills, script presets were never
re-reconciled after that refresh, so a wrapped script would go inert
until its preset was reinstalled. Covered by a new regression test.
Addresses the ruff/pytest CI failures and the Copilot review round on
this PR. Several other findings from that review (execute-bit
preservation after restore, symlink-safe writes/deletes, reserved
script names, wrap-strategy validation, the `python3 -m specify_cli`
fallback, and chain truncation vs. extension layers) were already
fixed in earlier commits on this branch. PowerShell/Python script
parity and the general override-only reconciliation-trigger gap
(pre-existing for commands too, not a regression here) remain
out of scope for this first increment, per the PR description.
Assisted-by: Claude Code (model: claude-sonnet-5, autonomous)
Posted by @Ashfaqbs with GitHub Copilot (model: Claude Sonnet 5, autonomous); this comment is fully AI-drafted.
Pushed 9d49447 addressing the CI failures and this review round.
CI
ruff was failing on tests/specify_cli/presets/test_manager.py: two local import os / import subprocess inside test methods shadowed the already-present module-level imports (F401 unused at top + F811 redefinition). Removed the redundant local imports; ruff check src tests is clean.
The pytest (ubuntu-latest, 3.14) failure was a transient PyPI fetch timeout during uv sync (Failed to fetch: https://pypi.org/simple/click/ ... operation timed out), not a code issue — should resolve on rerun.
Mojibake (—) in _manager.py and _resolver.py comments/docstrings, corrected to real em dashes.
specify init --force (and forced integration upgrades) rewrite .specify/scripts/bash/<name>.sh from the bundled core via install_shared_infra, clobbering a generated continuation dispatcher for an already-enabled script preset. Added PresetManager.reconcile_all_script_chains() and wired it in right after that refresh in command_init.py, with a regression test (test_reconcile_all_script_chains_restores_dispatcher_after_shared_infra_refresh).
Copilot findings — already fixed in earlier commits on this branch (the review appears to have run against an older revision):
Execute-bit preservation after _write_shared_text restores/writes the dispatcher or core script.
Symlink-safe writes/deletes via _ensure_safe_shared_directory / _ensure_safe_shared_destination on every ancestor, not just the leaf.
Reserved script names (common, continuation-runner) rejected via _RESERVED_SCRIPT_NAMES.
Wrap-strategy validation: a wrap missing $CORE_SCRIPT is now rejected rather than silently truncating the chain (see test_wrap_missing_core_script_placeholder_is_rejected).
python3 -m specify_cli fallback: src/specify_cli/__main__.py now exists.
Chain-truncation vs. extension layers: _reconcile_script_chain's provided_by_preset check uses every active declaration from collect_all_layers, not the truncated chain, so a lower-priority preset isn't treated as inert just because an extension's replace layer currently wins.
Stale generated stub cleanup when the last provider is removed (single-layer preset-provided scripts are always dispatched, never verbatim-copied, so the dispatcher marker check on removal reliably catches this case).
Explicitly deferred, out of scope for this increment:
PowerShell/Python script parity — the PR description already scopes this PR to Bash only, with PS/Python as a follow-up increment.
The general "override-only" reconciliation trigger (a standalone .specify/templates/overrides/scripts/<name>.sh with no preset behind it never gets reconciled outside install/remove/this refresh path) — this is a pre-existing limitation shared with commands (_reconcile_composed_commands has the same install/remove/re-registration-only triggers), not a regression introduced by this PR. Fixing it properly means redesigning the override-reconciliation trigger surface for both commands and scripts together, which is a larger, separate unit of work.
Local verification: ruff check src tests clean; full pytest suite 6809 passed / 387 skipped (one unrelated pre-existing failure in test_setup_tasks.py::test_setup_tasks_ps_core_template_resolved, confirmed present on upstream/main and untouched by this branch — a local PowerShell/JSON-encoding environment quirk, not a regression).
Leading-hyphen script names are parsed as Click options
src/specify_cli/presets/_manager.py:189
Preset script names currently permit a leading hyphen (_manifest.py:275-279), so a valid name such as -audit is passed to Click as an option and rejected before preset_script_chain() can validate it. Add the option terminator before the generated positional argument.
Unquoted CORE_SCRIPT paths break wrappers in spaced directories
src/specify_cli/presets/_manager.py:200
CORE_SCRIPT is now a filesystem path, but the existing wrapper contract allows $CORE_SCRIPT "$@" (including the reproduction in #4551). In a project path containing spaces, that unquoted expansion is split into multiple command words and the chain fails; the new test only uses "$CORE_SCRIPT", so it misses this compatibility case. Preserve the placeholder contract or explicitly migrate/validate wrapper syntax, with a regression test using a spaced project path.
…fy uvx limitation
Address the two Copilot findings from the latest review round:
- integration upgrade --force and integration switch/use --force both
refresh shared infra the same way specify init --force does, which
overwrites .specify/scripts/bash/<name>.sh with the bundled core and
clobbers a generated continuation dispatcher for an already-enabled
script preset. Both paths now call reconcile_all_script_chains()
afterward, mirroring the call already wired into command_init.py.
- The dispatcher's runtime CLI resolution (specify, or python3 -m
specify_cli) cannot work under the documented one-time uvx flow,
which discards its environment right after init. Rather than
redesigning script-chain resolution to be fully project-local (a
bigger change than this review round, and one that would reintroduce
the staleness-on-enable/disable gap the live-resolution design was
built to avoid), this documents the limitation in
docs/install/one-time.md and gives the dispatcher a clear,
actionable error message pointing at persistent installation instead
of a bare "'specify' is required" message.
Assisted-by: Claude Code (model: Claude Sonnet 5, autonomous)
Signed-off-by: Ashfaq <105435085+Ashfaqbs@users.noreply.github.com>
Posted on behalf of @Ashfaqbs by Claude Code (model: Claude Sonnet 5, autonomous); comment fully AI-drafted.
Pushed 47b3873 addressing both Copilot findings from the latest round:
integration upgrade --force and integration switch/use --force both refresh shared infra the same way specify init --force does, which can clobber a generated continuation dispatcher for an already-enabled script preset. Both now call reconcile_all_script_chains() afterward, mirroring the existing call in command_init.py. Added regression tests for each (test_upgrade_force_restores_script_preset_dispatcher, test_switch_same_force_restores_script_preset_dispatcher).
The dispatcher's runtime CLI resolution genuinely cannot work under the documented one-time uvx flow, since that flow discards its environment right after init. Making resolution fully project-local would be a larger redesign than this round, and would reintroduce the staleness-on-enable/disable gap that commands already have and that live resolution was specifically built to avoid for scripts. Instead of a silent architecture change, I documented the limitation in docs/install/one-time.md and gave the dispatcher a clear, actionable error message pointing at persistent installation, instead of the bare "'specify' is required" message.
Local verification: ruff@0.15.0 check src tests clean; tests/specify_cli/integrations/test_command_upgrade.py, test_command_upgrade_layout.py, test_command_switch.py, and tests/specify_cli/presets/test_manager.py all pass (344 + 96 tests, only the pre-existing unrelated skips).
Preserve compatibility for unquoted CORE_SCRIPT placeholders
src/specify_cli/presets/_manager.py:204
The runtime changes $CORE_SCRIPT from the documented literal replacement placeholder (docs/reference/presets.md:200) into a filesystem path. Existing wrappers use unquoted $CORE_SCRIPT (including the repository's resolver tests), so projects whose path contains spaces split this value and fail with “No such file or directory”; quoting it was not previously required and would not have been compatible with content substitution. Preserve compatibility with unquoted placeholders (for example via a space-free exported command/function) or introduce and document a migration rather than silently changing the contract.
Include disabled presets when determining dispatcher ownership
src/specify_cli/presets/_manager.py:655
This decides whether to retain a dispatcher from only the active resolution stack, which excludes disabled presets. If the last active provider is removed (or shared infrastructure is refreshed) while another provider is disabled, reconciliation restores the core copy; a later preset enable does not reconcile, so the enabled script remains inert. Determine dispatcher ownership from all installed declarations, including disabled presets, while continuing to resolve execution from only enabled layers.
Preserve lower-layer failures without requiring set -e
tests/test_script_continuation_bash.py:68
This set -e makes the wrapper—not the continuation runner—propagate the lower layer's failure, so the nonzero-status test does not cover ordinary wrappers allowed by the existing contract. Without set -e, the helper's trailing echo "...-after" turns a core exit 7 into status 0. Add a regression case without set -e and either preserve the lower status in generated/runtime behavior or document and validate the new wrapper requirement.
…target
integration switch <target> --refresh-shared-infra forces a shared-infra
refresh before installing the target integration, overwriting
.specify/scripts/bash/<name>.sh with the bundled core and clobbering any
generated continuation dispatcher for an already-enabled script preset.
The same-target case (--force) was already covered; this closes the gap
for switching to a different integration, which _set_default_integration's
own reconciliation doesn't reach because that call runs with
refresh_templates_force=False on this path (the force already happened
earlier, driven by --refresh-shared-infra, not that helper's parameter).
Covered by a new regression test exercising switch to a different
integration, per the outstanding review comment asking for that case
specifically (the existing test only covered switching to the
already-default integration).
Assisted-by: Claude Code (model: Claude Sonnet 5, autonomous)
Signed-off-by: Ashfaq <105435085+Ashfaqbs@users.noreply.github.com>
Posted on behalf of @Ashfaqbs by Claude Code (model: Claude Sonnet 5, autonomous); comment and the underlying commit are fully AI-drafted.
Status on the outstanding Copilot review threads on this diff:
Already fixed in earlier commits on this branch, thread just not yet marked resolved:
command_resolve.py:15 (registration-order test) — test_preset_commands_registered_once_in_stable_order already includes script-chain in the asserted order; passes locally.
_manager.py (uvx-only environments can't run the dispatcher) — the dispatcher now falls back from specify to python3 -m specify_cli and, failing both, prints an actionable error pointing at docs/install/one-time.md, which now documents the limitation explicitly. Landed in 9d49447.
_manager.py / command_init.py (forced re-init doesn't reconcile script chains) — reconcile_all_script_chains() added and wired into command_init.py right after install_shared_infra. Landed in 9d49447.
_helpers.py (integration use/switch --force doesn't reconcile) and command_upgrade.py (reconciliation ran before the second forced refresh and got clobbered by it) — both call reconcile_all_script_chains() at the correct point now, mirroring command_init.py. Landed in 47b3873, with new regression tests in test_command_switch.py and test_command_upgrade.py.
integration switch <different-target> --refresh-shared-infra was still missing reconciliation: its forced shared-infra refresh happens in phase 1, before _set_default_integration in phase 2 runs — and that call uses refresh_templates_force=False on this path, so the reconciliation gate added in 47b3873 never fired here. This closes that gap and adds test_switch_to_different_target_refresh_shared_infra_restores_script_preset_dispatcher, covering the different-target case the earlier test didn't.
Deliberately out of scope for this first increment (per the PR description — scoped to Bash only):
_manager.py:651-area (standalone override-only scripts, no preset declaring the name, never trigger reconciliation) — this is a pre-existing gap that also affects commands, not a regression introduced here. Tracking it properly needs a broader reconciliation-trigger design, not a point fix.
PowerShell/Python dispatcher parity — this PR intentionally ships the Bash runtime only as a first checkpoint; PS/Py adapters are follow-up work once this design is validated.
Full suite green locally (8121 passed / 471 skipped) aside from one pre-existing, unrelated Windows-only PowerShell JSON-parsing failure in test_setup_tasks.py that reproduces identically with this branch's changes stashed out.
Reserved script names are accepted during installation
src/specify_cli/presets/_manager.py:629
This reserved-name check occurs after the preset has already been copied and registered, and install_from_directory() catches the exception from reconciliation and only emits a warning. Thus a preset providing common or continuation-runner is reported as successfully installed but its script is unusable; the new private-method test does not exercise the public install path. Reject these names during manifest/pre-install validation and add an install-level regression test.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
triage-can-waitVerdict: valid and in-scope but deprioritized; held behind the evidence gate
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mnriem
added
the
triage-can-wait
Continues #4551 — implementing the design @mnriem and I settled on there. This is the first increment, scoped to Bash, since the full evidence plan (all three runtimes) is a larger unit of work than I want to land in one shot without a checkpoint.
What this adds
PresetResolver.resolve_script_chain(name)— returns the ordered file chain for a script name (highest priority down through the terminating"replace"layer), reusingcollect_all_layers()rather than a second resolution protocol.specify preset script-chain <name>(hidden) — prints that chain, one path per line. This is what the runtime adapter shells out to.scripts/bash/continuation-runner.sh— advances the chain one hop: execs into the next layer, and if more remain, re-exports the reduced list viaSPECKIT_SCRIPT_CONTINUATIONso a further"wrap"layer's own$CORE_SCRIPTcall continues correctly. No identity, no position — just the list it was handed, matching what we agreed on.PresetManager._reconcile_script_chain()— writes the project's canonical.specify/scripts/bash/<name>.sh: a verbatim copy when there's nothing to compose, or a fixed dispatcher stub when there is.Why this needs less reconciliation than commands
The dispatcher stub carries no stack-specific data — only the script's own name — so once it's written it never needs rewriting again: it resolves the live chain fresh on every invocation, not at materialization time. That means
install/remove(which change which layers exist) call_reconcile_script_chain, butenable/disable/set-priority(which only reorder existing layers) don't need to touch it at all — priority and enablement changes take effect on the next run for free. Commands can't do this because an agent reads the command file directly; a script is executed, so it can carry its own resolution logic.Evidence
TestResolveScriptChain/TestScriptChainReconciliationintests/test_presets.py: ordering, the"replace"-terminates-the-chain rule, a dangling-wrap-with-no-base case, install writing the stub vs. a verbatim copy, remove reverting it, and — importantly — a priority change viaregistry.update()alone (no reinstall) reorderingresolve_script_chain()'s output.tests/test_script_continuation_bash.py: a realbashsubprocess test with two wrap layers over a core script, asserting the actual execution order (outer-before → inner-before → core → inner-after → outer-after), arg propagation, exit-status propagation, and — the core claim — that a priority swap reorders execution while the dispatcher file stays byte-for-byte unchanged. (These are markedrequires_bashlike the existing bash-dependent tests in this suite; I additionally hand-verified the full chain and the priority-swap case against the realbash.exeoutside pytest, both passing, sincebashwasn't resolvable via my sandbox's bare-bashPATH probe.)tests/test_presets.py, 605 pre-existing tests) still green — no changes to command/template resolution behavior.Found and fixed a real bug while building the bash test:
specify's stdout carries CRLF line endings on Windows, and bash's$(...)only strips trailing newlines, not carriage returns — an unstripped\rwas corrupting the exec path with "No such file or directory". Fixed withtr -d '\r'at the source, plus a defensive strip in the runner's own read loop.Left for follow-up, pending your input
ContinuationRunner.ps1, arun_script_continuation()inscripts/python/common.py), but I wanted this checkpoint reviewed first rather than tripling the diff.type: scriptentry'sfile:field only ever points at a.sh— there's no way for a preset to declare.ps1/.pycounterparts of the same script name. That's a schema decision (separate fields? a per-languageprovidesblock?) I'd rather you weigh in on before I build the other two adapters against a shape I invented unilaterally.Ran the full existing test suite plus the new tests locally; happy to adjust the chain representation or the reconcile-only-on-install/remove call sites if you see it differently.
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, the install/remove-only reconciliation argument) follows directly from what we worked out together on #4551.