Repository navigation
fix(coding-agent): make a plan decision a session fact - #342
Merged
Merged
Conversation
A plan submission used to be a handoff: the tool left plan mode and ended the turn, and the caller was told to obtain approval. In an rpc host that already draws the proposal — the web and desktop apps — that made leaving the mode the only record of the decision, so the next message, a question as easily as an approval, ran with file mutations back on. The model could also end plan mode by itself, which let any imperative-sounding request turn into execution. The decision is now a session fact. `step-plan` records `pendingProposalHash` when a review-surface host submits a proposal and clears it on approval, revision, or leaving; the automatic-turn guard reads the fact instead of scanning the transcript for the shape of a handoff. `mode` and `hasUI` could not answer which host this is, so a host that can collect a decision sets `planReviewSurface` when it binds the session. - `/plan` gains absolute verbs: `on`, `off`, and `approve`. A host that can only send text can state "leave" and "approve" without racing a toggle against the state a turn left behind. `/plan approve` rereads the plan file and compares hashes, so a click cannot approve text the user did not read. - Leaving by user decision aborts the run in flight. The Default announcement only reaches the model's next request, so a streaming run kept going with the plan boundary already lifted and could start implementing the rejected plan. The "Plan mode disabled." notice is gone: the host draws the mode. - `/plan <task>` while a proposal waits replaces it and appends a hidden instruction to submit again, so the session never waits on a proposal nobody is looking at. It expands prompt templates, as the same text without `/plan` would. - The approval instruction is a hidden custom message rather than a user message: the command is the decision, and a second user bubble made one decision look like two things the user said. - `hasPendingExternalPlanApproval` is removed. A branch walk answers "is a handoff near the tail", which is not "is the user's decision outstanding".
…d races Review of the decision fact found places where it could disagree with itself or with the proposal the user actually read. - A pending write that the branch rejects now rolls back in memory, as exiting already did. A wait nobody recorded would hold every automatic turn back with no visible cause; a decision nobody recorded would let work past an unapproved plan. - `/plan approve <hash>` names the proposal it answers. A decision that arrives after a newer submission must not approve that one: the file being current says nothing about which proposal was on screen. - A changed file no longer clears the fact. Nothing was decided, so the gate has to keep holding until someone reviews the text that is on disk now. - Entering plan mode drops a wait left by a caller handoff, and `/plan off` on a handoff clears it. Neither the mode toggle nor an unrelated proposal may be approved by accident. - `/plan approve` answers a caller handoff too. Text is the only channel that caller has, so requiring the mode to still be on made the one verb that can answer it unusable there. - A revision requested mid-run sends its hidden instruction on the same follow-up queue as the notes. Steering it told the model to resubmit before it had read what to change. - `approvalSource` follows who owns the decision: a host that draws the proposal has a person answering, whatever transport carries it. - The "last assistant text" of an RPC / stdio caller asks the same fact before reporting the proposal, instead of assuming an unchanged transcript. - A session written before the fact recorded its wait only in the transcript. `restorePlanState` migrates it once, and never replays a handoff that a later plan entry superseded. `planReviewSurface` is a required context field, like `hasUI`: the runner always answers it, and the terminal host declares false because it answers the review inside the tool call. The extension guide documents it.
Second review round. The remaining problems were about copies of the same fact disagreeing, and about a hidden message that could not keep its place. - A caller handoff records the wait and leaves the mode in one write. Two writes left a window where the branch said a proposal waited but the caller never received one, or the reverse; the second write failing also stranded a wait with no result to answer. - The migration of a pre-fact session is written back once, so the guard, the RPC and SDK replies, and a host's card all read the same value instead of a migrated memory next to an entry that lacks it. - The proposal reported to a caller is the one whose details carry the waiting hash. "The tail ends in a handoff" cannot answer that: a question typed past the proposal is not an answer, and a submission recorded before its own result would be reported with the older text. - Reading that fact tolerates a session facade without a branch. It sits in the path of a reply the host owes its caller, so a missing manager must not turn into a hung query. - The hidden "submit again" instruction is gone. It could not ride the same request as the notes: the follow-up queue delivers one message per request by default, so it arrived alone and could send the model back to submit a plan it had not revised. Plan mode's own rules now say that changes to a submitted proposal mean updating the plan and submitting again, which the model reads from the block it already has. - `/plan off` on a handoff, `/plan approve <hash>`, and the legacy migration each have a test at the level that decides them: which request the model sees, which proposal a late decision answers, and which branch an upgrade reads.
Third review round. Each change is one the tests could not tell apart from a deleted feature, or a mismatch the previous round introduced. - The legacy migration keeps the summary boundary. `getBranch()` includes entries the model no longer reads, and a wait restored from behind a compaction is a wait nobody can be shown a proposal for. - The Plan block carries a content version, so a session resumed with an older block gets the current rules appended once. A rule only reaches sessions that entered plan mode after the upgrade otherwise — which mattered once the "changes mean revise and resubmit" rule replaced the hidden instruction. - The notes forwarded by `/plan <notes>` report the host's source. A turn that submits a proposal while they wait must not turn the user's change request into extension traffic that the pending-decision guard holds back. - `/plan approve` only names a proposal by its 64-hex snapshot hash; any other text falls through to the task branch instead of being swallowed. - `exit` only ever handles a user decision now that both caller handoffs leave through `handOffProposal`; the branch that distinguished them was dead. - A proposal result with no text no longer wins over the answer the caller did get. Tests now assert the writes, not the final state: a caller handoff produces exactly one entry, the migration is written back once and not again, `/plan off` answers a handoff and stays idempotent, `/plan approve` with nothing waiting writes nothing, and a change request sent mid-run still reaches the model.
The gate stops automatic work, but a prompt the user typed is how a decision gets answered — and it lost that standing as soon as it was queued. `/plan <notes>` sent while a turn runs waits in the follow-up queue; if that turn then submits a proposal, the gate holds the queue back. Their change request sat behind a decision only they could open, with nothing on screen to open it. Queued inputs now remember that a person wrote them, and the post-run gate only holds back what a feature queued. The source was already reaching the prompt path; it just was not carried into the queue. The two halves of the review-surface rule now have a test each: on a host that draws the proposal, another thing the user says does not release the decision, and on a caller handoff the next turn does — that being the only channel it has.
…deliver The previous attempt allowed a continuation whenever the queue contained anything the user wrote. Two things went wrong with that. A continuation delivers one queued message at a time, steering before follow-ups, so work a feature queued ahead of the user's words ran on their behalf past a decision they had not answered. And authorship was tracked by text, which cannot tell two queued messages with the same words apart, nor a queue entry from a mirror that never removes an image-only one. Queued messages are now marked by identity, and the gate asks the agent which message its next drain would return. That is the only one the user's answer can be. `Agent.peekNextQueued` exposes it without draining. Both halves have a test that fails without them: the change request queued before a submission still reaches the model, and automatic work queued ahead of it is still held.
…e run The gate decided once, before a continuation started, whether any queued message the user wrote was present. A continuation then delivers a batch at a time for as long as it runs, so: - a follow-up behind a tool result was not in the first request at all, while the run it authorized had already begun; - with "all" queue modes, or simply later in the same run, the automatic work queued behind the user's words was delivered on their behalf, past a decision they had not answered. The decision now governs delivery. `PendingMessageQueue.drain` takes a per-message predicate, `Agent.setQueueDeliveryFilter` installs it, and the loop asks it every time it takes a message. What the session refuses stays queued rather than being dropped, so a later turn after the decision can deliver it. The session installs one filter: while a proposal waits, only messages a person wrote may be delivered. Both directions have a test — the user's change request reaches the model, and automatic work queued ahead of it does not — and the agent-level test proves a refused message is held, not lost.
Follow-ups from review of the delivery filter: - The maintenance paths still asked the unfiltered queue whether to continue, so work the decision holds could justify a compaction turn that then had nothing to deliver. They ask the same filtered question as the turn gate. - The filter is removed on dispose. It belongs to the session's decision, and an agent that outlives the session must not inherit it. - A predicate is wrapped before `findIndex`/`some`, so one that also declares `(index, array)` cannot be handed them. - `QueueMode` says what "oldest" means once a filter can withhold messages. Two more tests: "all" mode splits a batch instead of passing or refusing it whole, and work held by a decision is delivered once the decision is answered.
liuedcson
requested review from
MelodyVAR,
Uking-xxx and
ZouR-Ma
as code owners
November 2, 2026 15:07
`continue()` only took a message from the queues when the transcript ended in an
assistant message, and `runContinuation` polls steering once before its first
request. A follow-up queued behind a tool result therefore waited for a request
the model took without ever seeing it.
The caller now states which it wants: `continue({ deliverQueued: true })` makes
the queued input this turn's prompt, which is what a caller returning after a
decision needs. Retries and context repairs leave it off, because they resume the
same work and a message the person queued belongs after it.
The queues also gain a per-message delivery filter: a refused message stays
queued in its original position, so a caller can hold part of the queue without
dropping or reordering what it releases.
The rule that lets a caller answer a plan handoff with its next message was attached to `before_agent_start`, which only a submitted prompt reaches. A message queued behind the submitting turn arrives without one, so the wait stayed armed after the model had already answered it — the RPC and SDK replies kept returning the old proposal, and automatic work stayed held. `message_start` now carries `fromUser`, and the plan extension settles the wait there. The delivery rule and the delivery event then agree about who spoke. Three more paths leaked automatic work past a decision the person had not answered: - Context an extension queued through `sendCustomMessage` was flushed into the next prompt before the decision was looked at. - Restoring a session let the transcript scan overwrite a `pendingProposalHash` the entry already carried, dropping a wait a newer build recorded under an older entry version. - Overflow and truncation recovery were skipped, turning an error into a dead end; they repair the transcript rather than starting work, so they still run. `/plan` states its verbs without case, and `approve` claims its whole input: a truncated or upper-cased hash is refused instead of being read as a task, which would drop the wait it meant to answer. A note forwarded by `/plan <task>` keeps the command's origin, so it can answer the wait it queued behind instead of being held as extension traffic. `/plan off` aborts the run in flight, since the mode change only reaches the model's next request.
A host that draws the proposal keeps it pending when its user asks a question, so the fact still says one is waiting. Reporting the proposal text in the RPC and stdio results then hid the answer the caller had just asked for, for as long as the decision stayed open. Any assistant text after the submission now wins.
The capability existed only for a host that binds the session in-process. A client on the wire had no way to say it draws the proposal and collects the decision, so it always got the handoff-and-exit behaviour — and the problem this change set out to fix, where the next message authorized work, stayed in place for every wire client. `--plan-review-surface`, or `RpcModeOptions.planReviewSurface` for an embedded host, states it. Nothing infers it from `--mode rpc`: every rpc client has a dialog bridge, and only the client knows whether it can draw a plan.
The Plan block's content version sat next to the `step-plan` entry's format version under a name that read like the same thing, and the two numbers did not agree. It now says which of the two it is.
The plan status chip styled its label with `ctx.ui.theme.fg(...)` and assumed a theme. A host that renders text rather than a terminal may answer nothing, and the throw took the whole command with it; styling is a presentation nicety, so fall back to the plain label. The mode announcement and the four decision refusals are marked `transient`: they restate state the host already draws from the session, so a host with a transcript must not keep them as rows in the conversation. The option is declared here so every host agrees on what it means.
The keep-alive test waits for a notification that arrives after the child exits and its details are read, which is real I/O. Its wait loop advanced fake timers 500 times for a total of about eight milliseconds of real time, so on a loaded runner the notification could not land inside the budget and the test failed with "notifications: background_done". It failed on two unrelated branches, and passes locally, which is the shape of that race rather than a product defect. The budget is now three seconds of real time, and each step yields a real macrotask: fake timers move virtual time only, so waiting on them alone cannot outlast real work. Confirmed by delaying the completion by 40 ms of real time: the old loop fails, this one passes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A plan submission used to be a handoff:
exit_plan_modeleft plan mode, ended the turn, and told the caller to obtain approval. In a host that already draws the proposal, that made leaving the mode the only record of the decision — the next message, a question as easily as an approval, ran with file mutations back on. The model could also end plan mode by itself, so any imperative-sounding request could turn into execution.The root cause is one value carrying two meanings: "plan mode is off" also meant "the user decided". This change separates them.
What
The decision is a session fact. The
step-planentry recordspendingProposalHashwhen a proposal is submitted — by a host that draws it, and by a caller handoff alike. The automatic-turn guard reads that fact instead of scanning the transcript for the shape of a handoff: a scan cannot tell an answer from a question, and cannot see a decision that wrote no user message (/plan off). It is cleared by approval, by a request for changes, by leaving plan mode, by entering it again, and — on a host with no review surface, where a message is the only answer channel — by the next message a person sends.The host declares its surface.
modeandhasUIcannot answer this: every rpc host has a dialog bridge, including clients with no person behind it. A host that can show a proposal and collect a decision says so when it binds the session:bindExtensions({ planReviewSurface: true })in-process,--plan-review-surfaceacross the wire. Without it, the previous behaviour is unchanged (exit, hand off, turn ends)./plangains absolute verbs.on,off, andapprove [hash], matched without case. A host that can only send text can state "leave" and "approve" without racing a toggle against whatever state a turn left behind.approverereads the plan file and compares hashes, so a click cannot approve text the user did not read; naming the hash binds the decision to the snapshot the user actually saw. The verb claims the whole input: a malformed hash is refused instead of being read as a task to plan./plan <task>keeps handing the task to the model as the user's own turn, and now expands a leading skill or prompt template the way the same text would without the prefix — the web composer puts its plan prefix in front of a picked skill. That also means a task whose text starts withon,off, orapprovehas to be rephrased.Leaving means stopping.
/plan off, and a bare/planthat turns the mode off, abort the run in flight. The Default announcement only reaches the model's next request, so a streaming run kept going with the plan boundary already lifted and could start implementing the rejected plan. The "Plan mode disabled." notice is gone: the host draws the mode.Queued work is judged at delivery. While a proposal waits, only messages a person wrote may be delivered. That rule is enforced where the loop takes a message (
Agent.setQueueDeliveryFilter), not once before a continuation starts: a continuation delivers a batch at a time, so a run-start check either missed the user's words (they were behind a tool result) or carried automatic work along with them. What the decision holds stays queued and is delivered by the answer;/plan offreleases it without starting a turn, so it runs on the next one. The same rule now covers context an extension queued (sendCustomMessage), which previously slipped into the next request's history.A caller that returns with deliverable queued input now asks for it:
Agent.continue({ deliverQueued: true })makes that input the next request's prompt. Retries and context repairs leave it off, because they resume the same work and a queued follow-up belongs after it.The wait is settled where a message is delivered, not where a prompt is submitted: a message queued behind the submitting turn arrives without a new
prompt, and that delivery is the only channel a caller handoff has.Context recovery is maintenance, not work. A pending decision no longer blocks overflow and truncation recovery; it only blocks starting new work. Threshold compaction stays deferred to the next request either way.
Compatibility. A session written before the fact recorded its wait only in the transcript;
restorePlanStatemigrates it once, from the still-unanswered handoff at the tail of the branch, and never replays one a later plan entry superseded. An entry that already carriespendingProposalHashkeeps it under any entry version, so a build that recorded the fact before this one is not mistaken for a legacy entry.The Plan block carries a content version (
PLAN_MODE_BLOCK_VERSION, distinct from the entry's format version). A session resumed from a block written before it appends the current block on its next request, once.A host that renders text rather than a terminal may have no theme; the plan status chip no longer assumes one, and the mode announcement and the four decision refusals are marked
transientso a host with a transcript does not keep them as rows.Behaviour by host
planReviewSurface: false)approvestarts the work--plan-review-surface; same behaviour as the app/plan approve, or/plan off; the reported result is driven by the fact, not by the shape of the transcriptThe SDK's in-process session supports
planReviewSurface; the framed--sdk-stdiohost does not declare it yet, so a client there keeps the handoff behaviour.external_approval_requiredcan now carryapprovalSource: "user"— a review-surface host that keeps planning. A client that gates onapprovalSource === "external"should decide fromplanModeExitedas well.On a review-surface host, a question typed past an unanswered proposal is answered normally:
get_last_assistant_textand the stdio host's result report that answer, not the proposal, from then on.Validation
pnpm run check(biome, boundary checks,tsgo --noEmit) passes.packages/agent-core: 564 passed, 1 skipped.packages/coding-agent: 5204 passed, 58 skipped. The sixcompletion-check-cli.test.tsfailures reproduce onorigin/mainin this environment (the fixture needs a Git worktree with a commit and fails before any model call); the same file contributes 38 of the passes, so excluding it the run is 5166 passed. A few suites skip without credentials or a builtdist/, includingtest/rpc.test.ts.apps/cli: 756 passed.test/step-subagent-events.test.tswaited for a notification that arrives after real I/O by advancing fake timers 500 times, which costs about 8 ms of real time, so a loaded runner lost that race (notifications: background_done). It failed on two other branches with the same message. The wait is now bounded by three seconds of real time and yields a real macrotask each step.Docs
docs/plan-approval.md,packages/agent-core/README.md,packages/coding-agent/docs/{extensions,rpc,step-integration}.md.Notes for reviewers
The web and desktop apps live on
feat/stepcode-desktop-mvp; this PR adds the capabilities they bind, not the UI. That branch has an earlier, parallel version of the same idea: it must be rebased onto this change with its own copy removed, not merged file-by-file, because the two versions disagree about who clears the wait.