fix: render extension skills for target agent, not just active agent …#3599
fix: render extension skills for target agent, not just active agent …#3599Nimraakram22 wants to merge 2 commits into
Conversation
…ithub#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 <agent> --skills' or 'integration upgrade <agent>' 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).
There was a problem hiding this comment.
Pull request overview
Makes extension skill rendering aware of the target integration rather than only the active agent.
Changes:
- Resolves per-agent skills mode from integration state.
- Threads target agent through skill-directory resolution and rendering.
- Updates extension re-registration behavior and documentation.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/_init_options.py |
Adds per-agent skills-mode resolution. |
src/specify_cli/extensions/__init__.py |
Targets extension skill generation by agent. |
src/specify_cli/integrations/_helpers.py |
Updates registration behavior documentation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
src/specify_cli/extensions/init.py:980
- The target-agent branch bypasses the repository's safe directory creation.
resolve_agent_skills_dironly constructs a path, and_ensure_usablefollows symlinked parents withmkdir;_register_extension_skillsthen writesSKILL.mdbeneath it. A project with (for example) a symlinked.github/skillscan therefore make registration write outside the project. Route this path through_ensure_safe_shared_directoryasresolve_active_skills_dirdoes (seeshared_infra.py:163-196) before writing.
skills_dir = resolve_agent_skills_dir(self.project_root, agent_name)
src/specify_cli/extensions/init.py:1856
registered_skillsis still a single global list of names, so merging a second agent's identically named skills loses which directories own them. For example, after Claude and non-active Copilot both renderspeckit-git-feature, uninstalling either agent removes its copy and clears the list inunregister_agent_artifacts; the other copy remains but is no longer tracked, so later extension removal leaves it orphaned. Track generated skills per agent (with legacy-list compatibility) before enabling multi-agent skill rendering.
existing_skills = self._valid_name_list(
metadata.get("registered_skills", [])
)
merged_skills = list(
dict.fromkeys(existing_skills + registered_skills)
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Medium
| works correctly for non-active agents (#2948). When omitted, | ||
| falls back to the previous behaviour of using the active agent | ||
| from init-options. |
| 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: |
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback and address test & lint errors
…thub#2948) 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.
|
@mnriem Thanks for the review! Pushed a follow-up commit addressing both points:
Also had to fix a regression this exposed: my agent-specific
|
Fixes #2948.
Problem
Extension skill rendering was scoped to the active agent (init-options.json's
aifield) even when registering extensions for a different, non-active agent — e.g. afterspecify integration install copilot --integration-options "--skills"while Claude remained active, orspecify integration upgrade copilot. The target agent received command files instead of SKILL.md files.Root cause
ExtensionManager._get_skills_dir()and_register_extension_skills()always resolved the skills directory/settings from the active agent ininit-options.json, ignoring theagent_nameargument passed toregister_enabled_extensions_for_agent().Fix
is_agent_skills_enabled()to resolve per-agent skills-mode from.specify/integration.json'sintegration_settings[agent].parsed_options.skills, falling back to the legacy globalai_skillsflag for the active agent.agent_namethrough_get_skills_dir()and_register_extension_skills().register_enabled_extensions_for_agent()now computesskills_mode_activeper the target agent, and always attempts skill registration for that agent (matching prior best-effort error handling).Testing
specify integration upgrade copilot --forcewith a non-active, skills-mode Copilot integration now renders.github/skills/speckit-git-*/SKILL.mdfiles instead of command files, with no regression to the active agent's own skills.pytest tests/ -k extension: 616/616 pass.mainwithout this change).