fix: address second round of review feedback (priority order, fail-closed, docs)

- register_enabled_presets_for_agent now processes presets in reverse
  priority order (lowest-precedence first) so the highest-precedence
  preset is written last and actually wins after `integration use`
  rescaffolds two overlapping preset command overrides. Verified this
  reproduces the previously reported reversed-priority bug and that the
  fix resolves it.
- _register_commands_for_active_agent now checks for the "ai" key's
  presence separately from its value: a missing key still falls back to
  detection-based registration for all agents, but a recorded, malformed
  value (non-string or empty, e.g. [] or null) now fails closed
  (registers nothing) instead of being treated as "no active
  integration" or reaching AGENT_CONFIGS.get() with an unhashable key
  and raising TypeError.
- Updated docs/reference/presets.md and docs/reference/integrations.md
  to describe active-only preset/extension registration and clarify
  that `integration use`/`switch` is the activation point for
  installed extensions and presets, and that `upgrade` only
  re-registers them for the active integration.

Adds regression tests: two enabled presets overriding the same command
with different priorities (priority winner must survive `use`
rescaffolding), and a malformed recorded `ai` value ([]) for
`extension add`.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
marcelsafin
2026-07-10 23:08:40 +02:00
parent e9e90727ac
commit 2486c08c57
5 changed files with 154 additions and 5 deletions

View File

