Skip to content

Commit d6bc9e9

Browse files
authored
fix(cli): report stash plan's real outcome — no 'Plan drafted' without the file (rc.3 M2) (#766)
* fix(cli): report stash plan's real outcome — no 'Plan drafted' without the file (rc.3 M2) `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 * 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`: <reason>" 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. * fix(cli): reject a non-file plan.md; cover revise-and-drafted (review feedback) 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.
1 parent 2aeba94 commit d6bc9e9

9 files changed

Lines changed: 303 additions & 11 deletions

File tree

.changeset/plan-honest-outcome.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'stash': patch
3+
---
4+
5+
`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.

packages/cli/src/commands/impl/steps/handoff-claude.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,6 @@ export const handoffClaudeStep: HandoffStep = {
8585
)
8686
}
8787

88-
return state
88+
return { ...state, agentLaunched: true }
8989
},
9090
}

packages/cli/src/commands/impl/steps/handoff-codex.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,6 @@ export const handoffCodexStep: HandoffStep = {
130130
)
131131
}
132132

133-
return state
133+
return { ...state, agentLaunched: true }
134134
},
135135
}

packages/cli/src/commands/impl/steps/handoff-wizard.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,6 @@ export const handoffWizardStep: HandoffStep = {
4444
)
4545
}
4646

47-
return state
47+
return { ...state, agentLaunched: true }
4848
},
4949
}

packages/cli/src/commands/init/types.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,13 @@ export interface InitState {
9090
agents?: AgentEnvironment
9191
/** What the user picked at the "how to proceed" step. */
9292
handoff?: HandoffChoice
93+
/** True when the handoff step actually launched an agent process
94+
* (`claude` / `codex` / the wizard), regardless of its exit code.
95+
* Deferred handoffs — AGENTS.md, or a CLI target that isn't installed —
96+
* leave it unset. `stash plan` uses this to tell "the agent ran but
97+
* wrote no plan" (an error) from "the plan is written later, when the
98+
* user drives their agent" (#738). */
99+
agentLaunched?: boolean
93100
/** Whether the handoff is producing a plan or executing one. Set by the
94101
* command itself: `stash plan` always sets `'plan'`, `stash impl` always
95102
* sets `'implement'`. */

packages/cli/src/commands/plan/index.ts

Lines changed: 88 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { existsSync } from 'node:fs'
1+
import { type Stats, statSync } from 'node:fs'
22
import { resolve } from 'node:path'
33
import { readManifest } from '@cipherstash/migrate'
44
import * as p from '@clack/prompts'
@@ -25,6 +25,33 @@ import {
2525
import { CancelledError, type InitState } from '../init/types.js'
2626
import { detectPackageManager, runnerCommand } from '../init/utils.js'
2727

28+
/**
29+
* Stat the plan file, returning its `Stats` only when it's a regular FILE and
30+
* treating "absent or not a usable plan" as `undefined`.
31+
*
32+
* `throwIfNoEntry: false` turns the common ENOENT into `undefined`. A non-file
33+
* — most realistically a DIRECTORY at `.cipherstash/plan.md` — also maps to
34+
* `undefined`: `statSync` succeeds for a directory, but the agent cannot have
35+
* written a plan there, so without the `isFile()` gate it would read as a
36+
* pre-existing/unchanged plan and let the command exit 0 against something no
37+
* agent can consume. Any OTHER fs error — ENOTDIR if `.cipherstash` is somehow
38+
* a file, EACCES on a locked path, an ELOOP symlink — is converted into a
39+
* controlled `CliExit(1)` with an actionable message instead of unwinding as a
40+
* generic "Fatal error". This command exists to give automation a reliable
41+
* signal about the plan's state (#738), so a filesystem hiccup must not become
42+
* an opaque crash, and a non-file must not read as success.
43+
*/
44+
function statPlan(planAbs: string): Stats | undefined {
45+
try {
46+
const stats = statSync(planAbs, { throwIfNoEntry: false })
47+
return stats?.isFile() ? stats : undefined
48+
} catch (err) {
49+
const message = err instanceof Error ? err.message : String(err)
50+
p.log.error(`Could not read \`${PLAN_REL_PATH}\`: ${message}`)
51+
throw new CliExit(1)
52+
}
53+
}
54+
2855
function buildStateFromContext(
2956
ctx: ContextFile,
3057
agents: AgentEnvironment,
@@ -195,8 +222,16 @@ export async function planCommand(
195222
// because the complete-rollout confirmation needs it too.
196223
const isInteractive = isInteractiveTty()
197224

225+
const planAbs = resolve(cwd, PLAN_REL_PATH)
226+
198227
try {
199-
if (existsSync(resolve(cwd, PLAN_REL_PATH))) {
228+
// Stat (not just existence) so the post-handoff check can tell a revised
229+
// plan from a pre-existing one the run never touched (#738). Inside the
230+
// try so a filesystem error routes through the same controlled exit as the
231+
// rest of the command rather than bypassing it.
232+
const planBefore = statPlan(planAbs)
233+
234+
if (planBefore) {
200235
p.log.warn(
201236
`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.`,
202237
)
@@ -235,11 +270,58 @@ export async function planCommand(
235270
return
236271
}
237272

238-
await howToProceedStep.run(target ? { ...state, handoff: target } : state)
273+
const handedOff = await howToProceedStep.run(
274+
target ? { ...state, handoff: target } : state,
275+
)
276+
277+
// The plan file is written by the handed-off agent, not by this command,
278+
// so the outcome is only knowable from the disk. Verify before claiming
279+
// anything — the unconditional "Plan drafted" this replaces let a failed
280+
// handoff exit 0 and send `stash impl` after a file that never existed
281+
// (#738).
282+
const planAfter = statPlan(planAbs)
283+
284+
if (!planAfter) {
285+
if (handedOff.agentLaunched) {
286+
// An agent ran and was told to write the plan, but didn't — the
287+
// deliverable is missing. Non-zero, so automation never reads this
288+
// as "a plan exists".
289+
p.log.error(
290+
`${messages.plan.notWritten} to \`${PLAN_REL_PATH}\`. The agent may have been interrupted before saving it — re-run \`${cli} plan\` to try again.`,
291+
)
292+
p.outro('No plan was drafted.')
293+
throw new CliExit(1)
294+
}
295+
// Deferred handoff (AGENTS.md target, or a CLI target that isn't
296+
// installed): the files-and-instructions contract was delivered and
297+
// the plan is written later, when the user drives their agent. That's
298+
// a success for what was runnable — but never claim the plan exists.
299+
p.outro(
300+
`${messages.plan.noPlanYet} — complete the handoff above, then review \`${PLAN_REL_PATH}\` and run \`${cli} impl\` to implement.`,
301+
)
302+
return
303+
}
304+
305+
// A pre-existing plan the run didn't modify is still usable (the agent
306+
// may have judged it current), but "drafted" would be a false claim —
307+
// report which of the two happened.
308+
//
309+
// Heuristic, deliberately not a content hash: an in-place revision that
310+
// preserves BOTH byte size and the mtime tick would misreport as
311+
// "unchanged". Blast radius is a cosmetic wording error — the plan is
312+
// usable either way, and a real agent write bumps mtime (and usually
313+
// size) — so it isn't worth hashing a large file on every run.
314+
const wrote =
315+
!planBefore ||
316+
planAfter.mtimeMs !== planBefore.mtimeMs ||
317+
planAfter.size !== planBefore.size
318+
const planLine = wrote
319+
? `${messages.plan.drafted} \`${PLAN_REL_PATH}\``
320+
: `Plan at \`${PLAN_REL_PATH}\` ${messages.plan.unchanged}`
239321

240322
if (isInteractive) {
241323
const proceed = await p.confirm({
242-
message: `Plan drafted at \`${PLAN_REL_PATH}\`. Continue to \`${cli} impl\` now?`,
324+
message: `${planLine}. Continue to \`${cli} impl\` now?`,
243325
initialValue: true,
244326
})
245327
if (!p.isCancel(proceed) && proceed) {
@@ -248,15 +330,13 @@ export async function planCommand(
248330
await implCommand({}, {})
249331
return
250332
}
251-
p.outro(
252-
`Plan drafted at \`${PLAN_REL_PATH}\`. Review it, then run \`${cli} impl\` to implement.`,
253-
)
333+
p.outro(`${planLine}. Review it, then run \`${cli} impl\` to implement.`)
254334
} else {
255335
// Mirror init's non-TTY hint: the next command will also hit the
256336
// agent-target picker, so name `--target` here rather than letting
257337
// the user re-discover the flag on the next exit-cleanly hint.
258338
p.outro(
259-
`Plan drafted at \`${PLAN_REL_PATH}\`. Review it, then run \`${cli} impl --target <claude-code|codex|agents-md|wizard>\` to implement. The \`--target\` flag is required when running non-interactively.`,
339+
`${planLine}. Review it, then run \`${cli} impl --target <claude-code|codex|agents-md|wizard>\` to implement. The \`--target\` flag is required when running non-interactively.`,
260340
)
261341
}
262342
} catch (err) {

packages/cli/src/messages.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,23 @@ export const messages = {
155155
/** Shown when `--yes` confirms the gate-skip (bypasses the prompt). */
156156
completeRolloutConfirmed:
157157
'Proceeding with --yes: the production-deploy gate is skipped',
158+
/**
159+
* Outcome honesty (#738): the plan file is written by the handed-off
160+
* agent, not by the CLI, so `stash plan` verifies it on disk after the
161+
* handoff and reports what actually happened. These are the stable
162+
* leaders the e2e suite asserts on; the path and next-step hints are
163+
* appended at the call sites.
164+
*/
165+
drafted: 'Plan drafted at',
166+
/** A pre-existing plan the run did not modify — usable, but not "drafted". */
167+
unchanged: 'left unchanged by this run',
168+
/** An agent was launched and told to write the plan, but the file is
169+
* absent. Exits non-zero — automation must not read this as success. */
170+
notWritten: 'The agent handoff finished but no plan was written',
171+
/** Deferred handoff (AGENTS.md target, or a CLI target that isn't
172+
* installed): the plan is written later, when the user drives their
173+
* agent. Exit 0, but never claim the plan exists. */
174+
noPlanYet: 'No plan drafted yet',
158175
},
159176
init: {
160177
/**
Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
import {
2+
chmodSync,
3+
mkdirSync,
4+
mkdtempSync,
5+
rmSync,
6+
symlinkSync,
7+
writeFileSync,
8+
} from 'node:fs'
9+
import { tmpdir } from 'node:os'
10+
import { delimiter, join } from 'node:path'
11+
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
12+
import { messages } from '../../src/messages.js'
13+
import { runPiped } from '../helpers/spawn-piped.js'
14+
15+
/**
16+
* #738: `stash plan` printed `Plan drafted at .cipherstash/plan.md` and exited
17+
* 0 unconditionally. The plan file is written by the handed-off agent, not by
18+
* the CLI, so a failed or deferred handoff produced a false success — and sent
19+
* `stash impl` (and any automation reading the outro) after a file that never
20+
* existed. The command now verifies the file on disk after the handoff and
21+
* reports the outcome that actually occurred.
22+
*
23+
* A fake `claude` binary prepended to PATH drives the "agent launched" paths
24+
* without the real agent — no DB, no network. (The e2e suite runs on POSIX
25+
* only, so a /bin/sh script is fine.)
26+
*/
27+
describe('stash plan — outcome reflects the plan file on disk', () => {
28+
let dir: string
29+
30+
beforeEach(() => {
31+
dir = mkdtempSync(join(tmpdir(), 'plan-outcome-e2e-'))
32+
mkdirSync(join(dir, '.cipherstash'), { recursive: true })
33+
writeFileSync(
34+
join(dir, '.cipherstash', 'context.json'),
35+
JSON.stringify({
36+
integration: 'postgresql',
37+
packageManager: 'npm',
38+
schemas: [],
39+
}),
40+
)
41+
})
42+
afterEach(() => {
43+
rmSync(dir, { recursive: true, force: true })
44+
})
45+
46+
/**
47+
* Write an executable fake `claude` into a bin dir under the fixture and
48+
* return a PATH that resolves it first. `spawnAgent` inherits the CLI's
49+
* cwd, so the script's relative paths land inside the fixture project.
50+
*/
51+
function fakeClaudePath(script: string): string {
52+
const bin = join(dir, 'fake-bin')
53+
mkdirSync(bin, { recursive: true })
54+
const file = join(bin, 'claude')
55+
writeFileSync(file, `#!/bin/sh\n${script}\n`)
56+
chmodSync(file, 0o755)
57+
return `${bin}${delimiter}${process.env.PATH ?? ''}`
58+
}
59+
60+
it('exits 1 when the launched agent writes no plan (no false success)', async () => {
61+
const r = await runPiped(['plan', '--target', 'claude-code'], {
62+
cwd: dir,
63+
env: { PATH: fakeClaudePath('exit 0') },
64+
timeoutMs: 20000,
65+
})
66+
expect(r.timedOut).toBe(false)
67+
expect(r.exitCode).toBe(1)
68+
const out = r.stdout + r.stderr
69+
expect(out).toContain(messages.plan.notWritten)
70+
expect(out).not.toContain(messages.plan.drafted)
71+
})
72+
73+
it('reports "Plan drafted" and exits 0 when the agent wrote the plan', async () => {
74+
const r = await runPiped(['plan', '--target', 'claude-code'], {
75+
cwd: dir,
76+
env: {
77+
PATH: fakeClaudePath('echo "# Plan" > .cipherstash/plan.md'),
78+
},
79+
timeoutMs: 20000,
80+
})
81+
expect(r.timedOut).toBe(false)
82+
expect(r.exitCode).toBe(0)
83+
const out = r.stdout + r.stderr
84+
expect(out).toContain(`${messages.plan.drafted} \`.cipherstash/plan.md\``)
85+
})
86+
87+
it('reports a pre-existing plan as unchanged, not drafted', async () => {
88+
writeFileSync(join(dir, '.cipherstash', 'plan.md'), '# old plan\n')
89+
const r = await runPiped(['plan', '--target', 'claude-code'], {
90+
cwd: dir,
91+
env: { PATH: fakeClaudePath('exit 0') },
92+
timeoutMs: 20000,
93+
})
94+
expect(r.timedOut).toBe(false)
95+
// A usable plan exists on disk, so this is not a failure — but the run
96+
// must not claim it drafted anything.
97+
expect(r.exitCode).toBe(0)
98+
const out = r.stdout + r.stderr
99+
expect(out).toContain(messages.plan.unchanged)
100+
expect(out).not.toContain(messages.plan.drafted)
101+
})
102+
103+
it('reports "drafted" when the agent revises a pre-existing plan', async () => {
104+
writeFileSync(join(dir, '.cipherstash', 'plan.md'), '# old plan\n')
105+
const r = await runPiped(['plan', '--target', 'claude-code'], {
106+
cwd: dir,
107+
// The agent appends — a real revision that changes size (and mtime), so
108+
// the change-detection limb reports "drafted", not "unchanged".
109+
env: { PATH: fakeClaudePath('echo "# revised" >> .cipherstash/plan.md') },
110+
timeoutMs: 20000,
111+
})
112+
expect(r.timedOut).toBe(false)
113+
expect(r.exitCode).toBe(0)
114+
const out = r.stdout + r.stderr
115+
expect(out).toContain(`${messages.plan.drafted} \`.cipherstash/plan.md\``)
116+
expect(out).not.toContain(messages.plan.unchanged)
117+
})
118+
119+
it('treats a plan.md directory as no plan, not a false "unchanged"', async () => {
120+
// `statSync` succeeds for a directory, but no agent can write a plan there.
121+
// Without the isFile() gate this would warn "already exists" and then, with
122+
// an agent that writes nothing, report a false "unchanged" exit 0.
123+
mkdirSync(join(dir, '.cipherstash', 'plan.md'))
124+
const r = await runPiped(['plan', '--target', 'claude-code'], {
125+
cwd: dir,
126+
env: { PATH: fakeClaudePath('exit 0') },
127+
timeoutMs: 20000,
128+
})
129+
expect(r.timedOut).toBe(false)
130+
expect(r.exitCode).toBe(1)
131+
const out = r.stdout + r.stderr
132+
expect(out).toContain(messages.plan.notWritten)
133+
expect(out).not.toContain(messages.plan.unchanged)
134+
expect(out).not.toContain(messages.plan.drafted)
135+
})
136+
137+
it('agents-md handoff says "No plan drafted yet" instead of claiming success', async () => {
138+
const r = await runPiped(['plan', '--target', 'agents-md'], {
139+
cwd: dir,
140+
timeoutMs: 20000,
141+
})
142+
expect(r.timedOut).toBe(false)
143+
// The deferred handoff delivered its files-and-instructions contract, so
144+
// exit 0 — but the plan is written later, by the user's editor agent.
145+
expect(r.exitCode).toBe(0)
146+
const out = r.stdout + r.stderr
147+
expect(out).toContain(messages.plan.noPlanYet)
148+
expect(out).not.toContain(messages.plan.drafted)
149+
})
150+
151+
it('claude-code target without claude on PATH defers honestly', async () => {
152+
const r = await runPiped(['plan', '--target', 'claude-code'], {
153+
cwd: dir,
154+
// A PATH with no `claude` anywhere: the handoff writes files and
155+
// prints install instructions instead of spawning.
156+
env: { PATH: '/usr/bin:/bin' },
157+
timeoutMs: 20000,
158+
})
159+
expect(r.timedOut).toBe(false)
160+
expect(r.exitCode).toBe(0)
161+
const out = r.stdout + r.stderr
162+
expect(out).toContain(messages.plan.noPlanYet)
163+
expect(out).not.toContain(messages.plan.drafted)
164+
})
165+
166+
it('exits 1 with a clear message when the plan path cannot be statted', async () => {
167+
// A self-referential symlink at plan.md makes `statSync` throw ELOOP, not
168+
// ENOENT — a non-ENOENT fs error that must surface as a controlled exit
169+
// (clear message + non-zero), never an opaque "Fatal error" crash.
170+
symlinkSync('plan.md', join(dir, '.cipherstash', 'plan.md'))
171+
const r = await runPiped(['plan', '--target', 'agents-md'], {
172+
cwd: dir,
173+
timeoutMs: 20000,
174+
})
175+
expect(r.timedOut).toBe(false)
176+
expect(r.exitCode).toBe(1)
177+
const out = r.stdout + r.stderr
178+
expect(out).toContain('Could not read')
179+
expect(out).not.toContain(messages.plan.drafted)
180+
})
181+
})

skills/stash-cli/SKILL.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,8 @@ Each column carries `path: "new" | "migrate"`. `stash impl` parses this to rende
263263

264264
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.
265265

266+
**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.
267+
266268
### `impl` — execute
267269

268270
```bash

0 commit comments

Comments
 (0)