From f173226664f0f763bb25cafa3adf677654110e86 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 29 Aug 2026 07:05:00 -0700 Subject: [PATCH 1/2] fix(pi): settle OMP status from the agent_end contract OMP exposes no agent_settled hook and ctx.isIdle() can stay false after a finished turn, so Orca's idle-recheck loop backed off and spun forever and the pane stayed "working" indefinitely. OMP instead marks non-terminal agent_end events with willContinue; honor that for configured and runtime-routed OMP and treat an absent flag as terminal. Extracted from #15658, which bundles this with a launch-authority change that conflicts with in-flight #17077. Only the settle half lands here. STA-4130 Co-authored-by: Bing.Z --- ...ent-status-extension-omp-lifecycle.test.ts | 89 ++++++++ .../pi/agent-status-extension-source.test.ts | 196 +----------------- .../pi/agent-status-extension-test-harness.ts | 190 +++++++++++++++++ src/main/pi/agent-status-handler-source.ts | 9 +- 4 files changed, 291 insertions(+), 193 deletions(-) create mode 100644 src/main/pi/agent-status-extension-omp-lifecycle.test.ts create mode 100644 src/main/pi/agent-status-extension-test-harness.ts diff --git a/src/main/pi/agent-status-extension-omp-lifecycle.test.ts b/src/main/pi/agent-status-extension-omp-lifecycle.test.ts new file mode 100644 index 00000000000..e759217cd63 --- /dev/null +++ b/src/main/pi/agent-status-extension-omp-lifecycle.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, it, vi } from 'vitest' + +import { createAgentStatusExtensionHarness } from './agent-status-extension-test-harness' + +function postedHookNames(fetchMock: ReturnType): string[] { + return fetchMock.mock.calls.map( + (call) => JSON.parse(String(call[1]?.body)).payload.hook_event_name as string + ) +} + +const OMP_RUNTIME_CASES = [ + ['configured OMP', { kind: 'omp' as const }], + ['title-routed OMP', { kind: 'pi' as const, title: 'omp' }], + ['argv-routed OMP', { kind: 'pi' as const, argv: ['node', '/usr/local/bin/omp'] }] +] as const + +describe('OMP agent_end contract', () => { + it.each(OMP_RUNTIME_CASES)( + 'keeps %s working when agent_end will continue', + async (_name, args) => { + vi.useFakeTimers() + try { + const harness = createAgentStatusExtensionHarness(args) + const context = { isIdle: vi.fn(() => true) } + + await harness.callHook('agent_start') + await harness.callHook('agent_end', { willContinue: true }, context) + await vi.advanceTimersByTimeAsync(1_000) + + expect(postedHookNames(harness.fetchMock)).toEqual(['agent_start']) + expect(context.isIdle).not.toHaveBeenCalled() + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } + } + ) + + it.each(OMP_RUNTIME_CASES)( + 'settles a completed %s turn without waiting for ctx.isIdle', + async (_name, args) => { + // Why: absent payload and absent flag are both terminal for a version that cannot send one. + for (const event of [{ willContinue: false }, {}, undefined]) { + const harness = createAgentStatusExtensionHarness(args) + const context = { isIdle: vi.fn(() => false) } + + await harness.callHook('agent_start') + await harness.callHook('agent_end', event, context) + + await vi.waitFor(() => + expect(postedHookNames(harness.fetchMock)).toEqual(['agent_start', 'agent_end']) + ) + expect(context.isIdle).not.toHaveBeenCalled() + } + } + ) + + it('settles a later terminal OMP agent_end after a continuation', async () => { + const harness = createAgentStatusExtensionHarness({ kind: 'omp' }) + const context = { isIdle: vi.fn(() => false) } + + await harness.callHook('agent_start') + await harness.callHook('agent_end', { willContinue: true }, context) + expect(postedHookNames(harness.fetchMock)).toEqual(['agent_start']) + + await harness.callHook('agent_end', { willContinue: false }, context) + await vi.waitFor(() => + expect(postedHookNames(harness.fetchMock)).toEqual(['agent_start', 'agent_end']) + ) + }) + + it('does not apply the OMP contract to Pi or Prime', async () => { + vi.useFakeTimers() + try { + for (const kind of ['pi', 'prime-agent'] as const) { + const harness = createAgentStatusExtensionHarness({ kind }) + const context = { isIdle: vi.fn(() => false) } + + await harness.callHook('agent_end', { willContinue: false }, context) + await vi.advanceTimersByTimeAsync(1_000) + + expect(postedHookNames(harness.fetchMock)).toEqual([]) + expect(context.isIdle).toHaveBeenCalled() + } + } finally { + vi.useRealTimers() + } + }) +}) diff --git a/src/main/pi/agent-status-extension-source.test.ts b/src/main/pi/agent-status-extension-source.test.ts index 45534662d55..fed179837db 100644 --- a/src/main/pi/agent-status-extension-source.test.ts +++ b/src/main/pi/agent-status-extension-source.test.ts @@ -1,193 +1,9 @@ -import { runInNewContext } from 'node:vm' -// TypeScript 7 is a native CLI; transpile tests still need the legacy JavaScript API. -import ts from 'typescript-api' import { describe, expect, it, vi } from 'vitest' -import { getPiAgentStatusExtensionSource } from './agent-status-extension-source' - -type HookContext = { - isIdle?: () => boolean - sessionManager?: { - getSessionId?: () => unknown - getSessionFile?: () => unknown - } -} - -type HookHandler = (event?: unknown, context?: HookContext) => Promise | void - -type FakeCurlChild = { - on: ReturnType - stdin: { - on: ReturnType - end: ReturnType - } -} - -type Harness = { - fetchMock: ReturnType - spawnMock: ReturnType - spawnedChildren: FakeCurlChild[] - fsMock: { - existsSync: ReturnType - readFileSync: ReturnType - statSync: ReturnType - } - handlers: Record - processEnv: Record - callHook: (name: string, event?: unknown, context?: HookContext) => Promise - // Re-invoke the extension factory in the same process (as Pi does on an - // in-process extension reload), swapping in the freshly registered handlers. - reload: () => void -} - -const BASE_ENV = { - ORCA_PANE_KEY: 'pane-1', - ORCA_AGENT_LAUNCH_TOKEN: 'launch-1', - ORCA_TAB_ID: 'tab-1', - ORCA_WORKTREE_ID: 'tree-1', - ORCA_AGENT_HOOK_PORT: '4321', - ORCA_AGENT_HOOK_TOKEN: 'token-1', - ORCA_AGENT_HOOK_ENV: 'env-1', - ORCA_AGENT_HOOK_VERSION: '1.2.3' -} satisfies Record - -// Why: ownership keys on process.pid, so reload and child-process tests need -// stable, distinct identities. -const SELF_PID = 4242 - -function createHarness(args: { - kind: 'pi' | 'omp' | 'prime-agent' - env?: Record - pid?: number - title?: string - argv?: string[] - existsSync?: (path: string) => boolean - readFileSync?: (path: string, encoding: string) => string - statSync?: (path: string) => { mtimeMs: number; size: number; ino: number } - fetchImpl?: (...params: Parameters) => Promise -}): Harness { - const fetchMock = vi.fn( - args.fetchImpl ?? - (async () => ({ - ok: true - })) - ) - - const spawnedChildren: FakeCurlChild[] = [] - const spawnMock = vi.fn(() => { - const child: FakeCurlChild = { - on: vi.fn(), - stdin: { - on: vi.fn(), - end: vi.fn() - } - } - spawnedChildren.push(child) - return child - }) - - const fsMock = { - existsSync: vi.fn(args.existsSync ?? (() => false)), - statSync: vi.fn( - args.statSync ?? - ((path: string) => { - throw Object.assign(new Error(`ENOENT: ${path}`), { code: 'ENOENT' }) - }) - ), - readFileSync: vi.fn( - args.readFileSync ?? - ((path: string) => { - throw Object.assign(new Error(`ENOENT: ${path}`), { code: 'ENOENT' }) - }) - ) - } - - const module = { - exports: {} as { default?: (pi: { on: (name: string, handler: HookHandler) => void }) => void } - } - const requireMock = vi.fn((specifier: string) => { - if (specifier === 'fs') { - return fsMock - } - if (specifier === 'child_process') { - return { spawn: spawnMock } - } - throw new Error(`unexpected require(${specifier})`) - }) - - const processMock = { - env: { - ...BASE_ENV, - ...(args.kind === 'prime-agent' ? { PRIME_AGENT_INTERNAL_DAEMON_WORKER: '1' } : {}), - ...args.env - }, - pid: args.pid ?? SELF_PID, - title: args.title ?? 'node', - argv: args.argv ?? ['node', '/usr/bin/orca'] - } - - const context = { - module, - exports: module.exports, - require: requireMock, - process: processMock, - fetch: fetchMock, - console: { - warn: vi.fn(), - error: vi.fn(), - log: vi.fn() - }, - Promise, - Buffer, - URL, - AbortController, - setTimeout, - clearTimeout - } as Record - context.globalThis = context - - const source = getPiAgentStatusExtensionSource(args.kind) - const output = ts.transpileModule(source, { - compilerOptions: { - module: ts.ModuleKind.CommonJS, - target: ts.ScriptTarget.ES2020 - } - }).outputText - runInNewContext(output, context) - - const register = module.exports.default - if (!register) { - throw new Error('expected default export from generated source') - } - - const handlers: Record = {} - const registerInto = (target: Record): void => { - register({ - on(name: string, handler: HookHandler) { - target[name] = handler - } - }) - } - registerInto(handlers) - - return { - fetchMock, - spawnMock, - spawnedChildren, - fsMock, - handlers, - processEnv: processMock.env, - callHook: async (name, event, hookContext) => { - await handlers[name]?.(event, hookContext) - }, - reload: () => { - for (const key of Object.keys(handlers)) { - delete handlers[key] - } - registerInto(handlers) - } - } -} +import { + AGENT_STATUS_EXTENSION_SELF_PID as SELF_PID, + createAgentStatusExtensionHarness as createHarness +} from './agent-status-extension-test-harness' describe('getPiAgentStatusExtensionSource', () => { it('registers Prime hooks only in the event-emitting daemon worker', () => { @@ -826,10 +642,10 @@ describe('getPiAgentStatusExtensionSource', () => { } }) - it('keeps reporting Pi-compatible agents once their agent_end handlers settle', async () => { + it('keeps polling Pi and Prime until their agent_end handlers settle', async () => { vi.useFakeTimers() try { - for (const kind of ['pi', 'omp', 'prime-agent'] as const) { + for (const kind of ['pi', 'prime-agent'] as const) { const harness = createHarness({ kind }) let idle = false const context = { isIdle: vi.fn(() => idle) } diff --git a/src/main/pi/agent-status-extension-test-harness.ts b/src/main/pi/agent-status-extension-test-harness.ts new file mode 100644 index 00000000000..eec2b615017 --- /dev/null +++ b/src/main/pi/agent-status-extension-test-harness.ts @@ -0,0 +1,190 @@ +import { runInNewContext } from 'node:vm' +// TypeScript 7 is a native CLI; transpile tests still need the legacy JavaScript API. +import ts from 'typescript-api' +import { vi } from 'vitest' + +import { getPiAgentStatusExtensionSource } from './agent-status-extension-source' + +export type HookContext = { + isIdle?: () => boolean + sessionManager?: { + getSessionId?: () => unknown + getSessionFile?: () => unknown + } +} + +export type HookHandler = (event?: unknown, context?: HookContext) => Promise | void + +type FakeCurlChild = { + on: ReturnType + stdin: { + on: ReturnType + end: ReturnType + } +} + +export type AgentStatusExtensionHarness = { + fetchMock: ReturnType + spawnMock: ReturnType + spawnedChildren: FakeCurlChild[] + fsMock: { + existsSync: ReturnType + readFileSync: ReturnType + statSync: ReturnType + } + handlers: Record + processEnv: Record + callHook: (name: string, event?: unknown, context?: HookContext) => Promise + // Re-invoke the extension factory in the same process (as Pi does on an + // in-process extension reload), swapping in the freshly registered handlers. + reload: () => void +} + +const BASE_ENV = { + ORCA_PANE_KEY: 'pane-1', + ORCA_AGENT_LAUNCH_TOKEN: 'launch-1', + ORCA_TAB_ID: 'tab-1', + ORCA_WORKTREE_ID: 'tree-1', + ORCA_AGENT_HOOK_PORT: '4321', + ORCA_AGENT_HOOK_TOKEN: 'token-1', + ORCA_AGENT_HOOK_ENV: 'env-1', + ORCA_AGENT_HOOK_VERSION: '1.2.3' +} satisfies Record + +// Why: ownership keys on process.pid, so reload and child-process tests need +// stable, distinct identities. +export const AGENT_STATUS_EXTENSION_SELF_PID = 4242 + +export function createAgentStatusExtensionHarness(args: { + kind: 'pi' | 'omp' | 'prime-agent' + env?: Record + pid?: number + title?: string + argv?: readonly string[] + existsSync?: (path: string) => boolean + readFileSync?: (path: string, encoding: string) => string + statSync?: (path: string) => { mtimeMs: number; size: number; ino: number } + fetchImpl?: (...params: Parameters) => Promise +}): AgentStatusExtensionHarness { + const fetchMock = vi.fn( + args.fetchImpl ?? + (async () => ({ + ok: true + })) + ) + + const spawnedChildren: FakeCurlChild[] = [] + const spawnMock = vi.fn(() => { + const child: FakeCurlChild = { + on: vi.fn(), + stdin: { + on: vi.fn(), + end: vi.fn() + } + } + spawnedChildren.push(child) + return child + }) + + const fsMock = { + existsSync: vi.fn(args.existsSync ?? (() => false)), + statSync: vi.fn( + args.statSync ?? + ((path: string) => { + throw Object.assign(new Error(`ENOENT: ${path}`), { code: 'ENOENT' }) + }) + ), + readFileSync: vi.fn( + args.readFileSync ?? + ((path: string) => { + throw Object.assign(new Error(`ENOENT: ${path}`), { code: 'ENOENT' }) + }) + ) + } + + const module = { + exports: {} as { default?: (pi: { on: (name: string, handler: HookHandler) => void }) => void } + } + const requireMock = vi.fn((specifier: string) => { + if (specifier === 'fs') { + return fsMock + } + if (specifier === 'child_process') { + return { spawn: spawnMock } + } + throw new Error(`unexpected require(${specifier})`) + }) + + const processMock = { + env: { + ...BASE_ENV, + ...(args.kind === 'prime-agent' ? { PRIME_AGENT_INTERNAL_DAEMON_WORKER: '1' } : {}), + ...args.env + }, + pid: args.pid ?? AGENT_STATUS_EXTENSION_SELF_PID, + title: args.title ?? 'node', + argv: args.argv ?? ['node', '/usr/bin/orca'] + } + + const context = { + module, + exports: module.exports, + require: requireMock, + process: processMock, + fetch: fetchMock, + console: { + warn: vi.fn(), + error: vi.fn(), + log: vi.fn() + }, + Promise, + Buffer, + URL, + AbortController, + setTimeout, + clearTimeout + } as Record + context.globalThis = context + + const source = getPiAgentStatusExtensionSource(args.kind) + const output = ts.transpileModule(source, { + compilerOptions: { + module: ts.ModuleKind.CommonJS, + target: ts.ScriptTarget.ES2020 + } + }).outputText + runInNewContext(output, context) + + const register = module.exports.default + if (!register) { + throw new Error('expected default export from generated source') + } + + const handlers: Record = {} + const registerInto = (target: Record): void => { + register({ + on(name: string, handler: HookHandler) { + target[name] = handler + } + }) + } + registerInto(handlers) + + return { + fetchMock, + spawnMock, + spawnedChildren, + fsMock, + handlers, + processEnv: processMock.env, + callHook: async (name, event, hookContext) => { + await handlers[name]?.(event, hookContext) + }, + reload: () => { + for (const key of Object.keys(handlers)) { + delete handlers[key] + } + registerInto(handlers) + } + } +} diff --git a/src/main/pi/agent-status-handler-source.ts b/src/main/pi/agent-status-handler-source.ts index 20f3f31929a..4eb4feac96c 100644 --- a/src/main/pi/agent-status-handler-source.ts +++ b/src/main/pi/agent-status-handler-source.ts @@ -115,7 +115,9 @@ export function getPiAgentStatusHandlerSourceLines(kind: PiAgentKind): string[] ' })', '', ' // Why: modern Pi stays non-idle across retry/compaction/follow-up work,', - ' // while legacy Pi/OMP becomes idle after its final agent_end handlers.', + ' // while legacy Pi becomes idle after its final agent_end handlers.', + ' // OMP instead marks non-terminal agent_end events with willContinue, so it', + ' // returns before the recheck timer is ever armed.', ' const AGENT_END_IDLE_RECHECK_MS = 25', ' const AGENT_END_IDLE_RECHECK_MAX_MS = 250', ' let agentSettledSupported = false', @@ -169,8 +171,9 @@ export function getPiAgentStatusHandlerSourceLines(kind: PiAgentKind): string[] '', " pi.on('agent_end', (event, ctx) => {", ...captureSessionMetadata, - ' if (event?.willContinue === true) {', - ' clearPendingAgentEndCheck()', + ' if (isOmpRuntime()) {', + ' if (event?.willContinue === true) return', + ' postAgentEndOnce()', ' return', ' }', ' if (agentSettledSupported) return', From 24042fb3fbbd642023307bf1843834e266393f65 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Sun, 30 Aug 2026 15:11:08 -0700 Subject: [PATCH 2/2] fix(pi): preserve non-terminal continuation guards Keep the base Pi and Prime willContinue guard while settling terminal OMP events directly. Add regression coverage so sibling runtimes cannot publish a false completion after conflict resolution. --- ...gent-status-extension-omp-lifecycle.test.ts | 18 ++++++++++++++++++ src/main/pi/agent-status-handler-source.ts | 5 ++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/main/pi/agent-status-extension-omp-lifecycle.test.ts b/src/main/pi/agent-status-extension-omp-lifecycle.test.ts index e759217cd63..89f8c94d9a5 100644 --- a/src/main/pi/agent-status-extension-omp-lifecycle.test.ts +++ b/src/main/pi/agent-status-extension-omp-lifecycle.test.ts @@ -86,4 +86,22 @@ describe('OMP agent_end contract', () => { vi.useRealTimers() } }) + + it('preserves non-terminal agent_end handling for Pi and Prime', async () => { + vi.useFakeTimers() + try { + for (const kind of ['pi', 'prime-agent'] as const) { + const harness = createAgentStatusExtensionHarness({ kind }) + const context = { isIdle: vi.fn(() => true) } + + await harness.callHook('agent_end', { willContinue: true }, context) + await vi.advanceTimersByTimeAsync(1_000) + + expect(postedHookNames(harness.fetchMock)).toEqual([]) + expect(context.isIdle).not.toHaveBeenCalled() + } + } finally { + vi.useRealTimers() + } + }) }) diff --git a/src/main/pi/agent-status-handler-source.ts b/src/main/pi/agent-status-handler-source.ts index 4eb4feac96c..71bcbf96534 100644 --- a/src/main/pi/agent-status-handler-source.ts +++ b/src/main/pi/agent-status-handler-source.ts @@ -171,8 +171,11 @@ export function getPiAgentStatusHandlerSourceLines(kind: PiAgentKind): string[] '', " pi.on('agent_end', (event, ctx) => {", ...captureSessionMetadata, + ' if (event?.willContinue === true) {', + ' clearPendingAgentEndCheck()', + ' return', + ' }', ' if (isOmpRuntime()) {', - ' if (event?.willContinue === true) return', ' postAgentEndOnce()', ' return', ' }',