@@ -88,6 +88,8 @@ Installs the specified integration into the current project. If another integrat
Installing an additional integration does not change the default integration. Use `specify integration use <key>` to change the default. Installing an additional integration does not change the default integration. Use `specify integration use <key>` to change the default.
Installed extensions and presets are not registered for a non-default integration at install time — they follow the currently active (default) integration only. `specify integration use <key>` (or `switch <key>`) is what rescaffolds them for the newly active integration.
> **Note:** All integration management commands require a project already initialized with `specify init`. To start a new project with a specific agent, use `specify init <project> --integration <key>` instead. > **Note:** All integration management commands require a project already initialized with `specify init`. To start a new project with a specific agent, use `specify init <project> --integration <key>` instead.
**Version note:** Controlled multi-install support was introduced in Spec Kit 0.8.5. If `specify integration install <key>` says another integration is already installed and only suggests `switch` or `uninstall`, check your local CLI with `specify version` and upgrade it. Running a one-shot command such as `uvx --from git+https://github.com/github/spec-kit.git specify ...` uses a temporary copy for that command only; it does not update the persistent `specify` executable on your `PATH`. **Version note:** Controlled multi-install support was introduced in Spec Kit 0.8.5. If `specify integration install <key>` says another integration is already installed and only suggests `switch` or `uninstall`, check your local CLI with `specify version` and upgrade it. Running a one-shot command such as `uvx --from git+https://github.com/github/spec-kit.git specify ...` uses a temporary copy for that command only; it does not update the persistent `specify` executable on your `PATH`.
@@ -121,7 +123,7 @@ specify integration switch <key>
| `--refresh-shared-infra` | Also overwrite shared infrastructure files even if you customized them (otherwise customizations are preserved) | | `--refresh-shared-infra` | Also overwrite shared infrastructure files even if you customized them (otherwise customizations are preserved) |
| `--integration-options` | Options for the target integration when it is not already installed | | `--integration-options` | Options for the target integration when it is not already installed |
If the target integration is not already installed, equivalent to running `uninstall` followed by `install` in a single step. In this mode, `--force` controls whether modified files from the removed integration are deleted. If the target integration is already installed, `switch` only changes the default integration, like `use`; in this mode, `--force` controls whether managed shared templates are overwritten while the default changes. `--integration-options` is rejected for already-installed targets because changing integration options requires reinstalling managed files; run `upgrade <key> --integration-options ...` first, then `use <key>`. If the target integration is not already installed, equivalent to running `uninstall` followed by `install` in a single step. In this mode, `--force` controls whether modified files from the removed integration are deleted. If the target integration is already installed, `switch` only changes the default integration, like `use`; in this mode, `--force` controls whether managed shared templates are overwritten while the default changes. `--integration-options` is rejected for already-installed targets because changing integration options requires reinstalling managed files; run `upgrade <key> --integration-options ...` first, then `use <key>`. Like `use`, `switch` rescaffolds installed extensions and presets for the target integration once it becomes the default.
## Use an Installed Integration ## Use an Installed Integration
@@ -135,6 +137,8 @@ specify integration use <key>
Sets the default integration without uninstalling any other installed integrations. This also refreshes managed shared templates so command references match the new default integration's invocation style. Modified or untracked shared templates are preserved unless `--force` is used. Sets the default integration without uninstalling any other installed integrations. This also refreshes managed shared templates so command references match the new default integration's invocation style. Modified or untracked shared templates are preserved unless `--force` is used.
`use` is also the activation point for installed extensions and presets: it re-registers every enabled extension's and preset's command overrides (and skills, for skills-mode agents) for the newly active integration, so artifacts installed while a different integration was active are rescaffolded here rather than at install time.
## Upgrade an Integration ## Upgrade an Integration
```bash ```bash
@@ -149,6 +153,8 @@ specify integration upgrade [<key>]
Reinstalls an installed integration with updated templates and commands (e.g., after upgrading Spec Kit). Defaults to the default integration; if a key is provided, it must be one of the installed integrations. Detects locally modified files and blocks the upgrade unless `--force` is used. Stale files from the previous install that are no longer needed are removed automatically. Shared templates stay aligned with the default integration even when upgrading a non-default integration. Reinstalls an installed integration with updated templates and commands (e.g., after upgrading Spec Kit). Defaults to the default integration; if a key is provided, it must be one of the installed integrations. Detects locally modified files and blocks the upgrade unless `--force` is used. Stale files from the previous install that are no longer needed are removed automatically. Shared templates stay aligned with the default integration even when upgrading a non-default integration.
Enabled extensions and presets are re-registered only when upgrading the currently active (default) integration; upgrading a non-default integration does not touch its extension or preset artifacts — `use`/`switch` that integration afterward to rescaffold them.
## Report Integration Status ## Report Integration Status
```bash ```bash
@@ -288,3 +294,7 @@ CLI-based integrations (like Claude Code, Gemini CLI) require the tool to be ins
### When should I use `upgrade` vs `switch`? ### When should I use `upgrade` vs `switch`?
Use `upgrade` when you've upgraded Spec Kit and want to refresh an installed integration's managed files. Use `switch` when you want to replace the current default with another integration; if the target is already installed, `switch` behaves like `use`. Use `upgrade` when you've upgraded Spec Kit and want to refresh an installed integration's managed files. Use `switch` when you want to replace the current default with another integration; if the target is already installed, `switch` behaves like `use`.
### Do extensions and presets I install apply to every installed integration?
No. Extensions (`specify extension add`) and presets (`specify preset add`) register their command overrides for the currently active (default) integration only, even if other integrations are installed. A non-default integration does not receive those artifacts until it becomes the default: `specify integration use <key>` (or `switch <key>`) rescaffolds every enabled extension and preset for the newly active integration. `specify integration upgrade` follows the same rule — it only re-registers extensions and presets when upgrading the active integration.

View File

@@ -139,7 +139,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. 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 detected agent directories instead of being re-resolved by agents. During preset install, Spec Kit registers command files for the preset being installed; post-install and post-removal reconciliation then recomputes and writes the effective command content for affected command names based on the active stack. Agents do not re-resolve the stack each time they run a command. 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. A non-active installed integration does not receive these command files until it becomes the default — `specify integration use <key>` (or `switch <key>`) rescaffolds enabled presets for the newly active integration. Agents do not re-resolve the stack each time they run a command.
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. 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.

View File

