diff --git a/src/permission/reactor-authorize.ts b/src/permission/reactor-authorize.ts index 3f44afdd8..496f64396 100644 --- a/src/permission/reactor-authorize.ts +++ b/src/permission/reactor-authorize.ts @@ -31,6 +31,7 @@ import { type WorkerGrantStore, } from "./worker-grant.js"; import { canonicalToolName } from "../agent/canonical-tool-name.js"; +import { prepareDispatchedToolCall } from "../agent/tool-aliases.js"; import { getSubAgentIdentity } from "../subagent/identity-context.js"; import { FLEET_VERBS, @@ -100,6 +101,23 @@ function resolveWorkerCwd(cwd: string | undefined): string { return cwd ?? getSubAgentIdentity()?.cwd ?? process.cwd(); } +/** + * The call as DynamicToolRunner dispatches it: canonical engine name plus + * hidden-alias argument coercion (gpt `shell` -> run_shell via + * coerceShellArgs). Authorize sees the raw wire call; the middleware that + * consumes an authorized retry sees the dispatched call. Keying both on the + * dispatched shape keeps one fingerprint per executed call. Invalid hidden + * arguments never dispatch, so they keep their raw shape. + */ +function dispatchedCall(call: ToolCallType): ToolCallType { + const engine = canonicalToolName(call.name); + try { + return prepareDispatchedToolCall(call, engine); + } catch { + return { ...call, name: engine }; + } +} + function workerCallIdentity( sessionId: string, call: ToolCallType, @@ -110,10 +128,11 @@ function workerCallIdentity( args: Record; cwd: string; } { + const dispatched = dispatchedCall(call); return { sessionId, - canonicalTool: canonicalToolName(call.name), - args: (call.arguments ?? {}) as Record, + canonicalTool: dispatched.name, + args: (dispatched.arguments ?? {}) as Record, cwd, }; } diff --git a/src/permission/worker-grant-dispatch-identity.test.ts b/src/permission/worker-grant-dispatch-identity.test.ts new file mode 100644 index 000000000..a419ae7fd --- /dev/null +++ b/src/permission/worker-grant-dispatch-identity.test.ts @@ -0,0 +1,147 @@ +// gpt-family workers call run_shell as `shell`. Reactor authz sees that raw +// wire call; the permission middleware sees DynamicToolRunner's +// prepareDispatchedToolCall output, whose coerceShellArgs rebuilds the +// arguments. The authorized retry must match the same envelope at both +// stages. +import { describe, expect, test } from "bun:test"; +import type { ToolCall } from "@intx/types/runtime"; + +import { prepareDispatchedToolCall } from "../agent/tool-aliases.js"; +import { gateToolCall } from "../plugins/permission-plugin.js"; +import { createPermissionGate } from "./gate.js"; +import { workerPermissionGate } from "./reactor-authorize.js"; +import { WorkerGrantStore } from "./worker-grant.js"; + +const CWD = process.cwd(); + +// The call shape a gpt-family worker sends for run_shell. +const wireCall = (id: string): ToolCall => ({ + id, + name: "shell", + arguments: { + command: "git status --short", + timeout: 120000, + cwd: CWD, + background: false, + stop: "", + }, +}); + +function extractRequestId(reason: string): string { + const match = /grant request ([0-9a-f-]{36})/.exec(reason); + if (match?.[1] === undefined) throw new Error(`no grant id: ${reason}`); + return match[1]; +} + +describe("gpt-family worker authorized retry identity", () => { + test("approved retry of a `shell` call executes once instead of minting a new grant request", async () => { + // Parent: reactor-gated interactive gate, nothing pre-approved, no auto. + const parentGate = createPermissionGate({ + approvals: [], + cwd: CWD, + skipPermissions: false, + interactive: true, + reactorGated: true, + auto: false, + requestApproval: async () => ({ allow: false }), + }); + const store = new WorkerGrantStore(); + const workerGate = workerPermissionGate(parentGate, { + sessionId: "worker-gpt", + store, + }); + + // 1. First attempt: reactor authz denies and records envelope A. + const denied = await workerGate.authorizeCall(wireCall("call-1")); + if (denied.effect !== "deny") throw new Error("expected worker deny"); + const requestId = extractRequestId(denied.reason); + const envelope = store.peek(requestId); + expect(envelope?.canonicalTool).toBe("run_shell"); + + // 2. approve_worker_call: parent approved the frozen call (allow-once, + // no session grant) and flipped the envelope to authorized. + expect(store.authorizeRetry(requestId)).toBe(true); + + // 3. Retry, stage 1: reactor authz on the raw wire call allows via + // hasAuthorizedRetry (does not consume). + const retryAuthz = await workerGate.authorizeCall(wireCall("call-2")); + expect(retryAuthz).toEqual({ effect: "allow" }); + expect(store.peek(requestId)?.status).toBe("authorized"); + + // 4. Retry, stage 2: DynamicToolRunner coerces the `shell` call onto + // run_shell, then the posix middleware runs gateToolCall. + const dispatched = prepareDispatchedToolCall( + wireCall("call-2"), + "run_shell", + ); + expect(dispatched.arguments).not.toEqual(wireCall("x").arguments); + + let executed = 0; + const result = await gateToolCall( + workerGate, + dispatched, + new AbortController().signal, + async (call) => { + executed++; + return { callId: call.id, content: "ran" }; + }, + ); + + expect(result.isError).not.toBe(true); + expect(executed).toBe(1); + expect(store.peek(requestId)?.status).toBe("consumed"); + + // One grant, one execution: a replay of the same call fails closed at + // both stages, and no second envelope was minted. + const replay = await gateToolCall( + workerGate, + prepareDispatchedToolCall(wireCall("call-3"), "run_shell"), + new AbortController().signal, + async (call) => { + executed++; + return { callId: call.id, content: "ran" }; + }, + ); + expect(replay.isError).toBe(true); + expect(String(replay.content)).toContain("already consumed"); + expect((await workerGate.authorizeCall(wireCall("call-4"))).effect).toBe( + "deny", + ); + expect(executed).toBe(1); + }); + + test("an altered retry (different command) still fails closed", async () => { + const parentGate = createPermissionGate({ + approvals: [], + cwd: CWD, + skipPermissions: false, + interactive: true, + reactorGated: true, + auto: false, + requestApproval: async () => ({ allow: false }), + }); + const store = new WorkerGrantStore(); + const workerGate = workerPermissionGate(parentGate, { + sessionId: "worker-gpt-alt", + store, + }); + const denied = await workerGate.authorizeCall(wireCall("call-1")); + if (denied.effect !== "deny") throw new Error("expected worker deny"); + const requestId = extractRequestId(denied.reason); + expect(store.authorizeRetry(requestId)).toBe(true); + + const altered: ToolCall = { + ...wireCall("call-2"), + arguments: { ...wireCall("x").arguments, command: "git push --force" }, + }; + expect((await workerGate.authorizeCall(altered)).effect).toBe("deny"); + const result = await gateToolCall( + workerGate, + prepareDispatchedToolCall(altered, "run_shell"), + new AbortController().signal, + async (call) => ({ callId: call.id, content: "ran" }), + ); + expect(result.isError).toBe(true); + expect(store.peek(requestId)?.status).toBe("authorized"); + }); +}); diff --git a/src/session/approval-resume.test.ts b/src/session/approval-resume.test.ts index 1c5bce3da..e9c868da1 100644 --- a/src/session/approval-resume.test.ts +++ b/src/session/approval-resume.test.ts @@ -12,6 +12,7 @@ import type { InboundMessage, } from "@intx/types/runtime"; +import { APPROVAL_TIMEOUT_RESULT_TEXT } from "../permission/decline-markers.js"; import type { PermissionGate } from "../permission/gate.js"; import { createExtraDeniedPathMatcher } from "../plugins/secret-guard-plugin.js"; import { @@ -23,6 +24,7 @@ import { resolveSuspendedApprovalFromStore, } from "./approval-resume.js"; import { createSessionOperationQueue } from "../tui/delivery-queue.js"; +import { createParkedOverlayAbortBinding } from "../tui/runner/parked-overlay-abort.js"; import { defined } from "../../testkit/defined.js"; import { redactedCorrelationId } from "./redacted-correlation.js"; import { @@ -936,6 +938,78 @@ describe("approval identity ordering", () => { expect(decisionBody(message).outcome).toBe("rejected"); }); + test("generation change during catch history drops the parked call once", async () => { + const ctx = deferredResume(); + const failure = new Error("gate exploded"); + ctx.resolveSuspended.mockImplementation(async () => { + throw failure; + }); + ctx.history.mockResolvedValueOnce([]).mockImplementationOnce(async () => { + ctx.invalidate(); + return []; + }); + const handling = ctx.resume.handle(suspension("corr-A", "echo same")); + ctx.lookup.resolve("call-A"); + await expect(handling).rejects.toBe(failure); + expect(ctx.onDropped).toHaveBeenCalledTimes(1); + expect(ctx.onDropped).toHaveBeenCalledWith(APPROVAL_DROPPED_NOTICE); + expect(ctx.deliver).toHaveBeenCalledTimes(1); + const message = ctx.deliver.mock.calls[0]?.[0]; + if (message === undefined) + throw new Error("expected cancellation rejection"); + expect(deliveredCorrelationId(message)).toBe("corr-A"); + expect(decisionBody(message)).toEqual({ + outcome: "rejected", + message: APPROVAL_DROPPED_NOTICE, + }); + }); + + test("a gate throw with no generation change rejects once as approval surface failed", async () => { + const ctx = deferredResume(); + const failure = new Error("gate exploded"); + ctx.resolveSuspended.mockImplementation(async () => { + throw failure; + }); + const handling = ctx.resume.handle(suspension("corr-A", "echo same")); + ctx.lookup.resolve("call-A"); + await expect(handling).rejects.toBe(failure); + expect(ctx.onDropped).not.toHaveBeenCalled(); + expect(ctx.deliver).toHaveBeenCalledTimes(1); + const message = ctx.deliver.mock.calls[0]?.[0]; + if (message === undefined) throw new Error("expected surface rejection"); + expect(deliveredCorrelationId(message)).toBe("corr-A"); + expect(decisionBody(message)).toEqual({ + outcome: "rejected", + message: "approval surface failed", + }); + }); + + test("generation change during catch history after a timeout does not reject again", async () => { + const ctx = deferredResume(); + const failure = new Error("gate exploded"); + ctx.resolveSuspended.mockImplementation(async () => { + throw failure; + }); + const reading = Promise.withResolvers(); + const history = Promise.withResolvers(); + ctx.history.mockResolvedValueOnce([]).mockImplementationOnce(() => { + reading.resolve(undefined); + return history.promise; + }); + const handling = ctx.resume.handle(suspension("corr-A", "echo same")); + ctx.lookup.resolve("call-A"); + await reading.promise; + expect(ctx.resolveSuspended).toHaveBeenCalledTimes(1); + expect(ctx.registered()).toBeUndefined(); + ctx.invalidate(); + history.resolve([timeoutTurn("call-A")]); + await expect(handling).rejects.toBe(failure); + expect(ctx.onDropped).toHaveBeenCalledTimes(1); + expect(ctx.onDropped).toHaveBeenCalledWith(APPROVAL_DROPPED_NOTICE); + expect(ctx.deliver).not.toHaveBeenCalled(); + expect(ctx.registered()).toBeUndefined(); + }); + for (const timedOutCallId of ["call-A", "call-B"]) { test(`generation change during post-gate history with ${timedOutCallId} timeout preserves exact-call cancellation`, async () => { const ctx = deferredResume(); @@ -1192,10 +1266,9 @@ describe("approval resume retry re-await", () => { expect(delivered).toHaveLength(1); }); - // Concurrent duplicates share one in-flight rejection (one gate, zero - // deliveries), then a retry re-gates and delivers exactly once, and later - // retries memoize. - test("a shared rejection re-gates once, delivers once, then memoizes", async () => { + // Concurrent duplicates share one in-flight gate failure (one gate, one + // rejection delivered), and later retries memoize. + test("a shared gate failure rejects the parked call once, then memoizes", async () => { const gate = Promise.withResolvers<{ allow: boolean }>(); const { resume, delivered, resolveSuspended } = retryHarness({ allow: true, @@ -1210,15 +1283,79 @@ describe("approval resume retry re-await", () => { await expect(first).rejects.toBe(failure); await expect(second).rejects.toBe(failure); expect(resolveSuspended).toHaveBeenCalledTimes(1); - expect(delivered).toHaveLength(0); - - expect(await resume.handle(suspension("corr-A", "echo alpha"))).toBe(true); - expect(resolveSuspended).toHaveBeenCalledTimes(2); + // Nothing retries a parked call, so the failure answers it. expect(delivered).toHaveLength(1); - expect(decisionBody(onlyDecision(delivered)).outcome).toBe("approved"); + expect(decisionBody(onlyDecision(delivered)).outcome).toBe("rejected"); expect(await resume.handle(suspension("corr-A", "echo alpha"))).toBe(true); - expect(resolveSuspended).toHaveBeenCalledTimes(2); + expect(resolveSuspended).toHaveBeenCalledTimes(1); expect(delivered).toHaveLength(1); }); + test("a gate failure after the reactor timed the call out delivers nothing", async () => { + const delivered: InboundMessage[] = []; + let timedOut = false; + const failure = new Error("gate exploded"); + const resume = createApprovalResume({ + getAgent: () => ({ + deliver: (message: InboundMessage) => { + delivered.push(message); + }, + history: async () => + timedOut + ? ([ + { + role: "user", + content: [ + { + type: "tool_result", + callId: "call-A", + content: [ + { type: "text", text: APPROVAL_TIMEOUT_RESULT_TEXT }, + ], + }, + ], + }, + ] as unknown as Awaited>) + : [], + }), + resolveParkedCallId: () => "call-A", + gate: { + resolveSuspended: async () => { + timedOut = true; + throw failure; + }, + } as unknown as PermissionGate, + }); + await expect( + resume.handle(suspension("corr-A", "echo alpha")), + ).rejects.toBe(failure); + expect(delivered).toEqual([]); + }); +}); + +describe("parked overlay registration after the operator answers", () => { + // approve_worker_call raises a nested permission prompt as soon as its + // redispatched call starts, while the decision that resumed it is still being + // delivered. That prompt's abort signal merges the parked overlay controller. + test("a permission prompt raised while the decision is delivered is not pre-aborted", async () => { + const binding = createParkedOverlayAbortBinding(); + const identity = new AbortController(); + const { agent, gate } = createApprovalResumeHarness(); + let promptSignal: AbortSignal | undefined; + const resume = createApprovalResume({ + getAgent: () => agent, + gate, + resolveParkedCallId: () => "call-A", + registerOverlayAbort: binding.registerOverlayAbort, + deliver: async (message) => { + promptSignal = binding.identitySignal(identity.signal); + agent.deliver(message); + }, + }); + expect( + await resume.handle(suspension("corr-A", "git status --short")), + ).toBe(true); + expect(promptSignal).toBeDefined(); + expect(promptSignal?.aborted).toBe(false); + }); }); diff --git a/src/session/approval-resume.ts b/src/session/approval-resume.ts index df51054d2..c83507262 100644 --- a/src/session/approval-resume.ts +++ b/src/session/approval-resume.ts @@ -543,8 +543,51 @@ export function createApprovalResume(args: { return timedOut; }); - const outcome = await args.gate.resolveSuspended(request, stillCurrent); + const outcome = await args.gate + .resolveSuspended(request, stillCurrent) + .catch(async (err: unknown) => { + // The surface resolves aborts and timeouts as outcomes, so a throw + // is a real failure. A parked call is never retried (it is not a + // stall), so reject it now instead of leaving it to the reactor's + // own timeout, unless that timeout already answered it. + if (!stillCurrent()) { + dropParked(); + throw err; + } + args.registerParkedCancel?.(undefined); + const timedOut = + overlayAbort?.signal.reason === APPROVAL_TIMEOUT_RESULT_TEXT || + (await timeoutResultFromHistory( + () => parkedAgent.history(), + parkedCallId, + )); + // A bump during that history read already cleared this cancel, so + // deliver the drop here. A call the reactor already timed out is not + // rejected again. + if (timedOut) canReject = false; + if (!stillCurrent()) { + dropParked(); + throw err; + } + if (!timedOut) { + await deliverDecision( + decisionMessage( + correlationId, + "rejected", + "approval surface failed", + ), + ).catch((deliverErr: unknown) => { + logger.warn`rejection after a failed approval surface not delivered correlation=${redactedCorrelationId(correlationId)} error=${deliverErr instanceof Error ? deliverErr.name : typeof deliverErr}`; + }); + } + throw err; + }); if (!overlayAbort.signal.aborted) overlayAbort.abort(); + // The overlay is gone, but its aborted controller still feeds every + // permission prompt's signal while registered. A resumed call can prompt + // (approve_worker_call checks the worker's frozen call) before delivery + // below resolves, and would be denied unseen. + args.registerOverlayAbort?.(undefined); const timedOutDuringOverlay = overlayAbort.signal.reason === APPROVAL_TIMEOUT_RESULT_TEXT || (await timeoutWatch); diff --git a/src/tui/runner/queued-approval-delivery.test.ts b/src/tui/runner/queued-approval-delivery.test.ts new file mode 100644 index 000000000..26f648ed4 --- /dev/null +++ b/src/tui/runner/queued-approval-delivery.test.ts @@ -0,0 +1,294 @@ +// An approval for a call parked during a turn that the TUI started by +// draining a queued delivery (ask_director wake, mailbox mail, or a queued +// operator prompt) must reach the reactor. +// +// The drained turn runs inside a sessionOps op (createLeftoverSend awaits the +// send). The operator's decision is delivered through session.ts's `deliver`, +// which enqueues another sessionOps op. If the drained op also awaited +// approvalResume.handle, that delivery would be chained behind the op waiting +// for it and never run: no deliver, no acceptance timeout, no log line, the +// reactor answering the parked call with `approval timed out` an hour later, +// and a wedged queue that hangs every later send. +// +// Uses the real createDeliverRouting + createSubmitPath (submit.ts), the real +// session queue / generation, the real createApprovalResume, and the real +// createApprovalDeliverer. The approvalResume `deliver` closure is copied +// verbatim from src/tui/runner/session.ts (createApprovalResume wiring). + +import { expect, test } from "bun:test"; +import type { Agent, SendResult } from "@intx/agent"; +import type { InboundMessage } from "@intx/types/runtime"; + +import { + createDeliveryGeneration, + createSessionOperationQueue, +} from "../delivery-queue.js"; +import { createApprovalDeliverer } from "../approval-delivery.js"; +import { + createApprovalResume, + createSuspendedApprovalRecovery, +} from "../../session/approval-resume.js"; +import type { PermissionGate } from "../../permission/gate.js"; +import { ASK_DIRECTOR_WAKE_PREFIX } from "../../subagent/fleet-report.js"; +import { withTestRenderer } from "../harness.js"; +import { setShellRunState } from "../shell/chrome.js"; +import { createAppShell } from "../shell/index.js"; +import { shellInternals, type AppShell } from "../shell/internals.js"; +import { createDeliverRouting, createSubmitPath } from "./submit.js"; +import type { RunnerServices, RunnerState } from "./state.js"; + +const CORRELATION = "00000000-0000-4000-8000-0000000000a1"; +const PARKED_CALL = "call_parked_approve_worker"; + +interface HarnessOptions { + // The operator's answer; defaults to allow-once. + resolveSuspended?: () => Promise<{ allow: boolean }>; + // Whether the stub reactor accepts a delivered decision. + acceptsDecisions?: boolean; + shell?: AppShell; +} + +function harness(opts: HarnessOptions = {}) { + const delivered: InboundMessage[] = []; + const waiters = new Map void>(); + const acceptance = { + wait: (id: string) => + new Promise((resolve) => { + waiters.set(id, resolve); + }), + settle: (id: string) => waiters.get(id)?.(), + abandon: (id: string) => { + waiters.delete(id); + }, + }; + // Stub reactor: send() parks approve_worker_call on the approval gate; + // deliver() of a correlated decision is accepted at once. + const agent = { + send: async (): Promise => + ({ + type: "suspended", + correlationId: CORRELATION, + approvalSnapshot: { + name: "approve_worker_call", + description: "Approve one exact pending worker-denied call.", + inputSchema: { type: "object" }, + arguments: { + target: "call_worker_qa_lead", + question_id: "ask-1", + request_id: "00000000-0000-4000-8000-0000000000b2", + }, + }, + }) as SendResult, + deliver: (message: InboundMessage) => { + delivered.push(message); + const id = message.headers.interchangeCorrelationId; + if (id !== undefined && opts.acceptsDecisions !== false) + acceptance.settle(id); + }, + history: async () => [], + } as unknown as Agent; + + let gateAsked = 0; + const gate = { + resolveSuspended: async () => { + gateAsked++; + return opts.resolveSuspended?.() ?? { allow: true }; + }, + } as unknown as PermissionGate; + + const sessionOps = createSessionOperationQueue(); + const deliveryGeneration = createDeliveryGeneration(); + const state = { + sessionId: "s", + config: { cwd: "/tmp", providers: [] }, + currentAgent: agent, + currentStorage: null, + fatalBuildError: null, + inFlight: 0, + sendAborted: false, + liveSource: { credentialId: undefined }, + approvalPersistNotice: {}, + credentialRecovery: { + begin: () => ({ failedProvider: "p" }), + settle: () => null, + }, + credentialRecoveryAttempts: new Map(), + reconnectRecovery: { begin: () => ({}), settle: () => null }, + reconnectRecoveryAttempts: new Map(), + host: { + bridge: { + setSuspendedApprovalRecovery: () => undefined, + parentCycleLive: false, + }, + shell: opts.shell ?? {}, + }, + } as unknown as RunnerState; + + const approvalDeliverer = createApprovalDeliverer({ + deliverToAgent: (message) => agent.deliver(message), + acceptance, + timeoutMs: 50, + }); + const approvalResume = createApprovalResume({ + getAgent: () => agent, + resolveParkedCallId: () => PARKED_CALL, + captureGeneration: deliveryGeneration.capture, + parkedTimeoutPollMs: 5, + // Verbatim from src/tui/runner/session.ts. + deliver: (message, stillCurrent) => { + return sessionOps.enqueue(async () => { + if (!stillCurrent()) return; + if (state.fatalBuildError !== null) throw state.fatalBuildError; + await approvalDeliverer.deliver(message); + }); + }, + gate, + }); + const suspendedApprovalRecovery = createSuspendedApprovalRecovery({ + storage: () => undefined, + resume: approvalResume, + }); + const services = { + sessionOps, + deliveryGeneration, + approvalResume, + suspendedApprovalRecovery, + providerFailureAttempts: { + begin: () => ({ observed: false, presented: false, error: undefined }), + sendSettled: () => undefined, + markPresented: () => undefined, + }, + } as unknown as RunnerServices; + const live = { + attemptIdentity: () => ({ providerId: "p" }) as never, + agentProxy: agent, + }; + createSubmitPath(state, services, live); + const route = createDeliverRouting(state, services, live); + return { + state, + route, + sessionOps, + delivered, + gateAsked: () => gateAsked, + }; +} + +const settleWithin = async (p: Promise, ms: number) => + Promise.race([ + p.then(() => "settled" as const), + new Promise<"pending">((r) => setTimeout(() => r("pending"), ms)), + ]); + +test("control: operator-typed send delivers the allow-once decision", async () => { + const h = harness(); + const send = h.state.sendWithAttemptIdentity; + if (send === undefined) throw new Error("no send path"); + // sendUserPrompt -> sendWithAttemptIdentity runs outside any sessionOps op. + await send({ + ref: { uid: 0, mailbox: "INBOX" }, + headers: { from: "user@local", to: ["agent@local"], date: "" }, + flags: [], + content: "approve it", + } as unknown as InboundMessage); + expect(h.gateAsked()).toBe(1); + expect(h.delivered.map((m) => m.headers.interchangeType)).toEqual([ + "approval.granted", + ]); +}); + +test("wake-drained turn: allow-once decision reaches the reactor", async () => { + const h = harness(); + let settled: unknown; + // ask_director wake drained at idle: host deliver(kind "queue") -> + // createLeftoverSend -> sessionOps op awaiting the whole send. + h.route( + `${ASK_DIRECTOR_WAKE_PREFIX} — worker qa-lead parked question ask-1`, + "queue", + [], + (result) => { + settled = result; + }, + false, + ); + await new Promise((r) => setTimeout(r, 200)); + expect(h.gateAsked()).toBe(1); // operator answered allow-once + // Before the fix the decision queued behind the op awaiting it. + expect(h.delivered.map((m) => m.headers.interchangeType)).toEqual([ + "approval.granted", + ]); + expect(settled).toEqual({ status: "accepted" }); + // And the queue must not be wedged: a later operator send awaits the tail. + expect(await settleWithin(h.sessionOps.awaitTail(), 200)).toBe("settled"); +}); + +// A fresh shell is on the landing, which defers system rows until it closes. +const systemRows = (shell: AppShell): string[] => + [ + ...(shellInternals(shell)?.landingDeferredRows ?? []), + ...shell.streamLog, + ].flatMap((row) => (row.role === "system" ? [row.text] : [])); + +const drainWake = (h: ReturnType): (() => unknown) => { + let settled: unknown; + h.route( + `${ASK_DIRECTOR_WAKE_PREFIX} — worker qa-lead parked question ask-1`, + "queue", + [], + (result) => { + settled = result; + }, + false, + ); + return () => settled; +}; + +// A decision that fails after the drained send returned cannot change the +// drain's result: the wake already reached the agent, and restoring it would +// resend it. The failure is surfaced as a notice, the parked call is answered +// rather than left to the reactor's timeout, and the shell keeps its run state +// so follow-ups queued behind this turn still drain when it ends. +for (const [name, opts, decisions] of [ + [ + "the approval surface throws", + { + resolveSuspended: () => + Promise.reject(new Error("approval overlay failed")), + }, + ["approval.denied"], + ], + [ + "the reactor never accepts the decision", + { acceptsDecisions: false }, + ["approval.granted"], + ], +] as const) { + test(`wake-drained turn: surfaces a lost decision when ${name}`, async () => { + await withTestRenderer(async (r) => { + const shell = createAppShell(r.renderer, { + title: "test", + cwd: "/tmp", + terminal: { columns: 80, rows: 24 }, + wireKeys: false, + }); + try { + const h = harness({ ...opts, shell }); + setShellRunState(shell, "busy"); + const settled = drainWake(h); + await new Promise((resolve) => setTimeout(resolve, 200)); + expect(h.gateAsked()).toBe(1); + expect(settled()).toEqual({ status: "accepted" }); + expect(h.delivered.map((m) => m.headers.interchangeType)).toEqual([ + ...decisions, + ]); + expect(systemRows(shell).length).toBe(1); + expect(shell.session.run).toBe("busy"); + expect(await settleWithin(h.sessionOps.awaitTail(), 200)).toBe( + "settled", + ); + } finally { + shell.dispose(); + } + }); + }); +} diff --git a/src/tui/runner/state.ts b/src/tui/runner/state.ts index 286a1a200..f5161affa 100644 --- a/src/tui/runner/state.ts +++ b/src/tui/runner/state.ts @@ -306,6 +306,7 @@ export interface RunnerState { message: InboundMessage, send?: (message: InboundMessage) => Promise, presentFailureNotice?: boolean, + detachApproval?: boolean, ) => Promise; /** Bound by the mounted host; receives only accepted operator message ids. */ acceptedInputObserver?: (messageId: string) => void; diff --git a/src/tui/runner/submit.ts b/src/tui/runner/submit.ts index 88520cee9..368cecb22 100644 --- a/src/tui/runner/submit.ts +++ b/src/tui/runner/submit.ts @@ -362,6 +362,7 @@ export function createSubmitPath( send: (message: InboundMessage) => Promise = (next) => live.agentProxy.send(next), presentFailureNotice = true, + detachApproval = false, ): Promise => { hostOf(state).bridge.setSuspendedApprovalRecovery( services.suspendedApprovalRecovery.tryResumeOnce, @@ -394,6 +395,36 @@ export function createSubmitPath( services.deliveryGeneration.capture(), ); } + // A queue-drained send runs inside a sessionOps op, and the decision is + // delivered through sessionOps too; awaiting it here would chain the + // delivery behind this op forever. + if (detachApproval && result.type === "suspended") { + void runWhileAgentBusy(state, async () => { + await services.approvalResume.handle(result); + services.suspendedApprovalRecovery.clear(); + }).catch((err: unknown) => { + tuiLogger.warn( + "approval for a queued turn failed to settle: {kind}", + { + kind: err instanceof Error ? err.name : typeof err, + }, + ); + // The drained message already reached the agent, so its delivery + // stays accepted and the queue posts no notice. Surface the lost + // decision here, but leave the run state alone: the run is still + // open, and settling the shell idle would strand follow-ups queued + // behind this turn. + if (state.sendAborted) return; + const detail = err instanceof Error ? err.message : String(err); + systemNotice( + sanitizeRunnerDiagnostic( + state, + `approval for a queued turn failed: ${detail}`, + ), + ); + }); + return; + } await services.approvalResume.handle(result); // Settled: nothing is left for the stall watchdog to re-present. services.suspendedApprovalRecovery.clear(); @@ -537,6 +568,7 @@ export function createDeliverRouting( userInboundMessage(text, pending, operatorOriginated), (message) => targetAgent.send(message), false, + true, ); }, recordSent: (text) => {