Skip to content

fix(coding-agent): keep plan mode until a request-bound approval - #329

Open
liuedcson wants to merge 5 commits into
mainfrom
fix/plan-mode-rpc-approval-gate
Open

liuedcson wants to merge 5 commits into
mainfrom
fix/plan-mode-rpc-approval-gate

Conversation

@liuedcson

@liuedcson liuedcson commented Oct 31, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Plan mode exited and the model started executing as soon as exit_plan_mode ran. A host that only rendered the tool result was never a blocking gate, and the tool could not tell a real reviewer from one that could not answer. The earlier revision of this PR also assumed the desktop app was an SDK stdio peer; it is an in-process mode: "rpc" host, so that bridge could not fix the reported behavior.

Fix

A plan review is now a persisted, request-bound state, separate from leaving plan mode.

  • One boundary, one spelling. The plan-file restriction is decided by a shared helper before the ordinary permission prompt. A forbidden edit is refused without asking; a write to the plan file still goes through the normal permission policy. A failed persist or an embedder that records no entries cannot drop the live restriction.
  • Submission and exit are separate. step-plan records review: { id, planHash, outcome } with pending before the review is shown and the decision afterwards. Only one review may be pending; changing modes, starting a new /plan task, navigating branches, or shutting down invalidates its signal, so a late approval cannot unlock a new round. Restoring a pending review records it as unanswered.
  • Who decides. The tool uses the host's native ui.reviewPlan when available, otherwise the host's structured ui.clarify question (used by the in-process Web/Desktop host, which already has a clarification panel and broker), otherwise the TUI dialog. Closing a structured question counts as a dismissal, matching Escape in the TUI. Free text is feedback, never approval. A malformed or missing answer is not a decision.
  • SDK stdio peers opt in. ctx.ui.reviewPlan exists only when the peer advertises capabilities.planReview: true at initialization. Unsupported peers keep the external handoff instead of waiting for a request they cannot answer. Timeouts and aborts emit user_dialog.cancelled; failed writes and shutdown settle immediately; an oversized proposal returns an actionable request to shorten it instead of a misleading "dismissed".
  • No proposal keeps plan mode on. The model may answer a read-only request directly, but a missing or unchanged plan file no longer disables the mode: only the user leaves with /plan.
  • Approval is bound to the reviewed snapshot. The tool rereads the plan and compares the hash before leaving plan mode; a changed or unreadable file requires a new review.
  • The turn actually ends. A new stopTurn result flag ends the turn on a single control decision, independent of sibling results. Sequential batches give later calls paired skipped results instead of executing them; parallel calls already in flight still settle. The existing all-results terminate behavior is unchanged.

Behavioral impact

  • TUI: unchanged dialog, but dismissal and unanswered reviews stop the turn; a rendering error stays a tool error instead of being recorded as a user decision.
  • Web/Desktop (in-process mode: "rpc"): the plan review blocks through the existing clarification channel and ends the turn unless the user approves. No new desktop-side protocol is required. A dedicated plan surface can consume intent.kind === "plan_review" later; the current panel shows the proposal text.
  • Headless and RPC hosts without a review bridge: the proposal still exits plan mode and is handed to the caller, but the turn ends immediately instead of letting the model continue.
  • Plan-mode entry still leaves the tool block byte-identical, so the cached prefix is preserved. /plan <task> still expands a leading skill or prompt template.

Tests

  • pnpm run check (Biome, dependency/layer/public-boundary checks, tsgo --noEmit, browser smoke).
  • ./test.sh with an isolated HOME: VALIDATION_EXIT=0, 6,986 tests passed / 65 skipped across all packages (coding-agent 5,159; agent-core 540; providers 461; CLI 751; others 92).
  • Focused coverage:
    • real ExtensionRunner capability probing (TUI / unbridged rpc / bridged rpc);
    • request-bound review state, single pending review, mode-change invalidation, restore-as-unanswered, hash recheck, failed persist;
    • SDK stdio opt-in, timeout/abort/late-frame/cancel-frame, failed write, shutdown, oversized proposal;
    • real AgentSession end-to-end: mixed batch through sdk-stdio, embedded rpc clarify approval and dismissal, no-proposal direct answer, /plan template expansion;
    • Agent Loop control barrier with a throwing result hook and paired skipped results;
    • CLI TUI abort closing the native review and the unanswered transcript summary.

…dged rpc hosts

exit_plan_mode auto-exited plan mode in every rpc context, so the desktop
app rendered a plan-review card that was not a blocking approval and the
model executed the plan without the user agreeing. Split the rpc handoff
by capability instead of by mode string: an rpc host whose UI context
carries a reviewPlan bridge (sdk-stdio) now waits for the reviewer's
answer, while a truly headless run or an rpc host without a bridge keeps
the external handoff.

The runner wrapper now spreads reviewPlan only when the bound UI context
has one, so ctx.ui.reviewPlan stays a truthful capability signal and the
TUI keeps rendering the in-process dialog. A bridge that is cancelled,
times out, errors, or answers unparseably keeps plan mode on and
terminates the tool batch, so a failed review can never read as
approval; the stdio bridge carries the plan snapshot and hash and fails
fast when the plan cannot fit a frame.
…robing

Drive exit_plan_mode through a real ExtensionRunner so the capability
probe sees what production hosts see: a TUI-shaped UI context renders
the in-process dialog, an rpc context without a bridge keeps the
external handoff, and a bridged rpc context waits for the answer and
fires the plan_review ui_prompt lifecycle. Cover an unanswered bridge
(timeout, abort, malformed answer) keeping plan mode with terminate and
bridgeFailed, an explicit dismissal counting as a decision, the
oversized-plan frame pre-check, and an already-aborted dialog falling
back without writing a request frame. The host now accepts an
injectable plan-review grace window so the timeout case does not wait
out the production budget.
Record the rpc review bridge in the sdk-stdio integration guide,
including the plan_review reverse request and response shapes and the
plan-review capability, extend the extensions guide with the reviewPlan
UI context method and its ui_prompt kind, and spell out in the plan
approval reference that an unanswered bridged review records an
external approval source rather than a user decision.
A tool result could only end a turn when every result in the batch set
`terminate`, so an ordinary sibling read kept the loop running after a
control decision. Add `stopTurn` for a single-result barrier: sequential
batches skip later calls with paired skipped results, in-flight parallel
calls settle, and no further model turn starts.

Preserve a stop requested before a result hook throws, and keep the
existing all-results `terminate` behavior for older tools.
`exit_plan_mode` left plan mode and let the model continue as soon as it
ran, so a host that only rendered the tool result could not gate
execution. Submission is now a persisted, request-bound state: one
pending review, invalidated by mode changes, a new planning task, branch
navigation, or shutdown, and a late approval cannot unlock a new round.

- Decide the plan-file boundary before the permission prompt in every
  mode, from one shared helper that also keeps the live restriction when
  a persist fails.
- Reuse the host's native review, `ui.clarify`, or the TUI dialog; SDK
  stdio peers opt in at initialization and receive a cancellation frame.
- Keep plan mode on when no proposal exists: the model may answer
  read-only questions, but only the user can leave. Reread the file before
  accepting approval, and report an oversized proposal as actionable.
- End the turn on dismissal, an unanswered review, or an external handoff
  instead of letting the model keep executing.

The built-in control results use `stopTurn`, so an ordinary sibling tool
result cannot cancel the decision.
@liuedcson liuedcson changed the title fix(coding-agent): gate plan mode exit on interactive review for bridged rpc hosts fix(coding-agent): keep plan mode until a request-bound approval Oct 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant