mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02:00
* Fix: scaffold self-contained namespaced preset commands (#4076) Preset command templates named `speckit.<ns>.<cmd>` were silently dropped whenever `.specify/extensions/<ns>/` was absent, while `speckit.<cmd>` 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 * Skip uncomposable commands in skills mode too (PR #4082 review) 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 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cfc4f1ce-6acb-465a-aa7b-999f2e4197fb
This commit is contained in:
Manfred Riem
co-authored by
Copilot App
Copilot App
parent
750ce47cb8
commit
e79fa25f3f
+135
-39
@@ -4260,8 +4260,14 @@ class TestSelfTestPreset:
|
||||
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.<ns>.<cmd>`` 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.<cmd>`` command. It is not
|
||||
dropped merely because ``.specify/extensions/fakeext/`` is absent.
|
||||
"""
|
||||
claude_dir = project_dir / ".claude" / "skills"
|
||||
claude_dir.mkdir(parents=True)
|
||||
|
||||
@@ -4297,11 +4303,13 @@ class TestSelfTestPreset:
|
||||
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."""
|
||||
@@ -6528,17 +6536,16 @@ class TestPresetSkills:
|
||||
"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.<ext>.<cmd>``) when the extension isn't installed.
|
||||
"""A self-contained ``speckit.<ns>.<cmd>`` preset command scaffolds and
|
||||
survives rescaffold, even when no matching extension is 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).
|
||||
The preset ships the command body itself, so it is materialized just
|
||||
like a short ``speckit.<cmd>`` command — both at install and through a
|
||||
later reconciliation/rescaffold pass. It is not dropped by the
|
||||
``speckit.<ns>.<cmd>`` name shape (#4076).
|
||||
"""
|
||||
self._write_init_options(project_dir, ai="copilot", ai_skills=False)
|
||||
commands_dir = project_dir / ".github" / "agents"
|
||||
@@ -6552,24 +6559,24 @@ class TestPresetSkills:
|
||||
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.<ns>.<cmd>`` 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)
|
||||
@@ -6586,28 +6593,80 @@ class TestPresetSkills:
|
||||
|
||||
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_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
|
||||
):
|
||||
@@ -10017,6 +10076,43 @@ class TestPresetSkills:
|
||||
"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.<cmd>`` and ``speckit.<ns>.<cmd>`` commands must
|
||||
scaffold identically in command mode, with no installed extension.
|
||||
|
||||
Regression: the 3-part (``speckit.<ns>.<cmd>``) form was silently
|
||||
dropped by a name-shape guard whenever ``.specify/extensions/<ns>/``
|
||||
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."""
|
||||
|
||||
Reference in New Issue
Block a user