allow hyphen in command ref token names - #4356
Conversation
the token pattern was matching only A-Z 0-9 and underscore so a command name like speckit.agent-context.update can not be written as a token and the token stays in the output as plain text now the character class allows a hyphen also in both places that resolve the token
There was a problem hiding this comment.
🟡 Changes recommended
The extension authoring guide still explicitly contradicts the newly supported syntax.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| A hyphen belongs to the segment it sits in rather than separating | ||
| segments, so ``__SPECKIT_COMMAND_AGENT-CONTEXT_UPDATE__`` resolves to | ||
| ``/speckit.agent-context.update``. |
the guide still said the token scheme does not carry hyphens, which is the opposite of what this branch does. added the real bundled command as the example since speckit.agent-context.update is the one that was unreachable before this.
|
This is now the canonical implementation for #4198. We selected this PR because it applies the requested option 2 with the smallest backward-compatible code change. Please address the existing focused documentation feedback so the authoring guide reflects the newly supported literal-hyphen syntax; no broader refactor is requested. #4204, #4361, and duplicate issue #4328 are being closed to consolidate the work here. Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol). |
|
documentation feedback is addressed in
that line is gone. the encoding rule now says a hyphen inside a segment is kept, and the table has a hyphenated row using the real bundled command rather than a made up one:
i used the sentence after the table now states the rule both ways, since a hyphen and an underscore doing different jobs in the same token is the part that is easy to get wrong:
markdownlint clean, 0 issues. no code changed in this commit, the regex is still the one character |
There was a problem hiding this comment.
🟡 Changes recommended
The development guide retains a contradictory, obsolete skills-mode limitation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
the callout said a command ref token reaches codex zcode and kimi verbatim. it does not. _resolve_command_ref_tokens inside _register_extension_skills resolves the same token shape against the active skill style. the callout was right that resolve_command_refs is never called there and wrong about what follows from it.
|
addressed in the callout said this:
the first half is still literally true. if is_dollar_skills_agent(selected_ai, ai_skills_enabled):
return "$" + command_name.replace("speckit.", "speckit-").replace(".", "-")
if is_slash_skills_agent(selected_ai, ai_skills_enabled):
return "/" + command_name.replace("speckit.", "speckit-").replace(".", "-")so a token becomes i ran it rather than take my own reading of it: that includes one thing worth flagging since it changes who owns the wording. the callout was already wrong before this branch. the skills side substitution and its the replacement says what both paths do rather than claiming a limitation either way. markdownlint 0 issues. |
There was a problem hiding this comment.
🟡 Changes recommended
The updated guide incorrectly documents Kimi’s final command invocation syntax.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
| > (`CommandRegistrar`). Skill bodies go through `_resolve_command_ref_tokens` in | ||
| > `_register_extension_skills`, which resolves the same token shape against the | ||
| > active skill style: `$speckit-bug-fix` for a dollar-prefixed agent such as | ||
| > Codex or ZCode, `/speckit-bug-fix` for a slash-prefixed one such as Kimi, and |
|
the red on run 33519638199, 6 of 7 jobs green: the cancelled one ran 14:28:48 to 14:43:56 and reports no step conclusions at all, so it never got to a result. macos 3.13 passed and both 3.14 cells on the other two platforms passed, so it is not the python version and not the platform. the workflow conclusion is failure because a job was cancelled, not because anything asserted false. a re-run should clear it, and i cannot trigger one from here.
|
problem
the command ref token can not hold a command name that has a hyphen in it. the pattern is
the bundled command speckit.agent-context.update has a hyphen in the middle segment so there is no way to write a token for it. the text SPECKIT_COMMAND_AGENT-CONTEXT_UPDATE matches nothing and it stays in the generated file as plain text. the agent then reads a raw token instead of a real command.
fix
allow a literal hyphen in the character class in the two places that resolve the token
src/specify_cli/integrations/base.pyinresolve_command_refssrc/specify_cli/extensions/__init__.pyin the extension skills resolverno decode change is needed. the existing replace call already leaves a hyphen untouched. so
AGENT-CONTEXT_UPDATEbecomesagent-context.updatewith the dot separator andagent-context-updatewith the hyphen separator.this is option 2 from the review on #4204 which is the direction that was asked for there.
tests
i added 4 tests
[A-Z]all 4 fail before the change and pass after it.
full suite after the change is 7139 passed and 9 failed. the same 9 fail on main without my change so they are not from this. they are the python parity template tests.
ruff 0.15.0 check src testspasses which is what the workflow runs.ai disclosure
i used an ai assistant to help me read the code and write the tests. i reviewed the change myself and ran the suite locally.
Fixes #4198