From a6b60356dd743c20fd7509e3f434bef94e5104ed Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Tue, 21 Jul 2026 17:36:30 +1000 Subject: [PATCH 1/4] fix(cli): survive an unwritable .codex/, and inline skills when it happens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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/codex-skills-eperm.md | 17 ++++ .../steps/__tests__/handoff-codex.test.ts | 96 +++++++++++++++---- .../src/commands/impl/steps/handoff-codex.ts | 51 ++++++++-- .../init/lib/__tests__/install-skills.test.ts | 42 +++++++- .../src/commands/init/lib/install-skills.ts | 38 +++++++- packages/wizard/src/lib/install-skills.ts | 14 ++- 6 files changed, 230 insertions(+), 28 deletions(-) create mode 100644 .changeset/codex-skills-eperm.md diff --git a/.changeset/codex-skills-eperm.md b/.changeset/codex-skills-eperm.md new file mode 100644 index 000000000..7f3f1963a --- /dev/null +++ b/.changeset/codex-skills-eperm.md @@ -0,0 +1,17 @@ +--- +'stash': patch +'@cipherstash/wizard': patch +--- + +Fix the Codex handoff installing zero skills — and losing `AGENTS.md` and `.cipherstash/` with them — when `.codex/` is not writable. + +Codex sandboxes deny writes under `.codex/`. `installSkills` created its destination with an unguarded `mkdirSync`, sitting directly above a per-skill copy loop that *was* 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 it was identified in that report as the primary driver of the Claude→Codex quality gap. + +Two changes: + +- **`installSkills` never throws.** Every filesystem step now degrades to a warning and a shorter return, so the caller decides what to do with an empty result instead of being skipped entirely. +- **The Codex handoff falls back to inlining.** When the skills cannot be written, their bodies are inlined into `AGENTS.md` — which lives at the project root and is writable — via the same `doctrine-plus-skills` path the editor-agent handoff already uses for Cursor / Windsurf / Cline. Codex still gets the API guidance, in one file instead of a directory, and the launch prompt points at wherever it actually ended up. + +The fallback is only claimed when there is something to inline: a stripped CLI build that ships no skills at all stays `doctrine-only` and says nothing, rather than reporting a fallback that did not happen. + +`@cipherstash/wizard` carries its own copy of `installSkills` with the same unguarded `mkdirSync` above the same guarded copy loop. It targets `.claude/skills` rather than `.codex/skills`, so the Codex sandbox case does not apply, but an unwritable destination crashed it identically — now guarded the same way. diff --git a/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts b/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts index b095fa981..ec297c0a2 100644 --- a/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts +++ b/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts @@ -1,21 +1,28 @@ -import { beforeEach, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import type { InitState } from '../../../init/types.js' // Same seam as the handoff-claude test: the launch prompt's skills clause is // the unit under test. Mock the skill installer to control whether skills were // "copied". The Codex handoff also writes AGENTS.md to disk before printing // the prompt, so stub `node:fs` and the AGENTS.md builders to keep the test -// hermetic (no file lands in the repo) — the assertion is purely on the -// launch-prompt text. +// hermetic (no file lands in the repo) — the assertions are on the +// launch-prompt text and the AGENTS.md mode chosen. const installSkills = vi.hoisted(() => vi.fn()) -vi.mock('../../../init/lib/install-skills.js', () => ({ installSkills })) +const availableSkills = vi.hoisted(() => vi.fn()) +vi.mock('../../../init/lib/install-skills.js', () => ({ + installSkills, + availableSkills, +})) vi.mock('../../../init/lib/handoff-helpers.js', () => ({ writeArtifacts: vi.fn(), spawnAgent: vi.fn(async () => 0), })) -vi.mock('../../../init/lib/build-agents-md.js', () => ({ - buildAgentsMdBody: vi.fn(() => '# managed doctrine'), -})) +// Typed with both parameters so `mock.calls[0][1]` (the AGENTS.md mode, which +// is what the fallback actually switches) type-checks. +const buildAgentsMdBody = vi.hoisted(() => + vi.fn((_integration: string, _mode: string) => '# managed doctrine'), +) +vi.mock('../../../init/lib/build-agents-md.js', () => ({ buildAgentsMdBody })) vi.mock('../../../init/lib/sentinel-upsert.js', () => ({ upsertManagedBlock: vi.fn(() => '# AGENTS.md'), })) @@ -36,19 +43,74 @@ import { handoffCodexStep } from '../handoff-codex.js' // prompt into a `p.note`. const state = { integration: 'postgresql' } as unknown as InitState -beforeEach(() => vi.clearAllMocks()) +const promptBody = () => vi.mocked(p.note).mock.calls[0][0] +const agentsMdMode = () => vi.mocked(buildAgentsMdBody).mock.calls[0][1] +const warnings = () => vi.mocked(p.log.warn).mock.calls.map(String).join('\n') -it('launch prompt drops .codex/skills/ but keeps AGENTS.md when no skills copied', async () => { - installSkills.mockReturnValue([]) - await handoffCodexStep.run(state) - const body = vi.mocked(p.note).mock.calls[0][0] - expect(body).not.toContain('.codex/skills/') - // The durable rules live in AGENTS.md regardless, so the prompt still names it. - expect(body).toContain('AGENTS.md') -}) +beforeEach(() => vi.clearAllMocks()) it('launch prompt names .codex/skills/ when skills were copied', async () => { installSkills.mockReturnValue(['stash-encryption']) + availableSkills.mockReturnValue(['stash-encryption']) await handoffCodexStep.run(state) - expect(vi.mocked(p.note).mock.calls[0][0]).toContain('.codex/skills/') + + expect(promptBody()).toContain('.codex/skills/') + // Skills are on disk, so AGENTS.md stays doctrine-only per Codex guidance. + expect(agentsMdMode()).toBe('doctrine-only') +}) + +// #736: Codex sandboxes deny writes under `.codex/`. The skills cannot land, +// but AGENTS.md (project root) can — so the guidance is inlined there rather +// than lost, and Codex is pointed at it. +describe('when .codex/skills could not be written', () => { + beforeEach(() => { + installSkills.mockReturnValue([]) + availableSkills.mockReturnValue(['stash-encryption', 'stash-cli']) + }) + + it('inlines the skills into AGENTS.md instead of shipping nothing', async () => { + await handoffCodexStep.run(state) + expect(agentsMdMode()).toBe('doctrine-plus-skills') + }) + + it('points the launch prompt at AGENTS.md, not the directory that failed', async () => { + await handoffCodexStep.run(state) + const body = promptBody() + expect(body).not.toContain('.codex/skills/') + expect(body).toContain('inlined in AGENTS.md') + }) + + it('says so, rather than reporting a silent success', async () => { + await handoffCodexStep.run(state) + expect(warnings()).toContain('.codex/skills/') + expect(warnings()).toContain('AGENTS.md') + }) +}) + +// A stripped CLI build ships no skills at all. Nothing to copy AND nothing to +// inline — claiming a fallback here would be a false success of the kind #714 +// and #687 removed elsewhere in init. +describe('when this build ships no skills at all', () => { + beforeEach(() => { + installSkills.mockReturnValue([]) + availableSkills.mockReturnValue([]) + }) + + it('stays doctrine-only — there is nothing to inline', async () => { + await handoffCodexStep.run(state) + expect(agentsMdMode()).toBe('doctrine-only') + }) + + it('drops the skills clause but still names AGENTS.md for the durable rules', async () => { + await handoffCodexStep.run(state) + const body = promptBody() + expect(body).not.toContain('.codex/skills/') + expect(body).not.toContain('inlined in AGENTS.md') + expect(body).toContain('AGENTS.md') + }) + + it('does not claim an inline fallback happened', async () => { + await handoffCodexStep.run(state) + expect(warnings()).not.toContain('inlining') + }) }) diff --git a/packages/cli/src/commands/impl/steps/handoff-codex.ts b/packages/cli/src/commands/impl/steps/handoff-codex.ts index 6ad7278a1..72febebff 100644 --- a/packages/cli/src/commands/impl/steps/handoff-codex.ts +++ b/packages/cli/src/commands/impl/steps/handoff-codex.ts @@ -3,7 +3,10 @@ import { resolve } from 'node:path' import * as p from '@clack/prompts' import { buildAgentsMdBody } from '../../init/lib/build-agents-md.js' import { spawnAgent, writeArtifacts } from '../../init/lib/handoff-helpers.js' -import { installSkills } from '../../init/lib/install-skills.js' +import { + availableSkills, + installSkills, +} from '../../init/lib/install-skills.js' import { upsertManagedBlock } from '../../init/lib/sentinel-upsert.js' import { CONTEXT_REL_PATH, @@ -24,6 +27,19 @@ const CODEX_INSTALL_URL = 'https://github.com/openai/codex' * * AGENTS.md is sentinel-upserted so re-runs replace only our region and * any user content outside it survives. + * + * ## When `.codex/` is not writable + * + * Codex sandboxes deny writes under `.codex/`, and the user cannot fix that + * from here. Rather than hand Codex a project with no guidance, the skill + * bodies are inlined into AGENTS.md — which lives at the project root and is + * writable — using the same `doctrine-plus-skills` path the editor-agent + * handoff uses for Cursor / Windsurf / Cline. + * + * This is why `installSkills` must never throw: it runs FIRST, so an + * exception there used to abort the whole step, taking AGENTS.md and + * `.cipherstash/` down with it. All five Codex runs of the rc.3 skilltester + * matrix landed here (#736). */ export const handoffCodexStep: HandoffStep = { id: 'handoff-codex', @@ -33,14 +49,33 @@ export const handoffCodexStep: HandoffStep = { const integration = state.integration ?? 'postgresql' const installed = installSkills(cwd, CODEX_SKILLS_DIR, integration) + + // Codex sandboxes deny writes under `.codex/`, so the skills copy can fail + // for reasons the user cannot fix (#736 — it took out all five Codex runs + // of the rc.3 matrix). AGENTS.md is written to the project root, which is + // writable, so fall back to inlining the skill bodies there: Codex still + // gets the API guidance, just in one file instead of a directory. + // + // Only when there were skills to install in the first place — a stripped + // build has nothing to inline, and saying otherwise would be a false claim. + const inlinable = installed.length === 0 ? availableSkills(integration) : [] + const useInlineFallback = inlinable.length > 0 + if (installed.length > 0) { p.log.success( `Installed ${installed.length} skill${installed.length !== 1 ? 's' : ''} into ${CODEX_SKILLS_DIR}/: ${installed.join(', ')}`, ) + } else if (useInlineFallback) { + p.log.warn( + `Could not write ${CODEX_SKILLS_DIR}/ — inlining ${inlinable.length} skill${inlinable.length !== 1 ? 's' : ''} into ${AGENTS_MD_REL_PATH} instead: ${inlinable.join(', ')}`, + ) } const agentsMdAbs = resolve(cwd, AGENTS_MD_REL_PATH) - const managed = buildAgentsMdBody(integration, 'doctrine-only') + const managed = buildAgentsMdBody( + integration, + useInlineFallback ? 'doctrine-plus-skills' : 'doctrine-only', + ) const existing = existsSync(agentsMdAbs) ? readFileSync(agentsMdAbs, 'utf-8') : undefined @@ -54,13 +89,17 @@ export const handoffCodexStep: HandoffStep = { writeArtifacts(cwd, state, 'codex', installed) const mode = state.mode ?? 'implement' - // Only reference the skills dir when skills were actually copied (a - // stripped build returns []); the durable rules live in AGENTS.md either - // way, so the prompt stays useful without them. + // Point Codex at wherever the guidance actually ended up: the skills + // directory when it was written, AGENTS.md when it was inlined instead, + // and neither when this build ships no skills at all (claiming otherwise + // sends the agent to read files that do not exist). The durable rules are + // in AGENTS.md in every case, so the prompt stays useful regardless. const skillsClause = installed.length > 0 ? `the skills under ${CODEX_SKILLS_DIR}/ have the API details; ` - : '' + : useInlineFallback + ? `the skill references inlined in ${AGENTS_MD_REL_PATH} have the API details; ` + : '' const launchPrompt = mode === 'plan' ? `Read ${SETUP_PROMPT_REL_PATH} and produce the planning deliverable it describes. AGENTS.md has the durable rules; ${skillsClause}${CONTEXT_REL_PATH} has the project facts. Do not edit code or run mutating commands during this phase.` diff --git a/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts b/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts index 1a1db19ec..ce04e73a8 100644 --- a/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts @@ -1,9 +1,17 @@ -import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' import type { Integration } from '../../types.js' import { + availableSkills, installSkills, readBundledSkill, SKILL_MAP, @@ -105,6 +113,38 @@ describe('installSkills', () => { expect(existsSync(join(tmp, '.claude'))).toBe(false) }) + // #736: a Codex sandbox denies writes under `.codex/`, and the unguarded + // `mkdirSync` threw PAST the per-skill fallback and past the caller — so the + // whole handoff step died and AGENTS.md / .cipherstash were never written. + // Degrading to `[]` is what lets the caller fall back to inlining. + it('returns [] instead of throwing when destDir cannot be created', () => { + // A FILE where the skills directory needs to be: mkdirSync recursive + // fails with ENOTDIR/EEXIST rather than succeeding. + mkdirSync(join(tmp, '.codex'), { recursive: true }) + writeFileSync(join(tmp, '.codex/skills'), 'not a directory', 'utf-8') + + let copied: string[] | undefined + expect(() => { + copied = installSkills(tmp, '.codex/skills', 'drizzle') + }).not.toThrow() + expect(copied).toEqual([]) + }) + + it('leaves the caller able to distinguish "unwritable" from "nothing to install"', () => { + // `availableSkills` reports what the BUNDLE has, independent of whether + // the destination could be written — the signal the Codex handoff uses to + // decide whether an inline fallback is honest. + mkdirSync(join(tmp, '.codex'), { recursive: true }) + writeFileSync(join(tmp, '.codex/skills'), 'not a directory', 'utf-8') + + expect(installSkills(tmp, '.codex/skills', 'drizzle')).toEqual([]) + expect(availableSkills('drizzle')).toEqual([ + 'stash-encryption', + 'stash-drizzle', + 'stash-cli', + ]) + }) + it('is idempotent — re-running does not throw and yields the same result', () => { const first = installSkills(tmp, '.claude/skills', 'postgresql') const second = installSkills(tmp, '.claude/skills', 'postgresql') diff --git a/packages/cli/src/commands/init/lib/install-skills.ts b/packages/cli/src/commands/init/lib/install-skills.ts index 7c16310e7..193b7bc1d 100644 --- a/packages/cli/src/commands/init/lib/install-skills.ts +++ b/packages/cli/src/commands/init/lib/install-skills.ts @@ -34,6 +34,24 @@ export function skillsFor(integration: Integration): readonly string[] { return SKILL_MAP[integration] ?? BASE_SKILLS } +/** + * Which of an integration's skills actually exist in THIS build's bundle. + * + * Separate from {@link installSkills} because callers need to tell "there were + * no skills to install" apart from "there were skills but they could not be + * written" — the two look identical in an empty return, and only the second + * has a fallback worth mentioning to the user. Claiming a fallback happened + * when the bundle was empty would be exactly the kind of false success #714 + * and #687 removed elsewhere in init. + */ +export function availableSkills(integration: Integration): string[] { + const bundledRoot = findBundledDir('skills') + if (!bundledRoot) return [] + return skillsFor(integration).filter((name) => + existsSync(join(bundledRoot, name)), + ) +} + /** * Copy the per-integration set of skills into `///`. * @@ -46,13 +64,18 @@ export function skillsFor(integration: Integration): readonly string[] { * * Idempotent: re-runs overwrite the skill folders so the user always gets * the latest content shipped with this CLI. + * + * **Never throws.** Every filesystem step degrades to a warning and a shorter + * return, because the destination is not always writable: Codex sandboxes deny + * writes under `.codex/`, which took out all five Codex runs of the rc.3 + * skilltester matrix (#736). The caller decides what to do with an empty + * result — the Codex handoff inlines the skills into AGENTS.md instead. */ export function installSkills( cwd: string, destDir: string, integration: Integration, ): string[] { - const skills = skillsFor(integration) const bundledRoot = findBundledDir('skills') if (!bundledRoot) { p.log.warn( @@ -61,11 +84,20 @@ export function installSkills( return [] } - const available = skills.filter((name) => existsSync(join(bundledRoot, name))) + const available = availableSkills(integration) if (available.length === 0) return [] const destRoot = resolve(cwd, destDir) - mkdirSync(destRoot, { recursive: true }) + try { + mkdirSync(destRoot, { recursive: true }) + } catch (err) { + // Previously unguarded, and therefore FATAL: it threw past the per-skill + // fallback below and past the caller, so a sandboxed `.codex/` aborted the + // whole handoff step — no skills, no AGENTS.md, no context.json. + const message = err instanceof Error ? err.message : String(err) + p.log.warn(`Could not create ${destDir}/: ${message}`) + return [] + } const copied: string[] = [] for (const name of available) { diff --git a/packages/wizard/src/lib/install-skills.ts b/packages/wizard/src/lib/install-skills.ts index 463addb5d..87a5de3dd 100644 --- a/packages/wizard/src/lib/install-skills.ts +++ b/packages/wizard/src/lib/install-skills.ts @@ -46,7 +46,19 @@ export async function maybeInstallSkills( if (p.isCancel(confirmed) || !confirmed) return [] const destRoot = resolve(cwd, '.claude', 'skills') - mkdirSync(destRoot, { recursive: true }) + try { + mkdirSync(destRoot, { recursive: true }) + } catch (err) { + // Guarded for the same reason as the copy loop below, which it sat above + // unprotected: an unwritable destination (read-only mount, restrictive + // permissions, a sandbox) threw past every fallback and took the caller + // with it. The `stash` copy of this function hit exactly that on + // `.codex/skills` — see cipherstash/stack#736. Degrade to "no skills" + // instead; the wizard's remaining output is still useful. + const message = err instanceof Error ? err.message : String(err) + p.log.warn(`Could not create ./.claude/skills/: ${message}`) + return [] + } const copied: string[] = [] for (const name of available) { From 3f9e297ed855316db3d39420ea778b122cfe1cca Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Tue, 21 Jul 2026 20:22:20 +1000 Subject: [PATCH 2/4] fix(cli): make the Codex skills fallback deliver what it claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .changeset/codex-skills-eperm.md | 13 +- AGENTS.md | 6 +- .../steps/__tests__/handoff-claude.test.ts | 25 +++- .../steps/__tests__/handoff-codex.test.ts | 119 +++++++++++++----- .../commands/impl/steps/handoff-agents-md.ts | 34 ++--- .../src/commands/impl/steps/handoff-claude.ts | 30 +++-- .../src/commands/impl/steps/handoff-codex.ts | 101 +++++++-------- .../commands/init/doctrine/AGENTS-doctrine.md | 5 +- .../lib/__tests__/build-agents-md.test.ts | 11 ++ .../init/lib/__tests__/install-skills.test.ts | 79 +++++++++--- .../init/lib/__tests__/read-context.test.ts | 1 + .../init/lib/__tests__/setup-prompt.test.ts | 85 +++++++++++-- .../src/commands/init/lib/build-agents-md.ts | 53 +++++--- .../src/commands/init/lib/handoff-helpers.ts | 45 ++++++- .../src/commands/init/lib/install-skills.ts | 72 ++++++++--- .../cli/src/commands/init/lib/setup-prompt.ts | 77 +++++++----- .../src/commands/init/lib/write-context.ts | 34 ++++- .../commands/status/__tests__/status.test.ts | 1 + packages/cli/tsup.config.ts | 13 +- .../src/__tests__/install-skills.test.ts | 78 ++++++++++++ packages/wizard/src/lib/install-skills.ts | 31 +++-- packages/wizard/src/run.ts | 16 ++- 22 files changed, 698 insertions(+), 231 deletions(-) create mode 100644 packages/wizard/src/__tests__/install-skills.test.ts diff --git a/.changeset/codex-skills-eperm.md b/.changeset/codex-skills-eperm.md index 7f3f1963a..d96e48f05 100644 --- a/.changeset/codex-skills-eperm.md +++ b/.changeset/codex-skills-eperm.md @@ -7,11 +7,12 @@ Fix the Codex handoff installing zero skills — and losing `AGENTS.md` and `.ci Codex sandboxes deny writes under `.codex/`. `installSkills` created its destination with an unguarded `mkdirSync`, sitting directly above a per-skill copy loop that *was* 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 it was identified in that report as the primary driver of the Claude→Codex quality gap. -Two changes: +The fix, hardened by a follow-up review of the first cut: -- **`installSkills` never throws.** Every filesystem step now degrades to a warning and a shorter return, so the caller decides what to do with an empty result instead of being skipped entirely. -- **The Codex handoff falls back to inlining.** When the skills cannot be written, their bodies are inlined into `AGENTS.md` — which lives at the project root and is writable — via the same `doctrine-plus-skills` path the editor-agent handoff already uses for Cursor / Windsurf / Cline. Codex still gets the API guidance, in one file instead of a directory, and the launch prompt points at wherever it actually ended up. +- **`installSkills` never throws, and reports what happened.** It returns `{ copied, failed }` instead of a flat list, so callers can tell "unwritable destination" from "stripped build" from "partial copy" without re-deriving it — every filesystem failure degrades to a warning plus a `failed` entry. +- **The Codex handoff inlines exactly the skills that failed.** Whatever could not be copied into `.codex/skills/` — all of them under a sandbox, or a subset after a partial failure — has its body inlined into `AGENTS.md` via the same `doctrine-plus-skills` path the editor-agent handoff uses. The launch prompt points at wherever each skill actually ended up, including both locations after a partial copy. A stripped build that ships no skills stays `doctrine-only` and says nothing. +- **The doctrine now ships where the published CLI can find it.** The bundled AGENTS.md doctrine was copied to `dist/commands/init/doctrine`, but the compiled resolver probes ancestor directories of the chunk in `dist/bin/` — so every published build silently wrote the minimal `AGENTS.md` stub instead of the doctrine (and the inline fallback would have inlined nothing). It now lands at `dist/doctrine`, like the skills bundle. `buildAgentsMdBody` also honours `doctrine-plus-skills` even when the doctrine fragment is missing, so inlined skills are never dropped with it. +- **The generated artifacts describe the fallback honestly.** `context.json` gains an `inlinedSkills` field, and `setup-prompt.md` distinguishes installed / inlined / failed skills instead of mislabelling an unwritable destination as a "stripped build". The Claude handoff now warns when skills exist but could not be installed, and the AGENTS.md handoff records what it inlined. +- **The rest of the handoff is guarded too.** 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. -The fallback is only claimed when there is something to inline: a stripped CLI build that ships no skills at all stays `doctrine-only` and says nothing, rather than reporting a fallback that did not happen. - -`@cipherstash/wizard` carries its own copy of `installSkills` with the same unguarded `mkdirSync` above the same guarded copy loop. It targets `.claude/skills` rather than `.codex/skills`, so the Codex sandbox case does not apply, but an unwritable destination crashed it identically — now guarded the same way. +`@cipherstash/wizard` carries its own copy of `installSkills` with the same unguarded `mkdirSync` above the same guarded copy loop. It targets `.claude/skills` rather than `.codex/skills`, so the Codex sandbox case does not apply, but an unwritable destination crashed it identically — now guarded the same way, with a confirmed-then-failed install recorded in the wizard changelog instead of vanishing with the terminal output. diff --git a/AGENTS.md b/AGENTS.md index 62b8299c7..e58980813 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -104,8 +104,10 @@ sentence in one of them the way you'd treat a wrong line of code: - `installSkills()` (`packages/cli/src/commands/init/lib/install-skills.ts`) copies the per-integration set into the user's `.claude/skills/` or `.codex/skills/` at handoff time. - `readBundledSkill()` inlines a skill's body into the user's `AGENTS.md` for editor - agents (Cursor / Windsurf / Cline). Only `SKILL.md` is inlined — content split into - sibling files is silently dropped on that path, so keep each `SKILL.md` self-sufficient. + agents (Cursor / Windsurf / Cline), and as the Codex fallback for skills that could + not be copied into `.codex/skills/` (#736). Only `SKILL.md` is inlined — content split + into sibling files is silently dropped on that path, so keep each `SKILL.md` + self-sufficient. **Every change to a package's public API, the CLI command surface, or a user-facing workflow must check the affected skills in the same PR.** These skills drift silently: diff --git a/packages/cli/src/commands/impl/steps/__tests__/handoff-claude.test.ts b/packages/cli/src/commands/impl/steps/__tests__/handoff-claude.test.ts index f82103438..d29a7eae8 100644 --- a/packages/cli/src/commands/impl/steps/__tests__/handoff-claude.test.ts +++ b/packages/cli/src/commands/impl/steps/__tests__/handoff-claude.test.ts @@ -25,16 +25,37 @@ import { handoffClaudeStep } from '../handoff-claude.js' // assert on. const state = { integration: 'postgresql' } as unknown as InitState +const warnings = () => vi.mocked(p.log.warn).mock.calls.map(String).join('\n') + beforeEach(() => vi.clearAllMocks()) it('launch prompt omits the skills dir when no skills were copied', async () => { - installSkills.mockReturnValue([]) + installSkills.mockReturnValue({ copied: [], failed: [] }) await handoffClaudeStep.run(state) expect(vi.mocked(p.note).mock.calls[0][0]).not.toContain('.claude/skills/') }) it('launch prompt names the skills dir when skills were copied', async () => { - installSkills.mockReturnValue(['stash-encryption']) + installSkills.mockReturnValue({ copied: ['stash-encryption'], failed: [] }) await handoffClaudeStep.run(state) expect(vi.mocked(p.note).mock.calls[0][0]).toContain('.claude/skills/') }) + +// Unlike the Codex handoff there is no AGENTS.md on this path to inline +// into — a failed install must be announced, not left looking complete +// (#736 follow-up review). +it('warns when skills exist but could not be installed', async () => { + installSkills.mockReturnValue({ + copied: [], + failed: ['stash-encryption', 'stash-cli'], + }) + await handoffClaudeStep.run(state) + expect(warnings()).toContain('could not be installed to .claude/skills/') + expect(warnings()).toContain('stash-encryption, stash-cli') +}) + +it('does not warn when this build simply ships no skills', async () => { + installSkills.mockReturnValue({ copied: [], failed: [] }) + await handoffClaudeStep.run(state) + expect(p.log.warn).not.toHaveBeenCalled() +}) diff --git a/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts b/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts index ec297c0a2..f5fed2b35 100644 --- a/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts +++ b/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts @@ -2,32 +2,24 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type { InitState } from '../../../init/types.js' // Same seam as the handoff-claude test: the launch prompt's skills clause is -// the unit under test. Mock the skill installer to control whether skills were -// "copied". The Codex handoff also writes AGENTS.md to disk before printing -// the prompt, so stub `node:fs` and the AGENTS.md builders to keep the test -// hermetic (no file lands in the repo) — the assertions are on the -// launch-prompt text and the AGENTS.md mode chosen. +// the unit under test. Mock the skill installer to control the copied/failed +// split, and the shared AGENTS.md writer to control whether the inline +// fallback landed. The assertions are on the launch-prompt text, the +// AGENTS.md mode chosen, and the delivery recorded into the artifacts. const installSkills = vi.hoisted(() => vi.fn()) -const availableSkills = vi.hoisted(() => vi.fn()) -vi.mock('../../../init/lib/install-skills.js', () => ({ - installSkills, - availableSkills, -})) +vi.mock('../../../init/lib/install-skills.js', () => ({ installSkills })) +const writeAgentsMd = vi.hoisted(() => vi.fn()) vi.mock('../../../init/lib/handoff-helpers.js', () => ({ + AGENTS_MD_REL_PATH: 'AGENTS.md', + writeAgentsMd, writeArtifacts: vi.fn(), spawnAgent: vi.fn(async () => 0), })) -// Typed with both parameters so `mock.calls[0][1]` (the AGENTS.md mode, which -// is what the fallback actually switches) type-checks. -const buildAgentsMdBody = vi.hoisted(() => - vi.fn((_integration: string, _mode: string) => '# managed doctrine'), -) +const buildAgentsMdBody = vi.hoisted(() => vi.fn()) vi.mock('../../../init/lib/build-agents-md.js', () => ({ buildAgentsMdBody })) -vi.mock('../../../init/lib/sentinel-upsert.js', () => ({ - upsertManagedBlock: vi.fn(() => '# AGENTS.md'), -})) vi.mock('node:fs', () => ({ existsSync: vi.fn(() => false), + mkdirSync: vi.fn(), readFileSync: vi.fn(() => ''), writeFileSync: vi.fn(), })) @@ -37,6 +29,7 @@ vi.mock('@clack/prompts', () => ({ })) import * as p from '@clack/prompts' +import { writeArtifacts } from '../../../init/lib/handoff-helpers.js' import { handoffCodexStep } from '../handoff-codex.js' // `agents` undefined → Codex "not installed" path, which routes the launch @@ -45,32 +38,44 @@ const state = { integration: 'postgresql' } as unknown as InitState const promptBody = () => vi.mocked(p.note).mock.calls[0][0] const agentsMdMode = () => vi.mocked(buildAgentsMdBody).mock.calls[0][1] +const inlinedList = () => vi.mocked(buildAgentsMdBody).mock.calls[0][2] +const delivery = () => vi.mocked(writeArtifacts).mock.calls[0][3] const warnings = () => vi.mocked(p.log.warn).mock.calls.map(String).join('\n') -beforeEach(() => vi.clearAllMocks()) +beforeEach(() => { + vi.clearAllMocks() + writeAgentsMd.mockReturnValue(true) +}) -it('launch prompt names .codex/skills/ when skills were copied', async () => { - installSkills.mockReturnValue(['stash-encryption']) - availableSkills.mockReturnValue(['stash-encryption']) +it('launch prompt names .codex/skills/ when all skills were copied', async () => { + installSkills.mockReturnValue({ copied: ['stash-encryption'], failed: [] }) await handoffCodexStep.run(state) expect(promptBody()).toContain('.codex/skills/') // Skills are on disk, so AGENTS.md stays doctrine-only per Codex guidance. expect(agentsMdMode()).toBe('doctrine-only') + expect(delivery()).toEqual({ + installed: ['stash-encryption'], + inlined: [], + failed: [], + }) }) // #736: Codex sandboxes deny writes under `.codex/`. The skills cannot land, // but AGENTS.md (project root) can — so the guidance is inlined there rather // than lost, and Codex is pointed at it. -describe('when .codex/skills could not be written', () => { +describe('when .codex/skills could not be written at all', () => { beforeEach(() => { - installSkills.mockReturnValue([]) - availableSkills.mockReturnValue(['stash-encryption', 'stash-cli']) + installSkills.mockReturnValue({ + copied: [], + failed: ['stash-encryption', 'stash-cli'], + }) }) - it('inlines the skills into AGENTS.md instead of shipping nothing', async () => { + it('inlines the failed skills into AGENTS.md instead of shipping nothing', async () => { await handoffCodexStep.run(state) expect(agentsMdMode()).toBe('doctrine-plus-skills') + expect(inlinedList()).toEqual(['stash-encryption', 'stash-cli']) }) it('points the launch prompt at AGENTS.md, not the directory that failed', async () => { @@ -85,6 +90,61 @@ describe('when .codex/skills could not be written', () => { expect(warnings()).toContain('.codex/skills/') expect(warnings()).toContain('AGENTS.md') }) + + it('records the skills as inlined, not installed, in the artifacts', async () => { + await handoffCodexStep.run(state) + expect(delivery()).toEqual({ + installed: [], + inlined: ['stash-encryption', 'stash-cli'], + failed: [], + }) + }) + + it('records the skills as failed when AGENTS.md could not be written either', async () => { + writeAgentsMd.mockReturnValue(false) + await handoffCodexStep.run(state) + expect(delivery()).toEqual({ + installed: [], + inlined: [], + failed: ['stash-encryption', 'stash-cli'], + }) + // Nothing was inlined, so the prompt must not claim otherwise. + expect(promptBody()).not.toContain('inlined in AGENTS.md') + }) +}) + +// A partial copy — mkdir succeeded, one skill's cpSync failed — must inline +// exactly the missing skill, not declare success and drop it (#736 follow-up +// review: the fallback used to be all-or-nothing on installed.length === 0). +describe('when only some skills could be written', () => { + beforeEach(() => { + installSkills.mockReturnValue({ + copied: ['stash-encryption'], + failed: ['stash-cli'], + }) + }) + + it('inlines only the failed skill', async () => { + await handoffCodexStep.run(state) + expect(agentsMdMode()).toBe('doctrine-plus-skills') + expect(inlinedList()).toEqual(['stash-cli']) + }) + + it('points the launch prompt at both locations', async () => { + await handoffCodexStep.run(state) + const body = promptBody() + expect(body).toContain('.codex/skills/') + expect(body).toContain('inlined in AGENTS.md') + }) + + it('records the split delivery in the artifacts', async () => { + await handoffCodexStep.run(state) + expect(delivery()).toEqual({ + installed: ['stash-encryption'], + inlined: ['stash-cli'], + failed: [], + }) + }) }) // A stripped CLI build ships no skills at all. Nothing to copy AND nothing to @@ -92,8 +152,7 @@ describe('when .codex/skills could not be written', () => { // and #687 removed elsewhere in init. describe('when this build ships no skills at all', () => { beforeEach(() => { - installSkills.mockReturnValue([]) - availableSkills.mockReturnValue([]) + installSkills.mockReturnValue({ copied: [], failed: [] }) }) it('stays doctrine-only — there is nothing to inline', async () => { @@ -109,8 +168,8 @@ describe('when this build ships no skills at all', () => { expect(body).toContain('AGENTS.md') }) - it('does not claim an inline fallback happened', async () => { + it('does not warn at all — there is no fallback to announce', async () => { await handoffCodexStep.run(state) - expect(warnings()).not.toContain('inlining') + expect(p.log.warn).not.toHaveBeenCalled() }) }) diff --git a/packages/cli/src/commands/impl/steps/handoff-agents-md.ts b/packages/cli/src/commands/impl/steps/handoff-agents-md.ts index 958eadb3f..c0aca3962 100644 --- a/packages/cli/src/commands/impl/steps/handoff-agents-md.ts +++ b/packages/cli/src/commands/impl/steps/handoff-agents-md.ts @@ -1,17 +1,17 @@ -import { existsSync, readFileSync, writeFileSync } from 'node:fs' -import { resolve } from 'node:path' import * as p from '@clack/prompts' import { buildAgentsMdBody } from '../../init/lib/build-agents-md.js' -import { writeArtifacts } from '../../init/lib/handoff-helpers.js' -import { upsertManagedBlock } from '../../init/lib/sentinel-upsert.js' +import { + AGENTS_MD_REL_PATH, + writeAgentsMd, + writeArtifacts, +} from '../../init/lib/handoff-helpers.js' +import { availableSkills } from '../../init/lib/install-skills.js' import { CONTEXT_REL_PATH, SETUP_PROMPT_REL_PATH, } from '../../init/lib/write-context.js' import type { HandoffStep, InitState } from '../../init/types.js' -const AGENTS_MD_REL_PATH = 'AGENTS.md' - /** * Write `AGENTS.md`, `.cipherstash/context.json`, and * `.cipherstash/setup-prompt.md`, then stop. @@ -32,19 +32,19 @@ export const handoffAgentsMdStep: HandoffStep = { const cwd = process.cwd() const integration = state.integration ?? 'postgresql' - const agentsMdAbs = resolve(cwd, AGENTS_MD_REL_PATH) - const managed = buildAgentsMdBody(integration, 'doctrine-plus-skills') - const existing = existsSync(agentsMdAbs) - ? readFileSync(agentsMdAbs, 'utf-8') - : undefined - writeFileSync( - agentsMdAbs, - upsertManagedBlock({ existing, managed }), - 'utf-8', + const inlinable = availableSkills(integration) + const managed = buildAgentsMdBody( + integration, + 'doctrine-plus-skills', + inlinable, ) - p.log.success(`Wrote ${AGENTS_MD_REL_PATH}`) + const written = writeAgentsMd(cwd, managed) - writeArtifacts(cwd, state, 'agents-md', []) + writeArtifacts(cwd, state, 'agents-md', { + installed: [], + inlined: written ? inlinable : [], + failed: written ? [] : inlinable, + }) p.note( [ diff --git a/packages/cli/src/commands/impl/steps/handoff-claude.ts b/packages/cli/src/commands/impl/steps/handoff-claude.ts index 07e1cf980..783a8cede 100644 --- a/packages/cli/src/commands/impl/steps/handoff-claude.ts +++ b/packages/cli/src/commands/impl/steps/handoff-claude.ts @@ -25,21 +25,37 @@ export const handoffClaudeStep: HandoffStep = { const cwd = process.cwd() const integration = state.integration ?? 'postgresql' - const installed = installSkills(cwd, CLAUDE_SKILLS_DIR, integration) - if (installed.length > 0) { + const { copied, failed } = installSkills( + cwd, + CLAUDE_SKILLS_DIR, + integration, + ) + if (copied.length > 0) { p.log.success( - `Installed ${installed.length} skill${installed.length !== 1 ? 's' : ''} into ${CLAUDE_SKILLS_DIR}/: ${installed.join(', ')}`, + `Installed ${copied.length} skill${copied.length !== 1 ? 's' : ''} into ${CLAUDE_SKILLS_DIR}/: ${copied.join(', ')}`, + ) + } + if (failed.length > 0) { + // Unlike the Codex handoff there is no AGENTS.md on this path to + // inline into, so the guidance is genuinely absent — say so rather + // than letting the run look complete (#736 follow-up review). + p.log.warn( + `${failed.length} skill${failed.length !== 1 ? 's' : ''} could not be installed to ${CLAUDE_SKILLS_DIR}/: ${failed.join(', ')}. The agent will run without them — https://cipherstash.com/docs covers the same ground.`, ) } - writeArtifacts(cwd, state, 'claude-code', installed) + writeArtifacts(cwd, state, 'claude-code', { + installed: copied, + inlined: [], + failed, + }) const mode = state.mode ?? 'implement' // Only point the agent at the skills dir when skills were actually copied. - // A stripped CLI build (no bundled skills) returns [] — claiming they're - // there would send the agent to read files that don't exist. + // A stripped CLI build (no bundled skills) copies nothing — claiming + // they're there would send the agent to read files that don't exist. const skillsClause = - installed.length > 0 + copied.length > 0 ? `The installed skills under ${CLAUDE_SKILLS_DIR}/ have the rules; ` : '' const launchPrompt = diff --git a/packages/cli/src/commands/impl/steps/handoff-codex.ts b/packages/cli/src/commands/impl/steps/handoff-codex.ts index 72febebff..29d6bdca9 100644 --- a/packages/cli/src/commands/impl/steps/handoff-codex.ts +++ b/packages/cli/src/commands/impl/steps/handoff-codex.ts @@ -1,20 +1,20 @@ -import { existsSync, readFileSync, writeFileSync } from 'node:fs' +import { existsSync } from 'node:fs' import { resolve } from 'node:path' import * as p from '@clack/prompts' import { buildAgentsMdBody } from '../../init/lib/build-agents-md.js' -import { spawnAgent, writeArtifacts } from '../../init/lib/handoff-helpers.js' import { - availableSkills, - installSkills, -} from '../../init/lib/install-skills.js' -import { upsertManagedBlock } from '../../init/lib/sentinel-upsert.js' + AGENTS_MD_REL_PATH, + spawnAgent, + writeAgentsMd, + writeArtifacts, +} from '../../init/lib/handoff-helpers.js' +import { installSkills } from '../../init/lib/install-skills.js' import { CONTEXT_REL_PATH, SETUP_PROMPT_REL_PATH, } from '../../init/lib/write-context.js' import type { HandoffStep, InitState } from '../../init/types.js' -const AGENTS_MD_REL_PATH = 'AGENTS.md' const CODEX_SKILLS_DIR = '.codex/skills' const CODEX_INSTALL_URL = 'https://github.com/openai/codex' @@ -31,15 +31,17 @@ const CODEX_INSTALL_URL = 'https://github.com/openai/codex' * ## When `.codex/` is not writable * * Codex sandboxes deny writes under `.codex/`, and the user cannot fix that - * from here. Rather than hand Codex a project with no guidance, the skill - * bodies are inlined into AGENTS.md — which lives at the project root and is - * writable — using the same `doctrine-plus-skills` path the editor-agent - * handoff uses for Cursor / Windsurf / Cline. + * from here. Rather than hand Codex a project with no guidance, any skill + * that could not be copied has its body inlined into AGENTS.md — which + * lives at the project root and is writable — using the same + * `doctrine-plus-skills` path the editor-agent handoff uses for Cursor / + * Windsurf / Cline. A partial copy inlines exactly the skills that failed. * * This is why `installSkills` must never throw: it runs FIRST, so an * exception there used to abort the whole step, taking AGENTS.md and * `.cipherstash/` down with it. All five Codex runs of the rc.3 skilltester - * matrix landed here (#736). + * matrix landed here (#736). The AGENTS.md write itself is guarded for the + * same reason (`writeAgentsMd`). */ export const handoffCodexStep: HandoffStep = { id: 'handoff-codex', @@ -48,58 +50,59 @@ export const handoffCodexStep: HandoffStep = { const cwd = process.cwd() const integration = state.integration ?? 'postgresql' - const installed = installSkills(cwd, CODEX_SKILLS_DIR, integration) + const { copied, failed } = installSkills(cwd, CODEX_SKILLS_DIR, integration) - // Codex sandboxes deny writes under `.codex/`, so the skills copy can fail - // for reasons the user cannot fix (#736 — it took out all five Codex runs - // of the rc.3 matrix). AGENTS.md is written to the project root, which is - // writable, so fall back to inlining the skill bodies there: Codex still - // gets the API guidance, just in one file instead of a directory. - // - // Only when there were skills to install in the first place — a stripped - // build has nothing to inline, and saying otherwise would be a false claim. - const inlinable = installed.length === 0 ? availableSkills(integration) : [] - const useInlineFallback = inlinable.length > 0 - - if (installed.length > 0) { + if (copied.length > 0) { p.log.success( - `Installed ${installed.length} skill${installed.length !== 1 ? 's' : ''} into ${CODEX_SKILLS_DIR}/: ${installed.join(', ')}`, + `Installed ${copied.length} skill${copied.length !== 1 ? 's' : ''} into ${CODEX_SKILLS_DIR}/: ${copied.join(', ')}`, ) - } else if (useInlineFallback) { + } + if (failed.length > 0) { + // installSkills already warned with the underlying error per failure; + // this line announces the recovery, not the cause. p.log.warn( - `Could not write ${CODEX_SKILLS_DIR}/ — inlining ${inlinable.length} skill${inlinable.length !== 1 ? 's' : ''} into ${AGENTS_MD_REL_PATH} instead: ${inlinable.join(', ')}`, + `Inlining ${failed.length} skill${failed.length !== 1 ? 's' : ''} that could not be installed to ${CODEX_SKILLS_DIR}/ into ${AGENTS_MD_REL_PATH} instead: ${failed.join(', ')}`, ) + if (copied.length === 0 && existsSync(resolve(cwd, CODEX_SKILLS_DIR))) { + p.log.warn( + `${CODEX_SKILLS_DIR}/ already exists from an earlier run and could not be refreshed — its contents may be stale; the inlined copies in ${AGENTS_MD_REL_PATH} are current.`, + ) + } } - const agentsMdAbs = resolve(cwd, AGENTS_MD_REL_PATH) const managed = buildAgentsMdBody( integration, - useInlineFallback ? 'doctrine-plus-skills' : 'doctrine-only', - ) - const existing = existsSync(agentsMdAbs) - ? readFileSync(agentsMdAbs, 'utf-8') - : undefined - writeFileSync( - agentsMdAbs, - upsertManagedBlock({ existing, managed }), - 'utf-8', + failed.length > 0 ? 'doctrine-plus-skills' : 'doctrine-only', + failed, ) - p.log.success(`Wrote ${AGENTS_MD_REL_PATH}`) + const agentsMdWritten = writeAgentsMd(cwd, managed) + // Skills are only "inlined" if AGENTS.md actually landed — otherwise + // they were delivered nowhere and the artifacts must say so. + const inlined = agentsMdWritten ? failed : [] + const undelivered = agentsMdWritten ? [] : failed - writeArtifacts(cwd, state, 'codex', installed) + writeArtifacts(cwd, state, 'codex', { + installed: copied, + inlined, + failed: undelivered, + }) const mode = state.mode ?? 'implement' // Point Codex at wherever the guidance actually ended up: the skills - // directory when it was written, AGENTS.md when it was inlined instead, - // and neither when this build ships no skills at all (claiming otherwise - // sends the agent to read files that do not exist). The durable rules are - // in AGENTS.md in every case, so the prompt stays useful regardless. + // directory, AGENTS.md, both after a partial copy, and neither when this + // build ships no skills at all (claiming otherwise sends the agent to + // read files that do not exist). The durable rules are in AGENTS.md in + // every case, so the prompt stays useful regardless. + const skillLocations = [ + ...(copied.length > 0 ? [`the skills under ${CODEX_SKILLS_DIR}/`] : []), + ...(inlined.length > 0 + ? [`the skill references inlined in ${AGENTS_MD_REL_PATH}`] + : []), + ] const skillsClause = - installed.length > 0 - ? `the skills under ${CODEX_SKILLS_DIR}/ have the API details; ` - : useInlineFallback - ? `the skill references inlined in ${AGENTS_MD_REL_PATH} have the API details; ` - : '' + skillLocations.length > 0 + ? `${skillLocations.join(' and ')} have the API details; ` + : '' const launchPrompt = mode === 'plan' ? `Read ${SETUP_PROMPT_REL_PATH} and produce the planning deliverable it describes. AGENTS.md has the durable rules; ${skillsClause}${CONTEXT_REL_PATH} has the project facts. Do not edit code or run mutating commands during this phase.` diff --git a/packages/cli/src/commands/init/doctrine/AGENTS-doctrine.md b/packages/cli/src/commands/init/doctrine/AGENTS-doctrine.md index a7e408a57..b89899195 100644 --- a/packages/cli/src/commands/init/doctrine/AGENTS-doctrine.md +++ b/packages/cli/src/commands/init/doctrine/AGENTS-doctrine.md @@ -2,7 +2,7 @@ This project uses [CipherStash](https://cipherstash.com) for searchable, field-level encryption. Plaintext values are encrypted client-side via `@cipherstash/stack` before they leave the application; ciphertext is stored as `jsonb` in Postgres (or as encrypted attributes in DynamoDB) and decrypted on read. -This document is the **durable rule book** for any agent working on this codebase. Read it before touching encryption-related code. Repeatable workflows (setup, schema design, migrations) live in skills installed alongside this file — see "Where to read more" at the bottom. +This document is the **durable rule book** for any agent working on this codebase. Read it before touching encryption-related code. Repeatable workflows (setup, schema design, migrations) live in skills — installed as directories alongside this file, or inlined at the bottom of this file — see "Where to read more" at the bottom. ## What you are working with @@ -41,9 +41,10 @@ Encryption migrations affect production data. Treat every change as **plan → i ## Where to read more -The CipherStash setup skills are installed alongside this file. Use them when you need API details — don't re-invent. Likely paths: +The CipherStash setup skills carry the API details. Use them when you need specifics — don't re-invent. Depending on how setup ran, they are installed as directories or inlined into this file: - `.claude/skills//SKILL.md` (Claude Code) - `.codex/skills//SKILL.md` (Codex) +- Under `## Skill references` at the bottom of this file (editor agents, or when a skills directory could not be written — if the section exists, it is the current copy) Skills relevant to this project depend on the integration. Common ones: `stash-encryption` (encryption API), `stash-cli` (`stash` commands), and one of `stash-drizzle` / `stash-supabase` / `stash-dynamodb` for the chosen ORM. diff --git a/packages/cli/src/commands/init/lib/__tests__/build-agents-md.test.ts b/packages/cli/src/commands/init/lib/__tests__/build-agents-md.test.ts index b87532e13..8bef16d28 100644 --- a/packages/cli/src/commands/init/lib/__tests__/build-agents-md.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/build-agents-md.test.ts @@ -60,4 +60,15 @@ describe('buildAgentsMdBody', () => { expect(out).toContain('# Skill: stash-encryption') expect(out).not.toContain('# Skill: stash-drizzle') }) + + it('inlines only the requested subset when a skill list is passed', () => { + // The Codex fallback passes just the skills that failed to copy, so a + // partial install inlines exactly the missing ones (#736 follow-up). + const out = buildAgentsMdBody('drizzle', 'doctrine-plus-skills', [ + 'stash-cli', + ]) + expect(out).toContain('# Skill: stash-cli') + expect(out).not.toContain('# Skill: stash-encryption') + expect(out).not.toContain('# Skill: stash-drizzle') + }) }) diff --git a/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts b/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts index ce04e73a8..43509ee7d 100644 --- a/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts @@ -1,4 +1,5 @@ import { + chmodSync, existsSync, mkdirSync, mkdtempSync, @@ -8,8 +9,15 @@ import { } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { Integration } from '../../types.js' + +// The module's documented contract is "every filesystem step degrades to a +// warning" — spy on the warn channel so a refactor to a silent +// `catch { return ... }` fails here instead of shipping. +vi.mock('@clack/prompts', () => ({ log: { warn: vi.fn() } })) + +import * as p from '@clack/prompts' import { availableSkills, installSkills, @@ -18,6 +26,8 @@ import { skillsFor, } from '../install-skills.js' +const warnings = () => vi.mocked(p.log.warn).mock.calls.map(String).join('\n') + // Every integration the init flow can resolve to. Kept in lockstep with the // `Integration` union and the init provider registry — a new integration added // without a SKILL_MAP entry crashes the skills install and the AGENTS.md builder @@ -85,6 +95,7 @@ describe('installSkills', () => { let tmp: string beforeEach(() => { + vi.clearAllMocks() tmp = mkdtempSync(join(tmpdir(), 'install-skills-test-')) }) @@ -93,8 +104,9 @@ describe('installSkills', () => { }) it('copies the per-integration skills into destDir', () => { - const copied = installSkills(tmp, '.claude/skills', 'drizzle') + const { copied, failed } = installSkills(tmp, '.claude/skills', 'drizzle') expect(copied).toEqual(['stash-encryption', 'stash-drizzle', 'stash-cli']) + expect(failed).toEqual([]) for (const name of copied) { expect( existsSync(join(tmp, '.claude/skills', name, 'SKILL.md')), @@ -104,7 +116,7 @@ describe('installSkills', () => { }) it('honours the destDir parameter (codex)', () => { - const copied = installSkills(tmp, '.codex/skills', 'supabase') + const { copied } = installSkills(tmp, '.codex/skills', 'supabase') expect(copied).toContain('stash-supabase') expect(existsSync(join(tmp, '.codex/skills/stash-supabase/SKILL.md'))).toBe( true, @@ -116,33 +128,64 @@ describe('installSkills', () => { // #736: a Codex sandbox denies writes under `.codex/`, and the unguarded // `mkdirSync` threw PAST the per-skill fallback and past the caller — so the // whole handoff step died and AGENTS.md / .cipherstash were never written. - // Degrading to `[]` is what lets the caller fall back to inlining. - it('returns [] instead of throwing when destDir cannot be created', () => { + // Degrading to `failed` (with a warning — the documented contract) is what + // lets the caller fall back to inlining exactly those skills. + it('degrades to failed-with-a-warning when destDir cannot be created', () => { // A FILE where the skills directory needs to be: mkdirSync recursive // fails with ENOTDIR/EEXIST rather than succeeding. mkdirSync(join(tmp, '.codex'), { recursive: true }) writeFileSync(join(tmp, '.codex/skills'), 'not a directory', 'utf-8') - let copied: string[] | undefined + let result: ReturnType | undefined expect(() => { - copied = installSkills(tmp, '.codex/skills', 'drizzle') + result = installSkills(tmp, '.codex/skills', 'drizzle') }).not.toThrow() - expect(copied).toEqual([]) - }) + expect(result).toEqual({ + copied: [], + failed: ['stash-encryption', 'stash-drizzle', 'stash-cli'], + }) + expect(warnings()).toContain('Could not create .codex/skills/') + }) + + // A pre-existing read-only skill directory (e.g. left by an earlier run + // under a restrictive sandbox) fails only that skill's copy — the result + // must report the split so the Codex fallback can inline exactly the + // missing ones instead of declaring success. chmod is advisory for root + // and on Windows, so skip there. + it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( + 'reports a partial copy as copied plus failed', + () => { + const blocked = join(tmp, '.codex/skills/stash-drizzle') + mkdirSync(blocked, { recursive: true }) + writeFileSync(join(blocked, 'SKILL.md'), 'stale', 'utf-8') + chmodSync(blocked, 0o555) + + try { + const { copied, failed } = installSkills( + tmp, + '.codex/skills', + 'drizzle', + ) + expect(copied).toEqual(['stash-encryption', 'stash-cli']) + expect(failed).toEqual(['stash-drizzle']) + expect(warnings()).toContain('Failed to install skill stash-drizzle') + } finally { + chmodSync(blocked, 0o755) + } + }, + ) it('leaves the caller able to distinguish "unwritable" from "nothing to install"', () => { - // `availableSkills` reports what the BUNDLE has, independent of whether - // the destination could be written — the signal the Codex handoff uses to - // decide whether an inline fallback is honest. + // An unwritable destination reports the bundled skills as `failed`; + // a stripped build reports both lists empty. The two no longer look + // identical, so the Codex handoff's inline fallback stays honest. mkdirSync(join(tmp, '.codex'), { recursive: true }) writeFileSync(join(tmp, '.codex/skills'), 'not a directory', 'utf-8') - expect(installSkills(tmp, '.codex/skills', 'drizzle')).toEqual([]) - expect(availableSkills('drizzle')).toEqual([ - 'stash-encryption', - 'stash-drizzle', - 'stash-cli', - ]) + const { copied, failed } = installSkills(tmp, '.codex/skills', 'drizzle') + expect(copied).toEqual([]) + expect(failed).toEqual(availableSkills('drizzle')) + expect(failed).toEqual(['stash-encryption', 'stash-drizzle', 'stash-cli']) }) it('is idempotent — re-running does not throw and yields the same result', () => { diff --git a/packages/cli/src/commands/init/lib/__tests__/read-context.test.ts b/packages/cli/src/commands/init/lib/__tests__/read-context.test.ts index eaced0e31..a9d4206bf 100644 --- a/packages/cli/src/commands/init/lib/__tests__/read-context.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/read-context.test.ts @@ -38,6 +38,7 @@ describe('readContextFile', () => { envKeys: [], schemas: [], installedSkills: [], + inlinedSkills: [], generatedAt: '2026-01-01T00:00:00.000Z', }) const ctx = readContextFile(cwd) diff --git a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts index 410cac990..c0b499e04 100644 --- a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts @@ -11,7 +11,11 @@ const baseCtx: SetupPromptContext = { cliInstalled: true, handoff: 'claude-code', mode: 'implement', - installedSkills: ['stash-encryption', 'stash-drizzle', 'stash-cli'], + skills: { + installed: ['stash-encryption', 'stash-drizzle', 'stash-cli'], + inlined: [], + failed: [], + }, } describe('renderSetupPrompt — orient + route (implement mode)', () => { @@ -99,7 +103,11 @@ describe('renderSetupPrompt — orient + route (implement mode)', () => { const out = renderSetupPrompt({ ...baseCtx, integration: 'supabase', - installedSkills: ['stash-encryption', 'stash-supabase', 'stash-cli'], + skills: { + installed: ['stash-encryption', 'stash-supabase', 'stash-cli'], + inlined: [], + failed: [], + }, }) expect(out).toContain('supabase migration new') }) @@ -136,7 +144,16 @@ describe('renderSetupPrompt — orient + route (implement mode)', () => { it('points each handoff at the right rule location', () => { const claude = renderSetupPrompt({ ...baseCtx, handoff: 'claude-code' }) const codex = renderSetupPrompt({ ...baseCtx, handoff: 'codex' }) - const agents = renderSetupPrompt({ ...baseCtx, handoff: 'agents-md' }) + // The agents-md handoff never installs a skills directory — it inlines. + const agents = renderSetupPrompt({ + ...baseCtx, + handoff: 'agents-md', + skills: { + installed: [], + inlined: ['stash-encryption', 'stash-drizzle', 'stash-cli'], + failed: [], + }, + }) expect(claude).toContain('.claude/skills/') expect(codex).toContain('.codex/skills/') @@ -153,7 +170,7 @@ describe('renderSetupPrompt — orient + route (implement mode)', () => { const out = renderSetupPrompt({ ...baseCtx, handoff: 'claude-code', - installedSkills: [], + skills: { installed: [], inlined: [], failed: [] }, }) expect(out).not.toMatch(/the {2,}skill/) // Still describes both flows so the agent can route. @@ -417,7 +434,7 @@ describe('renderSetupPrompt — honours what the handoff actually wrote', () => ...baseCtx, mode, handoff: 'claude-code', - installedSkills: [], + skills: { installed: [], inlined: [], failed: [] }, }) // Nothing was written, so don't send the agent to files that don't exist. expect(out).not.toContain('.claude/skills/') @@ -436,7 +453,7 @@ describe('renderSetupPrompt — honours what the handoff actually wrote', () => ...baseCtx, mode, handoff: 'codex', - installedSkills: [], + skills: { installed: [], inlined: [], failed: [] }, }) expect(out).not.toContain('.codex/skills/') expect(out).toContain('AGENTS.md') @@ -449,10 +466,64 @@ describe('renderSetupPrompt — honours what the handoff actually wrote', () => ...baseCtx, mode, handoff: 'claude-code', - installedSkills: ['stash-encryption'], + skills: { installed: ['stash-encryption'], inlined: [], failed: [] }, }) expect(out).toContain('.claude/skills/') expect(out).not.toContain('AGENTS.md') }) + + // #736: the Codex fallback inlines skills into AGENTS.md when + // `.codex/skills` cannot be written. The prompt must describe that — + // previously it rendered the stripped-build text, contradicting the + // launch prompt. + it(`codex with inlined skills points at AGENTS.md's skill references, not a stripped build (${mode})`, () => { + const out = renderSetupPrompt({ + ...baseCtx, + mode, + handoff: 'codex', + skills: { + installed: [], + inlined: ['stash-encryption', 'stash-cli'], + failed: [], + }, + }) + expect(out).toContain('inlined in `AGENTS.md`') + expect(out).not.toContain('.codex/skills/') + expect(out).not.toContain('stripped build') + // The per-skill purpose index survives the fallback. + expect(out).toContain('`stash-encryption`') + }) + + it(`codex with a partial copy points at both locations (${mode})`, () => { + const out = renderSetupPrompt({ + ...baseCtx, + mode, + handoff: 'codex', + skills: { + installed: ['stash-encryption'], + inlined: ['stash-cli'], + failed: [], + }, + }) + expect(out).toContain('.codex/skills/') + expect(out).toContain('inlined in `AGENTS.md`') + }) + + it(`a failed install is not mislabelled as a stripped build (${mode})`, () => { + const out = renderSetupPrompt({ + ...baseCtx, + mode, + handoff: 'claude-code', + skills: { + installed: [], + inlined: [], + failed: ['stash-encryption', 'stash-cli'], + }, + }) + expect(out).toContain('could not be installed') + expect(out).not.toContain('stripped build') + expect(out).toContain('cipherstash.com/docs') + expect(out).not.toContain('.claude/skills/') + }) } }) diff --git a/packages/cli/src/commands/init/lib/build-agents-md.ts b/packages/cli/src/commands/init/lib/build-agents-md.ts index fcdadf999..3455b28ef 100644 --- a/packages/cli/src/commands/init/lib/build-agents-md.ts +++ b/packages/cli/src/commands/init/lib/build-agents-md.ts @@ -15,33 +15,48 @@ export type AgentsMdMode = 'doctrine-only' | 'doctrine-plus-skills' * file on the second init run. * * doctrine-only — the durable AGENTS.md doctrine file. Used by - * the Codex handoff, where workflows live in - * `.codex/skills/` and AGENTS.md is reserved for - * durable rules per OpenAI's Codex guidance. + * the Codex handoff when the skills landed in + * `.codex/skills/`, where AGENTS.md is reserved + * for durable rules per OpenAI's Codex guidance. * - * doctrine-plus-skills — doctrine + the relevant skill SKILL.md bodies - * inlined under "## Skill references". Used by - * the AGENTS.md handoff for editor agents - * (Cursor / Windsurf / Cline) that don't auto- - * load skill directories. + * doctrine-plus-skills — doctrine + skill SKILL.md bodies inlined under + * "## Skill references". Used by the AGENTS.md + * handoff for editor agents (Cursor / Windsurf / + * Cline) that don't auto-load skill directories, + * and by the Codex handoff as the fallback for + * skills that could not be written to + * `.codex/skills/` (#736). + * + * `skills` names which skills to inline in `doctrine-plus-skills` mode — + * the Codex fallback passes only the skills that failed to copy, so a + * partial install inlines exactly the missing ones. Defaults to the full + * per-integration set. Ignored in `doctrine-only` mode. + * + * A build with no doctrine fragment still honours the mode: the skills are + * inlined under a minimal header rather than silently dropped, so the + * fallback never claims content that isn't there. */ export function buildAgentsMdBody( integration: Integration, mode: AgentsMdMode, + skills: readonly string[] = skillsFor(integration), ): string { const doctrine = readDoctrine() - if (!doctrine) { + const parts: string[] = [] + if (doctrine) { + parts.push(doctrine.trim()) + } else { p.log.warn( 'AGENTS.md doctrine fragment not found in this CLI build — writing a minimal AGENTS.md.', ) - return '# CipherStash\n\nSee `.cipherstash/setup-prompt.md` for the action plan and the installed skills for the rules.' + parts.push( + '# CipherStash\n\nSee `.cipherstash/setup-prompt.md` for the action plan and where the setup rules live.', + ) } - const parts: string[] = [doctrine.trim()] - if (mode === 'doctrine-plus-skills') { const skillBodies: string[] = [] - for (const name of skillsFor(integration)) { + for (const name of skills) { const body = readBundledSkill(name) if (body) { skillBodies.push(`---\n\n# Skill: ${name}\n\n${stripFrontmatter(body)}`) @@ -79,5 +94,15 @@ function readDoctrine(): string | undefined { const dir = findBundledDir('doctrine') if (!dir) return undefined const file = join(dir, 'AGENTS-doctrine.md') - return existsSync(file) ? readFileSync(file, 'utf-8') : undefined + if (!existsSync(file)) return undefined + try { + return readFileSync(file, 'utf-8') + } catch (err) { + // An existing-but-unreadable file must degrade like a missing one — + // this runs before any handoff artifact is written, so a throw here + // recreates the #736 total loss. + const message = err instanceof Error ? err.message : String(err) + p.log.warn(`Could not read the AGENTS.md doctrine fragment: ${message}`) + return undefined + } } diff --git a/packages/cli/src/commands/init/lib/handoff-helpers.ts b/packages/cli/src/commands/init/lib/handoff-helpers.ts index 20bd1ed87..5f09ddd55 100644 --- a/packages/cli/src/commands/init/lib/handoff-helpers.ts +++ b/packages/cli/src/commands/init/lib/handoff-helpers.ts @@ -1,16 +1,23 @@ import { spawn } from 'node:child_process' +import { existsSync, readFileSync, writeFileSync } from 'node:fs' import { resolve } from 'node:path' import * as p from '@clack/prompts' import type { HandoffChoice, InitState } from '../types.js' +import { upsertManagedBlock } from './sentinel-upsert.js' import { buildContextFile, buildSetupPromptContext, CONTEXT_REL_PATH, SETUP_PROMPT_REL_PATH, + type SkillsDelivery, writeContextFile, writeSetupPrompt, } from './write-context.js' +export type { SkillsDelivery } from './write-context.js' + +export const AGENTS_MD_REL_PATH = 'AGENTS.md' + /** * Spawn an interactive CLI agent (`claude` / `codex`) with the launch * prompt as a single argument. `stdio: 'inherit'` so the user sees tool @@ -40,29 +47,55 @@ export function spawnAgent( }) } +/** + * Sentinel-upsert `AGENTS.md` at the project root, degrading to a warning + * on failure. Returns whether the write landed, so callers can report the + * inline fallback honestly (skills are only "inlined" if this succeeded). + * + * Guarded for the same reason `installSkills` never throws: this runs + * before `writeArtifacts`, so an exception here — an unwritable root, or + * `upsertManagedBlock` refusing a malformed sentinel pair — used to abort + * the whole step and take `.cipherstash/` down with it (the #736 blast + * radius, one call further along). + */ +export function writeAgentsMd(cwd: string, managed: string): boolean { + const abs = resolve(cwd, AGENTS_MD_REL_PATH) + try { + const existing = existsSync(abs) ? readFileSync(abs, 'utf-8') : undefined + writeFileSync(abs, upsertManagedBlock({ existing, managed }), 'utf-8') + p.log.success(`Wrote ${AGENTS_MD_REL_PATH}`) + return true + } catch (err) { + const message = err instanceof Error ? err.message : String(err) + p.log.warn(`Could not write ${AGENTS_MD_REL_PATH}: ${message}`) + return false + } +} + /** * Write `.cipherstash/context.json` and `.cipherstash/setup-prompt.md` for * a non-wizard handoff. Shared across the Claude / Codex / AGENTS.md * paths, which all need the same artifacts with handoff-specific values * threaded into the setup prompt. * - * `installedSkills` is the list of skill names the handoff installed (or - * `[]` for the AGENTS.md path that inlines content instead of installing - * a skill directory). + * `skills` records where the skills actually ended up (installed as + * directories, inlined into AGENTS.md, or failed) so the generated prompt + * never mislabels an unwritable destination as a stripped build. */ export function writeArtifacts( cwd: string, state: InitState, handoff: HandoffChoice, - installedSkills: string[], + skills: SkillsDelivery, ): void { const ctx = buildContextFile(state) ctx.envKeys = state.envKeys ?? [] - ctx.installedSkills = installedSkills + ctx.installedSkills = skills.installed + ctx.inlinedSkills = skills.inlined writeContextFile(resolve(cwd, CONTEXT_REL_PATH), ctx) p.log.success(`Wrote ${CONTEXT_REL_PATH}`) - const promptCtx = buildSetupPromptContext(state, handoff, installedSkills) + const promptCtx = buildSetupPromptContext(state, handoff, skills) if (promptCtx) { writeSetupPrompt(resolve(cwd, SETUP_PROMPT_REL_PATH), promptCtx) p.log.success(`Wrote ${SETUP_PROMPT_REL_PATH}`) diff --git a/packages/cli/src/commands/init/lib/install-skills.ts b/packages/cli/src/commands/init/lib/install-skills.ts index 193b7bc1d..e55da02cc 100644 --- a/packages/cli/src/commands/init/lib/install-skills.ts +++ b/packages/cli/src/commands/init/lib/install-skills.ts @@ -37,21 +37,42 @@ export function skillsFor(integration: Integration): readonly string[] { /** * Which of an integration's skills actually exist in THIS build's bundle. * - * Separate from {@link installSkills} because callers need to tell "there were - * no skills to install" apart from "there were skills but they could not be - * written" — the two look identical in an empty return, and only the second - * has a fallback worth mentioning to the user. Claiming a fallback happened - * when the bundle was empty would be exactly the kind of false success #714 - * and #687 removed elsewhere in init. + * A skill counts only when its `SKILL.md` exists — that is the file every + * consumer needs (the skill registries read it, and `readBundledSkill` + * inlines it), so a bundle directory without one must not be promised + * anywhere. Keeping this predicate identical to {@link readBundledSkill}'s + * is what keeps "inlining N skills" claims honest (#714 / #687 removed + * exactly this kind of false success elsewhere in init). */ export function availableSkills(integration: Integration): string[] { const bundledRoot = findBundledDir('skills') if (!bundledRoot) return [] return skillsFor(integration).filter((name) => - existsSync(join(bundledRoot, name)), + existsSync(join(bundledRoot, name, 'SKILL.md')), ) } +/** + * Outcome of {@link installSkills}. `copied` and `failed` partition the + * bundled skill set for the integration: + * + * - both empty → this build ships no skills (nothing to do, + * nothing to fall back to) + * - `failed` non-empty → skills exist but could not all be written + * (unwritable destination, per-skill copy + * failure) — fallback candidates + * + * Callers used to infer these states from a flat `string[]` plus a second + * `availableSkills()` probe, which made "unwritable" indistinguishable from + * "stripped build" for anyone who forgot the probe (#736 follow-up review). + */ +export interface SkillsInstallResult { + /** Skills copied into `///`. */ + copied: string[] + /** Bundled skills that could not be copied. */ + failed: string[] +} + /** * Copy the per-integration set of skills into `///`. * @@ -65,27 +86,27 @@ export function availableSkills(integration: Integration): string[] { * Idempotent: re-runs overwrite the skill folders so the user always gets * the latest content shipped with this CLI. * - * **Never throws.** Every filesystem step degrades to a warning and a shorter - * return, because the destination is not always writable: Codex sandboxes deny + * **Never throws.** Every filesystem step degrades to a warning and a `failed` + * entry, because the destination is not always writable: Codex sandboxes deny * writes under `.codex/`, which took out all five Codex runs of the rc.3 - * skilltester matrix (#736). The caller decides what to do with an empty - * result — the Codex handoff inlines the skills into AGENTS.md instead. + * skilltester matrix (#736). The caller decides what to do with the failures — + * the Codex handoff inlines them into AGENTS.md instead. */ export function installSkills( cwd: string, destDir: string, integration: Integration, -): string[] { +): SkillsInstallResult { const bundledRoot = findBundledDir('skills') if (!bundledRoot) { p.log.warn( 'Skills bundle not found in this CLI build — skipping skills install.', ) - return [] + return { copied: [], failed: [] } } const available = availableSkills(integration) - if (available.length === 0) return [] + if (available.length === 0) return { copied: [], failed: [] } const destRoot = resolve(cwd, destDir) try { @@ -96,10 +117,11 @@ export function installSkills( // whole handoff step — no skills, no AGENTS.md, no context.json. const message = err instanceof Error ? err.message : String(err) p.log.warn(`Could not create ${destDir}/: ${message}`) - return [] + return { copied: [], failed: available } } const copied: string[] = [] + const failed: string[] = [] for (const name of available) { const src = join(bundledRoot, name) const dest = join(destRoot, name) @@ -109,10 +131,11 @@ export function installSkills( } catch (err) { const message = err instanceof Error ? err.message : String(err) p.log.warn(`Failed to install skill ${name}: ${message}`) + failed.push(name) } } - return copied + return { copied, failed } } /** @@ -120,14 +143,23 @@ export function installSkills( * builder when the handoff target is an editor agent (Cursor / Windsurf / * Cline) that doesn't auto-load skill directories — we inline the content. * - * Returns undefined if the bundle isn't found or the named skill isn't part - * of the bundle. Callers should treat that as "skip this skill" rather than - * a fatal error so a stripped CLI build still produces a usable AGENTS.md. + * Returns undefined if the bundle isn't found, the named skill isn't part + * of the bundle, or the file cannot be read (an existing-but-unreadable + * file would otherwise throw through the Codex inline fallback and abort + * the whole handoff — the #736 blast radius this module exists to prevent). + * Callers should treat undefined as "skip this skill" rather than a fatal + * error so a stripped CLI build still produces a usable AGENTS.md. */ export function readBundledSkill(name: string): string | undefined { const bundledRoot = findBundledDir('skills') if (!bundledRoot) return undefined const skillFile = join(bundledRoot, name, 'SKILL.md') if (!existsSync(skillFile)) return undefined - return readFileSync(skillFile, 'utf-8') + try { + return readFileSync(skillFile, 'utf-8') + } catch (err) { + const message = err instanceof Error ? err.message : String(err) + p.log.warn(`Could not read bundled skill ${name}: ${message}`) + return undefined + } } diff --git a/packages/cli/src/commands/init/lib/setup-prompt.ts b/packages/cli/src/commands/init/lib/setup-prompt.ts index 65b64e893..364728eea 100644 --- a/packages/cli/src/commands/init/lib/setup-prompt.ts +++ b/packages/cli/src/commands/init/lib/setup-prompt.ts @@ -1,6 +1,7 @@ import type { HandoffChoice, InitMode, Integration } from '../types.js' import { execCommand, type PackageManager, runnerCommand } from '../utils.js' import type { PlanStep } from './parse-plan.js' +import type { SkillsDelivery } from './write-context.js' export const PLAN_REL_PATH = '.cipherstash/plan.md' @@ -20,12 +21,12 @@ export interface SetupPromptContext { * to produce `.cipherstash/plan.md`; implement-mode is the orient-and- * route action prompt. */ mode: InitMode - /** Names of skills `stash init` copied into the project (e.g. - * `stash-encryption`, `stash-drizzle`, `stash-cli`). The action prompt - * names them so the agent knows which references to consult. Empty for - * the `agents-md` handoff (no skills directory installed) and for - * `wizard` (the wizard installs its own). */ - installedSkills: string[] + /** Where the per-integration skills actually ended up — installed as + * directories, inlined into AGENTS.md, or failed. The action prompt + * names the delivered skills so the agent knows which references to + * consult, and describes failures honestly instead of mislabelling an + * unwritable destination as a stripped build. */ + skills: SkillsDelivery /** In plan mode, which scope of plan to produce. The `stash plan` command * picks this by reading `cs_migrations` (`rollupPlanStep`); the user * override is `--complete-rollout`. Ignored in implement mode. Defaults @@ -76,19 +77,21 @@ function checked(line: string): string { } /** - * Phrase the "where the rules live" pointer for each handoff target. - * - * claude-code → skills loaded into `.claude/skills/` - * codex → AGENTS.md (durable doctrine) + skills in `.codex/skills/` - * agents-md → AGENTS.md only (Cursor / Windsurf / Cline don't load - * skill directories, so the rules are inlined there) - * wizard → handled separately; this prompt isn't written for wizard + * Phrase the "where the rules live" pointer from where the skills actually + * ended up, not from the handoff choice alone — the Codex fallback (#736) + * can inline some or all of them into AGENTS.md instead of `.codex/skills/`. */ -function rulesLocation(handoff: HandoffChoice): string { - if (handoff === 'claude-code') return '`.claude/skills/`' +function rulesLocation(handoff: HandoffChoice, skills: SkillsDelivery): string { + if (handoff === 'agents-md') return '`AGENTS.md` (Cursor / Windsurf / Cline)' + const dir = + handoff === 'claude-code' ? '`.claude/skills/`' : '`.codex/skills/`' + const inlined = 'inlined in `AGENTS.md` under "## Skill references"' + if (skills.installed.length > 0 && skills.inlined.length > 0) + return `${dir} and ${inlined}` + if (skills.inlined.length > 0) return inlined if (handoff === 'codex') return '`.codex/skills/` plus durable rules in `AGENTS.md`' - return '`AGENTS.md` (Cursor / Windsurf / Cline)' + return dir } /** @@ -124,22 +127,36 @@ function renderSkillIndex(installedSkills: string[]): string { } /** - * The "## Skills loaded" section, honouring what the handoff actually wrote — - * so the prompt never points the agent at files that don't exist: + * The "## Skills loaded" section, honouring what the handoff actually + * delivered — so the prompt never points the agent at files that don't + * exist, and never mislabels a failed install as a stripped build: * - * - No skills installed (a stripped CLI build): don't reference any skill - * directory; point only at whatever durable rules the handoff wrote - * (`AGENTS.md` for codex / agents-md; nothing for claude-code, so send the - * agent to the docs). - * - claude-code: the doctrine lives in the installed skills, not `AGENTS.md` - * (this handoff never writes one) — so don't name `AGENTS.md`. + * - Skills delivered (installed, inlined, or both): name them and where + * they actually are. + * - Skills exist but none were delivered: say the install failed and + * point at the docs — the bundle was fine, the destination wasn't. + * - Nothing bundled (a stripped CLI build): point only at whatever + * durable rules the handoff wrote (`AGENTS.md` for codex / agents-md; + * nothing for claude-code, so send the agent to the docs). + * - claude-code: the doctrine lives in the installed skills, not + * `AGENTS.md` (this handoff never writes one) — so don't name it. */ function skillsLoadedLines( handoff: HandoffChoice, - installedSkills: string[], + skills: SkillsDelivery, ): string[] { const wroteAgentsMd = handoff === 'codex' || handoff === 'agents-md' - if (installedSkills.length === 0) { + const delivered = [...skills.installed, ...skills.inlined] + if (delivered.length === 0) { + if (skills.failed.length > 0) { + return [ + '## Rules', + '', + wroteAgentsMd + ? 'The skills could not be installed (the destination was not writable) — the durable rules are in `AGENTS.md`; read it, and consult https://cipherstash.com/docs for the API details the skills would have carried.' + : 'The skills could not be installed (the destination was not writable) — consult https://cipherstash.com/docs for the encryption API, schema rules, and the rollout/cutover lifecycle.', + ] + } return [ '## Rules', '', @@ -154,9 +171,9 @@ function skillsLoadedLines( return [ '## Skills loaded', '', - `Reusable rules and worked examples live in ${rulesLocation(handoff)}:`, + `Reusable rules and worked examples live in ${rulesLocation(handoff, skills)}:`, '', - renderSkillIndex(installedSkills), + renderSkillIndex(delivered), '', doctrine, ] @@ -245,7 +262,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { '', ...setupChecklist(ctx), '', - ...skillsLoadedLines(ctx.handoff, ctx.installedSkills), + ...skillsLoadedLines(ctx.handoff, ctx.skills), '', '## The two options', '', @@ -371,7 +388,7 @@ function planSharedSetupBlock(ctx: SetupPromptContext): string[] { '', ...setupChecklist(ctx), '', - ...skillsLoadedLines(ctx.handoff, ctx.installedSkills), + ...skillsLoadedLines(ctx.handoff, ctx.skills), '', ] } diff --git a/packages/cli/src/commands/init/lib/write-context.ts b/packages/cli/src/commands/init/lib/write-context.ts index da21bf9eb..ba5bd3e95 100644 --- a/packages/cli/src/commands/init/lib/write-context.ts +++ b/packages/cli/src/commands/init/lib/write-context.ts @@ -19,6 +19,26 @@ import { renderSetupPrompt, type SetupPromptContext } from './setup-prompt.js' export const CONTEXT_REL_PATH = '.cipherstash/context.json' export const SETUP_PROMPT_REL_PATH = '.cipherstash/setup-prompt.md' +/** + * How the per-integration skills reached (or failed to reach) the project. + * Threaded into `context.json` and the setup prompt so both describe what + * actually happened, not what the handoff hoped for: + * + * installed — copied into a skills directory (`.claude/skills`, + * `.codex/skills`) + * inlined — bodies written into AGENTS.md under "## Skill references" + * (the editor-agent handoff, and the Codex fallback for an + * unwritable `.codex/` — #736) + * failed — bundled skills that ended up nowhere (destination + * unwritable with no inline fallback, or AGENTS.md itself + * unwritable) + */ +export interface SkillsDelivery { + installed: string[] + inlined: string[] + failed: string[] +} + export interface ContextFile { cliVersion: string integration: Integration @@ -32,9 +52,14 @@ export interface ContextFile { schemas: SchemaDef[] /** Names of skills `stash init` copied into the project (e.g. * `stash-encryption`, `stash-drizzle`, `stash-cli`). Empty for the - * AGENTS.md handoff (skill content is inlined into AGENTS.md instead) - * and for wizard (the wizard installs its own). */ + * AGENTS.md handoff (see `inlinedSkills`) and for wizard (the wizard + * installs its own). */ installedSkills: string[] + /** Names of skills whose SKILL.md bodies were inlined into AGENTS.md + * under "## Skill references" instead of being copied as directories — + * the AGENTS.md handoff always, the Codex handoff when `.codex/skills` + * could not be written (#736). */ + inlinedSkills: string[] /** Plan-step the CLI resolved at handoff time. Written by `stash plan` * so the wizard handoff (and any other reader of this file) can pick * up the same rollout/cutover/complete dispatch the prompt-driven @@ -107,6 +132,7 @@ export function buildContextFile(state: InitState): ContextFile { envKeys: [], schemas: state.schemas ?? [], installedSkills: [], + inlinedSkills: [], planStep: state.planStep, usesProxy: state.usesProxy, generatedAt: new Date().toISOString(), @@ -151,7 +177,7 @@ export function writeBaselineContextFile( export function buildSetupPromptContext( state: InitState, handoff: HandoffChoice, - installedSkills: string[], + skills: SkillsDelivery, ): SetupPromptContext | undefined { if (handoff === 'wizard') return undefined const integration = state.integration ?? 'postgresql' @@ -167,7 +193,7 @@ export function buildSetupPromptContext( cliInstalled: state.cliInstalled ?? false, handoff, mode: state.mode ?? 'implement', - installedSkills, + skills, planStep: state.planStep, } } diff --git a/packages/cli/src/commands/status/__tests__/status.test.ts b/packages/cli/src/commands/status/__tests__/status.test.ts index 7c80690e6..343acc230 100644 --- a/packages/cli/src/commands/status/__tests__/status.test.ts +++ b/packages/cli/src/commands/status/__tests__/status.test.ts @@ -53,6 +53,7 @@ const sampleContext = { { tableName: 'orders', columns: [] }, ], installedSkills: [], + inlinedSkills: [], generatedAt: '2026-01-01T00:00:00.000Z', } diff --git a/packages/cli/tsup.config.ts b/packages/cli/tsup.config.ts index 6b3f3f409..12aa690b8 100644 --- a/packages/cli/tsup.config.ts +++ b/packages/cli/tsup.config.ts @@ -89,10 +89,15 @@ export default defineConfig([ cpSync('../../skills', 'dist/skills', { recursive: true }) } // The AGENTS.md doctrine fragment is read at handoff time and - // wrapped in a sentinel block. The runtime resolver in - // src/commands/init/lib/build-agents-md.ts walks up looking for a - // sibling `doctrine/` dir, so mirror the source layout under dist. - cpSync('src/commands/init/doctrine', 'dist/commands/init/doctrine', { + // wrapped in a sentinel block. The runtime resolver + // (findBundledDir in src/commands/init/lib/bundled-paths.ts) probes + // ANCESTOR directories of the compiled chunk — which lives in + // dist/bin/, not in a mirror of the source layout — so the doctrine + // must land at dist/doctrine, exactly like dist/skills above. + // Mirroring the source path (dist/commands/init/doctrine) is never + // probed and left every published build writing the minimal + // AGENTS.md stub instead of the doctrine. + cpSync('src/commands/init/doctrine', 'dist/doctrine', { recursive: true, }) }, diff --git a/packages/wizard/src/__tests__/install-skills.test.ts b/packages/wizard/src/__tests__/install-skills.test.ts new file mode 100644 index 000000000..051228643 --- /dev/null +++ b/packages/wizard/src/__tests__/install-skills.test.ts @@ -0,0 +1,78 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +// Hermetic: mock the filesystem and prompts so the tests drive the failure +// paths directly. The real-FS behaviour is covered by the `stash` CLI's copy +// of this logic (packages/cli install-skills.test.ts); this file pins the +// wizard copy's never-throw contract, which previously had no coverage — +// exactly how the original unguarded mkdirSync shipped twice (see +// cipherstash/stack#736). +vi.mock('node:fs', () => ({ + existsSync: vi.fn(() => true), + mkdirSync: vi.fn(), + cpSync: vi.fn(), +})) +vi.mock('@clack/prompts', () => ({ + confirm: vi.fn(async () => true), + isCancel: vi.fn(() => false), + log: { warn: vi.fn(), success: vi.fn() }, +})) + +import { cpSync, existsSync, mkdirSync } from 'node:fs' +import * as p from '@clack/prompts' +import { maybeInstallSkills } from '../lib/install-skills.js' + +const warnings = () => vi.mocked(p.log.warn).mock.calls.map(String).join('\n') + +beforeEach(() => { + // resetAllMocks (not clearAllMocks): the throwing mkdirSync implementation + // set in one test must not leak into the next. + vi.resetAllMocks() + vi.mocked(existsSync).mockReturnValue(true) + vi.mocked(p.confirm).mockResolvedValue(true) + vi.mocked(p.isCancel).mockReturnValue(false) +}) + +describe('maybeInstallSkills', () => { + it('degrades to failed (not a throw) when the destination cannot be created', async () => { + vi.mocked(mkdirSync).mockImplementation(() => { + throw Object.assign(new Error('EACCES: permission denied'), { + code: 'EACCES', + }) + }) + + const result = await maybeInstallSkills('/project', 'drizzle') + + expect(result.copied).toEqual([]) + expect(result.failed).toEqual([ + 'stash-encryption', + 'stash-drizzle', + 'stash-cli', + ]) + expect(warnings()).toContain('Could not create ./.claude/skills/') + }) + + it('reports a partial copy as copied plus failed, with a warning per failure', async () => { + vi.mocked(cpSync).mockImplementation((src) => { + if (String(src).includes('stash-drizzle')) { + throw Object.assign(new Error('EACCES: permission denied'), { + code: 'EACCES', + }) + } + }) + + const result = await maybeInstallSkills('/project', 'drizzle') + + expect(result.copied).toEqual(['stash-encryption', 'stash-cli']) + expect(result.failed).toEqual(['stash-drizzle']) + expect(warnings()).toContain('Failed to install skill stash-drizzle') + }) + + it('returns nothing copied and nothing failed when the user declines', async () => { + vi.mocked(p.confirm).mockResolvedValue(false) + + const result = await maybeInstallSkills('/project', 'drizzle') + + expect(result).toEqual({ copied: [], failed: [] }) + expect(mkdirSync).not.toHaveBeenCalled() + }) +}) diff --git a/packages/wizard/src/lib/install-skills.ts b/packages/wizard/src/lib/install-skills.ts index 87a5de3dd..c9bf12856 100644 --- a/packages/wizard/src/lib/install-skills.ts +++ b/packages/wizard/src/lib/install-skills.ts @@ -9,7 +9,7 @@ import type { Integration } from './types.js' * alongside the CLI (see `tsup.config.ts` — `skills/` is copied into * `dist/skills/` at build time). The wizard offers to copy the matching * skills into the user's project so Claude Code picks them up during - * follow-up work (CIP-2992). + * follow-up work. */ const SKILL_MAP: Record = { drizzle: ['stash-encryption', 'stash-drizzle', 'stash-cli'], @@ -18,32 +18,43 @@ const SKILL_MAP: Record = { generic: ['stash-encryption', 'stash-cli'], } +/** + * Outcome of {@link maybeInstallSkills}. `copied` and `failed` partition the + * skills the user confirmed: both empty means declined, nothing bundled, or + * nothing to copy; `failed` non-empty means the user said yes but the + * filesystem said no — the caller should record that, since the transient + * terminal warning is the only other evidence. + */ +export interface SkillsInstallResult { + copied: string[] + failed: string[] +} + /** * Prompt the user, and if they say yes, copy the selected skills into - * `/.claude/skills//`. Returns the list of skill names - * actually copied (empty if declined or nothing to copy). + * `/.claude/skills//`. */ export async function maybeInstallSkills( cwd: string, integration: Integration, -): Promise { +): Promise { const skills = SKILL_MAP[integration] ?? SKILL_MAP.generic const bundledRoot = resolveBundledSkillsRoot() if (!bundledRoot) { p.log.warn( 'Skills bundle not found in this CLI build — skipping skills install.', ) - return [] + return { copied: [], failed: [] } } const available = skills.filter((name) => existsSync(join(bundledRoot, name))) - if (available.length === 0) return [] + if (available.length === 0) return { copied: [], failed: [] } const confirmed = await p.confirm({ message: `Install ${available.length} Claude skill(s) into ./.claude/skills/ (${available.join(', ')})?`, initialValue: true, }) - if (p.isCancel(confirmed) || !confirmed) return [] + if (p.isCancel(confirmed) || !confirmed) return { copied: [], failed: [] } const destRoot = resolve(cwd, '.claude', 'skills') try { @@ -57,10 +68,11 @@ export async function maybeInstallSkills( // instead; the wizard's remaining output is still useful. const message = err instanceof Error ? err.message : String(err) p.log.warn(`Could not create ./.claude/skills/: ${message}`) - return [] + return { copied: [], failed: available } } const copied: string[] = [] + const failed: string[] = [] for (const name of available) { const src = join(bundledRoot, name) const dest = join(destRoot, name) @@ -70,6 +82,7 @@ export async function maybeInstallSkills( } catch (err) { const message = err instanceof Error ? err.message : String(err) p.log.warn(`Failed to install skill ${name}: ${message}`) + failed.push(name) } } @@ -79,7 +92,7 @@ export async function maybeInstallSkills( ) } - return copied + return { copied, failed } } /** diff --git a/packages/wizard/src/run.ts b/packages/wizard/src/run.ts index fab221861..134732138 100644 --- a/packages/wizard/src/run.ts +++ b/packages/wizard/src/run.ts @@ -304,14 +304,22 @@ async function finalize(opts: { startTime: number outro: string }): Promise { - const installedSkills = await maybeInstallSkills( + const { copied, failed } = await maybeInstallSkills( opts.cwd, opts.selectedIntegration, ) - if (installedSkills.length > 0) { + if (copied.length > 0) { opts.changelog.action( - `Installed ${installedSkills.length} Claude skill(s).`, - installedSkills.map((name) => `.claude/skills/${name}`), + `Installed ${copied.length} Claude skill(s).`, + copied.map((name) => `.claude/skills/${name}`), + ) + } + if (failed.length > 0) { + // The user confirmed the install and the filesystem refused — without + // this note the flushed log is indistinguishable from declining, and + // "why does Claude have no skills" has no artifact trail. + opts.changelog.note( + `Skills install failed (destination not writable?): ${failed.join(', ')}.`, ) } From 3520939a54cda901e8b29e4903e3b64835efe306 Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Tue, 21 Jul 2026 21:15:27 +1000 Subject: [PATCH 3/4] test(cli): make the partial-copy fixture deterministic across platforms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../init/lib/__tests__/install-skills.test.ts | 44 +++++++------------ 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts b/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts index 43509ee7d..9672f22ac 100644 --- a/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts @@ -1,5 +1,4 @@ import { - chmodSync, existsSync, mkdirSync, mkdtempSync, @@ -147,33 +146,22 @@ describe('installSkills', () => { expect(warnings()).toContain('Could not create .codex/skills/') }) - // A pre-existing read-only skill directory (e.g. left by an earlier run - // under a restrictive sandbox) fails only that skill's copy — the result - // must report the split so the Codex fallback can inline exactly the - // missing ones instead of declaring success. chmod is advisory for root - // and on Windows, so skip there. - it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( - 'reports a partial copy as copied plus failed', - () => { - const blocked = join(tmp, '.codex/skills/stash-drizzle') - mkdirSync(blocked, { recursive: true }) - writeFileSync(join(blocked, 'SKILL.md'), 'stale', 'utf-8') - chmodSync(blocked, 0o555) - - try { - const { copied, failed } = installSkills( - tmp, - '.codex/skills', - 'drizzle', - ) - expect(copied).toEqual(['stash-encryption', 'stash-cli']) - expect(failed).toEqual(['stash-drizzle']) - expect(warnings()).toContain('Failed to install skill stash-drizzle') - } finally { - chmodSync(blocked, 0o755) - } - }, - ) + // One skill's copy failing must fail only that skill — the result reports + // the split so the Codex fallback can inline exactly the missing ones + // instead of declaring success. A FILE where the skill's directory must go + // makes cpSync throw ERR_FS_CP_DIR_TO_NON_DIR — a type-mismatch error, so + // it triggers on every platform and as root, unlike permission tricks + // (a 0o555 directory blocks nothing on Linux, where cpSync overwrites the + // writable SKILL.md in place — this test's first version failed CI there). + it('reports a partial copy as copied plus failed', () => { + mkdirSync(join(tmp, '.codex/skills'), { recursive: true }) + writeFileSync(join(tmp, '.codex/skills/stash-drizzle'), 'not a dir', 'utf-8') + + const { copied, failed } = installSkills(tmp, '.codex/skills', 'drizzle') + expect(copied).toEqual(['stash-encryption', 'stash-cli']) + expect(failed).toEqual(['stash-drizzle']) + expect(warnings()).toContain('Failed to install skill stash-drizzle') + }) it('leaves the caller able to distinguish "unwritable" from "nothing to install"', () => { // An unwritable destination reports the bundled skills as `failed`; From bd40087a296dfa075d5448632f8e949b008ed66d Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Tue, 21 Jul 2026 21:16:10 +1000 Subject: [PATCH 4/4] style(cli): format the partial-copy fixture line --- .../src/commands/init/lib/__tests__/install-skills.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts b/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts index 9672f22ac..62e76f26e 100644 --- a/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/install-skills.test.ts @@ -155,7 +155,11 @@ describe('installSkills', () => { // writable SKILL.md in place — this test's first version failed CI there). it('reports a partial copy as copied plus failed', () => { mkdirSync(join(tmp, '.codex/skills'), { recursive: true }) - writeFileSync(join(tmp, '.codex/skills/stash-drizzle'), 'not a dir', 'utf-8') + writeFileSync( + join(tmp, '.codex/skills/stash-drizzle'), + 'not a dir', + 'utf-8', + ) const { copied, failed } = installSkills(tmp, '.codex/skills', 'drizzle') expect(copied).toEqual(['stash-encryption', 'stash-cli'])