From 00033e98c446581b58ab0e799200c47e89f45e4d Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:56:12 -0500 Subject: [PATCH 1/2] Fix: scaffold self-contained namespaced preset commands (#4076) Preset command templates named `speckit..` were silently dropped whenever `.specify/extensions//` was absent, while `speckit.` always scaffolded. The `_extension_installed_for_command` guard filtered purely on name shape, conflating "override of an installed extension's command" with "a preset shipping its own namespaced command." Because a `type: command` template always ships its own body, such a command is self-contained and must scaffold like any short-named command. Remove the name-shape guard at all four call sites (registration, both reconciliation passes, and skills). The reconciliation loop already skips names that resolve to no layers (`if not layers: continue`), and the composed-None branch still cleans up commands whose base layer disappeared. Convert the command-mode "no base layer to compose onto" hard error into a warn + skip, matching the existing behavior in _reconcile_composed_commands so command-mode install and reconciliation stay consistent. Update the two tests that encoded the old drop behavior to assert the new consistent-scaffold contract, and add coverage proving 2-part and 3-part preset commands scaffold identically with no extension installed. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cfc4f1ce-6acb-465a-aa7b-999f2e4197fb --- src/specify_cli/presets/__init__.py | 115 +++++++++++----------------- tests/test_presets.py | 113 +++++++++++++++++---------- 2 files changed, 117 insertions(+), 111 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 45c3456fe8..6b7737ec27 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -801,25 +801,6 @@ def check_compatibility( return True - def _extension_installed_for_command(self, command_name: str) -> bool: - """Whether *command_name* may be materialized in this project. - - Extension command overrides follow ``speckit..``; - they must be skipped everywhere preset artifacts are written — - registration *and* reconciliation — when the extension isn't - installed, or reconciliation would materialize files that - registration refused to track. Core commands (single-dot names, - e.g. ``speckit.specify``) always pass. - """ - parts = command_name.split(".") - if len(parts) >= 3 and parts[0] == "speckit": - ext_id = parts[1] - if not ( - self.project_root / ".specify" / "extensions" / ext_id - ).is_dir(): - return False - return True - def _register_commands( self, manifest: PresetManifest, @@ -848,21 +829,20 @@ def _register_commands( if not command_templates: return {} - # Filter out extension command overrides if the extension isn't installed. - filtered = [ - cmd - for cmd in command_templates - if self._extension_installed_for_command(cmd["name"]) - ] - - if not filtered: - return {} - + # A preset command template always ships its own body, so it is + # self-contained and scaffolds regardless of whether any similarly + # named extension is installed. Namespaced names (speckit..) + # are treated exactly like short names (speckit.) — they are NOT + # filtered out just because ``.specify/extensions//`` is absent. + # The only command that cannot be materialized is a composition + # (prepend/append/wrap) with no base layer to compose onto; that case + # is handled per-command below (warn + skip), not by dropping names up + # front. # Handle composition strategies: resolve composed content for non-replace commands resolver = PresetResolver(self.project_root) composed_dir = None commands_to_register = [] - for cmd in filtered: + for cmd in command_templates: strategy = cmd.get("strategy", "replace") if strategy != "replace": # Only pre-compose if this preset is the top composing layer. @@ -885,13 +865,23 @@ def _register_commands( "file": f".composed/{cmd['name']}.md", }) else: - raise PresetValidationError( - f"Command '{cmd['name']}' uses '{strategy}' strategy " - f"but no base command layer exists to compose onto. " - f"Ensure a lower-priority preset, extension, or core " - f"command provides this command before using " - f"composition strategies." + # No base layer to compose onto (e.g. the command it + # would wrap comes from an extension that isn't + # installed). Warn and skip this single command rather + # than aborting the whole install — mirrors the + # "composed is None" branch in + # _reconcile_composed_commands so command-mode and + # reconciliation behave identically. + import warnings + warnings.warn( + f"Command '{cmd['name']}' uses '{strategy}' " + f"strategy but no base command layer exists to " + f"compose onto; skipping. Provide a lower-priority " + f"preset, extension, or core command for it before " + f"using composition strategies.", + stacklevel=2, ) + continue else: # Not the top layer — register raw file; reconciliation # will overwrite with the correct composed/winning content. @@ -1659,21 +1649,13 @@ def _reconcile_composed_commands( if not command_names: return set() - # Never materialize extension-scoped commands whose extension isn't - # installed. Registration (_register_commands / _register_skills) - # already refuses them, so a reconciliation pass writing them would - # create files no registry entry tracks. Filtering here — the single - # chokepoint every install/remove/rescaffold reconciliation funnels - # through — keeps all callers consistent without each one re-applying - # the filter when seeding names from manifest templates. - command_names = [ - name - for name in command_names - if self._extension_installed_for_command(name) - ] - if not command_names: - return set() - + # Every preset-owned command name flows through unchanged. Names are + # NOT filtered by the ``speckit..`` shape: a self-contained + # preset command scaffolds whether or not a like-named extension is + # installed (parity with _register_commands), and a name whose base + # layer has disappeared must still reach the loop below so its now + # uncomposable stale file gets unregistered. The loop already skips + # names that resolve to no layers at all (``if not layers: continue``). try: from ..agents import CommandRegistrar except ImportError: @@ -2114,14 +2096,11 @@ def _reconcile_skills( if not command_names: return set() - command_names = [ - name - for name in command_names - if self._extension_installed_for_command(name) - ] - if not command_names: - return set() - + # Preset-owned command names are not filtered by the + # ``speckit..`` shape here either: a self-contained preset + # command renders its skill whether or not a like-named extension is + # installed. The per-name loop below skips anything that doesn't + # resolve to a managed skill directory. resolver = PresetResolver(self.project_root) active_skills_dir = self._get_skills_dir() @@ -2651,17 +2630,11 @@ def _register_skills( if not command_templates: return {} - # Filter out extension command overrides if the extension isn't installed, - # matching the same logic used by _register_commands(). - filtered = [ - cmd - for cmd in command_templates - if self._extension_installed_for_command(cmd["name"]) - ] - - if not filtered: - return {} - + # Preset command templates are self-contained and render as skills + # regardless of whether a like-named extension is installed — the same + # rule _register_commands() uses. No ``speckit..`` name-shape + # filtering; the per-command loop below skips anything without a target + # skill directory. skills_dir = target_dir if target_dir is not None else self._get_skills_dir() if not skills_dir: return {} @@ -2695,7 +2668,7 @@ def _register_skills( written: List[str] = [] - for cmd_tmpl in filtered: + for cmd_tmpl in command_templates: cmd_name = cmd_tmpl["name"] cmd_file_rel = cmd_tmpl["file"] source_file = preset_dir / cmd_file_rel diff --git a/tests/test_presets.py b/tests/test_presets.py index 6c6a1ed8f2..6fc2f6eb63 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -4118,8 +4118,14 @@ def test_constitution_materialization_error_is_nonfatal( assert manifest.id == "invalid-wrap" assert manager.registry.is_installed("invalid-wrap") - def test_extension_command_skipped_when_extension_missing(self, project_dir, temp_dir): - """Test that extension command overrides are skipped if the extension isn't installed.""" + def test_selfcontained_namespaced_command_scaffolds_without_extension(self, project_dir, temp_dir): + """A preset shipping a self-contained ``speckit..`` command + scaffolds even when no matching extension is installed. + + The command template ships its own body, so it is self-contained and + must render just like a short ``speckit.`` command. It is not + dropped merely because ``.specify/extensions/fakeext/`` is absent. + """ claude_dir = project_dir / ".claude" / "skills" claude_dir.mkdir(parents=True) @@ -4155,11 +4161,13 @@ def test_extension_command_skipped_when_extension_missing(self, project_dir, tem manager = PresetManager(project_dir) manager.install_from_directory(preset_dir, "0.1.5") - # Extension not installed — command should NOT be registered - cmd_file = claude_dir / "speckit.fakeext.cmd.md" - assert not cmd_file.exists(), "Command registered for missing extension" + # Extension not installed, but the preset ships its own command body — + # it must scaffold (as a native-skill SKILL.md for claude) and be + # tracked in the preset's registered_commands. + skill_file = claude_dir / "speckit-fakeext-cmd" / "SKILL.md" + assert skill_file.exists(), "Self-contained namespaced command was dropped" metadata = manager.registry.get("ext-override") - assert metadata["registered_commands"] == {} + assert metadata["registered_commands"] != {} def test_extension_command_registered_when_extension_present(self, project_dir, temp_dir): """Test that extension command overrides ARE registered when the extension is installed.""" @@ -6229,17 +6237,16 @@ def test_rescaffold_toggle_command_to_skills_removes_stale_command_file( "sanity: the new skills-mode artifact should still be written" ) - def test_rescaffold_skips_extension_commands_when_extension_not_installed( + def test_rescaffold_scaffolds_selfcontained_namespaced_commands( self, project_dir, temp_dir ): - """Rescaffold must not materialize extension-scoped commands - (``speckit..``) when the extension isn't installed. - - ``_register_commands`` refuses them, but the rescaffold seeded its - final reconciliation pass with every command template name - unfiltered, so ``_reconcile_composed_commands`` wrote the command - file anyway — an artifact no registry entry tracks (review - 3623357358). + """A self-contained ``speckit..`` preset command scaffolds and + survives rescaffold, even when no matching extension is installed. + + The preset ships the command body itself, so it is materialized just + like a short ``speckit.`` command — both at install and through a + later reconciliation/rescaffold pass. It is not dropped by the + ``speckit..`` name shape (#4076). """ self._write_init_options(project_dir, ai="copilot", ai_skills=False) commands_dir = project_dir / ".github" / "agents" @@ -6253,24 +6260,24 @@ def test_rescaffold_skips_extension_commands_when_extension_not_installed( manager.install_from_directory(preset_dir, "0.1.5") ext_cmd = commands_dir / "speckit.git.feature.agent.md" - assert not ext_cmd.exists(), ( - "sanity: install must not write an extension command when the " - "extension isn't installed" + assert ext_cmd.exists(), ( + "sanity: install must scaffold a self-contained namespaced command " + "even when its like-named extension isn't installed" ) manager.register_enabled_presets_for_agent("copilot") - assert not ext_cmd.exists(), ( - "rescaffold must not materialize an extension-scoped command " - "whose extension isn't installed" + assert ext_cmd.exists(), ( + "rescaffold must keep the self-contained namespaced command" ) metadata = manager.registry.get("ext-scoped-preset") - assert not (metadata.get("registered_commands") or {}).get("copilot") + assert (metadata.get("registered_commands") or {}).get("copilot") - def test_rescaffold_skips_extension_skills_when_extension_not_installed( + def test_rescaffold_scaffolds_selfcontained_namespaced_skills( self, project_dir, temp_dir ): - """Historical tracking must not recreate a missing extension's skill.""" + """A self-contained ``speckit..`` preset command renders its + skill even when no matching extension is installed.""" self._write_init_options(project_dir, ai="copilot", ai_skills=True) skills_dir = project_dir / ".github" / "skills" skills_dir.mkdir(parents=True) @@ -6287,26 +6294,15 @@ def test_rescaffold_skips_extension_skills_when_extension_not_installed( skill_name = "speckit-git-feature" skill_file = skills_dir / skill_name / "SKILL.md" - assert not skill_file.exists() - - manager.registry.update( - "ext-scoped-skill-preset", - {"registered_skills": {"copilot": [skill_name]}}, - ) - overrides_dir = ( - project_dir / ".specify" / "templates" / "overrides" - ) - overrides_dir.mkdir(parents=True) - (overrides_dir / "speckit.git.feature.md").write_text( - "---\ndescription: Project override\n---\n\nOverride body\n", - encoding="utf-8", + assert skill_file.exists(), ( + "install must render a self-contained namespaced command's skill " + "even when its like-named extension isn't installed" ) manager.register_enabled_presets_for_agent("copilot") - assert not skill_file.exists(), ( - "rescaffold must not materialize an extension-scoped skill " - "whose extension isn't installed" + assert skill_file.exists(), ( + "rescaffold must keep the self-contained namespaced command's skill" ) def test_same_mode_partial_command_rescaffold_keeps_skipped_tracking( @@ -9569,6 +9565,43 @@ def test_unregister_agent_artifacts_migrates_legacy_skill_list_scoped( "claude's real ownership must be preserved in the migrated tracking" ) + def test_short_and_namespaced_commands_scaffold_consistently( + self, project_dir, temp_dir + ): + """A preset's ``speckit.`` and ``speckit..`` commands must + scaffold identically in command mode, with no installed extension. + + Regression: the 3-part (``speckit..``) form was silently + dropped by a name-shape guard whenever ``.specify/extensions//`` + was absent, even though the preset ships the command body itself. The + 2-part form always scaffolded. Both are self-contained and must behave + the same (#4076). + """ + self._write_init_options(project_dir, ai="gemini", ai_skills=False) + gemini_commands_dir = project_dir / ".gemini" / "commands" + gemini_commands_dir.mkdir(parents=True) + + short_preset = self._create_command_preset( + temp_dir, "short-cmd", "speckit.newcmd", "Short", "short body", + ) + ns_preset = self._create_command_preset( + temp_dir, "ns-cmd", "speckit.fakeext.newcmd", "Namespaced", "ns body", + ) + + manager = PresetManager(project_dir) + manager.install_from_directory(short_preset, "0.1.5") + manager.install_from_directory(ns_preset, "0.1.5") + + short_file = gemini_commands_dir / "speckit.newcmd.toml" + ns_file = gemini_commands_dir / "speckit.fakeext.newcmd.toml" + assert short_file.exists(), "2-part command should scaffold" + assert ns_file.exists(), ( + "3-part namespaced command must scaffold too, even without the " + "matching extension installed" + ) + assert manager.registry.get("short-cmd")["registered_commands"] != {} + assert manager.registry.get("ns-cmd")["registered_commands"] != {} + class TestPresetSetPriority: """Test preset set-priority CLI command.""" From d5c29983b017a215cdfb429a88803216aa4cffb3 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:54:19 -0500 Subject: [PATCH 2/2] Skip uncomposable commands in skills mode too (PR #4082 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When _register_commands skips an uncomposable composition command (a wrap/prepend/append with no base layer to compose onto — e.g. the command it wraps comes from an uninstalled extension), install still passed the full manifest to _register_skills. For a command-backed integration in skills mode, _register_skills created the missing skill and fell back to the raw preset body because no `.composed` file existed, materializing a broken SKILL.md — a literal `{CORE_TEMPLATE}` for wrap, or just the preset's own fragment for prepend/append. Previously the raise in _register_commands aborted before skills ran, so this never surfaced. Make _register_skills apply the same skip: for a composition-strategy command with no `.composed` file, resolve the stack and skip when no base exists (resolve_content is None). The skip is silent because _register_commands already warned for the same command in the same pass. Add a regression test proving an uncomposable wrap command renders no skill and never leaks a literal {CORE_TEMPLATE} in skills mode. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cfc4f1ce-6acb-465a-aa7b-999f2e4197fb --- src/specify_cli/presets/__init__.py | 25 ++++++++++++ tests/test_presets.py | 63 +++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 6b7737ec27..ea3b8560a0 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2639,6 +2639,8 @@ def _register_skills( if not skills_dir: return {} + resolver = PresetResolver(self.project_root) + from .. import SKILL_DESCRIPTIONS, load_init_options from ..agents import CommandRegistrar from ..integrations import get_integration @@ -2708,6 +2710,29 @@ def _register_skills( content = source_file.read_text(encoding="utf-8") frontmatter, body = registrar.parse_frontmatter(content) + # A composition-strategy command (wrap/prepend/append) needs a + # base layer to compose onto. When _register_commands produced no + # composed file for it and the stack still has no base + # (resolve_content is None) — e.g. the command it wraps comes from + # an extension that isn't installed — rendering the raw preset + # fragment as a skill would emit broken output: a literal + # {CORE_TEMPLATE} for wrap, or only the preset's own fragment for + # prepend/append. Skip it here too so command mode and skills mode + # agree (mirrors _register_commands, which skips the same command). + # _register_commands already warned for this command in the same + # pass, so the skip is silent here to avoid a duplicate warning. + effective_strategy = ( + cmd_tmpl.get("strategy") + or frontmatter.get("strategy") + or "replace" + ) + if ( + effective_strategy != "replace" + and not composed_file.exists() + and resolver.resolve_content(cmd_name, "command") is None + ): + continue + if frontmatter.get("strategy") == "wrap": body, core_frontmatter = _substitute_core_template(body, cmd_name, self.project_root, registrar) frontmatter = dict(frontmatter) diff --git a/tests/test_presets.py b/tests/test_presets.py index 6fc2f6eb63..b15e72eaf1 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -6305,6 +6305,69 @@ def test_rescaffold_scaffolds_selfcontained_namespaced_skills( "rescaffold must keep the self-contained namespaced command's skill" ) + def test_uncomposable_wrap_command_skips_skill_in_skills_mode( + self, project_dir, temp_dir + ): + """A wrap command with no base layer must not materialize a broken + skill in skills mode. + + When ``_register_commands`` skips an uncomposable wrap command (no + base to compose onto — e.g. the command it wraps comes from an + uninstalled extension), ``_register_skills`` must skip it too. Before + this fix, skills mode fell back to the raw preset body and wrote a + SKILL.md containing a literal ``{CORE_TEMPLATE}`` placeholder. + """ + self._write_init_options(project_dir, ai="copilot", ai_skills=True) + skills_dir = project_dir / ".github" / "skills" + skills_dir.mkdir(parents=True) + + preset_dir = temp_dir / "uncomposable-wrap" + preset_dir.mkdir() + (preset_dir / "commands").mkdir() + # speckit.git.feature has no core command template and no installed + # extension, so there is no base layer to wrap. + (preset_dir / "commands" / "speckit.git.feature.md").write_text( + "---\ndescription: Wrap\nstrategy: wrap\n---\n\n" + "wrap start\n{CORE_TEMPLATE}\nwrap end\n" + ) + manifest_data = { + "schema_version": "1.0", + "preset": { + "id": "uncomposable-wrap", + "name": "uncomposable-wrap", + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "command", + "name": "speckit.git.feature", + "file": "commands/speckit.git.feature.md", + "strategy": "wrap", + } + ] + }, + } + with open(preset_dir / "preset.yml", "w") as f: + yaml.dump(manifest_data, f) + + manager = PresetManager(project_dir) + with pytest.warns(UserWarning, match="no base command layer"): + manager.install_from_directory(preset_dir, "0.1.5") + + skill_file = skills_dir / "speckit-git-feature" / "SKILL.md" + assert not skill_file.exists(), ( + "an uncomposable wrap command must not be rendered as a skill" + ) + # Belt-and-suspenders: no artifact anywhere may leak the raw placeholder. + leaked = [ + p for p in skills_dir.rglob("*") + if p.is_file() and "{CORE_TEMPLATE}" in p.read_text(encoding="utf-8") + ] + assert not leaked, f"literal {{CORE_TEMPLATE}} leaked into {leaked}" + def test_same_mode_partial_command_rescaffold_keeps_skipped_tracking( self, project_dir, temp_dir ):