From ee683c02f476cc6be8c2ad9eb68b7e79d77858c6 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 2 Nov 2026 21:12:58 +0800 Subject: [PATCH 01/15] fix(coding-agent): require a user decision before leaving plan mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` 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". --- apps/cli/test/plan-review-tool-row.test.ts | 3 + docs/plan-approval.md | 25 +- .../coding-agent/src/core/agent-session.ts | 15 ++ .../src/core/extensions/runner.ts | 6 + .../coding-agent/src/core/extensions/types.ts | 9 + .../coding-agent/src/features/plan-handoff.ts | 39 +-- .../src/features/plan-mode-prompt.ts | 2 +- .../src/features/plan-mode-tools.ts | 56 +++- .../coding-agent/src/features/step-plan.ts | 178 +++++++++++-- .../test/step-plan-extension.test.ts | 245 ++++++++++++++++-- .../test/suite/plan-approval-gate.test.ts | 52 ++-- 11 files changed, 536 insertions(+), 94 deletions(-) diff --git a/apps/cli/test/plan-review-tool-row.test.ts b/apps/cli/test/plan-review-tool-row.test.ts index 1016821fe..2d8e7e803 100644 --- a/apps/cli/test/plan-review-tool-row.test.ts +++ b/apps/cli/test/plan-review-tool-row.test.ts @@ -47,6 +47,9 @@ function exitPlanModeDefinition(): ToolDefinition { isPlanModeActive: () => true, exitPlanMode: () => {}, resolvePlanFilePath: () => "/tmp/plan.md", + recordPendingProposal: () => {}, + pendingProposalHash: () => undefined, + clearPendingProposal: () => {}, } satisfies StepPlanModeController); return tools.get("exit_plan_mode") as ToolDefinition; } diff --git a/docs/plan-approval.md b/docs/plan-approval.md index 35d72277a..e975eee45 100644 --- a/docs/plan-approval.md +++ b/docs/plan-approval.md @@ -9,10 +9,16 @@ active and steers the notes into the next response; dismissal keeps it active an ends the turn. There is no deadline on that decision. The dialog closes when the run is cancelled, and the tool then reports the review as aborted. -Headless runs and RPC hosts cannot obtain approval. They exit plan mode and hand +A host declares which of the two it is. One that can show a proposal and collect a +decision — the web and desktop apps — sets `planReviewSurface` when it binds the +session; plan mode then stays on, the turn ends, and the proposal is recorded as +`pendingProposalHash` in the `step-plan` entry. `mode` and `hasUI` cannot answer +this: every rpc host passes a dialog bridge, including clients with no review +surface. + +A host without that surface cannot obtain approval. It exits plan mode and hands the proposal to the caller, but the turn ends immediately: leaving the mode does -not authorize execution. RPC hosts are always external, even when their dialog -bridge reports `hasUI: true`. The caller must approve the exact snapshot before +not authorize execution. The caller must approve the exact snapshot before starting an implementation turn. The host reports the proposal in place of the answer it does not have: print text mode writes the handoff to stdout, and the SDK result and the RPC `get_last_assistant_text` reply carry the handoff rather @@ -84,9 +90,10 @@ Both interactive and external results carry structured `details`: ``` `outcome: "external_approval_required"` identifies a handoff awaiting the -caller's decision. `planModeExited: true` describes the resulting mode state; it -does not mean approval was granted. External handoffs end the turn: the result -sets `stopTurn` in addition to `terminate`. +caller's decision. `planModeExited` describes the resulting mode state: `true` for +a caller that left the mode, `false` for a review-surface host that keeps planning +until the user answers. Neither means approval was granted. Every handoff ends the +turn: the result sets `stopTurn` in addition to `terminate`. `approvalSource` identifies who owns the decision: `"user"` for the TUI review, including feedback and dismissal, and `"external"` for the headless / RPC @@ -112,7 +119,9 @@ if (details?.outcome === "external_approval_required") { const expectedHash = createHash("sha256") .update(details.planContents, "utf8") .digest("hex"); - if (details.planModeExited !== true || details.planHash !== expectedHash) { + // `planModeExited` is `true` for a caller that left the mode, `false` for a + // review-surface host that keeps planning until its user answers. + if (details.planHash !== expectedHash) { throw new Error("Incomplete or inconsistent plan handoff"); } // Present details.planContents and bind the caller's decision to details.planHash. @@ -144,7 +153,7 @@ Native outcomes keep their existing behavior: | `approved` | `user` | `true` | The user selected execution; restore tools and continue. | | `feedback` | `user` | `false` | Keep planning and steer the user's notes into the next response. | | `dismissed` | `user` | `false` | Keep planning and stop the turn to wait for the user. | -| `external_approval_required` | `external` | `true` | Headless / RPC handoff; the turn ends and the caller must gate execution. | +| `external_approval_required` | `external` | `true` / `false` | The turn ends. `true` for a caller that left the mode; `false` for a review-surface host that keeps planning until its user answers. | The TUI still selects execution by default, and Enter confirms that selection. Interactive result details also include the hash of the reviewed snapshot. diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index daaee0a30..0299e48a1 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -253,6 +253,15 @@ export interface AgentSessionConfig { export interface ExtensionBindings { uiContext?: ExtensionUIContext; mode?: ExtensionMode; + /** + * The host can show a plan proposal and collect a decision for it. + * + * `mode` and `hasUI` cannot answer this: every rpc host passes a dialog + * bridge, including clients with no plan-review surface. A host that has one + * sets this, and the plan tool hands the proposal over instead of leaving the + * mode by itself. + */ + planReviewSurface?: boolean; commandContextActions?: ExtensionCommandContextActions; /** Host UI cleanup after the session cancels its run (for example, restore queued input). */ abortHandler?: () => void; @@ -419,6 +428,8 @@ export class AgentSession { private _sessionStartEvent: SessionStartEvent; private _extensionUIContext?: ExtensionUIContext; private _extensionMode: ExtensionMode = "print"; + /** Whether the bound host can show a plan proposal and collect a decision. */ + private _extensionPlanReviewSurface = false; private _extensionCommandContextActions?: ExtensionCommandContextActions; private _extensionAbortHandler?: () => void; /** Predicates that block automatic turn starts until the user answers. */ @@ -3501,6 +3512,9 @@ export class AgentSession { if (bindings.mode !== undefined) { this._extensionMode = bindings.mode; } + if (bindings.planReviewSurface !== undefined) { + this._extensionPlanReviewSurface = bindings.planReviewSurface; + } if (bindings.commandContextActions !== undefined) { this._extensionCommandContextActions = bindings.commandContextActions; } @@ -3682,6 +3696,7 @@ export class AgentSession { getScopedModels: () => this._scopedModels, isIdle: () => this.isIdle, isProjectTrusted: () => this.settingsManager.isProjectTrusted(), + hasPlanReviewSurface: () => this._extensionPlanReviewSurface, getSignal: () => this.agent.signal, abort: () => { this._abortCurrentRun(); diff --git a/packages/coding-agent/src/core/extensions/runner.ts b/packages/coding-agent/src/core/extensions/runner.ts index c34397d1d..8febc6080 100644 --- a/packages/coding-agent/src/core/extensions/runner.ts +++ b/packages/coding-agent/src/core/extensions/runner.ts @@ -280,6 +280,7 @@ export class ExtensionRunner { private getScopedModels: () => readonly ScopedModel[] = () => []; private isIdleFn: () => boolean = () => true; private isProjectTrustedFn: () => boolean = () => true; + private hasPlanReviewSurfaceFn: () => boolean = () => false; private getSignalFn: () => AbortSignal | undefined = () => undefined; private waitForIdleFn: () => Promise = async () => {}; private abortFn: () => void = () => {}; @@ -350,6 +351,7 @@ export class ExtensionRunner { this.getScopedModels = contextActions.getScopedModels; this.isIdleFn = contextActions.isIdle; this.isProjectTrustedFn = contextActions.isProjectTrusted; + this.hasPlanReviewSurfaceFn = contextActions.hasPlanReviewSurface ?? (() => false); this.getSignalFn = contextActions.getSignal; this.abortFn = contextActions.abort; this.hasPendingMessagesFn = contextActions.hasPendingMessages; @@ -808,6 +810,10 @@ export class ExtensionRunner { runner.assertActive(); return runner.isProjectTrustedFn(); }, + get planReviewSurface() { + runner.assertActive(); + return runner.hasPlanReviewSurfaceFn(); + }, get signal() { runner.assertActive(); return runner.getSignalFn(); diff --git a/packages/coding-agent/src/core/extensions/types.ts b/packages/coding-agent/src/core/extensions/types.ts index 792d3b3e4..ca3143e67 100644 --- a/packages/coding-agent/src/core/extensions/types.ts +++ b/packages/coding-agent/src/core/extensions/types.ts @@ -395,6 +395,13 @@ export interface ExtensionContext { isIdle(): boolean; /** Whether project-local trust is active for this context. */ isProjectTrusted(): boolean; + /** + * The host can show a plan proposal and collect a decision for it. + * + * Unlike `hasUI`, this is about one specific surface, not about having a + * dialog bridge at all: an rpc child has the bridge but no user to answer. + */ + planReviewSurface?: boolean; /** The current abort signal, or undefined when the agent is not streaming. */ signal: AbortSignal | undefined; /** Abort the current agent operation */ @@ -1861,6 +1868,8 @@ export interface ExtensionContextActions { getScopedModels: () => readonly ScopedModel[]; isIdle: () => boolean; isProjectTrusted: () => boolean; + /** Whether the host can collect a plan review decision (optional; defaults to false). */ + hasPlanReviewSurface?: () => boolean; getSignal: () => AbortSignal | undefined; abort: () => void; hasPendingMessages: () => boolean; diff --git a/packages/coding-agent/src/features/plan-handoff.ts b/packages/coding-agent/src/features/plan-handoff.ts index b474ef6fd..7372fd720 100644 --- a/packages/coding-agent/src/features/plan-handoff.ts +++ b/packages/coding-agent/src/features/plan-handoff.ts @@ -1,11 +1,14 @@ /** * The plan review handoff as a session fact. * - * A handoff leaves plan mode and ends its turn, so the proposal is no longer in - * an assistant message: it lives in the `exit_plan_mode` tool result. Hosts that - * report a final answer, print text mode, or decide whether to continue on their - * own all need the same reading of that result, and a second spelling would - * drift from the tool that writes it. + * A handoff ends its turn, so the proposal is no longer in an assistant message: + * it lives in the `exit_plan_mode` tool result. Hosts that report a final answer + * or print text mode need the same reading of that result, and a second spelling + * would drift from the tool that writes it. + * + * Whether the user has answered is a separate session fact (the `step-plan` + * entry's `pendingProposalHash`), because this scan cannot tell an answer from a + * question and cannot see a decision that wrote no user message. */ import type { AgentMessage } from "@step-harness/agent-core"; @@ -69,25 +72,11 @@ export function findPendingExternalPlanApproval(messages: readonly AgentMessage[ } /** - * {@link findPendingExternalPlanApproval} for a persisted session branch. + * There is deliberately no branch variant of the pending check. * - * Walks entries backwards and examines message entries in place, stopping at the - * first user message or at a summary. A compaction or branch summary is a - * boundary: the entries it replaced are no longer in the model's context, so a - * handoff behind it is not something the user can answer any more. + * Deciding whether work may continue past a handoff is the `step-plan` + * extension's `pendingProposalHash`, which a decision clears explicitly. A walk + * over the branch would answer "is a handoff near the tail" and be taken for "is + * the user's decision still outstanding" — the same two questions this file's + * header warns against conflating. */ -export function hasPendingExternalPlanApproval(manager: { - getBranch(): readonly { type: string; message?: AgentMessage }[]; -}): boolean { - const branch = manager.getBranch(); - for (let index = branch.length - 1; index >= 0; index--) { - const entry = branch[index]!; - if (entry.type === "compaction" || entry.type === "branch_summary") return false; - if (entry.type !== "message" || !entry.message) continue; - const message = entry.message; - if (message.role === "user") return false; - if (message.role !== "toolResult" || message.toolName !== "exit_plan_mode" || message.isError) continue; - if (readExternalPlanHandoff(message)) return true; - } - return false; -} diff --git a/packages/coding-agent/src/features/plan-mode-prompt.ts b/packages/coding-agent/src/features/plan-mode-prompt.ts index 3236489b7..6ad9a49f0 100644 --- a/packages/coding-agent/src/features/plan-mode-prompt.ts +++ b/packages/coding-agent/src/features/plan-mode-prompt.ts @@ -20,7 +20,7 @@ export function buildPlanModePrompt(planFilePath: string): string { The user turned on Plan Mode. Work in three phases and chat your way to a great plan before submitting it. A great plan is detailed in intent and implementation, so another engineer or agent can implement it right away. It must be decision complete: the implementer makes no decisions. ## Mode rules (strict) -- Plan Mode stays active until a later message ends it. It ends only when the user approves the proposal in review or turns plan mode off with /plan, or, in headless and RPC sessions, when exit_plan_mode hands approval to the caller. +- Plan Mode stays active until a later message ends it. It ends only when the user approves the proposal or turns plan mode off. You cannot end it yourself: submitting a proposal for review is not approval. - User intent, tone, or imperative language does not change the mode. If the user asks for execution while Plan Mode is active, treat it as a request to plan the execution, not to perform it. - A read-only question may be answered directly without a proposal. That does not end Plan Mode: a missing or unchanged plan file is not permission to exit. Only the user can turn the mode off when there is no proposal to review. - task_* tools, when available, still record progress, but a todo list is not the plan. diff --git a/packages/coding-agent/src/features/plan-mode-tools.ts b/packages/coding-agent/src/features/plan-mode-tools.ts index bf26d8751..c308e1ae3 100644 --- a/packages/coding-agent/src/features/plan-mode-tools.ts +++ b/packages/coding-agent/src/features/plan-mode-tools.ts @@ -34,6 +34,31 @@ export interface StepPlanModeController { exitPlanMode(outcome: StepPlanExitOutcome, extensionContext: ExtensionContext): void; /** Plan file path for this session (persisted or derived from the session id). */ resolvePlanFilePath(extensionContext: ExtensionContext): string; + /** + * A proposal is waiting for this user's answer. + * + * This is a session fact, not a projection: the handoff records the reviewed + * snapshot here, and every decision clears it. Hosts and the automatic-turn + * guard read it instead of inferring "still waiting" from the transcript, so + * a question typed past the proposal cannot look like an answer and an + * answered proposal cannot look pending. + */ + recordPendingProposal(planHash: string): void; + /** The hash of the proposal waiting for an answer, or undefined. */ + pendingProposalHash(): string | undefined; + /** The proposal was answered (approved, revised, or left); nothing waits now. */ + clearPendingProposal(): void; +} + +/** + * Identity of a proposal's exact text. + * + * Shared by the review that shows it and the approval that must match it: two + * spellings of "the same plan" would let a click approve text the user did not + * read. + */ +export function hashPlanContents(planContents: string): string { + return createHash("sha256").update(planContents, "utf8").digest("hex"); } /** Select a new per-session plan path; the extension retains saved paths on restore. */ @@ -134,7 +159,7 @@ export function registerPlanModeTools(pi: ExtensionAPI, planMode: StepPlanModeCo renderShell: "self", renderResult: renderPlanReviewResult, description: - "Submit the written proposal for review. Requires a readable, nonempty regular plan file. In the TUI the user approves execution, requests refinements, or dismisses; approval exits planning, while refining or dismissing keeps it active. In headless and RPC runs the proposal is handed to the caller instead: plan mode exits, this turn ends, and the caller must obtain approval before execution. If no proposal was written, answer read-only questions directly and keep plan mode active until the user turns it off.", + "Submit the written proposal for review. Requires a readable, nonempty regular plan file. The user decides; you do not. In a host with a review surface — the terminal dialog, or the web and desktop apps — the proposal is shown and this turn ends, and plan mode stays until they answer: approval leaves planning and starts the work, a request for changes arrives as a new user message, and leaving keeps the mode off. A host without that surface hands the proposal to its caller, which must obtain approval before execution. If no proposal was written, answer read-only questions directly and keep plan mode active until the user turns it off.", promptSnippet: "Submit the written proposal for review", parameters: Type.Object({}), // A decision barrier: later calls in this batch must not run behind it. @@ -162,7 +187,7 @@ export function registerPlanModeTools(pi: ExtensionAPI, planMode: StepPlanModeCo `Cannot review the plan at ${planFilePath}: ${error instanceof Error ? error.message : String(error)}. Provide a readable, nonempty regular file containing your proposal with write_file, then call exit_plan_mode again.`, ); } - const planHash = createHash("sha256").update(planContents, "utf8").digest("hex"); + const planHash = hashPlanContents(planContents); const reviewed = (outcome: PlanReviewOutcome, feedback?: string): PlanReviewDetails => ({ planFilePath, planContents, @@ -172,10 +197,31 @@ export function registerPlanModeTools(pi: ExtensionAPI, planMode: StepPlanModeCo outcome, ...(feedback === undefined ? {} : { feedback }), }); + // Every handoff leaves a proposal awaiting approval, so no automatic + // starter continues past it. A host that can ask the user keeps the fact + // until an explicit decision; one that cannot clears it on the caller's + // next message, which is the only channel it has. The terminal dialog + // answers in-process, so it never records one. + if (ctx.planReviewSurface) { + planMode.recordPendingProposal(planHash); + // The host draws the proposal and can collect an answer, which is what + // the terminal dialog does. Plan mode stays on until the user decides: + // leaving on submission would let any later message — a question as + // easily as an approval — authorize the work. The turn ends so the host + // can ask. + return reviewedResult( + `The proposal was submitted for review and this turn ends now. Do not call exit_plan_mode again in this turn; the user answers through the host, and a request for changes comes back as a new user message. Plan mode stays on, so file mutations stay limited to ${planFilePath}. Do not execute until the user approves.\n\nPlan file: ${planFilePath}\n\nPlan contents:\n${planContents}`, + reviewed("external_approval_required"), + true, + ); + } // An RPC child has a dialog bridge (hasUI is true) but no real user // behind it, so its dialog can be auto-cancelled. Exit plan mode, but end - // this turn: the caller still owns approval before execution. + // this turn: the caller still owns approval before execution. A proposal + // answered by nobody still has to hold automatic starters back, so the + // handoff records it here too. if (ctx.mode === "rpc") { + planMode.recordPendingProposal(planHash); planMode.exitPlanMode("auto_rpc", ctx); ctx.ui.notify( "RPC host has no plan-review bridge; caller approval is required before execution.", @@ -188,6 +234,7 @@ export function registerPlanModeTools(pi: ExtensionAPI, planMode: StepPlanModeCo ); } if (!ctx.hasUI) { + planMode.recordPendingProposal(planHash); planMode.exitPlanMode("auto_headless", ctx); return reviewedResult( `Plan mode exited. Plan file kept at ${planFilePath} for reference.\n\nPlan contents:\n${planContents}\n\nCannot show interactive prompt; caller must gate approval externally. This turn ends now.`, @@ -205,8 +252,7 @@ export function registerPlanModeTools(pi: ExtensionAPI, planMode: StepPlanModeCo // the click that approved the old text. let unchanged = false; try { - unchanged = - createHash("sha256").update(readPlanContents(planFilePath), "utf8").digest("hex") === planHash; + unchanged = hashPlanContents(readPlanContents(planFilePath)) === planHash; } catch { // A deleted or unreadable file cannot be the approved snapshot. } diff --git a/packages/coding-agent/src/features/step-plan.ts b/packages/coding-agent/src/features/step-plan.ts index 1f70d25c2..5014a7b0c 100644 --- a/packages/coding-agent/src/features/step-plan.ts +++ b/packages/coding-agent/src/features/step-plan.ts @@ -11,7 +11,7 @@ * cycle. */ -import { existsSync } from "node:fs"; +import { existsSync, readFileSync } from "node:fs"; import type { AgentMessage } from "@step-harness/agent-core"; import type { EventBus } from "../core/event-bus.ts"; import type { @@ -23,11 +23,11 @@ import type { import { resolveToCwd } from "../core/tools/path-utils.ts"; import { type StepTelemetryReporter, trackStepTelemetry } from "../step/telemetry.ts"; import { getThenRunCommand } from "../step/then-run.ts"; -import { hasPendingExternalPlanApproval } from "./plan-handoff.ts"; import { type LegacyPlanTodoFields, migrateLegacyPlanTodos } from "./plan-mode-migration.ts"; import { buildDefaultModePrompt, buildPlanModePrompt, PLAN_MODE_MESSAGE_TYPE } from "./plan-mode-prompt.ts"; import { getPlanFilePath, + hashPlanContents, registerPlanModeTools, type StepPlanExitOutcome, type StepPlanModeController, @@ -65,6 +65,13 @@ interface PlanState { */ toolsBeforePlanMode?: string[]; planFilePath?: string; + /** + * Hash of the proposal waiting for the user's answer. + * + * A decision fact, not a projection: set by the handoff, cleared by approval, + * revision, or leaving. Absent means nothing is waiting. + */ + pendingProposalHash?: string; /** "user" for /plan or --plan; "agent" only in sessions from the enter_plan_mode era. */ planSource?: StepPlanSource; } @@ -160,6 +167,8 @@ export interface StepPlanState { enabled: boolean; planFilePath?: string; planSource: StepPlanSource; + /** Hash of the proposal waiting for the user's answer, when one is. */ + pendingProposalHash?: string; } /** @@ -183,6 +192,9 @@ export function getStepPlanState( ...("planFilePath" in data && typeof data.planFilePath === "string" ? { planFilePath: data.planFilePath } : {}), + ...("pendingProposalHash" in data && typeof data.pendingProposalHash === "string" + ? { pendingProposalHash: data.pendingProposalHash } + : {}), }; } return undefined; @@ -336,6 +348,15 @@ export const createStepPlanExtension = let planModeAddedTools: string[] | undefined; let planFilePath: string | undefined; let planSource: StepPlanSource | undefined; + /** + * Hash of the proposal waiting for this user's answer. + * + * Set when the handoff records the reviewed snapshot, cleared by every + * decision. It is the one answer to "is a human decision pending", so the + * automatic-turn guard and the hosts read it instead of scanning the + * transcript for shape. + */ + let pendingProposal: string | undefined; // The event bus is optional so minimal embedder/test harnesses that stub // ExtensionAPI keep working without one. const events = (pi as { events?: EventBus }).events; @@ -355,6 +376,7 @@ export const createStepPlanExtension = enabled: nextEnabled, planFilePath, planSource: nextEnabled ? planSource : undefined, + ...(pendingProposal ? { pendingProposalHash: pendingProposal } : {}), ...(carriesLegacyTools ? { toolsBeforePlanMode: carriesLegacyTools } : {}), }); }; @@ -420,19 +442,58 @@ export const createStepPlanExtension = }; const exit = (outcome: StepPlanExitOutcome, ctx: ExtensionContext): void => { if (!enabled) return; - // Persist before lifting the live restriction: a failed write must leave - // the session in plan mode, not strand the branch saying it is still on. - persistPlanState(undefined, false); + // A user decision ends the wait. A handoff does not: the proposal still + // awaits the caller's approval, and the automatic-turn guard keeps + // holding queued work until that arrives. + const decided = outcome !== "auto_headless" && outcome !== "auto_rpc"; + const previousPending = pendingProposal; + if (decided) pendingProposal = undefined; + try { + // Persist before lifting the live restriction: a failed write must leave + // the session in plan mode, not strand the branch saying it is still on. + persistPlanState(undefined, false); + } catch (error) { + // The write is the record; without it the decision never happened, so + // the in-memory wait has to stay with the mode. + pendingProposal = previousPending; + throw error; + } recordPlanExit(options.telemetry, planSource, outcome); enabled = false; planSource = undefined; restoreTools(); updateStatus(ctx, enabled, planSource); }; + /** + * Leave plan mode because the user said so. + * + * Nothing else tells the model: the Default announcement only reaches its + * next request, so a run still streaming would keep going and, with the + * boundary lifted, start implementing the plan the user just rejected. End + * that work here instead of leaving it to read the mode change. + */ + const leaveByUser = (outcome: StepPlanExitOutcome, ctx: ExtensionContext): void => { + exit(outcome, ctx); + if (!ctx.isIdle()) ctx.abort(); + }; + const recordPendingProposal = (planHash: string): void => { + pendingProposal = planHash; + // Persisted with the mode: a resumed session still knows a proposal is + // waiting, and a decision made before the restart is not replayed. + persistPlanState(); + }; + const clearPendingProposal = (): void => { + if (pendingProposal === undefined) return; + pendingProposal = undefined; + persistPlanState(); + }; const planModeController: StepPlanModeController = { isPlanModeActive: () => enabled, exitPlanMode: exit, resolvePlanFilePath, + recordPendingProposal, + pendingProposalHash: () => pendingProposal, + clearPendingProposal, }; pi.registerFlag("plan", { @@ -441,16 +502,65 @@ export const createStepPlanExtension = default: false, }); pi.registerCommand("plan", { - description: "Toggle plan mode, or `/plan ` to start planning a task right away", + description: + "Toggle plan mode, `/plan ` to plan a task, `/plan off` to leave, or `/plan approve` to accept a submitted proposal", handler: async (args, ctx) => { const prompt = args.trim(); if (!prompt) { - if (enabled) { - exit("toggled_off", ctx); - ctx.ui.notify("Plan mode disabled.", "info"); - } else { - enter(ctx); + // The host draws the mode; a notice for it would be a row nobody + // typed, on the screen the user is already reading. + if (enabled) leaveByUser("toggled_off", ctx); + else enter(ctx); + return; + } + // Absolute verbs, so a host that can only send text can express + // "leave" and "approve" without racing a toggle. A queued `/plan` + // would otherwise flip whatever state the turn left behind. + if (prompt === "on") { + // The absolute counterpart of `off`, so a host never sends a toggle + // whose effect depends on the state a turn left behind. + if (!enabled) enter(ctx); + return; + } + if (prompt === "off") { + if (enabled) leaveByUser("toggled_off", ctx); + return; + } + if (prompt === "approve") { + // The user accepted the proposal the host showed them. Leaving the + // mode and starting the implementation turn are one decision: the + // only way this turn runs with file mutations enabled. + if (!enabled || pendingProposal === undefined) { + ctx.ui.notify("No plan is waiting for approval.", "warning"); + return; + } + // The approval belongs to the snapshot the user read. A file edited + // after the review cannot be approved by a click that approved other + // text, so reread it and compare — the same rule the terminal applies. + let current: string | undefined; + try { + current = hashPlanContents(readFileSync(resolvePlanFilePath(ctx), "utf8")); + } catch { + // Deleted or unreadable: not the reviewed snapshot either. } + if (current !== pendingProposal) { + clearPendingProposal(); + ctx.ui.notify("The plan changed since it was submitted; ask for a new review.", "warning"); + return; + } + exit("approved", ctx); + // The decision itself is the command; this is only the instruction it + // carries. As a user message it would read as a second thing the user + // said, next to the command row they actually sent. + pi.sendMessage( + { + customType: "step-plan-approved", + display: false, + content: + "The user approved the plan. Plan mode is off and file mutations are re-enabled. Implement it now, using the plan file as the reference; do not silently diverge from it.", + }, + { triggerTurn: true }, + ); return; } // `/plan ` is "plan this", never "toggle": make sure plan mode is on, @@ -459,7 +569,30 @@ export const createStepPlanExtension = // with the plan-mode tool set. Slash commands run even mid-stream; queue // the task behind the current turn in that case instead of throwing. if (!enabled) enter(ctx); - pi.sendUserMessage(prompt, ctx.isIdle() ? {} : { deliverAs: "followUp" }); + // A note on a proposal under review replaces it: the old snapshot is no + // longer waiting for an answer, and the model has to submit again or the + // session would wait on a proposal nobody is looking at. + const revising = pendingProposal !== undefined; + clearPendingProposal(); + // Expand a leading `/skill:` or prompt template in the task, as the same + // text sent without `/plan` would be (the web composer puts its plan + // prefix in front of a picked skill). + if (revising) { + // The "submit again" half is an instruction, not something the user + // typed: a hidden message keeps it out of their bubble. It goes first + // so it rides the same request as the notes — a follow-up queued after + // the notes would only arrive once this turn had already stopped. + pi.sendMessage({ + customType: "step-plan-revision", + display: false, + content: + "These notes revise the proposal under review. Update the plan file, then call exit_plan_mode again to submit the new proposal for approval.", + }); + } + pi.sendUserMessage(prompt, { + expandPromptTemplates: true, + ...(ctx.isIdle() ? {} : { deliverAs: "followUp" as const }), + }); }, }); registerPlanModeTools(pi, planModeController); @@ -482,6 +615,14 @@ export const createStepPlanExtension = // rewriting the system prompt or tool list. The block persists with the // turn, so later requests reuse it from the cached history. pi.on("before_agent_start", async (_event, ctx) => { + // Who can answer this proposal? + // + // A host with a review surface asks its user while the mode is on, so the + // fact stays until an explicit verb — there a question is not consent. But + // a pending left by a caller handoff (`enabled` already false) has no + // surface left to answer it, even on this host: opening such a session in + // the app must not lock it, so the next user turn is the decision. + if (pendingProposal !== undefined && (!enabled || !ctx.planReviewSurface)) clearPendingProposal(); const visible = lastAnnouncement( ctx.sessionManager.buildContextEntries().filter((entry) => entry.type === "custom_message"), ); @@ -544,6 +685,10 @@ export const createStepPlanExtension = enabled = restoredPlanState?.enabled ?? false; planFilePath = restoredPlanState?.planFilePath; planSource = enabled ? (restoredPlanState?.planSource ?? "unknown") : undefined; + // A proposal still waiting survives a restart, including a caller handoff + // that already left the mode: the guard must keep holding work until + // someone answers it, and only the answer clears this. + pendingProposal = restoredPlanState?.pendingProposalHash; if (legacyBaseline) { const baseline = unique([...legacyBaseline, ...activePlanTaskTools]); if (!sameToolOrder(pi.getActiveTools(), baseline)) pi.setActiveTools(baseline); @@ -570,10 +715,13 @@ export const createStepPlanExtension = // Two extensions in one runner receive different API objects, so the // presence marker is keyed by the session the pre-check also reads. markPlanExtensionMounted(ctx.sessionManager); - // A handoff that no user message answered must not be continued by an + // A proposal nobody has answered must not be continued past by an // automatic turn starter. The session asks this before any of them, so - // each feature does not need its own copy of the rule. - ctx.registerAutomaticTurnGuard?.(() => hasPendingExternalPlanApproval(ctx.sessionManager)); + // each feature does not need its own copy of the rule. The answer comes + // from the session fact the handoff records, not from the transcript: a + // scan cannot tell an answer from a question, and cannot see a decision + // that wrote no user message (`/plan off`). + ctx.registerAutomaticTurnGuard?.(() => pendingProposal !== undefined); restorePlanState(ctx, event.reason === "startup" && pi.getFlag("plan") === true); }); pi.on("session_tree", async (_event, ctx) => { diff --git a/packages/coding-agent/test/step-plan-extension.test.ts b/packages/coding-agent/test/step-plan-extension.test.ts index f91e02cc7..b6ed57392 100644 --- a/packages/coding-agent/test/step-plan-extension.test.ts +++ b/packages/coding-agent/test/step-plan-extension.test.ts @@ -34,6 +34,8 @@ interface Harness { appended: Array<{ customType: string; data: unknown }>; notifications: Array<{ message: string; type?: string }>; userMessages: Array>; + /** Hidden custom messages, so a test can assert an instruction is not in the user's bubble. */ + customMessages: Array>; statuses: Map; customCalls: () => number; reviewComponent: () => (Component & { handleInput?(data: string): void }) | undefined; @@ -106,6 +108,7 @@ function createHarness( const appended: Array<{ customType: string; data: unknown }> = []; const notifications: Array<{ message: string; type?: string }> = []; const userMessages: Array> = []; + const customMessages: Array> = []; const statuses = new Map(); const cwd = options.cwd ?? "/workspace"; let customCalls = 0; @@ -138,7 +141,9 @@ function createHarness( appended.push({ customType, data }); session.appendCustomEntry(customType, data); }, - sendMessage: () => {}, + sendMessage: (...args: Parameters) => { + customMessages.push(args); + }, sendUserMessage: (...args: Parameters) => { userMessages.push(args); }, @@ -205,6 +210,7 @@ function createHarness( appended, notifications, userMessages, + customMessages, statuses, customCalls: () => customCalls, reviewComponent: () => lastReviewComponent, @@ -528,14 +534,73 @@ test("plan telemetry carries the source dim across enter, update, and exit", asy // rpc children // --------------------------------------------------------------------------- -test.each([true, false])("rpc-mode exit_plan_mode hands approval to the caller (hasUI: %s)", async (hasUI) => { +test("an interactive rpc host keeps plan mode on until the user answers", async () => { + // The web and desktop apps land here: they draw the proposal themselves, so + // the tool must not leave the mode on submission. Leaving would let the next + // message — a question as easily as an approval — authorize the work. + const cwd = makeWorkspace(); + const { telemetry, events } = createTelemetryRecorder(); + const harness = createHarness({ cwd, review: [REVIEW_KEYS.enter] }); + createStepPlanExtension({ telemetry })(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + const planContents = "# The plan\n1. Do the thing\n"; + writeFileSync(planPath, planContents); + + const result = (await runTool(harness, "exit_plan_mode", {}, context)) as { + content: Array<{ type: string; text: string }>; + details?: PlanReviewDetails; + terminate?: boolean; + stopTurn?: boolean; + }; + expect(result.content[0]?.text).toContain("Plan mode stays on"); + expect(result.content[0]?.text).toContain(planContents); + expect(result.details).toEqual({ + outcome: "external_approval_required", + approvalSource: "external", + planFilePath: planPath, + planContents, + planHash: createHash("sha256").update(planContents, "utf8").digest("hex"), + planModeExited: false, + }); + expect(result.terminate).toBe(true); + expect(result.stopTurn).toBe(true); + expect(harness.userMessages).toEqual([]); + // Still planning: the mode, its tools, and the file boundary are untouched. + expect(harness.activeTools()).toContain("exit_plan_mode"); + expect(harness.statuses.get("plan-mode")).toBe("Planning (user)"); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); + expect(events.filter((event) => event.event === "plan_mode_exited")).toEqual([]); + + // The user's approval is one decision: leave the mode and start the work. + await harness.commands.get("plan")!.handler("approve", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(harness.statuses.get("plan-mode")).toBeUndefined(); + // The decision is the command; the instruction it carries is hidden, so the + // transcript does not read as two things the user said. + expect(harness.userMessages).toEqual([]); + expect( + harness.customMessages.some( + ([message]) => + message.customType === "step-plan-approved" && String(message.content).includes("Implement it now"), + ), + ).toBe(true); + expect(events.at(-1)).toEqual({ + event: "plan_mode_exited", + properties: { source: "user", outcome: "approved" }, + }); +}); + +test("an rpc child with no user behind its dialog hands approval to the caller", async () => { const cwd = makeWorkspace(); const { telemetry, events } = createTelemetryRecorder(); const harness = createHarness({ cwd, review: [REVIEW_KEYS.enter] }); const originalTools = harness.activeTools(); createStepPlanExtension({ telemetry })(harness.api); - // RPC can expose a dialog bridge (hasUI true), but approval still belongs to its caller. - const context = harness.ctx({ mode: "rpc", hasUI }); + const context = harness.ctx({ mode: "rpc", hasUI: false }); await enterPlanMode(harness, context); const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); @@ -567,7 +632,7 @@ test.each([true, false])("rpc-mode exit_plan_mode hands approval to the caller ( expect(result.stopTurn).toBe(true); expect(harness.userMessages).toEqual([]); - // Plan mode actually exited: tools restored, chip cleared, state persisted off. + // Nothing can answer inside the child, so it leaves and restores the tools. expect(harness.activeTools()).toEqual(originalTools); expect(harness.statuses.get("plan-mode")).toBeUndefined(); expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); @@ -582,6 +647,120 @@ test.each([true, false])("rpc-mode exit_plan_mode hands approval to the caller ( }); }); +test("a submitted proposal records the reviewed snapshot as the decision fact", async () => { + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + const planContents = "# Plan\n1. Do the thing\n"; + writeFileSync(planPath, planContents); + const hash = createHash("sha256").update(planContents, "utf8").digest("hex"); + + await runTool(harness, "exit_plan_mode", {}, context); + // The fact is persisted with the mode, so a restart still knows it waits. + expect(lastPlanEntry(harness)).toMatchObject({ enabled: true, pendingProposalHash: hash }); + + // Approval clears it: nothing waits, and the mutation boundary lifts. + await harness.commands.get("plan")!.handler("approve", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); + // The decision is the command; the instruction it carries is hidden, so the + // transcript does not read as two things the user said. + expect(harness.userMessages).toEqual([]); + expect( + harness.customMessages.some( + ([message]) => + message.customType === "step-plan-approved" && String(message.content).includes("Implement it now"), + ), + ).toBe(true); +}); + +test("a review requested change clears the fact so the session does not wait on the old proposal", async () => { + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + expect(lastPlanEntry(harness).pendingProposalHash).toBeDefined(); + + await harness.commands.get("plan")!.handler("split the second step", context); + // The notes replace the proposal: the old snapshot is not awaiting an answer, + // and the model is told to submit again. + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); + // The notes stay exactly what the user wrote; the "submit again" half is a + // hidden instruction, so their bubble does not grow internal text. + expect(harness.userMessages.at(-1)?.[0]).toBe("split the second step"); + expect( + harness.customMessages.some( + ([message]) => + message.customType === "step-plan-revision" && + String(message.content).includes("call exit_plan_mode again"), + ), + ).toBe(true); +}); + +test("/plan approve refuses a file edited after the review", async () => { + // Approval belongs to the snapshot the user read, the same rule the terminal + // dialog applies by rereading the file before it leaves. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + + writeFileSync(planPath, "# Plan\n1. Something else entirely\n"); + await harness.commands.get("plan")!.handler("approve", context); + + // Not approved: the mode stays on and no implementation turn starts. + expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); + expect(harness.userMessages).toEqual([]); + expect(harness.notifications.some((notice) => /plan changed since it was submitted/.test(notice.message))).toBe( + true, + ); +}); + +test("/plan off leaves without implementing, even when plan mode is already off", async () => { + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx(); + + await enterPlanMode(harness, context); + await harness.commands.get("plan")!.handler("off", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(harness.userMessages).toEqual([]); + + // Idempotent: a queued `/plan off` that arrives after the turn already left + // the mode must not turn it back on. + await harness.commands.get("plan")!.handler("off", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(harness.userMessages).toEqual([]); +}); + +test("/plan approve refuses when no proposal is waiting", async () => { + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx(); + + await harness.commands.get("plan")!.handler("approve", context); + expect(harness.userMessages).toEqual([]); + expect(harness.appended.filter((entry) => entry.customType === "step-plan")).toEqual([]); + expect(harness.notifications.some((notice) => /No plan is waiting for approval/.test(notice.message))).toBe(true); +}); + test("tui-mode exit_plan_mode still gates through the review dialog", async () => { const cwd = makeWorkspace(); const harness = createHarness({ cwd, review: [REVIEW_KEYS.escape] }); @@ -741,7 +920,7 @@ test.each(planEntryCases)( const toolsBefore = harness.activeTools(); // `/plan ` while planning hands over the task without re-entering. await harness.commands.get("plan")!.handler("refine the proposal", context); - expect(harness.userMessages.at(-1)).toEqual(["refine the proposal", {}]); + expect(harness.userMessages.at(-1)).toEqual(["refine the proposal", { expandPromptTemplates: true }]); expect(harness.appended).toHaveLength(snapshotsBefore); expect(events).toHaveLength(telemetryBefore); expect(events[0]).toEqual({ event: "plan_mode_entered", properties: { source } }); @@ -1606,7 +1785,8 @@ test.each(["tui", "print", "rpc"] as const)( const cwd = makeWorkspace(); const harness = createHarness({ cwd, review: [REVIEW_KEYS.enter] }); createStepPlanExtension()(harness.api); - const context = harness.ctx({ mode, hasUI: mode !== "print" }); + // `rpc` here is the interactive host case, which keeps planning on submit. + const context = harness.ctx({ mode, hasUI: mode !== "print", planReviewSurface: mode === "rpc" }); const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); mkdirSync(path.dirname(planPath), { recursive: true }); const proposal = `\uFEFF# Plan — café 🧪\r\n\r\n${"detail ".repeat(1000)}\r\nlast line \t\r\n`; @@ -1619,13 +1799,17 @@ test.each(["tui", "print", "rpc"] as const)( const hashes: string[] = []; for (const planContents of variants) { - await enterPlanMode(harness, context); + // An interactive rpc host stays in plan mode after each submission, so + // only enter when the previous round actually left it. + if (harness.statuses.get("plan-mode") !== "Planning (user)") await enterPlanMode(harness, context); writeFileSync(planPath, planContents); const result = (await runTool(harness, "exit_plan_mode", {}, context)) as { details?: PlanReviewDetails }; expect(result.details).toMatchObject({ planContents, planHash: createHash("sha256").update(planContents, "utf8").digest("hex"), - planModeExited: true, + // An interactive rpc host keeps planning until the user answers; a + // host with no review surface leaves on submission. + planModeExited: mode !== "rpc", }); hashes.push(result.details!.planHash!); } @@ -1830,6 +2014,32 @@ test("a step-only embedder does not inherit a plan boundary it cannot leave", as expect(result).toBeUndefined(); }); +test("leaving plan mode ends the work in flight instead of letting it implement", async () => { + // The Default announcement only reaches the model's next request, so a run + // still streaming would carry on with the plan boundary lifted — the exact + // opposite of what the user asked for by leaving. + const harness = createHarness({ cwd: makeWorkspace() }); + createStepPlanExtension()(harness.api); + const abort = vi.fn(); + const context = harness.ctx({ isIdle: () => false, abort }); + await harness.commands.get("plan")!.handler("", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); + + await harness.commands.get("plan")!.handler("off", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(abort).toHaveBeenCalledTimes(1); +}); + +test("leaving plan mode while idle does not abort anything", async () => { + const harness = createHarness({ cwd: makeWorkspace() }); + createStepPlanExtension()(harness.api); + const abort = vi.fn(); + const context = harness.ctx({ isIdle: () => true, abort }); + await harness.commands.get("plan")!.handler("", context); + await harness.commands.get("plan")!.handler("off", context); + expect(abort).not.toHaveBeenCalled(); +}); + test("a failed enter persist leaves plan mode off", async () => { const harness = createHarness({ cwd: makeWorkspace() }); createStepPlanExtension()(harness.api); @@ -1859,11 +2069,12 @@ test("exit_plan_mode describes review and mode-specific approval rather than unc expect(tool.executionMode).toBe("sequential"); expect(tool.description).toMatch(/^Submit .*proposal.*review/); expect(tool.description).toContain("readable, nonempty regular plan file"); - expect(tool.description).toContain("In the TUI the user approves execution"); - expect(tool.description).toContain("refining or dismissing keeps it active"); - expect(tool.description).toContain("In headless and RPC runs"); - expect(tool.description).toContain("this turn ends"); - expect(tool.description).toContain("caller must obtain approval before execution"); + // One rule for every host: the user decides, the model cannot. + expect(tool.description).toContain("The user decides; you do not"); + expect(tool.description).toContain("approval leaves planning and starts the work"); + expect(tool.description).toContain("a request for changes arrives as a new user message"); + expect(tool.description).toContain("hands the proposal to its caller"); + expect(tool.description).toContain("must obtain approval before execution"); expect(tool.promptSnippet).toMatch(/^Submit .*proposal.*review/); }); @@ -1880,7 +2091,7 @@ test("/plan with a task enables plan mode and sends the task as a user turn", as await harness.commands.get("plan")!.handler("实现一个俄罗斯方块,简单版", context); expect(harness.statuses.get("plan-mode")).toBe("Planning (user)"); - expect(harness.userMessages).toEqual([["实现一个俄罗斯方块,简单版", {}]]); + expect(harness.userMessages).toEqual([["实现一个俄罗斯方块,简单版", { expandPromptTemplates: true }]]); // The turn must start under the plan-mode tool set, not the unrestricted one. expect(harness.activeTools()).not.toContain("edit_file_unrestricted"); expect(harness.activeTools()).toContain("read_file"); @@ -1906,7 +2117,7 @@ test("/plan with a task queues as a follow-up while the agent is streaming", asy await harness.commands.get("plan")!.handler("plan this too", harness.ctx({ isIdle: () => false })); - expect(harness.userMessages).toEqual([["plan this too", { deliverAs: "followUp" }]]); + expect(harness.userMessages).toEqual([["plan this too", { expandPromptTemplates: true, deliverAs: "followUp" }]]); }); test("bare /plan still toggles plan mode and sends nothing", async () => { @@ -1993,7 +2204,7 @@ test("/plan announces plan mode on the turn that carries the task", async const harness = createHarness({ cwd: makeWorkspace() }); createStepPlanExtension()(harness.api); await harness.commands.get("plan")!.handler("plan the parser rewrite", harness.ctx()); - expect(harness.userMessages).toEqual([["plan the parser rewrite", {}]]); + expect(harness.userMessages).toEqual([["plan the parser rewrite", { expandPromptTemplates: true }]]); expect((await startTurn(harness))?.details.mode).toBe("plan"); }); diff --git a/packages/coding-agent/test/suite/plan-approval-gate.test.ts b/packages/coding-agent/test/suite/plan-approval-gate.test.ts index 2347c0132..8f6f19366 100644 --- a/packages/coding-agent/test/suite/plan-approval-gate.test.ts +++ b/packages/coding-agent/test/suite/plan-approval-gate.test.ts @@ -149,40 +149,46 @@ test("a reload does not leave an automatic-turn guard stranded", async () => { harness.setResponses([fauxAssistantMessage("after reload")]); await harness.session.reload(); expect(harness.session.hasPendingUserDecision).toBe(false); +}); - // The new generation registers its own guard: an unanswered handoff on the - // restored branch is gated again. The handoff is appended directly because the - // reload also drops the test provider registration. - harness.sessionManager.appendMessage({ - role: "toolResult", - toolCallId: "plan-after-reload", - toolName: "exit_plan_mode", - content: [{ type: "text", text: "Caller must approve." }], - details: { outcome: "external_approval_required" }, - isError: false, - timestamp: Date.now(), - } as unknown as Parameters[0]); - expect(harness.session.hasPendingUserDecision).toBe(true); - - harness.sessionManager.appendMessage({ - role: "user", - content: [{ type: "text", text: "Approved." }], - timestamp: Date.now(), - } as unknown as Parameters[0]); +test("a caller handoff leaves the decision pending across a resume", async () => { + // A handoff that leaves the mode still has an unanswered proposal, and the + // guard must keep holding automatic work after a restart: only the answer + // clears it. + const { harness, planPath } = await createPlanHarness("step-plan-gate-resume-"); + await handoff(harness, planPath); + const planEntry = harness.sessionManager + .getBranch() + .flatMap((entry) => + entry.type === "custom" && entry.customType === "step-plan" ? [(entry as { data?: unknown }).data] : [], + ) + .at(-1); + // The handoff left the mode but the proposal is still unanswered, so the fact + // survives with `enabled: false` — that is what keeps the guard holding. + expect(planEntry).toMatchObject({ enabled: false, pendingProposalHash: expect.any(String) }); + + // The guard keeps holding automatic work until someone answers, and only an + // answer clears the fact. + harness.setResponses([fauxAssistantMessage("implemented")]); + await harness.session.prompt("Approved."); expect(harness.session.hasPendingUserDecision).toBe(false); }); -test("a compaction ends the wait instead of leaving a phantom decision", async () => { +test("a compaction does not leave a decision nobody can answer", async () => { const { harness, planPath } = await createPlanHarness("step-plan-gate-compact-"); await handoff(harness, planPath); expect(harness.session.hasPendingUserDecision).toBe(true); - // The summary replaces the history that held the handoff, so it is no longer - // something the user can answer. Leaving the gate closed would stall goal, - // cron, and lane wake-ups with nothing to answer. + // The decision waits in the `step-plan` entry, not in the summarized history, + // so compaction changes neither answer nor wait. What must not happen is a + // decided proposal coming back after a summary. const firstMessage = harness.sessionManager.getBranch().find((entry) => entry.type === "message"); expect(firstMessage).toBeDefined(); harness.sessionManager.appendCompaction("Earlier work summarized.", firstMessage!.id, 1_000); + expect(harness.session.hasPendingUserDecision).toBe(true); + + harness.setResponses([fauxAssistantMessage("implemented")]); + await harness.session.prompt("Approved."); expect(harness.session.hasPendingUserDecision).toBe(false); }); From 8ae8f472dba2a7394ead058ab9aa1fe05ae31712 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 2 Nov 2026 21:31:18 +0800 Subject: [PATCH 02/15] fix(coding-agent): keep the decision fact consistent under failure and 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 ` 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. --- apps/cli/src/ui/interactive-mode.ts | 3 + docs/plan-approval.md | 30 +- packages/coding-agent/docs/extensions.md | 16 +- .../coding-agent/src/core/extensions/types.ts | 4 +- .../coding-agent/src/features/plan-handoff.ts | 60 +++- .../src/features/plan-mode-prompt.ts | 2 +- .../src/features/plan-mode-tools.ts | 7 +- .../coding-agent/src/features/step-plan.ts | 115 ++++++-- .../coding-agent/src/modes/rpc/rpc-mode.ts | 12 +- packages/coding-agent/src/step/stdio-host.ts | 13 +- .../extension-plan-review-surface.test.ts | 43 +++ .../coding-agent/test/plan-handoff.test.ts | 40 +++ .../test/runtime-info-extension.test.ts | 1 + .../test/step-plan-extension.test.ts | 258 +++++++++++++++++- .../test/suite/plan-approval-gate.test.ts | 2 + .../test/trigger-compact-extension.test.ts | 1 + 16 files changed, 539 insertions(+), 68 deletions(-) create mode 100644 packages/coding-agent/test/extension-plan-review-surface.test.ts create mode 100644 packages/coding-agent/test/plan-handoff.test.ts diff --git a/apps/cli/src/ui/interactive-mode.ts b/apps/cli/src/ui/interactive-mode.ts index 180975677..08e3e92ca 100644 --- a/apps/cli/src/ui/interactive-mode.ts +++ b/apps/cli/src/ui/interactive-mode.ts @@ -2461,6 +2461,9 @@ export class InteractiveMode { ui: this.createExtensionUIContext(), mode: "tui", hasUI: true, + // The terminal answers a plan review inside the tool call, so it never + // leaves a proposal waiting for a decision in the session. + planReviewSurface: false, cwd: this.sessionManager.getCwd(), sessionManager: this.sessionManager, modelRegistry: extensionRunner.getModelRegistry(), diff --git a/docs/plan-approval.md b/docs/plan-approval.md index e975eee45..37989d763 100644 --- a/docs/plan-approval.md +++ b/docs/plan-approval.md @@ -96,8 +96,8 @@ until the user answers. Neither means approval was granted. Every handoff ends t turn: the result sets `stopTurn` in addition to `terminate`. `approvalSource` identifies who owns the decision: `"user"` for the TUI review, -including feedback and dismissal, and `"external"` for the headless / RPC -caller's approval flow. It does not indicate that approval has occurred. Source +including feedback and dismissal, and for a review-surface host whose person +answers in the app; `"external"` for the headless / RPC caller's approval flow. It does not indicate that approval has occurred. Source and outcome are independent: a future caller-owned `"approved"` outcome can keep `approvalSource: "external"` without claiming the user clicked the TUI. Native headless/RPC exits currently emit `"external_approval_required"` only. @@ -153,7 +153,7 @@ Native outcomes keep their existing behavior: | `approved` | `user` | `true` | The user selected execution; restore tools and continue. | | `feedback` | `user` | `false` | Keep planning and steer the user's notes into the next response. | | `dismissed` | `user` | `false` | Keep planning and stop the turn to wait for the user. | -| `external_approval_required` | `external` | `true` / `false` | The turn ends. `true` for a caller that left the mode; `false` for a review-surface host that keeps planning until its user answers. | +| `external_approval_required` | `external` / `user` | `true` / `false` | The turn ends. `true` for a caller that left the mode (`external`); `false` for a review-surface host that keeps planning until its user answers (`user`). | The TUI still selects execution by default, and Enter confirms that selection. Interactive result details also include the hash of the reviewed snapshot. @@ -191,7 +191,12 @@ a host that must not proceed without approval keeps that gate on its own side which is why the headless and RPC handoffs are gated by the caller. Inside the session, the plan extension registers one automatic-turn guard with -`ctx.registerAutomaticTurnGuard`: a handoff that no user message has answered yet. +`ctx.registerAutomaticTurnGuard`: is a proposal still waiting for its decision. +The answer is the session fact `pendingProposalHash`, not the shape of the +transcript. A decision clears it — `/plan approve`, a request for changes, or +leaving plan mode — while a question typed past the proposal does not: on a host +with a review surface the user is still deciding, and on a caller handoff the +next turn owns the decision. Guards belong to the extension generation that registered them, so a reload drops the stale ones along with the runner whose context they capture. The session asks that guard before every turn it would start on the user's @@ -207,8 +212,15 @@ decision: cron keeps the job due, and a goal continuation stays pending. A user's own prompt always runs: that message is how the decision gets answered. A host bridge forwarding a real person's input reports its own `source`, so the message is not mistaken for extension-generated text. -The guard only recognizes *some* user message, because the approval belongs to -the caller and the session must not guess at its wording. A host that needs a -real decision records it on the tool result (the `tool_result` hook) or uses its -own approval channel; a follow-up question typed by the user also clears the -gate. +The guard never guesses from wording. On a host without a review surface the +decision belongs to the caller, and the only channel it has is a message; the +first turn after the handoff therefore settles the wait, and the guard stays +armed until then. A caller that wants the wait ended without writing a message +sends `/plan off` (leave without implementing) or `/plan approve` (accept the +proposal), and `/plan approve ` binds the decision to the snapshot the +caller actually read. + +A session written before the decision fact recorded its wait only in the +transcript. `restorePlanState` migrates it once, from the still-unanswered +handoff at the tail of the branch, so upgrading does not lift the gate on a +proposal nobody answered. diff --git a/packages/coding-agent/docs/extensions.md b/packages/coding-agent/docs/extensions.md index d13846b13..f00c75343 100644 --- a/packages/coding-agent/docs/extensions.md +++ b/packages/coding-agent/docs/extensions.md @@ -974,6 +974,12 @@ Current run mode: `"tui"`, `"rpc"`, `"json"`, or `"print"`. Use `ctx.mode === "t `true` in TUI and RPC modes. `false` in print mode (`-p`) and JSON mode. Use this to guard dialog methods (`select`, `confirm`, `input`, `editor`) and fire-and-forget methods (`notify`, `setStatus`, `setWidget`, `setTitle`, `setEditorText`) that work in both TUI and RPC modes. In RPC mode, some TUI-specific methods are no-ops or return defaults (see [rpc.md](rpc.md#extension-ui-protocol)). +### ctx.planReviewSurface + +`true` when the host can show a plan proposal and collect a decision for it. `mode` and `hasUI` cannot answer this: every RPC host has a dialog bridge, including clients with no review surface and no person to answer. + +`exit_plan_mode` reads it. With the surface, the proposal is shown, the turn ends, and plan mode stays on until the user decides; without it, the mode ends and approval belongs to the caller. Hosts set it when they bind the session (`bindExtensions({ planReviewSurface: true })`); an extension sees the value on every handler's `ctx`. + ### ctx.cwd Current working directory. @@ -3007,11 +3013,11 @@ const highlighted = highlightCode(code, lang, theme); ## Mode Behavior -| Mode | `ctx.mode` | `ctx.hasUI` | Notes | -|------|------------|-------------|-------| -| Interactive | `"tui"` | `true` | Full TUI with terminal rendering | -| RPC (`--mode rpc`) | `"rpc"` | `true` | Dialogs and notifications via JSON protocol; `custom()` returns `undefined`. See [rpc.md](rpc.md) | -| JSON (`--mode json`) | `"json"` | `false` | Event stream to stdout; UI methods are no-ops | +| Mode | `ctx.mode` | `ctx.hasUI` | `ctx.planReviewSurface` | Notes | +|------|------------|-------------|------------------------|-------| +| Interactive | `"tui"` | `true` | `false` | Full TUI with terminal rendering; a plan review is answered inside the tool call | +| RPC (`--mode rpc`) | `"rpc"` | `true` | set by the host | Dialogs and notifications via JSON protocol; `custom()` returns `undefined`. See [rpc.md](rpc.md) | +| JSON (`--mode json`) | `"json"` | `false` | `false` | Event stream to stdout; UI methods are no-ops | | Print (`-p`) | `"print"` | `false` | Extensions run but can't prompt | Use `ctx.mode === "tui"` before TUI-specific features (`custom()`, component factories, terminal input). Use `ctx.hasUI` before dialog and notification methods that work in both TUI and RPC modes. diff --git a/packages/coding-agent/src/core/extensions/types.ts b/packages/coding-agent/src/core/extensions/types.ts index ca3143e67..73ea28519 100644 --- a/packages/coding-agent/src/core/extensions/types.ts +++ b/packages/coding-agent/src/core/extensions/types.ts @@ -400,8 +400,10 @@ export interface ExtensionContext { * * Unlike `hasUI`, this is about one specific surface, not about having a * dialog bridge at all: an rpc child has the bridge but no user to answer. + * A host that cannot ask leaves this false, and the tool hands the proposal + * to its caller as before. */ - planReviewSurface?: boolean; + planReviewSurface: boolean; /** The current abort signal, or undefined when the agent is not streaming. */ signal: AbortSignal | undefined; /** Abort the current agent operation */ diff --git a/packages/coding-agent/src/features/plan-handoff.ts b/packages/coding-agent/src/features/plan-handoff.ts index 7372fd720..acb975bf4 100644 --- a/packages/coding-agent/src/features/plan-handoff.ts +++ b/packages/coding-agent/src/features/plan-handoff.ts @@ -17,6 +17,8 @@ import type { AgentMessage } from "@step-harness/agent-core"; export interface ExternalPlanHandoff { /** Model-facing handoff text: the explanation plus the reviewed plan. */ text: string; + /** Hash of the reviewed snapshot, when the result recorded one. */ + planHash?: string; } /** @@ -35,8 +37,10 @@ export function readExternalPlanHandoff(result: unknown): ExternalPlanHandoff | ) { return undefined; } + const planHash = (details as { planHash?: unknown }).planHash; + const hash = typeof planHash === "string" ? { planHash } : {}; const content = (result as { content?: unknown }).content; - if (!Array.isArray(content)) return { text: "" }; + if (!Array.isArray(content)) return { text: "", ...hash }; const text = content .filter((part): part is { type: "text"; text: string } => { if (typeof part !== "object" || part === null) return false; @@ -46,7 +50,7 @@ export function readExternalPlanHandoff(result: unknown): ExternalPlanHandoff | .map((part) => part.text) .join("\n") .trim(); - return { text }; + return { text, ...hash }; } /** @@ -72,11 +76,51 @@ export function findPendingExternalPlanApproval(messages: readonly AgentMessage[ } /** - * There is deliberately no branch variant of the pending check. + * The proposal a caller should be shown instead of the assistant's last line. * - * Deciding whether work may continue past a handoff is the `step-plan` - * extension's `pendingProposalHash`, which a decision clears explicitly. A walk - * over the branch would answer "is a handoff near the tail" and be taken for "is - * the user's decision still outstanding" — the same two questions this file's - * header warns against conflating. + * A handoff ends the turn on its tool result, so the last assistant message only + * introduces the submission. But "still unanswered" is the session fact, not the + * shape of the transcript: an approval that writes no user message — a decision + * made in a host that draws the card — would leave a scan reporting the old + * proposal forever. */ +export function pendingHandoffText( + state: { pendingProposalHash?: string } | undefined, + messages: readonly AgentMessage[], +): string | undefined { + if (state?.pendingProposalHash === undefined) return undefined; + return findPendingExternalPlanApproval(messages)?.text; +} + +/** + * The handoff a session written before the decision fact still leaves open. + * + * Only `restorePlanState` calls this, and only for an entry older than the + * `pendingProposalHash` field: those builds recorded the wait in the transcript + * alone, so a resumed session would otherwise resume with no gate at all. An + * entry written by this build always carries the fact, which a decision clears + * explicitly, and nothing else consults the transcript again. + * + * A `step-plan` entry after the handoff means the mode changed after it — the + * user left, or turned planning back on — and that is not a decision this + * function may replay. + */ +export function findLegacyUnansweredHandoff(manager: { + getBranch(): readonly { type: string; message?: AgentMessage; customType?: string }[]; +}): ExternalPlanHandoff | undefined { + const branch = manager.getBranch(); + for (let index = branch.length - 1; index >= 0; index--) { + const entry = branch[index]!; + if (entry.type === "compaction" || entry.type === "branch_summary") return undefined; + // A plan entry above the handoff is a later transition: the handoff's own + // exit is written before its result, so it is still below. + if (entry.type === "custom" && entry.customType === "step-plan") return undefined; + if (entry.type !== "message" || !entry.message) continue; + const message = entry.message; + if (message.role === "user") return undefined; + if (message.role !== "toolResult" || message.toolName !== "exit_plan_mode" || message.isError) continue; + const handoff = readExternalPlanHandoff(message); + if (handoff) return handoff; + } + return undefined; +} diff --git a/packages/coding-agent/src/features/plan-mode-prompt.ts b/packages/coding-agent/src/features/plan-mode-prompt.ts index 6ad9a49f0..68f3c46a5 100644 --- a/packages/coding-agent/src/features/plan-mode-prompt.ts +++ b/packages/coding-agent/src/features/plan-mode-prompt.ts @@ -20,7 +20,7 @@ export function buildPlanModePrompt(planFilePath: string): string { The user turned on Plan Mode. Work in three phases and chat your way to a great plan before submitting it. A great plan is detailed in intent and implementation, so another engineer or agent can implement it right away. It must be decision complete: the implementer makes no decisions. ## Mode rules (strict) -- Plan Mode stays active until a later message ends it. It ends only when the user approves the proposal or turns plan mode off. You cannot end it yourself: submitting a proposal for review is not approval. +- Plan Mode stays active until a later message ends it. It ends when the user approves the proposal or turns plan mode off; a host that cannot show the proposal may end it when handing it to its caller. You cannot end it yourself: submitting a proposal for review is not approval. - User intent, tone, or imperative language does not change the mode. If the user asks for execution while Plan Mode is active, treat it as a request to plan the execution, not to perform it. - A read-only question may be answered directly without a proposal. That does not end Plan Mode: a missing or unchanged plan file is not permission to exit. Only the user can turn the mode off when there is no proposal to review. - task_* tools, when available, still record progress, but a todo list is not the plan. diff --git a/packages/coding-agent/src/features/plan-mode-tools.ts b/packages/coding-agent/src/features/plan-mode-tools.ts index c308e1ae3..c4d5b85b0 100644 --- a/packages/coding-agent/src/features/plan-mode-tools.ts +++ b/packages/coding-agent/src/features/plan-mode-tools.ts @@ -159,7 +159,7 @@ export function registerPlanModeTools(pi: ExtensionAPI, planMode: StepPlanModeCo renderShell: "self", renderResult: renderPlanReviewResult, description: - "Submit the written proposal for review. Requires a readable, nonempty regular plan file. The user decides; you do not. In a host with a review surface — the terminal dialog, or the web and desktop apps — the proposal is shown and this turn ends, and plan mode stays until they answer: approval leaves planning and starts the work, a request for changes arrives as a new user message, and leaving keeps the mode off. A host without that surface hands the proposal to its caller, which must obtain approval before execution. If no proposal was written, answer read-only questions directly and keep plan mode active until the user turns it off.", + "Submit the written proposal for review. Requires a readable, nonempty regular plan file. The user decides; you do not. In a host that can show the proposal and collect a decision — the terminal dialog is one — the proposal is shown and this turn ends, and plan mode stays until they answer: approval leaves planning and starts the work, a request for changes arrives as a new user message, and leaving keeps the mode off. A host without that surface hands the proposal to its caller, which must obtain approval before execution. If no proposal was written, answer read-only questions directly and keep plan mode active until the user turns it off.", promptSnippet: "Submit the written proposal for review", parameters: Type.Object({}), // A decision barrier: later calls in this batch must not run behind it. @@ -193,7 +193,10 @@ export function registerPlanModeTools(pi: ExtensionAPI, planMode: StepPlanModeCo planContents, planHash, planModeExited: !planMode.isPlanModeActive(), - approvalSource: ctx.mode === "rpc" || !ctx.hasUI ? "external" : "user", + // Who owns the decision, not what carries it. A host that draws the + // proposal has a person answering, whatever transport it uses; without + // that surface the caller owns approval. + approvalSource: ctx.planReviewSurface || (ctx.mode !== "rpc" && ctx.hasUI) ? "user" : "external", outcome, ...(feedback === undefined ? {} : { feedback }), }); diff --git a/packages/coding-agent/src/features/step-plan.ts b/packages/coding-agent/src/features/step-plan.ts index 5014a7b0c..a97266ee4 100644 --- a/packages/coding-agent/src/features/step-plan.ts +++ b/packages/coding-agent/src/features/step-plan.ts @@ -23,6 +23,7 @@ import type { import { resolveToCwd } from "../core/tools/path-utils.ts"; import { type StepTelemetryReporter, trackStepTelemetry } from "../step/telemetry.ts"; import { getThenRunCommand } from "../step/then-run.ts"; +import { findLegacyUnansweredHandoff } from "./plan-handoff.ts"; import { type LegacyPlanTodoFields, migrateLegacyPlanTodos } from "./plan-mode-migration.ts"; import { buildDefaultModePrompt, buildPlanModePrompt, PLAN_MODE_MESSAGE_TYPE } from "./plan-mode-prompt.ts"; import { @@ -54,8 +55,12 @@ const PLAN_TASK_TOOLS = new Set(["exit_plan_mode", "task_create", "task_update", export type StepPlanSource = "user" | "agent" | "unknown"; interface PlanState { - /** Written by this build. Its presence marks an entry that never narrowed the tool list. */ - version?: 2; + /** + * Format of this entry. 2 marks an entry that never narrowed the tool list; + * 3 also records the decision fact, so a wait left by an older build — which + * only the transcript could describe — is migrated once at restore. + */ + version?: 2 | 3; enabled: boolean; /** * Legacy only: the list a narrowing entry replaced. Older builds dropped the @@ -372,7 +377,7 @@ export const createStepPlanExtension = */ const persistPlanState = (carriesLegacyTools?: string[], nextEnabled = enabled): void => { pi.appendEntry("step-plan", { - version: 2, + version: 3, enabled: nextEnabled, planFilePath, planSource: nextEnabled ? planSource : undefined, @@ -419,6 +424,11 @@ export const createStepPlanExtension = const enter = (ctx: ExtensionContext): void => { markPlanExtensionMounted(ctx.sessionManager); const previousPath = planFilePath; + // Entering is not a decision on a proposal from an earlier round. A fact + // left by a caller handoff would otherwise be approvable against a + // snapshot the user is no longer looking at. + const previousPending = pendingProposal; + pendingProposal = undefined; enabled = true; planSource = "user"; planFilePath = resolvePlanFilePath(ctx); @@ -430,6 +440,7 @@ export const createStepPlanExtension = enabled = false; planSource = undefined; planFilePath = previousPath; + pendingProposal = previousPending; throw error; } enable(); @@ -473,19 +484,35 @@ export const createStepPlanExtension = * that work here instead of leaving it to read the mode change. */ const leaveByUser = (outcome: StepPlanExitOutcome, ctx: ExtensionContext): void => { + // `exit` is a no-op outside the mode, and stopping a run the user never + // put into plan mode would cut an ordinary answer short. + if (!enabled) return; exit(outcome, ctx); if (!ctx.isIdle()) ctx.abort(); }; - const recordPendingProposal = (planHash: string): void => { - pendingProposal = planHash; - // Persisted with the mode: a resumed session still knows a proposal is - // waiting, and a decision made before the restart is not replayed. - persistPlanState(); + /** + * Late decisions must not be applied to the wrong proposal. + * + * The fact is written with the mode. A failed write leaves the branch + * saying something else, so the memory cannot keep the new value: a wait + * nobody recorded would hold every automatic turn back with no way to see + * why, and a decision nobody recorded would let work past a plan the user + * never approved. + */ + const writePendingProposal = (next: string | undefined): void => { + const previous = pendingProposal; + pendingProposal = next; + try { + persistPlanState(); + } catch (error) { + pendingProposal = previous; + throw error; + } }; + const recordPendingProposal = (planHash: string): void => writePendingProposal(planHash); const clearPendingProposal = (): void => { if (pendingProposal === undefined) return; - pendingProposal = undefined; - persistPlanState(); + writePendingProposal(undefined); }; const planModeController: StepPlanModeController = { isPlanModeActive: () => enabled, @@ -523,17 +550,33 @@ export const createStepPlanExtension = return; } if (prompt === "off") { + // Leaving is a decision even when the mode already ended: a caller + // handoff leaves the proposal waiting, and the user saying "off" is + // the answer that releases it. if (enabled) leaveByUser("toggled_off", ctx); + else clearPendingProposal(); return; } - if (prompt === "approve") { + // `/plan approve` answers the proposal, `/plan approve ` answers + // that one. Any other text after the verb is a task, not a decision. + const namedProposal = /^approve\s+(.+)$/.exec(prompt)?.[1]?.trim(); + if (prompt === "approve" || namedProposal !== undefined) { // The user accepted the proposal the host showed them. Leaving the // mode and starting the implementation turn are one decision: the // only way this turn runs with file mutations enabled. - if (!enabled || pendingProposal === undefined) { + // + // The verb may name the proposal it answers. A host that draws the + // card knows which snapshot the user clicked, and 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. + if (pendingProposal === undefined) { ctx.ui.notify("No plan is waiting for approval.", "warning"); return; } + if (namedProposal !== undefined && namedProposal !== pendingProposal) { + ctx.ui.notify("That proposal is no longer waiting for approval.", "warning"); + return; + } // The approval belongs to the snapshot the user read. A file edited // after the review cannot be approved by a click that approved other // text, so reread it and compare — the same rule the terminal applies. @@ -544,11 +587,15 @@ export const createStepPlanExtension = // Deleted or unreadable: not the reviewed snapshot either. } if (current !== pendingProposal) { - clearPendingProposal(); + // Nothing was decided: the wait stays, so automatic work keeps + // holding until someone reviews the text that is on disk now. ctx.ui.notify("The plan changed since it was submitted; ask for a new review.", "warning"); return; } - exit("approved", ctx); + // A caller handoff already left the mode; the caller answering in + // text is the decision it was waiting for. + if (enabled) exit("approved", ctx); + else clearPendingProposal(); // The decision itself is the command; this is only the instruction it // carries. As a user message it would read as a second thing the user // said, next to the command row they actually sent. @@ -574,24 +621,32 @@ export const createStepPlanExtension = // session would wait on a proposal nobody is looking at. const revising = pendingProposal !== undefined; clearPendingProposal(); - // Expand a leading `/skill:` or prompt template in the task, as the same - // text sent without `/plan` would be (the web composer puts its plan - // prefix in front of a picked skill). + // Both halves travel on the same channel. Left to the default, a hidden + // message sent mid-run is steered into the current request while the + // notes wait in the follow-up queue — the model would be told to + // resubmit before it had read what to change, and the notes would arrive + // after the turn had already stopped for approval. + const queued = !ctx.isIdle(); if (revising) { // The "submit again" half is an instruction, not something the user - // typed: a hidden message keeps it out of their bubble. It goes first - // so it rides the same request as the notes — a follow-up queued after - // the notes would only arrive once this turn had already stopped. - pi.sendMessage({ - customType: "step-plan-revision", - display: false, - content: - "These notes revise the proposal under review. Update the plan file, then call exit_plan_mode again to submit the new proposal for approval.", - }); + // typed: a hidden message keeps it out of their bubble, and going + // first keeps it ahead of the notes in the same request. + pi.sendMessage( + { + customType: "step-plan-revision", + display: false, + content: + "These notes revise the proposal under review. Update the plan file, then call exit_plan_mode again to submit the new proposal for approval.", + }, + queued ? { deliverAs: "followUp" } : undefined, + ); } + // Expand a leading `/skill:` or prompt template in the task, as the same + // text sent without `/plan` would be (the web composer puts its plan + // prefix in front of a picked skill). pi.sendUserMessage(prompt, { expandPromptTemplates: true, - ...(ctx.isIdle() ? {} : { deliverAs: "followUp" as const }), + ...(queued ? { deliverAs: "followUp" as const } : {}), }); }, }); @@ -689,6 +744,12 @@ export const createStepPlanExtension = // that already left the mode: the guard must keep holding work until // someone answers it, and only the answer clears this. pendingProposal = restoredPlanState?.pendingProposalHash; + // An entry older than the fact recorded the wait only in the transcript. + // Migrate it once, so upgrading does not silently lift the gate on a + // session whose proposal nobody has answered. + if (restoredPlanState && (restoredPlanState.version ?? 2) < 3) { + pendingProposal = findLegacyUnansweredHandoff(ctx.sessionManager)?.planHash; + } if (legacyBaseline) { const baseline = unique([...legacyBaseline, ...activePlanTaskTools]); if (!sameToolOrder(pi.getActiveTools(), baseline)) pi.setActiveTools(baseline); diff --git a/packages/coding-agent/src/modes/rpc/rpc-mode.ts b/packages/coding-agent/src/modes/rpc/rpc-mode.ts index a29362721..37dfbc4db 100644 --- a/packages/coding-agent/src/modes/rpc/rpc-mode.ts +++ b/packages/coding-agent/src/modes/rpc/rpc-mode.ts @@ -25,7 +25,8 @@ import { waitForRawStdoutBackpressure, writeRawStdout, } from "../../core/output-guard.ts"; -import { findPendingExternalPlanApproval } from "../../features/plan-handoff.ts"; +import { pendingHandoffText } from "../../features/plan-handoff.ts"; +import { getStepPlanState } from "../../features/step-plan.ts"; import { type Theme, theme } from "../../theme/theme.ts"; import { killTrackedDetachedChildren } from "../../utils/shell.ts"; import { toJsonEvent } from "../json-event.ts"; @@ -664,11 +665,10 @@ export async function runRpcMode(runtimeHost: AgentSessionRuntimeHost, options: } case "get_last_assistant_text": { - // A plan handoff ends the turn on its tool result, so the last - // assistant message only introduces the submission. The caller needs - // the proposal it has to approve, not that line. - const pendingHandoff = findPendingExternalPlanApproval(session.messages); - const text = pendingHandoff ? pendingHandoff.text : session.getLastAssistantText(); + // The caller needs the proposal it has to approve, not the line that + // introduced it — but only while the decision fact says one is waiting. + const pending = pendingHandoffText(getStepPlanState(session.sessionManager.getBranch()), session.messages); + const text = pending ?? session.getLastAssistantText(); return success(id, "get_last_assistant_text", { text }); } diff --git a/packages/coding-agent/src/step/stdio-host.ts b/packages/coding-agent/src/step/stdio-host.ts index 6f54c15d4..3bfb71b68 100644 --- a/packages/coding-agent/src/step/stdio-host.ts +++ b/packages/coding-agent/src/step/stdio-host.ts @@ -24,7 +24,8 @@ import type { WorkingIndicatorOptions, } from "../core/extensions/types.ts"; import { isToolCommandFailure } from "../core/tools/command-outcome.ts"; -import { findPendingExternalPlanApproval } from "../features/plan-handoff.ts"; +import { pendingHandoffText } from "../features/plan-handoff.ts"; +import { getStepPlanState } from "../features/step-plan.ts"; import type { Theme } from "../theme/theme.ts"; import { STEP_DEFAULT_PROVIDER } from "./defaults.ts"; import { listStepSessions } from "./session.ts"; @@ -664,9 +665,13 @@ export class StepStdioHost { // A plan handoff ends the turn on its tool result, so the query can settle // with no assistant text at all, or with an assistant line that introduces // the submission. Reporting either as the result would hide the proposal the - // caller has to approve, so an unanswered handoff wins. - const pendingHandoff = findPendingExternalPlanApproval(this.#session.messages); - const text = (pendingHandoff ? pendingHandoff.text : this.#session.getLastAssistantText()) ?? ""; + // caller has to approve, so the proposal wins while the decision fact says it + // is still waiting. + const pending = pendingHandoffText( + getStepPlanState(this.#session.sessionManager.getBranch()), + this.#session.messages, + ); + const text = pending ?? this.#session.getLastAssistantText() ?? ""; this.#emitMessage(query, { type: "result", subtype: isError ? "error_during_execution" : "success", diff --git a/packages/coding-agent/test/extension-plan-review-surface.test.ts b/packages/coding-agent/test/extension-plan-review-surface.test.ts new file mode 100644 index 000000000..eaecef3f4 --- /dev/null +++ b/packages/coding-agent/test/extension-plan-review-surface.test.ts @@ -0,0 +1,43 @@ +import { afterEach, beforeEach, expect, test, vi } from "vitest"; +import type { ExtensionContext } from "../src/core/extensions/types.ts"; +import { initTheme } from "../src/theme/theme.ts"; +import { createHarness, type Harness } from "./suite/harness.ts"; + +const harnesses: Harness[] = []; + +beforeEach(() => { + initTheme("dark"); + vi.spyOn(console, "error").mockImplementation(() => {}); +}); + +afterEach(() => { + for (const harness of harnesses.splice(0)) harness.cleanup(); + vi.restoreAllMocks(); +}); + +async function readSurface(bindings: { planReviewSurface?: boolean }): Promise { + let seen: boolean | undefined; + const harness = await createHarness({ + extensionFactories: [ + (pi) => { + pi.on("session_start", (_event, ctx: ExtensionContext) => { + seen = ctx.planReviewSurface; + }); + }, + ], + }); + harnesses.push(harness); + await harness.session.bindExtensions(bindings); + return seen; +} + +test("the host binding reaches the extension context", async () => { + // `mode` and `hasUI` cannot say which host this is: every rpc host has a dialog + // bridge, including clients with no review surface. The capability is declared + // where the host binds the session, and read from every handler's ctx. + expect(await readSurface({ planReviewSurface: true })).toBe(true); +}); + +test("a host that declares no review surface reads false", async () => { + expect(await readSurface({})).toBe(false); +}); diff --git a/packages/coding-agent/test/plan-handoff.test.ts b/packages/coding-agent/test/plan-handoff.test.ts new file mode 100644 index 000000000..65e6ab2ae --- /dev/null +++ b/packages/coding-agent/test/plan-handoff.test.ts @@ -0,0 +1,40 @@ +import type { AgentMessage } from "@step-harness/agent-core"; +import { expect, test } from "vitest"; +import { pendingHandoffText } from "../src/features/plan-handoff.ts"; + +const handoff = { + role: "toolResult" as const, + toolCallId: "handoff", + toolName: "exit_plan_mode", + content: [{ type: "text" as const, text: "Review this plan.\n\n# Plan\n1. Do it\n" }], + details: { + outcome: "external_approval_required", + planFilePath: "/workspace/.stepcode/plans/session-example.md", + planContents: "# Plan\n1. Do it\n", + planHash: "a".repeat(64), + planModeExited: false, + }, + isError: false, + timestamp: 1, +}; + +const user = { role: "user" as const, content: [{ type: "text" as const, text: "Approved." }], timestamp: 2 }; + +test("a waiting proposal is reported instead of the line that introduced it", () => { + const text = pendingHandoffText({ pendingProposalHash: "a".repeat(64) }, [handoff as AgentMessage]); + // The scan trims the joined text, so the trailing newline is gone. + expect(text).toBe("Review this plan.\n\n# Plan\n1. Do it"); +}); + +test("the session fact decides, not the shape of the transcript", () => { + // A host that draws the card approves with a hidden custom message, so the + // transcript can still end in a handoff after the decision is over. + const messages = [handoff] as AgentMessage[]; + expect(pendingHandoffText({}, messages)).toBeUndefined(); + expect(pendingHandoffText(undefined, messages)).toBeUndefined(); +}); + +test("a user message after the handoff ends the scan", () => { + const messages = [handoff, user] as AgentMessage[]; + expect(pendingHandoffText({ pendingProposalHash: "a".repeat(64) }, messages)).toBeUndefined(); +}); diff --git a/packages/coding-agent/test/runtime-info-extension.test.ts b/packages/coding-agent/test/runtime-info-extension.test.ts index 22059874c..eeddfa4b4 100644 --- a/packages/coding-agent/test/runtime-info-extension.test.ts +++ b/packages/coding-agent/test/runtime-info-extension.test.ts @@ -113,6 +113,7 @@ function harness(options: { flag?: boolean | string; mode?: ExtensionContext["mo ...forbidden, mode, hasUI: mode === "tui" || mode === "rpc", + planReviewSurface: false, ui: { notify(text: string) { notifications.push(text); diff --git a/packages/coding-agent/test/step-plan-extension.test.ts b/packages/coding-agent/test/step-plan-extension.test.ts index b6ed57392..046b9b60f 100644 --- a/packages/coding-agent/test/step-plan-extension.test.ts +++ b/packages/coding-agent/test/step-plan-extension.test.ts @@ -560,7 +560,9 @@ test("an interactive rpc host keeps plan mode on until the user answers", async expect(result.content[0]?.text).toContain(planContents); expect(result.details).toEqual({ outcome: "external_approval_required", - approvalSource: "external", + // Who owns the decision, not what carries it: this host shows the proposal + // and its person answers, so the answer is theirs. + approvalSource: "user", planFilePath: planPath, planContents, planHash: createHash("sha256").update(planContents, "utf8").digest("hex"), @@ -729,6 +731,147 @@ test("/plan approve refuses a file edited after the review", async () => { expect(harness.notifications.some((notice) => /plan changed since it was submitted/.test(notice.message))).toBe( true, ); + // Nothing was decided, so the wait stays too: clearing it here would let + // automatic work past a proposal nobody approved and no one can answer. + expect(lastPlanEntry(harness).pendingProposalHash).toBeDefined(); + + // The next review of the text that is on disk now is what can be approved. + await runTool(harness, "exit_plan_mode", {}, context); + await harness.commands.get("plan")!.handler("approve", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); +}); + +test("/plan approve names the proposal it answers", async () => { + // A click knows which card it came from. 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. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. First\n"); + await runTool(harness, "exit_plan_mode", {}, context); + const first = String(lastPlanEntry(harness).pendingProposalHash); + + writeFileSync(planPath, "# Plan\n1. Second\n"); + await runTool(harness, "exit_plan_mode", {}, context); + const second = String(lastPlanEntry(harness).pendingProposalHash); + expect(second).not.toBe(first); + + await harness.commands.get("plan")!.handler(`approve ${first}`, context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); + expect(lastPlanEntry(harness).pendingProposalHash).toBe(second); + expect(harness.notifications.some((notice) => /no longer waiting/.test(notice.message))).toBe(true); + + await harness.commands.get("plan")!.handler(`approve ${second}`, context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); +}); + +test("/plan approve answers a proposal a caller handoff left waiting", async () => { + // A host without a review surface hands approval to its caller, and text is + // the only channel that caller has. Requiring the mode to still be on would + // make the one verb that can answer the handoff unusable there. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: false }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(lastPlanEntry(harness).pendingProposalHash).toBeDefined(); + + await harness.commands.get("plan")!.handler("approve", context); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); + expect(harness.customMessages.some(([message]) => message.customType === "step-plan-approved")).toBe(true); +}); + +test("entering plan mode does not carry a proposal from an earlier round", async () => { + // The mode had ended with a handoff still waiting. Turning planning back on is + // not a decision about that snapshot, so it must not stay approvable. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: false }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + expect(lastPlanEntry(harness).pendingProposalHash).toBeDefined(); + + await harness.commands.get("plan")!.handler("", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); +}); + +test("a pending write the branch rejects leaves nothing waiting", async () => { + // 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. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + + const append = harness.api.appendEntry; + harness.api.appendEntry = (type, data) => { + if (type === "step-plan" && data && typeof data === "object" && "pendingProposalHash" in data) { + throw new Error("persist failed"); + } + append(type, data); + }; + await expect(runTool(harness, "exit_plan_mode", {}, context)).rejects.toThrow("persist failed"); + harness.api.appendEntry = append; + + // The memory agreed with the branch again: nothing waits, so approve refuses + // and the mode is still on with its boundary intact. + await harness.commands.get("plan")!.handler("approve", context); + expect(harness.notifications.some((notice) => /No plan is waiting for approval/.test(notice.message))).toBe(true); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); +}); + +test("a revision requested mid-run sends its instruction and the notes together", async () => { + // Steering the hidden instruction would tell the model to resubmit before it + // had read what to change, and the notes would arrive after the turn had + // already stopped for approval. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + + await harness.commands.get("plan")!.handler( + "split the second step", + harness.ctx({ + mode: "rpc", + hasUI: true, + planReviewSurface: true, + isIdle: () => false, + }), + ); + + expect(harness.customMessages.at(-1)?.[0]).toMatchObject({ customType: "step-plan-revision" }); + expect(harness.customMessages.at(-1)?.[1]).toEqual({ deliverAs: "followUp" }); + expect(harness.userMessages.at(-1)).toEqual([ + "split the second step", + { expandPromptTemplates: true, deliverAs: "followUp" }, + ]); }); test("/plan off leaves without implementing, even when plan mode is already off", async () => { @@ -761,6 +904,111 @@ test("/plan approve refuses when no proposal is waiting", async () => { expect(harness.notifications.some((notice) => /No plan is waiting for approval/.test(notice.message))).toBe(true); }); +/** + * A branch as an earlier process left it. + * + * The extension is mounted after the entries exist, so `session_start` replays + * them the way a restart does: nothing here comes from this process's memory. + */ +function resumedHarness(options: { + cwd: string; + planEntry: Record; + handoff: { planHash: string; planFilePath: string; planContents: string }; + afterHandoff?: { customType: string; data: unknown }; +}): Harness { + const harness = createHarness({ cwd: options.cwd }); + harness.session.appendCustomEntry("step-plan", options.planEntry); + harness.session.appendMessage({ + role: "toolResult", + toolCallId: "resumed-handoff", + toolName: "exit_plan_mode", + content: [{ type: "text", text: `Review this plan.\n\n${options.handoff.planContents}` }], + details: { + outcome: "external_approval_required", + approvalSource: "external", + planFilePath: options.handoff.planFilePath, + planContents: options.handoff.planContents, + planHash: options.handoff.planHash, + planModeExited: true, + }, + isError: false, + timestamp: Date.now(), + }); + if (options.afterHandoff) { + harness.session.appendCustomEntry(options.afterHandoff.customType, options.afterHandoff.data); + } + createStepPlanExtension()(harness.api); + return harness; +} + +test("a waiting proposal survives a restart", async () => { + // The wait is the session fact, so a new process reads it back instead of + // trusting that the transcript looks like a handoff. + const cwd = makeWorkspace(); + const planContents = "# Plan\n1. Do the thing\n"; + const planFilePath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planFilePath), { recursive: true }); + writeFileSync(planFilePath, planContents); + const planHash = createHash("sha256").update(planContents, "utf8").digest("hex"); + + const harness = resumedHarness({ + cwd, + planEntry: { version: 3, enabled: false, planFilePath, pendingProposalHash: planHash }, + handoff: { planHash, planFilePath, planContents }, + }); + await restoreLifecycle(harness, "session_start"); + + await harness.commands.get("plan")!.handler("approve", harness.ctx()); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); + expect(harness.customMessages.some(([message]) => message.customType === "step-plan-approved")).toBe(true); +}); + +test("a session written before the decision fact keeps its gate after an upgrade", async () => { + // Those builds recorded the wait only in the transcript, so the fact is read + // back from the handoff once — otherwise upgrading would silently release a + // proposal nobody answered. + const cwd = makeWorkspace(); + const planContents = "# Plan\n1. Do the thing\n"; + const planFilePath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planFilePath), { recursive: true }); + writeFileSync(planFilePath, planContents); + const planHash = createHash("sha256").update(planContents, "utf8").digest("hex"); + + const harness = resumedHarness({ + cwd, + planEntry: { version: 2, enabled: false, planFilePath }, + handoff: { planHash, planFilePath, planContents }, + }); + await restoreLifecycle(harness, "session_start"); + + await harness.commands.get("plan")!.handler("approve", harness.ctx()); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); + expect(harness.customMessages.some(([message]) => message.customType === "step-plan-approved")).toBe(true); +}); + +test("a plan entry after a legacy handoff is not replayed as a decision", async () => { + // The user left, or turned planning back on, after the handoff. That is a later + // transition, and the migration must not resurrect the proposal it replaced. + const cwd = makeWorkspace(); + const planContents = "# Plan\n1. Do the thing\n"; + const planFilePath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planFilePath), { recursive: true }); + writeFileSync(planFilePath, planContents); + const planHash = createHash("sha256").update(planContents, "utf8").digest("hex"); + + const harness = resumedHarness({ + cwd, + planEntry: { version: 2, enabled: false, planFilePath }, + handoff: { planHash, planFilePath, planContents }, + afterHandoff: { customType: "step-plan", data: { version: 2, enabled: false, planFilePath } }, + }); + await restoreLifecycle(harness, "session_start"); + + await harness.commands.get("plan")!.handler("approve", harness.ctx()); + expect(harness.notifications.some((notice) => /No plan is waiting for approval/.test(notice.message))).toBe(true); + expect(harness.customMessages).toEqual([]); +}); + test("tui-mode exit_plan_mode still gates through the review dialog", async () => { const cwd = makeWorkspace(); const harness = createHarness({ cwd, review: [REVIEW_KEYS.escape] }); @@ -862,7 +1110,7 @@ test.each(planEntryCases)( expect(fs.existsSync(planDir)).toBe(false); const source = "user"; expect(lastPlanEntry(harness)).toMatchObject({ - version: 2, + version: 3, enabled: true, planSource: source, planFilePath: planPath, @@ -1149,7 +1397,7 @@ test("legacy off snapshots recover the latest branch baseline and new entry capt const changedTools = ["read_file", "new_tool"]; harness.api.setActiveTools(changedTools); await harness.commands.get("plan")!.handler("", harness.ctx()); - expect(lastPlanEntry(harness)).toMatchObject({ version: 2, enabled: true }); + expect(lastPlanEntry(harness)).toMatchObject({ version: 3, enabled: true }); expect(lastPlanEntry(harness)).not.toHaveProperty("toolsBeforePlanMode"); await harness.commands.get("plan")!.handler("", harness.ctx()); expect(harness.activeTools()).toEqual(changedTools); @@ -1202,7 +1450,7 @@ test("a legacy todo migration keeps the replaced tool list for later restores", await restoreLifecycle(harness, "session_start"); const migrated = lastPlanEntry(harness); - expect(migrated).toMatchObject({ version: 2, enabled: true }); + expect(migrated).toMatchObject({ version: 3, enabled: true }); expect(migrated.toolsBeforePlanMode).toEqual(originalTools); // A fresh process must replay that list, not fall back to the profile default. @@ -1321,7 +1569,7 @@ test.each(["session_start", "session_tree"] as const)( ).resolves.toBeUndefined(); await enterPlanMode(harness, harness.ctx()); expect(lastPlanEntry(harness)).toMatchObject({ - version: 2, + version: 3, planFilePath: path.join(cwd, ".stepcode", "plans", "session-sess-1.md"), planSource: "user", }); diff --git a/packages/coding-agent/test/suite/plan-approval-gate.test.ts b/packages/coding-agent/test/suite/plan-approval-gate.test.ts index 8f6f19366..8cc8dccf0 100644 --- a/packages/coding-agent/test/suite/plan-approval-gate.test.ts +++ b/packages/coding-agent/test/suite/plan-approval-gate.test.ts @@ -167,6 +167,8 @@ test("a caller handoff leaves the decision pending across a resume", async () => // survives with `enabled: false` — that is what keeps the guard holding. expect(planEntry).toMatchObject({ enabled: false, pendingProposalHash: expect.any(String) }); + // The restart replaces the extension generation and replays the branch, so the + // new guard has to read the wait back from the entry rather than from memory. // The guard keeps holding automatic work until someone answers, and only an // answer clears the fact. harness.setResponses([fauxAssistantMessage("implemented")]); diff --git a/packages/coding-agent/test/trigger-compact-extension.test.ts b/packages/coding-agent/test/trigger-compact-extension.test.ts index fc94bd641..63562dc00 100644 --- a/packages/coding-agent/test/trigger-compact-extension.test.ts +++ b/packages/coding-agent/test/trigger-compact-extension.test.ts @@ -6,6 +6,7 @@ function createContext(tokens: number | null, compact = vi.fn()): ExtensionConte return { mode: "print", hasUI: false, + planReviewSurface: false, ui: {} as ExtensionContext["ui"], cwd: process.cwd(), sessionManager: {} as ExtensionContext["sessionManager"], From ba3cbaa54dca5e67d0e5a809f1940239b106b6da Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 2 Nov 2026 21:53:54 +0800 Subject: [PATCH 03/15] fix(coding-agent): one write per decision, and one message per revision 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 `, 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. --- apps/cli/src/ui/interactive-mode.ts | 7 +- apps/cli/test/plan-review-tool-row.test.ts | 2 +- docs/plan-approval.md | 10 +- packages/coding-agent/docs/extensions.md | 2 +- packages/coding-agent/docs/rpc.md | 9 +- .../coding-agent/src/features/plan-handoff.ts | 48 +++----- .../src/features/plan-mode-prompt.ts | 1 + .../src/features/plan-mode-tools.ts | 16 ++- .../coding-agent/src/features/step-plan.ts | 108 ++++++++++++------ .../coding-agent/src/modes/rpc/rpc-mode.ts | 4 +- packages/coding-agent/src/step/stdio-host.ts | 7 +- .../coding-agent/test/plan-handoff.test.ts | 20 +++- .../test/step-plan-extension.test.ts | 57 ++------- .../coding-agent/test/step-stdio-host.test.ts | 28 ++++- .../test/suite/plan-approval-gate.test.ts | 24 ++++ 15 files changed, 206 insertions(+), 137 deletions(-) diff --git a/apps/cli/src/ui/interactive-mode.ts b/apps/cli/src/ui/interactive-mode.ts index 08e3e92ca..fff1f3341 100644 --- a/apps/cli/src/ui/interactive-mode.ts +++ b/apps/cli/src/ui/interactive-mode.ts @@ -2241,6 +2241,9 @@ export class InteractiveMode { await this.session.bindExtensions({ uiContext, mode: "tui", + // The terminal answers a plan review inside the tool call, so it never + // leaves a proposal waiting for a decision in the session. + planReviewSurface: false, abortHandler: () => { this.restoreQueuedMessagesToEditor({ abort: true }); }, @@ -2461,8 +2464,8 @@ export class InteractiveMode { ui: this.createExtensionUIContext(), mode: "tui", hasUI: true, - // The terminal answers a plan review inside the tool call, so it never - // leaves a proposal waiting for a decision in the session. + // Same host as `bindCurrentSessionExtensions`, stated again because this + // context is built by hand rather than by the runner. planReviewSurface: false, cwd: this.sessionManager.getCwd(), sessionManager: this.sessionManager, diff --git a/apps/cli/test/plan-review-tool-row.test.ts b/apps/cli/test/plan-review-tool-row.test.ts index 2d8e7e803..554f04862 100644 --- a/apps/cli/test/plan-review-tool-row.test.ts +++ b/apps/cli/test/plan-review-tool-row.test.ts @@ -48,7 +48,7 @@ function exitPlanModeDefinition(): ToolDefinition { exitPlanMode: () => {}, resolvePlanFilePath: () => "/tmp/plan.md", recordPendingProposal: () => {}, - pendingProposalHash: () => undefined, + handOffProposal: () => {}, clearPendingProposal: () => {}, } satisfies StepPlanModeController); return tools.get("exit_plan_mode") as ToolDefinition; diff --git a/docs/plan-approval.md b/docs/plan-approval.md index 37989d763..49f447d84 100644 --- a/docs/plan-approval.md +++ b/docs/plan-approval.md @@ -186,12 +186,14 @@ the loop after the batch even when a sibling result does not set `terminate`: - Earlier calls in the batch are not rolled back. The barrier ends that Agent Loop invocation. The session may still start another -turn for input queued after `agent_end` or for automatic context maintenance, so -a host that must not proceed without approval keeps that gate on its own side — -which is why the headless and RPC handoffs are gated by the caller. +turn for input queued after `agent_end` or for automatic context maintenance. The +proposal that a handoff left waiting is still recorded in the session, so the +guard below holds those turns too; a caller that owns the approval also gates +execution on its own side. Inside the session, the plan extension registers one automatic-turn guard with -`ctx.registerAutomaticTurnGuard`: is a proposal still waiting for its decision. +`ctx.registerAutomaticTurnGuard`, and it asks one question: is a proposal still +waiting for its decision. The answer is the session fact `pendingProposalHash`, not the shape of the transcript. A decision clears it — `/plan approve`, a request for changes, or leaving plan mode — while a question typed past the proposal does not: on a host diff --git a/packages/coding-agent/docs/extensions.md b/packages/coding-agent/docs/extensions.md index f00c75343..18df7aa27 100644 --- a/packages/coding-agent/docs/extensions.md +++ b/packages/coding-agent/docs/extensions.md @@ -3018,7 +3018,7 @@ const highlighted = highlightCode(code, lang, theme); | Interactive | `"tui"` | `true` | `false` | Full TUI with terminal rendering; a plan review is answered inside the tool call | | RPC (`--mode rpc`) | `"rpc"` | `true` | set by the host | Dialogs and notifications via JSON protocol; `custom()` returns `undefined`. See [rpc.md](rpc.md) | | JSON (`--mode json`) | `"json"` | `false` | `false` | Event stream to stdout; UI methods are no-ops | -| Print (`-p`) | `"print"` | `false` | Extensions run but can't prompt | +| Print (`-p`) | `"print"` | `false` | `false` | Extensions run but can't prompt | Use `ctx.mode === "tui"` before TUI-specific features (`custom()`, component factories, terminal input). Use `ctx.hasUI` before dialog and notification methods that work in both TUI and RPC modes. diff --git a/packages/coding-agent/docs/rpc.md b/packages/coding-agent/docs/rpc.md index 00f14f7c5..2f08fa83a 100644 --- a/packages/coding-agent/docs/rpc.md +++ b/packages/coding-agent/docs/rpc.md @@ -791,10 +791,11 @@ Response: #### get_last_assistant_text -Get the text content of the last assistant message. When the run handed a plan -proposal to the caller and no user message has answered it yet, this returns the -handoff text (the proposal) instead, because the assistant message only -introduces the submission. +Get the text content of the last assistant message. While the session still +holds an unanswered plan proposal (`pendingProposalHash` in the `step-plan` +entry), this returns the handoff text for that exact snapshot instead, because +the assistant message only introduces the submission. `/plan approve`, a request +for changes, or `/plan off` clears the proposal and the assistant text returns. ```json {"type": "get_last_assistant_text"} diff --git a/packages/coding-agent/src/features/plan-handoff.ts b/packages/coding-agent/src/features/plan-handoff.ts index acb975bf4..7a87e7eda 100644 --- a/packages/coding-agent/src/features/plan-handoff.ts +++ b/packages/coding-agent/src/features/plan-handoff.ts @@ -53,43 +53,32 @@ export function readExternalPlanHandoff(result: unknown): ExternalPlanHandoff | return { text, ...hash }; } -/** - * The handoff this session has not been answered for yet. - * - * Approval belongs to the caller, so a run that hands the proposal over must not - * be continued by any automatic turn starter. The answer is the caller's next - * user message: once one arrives, the trailing cluster no longer ends in a - * handoff and continuation is allowed again. - * - * The scan is a reverse walk that stops at the first user message, so it reads - * only the tail of the conversation. - */ -export function findPendingExternalPlanApproval(messages: readonly AgentMessage[]): ExternalPlanHandoff | undefined { - for (let index = messages.length - 1; index >= 0; index--) { - const message = messages[index]!; - if (message.role === "user") return undefined; - if (message.role !== "toolResult" || message.toolName !== "exit_plan_mode" || message.isError) continue; - const handoff = readExternalPlanHandoff(message); - if (handoff) return handoff; - } - return undefined; -} - /** * The proposal a caller should be shown instead of the assistant's last line. * * A handoff ends the turn on its tool result, so the last assistant message only - * introduces the submission. But "still unanswered" is the session fact, not the - * shape of the transcript: an approval that writes no user message — a decision - * made in a host that draws the card — would leave a scan reporting the old - * proposal forever. + * introduces the submission. Which proposal is still waiting is the session + * fact: an approval that writes no user message — a decision made in a host that + * draws the card — would otherwise leave a transcript scan reporting it forever. + * + * The result is the one whose details carry that exact snapshot. "The tail ends + * in a handoff" cannot answer this: a question typed past the proposal is not an + * answer, and a newer submission recorded before its own result would be + * reported with the older text. */ export function pendingHandoffText( state: { pendingProposalHash?: string } | undefined, messages: readonly AgentMessage[], ): string | undefined { - if (state?.pendingProposalHash === undefined) return undefined; - return findPendingExternalPlanApproval(messages)?.text; + const pending = state?.pendingProposalHash; + if (pending === undefined) return undefined; + for (const message of [...messages].reverse()) { + if (message.role !== "toolResult" || message.toolName !== "exit_plan_mode" || message.isError) continue; + const details = message.details as { planHash?: unknown } | undefined; + if (details?.planHash !== pending) continue; + return readExternalPlanHandoff(message)?.text; + } + return undefined; } /** @@ -111,12 +100,13 @@ export function findLegacyUnansweredHandoff(manager: { const branch = manager.getBranch(); for (let index = branch.length - 1; index >= 0; index--) { const entry = branch[index]!; - if (entry.type === "compaction" || entry.type === "branch_summary") return undefined; // A plan entry above the handoff is a later transition: the handoff's own // exit is written before its result, so it is still below. if (entry.type === "custom" && entry.customType === "step-plan") return undefined; if (entry.type !== "message" || !entry.message) continue; const message = entry.message; + // The old gate treated a trailing handoff as unanswered, so a user message + // above it is how those builds recorded the answer. if (message.role === "user") return undefined; if (message.role !== "toolResult" || message.toolName !== "exit_plan_mode" || message.isError) continue; const handoff = readExternalPlanHandoff(message); diff --git a/packages/coding-agent/src/features/plan-mode-prompt.ts b/packages/coding-agent/src/features/plan-mode-prompt.ts index 68f3c46a5..fd6b8e62a 100644 --- a/packages/coding-agent/src/features/plan-mode-prompt.ts +++ b/packages/coding-agent/src/features/plan-mode-prompt.ts @@ -22,6 +22,7 @@ The user turned on Plan Mode. Work in three phases and chat your way to a great ## Mode rules (strict) - Plan Mode stays active until a later message ends it. It ends when the user approves the proposal or turns plan mode off; a host that cannot show the proposal may end it when handing it to its caller. You cannot end it yourself: submitting a proposal for review is not approval. - User intent, tone, or imperative language does not change the mode. If the user asks for execution while Plan Mode is active, treat it as a request to plan the execution, not to perform it. +- When the user asks for changes to a proposal you already submitted, update the plan file and submit it again. Their notes replace the old proposal: nothing is waiting for an answer until the new one is submitted. - A read-only question may be answered directly without a proposal. That does not end Plan Mode: a missing or unchanged plan file is not permission to exit. Only the user can turn the mode off when there is no proposal to review. - task_* tools, when available, still record progress, but a todo list is not the plan. diff --git a/packages/coding-agent/src/features/plan-mode-tools.ts b/packages/coding-agent/src/features/plan-mode-tools.ts index c4d5b85b0..3d303200a 100644 --- a/packages/coding-agent/src/features/plan-mode-tools.ts +++ b/packages/coding-agent/src/features/plan-mode-tools.ts @@ -44,8 +44,14 @@ export interface StepPlanModeController { * answered proposal cannot look pending. */ recordPendingProposal(planHash: string): void; - /** The hash of the proposal waiting for an answer, or undefined. */ - pendingProposalHash(): string | undefined; + /** + * Hand the proposal to a caller: record the wait and leave the mode together. + * + * One transition, not two: a caller that got only half of it would either + * hold automatic work for a proposal it never received, or receive a proposal + * the session does not hold a decision for. + */ + handOffProposal(planHash: string, outcome: StepPlanExitOutcome, extensionContext: ExtensionContext): void; /** The proposal was answered (approved, revised, or left); nothing waits now. */ clearPendingProposal(): void; } @@ -224,8 +230,7 @@ export function registerPlanModeTools(pi: ExtensionAPI, planMode: StepPlanModeCo // answered by nobody still has to hold automatic starters back, so the // handoff records it here too. if (ctx.mode === "rpc") { - planMode.recordPendingProposal(planHash); - planMode.exitPlanMode("auto_rpc", ctx); + planMode.handOffProposal(planHash, "auto_rpc", ctx); ctx.ui.notify( "RPC host has no plan-review bridge; caller approval is required before execution.", "warning", @@ -237,8 +242,7 @@ export function registerPlanModeTools(pi: ExtensionAPI, planMode: StepPlanModeCo ); } if (!ctx.hasUI) { - planMode.recordPendingProposal(planHash); - planMode.exitPlanMode("auto_headless", ctx); + planMode.handOffProposal(planHash, "auto_headless", ctx); return reviewedResult( `Plan mode exited. Plan file kept at ${planFilePath} for reference.\n\nPlan contents:\n${planContents}\n\nCannot show interactive prompt; caller must gate approval externally. This turn ends now.`, reviewed("external_approval_required"), diff --git a/packages/coding-agent/src/features/step-plan.ts b/packages/coding-agent/src/features/step-plan.ts index a97266ee4..c1a2c0478 100644 --- a/packages/coding-agent/src/features/step-plan.ts +++ b/packages/coding-agent/src/features/step-plan.ts @@ -205,6 +205,25 @@ export function getStepPlanState( return undefined; } +/** + * The wait recorded on a session's branch, tolerating a session without one. + * + * The protocol hosts accept lightweight session facades in tests and embeds, and + * a reply they owe the caller must not be the thing a missing branch breaks. + */ +export function pendingProposalOf(session: { + sessionManager?: { getBranch?: () => readonly { type: string; customType?: string; data?: unknown }[] }; +}): { pendingProposalHash?: string } | undefined { + let branch: readonly { type: string; customType?: string; data?: unknown }[]; + try { + branch = session.sessionManager?.getBranch?.() ?? []; + } catch { + // A session that cannot report its branch has no recorded wait. + return undefined; + } + return getStepPlanState(branch); +} + export interface PlanModeToolCallDecision { planFilePath: string; targetPath: string; @@ -510,6 +529,36 @@ export const createStepPlanExtension = } }; const recordPendingProposal = (planHash: string): void => writePendingProposal(planHash); + /** + * Record the wait and leave the mode in one write. + * + * Two writes would leave a window where the branch says a proposal waits + * but the caller never received one, or the reverse. The exit path is the + * only place that can publish the mode change, so it is inlined here rather + * than layered on top of `exit`. + */ + const handOffProposal = (planHash: string, outcome: StepPlanExitOutcome, ctx: ExtensionContext): void => { + if (!enabled) { + writePendingProposal(planHash); + return; + } + const previousPending = pendingProposal; + const previousSource = planSource; + pendingProposal = planHash; + enabled = false; + planSource = undefined; + try { + persistPlanState(undefined, false); + } catch (error) { + pendingProposal = previousPending; + enabled = true; + planSource = previousSource; + throw error; + } + recordPlanExit(options.telemetry, previousSource, outcome); + restoreTools(); + updateStatus(ctx, enabled, planSource); + }; const clearPendingProposal = (): void => { if (pendingProposal === undefined) return; writePendingProposal(undefined); @@ -519,7 +568,7 @@ export const createStepPlanExtension = exitPlanMode: exit, resolvePlanFilePath, recordPendingProposal, - pendingProposalHash: () => pendingProposal, + handOffProposal, clearPendingProposal, }; @@ -530,7 +579,7 @@ export const createStepPlanExtension = }); pi.registerCommand("plan", { description: - "Toggle plan mode, `/plan ` to plan a task, `/plan off` to leave, or `/plan approve` to accept a submitted proposal", + "Toggle plan mode with `/plan`, or `/plan on` and `/plan off` to state it outright. `/plan ` plans a task, `/plan approve [hash]` accepts a submitted proposal", handler: async (args, ctx) => { const prompt = args.trim(); if (!prompt) { @@ -610,43 +659,27 @@ export const createStepPlanExtension = ); return; } - // `/plan ` is "plan this", never "toggle": make sure plan mode is on, - // then hand the task to the model as an ordinary user turn. Tool - // restriction has already been applied by enter(), so the turn starts - // with the plan-mode tool set. Slash commands run even mid-stream; queue - // the task behind the current turn in that case instead of throwing. - if (!enabled) enter(ctx); + // `/plan ` is "plan this", never "toggle": make sure plan mode is + // on, then hand the task to the model as an ordinary user turn. Slash + // commands run even mid-stream; queue the task behind the current turn + // in that case instead of throwing. + // // A note on a proposal under review replaces it: the old snapshot is no - // longer waiting for an answer, and the model has to submit again or the - // session would wait on a proposal nobody is looking at. - const revising = pendingProposal !== undefined; + // longer waiting for an answer, so the session does not hold a gate for + // a proposal nobody is looking at. The notes are the whole message — + // plan mode already tells the model that changes mean updating the plan + // file and submitting again. A second, hidden instruction could not be + // kept in the same request as the notes: the follow-up queue delivers + // one message per request by default, so it would arrive on its own and + // could send the model back to submit a plan it had not revised. + if (!enabled) enter(ctx); clearPendingProposal(); - // Both halves travel on the same channel. Left to the default, a hidden - // message sent mid-run is steered into the current request while the - // notes wait in the follow-up queue — the model would be told to - // resubmit before it had read what to change, and the notes would arrive - // after the turn had already stopped for approval. - const queued = !ctx.isIdle(); - if (revising) { - // The "submit again" half is an instruction, not something the user - // typed: a hidden message keeps it out of their bubble, and going - // first keeps it ahead of the notes in the same request. - pi.sendMessage( - { - customType: "step-plan-revision", - display: false, - content: - "These notes revise the proposal under review. Update the plan file, then call exit_plan_mode again to submit the new proposal for approval.", - }, - queued ? { deliverAs: "followUp" } : undefined, - ); - } // Expand a leading `/skill:` or prompt template in the task, as the same // text sent without `/plan` would be (the web composer puts its plan // prefix in front of a picked skill). pi.sendUserMessage(prompt, { expandPromptTemplates: true, - ...(queued ? { deliverAs: "followUp" as const } : {}), + ...(ctx.isIdle() ? {} : { deliverAs: "followUp" as const }), }); }, }); @@ -749,6 +782,17 @@ export const createStepPlanExtension = // session whose proposal nobody has answered. if (restoredPlanState && (restoredPlanState.version ?? 2) < 3) { pendingProposal = findLegacyUnansweredHandoff(ctx.sessionManager)?.planHash; + // Written back once, so every reader of the branch — the automatic-turn + // guard, the RPC and SDK replies, a host's card — sees one fact rather + // than a migrated memory next to an entry that still lacks it. + if (pendingProposal !== undefined) { + try { + persistPlanState(); + } catch { + // The wait still holds this process; the next restore retries the + // migration because the entry stays below version 3. + } + } } if (legacyBaseline) { const baseline = unique([...legacyBaseline, ...activePlanTaskTools]); diff --git a/packages/coding-agent/src/modes/rpc/rpc-mode.ts b/packages/coding-agent/src/modes/rpc/rpc-mode.ts index 37dfbc4db..78b705d73 100644 --- a/packages/coding-agent/src/modes/rpc/rpc-mode.ts +++ b/packages/coding-agent/src/modes/rpc/rpc-mode.ts @@ -26,7 +26,7 @@ import { writeRawStdout, } from "../../core/output-guard.ts"; import { pendingHandoffText } from "../../features/plan-handoff.ts"; -import { getStepPlanState } from "../../features/step-plan.ts"; +import { pendingProposalOf } from "../../features/step-plan.ts"; import { type Theme, theme } from "../../theme/theme.ts"; import { killTrackedDetachedChildren } from "../../utils/shell.ts"; import { toJsonEvent } from "../json-event.ts"; @@ -667,7 +667,7 @@ export async function runRpcMode(runtimeHost: AgentSessionRuntimeHost, options: case "get_last_assistant_text": { // The caller needs the proposal it has to approve, not the line that // introduced it — but only while the decision fact says one is waiting. - const pending = pendingHandoffText(getStepPlanState(session.sessionManager.getBranch()), session.messages); + const pending = pendingHandoffText(pendingProposalOf(session), session.messages); const text = pending ?? session.getLastAssistantText(); return success(id, "get_last_assistant_text", { text }); } diff --git a/packages/coding-agent/src/step/stdio-host.ts b/packages/coding-agent/src/step/stdio-host.ts index 3bfb71b68..b8b6aa5b0 100644 --- a/packages/coding-agent/src/step/stdio-host.ts +++ b/packages/coding-agent/src/step/stdio-host.ts @@ -25,7 +25,7 @@ import type { } from "../core/extensions/types.ts"; import { isToolCommandFailure } from "../core/tools/command-outcome.ts"; import { pendingHandoffText } from "../features/plan-handoff.ts"; -import { getStepPlanState } from "../features/step-plan.ts"; +import { pendingProposalOf } from "../features/step-plan.ts"; import type { Theme } from "../theme/theme.ts"; import { STEP_DEFAULT_PROVIDER } from "./defaults.ts"; import { listStepSessions } from "./session.ts"; @@ -667,10 +667,7 @@ export class StepStdioHost { // the submission. Reporting either as the result would hide the proposal the // caller has to approve, so the proposal wins while the decision fact says it // is still waiting. - const pending = pendingHandoffText( - getStepPlanState(this.#session.sessionManager.getBranch()), - this.#session.messages, - ); + const pending = pendingHandoffText(pendingProposalOf(this.#session), this.#session.messages); const text = pending ?? this.#session.getLastAssistantText() ?? ""; this.#emitMessage(query, { type: "result", diff --git a/packages/coding-agent/test/plan-handoff.test.ts b/packages/coding-agent/test/plan-handoff.test.ts index 65e6ab2ae..983c504a1 100644 --- a/packages/coding-agent/test/plan-handoff.test.ts +++ b/packages/coding-agent/test/plan-handoff.test.ts @@ -34,7 +34,23 @@ test("the session fact decides, not the shape of the transcript", () => { expect(pendingHandoffText(undefined, messages)).toBeUndefined(); }); -test("a user message after the handoff ends the scan", () => { +test("a question typed past the proposal does not hide it", () => { + // A message is not a decision: the host's user is still deciding, and the + // caller still has to be shown what it would approve. const messages = [handoff, user] as AgentMessage[]; - expect(pendingHandoffText({ pendingProposalHash: "a".repeat(64) }, messages)).toBeUndefined(); + expect(pendingHandoffText({ pendingProposalHash: "a".repeat(64) }, messages)).toBe( + "Review this plan.\n\n# Plan\n1. Do it", + ); +}); + +test("a newer submission before its own result is not reported as the old text", () => { + // The fact names the snapshot; the transcript tail may still be the older + // round, whose text the user is no longer looking at. + const older = { ...handoff, details: { ...handoff.details, planHash: "a".repeat(64) } }; + const newer = { ...handoff, details: { ...handoff.details, planHash: "b".repeat(64) } }; + const messages = [older, newer] as AgentMessage[]; + expect(pendingHandoffText({ pendingProposalHash: "b".repeat(64) }, messages)).toBe( + "Review this plan.\n\n# Plan\n1. Do it", + ); + expect(pendingHandoffText({ pendingProposalHash: "c".repeat(64) }, messages)).toBeUndefined(); }); diff --git a/packages/coding-agent/test/step-plan-extension.test.ts b/packages/coding-agent/test/step-plan-extension.test.ts index 046b9b60f..b0bc880e2 100644 --- a/packages/coding-agent/test/step-plan-extension.test.ts +++ b/packages/coding-agent/test/step-plan-extension.test.ts @@ -697,16 +697,11 @@ test("a review requested change clears the fact so the session does not wait on // and the model is told to submit again. expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); - // The notes stay exactly what the user wrote; the "submit again" half is a - // hidden instruction, so their bubble does not grow internal text. + // The notes are the whole message. Plan mode already tells the model that + // changes mean updating the plan and submitting again; a second hidden + // instruction could not be kept in the same request as the notes. expect(harness.userMessages.at(-1)?.[0]).toBe("split the second step"); - expect( - harness.customMessages.some( - ([message]) => - message.customType === "step-plan-revision" && - String(message.content).includes("call exit_plan_mode again"), - ), - ).toBe(true); + expect(harness.customMessages).toEqual([]); }); test("/plan approve refuses a file edited after the review", async () => { @@ -842,10 +837,9 @@ test("a pending write the branch rejects leaves nothing waiting", async () => { expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); }); -test("a revision requested mid-run sends its instruction and the notes together", async () => { - // Steering the hidden instruction would tell the model to resubmit before it - // had read what to change, and the notes would arrive after the turn had - // already stopped for approval. +test("a revision requested mid-run is queued behind the running turn", async () => { + // The notes are the whole message, so there is no second delivery to fall out + // of step with them. const cwd = makeWorkspace(); const harness = createHarness({ cwd }); createStepPlanExtension()(harness.api); @@ -866,42 +860,15 @@ test("a revision requested mid-run sends its instruction and the notes together" }), ); - expect(harness.customMessages.at(-1)?.[0]).toMatchObject({ customType: "step-plan-revision" }); - expect(harness.customMessages.at(-1)?.[1]).toEqual({ deliverAs: "followUp" }); expect(harness.userMessages.at(-1)).toEqual([ "split the second step", { expandPromptTemplates: true, deliverAs: "followUp" }, ]); -}); - -test("/plan off leaves without implementing, even when plan mode is already off", async () => { - const cwd = makeWorkspace(); - const harness = createHarness({ cwd }); - createStepPlanExtension()(harness.api); - const context = harness.ctx(); - - await enterPlanMode(harness, context); - await harness.commands.get("plan")!.handler("off", context); - expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); - expect(harness.userMessages).toEqual([]); - - // Idempotent: a queued `/plan off` that arrives after the turn already left - // the mode must not turn it back on. - await harness.commands.get("plan")!.handler("off", context); - expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); - expect(harness.userMessages).toEqual([]); -}); - -test("/plan approve refuses when no proposal is waiting", async () => { - const cwd = makeWorkspace(); - const harness = createHarness({ cwd }); - createStepPlanExtension()(harness.api); - const context = harness.ctx(); - - await harness.commands.get("plan")!.handler("approve", context); - expect(harness.userMessages).toEqual([]); - expect(harness.appended.filter((entry) => entry.customType === "step-plan")).toEqual([]); - expect(harness.notifications.some((notice) => /No plan is waiting for approval/.test(notice.message))).toBe(true); + expect(harness.customMessages).toEqual([]); + // The old proposal is no longer waiting, so the session holds no gate while + // the model revises and submits again. + expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); }); /** diff --git a/packages/coding-agent/test/step-stdio-host.test.ts b/packages/coding-agent/test/step-stdio-host.test.ts index a5c34dbee..26976cf62 100644 --- a/packages/coding-agent/test/step-stdio-host.test.ts +++ b/packages/coding-agent/test/step-stdio-host.test.ts @@ -8,9 +8,12 @@ import { StepStdioHost } from "../src/step/stdio-host.ts"; function createFakeRuntime(withAgent = false): { runtime: AgentSessionRuntimeHost; session: AgentSession; + /** Entries the session branch reports, as the real manager's does. */ + branch: Array<{ type: string; customType?: string; data?: unknown }>; emit(event: AgentSessionEvent): void; } { const listeners = new Set(); + const branch: Array<{ type: string; customType?: string; data?: unknown }> = []; const session = { sessionId: "session-1", model: { provider: "step", id: "step-3.7-flash", name: "Step 3.7 Flash" }, @@ -29,6 +32,9 @@ function createFakeRuntime(withAgent = false): { sessionManager: { getCwd: () => "/workspace", getSessionDir: () => "/workspace/.sessions", + // The real manager always reports its branch; the host reads the plan + // decision fact from it. + getBranch: () => [...branch], }, extensionRunner: { getRegisteredCommands: () => [], @@ -77,6 +83,7 @@ function createFakeRuntime(withAgent = false): { return { runtime, session, + branch, emit: (event) => { for (const listener of listeners) listener(event); }, @@ -163,19 +170,26 @@ describe("StepStdioHost", () => { while (!predicate() && Date.now() < deadline) await new Promise((resolve) => setTimeout(resolve, 5)); expect(predicate()).toBe(true); }; - const { runtime, session } = createFakeRuntime(); + const { runtime, session, branch } = createFakeRuntime(); // The handoff ends the turn on its tool result, so the query settles with no // assistant text at all: an empty success result would hide the proposal. (session as unknown as { getLastAssistantText: () => string | undefined }).getLastAssistantText = () => undefined; + const planHash = "a".repeat(64); session.messages.push({ role: "toolResult", toolCallId: "plan-1", toolName: "exit_plan_mode", content: [{ type: "text", text: "Caller must approve.\n\nPlan contents:\n# Proposal\nDo it." }], - details: { outcome: "external_approval_required" }, + details: { outcome: "external_approval_required", planHash }, isError: false, timestamp: Date.now(), }); + // The wait is the session fact, not the shape of the transcript. + branch.push({ + type: "custom", + customType: "step-plan", + data: { version: 3, enabled: false, pendingProposalHash: planHash }, + }); const host = new StepStdioHost({ runtimeHost: runtime, input, output, onExitRequested: vi.fn() }); const running = host.run(); @@ -226,20 +240,26 @@ describe("StepStdioHost", () => { while (!predicate() && Date.now() < deadline) await new Promise((resolve) => setTimeout(resolve, 5)); expect(predicate()).toBe(true); }; - const { runtime, session } = createFakeRuntime(); + const { runtime, session, branch } = createFakeRuntime(); // The model often writes one line before calling the tool. That line is not // the proposal, so the pending handoff has to win over it. (session as unknown as { getLastAssistantText: () => string | undefined }).getLastAssistantText = () => "Plan is ready, submitting for review."; + const planHash = "b".repeat(64); session.messages.push({ role: "toolResult", toolCallId: "plan-1", toolName: "exit_plan_mode", content: [{ type: "text", text: "Caller must approve.\n\nPlan contents:\n# Proposal\nDo it." }], - details: { outcome: "external_approval_required" }, + details: { outcome: "external_approval_required", planHash }, isError: false, timestamp: Date.now(), }); + branch.push({ + type: "custom", + customType: "step-plan", + data: { version: 3, enabled: false, pendingProposalHash: planHash }, + }); const host = new StepStdioHost({ runtimeHost: runtime, input, output, onExitRequested: vi.fn() }); const running = host.run(); input.write( diff --git a/packages/coding-agent/test/suite/plan-approval-gate.test.ts b/packages/coding-agent/test/suite/plan-approval-gate.test.ts index 8cc8dccf0..932c37490 100644 --- a/packages/coding-agent/test/suite/plan-approval-gate.test.ts +++ b/packages/coding-agent/test/suite/plan-approval-gate.test.ts @@ -176,6 +176,30 @@ test("a caller handoff leaves the decision pending across a resume", async () => expect(harness.session.hasPendingUserDecision).toBe(false); }); +test("notes sent mid-run reach the model as they are, with no second message", async () => { + // The revision is one message: the user's notes. A hidden companion could not + // be kept in the same request — the follow-up queue delivers one message per + // request by default — and would tell the model to resubmit before it had + // read what to change. + const { harness } = await createPlanHarness("step-plan-gate-revision-"); + const seen: string[][] = []; + harness.setResponses([ + async () => { + await harness.session.sendUserMessage("NOTES", { deliverAs: "followUp" }); + return fauxAssistantMessage("still running"); + }, + (context) => { + seen.push(context.messages.map((message) => JSON.stringify(message))); + return fauxAssistantMessage("revised"); + }, + ]); + await harness.session.prompt("kick"); + + const request = seen.at(-1) ?? []; + expect(request.some((message) => message.includes("NOTES"))).toBe(true); + expect(request.some((message) => message.includes("call exit_plan_mode again"))).toBe(false); +}); + test("a compaction does not leave a decision nobody can answer", async () => { const { harness, planPath } = await createPlanHarness("step-plan-gate-compact-"); await handoff(harness, planPath); From a2624debf290fdd86d2c66b3a6500364a0f9a8d6 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 2 Nov 2026 22:06:33 +0800 Subject: [PATCH 04/15] fix(coding-agent): make the plan facts verifiable, not just consistent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` 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. --- .../coding-agent/docs/step-integration.md | 11 +- .../coding-agent/src/features/plan-handoff.ts | 8 +- .../src/features/plan-mode-prompt.ts | 12 +- .../coding-agent/src/features/step-plan.ts | 45 +++-- .../test/step-plan-extension.test.ts | 173 +++++++++++++++++- .../test/suite/plan-approval-gate.test.ts | 24 +++ 6 files changed, 250 insertions(+), 23 deletions(-) diff --git a/packages/coding-agent/docs/step-integration.md b/packages/coding-agent/docs/step-integration.md index 917756d85..6dc744cf6 100644 --- a/packages/coding-agent/docs/step-integration.md +++ b/packages/coding-agent/docs/step-integration.md @@ -348,9 +348,14 @@ for path rules and retention responsibilities. - `exit_plan_mode` submits the written proposal for review, requiring a readable, nonempty regular plan file. In the TUI, the user can approve execution or provide refinement notes; dismissal keeps planning active and ends the turn. - Headless and RPC runs hand the proposal to the caller instead: plan mode exits, - the turn ends immediately, and the caller must gate approval externally before - allowing execution. `/plan` can still explicitly toggle planning off. + A host that declares `planReviewSurface` — an interactive app that draws the + proposal itself — keeps plan mode on, ends the turn, and records the reviewed + snapshot as `pendingProposalHash` in the `step-plan` entry; approval, a change + request, or leaving clears it. Runs without that surface hand the proposal to + their caller instead: plan mode exits, the turn ends immediately, and the + caller must gate approval externally before allowing execution. `/plan` states + the mode outright (`on`, `off`) and answers a submitted proposal (`approve`, + `approve `). - `task_create`, `task_update`, `task_get`, and `task_list` are the only task tools and work with or without plan mode. Each execution checklist is a separate task plan, distinct from the Markdown proposal. Tasks retain their diff --git a/packages/coding-agent/src/features/plan-handoff.ts b/packages/coding-agent/src/features/plan-handoff.ts index 7a87e7eda..20b8d327b 100644 --- a/packages/coding-agent/src/features/plan-handoff.ts +++ b/packages/coding-agent/src/features/plan-handoff.ts @@ -76,7 +76,9 @@ export function pendingHandoffText( if (message.role !== "toolResult" || message.toolName !== "exit_plan_mode" || message.isError) continue; const details = message.details as { planHash?: unknown } | undefined; if (details?.planHash !== pending) continue; - return readExternalPlanHandoff(message)?.text; + const text = readExternalPlanHandoff(message)?.text; + // An empty result would otherwise win over the answer the caller did get. + if (text) return text; } return undefined; } @@ -100,6 +102,10 @@ export function findLegacyUnansweredHandoff(manager: { const branch = manager.getBranch(); for (let index = branch.length - 1; index >= 0; index--) { const entry = branch[index]!; + // A summary boundary means the handoff is no longer in the conversation the + // model reads: the old gate could not see it either, and a wait restored + // from behind a summary is one nobody can be shown. + if (entry.type === "compaction" || entry.type === "branch_summary") return undefined; // A plan entry above the handoff is a later transition: the handoff's own // exit is written before its result, so it is still below. if (entry.type === "custom" && entry.customType === "step-plan") return undefined; diff --git a/packages/coding-agent/src/features/plan-mode-prompt.ts b/packages/coding-agent/src/features/plan-mode-prompt.ts index fd6b8e62a..183ec2c51 100644 --- a/packages/coding-agent/src/features/plan-mode-prompt.ts +++ b/packages/coding-agent/src/features/plan-mode-prompt.ts @@ -12,6 +12,16 @@ /** Custom-message type carrying a collaboration-mode announcement. */ export const PLAN_MODE_MESSAGE_TYPE = "step-plan-mode"; +/** + * Version of the Plan block's content. + * + * A recorded announcement is append-only history, so a session resumed from an + * older block keeps the rules it was written with. Bumping this makes the next + * request append the current block; without it, a rule only reaches sessions + * that entered plan mode after the upgrade. + */ +export const PLAN_MODE_PROMPT_VERSION = 2; + /** Block announcing that plan mode is active, with the session plan file. */ export function buildPlanModePrompt(planFilePath: string): string { return ` @@ -22,7 +32,7 @@ The user turned on Plan Mode. Work in three phases and chat your way to a great ## Mode rules (strict) - Plan Mode stays active until a later message ends it. It ends when the user approves the proposal or turns plan mode off; a host that cannot show the proposal may end it when handing it to its caller. You cannot end it yourself: submitting a proposal for review is not approval. - User intent, tone, or imperative language does not change the mode. If the user asks for execution while Plan Mode is active, treat it as a request to plan the execution, not to perform it. -- When the user asks for changes to a proposal you already submitted, update the plan file and submit it again. Their notes replace the old proposal: nothing is waiting for an answer until the new one is submitted. +- When the user asks for changes to a proposal you already submitted, update the plan file and submit it again. Do not treat the old proposal as still under review: their notes replace it. - A read-only question may be answered directly without a proposal. That does not end Plan Mode: a missing or unchanged plan file is not permission to exit. Only the user can turn the mode off when there is no proposal to review. - task_* tools, when available, still record progress, but a todo list is not the plan. diff --git a/packages/coding-agent/src/features/step-plan.ts b/packages/coding-agent/src/features/step-plan.ts index c1a2c0478..ecb95ba8e 100644 --- a/packages/coding-agent/src/features/step-plan.ts +++ b/packages/coding-agent/src/features/step-plan.ts @@ -25,7 +25,12 @@ import { type StepTelemetryReporter, trackStepTelemetry } from "../step/telemetr import { getThenRunCommand } from "../step/then-run.ts"; import { findLegacyUnansweredHandoff } from "./plan-handoff.ts"; import { type LegacyPlanTodoFields, migrateLegacyPlanTodos } from "./plan-mode-migration.ts"; -import { buildDefaultModePrompt, buildPlanModePrompt, PLAN_MODE_MESSAGE_TYPE } from "./plan-mode-prompt.ts"; +import { + buildDefaultModePrompt, + buildPlanModePrompt, + PLAN_MODE_MESSAGE_TYPE, + PLAN_MODE_PROMPT_VERSION, +} from "./plan-mode-prompt.ts"; import { getPlanFilePath, hashPlanContents, @@ -85,6 +90,8 @@ interface PlanState { interface PlanModeAnnouncement { mode: "plan" | "default"; planFilePath?: string; + /** Content version of a Plan block; absent in blocks written before it. */ + version?: number; } interface AnnouncementRecord { @@ -103,7 +110,8 @@ function lastAnnouncement(records: readonly AnnouncementRecord[]): PlanModeAnnou if (details.mode === "plan") { const planFilePath = "planFilePath" in details && typeof details.planFilePath === "string" ? details.planFilePath : undefined; - return { mode: "plan", planFilePath }; + const version = "version" in details && typeof details.version === "number" ? details.version : undefined; + return { mode: "plan", planFilePath, ...(version === undefined ? {} : { version }) }; } } return undefined; @@ -126,8 +134,15 @@ function announcementFor( recorded: PlanModeAnnouncement | undefined, ) { if (planFilePath !== undefined) { - if (visible?.mode === "plan" && visible.planFilePath === planFilePath) return undefined; - const details: PlanModeAnnouncement = { mode: "plan", planFilePath }; + // A block written by an older build states older rules; resuming it keeps + // the model on those rules until a new turn appends the current one. + const current = visible?.version === PLAN_MODE_PROMPT_VERSION; + if (visible?.mode === "plan" && visible.planFilePath === planFilePath && current) return undefined; + const details: PlanModeAnnouncement = { + mode: "plan", + planFilePath, + version: PLAN_MODE_PROMPT_VERSION, + }; return { customType: PLAN_MODE_MESSAGE_TYPE, content: buildPlanModePrompt(planFilePath), @@ -470,14 +485,16 @@ export const createStepPlanExtension = "info", ); }; + /** + * Leave plan mode because its decision is over. + * + * Only a user decision comes through here — approval, or leaving. A caller + * handoff leaves through {@link handOffProposal}, which keeps the wait. + */ const exit = (outcome: StepPlanExitOutcome, ctx: ExtensionContext): void => { if (!enabled) return; - // A user decision ends the wait. A handoff does not: the proposal still - // awaits the caller's approval, and the automatic-turn guard keeps - // holding queued work until that arrives. - const decided = outcome !== "auto_headless" && outcome !== "auto_rpc"; const previousPending = pendingProposal; - if (decided) pendingProposal = undefined; + pendingProposal = undefined; try { // Persist before lifting the live restriction: a failed write must leave // the session in plan mode, not strand the branch saying it is still on. @@ -607,8 +624,9 @@ export const createStepPlanExtension = return; } // `/plan approve` answers the proposal, `/plan approve ` answers - // that one. Any other text after the verb is a task, not a decision. - const namedProposal = /^approve\s+(.+)$/.exec(prompt)?.[1]?.trim(); + // that one. A hash is 64 lowercase hex characters, so any other text + // after the verb is a task and falls through to the branch below. + const namedProposal = /^approve\s+([0-9a-f]{64})$/.exec(prompt)?.[1]; if (prompt === "approve" || namedProposal !== undefined) { // The user accepted the proposal the host showed them. Leaving the // mode and starting the implementation turn are one decision: the @@ -677,8 +695,13 @@ export const createStepPlanExtension = // Expand a leading `/skill:` or prompt template in the task, as the same // text sent without `/plan` would be (the web composer puts its plan // prefix in front of a picked skill). + // + // The words are the user's, forwarded: a proposal submitted by the turn + // this waited behind must not turn their request for changes into + // extension traffic that the pending-decision guard then holds back. pi.sendUserMessage(prompt, { expandPromptTemplates: true, + source: ctx.mode === "tui" ? "interactive" : "rpc", ...(ctx.isIdle() ? {} : { deliverAs: "followUp" as const }), }); }, diff --git a/packages/coding-agent/test/step-plan-extension.test.ts b/packages/coding-agent/test/step-plan-extension.test.ts index b0bc880e2..0ee745494 100644 --- a/packages/coding-agent/test/step-plan-extension.test.ts +++ b/packages/coding-agent/test/step-plan-extension.test.ts @@ -9,6 +9,7 @@ import { createEventBus } from "../src/core/event-bus.ts"; import type { ExtensionAPI, ExtensionContext, ToolDefinition } from "../src/core/extensions/types.ts"; import { KeybindingsManager } from "../src/core/keybindings.ts"; import { SessionManager } from "../src/core/session-manager.ts"; +import { PLAN_MODE_PROMPT_VERSION } from "../src/features/plan-mode-prompt.ts"; import { getPlanFilePath } from "../src/features/plan-mode-tools.ts"; import { createStepExtension } from "../src/features/step.ts"; import { createStepPlanExtension } from "../src/features/step-plan.ts"; @@ -154,6 +155,8 @@ function createHarness( ({ mode: "tui", hasUI: true, + // The terminal answers a plan review inside the tool call. + planReviewSurface: false, cwd, isIdle: () => true, hasPendingMessages: () => false, @@ -807,6 +810,97 @@ test("entering plan mode does not carry a proposal from an earlier round", async expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); }); +test("a caller handoff records the wait and the exit in one entry", async () => { + // Two writes would leave a window where the branch says a proposal waits but + // the caller never received one, or the reverse. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: false }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + const before = harness.appended.filter((entry) => entry.customType === "step-plan").length; + + await runTool(harness, "exit_plan_mode", {}, context); + + const written = harness.appended.filter((entry) => entry.customType === "step-plan"); + expect(written).toHaveLength(before + 1); + expect(written.at(-1)?.data).toMatchObject({ enabled: false, pendingProposalHash: expect.any(String) }); +}); + +test("/plan off answers a proposal a caller handoff left waiting", async () => { + // Leaving is a decision too: the handoff set the wait, and the mode is already + // off, so a `/plan off` that only looked at the mode would leave the gate held + // with nothing left for the user to click. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: false }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + expect(lastPlanEntry(harness).pendingProposalHash).toBeDefined(); + + await harness.commands.get("plan")!.handler("off", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); + // Leaving is not approving: no implementation turn starts. + expect(harness.customMessages).toEqual([]); +}); + +test("/plan off is idempotent and never turns the mode back on", async () => { + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx(); + + await enterPlanMode(harness, context); + await harness.commands.get("plan")!.handler("off", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(harness.userMessages).toEqual([]); + + // A queued `/plan off` that arrives after the turn already left the mode must + // not turn it back on. + await harness.commands.get("plan")!.handler("off", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(harness.userMessages).toEqual([]); +}); + +test("/plan approve refuses when no proposal is waiting", async () => { + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx(); + + await harness.commands.get("plan")!.handler("approve", context); + expect(harness.userMessages).toEqual([]); + expect(harness.appended.filter((entry) => entry.customType === "step-plan")).toEqual([]); + expect(harness.notifications.some((notice) => /No plan is waiting for approval/.test(notice.message))).toBe(true); +}); + +test("/plan approve is a task, not a decision", async () => { + // Only a snapshot hash names a proposal. Any other text after the verb is + // something to plan, and dropping it would silently lose what the user wrote. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + + await harness.commands.get("plan")!.handler("approve the payment flow", context); + expect(harness.userMessages.at(-1)?.[0]).toBe("approve the payment flow"); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); +}); + test("a pending write the branch rejects leaves nothing waiting", async () => { // 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. @@ -860,9 +954,12 @@ test("a revision requested mid-run is queued behind the running turn", async () }), ); + // The words are the user's, forwarded. A proposal submitted by the turn they + // waited behind must not turn their change request into extension traffic that + // the pending-decision guard holds back. expect(harness.userMessages.at(-1)).toEqual([ "split the second step", - { expandPromptTemplates: true, deliverAs: "followUp" }, + { expandPromptTemplates: true, source: "rpc", deliverAs: "followUp" }, ]); expect(harness.customMessages).toEqual([]); // The old proposal is no longer waiting, so the session holds no gate while @@ -946,13 +1043,49 @@ test("a session written before the decision fact keeps its gate after an upgrade planEntry: { version: 2, enabled: false, planFilePath }, handoff: { planHash, planFilePath, planContents }, }); + const before = harness.appended.filter((entry) => entry.customType === "step-plan").length; + await restoreLifecycle(harness, "session_start"); + + // Written back once, so the branch — which every reader consults — carries the + // wait too, not just this process's memory. + const written = harness.appended.filter((entry) => entry.customType === "step-plan"); + expect(written).toHaveLength(before + 1); + expect(written.at(-1)?.data).toMatchObject({ version: 3, enabled: false, pendingProposalHash: planHash }); + + // A second restore reads version 3 and leaves the branch alone. + const afterFirst = harness.appended.length; await restoreLifecycle(harness, "session_start"); + expect(harness.appended).toHaveLength(afterFirst); await harness.commands.get("plan")!.handler("approve", harness.ctx()); expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); expect(harness.customMessages.some(([message]) => message.customType === "step-plan-approved")).toBe(true); }); +test("a legacy handoff behind a summary is not restored as a wait", async () => { + // The old gate scanned the conversation the model reads; a summary boundary + // means the handoff is no longer in it, so a wait restored from behind one is + // a wait nobody can be shown a proposal for. + const cwd = makeWorkspace(); + const planContents = "# Plan\n1. Do the thing\n"; + const planFilePath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planFilePath), { recursive: true }); + writeFileSync(planFilePath, planContents); + const planHash = createHash("sha256").update(planContents, "utf8").digest("hex"); + + const harness = resumedHarness({ + cwd, + planEntry: { version: 2, enabled: false, planFilePath }, + handoff: { planHash, planFilePath, planContents }, + }); + harness.session.appendCompaction("Earlier work summarized.", harness.session.getEntries()[0]!.id, 1_000); + await restoreLifecycle(harness, "session_start"); + + await harness.commands.get("plan")!.handler("approve", harness.ctx()); + expect(harness.notifications.some((notice) => /No plan is waiting for approval/.test(notice.message))).toBe(true); + expect(harness.customMessages).toEqual([]); +}); + test("a plan entry after a legacy handoff is not replayed as a decision", async () => { // The user left, or turned planning back on, after the handoff. That is a later // transition, and the migration must not resurrect the proposal it replaced. @@ -1135,7 +1268,10 @@ test.each(planEntryCases)( const toolsBefore = harness.activeTools(); // `/plan ` while planning hands over the task without re-entering. await harness.commands.get("plan")!.handler("refine the proposal", context); - expect(harness.userMessages.at(-1)).toEqual(["refine the proposal", { expandPromptTemplates: true }]); + expect(harness.userMessages.at(-1)).toEqual([ + "refine the proposal", + { expandPromptTemplates: true, source: "interactive" }, + ]); expect(harness.appended).toHaveLength(snapshotsBefore); expect(events).toHaveLength(telemetryBefore); expect(events[0]).toEqual({ event: "plan_mode_entered", properties: { source } }); @@ -2306,7 +2442,9 @@ test("/plan with a task enables plan mode and sends the task as a user turn", as await harness.commands.get("plan")!.handler("实现一个俄罗斯方块,简单版", context); expect(harness.statuses.get("plan-mode")).toBe("Planning (user)"); - expect(harness.userMessages).toEqual([["实现一个俄罗斯方块,简单版", { expandPromptTemplates: true }]]); + expect(harness.userMessages).toEqual([ + ["实现一个俄罗斯方块,简单版", { expandPromptTemplates: true, source: "interactive" }], + ]); // The turn must start under the plan-mode tool set, not the unrestricted one. expect(harness.activeTools()).not.toContain("edit_file_unrestricted"); expect(harness.activeTools()).toContain("read_file"); @@ -2332,7 +2470,9 @@ test("/plan with a task queues as a follow-up while the agent is streaming", asy await harness.commands.get("plan")!.handler("plan this too", harness.ctx({ isIdle: () => false })); - expect(harness.userMessages).toEqual([["plan this too", { expandPromptTemplates: true, deliverAs: "followUp" }]]); + expect(harness.userMessages).toEqual([ + ["plan this too", { expandPromptTemplates: true, source: "interactive", deliverAs: "followUp" }], + ]); }); test("bare /plan still toggles plan mode and sends nothing", async () => { @@ -2419,7 +2559,9 @@ test("/plan announces plan mode on the turn that carries the task", async const harness = createHarness({ cwd: makeWorkspace() }); createStepPlanExtension()(harness.api); await harness.commands.get("plan")!.handler("plan the parser rewrite", harness.ctx()); - expect(harness.userMessages).toEqual([["plan the parser rewrite", { expandPromptTemplates: true }]]); + expect(harness.userMessages).toEqual([ + ["plan the parser rewrite", { expandPromptTemplates: true, source: "interactive" }], + ]); expect((await startTurn(harness))?.details.mode).toBe("plan"); }); @@ -2511,9 +2653,10 @@ test.each(["session_start", "session_tree"] as const)( "%s restore re-announces only when the visible block disagrees with the mode", async (type) => { const planFilePath = "/workspace/.stepcode/plans/session-sess-1.md"; - const announced = (mode: "plan" | "default") => (mode === "plan" ? { mode, planFilePath } : { mode }); + const announced = (mode: "plan" | "default") => + mode === "plan" ? { mode, planFilePath, version: PLAN_MODE_PROMPT_VERSION } : { mode }; - // A legacy agent-entered plan that was already announced: no repeat. + // A plan that was already announced in the current wording: no repeat. const legacy = createHarness({ entries: [ { type: "custom", customType: "step-plan", data: { enabled: true, planSource: "agent", planFilePath } }, @@ -2525,6 +2668,22 @@ test.each(["session_start", "session_tree"] as const)( expect(legacy.statuses.get("plan-mode")).toBe("Planning (agent)"); expect(await startTurn(legacy)).toBeUndefined(); + // A block written by an older build states older rules, so the current one is + // appended once; the next turn has nothing left to say. + const stale = createHarness({ + entries: [ + { type: "custom", customType: "step-plan", data: { enabled: true, planSource: "agent", planFilePath } }, + ], + }); + stale.session.appendCustomMessageEntry("step-plan-mode", "", false, { + mode: "plan", + planFilePath, + }); + createStepPlanExtension()(stale.api); + await restoreLifecycle(stale, type); + expect(await startTurn(stale)).toMatchObject({ details: announced("plan") }); + expect(await startTurn(stale)).toBeUndefined(); + // The same plan state recorded before announcements existed: announce once. const unannounced = createHarness({ entries: [ diff --git a/packages/coding-agent/test/suite/plan-approval-gate.test.ts b/packages/coding-agent/test/suite/plan-approval-gate.test.ts index 932c37490..b74666b77 100644 --- a/packages/coding-agent/test/suite/plan-approval-gate.test.ts +++ b/packages/coding-agent/test/suite/plan-approval-gate.test.ts @@ -200,6 +200,30 @@ test("notes sent mid-run reach the model as they are, with no second message", a expect(request.some((message) => message.includes("call exit_plan_mode again"))).toBe(false); }); +test("a change request is not held back by the proposal the running turn submits", async () => { + // The user's own words always run: that message is also how a decision gets + // answered. A turn that submits the old proposal while their notes are queued + // must not lock those notes behind the gate it just armed. + const { harness } = await createPlanHarness("step-plan-gate-notes-"); + const seen: string[][] = []; + harness.setResponses([ + async () => { + await harness.session.sendUserMessage("split the second step", { + deliverAs: "followUp", + source: "rpc", + }); + return fauxAssistantMessage("still running"); + }, + (context) => { + seen.push(context.messages.map((message) => JSON.stringify(message))); + return fauxAssistantMessage("revised"); + }, + ]); + await harness.session.prompt("kick"); + + expect(seen.at(-1)?.some((message) => message.includes("split the second step"))).toBe(true); +}); + test("a compaction does not leave a decision nobody can answer", async () => { const { harness, planPath } = await createPlanHarness("step-plan-gate-compact-"); await handoff(harness, planPath); From b87eef19cf54405928b36f46492b099613f7343e Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 2 Nov 2026 22:27:31 +0800 Subject: [PATCH 05/15] fix(coding-agent): let the user's own queued words answer the decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` 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. --- .../coding-agent/src/core/agent-session.ts | 32 ++++++++++--- .../test/step-plan-extension.test.ts | 46 ++++++++++++++++++- .../test/suite/plan-approval-gate.test.ts | 8 ++-- 3 files changed, 74 insertions(+), 12 deletions(-) diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index 0299e48a1..9b4b8c460 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -366,6 +366,11 @@ export class AgentSession { private _steeringMessages: string[] = []; /** Tracks pending follow-up messages for UI display. Removed when delivered. */ private _followUpMessages: string[] = []; + /** + * Queued inputs a person wrote, by text, so the decision gate below can tell + * the user's own words from work a feature queued on their behalf. + */ + private _queuedFromUser = new Set(); /** Messages queued to be included with the next user prompt as context ("asides"). */ private _pendingNextTurnMessages: CustomMessage[] = []; /** Context-only custom messages queued during a run, flushed once the current turn's tool results are in. */ @@ -1338,12 +1343,14 @@ export class AgentSession { const steeringIndex = this._steeringMessages.indexOf(messageText); if (steeringIndex !== -1) { this._steeringMessages.splice(steeringIndex, 1); + this._queuedFromUser.delete(messageText); this._emitQueueUpdate(); } else { // Check follow-up queue const followUpIndex = this._followUpMessages.indexOf(messageText); if (followUpIndex !== -1) { this._followUpMessages.splice(followUpIndex, 1); + this._queuedFromUser.delete(messageText); this._emitQueueUpdate(); } } @@ -2045,7 +2052,12 @@ export class AgentSession { // The turn ended on a decision the user has not answered yet. Continuing here // would run a queued input, or a compaction recovery request, on the user's // behalf; the recovered work starts from the user's next prompt instead. - if (this.hasPendingUserDecision) return false; + // + // The user's own words are not that. A prompt they typed before the decision + // existed is how they answer it, whether it arrived mid-turn or after, so it + // still runs — otherwise their request waits behind a gate only they can + // open, with nothing left on screen to open it. + if (this.hasPendingUserDecision && !this._hasQueuedUserInput()) return false; // CLM maintains context for a future request. A successful settled answer // needs no extra model call; the next prompt/continuation performs the @@ -2145,9 +2157,9 @@ export class AgentSession { ); } if (options.streamingBehavior === "followUp") { - await this._queueFollowUp(expandedText, currentImages); + await this._queueFollowUp(expandedText, currentImages, options?.source); } else { - await this._queueSteer(expandedText, currentImages); + await this._queueSteer(expandedText, currentImages, options?.source); } preflightResult?.(true); return; @@ -2157,7 +2169,7 @@ export class AgentSession { // answered; the message waits for the turn that answers it. A user's own // message always runs, because that is how the decision gets answered. if (options?.source === "extension" && this.hasPendingUserDecision) { - await this._queueFollowUp(expandedText, currentImages); + await this._queueFollowUp(expandedText, currentImages, options.source); preflightResult?.(true); return; } @@ -2395,8 +2407,14 @@ export class AgentSession { /** * Internal: Queue a steering message (already expanded, no extension command check). */ - private async _queueSteer(text: string, images?: ImageContent[]): Promise { + private _hasQueuedUserInput(): boolean { + const queued = [...this._steeringMessages, ...this._followUpMessages]; + return queued.some((text) => this._queuedFromUser.has(text)); + } + + private async _queueSteer(text: string, images?: ImageContent[], source?: InputSource): Promise { this._steeringMessages.push(text); + if (source !== "extension") this._queuedFromUser.add(text); this._emitQueueUpdate(); const content: (TextContent | ImageContent)[] = [{ type: "text", text }]; if (images) { @@ -2412,8 +2430,9 @@ export class AgentSession { /** * Internal: Queue a follow-up message (already expanded, no extension command check). */ - private async _queueFollowUp(text: string, images?: ImageContent[]): Promise { + private async _queueFollowUp(text: string, images?: ImageContent[], source?: InputSource): Promise { this._followUpMessages.push(text); + if (source !== "extension") this._queuedFromUser.add(text); this._emitQueueUpdate(); const content: (TextContent | ImageContent)[] = [{ type: "text", text }]; if (images) { @@ -2581,6 +2600,7 @@ export class AgentSession { const followUp = [...this._followUpMessages]; this._steeringMessages = []; this._followUpMessages = []; + this._queuedFromUser.clear(); // Custom messages held behind a pending decision are queue entries too: // leaving them would deliver content the caller believes it cleared. this._pendingCustomMessages = []; diff --git a/packages/coding-agent/test/step-plan-extension.test.ts b/packages/coding-agent/test/step-plan-extension.test.ts index 0ee745494..df6e09480 100644 --- a/packages/coding-agent/test/step-plan-extension.test.ts +++ b/packages/coding-agent/test/step-plan-extension.test.ts @@ -810,6 +810,45 @@ test("entering plan mode does not carry a proposal from an earlier round", async expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); }); +test("a question past a submitted proposal does not release the decision", async () => { + // The whole point of the review surface: the user is still deciding, so + // another thing they say — a question as easily as a note — is not consent. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + const waiting = lastPlanEntry(harness).pendingProposalHash; + expect(waiting).toBeDefined(); + + await startTurn(harness, context); + + expect(lastPlanEntry(harness).pendingProposalHash).toBe(waiting); +}); + +test("a caller handoff is settled by the next turn, because that is its only channel", async () => { + // The mirror case: no surface to answer with, so the caller's next message is + // the decision, and the gate lets the conversation continue. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: false }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + expect(lastPlanEntry(harness).pendingProposalHash).toBeDefined(); + + await startTurn(harness, context); + + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); +}); + test("a caller handoff records the wait and the exit in one entry", async () => { // Two writes would leave a window where the branch says a proposal waits but // the caller never received one, or the reverse. @@ -2499,10 +2538,13 @@ interface AnnouncementMessage { details: { mode: string; planFilePath?: string }; } -async function startTurn(harness: Harness): Promise { +async function startTurn( + harness: Harness, + context: ExtensionContext = harness.ctx(), +): Promise { const result = (await harness.emit( { type: "before_agent_start", prompt: "next", systemPrompt: "base prompt" }, - harness.ctx(), + context, )) as { message?: AnnouncementMessage } | undefined; // Persist like AgentSession does, so the next turn sees what was announced. if (result?.message) { diff --git a/packages/coding-agent/test/suite/plan-approval-gate.test.ts b/packages/coding-agent/test/suite/plan-approval-gate.test.ts index b74666b77..9477819df 100644 --- a/packages/coding-agent/test/suite/plan-approval-gate.test.ts +++ b/packages/coding-agent/test/suite/plan-approval-gate.test.ts @@ -200,10 +200,10 @@ test("notes sent mid-run reach the model as they are, with no second message", a expect(request.some((message) => message.includes("call exit_plan_mode again"))).toBe(false); }); -test("a change request is not held back by the proposal the running turn submits", async () => { - // The user's own words always run: that message is also how a decision gets - // answered. A turn that submits the old proposal while their notes are queued - // must not lock those notes behind the gate it just armed. +test("the user's own queued words are not held back by a proposal the run submits", async () => { + // The gate stops automatic work. A prompt the user typed is how they answer a + // decision, so it still runs — otherwise their change request waits behind a + // gate only they can open, with nothing on screen to open it. const { harness } = await createPlanHarness("step-plan-gate-notes-"); const seen: string[][] = []; harness.setResponses([ From 96142a5dd92cd3de0a11fe86686372bed86b6891 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 2 Nov 2026 22:47:54 +0800 Subject: [PATCH 06/15] fix(coding-agent): judge the queued message the agent would actually 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. --- docs/plan-approval.md | 6 +- packages/agent-core/src/agent.ts | 16 ++++ packages/agent-core/test/agent.test.ts | 19 +++++ .../coding-agent/src/core/agent-session.ts | 47 +++++------ packages/coding-agent/test/suite/harness.ts | 4 + .../test/suite/plan-approval-gate.test.ts | 78 +++++++++++++++---- 6 files changed, 132 insertions(+), 38 deletions(-) diff --git a/docs/plan-approval.md b/docs/plan-approval.md index 49f447d84..a30e7c910 100644 --- a/docs/plan-approval.md +++ b/docs/plan-approval.md @@ -213,7 +213,11 @@ decision: cron keeps the job due, and a goal continuation stays pending. A user's own prompt always runs: that message is how the decision gets answered. A host bridge forwarding a real person's input reports its own `source`, so the -message is not mistaken for extension-generated text. +message is not mistaken for extension-generated text. The same holds while a run +is in flight: input a person typed waits in the queue, and when the turn submits +a proposal, that input is still what the next request delivers. The gate reads +the message the agent would deliver next, not whether the queue contains one — +work a feature queued ahead of the user's words still waits. The guard never guesses from wording. On a host without a review surface the decision belongs to the caller, and the only channel it has is a message; the first turn after the handoff therefore settles the wait, and the guard stays diff --git a/packages/agent-core/src/agent.ts b/packages/agent-core/src/agent.ts index 556385f04..24f5c96cc 100644 --- a/packages/agent-core/src/agent.ts +++ b/packages/agent-core/src/agent.ts @@ -144,6 +144,11 @@ class PendingMessageQueue { return this.messages.length > 0; } + /** The message the next drain would return, without removing it. */ + peek(): AgentMessage | undefined { + return this.messages[0]; + } + drain(): AgentMessage[] { if (this.mode === "all") { const drained = this.messages.slice(); @@ -324,6 +329,17 @@ export class Agent { return this.steeringQueue.hasItems() || this.followUpQueue.hasItems(); } + /** + * The message the next drain would deliver, without removing it. + * + * A caller that has to decide whether continuing is safe needs to know which + * queued message would go first: steering drains before follow-ups, and a + * queue that will mix the two has no single "has queued work" answer. + */ + peekNextQueued(): AgentMessage | undefined { + return this.steeringQueue.peek() ?? this.followUpQueue.peek(); + } + /** Active abort signal for the current run, if any. */ get signal(): AbortSignal | undefined { return this.activeRun?.abortController.signal; diff --git a/packages/agent-core/test/agent.test.ts b/packages/agent-core/test/agent.test.ts index 8def7286a..2df0baad6 100644 --- a/packages/agent-core/test/agent.test.ts +++ b/packages/agent-core/test/agent.test.ts @@ -705,6 +705,25 @@ describe("Agent", () => { expect(agent.state.messages).not.toContainEqual(message); }); + it("peeks the next queued message without draining it", () => { + // A caller that must decide whether continuing is safe needs to know which + // message goes first: steering drains before follow-ups. + const agent = new Agent({ streamFn: unusedStreamFunction }); + expect(agent.peekNextQueued()).toBeUndefined(); + + const followUp = { role: "user" as const, content: "Follow-up", timestamp: Date.now() }; + const steer = { role: "user" as const, content: "Steering", timestamp: Date.now() }; + agent.followUp(followUp); + expect(agent.peekNextQueued()).toBe(followUp); + + agent.steer(steer); + expect(agent.peekNextQueued()).toBe(steer); + expect(agent.hasQueuedMessages()).toBe(true); + + // Peeking is not delivery. + expect(agent.peekNextQueued()).toBe(steer); + }); + it("should handle abort controller", () => { const agent = new Agent({ streamFn: unusedStreamFunction }); diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index 9b4b8c460..80adfd2ed 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -367,10 +367,14 @@ export class AgentSession { /** Tracks pending follow-up messages for UI display. Removed when delivered. */ private _followUpMessages: string[] = []; /** - * Queued inputs a person wrote, by text, so the decision gate below can tell - * the user's own words from work a feature queued on their behalf. + * Queued messages a person wrote, by identity, so the decision gate below can + * tell the user's own words from work a feature queued on their behalf. + * + * Identity, not text: the same words can be queued by both, and queued + * messages are delivered as objects the agent holds. A weak set keeps this + * from outliving the queue. */ - private _queuedFromUser = new Set(); + private _queuedFromUser = new WeakSet(); /** Messages queued to be included with the next user prompt as context ("asides"). */ private _pendingNextTurnMessages: CustomMessage[] = []; /** Context-only custom messages queued during a run, flushed once the current turn's tool results are in. */ @@ -1343,14 +1347,12 @@ export class AgentSession { const steeringIndex = this._steeringMessages.indexOf(messageText); if (steeringIndex !== -1) { this._steeringMessages.splice(steeringIndex, 1); - this._queuedFromUser.delete(messageText); this._emitQueueUpdate(); } else { // Check follow-up queue const followUpIndex = this._followUpMessages.indexOf(messageText); if (followUpIndex !== -1) { this._followUpMessages.splice(followUpIndex, 1); - this._queuedFromUser.delete(messageText); this._emitQueueUpdate(); } } @@ -2057,7 +2059,7 @@ export class AgentSession { // existed is how they answer it, whether it arrived mid-turn or after, so it // still runs — otherwise their request waits behind a gate only they can // open, with nothing left on screen to open it. - if (this.hasPendingUserDecision && !this._hasQueuedUserInput()) return false; + if (this.hasPendingUserDecision && !this._nextQueuedInputIsUser()) return false; // CLM maintains context for a future request. A successful settled answer // needs no extra model call; the next prompt/continuation performs the @@ -2407,24 +2409,29 @@ export class AgentSession { /** * Internal: Queue a steering message (already expanded, no extension command check). */ - private _hasQueuedUserInput(): boolean { - const queued = [...this._steeringMessages, ...this._followUpMessages]; - return queued.some((text) => this._queuedFromUser.has(text)); + /** + * Whether the user's own input is what the agent would deliver next. + * + * A continuation delivers one queued message at a time — steering before + * follow-ups. Only the message at the head of that order is the user's answer; + * work queued ahead of it is not, and letting the whole continuation run would + * deliver that work on the user's behalf. + */ + private _nextQueuedInputIsUser(): boolean { + const next = this.agent.peekNextQueued(); + return next !== undefined && this._queuedFromUser.has(next); } private async _queueSteer(text: string, images?: ImageContent[], source?: InputSource): Promise { this._steeringMessages.push(text); - if (source !== "extension") this._queuedFromUser.add(text); this._emitQueueUpdate(); const content: (TextContent | ImageContent)[] = [{ type: "text", text }]; if (images) { content.push(...images); } - this.agent.steer({ - role: "user", - content, - timestamp: Date.now(), - }); + const message: AgentMessage = { role: "user", content, timestamp: Date.now() }; + if (source !== "extension") this._queuedFromUser.add(message); + this.agent.steer(message); } /** @@ -2432,17 +2439,14 @@ export class AgentSession { */ private async _queueFollowUp(text: string, images?: ImageContent[], source?: InputSource): Promise { this._followUpMessages.push(text); - if (source !== "extension") this._queuedFromUser.add(text); this._emitQueueUpdate(); const content: (TextContent | ImageContent)[] = [{ type: "text", text }]; if (images) { content.push(...images); } - this.agent.followUp({ - role: "user", - content, - timestamp: Date.now(), - }); + const message: AgentMessage = { role: "user", content, timestamp: Date.now() }; + if (source !== "extension") this._queuedFromUser.add(message); + this.agent.followUp(message); } /** @@ -2600,7 +2604,6 @@ export class AgentSession { const followUp = [...this._followUpMessages]; this._steeringMessages = []; this._followUpMessages = []; - this._queuedFromUser.clear(); // Custom messages held behind a pending decision are queue entries too: // leaving them would deliver content the caller believes it cleared. this._pendingCustomMessages = []; diff --git a/packages/coding-agent/test/suite/harness.ts b/packages/coding-agent/test/suite/harness.ts index e33f95957..f380c1767 100644 --- a/packages/coding-agent/test/suite/harness.ts +++ b/packages/coding-agent/test/suite/harness.ts @@ -70,6 +70,8 @@ export interface HarnessOptions { excludedToolNames?: string[]; resourceLoader?: ResourceLoader; extensionFactories?: Array; + /** Extension bindings the session gets, as a host would pass them. */ + bindings?: Parameters[0]; withConfiguredAuth?: boolean; modelsJson?: Record; } @@ -198,6 +200,8 @@ export async function createHarness(options: HarnessOptions = {}): Promise { vi.restoreAllMocks(); }); +/** Record the request the model actually received. */ +function record(requests: string[][]) { + return (context: { messages: readonly unknown[] }) => { + requests.push(context.messages.map((message) => JSON.stringify(message))); + return fauxAssistantMessage("recorded"); + }; +} + function printHost(harness: Harness): AgentSessionRuntimeHost { return { session: harness.session, @@ -42,10 +50,11 @@ function printHost(harness: Harness): AgentSessionRuntimeHost { } as unknown as AgentSessionRuntimeHost; } -async function createPlanHarness(prefix: string) { +async function createPlanHarness(prefix: string, bindings?: Parameters[0]) { const storage = mkdtempSync(join(tmpdir(), prefix)); roots.push(storage); const harness = await createHarness({ + bindings, settings: { compaction: { enabled: false }, retry: { enabled: false } }, extensionFactories: [ (pi) => { @@ -200,28 +209,67 @@ test("notes sent mid-run reach the model as they are, with no second message", a expect(request.some((message) => message.includes("call exit_plan_mode again"))).toBe(false); }); -test("the user's own queued words are not held back by a proposal the run submits", async () => { - // The gate stops automatic work. A prompt the user typed is how they answer a - // decision, so it still runs — otherwise their change request waits behind a - // gate only they can open, with nothing on screen to open it. - const { harness } = await createPlanHarness("step-plan-gate-notes-"); - const seen: string[][] = []; +test("a change request queued before the submission still reaches the model", async () => { + // The user typed these words, and the turn they waited behind is the one that + // submitted the proposal. Holding the queue back would leave their request + // behind a gate only they can open, with nothing on screen to open it. + const { harness, planPath } = await createPlanHarness("step-plan-gate-notes-", { + mode: "rpc", + planReviewSurface: true, + }); + mkdirSync(dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Proposal\nDo it.\n"); + const requests: string[][] = []; harness.setResponses([ async () => { await harness.session.sendUserMessage("split the second step", { deliverAs: "followUp", source: "rpc", }); - return fauxAssistantMessage("still running"); + return fauxAssistantMessage([fauxToolCall("exit_plan_mode", {})]); }, - (context) => { - seen.push(context.messages.map((message) => JSON.stringify(message))); - return fauxAssistantMessage("revised"); + record(requests), + record(requests), + ]); + await harness.session.prompt("/plan write the proposal"); + + // The submission armed the gate: the proposal is waiting for an answer. + expect(harness.session.hasPendingUserDecision).toBe(true); + // Their words are the next thing the agent delivers, so they run. + expect(requests.some((request) => request.some((message) => message.includes("split the second step")))).toBe(true); +}); + +test("automatic work queued ahead of the user's words is still held", async () => { + // A continuation delivers one queued message at a time. Letting it run because + // the user said something later would deliver the automatic work first, on + // their behalf, past a decision they have not answered. + const { harness, planPath } = await createPlanHarness("step-plan-gate-auto-", { + mode: "rpc", + planReviewSurface: true, + }); + mkdirSync(dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Proposal\nDo it.\n"); + const requests: string[][] = []; + harness.setResponses([ + async () => { + await harness.session.sendCustomMessage( + { customType: "test-wake", content: "AUTOWORK", display: true, details: {} }, + { deliverAs: "followUp", triggerTurn: true }, + ); + await harness.session.sendUserMessage("split the second step", { + deliverAs: "followUp", + source: "rpc", + }); + return fauxAssistantMessage([fauxToolCall("exit_plan_mode", {})]); }, + record(requests), + record(requests), ]); - await harness.session.prompt("kick"); + await harness.session.prompt("/plan write the proposal"); - expect(seen.at(-1)?.some((message) => message.includes("split the second step"))).toBe(true); + expect(harness.session.hasPendingUserDecision).toBe(true); + expect(requests.some((request) => request.some((message) => message.includes("AUTOWORK")))).toBe(false); + expect(harness.session.pendingMessageCount).toBeGreaterThan(0); }); test("a compaction does not leave a decision nobody can answer", async () => { From 1da71d88baf463262c96ba89ca6db055e24ac5a7 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 2 Nov 2026 22:53:39 +0800 Subject: [PATCH 07/15] fix(agent-core,coding-agent): hold queued work at delivery, not at the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/agent-core/src/agent.ts | 80 +++++++++++++------ packages/agent-core/test/agent.test.ts | 57 +++++++++---- .../coding-agent/src/core/agent-session.ts | 27 ++++--- .../test/suite/plan-approval-gate.test.ts | 5 +- 4 files changed, 116 insertions(+), 53 deletions(-) diff --git a/packages/agent-core/src/agent.ts b/packages/agent-core/src/agent.ts index 24f5c96cc..bde4e44ee 100644 --- a/packages/agent-core/src/agent.ts +++ b/packages/agent-core/src/agent.ts @@ -144,24 +144,42 @@ class PendingMessageQueue { return this.messages.length > 0; } - /** The message the next drain would return, without removing it. */ - peek(): AgentMessage | undefined { - return this.messages[0]; - } - - drain(): AgentMessage[] { + /** + * Take the messages that may be delivered now. + * + * `allow` is asked per message, and a refused one stays queued: a caller can + * hold part of the queue without dropping what it holds or reordering what it + * releases. With no predicate this drains everything the mode allows. + */ + drain(allow?: (message: AgentMessage) => boolean): AgentMessage[] { + if (!allow) { + if (this.mode === "all") { + const drained = this.messages.slice(); + this.messages = []; + return drained; + } + const first = this.messages[0]; + if (!first) return []; + this.messages = this.messages.slice(1); + return [first]; + } if (this.mode === "all") { - const drained = this.messages.slice(); - this.messages = []; - return drained; + const released: AgentMessage[] = []; + const held: AgentMessage[] = []; + for (const message of this.messages) (allow(message) ? released : held).push(message); + this.messages = held; + return released; } + const index = this.messages.findIndex(allow); + if (index === -1) return []; + const [first] = this.messages.splice(index, 1); + return [first!]; + } - const first = this.messages[0]; - if (!first) { - return []; - } - this.messages = this.messages.slice(1); - return [first]; + /** Whether any queued message would pass `allow`. */ + hasDeliverable(allow?: (message: AgentMessage) => boolean): boolean { + if (!allow) return this.messages.length > 0; + return this.messages.some(allow); } clear(): void { @@ -330,14 +348,26 @@ export class Agent { } /** - * The message the next drain would deliver, without removing it. + * Whether either queue holds a message that may be delivered now. + * + * A caller that has to decide whether continuing is safe asks this rather than + * `hasQueuedMessages`: work the predicate refuses does not justify a turn. + */ + hasDeliverableQueuedMessage(allow: (message: AgentMessage) => boolean): boolean { + return this.steeringQueue.hasDeliverable(allow) || this.followUpQueue.hasDeliverable(allow); + } + + /** + * Filter consulted whenever the loop takes a queued message. * - * A caller that has to decide whether continuing is safe needs to know which - * queued message would go first: steering drains before follow-ups, and a - * queue that will mix the two has no single "has queued work" answer. + * Holding a message is the only way to keep automatic work out of a turn while + * the user's own input still runs: the queues are drained by the loop, one + * batch at a time, so a check made before starting the run cannot govern what + * it delivers later. */ - peekNextQueued(): AgentMessage | undefined { - return this.steeringQueue.peek() ?? this.followUpQueue.peek(); + private queueDeliveryFilter: ((message: AgentMessage) => boolean) | undefined; + setQueueDeliveryFilter(filter: ((message: AgentMessage) => boolean) | undefined): void { + this.queueDeliveryFilter = filter; } /** Active abort signal for the current run, if any. */ @@ -399,13 +429,13 @@ export class Agent { } if (lastMessage.role === "assistant") { - const queuedSteering = this.steeringQueue.drain(); + const queuedSteering = this.steeringQueue.drain(this.queueDeliveryFilter); if (queuedSteering.length > 0) { await this.runPromptMessages(queuedSteering, { skipInitialSteeringPoll: true }); return; } - const queuedFollowUps = this.followUpQueue.drain(); + const queuedFollowUps = this.followUpQueue.drain(this.queueDeliveryFilter); if (queuedFollowUps.length > 0) { await this.runPromptMessages(queuedFollowUps); return; @@ -511,9 +541,9 @@ export class Agent { skipInitialSteeringPoll = false; return []; } - return this.steeringQueue.drain(); + return this.steeringQueue.drain(this.queueDeliveryFilter); }, - getFollowUpMessages: async () => this.followUpQueue.drain(), + getFollowUpMessages: async () => this.followUpQueue.drain(this.queueDeliveryFilter), }; } diff --git a/packages/agent-core/test/agent.test.ts b/packages/agent-core/test/agent.test.ts index 2df0baad6..04c977955 100644 --- a/packages/agent-core/test/agent.test.ts +++ b/packages/agent-core/test/agent.test.ts @@ -705,23 +705,48 @@ describe("Agent", () => { expect(agent.state.messages).not.toContainEqual(message); }); - it("peeks the next queued message without draining it", () => { - // A caller that must decide whether continuing is safe needs to know which - // message goes first: steering drains before follow-ups. - const agent = new Agent({ streamFn: unusedStreamFunction }); - expect(agent.peekNextQueued()).toBeUndefined(); - - const followUp = { role: "user" as const, content: "Follow-up", timestamp: Date.now() }; - const steer = { role: "user" as const, content: "Steering", timestamp: Date.now() }; - agent.followUp(followUp); - expect(agent.peekNextQueued()).toBe(followUp); - - agent.steer(steer); - expect(agent.peekNextQueued()).toBe(steer); + it("holds a queued message a delivery filter refuses", async () => { + // Holding is per message: the loop takes a batch at a time, so refusing one + // message must not carry the ones behind it along, and the held message has + // to stay queued rather than be dropped. + let responseCount = 0; + const agent = new Agent({ + streamFn: () => { + const stream = new MockAssistantStream(); + responseCount++; + queueMicrotask(() => { + stream.push({ type: "done", reason: "stop", message: createAssistantMessage(`Reply ${responseCount}`) }); + }); + return stream; + }, + }); + agent.state.messages = [ + { role: "user", content: [{ type: "text", text: "Initial" }], timestamp: Date.now() - 10 }, + createAssistantMessage("Initial response"), + ]; + const held = { role: "user" as const, content: [{ type: "text" as const, text: "Held work" }], timestamp: 1 }; + const allowed = { + role: "user" as const, + content: [{ type: "text" as const, text: "The user's words" }], + timestamp: 2, + }; + agent.followUp(held); + agent.followUp(allowed); + + agent.setQueueDeliveryFilter((message) => message === allowed); + await agent.continue(); + + const texts = agent.state.messages + .filter((message) => message.role === "user") + .map((message) => + typeof message.content === "string" + ? message.content + : message.content.map((part) => (part.type === "text" ? part.text : "")).join(""), + ); + expect(texts).toContain("The user's words"); + expect(texts).not.toContain("Held work"); + // Refused, not dropped: it is still waiting for a turn that may deliver it. expect(agent.hasQueuedMessages()).toBe(true); - - // Peeking is not delivery. - expect(agent.peekNextQueued()).toBe(steer); }); it("should handle abort controller", () => { diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index 80adfd2ed..e3ba7ed4e 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -491,6 +491,9 @@ export class AgentSession { constructor(config: AgentSessionConfig) { this.agent = config.agent; + // Installed once: the loop drains the queues long after a run-start check, + // so the decision has to be consulted at delivery time. + this.agent.setQueueDeliveryFilter(this._queueDeliveryAllowed); this.sessionManager = config.sessionManager; this.settingsManager = config.settingsManager; this._scopedModels = config.scopedModels ?? []; @@ -2059,7 +2062,9 @@ export class AgentSession { // existed is how they answer it, whether it arrived mid-turn or after, so it // still runs — otherwise their request waits behind a gate only they can // open, with nothing left on screen to open it. - if (this.hasPendingUserDecision && !this._nextQueuedInputIsUser()) return false; + if (this.hasPendingUserDecision && !this.agent.hasDeliverableQueuedMessage(this._queueDeliveryAllowed)) { + return false; + } // CLM maintains context for a future request. A successful settled answer // needs no extra model call; the next prompt/continuation performs the @@ -2084,7 +2089,7 @@ export class AgentSession { // The agent loop drains both queues before emitting agent_end, except when a // tool result ended the turn first. Messages here were queued by `agent_end` // handlers, or were still pending when a control barrier ended the run. - return this.agent.hasQueuedMessages(); + return this.agent.hasDeliverableQueuedMessage(this._queueDeliveryAllowed); } /** @@ -2410,17 +2415,17 @@ export class AgentSession { * Internal: Queue a steering message (already expanded, no extension command check). */ /** - * Whether the user's own input is what the agent would deliver next. + * Whether this queued message may be delivered now. * - * A continuation delivers one queued message at a time — steering before - * follow-ups. Only the message at the head of that order is the user's answer; - * work queued ahead of it is not, and letting the whole continuation run would - * deliver that work on the user's behalf. + * A pending decision holds work that would run on the user's behalf — a goal + * continuation, a cron delivery, a lane waking its parent. It does not hold + * what the user wrote: their message is how they answer, and it may be waiting + * in the queue from before the decision existed. Asked per message, because + * the loop delivers a batch at a time and would otherwise carry the refused + * work along with the allowed one. */ - private _nextQueuedInputIsUser(): boolean { - const next = this.agent.peekNextQueued(); - return next !== undefined && this._queuedFromUser.has(next); - } + private _queueDeliveryAllowed = (message: AgentMessage): boolean => + !this.hasPendingUserDecision || this._queuedFromUser.has(message); private async _queueSteer(text: string, images?: ImageContent[], source?: InputSource): Promise { this._steeringMessages.push(text); diff --git a/packages/coding-agent/test/suite/plan-approval-gate.test.ts b/packages/coding-agent/test/suite/plan-approval-gate.test.ts index 6d1e1bc46..5edf392be 100644 --- a/packages/coding-agent/test/suite/plan-approval-gate.test.ts +++ b/packages/coding-agent/test/suite/plan-approval-gate.test.ts @@ -268,8 +268,11 @@ test("automatic work queued ahead of the user's words is still held", async () = await harness.session.prompt("/plan write the proposal"); expect(harness.session.hasPendingUserDecision).toBe(true); + // Their notes still get through — they are the answer the gate is waiting for. + expect(requests.some((request) => request.some((message) => message.includes("split the second step")))).toBe(true); + // The automatic work queued ahead of them does not, even though the turn that + // would deliver it is the one their notes justified. expect(requests.some((request) => request.some((message) => message.includes("AUTOWORK")))).toBe(false); - expect(harness.session.pendingMessageCount).toBeGreaterThan(0); }); test("a compaction does not leave a decision nobody can answer", async () => { From 5a54773e80921233958457a401e459fd89853b04 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:04:34 +0800 Subject: [PATCH 08/15] fix(agent-core,coding-agent): finish the delivery rule where it is asked 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. --- packages/agent-core/src/agent.ts | 13 ++++-- packages/agent-core/src/types.ts | 3 ++ packages/agent-core/test/agent.test.ts | 46 +++++++++++++++++++ .../coding-agent/src/core/agent-session.ts | 12 +++-- .../test/suite/plan-approval-gate.test.ts | 20 ++++++++ 5 files changed, 86 insertions(+), 8 deletions(-) diff --git a/packages/agent-core/src/agent.ts b/packages/agent-core/src/agent.ts index bde4e44ee..1f76d678b 100644 --- a/packages/agent-core/src/agent.ts +++ b/packages/agent-core/src/agent.ts @@ -151,7 +151,10 @@ class PendingMessageQueue { * hold part of the queue without dropping what it holds or reordering what it * releases. With no predicate this drains everything the mode allows. */ - drain(allow?: (message: AgentMessage) => boolean): AgentMessage[] { + drain(allowed?: (message: AgentMessage) => boolean): AgentMessage[] { + // Wrapped so a predicate that also declares (index, array) cannot be handed + // them by findIndex/some. + const allow = allowed === undefined ? undefined : (message: AgentMessage): boolean => allowed(message); if (!allow) { if (this.mode === "all") { const drained = this.messages.slice(); @@ -176,10 +179,10 @@ class PendingMessageQueue { return [first!]; } - /** Whether any queued message would pass `allow`. */ - hasDeliverable(allow?: (message: AgentMessage) => boolean): boolean { - if (!allow) return this.messages.length > 0; - return this.messages.some(allow); + /** Whether any queued message would pass `allowed`. */ + hasDeliverable(allowed?: (message: AgentMessage) => boolean): boolean { + if (!allowed) return this.messages.length > 0; + return this.messages.some((message: AgentMessage) => allowed(message)); } clear(): void { diff --git a/packages/agent-core/src/types.ts b/packages/agent-core/src/types.ts index 9127b2d7f..c36ae7996 100644 --- a/packages/agent-core/src/types.ts +++ b/packages/agent-core/src/types.ts @@ -46,6 +46,9 @@ export type ToolExecutionMode = "sequential" | "parallel"; * * - "all": drain and inject every queued message at that point. * - "one-at-a-time": drain and inject only the oldest queued message, leaving the rest queued for later drain points. + * + * A delivery filter can withhold messages: "oldest" then means the oldest one it + * allows, and withheld messages stay queued in their original order. */ export type QueueMode = "all" | "one-at-a-time"; diff --git a/packages/agent-core/test/agent.test.ts b/packages/agent-core/test/agent.test.ts index 04c977955..a1ce36ae6 100644 --- a/packages/agent-core/test/agent.test.ts +++ b/packages/agent-core/test/agent.test.ts @@ -749,6 +749,52 @@ describe("Agent", () => { expect(agent.hasQueuedMessages()).toBe(true); }); + it("holds only the refused messages when queue mode drains everything at once", async () => { + // "all" takes a batch: a filter has to split it rather than pass the whole + // batch through or refuse it entirely. + let responseCount = 0; + const agent = new Agent({ + followUpMode: "all", + streamFn: () => { + const stream = new MockAssistantStream(); + responseCount++; + queueMicrotask(() => { + stream.push({ type: "done", reason: "stop", message: createAssistantMessage(`Reply ${responseCount}`) }); + }); + return stream; + }, + }); + agent.state.messages = [ + { role: "user", content: [{ type: "text", text: "Initial" }], timestamp: Date.now() - 10 }, + createAssistantMessage("Initial response"), + ]; + const held = { role: "user" as const, content: [{ type: "text" as const, text: "Held work" }], timestamp: 1 }; + const first = { role: "user" as const, content: [{ type: "text" as const, text: "First words" }], timestamp: 2 }; + const second = { + role: "user" as const, + content: [{ type: "text" as const, text: "Second words" }], + timestamp: 3, + }; + agent.followUp(held); + agent.followUp(first); + agent.followUp(second); + + agent.setQueueDeliveryFilter((message) => message !== held); + await agent.continue(); + + const texts = agent.state.messages + .filter((message) => message.role === "user") + .map((message) => + typeof message.content === "string" + ? message.content + : message.content.map((part) => (part.type === "text" ? part.text : "")).join(""), + ); + expect(texts).toContain("First words"); + expect(texts).toContain("Second words"); + expect(texts).not.toContain("Held work"); + expect(agent.hasQueuedMessages()).toBe(true); + }); + it("should handle abort controller", () => { const agent = new Agent({ streamFn: unusedStreamFunction }); diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index e3ba7ed4e..812918941 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -1618,6 +1618,9 @@ export class AgentSession { * Call this when completely done with the session. */ dispose(): void { + // The filter is this session's, not the agent's: a caller that keeps the + // agent must not inherit a rule about a decision nobody owns any more. + this.agent.setQueueDeliveryFilter(undefined); try { this.abortRetry(); this.abortCompaction(); @@ -3287,7 +3290,9 @@ export class AgentSession { { systemPrompt: this.systemPrompt, messages: this.agent.state.messages, tools: this.agent.state.tools }, this._agentRunAbortController?.signal, ); - if (automatic?.attempted) return this.agent.hasQueuedMessages(); + // Held work does not justify a maintenance turn: the decision is still + // waiting, and a turn taken for it would deliver nothing. + if (automatic?.attempted) return this.agent.hasDeliverableQueuedMessage(this._queueDeliveryAllowed); return await this._runAutoCompaction("threshold", false); } return false; @@ -3478,8 +3483,9 @@ export class AgentSession { } // Auto-compaction can complete while follow-up/steering/custom messages are waiting. - // Continue once so queued messages are delivered. - return this.agent.hasQueuedMessages(); + // Continue once so queued messages are delivered — the ones the decision + // allows; work it holds stays queued for the turn that answers it. + return this.agent.hasDeliverableQueuedMessage(this._queueDeliveryAllowed); } catch (error) { if (started) { const aborted = this._autoCompactionAbortController?.signal.aborted ?? false; diff --git a/packages/coding-agent/test/suite/plan-approval-gate.test.ts b/packages/coding-agent/test/suite/plan-approval-gate.test.ts index 5edf392be..2540110a9 100644 --- a/packages/coding-agent/test/suite/plan-approval-gate.test.ts +++ b/packages/coding-agent/test/suite/plan-approval-gate.test.ts @@ -275,6 +275,26 @@ test("automatic work queued ahead of the user's words is still held", async () = expect(requests.some((request) => request.some((message) => message.includes("AUTOWORK")))).toBe(false); }); +test("work held by the decision is delivered once it is answered", async () => { + // Holding is not dropping: the automatic work queued before the decision must + // still run after the user answers, in one piece. + const { harness } = await createPlanHarness("step-plan-gate-held-"); + const requests: string[][] = []; + harness.setResponses([ + async () => { + await harness.session.sendCustomMessage( + { customType: "test-wake", content: "AUTOWORK", display: true, details: {} }, + { deliverAs: "followUp", triggerTurn: true }, + ); + return fauxAssistantMessage("planned"); + }, + record(requests), + ]); + await harness.session.prompt("kick"); + expect(harness.session.hasPendingUserDecision).toBe(false); + expect(requests.some((request) => request.some((message) => message.includes("AUTOWORK")))).toBe(true); +}); + test("a compaction does not leave a decision nobody can answer", async () => { const { harness, planPath } = await createPlanHarness("step-plan-gate-compact-"); await handoff(harness, planPath); From d060ac502c630f23a16943c7bb2b5658ba73cc0b Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Tue, 3 Nov 2026 12:49:31 +0800 Subject: [PATCH 09/15] fix(agent-core): let a caller ask continue() to deliver queued input `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. --- packages/agent-core/README.md | 22 +++++++ packages/agent-core/src/agent.ts | 29 ++++++++- packages/agent-core/test/agent.test.ts | 85 ++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 2 deletions(-) diff --git a/packages/agent-core/README.md b/packages/agent-core/README.md index 6fa8b66cc..5b87d7dfd 100644 --- a/packages/agent-core/README.md +++ b/packages/agent-core/README.md @@ -174,6 +174,17 @@ await agent.continue(); The last message in context must be `user` or `toolResult` (not `assistant`). +Pass `deliverQueued` when the next turn should start from queued input rather than +from the transcript. The drained message becomes that request's prompt, which is +what a caller returning after a decision needs: a follow-up queued behind the +finishing turn would otherwise wait for a request the model takes without seeing +it. A retry or a context repair must leave it off — those resume the same work, +and the message the person queued is meant to arrive after it finishes. + +```typescript +await agent.continue({ deliverQueued: true }); +``` + ### Event Types | Event | Description | @@ -389,6 +400,17 @@ agent.clearAllQueues(); Use clearSteeringQueue, clearFollowUpQueue, or clearAllQueues to drop queued messages. +A delivery filter withholds individual messages. `setQueueDeliveryFilter` is asked +per message whenever the loop takes queued input; 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. In `one-at-a-time` mode the delivered message is the +oldest one the filter allows. + +```typescript +agent.setQueueDeliveryFilter((message) => isFromPerson(message)); +agent.hasDeliverableQueuedMessage((message) => isFromPerson(message)); +``` + When steering messages are detected after a turn completes: 1. All tool calls from the current assistant message have already finished 2. Steering messages are injected diff --git a/packages/agent-core/src/agent.ts b/packages/agent-core/src/agent.ts index 1f76d678b..40df5a802 100644 --- a/packages/agent-core/src/agent.ts +++ b/packages/agent-core/src/agent.ts @@ -420,8 +420,16 @@ export class Agent { await this.runPromptMessages(messages); } - /** Continue from the current transcript. The last message must be a user or tool-result message. */ - async continue(): Promise { + /** + * Continue from the current transcript. + * + * `deliverQueued` asks for the queued input that may be delivered now to become + * this turn's prompt, instead of continuing from the transcript. A caller that + * is resuming after a retry or a context repair must leave it false: those + * resume the same work, and a follow-up the person queued is meant to arrive + * after it finishes, not to replace it. + */ + async continue(options: { deliverQueued?: boolean } = {}): Promise { if (this.activeRun) { throw new Error("Agent is already processing. Wait for completion before continuing."); } @@ -447,6 +455,23 @@ export class Agent { throw new Error("Cannot continue from message role: assistant"); } + // `runContinuation` only polls steering before its first request, so a queued + // follow-up would otherwise wait behind a request the model takes without ever + // seeing it. Delivering it here makes it that request's input. + if (options.deliverQueued) { + const queuedSteering = this.steeringQueue.drain(this.queueDeliveryFilter); + if (queuedSteering.length > 0) { + await this.runPromptMessages(queuedSteering, { skipInitialSteeringPoll: true }); + return; + } + + const queuedFollowUps = this.followUpQueue.drain(this.queueDeliveryFilter); + if (queuedFollowUps.length > 0) { + await this.runPromptMessages(queuedFollowUps); + return; + } + } + await this.runContinuation(); } diff --git a/packages/agent-core/test/agent.test.ts b/packages/agent-core/test/agent.test.ts index a1ce36ae6..aa6235583 100644 --- a/packages/agent-core/test/agent.test.ts +++ b/packages/agent-core/test/agent.test.ts @@ -705,6 +705,91 @@ describe("Agent", () => { expect(agent.state.messages).not.toContainEqual(message); }); + it("delivers a queued message before continuing from a tool result", async () => { + // The loop polls steering only before its first request, so continuing from a + // tool result would ask the model without the queued follow-up — a request + // taken on the user's behalf, past the decision the tool result waits for. + let responseCount = 0; + const seen: string[][] = []; + const agent = new Agent({ + streamFn: (_model, context) => { + const stream = new MockAssistantStream(); + responseCount++; + seen.push( + context.messages.map((message) => + typeof message.content === "string" + ? message.content + : message.content.map((part) => (part.type === "text" ? part.text : "")).join(""), + ), + ); + queueMicrotask(() => { + stream.push({ type: "done", reason: "stop", message: createAssistantMessage(`Reply ${responseCount}`) }); + }); + return stream; + }, + }); + agent.state.messages = [ + { role: "user", content: [{ type: "text", text: "Initial" }], timestamp: 1 }, + createAssistantMessage("Thinking"), + { role: "user", content: [{ type: "text", text: "Plan prompt" }], timestamp: 2 }, + { + role: "toolResult", + toolCallId: "call-1", + toolName: "exit_plan_mode", + content: [{ type: "text", text: "Submitted for review." }], + isError: false, + timestamp: 3, + }, + ]; + const followUp = { + role: "user" as const, + content: [{ type: "text" as const, text: "Their words" }], + timestamp: 4, + }; + agent.followUp(followUp); + + await agent.continue({ deliverQueued: true }); + + // One request, and the message it would have missed is the last thing in it. + expect(seen).toHaveLength(1); + expect(seen[0]?.at(-1)).toBe("Their words"); + }); + + it("continues from a tool result when the queue stays rejected", async () => { + // The ordinary continuation is unchanged: nothing deliverable means the model + // resumes from the transcript, and held work stays where it is. + let responseCount = 0; + const agent = new Agent({ + streamFn: () => { + const stream = new MockAssistantStream(); + responseCount++; + queueMicrotask(() => { + stream.push({ type: "done", reason: "stop", message: createAssistantMessage(`Reply ${responseCount}`) }); + }); + return stream; + }, + }); + agent.state.messages = [ + { role: "user", content: [{ type: "text", text: "Initial" }], timestamp: 1 }, + { + role: "toolResult", + toolCallId: "call-1", + toolName: "read_file", + content: [{ type: "text", text: "Contents" }], + isError: false, + timestamp: 2, + }, + ]; + const held = { role: "user" as const, content: [{ type: "text" as const, text: "Held" }], timestamp: 3 }; + agent.followUp(held); + agent.setQueueDeliveryFilter(() => false); + + await agent.continue({ deliverQueued: true }); + + expect(responseCount).toBe(1); + expect(agent.hasQueuedMessages()).toBe(true); + }); + it("holds a queued message a delivery filter refuses", async () => { // Holding is per message: the loop takes a batch at a time, so refusing one // message must not carry the ones behind it along, and the held message has From e6bc89a268b6823c97bcf432055e32a3415f9af4 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Tue, 3 Nov 2026 12:49:38 +0800 Subject: [PATCH 10/15] fix(coding-agent): settle a plan decision where messages are delivered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` 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. --- docs/plan-approval.md | 57 +++++-- packages/coding-agent/docs/extensions.md | 8 +- .../coding-agent/src/core/agent-session.ts | 69 +++++--- .../coding-agent/src/core/extensions/types.ts | 10 ++ .../coding-agent/src/features/step-plan.ts | 90 +++++++--- .../test/step-plan-extension.test.ts | 129 +++++++++++--- .../suite/agent-session-compaction.test.ts | 37 +++++ .../test/suite/agent-session-queue.test.ts | 35 ++++ .../test/suite/plan-approval-gate.test.ts | 157 ++++++++++++++---- packages/coding-agent/test/utilities.ts | 5 + 10 files changed, 486 insertions(+), 111 deletions(-) diff --git a/docs/plan-approval.md b/docs/plan-approval.md index a30e7c910..f504ea498 100644 --- a/docs/plan-approval.md +++ b/docs/plan-approval.md @@ -16,6 +16,11 @@ session; plan mode then stays on, the turn ends, and the proposal is recorded as this: every rpc host passes a dialog bridge, including clients with no review surface. +An rpc client states it at startup: `--plan-review-surface` (or +`RpcModeOptions.planReviewSurface`) for a host that draws the proposal and +collects the answer. Without the flag a wire client keeps the handoff behavior, +because the transport says nothing about what the client can draw. + A host without that surface cannot obtain approval. It exits plan mode and hands the proposal to the caller, but the turn ends immediately: leaving the mode does not authorize execution. The caller must approve the exact snapshot before @@ -97,7 +102,8 @@ turn: the result sets `stopTurn` in addition to `terminate`. `approvalSource` identifies who owns the decision: `"user"` for the TUI review, including feedback and dismissal, and for a review-surface host whose person -answers in the app; `"external"` for the headless / RPC caller's approval flow. It does not indicate that approval has occurred. Source +answers in the app; `"external"` for the headless / RPC caller's approval flow. +It does not indicate that approval has occurred. Source and outcome are independent: a future caller-owned `"approved"` outcome can keep `approvalSource: "external"` without claiming the user clicked the TUI. Native headless/RPC exits currently emit `"external_approval_required"` only. @@ -203,10 +209,15 @@ Guards belong to the extension generation that registered them, so a reload drop the stale ones along with the runner whose context they capture. The session asks that guard before every turn it would start on the user's behalf — a feature's `triggerTurn` message, a goal continuation, a cron delivery, -a background lane waking its parent, and post-run recovery. A feature therefore +a background lane waking its parent — and post-run recovery. A feature therefore does not need to know the other starters to respect the decision, and adding a new starter cannot silently bypass it. +Post-run recovery is the exception that proves the rule: overflow and truncation +recovery repair the transcript so the next request fits. A decision keeps them +from starting anything new, but the repair itself still runs; threshold compaction +stays deferred to the next request either way. + A feature that can defer its own trigger asks `ctx.hasPendingUserDecision()` and retries later, instead of firing into a queue that would replay work after the decision: cron keeps the job due, and a goal continuation stays pending. @@ -215,18 +226,42 @@ A user's own prompt always runs: that message is how the decision gets answered. A host bridge forwarding a real person's input reports its own `source`, so the message is not mistaken for extension-generated text. The same holds while a run is in flight: input a person typed waits in the queue, and when the turn submits -a proposal, that input is still what the next request delivers. The gate reads -the message the agent would deliver next, not whether the queue contains one — -work a feature queued ahead of the user's words still waits. +a proposal, that input is still what the next request delivers — as that request's +input, not after a request the model already took. The gate reads the message the +agent would deliver next, not whether the queue contains one — work a feature +queued ahead of the user's words still waits. +The wait is settled where the 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 an extension queued (`sendCustomMessage`) is held the same way: it is +flushed once the decision is answered, not into a question typed while it waits. The guard never guesses from wording. On a host without a review surface the decision belongs to the caller, and the only channel it has is a message; the -first turn after the handoff therefore settles the wait, and the guard stays -armed until then. A caller that wants the wait ended without writing a message -sends `/plan off` (leave without implementing) or `/plan approve` (accept the -proposal), and `/plan approve ` binds the decision to the snapshot the -caller actually read. +first message after the handoff therefore settles the wait. A caller that wants +the wait ended without writing a message sends `/plan off` (leave without +implementing) or `/plan approve` (accept the proposal), and `/plan approve +` binds the decision to the snapshot the caller actually read. + +`approve` claims the whole input, so a malformed hash is refused rather than read +as a task: answering nothing would drop the wait and hand the broken text to the +model as something to plan. `/plan approve` alone answers whatever is waiting; +`/plan approve ` must still match the reviewed snapshot after rereading the +file. Verbs are matched without case, since a host forwards what its user typed, +and a task whose text starts with one of them has to be rephrased. + +Approving, or answering with a message, delivers what the decision held. Leaving +without approving releases the gate but starts nothing, so held work runs on the +next turn that starts. + +Leaving is a stop, not only a state change: `/plan off` aborts the run in flight. +The Default announcement reaches the model's next request, so a streaming run +would otherwise keep going with the plan boundary already lifted and start +implementing the proposal the user just rejected. A session written before the decision fact recorded its wait only in the transcript. `restorePlanState` migrates it once, from the still-unanswered handoff at the tail of the branch, so upgrading does not lift the gate on a -proposal nobody answered. +proposal nobody answered. An entry that already carries `pendingProposalHash` +keeps it under any version: a build that recorded the fact before this one did +so without bumping the entry, and the transcript scan would drop a wait the +branch still reports. diff --git a/packages/coding-agent/docs/extensions.md b/packages/coding-agent/docs/extensions.md index 18df7aa27..22339c888 100644 --- a/packages/coding-agent/docs/extensions.md +++ b/packages/coding-agent/docs/extensions.md @@ -623,6 +623,10 @@ Fired for message lifecycle updates. ```typescript pi.on("message_start", async (event, ctx) => { // event.message + // event.fromUser — true when a person wrote this user message, false when an + // extension produced it. Present on user messages only; absent on other roles. + // A queued message and a prompt look the same in the transcript, so an extension + // that reacts to "the user spoke" reads this, not the content. }); pi.on("message_update", async (event, ctx) => { @@ -978,7 +982,7 @@ Current run mode: `"tui"`, `"rpc"`, `"json"`, or `"print"`. Use `ctx.mode === "t `true` when the host can show a plan proposal and collect a decision for it. `mode` and `hasUI` cannot answer this: every RPC host has a dialog bridge, including clients with no review surface and no person to answer. -`exit_plan_mode` reads it. With the surface, the proposal is shown, the turn ends, and plan mode stays on until the user decides; without it, the mode ends and approval belongs to the caller. Hosts set it when they bind the session (`bindExtensions({ planReviewSurface: true })`); an extension sees the value on every handler's `ctx`. +`exit_plan_mode` reads it. With the surface, the proposal is shown, the turn ends, and plan mode stays on until the user decides; without it, the mode ends and approval belongs to the caller. Hosts set it when they bind the session (`bindExtensions({ planReviewSurface: true })`), or across the wire with `--plan-review-surface`; an extension sees the value on every handler's `ctx`. ### ctx.cwd @@ -3016,7 +3020,7 @@ const highlighted = highlightCode(code, lang, theme); | Mode | `ctx.mode` | `ctx.hasUI` | `ctx.planReviewSurface` | Notes | |------|------------|-------------|------------------------|-------| | Interactive | `"tui"` | `true` | `false` | Full TUI with terminal rendering; a plan review is answered inside the tool call | -| RPC (`--mode rpc`) | `"rpc"` | `true` | set by the host | Dialogs and notifications via JSON protocol; `custom()` returns `undefined`. See [rpc.md](rpc.md) | +| RPC (`--mode rpc`) | `"rpc"` | `true` | set by `--plan-review-surface` | Dialogs and notifications via JSON protocol; `custom()` returns `undefined`. A client that draws the proposal passes the flag. See [rpc.md](rpc.md) | | JSON (`--mode json`) | `"json"` | `false` | `false` | Event stream to stdout; UI methods are no-ops | | Print (`-p`) | `"print"` | `false` | `false` | Extensions run but can't prompt | diff --git a/packages/coding-agent/src/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index 812918941..b00abc9e6 100644 --- a/packages/coding-agent/src/core/agent-session.ts +++ b/packages/coding-agent/src/core/agent-session.ts @@ -472,7 +472,7 @@ export class AgentSession { * the API it used. Tracking the source here keeps every command — built-in or * third-party — from having to repeat it at each call site. */ - private _commandSources: InputSource[] = []; + private _commandSources: Array<{ source: InputSource }> = []; private _modelRuntime: ModelRuntime; private _agentDir?: string; @@ -1509,6 +1509,7 @@ export class AgentSession { const extensionEvent: MessageStartEvent = { type: "message_start", message: event.message, + ...(event.message.role === "user" ? { fromUser: this._queuedFromUser.has(event.message) } : {}), }; await this._extensionRunner.emit(extensionEvent); } else if (event.type === "message_update") { @@ -2009,12 +2010,13 @@ export class AgentSession { } : undefined; await this.agent.prompt(messages); - while ( - !runAbortController.signal.aborted && - (await this._handlePostAgentRun()) && - !runAbortController.signal.aborted - ) { - await this.agent.continue(); + while (!runAbortController.signal.aborted) { + const next = await this._handlePostAgentRun(); + if (next === "stop" || runAbortController.signal.aborted) break; + // Only the queued-work decision turns a waiting message into this turn's + // input. A retry or a context repair resumes the same work, and a message + // the person queued behind it is meant to arrive after it finishes. + await this.agent.continue({ deliverQueued: next === "deliver" }); } } finally { this._checkedInitialRequest = undefined; @@ -2033,18 +2035,18 @@ export class AgentSession { } } - private async _handlePostAgentRun(): Promise { + private async _handlePostAgentRun(): Promise<"stop" | "retry" | "recover" | "deliver"> { const msg = this._lastAssistantMessage; this._lastAssistantMessage = undefined; if (!msg) { - return false; + return "stop"; } if (this._isRetryableError(msg) && (await this._prepareRetry(msg))) { - return true; + return "retry"; } - if (this._agentRunAbortController?.signal.aborted) return false; + if (this._agentRunAbortController?.signal.aborted) return "stop"; if (msg.stopReason === "error" && this._retryAttempt > 0) { this._emit({ @@ -2066,7 +2068,11 @@ export class AgentSession { // still runs — otherwise their request waits behind a gate only they can // open, with nothing left on screen to open it. if (this.hasPendingUserDecision && !this.agent.hasDeliverableQueuedMessage(this._queueDeliveryAllowed)) { - return false; + // Overflow and truncation recovery repair the transcript so the next + // request fits. They are maintenance, not work on the user's behalf, so a + // decision still holds them off from *starting* anything but lets them run: + // threshold compaction stays deferred to the next request either way. + return (await this._checkCompaction(msg, true, true)) ? "recover" : "stop"; } // CLM maintains context for a future request. A successful settled answer @@ -2080,19 +2086,19 @@ export class AgentSession { !this.agent.hasQueuedMessages() && this.pendingMessageCount === 0 ) { - return false; + return "stop"; } // Under the v2 trigger policy, threshold compaction waits for the next request's // before-request check; only overflow and truncation recovery run here. if (await this._checkCompaction(msg, true, this._usesTriggerPolicyV2())) { - return true; + return "recover"; } // The agent loop drains both queues before emitting agent_end, except when a // tool result ended the turn first. Messages here were queued by `agent_end` // handlers, or were still pending when a control barrier ended the run. - return this.agent.hasDeliverableQueuedMessage(this._queueDeliveryAllowed); + return this.agent.hasDeliverableQueuedMessage(this._queueDeliveryAllowed) ? "deliver" : "stop"; } /** @@ -2230,11 +2236,16 @@ export class AgentSession { if (currentImages) { userContent.push(...currentImages); } - messages.push({ + const userMessage: AgentMessage = { role: "user", content: userContent, timestamp: Date.now(), - }); + }; + // A prompt a person typed is theirs, whatever delivered it. Recorded by + // identity so the delivery rule and the delivery event can tell it from + // work an extension queued on their behalf. + if ((options?.source ?? "interactive") !== "extension") this._queuedFromUser.add(userMessage); + messages.push(userMessage); // Inject any pending "nextTurn" messages as context alongside the user message for (const msg of this._pendingNextTurnMessages) { @@ -2310,8 +2321,12 @@ export class AgentSession { const ctx = this._extensionRunner.createCommandContext(); const turns: Promise[] = []; + // The source entry travels with its own handler: two commands can overlap and + // finish out of order, and removing by position keeps each one's source with + // the work it forwarded. + const sourceEntry = { source }; this._openCommandTurns.push(turns); - this._commandSources.push(source); + this._commandSources.push(sourceEntry); try { await command.handler(args, ctx); } catch (err) { @@ -2322,9 +2337,10 @@ export class AgentSession { error: err instanceof Error ? err.message : String(err), }); } finally { - const index = this._openCommandTurns.indexOf(turns); - if (index !== -1) this._openCommandTurns.splice(index, 1); - this._commandSources.pop(); + const turnIndex = this._openCommandTurns.indexOf(turns); + if (turnIndex !== -1) this._openCommandTurns.splice(turnIndex, 1); + const sourceIndex = this._commandSources.indexOf(sourceEntry); + if (sourceIndex !== -1) this._commandSources.splice(sourceIndex, 1); } // The handler may have handed work to the agent and returned. Acceptance is @@ -2542,10 +2558,17 @@ export class AgentSession { /** * Append custom messages queued while the agent was running. * Called once the current turn's tool results are in agent state and session history. + * + * A pending decision holds them, like every other message no person wrote: these + * are context an extension queued on the user's behalf, and the rule that keeps + * automatic work out of a turn applies to them too. They flush once the decision + * is answered — at that turn's end, or at the next one. */ private _flushPendingCustomMessages(): void { if (this._pendingCustomMessages.length === 0) return; - + // Held like any other message no person wrote: it flushes once the decision is + // answered, not into a question typed while it waits. + if (this.hasPendingUserDecision) return; const pending = this._pendingCustomMessages; this._pendingCustomMessages = []; for (const appMessage of pending) { @@ -2598,7 +2621,7 @@ export class AgentSession { // through: the user typed the command, so the text may answer a pending // decision. Outside a command, the default keeps extension-generated // text behind the gate. - source: options?.source ?? this._commandSources.at(-1) ?? "extension", + source: options?.source ?? this._commandSources.at(-1)?.source ?? "extension", }); } diff --git a/packages/coding-agent/src/core/extensions/types.ts b/packages/coding-agent/src/core/extensions/types.ts index 73ea28519..510641a16 100644 --- a/packages/coding-agent/src/core/extensions/types.ts +++ b/packages/coding-agent/src/core/extensions/types.ts @@ -884,6 +884,16 @@ export interface TurnEndEvent { export interface MessageStartEvent { type: "message_start"; message: AgentMessage; + /** + * The message was written by a person, not generated on their behalf. + * + * Present on user messages: `true` when the person wrote it, `false` when an + * extension produced it. Absent on other roles. A queued message and a prompt + * look identical in the transcript, so an extension that must react to "the + * user spoke" — rather than to "some user message exists" — reads this instead + * of the content. + */ + fromUser?: boolean; } /** Fired during assistant message streaming with token-by-token updates */ diff --git a/packages/coding-agent/src/features/step-plan.ts b/packages/coding-agent/src/features/step-plan.ts index ecb95ba8e..39b4126a9 100644 --- a/packages/coding-agent/src/features/step-plan.ts +++ b/packages/coding-agent/src/features/step-plan.ts @@ -28,8 +28,8 @@ import { type LegacyPlanTodoFields, migrateLegacyPlanTodos } from "./plan-mode-m import { buildDefaultModePrompt, buildPlanModePrompt, + PLAN_MODE_BLOCK_VERSION, PLAN_MODE_MESSAGE_TYPE, - PLAN_MODE_PROMPT_VERSION, } from "./plan-mode-prompt.ts"; import { getPlanFilePath, @@ -136,12 +136,12 @@ function announcementFor( if (planFilePath !== undefined) { // A block written by an older build states older rules; resuming it keeps // the model on those rules until a new turn appends the current one. - const current = visible?.version === PLAN_MODE_PROMPT_VERSION; + const current = visible?.version === PLAN_MODE_BLOCK_VERSION; if (visible?.mode === "plan" && visible.planFilePath === planFilePath && current) return undefined; const details: PlanModeAnnouncement = { mode: "plan", planFilePath, - version: PLAN_MODE_PROMPT_VERSION, + version: PLAN_MODE_BLOCK_VERSION, }; return { customType: PLAN_MODE_MESSAGE_TYPE, @@ -609,13 +609,18 @@ export const createStepPlanExtension = // Absolute verbs, so a host that can only send text can express // "leave" and "approve" without racing a toggle. A queued `/plan` // would otherwise flip whatever state the turn left behind. - if (prompt === "on") { + // + // Matched without case: a host may capitalize what it forwards, and a verb + // it did not recognize would fall through to "plan this text", clearing the + // wait it meant to answer. + const verb = prompt.toLowerCase(); + if (verb === "on") { // The absolute counterpart of `off`, so a host never sends a toggle // whose effect depends on the state a turn left behind. if (!enabled) enter(ctx); return; } - if (prompt === "off") { + if (verb === "off") { // Leaving is a decision even when the mode already ended: a caller // handoff leaves the proposal waiting, and the user saying "off" is // the answer that releases it. @@ -624,10 +629,24 @@ export const createStepPlanExtension = return; } // `/plan approve` answers the proposal, `/plan approve ` answers - // that one. A hash is 64 lowercase hex characters, so any other text - // after the verb is a task and falls through to the branch below. - const namedProposal = /^approve\s+([0-9a-f]{64})$/.exec(prompt)?.[1]; - if (prompt === "approve" || namedProposal !== undefined) { + // that one. The verb claims the whole input: a host that sends a + // truncated or upper-cased hash must not have its text read as a task to + // plan — that would drop the wait it meant to answer and hand the broken + // hash to the model. + const approveMatch = /^approve(?:\s+(.*))?$/s.exec(verb); + if (approveMatch) { + // `verb` is already lowercased, so the argument matches a hash whichever + // case the host sent. + const namedProposal = approveMatch[1]?.trim(); + // An empty argument answers whatever is waiting. A named one has to be + // the snapshot hash exactly: anything else is not a decision. + if (namedProposal !== undefined && !/^[0-9a-f]{64}$/.test(namedProposal)) { + ctx.ui.notify( + "That is not a proposal hash. Use `/plan approve`, `/plan approve `, or `/plan ` to plan something else.", + "warning", + ); + return; + } // The user accepted the proposal the host showed them. Leaving the // mode and starting the implementation turn are one decision: the // only way this turn runs with file mutations enabled. @@ -655,8 +674,12 @@ export const createStepPlanExtension = } if (current !== pendingProposal) { // Nothing was decided: the wait stays, so automatic work keeps - // holding until someone reviews the text that is on disk now. - ctx.ui.notify("The plan changed since it was submitted; ask for a new review.", "warning"); + // holding until someone reviews the text that is on disk now. Leaving + // is the way out when the text will not be reviewed again. + ctx.ui.notify( + "The plan changed since it was submitted; ask for a new review, or use `/plan off` to leave plan mode.", + "warning", + ); return; } // A caller handoff already left the mode; the caller answering in @@ -698,10 +721,11 @@ export const createStepPlanExtension = // // The words are the user's, forwarded: a proposal submitted by the turn // this waited behind must not turn their request for changes into - // extension traffic that the pending-decision guard then holds back. + // extension traffic that the pending-decision guard then holds back. The + // session already knows who typed the command, so this forwards it rather + // than guessing from the transport. pi.sendUserMessage(prompt, { expandPromptTemplates: true, - source: ctx.mode === "tui" ? "interactive" : "rpc", ...(ctx.isIdle() ? {} : { deliverAs: "followUp" as const }), }); }, @@ -726,13 +750,11 @@ export const createStepPlanExtension = // rewriting the system prompt or tool list. The block persists with the // turn, so later requests reuse it from the cached history. pi.on("before_agent_start", async (_event, ctx) => { - // Who can answer this proposal? - // - // A host with a review surface asks its user while the mode is on, so the - // fact stays until an explicit verb — there a question is not consent. But - // a pending left by a caller handoff (`enabled` already false) has no - // surface left to answer it, even on this host: opening such a session in - // the app must not lock it, so the next user turn is the decision. + // A proposal the caller was handed has no surface left to answer it, even + // on a host that has one: opening such a session in the app must not lock + // it, so the next user turn is the decision. A host with a review surface + // asks its user while the mode is on, so there the fact stays until an + // explicit verb — a question is not consent. if (pendingProposal !== undefined && (!enabled || !ctx.planReviewSurface)) clearPendingProposal(); const visible = lastAnnouncement( ctx.sessionManager.buildContextEntries().filter((entry) => entry.type === "custom_message"), @@ -745,6 +767,24 @@ export const createStepPlanExtension = return message ? { message } : undefined; }); + // A person's message settles a wait only where that wait has no other channel. + // + // A host with a review surface asks its user while the mode is on, so there + // the fact stays until an explicit verb — a question is not consent. But a + // proposal the caller was handed (`enabled` already false) has no surface + // left to answer it, even on this host: opening such a session in the app + // must not lock it, so the next thing the person says is the decision. + // + // Judged at delivery rather than when 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. + pi.on("message_start", (event, ctx) => { + if (!event.fromUser) return; + if (pendingProposal === undefined) return; + if (enabled && ctx.planReviewSurface) return; + clearPendingProposal(); + }); + // The context hook reconciles the request view without writing history. // Compaction can summarize the announcement away, and the mode can change // mid-run (exit_plan_mode approval, /plan while streaming) without a new @@ -799,11 +839,17 @@ export const createStepPlanExtension = // A proposal still waiting survives a restart, including a caller handoff // that already left the mode: the guard must keep holding work until // someone answers it, and only the answer clears this. - pendingProposal = restoredPlanState?.pendingProposalHash; + const recordedPending = restoredPlanState?.pendingProposalHash; + pendingProposal = typeof recordedPending === "string" ? recordedPending : undefined; // An entry older than the fact recorded the wait only in the transcript. // Migrate it once, so upgrading does not silently lift the gate on a // session whose proposal nobody has answered. - if (restoredPlanState && (restoredPlanState.version ?? 2) < 3) { + // + // Only when the entry carries no fact of its own: a build that wrote the + // hash under an older entry version already answered this, and the + // transcript scan would drop it — a question typed past a proposal makes + // the scan report none, which is exactly the wait that build keeps. + if (restoredPlanState && pendingProposal === undefined && (restoredPlanState.version ?? 2) < 3) { pendingProposal = findLegacyUnansweredHandoff(ctx.sessionManager)?.planHash; // Written back once, so every reader of the branch — the automatic-turn // guard, the RPC and SDK replies, a host's card — sees one fact rather diff --git a/packages/coding-agent/test/step-plan-extension.test.ts b/packages/coding-agent/test/step-plan-extension.test.ts index df6e09480..f94bba9d1 100644 --- a/packages/coding-agent/test/step-plan-extension.test.ts +++ b/packages/coding-agent/test/step-plan-extension.test.ts @@ -9,7 +9,7 @@ import { createEventBus } from "../src/core/event-bus.ts"; import type { ExtensionAPI, ExtensionContext, ToolDefinition } from "../src/core/extensions/types.ts"; import { KeybindingsManager } from "../src/core/keybindings.ts"; import { SessionManager } from "../src/core/session-manager.ts"; -import { PLAN_MODE_PROMPT_VERSION } from "../src/features/plan-mode-prompt.ts"; +import { PLAN_MODE_BLOCK_VERSION } from "../src/features/plan-mode-prompt.ts"; import { getPlanFilePath } from "../src/features/plan-mode-tools.ts"; import { createStepExtension } from "../src/features/step.ts"; import { createStepPlanExtension } from "../src/features/step-plan.ts"; @@ -921,9 +921,10 @@ test("/plan approve refuses when no proposal is waiting", async () => { expect(harness.notifications.some((notice) => /No plan is waiting for approval/.test(notice.message))).toBe(true); }); -test("/plan approve is a task, not a decision", async () => { - // Only a snapshot hash names a proposal. Any other text after the verb is - // something to plan, and dropping it would silently lose what the user wrote. +test("/plan approve with text that is not a hash is refused, not read as a task", async () => { + // The verb claims the whole input. A host that sends a truncated or upper-cased + // hash must not have its text read as something to plan: that would drop the + // wait it meant to answer and hand the broken hash to the model. const cwd = makeWorkspace(); const harness = createHarness({ cwd }); createStepPlanExtension()(harness.api); @@ -933,13 +934,59 @@ test("/plan approve is a task, not a decision", async () => { mkdirSync(path.dirname(planPath), { recursive: true }); writeFileSync(planPath, "# Plan\n1. Do the thing\n"); await runTool(harness, "exit_plan_mode", {}, context); + const waiting = lastPlanEntry(harness).pendingProposalHash; await harness.commands.get("plan")!.handler("approve the payment flow", context); - expect(harness.userMessages.at(-1)?.[0]).toBe("approve the payment flow"); + // Nothing ran and nothing was decided: the wait is still there. + expect(harness.userMessages).toEqual([]); expect(lastPlanEntry(harness)).toMatchObject({ enabled: true }); + expect(lastPlanEntry(harness).pendingProposalHash).toBe(waiting); + expect(harness.notifications.some((notice) => /not a proposal hash/.test(notice.message))).toBe(true); +}); + +test("/plan approve accepts a hash in any case", async () => { + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + const hash = String(lastPlanEntry(harness).pendingProposalHash); + + await harness.commands.get("plan")!.handler(`approve ${hash.toUpperCase()}`, context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); }); +test("/plan verbs are matched without case", async () => { + // A host forwards what its user typed, and a capitalized verb must not fall + // through to "plan this text": that would clear the wait it meant to answer. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ mode: "rpc", hasUI: true, planReviewSurface: true }); + await enterPlanMode(harness, context); + const planPath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Plan\n1. Do the thing\n"); + await runTool(harness, "exit_plan_mode", {}, context); + const hash = String(lastPlanEntry(harness).pendingProposalHash); + + await harness.commands.get("plan")!.handler(`Approve ${hash.toUpperCase()}`, context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); + expect(harness.userMessages).toEqual([]); + + // `OFF` leaves without implementing anything. + await enterPlanMode(harness, context); + await harness.commands.get("plan")!.handler("OFF", context); + expect(lastPlanEntry(harness)).toMatchObject({ enabled: false }); + expect(harness.userMessages).toEqual([]); +}); + test("a pending write the branch rejects leaves nothing waiting", async () => { // 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. @@ -998,7 +1045,7 @@ test("a revision requested mid-run is queued behind the running turn", async () // the pending-decision guard holds back. expect(harness.userMessages.at(-1)).toEqual([ "split the second step", - { expandPromptTemplates: true, source: "rpc", deliverAs: "followUp" }, + { expandPromptTemplates: true, deliverAs: "followUp" }, ]); expect(harness.customMessages).toEqual([]); // The old proposal is no longer waiting, so the session holds no gate while @@ -1101,6 +1148,57 @@ test("a session written before the decision fact keeps its gate after an upgrade expect(harness.customMessages.some(([message]) => message.customType === "step-plan-approved")).toBe(true); }); +test("an older entry that already carries the fact keeps it instead of re-deriving it", async () => { + // A build that wrote `pendingProposalHash` under `version: 2` keeps a wait even + // when the user asked a question after submitting. The transcript scan cannot + // see that — it stops at the user message — so re-deriving would drop a wait + // the branch still reports. + const cwd = makeWorkspace(); + const planContents = "# Plan\n1. Do the thing\n"; + const planFilePath = path.join(cwd, ".stepcode", "plans", "session-sess-1.md"); + mkdirSync(path.dirname(planFilePath), { recursive: true }); + writeFileSync(planFilePath, planContents); + const planHash = createHash("sha256").update(planContents, "utf8").digest("hex"); + + const harness = createHarness({ cwd }); + harness.session.appendCustomEntry("step-plan", { + version: 2, + enabled: true, + planFilePath, + pendingProposalHash: planHash, + }); + harness.session.appendMessage({ + role: "toolResult", + toolCallId: "handoff", + toolName: "exit_plan_mode", + content: [{ type: "text", text: `Review this plan.\n\n${planContents}` }], + details: { + outcome: "external_approval_required", + planFilePath, + planContents, + planHash, + planModeExited: false, + }, + isError: false, + timestamp: Date.now(), + }); + // The user is still deciding; a question is not an answer. + harness.session.appendMessage({ + role: "user", + content: [{ type: "text", text: "Why the second step?" }], + timestamp: Date.now(), + }); + createStepPlanExtension()(harness.api); + const before = harness.appended.filter((entry) => entry.customType === "step-plan").length; + await restoreLifecycle(harness, "session_start"); + + // Nothing was migrated away: the recorded fact is the answer. + expect(harness.appended.filter((entry) => entry.customType === "step-plan")).toHaveLength(before); + await harness.commands.get("plan")!.handler("approve", harness.ctx()); + expect(lastPlanEntry(harness).pendingProposalHash).toBeUndefined(); + expect(harness.customMessages.some(([message]) => message.customType === "step-plan-approved")).toBe(true); +}); + test("a legacy handoff behind a summary is not restored as a wait", async () => { // The old gate scanned the conversation the model reads; a summary boundary // means the handoff is no longer in it, so a wait restored from behind one is @@ -1307,10 +1405,7 @@ test.each(planEntryCases)( const toolsBefore = harness.activeTools(); // `/plan ` while planning hands over the task without re-entering. await harness.commands.get("plan")!.handler("refine the proposal", context); - expect(harness.userMessages.at(-1)).toEqual([ - "refine the proposal", - { expandPromptTemplates: true, source: "interactive" }, - ]); + expect(harness.userMessages.at(-1)).toEqual(["refine the proposal", { expandPromptTemplates: true }]); expect(harness.appended).toHaveLength(snapshotsBefore); expect(events).toHaveLength(telemetryBefore); expect(events[0]).toEqual({ event: "plan_mode_entered", properties: { source } }); @@ -2481,9 +2576,7 @@ test("/plan with a task enables plan mode and sends the task as a user turn", as await harness.commands.get("plan")!.handler("实现一个俄罗斯方块,简单版", context); expect(harness.statuses.get("plan-mode")).toBe("Planning (user)"); - expect(harness.userMessages).toEqual([ - ["实现一个俄罗斯方块,简单版", { expandPromptTemplates: true, source: "interactive" }], - ]); + expect(harness.userMessages).toEqual([["实现一个俄罗斯方块,简单版", { expandPromptTemplates: true }]]); // The turn must start under the plan-mode tool set, not the unrestricted one. expect(harness.activeTools()).not.toContain("edit_file_unrestricted"); expect(harness.activeTools()).toContain("read_file"); @@ -2509,9 +2602,7 @@ test("/plan with a task queues as a follow-up while the agent is streaming", asy await harness.commands.get("plan")!.handler("plan this too", harness.ctx({ isIdle: () => false })); - expect(harness.userMessages).toEqual([ - ["plan this too", { expandPromptTemplates: true, source: "interactive", deliverAs: "followUp" }], - ]); + expect(harness.userMessages).toEqual([["plan this too", { expandPromptTemplates: true, deliverAs: "followUp" }]]); }); test("bare /plan still toggles plan mode and sends nothing", async () => { @@ -2601,9 +2692,7 @@ test("/plan announces plan mode on the turn that carries the task", async const harness = createHarness({ cwd: makeWorkspace() }); createStepPlanExtension()(harness.api); await harness.commands.get("plan")!.handler("plan the parser rewrite", harness.ctx()); - expect(harness.userMessages).toEqual([ - ["plan the parser rewrite", { expandPromptTemplates: true, source: "interactive" }], - ]); + expect(harness.userMessages).toEqual([["plan the parser rewrite", { expandPromptTemplates: true }]]); expect((await startTurn(harness))?.details.mode).toBe("plan"); }); @@ -2696,7 +2785,7 @@ test.each(["session_start", "session_tree"] as const)( async (type) => { const planFilePath = "/workspace/.stepcode/plans/session-sess-1.md"; const announced = (mode: "plan" | "default") => - mode === "plan" ? { mode, planFilePath, version: PLAN_MODE_PROMPT_VERSION } : { mode }; + mode === "plan" ? { mode, planFilePath, version: PLAN_MODE_BLOCK_VERSION } : { mode }; // A plan that was already announced in the current wording: no repeat. const legacy = createHarness({ diff --git a/packages/coding-agent/test/suite/agent-session-compaction.test.ts b/packages/coding-agent/test/suite/agent-session-compaction.test.ts index e0485d1e7..a1eb6407c 100644 --- a/packages/coding-agent/test/suite/agent-session-compaction.test.ts +++ b/packages/coding-agent/test/suite/agent-session-compaction.test.ts @@ -663,6 +663,43 @@ describe("AgentSession compaction characterization", () => { ); }); + it("recovers from a truncated response even while an automatic-turn guard holds", async () => { + // Recovery repairs the transcript for the next request; it is not work on the + // user's behalf. A guard that blocks new turns must not turn an error into a + // dead end, so the retry still happens once. + const harness = await createHarness({ + models: [{ id: "faux-1", contextWindow: 1_000_000, maxTokens: 100 }], + settings: { compaction: { keepRecentTokens: 1, reserveTokens: 0 } }, + bindings: { mode: "print" }, + extensionFactories: [ + (pi) => { + pi.on("session_start", (_event, ctx) => { + ctx.registerAutomaticTurnGuard?.(() => true); + }); + pi.on("session_before_compact", async (event) => ({ + compaction: { + summary: "overflow compacted", + firstKeptEntryId: event.preparation.firstKeptEntryId, + tokensBefore: event.preparation.tokensBefore, + details: {}, + }, + })); + }, + ], + }); + harnesses.push(harness); + harness.setResponses([ + () => fauxAssistantMessage("x".repeat(64), { stopReason: "length", timestamp: Date.now() + 10_000 }), + () => fauxAssistantMessage("y".repeat(64), { stopReason: "length", timestamp: Date.now() + 10_000 }), + ]); + + await harness.session.prompt("x".repeat(5000)); + + expect(harness.session.hasPendingUserDecision).toBe(true); + expect(harness.faux.state.callCount).toBe(2); + expect(harness.eventsOfType("compaction_start").filter((event) => event.reason === "overflow")).toHaveLength(1); + }); + it("cancels in-progress manual compaction when abortCompaction is called", async () => { const harness = await createHarness({ settings: { compaction: { keepRecentTokens: 1 } }, diff --git a/packages/coding-agent/test/suite/agent-session-queue.test.ts b/packages/coding-agent/test/suite/agent-session-queue.test.ts index a80a6f28c..5b6f23273 100644 --- a/packages/coding-agent/test/suite/agent-session-queue.test.ts +++ b/packages/coding-agent/test/suite/agent-session-queue.test.ts @@ -420,6 +420,41 @@ describe("AgentSession queue characterization", () => { ); }); + it("keeps a follow-up queued during a failed attempt out of the retry request", async () => { + // A retry resumes the same work. A message the person queued behind it is + // meant to arrive after that work finishes, so delivering it into the retried + // request would answer it instead of what was asked. + const harness = await createHarness({ + settings: { retry: { enabled: true, maxRetries: 2, baseDelayMs: 0 } }, + }); + harnesses.push(harness); + + const requests: string[][] = []; + harness.setResponses([ + async () => { + await harness.session.sendUserMessage("queued words", { deliverAs: "followUp" }); + return fauxAssistantMessage("", { stopReason: "error", errorMessage: "503 overloaded_error" }); + }, + (context) => { + requests.push(context.messages.map((message) => getMessageText(message))); + return fauxAssistantMessage("answered the original"); + }, + (context) => { + requests.push(context.messages.map((message) => getMessageText(message))); + return fauxAssistantMessage("answered the follow-up"); + }, + ]); + + await harness.session.prompt("original question"); + await harness.session.agent.waitForIdle(); + + // The retried request answers the original question, without the follow-up. + expect(requests[0]).not.toContain("queued words"); + // The follow-up runs afterwards, in its own request. + expect(requests.at(-1)).toContain("queued words"); + expect(getUserTexts(harness)).toEqual(["original question", "queued words"]); + }); + it("delivers follow-ups queued during agent_end", async () => { let sent = false; const harness = await createHarness({ diff --git a/packages/coding-agent/test/suite/plan-approval-gate.test.ts b/packages/coding-agent/test/suite/plan-approval-gate.test.ts index 2540110a9..caad206e8 100644 --- a/packages/coding-agent/test/suite/plan-approval-gate.test.ts +++ b/packages/coding-agent/test/suite/plan-approval-gate.test.ts @@ -160,10 +160,10 @@ test("a reload does not leave an automatic-turn guard stranded", async () => { expect(harness.session.hasPendingUserDecision).toBe(false); }); -test("a caller handoff leaves the decision pending across a resume", async () => { +test("a caller handoff leaves the decision pending across a reload", async () => { // A handoff that leaves the mode still has an unanswered proposal, and the - // guard must keep holding automatic work after a restart: only the answer - // clears it. + // guard must keep holding automatic work after the extension generation is + // replaced: only the answer clears it. const { harness, planPath } = await createPlanHarness("step-plan-gate-resume-"); await handoff(harness, planPath); const planEntry = harness.sessionManager @@ -176,37 +176,35 @@ test("a caller handoff leaves the decision pending across a resume", async () => // survives with `enabled: false` — that is what keeps the guard holding. expect(planEntry).toMatchObject({ enabled: false, pendingProposalHash: expect.any(String) }); - // The restart replaces the extension generation and replays the branch, so the + // The reload replaces the extension generation and replays the branch, so the // new guard has to read the wait back from the entry rather than from memory. - // The guard keeps holding automatic work until someone answers, and only an - // answer clears the fact. - harness.setResponses([fauxAssistantMessage("implemented")]); - await harness.session.prompt("Approved."); - expect(harness.session.hasPendingUserDecision).toBe(false); + harness.setResponses([fauxAssistantMessage("after reload")]); + await harness.session.reload(); + expect(harness.session.hasPendingUserDecision).toBe(true); }); test("notes sent mid-run reach the model as they are, with no second message", async () => { // The revision is one message: the user's notes. A hidden companion could not - // be kept in the same request — the follow-up queue delivers one message per - // request by default — and would tell the model to resubmit before it had - // read what to change. - const { harness } = await createPlanHarness("step-plan-gate-revision-"); - const seen: string[][] = []; - harness.setResponses([ - async () => { - await harness.session.sendUserMessage("NOTES", { deliverAs: "followUp" }); - return fauxAssistantMessage("still running"); - }, - (context) => { - seen.push(context.messages.map((message) => JSON.stringify(message))); - return fauxAssistantMessage("revised"); - }, - ]); - await harness.session.prompt("kick"); + // be kept in the same request, and would tell the model to resubmit before it + // had read what to change. + const { harness, planPath } = await createPlanHarness("step-plan-gate-revision-", { + mode: "rpc", + planReviewSurface: true, + }); + mkdirSync(dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Proposal\nDo it.\n"); + const requests: string[][] = []; + harness.setResponses([fauxAssistantMessage([fauxToolCall("exit_plan_mode", {})])]); + await harness.session.prompt("/plan write the proposal"); + expect(harness.session.hasPendingUserDecision).toBe(true); + + harness.setResponses([record(requests)]); + await harness.session.prompt("/plan split the second step"); - const request = seen.at(-1) ?? []; - expect(request.some((message) => message.includes("NOTES"))).toBe(true); - expect(request.some((message) => message.includes("call exit_plan_mode again"))).toBe(false); + const request = requests.at(-1) ?? []; + // The notes are the whole message: they arrive as the user's own text and are + // the last thing the model sees, with no hidden instruction appended after them. + expect(request.at(-1)).toContain("split the second step"); }); test("a change request queued before the submission still reaches the model", async () => { @@ -235,8 +233,11 @@ test("a change request queued before the submission still reaches the model", as // The submission armed the gate: the proposal is waiting for an answer. expect(harness.session.hasPendingUserDecision).toBe(true); - // Their words are the next thing the agent delivers, so they run. - expect(requests.some((request) => request.some((message) => message.includes("split the second step")))).toBe(true); + // Their words are the input of the next request, not something the model is + // asked about and told afterwards: the turn that submits ends, and the message + // it held enters before the model is asked anything. + expect(requests).toHaveLength(1); + expect(requests[0]?.at(-1)).toContain("split the second step"); }); test("automatic work queued ahead of the user's words is still held", async () => { @@ -278,7 +279,12 @@ test("automatic work queued ahead of the user's words is still held", async () = test("work held by the decision is delivered once it is answered", async () => { // Holding is not dropping: the automatic work queued before the decision must // still run after the user answers, in one piece. - const { harness } = await createPlanHarness("step-plan-gate-held-"); + const { harness, planPath } = await createPlanHarness("step-plan-gate-held-", { + mode: "rpc", + planReviewSurface: true, + }); + mkdirSync(dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Proposal\nDo it.\n"); const requests: string[][] = []; harness.setResponses([ async () => { @@ -286,15 +292,100 @@ test("work held by the decision is delivered once it is answered", async () => { { customType: "test-wake", content: "AUTOWORK", display: true, details: {} }, { deliverAs: "followUp", triggerTurn: true }, ); - return fauxAssistantMessage("planned"); + return fauxAssistantMessage([fauxToolCall("exit_plan_mode", {})]); }, record(requests), ]); - await harness.session.prompt("kick"); + await harness.session.prompt("/plan write the proposal"); + + // The submission holds the automatic work: it reached no request. + expect(harness.session.hasPendingUserDecision).toBe(true); + expect(requests.some((request) => request.some((message) => message.includes("AUTOWORK")))).toBe(false); + + // Answering releases it. The approval turn runs first, then the work it held. + harness.setResponses([fauxAssistantMessage("implementing"), record(requests), record(requests)]); + await harness.session.prompt("/plan approve"); expect(harness.session.hasPendingUserDecision).toBe(false); expect(requests.some((request) => request.some((message) => message.includes("AUTOWORK")))).toBe(true); }); +test("a caller handoff is settled by the message that answers it, at delivery", async () => { + // The message arrives through the queue, not through a new `prompt`: nothing + // runs `before_agent_start`, so the delivery itself has to settle the wait. + const { harness, planPath } = await createPlanHarness("step-plan-gate-delivery-", { mode: "rpc" }); + mkdirSync(dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Proposal\nDo it.\n"); + const requests: string[][] = []; + harness.setResponses([ + async () => { + await harness.session.sendUserMessage("Approved, go ahead.", { deliverAs: "followUp", source: "rpc" }); + return fauxAssistantMessage([fauxToolCall("exit_plan_mode", {})]); + }, + record(requests), + ]); + await harness.session.prompt("/plan write the proposal"); + + // Their words are the input of the next request, not a request the model takes + // before seeing them, and they settle the wait. + expect(requests).toHaveLength(1); + expect(requests[0]?.some((message) => message.includes("Approved, go ahead."))).toBe(true); + expect(harness.session.hasPendingUserDecision).toBe(false); +}); + +test("a custom message held by the decision is not folded into the user's question", async () => { + // Context an extension queued is not something the person wrote. It waits for the + // turn that answers the decision, even when that turn is a question: the model + // must not read automatic work past a decision that is still open. + const { harness, planPath } = await createPlanHarness("step-plan-gate-custom-", { + mode: "rpc", + planReviewSurface: true, + }); + mkdirSync(dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Proposal\nDo it.\n"); + harness.setResponses([fauxAssistantMessage([fauxToolCall("exit_plan_mode", {})])]); + await harness.session.prompt("/plan write the proposal"); + expect(harness.session.hasPendingUserDecision).toBe(true); + + await harness.session.sendCustomMessage( + { customType: "test-wake", content: "AUTOWORK", display: true, details: {} }, + { deliverAs: "followUp", triggerTurn: true }, + ); + + const requests: string[][] = []; + harness.setResponses([record(requests)]); + await harness.session.prompt("why the second step?"); + expect(requests).toHaveLength(1); + expect(requests[0]?.some((message) => message.includes("AUTOWORK"))).toBe(false); +}); + +test("notes forwarded by a command still answer the decision they queued behind", async () => { + // `/plan ` is the user's own words, forwarded by a command. The session + // tracks who typed it, so the note may answer the wait it queued behind instead + // of being held as extension traffic — otherwise a user asking for changes would + // have them silently dropped. + const { harness, planPath } = await createPlanHarness("step-plan-gate-command-notes-", { + mode: "rpc", + planReviewSurface: true, + }); + mkdirSync(dirname(planPath), { recursive: true }); + writeFileSync(planPath, "# Proposal\nDo it.\n"); + const requests: string[][] = []; + harness.setResponses([ + async () => { + // Queued during the turn that submits, through the command path (`/plan`). + await harness.session.prompt("/plan split the second step"); + return fauxAssistantMessage([fauxToolCall("exit_plan_mode", {})]); + }, + record(requests), + record(requests), + ]); + await harness.session.prompt("/plan write the proposal"); + + expect(harness.session.hasPendingUserDecision).toBe(true); + expect(requests).toHaveLength(1); + expect(requests[0]?.at(-1)).toContain("split the second step"); +}); + test("a compaction does not leave a decision nobody can answer", async () => { const { harness, planPath } = await createPlanHarness("step-plan-gate-compact-"); await handoff(harness, planPath); diff --git a/packages/coding-agent/test/utilities.ts b/packages/coding-agent/test/utilities.ts index ad7aa7a92..850bcb48a 100644 --- a/packages/coding-agent/test/utilities.ts +++ b/packages/coding-agent/test/utilities.ts @@ -249,6 +249,11 @@ export function createTestResourceLoader(options: CreateTestResourceLoaderOption getAppendSystemPrompt: () => [], getAppendSystemPromptSources: () => [], extendResources: () => {}, + // A no-op that keeps returning the same result: `session.reload()` here + // invalidates the shared runtime and hands the next runner the same, now + // stale copy. A real loader builds a new result and runtime. Tests that need + // to observe behavior *after* a reload must build the session from services + // (`createAgentSessionServices`), not from this loader. reload: async () => {}, }; } From 5c00017fe1fcc44eed7e6610d3e4f5c4e4ff7694 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Tue, 3 Nov 2026 12:49:42 +0800 Subject: [PATCH 11/15] fix(coding-agent): report a newer answer over a pending proposal 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. --- .../coding-agent/src/features/plan-handoff.ts | 22 +++++++++++++++++++ .../coding-agent/test/plan-handoff.test.ts | 9 ++++++++ 2 files changed, 31 insertions(+) diff --git a/packages/coding-agent/src/features/plan-handoff.ts b/packages/coding-agent/src/features/plan-handoff.ts index 20b8d327b..3cb1c7f9c 100644 --- a/packages/coding-agent/src/features/plan-handoff.ts +++ b/packages/coding-agent/src/features/plan-handoff.ts @@ -65,6 +65,11 @@ export function readExternalPlanHandoff(result: unknown): ExternalPlanHandoff | * in a handoff" cannot answer this: a question typed past the proposal is not an * answer, and a newer submission recorded before its own result would be * reported with the older text. + * + * A newer answer still ends the search. On a host that draws the proposal, a + * question does not settle the wait, so the proposal stays pending — but the + * model has answered that question, and reporting the proposal instead would hide + * what the caller asked for. */ export function pendingHandoffText( state: { pendingProposalHash?: string } | undefined, @@ -73,6 +78,9 @@ export function pendingHandoffText( const pending = state?.pendingProposalHash; if (pending === undefined) return undefined; for (const message of [...messages].reverse()) { + // Any answer after the submission, and the proposal is no longer the last + // thing the model said. + if (message.role === "assistant" && hasText(message)) return undefined; if (message.role !== "toolResult" || message.toolName !== "exit_plan_mode" || message.isError) continue; const details = message.details as { planHash?: unknown } | undefined; if (details?.planHash !== pending) continue; @@ -83,6 +91,20 @@ export function pendingHandoffText( return undefined; } +function hasText(message: AgentMessage): boolean { + const content = (message as { content?: unknown }).content; + if (typeof content === "string") return content.trim().length > 0; + if (!Array.isArray(content)) return false; + return content.some( + (part) => + typeof part === "object" && + part !== null && + (part as { type?: unknown }).type === "text" && + typeof (part as { text?: unknown }).text === "string" && + (part as { text: string }).text.trim().length > 0, + ); +} + /** * The handoff a session written before the decision fact still leaves open. * diff --git a/packages/coding-agent/test/plan-handoff.test.ts b/packages/coding-agent/test/plan-handoff.test.ts index 983c504a1..8b2227dff 100644 --- a/packages/coding-agent/test/plan-handoff.test.ts +++ b/packages/coding-agent/test/plan-handoff.test.ts @@ -43,6 +43,15 @@ test("a question typed past the proposal does not hide it", () => { ); }); +test("a newer answer is not hidden by the proposal it followed", () => { + // A host that draws the proposal keeps it pending when its user asks a question, + // so the fact still says one is waiting — but the model answered, and reporting + // the proposal instead would hide the answer the caller asked for. + const answer = { role: "assistant" as const, content: [{ type: "text" as const, text: "Because of the parser." }] }; + const messages = [handoff, user, answer] as AgentMessage[]; + expect(pendingHandoffText({ pendingProposalHash: "a".repeat(64) }, messages)).toBeUndefined(); +}); + test("a newer submission before its own result is not reported as the old text", () => { // The fact names the snapshot; the transcript tail may still be the older // round, whose text the user is no longer looking at. From 8a2c34e6779389c0086de1b5850e873e5485e701 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Tue, 3 Nov 2026 12:49:45 +0800 Subject: [PATCH 12/15] feat(coding-agent): let an rpc client declare a plan review surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/cli/src/main.ts | 5 ++++- packages/coding-agent/docs/rpc.md | 17 +++++++++++++++++ packages/coding-agent/src/cli/args.ts | 10 ++++++++++ packages/coding-agent/src/main.ts | 5 ++++- packages/coding-agent/src/modes/rpc/rpc-mode.ts | 12 ++++++++++++ packages/coding-agent/test/args.test.ts | 15 +++++++++++++++ 6 files changed, 62 insertions(+), 2 deletions(-) diff --git a/apps/cli/src/main.ts b/apps/cli/src/main.ts index d8b1c975e..0aa378f89 100644 --- a/apps/cli/src/main.ts +++ b/apps/cli/src/main.ts @@ -885,7 +885,10 @@ function readCredentialUid(credential: unknown): string | undefined { async function dispatchStepAppMode(prep: Extract): Promise { switch (prep.appMode) { case "rpc": { - await runRpcMode(prep.runtimeHost, { includeLiveEvents: prep.parsed.includeLiveEvents }); + await runRpcMode(prep.runtimeHost, { + includeLiveEvents: prep.parsed.includeLiveEvents, + planReviewSurface: prep.parsed.planReviewSurface, + }); break; } case "interactive": { diff --git a/packages/coding-agent/docs/rpc.md b/packages/coding-agent/docs/rpc.md index 2f08fa83a..f15a3f9dc 100644 --- a/packages/coding-agent/docs/rpc.md +++ b/packages/coding-agent/docs/rpc.md @@ -797,6 +797,10 @@ entry), this returns the handoff text for that exact snapshot instead, because the assistant message only introduces the submission. `/plan approve`, a request for changes, or `/plan off` clears the proposal and the assistant text returns. +A newer answer wins over the proposal: on a host that draws the proposal a +question does not settle the wait, but the model has answered it, and reporting +the proposal instead would hide that answer. + ```json {"type": "get_last_assistant_text"} ``` @@ -873,6 +877,19 @@ Each command has: **Note**: Built-in TUI commands (`/settings`, `/hotkeys`, etc.) are not included. They are handled only in interactive mode and would not execute if sent via `prompt`. +## Plan Review Surface + +`--plan-review-surface` tells the session that this client draws the proposal and +collects the user's decision. With it, `exit_plan_mode` keeps plan mode on, ends +the turn, and records the proposal as waiting; the client answers with `/plan +approve` or `/plan approve `, or ends it with `/plan off`. Without it the +rpc host keeps the handoff behavior: plan mode exits, the turn ends, and approval +belongs to the caller. + +The flag is not implied by `--mode rpc`: every rpc client has a dialog bridge, +and only the client knows whether it can show a plan. `RpcModeOptions.planReviewSurface` +carries the same value when the host is embedded in-process. + ## Events Events are streamed to stdout as JSON lines during agent operation. Events do not generally include an `id` field; `bash_execution_update` includes the `id` of its originating `bash` command when one was provided. diff --git a/packages/coding-agent/src/cli/args.ts b/packages/coding-agent/src/cli/args.ts index d5cdf5d8e..d6e2ef6ed 100644 --- a/packages/coding-agent/src/cli/args.ts +++ b/packages/coding-agent/src/cli/args.ts @@ -60,6 +60,13 @@ export interface Args { * would keep every one of them. */ includeLiveEvents?: boolean; + /** + * The rpc client draws plan proposals and collects the user's decision. + * + * Not implied by `--mode rpc`: a client with no review surface must keep the + * handoff behavior, so this is stated, never guessed. + */ + planReviewSurface?: boolean; export?: string; noSkills?: boolean; skills?: string[]; @@ -414,6 +421,8 @@ export function parseArgs(args: string[]): Args { } } else if (arg === "--include-live-events") { result.includeLiveEvents = true; + } else if (arg === "--plan-review-surface") { + result.planReviewSurface = true; } else if (arg === "--no-tools" || arg === "-nt") { result.noTools = true; } else if (arg === "--no-builtin-tools" || arg === "-nbt") { @@ -678,6 +687,7 @@ ${chalk.bold("Options:")} --append-system-prompt Append text or file contents to the system prompt (can be used multiple times) --mode Output mode: text (default), json, or rpc --include-live-events Live consumers/debugging only; makes JSON/RPC logs much larger + --plan-review-surface RPC client draws plan proposals and collects the decision ${stepPermissionOptionsText} --sdk-stdio Run the Step Agent SDK length-prefixed stdio host --print, -p Non-interactive mode: process prompt and exit diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index 6a74b8327..864475e17 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -1469,7 +1469,10 @@ async function dispatchAppMode( ): Promise { const { appMode, runtimeHost } = prep; if (appMode === "rpc") { - await runRpcMode(runtimeHost, { includeLiveEvents: prep.parsed.includeLiveEvents }); + await runRpcMode(runtimeHost, { + includeLiveEvents: prep.parsed.includeLiveEvents, + planReviewSurface: prep.parsed.planReviewSurface, + }); } else if (appMode === "interactive") { // The interactive TUI moved to @step-harness/cli (S4-0). coding-agent's own // main() no longer dispatches it; product shells construct InteractiveMode diff --git a/packages/coding-agent/src/modes/rpc/rpc-mode.ts b/packages/coding-agent/src/modes/rpc/rpc-mode.ts index 78b705d73..eb95251f2 100644 --- a/packages/coding-agent/src/modes/rpc/rpc-mode.ts +++ b/packages/coding-agent/src/modes/rpc/rpc-mode.ts @@ -63,6 +63,15 @@ export type { */ export interface RpcModeOptions { includeLiveEvents?: boolean; + /** + * This client can draw a plan proposal and collect the user's decision. + * + * `mode: "rpc"` alone cannot say so: every rpc host has a dialog bridge. A + * client that renders the proposal itself sets this and plan mode stays on + * until the user answers; without it the handoff-and-exit behavior is + * unchanged. Read from `--plan-review-surface`. + */ + planReviewSurface?: boolean; } export async function runRpcMode(runtimeHost: AgentSessionRuntimeHost, options: RpcModeOptions = {}): Promise { @@ -354,6 +363,9 @@ export async function runRpcMode(runtimeHost: AgentSessionRuntimeHost, options: await session.bindExtensions({ uiContext: createExtensionUIContext(), mode: "rpc", + // The client, not the transport, answers a plan review: declare only what + // it was told at startup. + planReviewSurface: options.planReviewSurface === true, commandContextActions: { waitForIdle: () => session.waitForIdle(), newSession: async (options) => runtimeHost.newSession(options), diff --git a/packages/coding-agent/test/args.test.ts b/packages/coding-agent/test/args.test.ts index 671ed23e6..c0087e1bd 100644 --- a/packages/coding-agent/test/args.test.ts +++ b/packages/coding-agent/test/args.test.ts @@ -11,6 +11,21 @@ test("help exposes the live-consumer opt-in and its log cost", () => { } }); +test("--plan-review-surface is stated, never inferred from --mode rpc", () => { + // An rpc client that cannot draw a proposal must keep the handoff behavior, so + // nothing about the transport sets this. + expect(parseArgs(["--mode", "rpc"]).planReviewSurface).toBeUndefined(); + expect(parseArgs(["--mode", "rpc", "--plan-review-surface"]).planReviewSurface).toBe(true); + + const log = vi.spyOn(console, "log").mockImplementation(() => {}); + try { + printHelp(); + expect(log.mock.calls.flat().join("\n")).toContain("--plan-review-surface"); + } finally { + log.mockRestore(); + } +}); + describe("parseArgs", () => { describe("--version flag", () => { test("parses --version flag", () => { From 4d2cb210659ff9e49f82dd6a5bef2b6eba7fe02d Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Tue, 3 Nov 2026 12:49:50 +0800 Subject: [PATCH 13/15] refactor(coding-agent): name the plan block version for what it versions 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. --- packages/coding-agent/src/features/plan-mode-prompt.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/src/features/plan-mode-prompt.ts b/packages/coding-agent/src/features/plan-mode-prompt.ts index 183ec2c51..86b11cb5f 100644 --- a/packages/coding-agent/src/features/plan-mode-prompt.ts +++ b/packages/coding-agent/src/features/plan-mode-prompt.ts @@ -19,8 +19,11 @@ export const PLAN_MODE_MESSAGE_TYPE = "step-plan-mode"; * older block keeps the rules it was written with. Bumping this makes the next * request append the current block; without it, a rule only reaches sessions * that entered plan mode after the upgrade. + * + * Not the same number as the `step-plan` entry's `version`: that one describes the + * persisted state format, this one the wording of a prompt block. */ -export const PLAN_MODE_PROMPT_VERSION = 2; +export const PLAN_MODE_BLOCK_VERSION = 2; /** Block announcing that plan mode is active, with the session plan file. */ export function buildPlanModePrompt(planFilePath: string): string { From 6845248e689d0961b2bed69f6083ee4c0497789c Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:05:38 +0800 Subject: [PATCH 14/15] fix(coding-agent): survive a host without a terminal theme 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. --- .../coding-agent/src/core/extensions/types.ts | 7 +++ .../coding-agent/src/features/step-plan.ts | 17 +++++- .../test/step-plan-extension.test.ts | 55 +++++++++++++++++-- 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/packages/coding-agent/src/core/extensions/types.ts b/packages/coding-agent/src/core/extensions/types.ts index 510641a16..66a774279 100644 --- a/packages/coding-agent/src/core/extensions/types.ts +++ b/packages/coding-agent/src/core/extensions/types.ts @@ -154,6 +154,13 @@ export interface ExtensionNotifyOptions { * entered. Modes without a transcript ignore it. */ echoesInput?: boolean; + /** + * The message only restates state the host already renders from the session + * (plan mode on/off, the permission tier). A host with a transcript shows it + * briefly and keeps it out of the conversation: it is an announcement, not an + * answer to something the user typed. + */ + transient?: boolean; } /** Raw terminal input listener for extensions. */ diff --git a/packages/coding-agent/src/features/step-plan.ts b/packages/coding-agent/src/features/step-plan.ts index 39b4126a9..106379159 100644 --- a/packages/coding-agent/src/features/step-plan.ts +++ b/packages/coding-agent/src/features/step-plan.ts @@ -349,7 +349,11 @@ function updateStatus( if (enabled && planFilePath) publishedPlanPaths.set(ctx.sessionManager, planFilePath); else publishedPlanPaths.delete(ctx.sessionManager); if (enabled) { - ctx.ui.setStatus("plan-mode", ctx.ui.theme.fg("warning", `Planning (${planSource ?? "unknown"})`)); + const label = `Planning (${planSource ?? "unknown"})`; + // Styling is a presentation nicety: a host without a terminal theme must + // still get the chip, not a crash inside its status update. + const theme = ctx.ui.theme; + ctx.ui.setStatus("plan-mode", theme ? theme.fg("warning", label) : label); } else { ctx.ui.setStatus("plan-mode", undefined); } @@ -480,9 +484,12 @@ export const createStepPlanExtension = enable(); recordPlanEnter(options.telemetry, planSource); updateStatus(ctx, enabled, planSource, planFilePath); + // Presentation state, not a transcript answer: a host that draws notices as + // rows would otherwise add one every time the mode is turned on. ctx.ui.notify( `Plan mode enabled. Draft your proposal at ${planFilePath}; file-editing tools are limited to this path. Commands still use normal permissions.`, "info", + { transient: true }, ); }; /** @@ -644,6 +651,7 @@ export const createStepPlanExtension = ctx.ui.notify( "That is not a proposal hash. Use `/plan approve`, `/plan approve `, or `/plan ` to plan something else.", "warning", + { transient: true }, ); return; } @@ -656,11 +664,13 @@ export const createStepPlanExtension = // arrives after a newer submission must not approve that one — the // file being current says nothing about which proposal was on screen. if (pendingProposal === undefined) { - ctx.ui.notify("No plan is waiting for approval.", "warning"); + ctx.ui.notify("No plan is waiting for approval.", "warning", { transient: true }); return; } if (namedProposal !== undefined && namedProposal !== pendingProposal) { - ctx.ui.notify("That proposal is no longer waiting for approval.", "warning"); + ctx.ui.notify("That proposal is no longer waiting for approval.", "warning", { + transient: true, + }); return; } // The approval belongs to the snapshot the user read. A file edited @@ -679,6 +689,7 @@ export const createStepPlanExtension = ctx.ui.notify( "The plan changed since it was submitted; ask for a new review, or use `/plan off` to leave plan mode.", "warning", + { transient: true }, ); return; } diff --git a/packages/coding-agent/test/step-plan-extension.test.ts b/packages/coding-agent/test/step-plan-extension.test.ts index f94bba9d1..2cb5b72d1 100644 --- a/packages/coding-agent/test/step-plan-extension.test.ts +++ b/packages/coding-agent/test/step-plan-extension.test.ts @@ -33,7 +33,7 @@ interface Harness { commands: Map unknown }>; handlerEvents: () => string[]; appended: Array<{ customType: string; data: unknown }>; - notifications: Array<{ message: string; type?: string }>; + notifications: Array<{ message: string; type?: string; options?: { transient?: boolean } }>; userMessages: Array>; /** Hidden custom messages, so a test can assert an instruction is not in the user's bubble. */ customMessages: Array>; @@ -107,7 +107,7 @@ function createHarness( const commands = new Map unknown }>(); const handlers = new Map unknown>>(); const appended: Array<{ customType: string; data: unknown }> = []; - const notifications: Array<{ message: string; type?: string }> = []; + const notifications: Array<{ message: string; type?: string; options?: { transient?: boolean } }> = []; const userMessages: Array> = []; const customMessages: Array> = []; const statuses = new Map(); @@ -165,8 +165,8 @@ function createHarness( // the end-to-end behavior is covered by test/suite/plan-approval-gate.test.ts. registerAutomaticTurnGuard: () => () => {}, ui: { - notify: (message: string, type?: string) => { - notifications.push({ message, type }); + notify: (message: string, type?: string, options?: { transient?: boolean }) => { + notifications.push({ message, type, options }); }, select: options.select ?? (async () => undefined), editor: options.editor ?? (async () => undefined), @@ -2525,6 +2525,53 @@ test("leaving plan mode while idle does not abort anything", async () => { expect(abort).not.toHaveBeenCalled(); }); +test("the status chip does not require a theme from the host", async () => { + // A host that renders text rather than a terminal may answer `undefined`; styling + // is a nicety, and a crash inside the status update loses the whole command. + const harness = createHarness({ cwd: makeWorkspace() }); + createStepPlanExtension()(harness.api); + const context = harness.ctx({ ui: { ...(harness.ctx().ui as object), theme: undefined } }); + + await expect(harness.commands.get("plan")!.handler("", context)).resolves.toBeUndefined(); + expect(harness.statuses.get("plan-mode")).toBe("Planning (user)"); +}); + +test("entering plan mode announces it without adding a transcript row", async () => { + // The host draws plan mode from session state; a durable notice would be a row + // nobody typed into the conversation. Leaving says nothing at all: the chip + // closing and the composer reopening are the report. + const harness = createHarness({ cwd: makeWorkspace() }); + createStepPlanExtension()(harness.api); + const context = harness.ctx(); + await harness.commands.get("plan")!.handler("", context); + await harness.commands.get("plan")!.handler("", context); + + const notices = harness.notifications.filter((notice) => /Plan mode (enabled|disabled)/.test(notice.message)); + expect(notices).toHaveLength(1); + expect(notices[0]?.options?.transient).toBe(true); +}); + +test("plan mode denies a mutation even when the branch carries no plan entry", async () => { + // The guard reads the branch so it can run before the plan extension has + // replayed `session_start`. That must not turn a failed persist — or an + // embedder whose `appendEntry` stores nothing — into an open door. + const cwd = makeWorkspace(); + const harness = createHarness({ cwd }); + createStepPlanExtension()(harness.api); + const context = harness.ctx(); + // Nothing reaches the branch: plan mode exists only in the live session. + harness.api.appendEntry = () => {}; + await harness.commands.get("plan")!.handler("add a readme", context); + expect(harness.appended.filter((entry) => entry.customType === "step-plan")).toHaveLength(0); + + await expect( + harness.emit( + { type: "tool_call", toolName: "write_file", toolCallId: "lost-entry", input: { path: "src/app.ts" } }, + context, + ), + ).resolves.toMatchObject({ block: true, reason: expect.stringContaining("Only the plan file may be written") }); +}); + test("a failed enter persist leaves plan mode off", async () => { const harness = createHarness({ cwd: makeWorkspace() }); createStepPlanExtension()(harness.api); From 089a1ce63afa50441b8a5c1c24db23eb0b5e8a76 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Tue, 3 Nov 2026 15:05:48 +0800 Subject: [PATCH 15/15] test(coding-agent): bound the idle-TTL wait by real time 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. --- .../test/step-subagent-events.test.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/coding-agent/test/step-subagent-events.test.ts b/packages/coding-agent/test/step-subagent-events.test.ts index 30fc2423d..5969fe0a0 100644 --- a/packages/coding-agent/test/step-subagent-events.test.ts +++ b/packages/coding-agent/test/step-subagent-events.test.ts @@ -1362,12 +1362,21 @@ test("idle keep-alive children are stopped after the TTL and a reply resumes qui * file I/O that a loaded CI runner loses. Advance in steps for a bounded * virtual budget instead, and report what did arrive. */ + const seen = (count: number): boolean => + sent.filter((m) => m.details?.event === "background_done").length >= count; const settle = async (count: number): Promise => { - for (let i = 0; i < 500; i += 1) { - if (sent.filter((m) => m.details?.event === "background_done").length >= count) return true; + // Bounded by real time, not by a step count. The notification waits on the + // child's exit and its details, which is real I/O: a fixed number of + // virtual-time steps is spent in a few milliseconds and loses that race on a + // loaded runner. Each step also yields a real macrotask, because fake timers + // advance virtual time only. + const deadline = Date.now() + 3_000; + while (Date.now() < deadline) { + if (seen(count)) return true; await vi.advanceTimersByTimeAsync(10); + await new Promise((resolve) => setImmediate(resolve)); } - return false; + return seen(count); }; const notifications = () => sent.map((m) => m.details?.event ?? "?").join(", ") || "none"; expect(await settle(1), `notifications after the first lane: ${notifications()}`).toBe(true);