diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index db53b7997f..506b9eedc2 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -1637,16 +1637,12 @@ def unregister_agent_artifacts(self, agent_name: str) -> None: def register_enabled_extensions_for_agent(self, agent_name: str) -> None: """Register installed, enabled extensions for ``agent_name``. - This is intended to be called after switching integrations. Command - registration is scoped to the explicit ``agent_name`` argument, but some - behavior still depends on the current init-options state (for example, - skills-mode handling uses the active ``ai`` / ``ai_skills`` settings). - - Callers should therefore pass the agent that has just been made active - in init-options; in normal use, ``agent_name`` is expected to match the - current ``ai`` value. This mirrors extension install behavior while - avoiding stale default-mode command directories when that active agent - is running in skills mode (notably Copilot ``--skills``). + Command-file registration is scoped to the explicit ``agent_name`` + argument, so this method can be used after install, upgrade, or switch. + Extension skill rendering is still scoped to the active ``ai`` / + ``ai_skills`` settings in init-options, so non-active skills-mode + targets receive command files here. Per-agent skills parity is tracked + separately in #2948. """ if not agent_name: return @@ -1698,11 +1694,22 @@ def register_enabled_extensions_for_agent(self, agent_name: str) -> None: if new_registered != registered_commands: updates["registered_commands"] = new_registered - registered_skills = self._register_extension_skills(manifest, ext_dir) - if registered_skills: - existing_skills = self._valid_name_list(metadata.get("registered_skills", [])) - merged_skills = list(dict.fromkeys(existing_skills + registered_skills)) - updates["registered_skills"] = merged_skills + # Extension *skills* are only ever rendered for the active agent: + # `_register_extension_skills` resolves the skills dir and + # frontmatter from init-options["ai"], ignoring ``agent_name``. + # When this method runs for a non-active agent — as install/upgrade + # now do for a secondary integration (#2886) — the skills pass would + # re-render the *active* agent's extension skills as a side effect, + # resurrecting skill files the user deliberately deleted. Skip it + # unless the target is the active agent; `switch` is unaffected + # because it activates the target before registering. (Rendering + # skills for a non-active target is tracked separately in #2948.) + if agent_name == active_agent: + registered_skills = self._register_extension_skills(manifest, ext_dir) + if registered_skills: + existing_skills = self._valid_name_list(metadata.get("registered_skills", [])) + merged_skills = list(dict.fromkeys(existing_skills + registered_skills)) + updates["registered_skills"] = merged_skills if updates: self.registry.update(ext_id, updates) diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index a95f36563a..9c18895904 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -3,7 +3,7 @@ import os from pathlib import Path -from typing import Any +from typing import Any, Callable import typer @@ -385,6 +385,93 @@ def _set_default_integration_or_exit(*args: Any, **kwargs: Any) -> None: raise typer.Exit(1) +# --------------------------------------------------------------------------- +# Extension (un)registration helpers (shared by use / switch / upgrade) +# --------------------------------------------------------------------------- + +def _best_effort_extension_op( + project_root: Path, + agent_key: str, + op: Callable[[Any, str], None], + *, + phase: str, + continuing: str, +) -> None: + """Run a best-effort ``ExtensionManager`` operation for ``agent_key``. + + ``op`` receives the ``ExtensionManager`` and ``agent_key``. Any failure is + surfaced as a warning via ``_print_cli_warning`` and never aborts the + surrounding integration operation. ``continuing`` describes what already + succeeded so the warning makes the partial outcome clear. + """ + try: + from ..extensions import ExtensionManager + + ext_mgr = ExtensionManager(project_root) + op(ext_mgr, agent_key) + except Exception as ext_err: + from .. import _print_cli_warning + + _print_cli_warning(phase, "integration", agent_key, ext_err, continuing=continuing) + + +def _register_extensions_for_agent( + project_root: Path, + agent_key: str, + *, + continuing: str, +) -> None: + """Register all enabled extensions' commands/skills for ``agent_key``. + + ``use`` / ``switch`` re-register enabled extensions for the agent they + activate; ``upgrade`` backfills them for the refreshed agent. Plain + ``install`` deliberately does not call this helper so adding a secondary + integration has no extension side effects until it is selected or upgraded. + See issue #2886. + + Known limitation: extension *skill* rendering is scoped to the active + agent (init-options track a single ``ai`` / ``ai_skills`` pair). A + skills-mode agent registered while it is *not* the active agent (e.g. + Copilot ``--skills`` registered while non-active) therefore + receives command files rather than skills here — matching ``extension + add``'s multi-agent behavior. ``use`` / ``switch`` avoid this because they + make the target the active agent first. Per-agent skills parity is tracked in + #2948. + + Best-effort: never aborts the surrounding integration operation. Callers + invoke it *after* the use/upgrade/switch transaction has committed so a + failure here cannot trigger a rollback. + """ + _best_effort_extension_op( + project_root, + agent_key, + lambda mgr, key: mgr.register_enabled_extensions_for_agent(key), + phase="register extension artifacts for", + continuing=continuing, + ) + + +def _unregister_extensions_for_agent( + project_root: Path, + agent_key: str, + *, + continuing: str, +) -> None: + """Best-effort removal of ``agent_key``'s extension artifacts. + + Used by ``switch`` when uninstalling the previous integration so its + extension command/skill files don't linger as orphans in the old agent's + directory. + """ + _best_effort_extension_op( + project_root, + agent_key, + lambda mgr, key: mgr.unregister_agent_artifacts(key), + phase="clean up extension artifacts for", + continuing=continuing, + ) + + # --------------------------------------------------------------------------- # CLI formatting helpers (re-exported from _commands.py) # --------------------------------------------------------------------------- diff --git a/src/specify_cli/integrations/_migrate_commands.py b/src/specify_cli/integrations/_migrate_commands.py index 01cb51d687..cecf683bdf 100644 --- a/src/specify_cli/integrations/_migrate_commands.py +++ b/src/specify_cli/integrations/_migrate_commands.py @@ -26,12 +26,14 @@ _get_speckit_version, _read_integration_json, _refresh_init_options_speckit_version, + _register_extensions_for_agent, _remove_integration_json, _resolve_integration_options, _resolve_integration_script_type, _resolve_script_type, _set_default_integration, _set_default_integration_or_exit, + _unregister_extensions_for_agent, _update_init_options_for_integration, _write_integration_json, ) @@ -170,19 +172,11 @@ def integration_switch( # Unregister extension commands for the old agent so they don't # remain as orphans in the old agent's directory. - try: - from ..extensions import ExtensionManager - - ext_mgr = ExtensionManager(project_root) - ext_mgr.unregister_agent_artifacts(installed_key) - except Exception as ext_err: - _print_cli_warning( - "clean up extension artifacts for", - "integration", - installed_key, - ext_err, - continuing="Continuing with integration switch; old extension artifacts may need manual cleanup.", - ) + _unregister_extensions_for_agent( + project_root, + installed_key, + continuing="Continuing with integration switch; old extension artifacts may need manual cleanup.", + ) # Clear metadata so a failed Phase 2 doesn't leave stale references installed_keys = [installed for installed in installed_keys if installed != installed_key] @@ -269,22 +263,6 @@ def integration_switch( parsed_options=parsed_options, ) - # Re-register extension commands for the new agent so that - # previously-installed extensions are available in the new integration. - try: - from ..extensions import ExtensionManager - - ext_mgr = ExtensionManager(project_root) - ext_mgr.register_enabled_extensions_for_agent(target) - except Exception as ext_err: - _print_cli_warning( - "register extension artifacts for", - "integration", - target, - ext_err, - continuing="The integration switch succeeded, but installed extensions may need re-registration.", - ) - except Exception as exc: # Attempt rollback of any files written by setup try: @@ -332,6 +310,15 @@ def integration_switch( ) raise typer.Exit(1) + # Re-register extension commands for the new agent so previously-installed + # extensions are available in it. Done after the try/except (the switch has + # committed) so this best-effort step can never trigger the rollback above. + _register_extensions_for_agent( + project_root, + target, + continuing="The integration switch succeeded, but installed extensions may need re-registration.", + ) + name = (target_integration.config or {}).get("name", target) console.print(f"\n[green]✓[/green] Switched to integration '{name}'") @@ -486,5 +473,17 @@ def integration_upgrade( if stale_removed: console.print(f" Removed {len(stale_removed)} stale file(s) from previous install") + # Re-register enabled extensions for the upgraded agent so its extension + # commands are (re)created — including agents installed before this + # back-fill existed. Mirrors switch for command registration; see #2886. + # Done after the upgrade has fully settled (Phase 2 included) and outside + # the try/except above so this best-effort step cannot affect upgrade + # success. + _register_extensions_for_agent( + project_root, + key, + continuing="The integration was upgraded, but installed extensions may need re-registration.", + ) + name = (integration.config or {}).get("name", key) console.print(f"\n[green]✓[/green] Integration '{name}' upgraded successfully") diff --git a/src/specify_cli/integrations/_query_commands.py b/src/specify_cli/integrations/_query_commands.py index dda74b0615..bb47e6142e 100644 --- a/src/specify_cli/integrations/_query_commands.py +++ b/src/specify_cli/integrations/_query_commands.py @@ -17,6 +17,7 @@ from ._commands import integration_app, integration_catalog_app from ._helpers import ( _read_integration_json, + _register_extensions_for_agent, _resolve_integration_options, _set_default_integration_or_exit, ) @@ -242,6 +243,11 @@ def integration_use( f"[cyan]specify integration use {key} --force[/cyan]." ), ) + _register_extensions_for_agent( + project_root, + key, + continuing="The integration was selected, but installed extensions may need re-registration.", + ) console.print(f"[green]✓[/green] Default integration set to [bold]{key}[/bold].") diff --git a/tests/integrations/test_integration_subcommand.py b/tests/integrations/test_integration_subcommand.py index 4c09a9163d..9bd7244fcf 100644 --- a/tests/integrations/test_integration_subcommand.py +++ b/tests/integrations/test_integration_subcommand.py @@ -15,19 +15,22 @@ runner = CliRunner() -def _init_project(tmp_path, integration="copilot"): +def _init_project(tmp_path, integration="copilot", integration_options=None): """Helper: init a spec-kit project with the given integration.""" project = tmp_path / "proj" project.mkdir() + args = [ + "init", "--here", + "--integration", integration, + "--script", "sh", + "--ignore-agent-tools", + ] + if integration_options: + args += ["--integration-options", integration_options] old_cwd = os.getcwd() try: os.chdir(project) - result = runner.invoke(app, [ - "init", "--here", - "--integration", integration, - "--script", "sh", - "--ignore-agent-tools", - ], catch_exceptions=False) + result = runner.invoke(app, args, catch_exceptions=False) finally: os.chdir(old_cwd) assert result.exit_code == 0, f"init failed: {result.output}" @@ -1236,6 +1239,137 @@ def test_install_bare_project_gets_shared_infra(self, tmp_path): assert "/speckit-specify" in script_content assert "/speckit.specify" not in script_content + def test_install_defers_extension_commands_until_use(self, tmp_path): + """Installing a second integration does not register enabled extensions. + + Maintainer-requested behavior for #2886: extension command back-fill is + limited to ``integration use`` / ``switch`` / ``upgrade``. Plain + ``install`` only adds the integration; selecting it with ``use`` then + registers the enabled extensions for that agent. + """ + project = _init_project(tmp_path, "claude") + + 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 "claude" in registered + assert "codex" not in registered, "precondition: codex not yet installed" + + result = _run_in_project(project, [ + "integration", "install", "codex", + "--script", "sh", + ]) + assert result.exit_code == 0, result.output + + # Install alone does not back-fill the git extension for the secondary + # agent. + registered = json.loads(registry_path.read_text(encoding="utf-8"))[ + "extensions" + ]["git"]["registered_commands"] + assert "claude" in registered, "existing agent registration preserved" + assert "codex" not in registered + assert not ( + project / ".agents" / "skills" / "speckit-git-feature" / "SKILL.md" + ).exists() + + 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"))[ + "extensions" + ]["git"]["registered_commands"] + assert "codex" in registered, "use should register extension commands (#2886)" + assert ( + project / ".agents" / "skills" / "speckit-git-feature" / "SKILL.md" + ).exists() + + def test_install_does_not_register_disabled_extensions(self, tmp_path): + """A disabled extension must not be registered for a newly installed agent.""" + project = _init_project(tmp_path, "claude") + + result = _run_in_project(project, ["extension", "add", "git"]) + assert result.exit_code == 0, f"extension add failed: {result.output}" + result = _run_in_project(project, ["extension", "disable", "git"]) + assert result.exit_code == 0, result.output + + result = _run_in_project(project, [ + "integration", "install", "codex", + "--script", "sh", + ]) + assert result.exit_code == 0, result.output + + registry_path = project / ".specify" / "extensions" / ".registry" + git_meta = json.loads(registry_path.read_text(encoding="utf-8"))[ + "extensions" + ]["git"] + assert git_meta["enabled"] is False + assert "codex" not in git_meta["registered_commands"] + assert not ( + project / ".agents" / "skills" / "speckit-git-feature" / "SKILL.md" + ).exists() + + def test_install_skills_mode_secondary_agent_defers_extension_artifacts(self, tmp_path): + """A non-active skills-mode agent gets extension artifacts only on use. + + Plain ``install`` has no extension side effects. Once the secondary + Copilot ``--skills`` integration is selected with ``use``, it becomes the + active agent and receives extension skills. + """ + project = _init_project(tmp_path, "claude") + + result = _run_in_project(project, ["extension", "add", "git"]) + assert result.exit_code == 0, f"extension add failed: {result.output}" + + # Copilot is not multi_install_safe, so --force is required to add it + # alongside the existing default integration. + result = _run_in_project(project, [ + "integration", "install", "copilot", + "--script", "sh", + "--integration-options", "--skills", + "--force", + ]) + assert result.exit_code == 0, result.output + + # Precondition that makes --skills load-bearing: copilot IS in skills + # mode, so its own core commands are scaffolded as skills. + assert ( + project / ".github" / "skills" / "speckit-specify" / "SKILL.md" + ).exists(), "precondition: copilot installed in skills mode" + + # The git extension is not registered for the non-active copilot agent + # during install. + git_meta = json.loads( + (project / ".specify" / "extensions" / ".registry").read_text(encoding="utf-8") + )["extensions"]["git"] + assert "copilot" not in git_meta["registered_commands"] + assert not ( + project / ".github" / "agents" / "speckit.git.feature.agent.md" + ).exists() + assert not ( + project / ".github" / "skills" / "speckit-git-feature" / "SKILL.md" + ).exists() + + result = _run_in_project(project, ["integration", "use", "copilot"]) + assert result.exit_code == 0, result.output + + git_meta = json.loads( + (project / ".specify" / "extensions" / ".registry").read_text(encoding="utf-8") + )["extensions"]["git"] + # `use` makes copilot active, so extension artifacts follow copilot's + # skills-mode layout. + assert "copilot" not in git_meta["registered_commands"] + assert "speckit-git-feature" in git_meta["registered_skills"] + assert not ( + project / ".github" / "agents" / "speckit.git.feature.agent.md" + ).exists() + assert ( + project / ".github" / "skills" / "speckit-git-feature" / "SKILL.md" + ).exists() + # ── uninstall ──────────────────────────────────────────────────────── @@ -2271,6 +2405,97 @@ def test_upgrade_migrates_opencode_legacy_dir(self, tmp_path): f"found: {[f.name for f in core_remaining]}" ) + def test_upgrade_backfills_extension_commands_for_agent(self, tmp_path): + """Upgrade re-registers enabled extensions for the upgraded agent. + + Regression for #2886: agents installed before extension back-fill + existed (or whose extension artifacts went missing) should regain the + enabled extensions' commands on ``upgrade``, reaching parity with + ``switch``. + """ + import shutil + + project = _init_project(tmp_path, "claude") + + result = _run_in_project(project, ["extension", "add", "git"]) + assert result.exit_code == 0, f"extension add failed: {result.output}" + + result = _run_in_project(project, [ + "integration", "install", "codex", + "--script", "sh", + ]) + assert result.exit_code == 0, result.output + + # Simulate a project created before the install/upgrade back-fill: drop + # codex's extension registration and its rendered artifacts. + registry_path = project / ".specify" / "extensions" / ".registry" + registry = json.loads(registry_path.read_text(encoding="utf-8")) + registry["extensions"]["git"]["registered_commands"].pop("codex", None) + registry_path.write_text(json.dumps(registry), encoding="utf-8") + agents_skills = project / ".agents" / "skills" + for skill_dir in agents_skills.glob("speckit-git-*"): + shutil.rmtree(skill_dir) + + # Precondition: codex is now missing the git extension. + assert "codex" not in json.loads(registry_path.read_text(encoding="utf-8"))[ + "extensions" + ]["git"]["registered_commands"] + assert not (agents_skills / "speckit-git-feature" / "SKILL.md").exists() + + result = _run_in_project(project, [ + "integration", "upgrade", "codex", + "--script", "sh", + ]) + assert result.exit_code == 0, result.output + + # Upgrade back-filled the git extension for codex. + registered = json.loads(registry_path.read_text(encoding="utf-8"))[ + "extensions" + ]["git"]["registered_commands"] + assert "codex" in registered, "upgrade should re-register extension commands (#2886)" + assert (agents_skills / "speckit-git-feature" / "SKILL.md").exists() + + def test_upgrade_non_active_agent_preserves_active_agent_skills(self, tmp_path): + """Upgrading a non-active agent must not touch the active agent's skills. + + Regression for the #2886 wiring: extension skill rendering is + active-agent-scoped, so routing upgrade of a *secondary* agent through + ``register_enabled_extensions_for_agent`` used to re-render the + *active* skills-mode agent's extension skills as a side effect — + resurrecting skill files the user had deliberately deleted. The skills + pass is now gated on the target being the active agent. (Skills parity + for non-active agents is tracked separately in #2948.) + """ + import shutil + + # Active agent: copilot in skills mode → git extension renders as skills. + project = _init_project(tmp_path, "copilot", integration_options="--skills") + result = _run_in_project(project, ["extension", "add", "git"]) + assert result.exit_code == 0, f"extension add failed: {result.output}" + + skill = project / ".github" / "skills" / "speckit-git-feature" / "SKILL.md" + assert skill.exists(), "precondition: active copilot has the git extension skill" + + # Add a secondary (non-active) agent; copilot is not multi_install_safe. + result = _run_in_project(project, [ + "integration", "install", "codex", "--script", "sh", "--force", + ]) + assert result.exit_code == 0, result.output + + # The user deliberately removes the active agent's git skill. + shutil.rmtree(skill.parent) + assert not skill.exists() + + # Upgrading the *non-active* agent must not re-render copilot's skills. + result = _run_in_project(project, [ + "integration", "upgrade", "codex", "--script", "sh", + ]) + assert result.exit_code == 0, result.output + assert not skill.exists(), ( + "upgrading a non-active agent must not resurrect the active agent's " + "deleted extension skill (#2886)" + ) + # ── Full lifecycle ───────────────────────────────────────────────────