fix: register extensions for the active integration only#3459
Open
marcelsafin wants to merge 17 commits into
Open
fix: register extensions for the active integration only#3459marcelsafin wants to merge 17 commits into
marcelsafin wants to merge 17 commits into
Conversation
extension add registered commands for every detected agent, and integration upgrade back-filled enabled extensions for non-active integrations. Maintainer direction on github#2948: treat the project as single-active. Only the active integration gets extension artifacts; use/switch rescaffold the target when the user selects it. - extension add now routes through the all-agents pass restricted to the active integration (only_agent), keeping detection and missing-skills-dir recovery safeguards. Projects without recorded init-options fall back to detection-based registration. - integration upgrade re-registers extensions only when upgrading the active integration, reversing the github#2886 back-fill for non-active targets at maintainer request. Fixes github#2948 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Scopes extension registration to the active integration and defers inactive integrations until use or switch.
Changes:
- Adds active-agent filtering to extension registration.
- Restricts upgrade re-registration to the active integration.
- Adds regression coverage for active-only behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/agents.py |
Adds optional single-agent filtering. |
src/specify_cli/extensions/__init__.py |
Routes extension installation to the active integration. |
src/specify_cli/integrations/_helpers.py |
Updates registration behavior documentation. |
src/specify_cli/integrations/_migrate_commands.py |
Skips extension backfill for inactive upgrades. |
tests/test_extensions.py |
Adapts registrar and naming tests. |
tests/integrations/test_integration_subcommand.py |
Tests active-only add and upgrade behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1007
to
+1014
| if not active_agent or active_agent not in registrar.AGENT_CONFIGS: | ||
| return registrar.register_commands_for_all_agents( | ||
| manifest, | ||
| extension_dir, | ||
| self.project_root, | ||
| link_outputs=link_outputs, | ||
| create_missing_active_skills_dir=True, | ||
| ) |
Comment on lines
+1465
to
+1469
| # Register commands with AI agents (active integration only, #2948) | ||
| registered_commands = {} | ||
| if register_commands: | ||
| registrar = CommandRegistrar() | ||
| # Register for all detected agents | ||
| registered_commands = registrar.register_commands_for_all_agents( | ||
| manifest, | ||
| dest_dir, | ||
| self.project_root, | ||
| link_outputs=link_commands, | ||
| create_missing_active_skills_dir=True, | ||
| registered_commands = self._register_commands_for_active_agent( | ||
| manifest, dest_dir, link_outputs=link_commands |
- 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>
| return | ||
|
|
||
| resolver = PresetResolver(self.project_root) | ||
| for pack_id, metadata in self.registry.list_by_priority(): |
Comment on lines
+1008
to
+1017
| active_agent = init_options.get("ai") | ||
|
|
||
| if not active_agent: | ||
| return registrar.register_commands_for_all_agents( | ||
| manifest, | ||
| extension_dir, | ||
| self.project_root, | ||
| link_outputs=link_outputs, | ||
| create_missing_active_skills_dir=True, | ||
| ) |
Comment on lines
+684
to
+688
| # 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. |
…osed, 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>
| manifest.id, | ||
| preset_dir, | ||
| self.project_root, | ||
| only_agent=active_agent, |
| try: | ||
| updates: Dict[str, Any] = {} | ||
|
|
||
| registered_commands = self._register_commands(manifest, pack_dir) |
Comment on lines
+747
to
+749
| merged_skills = list( | ||
| dict.fromkeys(existing_skills + registered_skills) | ||
| ) |
Comment on lines
+1014
to
+1021
| if "ai" not in init_options: | ||
| return registrar.register_commands_for_all_agents( | ||
| manifest, | ||
| extension_dir, | ||
| self.project_root, | ||
| link_outputs=link_outputs, | ||
| create_missing_active_skills_dir=True, | ||
| ) |
| # 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") |
…ics) Fixes five deeper active-only registration bugs surfaced by Copilot review after 2486c08, all in the presets/extensions single-active integration rule (github#2948): 1. presets: _reconcile_composed_commands (run after install/remove) bypassed the active-only filter entirely, writing composition-winner command files for every detected non-skill agent via register_commands_for_non_skill_agents. Added an only_agent param to that registrar method (mirroring register_commands_for_all_agents) and threaded it through all 5 reconciliation call sites. 2. presets: `integration use copilot` with --skills (ai_skills: true) wrote both the static .agent.md command file AND the SKILL.md mirror for the same override. Mirrored the extension path's ai_skills guard in both _register_commands and the reconciliation pass: a command-backed active agent running in skills mode is excluded from non-skill command registration. 3. presets: registered_skills was a flat list, so switching between two skill-mode agents (e.g. Claude -> Codex) and then removing the preset only restored the currently active agent's directory, permanently orphaning the other. _unregister_skills now restores every existing skill-mode agent directory instead of only the active one. 4. extensions: load_init_options() collapses "no file" and "corrupted file" into the same {}, so the round-2 fail-closed fix didn't actually distinguish them. Added a shared resolve_active_agent_for_registration() helper in _init_options.py that checks file existence separately from parse success, returning a distinct sentinel for "file absent" vs None for "corrupted or invalid". extensions/__init__.py now uses this helper. 5. presets: same corruption-collapsing bug in _register_commands's active_agent resolution. Now uses the same shared helper as (4). Adds regression tests for all five: reconciliation active-only filtering, copilot --skills dual-write prevention, multi-skill-agent switch+remove, and corrupted init-options fail-closed behavior for both extension add and preset add. Each test was verified to fail against the pre-fix code and pass with the fix. Targeted (883) and full (3923 passed, 109 skipped) suites pass; ruff check clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+1558
to
+1564
| skills_dir = _resolve_skills_dir(self.project_root, key) | ||
| if not skills_dir.is_dir(): | ||
| continue | ||
| try: | ||
| resolved = skills_dir.resolve() | ||
| except OSError: | ||
| continue |
Comment on lines
+1588
to
+1589
| for skills_dir, agent_name in self._tracked_skill_agent_dirs(): | ||
| self._unregister_skills_in_dir(skill_names, skills_dir, agent_name) |
Comment on lines
+1553
to
+1556
| if not ( | ||
| isinstance(integration, SkillsIntegration) | ||
| or getattr(integration, "_skills_mode", False) | ||
| ): |
…enance) Replace the "enumerate every skill-mode directory and restore all of them" approach from the previous round with precise per-agent provenance tracking, per reviewer feedback that the enumerate-and-restore-everything design was unsound: - registered_skills changes from a flat List[str] to Dict[str, List[str]] (agent name -> skill names actually written), mirroring the shape registered_commands already uses. _register_skills now returns this per-agent mapping instead of a bare list, and every call site (register_enabled_presets_for_agent, install_from_directory, the _reconcile_skills "was this skill previously managed" check) is updated to read/merge the new shape. Legacy flat-list registry entries from before this change are still readable: writes self-migrate the format, and _normalize_registered_skills() handles the transitional read paths. - _unregister_skills now restores exactly the agent directories recorded for a preset instead of guessing at every skill-mode integration that happens to exist on disk. This fixes two problems with the old enumerate-everything design: (1) it could silently overwrite or delete another preset's (or a user's) override in an agent directory the current preset never actually touched, and (2) it depended on transient per-process integration state (_skills_mode), which is unset in a fresh CLI invocation for mode-selectable integrations like Copilot --skills, permanently orphaning their overrides after a process restart. Registries written before this change (flat list, no agent provenance) fall back to best-effort restoration under only the currently active agent, matching the pre-existing guarantee level. - Every directory resolved from persisted provenance is now validated through the project's shared symlink/containment guard (_ensure_safe_shared_directory) before any file in it is read, written, or removed, since restoration may target an agent that isn't currently active and its directory can't be assumed safe just because a name was recorded for it. - _tracked_skill_agent_dirs() (the enumeration helper introduced last round) is removed; it's superseded by the provenance-based design. Adds regression tests: a symlinked skills directory is rejected during removal; removing one preset does not disturb a different preset's override in another agent's directory; and a Copilot --skills registration installed, then removed after switching agents in a fresh PresetManager instance (simulating a new process), is still correctly restored. Updates existing skill-registration assertions across test_presets.py and test_integration_claude.py for the new per-agent registry shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+72
to
+73
| if not path.exists(): | ||
| return MISSING_INIT_OPTIONS_FILE |
| try: | ||
| updates: Dict[str, Any] = {} | ||
|
|
||
| registered_commands = self._register_commands(manifest, pack_dir) |
Comment on lines
+1621
to
+1628
| if isinstance(registered_skills, dict): | ||
| for agent_name, skill_names in registered_skills.items(): | ||
| if not skill_names: | ||
| continue | ||
| skills_dir = self._safe_skills_dir_for_agent(agent_name) | ||
| if skills_dir is None: | ||
| continue | ||
| self._unregister_skills_in_dir(skill_names, skills_dir, agent_name) |
…fold reconciliation, shared skills dir) - _init_options.py: resolve_active_agent_for_registration() now treats a dangling init-options.json symlink as present (path.is_symlink() check alongside path.exists()), since Path.exists() follows symlinks and returns False for a broken one. Previously a broken symlink fell back to the legacy "no file" path and registered every detected agent instead of failing closed. - presets/__init__.py (register_enabled_presets_for_agent): the integration use/switch rescaffold path now collects affected command names across all presets processed and runs _reconcile_composed_commands/_reconcile_skills once after the loop, matching install/remove. Previously rescaffolding wrote each preset's raw content directly with no follow-up reconciliation, so a project-level override (the highest-priority layer) could be clobbered by a lower-precedence preset after switching agents. - presets/__init__.py (_unregister_skills): multiple integrations can share one physical skills directory (agy/codex/zed all resolve to .agents/skills). Provenance restoration now groups recorded agent entries by resolved directory and restores each physical directory exactly once, preferring the currently active agent's renderer when it owns that directory (otherwise any recorded owner, chosen deterministically). Previously each recorded agent key triggered its own restore pass against the same directory, with whichever agent was iterated last silently winning regardless of which agent was active. Adds regression tests for each: a dangling init-options.json symlink failing closed for both preset resolution and extension add; integration use rescaffold preserving a project override over a lower-priority preset; and a codex/agy shared-directory removal restoring the directory exactly once in the active agent's format. Targeted (tests/integrations/test_integration_subcommand.py, tests/test_presets.py, tests/test_extensions.py, tests/test_extension_skills.py, tests/integrations/test_integration_opencode.py, tests/integrations/test_integration_claude.py): 930 passed. Full suite: 3930 passed, 109 skipped. ruff check: clean on files touched by this change. Refs github#2948 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+718
to
+722
| agent_config | ||
| and is_ai_skills_enabled(init_options) | ||
| and agent_config.get("extension") != "/SKILL.md" | ||
| ): | ||
| return {} |
Comment on lines
+879
to
+883
| resolved_agent = resolve_active_agent_for_registration(self.project_root) | ||
| if resolved_agent is MISSING_INIT_OPTIONS_FILE: | ||
| only_agent: Optional[str] = None | ||
| elif resolved_agent is None: | ||
| only_agent = "" |
Comment on lines
+1612
to
+1615
| _ensure_safe_shared_directory( | ||
| self.project_root, skills_dir, | ||
| create=False, context="preset skills directory", | ||
| ) |
| @@ -1363,36 +1564,163 @@ def _register_skills( | |||
| skill_file.write_text(skill_content, encoding="utf-8") | |||
…conciliation Fix 4 issues from round-6 review of the active-only integration registration work (github#2948): - remove(): removed_cmd_names only collected primary command names from registered_commands + manifest aliases, missing commands that were only ever registered via skills mode (ai_skills guard returns no command names for command-backed integrations in skills mode). This skipped reconciliation entirely when removing a higher-priority skills-mode preset, causing _unregister_skills() to fall back to core/extension content instead of the surviving lower-priority preset's override. Now every command template's primary name is added to removed_cmd_names unconditionally. - _reconcile_composed_commands(): the "composed is None" branch (fires when no replace-strategy layer remains for a command, e.g. after removing a wrap/append preset's base) called unregister_commands() across every configured non-skill agent, ignoring only_agent. This deleted historical artifacts from integrations that were never active for the preset. Now filtered by only_agent like the rest of the file. - Added _validate_skill_subdir() helper (reusing _ensure_safe_shared_directory/_validate_safe_shared_directory from shared_infra.py) and applied it at every site that reads or writes an individual skill subdirectory (_register_skills, _unregister_skills_in_dir, _reconcile_skills' override_skills restoration loop). _safe_skills_dir_for_agent only validated the parent skills directory; a symlinked leaf subdirectory (e.g. .claude/skills/speckit-specify) would slip past that check since is_dir()/exists() follow symlinks, letting write_text/rmtree operate through it to an arbitrary location outside the project. Added regression tests: removing a higher-priority skills-only preset restores the surviving lower-priority preset's content; composed-is-None unregistration only touches the active agent; symlinked skill subdirectory rejected on restore; symlinked skill subdirectory rejected on write. Targeted (934) and full (3934 passed, 109 skipped) test suites and ruff check pass clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+774
to
+777
| if merged_commands != existing_commands: | ||
| updates["registered_commands"] = merged_commands | ||
|
|
||
| registered_skills = self._register_skills(manifest, pack_dir) |
Comment on lines
+4620
to
+4621
| claude_dir = project_dir / ".gemini" / "commands" | ||
| cmd_file = claude_dir / f"{cmd_name}.toml" |
…caffold Fix remaining round-6 review findings on the active-only integration registration work (github#2948): - register_enabled_presets_for_agent(): registered_commands and registered_skills were merged and persisted together in a single registry.update() call after both the commands and skills phases ran. If _register_skills() raised, the per-preset try/except swallowed it before that update() call was reached, even though _register_commands() had already written a real command file to disk. That file became untracked, so preset removal could no longer clean it up. install_from_directory() already persists registered_commands immediately after the commands phase, before starting the independently fallible skills phase; rescaffold now does the same. - test_presets.py: renamed a misleading claude_dir variable (pointing at Gemini's command directory) in test_composed_none_unregister_respects_active_agent to reuse the existing gemini_commands_dir variable already defined earlier in the same test. Added regression test test_rescaffold_persists_commands_before_fallible_skills_phase: simulates a skills-phase failure during rescaffold and asserts the command file already written to disk is still tracked in registered_commands. Verified all other round-6 findings (preset active-integration scoping, preset reconciliation/remove paths, skills-mode switching, override precedence during rescaffold, skill-subdirectory symlink safety) are already addressed by prior commits in this branch; re-checked each against current code before concluding no further change was needed. Targeted (tests/test_presets.py, tests/test_extensions.py: 689 passed) and full (3935 passed, 109 skipped) suites and ruff check on changed files pass clean. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lls toggle Fix an Important gap in register_enabled_presets_for_agent() surfaced by quality review (github#2948): toggling ai_skills for the *same already-active* command-backed agent (e.g. `integration upgrade copilot` after flipping ai_skills, with copilot staying active throughout) left a stale artifact from the previous mode behind, violating the command/skill mutual- exclusion invariant this PR otherwise enforces. - command -> skills: _register_commands()'s ai_skills guard makes the commands phase a no-op, but the previously-written command file (e.g. .agent.md) and its registered_commands[agent] entry were never cleaned up, so it lingered alongside the newly written SKILL.md. - skills -> command: _get_skills_dir() stops resolving a skills directory once ai_skills is off, making the skills phase a no-op, but the previously-written SKILL.md and its registered_skills[agent] entry were never cleaned up, so it lingered alongside the newly (re)written command file. register_enabled_presets_for_agent() now resolves once per call whether agent_name is a command-backed integration (extension != "/SKILL.md") and the current ai_skills state, then narrowly unregisters the stale opposite- mode entry for that agent via the existing _unregister_commands / _unregister_skills helpers before persisting updated tracking — mirroring the same per-agent, per-preset isolation already used elsewhere in this method. Native skill-only agents (claude, codex, ...) are unaffected: they have no command/skill toggle, so registered_commands and registered_skills legitimately co-exist for them by design. The trailing reconciliation pass, project-override precedence, and per-preset partial-failure isolation are all unchanged. Added red-first regression tests exercising the real install + register_enabled_presets_for_agent rescaffold path in both toggle directions: - test_rescaffold_toggle_command_to_skills_removes_stale_command_file - test_rescaffold_toggle_skills_to_command_removes_stale_skill_file Both failed against the prior code (stale artifact persisted / registry still tracked it) and pass after the fix. Targeted (tests/test_presets.py, tests/test_extensions.py: 691 passed) and full (3937 passed, 109 skipped) suites and ruff check on changed files pass clean. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Addressed the remaining review findings in 5b0e559 and b9d9053: preset command tracking is persisted before fallible skill registration, and same-agent command/skills mode toggles now remove stale opposite-mode artifacts and tracking. Full suite: 3937 passed, 109 skipped. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
Comment on lines
+824
to
+825
| if merged_skills != existing_skills: | ||
| self.registry.update(pack_id, {"registered_skills": merged_skills}) |
Comment on lines
+689
to
+695
| # Single-active rule (#2948): preset command overrides register for | ||
| # the active integration only. A project without a recorded active | ||
| # integration (init-options.json does not exist at all — a legacy | ||
| # pre-init-options layout or direct library use) 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. |
…en unchanged Fix a valid finding from GitHub Copilot's review of HEAD b9d9053 (github#2948): register_enabled_presets_for_agent() normalizes a legacy flat-list registered_skills value (predating per-agent provenance) to the {agent_name: [...]} dict shape in memory via _normalize_registered_skills, but the persistence check only compared the two *normalized* forms. When the freshly rescaffolded skill names are identical to what the legacy list already held — the common case, since nothing about the preset or skill actually changed — that comparison is a no-op and registry.update() is skipped, leaving the *raw* on-disk value as the un-migrated flat list. A later switch to a different skill-mode agent and removal then follows _unregister_skills's legacy best-effort path (restore only the currently active agent's directory) instead of the per-agent provenance path, permanently orphaning the first agent's override. Fix: track the raw (pre-normalization) existing value and force persistence whenever it's a non-empty list, independent of whether the normalized content changed. Traced registered_commands for the same class of bug: its registry value has always been Dict[str, List[str]] (no legacy flat-list format ever existed for it — the existing `if not isinstance(existing_commands, dict): existing_commands = {}` guard is not a lossy migration path), so this fix stays scoped to registered_skills only. Added red-first regression test test_rescaffold_migrates_legacy_flat_list_registered_skills: installs a preset, overwrites its registry entry with a raw legacy flat list, rescaffolds the *same* active agent with unchanged skill names, and asserts the raw registry is migrated to per-agent dict form. Extends the scenario with a switch to a second skill-mode agent and preset removal to prove both agents' directories restore cleanly instead of orphaning the first. Failed against the prior code (raw value stayed a list) and passes after the fix. Targeted (tests/test_presets.py, tests/test_extensions.py: 692 passed) and full (3938 passed, 109 skipped) suites and ruff check on changed files pass clean. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Fixed legacy registered_skills migration in 3a1e749 and updated the PR description to disclose preset active-only registration, use/switch rescaffolding, mode toggles, and per-agent provenance. Full suite: 3938 passed, 109 skipped. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
| if merged_commands != existing_commands: | ||
| self.registry.update(pack_id, {"registered_commands": merged_commands}) | ||
|
|
||
| registered_skills = self._register_skills(manifest, pack_dir) |
Comment on lines
+808
to
+811
| raw_existing_skills = metadata.get("registered_skills") | ||
| existing_skills = self._normalize_registered_skills( | ||
| raw_existing_skills, fallback_agent=agent_name | ||
| ) |
Comment on lines
+515
to
+521
| if key == installed_key: | ||
| _register_extensions_for_agent( | ||
| project_root, | ||
| key, | ||
| continuing="The integration was upgraded, but installed extensions may need re-registration.", | ||
| ) | ||
| _register_presets_for_agent( |
…nance, and unregister stale extension artifacts on toggle Three findings from the Copilot review on HEAD b9d9053/3a1e749: 1. `register_enabled_presets_for_agent()` only recorded a preset's command names into `affected_cmd_names` (the set later passed to `_reconcile_composed_commands`/`_reconcile_skills`) in the loop that ran *after* `_register_skills()`, inside the same per-preset `try` block. If `_register_skills` raised, the `except` caught it and `continue`d before that loop ever ran — so a preset whose commands phase already wrote real content to disk never got reconciled against the full priority stack, leaving its raw content in place instead of a project override or higher-precedence preset's content. Fix: record the manifest's command names immediately after the commands phase succeeds and persists, before calling the independently fallible `_register_skills()`. 2. The legacy flat-list `registered_skills` migration (added for the previous review round) attributed every name in the list to whichever agent was currently being (re)activated. If the first operation after upgrading from a pre-github#2948 registry was a direct switch to a *different* skill-mode agent (e.g. a legacy Claude override, then `integration use codex` with no intervening Claude rescaffold), the migrated dict only recorded `{"codex": [...]}`, permanently losing Claude's actual provenance and orphaning its override on later removal. Fix: added `_infer_legacy_skill_provenance()`, which probes every configured skill-mode agent's directory (via the same safe, symlink-validated helpers already used for restore/removal) for a `SKILL.md` whose frontmatter records this exact preset as the owner (`metadata.source == "preset:<pack_id>"`). A name found under more than one directory is attributed to every matching agent (the preset may have been active while the user switched between several skill-mode agents before provenance tracking existed); names that can't be matched to any directory still fall back to the previously-active best-effort behaviour. Directory grouping for shared-path aliases (e.g. agy/codex/zed all resolving to `.agents/skills`) intentionally does not call `.resolve()` on the path, since doing so diverges from `project_root`'s own resolution state on platforms where a path component is itself a symlink (e.g. macOS's `/var` -> `/private/var`) and made every subsequent containment check spuriously fail. 3. `register_enabled_extensions_for_agent()` has the same command/skill mutual-exclusion gap the preset path had (fixed in a previous round): toggling `ai_skills` for the *same active* agent left the opposite mode's artifact behind. Command -> skills left the extension's `.agent.md` file and its `registered_commands[agent]` entry in place once `skills_mode_active` made the commands phase a no-op. Skills -> command left the extension's `SKILL.md` file in place, since an empty `_register_extension_skills()` result (because this agent's skills directory no longer resolves once `ai_skills` is off) was treated as "nothing to register" rather than "this was rendered here before and is now stale". This diverges from the preset path in one respect: `registered_skills` for extensions has always been a flat list with no per-agent provenance (extension skills are only ever rendered for the active agent, never per-preset-per-agent tracked), so the fix resolves ownership by checking which of the extension's tracked skill names still exist as directories under this specific agent's directory before removing them — mirroring the same technique `unregister_agent_artifacts` already uses for full agent deactivation, but scoped narrowly to firing only when a toggle is actually detected (`skills_mode_active` / `command_mode_active`), so a same-mode re-run never disturbs already-correct artifacts or a user's manual customizations. Regression tests (all confirmed red before their respective fix, green after): - tests/test_presets.py::TestPresetSkills::test_rescaffold_reconciles_override_even_when_skills_phase_fails - tests/test_presets.py::TestPresetSkills::test_rescaffold_legacy_flat_list_direct_switch_preserves_original_agent - tests/test_extension_skills.py::TestExtensionSkillRegistration::test_rescaffold_toggle_command_to_skills_removes_stale_extension_command_file - tests/test_extension_skills.py::TestExtensionSkillRegistration::test_rescaffold_toggle_skills_to_command_removes_stale_extension_skill_file Verification: tests/test_presets.py + tests/test_extensions.py + tests/test_extension_skills.py (753 passed), tests/integrations/ (1768 passed, 1 skipped), full suite `pytest tests -q` (3942 passed, 109 skipped), `ruff check` on changed files clean. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
_infer_legacy_skill_provenance() only probed agents whose registrar config statically declares extension == "/SKILL.md", excluding command-backed agents (e.g. Copilot) that can also render preset overrides as SKILL.md files when ai_skills is enabled. A real preset-owned .github/skills/.../SKILL.md written while Copilot was the active skills-mode agent was therefore never probed and got misattributed entirely to whichever agent activated first after the upgrade, permanently orphaning Copilot's override on later removal. Broaden the candidate set to every configured integration (CommandRegistrar.AGENT_CONFIGS), reusing the existing safe-path helper (_safe_skills_dir_for_agent, itself built on the shared _get_skills_dir resolver) rather than inventing new path-construction logic. The existing preset-marker match (metadata.source == "preset:<pack_id>") continues to gate every attribution, so command-mode agents that never rendered this preset's skill are not falsely attributed. Add red-first regression tests: a legacy flat-list entry owned by Copilot in skills mode, switched directly to Claude with no intervening Copilot rescaffold, now migrates to a per-agent dict covering both agents, and removal restores both agents' files instead of orphaning Copilot's override; plus a negative-case test confirming a command-mode Copilot with no preset-owned skill marker is not falsely attributed during the same migration. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Fixed the latest review findings in 1bdb5b3 and ee03a50: reconciliation survives a fallible skills phase, legacy preset skill provenance is inferred across native and command-backed skills agents, and active extension mode toggles remove stale opposite-mode artifacts. Full suite: 3944 passed, 109 skipped. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
Comment on lines
+1931
to
+1949
| existing_skills = self._valid_name_list( | ||
| metadata.get("registered_skills", []) | ||
| ) | ||
| owned_here = [ | ||
| name | ||
| for name in existing_skills | ||
| if (agent_skills_dir / name).is_dir() | ||
| ] | ||
| if owned_here: | ||
| self._unregister_extension_skills( | ||
| owned_here, ext_id, skills_dir=agent_skills_dir | ||
| ) | ||
| remaining = [ | ||
| name | ||
| for name in existing_skills | ||
| if (agent_skills_dir / name).is_dir() | ||
| ] | ||
| if remaining != existing_skills: | ||
| updates["registered_skills"] = remaining |
The skills -> command toggle cleanup in register_enabled_extensions_for_agent() recomputed the remaining tracked registered_skills names by checking only the toggling agent's own skills directory. Since registered_skills is a single flat list shared across every agent an extension has ever been activated under (skills are only ever rendered for the active agent, so there is no per-agent registry key), a name whose mirror still existed under a *different*, previously-active agent's directory was incorrectly dropped from tracking as soon as the current agent's own copy was removed. A later full removal only iterates registered_skills, so the orphaned mirror under the other agent's directory was never found or cleaned up. Add _extension_owned_skill_names(), which re-verifies ownership across every configured agent's skills directory (deduped by shared path) the same way the existing _unregister_extension_skills() fallback scan already does, keeping a name only when a SKILL.md with a matching metadata.source == "extension:<id>" marker is found somewhere - read-only, no directory creation, no symlink escape. Use it instead of re-checking only the toggling agent's own directory when recomputing what remains tracked after narrow stale-mirror cleanup. Add a red-first regression test: Auggie is activated in skills mode first (writing a mirror), then Copilot is activated in skills mode (writing its own mirror for the same names), then Copilot toggles to command mode. Before the fix, registered_skills lost both names entirely even though Auggie's mirrors were untouched on disk; after the fix tracking is preserved and a subsequent full removal correctly cleans up Auggie's remaining mirrors too. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
_extension_owned_skill_names() and the fast/fallback paths of its sibling _unregister_extension_skills() called skills_candidate.resolve() and then checked children relative to that already-resolved candidate. If the candidate directory itself (e.g. .gemini/skills) was a symlink pointing outside the project root, both the resolve() call and the subsequent containment check silently passed through the symlink instead of rejecting it: - _extension_owned_skill_names() would falsely attribute ownership to a marker-matching SKILL.md living outside the project. - _unregister_extension_skills()'s fast path (an explicit skills_dir, as passed by the toggle-cleanup call site) and its fallback scan (used during full extension removal) would both shutil.rmtree() the external directory, deleting unrelated content outside the project. Fix by validating the candidate directory itself with the existing _validate_safe_shared_directory() shared-infra helper before any probe or delete: it rejects a symlink at any path component (walking down from the project root, including the final component) without ever resolving through it, and is already used elsewhere in the codebase for the same class of shared-directory containment check. Unsafe candidates are skipped/refused rather than followed. Add red-first security regression tests reproducing each of the three call sites with a `.gemini/skills` symlink pointing at an external directory containing a marker-matching SKILL.md and an unrelated precious_file.txt: provenance inference must not attribute the name, and both the explicit-skills_dir fast path and the None-skills_dir fallback scan must leave the external directory and file untouched. Existing valid shared/deduped directory tests (e.g. agy/amp/codex/zed sharing .agents/skills) continue to pass, confirming legitimate shared directories still clean up correctly. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Fixed the latest extension-skill tracking and path-safety findings in 37cceb7 and 28561e6: global tracking now preserves mirrors across agent directories, and every extension skill probe/delete path rejects symlinked candidate roots before touching files. Full suite: 3948 passed, 109 skipped. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
Comment on lines
+1271
to
+1275
| # Reject the candidate directory itself (any path component, | ||
| # including the final one) if it's a symlink escaping the | ||
| # project root, before probing or deleting anything inside it. | ||
| # A caller-supplied skills_dir (e.g. a specific agent's | ||
| # directory resolved without side effects) could have been |
Comment on lines
+1914
to
1918
| # Legacy flat-list format: no record of which agent directory these | ||
| # names were written under, so best-effort restore is limited to the | ||
| # currently active agent's directory (the pre-provenance behaviour). | ||
| skills_dir = self._get_skills_dir() | ||
| if not skills_dir: |
…direct remove - _unregister_extension_skills(): omitting skills_dir now always triggers the full multi-directory fallback scan instead of narrowing to the currently active agent's directory. Previously, remove() (the only caller that omits skills_dir) would resolve the active agent's dir and take the scoped fast path, orphaning a previously-active second agent's extension skill mirror during full removal. - PresetManager.remove(): infer legacy flat-list registered_skills provenance (reusing _infer_legacy_skill_provenance from the prior rescaffold fix) before invoking _unregister_skills, so a direct `preset remove` with no intervening rescaffold/switch also restores every previously-active agent's directory instead of only the currently active one. Added regression tests: - test_remove_while_second_agent_still_in_skills_mode_cleans_up_first_agent_mirror - test_remove_infers_legacy_flat_list_provenance_without_prior_rescaffold Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… absent ExtensionManager.unregister_agent_artifacts() converted its resolved agent_skills_dir to None whenever that directory didn't exist, before calling _unregister_extension_skills(). After 1d8f9e3, omitting skills_dir means "genuinely unscoped removal": scan every configured agent's directory, reserved for ExtensionManager.remove()'s full project cleanup. Since unregister_agent_artifacts is agent-scoped (used by switch to clean up the previous integration's artifacts), this caused it to delete every other agent's live extension skill mirrors whenever the target agent's own directory happened to be absent, e.g. unregistering an agent that was never activated. Fix: always pass the explicit, agent-scoped skills_dir, even when it doesn't exist on disk, so the fast path is a safe no-op for an absent directory instead of falling back to the all-agents scan. Registry reconciliation (dropping removed names from the flat registered_skills list) now only runs when the agent's directory actually exists, so an absent directory can't be misread as "these names were removed everywhere" and wipe tracking for mirrors that still legitimately live under other agents' directories. Added regression test: - test_unregister_agent_artifacts_stays_scoped_when_agent_dir_absent Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…facts
The present-directory branch of ExtensionManager.unregister_agent_artifacts()
recomputed "remaining" registered_skills only by checking whether each name
still existed under the just-cleaned agent's own directory. registered_skills
is a single flat list shared across every agent an extension was ever
activated under (skills are only ever rendered for the currently active
agent, so there's no per-agent registry key). Repro: auggie and copilot both
have mirrors for the same extension; unregister_agent_artifacts("auggie")
correctly removes auggie's own mirror, sees the names absent from auggie's
(now empty) directory, and stores an empty registered_skills list - even
though copilot's mirror is still live on disk and now untracked. A later full
remove() then reads an empty registry and leaves copilot's mirror orphaned.
Fix: after the agent-scoped cleanup, recompute remaining names with
_extension_owned_skill_names(), which scans every safe, configured agent
skills directory (not just the one just cleaned) and keeps a name only if a
marker-verified SKILL.md for this extension still exists somewhere. This is
the same helper already used for the analogous same-agent toggle-cleanup
case, so no new abstraction was introduced. Explicit per-agent cleanup,
marker ownership verification, and symlink/containment safety are unchanged.
Added regression test:
- test_unregister_agent_artifacts_preserves_tracking_for_other_agent_mirror
Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Addressed the 2 latest findings plus follow-up cross-agent tracking regressions in 1d8f9e3, 0ab9a5f, and 31c9b97: full removal scans all safe mirrors, direct legacy preset removal infers provenance, and agent-scoped cleanup preserves other agents’ files and global tracking. Full suite: 3952 passed, 109 skipped; changed files pass ruff. Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-5.6 Sol). @copilot review |
Comment on lines
+950
to
+956
| resolved_agent = resolve_active_agent_for_registration(self.project_root) | ||
| if resolved_agent is MISSING_INIT_OPTIONS_FILE: | ||
| only_agent: Optional[str] = None | ||
| elif resolved_agent is None: | ||
| only_agent = "" | ||
| else: | ||
| only_agent = resolved_agent |
Comment on lines
+1906
to
+1911
| for skills_dir, group in groups.items(): | ||
| agents = group["agents"] | ||
| renderer_agent = ( | ||
| active_agent if active_agent in agents else sorted(agents)[0] | ||
| ) | ||
| self._unregister_skills_in_dir(group["names"], skills_dir, renderer_agent) |
Comment on lines
+1280
to
+1283
| try: | ||
| _validate_safe_shared_directory(self.project_root, skills_dir) | ||
| except (ValueError, OSError): | ||
| return |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2948
Root cause
Extension and preset registration paths wrote artifacts for every detected integration, even though projects have one active integration. In multi-integration projects, inactive agents accumulated commands or skills they had not selected, and preset skill tracking lacked per-agent provenance for reliable switching/removal.
Maintainer direction in #2948: treat the project as single-active. Add/install operations register for the active integration only;
integration use/switchand active upgrades are the activation and rescaffold points.Changes
extension addand enabled-preset registration restrict command output to the active integration while preserving existing detection and missing-directory safeguards.integration use/switchand activeintegration upgraderescaffold enabled artifacts for the active integration, then reconcile compositions and project overrides.registered_skillstracking now uses{agent_name: [skill_name, ...]}. Legacy flat lists migrate by inferring existing preset-owned skills from on-disksource: preset:<id>metadata before fallback attribution, so removal restores every directory a preset wrote to.integration upgradeleaves non-active integrations untouched, removing the previous back-fill behavior.Before / after
extension add gitwith claude active and codex installedintegration use codexintegration upgrade codexwhile inactiveTests
Regression coverage includes active-only extension/preset registration, use/switch/active-upgrade rescaffolding, project-override reconciliation after partial failures, command/skills mode toggles, symlink guards, and provenance-safe legacy
registered_skillsmigration.AI assistance
GitHub Copilot was used for implementation, tests, and review-feedback analysis. Agent-authored review-round comments and commits are disclosed separately per repository policy.