From 49fba59925c91e1e874a073229ca79de4cb2579c Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 14 Sep 2026 14:45:54 -0700 Subject: [PATCH] fix(runtime): apply the tui-idle evidence ranking to mailbox delivery (#20578) fix(runtime): retry a delivery that the idle gate refused Gates delivery at the two points where each implementation commits to typing into the pane, rather than at each caller, and parks-and-re-offers a refusal so late idle evidence cannot strand a queued message. Refs #6011 --- .../orca-runtime-apply-tracked-pty-title.ts | 11 +- .../orca-runtime-deliver-pending-messages.ts | 3 +- .../orca-runtime-resolve-exit-waiters.ts | 70 +++++++ src/main/runtime/orca-runtime-runtime-id.ts | 3 + ...ntime-serialize-agent-prompt-submission.ts | 6 +- .../orca-runtime-stop-requested-pty-ids.ts | 1 + .../runtime/orca-runtime-sync-window-graph.ts | 1 + .../mailbox-pointer-delivery-contract.ts | 2 + .../orchestration/mailbox-pointer-delivery.ts | 10 + .../mailbox-pointer-stage.test.ts | 2 + .../runtime/runtime-terminal-idle-polls.ts | 17 +- .../tui-idle-delivery-and-quiescence.test.ts | 190 ++++++++++++++++++ 12 files changed, 311 insertions(+), 5 deletions(-) create mode 100644 src/main/runtime/tui-idle-delivery-and-quiescence.test.ts diff --git a/src/main/runtime/orca-runtime-apply-tracked-pty-title.ts b/src/main/runtime/orca-runtime-apply-tracked-pty-title.ts index fc86de47451..3459c7c8b6e 100644 --- a/src/main/runtime/orca-runtime-apply-tracked-pty-title.ts +++ b/src/main/runtime/orca-runtime-apply-tracked-pty-title.ts @@ -138,7 +138,16 @@ export class OrcaRuntimeWithApplyTrackedPtyTitle extends OrcaRuntimeWithGetUnper // an agent whose first live title is already idle (claude --resume at its // prompt) then shows no transition — the row would strand, which is // exactly #12536. Waiter semantics stay transition-only above. - if (agentStatus === 'idle' && (prevStatus !== 'idle' || !prevObservedLive)) { + // Why the title change joins the edge: a name-only frame routinely lands before the + // hook's `X ready`, and it consumes the working→idle transition. The later ready title + // is an idle→idle step, so gating delivery on `prevStatus !== 'idle'` meant the + // strongest evidence this pane will ever emit never reached delivery at all. The + // waiter branch above already re-offers on that step; the gate makes a repeat harmless. + if ( + agentStatus === 'idle' && + (prevStatus !== 'idle' || !prevObservedLive || prevLeafTitle !== recordedTitle) && + this.checkDeliverySettledAndArmRecheck(leaf) + ) { this.deliverPendingMessagesForLeaf(leaf) } } diff --git a/src/main/runtime/orca-runtime-deliver-pending-messages.ts b/src/main/runtime/orca-runtime-deliver-pending-messages.ts index 8d55cddd112..55c6a8dad5e 100644 --- a/src/main/runtime/orca-runtime-deliver-pending-messages.ts +++ b/src/main/runtime/orca-runtime-deliver-pending-messages.ts @@ -111,7 +111,8 @@ export class OrcaRuntimeWithDeliverPendingMessages extends OrcaRuntimeWithResolv if ( currentLeaf?.ptyId === probedPtyId && currentLeaf.lastAgentStatus === 'idle' && - currentLeaf.lastAgentStatusObservedLive + currentLeaf.lastAgentStatusObservedLive && + this.checkDeliverySettledAndArmRecheck(currentLeaf) ) { this.deliverPendingMessages(currentLeaf, { mailboxHandle, diff --git a/src/main/runtime/orca-runtime-resolve-exit-waiters.ts b/src/main/runtime/orca-runtime-resolve-exit-waiters.ts index 221bb873545..ea6329db3e6 100644 --- a/src/main/runtime/orca-runtime-resolve-exit-waiters.ts +++ b/src/main/runtime/orca-runtime-resolve-exit-waiters.ts @@ -117,6 +117,76 @@ export class OrcaRuntimeWithResolveExitWaiters extends OrcaRuntimeWithBindPtyInc }) } + /** + * Settled-enough-to-type check that also arms a retry when it says no. + * + * Why the retry: the wait path POLLS, so weak evidence that only becomes valid with the + * passage of time (a pane going quiet) eventually satisfies it. Delivery is edge-driven — + * a title transition, a graph sync, a new message — with no poll behind it, so a refusal + * at an edge is final unless another edge happens to arrive. A hookless Codex pane never + * emits an explicit `X ready`, so the refusal below would strand the queued message + * permanently once the pane fell quiet. One-shot timer, armed only for a leaf that + * actually refused, cleared as soon as any path delivers. + */ + protected checkDeliverySettledAndArmRecheck(leaf: { tabId: string; leafId: string }): boolean { + const leafKey = this.getLeafKey(leaf.tabId, leaf.leafId) + if (this.isAgentSettledForDelivery(leaf)) { + this.clearDeliveryRecheck(leafKey) + return true + } + this.armDeliveryRecheck(leafKey) + return false + } + + protected clearDeliveryRecheck(leafKey: string): void { + const timer = this.deliveryRecheckTimersByLeafKey.get(leafKey) + if (timer) { + clearTimeout(timer) + this.deliveryRecheckTimersByLeafKey.delete(leafKey) + } + } + + private armDeliveryRecheck(leafKey: string): void { + if (this.deliveryRecheckTimersByLeafKey.has(leafKey)) { + return + } + const live = this.leaves.get(leafKey) + // Why this delay: the only refusal that time alone can lift is tier 3 waiting on the + // stream to go quiet, so wake just after the window could have elapsed. A pane that is + // still producing output re-arms from its own fresher timestamp rather than spinning. + const elapsed = live?.lastOutputAt ? Date.now() - live.lastOutputAt : 0 + const delay = Math.max(TUI_IDLE_QUIESCENCE_MS - elapsed, 0) + 50 + const timer = setTimeout(() => { + this.deliveryRecheckTimersByLeafKey.delete(leafKey) + const current = this.leaves.get(leafKey) + if (!current) { + return + } + // Why the gate again here: delivery sites gate at the CALL, not inside + // deliverPendingMessagesForLeaf, so firing straight into it would hand the retry the + // very injection the gate exists to prevent. A pane that went busy again re-arms. + if (this.checkDeliverySettledAndArmRecheck(current)) { + this.deliverPendingMessagesForLeaf(current) + } + }, delay) + timer.unref?.() + this.deliveryRecheckTimersByLeafKey.set(leafKey, timer) + } + + /** + * Whether this pane is settled enough to TYPE INTO. + * + * Why the same ranking as the wait path: mailbox delivery writes the pointer plus Enter + * into the pane, so acting on a name-only `Codex` title mid-turn injects keystrokes into + * a running agent's session. That is the #6011 mis-settlement in a path with a worse + * failure mode than a racing script. Liveness stays a separate requirement — callers + * keep their own `lastAgentStatusObservedLive` checks. + */ + protected isAgentSettledForDelivery(leaf: { tabId: string; leafId: string }): boolean { + const live = this.leaves.get(this.getLeafKey(leaf.tabId, leaf.leafId)) + return live ? this.isTuiIdleSatisfiedForLeaf(live) : false + } + protected isTuiIdleSatisfiedForPty(pty: RuntimePtyWorktreeRecord): boolean { return isTuiIdleSatisfied({ record: pty, diff --git a/src/main/runtime/orca-runtime-runtime-id.ts b/src/main/runtime/orca-runtime-runtime-id.ts index 0806b705fdf..00ccdde9da9 100644 --- a/src/main/runtime/orca-runtime-runtime-id.ts +++ b/src/main/runtime/orca-runtime-runtime-id.ts @@ -274,6 +274,9 @@ export class OrcaRuntimeWithRuntimeId { return pty?.launchAgent ?? pty?.foregroundAgent ?? null } + /** One-shot delivery retries, keyed by leaf. See checkDeliverySettledAndArmRecheck. */ + protected deliveryRecheckTimersByLeafKey = new Map>() + protected leaves = new Map() // Why: PTY output is a per-keystroke hot path. Looking up affected leaves by diff --git a/src/main/runtime/orca-runtime-serialize-agent-prompt-submission.ts b/src/main/runtime/orca-runtime-serialize-agent-prompt-submission.ts index 1e321bc7584..dd3afbfa23e 100644 --- a/src/main/runtime/orca-runtime-serialize-agent-prompt-submission.ts +++ b/src/main/runtime/orca-runtime-serialize-agent-prompt-submission.ts @@ -139,7 +139,11 @@ export class OrcaRuntimeWithSerializeAgentPromptSubmission extends OrcaRuntimeWi leaf.lastAgentStatus = restoredStatus if (restoredStatus === 'idle') { this.resolveTuiIdleWaiters(leaf) - this.deliverPendingMessagesForLeaf(leaf) + // Why gated like every other delivery edge: a neutral-title restoration can + // reinstate `idle` from a name-only title, which is not evidence a turn ended. + if (this.checkDeliverySettledAndArmRecheck(leaf)) { + this.deliverPendingMessagesForLeaf(leaf) + } } } } diff --git a/src/main/runtime/orca-runtime-stop-requested-pty-ids.ts b/src/main/runtime/orca-runtime-stop-requested-pty-ids.ts index c7f883fcfcb..4989245fdf6 100644 --- a/src/main/runtime/orca-runtime-stop-requested-pty-ids.ts +++ b/src/main/runtime/orca-runtime-stop-requested-pty-ids.ts @@ -203,6 +203,7 @@ export class OrcaRuntimeWithStopRequestedPtyIds extends OrcaRuntimeWithRuntimeId getLeaf: (leafKey) => this.leaves.get(leafKey), getLeafKey: (tabId, leafId) => this.getLeafKey(tabId, leafId), getLiveLeafForHandle: (handle) => this.getLiveLeafForHandle(handle).leaf, + isAgentSettledForDelivery: (leaf) => this.checkDeliverySettledAndArmRecheck(leaf), getMessageWaiters: (mailboxHandle) => this.messageWaiters.get(mailboxHandle), getTabTitle: (tabId) => this.tabs.get(tabId)?.title, getCliCommand: (terminalHandle) => this.getTerminalOrchestrationCliCommand(terminalHandle), diff --git a/src/main/runtime/orca-runtime-sync-window-graph.ts b/src/main/runtime/orca-runtime-sync-window-graph.ts index 3a784fcd622..e820d15130b 100644 --- a/src/main/runtime/orca-runtime-sync-window-graph.ts +++ b/src/main/runtime/orca-runtime-sync-window-graph.ts @@ -283,6 +283,7 @@ export class OrcaRuntimeWithSyncWindowGraph extends OrcaRuntimeWithAttachWindow this._orchestrationDb && leaf.lastAgentStatus === 'idle' && leaf.lastAgentStatusObservedLive && + this.checkDeliverySettledAndArmRecheck(leaf) && leaf.writable && (!graphWasReady || previousLeaf?.ptyId !== leaf.ptyId || diff --git a/src/main/runtime/orchestration/mailbox-pointer-delivery-contract.ts b/src/main/runtime/orchestration/mailbox-pointer-delivery-contract.ts index a0b19b2d27a..91fcac67bd1 100644 --- a/src/main/runtime/orchestration/mailbox-pointer-delivery-contract.ts +++ b/src/main/runtime/orchestration/mailbox-pointer-delivery-contract.ts @@ -22,6 +22,8 @@ export type PointerDeliveryDependencies OrchestrationMailboxLeaf | undefined getLeafKey: (tabId: string, leafId: string) => string getLiveLeafForHandle: (handle: string) => OrchestrationMailboxLeaf + /** Whether the pane is settled enough to type the pointer plus Enter into it. */ + isAgentSettledForDelivery: (leaf: OrchestrationMailboxLeaf) => boolean getMessageWaiters: (mailboxHandle: string) => ReadonlySet | undefined getTabTitle: (tabId: string) => string | null | undefined getCliCommand: (terminalHandle: string) => OrchestrationCliCommand diff --git a/src/main/runtime/orchestration/mailbox-pointer-delivery.ts b/src/main/runtime/orchestration/mailbox-pointer-delivery.ts index b16e5c474b3..b8c8ad7a0c4 100644 --- a/src/main/runtime/orchestration/mailbox-pointer-delivery.ts +++ b/src/main/runtime/orchestration/mailbox-pointer-delivery.ts @@ -70,6 +70,16 @@ export class OrchestrationMailboxPointerDelivery WriteSettlement) { getLeaf: () => LEAF, getLeafKey: () => 'tab-1:leaf-1', getLiveLeafForHandle: () => LEAF, + // These cases exercise staging and Enter phases, not the idle gate; the pane is settled. + isAgentSettledForDelivery: () => true, getMessageWaiters: () => undefined, getTabTitle: () => null, getCliCommand: () => 'orca' as const, diff --git a/src/main/runtime/runtime-terminal-idle-polls.ts b/src/main/runtime/runtime-terminal-idle-polls.ts index 4fb5c470c2f..e1be9654611 100644 --- a/src/main/runtime/runtime-terminal-idle-polls.ts +++ b/src/main/runtime/runtime-terminal-idle-polls.ts @@ -17,6 +17,19 @@ import { type FirstPartyAgentStatus } from './tui-idle-evidence' import type { TuiAgent } from '../../shared/tui-agent' + +/** + * Why null counts as quiet: a record with no output timestamp has produced nothing the + * RUNTIME OBSERVED since it was created. That is not the same as silence — the reachable + * case is a daemon-hosted pane whose bytes never reach the runtime, which may still be + * streaming. The trade is deliberate: "never settles" becomes "settles uncorroborated", + * the caller keeps its timeout, and delivery cannot reach this lane. Reading it as `0ms since output` + * inverted that — `0 >= quiescenceMs` is false forever, so an adopted pane that never + * emitted could not settle no matter how long the caller waited. + */ +function isQuietForQuiescence(lastOutputAt: number | null, quiescenceMs: number): boolean { + return lastOutputAt === null ? true : Date.now() - lastOutputAt >= quiescenceMs +} import type { TerminalWaiter } from './runtime-terminal-contracts' import type { RuntimeLeafRecord, RuntimePtyWorktreeRecord } from './runtime-terminal-state-records' @@ -141,7 +154,7 @@ export class RuntimeTerminalIdlePolls { if ( foreground && !isShellProcess(foreground) && - (live.lastOutputAt ? Date.now() - live.lastOutputAt : 0) >= this.deps.quiescenceMs + isQuietForQuiescence(live.lastOutputAt, this.deps.quiescenceMs) ) { this.stop(entry) this.deps.resolve(waiter, buildTerminalWaitResult(waiter.handle, 'tui-idle', live)) @@ -206,7 +219,7 @@ export class RuntimeTerminalIdlePolls { if ( foreground && !isShellProcess(foreground) && - (pty.lastOutputAt ? Date.now() - pty.lastOutputAt : 0) >= this.deps.quiescenceMs + isQuietForQuiescence(pty.lastOutputAt, this.deps.quiescenceMs) ) { this.stop(entry) this.deps.resolve(waiter, buildPtyTerminalWaitResult(waiter.handle, 'tui-idle', pty)) diff --git a/src/main/runtime/tui-idle-delivery-and-quiescence.test.ts b/src/main/runtime/tui-idle-delivery-and-quiescence.test.ts new file mode 100644 index 00000000000..02d3a62dcc9 --- /dev/null +++ b/src/main/runtime/tui-idle-delivery-and-quiescence.test.ts @@ -0,0 +1,190 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { makeTuiIdleRuntime } from './tui-idle-wait-test-harness' +import type { RuntimeSyncWindowGraph } from '../../shared/runtime-types' +import type { OrcaRuntimeService } from './orca-runtime' +import type { TuiAgent } from '../../shared/tui-agent' + +// Follow-ons to #6011. The evidence ranking that fixed the wait path did not reach two +// other consumers of the same signal: mailbox delivery, which TYPES INTO the pane, and +// the idle poll's quiescence gate, which read a missing output clock as "never quiet". + +const WORKTREE_ID = 'repo-1::/tmp/followups' +const TAB_ID = 'c1c1c1c1-c1c1-4c1c-8c1c-c1c1c1c1c1c1' +const LEAF_ID = 'c2c2c2c2-c2c2-4c2c-8c2c-c2c2c2c2c2c2' +const PTY_ID = 'pty-followups' +const ESC = String.fromCharCode(27) +const BEL = String.fromCharCode(7) +const osc = (title: string) => `${ESC}]0;${title}${BEL}` +const agentStatus = (state: string, agentType: string) => + `${ESC}]9999;{"state":"${state}","agentType":"${agentType}"}${BEL}` + +const GRAPH: RuntimeSyncWindowGraph = { + tabs: [ + { tabId: TAB_ID, worktreeId: WORKTREE_ID, title: 'Agent', activeLeafId: LEAF_ID, layout: null } + ], + leaves: [ + { + tabId: TAB_ID, + worktreeId: WORKTREE_ID, + leafId: LEAF_ID, + paneRuntimeId: 1, + ptyId: PTY_ID, + paneTitle: null, + title: '' + } + ] +} + +async function makeRuntime(launchAgent: TuiAgent | null, foreground = 'codex') { + const runtime = makeTuiIdleRuntime({ + repoPath: '/tmp/followups', + getForegroundProcess: async () => foreground + }) + runtime.attachWindow(1) + runtime.syncWindowGraph(1, GRAPH) + runtime.registerPty(PTY_ID, WORKTREE_ID, null, { + tabId: TAB_ID, + leafId: LEAF_ID, + incarnationId: 'followups-inc', + ...(launchAgent ? { agentLaunchAuthority: { launchToken: 'tok', launchAgent } } : {}) + }) + const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`) + return { runtime, handle: terminals[0].handle } +} + +/** Counts real delivery attempts. Spies on the delivery entry point, NOT on the gate + * under test — the gate runs for real and decides whether this is ever reached. */ +function watchDelivery(runtime: OrcaRuntimeService) { + return vi + .spyOn( + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the delivery entry point is protected; the spy only needs its name and signature. + runtime as never as { deliverPendingMessagesForLeaf: (leaf: unknown) => void }, + 'deliverPendingMessagesForLeaf' + ) + .mockImplementation(() => {}) +} + +// Why fake timers: the retry fires on a real 3s quiescence window, and asserting around it +// with wall-clock sleeps made the result depend on how promptly a loaded CI runner schedules +// an interval. The clock is the thing under test, so it has to be the deterministic part. +describe('mailbox delivery honours the tui-idle evidence ranking', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + afterEach(() => { + vi.useRealTimers() + }) + + it('does not deliver into a pane that is only showing its agent name mid-turn', async () => { + const { runtime } = await makeRuntime('codex') + const deliver = watchDelivery(runtime) + runtime.onPtyData(PTY_ID, `${osc('⠋ Codex')}working\n`, Date.now()) + expect(deliver).not.toHaveBeenCalled() + + // The busy agent repaints its title to the bare product name. That reads as `idle` + // for display, but it is emitted just as often mid-turn — typing into the pane here + // injects the pointer plus Enter into a running turn. + runtime.onPtyData(PTY_ID, `${osc('Codex')}still working\n`, Date.now()) + expect(deliver).not.toHaveBeenCalled() + }) + + it('delivers once the agent states it is done', async () => { + const { runtime } = await makeRuntime('codex') + const deliver = watchDelivery(runtime) + runtime.onPtyData(PTY_ID, `${osc('⠋ Codex')}working\n`, Date.now()) + runtime.onPtyData(PTY_ID, `${osc('Codex ready')}done\n`, Date.now()) + expect(deliver).toHaveBeenCalled() + }) + + // Why this case exists: the wait path POLLS, so weak evidence that only becomes valid + // with time eventually satisfies it. Delivery is edge-driven with no poll behind it, so a + // refusal at an edge is final unless another edge arrives. A hookless Codex never emits an + // explicit `X ready`, so without a retry the queued message strands permanently once the + // pane falls quiet — trading a visible mis-delivery for an invisible lost message. + it('retries a refused delivery once the pane falls quiet', async () => { + const { runtime } = await makeRuntime('codex') + const deliver = watchDelivery(runtime) + runtime.onPtyData(PTY_ID, `${osc('\u280b Codex')}working\n`, Date.now()) + runtime.onPtyData(PTY_ID, `${osc('Codex')}output\n`, Date.now()) + expect(deliver).not.toHaveBeenCalled() + + // Output stops. No further title frame and no renderer graph sync — a daemon-hosted + // pane has nobody publishing one, so nothing re-fires an edge on its own. + await vi.advanceTimersByTimeAsync(5_000) + expect(deliver).toHaveBeenCalled() + }) + + it('does not retry into a pane that went busy again', async () => { + const { runtime } = await makeRuntime('codex') + const deliver = watchDelivery(runtime) + runtime.onPtyData(PTY_ID, `${osc('Codex')}output\n`, Date.now()) + // Keep the stream alive across the whole retry window. + // Deterministic streaming: one chunk every 250ms of virtual time, so the gap between + // chunks can never drift past the quiescence window the way a real interval can. + for (let tick = 0; tick < 20; tick += 1) { + runtime.onPtyData(PTY_ID, 'more output\n', Date.now()) + await vi.advanceTimersByTimeAsync(250) + } + expect(deliver).not.toHaveBeenCalled() + }) + + // Case B, the mainline path: a hooked Codex emits a name-only frame BEFORE the hook's + // `Codex ready`. The name-only frame consumes the working->idle transition, leaving the + // ready title as an idle->idle step that delivery was never offered — so the strongest + // evidence the agent ever emits could not reach it. + it('delivers when the ready title arrives after a name-only frame', async () => { + const { runtime } = await makeRuntime('codex') + const deliver = watchDelivery(runtime) + runtime.onPtyData(PTY_ID, `${osc('\u280b Codex')}working\n`, Date.now()) + runtime.onPtyData(PTY_ID, `${osc('Codex')}out\n`, Date.now()) + expect(deliver).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(100) + runtime.onPtyData(PTY_ID, osc('Codex ready'), Date.now()) + // Promptly, on the ready title itself — not after waiting out a quiescence window. + expect(deliver).toHaveBeenCalled() + }) + + // Case C: the agent's own status stream vetoes the idle title, then reports done with no + // edge behind it. `working` stays fresh for 30 minutes, so without a re-offer the veto + // outlives the turn it described. + it('delivers when a done status lands after the idle title was vetoed', async () => { + const { runtime } = await makeRuntime('claude') + const deliver = watchDelivery(runtime) + runtime.onPtyData( + PTY_ID, + `${agentStatus('working', 'claude')}${osc('\u280b Claude')}w\n`, + Date.now() + ) + runtime.onPtyData(PTY_ID, `${osc('claude')}out\n`, Date.now()) + expect(deliver).not.toHaveBeenCalled() + + runtime.onPtyData(PTY_ID, agentStatus('done', 'claude'), Date.now()) + await vi.advanceTimersByTimeAsync(4_500) + expect(deliver).toHaveBeenCalled() + }) + + it('still delivers for an agent whose name is its only rest signal', async () => { + const { runtime } = await makeRuntime('grok', 'grok') + const deliver = watchDelivery(runtime) + runtime.onPtyData(PTY_ID, `${osc('⠋ Grok')}working\n`, Date.now()) + runtime.onPtyData(PTY_ID, `${osc('grok')}banner\n`, Date.now()) + expect(deliver).toHaveBeenCalled() + }) +}) + +describe('quiescence treats a missing output clock as quiet', () => { + it('settles a pane that has never produced output but holds a live agent process', async () => { + // No launch metadata: Orca did not start this agent, so the quiet-foreground lane is + // the only evidence available, and `lastOutputAt` is null because nothing ever arrived. + const { runtime, handle } = await makeRuntime(null, 'codex') + const leaves = + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: reading the runtime's own leaf map to assert the precondition this test depends on. + (runtime as never as { leaves: Map }).leaves + expect([...leaves.values()][0].lastOutputAt).toBeNull() + + await expect( + runtime.waitForTerminal(handle, { condition: 'tui-idle', timeoutMs: 8_000 }) + ).resolves.toMatchObject({ condition: 'tui-idle', satisfied: true }) + }, 20_000) +})