From d48421a36d0d27187c42c0fe7dd357fd39ff689d Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Wed, 22 Jul 2026 23:11:23 +1000 Subject: [PATCH 1/3] =?UTF-8?q?fix(cli):=20report=20stash=20plan's=20real?= =?UTF-8?q?=20outcome=20=E2=80=94=20no=20'Plan=20drafted'=20without=20the?= =?UTF-8?q?=20file=20(rc.3=20M2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stash plan` printed `Plan drafted at .cipherstash/plan.md` and exited 0 unconditionally. The plan file is written by the handed-off agent, not by the CLI, so a failed or deferred handoff produced a false success — and sent `stash impl` (and any automation reading the outro) after a file that never existed. Same class of defect as #714's non-interactive init false success; this applies the #687/#714 honesty treatment to `plan`. The command now stats the plan file before and after the handoff and reports what actually happened: - File exists and was written this run → `Plan drafted at …` (unchanged happy path, including the interactive chain into `stash impl`). - An agent was launched (claude / codex / wizard) but the file is absent → error + exit 1, so exit 0 can never mean "a plan exists" when none was drafted. - Deferred handoff (`--target agents-md`, or a CLI target that isn't installed) → honest `No plan drafted yet` outro, exit 0 — the files-and-instructions contract was delivered; the plan lands when the user drives their agent. - Pre-existing plan the run didn't modify → reported as `left unchanged by this run`, not "drafted". The handoff steps now record whether they actually spawned an agent (`InitState.agentLaunched`) so `plan` can tell "the agent ran but wrote no plan" from "the plan is expected later". Outcome leaders live in `messages.plan` per the e2e-string convention; five new pty-less e2e cases drive the built CLI with a fake `claude` on PATH. Closes #738 --- .changeset/plan-honest-outcome.md | 5 + .../src/commands/impl/steps/handoff-claude.ts | 2 +- .../src/commands/impl/steps/handoff-codex.ts | 2 +- .../src/commands/impl/steps/handoff-wizard.ts | 2 +- packages/cli/src/commands/init/types.ts | 7 + packages/cli/src/commands/plan/index.ts | 60 ++++++-- packages/cli/src/messages.ts | 17 +++ .../cli/tests/e2e/plan-outcome.e2e.test.ts | 130 ++++++++++++++++++ skills/stash-cli/SKILL.md | 2 + 9 files changed, 216 insertions(+), 11 deletions(-) create mode 100644 .changeset/plan-honest-outcome.md create mode 100644 packages/cli/tests/e2e/plan-outcome.e2e.test.ts diff --git a/.changeset/plan-honest-outcome.md b/.changeset/plan-honest-outcome.md new file mode 100644 index 000000000..7b30d14d0 --- /dev/null +++ b/.changeset/plan-honest-outcome.md @@ -0,0 +1,5 @@ +--- +'stash': patch +--- + +`stash plan` now reports the outcome that actually occurred instead of unconditionally printing `Plan drafted at .cipherstash/plan.md` and exiting 0 (#738). The plan file is written by the handed-off agent, so the command verifies it on disk after the handoff: "Plan drafted" appears only when the file exists; if a launched agent (Claude Code, Codex, or the wizard) exits without writing it, `plan` errors and exits non-zero so automation never proceeds against a plan that was never created; deferred handoffs (`--target agents-md`, or a CLI target that isn't installed) end with an honest "No plan drafted yet" hint; and a pre-existing plan the run didn't modify is reported as left unchanged rather than drafted. diff --git a/packages/cli/src/commands/impl/steps/handoff-claude.ts b/packages/cli/src/commands/impl/steps/handoff-claude.ts index 783a8cede..1c64d8646 100644 --- a/packages/cli/src/commands/impl/steps/handoff-claude.ts +++ b/packages/cli/src/commands/impl/steps/handoff-claude.ts @@ -85,6 +85,6 @@ export const handoffClaudeStep: HandoffStep = { ) } - return state + return { ...state, agentLaunched: true } }, } diff --git a/packages/cli/src/commands/impl/steps/handoff-codex.ts b/packages/cli/src/commands/impl/steps/handoff-codex.ts index 29d6bdca9..be732d7c8 100644 --- a/packages/cli/src/commands/impl/steps/handoff-codex.ts +++ b/packages/cli/src/commands/impl/steps/handoff-codex.ts @@ -130,6 +130,6 @@ export const handoffCodexStep: HandoffStep = { ) } - return state + return { ...state, agentLaunched: true } }, } diff --git a/packages/cli/src/commands/impl/steps/handoff-wizard.ts b/packages/cli/src/commands/impl/steps/handoff-wizard.ts index 6e1f4dee1..ca8696c6c 100644 --- a/packages/cli/src/commands/impl/steps/handoff-wizard.ts +++ b/packages/cli/src/commands/impl/steps/handoff-wizard.ts @@ -44,6 +44,6 @@ export const handoffWizardStep: HandoffStep = { ) } - return state + return { ...state, agentLaunched: true } }, } diff --git a/packages/cli/src/commands/init/types.ts b/packages/cli/src/commands/init/types.ts index 59b2b89e2..d1317ef0b 100644 --- a/packages/cli/src/commands/init/types.ts +++ b/packages/cli/src/commands/init/types.ts @@ -90,6 +90,13 @@ export interface InitState { agents?: AgentEnvironment /** What the user picked at the "how to proceed" step. */ handoff?: HandoffChoice + /** True when the handoff step actually launched an agent process + * (`claude` / `codex` / the wizard), regardless of its exit code. + * Deferred handoffs — AGENTS.md, or a CLI target that isn't installed — + * leave it unset. `stash plan` uses this to tell "the agent ran but + * wrote no plan" (an error) from "the plan is written later, when the + * user drives their agent" (#738). */ + agentLaunched?: boolean /** Whether the handoff is producing a plan or executing one. Set by the * command itself: `stash plan` always sets `'plan'`, `stash impl` always * sets `'implement'`. */ diff --git a/packages/cli/src/commands/plan/index.ts b/packages/cli/src/commands/plan/index.ts index 5569d5bbd..ded83122e 100644 --- a/packages/cli/src/commands/plan/index.ts +++ b/packages/cli/src/commands/plan/index.ts @@ -1,4 +1,4 @@ -import { existsSync } from 'node:fs' +import { statSync } from 'node:fs' import { resolve } from 'node:path' import { readManifest } from '@cipherstash/migrate' import * as p from '@clack/prompts' @@ -195,8 +195,13 @@ export async function planCommand( // because the complete-rollout confirmation needs it too. const isInteractive = isInteractiveTty() + // Stat (not just existence) so the post-handoff check can tell a revised + // plan from a pre-existing one the run never touched (#738). + const planAbs = resolve(cwd, PLAN_REL_PATH) + const planBefore = statSync(planAbs, { throwIfNoEntry: false }) + try { - if (existsSync(resolve(cwd, PLAN_REL_PATH))) { + if (planBefore) { p.log.warn( `Plan already exists at \`${PLAN_REL_PATH}\`. The agent will be told to revise it; delete the file first if you want to start fresh.`, ) @@ -235,11 +240,52 @@ export async function planCommand( return } - await howToProceedStep.run(target ? { ...state, handoff: target } : state) + const handedOff = await howToProceedStep.run( + target ? { ...state, handoff: target } : state, + ) + + // The plan file is written by the handed-off agent, not by this command, + // so the outcome is only knowable from the disk. Verify before claiming + // anything — the unconditional "Plan drafted" this replaces let a failed + // handoff exit 0 and send `stash impl` after a file that never existed + // (#738). + const planAfter = statSync(planAbs, { throwIfNoEntry: false }) + + if (!planAfter) { + if (handedOff.agentLaunched) { + // An agent ran and was told to write the plan, but didn't — the + // deliverable is missing. Non-zero, so automation never reads this + // as "a plan exists". + p.log.error( + `${messages.plan.notWritten} to \`${PLAN_REL_PATH}\`. The agent may have been interrupted before saving it — re-run \`${cli} plan\` to try again.`, + ) + p.outro('No plan was drafted.') + throw new CliExit(1) + } + // Deferred handoff (AGENTS.md target, or a CLI target that isn't + // installed): the files-and-instructions contract was delivered and + // the plan is written later, when the user drives their agent. That's + // a success for what was runnable — but never claim the plan exists. + p.outro( + `${messages.plan.noPlanYet} — complete the handoff above, then review \`${PLAN_REL_PATH}\` and run \`${cli} impl\` to implement.`, + ) + return + } + + // A pre-existing plan the run didn't modify is still usable (the agent + // may have judged it current), but "drafted" would be a false claim — + // report which of the two happened. + const wrote = + !planBefore || + planAfter.mtimeMs !== planBefore.mtimeMs || + planAfter.size !== planBefore.size + const planLine = wrote + ? `${messages.plan.drafted} \`${PLAN_REL_PATH}\`` + : `Plan at \`${PLAN_REL_PATH}\` ${messages.plan.unchanged}` if (isInteractive) { const proceed = await p.confirm({ - message: `Plan drafted at \`${PLAN_REL_PATH}\`. Continue to \`${cli} impl\` now?`, + message: `${planLine}. Continue to \`${cli} impl\` now?`, initialValue: true, }) if (!p.isCancel(proceed) && proceed) { @@ -248,15 +294,13 @@ export async function planCommand( await implCommand({}, {}) return } - p.outro( - `Plan drafted at \`${PLAN_REL_PATH}\`. Review it, then run \`${cli} impl\` to implement.`, - ) + p.outro(`${planLine}. Review it, then run \`${cli} impl\` to implement.`) } else { // Mirror init's non-TTY hint: the next command will also hit the // agent-target picker, so name `--target` here rather than letting // the user re-discover the flag on the next exit-cleanly hint. p.outro( - `Plan drafted at \`${PLAN_REL_PATH}\`. Review it, then run \`${cli} impl --target \` to implement. The \`--target\` flag is required when running non-interactively.`, + `${planLine}. Review it, then run \`${cli} impl --target \` to implement. The \`--target\` flag is required when running non-interactively.`, ) } } catch (err) { diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index bf927d0fd..38da7141b 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -155,6 +155,23 @@ export const messages = { /** Shown when `--yes` confirms the gate-skip (bypasses the prompt). */ completeRolloutConfirmed: 'Proceeding with --yes: the production-deploy gate is skipped', + /** + * Outcome honesty (#738): the plan file is written by the handed-off + * agent, not by the CLI, so `stash plan` verifies it on disk after the + * handoff and reports what actually happened. These are the stable + * leaders the e2e suite asserts on; the path and next-step hints are + * appended at the call sites. + */ + drafted: 'Plan drafted at', + /** A pre-existing plan the run did not modify — usable, but not "drafted". */ + unchanged: 'left unchanged by this run', + /** An agent was launched and told to write the plan, but the file is + * absent. Exits non-zero — automation must not read this as success. */ + notWritten: 'The agent handoff finished but no plan was written', + /** Deferred handoff (AGENTS.md target, or a CLI target that isn't + * installed): the plan is written later, when the user drives their + * agent. Exit 0, but never claim the plan exists. */ + noPlanYet: 'No plan drafted yet', }, init: { /** diff --git a/packages/cli/tests/e2e/plan-outcome.e2e.test.ts b/packages/cli/tests/e2e/plan-outcome.e2e.test.ts new file mode 100644 index 000000000..f9e616fd4 --- /dev/null +++ b/packages/cli/tests/e2e/plan-outcome.e2e.test.ts @@ -0,0 +1,130 @@ +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from 'node:fs' +import { tmpdir } from 'node:os' +import { delimiter, join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { messages } from '../../src/messages.js' +import { runPiped } from '../helpers/spawn-piped.js' + +/** + * #738: `stash plan` printed `Plan drafted at .cipherstash/plan.md` and exited + * 0 unconditionally. The plan file is written by the handed-off agent, not by + * the CLI, so a failed or deferred handoff produced a false success — and sent + * `stash impl` (and any automation reading the outro) after a file that never + * existed. The command now verifies the file on disk after the handoff and + * reports the outcome that actually occurred. + * + * A fake `claude` binary prepended to PATH drives the "agent launched" paths + * without the real agent — no DB, no network. (The e2e suite runs on POSIX + * only, so a /bin/sh script is fine.) + */ +describe('stash plan — outcome reflects the plan file on disk', () => { + let dir: string + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'plan-outcome-e2e-')) + mkdirSync(join(dir, '.cipherstash'), { recursive: true }) + writeFileSync( + join(dir, '.cipherstash', 'context.json'), + JSON.stringify({ + integration: 'postgresql', + packageManager: 'npm', + schemas: [], + }), + ) + }) + afterEach(() => { + rmSync(dir, { recursive: true, force: true }) + }) + + /** + * Write an executable fake `claude` into a bin dir under the fixture and + * return a PATH that resolves it first. `spawnAgent` inherits the CLI's + * cwd, so the script's relative paths land inside the fixture project. + */ + function fakeClaudePath(script: string): string { + const bin = join(dir, 'fake-bin') + mkdirSync(bin, { recursive: true }) + const file = join(bin, 'claude') + writeFileSync(file, `#!/bin/sh\n${script}\n`) + chmodSync(file, 0o755) + return `${bin}${delimiter}${process.env.PATH ?? ''}` + } + + it('exits 1 when the launched agent writes no plan (no false success)', async () => { + const r = await runPiped(['plan', '--target', 'claude-code'], { + cwd: dir, + env: { PATH: fakeClaudePath('exit 0') }, + timeoutMs: 20000, + }) + expect(r.timedOut).toBe(false) + expect(r.exitCode).toBe(1) + const out = r.stdout + r.stderr + expect(out).toContain(messages.plan.notWritten) + expect(out).not.toContain(messages.plan.drafted) + }) + + it('reports "Plan drafted" and exits 0 when the agent wrote the plan', async () => { + const r = await runPiped(['plan', '--target', 'claude-code'], { + cwd: dir, + env: { + PATH: fakeClaudePath('echo "# Plan" > .cipherstash/plan.md'), + }, + timeoutMs: 20000, + }) + expect(r.timedOut).toBe(false) + expect(r.exitCode).toBe(0) + const out = r.stdout + r.stderr + expect(out).toContain(`${messages.plan.drafted} \`.cipherstash/plan.md\``) + }) + + it('reports a pre-existing plan as unchanged, not drafted', async () => { + writeFileSync(join(dir, '.cipherstash', 'plan.md'), '# old plan\n') + const r = await runPiped(['plan', '--target', 'claude-code'], { + cwd: dir, + env: { PATH: fakeClaudePath('exit 0') }, + timeoutMs: 20000, + }) + expect(r.timedOut).toBe(false) + // A usable plan exists on disk, so this is not a failure — but the run + // must not claim it drafted anything. + expect(r.exitCode).toBe(0) + const out = r.stdout + r.stderr + expect(out).toContain(messages.plan.unchanged) + expect(out).not.toContain(messages.plan.drafted) + }) + + it('agents-md handoff says "No plan drafted yet" instead of claiming success', async () => { + const r = await runPiped(['plan', '--target', 'agents-md'], { + cwd: dir, + timeoutMs: 20000, + }) + expect(r.timedOut).toBe(false) + // The deferred handoff delivered its files-and-instructions contract, so + // exit 0 — but the plan is written later, by the user's editor agent. + expect(r.exitCode).toBe(0) + const out = r.stdout + r.stderr + expect(out).toContain(messages.plan.noPlanYet) + expect(out).not.toContain(messages.plan.drafted) + }) + + it('claude-code target without claude on PATH defers honestly', async () => { + const r = await runPiped(['plan', '--target', 'claude-code'], { + cwd: dir, + // A PATH with no `claude` anywhere: the handoff writes files and + // prints install instructions instead of spawning. + env: { PATH: '/usr/bin:/bin' }, + timeoutMs: 20000, + }) + expect(r.timedOut).toBe(false) + expect(r.exitCode).toBe(0) + const out = r.stdout + r.stderr + expect(out).toContain(messages.plan.noPlanYet) + expect(out).not.toContain(messages.plan.drafted) + }) +}) diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 895239db2..39d03aec6 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -263,6 +263,8 @@ Each column carries `path: "new" | "migrate"`. `stash impl` parses this to rende To re-plan, delete `.cipherstash/plan.md` first — otherwise the agent is told to revise it rather than start fresh. `--complete-rollout` is the escape hatch for databases with no deployed application (local dev, sandboxes, test DBs); it's only safe when nothing in production writes to that database. +**The outro reports what actually happened.** The plan file is written by the handed-off agent, so `plan` verifies it on disk before claiming anything: `Plan drafted at .cipherstash/plan.md` appears only when the file exists after the handoff. If a launched agent exits without writing it, `plan` errors and **exits non-zero**. Deferred handoffs — `--target agents-md`, or a Claude Code / Codex target whose CLI isn't installed — end with `No plan drafted yet` and exit 0: the plan lands later, when you drive the agent yourself. A pre-existing plan the run didn't modify is reported as `left unchanged by this run`, not drafted. Either way, check that `.cipherstash/plan.md` exists before acting on it. + ### `impl` — execute ```bash From c483b3bc78180ab96c748b0386c8aee10f7083d0 Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Thu, 23 Jul 2026 11:34:29 +1000 Subject: [PATCH 2/3] fix(cli): route stash plan's fs stat errors through a controlled exit Copilot review on #766: the pre-handoff `statSync` sat outside the try block and both stat calls can throw for non-ENOENT filesystem errors (ENOTDIR when `.cipherstash` is a file, EACCES on a locked path, ELOOP on a symlink loop), surfacing as a generic "Fatal error" instead of the reliable outcome signal this command exists to give (#738). Wrap both stats in a `statPlan()` helper: `throwIfNoEntry: false` keeps ENOENT as `undefined`, any other fs error becomes a `CliExit(1)` with an actionable "Could not read `.cipherstash/plan.md`: " message. The pre-handoff stat also moves inside the try so it can't bypass the command's error handling. Adds an e2e case that makes plan.md a self-referential symlink (ELOOP) and asserts the controlled non-zero exit + clear message. --- .changeset/plan-honest-outcome.md | 2 +- packages/cli/src/commands/plan/index.ts | 33 ++++++++++++++++--- .../cli/tests/e2e/plan-outcome.e2e.test.ts | 17 ++++++++++ 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/.changeset/plan-honest-outcome.md b/.changeset/plan-honest-outcome.md index 7b30d14d0..b922825c6 100644 --- a/.changeset/plan-honest-outcome.md +++ b/.changeset/plan-honest-outcome.md @@ -2,4 +2,4 @@ 'stash': patch --- -`stash plan` now reports the outcome that actually occurred instead of unconditionally printing `Plan drafted at .cipherstash/plan.md` and exiting 0 (#738). The plan file is written by the handed-off agent, so the command verifies it on disk after the handoff: "Plan drafted" appears only when the file exists; if a launched agent (Claude Code, Codex, or the wizard) exits without writing it, `plan` errors and exits non-zero so automation never proceeds against a plan that was never created; deferred handoffs (`--target agents-md`, or a CLI target that isn't installed) end with an honest "No plan drafted yet" hint; and a pre-existing plan the run didn't modify is reported as left unchanged rather than drafted. +`stash plan` now reports the outcome that actually occurred instead of unconditionally printing `Plan drafted at .cipherstash/plan.md` and exiting 0 (#738). The plan file is written by the handed-off agent, so the command verifies it on disk after the handoff: "Plan drafted" appears only when the file exists; if a launched agent (Claude Code, Codex, or the wizard) exits without writing it, `plan` errors and exits non-zero so automation never proceeds against a plan that was never created; deferred handoffs (`--target agents-md`, or a CLI target that isn't installed) end with an honest "No plan drafted yet" hint; and a pre-existing plan the run didn't modify is reported as left unchanged rather than drafted. An unexpected filesystem error while reading the plan path (a locked or malformed `.cipherstash/`) now exits non-zero with a clear message rather than an opaque crash. diff --git a/packages/cli/src/commands/plan/index.ts b/packages/cli/src/commands/plan/index.ts index ded83122e..f05cd25b1 100644 --- a/packages/cli/src/commands/plan/index.ts +++ b/packages/cli/src/commands/plan/index.ts @@ -1,4 +1,4 @@ -import { statSync } from 'node:fs' +import { type Stats, statSync } from 'node:fs' import { resolve } from 'node:path' import { readManifest } from '@cipherstash/migrate' import * as p from '@clack/prompts' @@ -25,6 +25,26 @@ import { import { CancelledError, type InitState } from '../init/types.js' import { detectPackageManager, runnerCommand } from '../init/utils.js' +/** + * Stat the plan file, treating "absent" as `undefined`. + * + * `throwIfNoEntry: false` turns the common ENOENT into `undefined`. Any OTHER + * fs error — ENOTDIR if `.cipherstash` is somehow a file, EACCES on a locked + * path, an ELOOP symlink — is converted into a controlled `CliExit(1)` with an + * actionable message instead of unwinding as a generic "Fatal error". This + * command exists to give automation a reliable signal about the plan's state + * (#738), so a filesystem hiccup must not become an opaque crash. + */ +function statPlan(planAbs: string): Stats | undefined { + try { + return statSync(planAbs, { throwIfNoEntry: false }) ?? undefined + } catch (err) { + const message = err instanceof Error ? err.message : String(err) + p.log.error(`Could not read \`${PLAN_REL_PATH}\`: ${message}`) + throw new CliExit(1) + } +} + function buildStateFromContext( ctx: ContextFile, agents: AgentEnvironment, @@ -195,12 +215,15 @@ export async function planCommand( // because the complete-rollout confirmation needs it too. const isInteractive = isInteractiveTty() - // Stat (not just existence) so the post-handoff check can tell a revised - // plan from a pre-existing one the run never touched (#738). const planAbs = resolve(cwd, PLAN_REL_PATH) - const planBefore = statSync(planAbs, { throwIfNoEntry: false }) try { + // Stat (not just existence) so the post-handoff check can tell a revised + // plan from a pre-existing one the run never touched (#738). Inside the + // try so a filesystem error routes through the same controlled exit as the + // rest of the command rather than bypassing it. + const planBefore = statPlan(planAbs) + if (planBefore) { p.log.warn( `Plan already exists at \`${PLAN_REL_PATH}\`. The agent will be told to revise it; delete the file first if you want to start fresh.`, @@ -249,7 +272,7 @@ export async function planCommand( // anything — the unconditional "Plan drafted" this replaces let a failed // handoff exit 0 and send `stash impl` after a file that never existed // (#738). - const planAfter = statSync(planAbs, { throwIfNoEntry: false }) + const planAfter = statPlan(planAbs) if (!planAfter) { if (handedOff.agentLaunched) { diff --git a/packages/cli/tests/e2e/plan-outcome.e2e.test.ts b/packages/cli/tests/e2e/plan-outcome.e2e.test.ts index f9e616fd4..ffbb19b64 100644 --- a/packages/cli/tests/e2e/plan-outcome.e2e.test.ts +++ b/packages/cli/tests/e2e/plan-outcome.e2e.test.ts @@ -3,6 +3,7 @@ import { mkdirSync, mkdtempSync, rmSync, + symlinkSync, writeFileSync, } from 'node:fs' import { tmpdir } from 'node:os' @@ -127,4 +128,20 @@ describe('stash plan — outcome reflects the plan file on disk', () => { expect(out).toContain(messages.plan.noPlanYet) expect(out).not.toContain(messages.plan.drafted) }) + + it('exits 1 with a clear message when the plan path cannot be statted', async () => { + // A self-referential symlink at plan.md makes `statSync` throw ELOOP, not + // ENOENT — a non-ENOENT fs error that must surface as a controlled exit + // (clear message + non-zero), never an opaque "Fatal error" crash. + symlinkSync('plan.md', join(dir, '.cipherstash', 'plan.md')) + const r = await runPiped(['plan', '--target', 'agents-md'], { + cwd: dir, + timeoutMs: 20000, + }) + expect(r.timedOut).toBe(false) + expect(r.exitCode).toBe(1) + const out = r.stdout + r.stderr + expect(out).toContain('Could not read') + expect(out).not.toContain(messages.plan.drafted) + }) }) From 5fafc09ac3034490f2530886efcf728434805dbd Mon Sep 17 00:00:00 2001 From: Dan Draper Date: Thu, 23 Jul 2026 15:07:47 +1000 Subject: [PATCH 3/3] fix(cli): reject a non-file plan.md; cover revise-and-drafted (review feedback) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit + reviewer feedback on #766. CodeRabbit (Major): `statSync` succeeds for a DIRECTORY at `.cipherstash/plan.md`, so `statPlan` returned a truthy Stats for it — a plan.md directory read as a pre-existing plan (false "already exists" warning) and, with an agent that wrote nothing, as a false "unchanged" exit 0 against something no agent can consume. `statPlan` now gates on `stats.isFile()`, treating any non-file as absent, so a plan.md directory routes to the "no plan written" branch (exit 1 when an agent ran). New e2e covers it. Reviewer non-blocking notes also addressed: - Documents the change-detection heuristic's limit (an in-place rewrite keeping both byte size and the mtime tick would misreport as "unchanged"; acceptable, not worth hashing a large file per run). - Adds the missing e2e for pre-existing -> agent-revises -> "drafted", which is the case that specifically exercises the mtime/size change-detection limb. Full CLI unit suite (822) and plan e2e (8) green; code:check clean. --- packages/cli/src/commands/plan/index.ts | 29 +++++++++++----- .../cli/tests/e2e/plan-outcome.e2e.test.ts | 34 +++++++++++++++++++ 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/commands/plan/index.ts b/packages/cli/src/commands/plan/index.ts index f05cd25b1..094afd685 100644 --- a/packages/cli/src/commands/plan/index.ts +++ b/packages/cli/src/commands/plan/index.ts @@ -26,18 +26,25 @@ import { CancelledError, type InitState } from '../init/types.js' import { detectPackageManager, runnerCommand } from '../init/utils.js' /** - * Stat the plan file, treating "absent" as `undefined`. + * Stat the plan file, returning its `Stats` only when it's a regular FILE and + * treating "absent or not a usable plan" as `undefined`. * - * `throwIfNoEntry: false` turns the common ENOENT into `undefined`. Any OTHER - * fs error — ENOTDIR if `.cipherstash` is somehow a file, EACCES on a locked - * path, an ELOOP symlink — is converted into a controlled `CliExit(1)` with an - * actionable message instead of unwinding as a generic "Fatal error". This - * command exists to give automation a reliable signal about the plan's state - * (#738), so a filesystem hiccup must not become an opaque crash. + * `throwIfNoEntry: false` turns the common ENOENT into `undefined`. A non-file + * — most realistically a DIRECTORY at `.cipherstash/plan.md` — also maps to + * `undefined`: `statSync` succeeds for a directory, but the agent cannot have + * written a plan there, so without the `isFile()` gate it would read as a + * pre-existing/unchanged plan and let the command exit 0 against something no + * agent can consume. Any OTHER fs error — ENOTDIR if `.cipherstash` is somehow + * a file, EACCES on a locked path, an ELOOP symlink — is converted into a + * controlled `CliExit(1)` with an actionable message instead of unwinding as a + * generic "Fatal error". This command exists to give automation a reliable + * signal about the plan's state (#738), so a filesystem hiccup must not become + * an opaque crash, and a non-file must not read as success. */ function statPlan(planAbs: string): Stats | undefined { try { - return statSync(planAbs, { throwIfNoEntry: false }) ?? undefined + const stats = statSync(planAbs, { throwIfNoEntry: false }) + return stats?.isFile() ? stats : undefined } catch (err) { const message = err instanceof Error ? err.message : String(err) p.log.error(`Could not read \`${PLAN_REL_PATH}\`: ${message}`) @@ -298,6 +305,12 @@ export async function planCommand( // A pre-existing plan the run didn't modify is still usable (the agent // may have judged it current), but "drafted" would be a false claim — // report which of the two happened. + // + // Heuristic, deliberately not a content hash: an in-place revision that + // preserves BOTH byte size and the mtime tick would misreport as + // "unchanged". Blast radius is a cosmetic wording error — the plan is + // usable either way, and a real agent write bumps mtime (and usually + // size) — so it isn't worth hashing a large file on every run. const wrote = !planBefore || planAfter.mtimeMs !== planBefore.mtimeMs || diff --git a/packages/cli/tests/e2e/plan-outcome.e2e.test.ts b/packages/cli/tests/e2e/plan-outcome.e2e.test.ts index ffbb19b64..1ddb47ace 100644 --- a/packages/cli/tests/e2e/plan-outcome.e2e.test.ts +++ b/packages/cli/tests/e2e/plan-outcome.e2e.test.ts @@ -100,6 +100,40 @@ describe('stash plan — outcome reflects the plan file on disk', () => { expect(out).not.toContain(messages.plan.drafted) }) + it('reports "drafted" when the agent revises a pre-existing plan', async () => { + writeFileSync(join(dir, '.cipherstash', 'plan.md'), '# old plan\n') + const r = await runPiped(['plan', '--target', 'claude-code'], { + cwd: dir, + // The agent appends — a real revision that changes size (and mtime), so + // the change-detection limb reports "drafted", not "unchanged". + env: { PATH: fakeClaudePath('echo "# revised" >> .cipherstash/plan.md') }, + timeoutMs: 20000, + }) + expect(r.timedOut).toBe(false) + expect(r.exitCode).toBe(0) + const out = r.stdout + r.stderr + expect(out).toContain(`${messages.plan.drafted} \`.cipherstash/plan.md\``) + expect(out).not.toContain(messages.plan.unchanged) + }) + + it('treats a plan.md directory as no plan, not a false "unchanged"', async () => { + // `statSync` succeeds for a directory, but no agent can write a plan there. + // Without the isFile() gate this would warn "already exists" and then, with + // an agent that writes nothing, report a false "unchanged" exit 0. + mkdirSync(join(dir, '.cipherstash', 'plan.md')) + const r = await runPiped(['plan', '--target', 'claude-code'], { + cwd: dir, + env: { PATH: fakeClaudePath('exit 0') }, + timeoutMs: 20000, + }) + expect(r.timedOut).toBe(false) + expect(r.exitCode).toBe(1) + const out = r.stdout + r.stderr + expect(out).toContain(messages.plan.notWritten) + expect(out).not.toContain(messages.plan.unchanged) + expect(out).not.toContain(messages.plan.drafted) + }) + it('agents-md handoff says "No plan drafted yet" instead of claiming success', async () => { const r = await runPiped(['plan', '--target', 'agents-md'], { cwd: dir,