fix(codex): prefer skills over deprecated prompts#1143
Open
SIGMAREAL wants to merge 4 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes per-tool delivery decisions, migrates Codex to skills-only artifacts, adds OPSX→Codex skill mapping and transformers, and applies per-tool generation and cleanup across init, update, workspace skills, profile-sync drift, legacy-cleanup, tests, and docs. ChangesCodex Skill Migration and Per-Tool Delivery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
README.md (1)
152-152: ⚡ Quick winClarify tool-dependent refresh behavior.
This wording implies all tools get both artifacts; Codex is skills-only now. Consider “latest skills and/or slash commands (tool-dependent)” to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 152, Update the sentence that reads "Run this inside each project to regenerate AI guidance and ensure the latest skills and slash commands are active:" to clarify tool-dependent behavior by changing it to something like "Run this inside each project to regenerate AI guidance and ensure the latest skills and/or slash commands (tool-dependent) are active:" so readers understand Codex may only update skills while other tools may update slash commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/init.ts`:
- Around line 714-719: The current boolean gate using hasCommandSurface causes
only /opsx:* guidance to be printed even when Codex-style tools are present;
update the logic in the getting-started output (use the existing
gettingStartedTools and shouldGenerateCommandsForTool) to compute two
flags—hasCommandSurface (tools where shouldGenerateCommandsForTool(...) is true)
and hasCodexSurface (tools where it is false or explicitly Codex-capable)—and
when both are true print both invocation styles (e.g., show both the /opsx:* and
the $openspec-* variants) instead of choosing one; adjust the variables/strings
currently named commandPrefix and newInvocation (and the similar block at
722-725) to emit dual guidance when mixed surfaces are detected.
In `@src/core/update.ts`:
- Around line 194-195: The summary message currently uses the global delivery
value when reporting Codex command file removals even though per-tool predicates
(shouldGenerateSkillsForTool and shouldGenerateCommandsForTool) decide removal;
update the reporting logic that builds the removal summary (the code that reads
toolShouldGenerateSkills and toolShouldGenerateCommands) so it does not
hard-code "(delivery: skills)" from the global delivery variable — instead
compute and include the actual per-tool reason (e.g., "removed: per-tool
override (commands disabled)" or a neutral "removed: per-tool setting") or
attach the specific predicate result for each tool; apply the same change to the
other occurrences that use toolShouldGenerateSkills/toolShouldGenerateCommands
(including the blocks referenced around the other occurrence at the same file).
- Around line 687-703: The shared post-upgrade onboarding text still emits the
legacy opsx prompts for all tools; update the onboarding generation to be
tool-aware by branching on the tool identifier/value (use the same key used by
shouldGenerateSkillsForTool/shouldGenerateCommandsForTool and tool.value) and
emit the correct next-step instructions for Codex vs other tools. Locate the
shared onboarding block that currently hardcodes the `/opsx:new`,
`/opsx:continue`, `/opsx:apply` prompts and replace it with logic that selects
the appropriate command set based on toolId/tool.value (or a helper like
getSkillInstructionTransformer), so upgraded users receive the new Codex flow
when tool.value indicates Codex and the legacy opsx prompts for other tools.
Ensure the conditional lives alongside the existing generation calls (e.g.,
where you call generateSkillContent/getSkillInstructionTransformer and write
files with FileSystemUtils.writeFile) so the produced onboarding SKILL.md
reflects the tool-aware instructions.
---
Nitpick comments:
In `@README.md`:
- Line 152: Update the sentence that reads "Run this inside each project to
regenerate AI guidance and ensure the latest skills and slash commands are
active:" to clarify tool-dependent behavior by changing it to something like
"Run this inside each project to regenerate AI guidance and ensure the latest
skills and/or slash commands (tool-dependent) are active:" so readers understand
Codex may only update skills while other tools may update slash commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e5d782d-9774-4e8c-84c0-9d35dfa07ba2
📒 Files selected for processing (14)
README.mddocs/supported-tools.mdsrc/core/command-generation/adapters/codex.tssrc/core/init.tssrc/core/profile-sync-drift.tssrc/core/tool-delivery.tssrc/core/update.tssrc/core/workspace/skills.tssrc/utils/command-references.tssrc/utils/index.tstest/core/init.test.tstest/core/profile-sync-drift.test.tstest/core/update.test.tstest/utils/command-references.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
150-156:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the Codex invocation here as well.
This now says the active surface is tool-dependent, but the same README still teaches
/opsx:*as the default quick-start path. That leaves Codex users with contradictory guidance. Please add a matching$openspec-*example here or explicitly scope the slash-command examples to non-Codex tools.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 150 - 156, Clarify the README's "Refresh agent instructions" so Codex users aren't left with contradictory guidance: update the paragraph that currently shows "openspec update" and references "/opsx:*" to either (a) add a matching Codex invocation example (use the $openspec-* form as a concrete example) or (b) explicitly state that slash-command examples (e.g., /opsx:*) apply only to non-Codex tools and show the equivalent Codex command; edit the sentence and the example block that contains "openspec update" and the referenced "/opsx:*" to include the $openspec-* example or an explicit scoping note so readers can clearly see the correct command for Codex vs other tools.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/core/update.test.ts`:
- Around line 1199-1205: The test "should show Codex skill invocations when
upgrading legacy Codex artifacts" currently seeds a project-local
.codex/prompts/openspec-new.md but the migration looks for the managed global
prompt; create the real legacy artifact under CODEX_HOME by setting up
CODEX_HOME/prompts and writing an opsx-*.md file (e.g. opsx-openspec-new.md)
with the same 'old codex prompt' content before exercising update. Use the same
helpers (fs.mkdir and fs.writeFile) but target path.join(process.env.CODEX_HOME
|| testDir, 'prompts', 'opsx-openspec-new.md') (or set process.env.CODEX_HOME =
testDir) so the upgrade logic that expects CODEX_HOME/prompts/opsx-*.md is
exercised.
---
Outside diff comments:
In `@README.md`:
- Around line 150-156: Clarify the README's "Refresh agent instructions" so
Codex users aren't left with contradictory guidance: update the paragraph that
currently shows "openspec update" and references "/opsx:*" to either (a) add a
matching Codex invocation example (use the $openspec-* form as a concrete
example) or (b) explicitly state that slash-command examples (e.g., /opsx:*)
apply only to non-Codex tools and show the equivalent Codex command; edit the
sentence and the example block that contains "openspec update" and the
referenced "/opsx:*" to include the $openspec-* example or an explicit scoping
note so readers can clearly see the correct command for Codex vs other tools.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3060f94-a8ee-4d4b-b16c-41bd75866bbc
📒 Files selected for processing (5)
README.mdsrc/core/init.tssrc/core/update.tstest/core/init.test.tstest/core/update.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/update.ts
SIGMAREAL
force-pushed
the
fix/codex-skills-first
branch
from
70a358b to
8227157
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/supported-tools.md (1)
32-32: ⚡ Quick winConsider adding explicit migration guidance for existing Codex users.
The deprecation note is clear, but existing users with Codex prompt files may not realize they should run
openspec updateto remove deprecated prompts and regenerate skills. Consider adding a brief note like:Not generated (Codex custom prompts are deprecated; existing users should run
openspec updateto clean up old prompts and use$openspec-*skills)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/supported-tools.md` at line 32, Update the Codex table entry to include explicit migration guidance: amend the note for "Codex (`codex`) | `.codex/skills/openspec-*/SKILL.md`" to state that Codex custom prompts are deprecated and instruct existing users to run `openspec update` to remove deprecated prompts and regenerate skills (e.g., "Not generated (Codex custom prompts are deprecated; existing users should run `openspec update` to clean up old prompts and use `$openspec-*` skills)").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/command-references.ts`:
- Around line 42-44: The regex in the replacement (the line returning
text.replace(...)) can partially match identifiers like "/opsx:apply2" and
replace only the alphabetic prefix; update the pattern to enforce a trailing
token boundary so the whole identifier must match (for example add a negative
lookahead such as (?![a-z0-9-]) or an appropriate \b check after the capture).
Modify the regex used in the text.replace call that references
OPSX_TO_CODEX_SKILL so only full valid command ids are captured and substituted,
leaving invalid tokens untouched.
---
Nitpick comments:
In `@docs/supported-tools.md`:
- Line 32: Update the Codex table entry to include explicit migration guidance:
amend the note for "Codex (`codex`) | `.codex/skills/openspec-*/SKILL.md`" to
state that Codex custom prompts are deprecated and instruct existing users to
run `openspec update` to remove deprecated prompts and regenerate skills (e.g.,
"Not generated (Codex custom prompts are deprecated; existing users should run
`openspec update` to clean up old prompts and use `$openspec-*` skills)").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05c533ad-d094-4469-95e0-e431c15c15ab
📒 Files selected for processing (15)
README.mddocs/supported-tools.mdsrc/core/command-generation/adapters/codex.tssrc/core/init.tssrc/core/legacy-cleanup.tssrc/core/profile-sync-drift.tssrc/core/tool-delivery.tssrc/core/update.tssrc/core/workspace/skills.tssrc/utils/command-references.tssrc/utils/index.tstest/core/init.test.tstest/core/profile-sync-drift.test.tstest/core/update.test.tstest/utils/command-references.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/core/command-generation/adapters/codex.ts
- README.md
- src/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- test/utils/command-references.test.ts
- src/core/profile-sync-drift.ts
- test/core/update.test.ts
- test/core/init.test.ts
- src/core/workspace/skills.ts
- src/core/init.ts
- test/core/profile-sync-drift.test.ts
- src/core/update.ts
- src/core/legacy-cleanup.ts
alfred-openspec
previously approved these changes
Jun 1, 2026
Collaborator
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Direction is exactly right. Codex skills-first, legacy prompt cleanup, centralized per-tool delivery in tool-delivery.ts — all clean.
Tests cover the key flows: Codex-only init, mixed tools, legacy prompt removal, drift detection, and command-reference transformation. Docs are updated.
One real issue (minor): the regex in transformToCodexSkillReferences can partially match identifiers. /opsx:apply2 becomes $openspec-apply-change2 instead of being left unchanged, because the capture group [a-z][a-z-]* matches apply and stops at 2.
Fix: add a trailing boundary assertion:
text.replace(/\/opsx[:\-]([a-z][a-z-]*)(?![a-z0-9-])/g, ...)Unlikely to bite in practice since command IDs are well-known, but worth fixing since the function is new in this PR.
Nit: getCodexHome() is now duplicated between legacy-cleanup.ts and codex.ts adapter. Could extract to a shared utility.
LGTM pending the regex fix.
SIGMAREAL
force-pushed
the
fix/codex-skills-first
branch
from
9146383 to
086231d
Compare
alfred-openspec
approved these changes
Jun 1, 2026
Collaborator
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed the latest commit, including the boundary fix for /opsx:apply2; Codex now stays skills-first, removes legacy prompts, and generated skills no longer leak /opsx: or /opsx- references.
Verified with pnpm run lint, pnpm test, and a manual Codex init smoke check with CODEX_HOME pointed at a temp legacy prompt.
5 tasks
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
2 participants
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
~/.codex/prompts/opsx-*.mdcustom prompts and remove existing managed Codex prompt files during init/update/opsx:*and/opsx-*references to$openspec-*skill invocationscommandsTests
init --tools codex --profile coregeneration check: 5 skills generated, deprecated prompt removed, no/opsx:or/opsx-references in.codex/skillstest/commands/artifact-workflow.test.ts > creates skills for Cursor tool: this machine's global OpenSpec config hasdelivery: skills, while that legacy test expects.cursor/commands/opsx-explore.md)Fixes #1129
Addresses #1110
Summary by CodeRabbit
Documentation
Changes
Tests