diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index b3e3155c8e3..4d7413dd4ad 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -33601,6 +33601,209 @@ describe('OrcaRuntimeService', () => { } }) + it('delivers to a restored idle pane whose agent never emits another title', 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(), + // The agent is genuinely there and at its prompt — this is what + // separates a restored idle from a restored maybe-working pane. + getForegroundProcess: async () => 'claude' + }) + syncSinglePty(runtime) + + const [terminal] = (await runtime.listTerminals()).terminals + // A real Claude idle title, captured from a live pane. + runtime.seedTerminalRestoreTail('pty-1', { + lastTitle: '✳ Resuming session after a break' + }) + const message = db.insertMessage({ + from: 'term_worker', + to: terminal.handle, + subject: 'DO NOT SHIP PR 250', + type: 'worker_done' + }) + + runtime.notifyMessageArrived(terminal.handle, 'worker_done') + await Promise.resolve() + + // Why no live frame follows: an idle agent TUI is silent. Claude paints + // its title on the working→idle edge and then emits nothing until it is + // given work, so the liveness edge the seeded gate waits for never comes + // and the row strands for as long as the pane stays idle. + await vi.advanceTimersByTimeAsync(10 * 60_000) + + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 1 orchestration message') + ) + await vi.advanceTimersByTimeAsync(600) + expect(write).toHaveBeenCalledWith('pty-1', '\r') + expect(message.delivered_at).toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('delivers to a restored pane that came back with no agent status at all', 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 () => 'claude' + }) + // Why no seed: a cold relaunch returns its restore payload before the + // renderer publishes the graph, so the seeded title never reaches a leaf + // and the pane comes back with lastAgentStatus null. This is the shape the + // restart e2e reproduces, and the seeded-idle case above does not cover it. + syncSinglePty(runtime) + + const [terminal] = (await runtime.listTerminals()).terminals + const message = db.insertMessage({ + from: 'term_worker', + to: terminal.handle, + subject: 'restored with no status', + type: 'worker_done' + }) + + runtime.notifyMessageArrived(terminal.handle, 'worker_done') + await Promise.resolve() + await vi.advanceTimersByTimeAsync(10 * 60_000) + + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 1 orchestration message') + ) + expect(message.delivered_at).toBeNull() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('does not push to a restored idle pane that emits output inside the window', 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 () => 'claude' + }) + syncSinglePty(runtime) + + const [terminal] = (await runtime.listTerminals()).terminals + runtime.seedTerminalRestoreTail('pty-1', { lastTitle: 'Codex done' }) + db.insertMessage({ from: 'sender', to: terminal.handle, subject: 'restored but busy' }) + + runtime.notifyMessageArrived(terminal.handle, 'status') + await Promise.resolve() + + // A byte inside the window — a spinner repaint, tool output, or a human + // mid-keystroke. The seeded idle is historical, so this is the case the + // gate protects: submitting here would type into a working agent. + await vi.advanceTimersByTimeAsync(1_000) + runtime.onPtyData('pty-1', 'building...', 100) + await vi.advanceTimersByTimeAsync(30_000) + + expect(write).not.toHaveBeenCalled() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('does not push to a restored idle pane whose foreground is not an agent', 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(), + // The agent exited across the restart and the shell owns the pane; the + // seeded title is all that is left of it. + getForegroundProcess: async () => 'bash' + }) + syncSinglePty(runtime) + + const [terminal] = (await runtime.listTerminals()).terminals + runtime.seedTerminalRestoreTail('pty-1', { lastTitle: 'Codex done' }) + db.insertMessage({ from: 'sender', to: terminal.handle, subject: 'no agent left' }) + + runtime.notifyMessageArrived(terminal.handle, 'status') + await Promise.resolve() + await vi.advanceTimersByTimeAsync(30_000) + + expect(write).not.toHaveBeenCalled() + db.close() + } finally { + vi.useRealTimers() + } + }) + + it('opens one quiescence window for a burst of mail to a restored idle pane', async () => { + vi.useFakeTimers() + try { + const runtime = new OrcaRuntimeService(store) + const db = new InMemoryOrchestrationMessages() + const write = vi.fn().mockReturnValue(true) + // Why a pending promise and not mockResolvedValue: a probe that settles + // before the next window opens would hide a missing guard behind the + // early return on the flag it sets. Holding every probe in flight is what + // makes the count mean "windows opened". + const foregroundResolvers: ((process: string) => void)[] = [] + const getForegroundProcess = vi.fn( + () => + new Promise((resolve) => { + foregroundResolvers.push(resolve) + }) + ) + setInMemoryOrchestrationMessages(runtime, db) + runtime.setPtyController({ write, kill: vi.fn(), getForegroundProcess }) + syncSinglePty(runtime) + + const [terminal] = (await runtime.listTerminals()).terminals + runtime.seedTerminalRestoreTail('pty-1', { lastTitle: 'Codex done' }) + for (let index = 0; index < 5; index += 1) { + db.insertMessage({ from: 'sender', to: terminal.handle, subject: `burst ${index}` }) + runtime.notifyMessageArrived(terminal.handle, 'status') + } + await Promise.resolve() + await vi.advanceTimersByTimeAsync(2_500) + + // Five arrivals, one probe — otherwise a busy Run spawns a foreground + // read per message against the same pane, each able to retry for seconds. + expect(getForegroundProcess).toHaveBeenCalledTimes(1) + + foregroundResolvers[0]?.('claude') + await vi.advanceTimersByTimeAsync(1_000) + expect(write).toHaveBeenCalledWith( + 'pty-1', + expect.stringContaining('You have 5 orchestration messages') + ) + db.close() + } finally { + vi.useRealTimers() + } + }) + it('lets a resolved check consume its rows before a later same-tick notify pushes', async () => { vi.useFakeTimers() try { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index b352234c66e..e5789a62479 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1756,6 +1756,11 @@ const MOBILE_TERMINAL_CREATE_RESULT_TTL_MS = 60_000 const WORKTREE_CREATE_RESULT_TTL_MS = 60_000 const FOREGROUND_AGENT_WRAPPER_RETRY_INTERVAL_MS = 150 const FOREGROUND_AGENT_WRAPPER_RETRY_TIMEOUT_MS = 6_500 +// Why this long: it must exceed any agent's spinner repaint period, since one +// frame inside the window is what distinguishes a working pane from an idle +// one. Agent TUIs repaint several times a second, so this is an order of +// magnitude of headroom, and it only delays mail on a restored pane. +const RESTORED_IDLE_QUIESCENCE_MS = 2_000 const BRACKETED_PASTE_BEGIN = '\x1b[200~' const BRACKETED_PASTE_END = '\x1b[201~' const BRACKETED_PASTE_QUIET_MS = 1500 @@ -16582,6 +16587,9 @@ export class OrcaRuntimeService { // Why: probe dedupe shares one promise across callers, but each caller's // continuation would re-deliver the same unread rows; arm one per pty. private readonly probeDeferredDeliveryPtyIds = new Set() + // Why: mail arriving in a burst would otherwise open one quiescence window + // per message against the same pane. + private readonly restoredIdleProbePtyIds = new Set() private controllerKnowsPtyIsLive(ptyId: string): boolean { try { @@ -31345,12 +31353,91 @@ export class OrcaRuntimeService { // for a live observation to authorize it. if (leaf.lastAgentStatus === 'idle' && leaf.lastAgentStatusObservedLive) { this.deliverPendingMessages(leaf, { mailboxHandle: handle, reservedTypes }) + return + } + // Why null counts: a cold relaunch returns its restore payload before the + // renderer publishes the graph, so the seeded status lands on a pty with + // no leaves yet and the pane comes back carrying no status at all. That is + // the commonest shape of this bug, not the seeded idle. 'working' and + // 'permission' are positive evidence of not-idle and never probe. + if (leaf.lastAgentStatus === 'idle' || leaf.lastAgentStatus === null) { + this.confirmRestoredIdleThenDeliver(leaf, handle) } } catch { // Unknown/stale handles can't be pointed now; the persisted message stays available via explicit check or future idle delivery. } } + /** + * Authorize a seeded `idle` that no live frame will ever confirm. + * + * Why this exists: an idle agent TUI is silent. Claude paints its title on + * the working→idle edge and then emits nothing until it is given work, so a + * pane restored while its agent sits at the prompt never produces the live + * observation the seeded gate waits for, and its mail strands for as long as + * the pane stays idle. Quiescence plus a recognized foreground agent + * establishes the same fact the missing title would have carried: a byte in + * the window means a spinner, tool output, or a human typing — none of which + * are safe to submit into — and a foreground that is not a known agent means + * there is nothing there to wake. + */ + private confirmRestoredIdleThenDeliver(leaf: RuntimeLeafRecord, mailboxHandle: string): void { + const ptyId = leaf.ptyId + if (!ptyId || !this.ptyController || this.restoredIdleProbePtyIds.has(ptyId)) { + return + } + const leafKey = this.getLeafKey(leaf.tabId, leaf.leafId) + const outputAtBeforeWindow = leaf.lastOutputAt + this.restoredIdleProbePtyIds.add(ptyId) + setTimeout(() => { + void (async () => { + try { + // Why re-read rather than trust the captured leaf: a graph resync + // replaces leaf objects, and a live frame during the window already + // delivered on real evidence. + const settled = this.leaves.get(leafKey) + if ( + !settled || + settled.ptyId !== ptyId || + (settled.lastAgentStatus !== 'idle' && settled.lastAgentStatus !== null) || + settled.lastAgentStatusObservedLive || + settled.lastOutputAt !== outputAtBeforeWindow + ) { + return + } + const foregroundProcess = await this.ptyController?.getForegroundProcess(ptyId) + if ( + !foregroundProcess || + !(await this.isRecognizedForegroundAgentProcess(ptyId, foregroundProcess)) + ) { + return + } + // Why re-check after the await: the foreground read is itself slow + // enough for the pane to start working underneath it. + const confirmed = this.leaves.get(leafKey) + if ( + !confirmed || + confirmed.ptyId !== ptyId || + (confirmed.lastAgentStatus !== 'idle' && confirmed.lastAgentStatus !== null) || + confirmed.lastOutputAt !== outputAtBeforeWindow + ) { + return + } + // The probe IS the live observation, so later mail for this pane + // delivers immediately instead of paying the window again. + confirmed.lastAgentStatusObservedLive = true + // Why no reservedTypes: that snapshot guards a sub-millisecond race + // with a check resolving in the same drain, which this window has + // long outlived. Replaying it here could suppress a type forever; + // deliverPendingMessages re-reads the waiters that are still live. + this.deliverPendingMessages(confirmed, { mailboxHandle }) + } finally { + this.restoredIdleProbePtyIds.delete(ptyId) + } + })() + }, RESTORED_IDLE_QUIESCENCE_MS) + } + private deliverPendingMessagesForLeaf(leaf: RuntimeLeafRecord): void { this.deliverPendingMessages(leaf) if (!this._orchestrationDb) { diff --git a/tests/e2e/helpers/orchestration-mail-pane-agent.ts b/tests/e2e/helpers/orchestration-mail-pane-agent.ts index df9d0311d8c..ed47ab2d6fa 100644 --- a/tests/e2e/helpers/orchestration-mail-pane-agent.ts +++ b/tests/e2e/helpers/orchestration-mail-pane-agent.ts @@ -21,7 +21,15 @@ * 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 { + copyFileSync, + existsSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync +} from 'node:fs' import os from 'node:os' import path from 'node:path' @@ -111,13 +119,32 @@ process.once('exit', () => { } }) +/** + * Launch node under `processName` so the pane's foreground process is one Orca + * recognizes as an agent. Bare `node` is only a wrapper name, and delivery paths + * that require a *recognized* agent refuse it — so a spec asserting one of those + * cannot use the default launcher. The kernel records the path used at exec, so + * a link is enough to change what `getForegroundProcess` reports. + */ +function linkNodeAs(dir: string, processName: string): string { + const linkPath = path.join(dir, processName) + try { + symlinkSync(process.execPath, linkPath) + } catch { + // Windows needs a privilege for symlinks; a copy reports the same name. + copyFileSync(process.execPath, linkPath) + } + return linkPath +} + /** One isolated agent: its own script copy, ledger, and control file. */ -export function createMailPaneAgent(): MailPaneAgent { +export function createMailPaneAgent(options: { processName?: string } = {}): 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') + const nodePath = options.processName ? linkNodeAs(dir, options.processName) : 'node' writeFileSync(scriptPath, AGENT_SOURCE) writeFileSync(ledgerPath, '') @@ -143,7 +170,12 @@ export function createMailPaneAgent(): MailPaneAgent { } return { - launchCommand: `node ${quote(scriptPath)} ${quote(ledgerPath)} ${quote(controlPath)}`, + launchCommand: [ + options.processName ? quote(nodePath) : nodePath, + quote(scriptPath), + quote(ledgerPath), + quote(controlPath) + ].join(' '), setTitle: (title: string) => writeFileSync(controlPath, title), readLedger, readStdin: () => diff --git a/tests/e2e/orchestration-idle-mail-delivery.spec.ts b/tests/e2e/orchestration-idle-mail-delivery.spec.ts index fcfe81005f2..b876501b948 100644 --- a/tests/e2e/orchestration-idle-mail-delivery.spec.ts +++ b/tests/e2e/orchestration-idle-mail-delivery.spec.ts @@ -285,7 +285,10 @@ test.describe('orchestration push-on-idle mail delivery', () => { 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. + // resumed agent sits before it paints its prompt. This fixture's agent runs + // under bare `node`, which is a wrapper name rather than a recognized agent, + // so the quiescence path that now covers a restored pane deliberately + // refuses it and the first live frame is still what releases the row. const subject = 'First live idle frame' const messageId = await sendMail(client, pane.handle, { subject }) await expectStaysPending(orcaPage, userDataDir, pane, messageId) diff --git a/tests/e2e/orchestration-idle-mail-restore.spec.ts b/tests/e2e/orchestration-idle-mail-restore.spec.ts index 4e528614fa6..e84a4fc670e 100644 --- a/tests/e2e/orchestration-idle-mail-restore.spec.ts +++ b/tests/e2e/orchestration-idle-mail-restore.spec.ts @@ -8,16 +8,23 @@ * is a memory, not an observation. Typing on it would submit into an agent that * may be mid-turn. * - * 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. + * The catch that makes waiting insufficient: an idle agent TUI is silent. It + * paints its title on the working→idle edge and emits nothing after, so a pane + * restored while its agent sits at the prompt never produces the frame the gate + * waits for. Mail addressed to it strands until a human gives the agent + * something to do. So the runtime probes instead — quiescence plus a recognized + * foreground agent stands in for the title that is never coming. + * + * The two tests split on exactly that probe. The first pane runs under a name + * Orca recognizes as an agent and is delivered to without ever speaking again; + * the second runs under bare `node`, a wrapper rather than an agent, so the + * probe refuses it and only a live frame releases the row. Both leaves come + * back with no agent status at all — the seed reaches only leaves that exist + * when pty:spawn returns the restore payload, and a cold relaunch publishes its + * graph after that. The seeded-idle ordering is staged directly in + * src/main/runtime/orca-runtime.test.ts. What earns this spec its Electron + * launches is that none 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' @@ -73,6 +80,102 @@ async function waitForObservedTitle( .toBe(title) } +test('delivers mail after a restart to a quiet pane whose agent never speaks again', 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) + + 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 + + await waitForPtyShellEcho(first.page, ptyId, 60_000) + // Why a recognized process name: an agent Orca cannot name is not something + // it will submit Enter into, so the quiescence path refuses it by design. + const agent = createMailPaneAgent({ processName: 'codex' }) + 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) + + 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 === ptyId && 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() + expect(agent.titleEmitCount()).toBe(titlesBeforeRestart) + + const sent = await secondClient.call<{ message: { id: string } }>('orchestration.send', { + to: restoredHandle!, + from: 'e2e-sender', + subject: 'Quiet restored agent', + body: 'e2e body', + type: 'status' + }) + const messageId = sent.result.message.id + + // The regression this guards: a real idle agent emits nothing while it + // waits, so before this fix the pane sat unreachable until a human gave it + // something to do. No title is set anywhere below — the pointer arriving is + // the whole point. + await expect + .poll(() => agent.readStdin(), { + timeout: DELIVERY_TIMEOUT_MS, + message: 'restored quiet agent never received the pointer' + }) + .toContain(POINTER_COMMAND) + expect(agent.titleEmitCount()).toBe(titlesBeforeRestart) + expect(mailDisposition(readMailRow(session.userDataDir, messageId))).toBe('pending') + } finally { + if (firstApp) { + await session.close(firstApp) + } + if (secondApp) { + await session.close(secondApp) + } + await session.dispose() + } +}) + 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)