diff --git a/.changeset/plan-honest-outcome.md b/.changeset/plan-honest-outcome.md new file mode 100644 index 000000000..b922825c6 --- /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. 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/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..094afd685 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 { 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,33 @@ import { import { CancelledError, type InitState } from '../init/types.js' import { detectPackageManager, runnerCommand } from '../init/utils.js' +/** + * 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`. 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 { + 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}`) + throw new CliExit(1) + } +} + function buildStateFromContext( ctx: ContextFile, agents: AgentEnvironment, @@ -195,8 +222,16 @@ export async function planCommand( // because the complete-rollout confirmation needs it too. const isInteractive = isInteractiveTty() + const planAbs = resolve(cwd, PLAN_REL_PATH) + try { - if (existsSync(resolve(cwd, PLAN_REL_PATH))) { + // 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.`, ) @@ -235,11 +270,58 @@ 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 = statPlan(planAbs) + + 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. + // + // 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 || + 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 +330,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..1ddb47ace --- /dev/null +++ b/packages/cli/tests/e2e/plan-outcome.e2e.test.ts @@ -0,0 +1,181 @@ +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + symlinkSync, + 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('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, + 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) + }) + + 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) + }) +}) 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