diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 750db28652..87a3bda0d1 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -195,7 +195,7 @@ catalogs: Presets can provide command files, template files (like `plan-template.md`), and script files. Each file name is evaluated independently against the priority stack, so different files can come from different layers. -Templates and scripts are looked up from the stack when Spec Kit needs them. Commands use the same stack for replacement and composition, but are materialized into the active integration's directory only, instead of being re-resolved by agents or written to every detected agent directory (#2948). During preset install, Spec Kit registers command files for the preset being installed against the currently active integration; post-install and post-removal reconciliation then recomputes and writes the effective command content for affected command names based on the active stack. Install and rescaffold remain active-only, but removal may also update previously targeted inactive directories recorded by the removed preset to restore the surviving command or skill layer. A non-active installed integration does not otherwise receive these command files until it becomes the default — `specify integration use ` (or `switch `) rescaffolds enabled presets for the newly active integration. Agents do not re-resolve the stack each time they run a command. +Templates are looked up from the stack when Spec Kit needs them. Commands and scripts use the same stack for replacement and composition, but are materialized to disk at preset-lifecycle time (install, remove, enable, disable, priority change) rather than re-resolved by agents or at script-invocation time. Commands are materialized into the active integration's directory only, instead of being written to every detected agent directory (#2948): during preset install, Spec Kit registers command files for the preset being installed against the currently active integration; post-install and post-removal reconciliation then recomputes and writes the effective command content for affected command names based on the active stack. Install and rescaffold remain active-only, but removal may also update previously targeted inactive directories recorded by the removed preset to restore the surviving command or skill layer. A non-active installed integration does not otherwise receive these command files until it becomes the default — `specify integration use ` (or `switch `) rescaffolds enabled presets for the newly active integration. Scripts are materialized into `.specify/scripts/bash/` as a chain of fixed-path generated launcher files, one per composing layer, each pointing `$CORE_SCRIPT` at the next-lower layer's fixed path (#4551). Agents do not re-resolve the stack each time they run a command or script. By default, files use a **replace** strategy: the first match in the priority stack wins and is used entirely. Templates and commands can also use composition strategies: **prepend** places preset content before lower-priority content, **append** places it after lower-priority content, and **wrap** replaces `{CORE_TEMPLATE}` with lower-priority content. Scripts support **replace** and **wrap**; script wrappers use `$CORE_SCRIPT` as the placeholder. @@ -271,7 +271,7 @@ Run `specify preset resolve ` to trace the resolution stack and see which ### What's the difference between disabling and removing a preset? -**Disabling** (`specify preset disable`) keeps the preset installed but excludes it from future template and script resolution. Previously registered commands remain available in your AI coding agent until preset removal, so use removal when you need command changes to stop taking effect. Disabling is useful for temporarily testing template/script behavior without a preset, or comparing template/script output with and without it. Re-enable anytime with `specify preset enable`. +**Disabling** (`specify preset disable`) keeps the preset installed but excludes it from future template resolution, and immediately re-materializes its scripts' launcher chains without it. Previously registered commands remain available in your AI coding agent until preset removal, so use removal when you need command changes to stop taking effect. Disabling is useful for temporarily testing template/script behavior without a preset, or comparing template/script output with and without it. Re-enable anytime with `specify preset enable`. **Removing** (`specify preset remove`) fully uninstalls the preset — deletes its files, unregisters its commands from your AI coding agent, and removes it from the registry. diff --git a/presets/ARCHITECTURE.md b/presets/ARCHITECTURE.md index db540f7c72..09ed2c2ad8 100644 --- a/presets/ARCHITECTURE.md +++ b/presets/ARCHITECTURE.md @@ -74,6 +74,25 @@ those operations may reconcile the live file, but only if its provenance hash pr generated content. Missing files may be seeded when the preset is installed; authored or edited constitutions are never overwritten. +### Script chain lifecycle + +Unlike templates, scripts are executed rather than read, so composition must be fully resolved +before invocation (#4551). `PresetManager._reconcile_script_chain()` materializes the chain to disk: +each composing layer gets its own fixed-path generated launcher under `.specify/scripts/bash/`, the +topmost landing at the canonical `.sh` agents actually invoke, each pointing `$CORE_SCRIPT` at +the next-lower layer's fixed path and ending in a materialized copy of the base layer at +`.speckit-core.sh`. A single `replace`-strategy layer with no composition needs no intermediate +files at all. + +Preset installation, removal, enablement, disablement, and priority changes all call this +materializer for every script name the affected preset declares, so a chain never goes stale — unlike +`constitution-template`, there is no opt-in gate here. The one case that does not self-materialize: a +project-local override added directly to `.specify/templates/overrides/scripts/` outside any +lifecycle command is not picked up automatically and needs an explicit +`PresetManager.reconcile_all_script_chains()` call, the same call a forced shared-infrastructure +refresh (`specify init --force`, a forced integration switch/upgrade) already makes to restore chains +it just overwrote. + ## Command Registration When a preset is installed with `type: "command"` entries, the `PresetManager` registers them into all detected agent directories using the shared `CommandRegistrar` from `src/specify_cli/agents.py`. diff --git a/src/specify_cli/command_init.py b/src/specify_cli/command_init.py index 459551b36c..3c5859ca4c 100644 --- a/src/specify_cli/command_init.py +++ b/src/specify_cli/command_init.py @@ -821,6 +821,26 @@ def init( ensure_executable_scripts(project_path, tracker=tracker) + # install_shared_infra above may have just overwritten + # .specify/scripts/bash/.sh with the bundled core, + # clobbering any generated launcher chain for a script an + # already-enabled preset provides (e.g. on + # `specify init --force` against an existing project). + # Restore those chains before any *new* --preset install + # below runs its own reconciliation. + try: + from .presets import PresetManager as _ExistingPresetManager + + _ExistingPresetManager(project_path).reconcile_all_script_chains() + except Exception as exc: + _print_cli_warning( + "reconcile script presets after", + "init", + str(project_path), + exc, + continuing="Inspect .specify/scripts/bash/.sh to diagnose.", + ) + if preset: try: from .presets import PresetCatalog, PresetError, PresetManager diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index 38e87294ec..f547a94a55 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -345,6 +345,28 @@ def _set_default_integration( f"Failed to refresh shared infrastructure for '{key}': {exc}" ) from exc + # _install_shared_infra above may have just overwritten + # .specify/scripts/bash/.sh with the bundled core when + # refresh_templates_force is True, clobbering any generated + # launcher chain for a script an already-enabled preset provides. + # Restore those chains now, mirroring the same call in + # command_init.py and command_upgrade.py. + if refresh_templates_force: + try: + from ..presets import PresetManager as _ExistingPresetManager + + _ExistingPresetManager(project_root).reconcile_all_script_chains() + except Exception as exc: + from .. import _print_cli_warning + + _print_cli_warning( + "reconcile script presets after", + "integration use/switch", + str(project_root), + exc, + continuing="Inspect .specify/scripts/bash/.sh to diagnose.", + ) + _write_integration_json(project_root, key, installed_keys, settings) _update_init_options_for_integration( project_root, integration, script_type=resolved_script, parsed_options=parsed_options diff --git a/src/specify_cli/integrations/command_switch.py b/src/specify_cli/integrations/command_switch.py index d2e85602f5..ce32cb63f8 100644 --- a/src/specify_cli/integrations/command_switch.py +++ b/src/specify_cli/integrations/command_switch.py @@ -253,6 +253,28 @@ def integration_switch( from .. import ensure_executable_scripts ensure_executable_scripts(project_root) + # The forced refresh above may have just overwritten + # .specify/scripts/bash/.sh with the bundled core, clobbering any + # generated launcher chain for a script an already-enabled preset + # provides. Restore those chains now: this call happens before + # _set_default_integration below, whose own reconciliation + # (_helpers.py) is gated on refresh_templates_force and would not fire + # here since this phase's force comes from --refresh-shared-infra, not + # from that helper's own parameter. + if refresh_shared_infra: + try: + from ..presets import PresetManager as _ExistingPresetManager + + _ExistingPresetManager(project_root).reconcile_all_script_chains() + except Exception as exc: + _print_cli_warning( + "reconcile script presets after", + "integration switch --refresh-shared-infra", + str(project_root), + exc, + continuing="Inspect .specify/scripts/bash/.sh to diagnose.", + ) + # Phase 2: Install target integration console.print(f"Installing integration: [cyan]{target}[/cyan]") manifest = IntegrationManifest( diff --git a/src/specify_cli/integrations/command_upgrade.py b/src/specify_cli/integrations/command_upgrade.py index b4682b6825..f56d72df81 100644 --- a/src/specify_cli/integrations/command_upgrade.py +++ b/src/specify_cli/integrations/command_upgrade.py @@ -216,6 +216,27 @@ def integration_upgrade( from .. import ensure_executable_scripts ensure_executable_scripts(project_root) + # _install_shared_infra_or_exit above may have just overwritten + # .specify/scripts/bash/.sh with the bundled core when force=True, + # clobbering any generated launcher chain for a script an + # already-enabled preset provides. Restore those chains now, + # mirroring the same call in command_init.py. + if force: + try: + from ..presets import PresetManager as _ExistingPresetManager + + _ExistingPresetManager(project_root).reconcile_all_script_chains() + except Exception as exc: + from .. import _print_cli_warning + + _print_cli_warning( + "reconcile script presets after", + "integration upgrade", + str(project_root), + exc, + continuing="Inspect .specify/scripts/bash/.sh to diagnose.", + ) + # Phase 1: Install new files (overwrites existing; old-only files remain) console.print(f"Upgrading integration: [cyan]{key}[/cyan]") new_manifest = IntegrationManifest(key, project_root, version=_get_speckit_version()) diff --git a/src/specify_cli/presets/_manager.py b/src/specify_cli/presets/_manager.py index 33274dfdfc..2395b33e0d 100644 --- a/src/specify_cli/presets/_manager.py +++ b/src/specify_cli/presets/_manager.py @@ -2,6 +2,7 @@ import hashlib import json +import os import shutil import tempfile from pathlib import Path @@ -150,6 +151,81 @@ def _materialize_constitution_template( return result +# Generated files for script composition (#4551). The chain is materialized +# to disk at preset-lifecycle time (install, remove, enable, disable, +# priority-change) rather than resolved at script-invocation time: every +# layer above the base gets its own fixed-path generated launcher, each +# pointing $CORE_SCRIPT at the next-lower layer's fixed path (or the +# materialized core copy at the bottom). Regenerating the whole chain on +# every lifecycle event keeps this self-healing, so no runtime dependency on +# `specify` being importable or on PATH is ever needed. +_SCRIPT_LAUNCHER_MARKER = "# speckit-generated: script launcher" +_SCRIPT_CORE_SUFFIX = ".speckit-core.sh" +_SCRIPT_LAYER_SUFFIX_FMT = ".speckit-layer-{depth}.sh" +# A single replace-strategy layer and a restored bundled-core copy are both +# written to the canonical path verbatim, with no marker in their content -- +# so this sidecar is what tells a later reconcile (when nothing provides the +# script any more) that the canonical file is generated and safe to delete, +# as opposed to a real repository-committed script with the same name. +_SCRIPT_PROVENANCE_SUFFIX = ".speckit-generated" +# ``common`` is reserved because it's the library every generated launcher +# would otherwise be free to collide with. +_RESERVED_SCRIPT_NAMES = frozenset({"common"}) + + +def _shell_single_quote(value: str) -> str: + """Single-quote ``value`` for safe embedding in a generated shell script. + + Single quotes suppress ``$``, backtick, and double-quote expansion + entirely (unlike double quotes, which still expand ``$``) -- the only + escape needed is for an embedded single quote itself. + """ + return "'" + value.replace("'", "'\\''") + "'" + + +def _script_launcher_stub( + script_name: str, next_hop_name: str, original_path: Path, scripts_dir: Path +) -> str: + """Generated fixed-path launcher for one layer of a materialized script chain. + + Sets ``$CORE_SCRIPT`` to the next-lower layer's fixed path and execs the + preset's own authored file unmodified -- the preset's content still + contains the literal ``$CORE_SCRIPT`` reference as a shell variable, it is + never textually substituted. Run via bash so the original file needs no + execute bit (preset copies keep source modes); the launcher itself is + chmod +x since a layer above it invokes $CORE_SCRIPT as a bare command. + + The next hop is always a sibling generated file, so it's resolved via + ``$SCRIPT_DIR`` + basename at run time rather than an embedded path -- + keeps the generated file portable across checkouts/platforms. The + preset's own authored file (``original_path``) is project-owned too -- + it always lives under ``.specify/presets/`` or + ``.specify/templates/overrides/`` -- so it is likewise referenced + relative to ``$SCRIPT_DIR`` rather than as an absolute path: an absolute + path would go stale the moment the checkout is moved or copied. The + relative fragment is still single-quoted defensively, since a project + path can itself contain ``$``, backticks, or spaces that double quotes + alone would not neutralize. + """ + relative_original = Path(os.path.relpath(original_path, scripts_dir)).as_posix() + quoted_original = _shell_single_quote(relative_original) + return ( + "#!/usr/bin/env bash\n" + f"{_SCRIPT_LAUNCHER_MARKER}\n" + f'# Generated by specify for the "{script_name}" script. Do not edit\n' + "# directly; customize via presets/overrides instead " + "(`specify preset add`).\n" + "set -e\n" + 'SCRIPT_DIR="$(CDPATH="" cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)"\n' + f'export CORE_SCRIPT="$SCRIPT_DIR/{next_hop_name}"\n' + # Adjacent "..."'...' quoting: bash concatenates the two literals with + # no space between them, so $SCRIPT_DIR still expands inside the + # double-quoted prefix while the project-owned suffix stays inert + # inside single quotes. + f'exec bash "$SCRIPT_DIR/"{quoted_original} "$@"\n' + ) + + class PresetManager(_PresetCommandMethods, _PresetSkillMethods): """Manages preset lifecycle: installation, removal, updates.""" @@ -481,6 +557,29 @@ def install_from_directory( stacklevel=2, ) + # Materialize the generated launcher chain for every script this + # preset provides (#4551). Unlike the runtime-dispatch design this + # replaces, the chain is fixed to disk here and must be regenerated + # again on every later lifecycle event (remove, enable, disable, + # set-priority) that could change which layers apply or their order. + 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"Inspect .specify/scripts/bash/{script_name}.sh and its " + f"generated layer files to diagnose.", + stacklevel=2, + ) + # TODO: constitution-sync is a named preset with core-owned side effects. # Give synchronization an explicit owner without changing its opt-in # behavior or overwriting authored constitutions. @@ -492,6 +591,234 @@ def install_from_directory( return manifest + def _cleanup_stale_script_layers( + self, scripts_dir: Path, script_name: str, keep: Set[Path] + ) -> None: + """Remove generated core/layer files from a since-shrunk chain. + + Only unlinks files this manager generated (detected via the launcher + marker, or the core file's fixed name) so a user's own same-named + file is never touched. + """ + core_path = scripts_dir / f"{script_name}{_SCRIPT_CORE_SUFFIX}" + candidates = [core_path] if core_path not in keep else [] + candidates.extend( + p + for p in scripts_dir.glob(f"{script_name}{_SCRIPT_LAYER_SUFFIX_FMT.format(depth='*')}") + if p not in keep + ) + for stale in candidates: + if not stale.is_file(): + continue + if stale == core_path: + stale.unlink() + continue + first_line = stale.read_text(encoding="utf-8").splitlines()[:2] + if any(_SCRIPT_LAUNCHER_MARKER in line for line in first_line): + stale.unlink() + + def _reconcile_script_chain(self, script_name: str) -> None: + """Materialize the project's generated launcher chain for ``script_name``. + + Scripts are executed rather than read, so composition must be fully + resolved *before* invocation (#4551): while any preset provides this + script, every layer above the base gets its own fixed-path generated + launcher, and the canonical ``.specify/scripts/bash/.sh`` is the + topmost one. Each launcher's ``$CORE_SCRIPT`` points at the + next-lower layer's fixed path, ending in a materialized copy of the + base/core content at ``.speckit-core.sh``. Because this is + baked in at materialization time rather than resolved live, install, + remove, enable, disable, and set-priority must all call this again + for it to take effect. When no preset provides the script any more, + the bundled core script is restored (or a stale generated file + 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 launcher 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" + provenance = scripts_dir / f"{script_name}{_SCRIPT_PROVENANCE_SUFFIX}" + + # 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) + _ensure_safe_shared_destination(self.project_root, provenance) + + # 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 launcher chain 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 and len(chain) > 1: + core_path = scripts_dir / f"{script_name}{_SCRIPT_CORE_SUFFIX}" + _ensure_safe_shared_destination(self.project_root, core_path) + _write_shared_text( + self.project_root, core_path, chain[-1].read_text(encoding="utf-8") + ) + generated = [core_path] + + # Wrap layers run from chain[-2] (closest to the base) up to + # chain[0] (the topmost, which lands at the canonical path). + # Fixed depth numbering counts up from the base so regenerating + # a chain of the same shape always reuses the same filenames. + next_path = core_path + last_index = len(chain) - 2 + for depth, index in enumerate(range(last_index, -1, -1), start=1): + layer = chain[index] + target = canonical if index == 0 else ( + scripts_dir / f"{script_name}{_SCRIPT_LAYER_SUFFIX_FMT.format(depth=depth)}" + ) + if target != canonical: + _ensure_safe_shared_destination(self.project_root, target) + _write_shared_text( + self.project_root, + target, + _script_launcher_stub(script_name, next_path.name, layer, scripts_dir), + ) + generated.append(target) + next_path = target + + if os.name != "nt": + for path in generated: + path.chmod(path.stat().st_mode | 0o111) + _write_shared_text(self.project_root, provenance, "") + self._cleanup_stale_script_layers( + scripts_dir, script_name, keep=set(generated) + ) + return + + if provided_by_preset: + # A single replace-strategy layer: no $CORE_SCRIPT to satisfy, + # write it verbatim with no intermediate files at all. + _write_shared_text( + self.project_root, canonical, chain[0].read_text(encoding="utf-8") + ) + if os.name != "nt": + canonical.chmod(canonical.stat().st_mode | 0o111) + _write_shared_text(self.project_root, provenance, "") + self._cleanup_stale_script_layers(scripts_dir, script_name, keep=set()) + return + + # No preset layer left: restore core, or drop a stale generated file. + if chain: + _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) + _write_shared_text(self.project_root, provenance, "") + elif provenance.is_file(): + # Only ever unlink a canonical file this manager generated -- + # detected via the sidecar, since a verbatim-written single + # layer or core restore carries no in-content marker. + if canonical.is_file(): + canonical.unlink() + provenance.unlink() + self._cleanup_stale_script_layers(scripts_dir, script_name, keep=set()) + + def reconcile_scripts_for_preset( + self, preset_id: str, failure_context: str + ) -> None: + """Re-materialize every script this preset declares without failing a change. + + Call this after enable/disable/set-priority update the registry, the + same way ``reconcile_constitution`` is called alongside those events. + Re-resolving by name recomputes the whole chain for that name against + the current stack, so this is correct even for a priority change that + also affects another preset sharing the same script name -- only the + just-changed preset's own declared names need to be iterated here. + """ + manifest = self.get_pack(preset_id) + if manifest is None: + return + 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"{failure_context} (script '{script_name}'): {exc}.", + stacklevel=2, + ) + + def reconcile_all_script_chains(self) -> None: + """Re-materialize every active preset- or override-provided script. + + ``install_shared_infra`` (re)writes ``.specify/scripts/bash/.sh`` + from the bundled core on ``specify init --force`` and forced + integration upgrades, overwriting any generated launcher chain in + place. Unlike commands/skills, script presets are not re-registered + as part of that refresh, so a previously-installed chain would + otherwise go inert until its preset was reinstalled. Call this once + after any shared-infrastructure refresh to restore the chain for + every script name currently provided by an enabled preset. + + A standalone project-local override (``.specify/templates/overrides/ + scripts/.sh``) with no preset declaring that name is the other + way a script can be "provided" (see #4551's override-only + reproduction): this is the only lifecycle point that iterates every + known script name, so it also scans the overrides directory and + reconciles those names, not just ones a preset manifest declares. + + This remains the one case where an on-disk change does not + self-materialize: a project-local override added directly to the + filesystem outside any lifecycle command still needs this called + explicitly. + """ + script_names: Set[str] = set() + for pack_id, _metadata in self.registry.list_by_priority(): + manifest = PresetResolver(self.project_root)._get_manifest( + self.presets_dir / pack_id + ) + if manifest is None: + continue + script_names.update( + t["name"] + for t in manifest.templates + if t.get("type") == "script" and isinstance(t.get("name"), str) + ) + overrides_scripts_dir = ( + self.project_root / ".specify" / "templates" / "overrides" / "scripts" + ) + if overrides_scripts_dir.is_dir(): + script_names.update( + override_file.stem + for override_file in overrides_scripts_dir.glob("*.sh") + ) + for script_name in script_names: + try: + self._reconcile_script_chain(script_name) + except Exception as exc: + import warnings + warnings.warn( + f"Post-refresh script reconciliation failed for " + f"script '{script_name}': {exc}. " + f"Inspect .specify/scripts/bash/{script_name}.sh and its " + f"generated layer files to diagnose.", + stacklevel=2, + ) + def _seed_constitution_from_preset( self, manifest: PresetManifest, preset_dir: Path ) -> None: @@ -702,6 +1029,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 ( @@ -740,6 +1068,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. @@ -881,6 +1213,20 @@ 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"Inspect .specify/scripts/bash/{script_name}.sh and its " + f"generated layer files to diagnose.", + stacklevel=2, + ) + if removed_constitution: try: self._reconcile_constitution() diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index ea1c4e03f9..51b32622fe 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -682,6 +682,80 @@ 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 stays a shell + variable rather than being textually substituted; ``PresetManager`` + consumes this chain to generate the fixed-path launcher files that + materialize that continuation on disk at lifecycle time (install, + remove, enable, disable, set-priority), 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 = 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( + (i for i, layer in enumerate(layers) if layer["strategy"] == "replace"), + None, + ) + if base_idx is None: + return [] + 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, template_name: str, diff --git a/src/specify_cli/presets/command_disable.py b/src/specify_cli/presets/command_disable.py index 85c0231abd..a217eeb73b 100644 --- a/src/specify_cli/presets/command_disable.py +++ b/src/specify_cli/presets/command_disable.py @@ -41,6 +41,10 @@ def preset_disable( manager.reconcile_constitution( f"Failed to reconcile constitution after disabling preset {preset_id}" ) + manager.reconcile_scripts_for_preset( + preset_id, + f"Failed to reconcile scripts after disabling preset {preset_id}", + ) console.print(f"[green]✓[/green] Preset '{preset_id}' disabled") console.print("\nTemplates from this preset will be skipped during resolution.") diff --git a/src/specify_cli/presets/command_enable.py b/src/specify_cli/presets/command_enable.py index c3cd3ba5ff..90427cd21c 100644 --- a/src/specify_cli/presets/command_enable.py +++ b/src/specify_cli/presets/command_enable.py @@ -41,6 +41,10 @@ def preset_enable( manager.reconcile_constitution( f"Failed to reconcile constitution after enabling preset {preset_id}" ) + manager.reconcile_scripts_for_preset( + preset_id, + f"Failed to reconcile scripts after enabling preset {preset_id}", + ) console.print(f"[green]✓[/green] Preset '{preset_id}' enabled") console.print("\nTemplates from this preset will now be included in resolution.") diff --git a/src/specify_cli/presets/command_set_priority.py b/src/specify_cli/presets/command_set_priority.py index 8337c251fa..5fc7839d4b 100644 --- a/src/specify_cli/presets/command_set_priority.py +++ b/src/specify_cli/presets/command_set_priority.py @@ -61,6 +61,10 @@ def preset_set_priority( manager.reconcile_constitution( f"Failed to reconcile constitution after changing priority for preset {preset_id}" ) + manager.reconcile_scripts_for_preset( + preset_id, + f"Failed to reconcile scripts after changing priority for preset {preset_id}", + ) console.print( f"[green]✓[/green] Preset '{preset_id}' priority changed: {old_priority} → {priority}" diff --git a/tests/specify_cli/integrations/test_command_switch.py b/tests/specify_cli/integrations/test_command_switch.py index b1824102aa..27a47bbebe 100644 --- a/tests/specify_cli/integrations/test_command_switch.py +++ b/tests/specify_cli/integrations/test_command_switch.py @@ -96,6 +96,118 @@ def test_switch_same_force_refreshes_shared_templates(self, tmp_path): assert "/speckit-plan" in template.read_text(encoding="utf-8") assert "/speckit-plan" in script.read_text(encoding="utf-8") + def test_switch_same_force_restores_script_preset_launcher(self, tmp_path): + """``integration switch --force`` refreshes shared infra + (including ``.specify/scripts/bash/.sh``) from the bundled + core, clobbering a generated launcher chain for an already-enabled + script preset. The switch must reconcile script chains afterward + so the preset's script keeps working.""" + from tests.specify_cli.presets._helpers import create_pack + from specify_cli.presets import PresetManager + + project = _init_project(tmp_path, "claude") + + pack_dir = create_pack( + tmp_path, + { + "schema_version": "1.0", + "preset": { + "id": "switch-script-pack", + "name": "Switch Script Pack", + "version": "1.0.0", + "description": "A test preset with a script", + "author": "Test Author", + "repository": "https://github.com/test/switch-script-pack", + "license": "MIT", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": {"templates": []}, + "tags": ["testing"], + }, + "switch-script-pack", + "echo custom\n", + strategy="replace", + template_type="script", + template_name="switch-me", + ) + PresetManager(project).install_from_directory(pack_dir, "0.1.5") + + canonical = project / ".specify" / "scripts" / "bash" / "switch-me.sh" + provenance = ( + project / ".specify" / "scripts" / "bash" / "switch-me.speckit-generated" + ) + assert canonical.read_text(encoding="utf-8") == "echo custom\n" + assert provenance.is_file() + + old_cwd = os.getcwd() + try: + os.chdir(project) + result = runner.invoke(app, [ + "integration", "switch", "claude", + "--force", + ], catch_exceptions=False) + finally: + os.chdir(old_cwd) + + assert result.exit_code == 0, result.output + assert canonical.read_text(encoding="utf-8") == "echo custom\n" + + def test_switch_to_different_target_refresh_shared_infra_restores_script_preset_launcher( + self, tmp_path + ): + """``integration switch --refresh-shared-infra`` + forces its own shared-infra refresh in phase 1 (before phase 2 ever + calls ``_set_default_integration``), clobbering a generated + launcher chain the same way the same-target case does. The switch + must reconcile script chains for this path too, not just for + switching to the already-default integration.""" + from tests.specify_cli.presets._helpers import create_pack + from specify_cli.presets import PresetManager + + project = _init_project(tmp_path, "claude") + + pack_dir = create_pack( + tmp_path, + { + "schema_version": "1.0", + "preset": { + "id": "switch-target-script-pack", + "name": "Switch Target Script Pack", + "version": "1.0.0", + "description": "A test preset with a script", + "author": "Test Author", + "repository": "https://github.com/test/switch-target-script-pack", + "license": "MIT", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": {"templates": []}, + "tags": ["testing"], + }, + "switch-target-script-pack", + "echo custom\n", + strategy="replace", + template_type="script", + template_name="switch-target-me", + ) + PresetManager(project).install_from_directory(pack_dir, "0.1.5") + + canonical = project / ".specify" / "scripts" / "bash" / "switch-target-me.sh" + assert canonical.read_text(encoding="utf-8") == "echo custom\n" + + old_cwd = os.getcwd() + try: + os.chdir(project) + result = runner.invoke(app, [ + "integration", "switch", "copilot", + "--script", "sh", + "--refresh-shared-infra", + ], catch_exceptions=False) + finally: + os.chdir(old_cwd) + + assert result.exit_code == 0, result.output + assert canonical.read_text(encoding="utf-8") == "echo custom\n" + def test_switch_installed_target_rejects_integration_options(self, tmp_path): project = _init_project(tmp_path, "claude") old_cwd = os.getcwd() diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index 42d5ea0a6c..309d03fc18 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -1565,6 +1565,51 @@ def setup(self, project_root, manifest, **kwargs): assert "upgrade exploded with context" in normalized assert "previous integration files may still be in place" in normalized + def test_upgrade_force_restores_script_preset_launcher(self, tmp_path): + """``integration upgrade --force`` rewrites the canonical script from + the bundled core (via ``_install_shared_infra_or_exit(force=True)``), + clobbering a generated launcher chain for an already-enabled script + preset. The upgrade must reconcile script chains afterward so the + preset's script keeps working, mirroring ``specify init --force``.""" + from tests.specify_cli.presets._helpers import create_pack + + project = _init_project(tmp_path, "claude") + + from specify_cli.presets import PresetManager + + pack_dir = create_pack( + tmp_path, + { + "schema_version": "1.0", + "preset": { + "id": "upgrade-script-pack", + "name": "Upgrade Script Pack", + "version": "1.0.0", + "description": "A test preset with a script", + "author": "Test Author", + "repository": "https://github.com/test/upgrade-script-pack", + "license": "MIT", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": {"templates": []}, + "tags": ["testing"], + }, + "upgrade-script-pack", + "echo custom\n", + strategy="replace", + template_type="script", + template_name="upgrade-me", + ) + PresetManager(project).install_from_directory(pack_dir, "0.1.5") + + canonical = project / ".specify" / "scripts" / "bash" / "upgrade-me.sh" + assert canonical.read_text(encoding="utf-8") == "echo custom\n" + + result = _run_in_project(project, ["integration", "upgrade", "claude", "--force"]) + + assert result.exit_code == 0, result.output + assert canonical.read_text(encoding="utf-8") == "echo custom\n" + class TestIntegrationUpgradeBasic: """Test ``specify integration upgrade``.""" diff --git a/tests/specify_cli/presets/_helpers.py b/tests/specify_cli/presets/_helpers.py index 32cb132adb..365e31826a 100644 --- a/tests/specify_cli/presets/_helpers.py +++ b/tests/specify_cli/presets/_helpers.py @@ -252,3 +252,45 @@ def _create_multi_command_preset_with_aliases(self, temp_dir, preset_id, command with open(preset_dir / "preset.yml", "w") as f: yaml.dump(manifest_data, f) return preset_dir + + +def create_pack(temp_dir, valid_pack_data, pack_id, content, + strategy="replace", template_type="template", + template_name="spec-template"): + """Helper to create a preset pack directory.""" + pack_data = {**valid_pack_data} + pack_data["preset"] = {**valid_pack_data["preset"], "id": pack_id, "name": pack_id} + + tmpl_entry = { + "type": template_type, + "name": template_name, + } + if template_type == "script": + tmpl_entry["file"] = f"scripts/{template_name}.sh" + elif template_type == "command": + tmpl_entry["file"] = f"commands/{template_name}.md" + else: + tmpl_entry["file"] = f"templates/{template_name}.md" + if strategy != "replace": + tmpl_entry["strategy"] = strategy + pack_data["provides"] = {"templates": [tmpl_entry]} + + pack_dir = temp_dir / pack_id + pack_dir.mkdir(exist_ok=True) + with open(pack_dir / "preset.yml", 'w') as f: + yaml.dump(pack_data, f) + + if template_type == "script": + subdir = pack_dir / "scripts" + subdir.mkdir(exist_ok=True) + (subdir / f"{template_name}.sh").write_text(content) + elif template_type == "command": + subdir = pack_dir / "commands" + subdir.mkdir(exist_ok=True) + (subdir / f"{template_name}.md").write_text(content) + else: + subdir = pack_dir / "templates" + subdir.mkdir(exist_ok=True) + (subdir / f"{template_name}.md").write_text(content) + + return pack_dir diff --git a/tests/specify_cli/presets/test_command_enable.py b/tests/specify_cli/presets/test_command_enable.py index 4eb4ebfe9d..465a7581bd 100644 --- a/tests/specify_cli/presets/test_command_enable.py +++ b/tests/specify_cli/presets/test_command_enable.py @@ -6,6 +6,7 @@ PresetManager, ) from tests.specify_cli.presets._helpers import ( + create_pack as _create_pack, install_constitution_sync_preset, install_self_test_preset, make_convention_constitution_preset as _make_convention_constitution_preset, @@ -79,6 +80,56 @@ def test_enable_disable_reconciles_generated_constitution( assert enabled.exit_code == 0, enabled.output assert memory.read_text() == "# Convention Constitution\n" + def test_enable_disable_reconciles_generated_script( + self, project_dir, temp_dir, valid_pack_data + ): + """Enable and disable rematerialize the canonical generated script + chain through the real CLI command handlers, not just a direct + manager/registry call (#4709 review: the manager-level coverage for + this invokes reconcile_scripts_for_preset() itself, which cannot + catch missing wiring in command_enable.py/command_disable.py).""" + from unittest.mock import patch + + from typer.testing import CliRunner + + from specify_cli import app + + core_script = ( + project_dir / ".specify" / "templates" / "scripts" / "toggle-me.sh" + ) + core_script.parent.mkdir(parents=True, exist_ok=True) + core_script.write_text("echo core\n") + + manager = PresetManager(project_dir) + low_pack = _create_pack( + temp_dir, valid_pack_data, "low-pack", "echo low-before\n$CORE_SCRIPT\n", + strategy="wrap", template_type="script", template_name="toggle-me", + ) + manager.install_from_directory(low_pack, "0.1.5", priority=20) + high_pack = _create_pack( + temp_dir, valid_pack_data, "high-pack", "echo high-before\n$CORE_SCRIPT\n", + strategy="wrap", template_type="script", template_name="toggle-me", + ) + manager.install_from_directory(high_pack, "0.1.5", priority=5) + + canonical = ( + project_dir / ".specify" / "scripts" / "bash" / "toggle-me.sh" + ) + assert "high-pack" in canonical.read_text() + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + disabled = runner.invoke(app, ["preset", "disable", "high-pack"]) + assert disabled.exit_code == 0, disabled.output + content_after_disable = canonical.read_text() + assert "high-pack" not in content_after_disable + assert "low-pack" in content_after_disable + + with patch.object(Path, "cwd", return_value=project_dir): + enabled = runner.invoke(app, ["preset", "enable", "high-pack"]) + assert enabled.exit_code == 0, enabled.output + assert "high-pack" in canonical.read_text() + def test_stack_changes_do_not_create_missing_constitution( self, project_dir, pack_dir ): diff --git a/tests/specify_cli/presets/test_command_set_priority.py b/tests/specify_cli/presets/test_command_set_priority.py index 14afba58a0..02f28f65bb 100644 --- a/tests/specify_cli/presets/test_command_set_priority.py +++ b/tests/specify_cli/presets/test_command_set_priority.py @@ -7,6 +7,7 @@ ) from tests.conftest import strip_ansi from tests.specify_cli.presets._helpers import ( + create_pack as _create_pack, install_constitution_sync_preset, install_self_test_preset, make_convention_constitution_preset as _make_convention_constitution_preset, @@ -72,6 +73,51 @@ def test_set_priority_reconciles_generated_constitution( assert result.exit_code == 0, result.output assert memory.read_text() == "# Convention Constitution\n" + def test_set_priority_reconciles_generated_script( + self, project_dir, temp_dir, valid_pack_data + ): + """A priority change reorders the materialized script chain through + the real CLI command handler, not just a direct manager/registry + call (#4709 review: the manager-level coverage for this invokes + reconcile_scripts_for_preset() itself, which cannot catch missing + wiring in command_set_priority.py).""" + from unittest.mock import patch + + from typer.testing import CliRunner + + from specify_cli import app + + core_script = ( + project_dir / ".specify" / "templates" / "scripts" / "toggle-me.sh" + ) + core_script.parent.mkdir(parents=True, exist_ok=True) + core_script.write_text("echo core\n") + + manager = PresetManager(project_dir) + low_pack = _create_pack( + temp_dir, valid_pack_data, "low-pack", "echo low-before\n$CORE_SCRIPT\n", + strategy="wrap", template_type="script", template_name="toggle-me", + ) + manager.install_from_directory(low_pack, "0.1.5", priority=20) + high_pack = _create_pack( + temp_dir, valid_pack_data, "high-pack", "echo high-before\n$CORE_SCRIPT\n", + strategy="wrap", template_type="script", template_name="toggle-me", + ) + manager.install_from_directory(high_pack, "0.1.5", priority=5) + + canonical = ( + project_dir / ".specify" / "scripts" / "bash" / "toggle-me.sh" + ) + assert "high-pack" in canonical.read_text() + + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["preset", "set-priority", "high-pack", "30"] + ) + + assert result.exit_code == 0, result.output + assert "low-pack" in canonical.read_text() + def test_set_priority_same_value_no_change(self, project_dir, pack_dir): """Test set-priority with same value shows already set message.""" from unittest.mock import patch diff --git a/tests/specify_cli/presets/test_manager.py b/tests/specify_cli/presets/test_manager.py index 91e0805593..a0b05741b0 100644 --- a/tests/specify_cli/presets/test_manager.py +++ b/tests/specify_cli/presets/test_manager.py @@ -1,6 +1,8 @@ """Tests for preset installation and removal in specify_cli.presets._manager.""" import json +import os +import sys import tarfile import zipfile from pathlib import Path @@ -25,6 +27,7 @@ from tests.specify_cli.presets._helpers import ( make_convention_constitution_preset as _make_convention_constitution_preset, ) +from tests.specify_cli.presets._helpers import create_pack as _create_pack class TestPresetManifest: @@ -1607,3 +1610,321 @@ def test_remove_restores_lower_priority_command( cmd_files = list(gemini_dir.glob("*specify*")) assert cmd_files, "Command file should still exist after removal" assert "Lo content" in cmd_files[0].read_text() + + +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 the topmost launcher of a materialized chain when there is. + """ + + def _canonical(self, project_dir, name): + return project_dir / ".specify" / "scripts" / "bash" / f"{name}.sh" + + def _core(self, project_dir, name): + return project_dir / ".specify" / "scripts" / "bash" / f"{name}.speckit-core.sh" + + def test_install_replace_script_writes_verbatim_with_no_intermediate_files( + self, project_dir, temp_dir, valid_pack_data + ): + """A single-layer replace has nothing to compose, so it's written + verbatim with no generated launcher/core files at all.""" + 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.read_text() == "echo overridden\n" + assert not self._core(project_dir, "plain-override").exists() + chain = PresetResolver(project_dir).resolve_script_chain("plain-override") + assert [p.read_text() for p in chain] == ["echo overridden\n"] + + 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() + 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 + ): + 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_launcher_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 "speckit-generated: script launcher" in content + assert "CORE_SCRIPT" in content + # Unlike the old runtime dispatcher, the materialized launcher DOES + # encode stack-specific data: it execs the preset's own installed + # file directly rather than re-resolving anything at invocation + # time, so the resolved path is baked in. + assert "stub-pack" in content + + core_path = self._core(project_dir, "stub-target") + assert core_path.read_text() == "echo core\n" + assert core_path.name in content + + @pytest.mark.parametrize("reserved", ["common"]) + def test_reserved_helper_names_are_refused(self, project_dir, reserved): + """A launcher 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_launcher_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 generated + marker 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") + # Single-layer chain: written verbatim with no in-content marker, so + # provenance tracking (not canonical's own content) is what lets a + # later reconcile tell this apart from a real repo-committed script. + assert canonical.read_text() == "echo ext\n" + provenance = ( + project_dir / ".specify" / "scripts" / "bash" / "shadowed.speckit-generated" + ) + assert provenance.is_file() + + 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") + content = canonical.read_text() + assert "speckit-generated: script launcher" in content + assert "CORE_SCRIPT" in content + core_path = self._core(project_dir, "revert-me") + assert core_path.read_text() == "echo core\n" + + manager.remove("revert-pack") + + assert canonical.read_text() == "echo core\n" + assert not core_path.exists() + + def test_reconcile_all_script_chains_restores_chain_after_shared_infra_refresh( + self, project_dir, temp_dir, valid_pack_data + ): + """``specify init --force`` rewrites the canonical script from the + bundled core, clobbering a generated launcher chain for an + already-enabled script preset. ``reconcile_all_script_chains()`` is + what a forced refresh calls afterward to restore it.""" + manager = PresetManager(project_dir) + pack_dir = _create_pack( + temp_dir, valid_pack_data, "reinit-pack", "echo wrapped\n", + strategy="replace", template_type="script", template_name="reinit-me", + ) + manager.install_from_directory(pack_dir, "0.1.5") + + canonical = self._canonical(project_dir, "reinit-me") + assert canonical.read_text() == "echo wrapped\n" + + # Simulate install_shared_infra --force overwriting the canonical + # script with the bundled core, as it would on `specify init --force`. + canonical.write_text("echo bundled-core\n") + + manager.reconcile_all_script_chains() + + assert canonical.read_text() == "echo wrapped\n" + + def test_reconcile_all_script_chains_covers_standalone_override( + self, project_dir + ): + """A project-local override with no preset declaring its script name + (the override-only reproduction from #4551) is still "provided": it + must be re-materialized once shared infrastructure is reconciled, + not just names that appear in an installed preset's manifest.""" + override_dir = ( + project_dir / ".specify" / "templates" / "overrides" / "scripts" + ) + override_dir.mkdir(parents=True, exist_ok=True) + (override_dir / "setup-plan.sh").write_text("echo overridden\n") + + canonical = self._canonical(project_dir, "setup-plan") + canonical.parent.mkdir(parents=True, exist_ok=True) + canonical.write_text("echo bundled-core\n") + + manager = PresetManager(project_dir) + manager.reconcile_all_script_chains() + + assert canonical.read_text() == "echo overridden\n" + chain = PresetResolver(project_dir).resolve_script_chain("setup-plan") + assert [p.read_text() for p in chain] == ["echo overridden\n"] + + def test_enable_disable_and_set_priority_rematerialize_scripts( + self, project_dir, temp_dir, valid_pack_data + ): + """Unlike commands (which stay stale until removal), scripts must + take effect immediately on enable/disable/set-priority (#4551) -- + mnriem's pivot explicitly closes this gap.""" + core_script = ( + project_dir / ".specify" / "templates" / "scripts" / "toggle-me.sh" + ) + core_script.parent.mkdir(parents=True, exist_ok=True) + core_script.write_text("echo core\n") + + manager = PresetManager(project_dir) + low_pack = _create_pack( + temp_dir, valid_pack_data, "low-pack", "echo low-before\n$CORE_SCRIPT\n", + strategy="wrap", template_type="script", template_name="toggle-me", + ) + manager.install_from_directory(low_pack, "0.1.5", priority=20) + high_pack = _create_pack( + temp_dir, valid_pack_data, "high-pack", "echo high-before\n$CORE_SCRIPT\n", + strategy="wrap", template_type="script", template_name="toggle-me", + ) + manager.install_from_directory(high_pack, "0.1.5", priority=5) + + canonical = self._canonical(project_dir, "toggle-me") + assert "high-pack" in canonical.read_text() + + # Disabling the higher-priority preset must immediately drop it from + # the materialized chain, not just the live resolver's view. + manager.registry.update("high-pack", {"enabled": False}) + manager.reconcile_scripts_for_preset("high-pack", "test disable") + content_after_disable = canonical.read_text() + assert "high-pack" not in content_after_disable + assert "low-pack" in content_after_disable + + # Re-enabling restores it. + manager.registry.update("high-pack", {"enabled": True}) + manager.reconcile_scripts_for_preset("high-pack", "test enable") + assert "high-pack" in canonical.read_text() + + # A priority change must reorder the chain immediately too. + manager.registry.update("high-pack", {"priority": 30}) + manager.reconcile_scripts_for_preset("high-pack", "test set-priority") + assert "low-pack" in canonical.read_text() + assert "high-pack" not in canonical.read_text() diff --git a/tests/specify_cli/presets/test_resolver.py b/tests/specify_cli/presets/test_resolver.py index d1e416f7a1..5d98e3fd08 100644 --- a/tests/specify_cli/presets/test_resolver.py +++ b/tests/specify_cli/presets/test_resolver.py @@ -17,6 +17,7 @@ CORE_TEMPLATE_NAMES, install_self_test_preset, ) +from tests.specify_cli.presets._helpers import create_pack as _create_pack class TestPresetResolver: @@ -1760,43 +1761,166 @@ def test_layers_read_strategy_from_manifest(self, project_dir, temp_dir, valid_p assert layers[1]["strategy"] == "replace" -def _create_pack(temp_dir, valid_pack_data, pack_id, content, - strategy="replace", template_type="template", - template_name="spec-template"): - """Helper to create a preset pack directory.""" - pack_data = {**valid_pack_data} - pack_data["preset"] = {**valid_pack_data["preset"], "id": pack_id, "name": pack_id} - - tmpl_entry = { - "type": template_type, - "name": template_name, - } - if template_type == "script": - tmpl_entry["file"] = f"scripts/{template_name}.sh" - elif template_type == "command": - tmpl_entry["file"] = f"commands/{template_name}.md" - else: - tmpl_entry["file"] = f"templates/{template_name}.md" - if strategy != "replace": - tmpl_entry["strategy"] = strategy - pack_data["provides"] = {"templates": [tmpl_entry]} - - pack_dir = temp_dir / pack_id - pack_dir.mkdir(exist_ok=True) - with open(pack_dir / "preset.yml", 'w') as f: - yaml.dump(pack_data, f) - - if template_type == "script": - subdir = pack_dir / "scripts" - subdir.mkdir(exist_ok=True) - (subdir / f"{template_name}.sh").write_text(content) - elif template_type == "command": - subdir = pack_dir / "commands" - subdir.mkdir(exist_ok=True) - (subdir / f"{template_name}.md").write_text(content) - else: - subdir = pack_dir / "templates" - subdir.mkdir(exist_ok=True) - (subdir / f"{template_name}.md").write_text(content) - - return pack_dir +class TestResolveScriptChain: + """Test PresetResolver.resolve_script_chain() (#4551). + + Unlike resolve_content(), which splices script content together ahead + of time, this returns the ordered *files* that PresetManager's + materializer (_reconcile_script_chain) walks to write a fixed-path + generated launcher per layer -- this resolver call itself is unchanged + by the materialization pivot; only when it's invoked (at reconcile + time, not script-invocation time) and what consumes its output did. + """ + + 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 + PresetManager.reconcile_scripts_for_preset() relies on when + set-priority re-materializes the chain afterward.""" + 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]) + + + 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" diff --git a/tests/test_script_continuation_bash.py b/tests/test_script_continuation_bash.py new file mode 100644 index 0000000000..fe0a92c595 --- /dev/null +++ b/tests/test_script_continuation_bash.py @@ -0,0 +1,187 @@ +"""End-to-end bash test for the materialized script launcher chain (#4551). + +Proves the materialized 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 shutil +import subprocess +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" + + +@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") + 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_chain_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 launcher chain" + + result = subprocess.run( + ["bash", str(canonical), "arg1", "arg2"], + cwd=project_dir, + capture_output=True, + text=True, + check=False, + ) + 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_requires_explicit_reconcile_to_take_effect( + project_dir: Path, tmp_path: Path +) -> None: + """Unlike the old runtime-resolution design, the chain is materialized + to disk: updating the registry alone does not change execution order + until something re-materializes it (mirroring how `specify preset + set-priority` now calls `reconcile_scripts_for_preset`, #4551).""" + 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}) + + # Updating the registry alone must not change the materialized chain. + assert canonical.read_bytes() == before_bytes + result = subprocess.run( + ["bash", str(canonical)], + cwd=project_dir, + capture_output=True, + text=True, + check=False, + ) + lines = [line for line in result.stdout.splitlines() if line.strip()] + assert lines == ["a-before ", "b-before ", "core", "b-after", "a-after"] + + # Re-materializing (what `set-priority` now does) must change both the + # on-disk chain and the next invocation's order. + manager._reconcile_script_chain("reorder") + assert canonical.read_bytes() != before_bytes + + result = subprocess.run( + ["bash", str(canonical)], + cwd=project_dir, + capture_output=True, + text=True, + check=False, + ) + 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, + ) + assert result.returncode == 7