@@ -995,6 +995,11 @@ class ExtensionManager:
from ``AGENT_CONFIGS``) is not treated as "no active integration" from ``AGENT_CONFIGS``) is not treated as "no active integration"
it must not cause registration to target other detected agents. it must not cause registration to target other detected agents.
A recorded but malformed ``ai`` value (non-string, e.g. ``[]`` or
``null``) is also not "no active integration" — corrupted
init-options must fail closed (register nothing) rather than
fall back to registering every detected agent.
Returns: Returns:
Mapping of agent name to registered command names, matching the Mapping of agent name to registered command names, matching the
``registered_commands`` registry shape. ``registered_commands`` registry shape.
@@ -1005,9 +1010,8 @@ class ExtensionManager:
init_options = load_init_options(self.project_root) init_options = load_init_options(self.project_root)
if not isinstance(init_options, dict): if not isinstance(init_options, dict):
init_options = {} init_options = {}
active_agent = init_options.get("ai")
if not active_agent: if "ai" not in init_options:
return registrar.register_commands_for_all_agents( return registrar.register_commands_for_all_agents(
manifest, manifest,
extension_dir, extension_dir,
@@ -1016,6 +1020,14 @@ class ExtensionManager:
create_missing_active_skills_dir=True, create_missing_active_skills_dir=True,
) )
active_agent = init_options.get("ai")
if not isinstance(active_agent, str) or not active_agent:
# A recorded key was found but it is malformed (not a non-empty
# string). Fail closed instead of falling back to all agents or
# passing a non-string key into AGENT_CONFIGS.get() below, which
# would raise TypeError for unhashable values like a list.
return {}
# A recorded active key with no registrar config (e.g. "generic", # A recorded active key with no registrar config (e.g. "generic",
# deliberately excluded from AGENT_CONFIGS) has nothing to register # deliberately excluded from AGENT_CONFIGS) has nothing to register
# through this path, but it is still an active integration. Passing # through this path, but it is still an active integration. Passing

View File

@@ -707,12 +707,18 @@ class PresetManager:
active integration from init-options themselves, so this re-runs them active integration from init-options themselves, so this re-runs them
for every enabled preset and merges the fresh result for for every enabled preset and merges the fresh result for
``agent_name`` into its stored registry metadata. ``agent_name`` into its stored registry metadata.
Presets are processed in *reverse* priority order (lowest-precedence
first). Each pass overwrites the same target command/skill files, so
writing the highest-precedence preset last is what makes it win when
two enabled presets override the same command — matching the
priority stack documented for ``list_by_priority()``.
""" """
if not agent_name: if not agent_name:
return return
resolver = PresetResolver(self.project_root) resolver = PresetResolver(self.project_root)
for pack_id, metadata in self.registry.list_by_priority(): for pack_id, metadata in reversed(self.registry.list_by_priority()):
pack_dir = self.presets_dir / pack_id pack_dir = self.presets_dir / pack_id
manifest = resolver._get_manifest(pack_dir) manifest = resolver._get_manifest(pack_dir)
if manifest is None: if manifest is None:

View File

