fix(cli): survive an unwritable .codex/, inline skills when it happens, and ship the doctrine where the CLI can find it#745
Conversation
…ppens Codex sandboxes deny writes under `.codex/`. `installSkills` created its destination with an unguarded `mkdirSync` sitting directly ABOVE a per-skill copy loop that was already guarded — so the failure threw past that fallback and past the caller, aborting the whole handoff step. Because the skills install runs first, nothing after it ran either: no AGENTS.md, no .cipherstash/context.json, no .cipherstash/setup-prompt.md. All five Codex runs of the rc.3 skilltester matrix landed here, and that report named it the primary driver of the Claude→Codex quality gap (#736). Two changes. `installSkills` never throws. Every filesystem step degrades to a warning and a shorter return, so the caller decides what an empty result means rather than being skipped entirely. The Codex handoff falls back to inlining. When the skills cannot be written, their bodies go into AGENTS.md — project root, writable — through the same `doctrine-plus-skills` path the editor-agent handoff already uses for Cursor / Windsurf / Cline. No new mechanism; the existing one just was not reachable from this branch. The launch prompt points at wherever the guidance actually ended up. The fallback is only claimed when there is something to inline. A stripped build ships no skills at all, so it stays doctrine-only and says nothing — reporting a fallback that did not happen would be the same false-success bug #714 and #687 removed elsewhere in init. That is why `availableSkills` is split out: an empty install result alone cannot distinguish "could not write" from "nothing to write". @cipherstash/wizard carries its own copy of `installSkills` with the identical unguarded `mkdirSync` above the identical guarded copy loop. It targets `.claude/skills`, so the Codex sandbox case does not apply, but an unwritable destination crashed it the same way — guarded to match. Tests: 2 for the mkdirSync guard (including that the caller can still distinguish unwritable from empty), 6 for the handoff across all three states — copied, unwritable-so-inlined, and nothing-to-ship. 645 unit + 68 pty E2E pass; typecheck and build clean; 0 biome errors. Checked per packages/cli/AGENTS.md: no E2E asserts on these strings (the only codex assertion is the target list, unchanged), and no skill documents the install location, so neither needed updating.
🦋 Changeset detectedLatest commit: bd40087 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSkill installation now returns copied and failed results without throwing on unwritable destinations. Codex can inline failed skills into ChangesSkill handoff resilience
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HandoffCodexStep
participant InstallSkills
participant AgentsMdBuilder
participant WriteAgentsMd
participant Codex
HandoffCodexStep->>InstallSkills: Install skills in .codex/skills/
InstallSkills-->>HandoffCodexStep: Return copied and failed skills
HandoffCodexStep->>AgentsMdBuilder: Build doctrine-plus-skills for failed skills
AgentsMdBuilder-->>HandoffCodexStep: Return AGENTS.md content
HandoffCodexStep->>WriteAgentsMd: Write managed AGENTS.md
WriteAgentsMd-->>HandoffCodexStep: Return write status
HandoffCodexStep->>Codex: Point to installed or inlined skill guidance
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Code review — 15 findings (ranked by severity)Multi-pass review of this branch (10 independent finder angles, per-candidate adversarial verification, then a gap sweep). The fix's core idea is right and the never-throw guard works, but the fallback path has one production-blocking problem and several places where its claims and its artifacts disagree. Critical1. In the published build, the fallback inlines nothing — while claiming it did. 2. The fallback produces self-contradictory artifacts. 3. A partial copy failure suppresses the fallback entirely. The never-throws contract stops one call too early4. The AGENTS.md write path can still abort the whole step with zero artifacts. 5. Unwritable 6. Fallback content correctness7. The inlined doctrine points Codex at the directory that just failed. 8. 9. The fallback warning asserts a cause it never observed. Design / state management10. 11. Stale skill state accumulates across runs and target switches. 12. A confirmed-then-failed wizard install leaves no trace in the wizard log. Tests & docs13. The regression coverage doesn't pin the contract it documents. 14. The diff makes 15. An internal issue-tracker ID ships again in a file this PR edits. Refuted / dropped during verification: the wizard |
Address the 15 findings from the PR #745 review. The structural change: installSkills returns { copied, failed } instead of a flat list, so callers distinguish unwritable / stripped / partial outcomes without a second probe. The Codex handoff inlines exactly the failed skills — a partial copy inlines the missing ones and the launch prompt points at both locations. The production-blocking find: the bundled AGENTS.md doctrine shipped at dist/commands/init/doctrine, but findBundledDir probes ancestors of the compiled chunk in dist/bin/ — so every published build wrote the minimal AGENTS.md stub, and the inline fallback would have inlined nothing while claiming otherwise. The doctrine now lands at dist/doctrine, like the skills bundle, and buildAgentsMdBody honours doctrine-plus-skills even when the doctrine fragment is missing. Honesty and hardening around the fallback: - writeArtifacts takes a SkillsDelivery (installed/inlined/failed); context.json gains inlinedSkills and setup-prompt.md stops mislabelling an unwritable destination as a "stripped build" — on the Claude and AGENTS.md handoffs too - availableSkills filters on SKILL.md, the predicate the inliner actually uses, so "inlining N skills" never overstates - the AGENTS.md upsert (which refuses malformed sentinel pairs) and the bundled-file reads degrade to warnings instead of aborting the step before .cipherstash/ is written; skills only count as inlined when the write landed - the fallback warning announces the observed recovery instead of asserting an unverified cause, and flags a stale .codex/skills/ it could not refresh - a confirmed-then-failed wizard install is recorded in the wizard changelog instead of vanishing with the terminal output Tests pin the degrade-to-warning contract in both packages (the wizard suite is new), cover the partial-copy split end to end, and replace the vacuously-true warning assertions. The doctrine's "Where to read more", the build-agents-md docblock, and root AGENTS.md now describe the real mode consumers.
Review response — all 15 findings addressed in 3f9e297Point-by-point resolution of the review above: 1. Fallback inlines nothing in the published build — fixed at the root. The doctrine now ships at 2. Self-contradictory artifacts — fixed. 3. Partial copy suppresses the fallback — fixed structurally. 4. Unguarded AGENTS.md write path — fixed. A shared 5. Claude handoff silent loss + misattribution — fixed. It now warns "N skills could not be installed to 6. 7. Doctrine points at the failed directory — fixed. "Where to read more" now lists the inlined 8. Predicate mismatch — fixed. 9. Wrong-cause warning — fixed. The handoff's message now announces the observed recovery ("Inlining N skills that could not be installed to 10. Flat return shape — fixed (see 3); the 11. Stale-state residue — partially fixed. When the fallback fires and a pre-existing 12. Wizard changelog silence — fixed. 13. Test gaps — fixed. The CLI suite mocks the warn channel and asserts the degrade-to-warning contract; the wizard gets its first 14. Doc drift — fixed. The 15. Internal tracker ID — removed from the wizard comment. Deferred (called out, not fixed): the cross-target AGENTS.md residue from finding 11, and the pre-existing launch-prompt duplication between the two handoffs (finding dropped in review as pre-existing; the divergence is partly semantic). Verification: 660 CLI unit tests, 68 pty e2e tests, and 148 wizard tests pass; Biome check is error-free; |
coderdan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
packages/cli/src/commands/init/lib/handoff-helpers.ts (1)
85-103: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
writeContextFile/writeSetupPromptare still unguarded — the exact#736failure mode, one directory over.
writeAgentsMdis correctly hardened, butwriteArtifactscallswriteContextFile(Line 95) andwriteSetupPrompt(Line 100) directly; per thebuildContextFile/writeContextFiledefinitions these doensureDir+writeFileSyncwith no try/catch. If.cipherstash/is unwritable in the same sandboxed environment that made.codex/unwritable, this throws and aborts the entire handoff step, discarding all the copied/inlined skill work just performed — the same blast radius#736described, just for a different directory.🛡️ Proposed fix to guard the remaining artifact writes
const ctx = buildContextFile(state) ctx.envKeys = state.envKeys ?? [] ctx.installedSkills = skills.installed ctx.inlinedSkills = skills.inlined - writeContextFile(resolve(cwd, CONTEXT_REL_PATH), ctx) - p.log.success(`Wrote ${CONTEXT_REL_PATH}`) + try { + writeContextFile(resolve(cwd, CONTEXT_REL_PATH), ctx) + p.log.success(`Wrote ${CONTEXT_REL_PATH}`) + } catch (err) { + const message = err instanceof Error ? err.message : String(err) + p.log.warn(`Could not write ${CONTEXT_REL_PATH}: ${message}`) + } const promptCtx = buildSetupPromptContext(state, handoff, skills) if (promptCtx) { - writeSetupPrompt(resolve(cwd, SETUP_PROMPT_REL_PATH), promptCtx) - p.log.success(`Wrote ${SETUP_PROMPT_REL_PATH}`) + try { + writeSetupPrompt(resolve(cwd, SETUP_PROMPT_REL_PATH), promptCtx) + p.log.success(`Wrote ${SETUP_PROMPT_REL_PATH}`) + } catch (err) { + const message = err instanceof Error ? err.message : String(err) + p.log.warn(`Could not write ${SETUP_PROMPT_REL_PATH}: ${message}`) + } }🤖 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 `@packages/cli/src/commands/init/lib/handoff-helpers.ts` around lines 85 - 103, Guard the artifact writes in writeArtifacts so failures from writeContextFile and writeSetupPrompt do not abort the handoff after skill delivery. Catch and handle each write failure independently, preserving successful writes and the existing success logs while allowing the function to complete when .cipherstash/ or the setup prompt location is unwritable.packages/cli/src/commands/impl/steps/handoff-codex.ts (1)
90-109: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift"AGENTS.md has the durable rules" is claimed even when the AGENTS.md write itself failed.
None of the messaging surfaces key off the actual per-run AGENTS.md write outcome (
agentsMdWritteninhandoff-codex.ts,writteninhandoff-agents-md.ts) — they either hardcode the "AGENTS.md has the rules" text or infer it from handoff type alone (wroteAgentsMdinsetup-prompt.ts). This is provably wrong in the exact failure case the PR targets: wheneverskills.failed.length > 0reaches the "nothing delivered" branch forcodex/agents-md, the underlying write is guaranteed to have failed (perundelivered = agentsMdWritten ? [] : failed/failed: written ? [] : inlinable), yet the rendered text still tells the agent to "read AGENTS.md" for the durable rules.
packages/cli/src/commands/impl/steps/handoff-codex.ts#L90-L109: gate the "AGENTS.md has the durable rules;" clause inlaunchPromptonagentsMdWritten, falling back to a docs pointer when it's false.packages/cli/src/commands/impl/steps/handoff-agents-md.ts#L35-L59: gate thep.note"Rules at AGENTS.md" line onwritten, telling the user the write failed instead when it's false.packages/cli/src/commands/init/lib/setup-prompt.ts#L144-L167: replace the handoff-type-onlywroteAgentsMdinference with a real write-success signal (e.g. anagentsMdWrittenfield threaded throughSkillsDelivery/SetupPromptContextfrom the handoff steps) so the "read AGENTS.md" message isn't shown when the write actually failed.🤖 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 `@packages/cli/src/commands/impl/steps/handoff-codex.ts` around lines 90 - 109, Gate the AGENTS.md guidance in packages/cli/src/commands/impl/steps/handoff-codex.ts:90-109 on agentsMdWritten and use a docs fallback when false; in packages/cli/src/commands/impl/steps/handoff-agents-md.ts:35-59, show the failure message instead of “Rules at AGENTS.md” when written is false. In packages/cli/src/commands/init/lib/setup-prompt.ts:144-167, replace handoff-type-only wroteAgentsMd inference with the actual write-success signal, threading agentsMdWritten through SkillsDelivery/SetupPromptContext so “read AGENTS.md” is only displayed after a successful write.
🧹 Nitpick comments (1)
packages/cli/src/commands/init/lib/install-skills.ts (1)
125-136: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPartial
cpSyncfailures can leave stale files in the destination.
cpSynccopies recursively; if it fails partway (e.g. disk full, permission denied on a nested file), whatever was already written underdeststays behind even though the skill is reported asfailed. A later successful retry withforce: truewill likely overwrite it, but until then an agent reading.codex/skills/<name>/(or.claude/skills/<name>/) could see incomplete skill content without any signal that it's partial.Consider a best-effort cleanup on failure:
🧹 Proposed cleanup on copy failure
try { cpSync(src, dest, { recursive: true, force: true }) copied.push(name) } catch (err) { const message = err instanceof Error ? err.message : String(err) p.log.warn(`Failed to install skill ${name}: ${message}`) + try { + rmSync(dest, { recursive: true, force: true }) + } catch { + // best-effort; the skill is already reported via `failed` + } failed.push(name) }🤖 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 `@packages/cli/src/commands/init/lib/install-skills.ts` around lines 125 - 136, Update the cpSync failure handling in the skill-installation loop to best-effort remove the partially copied destination before logging the warning and recording the skill in failed. Use the existing dest path and filesystem utilities, while preserving the current error reporting and failed-list behavior even if cleanup also fails.
🤖 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 `@packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts`:
- Around line 155-176: Update the skipped partial-copy test around installSkills
to simulate the blocked stash-drizzle destination with a regular file, reusing
the file-instead-of-directory setup from the preceding test. Remove the
permission-based directory creation and cleanup, while preserving the expected
copied, failed, and warning assertions.
---
Outside diff comments:
In `@packages/cli/src/commands/impl/steps/handoff-codex.ts`:
- Around line 90-109: Gate the AGENTS.md guidance in
packages/cli/src/commands/impl/steps/handoff-codex.ts:90-109 on agentsMdWritten
and use a docs fallback when false; in
packages/cli/src/commands/impl/steps/handoff-agents-md.ts:35-59, show the
failure message instead of “Rules at AGENTS.md” when written is false. In
packages/cli/src/commands/init/lib/setup-prompt.ts:144-167, replace
handoff-type-only wroteAgentsMd inference with the actual write-success signal,
threading agentsMdWritten through SkillsDelivery/SetupPromptContext so “read
AGENTS.md” is only displayed after a successful write.
In `@packages/cli/src/commands/init/lib/handoff-helpers.ts`:
- Around line 85-103: Guard the artifact writes in writeArtifacts so failures
from writeContextFile and writeSetupPrompt do not abort the handoff after skill
delivery. Catch and handle each write failure independently, preserving
successful writes and the existing success logs while allowing the function to
complete when .cipherstash/ or the setup prompt location is unwritable.
---
Nitpick comments:
In `@packages/cli/src/commands/init/lib/install-skills.ts`:
- Around line 125-136: Update the cpSync failure handling in the
skill-installation loop to best-effort remove the partially copied destination
before logging the warning and recording the skill in failed. Use the existing
dest path and filesystem utilities, while preserving the current error reporting
and failed-list behavior even if cleanup also fails.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4639202-f0bb-4fcd-9e28-693981317729
📒 Files selected for processing (22)
.changeset/codex-skills-eperm.mdAGENTS.mdpackages/cli/src/commands/impl/steps/__tests__/handoff-claude.test.tspackages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.tspackages/cli/src/commands/impl/steps/handoff-agents-md.tspackages/cli/src/commands/impl/steps/handoff-claude.tspackages/cli/src/commands/impl/steps/handoff-codex.tspackages/cli/src/commands/init/doctrine/AGENTS-doctrine.mdpackages/cli/src/commands/init/lib/__tests__/build-agents-md.test.tspackages/cli/src/commands/init/lib/__tests__/install-skills.test.tspackages/cli/src/commands/init/lib/__tests__/read-context.test.tspackages/cli/src/commands/init/lib/__tests__/setup-prompt.test.tspackages/cli/src/commands/init/lib/build-agents-md.tspackages/cli/src/commands/init/lib/handoff-helpers.tspackages/cli/src/commands/init/lib/install-skills.tspackages/cli/src/commands/init/lib/setup-prompt.tspackages/cli/src/commands/init/lib/write-context.tspackages/cli/src/commands/status/__tests__/status.test.tspackages/cli/tsup.config.tspackages/wizard/src/__tests__/install-skills.test.tspackages/wizard/src/lib/install-skills.tspackages/wizard/src/run.ts
The read-only-directory trigger was platform-dependent: on macOS cpSync unlinks the destination file first (EACCES from the 0o555 parent), but on Linux it overwrites the writable SKILL.md in place and the copy succeeds — so the test failed on CI while passing locally. Block the skill with a FILE where its directory must go instead: cpSync then throws ERR_FS_CP_DIR_TO_NON_DIR, a type-mismatch error raised before any permission-dependent operation, so it fires on every platform and as root. Same technique the neighbouring mkdir test already uses. Drops the skipIf guard the chmod approach needed.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Closes #736.
Codex sandboxes deny writes under
.codex/.installSkillscreated its destination with an unguardedmkdirSyncsitting directly above a per-skill copy loop that was already guarded:The graceful-degradation path existed; it just sat one line below the thing that actually failed.
And the blast radius was wider than the skills.
installSkillsruns first in the handoff step, so the throw took everything after it: noAGENTS.md, no.cipherstash/context.json, no.cipherstash/setup-prompt.md. All five Codex runs of the rc.3 matrix landed here, and that report named it the primary driver of the Claude→Codex quality gap.The fix
1.
installSkillsnever throws, and reports what happened. It returns{ copied, failed }— every filesystem failure degrades to a warning plus afailedentry, so callers distinguish unwritable from stripped build from partial copy directly from the result instead of re-deriving it.2. The Codex handoff inlines exactly the skills that failed. Whatever couldn't be written goes into
AGENTS.md— project root, writable — via the samedoctrine-plus-skillspath the editor-agent handoff already uses for Cursor / Windsurf / Cline. A partial copy inlines just the missing skills. The launch prompt points at wherever the guidance actually ended up:.codex/skills/doctrine-only.codex/skills/.codex/unwritableAGENTS.mddoctrine-plus-skillsdoctrine-plus-skills(failed subset)doctrine-onlyThe doctrine wasn't shipping where the CLI could find it
The review of the first cut found that in every published build, the fallback would have inlined nothing while claiming it did — and the cause predates this PR:
tsupcopied the AGENTS.md doctrine todist/commands/init/doctrine, mirroring the source layout.findBundledDirprobes ancestor directories of the compiled chunk, which lives indist/bin/—dist/bin/doctrine,dist/doctrine, package root, … — and never reachesdist/commands/init/doctrine.readDoctrine()returnedundefinedin every published build, andbuildAgentsMdBodyearly-returned a minimal stub that ignored the mode entirely — no doctrine and no inlined skills. Dev runs and unit tests resolve the source-tree copy one level up, which is why nothing ever caught it.Two fixes, and they compound with the fallback:
dist/doctrine, exactly likedist/skills— the second ancestor probe fromdist/bin/resolves it (verified against a clean rebuild).buildAgentsMdBodyhonoursdoctrine-plus-skillseven without the doctrine fragment — skills inline under a minimal header instead of being silently dropped with it, so the two failure modes can no longer combine into an empty AGENTS.md that the prompt calls complete.Side effect worth noting: this also repairs plain
doctrine-onlyAGENTS.md output — published builds have been writing the stub instead of the doctrine on the happy path too.The honesty case
The generated artifacts describe what actually happened, not what the handoff hoped for:
writeArtifactstakes aSkillsDelivery(installed/inlined/failed);context.jsongains aninlinedSkillsfield, andsetup-prompt.mdrenders distinct branches for installed, inlined, both, failed-undelivered, and genuinely-stripped — an unwritable destination is no longer mislabelled a "stripped build" (the Claude and editor-agent handoffs had the same pre-existing misattribution; fixed there too).writeAgentsMdcatches the malformed-sentinel refusal and fs errors, warns, and keeps going so.cipherstash/is still written), and on failure the skills are recorded asfailedwith no inline claim in the prompt.availableSkillsfilters onSKILL.md— the predicate the inliner actually uses — so "inlining N skills" can never overstate;readBundledSkill/readDoctrineguard their reads so an unreadable bundled file degrades instead of aborting pre-artifact.doctrine-onlyand says nothing — reporting a fallback that didn't happen would be the same false-success bug Non-interactivestash initreports false success (skew, EQL-missing, skills) #714 and feat(cli): honest non-interactive init — fail on skew, no false success (rc.2 M4) #687 removed elsewhere in init..codex/skills/from an earlier run couldn't be refreshed, the handoff says so and notes the inlined copies are current.Also: the wizard had the same bug
@cipherstash/wizardcarries its own copy ofinstallSkills, with the identical unguardedmkdirSyncabove the identical guarded copy loop. It targets.claude/skills, so the Codex sandbox case doesn't apply — but an unwritable destination (read-only mount, restrictive permissions) crashed it the same way. Guarded to match, returning the same{ copied, failed }shape — and a confirmed-then-failed install is now recorded in the wizard changelog instead of vanishing with the terminal output.Validation
install-skillssuite: mkdir failure, partial copy, decline).Per
packages/cli/AGENTS.mdI checked both drift surfaces: no E2E asserts on these strings (the only codex assertion is the target list, unchanged), and no skill documents the install location — so neither needed updating. The doctrine's "Where to read more", thebuild-agents-mddocblock, and rootAGENTS.mdwere updated to describe the inline path's real consumers.Not in scope
plan.mdcan't be written — belongs to stash plan reports "Plan drafted at .cipherstash/plan.md" and exits 0 without writing the file (rc.3 M2) #738 (stash planreports a file it never wrote). Left there rather than split across two PRs.Summary by CodeRabbit
.codex/directory cannot be written.AGENTS.mdwhen needed.context.jsonand related setup guidance now track inlined skills and failure states accurately.