fix(cli): resolve plan_exit to the plan file actually saved - #11896
Conversation
0a5fe8d to
e9d94c2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9d94c2d36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
plan_exit fell back to Session.plan()'s generated slug path whenever the model omitted an explicit path, which no longer matched the agent-chosen filename plan mode now instructs. That mismatch caused both the wrong filename shown after "Plan is ready" and a silently missing implement-next-session prompt (issue #11859). plan_exit and the follow-up flow now verify the plan file exists, recovering it via a session-timestamp glob or the plan agent's last markdown write when the exact path is missing, and fail loudly with actionable guidance when nothing was written. The failure message also names an explicit path parameter that got rejected (outside the project, or a directory), instead of silently substituting a freshly-guessed filename that never matches what the model actually wrote. This only affects wording of the final error: Session.plan() and locate()'s recovery tiers still run first, so a rejected path that's actually the canonical non-git plans dir (which sits outside the project boundary by design) still recovers via the timestamp glob rather than failing outright.
e9d94c2 to
08a9d69
Compare
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The Files Reviewed (6 files)
Reviewed by claude-sonnet-5-20260630 · Input: 34 · Output: 19.1K · Cached: 1.3M Review guidance: REVIEW.md from base branch |
…#11896) plan_exit fell back to Session.plan()'s generated slug path whenever the model omitted an explicit path, which no longer matched the agent-chosen filename plan mode now instructs. That mismatch caused both the wrong filename shown after "Plan is ready" and a silently missing implement-next-session prompt (issue Kilo-Org#11859). plan_exit and the follow-up flow now verify the plan file exists, recovering it via a session-timestamp glob or the plan agent's last markdown write when the exact path is missing, and fail loudly with actionable guidance when nothing was written. The failure message also names an explicit path parameter that got rejected (outside the project, or a directory), instead of silently substituting a freshly-guessed filename that never matches what the model actually wrote. This only affects wording of the final error: Session.plan() and locate()'s recovery tiers still run first, so a rejected path that's actually the canonical non-git plans dir (which sits outside the project boundary by design) still recovers via the timestamp glob rather than failing outright.
Issue
Fixes #11859
Context
Plan mode had two related bugs on finalize:
plan_exitreported a filename that didn't match what was actually written, and the "implement in new session?" follow-up prompt would silently fail to appear.Root cause:
plan_exitfell back toSession.plan()'s randomly-slugged path whenever the model omitted an explicitpath. That was fine while the reminder pinned an exact filename, but a later change (#11170) moved to agent-chosen descriptive filenames without updating the fallback - so whenever the model omittedpath,plan_exitreported a file that was never written. The follow-up flow read a plan file at that same wrong path, got an empty read, and silently skipped the prompt - same root cause, different symptom.Implementation
plan_exitand the follow-up flow now verify the plan file exists on disk instead of trusting a guessed path:PlanFile.resolve()now rejects a path that exists but isn't a file (e.g. a directory), in addition to its existing containment check.PlanFile.locate()recovers the real file in tiers: exact target → newest file matching the session's generated-name pattern (<session-created>-*.md) → the last markdown file written by a planning agent (covers custom architect-slug agents too, via a caller-supplied agent name). This is intentionally narrow - a false-positive match in this fallback would finalize the wrong file, so it only looks atwrite/edittool calls from plan/architect-agent turns.plan_exitnow fails with an actionable error ("write the plan file first, or pass the exact path") instead of silently succeeding with a nonexistent path.Filesystem.statwas hardened to never throw (it previously only suppressed ENOENT; ENOTDIR/EACCES could throw and crash the new verification path) - every existing caller already treated its result as optional, so this only removes latent crash paths.This diff went through several rounds of adversarial code review; each round found and fixed real issues (a too-loose write-history fallback, a directory passing as a valid plan path, an agent-name filter that excluded custom architect slugs) before converging.
Screenshots / Video
plan.mov
How to Test
Manual/local verification
bun test test/kilocode/plan-file.test.ts test/kilocode/plan-followup.test.ts test/kilocode/plan-exit-detection.test.ts— 54 passing (agent)bunx tsc --noEmit -p packages/opencode— clean (agent)bun turbo typecheck(full monorepo, via pre-push hook) — 20/20 tasks passing (agent)Reviewer test steps
plan_exitwithout an explicit path - the "Plan is ready at X" link should point at the real file.plan_exit- expect a visible "Plan file not found" tool error (not a silent success or a stuck session).Checklist