mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
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:
@@ -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.
|
||||
|
||||
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.
|
||||
|
||||
**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) |
|
||||
| `--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
|
||||
|
||||
@@ -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.
|
||||
|
||||
`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
|
||||
|
||||
```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.
|
||||
|
||||
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
|
||||
|
||||
```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`?
|
||||
|
||||
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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
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.
|
||||
|
||||
|
||||
@@ -995,6 +995,11 @@ class ExtensionManager:
|
||||
from ``AGENT_CONFIGS``) is not treated as "no active integration" —
|
||||
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:
|
||||
Mapping of agent name to registered command names, matching the
|
||||
``registered_commands`` registry shape.
|
||||
@@ -1005,9 +1010,8 @@ class ExtensionManager:
|
||||
init_options = load_init_options(self.project_root)
|
||||
if not isinstance(init_options, dict):
|
||||
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(
|
||||
manifest,
|
||||
extension_dir,
|
||||
@@ -1016,6 +1020,14 @@ class ExtensionManager:
|
||||
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",
|
||||
# deliberately excluded from AGENT_CONFIGS) has nothing to register
|
||||
# through this path, but it is still an active integration. Passing
|
||||
|
||||
@@ -707,12 +707,18 @@ class PresetManager:
|
||||
active integration from init-options themselves, so this re-runs them
|
||||
for every enabled preset and merges the fresh result for
|
||||
``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:
|
||||
return
|
||||
|
||||
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
|
||||
manifest = resolver._get_manifest(pack_dir)
|
||||
if manifest is None:
|
||||
|
||||
@@ -1453,6 +1453,43 @@ class TestIntegrationInstall:
|
||||
"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 ────────────────────────────────────────────────────────
|
||||
|
||||
@@ -1725,6 +1762,90 @@ class TestIntegrationUse:
|
||||
assert "codex" in registered, "use registers presets for the new active agent"
|
||||
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):
|
||||
project = _init_project(tmp_path, "claude")
|
||||
template = project / ".specify" / "templates" / "plan-template.md"
|
||||
|
||||
Reference in New Issue
Block a user