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/apps/cli/src/ui/interactive-mode.ts b/apps/cli/src/ui/interactive-mode.ts index 180975677..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,6 +2464,9 @@ export class InteractiveMode { ui: this.createExtensionUIContext(), mode: "tui", hasUI: true, + // 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, modelRegistry: extensionRunner.getModelRegistry(), diff --git a/apps/cli/test/plan-review-tool-row.test.ts b/apps/cli/test/plan-review-tool-row.test.ts index 1016821fe..554f04862 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: () => {}, + 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 35d72277a..f504ea498 100644 --- a/docs/plan-approval.md +++ b/docs/plan-approval.md @@ -9,10 +9,21 @@ 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. + +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. 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,13 +95,15 @@ 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 -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. @@ -112,7 +125,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 +159,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` / `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. @@ -177,29 +192,76 @@ 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`: a handoff that no user message has answered yet. +`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 +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 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. 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. +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 — 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 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. 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/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 556385f04..40df5a802 100644 --- a/packages/agent-core/src/agent.ts +++ b/packages/agent-core/src/agent.ts @@ -144,19 +144,45 @@ class PendingMessageQueue { return this.messages.length > 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(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(); + 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 `allowed`. */ + hasDeliverable(allowed?: (message: AgentMessage) => boolean): boolean { + if (!allowed) return this.messages.length > 0; + return this.messages.some((message: AgentMessage) => allowed(message)); } clear(): void { @@ -324,6 +350,29 @@ export class Agent { return this.steeringQueue.hasItems() || this.followUpQueue.hasItems(); } + /** + * 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. + * + * 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. + */ + 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. */ get signal(): AbortSignal | undefined { return this.activeRun?.abortController.signal; @@ -371,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."); } @@ -383,13 +440,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; @@ -398,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(); } @@ -495,9 +569,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/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 8def7286a..aa6235583 100644 --- a/packages/agent-core/test/agent.test.ts +++ b/packages/agent-core/test/agent.test.ts @@ -705,6 +705,181 @@ 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 + // 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); + }); + + 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/docs/extensions.md b/packages/coding-agent/docs/extensions.md index d13846b13..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) => { @@ -974,6 +978,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 })`), or across the wire with `--plan-review-surface`; an extension sees the value on every handler's `ctx`. + ### ctx.cwd Current working directory. @@ -3007,12 +3017,12 @@ 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 | -| Print (`-p`) | `"print"` | `false` | Extensions run but can't prompt | +| 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 `--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 | 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..f15a3f9dc 100644 --- a/packages/coding-agent/docs/rpc.md +++ b/packages/coding-agent/docs/rpc.md @@ -791,10 +791,15 @@ 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. + +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"} @@ -872,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/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/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/core/agent-session.ts b/packages/coding-agent/src/core/agent-session.ts index daaee0a30..b00abc9e6 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; @@ -357,6 +366,15 @@ export class AgentSession { private _steeringMessages: string[] = []; /** Tracks pending follow-up messages for UI display. Removed when delivered. */ private _followUpMessages: string[] = []; + /** + * 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 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. */ @@ -419,6 +437,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. */ @@ -452,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; @@ -471,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 ?? []; @@ -1486,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") { @@ -1595,6 +1619,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(); @@ -1983,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; @@ -2007,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({ @@ -2034,7 +2062,18 @@ 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.agent.hasDeliverableQueuedMessage(this._queueDeliveryAllowed)) { + // 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 // needs no extra model call; the next prompt/continuation performs the @@ -2047,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.hasQueuedMessages(); + return this.agent.hasDeliverableQueuedMessage(this._queueDeliveryAllowed) ? "deliver" : "stop"; } /** @@ -2134,9 +2173,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; @@ -2146,7 +2185,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; } @@ -2197,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) { @@ -2277,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) { @@ -2289,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 @@ -2384,35 +2433,44 @@ export class AgentSession { /** * Internal: Queue a steering message (already expanded, no extension command check). */ - private async _queueSteer(text: string, images?: ImageContent[]): Promise { + /** + * Whether this queued message may be delivered now. + * + * 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 _queueDeliveryAllowed = (message: AgentMessage): boolean => + !this.hasPendingUserDecision || this._queuedFromUser.has(message); + + private async _queueSteer(text: string, images?: ImageContent[], source?: InputSource): Promise { this._steeringMessages.push(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); } /** * 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); 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); } /** @@ -2500,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) { @@ -2556,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", }); } @@ -3248,7 +3313,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; @@ -3439,8 +3506,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; @@ -3501,6 +3569,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 +3753,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..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. */ @@ -395,6 +402,15 @@ 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. + * A host that cannot ask leaves this false, and the tool hands the proposal + * to its caller as before. + */ + planReviewSurface: boolean; /** The current abort signal, or undefined when the agent is not streaming. */ signal: AbortSignal | undefined; /** Abort the current agent operation */ @@ -875,6 +891,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 */ @@ -1861,6 +1887,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..3cb1c7f9c 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"; @@ -14,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; } /** @@ -32,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; @@ -43,51 +50,95 @@ export function readExternalPlanHandoff(result: unknown): ExternalPlanHandoff | .map((part) => part.text) .join("\n") .trim(); - return { text }; + return { text, ...hash }; } /** - * The handoff this session has not been answered for yet. + * 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. 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. * - * 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 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. * - * The scan is a reverse walk that stops at the first user message, so it reads - * only the tail of the conversation. + * 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 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; +export function pendingHandoffText( + state: { pendingProposalHash?: string } | undefined, + messages: readonly AgentMessage[], +): string | undefined { + 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 handoff = readExternalPlanHandoff(message); - if (handoff) return handoff; + const details = message.details as { planHash?: unknown } | undefined; + if (details?.planHash !== pending) continue; + const text = readExternalPlanHandoff(message)?.text; + // An empty result would otherwise win over the answer the caller did get. + if (text) return text; } 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, + ); +} + /** - * {@link findPendingExternalPlanApproval} for a persisted session branch. + * 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. * - * 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. + * 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 hasPendingExternalPlanApproval(manager: { - getBranch(): readonly { type: string; message?: AgentMessage }[]; -}): boolean { +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 false; + // 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; if (entry.type !== "message" || !entry.message) continue; const message = entry.message; - if (message.role === "user") return false; + // 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; - if (readExternalPlanHandoff(message)) return true; + const handoff = readExternalPlanHandoff(message); + if (handoff) return handoff; } - return false; + 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 3236489b7..86b11cb5f 100644 --- a/packages/coding-agent/src/features/plan-mode-prompt.ts +++ b/packages/coding-agent/src/features/plan-mode-prompt.ts @@ -12,6 +12,19 @@ /** 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. + * + * 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_BLOCK_VERSION = 2; + /** Block announcing that plan mode is active, with the session plan file. */ export function buildPlanModePrompt(planFilePath: string): string { return ` @@ -20,8 +33,9 @@ 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 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. 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/plan-mode-tools.ts b/packages/coding-agent/src/features/plan-mode-tools.ts index bf26d8751..3d303200a 100644 --- a/packages/coding-agent/src/features/plan-mode-tools.ts +++ b/packages/coding-agent/src/features/plan-mode-tools.ts @@ -34,6 +34,37 @@ 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; + /** + * 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; +} + +/** + * 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 +165,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 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. @@ -162,21 +193,44 @@ 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, 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 }), }); + // 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.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", @@ -188,7 +242,7 @@ export function registerPlanModeTools(pi: ExtensionAPI, planMode: StepPlanModeCo ); } if (!ctx.hasUI) { - 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"), @@ -205,8 +259,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..106379159 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,17 @@ 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 { 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_BLOCK_VERSION, + PLAN_MODE_MESSAGE_TYPE, +} from "./plan-mode-prompt.ts"; import { getPlanFilePath, + hashPlanContents, registerPlanModeTools, type StepPlanExitOutcome, type StepPlanModeController, @@ -54,8 +60,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 @@ -65,6 +75,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; } @@ -73,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 { @@ -91,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; @@ -114,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_BLOCK_VERSION; + if (visible?.mode === "plan" && visible.planFilePath === planFilePath && current) return undefined; + const details: PlanModeAnnouncement = { + mode: "plan", + planFilePath, + version: PLAN_MODE_BLOCK_VERSION, + }; return { customType: PLAN_MODE_MESSAGE_TYPE, content: buildPlanModePrompt(planFilePath), @@ -160,6 +187,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,11 +212,33 @@ 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; } +/** + * 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; @@ -298,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); } @@ -336,6 +391,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; @@ -351,10 +415,11 @@ 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, + ...(pendingProposal ? { pendingProposalHash: pendingProposal } : {}), ...(carriesLegacyTools ? { toolsBeforePlanMode: carriesLegacyTools } : {}), }); }; @@ -397,6 +462,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); @@ -408,31 +478,122 @@ export const createStepPlanExtension = enabled = false; planSource = undefined; planFilePath = previousPath; + pendingProposal = previousPending; throw error; } 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 }, ); }; + /** + * 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; - // 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); + const previousPending = pendingProposal; + 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` 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(); + }; + /** + * 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); + /** + * 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); + }; const planModeController: StepPlanModeController = { isPlanModeActive: () => enabled, exitPlanMode: exit, resolvePlanFilePath, + recordPendingProposal, + handOffProposal, + clearPendingProposal, }; pi.registerFlag("plan", { @@ -441,25 +602,143 @@ 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 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) { - 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. + // + // 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 (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. + if (enabled) leaveByUser("toggled_off", ctx); + else clearPendingProposal(); + return; + } + // `/plan approve` answers the proposal, `/plan approve ` answers + // 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", + { transient: true }, + ); + 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. + // + // 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", { transient: true }); + return; + } + if (namedProposal !== undefined && namedProposal !== pendingProposal) { + 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 + // 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) { + // Nothing was decided: the wait stays, so automatic work keeps + // 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", + { transient: true }, + ); + return; + } + // 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. + 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, - // 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. + // `/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, 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); - pi.sendUserMessage(prompt, ctx.isIdle() ? {} : { deliverAs: "followUp" }); + 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). + // + // 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. The + // session already knows who typed the command, so this forwards it rather + // than guessing from the transport. + pi.sendUserMessage(prompt, { + expandPromptTemplates: true, + ...(ctx.isIdle() ? {} : { deliverAs: "followUp" as const }), + }); }, }); registerPlanModeTools(pi, planModeController); @@ -482,6 +761,12 @@ 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) => { + // 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"), ); @@ -493,6 +778,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 @@ -544,6 +847,33 @@ 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. + 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. + // + // 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 + // 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]); if (!sameToolOrder(pi.getActiveTools(), baseline)) pi.setActiveTools(baseline); @@ -570,10 +900,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/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 a29362721..eb95251f2 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 { 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"; @@ -62,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 { @@ -353,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), @@ -664,11 +677,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(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 6f54c15d4..b8b6aa5b0 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 { 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"; @@ -664,9 +665,10 @@ 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(pendingProposalOf(this.#session), 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/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", () => { 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..8b2227dff --- /dev/null +++ b/packages/coding-agent/test/plan-handoff.test.ts @@ -0,0 +1,65 @@ +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 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)).toBe( + "Review this plan.\n\n# Plan\n1. Do 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. + 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/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 f91e02cc7..2cb5b72d1 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_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"; @@ -32,8 +33,10 @@ 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>; statuses: Map; customCalls: () => number; reviewComponent: () => (Component & { handleInput?(data: string): void }) | undefined; @@ -104,8 +107,9 @@ 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(); const cwd = options.cwd ?? "/workspace"; let customCalls = 0; @@ -138,7 +142,9 @@ function createHarness( appended.push({ customType, data }); session.appendCustomEntry(customType, data); }, - sendMessage: () => {}, + sendMessage: (...args: Parameters) => { + customMessages.push(args); + }, sendUserMessage: (...args: Parameters) => { userMessages.push(args); }, @@ -149,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, @@ -157,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), @@ -205,6 +213,7 @@ function createHarness( appended, notifications, userMessages, + customMessages, statuses, customCalls: () => customCalls, reviewComponent: () => lastReviewComponent, @@ -528,14 +537,75 @@ 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", + // 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"), + 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 +637,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 +652,600 @@ 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 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).toEqual([]); +}); + +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, + ); + // 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 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. + 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 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); + 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; + + await harness.commands.get("plan")!.handler("approve the payment flow", context); + // 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. + 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 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); + 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, + }), + ); + + // 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" }, + ]); + 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(); +}); + +/** + * 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 }, + }); + 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("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 + // 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. + 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] }); @@ -683,7 +1347,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, @@ -741,7 +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", {}]); + 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 } }); @@ -970,7 +1634,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); @@ -1023,7 +1687,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. @@ -1142,7 +1806,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", }); @@ -1606,7 +2270,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 +2284,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 +2499,79 @@ 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("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); @@ -1859,11 +2601,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 +2623,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 +2649,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 () => { @@ -1933,10 +2676,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) { @@ -1993,7 +2739,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"); }); @@ -2085,9 +2831,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_BLOCK_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 } }, @@ -2099,6 +2846,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/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/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); 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/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) => { @@ -149,40 +158,249 @@ 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); +}); + +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 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 + .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 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. + 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, 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 = 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 () => { + // 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([fauxToolCall("exit_plan_mode", {})]); + }, + 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 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 () => { + // 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("/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); +}); - // 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]); +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, 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 () => { + await harness.session.sendCustomMessage( + { customType: "test-wake", content: "AUTOWORK", display: true, details: {} }, + { deliverAs: "followUp", triggerTurn: true }, + ); + return fauxAssistantMessage([fauxToolCall("exit_plan_mode", {})]); + }, + record(requests), + ]); + 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); - harness.sessionManager.appendMessage({ - role: "user", - content: [{ type: "text", text: "Approved." }], - timestamp: Date.now(), - } as unknown as Parameters[0]); + // 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("a compaction ends the wait instead of leaving a phantom decision", async () => { +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); 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); }); 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"], 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 () => {}, }; }