fix: resolve __SPECKIT_COMMAND_*__ refs in preset skill rendering (#2717)#2718
Open
huy1010 wants to merge 1 commit into
Open
fix: resolve __SPECKIT_COMMAND_*__ refs in preset skill rendering (#2717)#2718huy1010 wants to merge 1 commit into
huy1010 wants to merge 1 commit into
Conversation
…thub#2717) The preset skill layer mirrors command templates into SKILL.md files but only ran resolve_skill_placeholders(), leaving command cross-references as raw __SPECKIT_COMMAND_<NAME>__ placeholders instead of rendering them as /speckit-<cmd> the way CommandRegistrar.register_commands() does. As a result, presets that override core commands under the agent skill layer (e.g. Claude --ai-skills) leaked the raw tokens into SKILL.md. Add a shared PresetManager._resolve_skill_command_refs() helper that maps the agent's invoke separator to IntegrationBase.resolve_command_refs(), and call it right after resolve_skill_placeholders() in every preset skill-rendering path: _register_skills() (install), the _reconcile_skills() override-restoration block, and both _unregister_skills() restore paths. This mirrors register_commands() and addresses the path divergence flagged in github#1976. Add regression tests covering the install and restore paths. AI assistance: authored with Claude Code (Anthropic) — analysis, patch, and tests. Verified via the existing pytest suite plus a manual CLI install and remove cycle on a Claude --ai-skills project.
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.
Description
Presets that override core commands under the agent skill layer (e.g. Claude with
--ai-skills) were leaking raw__SPECKIT_COMMAND_*__placeholders into the generatedSKILL.mdinstead of rendering them as slash-command invocations like/speckit-plan.Root cause: the preset skill-rendering paths in
src/specify_cli/presets.pyran onlyresolve_skill_placeholders(), whereas the command layer (CommandRegistrar.register_commands()) additionally callsIntegrationBase.resolve_command_refs(). The skill paths never resolved the command-reference tokens —{ARGS}→$ARGUMENTSresolved correctly, so only the command tokens leaked, matching the report.Fixes #2717. This is a concrete instance of the path divergence tracked in #1976.
Change: added a small shared helper
PresetManager._resolve_skill_command_refs()that maps the agent's invoke separator toresolve_command_refs(), and called it right afterresolve_skill_placeholders()in every preset skill-rendering path:_register_skills()— preset install (the path reported in the issue)_reconcile_skills()— override-restoration block_unregister_skills()— both restore paths (core + extension templates)The issue's proposed patch covered only
_register_skills(); I extended the same one-line fix to the sibling restore/reconcile paths because they share the identical omission and would otherwise still leak tokens on preset removal / reconciliation.extensions.pyis intentionally left out of scope (separate concern).Testing
uv run specify --helpuv sync && uv run pytest— 2988 passed, 35 skipped--ai-skills:preset add --dev+preset remove)Manual test results
Agent: Claude Code (CLI) | OS/Shell: macOS / zsh
Repro: scaffold a Claude
--ai-skillsproject, install a local preset overridingspeckit.specifywhose body references__SPECKIT_COMMAND_PLAN__/__SPECKIT_COMMAND_TASKS__, then remove it (restoring from a core template that also carries the tokens). Inspected.claude/skills/speckit-specify/SKILL.md.specify init --integration claude.claude/skills;speckit-specifypresentspecify preset add --dev <preset>SKILL.mdrenders/speckit-plan,/speckit-tasks; 0 raw__SPECKIT_COMMAND_*__(was 2 before the fix)specify preset remove <preset>SKILL.mdrenders/speckit-plan; 0 raw tokens (was 1 before the fix)Test selection reasoning
src/specify_cli/presets.pyspecify preset add/remove, preset skill overridessrc/specify_cli/*.py→ CLI preset commands; preset command overrides mirror into the skill layerRequired tests (prerequisites first):
specify init --integration claude— scaffold the skills layer (prerequisite for T2/T3)specify preset add --dev— install path (_register_skills)specify preset remove— restore path (_unregister_skills); requires T2New automated regression tests added in
tests/test_presets.py:test_register_skills_resolves_command_refs(install) andtest_restore_skill_resolves_command_refs(restore). Both fail onmainand pass with the fix.AI Disclosure
This PR was developed with Claude Code (Anthropic): issue analysis, the patch, and the regression tests were AI-generated. The full
pytestsuite and the manual CLI install/remove cycle reported above were run as part of development, and the change was reviewed (including a follow-up pass to remove a lint-suppression# noqain favor of a top-level import and to make the helper docstring describe behavior) before submission.