diff --git a/.changeset/codex-skills-eperm.md b/.changeset/codex-skills-eperm.md new file mode 100644 index 000000000..d96e48f05 --- /dev/null +++ b/.changeset/codex-skills-eperm.md @@ -0,0 +1,18 @@ +--- +'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. + +The fix, hardened by a follow-up review of the first cut: + +- **`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. + +`@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 b095fa981..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 @@ -1,26 +1,25 @@ -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. +// 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()) 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), })) -vi.mock('../../../init/lib/build-agents-md.js', () => ({ - buildAgentsMdBody: vi.fn(() => '# managed doctrine'), -})) -vi.mock('../../../init/lib/sentinel-upsert.js', () => ({ - upsertManagedBlock: vi.fn(() => '# AGENTS.md'), -})) +const buildAgentsMdBody = vi.hoisted(() => vi.fn()) +vi.mock('../../../init/lib/build-agents-md.js', () => ({ buildAgentsMdBody })) vi.mock('node:fs', () => ({ existsSync: vi.fn(() => false), + mkdirSync: vi.fn(), readFileSync: vi.fn(() => ''), writeFileSync: vi.fn(), })) @@ -30,25 +29,147 @@ 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 // 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 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') -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() + writeAgentsMd.mockReturnValue(true) }) -it('launch prompt names .codex/skills/ when skills were copied', async () => { - installSkills.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(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') + 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 at all', () => { + beforeEach(() => { + installSkills.mockReturnValue({ + copied: [], + failed: ['stash-encryption', 'stash-cli'], + }) + }) + + 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 () => { + 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') + }) + + 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 +// 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({ copied: [], failed: [] }) + }) + + 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 warn at all — there is no fallback to announce', async () => { + await handoffCodexStep.run(state) + 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 6ad7278a1..29d6bdca9 100644 --- a/packages/cli/src/commands/impl/steps/handoff-codex.ts +++ b/packages/cli/src/commands/impl/steps/handoff-codex.ts @@ -1,17 +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 { + AGENTS_MD_REL_PATH, + spawnAgent, + writeAgentsMd, + writeArtifacts, +} from '../../init/lib/handoff-helpers.js' import { installSkills } from '../../init/lib/install-skills.js' -import { upsertManagedBlock } from '../../init/lib/sentinel-upsert.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' @@ -24,6 +27,21 @@ 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, 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). The AGENTS.md write itself is guarded for the + * same reason (`writeAgentsMd`). */ export const handoffCodexStep: HandoffStep = { id: 'handoff-codex', @@ -32,34 +50,58 @@ export const handoffCodexStep: HandoffStep = { const cwd = process.cwd() const integration = state.integration ?? 'postgresql' - const installed = installSkills(cwd, CODEX_SKILLS_DIR, integration) - if (installed.length > 0) { + const { copied, failed } = installSkills(cwd, CODEX_SKILLS_DIR, integration) + + 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(', ')}`, + ) + } + if (failed.length > 0) { + // installSkills already warned with the underlying error per failure; + // this line announces the recovery, not the cause. + p.log.warn( + `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, 'doctrine-only') - const existing = existsSync(agentsMdAbs) - ? readFileSync(agentsMdAbs, 'utf-8') - : undefined - writeFileSync( - agentsMdAbs, - upsertManagedBlock({ existing, managed }), - 'utf-8', + const managed = buildAgentsMdBody( + integration, + 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' - // 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, 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; ` + skillLocations.length > 0 + ? `${skillLocations.join(' and ')} have the API details; ` : '' const launchPrompt = mode === 'plan' 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 1a1db19ec..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 @@ -1,15 +1,32 @@ -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 { 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, readBundledSkill, SKILL_MAP, 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 @@ -77,6 +94,7 @@ describe('installSkills', () => { let tmp: string beforeEach(() => { + vi.clearAllMocks() tmp = mkdtempSync(join(tmpdir(), 'install-skills-test-')) }) @@ -85,8 +103,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')), @@ -96,7 +115,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, @@ -105,6 +124,62 @@ 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 `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 result: ReturnType | undefined + expect(() => { + result = installSkills(tmp, '.codex/skills', 'drizzle') + }).not.toThrow() + expect(result).toEqual({ + copied: [], + failed: ['stash-encryption', 'stash-drizzle', 'stash-cli'], + }) + expect(warnings()).toContain('Could not create .codex/skills/') + }) + + // 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`; + // 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') + + 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', () => { const first = installSkills(tmp, '.claude/skills', 'postgresql') const second = installSkills(tmp, '.claude/skills', 'postgresql') 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 7c16310e7..e55da02cc 100644 --- a/packages/cli/src/commands/init/lib/install-skills.ts +++ b/packages/cli/src/commands/init/lib/install-skills.ts @@ -34,6 +34,45 @@ 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. + * + * 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, '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 `///`. * @@ -46,28 +85,43 @@ 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 `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 the failures — + * the Codex handoff inlines them into AGENTS.md instead. */ export function installSkills( cwd: string, destDir: string, integration: Integration, -): string[] { - const skills = skillsFor(integration) +): 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 = skills.filter((name) => existsSync(join(bundledRoot, name))) - if (available.length === 0) return [] + const available = availableSkills(integration) + if (available.length === 0) return { copied: [], failed: [] } 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 { copied: [], failed: available } + } const copied: string[] = [] + const failed: string[] = [] for (const name of available) { const src = join(bundledRoot, name) const dest = join(destRoot, name) @@ -77,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 } } /** @@ -88,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 463addb5d..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,37 +18,61 @@ 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') - 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 { copied: [], failed: available } + } const copied: string[] = [] + const failed: string[] = [] for (const name of available) { const src = join(bundledRoot, name) const dest = join(destRoot, name) @@ -58,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) } } @@ -67,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(', ')}.`, ) }