From 2b42de1f522ae3bb8fe397b9217e223e3c5cf6f6 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Thu, 6 Aug 2026 22:50:09 -0700 Subject: [PATCH] fix(orchestration): wake coordinators with mail pointers (#12988) Wake idle Run coordinators with durable orchestration mail pointers while keeping message payloads in the store until check consumes them. Preserve waiter, Cursor, restart, real Codex title, and PTY replacement behavior.\n\nPart of #12953. --- src/main/runtime/orca-runtime.test.ts | 240 +++++++++++++++--- src/main/runtime/orca-runtime.ts | 178 ++++++++----- .../runtime/orchestration/formatter.test.ts | 14 +- src/main/runtime/orchestration/formatter.ts | 5 + .../terminal-send-stale-leaf-liveness.test.ts | 97 +++---- src/shared/agent-title-status.ts | 10 +- src/shared/terminal-output-side-effects.ts | 7 +- .../helpers/orchestration-mail-pane-agent.ts | 6 +- tests/e2e/helpers/orchestration-mail-store.ts | 16 +- .../orchestration-idle-mail-delivery.spec.ts | 132 ++++++++-- .../orchestration-idle-mail-restore.spec.ts | 18 +- 11 files changed, 517 insertions(+), 206 deletions(-) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index cabbb9ac0..90b7bb45f 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -1029,6 +1029,11 @@ class InMemoryOrchestrationMessages { private messages: MessageRow[] = [] + private runs = new Map< + string, + { id: string; coordinator_handle: string | null; coordinator_pane_key: string | null } + >() + insertMessage(msg: { from: string to: string @@ -1084,6 +1089,30 @@ class InMemoryOrchestrationMessages { return this.activeCoordinatorRun } + setRun(run: { + id: string + coordinator_handle: string | null + coordinator_pane_key?: string | null + }): void { + this.runs.set(run.id, { coordinator_pane_key: null, ...run }) + } + + getRun( + id: string + ): + | { id: string; coordinator_handle: string | null; coordinator_pane_key: string | null } + | undefined { + return this.runs.get(id) + } + + getCurrentRunForPane( + paneKey: string + ): + | { id: string; coordinator_handle: string | null; coordinator_pane_key: string | null } + | undefined { + return [...this.runs.values()].find((run) => run.coordinator_pane_key === paneKey) + } + markAsDelivered(ids: string[]): void { const deliveredIds = new Set(ids) for (const message of this.messages) { @@ -18468,23 +18497,24 @@ describe('OrcaRuntimeService', () => { runtime.deliverPendingMessagesForHandle(terminal.handle) - expect(write).toHaveBeenCalledWith('pty-1', expect.stringContaining('Subject: hello')) - // Why: the split Enter write lands after the 500ms delay; advance past it before asserting on delivered_at. + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 1 orchestration message') + ) await vi.advanceTimersByTimeAsync(500) expect(write).toHaveBeenCalledWith('pty-1', '\r') - // Why: design doc §3.2 splits delivered vs. read — push-on-idle stamps delivered_at but must not flip read; only the agent's check consumes, so rows stay unread. const unread = db.getUnreadMessages(terminal.handle) expect(unread).toHaveLength(1) expect(unread[0].read).toBe(0) - expect(unread[0].delivered_at).not.toBeNull() + expect(unread[0].delivered_at).toBeNull() db.close() } finally { vi.useRealTimers() } }) - it('injects pending orchestration messages into the active coordinator without auto-submitting', async () => { + it('submits the mail pointer in an active coordinator pane', async () => { vi.useFakeTimers() try { const runtime = new OrcaRuntimeService(store) @@ -18512,18 +18542,18 @@ describe('OrcaRuntimeService', () => { expect(write).toHaveBeenCalledWith( 'pty-1', - expect.stringContaining('Subject: hello coordinator') + expect.stringContaining('You have 1 orchestration message') ) await vi.advanceTimersByTimeAsync(500) const submitWrites = write.mock.calls.filter( ([ptyId, text]) => ptyId === 'pty-1' && text === '\r' ) - expect(submitWrites).toHaveLength(0) + expect(submitWrites).toHaveLength(1) const unread = db.getUnreadMessages(terminal.handle) expect(unread).toHaveLength(1) expect(unread[0].read).toBe(0) - expect(unread[0].delivered_at).not.toBeNull() + expect(unread[0].delivered_at).toBeNull() db.close() } finally { vi.useRealTimers() @@ -18551,18 +18581,20 @@ describe('OrcaRuntimeService', () => { runtime.deliverPendingMessagesForHandle(terminal.handle) - expect(write).toHaveBeenCalledWith('pty-1', expect.stringContaining('Subject: hello cursor')) + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 1 orchestration message') + ) await vi.advanceTimersByTimeAsync(500) const submitWrites = write.mock.calls.filter( ([ptyId, text]) => ptyId === 'pty-1' && text === '\r' ) expect(submitWrites).toHaveLength(0) - // Why: Cursor Agent treats injected PTY text as editable prompt input, so the user submits manually but the banner must not replay on the next idle transition. const unread = db.getUnreadMessages(terminal.handle) expect(unread).toHaveLength(1) expect(unread[0].read).toBe(0) - expect(unread[0].delivered_at).not.toBeNull() + expect(unread[0].delivered_at).toBeNull() db.close() } finally { vi.useRealTimers() @@ -18591,7 +18623,10 @@ describe('OrcaRuntimeService', () => { runtime.deliverPendingMessagesForHandle(terminal.handle) await vi.advanceTimersByTimeAsync(500) - expect(write).toHaveBeenCalledWith('pty-1', expect.stringContaining('Subject: hello claude')) + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 1 orchestration message') + ) expect(write).toHaveBeenCalledWith('pty-1', '\r') db.close() } finally { @@ -18622,16 +18657,16 @@ describe('OrcaRuntimeService', () => { await vi.advanceTimersByTimeAsync(500) const firstInjections = write.mock.calls.filter( - (c) => typeof c[1] === 'string' && c[1].includes('Subject: hello') + (c) => typeof c[1] === 'string' && c[1].includes('orca orchestration check') ).length expect(firstInjections).toBe(1) - // Second idle transition: the row is unread but already delivered, so push-on-idle must skip it to avoid the replay bug. + // The row remains pending, so the in-memory sequence watermark prevents replay. runtime.deliverPendingMessagesForHandle(terminal.handle) await vi.advanceTimersByTimeAsync(500) const totalInjections = write.mock.calls.filter( - (c) => typeof c[1] === 'string' && c[1].includes('Subject: hello') + (c) => typeof c[1] === 'string' && c[1].includes('orca orchestration check') ).length expect(totalInjections).toBe(1) db.close() @@ -33284,16 +33319,124 @@ describe('OrcaRuntimeService', () => { // The push is deferred one microtask so it lands behind any resolved check. await Promise.resolve() - expect(write).toHaveBeenCalledWith('pty-1', expect.stringContaining('Subject: after wait')) + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 1 orchestration message') + ) + expect(write).not.toHaveBeenCalledWith('pty-1', expect.stringContaining('after wait')) await vi.advanceTimersByTimeAsync(500) expect(write).toHaveBeenCalledWith('pty-1', '\r') - expect(message.delivered_at).not.toBeNull() + expect(message.delivered_at).toBeNull() db.close() } finally { vi.useRealTimers() } }) + it('points a Run mailbox at its live-idle coordinator without replaying pending rows', async () => { + vi.useFakeTimers() + try { + const runtime = new OrcaRuntimeService(store) + const db = new InMemoryOrchestrationMessages() + const write = vi.fn().mockReturnValue(true) + setInMemoryOrchestrationMessages(runtime, db) + runtime.setPtyController({ + write, + kill: vi.fn(), + getForegroundProcess: async () => null + }) + syncSinglePty(runtime) + + const [terminal] = (await runtime.listTerminals()).terminals + db.setRun({ + id: 'run_mailbox', + coordinator_handle: terminal.handle, + coordinator_pane_key: `${terminal.tabId}:${terminal.leafId}` + }) + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + const message = db.insertMessage({ + from: 'term_worker', + to: 'run:run_mailbox', + subject: 'one P3 finding', + body: 'private worker report', + type: 'worker_done' + }) + + runtime.notifyMessageArrived('run:run_mailbox', 'worker_done') + await Promise.resolve() + expect(write).not.toHaveBeenCalled() + + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + expect(write).toHaveBeenCalledWith( + 'pty-1', + '\nYou have 1 orchestration message. Run `orca orchestration check`.\n' + ) + expect(write).not.toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('private worker report') + ) + await vi.advanceTimersByTimeAsync(500) + expect(write).toHaveBeenCalledWith('pty-1', '\r') + expect(message.delivered_at).toBeNull() + + runtime.notifyMessageArrived('run:run_mailbox', 'worker_done') + await Promise.resolve() + await vi.advanceTimersByTimeAsync(500) + expect( + write.mock.calls.filter( + ([, payload]) => + typeof payload === 'string' && payload.includes('orca orchestration check') + ) + ).toHaveLength(1) + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('points already-idle Run mail after Codex replaces its completion title', async () => { + const runtime = new OrcaRuntimeService(store) + const db = new InMemoryOrchestrationMessages() + const write = vi.fn().mockReturnValue(true) + setInMemoryOrchestrationMessages(runtime, db) + runtime.setPtyController({ + write, + kill: vi.fn(), + getForegroundProcess: async () => 'codex' + }) + syncSinglePty(runtime) + + const [terminal] = (await runtime.listTerminals()).terminals + db.setRun({ + id: 'run_codex_native_title', + coordinator_handle: terminal.handle, + coordinator_pane_key: `${terminal.tabId}:${terminal.leafId}` + }) + runtime.ingestSyntheticTitleFrame('pty-1', '\x1b]0;Codex ready\x07') + runtime.onPtyData('pty-1', '\x1b]0;fix-12953-orchestration-mail-pointer\x07', 101) + await Promise.resolve() + await Promise.resolve() + await Promise.resolve() + db.insertMessage({ + from: 'term_worker', + to: 'run:run_codex_native_title', + subject: 'real-agent smoke complete', + body: 'The package name is orca.', + type: 'worker_done' + }) + + runtime.notifyMessageArrived('run:run_codex_native_title', 'worker_done') + await Promise.resolve() + + await vi.waitFor(() => { + expect(write).toHaveBeenCalledWith( + 'pty-1', + '\nYou have 1 orchestration message. Run `orca orchestration check`.\n' + ) + }) + db.close() + }) + it('does not inject pending mail on notify when the recipient is still working', async () => { const runtime = new OrcaRuntimeService(store) const db = new InMemoryOrchestrationMessages() @@ -33355,9 +33498,12 @@ describe('OrcaRuntimeService', () => { // is no transition — only the liveness edge can release the row (#12536). runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 100) - expect(write).toHaveBeenCalledWith('pty-1', expect.stringContaining('Subject: restored idle')) + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 1 orchestration message') + ) await vi.advanceTimersByTimeAsync(600) - expect(message.delivered_at).not.toBeNull() + expect(message.delivered_at).toBeNull() db.close() } finally { vi.useRealTimers() @@ -33399,9 +33545,12 @@ describe('OrcaRuntimeService', () => { // The first live idle frame authorizes it and the row still delivers. runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) - expect(write).toHaveBeenCalledWith('pty-1', expect.stringContaining('Subject: seeded idle')) + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 1 orchestration message') + ) await vi.advanceTimersByTimeAsync(600) - expect(message.delivered_at).not.toBeNull() + expect(message.delivered_at).toBeNull() db.close() } finally { vi.useRealTimers() @@ -33503,9 +33652,11 @@ describe('OrcaRuntimeService', () => { const payloads = write.mock.calls .map(([, data]) => data) .filter((data): data is string => typeof data === 'string') - expect(payloads.some((data) => data.includes('Subject: unclaimed status'))).toBe(true) - expect(payloads.some((data) => data.includes('Subject: reserved completion'))).toBe(false) - expect(status.delivered_at).not.toBeNull() + expect(payloads).toContain( + '\nYou have 1 orchestration message. Run `orca orchestration check`.\n' + ) + expect(payloads.some((data) => data.includes('reserved completion'))).toBe(false) + expect(status.delivered_at).toBeNull() expect(done.delivered_at).toBeNull() db.close() } finally { @@ -33684,10 +33835,10 @@ describe('OrcaRuntimeService', () => { expect(write).toHaveBeenCalledWith( 'pty-1', - expect.stringContaining('Subject: after republish') + expect.stringContaining('You have 1 orchestration message') ) await vi.advanceTimersByTimeAsync(600) - expect(message.delivered_at).not.toBeNull() + expect(message.delivered_at).toBeNull() db.close() } finally { vi.useRealTimers() @@ -33738,10 +33889,10 @@ describe('OrcaRuntimeService', () => { runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 200) expect(write).toHaveBeenCalledWith( 'pty-1', - expect.stringContaining('Subject: after same id respawn') + expect.stringContaining('You have 1 orchestration message') ) await vi.advanceTimersByTimeAsync(600) - expect(message.delivered_at).not.toBeNull() + expect(message.delivered_at).toBeNull() db.close() } finally { vi.useRealTimers() @@ -33787,10 +33938,10 @@ describe('OrcaRuntimeService', () => { await Promise.resolve() expect(write).toHaveBeenCalledWith( 'pty-1', - expect.stringContaining('Subject: unfiltered status') + expect.stringContaining('You have 1 orchestration message') ) await vi.advanceTimersByTimeAsync(600) - expect(message.delivered_at).not.toBeNull() + expect(message.delivered_at).toBeNull() // The filtered waiter stays blocked; the push did not consume its wake. await vi.advanceTimersByTimeAsync(5_000) @@ -33865,10 +34016,10 @@ describe('OrcaRuntimeService', () => { runtime.notifyMessageArrived(terminal.handle, 'status') await Promise.resolve() - const payloadWrites = write.mock.calls.filter( - ([, payload]) => typeof payload === 'string' && payload.includes('Subject: once only') + const pointerWrites = write.mock.calls.filter( + ([, payload]) => typeof payload === 'string' && payload.includes('orca orchestration check') ) - expect(payloadWrites).toHaveLength(1) + expect(pointerWrites).toHaveLength(1) await vi.advanceTimersByTimeAsync(500) const enterWrites = write.mock.calls.filter(([, payload]) => payload === '\r') @@ -33910,9 +34061,10 @@ describe('OrcaRuntimeService', () => { await Promise.resolve() expect( write.mock.calls.filter( - ([, payload]) => typeof payload === 'string' && payload.includes('Subject: second') + ([, payload]) => + typeof payload === 'string' && payload.includes('orca orchestration check') ) - ).toHaveLength(0) + ).toHaveLength(1) expect(second.delivered_at).toBeNull() // Why: release must not require another agent-status OSC — only the @@ -33920,13 +34072,18 @@ describe('OrcaRuntimeService', () => { // timer-only settle (CodeRabbit settling-timeout gap, #12584). await vi.advanceTimersByTimeAsync(3_000) expect(write).toHaveBeenCalledWith('pty-1', '\r') - expect(first.delivered_at).not.toBeNull() + expect(first.delivered_at).toBeNull() expect( write.mock.calls.filter( - ([, payload]) => typeof payload === 'string' && payload.includes('Subject: second') + ([, payload]) => + typeof payload === 'string' && payload.includes('orca orchestration check') ) - ).toHaveLength(1) - expect(second.delivered_at).not.toBeNull() + ).toHaveLength(2) + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 2 orchestration messages') + ) + expect(second.delivered_at).toBeNull() db.close() } finally { vi.useRealTimers() @@ -33953,7 +34110,10 @@ describe('OrcaRuntimeService', () => { runtime.deliverPendingMessagesForHandle(terminal.handle) - expect(write).toHaveBeenCalledWith('pty-1', expect.stringContaining('Subject: after wait')) + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 1 orchestration message') + ) db.close() }) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index ea146b523..7983ebacb 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -134,7 +134,7 @@ import type { OrchestrationWorkerServer } from './orchestration/environment-transport' import { syncFederatedDispatch } from './orchestration/federation-sync' -import { formatMessagesForInjection } from './orchestration/formatter' +import { formatMessagePointer } from './orchestration/formatter' import { selectExactWorkerProviderSession } from './orchestration/worker-provider-session' import type { Automation, @@ -2878,6 +2878,8 @@ export class OrcaRuntimeService { private handles = new Map() private handleByLeafKey = new Map() private handleByPtyId = new Map() + // Why: pointer state is process-local; one harmless replay after restart avoids a wire or schema change. + private readonly lastPointedMessageSequenceByHandle = new Map() private syntheticTerminalHandles = new Set() private detachedPreAllocatedLeaves = new Map() private graphSyncCallbacks: (() => void)[] = [] @@ -10318,7 +10320,7 @@ export class OrcaRuntimeService { // 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)) { - this.deliverPendingMessages(leaf) + this.deliverPendingMessagesForLeaf(leaf) } } return ptyRecordChanged @@ -16793,7 +16795,22 @@ export class OrcaRuntimeService { result.available && recognizeAgentProcess(result.process) !== null ) { - this.ptyTitleTrackersByPtyId.get(ptyId)?.tracker.restoreLastAgentExit() + const restoredStatus = this.ptyTitleTrackersByPtyId + .get(ptyId) + ?.tracker.restoreLastAgentExit() + if (restoredStatus !== null && restoredStatus !== undefined) { + current.lastAgentStatus = restoredStatus + for (const leaf of this.getLeavesForPty(ptyId)) { + if (leaf.lastAgentStatus !== null) { + continue + } + // Why: the foreground agent disproved the neutral title's exit signal; keep runtime delivery state aligned with the restored tracker. + leaf.lastAgentStatus = restoredStatus + if (restoredStatus === 'idle') { + this.deliverPendingMessagesForLeaf(leaf) + } + } + } return } this.recordTerminalSideEffectFact(ptyId, { kind: 'agent-exited' }) @@ -31118,25 +31135,40 @@ export class OrcaRuntimeService { } deliverPendingMessagesForHandle(handle: string, reservedTypes?: ReadonlySet): void { - // Why before the try: `dispatch:`/`run:` mailbox addresses are never terminal - // handles, and federation sync notifies once per relayed item — letting each - // one build and discard a `terminal_handle_stale` Error (stack capture) is - // pure waste. getLiveLeafForHandle would reject them on the same lookup. - if (!this.handles.has(handle)) { - return + let terminalHandle = handle + if (!this.handles.has(terminalHandle)) { + const runId = handle.startsWith('run:') ? handle.slice('run:'.length) : '' + const coordinatorHandle = runId + ? this._orchestrationDb?.getRun(runId)?.coordinator_handle + : null + if (!coordinatorHandle || !this.handles.has(coordinatorHandle)) { + return + } + terminalHandle = coordinatorHandle } try { - const { leaf } = this.getLiveLeafForHandle(handle) + const { leaf } = this.getLiveLeafForHandle(terminalHandle) // Why lastAgentStatusObservedLive: a cold restore seeds `idle` from the // title persisted at snapshot time, so an agent that went busy across the // relaunch still reads idle until its first live frame. Pushing on that - // would type a message plus Enter into a working agent and stamp the row - // delivered. Seeded state waits for a live observation to authorize it. + // would type a message plus Enter into a working agent. Seeded state waits + // for a live observation to authorize it. if (leaf.lastAgentStatus === 'idle' && leaf.lastAgentStatusObservedLive) { - this.deliverPendingMessages(leaf, false, reservedTypes) + this.deliverPendingMessages(leaf, { mailboxHandle: handle, reservedTypes }) } } catch { - // Unknown/stale handles can't be pushed now; the persisted message stays available via explicit check or future idle delivery. + // Unknown/stale handles can't be pointed now; the persisted message stays available via explicit check or future idle delivery. + } + } + + private deliverPendingMessagesForLeaf(leaf: RuntimeLeafRecord): void { + this.deliverPendingMessages(leaf) + if (!this._orchestrationDb) { + return + } + const run = this._orchestrationDb.getCurrentRunForPane?.(`${leaf.tabId}:${leaf.leafId}`) + if (run) { + this.deliverPendingMessages(leaf, { mailboxHandle: `run:${run.id}` }) } } @@ -31147,10 +31179,8 @@ export class OrcaRuntimeService { // deliver now (#12536). deliverPendingMessagesForHandle no-ops when the // leaf is not idle. Main's messageDeliveryFlights serialize mid-Enter // re-notifies without a separate settle barrier. - // Why skip when a waiter will consume this: deliverPendingMessages stamps - // delivered_at but not read, so a blocked orchestration.check --wait would - // still re-read the row and the pane would also receive it (double - // delivery). The pull wins; the push stays pending for a later notify. + // Why skip when a waiter will consume this: a blocked check owns the row, + // so also pointing and submitting the pane would wake it twice. The pull wins. // Why "will consume" and not "exists": a waiter filtered to other types // never returns this row — check re-reads under the same filter on timeout // — so treating it as the consumer strands the message in exactly the @@ -31767,10 +31797,8 @@ export class OrcaRuntimeService { return null } - // Why: delivered_at for Claude targets stamps only in the delayed-Enter - // callback, so the whole write→settle span must be single-flight per pty — - // a second read inside it would re-inject the same unread rows. Triggers - // landing mid-flight park the latest leaf and re-run once on settle. The + // Why: the whole pointer→Enter span must be single-flight per pty. Triggers + // landing mid-flight park their mailbox and re-run once on settle. The // flight object is the settle identity: a stale settle surviving an exit // retire must not clear a newer same-id flight or flush its parked trigger. private readonly messageDeliveryFlightsByPtyId = new Map< @@ -31778,7 +31806,10 @@ export class OrcaRuntimeService { { enterTimer: ReturnType | null } >() - private readonly parkedMessageRedeliveryLeavesByPtyId = new Map() + private readonly parkedMessageRedeliveriesByPtyId = new Map< + string, + Map }> + >() private settlePendingMessageDelivery( ptyId: string, @@ -31788,31 +31819,47 @@ export class OrcaRuntimeService { return } this.messageDeliveryFlightsByPtyId.delete(ptyId) - const parkedLeaf = this.parkedMessageRedeliveryLeavesByPtyId.get(ptyId) - if (!parkedLeaf) { + const parked = this.parkedMessageRedeliveriesByPtyId.get(ptyId) + if (!parked) { return } - this.parkedMessageRedeliveryLeavesByPtyId.delete(ptyId) - this.deliverPendingMessages(parkedLeaf) + this.parkedMessageRedeliveriesByPtyId.delete(ptyId) + for (const [mailboxHandle, delivery] of parked) { + this.deliverPendingMessages(delivery.leaf, { + mailboxHandle, + reservedTypes: delivery.reservedTypes + }) + } } - // Why: an Enter armed for a dead session must not fire into a same-id cold - // restore — it would inject \r and stamp rows the replacement never saw. - // Retire without stamping; the rows re-deliver on the replacement's next idle. + // Why: a dead session's Enter or watermark must not affect a same-id cold restore. private retirePendingMessageDeliveryForPty(ptyId: string): void { const flight = this.messageDeliveryFlightsByPtyId.get(ptyId) if (flight?.enterTimer != null) { clearTimeout(flight.enterTimer) } this.messageDeliveryFlightsByPtyId.delete(ptyId) - this.parkedMessageRedeliveryLeavesByPtyId.delete(ptyId) + this.parkedMessageRedeliveriesByPtyId.delete(ptyId) + for (const leaf of this.getLeavesForPty(ptyId)) { + const handle = this.handleByLeafKey.get(this.getLeafKey(leaf.tabId, leaf.leafId)) + if (handle) { + this.lastPointedMessageSequenceByHandle.delete(handle) + } + const run = this._orchestrationDb?.getCurrentRunForPane?.(`${leaf.tabId}:${leaf.leafId}`) + if (run) { + this.lastPointedMessageSequenceByHandle.delete(`run:${run.id}`) + } + } } // Why: push-on-idle delivery is event-driven (no polling) because the runtime owns both the message store and terminal status detection. private deliverPendingMessages( leaf: RuntimeLeafRecord, - skipAbsenceProbe = false, - reservedTypes?: ReadonlySet + options: { + mailboxHandle?: string + reservedTypes?: ReadonlySet + skipAbsenceProbe?: boolean + } = {} ): void { if (!this._orchestrationDb) { return @@ -31822,10 +31869,20 @@ export class OrcaRuntimeService { if (!handle) { return } + const mailboxHandle = options.mailboxHandle ?? handle - // Why before reading rows: rows read mid-flight are the not-yet-stamped ones. if (leaf.ptyId && this.messageDeliveryFlightsByPtyId.has(leaf.ptyId)) { - this.parkedMessageRedeliveryLeavesByPtyId.set(leaf.ptyId, leaf) + let parked = this.parkedMessageRedeliveriesByPtyId.get(leaf.ptyId) + if (!parked) { + parked = new Map() + this.parkedMessageRedeliveriesByPtyId.set(leaf.ptyId, parked) + } + const priorReservedTypes = parked.get(mailboxHandle)?.reservedTypes + const reservedTypes = + priorReservedTypes || options.reservedTypes + ? new Set([...(priorReservedTypes ?? []), ...(options.reservedTypes ?? [])]) + : undefined + parked.set(mailboxHandle, { leaf, reservedTypes }) return } @@ -31834,12 +31891,13 @@ export class OrcaRuntimeService { // into the pane AND returned by that pull's check. Live waiters cover the // still-blocked case; reservedTypes carries the notify-time snapshot for a // waiter resolved later in the same drain, which is already gone from the map. - const waiters = this.messageWaitersByHandle.get(handle) + const waiters = this.messageWaitersByHandle.get(mailboxHandle) const unread = this._orchestrationDb - .getUndeliveredUnreadMessages(handle) + .getUndeliveredUnreadMessages(mailboxHandle) .filter( (message) => - !reservedTypes?.has(message.type) && !messageTypeHasLiveWaiter(waiters, message.type) + !options.reservedTypes?.has(message.type) && + !messageTypeHasLiveWaiter(waiters, message.type) ) if (unread.length === 0) { return @@ -31848,9 +31906,16 @@ export class OrcaRuntimeService { if (!leaf.writable || !leaf.ptyId) { return } + const newestSequence = unread.at(-1)?.sequence + if ( + newestSequence === undefined || + newestSequence <= (this.lastPointedMessageSequenceByHandle.get(mailboxHandle) ?? -1) + ) { + return + } if ( - !skipAbsenceProbe && + !options.skipAbsenceProbe && this.ptyController?.probePtyLiveness && !this.controllerKnowsPtyIsLive(leaf.ptyId) ) { @@ -31859,8 +31924,7 @@ export class OrcaRuntimeService { // them queued for a future surface; unknown liveness still delivers. const probedPtyId = leaf.ptyId // Why: triggers arriving mid-probe must not each arm a continuation — the - // Claude Enter delay stamps delivered_at late, so every continuation would - // re-read the same unread rows and double-deliver. The single armed + // Every continuation would re-read the same unread rows. The single armed // continuation re-reads fresh rows when it fires, so nothing is lost. if (this.probeDeferredDeliveryPtyIds.has(probedPtyId)) { return @@ -31882,16 +31946,18 @@ export class OrcaRuntimeService { // Why current state, not the closure: the gate that authorized this // push ran before the probe. A same-id cold restore inside the probe // window keeps ptyId identical and makes the leaf writable again, so - // an id-only check would type the payload plus Enter into a process - // whose idle was never observed — and stamp the row delivered, which - // loses it. Re-read the leaf and re-apply the live-idle gate. + // an id-only check would type the pointer plus Enter into a process + // whose idle was never observed. Re-read the live-idle gate. const currentLeaf = this.leaves.get(this.getLeafKey(leaf.tabId, leaf.leafId)) if ( currentLeaf?.ptyId === probedPtyId && currentLeaf.lastAgentStatus === 'idle' && currentLeaf.lastAgentStatusObservedLive ) { - this.deliverPendingMessages(currentLeaf, true) + this.deliverPendingMessages(currentLeaf, { + mailboxHandle, + skipAbsenceProbe: true + }) } }, 0) } @@ -31905,32 +31971,25 @@ export class OrcaRuntimeService { const deliveryPtyId = leaf.ptyId const flight: { enterTimer: ReturnType | null } = { enterTimer: null } this.messageDeliveryFlightsByPtyId.set(deliveryPtyId, flight) - // Why: every sync outcome — failed write, sync-stamped branch, or a throw — + // Why: every sync outcome — failed write, Cursor branch, or a throw — // must end the flight here, or a leaked flag parks this pty's deliveries // forever. Only an armed Enter hands settling to its own callback. let settlesInEnterCallback = false try { - const payload = formatMessagesForInjection(unread) + const payload = formatMessagePointer(unread.length) const wrote = this.ptyController?.write(deliveryPtyId, payload) ?? false if (!wrote) { return } - - // The active coordinator prompt is user-owned input, so push-on-idle must not synthesize Enter. - if (this._orchestrationDb.getActiveCoordinatorRun()?.coordinator_handle === handle) { - this._orchestrationDb.markAsDelivered(unread.map((m) => m.id)) - return - } + this.lastPointedMessageSequenceByHandle.set(mailboxHandle, newestSequence) const tabTitle = this.tabs.get(leaf.tabId)?.title if (isCursorAgentOrchestrationTarget(leaf, tabTitle)) { // Why: Cursor Agent treats injected PTY text as editable prompt input, so submitting must stay under user control. - this._orchestrationDb.markAsDelivered(unread.map((m) => m.id)) return } - // Why: Claude Code treats a large PTY write as a paste and swallows a \r in the same write; send Enter separately after a delay, stamping delivered_at only once \r is confirmed. - // Important (design doc §3.2, feedback #2): stamp delivered_at, not read — read means "a check-caller consumed this"; flipping it would hide the message from check --unread. + // Why: agent TUIs can swallow a \r in the same PTY write; submit separately after a delay. flight.enterTimer = setTimeout(() => { try { // Why current state, not the closure: graph resync replaces leaf @@ -31943,12 +32002,9 @@ export class OrcaRuntimeService { if (!currentLeaf || currentLeaf.ptyId !== deliveryPtyId || !currentLeaf.writable) { return } - const submitted = this.ptyController?.write(deliveryPtyId, '\r') ?? false - if (submitted) { - this._orchestrationDb?.markAsDelivered(unread.map((m) => m.id)) - } + this.ptyController?.write(deliveryPtyId, '\r') } catch { - // Terminal may have closed during the delay — messages stay queued (delivered_at NULL) and re-deliver on next idle. + // Terminal may have closed during the delay; mail remains queued for check. } finally { // Why finally: every outcome — submit, refusal, throw — ends the flight, // and settle re-runs any trigger parked during it so nothing strands. diff --git a/src/main/runtime/orchestration/formatter.test.ts b/src/main/runtime/orchestration/formatter.test.ts index b4732f04a..f7d9c14d8 100644 --- a/src/main/runtime/orchestration/formatter.test.ts +++ b/src/main/runtime/orchestration/formatter.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { formatMessageBanner, formatMessagesForInjection } from './formatter' +import { formatMessageBanner, formatMessagePointer, formatMessagesForInjection } from './formatter' import type { MessageRow } from './types' function makeMessage(overrides: Partial = {}): MessageRow { @@ -183,3 +183,15 @@ describe('formatMessagesForInjection', () => { expect(result).toContain(`${bannerA}\n\n${bannerB}`) }) }) + +describe('formatMessagePointer', () => { + it('formats a singular pointer without message content', () => { + expect(formatMessagePointer(1)).toBe( + '\nYou have 1 orchestration message. Run `orca orchestration check`.\n' + ) + }) + + it('pluralizes a batched pointer', () => { + expect(formatMessagePointer(3)).toContain('3 orchestration messages') + }) +}) diff --git a/src/main/runtime/orchestration/formatter.ts b/src/main/runtime/orchestration/formatter.ts index 269ba2930..678c5e084 100644 --- a/src/main/runtime/orchestration/formatter.ts +++ b/src/main/runtime/orchestration/formatter.ts @@ -107,3 +107,8 @@ export function formatMessagesForInjection(messages: MessageRow[]): string { const banners = messages.map(formatMessageBanner).join('\n\n') return `\n--- Orchestration Messages (${messages.length}) ---\n${banners}\n---\n` } + +export function formatMessagePointer(count: number): string { + const noun = count === 1 ? 'message' : 'messages' + return `\nYou have ${count} orchestration ${noun}. Run \`orca orchestration check\`.\n` +} diff --git a/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts b/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts index 823c04e5a..41e272ecd 100644 --- a/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts +++ b/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts @@ -267,6 +267,7 @@ function makeOrchestrationDbStub(toHandle: () => string) { getUndeliveredUnreadMessages: (handle: string) => rows.filter((row) => row.to_handle === handle && row.read === 0 && !row.delivered_at), getActiveCoordinatorRun: () => null, + getCurrentRunForPane: () => undefined, // Consulted by onPtyExit's dispatch-failure path. getActiveDispatchForTerminal: () => null, markAsDelivered, @@ -327,7 +328,7 @@ describe('push-on-idle orchestration delivery absence gate', () => { expect(write).toHaveBeenCalledWith( STALE_PTY_ID, - expect.stringContaining('Subject: for the old session') + expect.stringContaining('You have 1 orchestration message') ) }) @@ -369,7 +370,10 @@ describe('push-on-idle orchestration delivery absence gate', () => { const payloads = write.mock.calls .map(([, data]) => data) .filter((data): data is string => typeof data === 'string') - expect(payloads.some((data) => data.includes('Subject: unclaimed status'))).toBe(true) + const pointers = payloads.filter((data) => data.includes('orca orchestration check')) + expect(pointers).toHaveLength(1) + expect(pointers[0]).toContain('You have 1 orchestration message') + expect(payloads.some((data) => data.includes('unclaimed status'))).toBe(false) expect(payloads.some((data) => data.includes('Subject: worker completion'))).toBe(false) }) @@ -411,8 +415,11 @@ describe('push-on-idle orchestration delivery absence gate', () => { const payloads = write.mock.calls .map(([, data]) => data) .filter((data): data is string => typeof data === 'string') - expect(payloads.some((data) => data.includes('Subject: unclaimed status'))).toBe(true) - expect(payloads.some((data) => data.includes('Subject: late completion'))).toBe(true) + const pointers = payloads.filter((data) => data.includes('orca orchestration check')) + expect(pointers).toHaveLength(1) + expect(pointers[0]).toContain('You have 2 orchestration messages') + expect(payloads.some((data) => data.includes('unclaimed status'))).toBe(false) + expect(payloads.some((data) => data.includes('late completion'))).toBe(false) }) it('keeps messages queued instead of marking a proven-absent pty delivered', async () => { @@ -440,12 +447,14 @@ describe('push-on-idle orchestration delivery absence gate', () => { // Why twice: the probe continuation yields a turn before delivering. await new Promise((resolve) => setTimeout(resolve, 0)) - expect(write).toHaveBeenCalledWith(STALE_PTY_ID, expect.stringContaining('Subject: hello')) + expect(write).toHaveBeenCalledWith( + STALE_PTY_ID, + expect.stringContaining('You have 1 orchestration message') + ) }) - // Why: delivered_at stamps only in the delayed-Enter callback, so the whole - // write→settle span — not just the probe — must be single-flight; a trigger - // landing inside the 500ms window would re-read the same un-stamped rows. + // Why: the whole pointer→Enter span must be single-flight; a trigger inside + // the 500ms window must park until the sequence watermark advances. it('delivers once across concurrent probe triggers and an in-window re-trigger, then flushes parked rows', async () => { vi.useFakeTimers() try { @@ -464,36 +473,33 @@ describe('push-on-idle orchestration delivery absence gate', () => { resolveProbe(null) await vi.advanceTimersByTimeAsync(0) - const firstSubjectWrites = () => + const pointerWrites = () => write.mock.calls.filter( - ([, data]) => typeof data === 'string' && data.includes('Subject: exactly once') + ([, data]) => typeof data === 'string' && data.includes('orca orchestration check') ) - expect(firstSubjectWrites()).toHaveLength(1) + expect(pointerWrites()).toHaveLength(1) + expect(pointerWrites()[0]?.[1]).toContain('You have 1 orchestration message') - // Re-trigger INSIDE the 500ms Enter window: the first batch is written but - // not yet stamped, so a fresh probe cycle would re-inject it. (On the - // fixed code no new probe is armed — the trigger parks; resolveProbe then - // re-resolves the settled first probe, a no-op.) + // Re-trigger inside the Enter window parks; resolving the settled first + // probe again is a no-op. stub.insert('second message') runtime.deliverPendingMessagesForHandle(handle) resolveProbe(null) await vi.advanceTimersByTimeAsync(0) - expect(firstSubjectWrites()).toHaveLength(1) + expect(pointerWrites()).toHaveLength(1) - // Enter fires, delivered_at stamps, the flight settles, and the parked - // trigger re-runs on its own — arming a fresh probe for the new row. + // Enter settles the flight and re-runs the parked trigger, arming a fresh + // probe for the newer sequence. await vi.advanceTimersByTimeAsync(500) resolveProbe(null) await vi.advanceTimersByTimeAsync(0) - const secondSubjectWrites = write.mock.calls.filter( - ([, data]) => typeof data === 'string' && data.includes('Subject: second message') - ) - expect(secondSubjectWrites).toHaveLength(1) - expect(firstSubjectWrites()).toHaveLength(1) + expect(pointerWrites()).toHaveLength(2) + expect(pointerWrites()[1]?.[1]).toContain('You have 2 orchestration messages') await vi.advanceTimersByTimeAsync(500) - expect(stub.rows.every((row) => row.delivered_at !== null)).toBe(true) + expect(stub.markAsDelivered).not.toHaveBeenCalled() + expect(stub.rows.every((row) => row.delivered_at === null)).toBe(true) } finally { vi.useRealTimers() } @@ -514,35 +520,30 @@ describe('push-on-idle orchestration delivery absence gate', () => { stub.insert('second') runtime.deliverPendingMessagesForHandle(handle) - const firstSubjectWrites = () => + const pointerWrites = () => write.mock.calls.filter( - ([, data]) => typeof data === 'string' && data.includes('Subject: first') + ([, data]) => typeof data === 'string' && data.includes('orca orchestration check') ) - expect(firstSubjectWrites()).toHaveLength(1) + expect(pointerWrites()).toHaveLength(1) + expect(pointerWrites()[0]?.[1]).toContain('You have 1 orchestration message') expect(probe).not.toHaveBeenCalled() - // Settle flushes the parked trigger; the second row delivers alone — - // its batch must not re-contain the already-stamped first row. + // Settle flushes the parked trigger; both still-pending rows are counted, + // while the newer sequence authorizes exactly one fresh pointer. await vi.advanceTimersByTimeAsync(500) - const secondOnlyWrites = write.mock.calls.filter( - ([, data]) => - typeof data === 'string' && - data.includes('Subject: second') && - !data.includes('Subject: first') - ) - expect(secondOnlyWrites).toHaveLength(1) - expect(firstSubjectWrites()).toHaveLength(1) + expect(pointerWrites()).toHaveLength(2) + expect(pointerWrites()[1]?.[1]).toContain('You have 2 orchestration messages') await vi.advanceTimersByTimeAsync(500) - expect(stub.rows.every((row) => row.delivered_at !== null)).toBe(true) + expect(stub.markAsDelivered).not.toHaveBeenCalled() + expect(stub.rows.every((row) => row.delivered_at === null)).toBe(true) } finally { vi.useRealTimers() } }) // Why: cold restore respawns under the SAME session id. An Enter armed for - // the dead incarnation must not fire into the replacement — it would inject - // \r and stamp rows the new session never received. + // the dead incarnation must not submit stale input into the replacement. it('retires an armed Enter when the pty exits and respawns under the same id inside the window', async () => { vi.useFakeTimers() try { @@ -569,18 +570,19 @@ describe('push-on-idle orchestration delivery absence gate', () => { runtime.deliverPendingMessagesForHandle(handle) expect( write.mock.calls.filter( - ([, data]) => typeof data === 'string' && data.includes('Subject: for the old session') + ([, data]) => typeof data === 'string' && data.includes('orca orchestration check') ) ).toHaveLength(1) runtime.onPtyData(STALE_PTY_ID, '\x1b]0;Codex working\x07', 200) runtime.onPtyData(STALE_PTY_ID, '\x1b]0;Codex done\x07', 201) const payloadWrites = write.mock.calls.filter( - ([, data]) => typeof data === 'string' && data.includes('Subject: for the old session') + ([, data]) => typeof data === 'string' && data.includes('orca orchestration check') ) expect(payloadWrites).toHaveLength(2) await vi.advanceTimersByTimeAsync(500) expect(write.mock.calls.filter(([, data]) => data === '\r')).toHaveLength(1) - expect(stub.rows[0].delivered_at).not.toBeNull() + expect(stub.markAsDelivered).not.toHaveBeenCalled() + expect(stub.rows[0].delivered_at).toBeNull() } finally { vi.useRealTimers() } @@ -595,18 +597,21 @@ describe('push-on-idle orchestration delivery absence gate', () => { }) const internals = runtime as unknown as { messageDeliveryFlightsByPtyId: Map - parkedMessageRedeliveryLeavesByPtyId: Map + parkedMessageRedeliveriesByPtyId: Map + lastPointedMessageSequenceByHandle: Map } stub.insert('first') runtime.deliverPendingMessagesForHandle(handle) stub.insert('second') runtime.deliverPendingMessagesForHandle(handle) expect(internals.messageDeliveryFlightsByPtyId.size).toBe(1) - expect(internals.parkedMessageRedeliveryLeavesByPtyId.size).toBe(1) + expect(internals.parkedMessageRedeliveriesByPtyId.size).toBe(1) + expect(internals.lastPointedMessageSequenceByHandle.size).toBe(1) runtime.onPtyExit(STALE_PTY_ID, 0) expect(internals.messageDeliveryFlightsByPtyId.size).toBe(0) - expect(internals.parkedMessageRedeliveryLeavesByPtyId.size).toBe(0) + expect(internals.parkedMessageRedeliveriesByPtyId.size).toBe(0) + expect(internals.lastPointedMessageSequenceByHandle.size).toBe(0) await vi.advanceTimersByTimeAsync(500) expect(write.mock.calls.filter(([, data]) => data === '\r')).toHaveLength(0) diff --git a/src/shared/agent-title-status.ts b/src/shared/agent-title-status.ts index 68c9dd463..09ffc07a6 100644 --- a/src/shared/agent-title-status.ts +++ b/src/shared/agent-title-status.ts @@ -56,7 +56,7 @@ export function createAgentStatusTracker( ): { handleTitle: (title: string) => void seedTitle: (title: string) => void - restoreLastExit: () => void + restoreLastExit: () => AgentStatus | null reset: () => void } { // Why: trackers restored mid-session need a last-known status without firing @@ -92,11 +92,13 @@ export function createAgentStatusTracker( lastStatus = detectAgentStatusFromTitle(title) restorableExitStatus = null }, - restoreLastExit(): void { - if (lastStatus === null && restorableExitStatus !== null) { - lastStatus = restorableExitStatus + restoreLastExit(): AgentStatus | null { + const restoredStatus = lastStatus === null ? restorableExitStatus : null + if (restoredStatus !== null) { + lastStatus = restoredStatus } restorableExitStatus = null + return restoredStatus }, reset(): void { lastStatus = null diff --git a/src/shared/terminal-output-side-effects.ts b/src/shared/terminal-output-side-effects.ts index 227df5bd4..32dc33724 100644 --- a/src/shared/terminal-output-side-effects.ts +++ b/src/shared/terminal-output-side-effects.ts @@ -5,6 +5,7 @@ */ import { + type AgentStatus, clearWorkingIndicators, createAgentStatusTracker, detectAgentStatusFromTitle, @@ -91,7 +92,7 @@ export type TerminalTitleTracker = { */ seedInitialTitle: (rawTitle: string) => void /** Restore the status consumed by the latest exit candidate when process evidence disproves it. */ - restoreLastAgentExit: () => void + restoreLastAgentExit: () => AgentStatus | null /** Last title surfaced through onTitle, after normalization. */ getLastNormalizedTitle: () => string | null /** @@ -264,8 +265,8 @@ export function createTerminalTitleTracker( lastEmittedTitle = normalizeTerminalTitle(rawTitle) agentTracker?.seedTitle(rawTitle) }, - restoreLastAgentExit(): void { - agentTracker?.restoreLastExit() + restoreLastAgentExit(): AgentStatus | null { + return agentTracker?.restoreLastExit() ?? null }, getLastNormalizedTitle: () => lastEmittedTitle, setTransientFactScanningSuppressed(suppressed: boolean): void { diff --git a/tests/e2e/helpers/orchestration-mail-pane-agent.ts b/tests/e2e/helpers/orchestration-mail-pane-agent.ts index f948bfa8c..df9d0311d 100644 --- a/tests/e2e/helpers/orchestration-mail-pane-agent.ts +++ b/tests/e2e/helpers/orchestration-mail-pane-agent.ts @@ -6,7 +6,7 @@ * writing into the pane's foreground process. A shell echoes rather than * records, so it can prove the gate but never the payload. This process owns * both sides — the test drives its title through a control file and it appends - * every stdin chunk to a ledger, which is what makes "the banner and the Enter + * every stdin chunk to a ledger, which is what makes "the pointer and the Enter * reached the agent" an assertion instead of an inference. * * Titles come from a polled file, not stdin, because orchestration writes to @@ -54,12 +54,12 @@ log({ event: 'start' }) // Raw mode is what every agent TUI does, and it is load-bearing here: a cooked // PTY applies ICRNL, so the synthesized Enter would arrive as \\n and be -// indistinguishable from the banner's own newlines. +// indistinguishable from the pointer's own newlines. if (process.stdin.isTTY) { process.stdin.setRawMode(true) } -// Every byte orchestration pushes lands here — banner text and Enter alike. +// Every byte orchestration pushes lands here — pointer text and Enter alike. process.stdin.on('data', (chunk) => log({ event: 'stdin', data: chunk.toString() })) process.stdin.resume() diff --git a/tests/e2e/helpers/orchestration-mail-store.ts b/tests/e2e/helpers/orchestration-mail-store.ts index 520498dad..7232c8d6a 100644 --- a/tests/e2e/helpers/orchestration-mail-store.ts +++ b/tests/e2e/helpers/orchestration-mail-store.ts @@ -3,9 +3,8 @@ * * Why read SQLite instead of `orchestration.check`: check is itself a consumer — * it marks rows read and backfills `delivered_at` — so using it to observe would - * destroy the very distinction these specs exist to test. The two markers are - * independent on purpose: `delivered_at` means a push typed the row into a pane, - * `read` means a pull consumed it. Only an out-of-band read can tell them apart. + * destroy the very distinction these specs exist to test. A pointer changes + * neither marker; only an out-of-band read can prove that before check consumes. */ import path from 'node:path' import Database from '../../../src/main/sqlite/sync-database' @@ -49,13 +48,10 @@ export function readMailbox(userDataDir: string, toHandle: string): MailRow[] { } /** - * Mark `handle` as the running coordinator — the state that makes push delivery - * withhold the synthesized Enter, because that prompt holds user-typed input. + * Mark `handle` as a legacy running coordinator to prove it no longer suppresses Enter. * - * Why seed the row instead of calling `orchestration.run`: that RPC also starts - * a live coordinator loop which dispatches workers on a timer, and its - * scheduling would race every assertion here. The carve-out reads nothing but - * this row. + * Why seed instead of calling `orchestration.run`: that RPC starts a coordinator + * loop whose scheduling would race the assertion. */ export function startCoordinatorRun(userDataDir: string, handle: string): void { withMailDb(userDataDir, (db) => { @@ -67,7 +63,7 @@ export function startCoordinatorRun(userDataDir: string, handle: string): void { } /** - * How a row was consumed, if at all. + * How a row was consumed under either the current or historical push behavior. * * `read` is checked first because a pull backfills `delivered_at` via COALESCE, * so a pulled row also carries a delivery stamp — the stamp alone cannot prove diff --git a/tests/e2e/orchestration-idle-mail-delivery.spec.ts b/tests/e2e/orchestration-idle-mail-delivery.spec.ts index 2d1ec41b6..fcfe81005 100644 --- a/tests/e2e/orchestration-idle-mail-delivery.spec.ts +++ b/tests/e2e/orchestration-idle-mail-delivery.spec.ts @@ -9,7 +9,7 @@ * * These specs drive real PTYs: the recipient is a fake `codex` on PATH whose OSC * titles the test controls through a file, and which appends every stdin chunk - * to a ledger. That ledger is the oracle — it proves the banner and the + * to a ledger. That ledger is the oracle — it proves the pointer and the * synthesized Enter reached the agent process, which no store or DB read can. * * The ordering fixes on this path (microtask deferral, probe-window respawn, @@ -19,6 +19,7 @@ */ import { test, expect } from './helpers/orca-app' import type { ElectronApplication, Page } from '@stablyai/playwright-test' +import { randomUUID } from 'node:crypto' import { waitForSessionReady, waitForActiveWorktree, ensureTerminalVisible } from './helpers/store' import { execInTerminal, @@ -37,16 +38,16 @@ import { } from './helpers/orchestration-mail-pane-agent' import { mailDisposition, + readMailbox, readMailRow, startCoordinatorRun } from './helpers/orchestration-mail-store' import { waitForPtyShellEcho } from './terminal-pty-readiness' -/** The wrapper `formatMessagesForInjection` puts around every pushed batch. */ -const BANNER_PREFIX = '--- Orchestration Messages' +const POINTER_COMMAND = 'orca orchestration check' // Why generous: the push runs a microtask behind the send, may defer once more -// behind a liveness probe, and only stamps delivered_at after a 500ms Enter. +// behind a liveness probe, and submits Enter after a 500ms delay. const DELIVERY_TIMEOUT_MS = 20_000 // Why 3s: long enough to cover that same chain, so "still pending" means the // gate refused rather than that the push had not run yet. @@ -175,18 +176,19 @@ async function sendMail( return sent.result.message.id } -async function expectPushed(pane: AgentPane, subject: string): Promise { +async function expectPointed(pane: AgentPane, count = 1): Promise { await expect .poll(() => pane.agent.readStdin(), { timeout: DELIVERY_TIMEOUT_MS, - message: 'banner never reached the agent process' + message: 'mail pointer never reached the agent process' }) - .toContain(BANNER_PREFIX) - expect(pane.agent.readStdin()).toContain(`Subject: ${subject}`) + .toContain(POINTER_COMMAND) + const noun = count === 1 ? 'message' : 'messages' + expect(pane.agent.readStdin()).toContain(`${count} orchestration ${noun}`) } /** - * The synthesized Enter is a separate write ~500ms after the banner. The banner + * The synthesized Enter is a separate write ~500ms after the pointer. The pointer * itself is `\n`-joined, so a `\r` anywhere in stdin can only be that submit — * which keeps the assertion independent of how the PTY chunks the two writes. */ @@ -219,7 +221,7 @@ async function expectStaysPending( expect(readMailRow(userDataDir, messageId)).toBeDefined() await page.waitForTimeout(NO_DELIVERY_SETTLE_MS) expect(mailDisposition(readMailRow(userDataDir, messageId))).toBe('pending') - expect(pane.agent.readStdin()).not.toContain(BANNER_PREFIX) + expect(pane.agent.readStdin()).not.toContain(POINTER_COMMAND) } test.describe('orchestration push-on-idle mail delivery', () => { @@ -237,13 +239,13 @@ test.describe('orchestration push-on-idle mail delivery', () => { const subject = 'Already idle delivery' const messageId = await sendMail(client, pane.handle, { subject }) - await expectPushed(pane, subject) + await expectPointed(pane) await expectSubmitted(pane) await expect .poll(() => mailDisposition(readMailRow(userDataDir, messageId)), { timeout: DELIVERY_TIMEOUT_MS }) - .toBe('pushed') + .toBe('pending') }) test('holds mail while the agent is working and releases it on the idle frame', async ({ @@ -263,12 +265,12 @@ test.describe('orchestration push-on-idle mail delivery', () => { // Releasing the gate proves the silence above was the working status and not // a harness that never wired the send to this pane at all. pane.agent.setTitle(CODEX_IDLE_TITLE) - await expectPushed(pane, subject) + await expectPointed(pane) await expect .poll(() => mailDisposition(readMailRow(userDataDir, messageId)), { timeout: DELIVERY_TIMEOUT_MS }) - .toBe('pushed') + .toBe('pending') }) // Guards the null→idle path rather than reproducing #12536: a fresh pane has @@ -291,7 +293,7 @@ test.describe('orchestration push-on-idle mail delivery', () => { // Idle is this pane's FIRST live status, so there is no busy→idle edge here // either; delivery has to hang off the liveness of the observation. pane.agent.setTitle(CODEX_IDLE_TITLE) - await expectPushed(pane, subject) + await expectPointed(pane) await expectSubmitted(pane) }) @@ -325,7 +327,7 @@ test.describe('orchestration push-on-idle mail delivery', () => { const pulled = await waiting expect(pulled.result.messages.map((message) => message.subject)).toContain(subject) - expect(pane.agent.readStdin()).not.toContain(BANNER_PREFIX) + expect(pane.agent.readStdin()).not.toContain(POINTER_COMMAND) // Pending, not pushed: the pull won, and the push stays available for a // later notify rather than racing this one. expect(mailDisposition(readMailRow(userDataDir, messageId))).toBe('pending') @@ -355,16 +357,95 @@ test.describe('orchestration push-on-idle mail delivery', () => { const subject = 'Filtered waiter' const messageId = await sendMail(client, pane.handle, { subject, type: 'status' }) - await expectPushed(pane, subject) + await expectPointed(pane) await expect .poll(() => mailDisposition(readMailRow(userDataDir, messageId)), { timeout: DELIVERY_TIMEOUT_MS }) - .toBe('pushed') + .toBe('pending') await waiting }) - test('writes the banner but never Enter for the active coordinator pane', async ({ + test('worker completion points and wakes its idle Run coordinator without consuming mail', async ({ + orcaPage, + electronApp + }) => { + test.setTimeout(180_000) + const { client, userDataDir, openAgentPane } = await setUpMailFixture(orcaPage, electronApp) + const pane = await openAgentPane() + await driveToLiveIdle(client, pane) + + const run = await client.call<{ run: { id: string } }>('orchestration.runCreate', { + objective: 'Verify worker completion pointer delivery', + from: pane.handle + }) + const task = await client.call<{ task: { id: string } }>('orchestration.taskCreate', { + spec: 'Report one P3 finding', + run: run.result.run.id, + callerTerminalHandle: pane.handle + }) + const dispatched = await client.call<{ dispatch: { id: string } }>('orchestration.dispatch', { + task: task.result.task.id, + run: run.result.run.id, + from: pane.handle, + to: pane.handle + }) + const body = 'full private review finding must remain in SQLite' + const payload = JSON.stringify({ + taskId: task.result.task.id, + dispatchId: dispatched.result.dispatch.id, + outcome: 'succeeded' + }) + const sendParams = { + from: pane.handle, + to: pane.handle, + subject: 'review: one P3 finding', + body, + type: 'worker_done', + payload + } + const orchestrationRequestId = randomUUID() + const sent = await client.call<{ message: { id: string; to_handle: string } }>( + 'orchestration.send', + sendParams, + { orchestrationRequestId } + ) + const runAddress = `run:${run.result.run.id}` + expect(sent.result.message.to_handle).toBe(runAddress) + + await expectPointed(pane) + await expectSubmitted(pane) + expect(pane.agent.readStdin()).not.toContain(body) + const pointedRow = readMailRow(userDataDir, sent.result.message.id) + expect(pointedRow).toMatchObject({ to_handle: runAddress, read: 0, delivered_at: null }) + + const stdinAfterFirstPointer = pane.agent.readStdin() + const duplicate = await client.call<{ message: { id: string } }>( + 'orchestration.send', + sendParams, + { orchestrationRequestId } + ) + pane.agent.setTitle(CODEX_WORKING_TITLE) + await waitForObservedTitle(client, pane.handle, CODEX_WORKING_TITLE) + pane.agent.setTitle(CODEX_IDLE_TITLE) + await waitForObservedTitle(client, pane.handle, CODEX_IDLE_TITLE) + await orcaPage.waitForTimeout(NO_DELIVERY_SETTLE_MS) + expect(pane.agent.readStdin()).toBe(stdinAfterFirstPointer) + expect(duplicate.result.message.id).toBe(sent.result.message.id) + expect(readMailbox(userDataDir, runAddress).filter((row) => row.read === 0)).toEqual([ + expect.objectContaining({ id: sent.result.message.id }) + ]) + + const checked = await client.call<{ messages: { id: string; body: string }[] }>( + 'orchestration.check', + { terminal: pane.handle, run: run.result.run.id } + ) + expect(checked.result.messages).toEqual([ + expect.objectContaining({ id: sent.result.message.id, body }) + ]) + }) + + test('writes and submits the pointer for the active coordinator pane', async ({ orcaPage, electronApp }) => { @@ -374,17 +455,14 @@ test.describe('orchestration push-on-idle mail delivery', () => { await driveToLiveIdle(client, pane) startCoordinatorRun(userDataDir, pane.handle) - // The coordinator prompt is user-owned input; synthesizing Enter there would - // submit whatever the human was mid-way through typing (#7337). - const subject = 'Coordinator no-submit' + const subject = 'Coordinator pointer submit' await sendMail(client, pane.handle, { subject }) - await expectPushed(pane, subject) - await orcaPage.waitForTimeout(2_000) - expectNotSubmitted(pane) + await expectPointed(pane) + await expectSubmitted(pane) }) - test('writes the banner but never Enter for a Cursor agent pane', async ({ + test('writes the pointer but never Enter for a Cursor agent pane', async ({ orcaPage, electronApp }) => { @@ -399,7 +477,7 @@ test.describe('orchestration push-on-idle mail delivery', () => { const subject = 'Cursor no-submit' await sendMail(client, pane.handle, { subject }) - await expectPushed(pane, subject) + await expectPointed(pane) await orcaPage.waitForTimeout(2_000) expectNotSubmitted(pane) }) diff --git a/tests/e2e/orchestration-idle-mail-restore.spec.ts b/tests/e2e/orchestration-idle-mail-restore.spec.ts index 6e38b9fd8..4e528614f 100644 --- a/tests/e2e/orchestration-idle-mail-restore.spec.ts +++ b/tests/e2e/orchestration-idle-mail-restore.spec.ts @@ -1,12 +1,12 @@ /** * Mail must survive a restart: never injected on restored state alone, always - * delivered once the agent speaks again (#12536). + * pointed once the agent speaks again (#12536). * * Push-on-idle now fires when mail arrives rather than only on a busy→idle edge, * which puts restart squarely on the delivery path — a pane comes back carrying * the title it had at snapshot time, and anything the runtime infers from that * is a memory, not an observation. Typing on it would submit into an agent that - * may be mid-turn and stamp the row delivered, losing it. + * may be mid-turn. * * Scope, stated plainly: this covers the restart path, not the * `lastAgentStatusObservedLive` gate itself. The seed only reaches leaves that @@ -39,7 +39,7 @@ import { import { mailDisposition, readMailRow } from './helpers/orchestration-mail-store' import { waitForPtyShellEcho } from './terminal-pty-readiness' -const BANNER_PREFIX = '--- Orchestration Messages' +const POINTER_COMMAND = 'orca orchestration check' const NO_DELIVERY_SETTLE_MS = 5_000 const DELIVERY_TIMEOUT_MS = 20_000 @@ -159,10 +159,10 @@ test('keeps mail pending across a restart and delivers it when the agent reports expect(readMailRow(session.userDataDir, messageId)).toBeDefined() await second.page.waitForTimeout(NO_DELIVERY_SETTLE_MS) expect(mailDisposition(readMailRow(session.userDataDir, messageId))).toBe('pending') - expect(agent.readStdin()).not.toContain(BANNER_PREFIX) + expect(agent.readStdin()).not.toContain(POINTER_COMMAND) // Re-emitting the SAME idle title changes no status — only its liveness — so - // the row moving here is delivery resuming on the agent's own signal. + // the pointer appearing here is delivery resuming on the agent's own signal. agent.setTitle(CODEX_IDLE_TITLE) await expect .poll(() => agent.titleEmitCount(), { timeout: 30_000 }) @@ -172,12 +172,8 @@ test('keeps mail pending across a restart and delivers it when the agent reports timeout: DELIVERY_TIMEOUT_MS, message: 'live idle frame never released the pending mail' }) - .toContain(BANNER_PREFIX) - await expect - .poll(() => mailDisposition(readMailRow(session.userDataDir, messageId)), { - timeout: DELIVERY_TIMEOUT_MS - }) - .toBe('pushed') + .toContain(POINTER_COMMAND) + expect(mailDisposition(readMailRow(session.userDataDir, messageId))).toBe('pending') } finally { if (firstApp) { await session.close(firstApp)