From a025a714472869bb7759aa62d852ec3bb1c53807 Mon Sep 17 00:00:00 2001 From: BingZ Date: Fri, 7 Aug 2026 08:08:28 +0800 Subject: [PATCH] fix(orchestration): deliver pending mail to already-idle agents (#12584) Co-authored-by: Jinwoo-H --- src/main/runtime/orca-runtime.test.ts | 679 ++++++++++++++++++ src/main/runtime/orca-runtime.ts | 159 +++- .../runtime/rpc/methods/orchestration.test.ts | 4 +- .../terminal-send-stale-leaf-liveness.test.ts | 145 +++- .../helpers/orchestration-mail-pane-agent.ts | 157 ++++ tests/e2e/helpers/orchestration-mail-store.ts | 84 +++ .../orchestration-idle-mail-delivery.spec.ts | 406 +++++++++++ .../orchestration-idle-mail-restore.spec.ts | 190 +++++ 8 files changed, 1803 insertions(+), 21 deletions(-) create mode 100644 tests/e2e/helpers/orchestration-mail-pane-agent.ts create mode 100644 tests/e2e/helpers/orchestration-mail-store.ts create mode 100644 tests/e2e/orchestration-idle-mail-delivery.spec.ts create mode 100644 tests/e2e/orchestration-idle-mail-restore.spec.ts diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index d46b6e6bd..cabbb9ac0 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -33254,6 +33254,685 @@ describe('OrcaRuntimeService', () => { await task }) + it('delivers pending mail via notifyMessageArrived when the recipient is already idle', 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 + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + const message = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'after wait' + }) + + // Why: notifyMessageArrived is the send-path hook; it must push-on-idle + // without requiring another agent-status transition (#12536). + runtime.notifyMessageArrived(terminal.handle, 'status') + + // 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')) + await vi.advanceTimersByTimeAsync(500) + expect(write).toHaveBeenCalledWith('pty-1', '\r') + expect(message.delivered_at).not.toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('does not inject pending mail on notify when the recipient is still working', 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 () => null + }) + syncSinglePty(runtime) + + const [terminal] = (await runtime.listTerminals()).terminals + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + const message = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'while working' + }) + write.mockClear() + + runtime.notifyMessageArrived(terminal.handle, 'status') + await Promise.resolve() + + expect(write).not.toHaveBeenCalled() + // Why: busy must leave the row undelivered so a later idle can push it; + // a stamp-without-write would suppress later delivery (#12584 CodeRabbit). + expect(message.delivered_at).toBeNull() + db.close() + }) + + it('delivers on a first live idle frame that follows a seeded idle with no transition', 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 + runtime.seedTerminalRestoreTail('pty-1', { lastTitle: 'Codex done' }) + const message = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'restored idle' + }) + runtime.notifyMessageArrived(terminal.handle, 'status') + await Promise.resolve() + write.mockClear() + + // Why no working frame: a resumed agent sitting at its prompt emits an + // already-idle title first. The seed left lastAgentStatus 'idle', so there + // 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')) + await vi.advanceTimersByTimeAsync(600) + expect(message.delivered_at).not.toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('does not push on a cold-restore seeded idle status with no live observation', 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 + // Why: the persisted title is historical — the agent may have gone busy + // across the relaunch, so a seeded 'idle' must not authorize a PTY write. + runtime.seedTerminalRestoreTail('pty-1', { lastTitle: 'Codex done' }) + const message = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'seeded idle' + }) + write.mockClear() + + runtime.notifyMessageArrived(terminal.handle, 'status') + await Promise.resolve() + + expect(write).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(600) + expect(message.delivered_at).toBeNull() + + // 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')) + await vi.advanceTimersByTimeAsync(600) + expect(message.delivered_at).not.toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('lets a resolved check consume its rows before a later same-tick notify pushes', 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 + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + write.mockClear() + + // Why: resolveMessageWaiter removes the waiter synchronously, but the check + // handler marks its rows read a microtask later. Two sends resuming off one + // shared in-flight promise put a no-waiter notify inside that window, so the + // push must not inject rows the resolved check is about to return. + const consumed = runtime + .waitForMessage(terminal.handle, { timeoutMs: 5_000 }) + .then(() => db.getUnreadMessages(terminal.handle).map((row) => (row.read = 1))) + const first = db.insertMessage({ from: 'sender', to: terminal.handle, subject: 'pulled' }) + runtime.notifyMessageArrived(terminal.handle, 'status') + const second = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'also pulled' + }) + runtime.notifyMessageArrived(terminal.handle, 'status') + + await consumed + await Promise.resolve() + expect(write).not.toHaveBeenCalled() + expect(first.delivered_at).toBeNull() + expect(second.delivered_at).toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('leaves rows a live filtered waiter reserved out of the pushed batch', 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 + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + write.mockClear() + + // Why: the push reads every pending row, not just the one that woke it. A + // `status` notify is unclaimed and pushes, but the worker_done row landing + // in the same drain belongs to this waiter's check — injecting it too would + // deliver that completion twice (pane + check return). + const waitPromise = runtime.waitForMessage(terminal.handle, { + typeFilter: ['worker_done'], + timeoutMs: 5_000 + }) + const status = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'unclaimed status', + type: 'status' + }) + runtime.notifyMessageArrived(terminal.handle, 'status') + const done = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'reserved completion', + type: 'worker_done' + }) + runtime.notifyMessageArrived(terminal.handle, 'worker_done') + + await expect(waitPromise).resolves.toBe('notified') + await vi.advanceTimersByTimeAsync(600) + 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(done.delivered_at).toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('skips rows claimed by a waiter that registered after the notify', 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 + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + write.mockClear() + + const message = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'claimed late', + type: 'status' + }) + // Why: the notify snapshot is empty — no waiter existed yet. A check that + // blocks before the deferred push runs still owns this row, so only the + // push-time read of live waiters can keep it out of the pane. + runtime.notifyMessageArrived(terminal.handle, 'status') + const waitPromise = runtime.waitForMessage(terminal.handle, { + typeFilter: ['status'], + timeoutMs: 5_000 + }) + await Promise.resolve() + + expect(write).not.toHaveBeenCalled() + expect(message.delivered_at).toBeNull() + + await vi.advanceTimersByTimeAsync(5_000) + await expect(waitPromise).resolves.toBe('timed_out') + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('does not carry pty-record live authority into a rebuilt leaf after a same-id respawn', async () => { + vi.useFakeTimers() + try { + const runtime = new OrcaRuntimeService(store) + const db = new InMemoryOrchestrationMessages() + const write = vi.fn().mockReturnValue(true) + runtime.setPtyController({ + write, + kill: vi.fn(), + getForegroundProcess: async () => null + }) + // Why a UUID leaf id: the retirement fence's pty-candidate clause compares + // parsePaneKey(pty.paneKey).leafId to the republished leafId, and a non-UUID + // id falls back to `tabId:paneRuntimeId`, so it is always fenced after exit. + const leafId = '11111111-1111-1111-8111-111111111111' + const syncUuidLeaf = (): void => { + runtime.attachWindow(1) + runtime.syncWindowGraph(1, { + tabs: [ + { + tabId: 'tab-1', + worktreeId: TEST_WORKTREE_ID, + title: 'Codex', + activeLeafId: leafId, + layout: null + } + ], + leaves: [ + { + tabId: 'tab-1', + worktreeId: TEST_WORKTREE_ID, + leafId, + paneRuntimeId: 1, + ptyId: 'pty-1', + paneTitle: null + } + ] + }) + } + syncUuidLeaf() + + const [terminal] = (await runtime.listTerminals()).terminals + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + write.mockClear() + + runtime.onPtyExit('pty-1', 0) + runtime.onPtySpawned('pty-1', undefined, { awaitsRegistration: false }) + // Drop the leaf, then republish it: the rebuilt record's tailSource is the + // PTY record rather than the previous leaf, which is what pins the clear + // onPtyExit applies at the pty level. + runtime.syncWindowGraph(1, { tabs: [], leaves: [] }) + syncUuidLeaf() + + const leaves = ( + runtime as unknown as { + leaves: Map< + string, + { lastAgentStatus: string | null; lastAgentStatusObservedLive: boolean } + > + } + ).leaves + expect(leaves.size).toBeGreaterThan(0) + const rebuilt = [...leaves.values()][0] + expect(rebuilt.lastAgentStatus).toBe('idle') + expect(rebuilt.lastAgentStatusObservedLive).toBe(false) + + setInMemoryOrchestrationMessages(runtime, db) + const [republished] = (await runtime.listTerminals()).terminals + const message = db.insertMessage({ + from: 'sender', + to: republished.handle, + subject: 'rebuilt leaf' + }) + runtime.notifyMessageArrived(republished.handle, 'status') + await Promise.resolve() + + expect(write).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(600) + expect(message.delivered_at).toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('keeps live idle authority across a renderer graph republish', 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 + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + write.mockClear() + + // Why: syncWindowGraph rebuilds every leaf record on any pane/tab change. + // An idle agent emits no new title, so dropping the live-status carry here + // would strand mail until the next OSC frame — the #12536 symptom. + syncSinglePty(runtime) + + const [republished] = (await runtime.listTerminals()).terminals + const message = db.insertMessage({ + from: 'sender', + to: republished.handle, + subject: 'after republish' + }) + + runtime.notifyMessageArrived(republished.handle, 'status') + await Promise.resolve() + + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('Subject: after republish') + ) + await vi.advanceTimersByTimeAsync(600) + expect(message.delivered_at).not.toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('does not reuse the dead process live idle authority after a same-id respawn', async () => { + vi.useFakeTimers() + try { + const runtime = new OrcaRuntimeService(store) + const db = new InMemoryOrchestrationMessages() + const write = vi.fn().mockReturnValue(true) + runtime.setPtyController({ + write, + kill: vi.fn(), + getForegroundProcess: async () => null + }) + syncSinglePty(runtime) + + const [terminal] = (await runtime.listTerminals()).terminals + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + write.mockClear() + + // Why: a cold restore respawns under the same session id and makes the + // leaf writable again before any new title. The dead process's live idle + // must not authorize typing into its replacement mid-turn. + runtime.onPtyExit('pty-1', 0) + runtime.onPtySpawned('pty-1', undefined, { awaitsRegistration: false }) + setInMemoryOrchestrationMessages(runtime, db) + const message = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'after same id respawn' + }) + + runtime.notifyMessageArrived(terminal.handle, 'status') + await Promise.resolve() + + expect(write).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(600) + expect(message.delivered_at).toBeNull() + + // The replacement's first live idle frame re-authorizes delivery — with no + // working frame, since exit keeps lastAgentStatus 'idle' for `ps` and the + // replacement can come up straight at an idle prompt (no transition). + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 200) + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('Subject: after same id respawn') + ) + await vi.advanceTimersByTimeAsync(600) + expect(message.delivered_at).not.toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('pushes to an idle pane when the only live waiter filters out the message type', 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 + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + + // Why: a `check --wait --types worker_done` waiter never returns a status + // row — check re-reads under the same filter — so it is not the consumer + // and treating it as one would strand the message (#12536). + const waitPromise = runtime.waitForMessage(terminal.handle, { + typeFilter: ['worker_done'], + timeoutMs: 5_000 + }) + const message = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'unfiltered status', + type: 'status' + }) + write.mockClear() + + runtime.notifyMessageArrived(terminal.handle, 'status') + + await Promise.resolve() + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('Subject: unfiltered status') + ) + await vi.advanceTimersByTimeAsync(600) + expect(message.delivered_at).not.toBeNull() + + // The filtered waiter stays blocked; the push did not consume its wake. + await vi.advanceTimersByTimeAsync(5_000) + await expect(waitPromise).resolves.toBe('timed_out') + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('resolves a registered waiter without PTY-injecting when the leaf is already idle', 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 + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + const message = db.insertMessage({ + from: 'sender', + to: terminal.handle, + subject: 'for check wait' + }) + write.mockClear() + + // Why: blocked orchestration.check --wait is an explicit pull; push must + // not stamp delivered_at or type into the pane (double delivery, #12584). + const waitPromise = runtime.waitForMessage(terminal.handle, { timeoutMs: 5_000 }) + runtime.notifyMessageArrived(terminal.handle, 'status') + + await expect(waitPromise).resolves.toBe('notified') + expect(write).not.toHaveBeenCalled() + expect(message.delivered_at).toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('does not re-inject the same message when notify fires again during Enter delay', 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 + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + db.insertMessage({ from: 'sender', to: terminal.handle, subject: 'once only' }) + + runtime.notifyMessageArrived(terminal.handle, 'status') + await Promise.resolve() + runtime.notifyMessageArrived(terminal.handle, 'status') + await Promise.resolve() + + const payloadWrites = write.mock.calls.filter( + ([, payload]) => typeof payload === 'string' && payload.includes('Subject: once only') + ) + expect(payloadWrites).toHaveLength(1) + + await vi.advanceTimersByTimeAsync(500) + const enterWrites = write.mock.calls.filter(([, payload]) => payload === '\r') + expect(enterWrites).toHaveLength(1) + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('delivers a second message parked during Enter delay once the flight settles', 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 + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + await runtime.waitForTerminal(terminal.handle, { condition: 'tui-idle' }) + const first = db.insertMessage({ from: 'sender', to: terminal.handle, subject: 'first' }) + runtime.notifyMessageArrived(terminal.handle, 'status') + // Why the flush: the deferred push must actually arm its flight before the + // second message arrives, or this exercises a plain batch instead. + await Promise.resolve() + + // Why: mid-flight notify parks the leaf; flight settle re-runs delivery + // so the second row is not lost and is not double-injected with the first. + const second = db.insertMessage({ from: 'sender', to: terminal.handle, subject: 'second' }) + runtime.notifyMessageArrived(terminal.handle, 'status') + await Promise.resolve() + expect( + write.mock.calls.filter( + ([, payload]) => typeof payload === 'string' && payload.includes('Subject: second') + ) + ).toHaveLength(0) + expect(second.delivered_at).toBeNull() + + // Why: release must not require another agent-status OSC — only the + // delayed-Enter flight timer. Advancing 3s with no status output covers + // 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( + write.mock.calls.filter( + ([, payload]) => typeof payload === 'string' && payload.includes('Subject: second') + ) + ).toHaveLength(1) + expect(second.delivered_at).not.toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + it('keeps already-idle status after tui-idle wait for immediate message delivery', async () => { const runtime = new OrcaRuntimeService(store) const db = new InMemoryOrchestrationMessages() diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 5426f28d9..ea146b523 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1214,6 +1214,10 @@ type RuntimeLeafRecord = RuntimeSyncedLeaf & { // full tail. See computeTerminalTailWaitState. tailWaitState?: TerminalTailWaitState lastAgentStatus: AgentStatus | null + // Why: seeded status is a historical title replayed on restore, so it cannot + // authorize a PTY write. Only a live OSC observation sets this true; push + // delivery reads it so a cold-restored `idle` never types into a working agent. + lastAgentStatusObservedLive: boolean // Why: the most recent OSC title observed on this leaf's PTY data. Used by // worktree.ps so daemon-hosted terminals (no renderer pushing pane titles) // still recompute working/idle from the live title each call instead of @@ -1253,6 +1257,8 @@ type RuntimePtyWorktreeRecord = { disconnectedAt: number | null lastExitCode: number | null lastAgentStatus: AgentStatus | null + /** False until a live OSC frame sets the status; restore seeds never set it. */ + lastAgentStatusObservedLive: boolean lastOscTitle: string | null lastOscTitleAt: number | null // Why a second stamp: `lastOscTitleAt` is a title-observation sequence number, @@ -1976,6 +1982,24 @@ type MessageWaiter = { export type MessageWaitResult = 'notified' | 'timed_out' | 'cancelled' | 'waiter_exists' +// Why: an unfiltered waiter claims every type. A row a live waiter will return +// from orchestration.check must not also be pushed into the pane — check reads +// by `read`, push stamps `delivered_at`, so neither hides the row from the other. +function messageTypeHasLiveWaiter( + waiters: Set | undefined, + messageType: string +): boolean { + if (!waiters) { + return false + } + for (const waiter of waiters) { + if (!waiter.typeFilter || waiter.typeFilter.includes(messageType)) { + return true + } + } + return false +} + function omitUndefinedProperties>(value: T): Partial { return Object.fromEntries( Object.entries(value).filter(([, entry]) => entry !== undefined) @@ -5499,6 +5523,7 @@ export class OrcaRuntimeService { preview: tailSource?.preview ?? '', waitBlockedAt: tailSource?.waitBlockedAt ?? null, lastAgentStatus: tailSource?.lastAgentStatus ?? null, + lastAgentStatusObservedLive: tailSource?.lastAgentStatusObservedLive ?? false, lastOscTitle: tailSource?.lastOscTitle ?? null, lastOscTitleAt: tailSource?.lastOscTitleAt ?? null, paneTitleUpdatedAt: @@ -10229,6 +10254,7 @@ export class OrcaRuntimeService { pty.lastOscTitleAt = observedAt pty.lastOscTitleEpochMs = Date.now() pty.lastAgentStatus = agentStatus + pty.lastAgentStatusObservedLive = true this.setPtyManagementTitleFromObservedTitle(pty, normalizedTitle, observedAt) ptyRecordChanged = prevTitle !== normalizedTitle || prevStatus !== agentStatus if (agentStatus === 'idle' && prevStatus !== 'idle') { @@ -10266,6 +10292,7 @@ export class OrcaRuntimeService { leaf.lastOscTitle = normalizedTitle leaf.lastOscTitleAt = this.nextTitleObservationSequence() const prevStatus = leaf.lastAgentStatus + const prevObservedLive = leaf.lastAgentStatusObservedLive // Why: when a new OSC title doesn't classify as an agent state (e.g. // bare shell title after the agent exits), clear lastAgentStatus so // it is no longer sticky. Tui-idle waiters that needed the previous @@ -10274,6 +10301,7 @@ export class OrcaRuntimeService { // exits would observe the cleared value, and they correctly fall // back to title-based detection / polling. leaf.lastAgentStatus = agentStatus + leaf.lastAgentStatusObservedLive = true // Why: resolve tui-idle on any transition TO idle (not just working→idle). // Claude Code may skip "working" entirely on fast tasks, going null→idle, // and the coordinator's tui-idle waiter would hang forever waiting for a @@ -10282,6 +10310,14 @@ export class OrcaRuntimeService { // which isn't a task-completion signal. if (agentStatus === 'idle' && prevStatus !== 'idle') { this.resolveTuiIdleWaiters(leaf) + } + // Why the second condition: push delivery is gated on LIVE idle, so its + // authorizing edge is liveness as well as status. A restore seed or a + // status kept across a same-id respawn leaves a stale 'idle' behind, and + // 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)) { this.deliverPendingMessages(leaf) } } @@ -10311,6 +10347,9 @@ export class OrcaRuntimeService { pty.lastOscTitleAt = null pty.lastOscTitleEpochMs = null pty.lastAgentStatus = null + // Why: the prior process's live frames say nothing about the replacement, + // so the seed a same-id restore applies must not inherit its authority. + pty.lastAgentStatusObservedLive = false pty.managementTitle = null pty.managementTitleAt = null } @@ -10318,6 +10357,7 @@ export class OrcaRuntimeService { leaf.lastOscTitle = null leaf.lastOscTitleAt = null leaf.lastAgentStatus = null + leaf.lastAgentStatusObservedLive = false } this.clearAgentRowSnapshotsForPty(ptyId) } @@ -11199,10 +11239,11 @@ export class OrcaRuntimeService { // Why: seed-derived agent status reflects historical state. Orchestration // waiters (resolveTuiIdleWaiters, deliverPendingMessages) must only react - // to LIVE transitions, so this helper writes leaf.lastAgentStatus only and - // never resolves waiters. detectAgentStatusFromTitle wrap mirrors the live - // path so seeded and live values are the same union member, keeping - // downstream `=== 'idle'` checks correct. + // to LIVE transitions, so this helper writes leaf.lastAgentStatus only, + // leaves lastAgentStatusObservedLive untouched, and never resolves waiters. + // detectAgentStatusFromTitle wrap mirrors the live path so seeded and live + // values are the same union member, keeping downstream `=== 'idle'` checks + // correct. private applySeededAgentStatus(ptyId: string, title: string): void { if (!title) { return @@ -13470,6 +13511,11 @@ export class OrcaRuntimeService { this.setPairedRendererSessionOwnership(pty.ptyId, false) pty.disconnectedAt = Date.now() pty.lastExitCode = exitCode + // Why: the exited process's live frames say nothing about a replacement. + // A same-id respawn makes the leaf writable again before any new title, + // so leaving this true would let push delivery type into the new process + // on the dead one's idle. lastAgentStatus itself stays for `ps` display. + pty.lastAgentStatusObservedLive = false this.resolvePtyExitWaiters(pty, ptyId) this.pruneDisconnectedPtyTranscript(pty) } @@ -13487,6 +13533,7 @@ export class OrcaRuntimeService { leaf.connected = false leaf.writable = false leaf.lastExitCode = exitCode + leaf.lastAgentStatusObservedLive = false this.resolveExitWaiters(leaf) if (!preservesAbnormalSshSurface) { this.failActiveDispatchOnExit(leaf, exitCode) @@ -28894,6 +28941,7 @@ export class OrcaRuntimeService { disconnectedAt: state.connected === false ? Date.now() : null, lastExitCode: null, lastAgentStatus: null, + lastAgentStatusObservedLive: false, lastOscTitle: null, lastOscTitleAt: null, lastOscTitleEpochMs: null, @@ -31069,11 +31117,23 @@ export class OrcaRuntimeService { return this.getLeavesForPty(ptyId)[0] ?? null } - deliverPendingMessagesForHandle(handle: string): void { + 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 + } try { const { leaf } = this.getLiveLeafForHandle(handle) - if (leaf.lastAgentStatus === 'idle') { - this.deliverPendingMessages(leaf) + // 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. + if (leaf.lastAgentStatus === 'idle' && leaf.lastAgentStatusObservedLive) { + this.deliverPendingMessages(leaf, false, reservedTypes) } } catch { // Unknown/stale handles can't be pushed now; the persisted message stays available via explicit check or future idle delivery. @@ -31082,15 +31142,44 @@ export class OrcaRuntimeService { // Why: wake blocking orchestration.check --wait calls on this handle so they return the new message immediately instead of polling. notifyMessageArrived(handle: string, messageType?: string): void { + // Why: push-on-idle is driven by status transitions; a message that + // arrives while the recipient is already idle never sees a transition, so + // 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 "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 + // already-idle state #12536 is about. const waiters = this.messageWaitersByHandle.get(handle) - if (!waiters || waiters.size === 0) { + // Why: don't wake a coordinator waiting for worker_done/escalation on heartbeat noise it would misread as idleness. + const consumers = waiters + ? [...waiters].filter( + (waiter) => !messageType || !waiter.typeFilter || waiter.typeFilter.includes(messageType) + ) + : [] + if (consumers.length === 0) { + // Why snapshot the reservation here: every remaining waiter filters this + // type out, but the push reads ALL pending rows. A waiter resolved later in + // this same drain is gone by the time the push runs, so reading waiters + // then would miss what its check is about to return. Captured now, the + // types those waiters claim stay out of the batch. + const reservedTypes = new Set(waiters ? [...waiters].flatMap((w) => w.typeFilter ?? []) : []) + // Why queueMicrotask: resolveMessageWaiter removes the waiter synchronously + // but its check handler marks the rows read a microtask later. Two sends + // that resumed adjacently off one shared in-flight promise (group send + // awaits listTerminals) put a no-waiter notify inside that window, where a + // synchronous push would inject rows the resolved check is about to return. + // Deferring one hop puts the push behind any already-queued check, and it + // re-reads undelivered rows when it runs so nothing strands. + queueMicrotask(() => this.deliverPendingMessagesForHandle(handle, reservedTypes)) return } - for (const waiter of [...waiters]) { - // Why: don't wake a coordinator waiting for worker_done/escalation on heartbeat noise it would misread as idleness. - if (messageType && waiter.typeFilter && !waiter.typeFilter.includes(messageType)) { - continue - } + for (const waiter of consumers) { this.resolveMessageWaiter(waiter, 'notified') } } @@ -31720,7 +31809,11 @@ export class OrcaRuntimeService { } // 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): void { + private deliverPendingMessages( + leaf: RuntimeLeafRecord, + skipAbsenceProbe = false, + reservedTypes?: ReadonlySet + ): void { if (!this._orchestrationDb) { return } @@ -31736,7 +31829,18 @@ export class OrcaRuntimeService { return } - const unread = this._orchestrationDb.getUndeliveredUnreadMessages(handle) + // Why filter here and not at the trigger: the push reads every pending row, + // not just the one that woke it, so a row a pull has claimed would be typed + // 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 unread = this._orchestrationDb + .getUndeliveredUnreadMessages(handle) + .filter( + (message) => + !reservedTypes?.has(message.type) && !messageTypeHasLiveWaiter(waiters, message.type) + ) if (unread.length === 0) { return } @@ -31766,7 +31870,30 @@ export class OrcaRuntimeService { .then((absent) => { this.probeDeferredDeliveryPtyIds.delete(probedPtyId) if (!absent && leaf.ptyId === probedPtyId) { - this.deliverPendingMessages(leaf, true) + // Why a macrotask and not the stale reservation snapshot: a `remote:` + // pty answers the probe null before its first await, so this chain can + // settle in microtasks and overtake the resumption of a check resolved + // meanwhile — that check's waiter is already out of the map and its + // rows are not yet read, so the push would inject what it returns. + // Yielding the turn lets every queued check mark its rows read first; + // re-reading then (rather than replaying a reservation this probe may + // have outlived) is what keeps an orphaned row from stranding. + setTimeout(() => { + // 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. + const currentLeaf = this.leaves.get(this.getLeafKey(leaf.tabId, leaf.leafId)) + if ( + currentLeaf?.ptyId === probedPtyId && + currentLeaf.lastAgentStatus === 'idle' && + currentLeaf.lastAgentStatusObservedLive + ) { + this.deliverPendingMessages(currentLeaf, true) + } + }, 0) } }) .catch(() => { diff --git a/src/main/runtime/rpc/methods/orchestration.test.ts b/src/main/runtime/rpc/methods/orchestration.test.ts index a90bbe804..d9d91237d 100644 --- a/src/main/runtime/rpc/methods/orchestration.test.ts +++ b/src/main/runtime/rpc/methods/orchestration.test.ts @@ -298,6 +298,8 @@ describe('orchestration RPC methods', () => { describe('orchestration.send', () => { it('sends a message', async () => { setup() + // Why: send notifies arrival so already-idle recipients get push-on-idle + // delivery without waiting for a status transition (#12536). vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) const result = (await call('orchestration.send', { from: 'term_coord', @@ -308,7 +310,7 @@ describe('orchestration RPC methods', () => { expect(result.message.id).toMatch(/^msg_/) expect(result.message.from_handle).toBe('term_coord') expect(result.message.run_id).toBe(activeRunId) - expect(runtime.deliverPendingMessagesForHandle).not.toHaveBeenCalled() + expect(runtime.deliverPendingMessagesForHandle).toHaveBeenCalled() }) it('routes exact Dispatch mail independently of terminal handles', async () => { 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 01aed591d..823c04e5a 100644 --- a/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts +++ b/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts @@ -243,7 +243,7 @@ function makeOrchestrationDbStub(toHandle: () => string) { return { rows, markAsDelivered, - insert(subject: string): void { + insert(subject: string, type: StoredMessageRow['type'] = 'status'): void { rows.push({ id: `msg_${rows.length + 1}`, run_id: 'run_test', @@ -251,7 +251,7 @@ function makeOrchestrationDbStub(toHandle: () => string) { to_handle: toHandle(), subject, body: '', - type: 'status', + type, priority: 'normal', thread_id: null, payload: null, @@ -263,8 +263,9 @@ function makeOrchestrationDbStub(toHandle: () => string) { }) }, db: { + // Mirrors the real query: `read = 0 AND delivered_at IS NULL`. getUndeliveredUnreadMessages: (handle: string) => - rows.filter((row) => row.to_handle === handle && !row.delivered_at), + rows.filter((row) => row.to_handle === handle && row.read === 0 && !row.delivered_at), getActiveCoordinatorRun: () => null, // Consulted by onPtyExit's dispatch-failure path. getActiveDispatchForTerminal: () => null, @@ -289,6 +290,131 @@ describe('push-on-idle orchestration delivery absence gate', () => { return { runtime, handle, write, stub } } + // Why: the gate that authorizes a push runs BEFORE the probe defers, so a + // same-id cold restore inside the probe window would otherwise be written to on + // the dead process's authority — ptyId is exactly what a same-id respawn keeps. + it('re-applies the live-idle gate when the probe answers after a same-id respawn', async () => { + let resolveProbe!: (value: boolean | null) => void + const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ + probePtyLiveness: () => + new Promise((resolve) => { + resolveProbe = resolve + }) + }) + stub.insert('for the old session') + + runtime.notifyMessageArrived(handle, 'status') + await Promise.resolve() + expect(write).not.toHaveBeenCalled() + + // The session dies and cold-restores under the same id while the probe is out. + runtime.onPtyExit(STALE_PTY_ID, 0) + runtime.onPtySpawned(STALE_PTY_ID, undefined, { awaitsRegistration: false }) + + resolveProbe(null) + await new Promise((resolve) => setTimeout(resolve, 0)) + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(write).not.toHaveBeenCalled() + expect(stub.rows[0].delivered_at).toBeNull() + + // The replacement's own live idle frame releases the row — through a fresh + // probe, since this leaf's pty is still unknown to the provider. + runtime.onPtyData(STALE_PTY_ID, '\x1b]0;Codex done\x07', 200) + resolveProbe(null) + await new Promise((resolve) => setTimeout(resolve, 0)) + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(write).toHaveBeenCalledWith( + STALE_PTY_ID, + expect.stringContaining('Subject: for the old session') + ) + }) + + // Why: a `remote:` pty answers probePtyLiveness with null before its first + // await (ipc/pty.ts), so the probe settles on a pure microtask chain. Without a + // macrotask hop the continuation runs BEFORE the resumption of a check resolved + // in the meantime — the waiter is already out of the map and its rows are not + // yet read, so the push injects exactly what that check is about to return. + it('waits a macrotask before delivering so a resolved check consumes its rows first', async () => { + const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ + probePtyLiveness: async () => null + }) + + const pulled: string[] = [] + const checkResumed = runtime + .waitForMessage(handle, { typeFilter: ['worker_done'], timeoutMs: 60_000 }) + .then(() => { + for (const row of stub.rows) { + if (row.type === 'worker_done' && row.read === 0) { + row.read = 1 + pulled.push(row.subject) + } + } + }) + + stub.insert('unclaimed status') + runtime.notifyMessageArrived(handle, 'status') + // Land the completion while the probe chain is mid-flight — the slot where + // the continuation would otherwise overtake the check's resumption. + await Promise.resolve() + await Promise.resolve() + stub.insert('worker completion', 'worker_done') + runtime.notifyMessageArrived(handle, 'worker_done') + + await checkResumed + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(pulled).toEqual(['worker completion']) + 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: worker completion'))).toBe(false) + }) + + // Why: the notify-time reservation snapshot exists for a waiter resolved inside + // one microtask drain. The probe continuation runs many macrotasks later, and + // the probe dedup swallows every notify arriving meanwhile — so a reservation + // carried in here would skip a row with nothing left to retry it (#12536 again). + it('does not carry a stale waiter reservation into the probe continuation', async () => { + let resolveProbe!: (value: boolean | null) => void + const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ + probePtyLiveness: () => + new Promise((resolve) => { + resolveProbe = resolve + }) + }) + + const waitPromise = runtime.waitForMessage(handle, { + typeFilter: ['worker_done'], + timeoutMs: 60_000 + }) + stub.insert('unclaimed status') + runtime.notifyMessageArrived(handle, 'status') + await Promise.resolve() + expect(write).not.toHaveBeenCalled() + + // The reserving waiter goes away, then its type finally arrives — and the + // probe dedup drops this notify, so only the continuation can deliver it. + runtime.cancelMessageWaiters(handle) + await expect(waitPromise).resolves.toBe('cancelled') + stub.insert('late completion', 'worker_done') + runtime.notifyMessageArrived(handle, 'worker_done') + await Promise.resolve() + + resolveProbe(null) + await new Promise((resolve) => setTimeout(resolve, 0)) + // Why twice: the probe continuation yields a turn before delivering. + await new Promise((resolve) => setTimeout(resolve, 0)) + + 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) + }) + it('keeps messages queued instead of marking a proven-absent pty delivered', async () => { const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ probePtyLiveness: async () => false @@ -311,6 +437,8 @@ describe('push-on-idle orchestration delivery absence gate', () => { runtime.deliverPendingMessagesForHandle(handle) await new Promise((resolve) => setTimeout(resolve, 0)) + // 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')) }) @@ -435,8 +563,17 @@ describe('push-on-idle orchestration delivery absence gate', () => { expect(stub.markAsDelivered).not.toHaveBeenCalled() expect(stub.rows[0].delivered_at).toBeNull() - // The replacement's own delivery starts a fresh flight and completes. + // The replacement's own delivery starts a fresh flight and completes — + // but only once ITS live title proves idle; the dead session's live status + // no longer authorizes a write into the new process. runtime.deliverPendingMessagesForHandle(handle) + expect( + write.mock.calls.filter( + ([, data]) => typeof data === 'string' && data.includes('Subject: for the old session') + ) + ).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') ) diff --git a/tests/e2e/helpers/orchestration-mail-pane-agent.ts b/tests/e2e/helpers/orchestration-mail-pane-agent.ts new file mode 100644 index 000000000..f948bfa8c --- /dev/null +++ b/tests/e2e/helpers/orchestration-mail-pane-agent.ts @@ -0,0 +1,157 @@ +/** + * A scriptable stand-in for an agent CLI, for orchestration push-delivery E2E. + * + * Why a purpose-built process and not a bare shell emitting titles: push-on-idle + * is gated on the status Orca infers from live OSC titles and delivers by + * 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 + * reached the agent" an assertion instead of an inference. + * + * Titles come from a polled file, not stdin, because orchestration writes to + * stdin itself; a stdin control channel could not tell a test command apart from + * the delivery under test. + * + * Why it runs in the pane the fixture already opened, rather than a pane created + * for it: terminal.create waits up to 10s for a renderer graph sync to bind the + * new tab's handle, and a headless CI renderer misses that deadline — every spec + * here died on 'Timed out waiting for terminal handle after creation'. Nothing + * on the delivery path reads a pane's agent metadata (it resolves the leaf, the + * OSC title, and PTY liveness), so a foreground process in a mounted pane + * exercises the same code with none of that startup race. + */ +import { mkdtempSync, existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +/** `detectAgentStatusFromTitle` reads these as agent-name + strong keyword. */ +export const CODEX_IDLE_TITLE = 'Codex done' +export const CODEX_WORKING_TITLE = 'Codex working' +/** Also satisfies `isCursorAgentTitle`, which suppresses the synthesized Enter. */ +export const CURSOR_IDLE_TITLE = 'Cursor Ready' + +export type AgentLedgerEntry = { + pid: number + at: number + event: 'start' | 'stdin' | 'title' + data?: string + title?: string +} + +const AGENT_SOURCE = ` +const { appendFileSync, existsSync, readFileSync, statSync } = require('node:fs') + +const [ledgerPath, controlPath] = process.argv.slice(2) + +function log(entry) { + try { + appendFileSync(ledgerPath, JSON.stringify({ pid: process.pid, at: Date.now(), ...entry }) + '\\n') + } catch {} +} + +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. +if (process.stdin.isTTY) { + process.stdin.setRawMode(true) +} + +// Every byte orchestration pushes lands here — banner text and Enter alike. +process.stdin.on('data', (chunk) => log({ event: 'stdin', data: chunk.toString() })) +process.stdin.resume() + +// No title is emitted until the test asks for one, so a pane can be held in the +// "no live agent status yet" state some cases depend on. Keyed on mtime rather +// than content so a test can re-emit the SAME title: proving a restored pane +// needed a LIVE frame means sending an idle it already appears to have. +let lastStamp = null +setInterval(() => { + if (!existsSync(controlPath)) return + let title + let stamp + try { + stamp = statSync(controlPath).mtimeMs + if (stamp === lastStamp) return + title = readFileSync(controlPath, 'utf8').trim() + } catch { + return + } + if (!title) return + lastStamp = stamp + process.stdout.write('\\u001b]0;' + title + '\\u0007') + log({ event: 'title', title }) +}, 50) + +setInterval(() => {}, 60_000) +` + +export type MailPaneAgent = { + /** Shell-agnostic command that starts the agent; no trailing carriage return. */ + launchCommand: string + /** Emit `title` as an OSC title from the live process. */ + setTitle: (title: string) => void + readLedger: () => AgentLedgerEntry[] + /** Concatenated stdin — what the agent actually received. */ + readStdin: () => string + hasStarted: () => boolean + /** Emitted-title count; the readiness signal when a title is re-sent as-is. */ + titleEmitCount: () => number +} + +// Why worker exit and not a spec's afterAll: Playwright reuses a worker across +// spec files, and a temp dir removed while another spec still polls its ledger +// surfaces as an agent that mysteriously stopped reporting. +const agentDirs: string[] = [] +process.once('exit', () => { + for (const dir of agentDirs) { + rmSync(dir, { recursive: true, force: true }) + } +}) + +/** One isolated agent: its own script copy, ledger, and control file. */ +export function createMailPaneAgent(): MailPaneAgent { + const dir = mkdtempSync(path.join(os.tmpdir(), 'orca-e2e-mail-agent-')) + agentDirs.push(dir) + const scriptPath = path.join(dir, 'agent.cjs') + const ledgerPath = path.join(dir, 'ledger.jsonl') + const controlPath = path.join(dir, 'title') + writeFileSync(scriptPath, AGENT_SOURCE) + writeFileSync(ledgerPath, '') + + // Why forward slashes: valid for node on Windows and parsed identically by + // PowerShell, cmd, and POSIX shells, where raw backslashes would be eaten. + const quote = (value: string): string => `"${value.replaceAll('\\', '/')}"` + + const readLedger = (): AgentLedgerEntry[] => { + if (!existsSync(ledgerPath)) { + return [] + } + return readFileSync(ledgerPath, 'utf8') + .split(/\r?\n/) + .filter(Boolean) + .flatMap((line) => { + try { + return [JSON.parse(line) as AgentLedgerEntry] + } catch { + // A torn final line just means the agent is mid-append; the poll retries. + return [] + } + }) + } + + return { + launchCommand: `node ${quote(scriptPath)} ${quote(ledgerPath)} ${quote(controlPath)}`, + setTitle: (title: string) => writeFileSync(controlPath, title), + readLedger, + readStdin: () => + readLedger() + .filter((entry) => entry.event === 'stdin') + .map((entry) => entry.data ?? '') + .join(''), + hasStarted: () => readLedger().some((entry) => entry.event === 'start'), + titleEmitCount: () => readLedger().filter((entry) => entry.event === 'title').length + } +} diff --git a/tests/e2e/helpers/orchestration-mail-store.ts b/tests/e2e/helpers/orchestration-mail-store.ts new file mode 100644 index 000000000..520498dad --- /dev/null +++ b/tests/e2e/helpers/orchestration-mail-store.ts @@ -0,0 +1,84 @@ +/** + * Direct reads of the orchestration mailbox for E2E assertions. + * + * 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. + */ +import path from 'node:path' +import Database from '../../../src/main/sqlite/sync-database' + +export type MailRow = { + id: string + type: string + to_handle: string + subject: string + read: number + delivered_at: string | null +} + +export type MailDisposition = 'pending' | 'pushed' | 'pulled' + +function withMailDb(userDataDir: string, read: (db: Database) => T): T { + const db = new Database(path.join(userDataDir, 'orchestration.db')) + try { + return read(db) + } finally { + db.close() + } +} + +export function readMailRow(userDataDir: string, id: string): MailRow | undefined { + return withMailDb(userDataDir, (db) => + db + .prepare('SELECT id, type, to_handle, subject, read, delivered_at FROM messages WHERE id = ?') + .get(id) + ) as MailRow | undefined +} + +export function readMailbox(userDataDir: string, toHandle: string): MailRow[] { + return withMailDb(userDataDir, (db) => + db + .prepare( + 'SELECT id, type, to_handle, subject, read, delivered_at FROM messages WHERE to_handle = ? ORDER BY sequence' + ) + .all(toHandle) + ) as MailRow[] +} + +/** + * Mark `handle` as the running coordinator — the state that makes push delivery + * withhold the synthesized Enter, because that prompt holds user-typed input. + * + * 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. + */ +export function startCoordinatorRun(userDataDir: string, handle: string): void { + withMailDb(userDataDir, (db) => { + db.prepare( + `INSERT INTO coordinator_runs (id, spec, status, coordinator_handle) + VALUES (?, 'e2e coordinator Enter carve-out', 'running', ?)` + ).run(`e2e-coordinator-${handle}`, handle) + }) +} + +/** + * How a row was consumed, if at all. + * + * `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 + * a push happened. + */ +export function mailDisposition(row: MailRow | undefined): MailDisposition | 'missing' { + if (!row) { + return 'missing' + } + if (row.read === 1) { + return 'pulled' + } + return row.delivered_at === null ? 'pending' : 'pushed' +} diff --git a/tests/e2e/orchestration-idle-mail-delivery.spec.ts b/tests/e2e/orchestration-idle-mail-delivery.spec.ts new file mode 100644 index 000000000..2d1ec41b6 --- /dev/null +++ b/tests/e2e/orchestration-idle-mail-delivery.spec.ts @@ -0,0 +1,406 @@ +/** + * Push-on-idle mail delivery, end to end (#12536). + * + * Orchestration hands a message to an agent one of two ways: a supervised agent + * pulls with `orchestration.check --wait`, and an unsupervised one has the text + * typed into its pane when the runtime sees it go idle. The push half was driven + * only by a busy→idle transition, so mail that arrived while the recipient was + * ALREADY idle waited for a transition that never came and sat unread forever. + * + * 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 + * synthesized Enter reached the agent process, which no store or DB read can. + * + * The ordering fixes on this path (microtask deferral, probe-window respawn, + * waiter reservations) are sub-millisecond races that E2E cannot steer; they are + * covered in src/main/runtime/orca-runtime.test.ts. What lives here is every + * behavior that needs a real process, a real title, or a real pane. + */ +import { test, expect } from './helpers/orca-app' +import type { ElectronApplication, Page } from '@stablyai/playwright-test' +import { waitForSessionReady, waitForActiveWorktree, ensureTerminalVisible } from './helpers/store' +import { + execInTerminal, + waitForActivePaneHookDescriptor, + waitForActivePanePtyId, + waitForActiveTerminalManager +} from './helpers/terminal' +import { RuntimeClient } from '../../src/cli/runtime-client' +import type { RuntimeTerminalListResult } from '../../src/shared/runtime-types' +import { + CODEX_IDLE_TITLE, + CODEX_WORKING_TITLE, + CURSOR_IDLE_TITLE, + createMailPaneAgent, + type MailPaneAgent +} from './helpers/orchestration-mail-pane-agent' +import { + mailDisposition, + 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' + +// 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. +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. +const NO_DELIVERY_SETTLE_MS = 3_000 + +type AgentPane = { + handle: string + agent: MailPaneAgent + ptyId: string +} + +type MailFixture = { + client: RuntimeClient + userDataDir: string + worktreeId: string + openAgentPane: () => Promise +} + +/** + * Why retry: Electron can recreate the evaluated main-world context during + * startup, which surfaces as a one-off 'Execution context was destroyed' rather + * than a real failure. Same guard as installTerminalPtyWriteSpy. + */ +async function readUserDataDir(electronApp: ElectronApplication): Promise { + for (let attempt = 1; ; attempt += 1) { + try { + return await electronApp.evaluate(({ app }) => app.getPath('userData')) + } catch (error) { + const transient = + error instanceof Error && error.message.includes('Execution context was destroyed') + if (!transient || attempt >= 5) { + throw error + } + await new Promise((resolve) => setTimeout(resolve, 250)) + } + } +} + +async function setUpMailFixture( + orcaPage: Page, + electronApp: ElectronApplication +): Promise { + await waitForSessionReady(orcaPage) + const worktreeId = await waitForActiveWorktree(orcaPage) + await ensureTerminalVisible(orcaPage) + await waitForActiveTerminalManager(orcaPage) + + const userDataDir = await readUserDataDir(electronApp) + const client = new RuntimeClient(userDataDir, 30_000, null, null) + + // Why: the renderer publishes the active worktree before the runtime finishes + // registering it, and terminal.create resolves its selector against the + // runtime — racing that yields selector_not_found, not a slow create. + await expect + .poll( + async () => { + const listed = await client.call<{ worktrees: { id: string }[] }>('worktree.list', {}) + return listed.result.worktrees.some((worktree) => worktree.id === worktreeId) + }, + { timeout: 60_000, message: 'runtime never registered the active worktree' } + ) + .toBe(true) + + const openAgentPane = async (): Promise => { + // The fixture's pane is already mounted, so its leaf exists — which is what + // push delivery resolves the write target through. + const ptyId = await waitForActivePanePtyId(orcaPage) + const { paneKey } = await waitForActivePaneHookDescriptor(orcaPage) + const resolved = await client.call<{ terminal: { handle: string } }>('terminal.resolvePane', { + paneKey + }) + const handle = resolved.result.terminal.handle + + // Why prove the shell echoes first: keystrokes typed at a shell that has not + // reached its prompt are simply dropped, and the agent then never starts for + // a reason unrelated to anything under test. + await waitForPtyShellEcho(orcaPage, ptyId, 60_000) + const agent = createMailPaneAgent() + await execInTerminal(orcaPage, ptyId, agent.launchCommand) + await expect + .poll(() => agent.hasStarted(), { timeout: 60_000, message: 'agent never started' }) + .toBe(true) + return { handle, agent, ptyId } + } + + return { client, userDataDir, worktreeId, openAgentPane } +} + +/** Wait until the runtime has observed `title` as a LIVE frame from the pane. */ +async function waitForObservedTitle( + client: RuntimeClient, + handle: string, + title: string +): Promise { + await expect + .poll( + async () => { + const listed = await client.call('terminal.list') + return listed.result.terminals.find((entry) => entry.handle === handle)?.title ?? null + }, + { timeout: 30_000, message: `runtime never observed the title ${title}` } + ) + .toBe(title) +} + +/** Put the pane in the state #12536 is about: idle, observed live, no transition pending. */ +async function driveToLiveIdle(client: RuntimeClient, pane: AgentPane): Promise { + 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) +} + +async function sendMail( + client: RuntimeClient, + to: string, + overrides: { subject: string; type?: string; body?: string } +): Promise { + const sent = await client.call<{ message: { id: string } }>('orchestration.send', { + to, + from: 'e2e-sender', + subject: overrides.subject, + body: overrides.body ?? 'e2e body', + type: overrides.type ?? 'status' + }) + return sent.result.message.id +} + +async function expectPushed(pane: AgentPane, subject: string): Promise { + await expect + .poll(() => pane.agent.readStdin(), { + timeout: DELIVERY_TIMEOUT_MS, + message: 'banner never reached the agent process' + }) + .toContain(BANNER_PREFIX) + expect(pane.agent.readStdin()).toContain(`Subject: ${subject}`) +} + +/** + * The synthesized Enter is a separate write ~500ms after the banner. The banner + * 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. + */ +async function expectSubmitted(pane: AgentPane): Promise { + await expect + .poll(() => pane.agent.readStdin().includes('\r'), { + timeout: DELIVERY_TIMEOUT_MS, + message: 'orchestration never synthesized Enter' + }) + .toBe(true) +} + +/** Inverse of expectSubmitted, for the panes whose submit stays user-owned. */ +function expectNotSubmitted(pane: AgentPane): void { + expect(pane.agent.readStdin()).not.toContain('\r') +} + +/** + * Why a fixed wait and not expect.poll: poll settles the instant the value + * matches, so polling for 'pending' would pass before the push had any chance + * to run and would assert nothing at all. The window has to elapse in full. + */ +async function expectStaysPending( + page: Page, + userDataDir: string, + pane: AgentPane, + messageId: string +): Promise { + // The row must exist first, or "pending" could just mean the send never landed. + 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) +} + +test.describe('orchestration push-on-idle mail delivery', () => { + test('delivers mail that arrives while the agent is already idle', async ({ + orcaPage, + electronApp + }) => { + test.setTimeout(180_000) + const { client, userDataDir, openAgentPane } = await setUpMailFixture(orcaPage, electronApp) + const pane = await openAgentPane() + await driveToLiveIdle(client, pane) + + // The regression: no busy→idle edge follows this send, so before #12536 the + // row stayed pending until something unrelated made the agent transition. + const subject = 'Already idle delivery' + const messageId = await sendMail(client, pane.handle, { subject }) + + await expectPushed(pane, subject) + await expectSubmitted(pane) + await expect + .poll(() => mailDisposition(readMailRow(userDataDir, messageId)), { + timeout: DELIVERY_TIMEOUT_MS + }) + .toBe('pushed') + }) + + test('holds mail while the agent is working and releases it on the idle frame', async ({ + orcaPage, + electronApp + }) => { + test.setTimeout(180_000) + const { client, userDataDir, openAgentPane } = await setUpMailFixture(orcaPage, electronApp) + const pane = await openAgentPane() + pane.agent.setTitle(CODEX_WORKING_TITLE) + await waitForObservedTitle(client, pane.handle, CODEX_WORKING_TITLE) + + const subject = 'Held while working' + const messageId = await sendMail(client, pane.handle, { subject }) + await expectStaysPending(orcaPage, userDataDir, pane, messageId) + + // 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 expect + .poll(() => mailDisposition(readMailRow(userDataDir, messageId)), { + timeout: DELIVERY_TIMEOUT_MS + }) + .toBe('pushed') + }) + + // Guards the null→idle path rather than reproducing #12536: a fresh pane has + // no status, so idle IS a transition here. The no-transition variant needs a + // restore-seeded idle and lives in orchestration-idle-mail-restore.spec.ts. + test('delivers mail queued before a fresh agent has reported any status', async ({ + orcaPage, + electronApp + }) => { + test.setTimeout(180_000) + const { client, userDataDir, openAgentPane } = await setUpMailFixture(orcaPage, electronApp) + const pane = await openAgentPane() + + // No title at all yet — the pane has no live agent status, which is where a + // resumed agent sits before it paints its prompt. + const subject = 'First live idle frame' + const messageId = await sendMail(client, pane.handle, { subject }) + await expectStaysPending(orcaPage, userDataDir, pane, messageId) + + // 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 expectSubmitted(pane) + }) + + test('leaves the mail to a live waiter instead of pushing it into the pane', async ({ + orcaPage, + electronApp + }) => { + test.setTimeout(180_000) + const { client, userDataDir, openAgentPane } = await setUpMailFixture(orcaPage, electronApp) + const pane = await openAgentPane() + await driveToLiveIdle(client, pane) + + // A supervised agent is parked in a long-poll. Pushing as well would deliver + // the same row twice — check consumes by `read` and push stamps + // `delivered_at`, so neither marker hides the row from the other. + // Why peek: this pane is bound to no Run, and that legacy mailbox refuses a + // consuming read. Peek still registers the same unfiltered waiter, which is + // what suppresses the push — the pull's own bookkeeping is not under test. + const waiting = client.call<{ messages: { subject: string }[] }>('orchestration.check', { + terminal: pane.handle, + peek: true, + wait: true, + timeoutMs: 30_000 + }) + // Why a settle: the waiter must be registered before the send, or the send + // correctly sees no consumer and this asserts the wrong branch. + await orcaPage.waitForTimeout(1_000) + + const subject = 'Waiter claims it' + const messageId = await sendMail(client, pane.handle, { subject }) + + const pulled = await waiting + expect(pulled.result.messages.map((message) => message.subject)).toContain(subject) + expect(pane.agent.readStdin()).not.toContain(BANNER_PREFIX) + // 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') + }) + + test('pushes to the pane when the only waiter filters this message type out', async ({ + orcaPage, + electronApp + }) => { + test.setTimeout(180_000) + const { client, userDataDir, openAgentPane } = await setUpMailFixture(orcaPage, electronApp) + const pane = await openAgentPane() + await driveToLiveIdle(client, pane) + + // A waiter scoped to worker_done never returns a status row, so treating it + // as this message's consumer would strand the row exactly as #12536 did. + const waiting = client + .call('orchestration.check', { + terminal: pane.handle, + types: 'worker_done', + wait: true, + timeoutMs: 8_000 + }) + .catch(() => undefined) + await orcaPage.waitForTimeout(1_000) + + const subject = 'Filtered waiter' + const messageId = await sendMail(client, pane.handle, { subject, type: 'status' }) + + await expectPushed(pane, subject) + await expect + .poll(() => mailDisposition(readMailRow(userDataDir, messageId)), { + timeout: DELIVERY_TIMEOUT_MS + }) + .toBe('pushed') + await waiting + }) + + test('writes the banner but never Enter for the active coordinator pane', async ({ + orcaPage, + electronApp + }) => { + test.setTimeout(180_000) + const { client, userDataDir, openAgentPane } = await setUpMailFixture(orcaPage, electronApp) + const pane = await openAgentPane() + 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' + await sendMail(client, pane.handle, { subject }) + + await expectPushed(pane, subject) + await orcaPage.waitForTimeout(2_000) + expectNotSubmitted(pane) + }) + + test('writes the banner but never Enter for a Cursor agent pane', async ({ + orcaPage, + electronApp + }) => { + test.setTimeout(180_000) + const { client, openAgentPane } = await setUpMailFixture(orcaPage, electronApp) + const pane = await openAgentPane() + // Cursor treats injected PTY text as editable prompt content, so submitting + // has to stay under user control there too. + pane.agent.setTitle(CURSOR_IDLE_TITLE) + await waitForObservedTitle(client, pane.handle, CURSOR_IDLE_TITLE) + + const subject = 'Cursor no-submit' + await sendMail(client, pane.handle, { subject }) + + await expectPushed(pane, subject) + 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 new file mode 100644 index 000000000..6e38b9fd8 --- /dev/null +++ b/tests/e2e/orchestration-idle-mail-restore.spec.ts @@ -0,0 +1,190 @@ +/** + * Mail must survive a restart: never injected on restored state alone, always + * delivered 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. + * + * Scope, stated plainly: this covers the restart path, not the + * `lastAgentStatusObservedLive` gate itself. The seed only reaches leaves that + * already exist when pty:spawn returns the restore payload, and a cold relaunch + * publishes its graph after that — so the leaf here comes back with no agent + * status rather than a seeded idle, and this spec passes with the gate removed. + * The gate is pinned in src/main/runtime/orca-runtime.test.ts + * ('does not push on a cold-restore seeded idle status with no live + * observation'), which can stage that ordering directly. What earns this spec + * its two Electron launches is that neither half of the restart behavior above + * is reachable from a single-launch spec at all. + */ +import { existsSync, readFileSync } from 'node:fs' +import type { ElectronApplication } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { TEST_REPO_PATH_FILE } from './global-setup' +import { attachRepoAndOpenTerminal, createRestartSession } from './helpers/orca-restart' +import { + execInTerminal, + waitForActivePaneHookDescriptor, + waitForActivePanePtyId +} from './helpers/terminal' +import { RuntimeClient } from '../../src/cli/runtime-client' +import type { RuntimeTerminalListResult } from '../../src/shared/runtime-types' +import { + CODEX_IDLE_TITLE, + CODEX_WORKING_TITLE, + createMailPaneAgent +} from './helpers/orchestration-mail-pane-agent' +import { mailDisposition, readMailRow } from './helpers/orchestration-mail-store' +import { waitForPtyShellEcho } from './terminal-pty-readiness' + +const BANNER_PREFIX = '--- Orchestration Messages' +const NO_DELIVERY_SETTLE_MS = 5_000 +const DELIVERY_TIMEOUT_MS = 20_000 + +test.describe.configure({ mode: 'serial' }) + +async function waitForRegisteredWorktree(client: RuntimeClient, worktreeId: string): Promise { + await expect + .poll( + async () => { + const listed = await client.call<{ worktrees: { id: string }[] }>('worktree.list', {}) + return listed.result.worktrees.some((worktree) => worktree.id === worktreeId) + }, + { timeout: 60_000, message: 'runtime never registered the worktree' } + ) + .toBe(true) +} + +async function waitForObservedTitle( + client: RuntimeClient, + handle: string, + title: string +): Promise { + await expect + .poll( + async () => { + const listed = await client.call('terminal.list') + return listed.result.terminals.find((entry) => entry.handle === handle)?.title ?? null + }, + { timeout: 30_000, message: `runtime never observed the title ${title}` } + ) + .toBe(title) +} + +test('keeps mail pending across a restart and delivers it when the agent reports live', async (// oxlint-disable-next-line no-empty-pattern -- this spec owns both Electron launches and opts out of the shared app fixture. +{}, testInfo) => { + test.setTimeout(300_000) + const repoPath = existsSync(TEST_REPO_PATH_FILE) + ? readFileSync(TEST_REPO_PATH_FILE, 'utf8').trim() + : '' + test.skip(!repoPath || !existsSync(repoPath), 'Global setup did not produce a seeded test repo') + + const session = createRestartSession(testInfo) + let firstApp: ElectronApplication | null = null + let secondApp: ElectronApplication | null = null + + try { + const first = await session.launch() + firstApp = first.app + const worktreeId = await attachRepoAndOpenTerminal(first.page, repoPath) + const firstClient = new RuntimeClient(session.userDataDir, 30_000, null, null) + await waitForRegisteredWorktree(firstClient, worktreeId) + + // The pane attachRepoAndOpenTerminal already opened is mounted, so its leaf + // exists; terminal.create would instead race a 10s renderer graph-sync wait + // that a headless CI renderer loses. + const ptyId = await waitForActivePanePtyId(first.page) + const { paneKey } = await waitForActivePaneHookDescriptor(first.page) + const originalHandle = ( + await firstClient.call<{ terminal: { handle: string } }>('terminal.resolvePane', { paneKey }) + ).result.terminal.handle + const originalPtyId = ptyId + + // Keystrokes typed before the shell reaches its prompt are dropped outright. + await waitForPtyShellEcho(first.page, ptyId, 60_000) + const agent = createMailPaneAgent() + await execInTerminal(first.page, ptyId, agent.launchCommand) + await expect + .poll(() => agent.hasStarted(), { timeout: 60_000, message: 'agent never started' }) + .toBe(true) + + agent.setTitle(CODEX_WORKING_TITLE) + await waitForObservedTitle(firstClient, originalHandle, CODEX_WORKING_TITLE) + agent.setTitle(CODEX_IDLE_TITLE) + await waitForObservedTitle(firstClient, originalHandle, CODEX_IDLE_TITLE) + const titlesBeforeRestart = agent.titleEmitCount() + + await session.close(firstApp) + firstApp = null + + const second = await session.launch() + secondApp = second.app + const secondClient = new RuntimeClient(session.userDataDir, 30_000, null, null) + + // The PTY outlives the app, so the restored pane is found by process + // identity; its handle may or may not be the one the first launch minted. + let restoredHandle: string | null = null + await expect + .poll( + async () => { + const listed = await secondClient.call('terminal.list') + const restored = listed.result.terminals.find( + (entry) => entry.ptyId === originalPtyId && entry.writable + ) + restoredHandle = restored?.handle ?? null + return restored?.title ?? null + }, + { timeout: 120_000, message: 'agent pane never came back writable after restart' } + ) + .toBe(CODEX_IDLE_TITLE) + expect(restoredHandle).toBeTruthy() + + // The process has emitted nothing since the restart, so whatever the runtime + // believes about this pane's status came back with the graph, not from it. + expect(agent.titleEmitCount()).toBe(titlesBeforeRestart) + + const sent = await secondClient.call<{ message: { id: string } }>('orchestration.send', { + to: restoredHandle!, + from: 'e2e-sender', + subject: 'Seeded idle must wait', + body: 'e2e body', + type: 'status' + }) + const messageId = sent.result.message.id + + // Why a fixed wait: expect.poll would settle on the first 'pending' reading, + // before the push had any chance to run, and assert nothing. + 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) + + // 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. + agent.setTitle(CODEX_IDLE_TITLE) + await expect + .poll(() => agent.titleEmitCount(), { timeout: 30_000 }) + .toBeGreaterThan(titlesBeforeRestart) + await expect + .poll(() => agent.readStdin(), { + 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') + } finally { + if (firstApp) { + await session.close(firstApp) + } + if (secondApp) { + await session.close(secondApp) + } + await session.dispose() + } +})