From 0f4a6c941cea52bda3e7e703cee7a9b31ca8b5f9 Mon Sep 17 00:00:00 2001 From: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Date: Mon, 20 Jul 2026 17:55:19 -0700 Subject: [PATCH] fix(browser): keep embedded commands on registered target --- src/main/browser/agent-browser-bridge.test.ts | 318 ++++++++++++++++-- src/main/browser/agent-browser-bridge.ts | 162 +++++++-- src/main/runtime/orca-runtime-browser.ts | 4 +- 3 files changed, 437 insertions(+), 47 deletions(-) diff --git a/src/main/browser/agent-browser-bridge.test.ts b/src/main/browser/agent-browser-bridge.test.ts index 4629c844f08..e8f77d46193 100644 --- a/src/main/browser/agent-browser-bridge.test.ts +++ b/src/main/browser/agent-browser-bridge.test.ts @@ -67,7 +67,8 @@ import { // inside a try/catch. Override the private method to inject our mock. // eslint-disable-next-line @typescript-eslint/no-explicit-any ;(AgentBrowserBridge.prototype as any).getWebContents = function (id: number) { - return webContentsFromIdMock(id) ?? null + const target = webContentsFromIdMock(id) + return target && !target.isDestroyed() ? target : null } function mockBrowserManager( @@ -89,10 +90,14 @@ function mockBrowserManager( } function mockWebContents(id: number, url = 'https://example.com', title = 'Example') { + let currentUrl = url return { id, - getURL: () => url, + getURL: () => currentUrl, getTitle: () => title, + loadURL: vi.fn(async (nextUrl: string) => { + currentUrl = nextUrl + }), isDestroyed: () => false, invalidate: vi.fn(), focus: vi.fn(), @@ -308,9 +313,9 @@ describe('AgentBrowserBridge', () => { expect(args[args.indexOf('--session') + 1]).toBe('orca-tab-tab-1') }) - // ── --cdp first-use only ── + // ── Embedded CDP ownership ── - it('passes --cdp only on first command for a session', async () => { + it('passes --cdp on every helper command family so a restarted daemon cannot launch Chrome', async () => { succeedWith({ snapshot: '...' }) await bridge.snapshot() @@ -322,16 +327,24 @@ describe('AgentBrowserBridge', () => { const cdpIdx = (snapshotCall![1] as string[]).indexOf('--cdp') expect((snapshotCall![1] as string[])[cdpIdx + 1]).toBe('9222') - succeedWith({ clicked: '@e1' }) await bridge.click('@e1') + await bridge.mouseMove(10, 20) + await bridge.setOffline('on') + await bridge.consoleLog() + await bridge.exec('get title') - const clickCall = execFileMock.mock.calls.find((c: unknown[]) => - (c[1] as string[]).includes('click') - ) - expect(clickCall![1]).not.toContain('--cdp') + for (const command of ['click', 'mouse', 'set', 'console', 'get']) { + const call = execFileMock.mock.calls.find((candidate: unknown[]) => + (candidate[1] as string[]).includes(command) + ) + expect(call).toBeDefined() + const args = call![1] as string[] + expect(args).toContain('--cdp') + expect(args[args.indexOf('--cdp') + 1]).toBe('9222') + } }) - it('continues when stale agent-browser session close hangs during session creation', async () => { + it('fails closed when stale agent-browser session ownership cannot be reset', async () => { vi.useFakeTimers() try { const closeKill = vi.fn() @@ -349,17 +362,17 @@ describe('AgentBrowserBridge', () => { ) const promise = bridge.snapshot() - let settled = false - void promise.finally(() => { - settled = true + const rejection = expect(promise).rejects.toMatchObject({ + code: 'browser_owner_unavailable', + message: + 'Could not reset stale helper session orca-tab-tab-1; retry after agent-browser exits' }) await vi.advanceTimersByTimeAsync(3_000) - await Promise.resolve() - expect(settled).toBe(true) - await expect(promise).resolves.toEqual({ browserPageId: 'tab-1', snapshot: 'ready' }) + await rejection expect(closeKill).toHaveBeenCalled() + expect(execFileMock.mock.calls.some((call) => call[1].includes('snapshot'))).toBe(false) } finally { vi.useRealTimers() } @@ -1119,7 +1132,11 @@ describe('AgentBrowserBridge', () => { const killedError = Object.assign(new Error('timeout'), { killed: true }) execFileMock.mockImplementation( - (_bin: string, _args: string[], _opts: unknown, cb: Function) => { + (_bin: string, args: string[], _opts: unknown, cb: Function) => { + if (args.includes('close')) { + cb(null, JSON.stringify({ success: true, data: null }), '') + return + } cb(killedError, '', '') } ) @@ -1588,13 +1605,256 @@ describe('AgentBrowserBridge', () => { // ── goto command ── - it('passes url to goto command', async () => { - succeedWith({ url: 'https://example.com', title: 'Example' }) - await bridge.goto('https://example.com') + it('navigates the registered webContents without spawning agent-browser', async () => { + const wc = mockWebContents(100, 'https://example.com/start', 'Example') + webContentsFromIdMock.mockReturnValue(wc) - const args = execFileMock.mock.calls.at(-1)![1] as string[] - expect(args).toContain('goto') - expect(args).toContain('https://example.com') + await expect(bridge.goto('https://example.com/next')).resolves.toEqual({ + url: 'https://example.com/next', + title: 'Example' + }) + + expect(wc.loadURL).toHaveBeenCalledWith('https://example.com/next') + expect(execFileMock).not.toHaveBeenCalled() + }) + + it('preserves scheme-less navigation semantics at the direct WebContents boundary', async () => { + const wc = mockWebContents(100) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.goto('example.com')).resolves.toEqual({ + url: 'https://example.com/', + title: 'Example' + }) + expect(wc.loadURL).toHaveBeenCalledWith('https://example.com/') + }) + + it('rejects unsupported direct navigation URLs without falling back to agent-browser', async () => { + const wc = mockWebContents(100) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.goto('javascript:alert(1)')).rejects.toMatchObject({ + code: 'invalid_argument', + message: 'Unsupported browser URL: javascript:alert(1)' + }) + expect(wc.loadURL).not.toHaveBeenCalled() + expect(execFileMock).not.toHaveBeenCalled() + }) + + it('fails closed and releases the command queue when direct navigation never settles', async () => { + vi.useFakeTimers() + try { + const wc = mockWebContents(100) + wc.loadURL.mockReturnValue(new Promise(() => {})) + webContentsFromIdMock.mockReturnValue(wc) + + const navigation = bridge.goto('https://example.com/hangs') + const rejection = expect(navigation).rejects.toMatchObject({ + code: 'browser_error', + message: 'Failed to navigate browser page tab-1: Browser navigation timed out after 30000ms' + }) + await vi.advanceTimersByTimeAsync(30_000) + + await rejection + expect(execFileMock).not.toHaveBeenCalled() + expect( + (bridge as unknown as { commandQueues: Map }).commandQueues.size + ).toBe(0) + } finally { + vi.useRealTimers() + } + }) + + it.each([ + { + command: 'goto', + run: (b: AgentBrowserBridge) => b.goto('https://embedded.example/next'), + helperArg: 'goto', + directMethod: 'loadURL' + }, + { + command: 'evaluate', + run: (b: AgentBrowserBridge) => b.evaluate('document.title'), + helperArg: 'eval', + directMethod: 'Runtime.evaluate' + } + ])( + 'does not route $command to another browser when the helper session is stale', + async ({ run, helperArg, directMethod }) => { + const wc = mockWebContents(100, 'https://embedded.example/current', 'Embedded') + wc.debugger.sendCommand.mockImplementation(async (_method: string, params?: unknown) => ({ + result: { + value: + (params as { expression?: string } | undefined)?.expression === 'location.origin' + ? 'https://embedded.example' + : 'Embedded' + } + })) + webContentsFromIdMock.mockReturnValue(wc) + const wrongOwnerCalls: string[][] = [] + let helperSessionIsStale = false + + execFileMock.mockImplementation( + (_bin: string, args: string[], _opts: unknown, cb: Function) => { + if (args.includes('close')) { + cb(null, JSON.stringify({ success: true, data: null }), '') + } else if (args.includes('snapshot')) { + cb(null, JSON.stringify({ success: true, data: { snapshot: 'ready' } }), '') + } else { + if (helperSessionIsStale && !args.includes('--cdp')) { + wrongOwnerCalls.push(args) + } + cb( + null, + JSON.stringify({ + success: true, + data: { url: 'https://external.example', title: 'External', result: 'external' } + }), + '' + ) + } + return { kill: vi.fn() } + } + ) + + await bridge.snapshot() + helperSessionIsStale = true + await run(bridge) + + expect(wrongOwnerCalls).toEqual([]) + expect( + execFileMock.mock.calls.some((call) => (call[1] as string[]).includes(helperArg)) + ).toBe(false) + if (directMethod === 'loadURL') { + expect(wc.loadURL).toHaveBeenCalledWith('https://embedded.example/next') + } else { + expect(wc.debugger.sendCommand).toHaveBeenCalledWith( + 'Runtime.evaluate', + expect.objectContaining({ expression: 'document.title' }) + ) + } + } + ) + + it('returns direct evaluation value and full page URL semantics without spawning agent-browser', async () => { + const wc = mockWebContents(100, 'https://example.com/path?query=1') + wc.debugger.sendCommand.mockResolvedValue({ result: { value: 42 } }) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.evaluate('6 * 7')).resolves.toEqual({ + result: '42', + origin: 'https://example.com/path?query=1' + }) + expect(execFileMock).not.toHaveBeenCalled() + }) + + it('surfaces direct evaluation exceptions without falling back to agent-browser', async () => { + const wc = mockWebContents(100) + wc.debugger.sendCommand.mockResolvedValue({ + result: { type: 'object' }, + exceptionDetails: { + text: 'Uncaught', + exception: { description: 'ReferenceError: missingValue is not defined' } + } + }) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.evaluate('missingValue')).rejects.toMatchObject({ + code: 'browser_eval_error', + message: 'ReferenceError: missingValue is not defined' + }) + expect(execFileMock).not.toHaveBeenCalled() + }) + + it('fails closed when the registered webContents debugger is stale', async () => { + const wc = mockWebContents(100) + wc.debugger.sendCommand.mockRejectedValue(new Error('Debugger is detached')) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.evaluate('document.title')).rejects.toMatchObject({ + code: 'browser_error', + message: 'Failed to evaluate in browser page tab-1: Debugger is detached' + }) + expect(execFileMock).not.toHaveBeenCalled() + }) + + it.each([ + ['goto', (b: AgentBrowserBridge) => b.goto('https://example.com/next', undefined, 'tab-1')], + ['evaluate', (b: AgentBrowserBridge) => b.evaluate('document.title', undefined, 'tab-1')] + ])('fails closed when direct %s targets a destroyed webContents', async (_command, run) => { + const wc = mockWebContents(100) + wc.isDestroyed = () => true + webContentsFromIdMock.mockReturnValue(wc) + const unregisterGuest = vi.fn() + const b = new AgentBrowserBridge( + mockBrowserManager(new Map([['tab-1', 100]]), new Map(), { unregisterGuest }) + ) + b.setActiveTab(100) + + await expect(run(b)).rejects.toMatchObject({ + code: 'browser_tab_not_found', + message: 'Browser page tab-1 is no longer available' + }) + expect(unregisterGuest).toHaveBeenCalledWith('tab-1') + expect(execFileMock).not.toHaveBeenCalled() + }) + + it('returns navigation state from a replacement registered during load', async () => { + const tabs = new Map([['tab-1', 100]]) + const oldWc = mockWebContents(100, 'https://example.com/start', 'Old') + const replacementWc = mockWebContents(200, 'https://example.com/final', 'Replacement') + oldWc.loadURL.mockImplementation(async () => { + tabs.set('tab-1', 200) + }) + webContentsFromIdMock.mockImplementation((id: number) => + id === 100 ? oldWc : id === 200 ? replacementWc : null + ) + const b = new AgentBrowserBridge(mockBrowserManager(tabs)) + b.setActiveTab(100) + + await expect(b.goto('https://example.com/next')).resolves.toEqual({ + url: 'https://example.com/final', + title: 'Replacement' + }) + expect(execFileMock).not.toHaveBeenCalled() + }) + + it('routes direct commands to the replacement registration, not the stale session owner', async () => { + const tabs = new Map([['tab-1', 100]]) + const oldWc = mockWebContents(100, 'https://old.example', 'Old') + const replacementWc = mockWebContents(200, 'https://new.example', 'New') + replacementWc.debugger.sendCommand.mockImplementation( + async (_method: string, params?: unknown) => ({ + result: { + value: + (params as { expression?: string } | undefined)?.expression === 'location.origin' + ? 'https://new.example' + : 'New' + } + }) + ) + webContentsFromIdMock.mockImplementation((id: number) => + id === 100 ? oldWc : id === 200 ? replacementWc : null + ) + const b = new AgentBrowserBridge(mockBrowserManager(tabs)) + b.setActiveTab(100) + + succeedWith({ snapshot: 'ready' }) + await b.snapshot() + tabs.set('tab-1', 200) + await b.onProcessSwap('tab-1', 200, 100) + execFileMock.mockClear() + + await expect(b.evaluate('document.title', undefined, 'tab-1')).resolves.toEqual({ + result: 'New', + origin: 'https://new.example' + }) + expect(replacementWc.debugger.sendCommand).toHaveBeenCalledWith( + 'Runtime.evaluate', + expect.objectContaining({ expression: 'document.title' }) + ) + expect(oldWc.debugger.sendCommand).not.toHaveBeenCalled() + expect(execFileMock).not.toHaveBeenCalled() }) it('rejects oversized browser clipboard writes before spawning agent-browser', async () => { @@ -2121,7 +2381,11 @@ describe('AgentBrowserBridge', () => { it('passes stderr through as error message on execFile failure', async () => { execFileMock.mockImplementation( - (_bin: string, _args: string[], _opts: unknown, cb: Function) => { + (_bin: string, args: string[], _opts: unknown, cb: Function) => { + if (args.includes('close')) { + cb(null, JSON.stringify({ success: true, data: null }), '') + return + } cb(new Error('exit code 1'), '', 'daemon crashed: segfault') } ) @@ -2130,7 +2394,11 @@ describe('AgentBrowserBridge', () => { it('falls back to error.message when stderr is empty', async () => { execFileMock.mockImplementation( - (_bin: string, _args: string[], _opts: unknown, cb: Function) => { + (_bin: string, args: string[], _opts: unknown, cb: Function) => { + if (args.includes('close')) { + cb(null, JSON.stringify({ success: true, data: null }), '') + return + } cb(new Error('Command failed'), '', '') } ) diff --git a/src/main/browser/agent-browser-bridge.ts b/src/main/browser/agent-browser-bridge.ts index 0ce3b7f1342..a6a87a81c03 100644 --- a/src/main/browser/agent-browser-bridge.ts +++ b/src/main/browser/agent-browser-bridge.ts @@ -48,6 +48,7 @@ import type { BrowserCookie } from '../../shared/runtime-types' import { assertClipboardTextWriteWithinLimitWithYield } from '../../shared/clipboard-text' +import { normalizeBrowserNavigationUrl } from '../../shared/browser-url' import { iterateBrowserTextInsertionChunks } from './browser-text-insertion' // Why: must exceed agent-browser's internal timeouts (goto 30s, wait 60s) so the bridge never kills a command before its own timeout fires. @@ -55,6 +56,7 @@ const EXEC_TIMEOUT_MS = 90_000 const CONSECUTIVE_TIMEOUT_LIMIT = 3 const WAIT_PROCESS_TIMEOUT_GRACE_MS = 1_000 const STALE_SESSION_CLOSE_TIMEOUT_MS = 3_000 +const EMBEDDED_NAVIGATION_TIMEOUT_MS = 30_000 export const AGENT_BROWSER_TEXT_ARGUMENT_MAX_BYTES = 8 * 1024 export const AGENT_BROWSER_CLIPBOARD_WRITE_MAX_BYTES = AGENT_BROWSER_TEXT_ARGUMENT_MAX_BYTES @@ -766,9 +768,53 @@ export class AgentBrowserBridge { } async goto(url: string, worktreeId?: string, browserPageId?: string): Promise { - return this.enqueueTargetedCommand(worktreeId, browserPageId, async (sessionName) => { - return (await this.execAgentBrowser(sessionName, ['goto', url])) as BrowserGotoResult - }) + return this.enqueueTargetedCommand( + worktreeId, + browserPageId, + async (_sessionName, target) => { + const wc = this.requireTargetWebContents(target) + const navigationUrl = normalizeBrowserNavigationUrl(url) + if (!navigationUrl) { + throw new BrowserError('invalid_argument', `Unsupported browser URL: ${url}`) + } + let navigationTimeout: ReturnType | null = null + try { + await Promise.race([ + wc.loadURL(navigationUrl), + new Promise((_resolve, reject) => { + navigationTimeout = setTimeout( + () => + reject( + new Error( + `Browser navigation timed out after ${EMBEDDED_NAVIGATION_TIMEOUT_MS}ms` + ) + ), + EMBEDDED_NAVIGATION_TIMEOUT_MS + ) + navigationTimeout.unref?.() + }) + ]) + } catch (error) { + if (!this.getWebContents(target.webContentsId)) { + throw this.createPageUnavailableError(`orca-tab-${target.browserPageId}`) + } + throw new BrowserError( + 'browser_error', + `Failed to navigate browser page ${target.browserPageId}: ${error instanceof Error ? error.message : String(error)}` + ) + } finally { + if (navigationTimeout) { + clearTimeout(navigationTimeout) + } + } + + // Why: cross-process navigation can replace the guest while retaining the same authoritative page id. + const navigatedTarget = this.resolveCommandTarget(worktreeId, target.browserPageId) + const navigatedWebContents = this.requireTargetWebContents(navigatedTarget) + return { url: navigatedWebContents.getURL(), title: navigatedWebContents.getTitle() } + }, + { ensureSession: false } + ) } async fill( @@ -1403,9 +1449,57 @@ export class AgentBrowserBridge { worktreeId?: string, browserPageId?: string ): Promise { - return this.enqueueTargetedCommand(worktreeId, browserPageId, async (sessionName) => { - return (await this.execAgentBrowser(sessionName, ['eval', expression])) as BrowserEvalResult - }) + return this.enqueueTargetedCommand( + worktreeId, + browserPageId, + async (_sessionName, target) => { + const wc = this.requireTargetWebContents(target) + let releaseDebugger = (): void => {} + try { + releaseDebugger = acquireElectronDebugger(wc).release + const { result, exceptionDetails } = (await wc.debugger.sendCommand('Runtime.evaluate', { + expression, + returnByValue: true, + awaitPromise: true + })) as { + result: { value?: unknown; description?: string } + exceptionDetails?: { text: string; exception?: { description?: string } } + } + if (exceptionDetails) { + throw new BrowserError( + 'browser_eval_error', + exceptionDetails.exception?.description ?? exceptionDetails.text + ) + } + + const currentTarget = this.resolveCommandTarget(worktreeId, target.browserPageId) + if (currentTarget.webContentsId !== target.webContentsId) { + throw new BrowserError( + 'browser_tab_changed', + `Browser page ${target.browserPageId} changed while evaluating; retry the command` + ) + } + return { + result: result.value !== undefined ? String(result.value) : (result.description ?? ''), + origin: wc.getURL() + } + } catch (error) { + if (error instanceof BrowserError) { + throw error + } + if (!this.getWebContents(target.webContentsId)) { + throw this.createPageUnavailableError(`orca-tab-${target.browserPageId}`) + } + throw new BrowserError( + 'browser_error', + `Failed to evaluate in browser page ${target.browserPageId}: ${error instanceof Error ? error.message : String(error)}` + ) + } finally { + releaseDebugger() + } + }, + { ensureSession: false } + ) } async hover( @@ -2283,12 +2377,9 @@ export class AgentBrowserBridge { const managesInterceptRoutes = commandArgs[0] === 'network' && (commandArgs[1] === 'route' || commandArgs[1] === 'unroute') - // Why: --cdp is init-only; pass the port (not a ws:// URL) so agent-browser's /json discovery sees only the proxy's webview, not the host renderer page. const needsInit = !session.initialized - if (needsInit) { - const port = session.proxy.getPort() - args.push('--cdp', String(port)) - } + // Why: a restarted named daemon auto-launches Chrome unless every invocation reasserts Orca's CDP owner. + args.push('--cdp', String(session.proxy.getPort())) // Why: exec passthrough can produce a large argv; spreading into push risks V8 argument limits. for (const commandArg of commandArgs) { @@ -2368,23 +2459,32 @@ export class AgentBrowserBridge { } private closeStaleAgentBrowserSession(sessionName: string): Promise { - return new Promise((resolve) => { + return new Promise((resolve, reject) => { let child: ReturnType | null = null let settled = false - const finish = (): void => { + const finish = (error?: Error): void => { if (settled) { return } settled = true clearTimeout(timeout) - resolve() + if (error) { + reject(error) + } else { + resolve() + } } - // Why: best-effort daemon cleanup — a wedged close must not block the real browser action. + // Why: proceeding after an unverified close can reuse a daemon that owns an unrelated browser. const timeout = setTimeout(() => { child?.kill() - finish() + finish( + new BrowserError( + 'browser_owner_unavailable', + `Could not reset stale helper session ${sessionName}; retry after agent-browser exits` + ) + ) }, STALE_SESSION_CLOSE_TIMEOUT_MS) try { @@ -2392,10 +2492,23 @@ export class AgentBrowserBridge { this.agentBrowserBin, ['--session', sessionName, 'close'], { timeout: STALE_SESSION_CLOSE_TIMEOUT_MS }, - finish + (error) => + finish( + error + ? new BrowserError( + 'browser_owner_unavailable', + `Could not reset stale helper session ${sessionName}: ${error.message}` + ) + : undefined + ) + ) + } catch (error) { + finish( + new BrowserError( + 'browser_owner_unavailable', + `Could not reset stale helper session ${sessionName}: ${error instanceof Error ? error.message : String(error)}` + ) ) - } catch { - finish() } }) } @@ -2525,10 +2638,19 @@ export class AgentBrowserBridge { return null } + private requireTargetWebContents(target: ResolvedBrowserCommandTarget): WebContents { + const wc = this.getWebContents(target.webContentsId) + if (!wc || wc.isDestroyed()) { + throw this.createPageUnavailableError(`orca-tab-${target.browserPageId}`) + } + return wc + } + private getWebContents(webContentsId: number): Electron.WebContents | null { try { const { webContents } = require('electron') - return webContents.fromId(webContentsId) ?? null + const target = webContents.fromId(webContentsId) + return target && !target.isDestroyed() ? target : null } catch { return null } diff --git a/src/main/runtime/orca-runtime-browser.ts b/src/main/runtime/orca-runtime-browser.ts index 9070f90bef8..a2d72969226 100644 --- a/src/main/runtime/orca-runtime-browser.ts +++ b/src/main/runtime/orca-runtime-browser.ts @@ -299,7 +299,7 @@ export class RuntimeBrowserCommands { await waitForTabRegistration(browserPageId) } - // Why: CDP navigation bypasses Electron's webview events, so the renderer's did-navigate listeners never fire; push updates to keep the UI in sync. + // Why: helper-driven clicks can bypass Electron navigation events; push authoritative URL/title updates after automation. private notifyRendererNavigation(browserPageId: string, url: string, title: string): void { try { const win = this.host.getAuthoritativeWindow() @@ -1345,7 +1345,7 @@ export class RuntimeBrowserCommands { bridge.setActiveTab(wcId, worktreeId) } - // Why: the webview loads about:blank first, so navigate via the bridge to make agent-browser's CDP session track the real URL. + // Why: the webview loads about:blank first; route navigation through the bridge so its registered owner remains authoritative. if (url && url !== 'about:blank') { try { const result = await bridge.goto(url, worktreeId, browserPageId)