feat(codex): migrate .codex skills to .agents#1157
Open
satyaborg wants to merge 6 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughMigrates Codex skill generation from .codex/skills to .agents/skills, adds legacy-detection metadata and shared path helpers, implements a migration module to remove managed legacy directories after successful regeneration, integrates removals into init/update/workspace flows, refactors drift/migration checks, and updates tests/docs. ChangesCodex Skills Migration
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
Author
|
related to #1143, this PR handles the separate path migration from legacy .codex/skills to current .agents/skills, including safe cleanup and workspace coverage |
![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 (2)
test/core/migration.test.ts (1)
90-101: 💤 Low value
CODEX_HOMEappears unnecessary for this test.
migrateIfNeededscansprojectDirand resolves legacy skills via the tool'slegacySkillsDirs; it does not readCODEX_HOME. Setting it (Line 91) doesn't affect the assertions and may mislead future readers into thinking global resolution is exercised. Consider dropping it unless it guards against an environment side-effect.🤖 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 `@test/core/migration.test.ts` around lines 90 - 101, The test unnecessarily sets process.env.CODEX_HOME; remove the assignment (process.env.CODEX_HOME = ...) on the test that calls migrateIfNeeded so the test only relies on projectDir-based legacy detection; locate the lines around migrateIfNeeded(projectDir, [ensureCodexTool()]), writeLegacyCodexSkill(...) and readRawConfig() in test/core/migration.test.ts and delete the CODEX_HOME assignment (or clear it with delete process.env.CODEX_HOME) to avoid misleading readers or environment side-effects.src/core/profile-sync-drift.ts (1)
206-222: 💤 Low valueReuse
getToolSkillDirectoriesinstead of rebuilding the current+legacy list.
migration.tsalready uses the sharedgetToolSkillDirectorieshelper for exactly this (current dir + legacy when included). The manual construction here duplicates that logic; the directory list is only consumed inside theincludeSkillsblock.♻️ Proposed refactor
- const installed = new Set<WorkflowId>(); - const skillDirectories = [getToolCurrentSkillDirectory(projectPath, tool)].filter( - (skillsDir): skillsDir is string => skillsDir !== null - ); - - if (options.includeSkills) { - skillDirectories.push(...getToolLegacySkillDirectories(projectPath, tool)); - - for (const skillsDir of skillDirectories) { + const installed = new Set<WorkflowId>(); + + if (options.includeSkills) { + for (const skillsDir of getToolSkillDirectories(projectPath, tool)) { for (const workflow of ALL_WORKFLOWS) { const dirName = WORKFLOW_TO_SKILL_DIR[workflow]; const skillFile = path.join(skillsDir, dirName, 'SKILL.md'); if (fs.existsSync(skillFile)) { installed.add(workflow); } } } }Update the import to pull
getToolSkillDirectoriesfrom./shared/index.js.🤖 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 `@src/core/profile-sync-drift.ts` around lines 206 - 222, Replace the manual construction of skillDirectories (which uses getToolCurrentSkillDirectory and getToolLegacySkillDirectories) with the shared helper getToolSkillDirectories; import getToolSkillDirectories from ./shared/index.js and set skillDirectories = getToolSkillDirectories(projectPath, tool, options.includeSkills) (or call the helper in the same way other code does), then keep the existing loop over skillDirectories/ALL_WORKFLOWS/WORKFLOW_TO_SKILL_DIR to populate installed — remove the duplicated legacy/current logic and its imports.
🤖 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/codex-skill-migration.ts`:
- Around line 33-40: The removeManagedLegacyCodexSkills function currently
propagates any rm error and fails the whole setup; change it to perform
best-effort per-directory removal by wrapping the await fs.promises.rm(skillDir,
...) call in a try/catch inside the for loop, log or warn on error (e.g., using
processLogger.warn or console.warn) including the skillDir and error, do not
rethrow so the loop continues, and only increment removed when rm succeeds; keep
using getManagedLegacyCodexSkillDirectories to locate dirs and return the
accurate removed count.
In `@src/core/init.ts`:
- Around line 551-553: The migration call removeManagedLegacyCodexSkills is
currently inside the skillTemplates loop and runs once per template; move the
call so it runs once per tool by relocating the await
removeManagedLegacyCodexSkills(projectPath) out of the for (const { template,
dirName } of skillTemplates) loop and into the surrounding scope that iterates
tools where you check if (tool.value === 'codex'), updating
migratedLegacyCodexSkillCount accordingly; ensure the reference to
migratedLegacyCodexSkillCount and projectPath remain correct and that the call
executes exactly once when tool.value === 'codex'.
In `@src/core/update.ts`:
- Around line 708-710: The call to removeManagedLegacyCodexSkills inside
upgradeLegacyTools discards its returned migration count, so update
upgradeLegacyTools to return that count (or include it on the existing
newlyConfigured return object) and then update the caller to destructure the
returned value and add it into migratedLegacyCodexSkillCount; specifically,
change upgradeLegacyTools to accumulate the result of
removeManagedLegacyCodexSkills into a local migrated count and return it, and at
the call site add that returned count to the existing
migratedLegacyCodexSkillCount accumulator.
---
Nitpick comments:
In `@src/core/profile-sync-drift.ts`:
- Around line 206-222: Replace the manual construction of skillDirectories
(which uses getToolCurrentSkillDirectory and getToolLegacySkillDirectories) with
the shared helper getToolSkillDirectories; import getToolSkillDirectories from
./shared/index.js and set skillDirectories =
getToolSkillDirectories(projectPath, tool, options.includeSkills) (or call the
helper in the same way other code does), then keep the existing loop over
skillDirectories/ALL_WORKFLOWS/WORKFLOW_TO_SKILL_DIR to populate installed —
remove the duplicated legacy/current logic and its imports.
In `@test/core/migration.test.ts`:
- Around line 90-101: The test unnecessarily sets process.env.CODEX_HOME; remove
the assignment (process.env.CODEX_HOME = ...) on the test that calls
migrateIfNeeded so the test only relies on projectDir-based legacy detection;
locate the lines around migrateIfNeeded(projectDir, [ensureCodexTool()]),
writeLegacyCodexSkill(...) and readRawConfig() in test/core/migration.test.ts
and delete the CODEX_HOME assignment (or clear it with delete
process.env.CODEX_HOME) to avoid misleading readers or environment side-effects.
🪄 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: 5edf0d17-cf24-4d64-927d-e9ded3b6ac71
📒 Files selected for processing (28)
docs/cli.mddocs/commands.mddocs/supported-tools.mdopenspec/changes/migrate-codex-skills-to-agents/.openspec.yamlopenspec/changes/migrate-codex-skills-to-agents/design.mdopenspec/changes/migrate-codex-skills-to-agents/proposal.mdopenspec/changes/migrate-codex-skills-to-agents/specs/ai-tool-paths/spec.mdopenspec/changes/migrate-codex-skills-to-agents/specs/cli-init/spec.mdopenspec/changes/migrate-codex-skills-to-agents/specs/cli-update/spec.mdopenspec/changes/migrate-codex-skills-to-agents/specs/workspace-links/spec.mdopenspec/changes/migrate-codex-skills-to-agents/tasks.mdsrc/commands/workspace.tssrc/core/codex-skill-migration.tssrc/core/config.tssrc/core/init.tssrc/core/migration.tssrc/core/profile-sync-drift.tssrc/core/shared/index.tssrc/core/shared/tool-detection.tssrc/core/update.tssrc/core/workspace/skills.tstest/commands/workspace.test.tstest/core/available-tools.test.tstest/core/init.test.tstest/core/migration.test.tstest/core/shared/tool-detection.test.tstest/core/update.test.tstest/core/workspace/skills.test.ts
Comment on lines
+33
to
+40
| export async function removeManagedLegacyCodexSkills(projectPath: string): Promise<number> { | ||
| let removed = 0; | ||
|
|
||
| for (const skillDir of getManagedLegacyCodexSkillDirectories(projectPath)) { | ||
| if (!fs.existsSync(skillDir)) continue; | ||
| await fs.promises.rm(skillDir, { recursive: true, force: true }); | ||
| removed++; | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t fail full setup on legacy cleanup errors.
A single rm failure here bubbles up and marks Codex setup as failed in init/update/workspace flows, even when new .agents/skills were already generated. Make cleanup best-effort per directory (warn and continue), while still returning accurate removal count.
Suggested patch
export async function removeManagedLegacyCodexSkills(projectPath: string): Promise<number> {
let removed = 0;
for (const skillDir of getManagedLegacyCodexSkillDirectories(projectPath)) {
if (!fs.existsSync(skillDir)) continue;
- await fs.promises.rm(skillDir, { recursive: true, force: true });
- removed++;
+ try {
+ await fs.promises.rm(skillDir, { recursive: true, force: true });
+ removed++;
+ } catch {
+ // Best-effort cleanup: don't fail setup if legacy deletion is blocked.
+ continue;
+ }
}
return removed;
}🤖 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 `@src/core/codex-skill-migration.ts` around lines 33 - 40, The
removeManagedLegacyCodexSkills function currently propagates any rm error and
fails the whole setup; change it to perform best-effort per-directory removal by
wrapping the await fs.promises.rm(skillDir, ...) call in a try/catch inside the
for loop, log or warn on error (e.g., using processLogger.warn or console.warn)
including the skillDir and error, do not rethrow so the loop continues, and only
increment removed when rm succeeds; keep using
getManagedLegacyCodexSkillDirectories to locate dirs and return the accurate
removed count.
Comment on lines
+551
to
+553
| if (tool.value === 'codex') { | ||
| migratedLegacyCodexSkillCount += await removeManagedLegacyCodexSkills(projectPath); | ||
| } |
Contributor
There was a problem hiding this comment.
Move legacy migration outside the skill template loop.
The removeManagedLegacyCodexSkills call is currently inside the for (const { template, dirName } of skillTemplates) loop (lines 538-549), so it will execute once per skill template rather than once per tool. For core profile with 5 skills, this means the migration runs 5 times, which is inefficient and may produce incorrect counts if the function is not idempotent.
🐛 Proposed fix: move migration after the skill loop
await FileSystemUtils.writeFile(skillFile, skillContent);
}
-
- if (tool.value === 'codex') {
- migratedLegacyCodexSkillCount += await removeManagedLegacyCodexSkills(projectPath);
- }
}
+
+ if (shouldGenerateSkills && tool.value === 'codex') {
+ migratedLegacyCodexSkillCount += await removeManagedLegacyCodexSkills(projectPath);
+ }
+
if (!shouldGenerateSkills) {🤖 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 `@src/core/init.ts` around lines 551 - 553, The migration call
removeManagedLegacyCodexSkills is currently inside the skillTemplates loop and
runs once per template; move the call so it runs once per tool by relocating the
await removeManagedLegacyCodexSkills(projectPath) out of the for (const {
template, dirName } of skillTemplates) loop and into the surrounding scope that
iterates tools where you check if (tool.value === 'codex'), updating
migratedLegacyCodexSkillCount accordingly; ensure the reference to
migratedLegacyCodexSkillCount and projectPath remain correct and that the call
executes exactly once when tool.value === 'codex'.
alfred-openspec
requested changes
Jun 4, 2026
Collaborator
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for pushing this migration. I found one cleanup gap: commands-only Codex updates can leave managed legacy .codex/skills directories in place even after --force, and this still needs a rebase or stack on #1143 so it keeps the Codex skills-first behavior instead of reintroducing prompt generation.
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/skills/openspec-*to.agents/skills/openspec-*tests
pnpm run buildpnpm run linttests
git diff --checkSummary by CodeRabbit
New Features
Documentation
Tests