mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix: address review feedback on active-only extension registration
- Restrict the extension-add active-integration fallback to projects with no recorded active key at all. A recorded but unsupported key (e.g. "generic", deliberately excluded from AGENT_CONFIGS) no longer falls back to registering every detected agent. - Apply the same single-active rule to preset command overrides: PresetManager._register_commands now scopes registration to the active integration via only_agent. - Add PresetManager.register_enabled_presets_for_agent, mirroring ExtensionManager.register_enabled_extensions_for_agent, and call it from integration use/switch/upgrade (active only) alongside the existing extension re-registration so presets are rescaffolded on activation instead of being written for inactive integrations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -988,9 +988,12 @@ class ExtensionManager:
|
||||
init-options gets command files. Non-active integrations receive them
|
||||
when selected via ``integration use`` / ``switch`` (rescaffold).
|
||||
|
||||
Projects without a recorded active integration (pre-init-options
|
||||
Projects without a recorded active integration at all (pre-init-options
|
||||
layouts or direct library use) fall back to detection-based
|
||||
registration for all agents.
|
||||
registration for all agents. A *recorded* active key that has no
|
||||
registrar config (e.g. ``generic``, which is deliberately excluded
|
||||
from ``AGENT_CONFIGS``) is not treated as "no active integration" —
|
||||
it must not cause registration to target other detected agents.
|
||||
|
||||
Returns:
|
||||
Mapping of agent name to registered command names, matching the
|
||||
@@ -1004,7 +1007,7 @@ class ExtensionManager:
|
||||
init_options = {}
|
||||
active_agent = init_options.get("ai")
|
||||
|
||||
if not active_agent or active_agent not in registrar.AGENT_CONFIGS:
|
||||
if not active_agent:
|
||||
return registrar.register_commands_for_all_agents(
|
||||
manifest,
|
||||
extension_dir,
|
||||
@@ -1013,9 +1016,15 @@ class ExtensionManager:
|
||||
create_missing_active_skills_dir=True,
|
||||
)
|
||||
|
||||
agent_config = registrar.AGENT_CONFIGS[active_agent]
|
||||
# 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
|
||||
# it as only_agent below naturally yields no matches instead of
|
||||
# falling back to registering every detected agent.
|
||||
agent_config = registrar.AGENT_CONFIGS.get(active_agent)
|
||||
if (
|
||||
is_ai_skills_enabled(init_options)
|
||||
agent_config
|
||||
and is_ai_skills_enabled(init_options)
|
||||
and agent_config.get("extension") != "/SKILL.md"
|
||||
):
|
||||
# Active agent runs skills mode: extension artifacts render as
|
||||
|
||||
@@ -419,6 +419,38 @@ def _unregister_extensions_for_agent(
|
||||
)
|
||||
|
||||
|
||||
def _register_presets_for_agent(
|
||||
project_root: Path,
|
||||
agent_key: str,
|
||||
*,
|
||||
continuing: str,
|
||||
) -> None:
|
||||
"""Register all enabled presets' command overrides/skills for ``agent_key``.
|
||||
|
||||
Presets follow the same single-active rule as extensions (#2948):
|
||||
``use`` / ``switch`` re-register enabled presets for the agent they
|
||||
activate (rescaffold), so a preset installed while a different
|
||||
integration was active is not left targeting that inactive integration.
|
||||
|
||||
Best-effort: never aborts the surrounding integration operation.
|
||||
"""
|
||||
try:
|
||||
from ..presets import PresetManager
|
||||
|
||||
preset_mgr = PresetManager(project_root)
|
||||
preset_mgr.register_enabled_presets_for_agent(agent_key)
|
||||
except Exception as preset_err:
|
||||
from .. import _print_cli_warning
|
||||
|
||||
_print_cli_warning(
|
||||
"register preset artifacts for",
|
||||
"integration",
|
||||
agent_key,
|
||||
preset_err,
|
||||
continuing=continuing,
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# CLI formatting helpers (re-exported from _commands.py)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -28,6 +28,7 @@ from ._helpers import (
|
||||
_read_integration_json,
|
||||
_refresh_init_options_speckit_version,
|
||||
_register_extensions_for_agent,
|
||||
_register_presets_for_agent,
|
||||
_remove_integration_json,
|
||||
_resolve_integration_options,
|
||||
_resolve_integration_script_type,
|
||||
@@ -130,6 +131,14 @@ def integration_switch(
|
||||
"need re-registration."
|
||||
),
|
||||
)
|
||||
_register_presets_for_agent(
|
||||
project_root,
|
||||
target,
|
||||
continuing=(
|
||||
"The integration switch succeeded, but installed presets may "
|
||||
"need re-registration."
|
||||
),
|
||||
)
|
||||
console.print(f"\n[green]✓[/green] Default integration set to [bold]{target}[/bold].")
|
||||
raise typer.Exit(0)
|
||||
|
||||
@@ -327,6 +336,11 @@ def integration_switch(
|
||||
target,
|
||||
continuing="The integration switch succeeded, but installed extensions may need re-registration.",
|
||||
)
|
||||
_register_presets_for_agent(
|
||||
project_root,
|
||||
target,
|
||||
continuing="The integration switch succeeded, but installed presets may need re-registration.",
|
||||
)
|
||||
|
||||
name = (target_integration.config or {}).get("name", target)
|
||||
console.print(f"\n[green]✓[/green] Switched to integration '{name}'")
|
||||
@@ -491,10 +505,10 @@ def integration_upgrade(
|
||||
if stale_removed:
|
||||
console.print(f" Removed {len(stale_removed)} stale file(s) from previous install")
|
||||
|
||||
# Re-register enabled extensions only when upgrading the *active*
|
||||
# integration, so its extension commands are (re)created after the
|
||||
# upgrade settled (Phase 2 included). Done outside the try/except above
|
||||
# so this best-effort step cannot affect upgrade success. Non-active
|
||||
# Re-register enabled extensions and presets only when upgrading the
|
||||
# *active* integration, so its command artifacts are (re)created after
|
||||
# the upgrade settled (Phase 2 included). Done outside the try/except
|
||||
# above so this best-effort step cannot affect upgrade success. Non-active
|
||||
# integrations are rescaffolded by `use` / `switch` instead — the #2886
|
||||
# back-fill for non-active agents was removed at maintainer request
|
||||
# (#2948).
|
||||
@@ -504,6 +518,11 @@ def integration_upgrade(
|
||||
key,
|
||||
continuing="The integration was upgraded, but installed extensions may need re-registration.",
|
||||
)
|
||||
_register_presets_for_agent(
|
||||
project_root,
|
||||
key,
|
||||
continuing="The integration was upgraded, but installed presets may need re-registration.",
|
||||
)
|
||||
|
||||
name = (integration.config or {}).get("name", key)
|
||||
console.print(f"\n[green]✓[/green] Integration '{name}' upgraded successfully")
|
||||
|
||||
@@ -18,6 +18,7 @@ from ._commands import integration_app, integration_catalog_app
|
||||
from ._helpers import (
|
||||
_read_integration_json,
|
||||
_register_extensions_for_agent,
|
||||
_register_presets_for_agent,
|
||||
_resolve_integration_options,
|
||||
_set_default_integration_or_exit,
|
||||
)
|
||||
@@ -248,6 +249,11 @@ def integration_use(
|
||||
key,
|
||||
continuing="The integration was selected, but installed extensions may need re-registration.",
|
||||
)
|
||||
_register_presets_for_agent(
|
||||
project_root,
|
||||
key,
|
||||
continuing="The integration was selected, but installed presets may need re-registration.",
|
||||
)
|
||||
console.print(f"[green]✓[/green] Default integration set to [bold]{key}[/bold].")
|
||||
|
||||
|
||||
|
||||
@@ -28,7 +28,7 @@ from packaging import version as pkg_version
|
||||
from packaging.specifiers import SpecifierSet, InvalidSpecifier
|
||||
|
||||
from ..extensions import REINSTALL_COMMAND, ExtensionRegistry, normalize_priority
|
||||
from .._init_options import is_ai_skills_enabled
|
||||
from .._init_options import is_ai_skills_enabled, load_init_options
|
||||
from ..integrations.base import IntegrationBase
|
||||
from .._utils import dump_frontmatter, version_satisfies
|
||||
from ..shared_infra import verify_archive_sha256
|
||||
@@ -680,10 +680,84 @@ class PresetManager:
|
||||
return {}
|
||||
|
||||
registrar = CommandRegistrar()
|
||||
|
||||
# Single-active rule (#2948): preset command overrides register for
|
||||
# the active integration only. A project without a recorded active
|
||||
# integration falls back to detection-based registration for all
|
||||
# agents; a recorded key with no registrar config (e.g. "generic")
|
||||
# naturally yields no matches via only_agent instead of falling back.
|
||||
active_agent = load_init_options(self.project_root).get("ai")
|
||||
return registrar.register_commands_for_all_agents(
|
||||
commands_to_register, manifest.id, preset_dir, self.project_root
|
||||
commands_to_register,
|
||||
manifest.id,
|
||||
preset_dir,
|
||||
self.project_root,
|
||||
only_agent=active_agent,
|
||||
)
|
||||
|
||||
def register_enabled_presets_for_agent(self, agent_name: str) -> None:
|
||||
"""Re-register enabled presets' command overrides and skills for ``agent_name``.
|
||||
|
||||
Mirrors ``ExtensionManager.register_enabled_extensions_for_agent`` for
|
||||
presets (#2948): ``integration use`` / ``switch`` call this for the
|
||||
newly active agent so a preset installed while a different
|
||||
integration was active gets rescaffolded on activation, instead of
|
||||
writing artifacts for inactive integrations at install time.
|
||||
``_register_commands`` / ``_register_skills`` already resolve the
|
||||
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.
|
||||
"""
|
||||
if not agent_name:
|
||||
return
|
||||
|
||||
resolver = PresetResolver(self.project_root)
|
||||
for pack_id, metadata in self.registry.list_by_priority():
|
||||
pack_dir = self.presets_dir / pack_id
|
||||
manifest = resolver._get_manifest(pack_dir)
|
||||
if manifest is None:
|
||||
continue
|
||||
|
||||
# Isolate per-preset failures: one preset that fails to register
|
||||
# must not abort registration of the remaining enabled presets.
|
||||
try:
|
||||
updates: Dict[str, Any] = {}
|
||||
|
||||
registered_commands = self._register_commands(manifest, pack_dir)
|
||||
existing_commands = metadata.get("registered_commands", {})
|
||||
if not isinstance(existing_commands, dict):
|
||||
existing_commands = {}
|
||||
merged_commands = copy.deepcopy(existing_commands)
|
||||
if registered_commands.get(agent_name):
|
||||
merged_commands[agent_name] = registered_commands[agent_name]
|
||||
if merged_commands != existing_commands:
|
||||
updates["registered_commands"] = merged_commands
|
||||
|
||||
registered_skills = self._register_skills(manifest, pack_dir)
|
||||
if registered_skills:
|
||||
existing_skills = metadata.get("registered_skills", [])
|
||||
if not isinstance(existing_skills, list):
|
||||
existing_skills = []
|
||||
merged_skills = list(
|
||||
dict.fromkeys(existing_skills + registered_skills)
|
||||
)
|
||||
if merged_skills != existing_skills:
|
||||
updates["registered_skills"] = merged_skills
|
||||
|
||||
if updates:
|
||||
self.registry.update(pack_id, updates)
|
||||
except Exception as pack_err:
|
||||
from .. import _print_cli_warning
|
||||
|
||||
_print_cli_warning(
|
||||
"register preset artifacts for",
|
||||
"preset",
|
||||
pack_id,
|
||||
pack_err,
|
||||
continuing="Continuing with the remaining presets.",
|
||||
)
|
||||
continue
|
||||
|
||||
def _unregister_commands(self, registered_commands: Dict[str, List[str]]) -> None:
|
||||
"""Remove previously registered command files from agent directories.
|
||||
|
||||
|
||||
@@ -1418,6 +1418,41 @@ class TestIntegrationInstall:
|
||||
project / ".agents" / "skills" / "speckit-git-feature" / "SKILL.md"
|
||||
).exists()
|
||||
|
||||
def test_extension_add_generic_active_does_not_backfill_other_agents(self, tmp_path):
|
||||
"""A recorded but unsupported active key (``generic``) must not
|
||||
fall back to registering every detected agent.
|
||||
|
||||
``generic`` is deliberately excluded from ``AGENT_CONFIGS`` because
|
||||
its output directory is only known via ``--commands-dir``, not a
|
||||
static config. Before the fix, treating that active key like "no
|
||||
active integration recorded" made the fallback register the
|
||||
extension for every other detected agent — exactly the multi-target
|
||||
behavior #2948 is meant to stop.
|
||||
"""
|
||||
project = _init_project(
|
||||
tmp_path, "generic",
|
||||
integration_options="--commands-dir .myagent/commands",
|
||||
)
|
||||
|
||||
result = _run_in_project(project, [
|
||||
"integration", "install", "codex",
|
||||
"--script", "sh",
|
||||
"--force",
|
||||
])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
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 "codex" not in registered, (
|
||||
"a recorded but unsupported active key must not target other "
|
||||
"detected agents (#2948)"
|
||||
)
|
||||
|
||||
|
||||
# ── uninstall ────────────────────────────────────────────────────────
|
||||
|
||||
@@ -1624,6 +1659,72 @@ class TestIntegrationUse:
|
||||
assert result.exit_code != 0
|
||||
assert "not installed" in result.output
|
||||
|
||||
def test_use_registers_presets_for_the_newly_active_agent(self, tmp_path):
|
||||
"""``integration use`` is the single rescaffold point for presets too.
|
||||
|
||||
Mirrors the extension single-active rule (#2948): a preset command
|
||||
override installed while ``claude`` was active must not target the
|
||||
inactive ``codex`` integration, and switching via ``integration use``
|
||||
must rescaffold it there.
|
||||
"""
|
||||
project = _init_project(tmp_path, "claude")
|
||||
|
||||
result = _run_in_project(project, [
|
||||
"integration", "install", "codex",
|
||||
"--script", "sh",
|
||||
])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
preset_src = tmp_path / "cmd-preset"
|
||||
(preset_src / "commands").mkdir(parents=True)
|
||||
(preset_src / "commands" / "speckit.specify.md").write_text(
|
||||
"---\ndescription: Overridden specify\n---\nOverridden content\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
manifest_data = {
|
||||
"schema_version": "1.0",
|
||||
"preset": {
|
||||
"id": "cmd-preset",
|
||||
"name": "Command Preset",
|
||||
"version": "1.0.0",
|
||||
"description": "Test preset with a command override",
|
||||
},
|
||||
"requires": {"speckit_version": ">=0.1.0"},
|
||||
"provides": {
|
||||
"templates": [
|
||||
{
|
||||
"type": "command",
|
||||
"name": "speckit.specify",
|
||||
"file": "commands/speckit.specify.md",
|
||||
}
|
||||
]
|
||||
},
|
||||
}
|
||||
import yaml
|
||||
|
||||
(preset_src / "preset.yml").write_text(yaml.dump(manifest_data), encoding="utf-8")
|
||||
|
||||
result = _run_in_project(project, ["preset", "add", "--dev", str(preset_src)])
|
||||
assert result.exit_code == 0, f"preset add failed: {result.output}"
|
||||
|
||||
registry_path = project / ".specify" / "presets" / ".registry"
|
||||
registered = json.loads(registry_path.read_text(encoding="utf-8"))[
|
||||
"presets"
|
||||
]["cmd-preset"]["registered_commands"]
|
||||
assert "claude" in registered, "active integration gets the preset command override"
|
||||
assert "codex" not in registered, (
|
||||
"non-active integration must not be registered on preset add (#2948)"
|
||||
)
|
||||
|
||||
result = _run_in_project(project, ["integration", "use", "codex"])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
registered = json.loads(registry_path.read_text(encoding="utf-8"))[
|
||||
"presets"
|
||||
]["cmd-preset"]["registered_commands"]
|
||||
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_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