fix: resolve skill placeholders for all SKILL.md agents, not just codex/kimi#2313
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes placeholder resolution in generated SKILL.md files so that script/agent tokens are resolved for all skills-based agents (not just codex and kimi), and adds coverage to prevent regressions.
Changes:
- Update SKILL placeholder resolution gating to use the agent registry (
AGENT_CONFIGS) rather than a hardcoded agent-name set. - Add a parametrized test to validate placeholder resolution across all current SKILL.md agents.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/specify_cli/agents.py |
Ensures SKILL placeholder resolution runs for any agent configured to emit /SKILL.md outputs. |
tests/test_extensions.py |
Adds parametrized coverage asserting {SCRIPT}, {ARGS}, and __AGENT__ are fully resolved for all SKILL agents. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
left a comment
There was a problem hiding this comment.
Nice fix — the registry-based check is consistent with how presets.py already identifies skill agents.
One small follow-up: there's a NATIVE_SKILLS_AGENTS = {"codex", "kimi"} constant in init.py (around line 923) that's the same hardcoded set you're removing here. It's defined but never referenced anywhere. Could you remove it in this PR as well to avoid leaving a stale breadcrumb?
|
@mnriem done 👍 |
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
|
Thank you! |
Description
When using Claude Code, adding a spec-kit extension produced a
SKILL.mdwith unresolved placeholders: raw{SCRIPT}and__AGENT__tokens instead of the actual script path and agent name. With Codex it worked correctly, so the bug wasn't obvious until I tested with a different agent.The root cause was a hardcoded check that only allowed
codexandkimito go through placeholder resolution. Any agent added after that was silently skipped.The hardcoded set was replaced with a registry check so all current and future SKILL.md agents are covered automatically.
A parametrized test covering all six skill agents has been added as well.
Testing
uv run specify --helpuv sync && uv run pytestTest selection reasoning
src/specify_cli/agents.pyspecify extension add(skill agents)render_skill_command()runs when an extension is added to a SKILL.md agent — the fix must be validated end-to-end by adding an extension and inspecting the written SKILL.mdsrc/specify_cli/agents.pyspecify init(skill agents)tests/test_extensions.pyRequired tests
specify initwith a skill agent (e.g.,--ai codex) — prerequisite; creates the project and installs the integration before extension commands can be testedspecify extension add <ext> --devon the T1 project using each skill agent type — verifies that the SKILL.md written by the fixedrender_skill_command()contains resolved script paths and agent names with no raw{SCRIPT}or__AGENT__tokens remainingManual test results
Agent: Codex CLI + Claude Code (both tested) | OS/Shell: macOS/zsh
specify initwith a skill agent (--ai codex,--ai claude)specify extension add <ext> --devon each skill agent typeAI Disclosure
I used Copilot to understand the project structure and code intention, but the implementation changes are my own. The test result table and test selection analysis in this PR were also generated with Copilot (as requested in CONTRIBUTING.md).