fix(cli): report stash plan's real outcome — no 'Plan drafted' without the file (rc.3 M2)#766
Conversation
…t 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
🦋 Changeset detectedLatest commit: 5fafc09 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthrough
ChangesPlan outcome verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant StashPlan
participant Handoff
participant Agent
participant PlanFile
User->>StashPlan: run stash plan
StashPlan->>PlanFile: stat before handoff
StashPlan->>Handoff: run selected target
Handoff->>Agent: launch when available
Agent->>PlanFile: write plan.md
StashPlan->>PlanFile: stat after handoff
StashPlan-->>User: report actual plan outcome
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness gap in the stash CLI where stash plan could claim success (“Plan drafted…”) and exit 0 even when .cipherstash/plan.md was never created, by verifying the plan file on disk after the handoff and reporting the real outcome.
Changes:
- Add post-handoff plan file verification in
stash plan, including distinguishing “drafted”, “unchanged”, “deferred”, and “agent ran but wrote nothing” outcomes. - Thread a new optional
InitState.agentLaunchedsignal from handoff steps soplancan tell deferred handoffs from failed “agent wrote nothing” runs. - Add e2e coverage + stable message leaders for the new outcome contract; document the contract in the shipped
stash-cliskill and add a patch changeset.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/stash-cli/SKILL.md | Documents the new “outro reflects real outcome” contract for stash plan. |
| packages/cli/tests/e2e/plan-outcome.e2e.test.ts | Adds piped-stdio e2e cases covering drafted/unchanged/deferred/error outcomes. |
| packages/cli/src/messages.ts | Centralizes stable messages.plan.* leaders asserted by e2e tests. |
| packages/cli/src/commands/plan/index.ts | Implements pre/post-handoff stat checks and outcome-specific messaging/exit codes. |
| packages/cli/src/commands/init/types.ts | Adds optional agentLaunched?: boolean to InitState. |
| packages/cli/src/commands/impl/steps/handoff-wizard.ts | Marks agentLaunched: true after spawning the wizard. |
| packages/cli/src/commands/impl/steps/handoff-codex.ts | Marks agentLaunched: true after spawning Codex. |
| packages/cli/src/commands/impl/steps/handoff-claude.ts | Marks agentLaunched: true after spawning Claude Code. |
| .changeset/plan-honest-outcome.md | Patch changeset for the stash package describing the behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
|
Addressed Copilot's two Both were valid: the pre-handoff stat sat outside the Fix: both stats now go through a Added an e2e case that makes Worth noting for context: the trigger is unlikely in practice — the earlier |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/plan/index.ts`:
- Around line 38-44: Update statPlan to return a valid result only when
statSync(planAbs, ...) yields stats.isFile(); treat directories and other
non-file paths as absent or unreadable so pre-existing-plan and post-handoff
unchanged flows cannot succeed. Add an E2E case covering a plan.md directory and
verify the expected rejection behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bc9ba69-fd55-4537-b4b1-995196cd64f8
📒 Files selected for processing (9)
.changeset/plan-honest-outcome.mdpackages/cli/src/commands/impl/steps/handoff-claude.tspackages/cli/src/commands/impl/steps/handoff-codex.tspackages/cli/src/commands/impl/steps/handoff-wizard.tspackages/cli/src/commands/init/types.tspackages/cli/src/commands/plan/index.tspackages/cli/src/messages.tspackages/cli/tests/e2e/plan-outcome.e2e.test.tsskills/stash-cli/SKILL.md
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed by Claude Code on behalf of James Sadler.
Verdict: APPROVE
This is a correct, well-scoped honesty fix. stash plan no longer claims "Plan drafted" unless the file is actually on disk after the handoff, and it distinguishes the four real outcomes (drafted / launched-but-missing -> exit 1 / deferred -> exit 0 / pre-existing-unchanged). I traced every branch against the head-of-branch source, not just the diff, and the logic holds up.
What I verified (beyond the diff)
- Propagation of
agentLaunchedis the load-bearing part of the fix, and it's sound.howToProceedStep.runreturns the handoff step's result directly (return handoffClaudeStep.run(next)etc.), so the new field reacheshandedOffinplanCommand. Had it returned a locally-mergedstate, the "agent ran but wrote nothing" case would have silently fallen into the deferred exit-0 branch and defeated the whole PR. It doesn't. - The launch-vs-defer split is set at the right seams.
handoff-claude/-codexsetagentLaunched: trueonly afterspawnAgent(...); their not-installed branches return barestate.handoff-agents-mdnever sets it.handoff-wizardalways sets it (it always spawnsrunWizardSpawn). So!planAfter && agentLaunchedfires exactly when an agent process actually ran and produced nothing. - Error routing is correct.
statPlanconverts non-ENOENT fs errors (ELOOP/EACCES/ENOTDIR) into a controlledCliExit(1)with a clear message; thecatchinplanCommandonly special-casesCancelledErrorand re-throws everything else, so both thestatPlanexit and the notWrittenCliExit(1)reach the top-level exit handler. E2E confirms the exit codes. implis genuinely unaffected - it callsawait howToProceedStep.run(...)and discards the result, so the new optional field is ignored.- Message composition and the
.not.toContain(messages.plan.drafted)assertions don't collide - "No plan drafted yet" / "No plan was drafted." don't contain the exact leader "Plan drafted at" (case + wording differ), so the negative assertions are meaningful, not accidentally-passing.
Test coverage is strong: five piped-stdio e2e cases plus the ELOOP fs-error case, driving the built CLI with a fake claude on PATH and no DB/network. This mirrors the #714 treatment.
Non-blocking observations
- Change detection is a
mtimeMs-or-sizeheuristic (plan/index.ts). An in-place revision that keeps the same byte size and lands within the same mtime tick would be misreported as "left unchanged by this run". In practice agent writes almost always bump mtime, and the file is still usable either way, so the blast radius is a cosmetic wording error - not worth a content hash. Worth a comment acknowledging the heuristic's limit if you touch this again. - The revise-and-drafted path isn't covered by e2e - you test pre-existing->unchanged (agent writes nothing) and no-plan->drafted (agent writes fresh), but not pre-existing + agent-rewrites -> "drafted". That's the one combination exercising the
mtimeMs !== ... || size !== ...limb specifically. Cheap to add (fakeclaudethat appends to an existingplan.md). - Behavior change for a broken/looping symlink at
plan.md: previouslyexistsSyncreturned false (treated as "no plan"); nowstatPlanerrors and exits 1. This is intentional and defensible ("opaque fs state must not read as success"), and it's called out in the changeset - flagging only so it's a conscious call. - Wizard always errors-on-no-plan even when the wizard couldn't really start (gateway/network failure still returns a non-zero exit code but
agentLaunched: true). Consistent with the stated "regardless of exit code" intent, so acceptable, but it means a wizard that never got off the ground reports as "agent ran, wrote nothing" rather than a deferral. Minor.
None of these block. Good, honest fix with the right test discipline.
|
Verified against 1. Deferred handoff + pre-existing plan loses the deferral signal
Under a PTY, a bare Enter on the default-yes confirm chains straight through: The AGENTS.md handoff hasn't been driven yet. The "pre-existing plan" test uses a fake 2.
|
… 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.
|
Addressed the review feedback in CodeRabbit — reject non-file plan paths (Major): valid. @freshtonic — thanks for the thorough trace. On the four non-blocking notes:
Full CLI unit suite (822) and the plan e2e suite (8) pass; |
Closes #738.
Problem
stash planprintedPlan drafted at .cipherstash/plan.mdand 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 a downstream agent following the documented handoff read a file that was never created, with no signal anything went wrong (rc.3 skilltester finding M2). Same class of defect as #714 (non-interactiveinitfalse success); this applies the #687/#714 honesty treatment toplan.Fix
planCommandnow stats.cipherstash/plan.mdbefore and after the handoff and reports the outcome that actually occurred:Plan drafted at …— unchanged happy path, including the interactive chain intostash impl--target agents-md, or a CLI target that isn't installed)No plan drafted yetoutro, exit 0 — the files-and-instructions contract was delivered; the plan lands when the user drives their agentleft unchanged by this run, not "drafted"To distinguish "agent ran but wrote no plan" (an error) from "plan expected later" (deferred), the handoff steps now record whether they actually spawned an agent via a new optional
InitState.agentLaunchedfield.init/implignore the field, so their behaviour is unchanged.The exit-0-for-deferred choice mirrors the existing exit-0 "No handoff performed." hint path: those targets succeed at their documented contract (write files + instructions), and the outro no longer makes a false claim automation could act on.
Changes
packages/cli/src/commands/plan/index.ts— the outcome verification (core fix)packages/cli/src/commands/impl/steps/handoff-{claude,codex,wizard}.ts— reportagentLaunched: trueafter a spawn (regardless of exit code); the not-installed branches andagents-mdleave it unsetpackages/cli/src/commands/init/types.ts— the new optionalInitState.agentLaunchedfieldpackages/cli/src/messages.ts— stable outcome leaders undermessages.plan(the e2e suite asserts on them, per the messages.ts convention)packages/cli/tests/e2e/plan-outcome.e2e.test.ts— five piped-stdio e2e cases driving the built CLI with a fakeclaudeon PATH (no DB, no network): agent-wrote-no-plan → exit 1; agent-wrote-plan → drafted; pre-existing-unchanged;agents-mddeferral; claude-not-on-PATH deferralskills/stash-cli/SKILL.md— documents the outcome contract in theplansection (skills ship in thestashtarball).changeset/plan-honest-outcome.md—stashpatchTesting
pnpm --filter stash test— 822 unit tests passpnpm --filter stash build && vitest run --config vitest.integration.config.ts— full e2e suite, 73 tests pass (incl. the 5 new ones)pnpm -w run code:check— clean (exit 0)Out of scope
claudespawn hanging inside an agent session) — different defect, loud rather than silent; unchanged here.stash impl's outro wording — it doesn't claim a file exists, and its deliverable isn't a single on-disk artifact; left as is.stash status's "Plan drafted" lines — already gated on the file existing.Summary by CodeRabbit
stash plannow verifies that a plan was actually created before reporting success.stash planguidance to explain plan outcome messages and follow-up actions.Summary by CodeRabbit
stash plannow verifies that a plan was actually created or updated before reporting success.stash impl.