@@ -1453,6 +1453,43 @@ class TestIntegrationInstall:
"detected agents (#2948)" "detected agents (#2948)"
) )
def test_extension_add_malformed_ai_value_fails_closed(self, tmp_path):
"""A recorded but malformed ``ai`` value (e.g. a list) must not be
treated as "no active integration recorded" and must not crash.
Before the fix, ``init_options.get("ai")`` being falsy (``[]``,
``""``, ``0``) triggered the same all-agents fallback as a missing
key, and a *truthy* non-string value (e.g. a non-empty list) would
reach ``AGENT_CONFIGS.get(active_agent)`` and raise ``TypeError``
because a list is unhashable. Corrupted init-options must instead
fail closed: register nothing rather than crash or back-fill every
detected agent.
"""
project = _init_project(tmp_path, "claude")
result = _run_in_project(project, [
"integration", "install", "codex",
"--script", "sh",
])
assert result.exit_code == 0, result.output
init_options_path = project / ".specify" / "init-options.json"
init_options = json.loads(init_options_path.read_text(encoding="utf-8"))
init_options["ai"] = []
init_options_path.write_text(json.dumps(init_options), encoding="utf-8")
result = _run_in_project(project, ["extension", "add", "git"])
assert result.exit_code == 0, f"extension add failed: {result.output}"
registry_path = project / ".specify" / "extensions" / ".registry"
registered = json.loads(registry_path.read_text(encoding="utf-8"))[
"extensions"
]["git"]["registered_commands"]
assert registered == {}, (
"a malformed recorded 'ai' value must fail closed, not "
"back-fill every detected agent (#2948)"
)
# ── uninstall ──────────────────────────────────────────────────────── # ── uninstall ────────────────────────────────────────────────────────
@@ -1725,6 +1762,90 @@ class TestIntegrationUse:
assert "codex" in registered, "use registers presets for the new active agent" assert "codex" in registered, "use registers presets for the new active agent"
assert "claude" in registered, "the previous agent's registration is preserved" assert "claude" in registered, "the previous agent's registration is preserved"
def test_use_reregisters_presets_highest_precedence_last(self, tmp_path):
"""When two enabled presets override the same command, the
higher-precedence preset (lower priority number) must win the
materialized file after ``integration use`` rescaffolds them.
``register_enabled_presets_for_agent`` iterates presets and each
pass overwrites the same target file, so the write order matters.
Before the fix, presets were processed lowest-number-first (highest
precedence first), so the lower-precedence preset was written last
and won -- reversing the documented priority stack (#2948).
"""
project = _init_project(tmp_path, "claude")
result = _run_in_project(project, [
"integration", "install", "codex",
"--script", "sh",
])
assert result.exit_code == 0, result.output
import yaml
def _make_preset(pack_id: str, content: str) -> Path:
src = tmp_path / pack_id
(src / "commands").mkdir(parents=True)
(src / "commands" / "speckit.specify.md").write_text(
f"---\ndescription: {pack_id}\n---\n{content}\n",
encoding="utf-8",
)
manifest_data = {
"schema_version": "1.0",
"preset": {
"id": pack_id,
"name": pack_id,
"version": "1.0.0",
"description": f"Test preset {pack_id}",
},
"requires": {"speckit_version": ">=0.1.0"},
"provides": {
"templates": [
{
"type": "command",
"name": "speckit.specify",
"file": "commands/speckit.specify.md",
}
]
},
}
(src / "preset.yml").write_text(yaml.dump(manifest_data), encoding="utf-8")
return src
# Lower-precedence preset (higher priority number), installed first.
low_precedence_src = _make_preset("low-precedence-preset", "LOW PRECEDENCE CONTENT")
result = _run_in_project(project, [
"preset", "add", "--dev", str(low_precedence_src), "--priority", "20",
])
assert result.exit_code == 0, f"preset add (low) failed: {result.output}"
# Higher-precedence preset (lower priority number), installed second.
high_precedence_src = _make_preset("high-precedence-preset", "HIGH PRECEDENCE CONTENT")
result = _run_in_project(project, [
"preset", "add", "--dev", str(high_precedence_src), "--priority", "1",
])
assert result.exit_code == 0, f"preset add (high) failed: {result.output}"
# Sanity: the priority stack already picks the high-precedence
# preset's content for the active (claude) integration.
claude_skill = project / ".claude" / "skills" / "speckit-specify" / "SKILL.md"
assert "HIGH PRECEDENCE CONTENT" in claude_skill.read_text(encoding="utf-8")
assert "LOW PRECEDENCE CONTENT" not in claude_skill.read_text(encoding="utf-8")
result = _run_in_project(project, ["integration", "use", "codex"])
assert result.exit_code == 0, result.output
# After rescaffolding for the newly active codex integration, the
# high-precedence preset must still win -- not whichever preset
# register_enabled_presets_for_agent happened to write last.
codex_skill = project / ".agents" / "skills" / "speckit-specify" / "SKILL.md"
content = codex_skill.read_text(encoding="utf-8")
assert "HIGH PRECEDENCE CONTENT" in content, (
"highest-precedence preset must win after `use` rescaffolds "
"presets for the newly active integration (#2948)"
)
assert "LOW PRECEDENCE CONTENT" not in content
def test_use_refreshes_shared_templates_between_command_styles(self, tmp_path): def test_use_refreshes_shared_templates_between_command_styles(self, tmp_path):
project = _init_project(tmp_path, "claude") project = _init_project(tmp_path, "claude")
template = project / ".specify" / "templates" / "plan-template.md" template = project / ".specify" / "templates" / "plan-template.md"