feat: migrate Codex installer from .codex/prompts to .agents/skills format#1729
Conversation
…ormat Switch CodexSetup to write BMAD artifacts as Agent Skills (agentskills.io format) in .agents/skills/<name>/SKILL.md instead of flat files in .codex/prompts. Remove global/project location prompt. Add legacy cleanup of old .codex/prompts directories during install and uninstall. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…utput Normalize all content to LF in transformToSkillFormat, then convert to os.EOL (CRLF on Windows, LF on Linux/macOS) before writing SKILL.md files in both writeSkillArtifacts and installCustomAgentLauncher. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🤖 Augment PR SummarySummary: Migrates the Codex installer from legacy Changes:
Technical Notes: Skills are now directory-based (one skill per folder) and legacy 🤖 Was this summary useful? React with 👍 or 👎 |
📝 WalkthroughWalkthroughThe PR adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/cli/installers/lib/ide/codex.js (2)
108-123:⚠️ Potential issue | 🟡 Minor
detect()doesn't verify entries are directories — false positive risk.
fs.readdir(dir)returns both files and subdirectories. Theentry.startsWith('bmad')check will match a stale flat file (e.g.,bmad-draft.md) as readily as a legitimate skill directory. Use{ withFileTypes: true }and checkentry.isDirectory()to avoid false positives.🔧 Proposed fix
- const entries = await fs.readdir(dir); - if (entries && entries.some((entry) => entry && typeof entry === 'string' && entry.startsWith('bmad'))) { + const entries = await fs.readdir(dir, { withFileTypes: true }); + if (entries && entries.some((entry) => entry.isDirectory() && entry.name.startsWith('bmad'))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/codex.js` around lines 108 - 123, The detect function may return false positives because it treats any filename starting with "bmad" as a skill; update detect (and refer to getCodexSkillsDir for locating the dir) to call fs.readdir(dir, { withFileTypes: true }) and only consider entries where entry.isDirectory() is true and entry.name.startsWith('bmad'); preserve the try/catch behavior and return true only when such a directory is found, otherwise return false.
278-353: 🛠️ Refactor suggestion | 🟠 Major
clearOldBmadSkillsandclearOldBmadFilesare identical — DRY violation.Both methods implement the exact same algorithm: check directory exists,
readdir, skip non-bmad-prefixed entries,fs.remove. The only difference is the method name and JSDoc. Either haveclearOldBmadSkillsdelegate toclearOldBmadFiles, or extract a singleclearBmadEntries(dir, options)helper both call.♻️ Proposed refactor
- async clearOldBmadSkills(destDir, options = {}) { - if (!(await fs.pathExists(destDir))) { - return; - } - // ... (identical body) - } + async clearOldBmadSkills(destDir, options = {}) { + return this.clearOldBmadFiles(destDir, options); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/codex.js` around lines 278 - 353, The two functions clearOldBmadSkills and clearOldBmadFiles are identical; extract the shared logic into a single helper (e.g., clearBmadEntries(destDir, options)) that performs the pathExists check, readdir, filtering on entry.startsWith('bmad'), and fs.remove with the same error handling, then have both clearOldBmadSkills and clearOldBmadFiles simply call this helper (or have clearOldBmadSkills delegate to clearOldBmadFiles) to remove duplication while preserving behavior and log handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 40: Update the .agents ignore rule so it only matches the directory by
changing the entry from ".agents" to ".agents/"; modify the ignore line that
currently contains ".agents" to ".agents/" so Git ignores the generated .agents/
directory (and its contents) but not files named ".agents", matching the
existing ".junie/" pattern.
In `@tools/cli/installers/lib/ide/codex.js`:
- Around line 35-44: The current setup unconditionally calls
clearOldBmadFiles(oldGlobalDir, options) and clears skills via
clearOldBmadSkills(destDir, options) which can destructively delete user data
and is non-atomic; modify setup so global cleanup (getOldCodexPromptDir +
clearOldBmadFiles) runs only when a migration/first-run condition is detected
(e.g., check a persisted "didMigrateCodex" flag or test for presence of legacy
files before calling clearOldBmadFiles), and change the skills install flow
(getCodexSkillsDir + clearOldBmadSkills) to write new skills into a temporary
staging directory and only rename/move it into place on successful write (or
only delete legacy bmad* entries after verifying new content written), using
getCodexSkillsDir, clearOldBmadSkills, and any existing writeSkill* helpers to
implement the atomic swap or guarded-delete behavior.
- Line 52: In tools/cli/installers/lib/ide/codex.js change calls to
getTasksFromBmad so tasks are not filtered by selectedModules: in setup() (the
call currently getTasksFromBmad(bmadDir, options.selectedModules || [])) call
getTasksFromBmad(bmadDir) instead, and make the same change inside
collectClaudeArtifacts() (replace getTasksFromBmad(bmadDir, selectedModules)
with getTasksFromBmad(bmadDir)); keep agent filtering behavior separate (only
agent collection functions should honor selectedModules).
- Line 33: collectClaudeArtifacts currently does an initial disk sweep to build
an artifacts array but then performs writing via three independent collectors
(the writers) so the returned artifacts can diverge and disk I/O is duplicated;
fix by making the write step the single source of truth: either (A) have
collectClaudeArtifacts stop doing the initial read and instead invoke the three
writer collectors to read/write and aggregate their actual write results into
the artifacts and counts returned, or (B) keep the initial collection but remove
the separate writers and drive all disk writes from the artifacts array
collected initially; update collectClaudeArtifacts (and its callers) so
artifacts and counts are built from the actual write results (the writer
functions' return values) to avoid stale lists and duplicate I/O.
- Around line 406-436: installCustomAgentLauncher currently accepts a metadata
argument but never uses it; update the function so the SKILL.md frontmatter uses
metadata.description (falling back to `${agentName} agent` if absent) when
building fm (the YAML string) and/or include other metadata fields as needed
(e.g., metadata.name) instead of always using skillName/agentName, so
caller-supplied data is preserved; locate the code constructing fm and
skillContent (symbols: installCustomAgentLauncher, customAgentDashName,
skillName, SKILL.md) and wire metadata into the YAML/frontmatter output (or if
metadata is intentionally unused, remove the metadata parameter and any
tests/callers relying on it).
- Line 265: The assignment to description uses a falsy check that overwrites an
explicit empty string (parsed?.description || `${skillName} skill`); change this
to use nullish coalescing so an explicit "" is preserved (i.e., use
parsed?.description ?? `${skillName} skill`) or alternatively validate/normalize
parsed.description before assignment; update the expression around the variables
description, parsed, and skillName in codex.js to use ?? (or add an explicit
check for undefined/null) so empty strings are not replaced.
- Around line 270-272: The reconstructed frontmatter may lack a blank line
between the closing --- and the body; update the return logic around
newFrontmatter/yaml.stringify so that the output always includes an empty line
separator before body (e.g., ensure you emit
`---\n${newFrontmatter}\n---\n\n${body}` or conditionally insert a single
additional newline when the variable body does not already start with a
newline), keeping newFrontmatter generation unchanged.
---
Outside diff comments:
In `@tools/cli/installers/lib/ide/codex.js`:
- Around line 108-123: The detect function may return false positives because it
treats any filename starting with "bmad" as a skill; update detect (and refer to
getCodexSkillsDir for locating the dir) to call fs.readdir(dir, { withFileTypes:
true }) and only consider entries where entry.isDirectory() is true and
entry.name.startsWith('bmad'); preserve the try/catch behavior and return true
only when such a directory is found, otherwise return false.
- Around line 278-353: The two functions clearOldBmadSkills and
clearOldBmadFiles are identical; extract the shared logic into a single helper
(e.g., clearBmadEntries(destDir, options)) that performs the pathExists check,
readdir, filtering on entry.startsWith('bmad'), and fs.remove with the same
error handling, then have both clearOldBmadSkills and clearOldBmadFiles simply
call this helper (or have clearOldBmadSkills delegate to clearOldBmadFiles) to
remove duplication while preserving behavior and log handling.
|
The |
What
Migrate the existing Codex installer in-place from
.codex/prompts(custom prompts) to.agents/skills(agentskills.io format).Why
Related to #1393
Follows up on #1714
How
codex.jsto write each artifact as a skill directory (<skill-name>/SKILL.md) instead of flat prompt files.yamldependency for frontmatter generation, strippingdisable-model-invocationand normalizing to skills-spec fields (name,description).collectConfiguration()--.agents/skillsis auto-discovered by Codex withoutCODEX_HOME..codex/promptslocations (both global and project-scoped) during installation so upgrades are clean.SKILL.mdfiles..agentsto.gitignoresince generated skills are not intended to be committed.Testing
node "/path/to/BMAD-METHOD/tools/bmad-npx-wrapper.js" install.agents/skills/<skill-name>/SKILL.md/skillsto list them, or$skill-nameas a shortcut.codex/promptsbmad files were cleaned upConcerns
It will look for the workflow files in _bmad relative to the skill directory and when it doesn't find them, it falls back to looking in the project. This works, but is not optimal. Eventually, skills probably need to be self-contained. At that point they will likely be getting distributed in some other way.
As a workaround, add a line to AGENTS.md saying the following will prevent the extra search: