Skip to content

Commit e79fa25

Browse files
mnriemCopilot
andauthored
Fix: scaffold self-contained namespaced preset commands (#4076) (#4082)
* 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
1 parent 750ce47 commit e79fa25

2 files changed

Lines changed: 203 additions & 109 deletions

File tree

src/specify_cli/presets/__init__.py

Lines changed: 69 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -823,25 +823,6 @@ def check_compatibility(
823823

824824
return True
825825

826-
def _extension_installed_for_command(self, command_name: str) -> bool:
827-
"""Whether *command_name* may be materialized in this project.
828-
829-
Extension command overrides follow ``speckit.<ext-id>.<cmd-name>``;
830-
they must be skipped everywhere preset artifacts are written —
831-
registration *and* reconciliation — when the extension isn't
832-
installed, or reconciliation would materialize files that
833-
registration refused to track. Core commands (single-dot names,
834-
e.g. ``speckit.specify``) always pass.
835-
"""
836-
parts = command_name.split(".")
837-
if len(parts) >= 3 and parts[0] == "speckit":
838-
ext_id = parts[1]
839-
if not (
840-
self.project_root / ".specify" / "extensions" / ext_id
841-
).is_dir():
842-
return False
843-
return True
844-
845826
def _register_commands(
846827
self,
847828
manifest: PresetManifest,
@@ -870,21 +851,20 @@ def _register_commands(
870851
if not command_templates:
871852
return {}
872853

873-
# Filter out extension command overrides if the extension isn't installed.
874-
filtered = [
875-
cmd
876-
for cmd in command_templates
877-
if self._extension_installed_for_command(cmd["name"])
878-
]
879-
880-
if not filtered:
881-
return {}
882-
854+
# A preset command template always ships its own body, so it is
855+
# self-contained and scaffolds regardless of whether any similarly
856+
# named extension is installed. Namespaced names (speckit.<ns>.<cmd>)
857+
# are treated exactly like short names (speckit.<cmd>) — they are NOT
858+
# filtered out just because ``.specify/extensions/<ns>/`` is absent.
859+
# The only command that cannot be materialized is a composition
860+
# (prepend/append/wrap) with no base layer to compose onto; that case
861+
# is handled per-command below (warn + skip), not by dropping names up
862+
# front.
883863
# Handle composition strategies: resolve composed content for non-replace commands
884864
resolver = PresetResolver(self.project_root)
885865
composed_dir = None
886866
commands_to_register = []
887-
for cmd in filtered:
867+
for cmd in command_templates:
888868
strategy = cmd.get("strategy", "replace")
889869
if strategy != "replace":
890870
# Only pre-compose if this preset is the top composing layer.
@@ -907,13 +887,23 @@ def _register_commands(
907887
"file": f".composed/{cmd['name']}.md",
908888
})
909889
else:
910-
raise PresetValidationError(
911-
f"Command '{cmd['name']}' uses '{strategy}' strategy "
912-
f"but no base command layer exists to compose onto. "
913-
f"Ensure a lower-priority preset, extension, or core "
914-
f"command provides this command before using "
915-
f"composition strategies."
890+
# No base layer to compose onto (e.g. the command it
891+
# would wrap comes from an extension that isn't
892+
# installed). Warn and skip this single command rather
893+
# than aborting the whole install — mirrors the
894+
# "composed is None" branch in
895+
# _reconcile_composed_commands so command-mode and
896+
# reconciliation behave identically.
897+
import warnings
898+
warnings.warn(
899+
f"Command '{cmd['name']}' uses '{strategy}' "
900+
f"strategy but no base command layer exists to "
901+
f"compose onto; skipping. Provide a lower-priority "
902+
f"preset, extension, or core command for it before "
903+
f"using composition strategies.",
904+
stacklevel=2,
916905
)
906+
continue
917907
else:
918908
# Not the top layer — register raw file; reconciliation
919909
# will overwrite with the correct composed/winning content.
@@ -1681,21 +1671,13 @@ def _reconcile_composed_commands(
16811671
if not command_names:
16821672
return set()
16831673

1684-
# Never materialize extension-scoped commands whose extension isn't
1685-
# installed. Registration (_register_commands / _register_skills)
1686-
# already refuses them, so a reconciliation pass writing them would
1687-
# create files no registry entry tracks. Filtering here — the single
1688-
# chokepoint every install/remove/rescaffold reconciliation funnels
1689-
# through — keeps all callers consistent without each one re-applying
1690-
# the filter when seeding names from manifest templates.
1691-
command_names = [
1692-
name
1693-
for name in command_names
1694-
if self._extension_installed_for_command(name)
1695-
]
1696-
if not command_names:
1697-
return set()
1698-
1674+
# Every preset-owned command name flows through unchanged. Names are
1675+
# NOT filtered by the ``speckit.<ns>.<cmd>`` shape: a self-contained
1676+
# preset command scaffolds whether or not a like-named extension is
1677+
# installed (parity with _register_commands), and a name whose base
1678+
# layer has disappeared must still reach the loop below so its now
1679+
# uncomposable stale file gets unregistered. The loop already skips
1680+
# names that resolve to no layers at all (``if not layers: continue``).
16991681
try:
17001682
from ..agents import CommandRegistrar
17011683
except ImportError:
@@ -2136,14 +2118,11 @@ def _reconcile_skills(
21362118
if not command_names:
21372119
return set()
21382120

2139-
command_names = [
2140-
name
2141-
for name in command_names
2142-
if self._extension_installed_for_command(name)
2143-
]
2144-
if not command_names:
2145-
return set()
2146-
2121+
# Preset-owned command names are not filtered by the
2122+
# ``speckit.<ns>.<cmd>`` shape here either: a self-contained preset
2123+
# command renders its skill whether or not a like-named extension is
2124+
# installed. The per-name loop below skips anything that doesn't
2125+
# resolve to a managed skill directory.
21472126
resolver = PresetResolver(self.project_root)
21482127
active_skills_dir = self._get_skills_dir()
21492128

@@ -2673,21 +2652,17 @@ def _register_skills(
26732652
if not command_templates:
26742653
return {}
26752654

2676-
# Filter out extension command overrides if the extension isn't installed,
2677-
# matching the same logic used by _register_commands().
2678-
filtered = [
2679-
cmd
2680-
for cmd in command_templates
2681-
if self._extension_installed_for_command(cmd["name"])
2682-
]
2683-
2684-
if not filtered:
2685-
return {}
2686-
2655+
# Preset command templates are self-contained and render as skills
2656+
# regardless of whether a like-named extension is installed — the same
2657+
# rule _register_commands() uses. No ``speckit.<ns>.<cmd>`` name-shape
2658+
# filtering; the per-command loop below skips anything without a target
2659+
# skill directory.
26872660
skills_dir = target_dir if target_dir is not None else self._get_skills_dir()
26882661
if not skills_dir:
26892662
return {}
26902663

2664+
resolver = PresetResolver(self.project_root)
2665+
26912666
from .. import SKILL_DESCRIPTIONS, load_init_options
26922667
from ..agents import CommandRegistrar
26932668
from ..integrations import get_integration
@@ -2717,7 +2692,7 @@ def _register_skills(
27172692

27182693
written: List[str] = []
27192694

2720-
for cmd_tmpl in filtered:
2695+
for cmd_tmpl in command_templates:
27212696
cmd_name = cmd_tmpl["name"]
27222697
cmd_file_rel = cmd_tmpl["file"]
27232698
source_file = preset_dir / cmd_file_rel
@@ -2757,6 +2732,29 @@ def _register_skills(
27572732
content = source_file.read_text(encoding="utf-8")
27582733
frontmatter, body = registrar.parse_frontmatter(content)
27592734

2735+
# A composition-strategy command (wrap/prepend/append) needs a
2736+
# base layer to compose onto. When _register_commands produced no
2737+
# composed file for it and the stack still has no base
2738+
# (resolve_content is None) — e.g. the command it wraps comes from
2739+
# an extension that isn't installed — rendering the raw preset
2740+
# fragment as a skill would emit broken output: a literal
2741+
# {CORE_TEMPLATE} for wrap, or only the preset's own fragment for
2742+
# prepend/append. Skip it here too so command mode and skills mode
2743+
# agree (mirrors _register_commands, which skips the same command).
2744+
# _register_commands already warned for this command in the same
2745+
# pass, so the skip is silent here to avoid a duplicate warning.
2746+
effective_strategy = (
2747+
cmd_tmpl.get("strategy")
2748+
or frontmatter.get("strategy")
2749+
or "replace"
2750+
)
2751+
if (
2752+
effective_strategy != "replace"
2753+
and not composed_file.exists()
2754+
and resolver.resolve_content(cmd_name, "command") is None
2755+
):
2756+
continue
2757+
27602758
if frontmatter.get("strategy") == "wrap":
27612759
body, core_frontmatter = _substitute_core_template(body, cmd_name, self.project_root, registrar)
27622760
frontmatter = dict(frontmatter)

0 commit comments

Comments
 (0)