fix: register extensions for the active integration only#3459
fix: register extensions for the active integration only#3459marcelsafin wants to merge 51 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>
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.
- 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>
…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>
…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>
…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>
…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>
…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>
…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>
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/specify_cli/extensions/init.py:1352
- Validate the child against
skills_dir, not the broader project/home root. A corrupted registry entry of".."passes the current single-component check, andskills_dir / ".."then validates as being insidetrusted_root; if that parent contains a matchingSKILL.md,shutil.rmtreecan delete the entire agent directory.
_validate_safe_shared_directory(trusted_root, skill_subdir)
src/specify_cli/extensions/init.py:1421
- Keep the fallback child validation rooted at
skills_candidate. Withtrusted_rootset to the project root, the registry name".."passes the one-part guard and permits cleanup of the skills directory's parent, potentially deleting the whole agent directory when itsSKILL.mdmarker matches.
_validate_safe_shared_directory(
trusted_root, skill_subdir
)
src/specify_cli/extensions/init.py:1523
- Scope this provenance probe to
skills_candidate. The current project/home-root validation accepts a persisted skill name of"..", so a marker in the agent directory's parent is incorrectly treated as an extension-owned skill and retained in registry tracking.
_validate_safe_shared_directory(trusted_root, skill_subdir)
…y-extension-registration # Conflicts: # src/specify_cli/integrations/_migrate_commands.py
|
@copilot review |
…ctive-agent layout change with presets Two follow-ups to the upstream-main merge: - Preset reconciliation (_reconcile_composed_commands) now skips extension-scoped commands (speckit.<ext>.<cmd>) whose extension is not installed, at the single chokepoint every install/remove/rescaffold pass funnels through. Registration already refused them, so reconciliation could materialize files no registry entry tracks. The duplicated per-call-site filters collapse into one _extension_installed_for_command helper. - The github#3415 layout-change guard predates this PR's agent-scoped preset rescaffold: for the active integration, _register_presets_for_agent now re-registers enabled presets in the new layout and retires the old layout's stale files, so an active-agent command<->skills toggle proceeds and reconciles instead of being rejected. The guard still rejects non-active agents (no rescaffold runs for them) and still fails closed on an unreadable registry. _installed_presets_affecting_agent also understands the per-agent dict shape of registered_skills this PR writes, instead of raising 'malformed'. Regression tests: rescaffold with an uninstalled extension's command, CLI-level legacy<->skills toggle with an installed preset (both directions), secondary-agent rejection, and dict-shaped registered_skills in the guard helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot review |
The post-upgrade preset rescaffold iterates enabled presets only, and a disabled preset's artifacts are deliberately frozen until removal, so an active-agent command<->skills layout change cannot reconcile them. _installed_presets_affecting_agent now reports each preset's enabled state and the guard rejects the migration while any affected preset is disabled, with re-enable/remove guidance. Enabled presets and non-active rejection behave as before. Regression test: disabled preset blocks the toggle untouched; re-enabling unblocks it and reconciles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/specify_cli/integrations/_migrate_commands.py:129
- This new per-agent branch does not validate the nested value. A corrupted entry such as
{"bob": ""}is treated as having no Bob artifacts, so a layout-changing upgrade proceeds instead of failing closed; with a disabled preset, that can leave its old-layout files and registry inconsistent. Validate every agent key and skill-name list before computinghas_skills, as this helper already promises for malformed registry state.
if isinstance(registered_skills, dict):
# Per-agent provenance ({agent: [skill names]}): only entries for
# *this* agent make the preset affect it.
has_skills = bool(registered_skills.get(agent_key))
Comment-only: spell out why skill deletion is restricted to project-local directories (flat/legacy provenance cannot prove home-directory ownership) instead of an undefined placeholder word. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/specify_cli/extensions/init.py:1366
- A corrupted registry value of
..passes this single-component check. Since the subsequent safety check is rooted at the project/home root rather thanskills_dir,skills_dir / '..'is accepted; if itsSKILL.mdhas the matching marker,shutil.rmtree()can delete the parent agent directory. Reject traversal components before constructing the path.
sn_path = Path(skill_name)
if sn_path.is_absolute() or len(sn_path.parts) != 1:
continue
src/specify_cli/extensions/init.py:1439
- The fallback cleanup has the same traversal gap:
..is a one-part relativePath, and validating againsttrusted_rootpermits it to resolve above the skills directory. A matching marker could therefore make removal recursively delete the agent directory instead of one skill. Apply the same plain-component validation used by preset registry names.
sn_path = Path(skill_name)
if sn_path.is_absolute() or len(sn_path.parts) != 1:
continue
src/specify_cli/extensions/init.py:1543
..also passes this provenance-scan check, allowing the scan to read aSKILL.mdoutside the skills subtree and retain the unsafe registry name for later cleanup. Reject./..and require the parsed component name to equal the original string, as_is_safe_registry_skill_name()does for presets.
sn_path = Path(skill_name)
if sn_path.is_absolute() or len(sn_path.parts) != 1:
continue
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot review |
A dict-shaped registered_skills/registered_commands entry with a non-list value (e.g. null) left ownership undecidable but read as "no artifacts", letting a layout-changing upgrade proceed on a malformed registry. Validate values are lists and raise _PresetRegistryUnreadableError otherwise, matching the guard's fail-closed contract. Unit test covers both fields. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot review |
Unregistering the agent's extension artifacts before re-registration deleted files and registry tracking up front, so a failed or partial re-registration left the extension with no artifacts at all. Retirement of each opposite-mode artifact already belongs to register_enabled_extensions_for_agent's deferred toggle cleanup, which removes an old artifact only after its replacement is confirmed. Also keeps disabled extensions consistent with disabled presets: artifacts stay frozen in place with intact tracking. Regression test corrupts the installed extension manifest so re-registration fails, then asserts the old-layout artifacts and their registry tracking survive the upgrade. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
docs/reference/presets.md:142
- This says commands exist only in the currently active directory, but switching with
useintentionally preserves the previous agent's registration (for example,test_use_registers_presets_for_the_newly_active_agentasserts both Claude and Codex remain tracked). Clarify that new registration/rescaffolding targets only the active integration, while artifacts written during earlier activations may remain until cleanup/removal.
Templates and scripts are looked up from the stack when Spec Kit needs them. Commands use the same stack for replacement and composition, but are materialized into the active integration's directory only, instead of being re-resolved by agents or written to every detected agent directory (#2948). During preset install, Spec Kit registers command files for the preset being installed against the currently active integration; post-install and post-removal reconciliation then recomputes and writes the effective command content for affected command names based on the active stack. Install and rescaffold remain active-only, but removal may also update previously targeted inactive directories recorded by the removed preset to restore the surviving command or skill layer. A non-active installed integration does not otherwise receive these command files until it becomes the default — `specify integration use <key>` (or `switch <key>`) rescaffolds enabled presets for the newly active integration. Agents do not re-resolve the stack each time they run a command.
When Phase 2 of a switch fails, rollback restores another installed integration as the default via _set_default_integration but never re-registered extensions or presets for it. Under active-only registration the fallback may never have received any artifacts (it was installed while another integration was active), and Phase 1 already unregistered the outgoing agent's artifacts — leaving the restored default unusable. Rescaffold both extensions and presets (best-effort) after the fallback default is successfully restored. Regression test: secondary codex install with the git extension, a failing switch to generic, then asserts codex ends up with registered extension artifacts after rollback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot review |
|
@copilot review |
The per-skill _validate_skill_subdir(create=True) loop looks like dead code (its result is unused), but it re-creates the tracked skill subdirectories that _unregister_skills just deleted so _register_skills's only-overwrite-existing gate passes during a historical-directory restore. Removing it fails test_skill_reconciliation_preserves_per_directory_names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot review |
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.