fix(codex): inject dot-to-hyphen hook command note in Codex skills#2503
Open
picklebento wants to merge 2 commits into
Open
fix(codex): inject dot-to-hyphen hook command note in Codex skills#2503picklebento wants to merge 2 commits into
picklebento wants to merge 2 commits into
Conversation
Hook commands in `.specify/extensions.yml` use dotted ids like `speckit.git.commit`, but Codex skills are named with hyphens (`speckit-git-commit`). The Claude integration handles this via an explicit instruction injected into each generated SKILL.md by `ClaudeIntegration.post_process_skill_content`, but the Codex integration had no such override, so Codex would emit `/speckit.git.commit` (which does not resolve) instead of `/speckit-git-commit`. This adds the same `_inject_hook_command_note` helper and a `post_process_skill_content` override to `CodexIntegration`, plus a small `setup()` override that applies the post-process to each generated SKILL.md (mirroring the pattern in `ClaudeIntegration`). Also widens the existing `test_non_claude_post_process_is_identity` test to use `agy` (another `SkillsIntegration` with no override), since asserting identity behavior on Codex would now incorrectly fail. Tests: - New `TestCodexHookCommandNote` class mirrors `TestClaudeHookCommandNote`: setup-level injection, no-op when no hook block is present, idempotency, and indentation preservation. - `pytest tests/` → 2866 passed, 34 skipped. Signed-off-by: Chao Zhang <1175468+picklebento@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Codex skills generation so hook command IDs from .specify/extensions.yml (dot notation like speckit.git.commit) are correctly translated into Codex’s hyphenated skill slash-command names (/speckit-git-commit) by injecting an explicit “dot → hyphen” instruction into generated SKILL.md files—matching the existing Claude integration behavior.
Changes:
- Added hook-command normalization note injection +
post_process_skill_content()override toCodexIntegration. - Added a
CodexIntegration.setup()post-pass to rewrite generatedSKILL.mdfiles with the injected note (mirrors Claude’s approach). - Added Codex-focused tests for note injection behavior and updated the Claude test that asserts default
SkillsIntegrationpost-processing is an identity function (now usingagy).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/integrations/test_integration_codex.py | Adds tests validating dot-to-hyphen note injection, idempotency, and indentation preservation for Codex skills. |
| tests/integrations/test_integration_claude.py | Updates the “default post-process is identity” assertion to use agy instead of codex now that Codex post-processes skill content. |
| src/specify_cli/integrations/codex/init.py | Implements hook-note injection and a setup post-processing pass so generated Codex skills include the normalization instruction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
previously approved these changes
May 11, 2026
Comment on lines
+97
to
+101
| return re.sub( | ||
| r"(?m)^(\s*)(- For each executable hook, output the following[^\r\n]*)(\r\n|\n|$)", | ||
| repl, | ||
| content, | ||
| ) |
Author
There was a problem hiding this comment.
Fixed in d1d8ef3 by defaulting eol to \n when the regex captures an empty group via the $ alternative. Added test_hook_note_when_instruction_is_final_line_without_newline covering the case where both indent and eol are empty (the only configuration that would have triggered the concatenation).
🤖 Generated with Claude Code
Collaborator
|
Please address Copilot feedback |
…ewline The hook-note injection regex allowed end-of-string matches via ``$``, which left the captured ``eol`` empty. When the matched indent was also empty, the substitution concatenated the note onto the same line as the instruction. Default ``eol`` to ``\n`` when the capture is empty. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Summary
Hook commands in
.specify/extensions.ymluse dotted ids likespeckit.git.commit, but Codex skills are named with hyphens (speckit-git-commit). The Claude integration handles this with an explicit instruction injected into each generated SKILL.md byClaudeIntegration.post_process_skill_content. The Codex integration had no such override, so Codex would emit/speckit.git.commit(which does not resolve) instead of/speckit-git-commit.This PR mirrors Claude's pattern in
CodexIntegration:_HOOK_COMMAND_NOTEand_inject_hook_command_note(identical regex / idempotency check to Claude's).post_process_skill_contentto apply the note.setup()override that re-reads each generated SKILL.md and appliespost_process_skill_content(Claude already follows this same pattern).Why not lift to
SkillsIntegrationbase?Vibe and Copilot are also affected by this latent bug, so a base-class lift would be a strictly better long-term fix. Keeping this PR focused on Codex per the contributing guideline ("Keep your change as focused as possible"); happy to follow up with a base-class refactor that deduplicates
_inject_hook_command_noteacross Claude / Codex / Vibe / Copilot if maintainers prefer.Test plan
TestCodexHookCommandNoteclass intests/integrations/test_integration_codex.pymirrorsTestClaudeHookCommandNote:test_hook_note_injected_in_skills_with_hooks— fullsetup()produces a SKILL.md containing the note.test_hook_note_not_in_skills_without_hooks— no false positives.test_hook_note_idempotent— re-running injection is a no-op.test_hook_note_preserves_indentation— note matches surrounding indentation.test_non_claude_post_process_is_identity(which asserted oncodex) to useagyinstead, sinceagyis the closest remainingSkillsIntegrationwith nopost_process_skill_contentoverride.uv run pytest tests/→ 2866 passed, 34 skipped, 0 failures locally on Python 3.12.Repro
Before this PR, against any project initialized with
specify init . --integration codex --integration-options="--skills":After this PR:
Discovered while shipping the Codex skills tree at instacart/carrot#787833 (Olive review comment surfaced the missing translation rule).