From 082e421e53d551256860eb53c42b288f5813bc40 Mon Sep 17 00:00:00 2001 From: Nimraakram22 Date: Mon, 20 Jul 2026 20:47:40 +0500 Subject: [PATCH 1/3] fix: render extension skills for target agent, not just active agent (#2948) Extension skill rendering was previously scoped only to the active agent (init-options.json's 'ai' field), even when register_enabled_extensions_for_agent was called for a different, non-active agent (e.g. after 'integration install --skills' or 'integration upgrade ' for a secondary integration). This meant a skills-mode agent that wasn't the active one received command files instead of skills. Changes: - Add is_agent_skills_enabled() in _init_options.py: resolves per-agent skills-mode from .specify/integration.json's integration_settings[agent].parsed_options.skills, falling back to the legacy global init-options ai_skills flag for the active agent. - ExtensionManager._get_skills_dir() now accepts an optional agent_name and resolves the skills directory for that specific agent instead of always using the active agent. - ExtensionManager._register_extension_skills() now accepts and threads through agent_name. - register_enabled_extensions_for_agent() now computes skills_mode_active per the target agent_name (not the active agent), and always attempts skill registration for that agent (matching the prior best-effort error handling), while still skipping duplicate command-file registration when the target is in skills mode. Verified manually: 'specify integration upgrade copilot --force' with a non-active, skills-mode Copilot integration now renders .github/skills/speckit-git-*/SKILL.md files instead of command files, with no change to the active agent's own skills. Test suite: 616/616 extension-related tests pass. Full suite has 44 pre-existing failures unrelated to this change (Windows symlink-privilege and PowerShell path-resolution issues, confirmed present on main via git stash). --- src/specify_cli/_init_options.py | 25 ++++ src/specify_cli/extensions/__init__.py | 163 +++++++++++++---------- src/specify_cli/integrations/_helpers.py | 14 +- 3 files changed, 124 insertions(+), 78 deletions(-) diff --git a/src/specify_cli/_init_options.py b/src/specify_cli/_init_options.py index dd225d8251..f541456610 100644 --- a/src/specify_cli/_init_options.py +++ b/src/specify_cli/_init_options.py @@ -34,3 +34,28 @@ def load_init_options(project_path: Path) -> dict[str, Any]: def is_ai_skills_enabled(opts: Mapping[str, Any] | None) -> bool: """Return True only when init options explicitly enable AI skills.""" return isinstance(opts, Mapping) and opts.get("ai_skills") is True + + +def is_agent_skills_enabled(project_path: Path, agent_name: str, opts: Mapping[str, Any] | None) -> bool: + """Return True when *agent_name* should render extension skills. + + Prefers the per-agent ``skills`` flag recorded in + ``.specify/integration.json`` (``integration_settings[agent_name].parsed_options.skills``), + which reflects the ``--skills`` option passed to that specific agent's + install/upgrade/switch. Falls back to the legacy global + ``init-options.json`` ``ai_skills`` flag only when *agent_name* is the + active agent recorded there (pre-multi-install behaviour). + """ + from .integration_state import try_read_integration_json, integration_setting + + state, _error = try_read_integration_json(project_path) + if state: + setting = integration_setting(state, agent_name) + parsed = setting.get("parsed_options") + if isinstance(parsed, Mapping) and "skills" in parsed: + return parsed.get("skills") is True + + if isinstance(opts, Mapping) and opts.get("ai") == agent_name: + return is_ai_skills_enabled(opts) + + return False diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 05aa35f7fb..d99a6e3ce6 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -910,8 +910,15 @@ def _ignore(directory: str, entries: List[str]) -> Set[str]: return _ignore - def _get_skills_dir(self) -> Optional[Path]: - """Return the active skills directory for extension skill registration. + def _get_skills_dir(self, agent_name: Optional[str] = None) -> Optional[Path]: + """Return the skills directory for extension skill registration. + + When *agent_name* is given, resolves the skills directory and + skills-enabled state for that specific agent (using per-agent + settings in ``.specify/integration.json``), so skill rendering + works correctly for non-active agents (#2948). When omitted, + falls back to the previous behaviour of using the active agent + from init-options. Delegates to :func:`resolve_active_skills_dir` which reads init-options, applies the Kimi native-skills fallback, and @@ -926,6 +933,8 @@ def _get_skills_dir(self) -> Optional[Path]: load_init_options, resolve_active_skills_dir, ) + from .._init_options import is_agent_skills_enabled + from .. import _get_skills_dir as resolve_agent_skills_dir def _ensure_usable(skills_dir: Path) -> Optional[Path]: try: @@ -943,26 +952,42 @@ def _ensure_usable(skills_dir: Path) -> Optional[Path]: return None return skills_dir - try: - skills_dir = resolve_active_skills_dir(self.project_root) - except (ValueError, OSError) as exc: - _print_cli_warning( - "resolve", - "skills directory", - None, - exc, - continuing="Continuing without skill registration.", - ) - return None - if skills_dir is None: - return None - opts = load_init_options(self.project_root) if not isinstance(opts, dict): - return _ensure_usable(skills_dir) - selected_ai = opts.get("ai") - if not isinstance(selected_ai, str) or not selected_ai: - return _ensure_usable(skills_dir) + opts = {} + + if agent_name is None: + try: + skills_dir = resolve_active_skills_dir(self.project_root) + except (ValueError, OSError) as exc: + _print_cli_warning( + "resolve", + "skills directory", + None, + exc, + continuing="Continuing without skill registration.", + ) + return None + if skills_dir is None: + return None + selected_ai = opts.get("ai") + if not isinstance(selected_ai, str) or not selected_ai: + return _ensure_usable(skills_dir) + else: + if not is_agent_skills_enabled(self.project_root, agent_name, opts): + return None + try: + skills_dir = resolve_agent_skills_dir(self.project_root, agent_name) + except (ValueError, OSError) as exc: + _print_cli_warning( + "resolve", + "skills directory", + None, + exc, + continuing="Continuing without skill registration.", + ) + return None + selected_ai = agent_name from ..agents import CommandRegistrar @@ -980,6 +1005,7 @@ def _register_extension_skills( manifest: ExtensionManifest, extension_dir: Path, link_outputs: bool = False, + agent_name: Optional[str] = None, ) -> List[str]: """Generate SKILL.md files for extension commands as agent skills. @@ -997,11 +1023,12 @@ def _register_extension_skills( Returns: List of skill names that were created (for registry storage). """ - skills_dir = self._get_skills_dir() + skills_dir = self._get_skills_dir(agent_name) if not skills_dir: return [] from .. import load_init_options + from .._init_options import is_agent_skills_enabled from ..agents import CommandRegistrar from ..integrations import get_integration from ..integrations.base import IntegrationBase @@ -1010,13 +1037,13 @@ def _register_extension_skills( opts = load_init_options(self.project_root) if not isinstance(opts, dict): opts = {} - selected_ai = opts.get("ai") + selected_ai = agent_name if agent_name else opts.get("ai") if not isinstance(selected_ai, str) or not selected_ai: return [] registrar = CommandRegistrar() agent_config = registrar.AGENT_CONFIGS.get(selected_ai, {}) integration = get_integration(selected_ai) - ai_skills_enabled = is_ai_skills_enabled(opts) + ai_skills_enabled = is_agent_skills_enabled(self.project_root, selected_ai, opts) def _resolve_command_ref_tokens(body: str) -> str: """Resolve explicit command-ref tokens with the active skill style.""" @@ -1731,17 +1758,16 @@ 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``. - 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. + Both command-file and skill registration are scoped to the explicit + ``agent_name`` argument, so this method can be used after install, + upgrade, or switch and renders skills correctly for any enabled + skills-mode agent, not just the active one (#2948). """ if not agent_name: return from .. import load_init_options + from .._init_options import is_agent_skills_enabled registrar = CommandRegistrar() agent_config = registrar.AGENT_CONFIGS.get(agent_name) @@ -1750,10 +1776,11 @@ def register_enabled_extensions_for_agent(self, agent_name: str) -> None: init_options = {} active_agent = init_options.get("ai") - ai_skills_enabled = is_ai_skills_enabled(init_options) + ai_skills_enabled = is_agent_skills_enabled( + self.project_root, agent_name, init_options + ) skills_mode_active = ( - active_agent == agent_name - and ai_skills_enabled + ai_skills_enabled and bool(agent_config) and agent_config.get("extension") != "/SKILL.md" ) @@ -1793,46 +1820,42 @@ def register_enabled_extensions_for_agent(self, agent_name: str) -> None: if new_registered != registered_commands: updates["registered_commands"] = new_registered - # 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: - try: - registered_skills = self._register_extension_skills( - manifest, ext_dir + # Extension skills are rendered whenever *agent_name* itself + # has skills mode enabled (checked via per-agent settings in + # `.specify/integration.json`, falling back to the legacy + # global init-options for the active agent). This makes + # skill rendering agent-aware instead of only ever + # targeting the active agent (#2948), while still avoiding + # unrelated side effects on other installed agents. + try: + registered_skills = self._register_extension_skills( + manifest, ext_dir, agent_name=agent_name + ) + except Exception as skills_err: + # Skills are a companion artifact. If command registration + # already succeeded, still persist it so later cleanup can + # find those command files. + from .. import _print_cli_warning + + _print_cli_warning( + "register extension skills for", + "extension", + ext_id, + skills_err, + continuing=( + "Continuing with available registration results for this " + "extension and the remaining extensions." + ), + ) + else: + if registered_skills: + existing_skills = self._valid_name_list( + metadata.get("registered_skills", []) ) - except Exception as skills_err: - # Skills are a companion artifact. If command registration - # already succeeded, still persist it so later cleanup can - # find those command files. - from .. import _print_cli_warning - - _print_cli_warning( - "register extension skills for", - "extension", - ext_id, - skills_err, - continuing=( - "Continuing with available registration results for this " - "extension and the remaining extensions." - ), + merged_skills = list( + dict.fromkeys(existing_skills + registered_skills) ) - else: - 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 + 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 07a62efeed..b79a1710ce 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -402,14 +402,12 @@ def _register_extensions_for_agent( 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. + Extension *skill* rendering is scoped to ``agent_key`` itself (using + per-agent settings in ``.specify/integration.json`` when available, with + a fallback to the legacy global init-options for the active agent), so a + skills-mode agent registered while it is not the active agent (e.g. + Copilot ``--skills`` registered while non-active) still receives skill + files here instead of only command files (#2948). Best-effort: never aborts the surrounding integration operation. Callers invoke it *after* the use/upgrade/switch transaction has committed so a From acab576070a4e121efb5f47b3c35a6514a90d5a0 Mon Sep 17 00:00:00 2001 From: Nimraakram22 Date: Wed, 22 Jul 2026 02:01:54 +0500 Subject: [PATCH 2/3] fix: also render skills for non-active agents via 'extension add' (#2948) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR review feedback: install_from_directory() (the 'extension add' / dev-install path) still called _register_extension_skills() with no agent_name, so it remained scoped to only the active agent, unlike register_enabled_extensions_for_agent() which was already fixed to target a specific agent. Changes: - Add ExtensionManager._register_extension_skills_for_installed_agents(), which renders skills for the active agent (legacy single-agent projects) plus every agent listed in .specify/integration.json's installed_integrations, for whichever of those are individually in skills mode. Failures for one agent are warned and do not block the others. - install_from_directory() now calls this helper instead of a single active-agent-only call. - _get_skills_dir()'s agent_name branch now mirrors resolve_active_skills_dir()'s safety checks (_ensure_safe_shared_directory symlink/containment/is-a-directory validation, plus the Kimi native-skills-dir-must-already-exist fallback) instead of resolving the naive per-agent path unchecked — fixes a regression this exposed in the Hermes marker-file detection test and restores exact parity with the active-agent path. Tests: - Added TestNonActiveAgentSkillRegistration with 3 new tests covering register_enabled_extensions_for_agent for a non-active skills-mode agent, isolation from the active agent's own skills, and extension add rendering skills for all installed skills-mode agents. - pytest tests/ -k extension: 619/619 pass (616 previous + 3 new). - Manually verified: 'specify extension add git' with Claude active and Copilot installed in --skills mode now renders SKILL.md files for both agents ('5 agent skill(s) auto-registered'), with no change to Claude's own skills. - git diff --check: clean, no whitespace errors. --- src/specify_cli/extensions/__init__.py | 96 ++++++++++++++++++- tests/test_extension_skills.py | 128 +++++++++++++++++++++++++ 2 files changed, 220 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index d99a6e3ce6..a2a2f6ebf4 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -974,10 +974,29 @@ def _ensure_usable(skills_dir: Path) -> Optional[Path]: if not isinstance(selected_ai, str) or not selected_ai: return _ensure_usable(skills_dir) else: - if not is_agent_skills_enabled(self.project_root, agent_name, opts): + from ..shared_infra import _ensure_safe_shared_directory + + ai_skills_enabled = is_agent_skills_enabled( + self.project_root, agent_name, opts + ) + if not ai_skills_enabled and agent_name != "kimi": return None try: skills_dir = resolve_agent_skills_dir(self.project_root, agent_name) + if not ai_skills_enabled: + # Kimi native-skills fallback: only use the directory if + # it already exists; do not create it on demand. + if not skills_dir.is_dir(): + return None + _ensure_safe_shared_directory( + self.project_root, skills_dir, + create=False, context="agent skills directory", + ) + else: + _ensure_safe_shared_directory( + self.project_root, skills_dir, + context="agent skills directory", + ) except (ValueError, OSError) as exc: _print_cli_warning( "resolve", @@ -1191,6 +1210,73 @@ def _replacement(match: re.Match[str]) -> str: return written + def _register_extension_skills_for_installed_agents( + self, + manifest: ExtensionManifest, + extension_dir: Path, + link_outputs: bool = False, + ) -> List[str]: + """Render extension skills for every skills-mode agent detected. + + Used by paths (like ``extension add``) that register an extension + for all agents at once, rather than a single explicit target agent. + Checks the active agent (legacy single-agent projects) plus every + agent recorded in ``.specify/integration.json``'s installed + integrations, and renders skills for whichever of those have skills + mode enabled for them specifically, instead of only the active + agent (#2948). + """ + from .. import load_init_options + from ..integration_state import ( + try_read_integration_json, + installed_integration_keys, + ) + + opts = load_init_options(self.project_root) + if not isinstance(opts, dict): + opts = {} + + candidate_agents: List[str] = [] + active_agent = opts.get("ai") + if isinstance(active_agent, str) and active_agent: + candidate_agents.append(active_agent) + + state, _error = try_read_integration_json(self.project_root) + if state: + for key in installed_integration_keys(state): + if key not in candidate_agents: + candidate_agents.append(key) + + combined: List[str] = [] + seen = set() + for agent_name in candidate_agents: + try: + agent_skills = self._register_extension_skills( + manifest, + extension_dir, + link_outputs=link_outputs, + agent_name=agent_name, + ) + except Exception as skills_err: + from .. import _print_cli_warning + _print_cli_warning( + "register extension skills for", + "extension", + manifest.id, + skills_err, + continuing=( + "Continuing with available registration results for " + "this extension and the remaining agents." + ), + ) + continue + for skill_name in agent_skills: + if skill_name not in seen: + seen.add(skill_name) + combined.append(skill_name) + + return combined + @staticmethod def _is_expected_dev_symlink(skill_file: Path, cache_file: Path) -> bool: """Return True when an existing skill file links to its dev cache.""" @@ -1475,9 +1561,11 @@ def install_from_directory( create_missing_active_skills_dir=True, ) - # Auto-register extension commands as agent skills when skills mode - # was used during project initialisation (feature parity). - registered_skills = self._register_extension_skills( + # Auto-register extension commands as agent skills for every + # skills-mode agent detected (active agent plus any other + # installed integrations in skills mode), not just the active + # agent (#2948). + registered_skills = self._register_extension_skills_for_installed_agents( manifest, dest_dir, link_outputs=link_commands ) diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index dea42a3852..35c3c62b33 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -219,6 +219,40 @@ def no_skills_project(project_dir): return project_dir +def _create_integration_json( + project_root: Path, + *, + default_agent: str, + installed: list, + skills_by_agent: dict, +): + """Write a .specify/integration.json with per-agent skills settings. + + ``skills_by_agent`` maps agent key -> bool for + ``integration_settings[agent].parsed_options.skills``. + """ + specify_dir = project_root / ".specify" + specify_dir.mkdir(parents=True, exist_ok=True) + settings = {} + for agent in installed: + entry = {"script": "sh", "invoke_separator": "-"} + if agent in skills_by_agent: + entry["raw_options"] = "--skills" if skills_by_agent[agent] else "" + entry["parsed_options"] = {"skills": skills_by_agent[agent]} + settings[agent] = entry + payload = { + "version": "0.1.0", + "integration_state_schema": 1, + "installed_integrations": installed, + "integration_settings": settings, + "integration": default_agent, + "default_integration": default_agent, + } + (specify_dir / "integration.json").write_text( + json.dumps(payload), encoding="utf-8" + ) + + # ===== ExtensionManager._get_skills_dir Tests ===== class TestExtensionManagerGetSkillsDir: @@ -1906,3 +1940,97 @@ def test_remove_cleans_up_when_ai_skills_toggled(self, skills_project, extension assert result is True assert not (skills_dir / "speckit-test-ext-hello").exists() assert not (skills_dir / "speckit-test-ext-world").exists() + + +# ===== Per-agent (non-active) skills mode tests (#2948) ===== +class TestNonActiveAgentSkillRegistration: + """Skills should render for any skills-mode agent, not just the active one.""" + + def test_register_enabled_extensions_for_agent_renders_non_active_agent_skills( + self, project_dir, extension_dir + ): + """upgrade/install-style registration should target the given agent.""" + _create_init_options(project_dir, ai="claude", ai_skills=False) + _create_integration_json( + project_dir, + default_agent="claude", + installed=["claude", "copilot"], + skills_by_agent={"copilot": True}, + ) + copilot_skills_dir = _create_skills_dir(project_dir, ai="copilot") + + manager = ExtensionManager(project_dir) + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + manifest = manager.get_extension("test-ext") + + manager.register_enabled_extensions_for_agent("copilot") + + metadata = manager.registry.get(manifest.id) + assert "speckit-test-ext-hello" in metadata["registered_skills"] + assert ( + copilot_skills_dir / "speckit-test-ext-hello" / "SKILL.md" + ).exists() + + def test_register_enabled_extensions_for_agent_does_not_affect_active_agent( + self, project_dir, extension_dir + ): + """Rendering skills for a non-active agent must not touch the active agent's own skills.""" + _create_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = _create_skills_dir(project_dir, ai="claude") + _create_integration_json( + project_dir, + default_agent="claude", + installed=["claude", "copilot"], + skills_by_agent={"copilot": True}, + ) + copilot_skills_dir = _create_skills_dir(project_dir, ai="copilot") + + manager = ExtensionManager(project_dir) + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Simulate the user deleting the claude skill files before re-running + # registration for a different (copilot) agent. + claude_skill_dir = claude_skills_dir / "speckit-test-ext-hello" + shutil.rmtree(claude_skill_dir) + assert not claude_skill_dir.exists() + + manager.register_enabled_extensions_for_agent("copilot") + + # Copilot's skills were rendered... + assert ( + copilot_skills_dir / "speckit-test-ext-hello" / "SKILL.md" + ).exists() + # ...but claude's deleted skill was not resurrected. + assert not claude_skill_dir.exists() + + def test_extension_add_renders_skills_for_all_installed_skills_mode_agents( + self, project_dir, extension_dir + ): + """extension add should render skills for every installed skills-mode agent.""" + _create_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = _create_skills_dir(project_dir, ai="claude") + _create_integration_json( + project_dir, + default_agent="claude", + installed=["claude", "copilot"], + skills_by_agent={"copilot": True}, + ) + copilot_skills_dir = _create_skills_dir(project_dir, ai="copilot") + + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + metadata = manager.registry.get(manifest.id) + assert "speckit-test-ext-hello" in metadata["registered_skills"] + assert ( + claude_skills_dir / "speckit-test-ext-hello" / "SKILL.md" + ).exists() + assert ( + copilot_skills_dir / "speckit-test-ext-hello" / "SKILL.md" + ).exists() From d7a81faaa9050f9dca50f7672b8eab9644241192 Mon Sep 17 00:00:00 2001 From: Nimraakram22 Date: Thu, 23 Jul 2026 03:06:50 +0500 Subject: [PATCH 3/3] fix: clean up extension skills across all agents on remove (#2948) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot review feedback: remove() called _unregister_extension_skills() without a directory, so its fast path removed skill files only from the resolved active-agent directory whenever one resolved, and never scanned the directories of other agents whose skills were rendered via the new multi-agent registration paths added earlier in this PR — leaving those orphaned on the filesystem after removal. _unregister_extension_skills() now always scans every known agent skills directory (previously this broader scan only ran as a fallback when no active-agent directory could be resolved at all), in addition to any explicitly-passed or resolved directory. Each candidate directory's matching skill subdirectories are still verified against the SKILL.md metadata.source field before removal, so this doesn't touch the existing safety guarantees or the registered_skills schema (intentionally kept as a flat, deduped list rather than a per-agent dict, to avoid a breaking registry-schema change for this fix). Tests: - Added test_remove_cleans_up_skills_for_non_active_agent, covering removal after skills were rendered for both an active and a non-active agent. - pytest tests/ -k extension: 620/620 pass (619 previous + 1 new). - git diff --check: clean. Note: a related Copilot suggestion to also have plain 'integration install' immediately register extension commands/skills for the newly-installed agent was evaluated but not applied here, since it directly conflicts with the maintainer-requested, explicitly-tested deferred-registration design from #2886 (see tests/integrations/test_integration_subcommand.py::TestIntegrationInstall::test_install_defers_extension_commands_until_use). Flagging this for maintainer input rather than silently reversing that prior decision. --- src/specify_cli/extensions/__init__.py | 27 ++++++++++-------- tests/test_extension_skills.py | 39 ++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index a2a2f6ebf4..32526bd72c 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1299,19 +1299,19 @@ def _unregister_extension_skills( Called during extension removal to clean up skill files that were created by ``_register_extension_skills()``. - If *skills_dir* is not provided and ``_get_skills_dir()`` returns - ``None`` (e.g. the user removed init-options.json or toggled - ai_skills after installation), we fall back to scanning all known - agent skills directories so that orphaned skill directories are - still cleaned up. In that case each candidate directory is - verified against the SKILL.md ``metadata.source`` field before - removal to avoid accidentally deleting user-created skills with - the same name. + Skills may have been rendered for more than one agent (#2948), + so this always scans every known agent skills directory (plus + *skills_dir* or the resolved active-agent directory, if any) + rather than only a single resolved directory, to avoid orphaning + skills registered for a non-active agent. Each candidate + directory is verified against the SKILL.md ``metadata.source`` + field before removal to avoid accidentally deleting user-created + skills with the same name. Args: skill_names: List of skill names to remove. extension_id: Extension ID used to verify ownership during - fallback candidate scanning. + candidate scanning. skills_dir: Optional explicit skills directory to use instead of resolving via ``_get_skills_dir()``. Useful when the caller needs to target a specific agent's skills directory @@ -1365,8 +1365,13 @@ def _unregister_extension_skills( except (OSError, UnicodeDecodeError, Exception): continue shutil.rmtree(skill_subdir) - else: - # Fallback: scan all possible agent skills directories + # Additionally always scan every other known agent skills + # directory: skills may have been rendered for more than one + # agent (#2948), so limiting cleanup to only the directory above + # would orphan skills registered for a non-active agent. + # Directories already handled above are silently skipped since + # their matching skill subdirectories no longer exist. + if True: from .. import AGENT_CONFIG, DEFAULT_SKILLS_DIR candidate_dirs: set[Path] = set() diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index 35c3c62b33..51d165729d 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -2034,3 +2034,42 @@ def test_extension_add_renders_skills_for_all_installed_skills_mode_agents( assert ( copilot_skills_dir / "speckit-test-ext-hello" / "SKILL.md" ).exists() + + def test_remove_cleans_up_skills_for_non_active_agent( + self, project_dir, extension_dir + ): + """remove() must clean up skills rendered for a non-active agent too (#2948).""" + _create_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = _create_skills_dir(project_dir, ai="claude") + _create_integration_json( + project_dir, + default_agent="claude", + installed=["claude", "copilot"], + skills_by_agent={"copilot": True}, + ) + copilot_skills_dir = _create_skills_dir(project_dir, ai="copilot") + + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Precondition: both agents got the skill rendered. + assert ( + claude_skills_dir / "speckit-test-ext-hello" / "SKILL.md" + ).exists() + assert ( + copilot_skills_dir / "speckit-test-ext-hello" / "SKILL.md" + ).exists() + + result = manager.remove(manifest.id, keep_config=False) + assert result is True + + # Removal must clean up both agents' skill directories, not just + # the active one. + assert not ( + claude_skills_dir / "speckit-test-ext-hello" + ).exists() + assert not ( + copilot_skills_dir / "speckit-test-ext-hello" + ).exists()