diff --git a/src/main/browser/agent-browser-bridge.test.ts b/src/main/browser/agent-browser-bridge.test.ts index 4629c844f..5df35b0d3 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() } @@ -876,6 +889,8 @@ describe('AgentBrowserBridge', () => { ) expect(routeCalls).toHaveLength(2) expect(routeCalls.at(-1)).toContain('https://old.example/**') + expect(routeCalls.at(-1)).toContain('--cdp') + expect(routeCalls.at(-1)).toContain('9222') }) it('clears stale sessions after direct CDP visibility re-registration', async () => { @@ -1119,7 +1134,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 +1607,282 @@ 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('fails closed when direct navigation is aborted', async () => { + const wc = mockWebContents(100, 'https://example.com/current', 'Current') + wc.loadURL.mockRejectedValue( + Object.assign(new Error('ERR_ABORTED (-3)'), { code: 'ERR_ABORTED' }) + ) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.goto('https://example.com/download')).rejects.toMatchObject({ + code: 'browser_error', + message: 'Failed to navigate browser page tab-1: ERR_ABORTED (-3)' + }) + expect(execFileMock).not.toHaveBeenCalled() + }) + + 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.each([ + [{ answer: 42 }, '{"answer":42}'], + [['a', 'b'], '["a","b"]'] + ])('preserves structured direct evaluation values as JSON text', async (value, expected) => { + const wc = mockWebContents(100) + wc.debugger.sendCommand.mockResolvedValue({ result: { value } }) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.evaluate('structuredValue')).resolves.toMatchObject({ result: expected }) + 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 +2409,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 +2422,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 0ce3b7f13..3011fd56b 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,62 @@ 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 + ? typeof result.value === 'object' && result.value !== null + ? JSON.stringify(result.value) + : 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 +2382,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) { @@ -2323,6 +2419,8 @@ export class AgentBrowserBridge { await this.runAgentBrowserRaw(sessionName, [ '--session', sessionName, + '--cdp', + String(session.proxy.getPort()), 'network', 'route', urlPattern, @@ -2368,23 +2466,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 +2499,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 +2645,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 9070f90be..a2d729692 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) diff --git a/src/shared/runtime-types.ts b/src/shared/runtime-types.ts index 5c8dfb9cf..a22a9b96c 100644 --- a/src/shared/runtime-types.ts +++ b/src/shared/runtime-types.ts @@ -1059,6 +1059,8 @@ export type BrowserErrorCode = | 'browser_no_tab' | 'browser_tab_not_found' | 'browser_tab_closed' + | 'browser_tab_changed' + | 'browser_owner_unavailable' | 'browser_stale_ref' | 'browser_ref_not_found' | 'browser_navigation_failed' diff --git a/tests/e2e/browser-embedded-owner-routing.spec.ts b/tests/e2e/browser-embedded-owner-routing.spec.ts new file mode 100644 index 000000000..dae6677e1 --- /dev/null +++ b/tests/e2e/browser-embedded-owner-routing.spec.ts @@ -0,0 +1,240 @@ +import { execFile } from 'node:child_process' +import { chmodSync, existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { createServer, type Server } from 'node:http' +import type { AddressInfo } from 'node:net' +import os from 'node:os' +import path from 'node:path' +import { promisify } from 'node:util' +import type { Page } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { ensureTerminalVisible, getActiveWorktreeId, waitForActiveWorktree } from './helpers/store' + +const execFileAsync = promisify(execFile) +// Why: Unix-domain helper sockets can exceed platform path limits under macOS's long temp root. +const shortTempRoot = + process.platform === 'win32' ? os.tmpdir() : path.join(path.parse(os.tmpdir()).root, 'tmp') +const helperSocketDir = mkdtempSync(path.join(shortTempRoot, 'ob-')) +const blockedBrowserPath = path.join( + helperSocketDir, + process.platform === 'win32' ? 'blocked-browser.cmd' : 'blocked-browser' +) +const externalLaunchMarker = `${blockedBrowserPath}.attempted` + +writeFileSync( + blockedBrowserPath, + process.platform === 'win32' + ? '@echo off\r\ntype nul > "%~f0.attempted"\r\nexit /b 97\r\n' + : '#!/bin/sh\n: > "$0.attempted"\nexit 97\n' +) +if (process.platform !== 'win32') { + chmodSync(blockedBrowserPath, 0o755) +} + +test.use({ + orcaAppExtraEnv: { + AGENT_BROWSER_SOCKET_DIR: helperSocketDir, + PATH: `${path.join(process.cwd(), 'node_modules', '.bin')}${path.delimiter}${process.env.PATH ?? ''}` + } +}) + +type CreatedBrowserTab = { + id: string + pageId: string +} + +type RuntimeResponse = { + ok: boolean + result?: unknown + error?: { code?: string; message?: string } +} + +async function startOwnershipServer(): Promise<{ + sourceUrl: string + destinationUrl: string + close: () => Promise +}> { + const server = createServer((request, response) => { + const pathname = new URL(request.url ?? '/', 'http://127.0.0.1').pathname + const destination = pathname === '/destination' + response.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' }) + response.end(` + + ${destination ? 'Owned destination' : 'Owned source'} +

${destination ? 'destination-webview' : 'source-webview'}

+ `) + }) + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const origin = `http://127.0.0.1:${(server.address() as AddressInfo).port}` + return { + sourceUrl: `${origin}/source`, + destinationUrl: `${origin}/destination`, + close: () => closeServer(server) + } +} + +async function closeServer(server: Server): Promise { + await new Promise((resolve, reject) => + server.close((error) => { + if (error) { + reject(error) + return + } + resolve() + }) + ) +} + +async function createBrowserTab( + page: Page, + worktreeId: string, + url: string +): Promise { + const browserTab = await page.evaluate( + ({ targetWorktreeId, targetUrl }) => { + const created = window.__store?.getState().createBrowserTab(targetWorktreeId, targetUrl, { + title: 'Embedded owner smoke', + activate: true + }) + return created?.activePageId ? { id: created.id, pageId: created.activePageId } : null + }, + { targetWorktreeId: worktreeId, targetUrl: url } + ) + if (!browserTab) { + throw new Error('Failed to create the embedded browser page') + } + return browserTab +} + +async function callBrowserRuntime( + page: Page, + method: string, + params: Record +): Promise { + return (await page.evaluate( + ({ targetMethod, targetParams }) => + window.api.runtime.call({ method: targetMethod, params: targetParams }), + { targetMethod: method, targetParams: params } + )) as RuntimeResponse +} + +async function readEmbeddedPage( + page: Page, + browserTabId: string +): Promise<{ marker: string | null; title: string; url: string } | null> { + return page.evaluate(async (targetBrowserTabId) => { + const overlay = document.querySelector(`[data-browser-overlay-tab-id="${targetBrowserTabId}"]`) + const webview = overlay?.querySelector('webview') as Electron.WebviewTag | null + if (!webview) { + return null + } + try { + return (await webview.executeJavaScript(`({ + marker: document.querySelector('#marker')?.textContent ?? null, + title: document.title, + url: location.href + })`)) as { marker: string | null; title: string; url: string } + } catch { + return null + } + }, browserTabId) +} + +function agentBrowserBinary(): string { + const suffix = process.platform === 'win32' ? '.exe' : '' + return path.join( + process.cwd(), + 'node_modules', + 'agent-browser', + 'bin', + `agent-browser-${process.platform}-${process.arch}${suffix}` + ) +} + +async function stopHelperDaemon(sessionName: string): Promise { + await execFileAsync(agentBrowserBinary(), ['--session', sessionName, 'close', '--json'], { + env: { ...process.env, AGENT_BROWSER_SOCKET_DIR: helperSocketDir }, + timeout: 10_000 + }) + await expect + .poll(() => existsSync(path.join(helperSocketDir, `${sessionName}.pid`)), { timeout: 5_000 }) + .toBe(false) +} + +test('stale helper cannot take goto or eval away from the real embedded webview', async ({ + electronApp, + orcaPage, + registerPostElectronShutdownCleanup +}) => { + registerPostElectronShutdownCleanup(async () => { + rmSync(helperSocketDir, { recursive: true, force: true }) + }) + const server = await startOwnershipServer() + try { + await waitForActiveWorktree(orcaPage) + await ensureTerminalVisible(orcaPage) + const worktreeId = await getActiveWorktreeId(orcaPage) + if (!worktreeId) { + throw new Error('Expected an active worktree for the embedded browser smoke test') + } + const browserTab = await createBrowserTab(orcaPage, worktreeId, server.sourceUrl) + + await expect + .poll(() => readEmbeddedPage(orcaPage, browserTab.id), { timeout: 10_000 }) + .toMatchObject({ marker: 'source-webview', title: 'Owned source', url: server.sourceUrl }) + + const snapshot = await callBrowserRuntime(orcaPage, 'browser.snapshot', { + page: browserTab.pageId + }) + expect(snapshot, JSON.stringify(snapshot)).toMatchObject({ ok: true }) + + const sessionName = `orca-tab-${browserTab.pageId}` + await expect + .poll(() => existsSync(path.join(helperSocketDir, `${sessionName}.pid`)), { + timeout: 5_000 + }) + .toBe(true) + + // Why: if routing escapes the registered webview, this executable records the attempt without launching Chrome. + await electronApp.evaluate((_electron, executablePath) => { + process.env.AGENT_BROWSER_EXECUTABLE_PATH = executablePath + }, blockedBrowserPath) + await stopHelperDaemon(sessionName) + + const navigation = await callBrowserRuntime(orcaPage, 'browser.goto', { + page: browserTab.pageId, + url: server.destinationUrl + }) + expect(existsSync(externalLaunchMarker)).toBe(false) + expect(navigation).toMatchObject({ + ok: true, + result: { url: server.destinationUrl, title: 'Owned destination' } + }) + await expect + .poll(() => readEmbeddedPage(orcaPage, browserTab.id), { timeout: 10_000 }) + .toMatchObject({ + marker: 'destination-webview', + title: 'Owned destination', + url: server.destinationUrl + }) + await expect(orcaPage.locator(`[data-tab-id="${browserTab.id}"]`)).toContainText( + 'Owned destination' + ) + + const evaluation = await callBrowserRuntime(orcaPage, 'browser.eval', { + page: browserTab.pageId, + expression: 'document.querySelector("#marker")?.textContent' + }) + expect(existsSync(externalLaunchMarker)).toBe(false) + expect(evaluation).toMatchObject({ + ok: true, + result: { result: 'destination-webview', origin: server.destinationUrl } + }) + // Why: the stopped helper has no cleanup left; a PID after eval means a forbidden relaunch. + expect(existsSync(path.join(helperSocketDir, `${sessionName}.pid`))).toBe(false) + } finally { + await electronApp.evaluate(() => { + delete process.env.AGENT_BROWSER_EXECUTABLE_PATH + }) + await server.close() + } +})