diff --git a/apps/desktop/src/main/browser-agent/cdp.test.ts b/apps/desktop/src/main/browser-agent/cdp.test.ts index 2f21c1ea6d9..f2cb1f6c0d7 100644 --- a/apps/desktop/src/main/browser-agent/cdp.test.ts +++ b/apps/desktop/src/main/browser-agent/cdp.test.ts @@ -548,6 +548,8 @@ describe('browser-agent screenshot capture', () => { expect(shot).toEqual({ dataUrl: `data:image/jpeg;base64,${Buffer.from('resized').toString('base64')}`, scale: 0.5, + viewport: { width: 2048, height: 1024 }, + imageSize: { width: 1024, height: 512 }, }) }) @@ -558,7 +560,12 @@ describe('browser-agent screenshot capture', () => { const image = vi.mocked(nativeImage.createFromBuffer).mock.results[0].value expect(image.resize).not.toHaveBeenCalled() - expect(shot).toEqual({ dataUrl: 'data:image/jpeg;base64,c2lt', scale: 0.5 }) + expect(shot).toEqual({ + dataUrl: 'data:image/jpeg;base64,c2lt', + scale: 0.5, + viewport: { width: 2048, height: 1024 }, + imageSize: { width: 1024, height: 512 }, + }) }) it('returns the raw capture when the image cannot be decoded', async () => { @@ -566,6 +573,102 @@ describe('browser-agent screenshot capture', () => { const shot = await captureScreenshot(contents) - expect(shot).toEqual({ dataUrl: 'data:image/jpeg;base64,c2lt', scale: 0.5 }) + expect(shot).toEqual({ + dataUrl: 'data:image/jpeg;base64,c2lt', + scale: 0.5, + viewport: { width: 2048, height: 1024 }, + imageSize: null, + }) }) + + it('does not expose deprecated device-pixel metrics as a CSS viewport', async () => { + const { contents } = captureFixture({ width: 1024, height: 512 }) + vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + return Promise.resolve({ layoutViewport: { clientWidth: 2048, clientHeight: 1024 } }) + } + if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' }) + return Promise.resolve(undefined) + }) + + const shot = await captureScreenshot(contents) + + expect(shot.viewport).toBeNull() + expect(shot.imageSize).toEqual({ width: 1024, height: 512 }) + }) + + it('accepts stable finite scroll offsets around the capture', async () => { + const { contents } = captureFixture({ width: 1024, height: 512 }) + vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + return Promise.resolve({ + cssLayoutViewport: { + clientWidth: 2048, + clientHeight: 1024, + pageX: 12, + pageY: 34, + }, + }) + } + if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' }) + return Promise.resolve(undefined) + }) + + await expect(captureScreenshot(contents)).resolves.toMatchObject({ + viewport: { width: 2048, height: 1024 }, + imageSize: { width: 1024, height: 512 }, + }) + }) + + it.each([ + [ + 'dimensions', + { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024 } }, + { cssLayoutViewport: { clientWidth: 1024, clientHeight: 512 } }, + ], + [ + 'metric units', + { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024 } }, + { layoutViewport: { clientWidth: 2048, clientHeight: 1024 } }, + ], + [ + 'horizontal scroll offset', + { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024, pageX: 0, pageY: 20 } }, + { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024, pageX: 10, pageY: 20 } }, + ], + [ + 'vertical scroll offset', + { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024, pageX: 10, pageY: 20 } }, + { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024, pageX: 10, pageY: 30 } }, + ], + [ + 'offset validity', + { cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024, pageX: 0, pageY: 0 } }, + { + cssLayoutViewport: { + clientWidth: 2048, + clientHeight: 1024, + pageX: 0, + pageY: Number.NaN, + }, + }, + ], + ['availability', {}, {}], + ])( + 'rejects a capture when viewport %s change during CDP capture', + async (_label, before, after) => { + const { contents } = captureFixture({ width: 1024, height: 512 }) + let metricsRead = 0 + vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + metricsRead++ + return Promise.resolve(metricsRead === 1 ? before : after) + } + if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' }) + return Promise.resolve(undefined) + }) + + await expect(captureScreenshot(contents)).rejects.toThrow(/viewport changed/) + } + ) }) diff --git a/apps/desktop/src/main/browser-agent/cdp.ts b/apps/desktop/src/main/browser-agent/cdp.ts index 36dffe4da0d..75cf1adbc80 100644 --- a/apps/desktop/src/main/browser-agent/cdp.ts +++ b/apps/desktop/src/main/browser-agent/cdp.ts @@ -382,6 +382,71 @@ const SCREENSHOT_CAPTURE_QUALITY = 90 interface CdpViewport { clientWidth: number clientHeight: number + pageX?: number + pageY?: number +} + +interface ScreenshotViewportMetrics extends ScreenshotSize { + pageX: number | null + pageY: number | null + unit: 'css' | 'device' +} + +interface ScreenshotSize { + width: number + height: number +} + +export interface ScreenshotCapture { + dataUrl: string + scale: number + viewport: ScreenshotSize | null + imageSize: ScreenshotSize | null +} + +function screenshotViewportMetrics( + metrics: { + cssLayoutViewport?: CdpViewport + layoutViewport?: CdpViewport + } | null +): ScreenshotViewportMetrics | null { + const viewport = metrics?.cssLayoutViewport ?? metrics?.layoutViewport + const width = viewport?.clientWidth ?? 0 + const height = viewport?.clientHeight ?? 0 + if (width <= 0 || height <= 0) return null + const pageX = viewport?.pageX + const pageY = viewport?.pageY + const hasPagePosition = pageX !== undefined || pageY !== undefined + if ( + hasPagePosition && + (pageX === undefined || + pageY === undefined || + !Number.isFinite(pageX) || + !Number.isFinite(pageY)) + ) { + return null + } + return { + width, + height, + pageX: pageX ?? null, + pageY: pageY ?? null, + unit: metrics?.cssLayoutViewport ? 'css' : 'device', + } +} + +function sameScreenshotViewport( + before: ScreenshotViewportMetrics | null, + after: ScreenshotViewportMetrics | null +): boolean { + if (!before || !after) return false + return ( + before.unit === after.unit && + before.width === after.width && + before.height === after.height && + before.pageX === after.pageX && + before.pageY === after.pageY + ) } /** @@ -401,17 +466,18 @@ interface CdpViewport { * (cssX = imageX / scale) — including on a 2x display, where an unclipped * capture arrives at device resolution and this is what brings it back down. */ -export async function captureScreenshot( - contents: WebContents -): Promise<{ dataUrl: string; scale: number }> { +export async function captureScreenshot(contents: WebContents): Promise { const metrics = await send<{ cssLayoutViewport?: CdpViewport layoutViewport?: CdpViewport }>(contents, 'Page.getLayoutMetrics').catch(() => null) - const viewport = metrics?.cssLayoutViewport ?? metrics?.layoutViewport - const width = viewport?.clientWidth ?? 0 - const height = viewport?.clientHeight ?? 0 + const captureViewport = screenshotViewportMetrics(metrics) + const width = captureViewport?.width ?? 0 + const height = captureViewport?.height ?? 0 + const cssWidth = metrics?.cssLayoutViewport?.clientWidth ?? 0 + const cssHeight = metrics?.cssLayoutViewport?.clientHeight ?? 0 + const cssViewport = cssWidth > 0 && cssHeight > 0 ? { width: cssWidth, height: cssHeight } : null const scale = width > 0 && height > 0 ? Math.min(1, MAX_SCREENSHOT_EDGE / Math.max(width, height)) : 1 @@ -419,26 +485,32 @@ export async function captureScreenshot( format: 'jpeg', quality: SCREENSHOT_CAPTURE_QUALITY, }) + const metricsAfterCapture = await send<{ + cssLayoutViewport?: CdpViewport + layoutViewport?: CdpViewport + }>(contents, 'Page.getLayoutMetrics').catch(() => null) + if (!sameScreenshotViewport(captureViewport, screenshotViewportMetrics(metricsAfterCapture))) { + throw new Error('The page viewport changed or could not be verified during screenshot capture') + } const captured = `data:image/jpeg;base64,${result.data}` const targetWidth = Math.round(width * scale) const targetHeight = Math.round(height * scale) - // Without layout metrics there is no CSS frame of reference to resize - // against, so the raw capture is the honest answer — the same fallback the - // clipped path took. - if (targetWidth <= 0 || targetHeight <= 0) return { dataUrl: captured, scale } - const image = nativeImage.createFromBuffer(Buffer.from(result.data, 'base64')) const size = image.isEmpty() ? { width: 0, height: 0 } : image.getSize() - if (size.width === 0 || size.height === 0) return { dataUrl: captured, scale } + if (size.width === 0 || size.height === 0) { + return { dataUrl: captured, scale, viewport: cssViewport, imageSize: null } + } if (size.width === targetWidth && size.height === targetHeight) { - return { dataUrl: captured, scale } + return { dataUrl: captured, scale, viewport: cssViewport, imageSize: size } } const resized = image.resize({ width: targetWidth, height: targetHeight, quality: 'good' }) return { dataUrl: `data:image/jpeg;base64,${resized.toJPEG(SCREENSHOT_QUALITY).toString('base64')}`, scale, + viewport: cssViewport, + imageSize: { width: targetWidth, height: targetHeight }, } } diff --git a/apps/desktop/src/main/browser-agent/driver.test.ts b/apps/desktop/src/main/browser-agent/driver.test.ts index 84613c62d1a..ade81aff505 100644 --- a/apps/desktop/src/main/browser-agent/driver.test.ts +++ b/apps/desktop/src/main/browser-agent/driver.test.ts @@ -3,7 +3,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) -import { BrowserWindow, Menu } from 'electron' +import { BrowserWindow, Menu, nativeImage } from 'electron' import * as cdp from '@/main/browser-agent/cdp' import * as driverModule from '@/main/browser-agent/driver' import * as session from '@/main/browser-agent/session' @@ -1088,6 +1088,22 @@ describe('executeTool', () => { }) }) +describe('browserToolWatchdogMs', () => { + it.each([ + ['number', 30_000, 35_000], + ['numeric string', '30000', 35_000], + ['absent', undefined, 15_000], + ['non-numeric', 'soon', 15_000], + ['zero', 0, 15_000], + ['negative', -5_000, 15_000], + ['above the wait clamp', 500_000, 125_000], + ])('normalizes browser_wait_for timeout (%s)', (_label, timeoutMs, expected) => { + const params = timeoutMs === undefined ? {} : { timeoutMs } + + expect(driverModule.browserToolWatchdogMs('browser_wait_for', params)).toBe(expected) + }) +}) + /** * Trusted CDP input never enters the page, so a focused credential field can * only be ruled out in the driver. These cover that seam; the page-side @@ -1159,6 +1175,15 @@ describe('credential protection', () => { .mock.calls.filter(([called]) => called === method) } + function mockScreenshotImage(size: { width: number; height: number } | null): void { + vi.mocked(nativeImage.createFromBuffer).mockReturnValueOnce({ + isEmpty: vi.fn(() => size === null), + getSize: vi.fn(() => size ?? { width: 0, height: 0 }), + resize: vi.fn(() => ({ toJPEG: vi.fn(() => Buffer.from('resized')) })), + toJPEG: vi.fn(() => Buffer.alloc(0)), + } as unknown as ReturnType) + } + it('refuses a keystroke while a password field holds focus', async () => { const contents = await openPage() respondWith(contents, { activeElementSecrecy: 'secret' }) @@ -1167,6 +1192,8 @@ describe('credential protection', () => { expect(result.ok).toBe(false) expect(result.error).toMatch(/Refusing to act on a password field/) + expect(result.error).toMatch(/visible browser/) + expect(result.error).not.toContain('browser_request_takeover') expect(cdpCalls(contents, 'Input.dispatchKeyEvent')).toHaveLength(0) }) @@ -1532,6 +1559,24 @@ describe('credential protection', () => { }) }) + it('rejects an unsupported browser_scroll direction instead of treating it as down', async () => { + const contents = await openPage() + + const result = await driver.executeTool('chat-test', 'browser_scroll', { + direction: 'sideways', + }) + + expect(result).toMatchObject({ + ok: false, + error: 'Scroll direction must be "up" or "down".', + }) + expect( + vi + .mocked(contents.executeJavaScript) + .mock.calls.some(([expression]) => isPageCall(String(expression), 'scrollPage')) + ).toBe(false) + }) + it('confirms a click when the requested target changes semantic state', async () => { const contents = await openPage() let actionReads = 0 @@ -2179,6 +2224,83 @@ describe('credential protection', () => { expect(cdpCalls(contents, 'Input.insertText')).toHaveLength(1) }) + it('reports top-page effects observed after inserting text in a child frame', async () => { + const contents = await openPage() + const mainFrame = { + frameTreeNodeId: 1, + detached: false, + isDestroyed: vi.fn(() => false), + origin: 'https://example.com', + parent: null, + framesInSubtree: [] as unknown[], + } + const childFrame = { + frameTreeNodeId: 2, + detached: false, + isDestroyed: vi.fn(() => false), + origin: 'https://mail-widget.example', + parent: mainFrame, + url: 'https://mail-widget.example/compose', + } + mainFrame.framesInSubtree = [mainFrame, childFrame] + Object.defineProperty(contents, 'mainFrame', { configurable: true, value: mainFrame }) + Object.defineProperty(contents, 'focusedFrame', { configurable: true, value: childFrame }) + let topPageReads = 0 + vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => { + if (isPageCall(expression, 'readPageActionState')) { + topPageReads++ + return Promise.resolve({ + url: + topPageReads === 1 ? 'https://example.com/compose' : 'https://example.com/message/sent', + title: 'Mail', + focus: 'iframe', + mutationRevision: topPageReads, + dialogs: [], + scroll: [0], + }) + } + return Promise.resolve(undefined) + }) + const isolatedFrameEval = vi + .spyOn(cdp, 'evaluateInIsolatedFrame') + .mockImplementation((_contents, _frame, expression) => { + if (isPageCall(expression, 'activeElementSecrecy')) return Promise.resolve('safe') + if (isPageCall(expression, 'describeFocusedEditable')) { + return Promise.resolve({ editable: true, kind: 'input' }) + } + if (isPageCall(expression, 'readActiveElementState')) { + return Promise.resolve({ activeElement: 'input', valueLength: 4 }) + } + if (isPageCall(expression, 'readPageActionState')) { + return Promise.resolve({ + url: 'https://mail-widget.example/compose', + title: 'Compose', + focus: 'input', + mutationRevision: 0, + dialogs: [], + scroll: [0], + }) + } + return Promise.resolve(undefined) + }) + + try { + const result = await driver.executeTool('chat-test', 'browser_insert_text', { text: 'sent' }) + + expect(result.ok, result.error).toBe(true) + expect(result).toMatchObject({ + ok: true, + result: { + effectObserved: true, + possibleEffectObserved: true, + effect: { urlChanged: true }, + }, + }) + } finally { + isolatedFrameEval.mockRestore() + } + }) + it('refuses insertion when nothing editable holds focus', async () => { const contents = await openPage() respondWith(contents, { @@ -2272,6 +2394,7 @@ describe('credential protection', () => { it('returns the screenshot scale for coordinate mapping', async () => { const contents = await openPage() + mockScreenshotImage({ width: 1024, height: 512 }) vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { if (method === 'Page.getLayoutMetrics') { return Promise.resolve({ @@ -2287,6 +2410,229 @@ describe('credential protection', () => { const result = await driver.executeTool('chat-test', 'browser_screenshot', {}) - expect(result).toMatchObject({ ok: true, result: { scale: 0.5 } }) + expect(result).toMatchObject({ + ok: true, + result: { + scale: 0.5, + viewport: { + url: 'https://example.com/login', + title: 'Example', + width: 2048, + height: 1024, + }, + }, + }) + expect( + vi + .mocked(contents.executeJavaScript) + .mock.calls.some(([expression]) => isPageCall(String(expression), 'getViewportInfo')) + ).toBe(false) + }) + + it('uses the in-page CSS viewport when CDP exposes only deprecated device metrics', async () => { + const contents = await openPage() + mockScreenshotImage({ width: 1024, height: 512 }) + vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + return Promise.resolve({ layoutViewport: { clientWidth: 2048, clientHeight: 1024 } }) + } + if (method === 'Page.captureScreenshot') { + return Promise.resolve({ data: 'c2lt' }) + } + return Promise.resolve(undefined) + }) + respondWith(contents, { + getViewportInfo: { + url: 'https://example.com/login', + title: 'Example', + width: 1024, + height: 512, + }, + }) + + const result = await driver.executeTool('chat-test', 'browser_screenshot', {}) + + expect(result).toMatchObject({ + ok: true, + result: { + scale: 1, + viewport: { + url: 'https://example.com/login', + title: 'Example', + width: 1024, + height: 512, + }, + }, + }) + if ( + !result.ok || + typeof result.result !== 'object' || + result.result === null || + !('scale' in result.result) || + typeof result.result.scale !== 'number' + ) { + throw new Error('browser_screenshot did not return a numeric coordinate scale') + } + expect(1024 / result.result.scale).toBe(1024) + expect( + vi + .mocked(contents.executeJavaScript) + .mock.calls.some(([expression]) => isPageCall(String(expression), 'getViewportInfo')) + ).toBe(true) + }) + + it('accepts stable truncated page identity with deprecated device metrics', async () => { + const contents = await openPage() + const fullUrl = `https://example.com/${'u'.repeat(5000)}` + const fullTitle = `Example ${'t'.repeat(600)}` + vi.mocked(contents.getURL).mockReturnValue(fullUrl) + vi.mocked(contents.getTitle).mockReturnValue(fullTitle) + mockScreenshotImage({ width: 1024, height: 512 }) + vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + return Promise.resolve({ layoutViewport: { clientWidth: 2048, clientHeight: 1024 } }) + } + if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' }) + return Promise.resolve(undefined) + }) + respondWith(contents, { + getViewportInfo: { + url: fullUrl.slice(0, 4096), + title: fullTitle.slice(0, 500), + width: 1024, + height: 512, + }, + }) + + const result = await driver.executeTool('chat-test', 'browser_screenshot', {}) + + expect(result).toMatchObject({ + ok: true, + result: { + scale: 1, + viewport: { + url: fullUrl.slice(0, 4096), + title: fullTitle.slice(0, 500), + width: 1024, + height: 512, + }, + }, + }) + }) + + it('rejects an undecodable screenshot instead of returning an unverified scale', async () => { + const contents = await openPage() + mockScreenshotImage(null) + vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + return Promise.resolve({ + cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024 }, + }) + } + if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' }) + return Promise.resolve(undefined) + }) + + const result = await driver.executeTool('chat-test', 'browser_screenshot', {}) + + expect(result.ok).toBe(false) + expect(result.error).toMatch(/verify the screenshot dimensions/) }) + + it('rejects a screenshot when no CSS viewport can be established', async () => { + const contents = await openPage() + mockScreenshotImage({ width: 1024, height: 512 }) + vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + return Promise.resolve({ layoutViewport: { clientWidth: 2048, clientHeight: 1024 } }) + } + if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' }) + return Promise.resolve(undefined) + }) + respondWith(contents, { getViewportInfo: null }) + + const result = await driver.executeTool('chat-test', 'browser_screenshot', {}) + + expect(result.ok).toBe(false) + expect(result.error).toMatch(/verify the page viewport/) + }) + + it('rejects coordinate mapping when the viewport changes during capture', async () => { + const contents = await openPage() + mockScreenshotImage({ width: 1024, height: 256 }) + vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + return Promise.resolve({ layoutViewport: { clientWidth: 1024, clientHeight: 256 } }) + } + if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' }) + return Promise.resolve(undefined) + }) + respondWith(contents, { + getViewportInfo: { + url: 'https://example.com/login', + title: 'Example', + width: 1024, + height: 512, + }, + }) + + const result = await driver.executeTool('chat-test', 'browser_screenshot', {}) + + expect(result.ok).toBe(false) + expect(result.error).toMatch(/viewport changed while the screenshot was captured/) + }) + + it('rejects a screenshot when the document navigates during capture', async () => { + const contents = await openPage() + mockScreenshotImage({ width: 1024, height: 512 }) + vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + return Promise.resolve({ + cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024 }, + }) + } + if (method === 'Page.captureScreenshot') { + emitContentsEvent(contents, 'did-navigate') + return Promise.resolve({ data: 'c2lt' }) + } + return Promise.resolve(undefined) + }) + + const result = await driver.executeTool('chat-test', 'browser_screenshot', {}) + + expect(result.ok).toBe(false) + expect(result.error).toMatch(/page changed while its screenshot was being captured/) + }) + + it.each(['url', 'title'] as const)( + 'rejects a screenshot when the page %s changes during capture', + async (identityField) => { + const contents = await openPage() + mockScreenshotImage({ width: 1024, height: 512 }) + const initialUrl = contents.getURL() + const initialTitle = contents.getTitle() + let currentUrl = initialUrl + let currentTitle = initialTitle + vi.mocked(contents.getURL).mockImplementation(() => currentUrl) + vi.mocked(contents.getTitle).mockImplementation(() => currentTitle) + vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + return Promise.resolve({ + cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024 }, + }) + } + if (method === 'Page.captureScreenshot') { + if (identityField === 'url') currentUrl = 'https://example.com/changed' + else currentTitle = 'Changed title' + return Promise.resolve({ data: 'c2lt' }) + } + return Promise.resolve(undefined) + }) + + const result = await driver.executeTool('chat-test', 'browser_screenshot', {}) + + expect(result.ok).toBe(false) + expect(result.error).toMatch(/page changed while its screenshot was being captured/) + } + ) }) diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index d8ce0631824..216ecd958de 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -24,6 +24,7 @@ import { type BrowserPanelAction, type BrowserTabsState, type BrowserToolName, + normalizeBrowserWaitForTimeoutMs, } from '@sim/browser-protocol' import type { BrowserDownloadsState, BrowserToolbarCommand } from '@sim/desktop-bridge' import { createLogger } from '@sim/logger' @@ -71,8 +72,6 @@ const logger = createLogger('BrowserAgentDriver') const NAVIGATION_TIMEOUT_MS = 25_000 const NAVIGATION_SETTLE_MS = 400 -const DEFAULT_WAIT_FOR_TIMEOUT_MS = 10_000 -const MAX_WAIT_FOR_TIMEOUT_MS = 120_000 const TAKEOVER_POLL_MS = 1_500 /** * Hard ceiling on any single tool execution (takeover excepted): whatever @@ -718,10 +717,9 @@ function num(params: Record, key: string): number | undefined { } /** - * Native execution must time out before the renderer gives up (30s default, - * 45s navigation, requested wait + 15s). Otherwise the abandoned native - * promise keeps owning the serialized queue and every later browser action - * times out behind it. + * Bounds native execution after a call reaches the head of its serialized + * scope queue. The renderer separately budgets authorization, queueing, and + * bridge delivery around this watchdog. */ export function browserToolWatchdogMs( tool: BrowserToolName, @@ -738,10 +736,7 @@ export function browserToolWatchdogMs( return NAVIGATION_TOOL_WATCHDOG_MS } if (tool === 'browser_wait_for') { - const requested = Math.min( - num(params, 'timeoutMs') ?? DEFAULT_WAIT_FOR_TIMEOUT_MS, - MAX_WAIT_FOR_TIMEOUT_MS - ) + const requested = normalizeBrowserWaitForTimeoutMs(params.timeoutMs) return requested + WAIT_FOR_TOOL_WATCHDOG_GRACE_MS } return DEFAULT_TOOL_WATCHDOG_MS @@ -889,12 +884,11 @@ function sanitizeBrowserResult( /** * Covers focusing, clicking, and typing: the agent has no legitimate reason to - * reach a credential field, and takeover is the sanctioned path when a task - * needs one. + * reach a credential field. The user can enter credentials directly in the + * visible embedded browser before the agent resumes from a fresh snapshot. */ const PASSWORD_REFUSAL = - 'Refusing to act on a password field. Call browser_request_takeover so the user ' + - 'can enter their credentials themselves.' + 'Refusing to act on a password field. Ask the user to enter their credentials in the visible browser, then take a fresh browser_snapshot.' /** Maps sentinel `{ error: ... }` results from injected functions to ToolErrors. */ function unwrapPageResult(result: unknown): unknown { @@ -2069,10 +2063,7 @@ async function executeToolInner( case 'browser_wait_for': { const text = str(params, 'text') - const timeoutMs = Math.min( - num(params, 'timeoutMs') ?? DEFAULT_WAIT_FOR_TIMEOUT_MS, - MAX_WAIT_FOR_TIMEOUT_MS - ) + const timeoutMs = normalizeBrowserWaitForTimeoutMs(params.timeoutMs) const startedAt = Date.now() if (!text) { await sleep(timeoutMs) @@ -2136,22 +2127,102 @@ async function executeToolInner( } case 'browser_screenshot': { - const contents = session.requireAutomationTab().view.webContents - const shot = await cdp.captureScreenshot(contents).catch(() => null) - if (shot === null) { + const capturedTab = session.requireAutomationTab() + const contents = capturedTab.view.webContents + const capturedNavigationEpoch = navigationEpoch(contents) + const capturedUrl = contents.getURL() + const capturedTitle = contents.getTitle() + const capturedViewportUrl = capturedUrl.slice(0, 4096) + const capturedViewportTitle = capturedTitle.slice(0, 500) + const captureIsCurrent = (): boolean => { + const activeTab = session.automationTab() + return ( + activeTab?.id === capturedTab.id && + activeTab.view.webContents === contents && + !contents.isDestroyed() && + navigationEpoch(contents) === capturedNavigationEpoch && + contents.getURL() === capturedUrl && + contents.getTitle() === capturedTitle + ) + } + const assertCaptureIsCurrent = (): void => { + if (captureIsCurrent()) return + throw new ToolError( + 'The page changed while its screenshot was being captured. Retry browser_screenshot before using image coordinates.' + ) + } + const shot = await cdp.captureScreenshot(contents).catch((error) => { + logger.warn('Browser screenshot capture failed', { error: getErrorMessage(error) }) + return null + }) + if (!shot) { throw new ToolError( 'Could not capture the page. Use browser_snapshot or browser_read_text instead.' ) } + assertCaptureIsCurrent() if (shot.dataUrl.length > 8_000_000) { throw new ToolError( 'The screenshot result was too large to return safely. Use browser_snapshot or browser_read_text instead.' ) } - const viewport = await execInPage(contents, getViewportInfo, []).catch(() => null) + if (!shot.imageSize) { + throw new ToolError( + 'Could not verify the screenshot dimensions. Retry browser_screenshot or use browser_snapshot instead.' + ) + } + const viewport = shot.viewport + ? { + url: capturedViewportUrl, + title: capturedViewportTitle, + ...shot.viewport, + } + : await execInPage(contents, getViewportInfo, []).catch(() => null) + assertCaptureIsCurrent() + if ( + !shot.viewport && + isRecordLike(viewport) && + (viewport.url !== capturedViewportUrl || viewport.title !== capturedViewportTitle) + ) { + throw new ToolError( + 'The page changed while its screenshot viewport was being verified. Retry browser_screenshot before using image coordinates.' + ) + } + let scale = shot.scale + const viewportWidth = + isRecordLike(viewport) && typeof viewport.width === 'number' ? viewport.width : 0 + const viewportHeight = + isRecordLike(viewport) && typeof viewport.height === 'number' ? viewport.height : 0 + if ( + !Number.isFinite(viewportWidth) || + !Number.isFinite(viewportHeight) || + viewportWidth <= 0 || + viewportHeight <= 0 + ) { + throw new ToolError( + 'Could not verify the page viewport for this screenshot. Retry browser_screenshot or use browser_snapshot instead.' + ) + } + if (!shot.viewport) { + const widthScale = shot.imageSize.width / viewportWidth + const heightScale = shot.imageSize.height / viewportHeight + const scaleDelta = Math.abs(widthScale - heightScale) + if ( + !Number.isFinite(widthScale) || + !Number.isFinite(heightScale) || + widthScale <= 0 || + heightScale <= 0 || + scaleDelta > Math.max(widthScale, heightScale) * 0.02 + ) { + throw new ToolError( + 'The page viewport changed while the screenshot was captured. Retry browser_screenshot before using image coordinates.' + ) + } + scale = widthScale + } // scale maps image pixels back to CSS viewport pixels for the // coordinate tools: cssX = imageX / scale. - return { dataUrl: shot.dataUrl, viewport, scale: shot.scale } + return { dataUrl: shot.dataUrl, viewport, scale } } case 'browser_extract': { @@ -2928,8 +2999,8 @@ async function executeToolInner( if (secrecy === 'opaque' && !safeForOpaqueFocus) { throw new ToolError( 'Focus is inside a cross-origin frame whose contents cannot be inspected, so this ' + - 'keystroke could mutate or activate a password field. Call browser_request_takeover if the ' + - 'user needs to type here.' + 'keystroke could mutate or activate a password field. Ask the user to type in the visible ' + + 'browser, then take a fresh browser_snapshot.' ) } let trusted = true @@ -3051,6 +3122,10 @@ async function executeToolInner( } case 'browser_scroll': { + const direction = requireStr(params, 'direction') + if (direction !== 'up' && direction !== 'down') { + throw new ToolError('Scroll direction must be "up" or "down".') + } const contents = session.requireAutomationTab().view.webContents const elementId = num(params, 'elementId') const target = @@ -3071,7 +3146,7 @@ async function executeToolInner( await execInPage( target, scrollPage, - [requireStr(params, 'direction'), num(params, 'amount'), elementId], + [direction, num(params, 'amount'), elementId], false, executionDeadline ) @@ -3379,7 +3454,7 @@ async function executeToolInner( const notes: string[] = [] if (pointTarget.secret === true) { notes.push( - 'The point resolves to a password field. Focusing it is fine, but typing there is refused — call browser_request_takeover for credentials.' + 'The point resolves to a password field. Focusing it is fine, but typing there is refused — ask the user to enter credentials in the visible browser, then take a fresh browser_snapshot.' ) } if (pointTarget.crossOriginFrame === true) { @@ -3424,7 +3499,8 @@ async function executeToolInner( if (secrecy === 'opaque') { throw new ToolError( 'Focus is inside a cross-origin frame whose contents cannot be inspected, so this ' + - 'insertion could reach a password field. Call browser_request_takeover if the user needs to type here.' + 'insertion could reach a password field. Ask the user to type in the visible browser, then ' + + 'take a fresh browser_snapshot.' ) } const focusState = unwrapPageResult( @@ -3491,15 +3567,15 @@ async function executeToolInner( const topObservation = insertInFrame ? pageEffect(beforeTopPage, await pageActionState(contents, true), beforeElement, state) : observation + const effect: Record = { + ...observation.effect, + urlChanged: observation.effect.urlChanged || topObservation.effect.urlChanged, + dialogChanged: observation.effect.dialogChanged || topObservation.effect.dialogChanged, + } // targetChanged is deliberately absent: this tool has no elementId, so // pageActionState captures no targetState and the term could only ever // be false. Listing it read as coverage this tool does not have. - const effectObserved = - observation.effect.fieldChanged || - observation.effect.urlChanged || - observation.effect.dialogChanged || - topObservation.effect.urlChanged || - topObservation.effect.dialogChanged + const effectObserved = effect.fieldChanged || effect.urlChanged || effect.dialogChanged return { dispatched: true, trusted: true, @@ -3507,8 +3583,9 @@ async function executeToolInner( insertedChars: text.length, ...state, effectObserved, - possibleEffectObserved: observation.possibleEffectObserved, - effect: observation.effect, + possibleEffectObserved: + observation.possibleEffectObserved || topObservation.possibleEffectObserved, + effect, submitRequested: submit, submitDispatched, ...(focusState.kind === 'canvas' || focusState.kind === 'textbox-role' @@ -3676,6 +3753,7 @@ export async function executeTool( toolCallId?: string, authorizationBoundary?: BrowserToolQueueBoundary ): Promise<{ ok: boolean; result?: unknown; error?: string }> { + const queuedAt = Date.now() const resolvedScopeId = resolveDriverScopeId(scopeId) if (session.isBrowserScopeSuspended(resolvedScopeId)) { return { @@ -3688,6 +3766,8 @@ export async function executeTool( const invocationEpoch = ++state.toolInvocationEpoch const queueCancellationEpoch = state.toolQueueCancellationEpoch const run = async () => { + const queueWaitMs = Date.now() - queuedAt + const executionStartedAt = Date.now() if ( (authorizationBoundary && !isBrowserToolQueueBoundaryCurrent(authorizationBoundary)) || queueCancellationEpoch !== state.toolQueueCancellationEpoch || @@ -3702,7 +3782,12 @@ export async function executeTool( }) state.activeToolCancel = cancelActiveExecution return await session.withBrowserScope(resolvedScopeId, async () => { - logger.info('Executing browser tool', { tool, scopeId: resolvedScopeId }) + logger.info('Executing browser tool', { + tool, + toolCallId, + scopeId: resolvedScopeId, + queueWaitMs, + }) const keepHiddenPageActive = tool !== 'browser_request_takeover' if (keepHiddenPageActive) { session.setAutomationActive(true) @@ -3732,7 +3817,15 @@ export async function executeTool( invalidateSnapshot(state) } }) - return withNotices(await Promise.race([guardedExecution, cancellation])) + const result = withNotices(await Promise.race([guardedExecution, cancellation])) + logger.info('Browser tool completed', { + tool, + toolCallId, + scopeId: resolvedScopeId, + queueWaitMs, + executionMs: Date.now() - executionStartedAt, + }) + return result } finally { if (keepHiddenPageActive) { session.setAutomationActive(false) @@ -3757,7 +3850,13 @@ export async function executeTool( invalidateSnapshot(state) } const message = String(sanitizeBrowserResult(getErrorMessage(error), undefined, 0, 'error')) - logger.warn('Browser tool failed', { tool, error: message }) + logger.warn('Browser tool failed', { + tool, + toolCallId, + scopeId: resolvedScopeId, + totalMs: Date.now() - queuedAt, + error: message, + }) return { ok: false, error: message } } } diff --git a/apps/desktop/src/main/ipc.test.ts b/apps/desktop/src/main/ipc.test.ts index b8bfc8cc8c2..118288347a2 100644 --- a/apps/desktop/src/main/ipc.test.ts +++ b/apps/desktop/src/main/ipc.test.ts @@ -799,6 +799,45 @@ describe('registerIpcHandlers', () => { cancelActive.mockRestore() }) + it('rejects browser tools whose server authorization exceeds its execution budget', async () => { + const { invoke } = collectHandlers() + const executeTool = vi.spyOn(browserDriver, 'executeTool') + const authorizationController = new AbortController() + const timeout = vi.spyOn(AbortSignal, 'timeout').mockReturnValue(authorizationController.signal) + const fetchAuthorization = vi.fn((_url: string, request?: RequestInit) => { + const signal = request?.signal + return new Promise((_resolve, reject) => { + signal?.addEventListener('abort', () => reject(signal.reason), { once: true }) + }) + }) + const delayedEvent = { + senderFrame: { url: `${APP}/workspace/ws1` }, + sender: { session: { fetch: fetchAuthorization } }, + } + + const execution = invoke.get('browser-agent:execute-tool')?.( + delayedEvent, + 'tool-stalled-authorization', + 'browser_snapshot', + {}, + 'chat-stalled-authorization' + ) + authorizationController.abort(new DOMException('timed out', 'TimeoutError')) + + await expect(execution).resolves.toMatchObject({ + ok: false, + error: expect.stringContaining('authorized pending Copilot tool call'), + }) + expect(fetchAuthorization).toHaveBeenCalledWith( + `${APP}/api/desktop/tool/authorize`, + expect.objectContaining({ signal: expect.any(AbortSignal) }) + ) + expect(timeout).toHaveBeenCalledWith(8_000) + expect(executeTool).not.toHaveBeenCalled() + timeout.mockRestore() + executeTool.mockRestore() + }) + it('rejects a browser tool when the renderer claims a different scope than authorization', async () => { const { invoke } = collectHandlers() const handler = invoke.get('browser-agent:execute-tool') @@ -821,6 +860,40 @@ describe('registerIpcHandlers', () => { }) }) + it('rejects a retired browser tool even if authorization echoes it', async () => { + const { invoke } = collectHandlers() + const executeTool = vi.spyOn(browserDriver, 'executeTool') + const authorizedEvent = { + senderFrame: { url: `${APP}/workspace/ws1` }, + sender: { + session: { + fetch: vi.fn(async () => + Response.json({ + chatId: 'chat-1', + toolName: 'browser_request_takeover', + args: { reason: 'Legacy handoff' }, + }) + ), + }, + }, + } + + await expect( + invoke.get('browser-agent:execute-tool')?.( + authorizedEvent, + 'tool-retired', + 'browser_request_takeover', + {}, + 'chat-1' + ) + ).resolves.toMatchObject({ + ok: false, + error: expect.stringContaining('authorized pending Copilot tool call'), + }) + expect(executeTool).not.toHaveBeenCalled() + executeTool.mockRestore() + }) + it('rejects a browser tool authorized after its scope cancellation boundary', async () => { const { invoke } = collectHandlers() const executeHandler = invoke.get('browser-agent:execute-tool') diff --git a/apps/desktop/src/main/ipc.ts b/apps/desktop/src/main/ipc.ts index a2a3936d061..8221c022fdf 100644 --- a/apps/desktop/src/main/ipc.ts +++ b/apps/desktop/src/main/ipc.ts @@ -7,7 +7,7 @@ import { type BrowserPanelSnapshot, isBrowserDataKind, isBrowserTheme, - isBrowserToolName, + isCurrentBrowserToolName, } from '@sim/browser-protocol' import { type DesktopNotificationPayload, @@ -94,6 +94,7 @@ const logger = createLogger('DesktopIpc') /** Workspace/chat ids are opaque tokens; anything else never reaches a URL. */ const ID_PATTERN = /^[A-Za-z0-9_-]{1,128}$/ const TERMINAL_WRITE_CHUNK_CHARACTERS = 64 * 1024 +const DESKTOP_TOOL_AUTHORIZATION_TIMEOUT_MS = 8_000 function writeTerminalText( terminal: TerminalRegistry, @@ -503,6 +504,7 @@ async function fetchDesktopToolAuthorization( if (typeof toolCallId !== 'string' || toolCallId.length < 1 || toolCallId.length > 256) { return null } + const startedAt = Date.now() try { const response = await event.sender.session.fetch( `${deps.appOrigin()}/api/desktop/tool/authorize`, @@ -511,9 +513,17 @@ async function fetchDesktopToolAuthorization( credentials: 'include', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ toolCallId }), + signal: AbortSignal.timeout(DESKTOP_TOOL_AUTHORIZATION_TIMEOUT_MS), } ) - if (!response.ok) return null + if (!response.ok) { + logger.warn('Desktop tool authorization was rejected', { + toolCallId, + status: response.status, + durationMs: Date.now() - startedAt, + }) + return null + } const authorization = (await response.json()) as { chatId?: unknown toolName?: unknown @@ -527,6 +537,10 @@ async function fetchDesktopToolAuthorization( authorization.args === null || Array.isArray(authorization.args) ) { + logger.warn('Desktop tool authorization returned a malformed response', { + toolCallId, + durationMs: Date.now() - startedAt, + }) return null } return { @@ -534,7 +548,12 @@ async function fetchDesktopToolAuthorization( toolName: authorization.toolName, args: authorization.args as Record, } - } catch { + } catch (error) { + logger.warn('Desktop tool authorization failed', { + toolCallId, + durationMs: Date.now() - startedAt, + error: getErrorMessage(error), + }) return null } } @@ -825,7 +844,7 @@ export function registerIpcHandlers(deps: IpcDeps): void { typeof scope !== 'string' || typeof toolCallId !== 'string' || typeof tool !== 'string' || - !isBrowserToolName(tool) + !isCurrentBrowserToolName(tool) ) { return { ok: false, error: `Unknown browser tool: ${String(tool)}` } } @@ -1893,7 +1912,7 @@ export function registerIpcHandlers(deps: IpcDeps): void { authorization.chatId !== requestedScope || typeof requestedTool !== 'string' || authorization.toolName !== requestedTool || - !isBrowserToolName(authorization.toolName) + !isCurrentBrowserToolName(authorization.toolName) ) { return { ok: false, diff --git a/apps/sim/app/api/copilot/confirm/route.test.ts b/apps/sim/app/api/copilot/confirm/route.test.ts index 71c962e78c6..4e6cf0f262e 100644 --- a/apps/sim/app/api/copilot/confirm/route.test.ts +++ b/apps/sim/app/api/copilot/confirm/route.test.ts @@ -9,6 +9,8 @@ const { getAsyncToolCall, getRunSegment, completeAsyncToolCall, + completeClaimedAsyncToolCall, + completePendingAsyncToolCall, detachAsyncToolCall, publishToolConfirmation, encryptSecret, @@ -17,6 +19,8 @@ const { getAsyncToolCall: vi.fn(), getRunSegment: vi.fn(), completeAsyncToolCall: vi.fn(), + completeClaimedAsyncToolCall: vi.fn(), + completePendingAsyncToolCall: vi.fn(), detachAsyncToolCall: vi.fn(), publishToolConfirmation: vi.fn(), encryptSecret: vi.fn(), @@ -29,6 +33,8 @@ vi.mock('@/lib/copilot/async-runs/repository', () => ({ getAsyncToolCall, getRunSegment, completeAsyncToolCall, + completeClaimedAsyncToolCall, + completePendingAsyncToolCall, detachAsyncToolCall, getClaimedWorkflowExecutionId: (claimedBy?: string | null) => claimedBy?.startsWith('workflow:') ? claimedBy.slice('workflow:'.length) : undefined, @@ -72,6 +78,8 @@ describe('Copilot Confirm API Route', () => { workflowId: 'workflow-from-run', }) completeAsyncToolCall.mockResolvedValue(existingRow) + completeClaimedAsyncToolCall.mockResolvedValue(existingRow) + completePendingAsyncToolCall.mockResolvedValue(existingRow) detachAsyncToolCall.mockResolvedValue(existingRow) encryptSecret.mockResolvedValue({ encrypted: 'sealed-client-result', iv: 'iv' }) getTrustedWorkflowToolExecution.mockResolvedValue({ status: 'completed' }) @@ -241,7 +249,7 @@ describe('Copilot Confirm API Route', () => { ) }) - it('rejects a native confirmation before the desktop authorization claim', async () => { + it('rejects a native success before the desktop authorization claim', async () => { getAsyncToolCall.mockResolvedValue({ ...existingRow, toolName: 'browser_snapshot', @@ -263,6 +271,210 @@ describe('Copilot Confirm API Route', () => { expect(publishToolConfirmation).not.toHaveBeenCalled() }) + it.each([ + ['browser_snapshot', 'error', 'failed'], + ['browser_snapshot', 'cancelled', 'cancelled'], + ['terminal', 'error', 'failed'], + ['terminal', 'cancelled', 'cancelled'], + ] as const)( + 'accepts a pending %s %s before the desktop authorization claim', + async (toolName, status, durableStatus) => { + getAsyncToolCall.mockResolvedValue({ + ...existingRow, + toolName, + status: 'pending', + }) + + const response = await POST( + createMockPostRequest({ + toolCallId: 'tool-call-123', + status, + message: 'The desktop action did not start.', + }) + ) + + expect(response.status).toBe(200) + expect(completePendingAsyncToolCall).toHaveBeenCalledWith({ + toolCallId: 'tool-call-123', + status: durableStatus, + result: { __sealedClientToolCompletionV1: 'sealed-client-result' }, + error: status === 'error' ? 'Tool failed' : 'Tool cancelled', + }) + expect(completeAsyncToolCall).not.toHaveBeenCalled() + expect(detachAsyncToolCall).not.toHaveBeenCalled() + expect(publishToolConfirmation).toHaveBeenCalledWith( + expect.objectContaining({ toolCallId: 'tool-call-123', status }) + ) + } + ) + + it.each([ + ['browser_snapshot', 'error'], + ['terminal', 'cancelled'], + ] as const)( + 'rejects a pending %s %s when the native authorization claim wins the race', + async (toolName, status) => { + getAsyncToolCall.mockResolvedValue({ + ...existingRow, + toolName, + status: 'pending', + }) + completePendingAsyncToolCall.mockResolvedValueOnce(null) + + const response = await POST( + createMockPostRequest({ + toolCallId: 'tool-call-123', + status, + message: 'The desktop action did not start.', + }) + ) + + expect(response.status).toBe(404) + expect(await response.json()).toEqual({ error: 'Pending client tool call not found' }) + expect(completePendingAsyncToolCall).toHaveBeenCalledOnce() + expect(completeClaimedAsyncToolCall).not.toHaveBeenCalled() + expect(completeAsyncToolCall).not.toHaveBeenCalled() + expect(detachAsyncToolCall).not.toHaveBeenCalled() + expect(publishToolConfirmation).not.toHaveBeenCalled() + } + ) + + it.each([ + ['browser_snapshot', 'desktop-browser'], + ['terminal', 'desktop-terminal'], + ] as const)( + 'settles an indeterminate pending %s result when the exact %s claim wins the race', + async (toolName, claimOwner) => { + getAsyncToolCall.mockResolvedValue({ + ...existingRow, + toolName, + status: 'pending', + }) + completePendingAsyncToolCall.mockResolvedValueOnce(null) + + const response = await POST( + createMockPostRequest({ + toolCallId: 'tool-call-123', + status: 'error', + message: 'untrusted page-exit message', + data: { outcomeUnknown: true, doNotRetry: true, untrusted: 'discard me' }, + }) + ) + + expect(response.status).toBe(200) + expect(completePendingAsyncToolCall).toHaveBeenCalledOnce() + expect(completeClaimedAsyncToolCall).toHaveBeenCalledWith( + { + toolCallId: 'tool-call-123', + status: 'failed', + result: { __sealedClientToolCompletionV1: 'sealed-client-result' }, + error: 'Tool failed', + }, + claimOwner + ) + expect(encryptSecret).toHaveBeenCalledWith(expect.stringContaining('"outcomeUnknown":true')) + expect(encryptSecret).toHaveBeenCalledWith(expect.not.stringContaining('discard me')) + expect(publishToolConfirmation).toHaveBeenCalledOnce() + } + ) + + it('does not publish when another terminal transition wins indeterminate claim reconciliation', async () => { + getAsyncToolCall.mockResolvedValue({ + ...existingRow, + toolName: 'browser_snapshot', + status: 'pending', + }) + completePendingAsyncToolCall.mockResolvedValueOnce(null) + completeClaimedAsyncToolCall.mockResolvedValueOnce(null) + + const response = await POST( + createMockPostRequest({ + toolCallId: 'tool-call-123', + status: 'error', + data: { outcomeUnknown: true, doNotRetry: true }, + }) + ) + + expect(response.status).toBe(404) + expect(completeClaimedAsyncToolCall).toHaveBeenCalledWith(expect.any(Object), 'desktop-browser') + expect(publishToolConfirmation).not.toHaveBeenCalled() + }) + + it('returns 500 without publishing when exact claim reconciliation fails', async () => { + getAsyncToolCall.mockResolvedValue({ + ...existingRow, + toolName: 'browser_snapshot', + status: 'pending', + }) + completePendingAsyncToolCall.mockResolvedValueOnce(null) + completeClaimedAsyncToolCall.mockRejectedValueOnce(new Error('database unavailable')) + + const response = await POST( + createMockPostRequest({ + toolCallId: 'tool-call-123', + status: 'error', + data: { outcomeUnknown: true, doNotRetry: true }, + }) + ) + + expect(response.status).toBe(500) + expect(publishToolConfirmation).not.toHaveBeenCalled() + }) + + it.each(['error', 'cancelled'] as const)( + 'rejects a pending retired browser tool %s before a native claim', + async (status) => { + getAsyncToolCall.mockResolvedValue({ + ...existingRow, + toolName: 'browser_request_takeover', + status: 'pending', + }) + + const response = await POST( + createMockPostRequest({ + toolCallId: 'tool-call-123', + status, + message: 'A stale renderer tried to finalize this call.', + }) + ) + + expect(response.status).toBe(404) + expect(completePendingAsyncToolCall).not.toHaveBeenCalled() + expect(completeAsyncToolCall).not.toHaveBeenCalled() + expect(detachAsyncToolCall).not.toHaveBeenCalled() + expect(encryptSecret).not.toHaveBeenCalled() + expect(publishToolConfirmation).not.toHaveBeenCalled() + } + ) + + it('accepts a running retired browser tool completion for historical compatibility', async () => { + getAsyncToolCall.mockResolvedValue({ + ...existingRow, + toolName: 'browser_request_takeover', + status: 'running', + }) + + const response = await POST( + createMockPostRequest({ + toolCallId: 'tool-call-123', + status: 'success', + data: { completed: true }, + }) + ) + + expect(response.status).toBe(200) + expect(completePendingAsyncToolCall).not.toHaveBeenCalled() + expect(completeAsyncToolCall).toHaveBeenCalledWith({ + toolCallId: 'tool-call-123', + status: 'completed', + result: { __sealedClientToolCompletionV1: 'sealed-client-result' }, + error: null, + }) + expect(publishToolConfirmation).toHaveBeenCalledWith( + expect.objectContaining({ toolCallId: 'tool-call-123', status: 'success' }) + ) + }) + it('rejects a workflow confirmation before the server starts the tool call', async () => { getAsyncToolCall.mockResolvedValue({ ...existingRow, diff --git a/apps/sim/app/api/copilot/confirm/route.ts b/apps/sim/app/api/copilot/confirm/route.ts index 96dbdcccae7..40001f5392b 100644 --- a/apps/sim/app/api/copilot/confirm/route.ts +++ b/apps/sim/app/api/copilot/confirm/route.ts @@ -1,4 +1,4 @@ -import { isBrowserToolName } from '@sim/browser-protocol' +import { isBrowserToolName, isCurrentBrowserToolName } from '@sim/browser-protocol' import { createLogger } from '@sim/logger' import { isTerminalToolName } from '@sim/terminal-protocol' import { getErrorMessage, toError } from '@sim/utils/errors' @@ -11,12 +11,15 @@ import { ASYNC_TOOL_STATUS, type AsyncCompletionData, type AsyncConfirmationStatus, + DESKTOP_TOOL_CLAIM_OWNER, isDeliveredAsyncStatus, isTerminalAsyncStatus, isWorkflowToolExecutionClaimable, } from '@/lib/copilot/async-runs/lifecycle' import { completeAsyncToolCall, + completeClaimedAsyncToolCall, + completePendingAsyncToolCall, detachAsyncToolCall, getAsyncToolCall, getClaimedWorkflowExecutionId, @@ -52,6 +55,17 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { getTrustedWorkflowToolExecution } from '@/lib/workflows/executor/execution-state' const logger = createLogger('CopilotConfirmAPI') +const NATIVE_HANDOFF_INTERRUPTED_MESSAGE = + 'The desktop action was interrupted during handoff. Its outcome is unknown; do not retry it automatically.' + +type ToolCallStatusUpdateOutcome = 'updated' | 'conflict' | 'failed' + +interface UpdateToolCallStatusOptions { + executionId?: string + completionGuard?: + | { status: typeof ASYNC_TOOL_STATUS.pending } + | { status: typeof ASYNC_TOOL_STATUS.running; claimedBy: string } +} function getClientToolCompletionMessage(status: AsyncConfirmationStatus): string { if (status === ASYNC_TOOL_CONFIRMATION_STATUS.success) return 'Tool completed' @@ -74,24 +88,24 @@ async function updateToolCallStatus( status: AsyncConfirmationStatus, message?: string, data?: AsyncCompletionData, - executionId?: string -): Promise { + options: UpdateToolCallStatusOptions = {} +): Promise { const toolCallId = existing.toolCallId try { if (status === ASYNC_TOOL_CONFIRMATION_STATUS.background) { - const detached = executionId + const detached = options.executionId ? await detachAsyncToolCall(toolCallId, { preserveClaim: true }) : await detachAsyncToolCall(toolCallId) - if (!detached) return false + if (!detached) return 'conflict' publishToolConfirmation({ toolCallId, status, message: message || undefined, timestamp: new Date().toISOString(), data, - ...(executionId ? { executionId } : {}), + ...(options.executionId ? { executionId: options.executionId } : {}), }) - return true + return 'updated' } const durableStatus = status === ASYNC_TOOL_CONFIRMATION_STATUS.success @@ -99,29 +113,35 @@ async function updateToolCallStatus( : status === ASYNC_TOOL_CONFIRMATION_STATUS.cancelled ? ASYNC_TOOL_STATUS.cancelled : ASYNC_TOOL_STATUS.failed - const completed = await completeAsyncToolCall({ + const completionInput = { toolCallId, status: durableStatus, result: data ?? null, error: status === 'success' ? null : message || status, - }) - if (!completed) return false + } + const completed = + options.completionGuard?.status === ASYNC_TOOL_STATUS.pending + ? await completePendingAsyncToolCall(completionInput) + : options.completionGuard?.status === ASYNC_TOOL_STATUS.running + ? await completeClaimedAsyncToolCall(completionInput, options.completionGuard.claimedBy) + : await completeAsyncToolCall(completionInput) + if (!completed) return 'conflict' publishToolConfirmation({ toolCallId, status, message: message || undefined, timestamp: new Date().toISOString(), data, - ...(executionId ? { executionId } : {}), + ...(options.executionId ? { executionId: options.executionId } : {}), }) - return true + return 'updated' } catch (error) { logger.error('Failed to update tool call status', { toolCallId, status, error: toError(error).message, }) - return false + return 'failed' } } @@ -263,18 +283,30 @@ export const POST = withRouteHandler((req: NextRequest) => { ) } - const isUnboundTerminalWorkflowOutcome = + const isErrorOrCancelledOutcome = status === ASYNC_TOOL_CONFIRMATION_STATUS.error || status === ASYNC_TOOL_CONFIRMATION_STATUS.cancelled + const isNativeClientTool = + isBrowserToolName(existing.toolName) || isTerminalToolName(existing.toolName) + const isPreclaimNativeTerminalOutcome = + (isCurrentBrowserToolName(existing.toolName) || isTerminalToolName(existing.toolName)) && + existing.status === ASYNC_TOOL_STATUS.pending && + isErrorOrCancelledOutcome + const nativeClaimOwner = isCurrentBrowserToolName(existing.toolName) + ? DESKTOP_TOOL_CLAIM_OWNER.browser + : isTerminalToolName(existing.toolName) + ? DESKTOP_TOOL_CLAIM_OWNER.terminal + : undefined + const isIndeterminateNativeExit = + isPreclaimNativeTerminalOutcome && + status === ASYNC_TOOL_CONFIRMATION_STATUS.error && + isPlainRecord(data) && + data.outcomeUnknown === true && + data.doNotRetry === true const isMutableClientToolCall = isWorkflowTool ? isWorkflowToolExecutionClaimable(existing.status, existing.permissionDecision) - : existing.status === ASYNC_TOOL_STATUS.running - if ( - (isBrowserToolName(existing.toolName) || - isTerminalToolName(existing.toolName) || - isWorkflowTool) && - !isMutableClientToolCall - ) { + : existing.status === ASYNC_TOOL_STATUS.running || isPreclaimNativeTerminalOutcome + if ((isNativeClientTool || isWorkflowTool) && !isMutableClientToolCall) { span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) return createNotFoundResponse('Running client tool call not found') } @@ -314,7 +346,7 @@ export const POST = withRouteHandler((req: NextRequest) => { if (status !== ASYNC_TOOL_CONFIRMATION_STATUS.background) { if (trustedExecution) { effectiveStatus = getWorkflowToolConfirmationStatus(trustedExecution.status) - } else if (!isUnboundTerminalWorkflowOutcome) { + } else if (!isErrorOrCancelledOutcome) { span.setAttribute( TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound @@ -327,7 +359,7 @@ export const POST = withRouteHandler((req: NextRequest) => { } else if (trustedExecution) { executionId = trustedExecution.executionId effectiveStatus = getWorkflowToolConfirmationStatus(trustedExecution.status) - } else if (!isUnboundTerminalWorkflowOutcome) { + } else if (!isErrorOrCancelledOutcome) { effectiveStatus = ASYNC_TOOL_CONFIRMATION_STATUS.error executionId = undefined } else { @@ -365,21 +397,58 @@ export const POST = withRouteHandler((req: NextRequest) => { toolCallId, runId: existing.runId, userId: authenticatedUserId, - ...(message !== undefined ? { message } : {}), - ...(data !== undefined ? { data } : {}), + ...(isIndeterminateNativeExit + ? { + message: NATIVE_HANDOFF_INTERRUPTED_MESSAGE, + data: { + error: NATIVE_HANDOFF_INTERRUPTED_MESSAGE, + outcomeUnknown: true, + doNotRetry: true, + }, + } + : { + ...(message !== undefined ? { message } : {}), + ...(data !== undefined ? { data } : {}), + }), })), }, } - const updated = await updateToolCallStatus( + const updateOutcome = await updateToolCallStatus( existing, effectiveStatus, projected.message, projected.data, - isWorkflowTool ? executionId : undefined + { + ...(isWorkflowTool && executionId ? { executionId } : {}), + ...(isPreclaimNativeTerminalOutcome + ? { completionGuard: { status: ASYNC_TOOL_STATUS.pending } as const } + : {}), + } ) - if (!updated) { + const reconciledOutcome = + updateOutcome === 'conflict' && isIndeterminateNativeExit && nativeClaimOwner + ? await updateToolCallStatus( + existing, + ASYNC_TOOL_CONFIRMATION_STATUS.error, + projected.message, + projected.data, + { + completionGuard: { + status: ASYNC_TOOL_STATUS.running, + claimedBy: nativeClaimOwner, + }, + } + ) + : updateOutcome + + if (reconciledOutcome === 'conflict' && isPreclaimNativeTerminalOutcome) { + span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) + return createNotFoundResponse('Pending client tool call not found') + } + + if (reconciledOutcome !== 'updated') { logger.error(`[${tracker.requestId}] Failed to update tool call status`, { userId: authenticatedUserId, toolCallId, diff --git a/apps/sim/app/api/desktop/tool/authorize/route.test.ts b/apps/sim/app/api/desktop/tool/authorize/route.test.ts index 8100abbd548..35464baf76f 100644 --- a/apps/sim/app/api/desktop/tool/authorize/route.test.ts +++ b/apps/sim/app/api/desktop/tool/authorize/route.test.ts @@ -82,6 +82,21 @@ describe('desktop tool authorization', () => { expect(claimPendingAsyncToolCall).toHaveBeenCalledWith('browser-tool', 'desktop-browser') }) + it('rejects retired browser tools retained only for history', async () => { + getAsyncToolCall.mockResolvedValueOnce({ + toolCallId: 'retired-browser-tool', + runId: 'run-1', + status: 'pending', + toolName: 'browser_request_takeover', + args: { reason: 'Legacy handoff' }, + }) + + const response = await POST(request('retired-browser-tool')) + + expect(response.status).toBe(403) + expect(claimPendingAsyncToolCall).not.toHaveBeenCalled() + }) + it('rejects a replayed browser action after its pending row was claimed', async () => { getAsyncToolCall.mockResolvedValueOnce({ toolCallId: 'browser-tool', diff --git a/apps/sim/app/api/desktop/tool/authorize/route.ts b/apps/sim/app/api/desktop/tool/authorize/route.ts index 27c8515e78c..f7503a9f792 100644 --- a/apps/sim/app/api/desktop/tool/authorize/route.ts +++ b/apps/sim/app/api/desktop/tool/authorize/route.ts @@ -1,9 +1,10 @@ -import { isBrowserToolName } from '@sim/browser-protocol' +import { isCurrentBrowserToolName } from '@sim/browser-protocol' import { isTerminalToolName } from '@sim/terminal-protocol' import { isRecordLike } from '@sim/utils/object' import { type NextRequest, NextResponse } from 'next/server' import { authorizeDesktopToolContract } from '@/lib/api/contracts/desktop-tool-authorization' import { parseRequest } from '@/lib/api/server' +import { DESKTOP_TOOL_CLAIM_OWNER } from '@/lib/copilot/async-runs/lifecycle' import { claimPendingAsyncToolCall, getAsyncToolCall, @@ -45,7 +46,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { } const args = isRecordLike(toolCall.args) ? (toolCall.args as Record) : {} - const isBrowserTool = isBrowserToolName(toolCall.toolName) + const isBrowserTool = isCurrentBrowserToolName(toolCall.toolName) const isTerminalTool = isTerminalToolName(toolCall.toolName) const authorized = isBrowserTool || isTerminalTool || isUserLocalVfsToolCall(toolCall.toolName, args) @@ -66,7 +67,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => { } const claimed = await claimPendingAsyncToolCall( toolCall.toolCallId, - isBrowserTool ? 'desktop-browser' : 'desktop-terminal' + isBrowserTool ? DESKTOP_TOOL_CLAIM_OWNER.browser : DESKTOP_TOOL_CLAIM_OWNER.terminal ) if (!claimed) { return createNotFoundResponse('Pending client tool call not found') diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.ts index 3635ac65d5c..5f0de25c6e0 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.ts @@ -1,4 +1,4 @@ -import { isBrowserToolName } from '@sim/browser-protocol' +import { isCurrentBrowserToolName } from '@sim/browser-protocol' import { isTerminalToolName } from '@sim/terminal-protocol' import { MothershipStreamV1ToolPhase, @@ -186,7 +186,7 @@ export function handleToolEvent(ctx: StreamLoopContext, parsed: ToolEvent): void deps.startClientLocalFilesystemTool(rawId, name, localFilesystemArgs ?? {}) } } - if (isBrowserToolName(name) && !isPartial) { + if (isCurrentBrowserToolName(name) && !isPartial) { const shouldStartBrowserTool = !deps.options.suppressedWorkflowToolStartIds?.has(rawId) && node?.kind === 'tool' && diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts index c5cdcc96d23..8a9a6399493 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts @@ -7,7 +7,7 @@ import { useRef, useState, } from 'react' -import { isBrowserToolName } from '@sim/browser-protocol' +import { isBrowserToolName, isCurrentBrowserToolName } from '@sim/browser-protocol' import { isPendingDesktopScopeId } from '@sim/desktop-bridge' import { createLogger } from '@sim/logger' import { isTerminalToolName } from '@sim/terminal-protocol' @@ -2137,7 +2137,7 @@ export function useChat( eventTs?: string, signal?: AbortSignal ) => { - if (!isBrowserToolName(toolName)) { + if (!isCurrentBrowserToolName(toolName)) { return } openBrowserResource() diff --git a/apps/sim/lib/browser-agent/transport.test.ts b/apps/sim/lib/browser-agent/transport.test.ts index 76f80023fed..bc3237dffe3 100644 --- a/apps/sim/lib/browser-agent/transport.test.ts +++ b/apps/sim/lib/browser-agent/transport.test.ts @@ -489,6 +489,40 @@ describe('browser panel transport', () => { } }) + it('cancels the exact native tool when the renderer response watchdog expires', async () => { + vi.useFakeTimers() + let settleNative: (response: { ok: boolean; error?: string }) => void = () => {} + try { + executeTool.mockImplementation( + () => + new Promise((resolve) => { + settleNative = resolve + }) + ) + const onCancel = vi.fn() + const execution = executeBrowserTool( + 'tool-timeout', + 'browser_snapshot', + {}, + 1_000, + 'chat-timeout', + onCancel + ) + const timedOut = expect(execution).rejects.toThrow( + 'The browser did not respond within 1000ms. Its outcome is unknown' + ) + + await vi.advanceTimersByTimeAsync(1_000) + + await timedOut + expect(cancelTool).toHaveBeenCalledWith('tool-timeout', 'chat-timeout') + expect(onCancel).not.toHaveBeenCalled() + } finally { + settleNative({ ok: false, error: 'cancelled' }) + vi.useRealTimers() + } + }) + it('starts the native scope boundary without waiting for exact cancellation', async () => { let settleNative: (response: { ok: boolean; error?: string }) => void = () => {} let settleExactCancellation: (cancelled: boolean) => void = () => {} diff --git a/apps/sim/lib/browser-agent/transport.ts b/apps/sim/lib/browser-agent/transport.ts index 39d78892cc8..bf6a64a776e 100644 --- a/apps/sim/lib/browser-agent/transport.ts +++ b/apps/sim/lib/browser-agent/transport.ts @@ -36,9 +36,17 @@ import type { SimDesktopBrowserAgentApi, } from '@sim/desktop-bridge' import { isPendingDesktopScopeId } from '@sim/desktop-bridge' +import { createLogger } from '@sim/logger' +import { toError } from '@sim/utils/errors' import { getDesktopBridge, isBrowserAgentEnabled } from '@/lib/desktop' import { useBrowserSessionStore } from '@/stores/browser-session/store' +const logger = createLogger('BrowserAgentTransport') + +class BrowserOutcomeUnknownError extends Error { + readonly outcomeUnknown = true +} + let initialized = false let activeScopeId: string | null = null /** Last VISIBLE rect per scope; a hidden/unmounted panel has no entry. */ @@ -195,10 +203,45 @@ export async function executeBrowserTool( : await Promise.race([ invocation, new Promise((_, reject) => { - timeoutId = setTimeout( - () => reject(new Error(`The browser did not respond within ${timeoutMs}ms`)), - timeoutMs - ) + timeoutId = setTimeout(() => { + try { + const cancellation = agent.cancelTool?.(toolCallId, activeTool.scopeId) + if (cancellation) { + void cancellation + .then((cancelled) => { + if (!cancelled) { + logger.warn('Native browser timeout cancellation was not accepted', { + toolCallId, + tool, + }) + } + }) + .catch((error) => { + logger.warn('Native browser timeout cancellation failed', { + toolCallId, + tool, + error: toError(error).message, + }) + }) + } else { + logger.warn('Installed desktop shell cannot cancel a timed-out browser tool', { + toolCallId, + tool, + }) + } + } catch (error) { + logger.warn('Native browser timeout cancellation threw synchronously', { + toolCallId, + tool, + error: toError(error).message, + }) + } + reject( + new BrowserOutcomeUnknownError( + `The browser did not respond within ${timeoutMs}ms. Its outcome is unknown and the action may already have taken effect. Do not retry it automatically; take a fresh browser snapshot before deciding what to do.` + ) + ) + }, timeoutMs) }), ]) if (!response.ok) { diff --git a/apps/sim/lib/copilot/async-runs/lifecycle.ts b/apps/sim/lib/copilot/async-runs/lifecycle.ts index d86ae06442a..4f77e1d173a 100644 --- a/apps/sim/lib/copilot/async-runs/lifecycle.ts +++ b/apps/sim/lib/copilot/async-runs/lifecycle.ts @@ -12,6 +12,11 @@ export const EXECUTABLE_TOOL_PERMISSION_DECISIONS = [ 'always_allow', ] as const satisfies readonly CopilotToolPermissionDecision[] +export const DESKTOP_TOOL_CLAIM_OWNER = { + browser: 'desktop-browser', + terminal: 'desktop-terminal', +} as const + export type AsyncLifecycleStatus = | typeof ASYNC_TOOL_STATUS.pending | typeof ASYNC_TOOL_STATUS.running diff --git a/apps/sim/lib/copilot/async-runs/repository.test.ts b/apps/sim/lib/copilot/async-runs/repository.test.ts index 81bdd061986..c150413ad14 100644 --- a/apps/sim/lib/copilot/async-runs/repository.test.ts +++ b/apps/sim/lib/copilot/async-runs/repository.test.ts @@ -2,13 +2,15 @@ * @vitest-environment node */ -import { dbChainMockFns, resetDbChainMock } from '@sim/testing' +import { dbChainMockFns, hasMockCondition, resetDbChainMock } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' import { claimCompletedAsyncToolCall, claimPendingAsyncToolCall, claimWorkflowToolExecution, completeAsyncToolCall, + completeClaimedAsyncToolCall, + completePendingAsyncToolCall, detachAsyncToolCall, getClaimedWorkflowExecutionId, markAsyncToolRunning, @@ -65,6 +67,110 @@ describe('async tool repository single-row semantics', () => { expect(dbChainMockFns.limit).not.toHaveBeenCalled() }) + it('atomically completes a native preclaim failure only while the row is pending', async () => { + const failedRow = { + toolCallId: 'browser-tool', + status: 'failed', + result: { error: 'Desktop action did not start' }, + error: 'Desktop action did not start', + } + dbChainMockFns.returning.mockResolvedValueOnce([failedRow]) + + const result = await completePendingAsyncToolCall({ + toolCallId: 'browser-tool', + status: 'failed', + result: { error: 'Desktop action did not start' }, + error: 'Desktop action did not start', + }) + + expect(result).toEqual(failedRow) + expect(dbChainMockFns.set).toHaveBeenCalledWith( + expect.objectContaining({ + status: 'failed', + claimedBy: null, + claimedAt: null, + completedAt: expect.any(Date), + }) + ) + const where = dbChainMockFns.where.mock.calls[0]?.[0] + expect( + hasMockCondition( + where, + (condition) => + condition.type === 'inArray' && + Array.isArray(condition.values) && + condition.values.length === 1 && + condition.values[0] === 'pending' + ) + ).toBe(true) + }) + + it('returns null when a native authorization claim wins the pending completion race', async () => { + dbChainMockFns.returning.mockResolvedValueOnce([]) + + await expect( + completePendingAsyncToolCall({ + toolCallId: 'browser-tool', + status: 'cancelled', + result: { cancelled: true }, + error: 'Tool cancelled', + }) + ).resolves.toBeNull() + }) + + it('atomically completes only the exact running native claim', async () => { + const failedRow = { + toolCallId: 'browser-tool', + status: 'failed', + claimedBy: null, + } + dbChainMockFns.returning.mockResolvedValueOnce([failedRow]) + + const result = await completeClaimedAsyncToolCall( + { + toolCallId: 'browser-tool', + status: 'failed', + result: { outcomeUnknown: true, doNotRetry: true }, + error: 'Native outcome unknown', + }, + 'desktop-browser' + ) + + expect(result).toEqual(failedRow) + const where = dbChainMockFns.where.mock.calls[0]?.[0] + expect( + hasMockCondition( + where, + (condition) => + condition.type === 'inArray' && + Array.isArray(condition.values) && + condition.values.length === 1 && + condition.values[0] === 'running' + ) + ).toBe(true) + expect( + hasMockCondition( + where, + (condition) => condition.type === 'eq' && condition.right === 'desktop-browser' + ) + ).toBe(true) + }) + + it('returns null when the exact native claim is no longer running', async () => { + dbChainMockFns.returning.mockResolvedValueOnce([]) + + await expect( + completeClaimedAsyncToolCall( + { + toolCallId: 'browser-tool', + status: 'failed', + error: 'Native outcome unknown', + }, + 'desktop-browser' + ) + ).resolves.toBeNull() + }) + it('atomically detaches a live background call and clears the claim fields', async () => { dbChainMockFns.returning.mockResolvedValueOnce([ { diff --git a/apps/sim/lib/copilot/async-runs/repository.ts b/apps/sim/lib/copilot/async-runs/repository.ts index 58efd23875b..e22c92651d2 100644 --- a/apps/sim/lib/copilot/async-runs/repository.ts +++ b/apps/sim/lib/copilot/async-runs/repository.ts @@ -280,7 +280,8 @@ async function markAsyncToolStatus( error?: string | null completedAt?: Date | null } = {}, - expectedStatuses?: CopilotAsyncToolStatus[] + expectedStatuses?: CopilotAsyncToolStatus[], + expectedClaimedBy?: string ) { return await withDbSpan( TraceSpan.CopilotAsyncRunsMarkAsyncToolStatus, @@ -290,7 +291,7 @@ async function markAsyncToolStatus( [TraceAttr.ToolCallId]: toolCallId, [TraceAttr.CopilotAsyncToolStatus]: status, [TraceAttr.CopilotAsyncToolHasError]: !!updates.error, - [TraceAttr.CopilotAsyncToolClaimedBy]: updates.claimedBy ?? undefined, + [TraceAttr.CopilotAsyncToolClaimedBy]: expectedClaimedBy ?? updates.claimedBy ?? undefined, }, async () => { const claimedAt = @@ -314,12 +315,11 @@ async function markAsyncToolStatus( updatedAt: new Date(), }) .where( - expectedStatuses - ? and( - eq(copilotAsyncToolCalls.toolCallId, toolCallId), - inArray(copilotAsyncToolCalls.status, expectedStatuses) - ) - : eq(copilotAsyncToolCalls.toolCallId, toolCallId) + and( + eq(copilotAsyncToolCalls.toolCallId, toolCallId), + expectedStatuses ? inArray(copilotAsyncToolCalls.status, expectedStatuses) : undefined, + expectedClaimedBy ? eq(copilotAsyncToolCalls.claimedBy, expectedClaimedBy) : undefined + ) ) .returning() @@ -453,13 +453,19 @@ export async function claimPendingAsyncToolCall(toolCallId: string, claimedBy: s ) } -export async function completeAsyncToolCall(input: { +interface CompleteAsyncToolCallInput { toolCallId: string status: Extract result?: AsyncCompletionData | null error?: string | null -}) { - return markAsyncToolStatus( +} + +async function completeAsyncToolCallFromStatuses( + input: CompleteAsyncToolCallInput, + expectedStatuses: CopilotAsyncToolStatus[], + expectedClaimedBy?: string +) { + return await markAsyncToolStatus( input.toolCallId, input.status, { @@ -469,10 +475,35 @@ export async function completeAsyncToolCall(input: { error: input.error ?? null, completedAt: new Date(), }, - [ASYNC_TOOL_STATUS.pending, ASYNC_TOOL_STATUS.running] + expectedStatuses, + expectedClaimedBy ) } +export async function completeAsyncToolCall(input: CompleteAsyncToolCallInput) { + return await completeAsyncToolCallFromStatuses(input, [ + ASYNC_TOOL_STATUS.pending, + ASYNC_TOOL_STATUS.running, + ]) +} + +/** + * Finalizes a client tool only while it remains unclaimed. This is the inverse + * CAS of `claimPendingAsyncToolCall`: exactly one of a renderer-side preclaim + * failure or the native authorization claim may transition the pending row. + */ +export async function completePendingAsyncToolCall(input: CompleteAsyncToolCallInput) { + return await completeAsyncToolCallFromStatuses(input, [ASYNC_TOOL_STATUS.pending]) +} + +/** Finalizes only the exact native claim that won a pending completion race. */ +export async function completeClaimedAsyncToolCall( + input: CompleteAsyncToolCallInput, + claimedBy: string +) { + return await completeAsyncToolCallFromStatuses(input, [ASYNC_TOOL_STATUS.running], claimedBy) +} + /** * Atomically detaches a live client tool after the browser reports that it is * continuing in the background. Whichever terminal or detach transition wins diff --git a/apps/sim/lib/copilot/request/handlers/handlers.test.ts b/apps/sim/lib/copilot/request/handlers/handlers.test.ts index aa4ef86a6a6..87321713554 100644 --- a/apps/sim/lib/copilot/request/handlers/handlers.test.ts +++ b/apps/sim/lib/copilot/request/handlers/handlers.test.ts @@ -939,7 +939,7 @@ describe('sse-handlers tool lifecycle', () => { expect(executeTool).not.toHaveBeenCalled() }) - it('waits for a browser takeover without a client-tool deadline', async () => { + it('bounds a retired browser takeover that can no longer execute in the client', async () => { isSimExecuted.mockReturnValue(false) waitForClientToolCompletion.mockResolvedValueOnce({ status: 'success', @@ -970,7 +970,7 @@ describe('sse-handlers tool lifecycle', () => { toolCallId: 'tool-browser-takeover', runId: context.runId, userId: 'user-1', - timeoutMs: null, + timeoutMs: 1000, abortSignal: undefined, registry: execContext.resolvedSecretTraceRegistry, }) diff --git a/apps/sim/lib/copilot/request/handlers/tool.ts b/apps/sim/lib/copilot/request/handlers/tool.ts index 371fa28894f..1a21880d70a 100644 --- a/apps/sim/lib/copilot/request/handlers/tool.ts +++ b/apps/sim/lib/copilot/request/handlers/tool.ts @@ -1,4 +1,4 @@ -import { isBrowserToolName } from '@sim/browser-protocol' +import { isCurrentBrowserToolName } from '@sim/browser-protocol' import { createLogger } from '@sim/logger' import { isTerminalToolName } from '@sim/terminal-protocol' import { getErrorMessage, toError } from '@sim/utils/errors' @@ -45,7 +45,6 @@ import type { import { getToolEntry, isSimExecuted } from '@/lib/copilot/tool-executor' import { isToolHiddenInUi } from '@/lib/copilot/tools/client/hidden-tools' import { isUserLocalVfsToolCall } from '@/lib/copilot/tools/local-filesystem' -import { RETIRED_BROWSER_REQUEST_TAKEOVER_ID } from '@/lib/copilot/tools/retired-tools' import { extractStreamingStringArgument } from '@/lib/copilot/tools/streaming-args' import { getToolDisplayTitle } from '@/lib/copilot/tools/tool-display' import { isWorkflowToolName, resolveWorkflowToolTargetId } from '@/lib/copilot/tools/workflow-tools' @@ -274,7 +273,7 @@ export async function prePersistClientExecutableToolCall( // client tools retain the established "already dispatched" running state. // A gated tool is likewise pending: nothing has been dispatched yet. status: - gated || isBrowserToolName(data.toolName) || isTerminalToolName(data.toolName) + gated || isCurrentBrowserToolName(data.toolName) || isTerminalToolName(data.toolName) ? MothershipStreamV1AsyncToolRecordStatus.pending : MothershipStreamV1AsyncToolRecordStatus.running, }).catch((err) => { @@ -807,14 +806,13 @@ async function dispatchToolExecution( */ function waitForClientExecution(): Promise { toolCall.status = 'executing' - const waitsForHuman = toolName === RETIRED_BROWSER_REQUEST_TAKEOVER_ID - const timeoutMs = waitsForHuman ? null : options.timeout || STREAM_TIMEOUT_MS + const timeoutMs = options.timeout || STREAM_TIMEOUT_MS return withCopilotSpan( TraceSpan.CopilotToolWaitForClientResult, { [TraceAttr.ToolName]: toolName, [TraceAttr.ToolCallId]: toolCallId, - ...(timeoutMs !== null ? { [TraceAttr.ToolTimeoutMs]: timeoutMs } : {}), + [TraceAttr.ToolTimeoutMs]: timeoutMs, ...(context.runId ? { [TraceAttr.RunId]: context.runId } : {}), }, async (span) => { @@ -823,7 +821,7 @@ async function dispatchToolExecution( const race = await raceWorkflowToolClientPickup({ toolCallId, workflowId: resolveWorkflowToolTargetId(args, execContext.workflowId), - timeoutMs: timeoutMs ?? STREAM_TIMEOUT_MS, + timeoutMs, graceMs: COPILOT_WORKFLOW_TOOL_CLIENT_GRACE_MS, abortSignal: options.abortSignal, registry: execContext.resolvedSecretTraceRegistry, diff --git a/apps/sim/lib/copilot/request/lifecycle/run.test.ts b/apps/sim/lib/copilot/request/lifecycle/run.test.ts index 5eacd781290..891fb1b00bb 100644 --- a/apps/sim/lib/copilot/request/lifecycle/run.test.ts +++ b/apps/sim/lib/copilot/request/lifecycle/run.test.ts @@ -30,7 +30,7 @@ const { mockPrepareCopilotEnvironmentContext: vi.fn(), mockPrepareExecutionContext: vi.fn(), mockRunStreamLoop: vi.fn(), - mockPendingToolWaitBudgetMs: vi.fn((_toolCall?: { name?: string }) => 60_000 as number | null), + mockPendingToolWaitBudgetMs: vi.fn((_toolCall?: { name?: string; status?: string }) => 60_000), mockGetAutoAllowedTools: vi.fn(async () => new Set()), mockFilterModelSafeWorkspaceFileAttachments: vi.fn(async (attachments: unknown[]) => attachments), mockUpdateRunStatus: vi.fn(), @@ -1966,12 +1966,11 @@ describe('runCopilotLifecycle', () => { expect(result.errors).toEqual(['The provider is overloaded']) }) - it('keeps a human wait durable while force-failing a hung parallel tool', async () => { + it('force-fails a hung tool promise and resumes with an error result instead of wedging', async () => { vi.useFakeTimers() try { - let releaseTakeover = () => {} - let lifecycleSettled = false const fetchUrls: string[] = [] + const bodies: Record[] = [] const executionContext: ExecutionContext = { userId: 'user-1', workflowId: '', @@ -1979,9 +1978,8 @@ describe('runCopilotLifecycle', () => { chatId: 'chat-1', } - mockPendingToolWaitBudgetMs.mockImplementation((toolCall) => - toolCall?.name === 'browser_request_takeover' ? null : 60_000 - ) + // Mirror the real helper: settle the tool call into a terminal error + // state so the resume loop can serialize an error result for it. mockForceFailHungToolCall.mockImplementation( async (toolCallId: string, context: StreamingContext, message: string) => { const tool = context.toolCalls.get(toolCallId) @@ -1993,33 +1991,196 @@ describe('runCopilotLifecycle', () => { } ) + // Initial leg checkpoints on an async tool whose promise NEVER settles — + // the exact shape of the prod incident (claimed, marked running, hung). mockRunStreamLoop.mockImplementationOnce( async ( fetchUrl: string, - _fetchOptions: RequestInit, + fetchOptions: RequestInit, context: StreamingContext ): Promise => { fetchUrls.push(fetchUrl) - const takeoverId = 'tool-takeover' - context.toolCalls.set(takeoverId, { - id: takeoverId, - name: 'browser_request_takeover', + bodies.push(JSON.parse(String(fetchOptions.body))) + context.toolCalls.set('tool-hung', { + id: 'tool-hung', + name: 'read', status: 'executing', }) - const takeover = new Promise<{ status: 'success' }>((resolve) => { - releaseTakeover = () => { - const tool = context.toolCalls.get(takeoverId) - if (tool) { - tool.status = MothershipStreamV1ToolOutcome.success - tool.endTime = Date.now() - tool.result = { success: true, output: { completed: true } } - } - context.pendingToolPromises.delete(takeoverId) - resolve({ status: 'success' }) - } + context.pendingToolPromises.set('tool-hung', new Promise(() => {})) + context.awaitingAsyncContinuation = { + checkpointId: 'ckpt-1', + pendingToolCallIds: ['tool-hung'], + } + } + ) + + // Resume leg completes normally with the error result delivered. + mockRunStreamLoop.mockImplementationOnce( + async ( + fetchUrl: string, + fetchOptions: RequestInit, + context: StreamingContext + ): Promise => { + fetchUrls.push(fetchUrl) + bodies.push(JSON.parse(String(fetchOptions.body))) + context.accumulatedContent = 'The file read failed, but here is what I know.' + } + ) + + const lifecycle = runCopilotLifecycle( + { message: 'hello', messageId: 'stream-1' }, + { + userId: 'user-1', + workspaceId: 'ws-1', + chatId: 'chat-1', + executionId: 'exec-1', + runId: 'run-1', + executionContext, + } + ) + + // Wait budget = watchdog (60s, mocked) + resume grace (30s). Advance past it. + await vi.advanceTimersByTimeAsync(91_000) + const result = await lifecycle + + expect(mockForceFailHungToolCall).toHaveBeenCalledWith( + 'tool-hung', + expect.anything(), + expect.stringContaining('hung') + ) + expect(fetchUrls[1]).toBe('http://mothership.test/api/tools/resume') + expect(bodies[1].results).toEqual([ + expect.objectContaining({ + callId: 'tool-hung', + name: 'read', + success: false, + data: { error: expect.stringContaining('hung') }, + }), + ]) + expect(result.success).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it('cancels promptly while a sequential tool promise remains unsettled', async () => { + vi.useFakeTimers() + try { + const controller = new AbortController() + const fetchUrls: string[] = [] + let capturedContext: StreamingContext | null = null + const executionContext: ExecutionContext = { + userId: 'user-1', + workflowId: '', + workspaceId: 'ws-1', + chatId: 'chat-1', + } + + mockPendingToolWaitBudgetMs.mockReturnValue(3_600_000) + mockRunStreamLoop.mockImplementationOnce( + async (fetchUrl: string, _fetchOptions: RequestInit, context: StreamingContext) => { + fetchUrls.push(fetchUrl) + capturedContext = context + context.toolCalls.set('tool-hung', { + id: 'tool-hung', + name: 'terminal', + status: 'awaiting_approval', }) - context.pendingToolPromises.set(takeoverId, takeover) + context.pendingToolPromises.set('tool-hung', new Promise(() => {})) + context.awaitingAsyncContinuation = { + checkpointId: 'ckpt-1', + pendingToolCallIds: ['tool-hung'], + } + } + ) + + const lifecycle = runCopilotLifecycle( + { message: 'hello', messageId: 'stream-aborted-tool-wait' }, + { + userId: 'user-1', + workspaceId: 'ws-1', + chatId: 'chat-1', + executionId: 'exec-1', + runId: 'run-1', + executionContext, + abortSignal: controller.signal, + } + ) + + await vi.advanceTimersByTimeAsync(0) + expect(mockPendingToolWaitBudgetMs).toHaveBeenCalled() + controller.abort('user_stop') + await vi.advanceTimersByTimeAsync(0) + const result = await lifecycle + expect(result.success).toBe(false) + expect(result.cancelled).toBe(true) + expect(fetchUrls).toEqual(['http://mothership.test/api/copilot']) + expect(mockForceFailHungToolCall).not.toHaveBeenCalled() + expect(capturedContext?.toolCalls.get('tool-hung')).toMatchObject({ + status: MothershipStreamV1ToolOutcome.cancelled, + error: 'Stopped by user', + }) + + await vi.advanceTimersByTimeAsync(3_700_000) + expect(mockForceFailHungToolCall).not.toHaveBeenCalled() + expect(fetchUrls).toEqual(['http://mothership.test/api/copilot']) + } finally { + vi.useRealTimers() + } + }) + + it('force-fails each hung tool on its own budget while awaiting a long approval', async () => { + vi.useFakeTimers() + try { + let releaseApproval = () => {} + let lifecycleSettled = false + const fetchUrls: string[] = [] + const executionContext: ExecutionContext = { + userId: 'user-1', + workflowId: '', + workspaceId: 'ws-1', + chatId: 'chat-1', + } + + mockPendingToolWaitBudgetMs.mockImplementation((toolCall) => + toolCall?.status === 'awaiting_approval' ? 3_600_000 : 60_000 + ) + mockForceFailHungToolCall.mockImplementation( + async (toolCallId: string, context: StreamingContext, message: string) => { + const tool = context.toolCalls.get(toolCallId) + if (!tool) return + tool.status = MothershipStreamV1ToolOutcome.error + tool.endTime = Date.now() + tool.result = { success: false } + tool.error = message + } + ) + + mockRunStreamLoop.mockImplementationOnce( + async (fetchUrl: string, _fetchOptions: RequestInit, context: StreamingContext) => { + fetchUrls.push(fetchUrl) + const approvalId = 'tool-approval' + context.toolCalls.set(approvalId, { + id: approvalId, + name: 'terminal', + status: 'awaiting_approval', + }) + context.pendingToolPromises.set( + approvalId, + new Promise<{ status: 'success' }>((resolve) => { + releaseApproval = () => { + const tool = context.toolCalls.get(approvalId) + if (tool) { + tool.status = MothershipStreamV1ToolOutcome.success + tool.endTime = Date.now() + tool.result = { success: true, output: { approved: true } } + } + context.pendingToolPromises.delete(approvalId) + resolve({ status: 'success' }) + } + }) + ) context.toolCalls.set('tool-hung', { id: 'tool-hung', name: 'read', @@ -2028,14 +2189,14 @@ describe('runCopilotLifecycle', () => { context.pendingToolPromises.set('tool-hung', new Promise(() => {})) context.awaitingAsyncContinuation = { checkpointId: 'ckpt-1', - pendingToolCallIds: [takeoverId, 'tool-hung'], + pendingToolCallIds: [approvalId, 'tool-hung'], } } ) mockRunStreamLoop.mockImplementationOnce( async (fetchUrl: string, _fetchOptions: RequestInit, context: StreamingContext) => { fetchUrls.push(fetchUrl) - context.accumulatedContent = 'Continued after browser control resumed.' + context.accumulatedContent = 'Continued after approval.' } ) @@ -2063,7 +2224,7 @@ describe('runCopilotLifecycle', () => { expect(lifecycleSettled).toBe(false) expect(fetchUrls).toEqual(['http://mothership.test/api/copilot']) - releaseTakeover() + releaseApproval() await vi.advanceTimersByTimeAsync(0) const result = await lifecycle @@ -2074,11 +2235,13 @@ describe('runCopilotLifecycle', () => { } }) - it('force-fails a hung tool promise and resumes with an error result instead of wedging', async () => { + it('does not let a stale watchdog fail or delete a replacement promise', async () => { vi.useFakeTimers() try { + let capturedContext: StreamingContext | null = null + let releaseReplacement = () => {} + let lifecycleSettled = false const fetchUrls: string[] = [] - const bodies: Record[] = [] const executionContext: ExecutionContext = { userId: 'user-1', workflowId: '', @@ -2086,8 +2249,95 @@ describe('runCopilotLifecycle', () => { chatId: 'chat-1', } - // Mirror the real helper: settle the tool call into a terminal error - // state so the resume loop can serialize an error result for it. + mockRunStreamLoop.mockImplementationOnce( + async (fetchUrl: string, _fetchOptions: RequestInit, context: StreamingContext) => { + fetchUrls.push(fetchUrl) + capturedContext = context + context.toolCalls.set('tool-replaced', { + id: 'tool-replaced', + name: 'read', + status: 'executing', + }) + context.pendingToolPromises.set('tool-replaced', new Promise(() => {})) + context.awaitingAsyncContinuation = { + checkpointId: 'ckpt-1', + pendingToolCallIds: ['tool-replaced'], + } + } + ) + mockRunStreamLoop.mockImplementationOnce( + async (fetchUrl: string, _fetchOptions: RequestInit, context: StreamingContext) => { + fetchUrls.push(fetchUrl) + context.accumulatedContent = 'Continued after the replacement completed.' + } + ) + + const lifecycle = runCopilotLifecycle( + { message: 'hello', messageId: 'stream-1' }, + { + userId: 'user-1', + workspaceId: 'ws-1', + chatId: 'chat-1', + executionId: 'exec-1', + runId: 'run-1', + executionContext, + } + ).finally(() => { + lifecycleSettled = true + }) + + await vi.advanceTimersByTimeAsync(0) + if (!capturedContext) throw new Error('Initial stream did not establish its context') + const context: StreamingContext = capturedContext + const replacement = new Promise<{ status: 'success' }>((resolve) => { + releaseReplacement = () => { + const tool = context.toolCalls.get('tool-replaced') + if (tool) { + tool.status = MothershipStreamV1ToolOutcome.success + tool.endTime = Date.now() + tool.result = { success: true, output: { replacement: true } } + } + context.pendingToolPromises.delete('tool-replaced') + resolve({ status: 'success' }) + } + }) + context.pendingToolPromises.set('tool-replaced', replacement) + + await vi.advanceTimersByTimeAsync(91_000) + expect(mockForceFailHungToolCall).not.toHaveBeenCalled() + expect(context.pendingToolPromises.get('tool-replaced')).toBe(replacement) + expect(lifecycleSettled).toBe(false) + expect(fetchUrls).toEqual(['http://mothership.test/api/copilot']) + + releaseReplacement() + await vi.advanceTimersByTimeAsync(0) + const result = await lifecycle + + expect(mockForceFailHungToolCall).not.toHaveBeenCalled() + expect(fetchUrls[1]).toBe('http://mothership.test/api/tools/resume') + expect(result.success).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it('bounds a replaced short promise without waiting for a parallel long approval', async () => { + vi.useFakeTimers() + try { + let capturedContext: StreamingContext | null = null + let releaseApproval = () => {} + let lifecycleSettled = false + const fetchUrls: string[] = [] + const executionContext: ExecutionContext = { + userId: 'user-1', + workflowId: '', + workspaceId: 'ws-1', + chatId: 'chat-1', + } + + mockPendingToolWaitBudgetMs.mockImplementation((toolCall) => + toolCall?.status === 'awaiting_approval' ? 3_600_000 : 60_000 + ) mockForceFailHungToolCall.mockImplementation( async (toolCallId: string, context: StreamingContext, message: string) => { const tool = context.toolCalls.get(toolCallId) @@ -2099,39 +2349,46 @@ describe('runCopilotLifecycle', () => { } ) - // Initial leg checkpoints on an async tool whose promise NEVER settles — - // the exact shape of the prod incident (claimed, marked running, hung). mockRunStreamLoop.mockImplementationOnce( - async ( - fetchUrl: string, - fetchOptions: RequestInit, - context: StreamingContext - ): Promise => { + async (fetchUrl: string, _fetchOptions: RequestInit, context: StreamingContext) => { fetchUrls.push(fetchUrl) - bodies.push(JSON.parse(String(fetchOptions.body))) - context.toolCalls.set('tool-hung', { - id: 'tool-hung', + capturedContext = context + context.toolCalls.set('tool-approval', { + id: 'tool-approval', + name: 'terminal', + status: 'awaiting_approval', + }) + context.pendingToolPromises.set( + 'tool-approval', + new Promise<{ status: 'success' }>((resolve) => { + releaseApproval = () => { + const tool = context.toolCalls.get('tool-approval') + if (tool) { + tool.status = MothershipStreamV1ToolOutcome.success + tool.endTime = Date.now() + tool.result = { success: true, output: { approved: true } } + } + context.pendingToolPromises.delete('tool-approval') + resolve({ status: 'success' }) + } + }) + ) + context.toolCalls.set('tool-replaced', { + id: 'tool-replaced', name: 'read', status: 'executing', }) - context.pendingToolPromises.set('tool-hung', new Promise(() => {})) + context.pendingToolPromises.set('tool-replaced', new Promise(() => {})) context.awaitingAsyncContinuation = { checkpointId: 'ckpt-1', - pendingToolCallIds: ['tool-hung'], + pendingToolCallIds: ['tool-approval', 'tool-replaced'], } } ) - - // Resume leg completes normally with the error result delivered. mockRunStreamLoop.mockImplementationOnce( - async ( - fetchUrl: string, - fetchOptions: RequestInit, - context: StreamingContext - ): Promise => { + async (fetchUrl: string, _fetchOptions: RequestInit, context: StreamingContext) => { fetchUrls.push(fetchUrl) - bodies.push(JSON.parse(String(fetchOptions.body))) - context.accumulatedContent = 'The file read failed, but here is what I know.' + context.accumulatedContent = 'Continued after approval.' } ) @@ -2145,31 +2402,40 @@ describe('runCopilotLifecycle', () => { runId: 'run-1', executionContext, } - ) + ).finally(() => { + lifecycleSettled = true + }) + + await vi.advanceTimersByTimeAsync(0) + if (!capturedContext) throw new Error('Initial stream did not establish its context') + const context: StreamingContext = capturedContext + context.pendingToolPromises.set('tool-replaced', new Promise(() => {})) - // Wait budget = watchdog (60s, mocked) + resume grace (30s). Advance past it. await vi.advanceTimersByTimeAsync(91_000) - const result = await lifecycle + expect(mockForceFailHungToolCall).not.toHaveBeenCalled() + expect(lifecycleSettled).toBe(false) + await vi.advanceTimersByTimeAsync(90_000) + expect(mockForceFailHungToolCall).toHaveBeenCalledTimes(1) expect(mockForceFailHungToolCall).toHaveBeenCalledWith( - 'tool-hung', + 'tool-replaced', expect.anything(), expect.stringContaining('hung') ) + expect(lifecycleSettled).toBe(false) + expect(fetchUrls).toEqual(['http://mothership.test/api/copilot']) + + releaseApproval() + await vi.advanceTimersByTimeAsync(0) + const result = await lifecycle + expect(fetchUrls[1]).toBe('http://mothership.test/api/tools/resume') - expect(bodies[1].results).toEqual([ - expect.objectContaining({ - callId: 'tool-hung', - name: 'read', - success: false, - data: { error: expect.stringContaining('hung') }, - }), - ]) expect(result.success).toBe(true) } finally { vi.useRealTimers() } }) + it('completes the turn when a pending subagent tool has no result', async () => { // A tool that never reached a terminal state used to throw // "Cannot resume subagent chain ...: missing result for tool call ...", @@ -2221,6 +2487,57 @@ describe('runCopilotLifecycle', () => { expect(result.success).toBe(true) }) + it('cancels promptly while a per-subagent tool promise remains unsettled', async () => { + const controller = new AbortController() + const addAbortListener = vi.spyOn(controller.signal, 'addEventListener') + const fetchUrls: string[] = [] + let capturedContext: StreamingContext | null = null + mockRunStreamLoop.mockImplementationOnce( + async (fetchUrl: string, _fetchOptions: RequestInit, context: StreamingContext) => { + fetchUrls.push(fetchUrl) + capturedContext = context + context.toolCalls.set('tool-hung', { + id: 'tool-hung', + name: 'read', + status: 'executing', + }) + context.pendingToolPromises.set('tool-hung', new Promise(() => {})) + context.awaitingAsyncContinuation = { + checkpointId: 'cp-root', + pendingToolCallIds: ['tool-hung'], + frames: [ + { + parentToolCallId: 'subagent-file', + parentToolName: 'file', + pendingToolIds: ['tool-hung'], + checkpointId: 'cp-file', + }, + ], + } + } + ) + + const lifecycle = runCopilotLifecycle( + { message: 'hello', messageId: 'stream-aborted-subagent-wait' }, + { userId: 'user-1', workspaceId: 'ws-1', abortSignal: controller.signal } + ) + + await vi.waitFor(() => { + expect(addAbortListener).toHaveBeenCalledWith('abort', expect.any(Function), { once: true }) + }) + controller.abort('user_stop') + const result = await lifecycle + + expect(result.success).toBe(false) + expect(result.cancelled).toBe(true) + expect(fetchUrls).toEqual(['http://mothership.test/api/copilot']) + expect(mockForceFailHungToolCall).not.toHaveBeenCalled() + expect(capturedContext?.toolCalls.get('tool-hung')).toMatchObject({ + status: MothershipStreamV1ToolOutcome.cancelled, + error: 'Stopped by user', + }) + }) + it('classifies a Stop landing during a subagent fanout as cancelled', async () => { // Guards the trap in the fanout fix: `wasAborted` is now isolated per leg, so // a user Stop must still reach the turn — via the abort signal or the folded diff --git a/apps/sim/lib/copilot/request/lifecycle/run.ts b/apps/sim/lib/copilot/request/lifecycle/run.ts index 7c8a1e0e103..7add7419cd2 100644 --- a/apps/sim/lib/copilot/request/lifecycle/run.ts +++ b/apps/sim/lib/copilot/request/lifecycle/run.ts @@ -2,7 +2,7 @@ import type { Context } from '@opentelemetry/api' import { createLogger } from '@sim/logger' import type { PermissionType } from '@sim/platform-authz/workspace' import { toError } from '@sim/utils/errors' -import { sleep } from '@sim/utils/helpers' +import { interruptibleSleep, sleep } from '@sim/utils/helpers' import { generateId } from '@sim/utils/id' import { omit } from '@sim/utils/object' import { @@ -12,6 +12,7 @@ import { createAttributedBillingRequestEnvelope, } from '@/lib/billing/core/billing-attribution' import { isWorkspaceOnEnterprisePlan } from '@/lib/billing/core/subscription' +import type { AsyncCompletionSignal } from '@/lib/copilot/async-runs/lifecycle' import { createRunSegment, updateRunStatus } from '@/lib/copilot/async-runs/repository' import { SIM_AGENT_VERSION, TOOL_WATCHDOG_RESUME_GRACE_MS } from '@/lib/copilot/constants' import { @@ -537,13 +538,34 @@ export function mergeResumeLegOutputs( if (leg.completionStatus) context.completionStatus = leg.completionStatus } -async function waitForToolIds(context: StreamingContext, toolIds: string[]): Promise { +async function waitForToolIds( + context: StreamingContext, + toolIds: string[], + abortSignal?: AbortSignal +): Promise { const promises: Promise[] = [] for (const id of toolIds) { const p = context.pendingToolPromises.get(id) if (p) promises.push(p) } - if (promises.length > 0) await Promise.allSettled(promises) + if (promises.length === 0) return true + if (!abortSignal) { + await Promise.allSettled(promises) + return true + } + if (abortSignal.aborted) return false + + let onAbort = () => {} + const aborted = new Promise((resolve) => { + onAbort = () => resolve(false) + abortSignal.addEventListener('abort', onAbort, { once: true }) + if (abortSignal.aborted) onAbort() + }) + try { + return await Promise.race([Promise.allSettled(promises).then(() => true as const), aborted]) + } finally { + abortSignal.removeEventListener('abort', onAbort) + } } interface ResumeToolResult { @@ -688,7 +710,8 @@ async function driveOneChildChain( for (;;) { if (isAborted(options, context)) return null - await waitForToolIds(context, toolIds) + const toolsSettled = await waitForToolIds(context, toolIds, options.abortSignal) + if (!toolsSettled || isAborted(options, context)) return null const results = collectResultsForToolIds(context, toolIds, checkpointId) const leg = makeResumeLegContext(context) @@ -1006,7 +1029,15 @@ async function runCheckpointLoop( next = null break } - await waitForToolIds(context, next.pendingToolCallIds) + const toolsSettled = await waitForToolIds( + context, + next.pendingToolCallIds, + options.abortSignal + ) + if (!toolsSettled || isAborted(options, context)) { + next = null + break + } next = await driveSubagentChains( next, context, @@ -1017,71 +1048,138 @@ async function runCheckpointLoop( hostedBillingRequest ) } - if (!next) break + if (!next) { + if (isAborted(options, context)) cancelPendingTools(context) + break + } continuation = next } if (context.pendingToolPromises.size > 0) { - // Snapshot the gate by tool. Human waits remain durable, but they must - // not disable the structural watchdog for an unrelated parallel tool. - const pendingTools = Array.from(context.pendingToolPromises.entries()).map( - ([toolCallId, promise]) => ({ - toolCallId, - promise, - waitBudgetMs: pendingToolWaitBudgetMs(context.toolCalls.get(toolCallId)), - }) - ) - const durableTools = pendingTools.filter((tool) => tool.waitBudgetMs === null) - const boundedTools = pendingTools.flatMap((tool) => - tool.waitBudgetMs === null ? [] : [{ ...tool, waitBudgetMs: tool.waitBudgetMs }] - ) - const boundedWaitBudgetMs = - boundedTools.length > 0 - ? Math.max(...boundedTools.map((tool) => tool.waitBudgetMs)) + - TOOL_WATCHDOG_RESUME_GRACE_MS - : null const waitSpan = context.trace.startSpan('Wait for Tools', 'lifecycle.wait_tools', { checkpointId: continuation.checkpointId, pendingCount: context.pendingToolPromises.size, - durableCount: durableTools.length, - ...(boundedWaitBudgetMs !== null ? { waitBudgetMs: boundedWaitBudgetMs } : {}), - }) - logger.info('Waiting for in-flight tool executions before resume', { - checkpointId: continuation.checkpointId, - pendingCount: context.pendingToolPromises.size, - durableCount: durableTools.length, - waitBudgetMs: boundedWaitBudgetMs, }) - const boundedSettledInTime = - boundedWaitBudgetMs === null - ? true - : await Promise.race([ - Promise.allSettled(boundedTools.map((tool) => tool.promise)).then(() => true), - sleep(boundedWaitBudgetMs).then(() => false), - ]) - if (!boundedSettledInTime) { - const hungToolCallIds = boundedTools - .filter( - ({ toolCallId, promise }) => context.pendingToolPromises.get(toolCallId) === promise + let maximumWaitBudgetMs = 0 + let timedOutCount = 0 + const pendingWatchdogs = new Map< + string, + { + promise: Promise + settlement: Promise<{ + toolCallId: string + promise: Promise + }> + deadlineAt: number + waitBudgetMs: number + } + >() + + /** + * A long-running approval must not lend its deadline to an unrelated + * short tool. Wake for the earliest promise settlement or deadline so a + * replaced call can receive its own watchdog without waiting for a long + * sibling. Unchanged promises retain their absolute deadlines. + */ + while (context.pendingToolPromises.size > 0) { + if (isAborted(options, context)) break + const now = Date.now() + for (const [toolCallId, watchdog] of pendingWatchdogs) { + if (context.pendingToolPromises.get(toolCallId) !== watchdog.promise) { + pendingWatchdogs.delete(toolCallId) + } + } + for (const [toolCallId, promise] of context.pendingToolPromises) { + if (pendingWatchdogs.get(toolCallId)?.promise === promise) continue + const waitBudgetMs = + pendingToolWaitBudgetMs(context.toolCalls.get(toolCallId)) + + TOOL_WATCHDOG_RESUME_GRACE_MS + pendingWatchdogs.set(toolCallId, { + promise, + settlement: promise.then( + () => ({ toolCallId, promise }), + () => ({ toolCallId, promise }) + ), + deadlineAt: now + waitBudgetMs, + waitBudgetMs, + }) + maximumWaitBudgetMs = Math.max(maximumWaitBudgetMs, waitBudgetMs) + } + + const expiredTools = Array.from(pendingWatchdogs.entries()).filter( + ([toolCallId, watchdog]) => + watchdog.deadlineAt <= now && + context.pendingToolPromises.get(toolCallId) === watchdog.promise + ) + if (expiredTools.length > 0) { + await Promise.all( + expiredTools.map(async ([toolCallId, watchdog]) => { + logger.error( + 'Pending tool execution exceeded its resume wait budget; force-failing', + { + checkpointId: continuation.checkpointId, + toolCallId, + waitBudgetMs: watchdog.waitBudgetMs, + } + ) + await forceFailHungToolCall( + toolCallId, + context, + 'Tool execution hung on the Sim executor and was abandoned so the conversation could continue.' + ) + if (context.pendingToolPromises.get(toolCallId) === watchdog.promise) { + context.pendingToolPromises.delete(toolCallId) + } + pendingWatchdogs.delete(toolCallId) + }) ) - .map(({ toolCallId }) => toolCallId) - logger.error('Pending tool executions exceeded the resume wait budget; force-failing', { + timedOutCount += expiredTools.length + continue + } + + const activeWatchdogs = Array.from(pendingWatchdogs.entries()) + if (activeWatchdogs.length === 0) continue + const nextDeadlineAt = Math.min( + ...activeWatchdogs.map(([, watchdog]) => watchdog.deadlineAt) + ) + logger.info('Waiting for in-flight tool executions before resume', { checkpointId: continuation.checkpointId, - waitBudgetMs: boundedWaitBudgetMs, - hungToolCallIds, + pendingCount: activeWatchdogs.length, + maximumWaitBudgetMs, + nextDeadlineAt, }) - for (const toolCallId of hungToolCallIds) { - await forceFailHungToolCall( - toolCallId, - context, - 'Tool execution hung on the Sim executor and was abandoned so the conversation could continue.' - ) - context.pendingToolPromises.delete(toolCallId) + + const watchdogController = new AbortController() + const waitSignal = options.abortSignal + ? AbortSignal.any([watchdogController.signal, options.abortSignal]) + : watchdogController.signal + try { + const wake = await Promise.race([ + ...activeWatchdogs.map(([, watchdog]) => watchdog.settlement), + interruptibleSleep(Math.max(0, nextDeadlineAt - Date.now()), waitSignal).then( + () => null + ), + ]) + if (isAborted(options, context)) break + if (wake && context.pendingToolPromises.get(wake.toolCallId) === wake.promise) { + context.pendingToolPromises.delete(wake.toolCallId) + } + } finally { + watchdogController.abort() } } - await Promise.allSettled(durableTools.map((tool) => tool.promise)) - waitSpan.attributes = { ...waitSpan.attributes, settledInTime: boundedSettledInTime } - context.trace.endSpan(waitSpan) + const waitWasAborted = isAborted(options, context) + waitSpan.attributes = { + ...waitSpan.attributes, + waitBudgetMs: maximumWaitBudgetMs, + timedOutCount, + aborted: waitWasAborted, + settledInTime: timedOutCount === 0 && !waitWasAborted, + } + context.trace.endSpan( + waitSpan, + waitWasAborted ? RequestTraceV1SpanStatus.cancelled : RequestTraceV1SpanStatus.ok + ) } if (isAborted(options, context)) { diff --git a/apps/sim/lib/copilot/request/tools/executor.test.ts b/apps/sim/lib/copilot/request/tools/executor.test.ts index 4bc032fae61..359d594252e 100644 --- a/apps/sim/lib/copilot/request/tools/executor.test.ts +++ b/apps/sim/lib/copilot/request/tools/executor.test.ts @@ -124,10 +124,10 @@ describe('toolWatchdogTimeoutMs', () => { }) describe('pendingToolWaitBudgetMs', () => { - it('does not put a deadline on an executing browser takeover', () => { - expect( - pendingToolWaitBudgetMs({ name: 'browser_request_takeover', status: 'executing' }) - ).toBeNull() + it('bounds retired browser calls that can no longer be executed by the client', () => { + expect(pendingToolWaitBudgetMs({ name: 'browser_request_takeover', status: 'executing' })).toBe( + TOOL_WATCHDOG_DEFAULT_MS + ) }) it('waits on a person for as long as the whole turn allows', () => { @@ -138,6 +138,17 @@ describe('pendingToolWaitBudgetMs', () => { ) }) + it('matches the requested browser_wait_for renderer budget', () => { + expect(pendingToolWaitBudgetMs({ name: 'browser_wait_for', status: 'executing' })).toBe(25_000) + expect( + pendingToolWaitBudgetMs({ + name: 'browser_wait_for', + status: 'executing', + params: { timeoutMs: 120_000 }, + }) + ).toBe(135_000) + }) + it('falls back to the tool\u2019s own watchdog once it is actually executing', () => { expect(pendingToolWaitBudgetMs({ name: 'terminal_run', status: 'executing' })).toBe( TOOL_WATCHDOG_DEFAULT_MS diff --git a/apps/sim/lib/copilot/request/tools/executor.ts b/apps/sim/lib/copilot/request/tools/executor.ts index f96192e579d..158cd08897e 100644 --- a/apps/sim/lib/copilot/request/tools/executor.ts +++ b/apps/sim/lib/copilot/request/tools/executor.ts @@ -1,3 +1,7 @@ +import { + BROWSER_WAIT_FOR_RENDERER_GRACE_MS, + normalizeBrowserWaitForTimeoutMs, +} from '@sim/browser-protocol' import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { isRecordLike } from '@sim/utils/object' @@ -80,7 +84,6 @@ import { type ToolCallState, } from '@/lib/copilot/request/types' import { ensureHandlersRegistered, executeTool } from '@/lib/copilot/tool-executor' -import { RETIRED_BROWSER_REQUEST_TAKEOVER_ID } from '@/lib/copilot/tools/retired-tools' import { isMcpTool } from '@/executor/constants' export { waitForToolCompletion } from '@/lib/copilot/request/tools/client' @@ -251,21 +254,22 @@ export function toolWatchdogTimeoutMs(toolName: string | undefined): number { } /** - * How long the resume gate may wait on one pending tool call. Null means the - * tool is durably waiting on a person and has no deadline. - * - * A call sitting on a permission prompt is waiting on a person, not on the - * executor, so the tool's own watchdog is the wrong bound — the 60s default - * would force-fail the prompt while the user was still reading it. Such a call - * gets the long-running budget, which matches the gate's own wait timeout. + * How long the resume gate may wait on one pending tool call. Permission + * prompts receive the long-running budget, while `browser_wait_for` receives + * its normalized requested timeout plus renderer delivery grace. */ export function pendingToolWaitBudgetMs( - toolCall: Pick | undefined -): number | null { - if (toolCall?.name === RETIRED_BROWSER_REQUEST_TAKEOVER_ID && toolCall?.status === 'executing') { - return null - } + toolCall: + | (Pick & Partial>) + | undefined +): number { if (toolCall?.status === 'awaiting_approval') return TOOL_WATCHDOG_LONG_RUNNING_MS + if (toolCall?.name === 'browser_wait_for') { + return ( + normalizeBrowserWaitForTimeoutMs(toolCall.params?.timeoutMs) + + BROWSER_WAIT_FOR_RENDERER_GRACE_MS + ) + } return toolWatchdogTimeoutMs(toolCall?.name) } diff --git a/apps/sim/lib/copilot/tools/browser-protocol-contract.test.ts b/apps/sim/lib/copilot/tools/browser-protocol-contract.test.ts new file mode 100644 index 00000000000..e98507283c9 --- /dev/null +++ b/apps/sim/lib/copilot/tools/browser-protocol-contract.test.ts @@ -0,0 +1,23 @@ +import { + CURRENT_BROWSER_TOOL_NAMES, + isBrowserToolName, + isCurrentBrowserToolName, +} from '@sim/browser-protocol' +import { describe, expect, it } from 'vitest' +import { TOOL_CATALOG } from '@/lib/copilot/generated/tool-catalog-v1' + +describe('browser tool protocol contract', () => { + it('matches the current model-visible browser catalog after legacy exclusions', () => { + const protocolTools = [...CURRENT_BROWSER_TOOL_NAMES].sort() + const catalogTools = Object.keys(TOOL_CATALOG) + .filter((name) => name.startsWith('browser_')) + .sort() + + expect(protocolTools).toEqual(catalogTools) + }) + + it('recognizes retired browser history without treating it as executable', () => { + expect(isBrowserToolName('browser_request_takeover')).toBe(true) + expect(isCurrentBrowserToolName('browser_request_takeover')).toBe(false) + }) +}) diff --git a/apps/sim/lib/copilot/tools/client/browser-tool-execution.test.ts b/apps/sim/lib/copilot/tools/client/browser-tool-execution.test.ts index 82b9cc74da1..8e18c68a2b3 100644 --- a/apps/sim/lib/copilot/tools/client/browser-tool-execution.test.ts +++ b/apps/sim/lib/copilot/tools/client/browser-tool-execution.test.ts @@ -1,18 +1,21 @@ /** * @vitest-environment jsdom */ +import { Blob as NodeBlob } from 'node:buffer' import { sleep } from '@sim/utils/helpers' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { mockCancelBrowserTool, mockExecuteBrowserTool, mockReportCompletion, + mockReportCompletionOnPageExit, mockRestoreBrowserScope, } = vi.hoisted(() => ({ mockCancelBrowserTool: vi.fn(), mockExecuteBrowserTool: vi.fn(), mockReportCompletion: vi.fn(), + mockReportCompletionOnPageExit: vi.fn(), mockRestoreBrowserScope: vi.fn(), })) @@ -23,6 +26,7 @@ vi.mock('@/lib/browser-agent/transport', () => ({ })) vi.mock('@/lib/copilot/tools/client/completion', () => ({ reportClientToolCompletion: mockReportCompletion, + reportClientToolCompletionOnPageExit: mockReportCompletionOnPageExit, })) import { executeBrowserToolOnClient } from '@/lib/copilot/tools/client/browser-tool-execution' @@ -44,7 +48,12 @@ function nextToolCallId(): string { describe('executeBrowserToolOnClient', () => { beforeEach(() => { vi.clearAllMocks() + vi.stubGlobal('Blob', NodeBlob) window.sessionStorage.clear() + Object.defineProperty(navigator, 'sendBeacon', { + configurable: true, + value: vi.fn(() => true), + }) const session = { pageState: null, tabs: [], @@ -62,10 +71,15 @@ describe('executeBrowserToolOnClient', () => { sessions: { [CHAT_SCOPE]: session }, }) mockReportCompletion.mockResolvedValue(undefined) + mockReportCompletionOnPageExit.mockResolvedValue(undefined) mockRestoreBrowserScope.mockResolvedValue(false) mockCancelBrowserTool.mockResolvedValue(true) }) + afterEach(() => { + vi.unstubAllGlobals() + }) + it('executes the tool and reports success when the session is alive', async () => { mockExecuteBrowserTool.mockResolvedValue({ text: 'page content' }) const toolCallId = nextToolCallId() @@ -86,6 +100,49 @@ describe('executeBrowserToolOnClient', () => { }) }) + it('uses unload-safe delivery without reporting a successful action as failed', async () => { + mockExecuteBrowserTool.mockResolvedValue({ text: 'page content' }) + mockReportCompletion.mockRejectedValue(new Error('confirmation unavailable')) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_snapshot', {}) + await flush() + + expect(mockReportCompletion).toHaveBeenCalledOnce() + expect(mockReportCompletion).toHaveBeenCalledWith(toolCallId, 'success', expect.any(String), { + text: 'page content', + }) + expect(mockReportCompletionOnPageExit).toHaveBeenCalledWith( + toolCallId, + 'success', + 'Browser action completed', + { text: 'page content' } + ) + }) + + it('retains a known result for page-exit flush after both delivery attempts fail', async () => { + mockExecuteBrowserTool.mockResolvedValue({ text: 'page content' }) + mockReportCompletion.mockRejectedValue(new Error('confirmation unavailable')) + mockReportCompletionOnPageExit.mockRejectedValue(new Error('keepalive unavailable')) + const sendBeacon = vi.mocked(navigator.sendBeacon) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_snapshot', {}) + await flush() + window.dispatchEvent(new Event('pagehide')) + await flush() + + expect(mockCancelBrowserTool).not.toHaveBeenCalled() + expect(sendBeacon).toHaveBeenCalledOnce() + const beaconPayload = sendBeacon.mock.calls[0]?.[1] + expect(JSON.parse(await (beaconPayload as NodeBlob).text())).toEqual({ + toolCallId, + status: 'success', + message: 'Browser action completed', + data: { text: 'page content' }, + }) + }) + it('preserves a takeover instruction and waits without a renderer deadline', async () => { mockExecuteBrowserTool.mockResolvedValue({ completed: true, @@ -144,6 +201,252 @@ describe('executeBrowserToolOnClient', () => { expect(mockReportCompletion).not.toHaveBeenCalled() }) + it('cancels native work and reports the lost result when the page exits', async () => { + let resolveTool: (value: unknown) => void = () => {} + mockExecuteBrowserTool.mockImplementation( + () => + new Promise((resolve) => { + resolveTool = resolve + }) + ) + const sendBeacon = vi.mocked(navigator.sendBeacon) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_snapshot', {}) + window.dispatchEvent(new Event('pagehide')) + await flush() + + expect(mockCancelBrowserTool).toHaveBeenCalledWith(toolCallId, CHAT_SCOPE, 'browser_snapshot') + expect(sendBeacon).toHaveBeenCalledOnce() + const beaconPayload = sendBeacon.mock.calls[0]?.[1] + expect(JSON.parse(await (beaconPayload as NodeBlob).text())).toMatchObject({ + message: expect.stringContaining('may already have taken effect'), + data: { outcomeUnknown: true, doNotRetry: true }, + }) + resolveTool({ text: 'late result' }) + await flush() + expect(mockReportCompletion).not.toHaveBeenCalled() + }) + + it('cancels before dispatch and reports the lost result when the page exits during restore', async () => { + useBrowserSessionStore.getState().setSessionAlive(false, CHAT_SCOPE) + let finishRestore: () => void = () => {} + mockRestoreBrowserScope.mockImplementation( + () => + new Promise((resolve) => { + finishRestore = () => { + useBrowserSessionStore.getState().setSessionAlive(true, CHAT_SCOPE) + resolve(true) + } + }) + ) + const sendBeacon = vi.mocked(navigator.sendBeacon) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_snapshot', {}) + window.dispatchEvent(new Event('pagehide')) + await flush() + + expect(mockCancelBrowserTool).toHaveBeenCalledWith(toolCallId, CHAT_SCOPE, 'browser_snapshot') + expect(sendBeacon).toHaveBeenCalledOnce() + const beaconPayload = sendBeacon.mock.calls[0]?.[1] + expect(JSON.parse(await (beaconPayload as NodeBlob).text())).toMatchObject({ + message: expect.stringContaining('before this browser action started'), + data: { outcomeUnknown: false, doNotRetry: false }, + }) + finishRestore() + await flush() + expect(mockExecuteBrowserTool).not.toHaveBeenCalled() + expect(mockReportCompletion).not.toHaveBeenCalled() + }) + + it.each([ + ['not accepted', () => false], + [ + 'throws', + () => { + throw new Error('beacon unavailable') + }, + ], + ])('falls back to the completion reporter when the page-exit beacon %s', async (_label, send) => { + mockExecuteBrowserTool.mockImplementation(() => new Promise(() => {})) + Object.defineProperty(navigator, 'sendBeacon', { + configurable: true, + value: vi.fn(send), + }) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_click', { elementId: 1 }) + window.dispatchEvent(new Event('pagehide')) + await flush() + + expect(mockCancelBrowserTool).toHaveBeenCalledWith(toolCallId, CHAT_SCOPE, 'browser_click') + expect(mockReportCompletionOnPageExit).toHaveBeenCalledWith( + toolCallId, + 'error', + expect.stringContaining('may already have taken effect'), + expect.objectContaining({ outcomeUnknown: true, doNotRetry: true }) + ) + }) + + it('re-delivers known success when the page exits during confirmation', async () => { + mockExecuteBrowserTool.mockResolvedValue({ text: 'page content' }) + let finishReport: () => void = () => {} + mockReportCompletion.mockImplementation( + () => + new Promise((resolve) => { + finishReport = resolve + }) + ) + const sendBeacon = vi.mocked(navigator.sendBeacon) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_snapshot', {}) + await flush() + window.dispatchEvent(new Event('pagehide')) + await flush() + + expect(mockCancelBrowserTool).not.toHaveBeenCalled() + expect(sendBeacon).toHaveBeenCalledOnce() + const beaconPayload = sendBeacon.mock.calls[0]?.[1] + expect(JSON.parse(await (beaconPayload as NodeBlob).text())).toEqual({ + toolCallId, + status: 'success', + message: 'Browser action completed', + data: { text: 'page content' }, + }) + expect(mockReportCompletion).toHaveBeenCalledWith(toolCallId, 'success', expect.any(String), { + text: 'page content', + }) + finishReport() + await flush() + }) + + it('re-delivers a known native error when the page exits during confirmation', async () => { + mockExecuteBrowserTool.mockRejectedValue(new Error('element disappeared')) + let finishReport: () => void = () => {} + mockReportCompletion.mockImplementation( + () => + new Promise((resolve) => { + finishReport = resolve + }) + ) + const sendBeacon = vi.mocked(navigator.sendBeacon) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_click', { elementId: 1 }) + await flush() + window.dispatchEvent(new Event('pagehide')) + await flush() + + expect(mockCancelBrowserTool).not.toHaveBeenCalled() + const beaconPayload = sendBeacon.mock.calls[0]?.[1] + expect(JSON.parse(await (beaconPayload as NodeBlob).text())).toEqual({ + toolCallId, + status: 'error', + message: 'element disappeared', + data: { error: 'element disappeared' }, + }) + finishReport() + await flush() + }) + + it('re-delivers a known session-closed error when the page exits during confirmation', async () => { + useBrowserSessionStore.getState().setSessionAlive(false, CHAT_SCOPE) + let finishReport: () => void = () => {} + mockReportCompletion.mockImplementation( + () => + new Promise((resolve) => { + finishReport = resolve + }) + ) + const sendBeacon = vi.mocked(navigator.sendBeacon) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_snapshot', {}) + await flush() + window.dispatchEvent(new Event('pagehide')) + await flush() + + expect(mockExecuteBrowserTool).not.toHaveBeenCalled() + expect(mockCancelBrowserTool).not.toHaveBeenCalled() + const beaconPayload = sendBeacon.mock.calls[0]?.[1] + expect(JSON.parse(await (beaconPayload as NodeBlob).text())).toMatchObject({ + toolCallId, + status: 'error', + data: { sessionClosed: true }, + }) + finishReport() + await flush() + }) + + it('compacts a large known result before unload-safe delivery', async () => { + mockExecuteBrowserTool.mockResolvedValue({ + dataUrl: `data:image/jpeg;base64,${'A'.repeat(64 * 1024)}`, + url: 'https://example.com', + }) + let finishReport: () => void = () => {} + mockReportCompletion.mockImplementation( + () => + new Promise((resolve) => { + finishReport = resolve + }) + ) + const sendBeacon = vi.mocked(navigator.sendBeacon) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_screenshot', {}) + await flush() + window.dispatchEvent(new Event('pagehide')) + await flush() + + const beaconPayload = sendBeacon.mock.calls[0]?.[1] as NodeBlob + const payload = JSON.parse(await beaconPayload.text()) + expect(beaconPayload.size).toBeLessThanOrEqual(48 * 1024) + expect(payload).toMatchObject({ + toolCallId, + status: 'success', + data: { resultOmittedDuringPageExit: true }, + }) + expect(payload.data.attachment).toBeUndefined() + finishReport() + await flush() + }) + + it.each([ + ['not accepted', () => false], + [ + 'throws', + () => { + throw new Error('beacon unavailable') + }, + ], + ])( + 'uses keepalive fallback for known success when the page-exit beacon %s', + async (_label, send) => { + mockExecuteBrowserTool.mockResolvedValue({ text: 'page content' }) + mockReportCompletion.mockImplementation(() => new Promise(() => {})) + Object.defineProperty(navigator, 'sendBeacon', { + configurable: true, + value: vi.fn(send), + }) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_snapshot', {}) + await flush() + window.dispatchEvent(new Event('pagehide')) + await flush() + + expect(mockCancelBrowserTool).not.toHaveBeenCalled() + expect(mockReportCompletionOnPageExit).toHaveBeenCalledWith( + toolCallId, + 'success', + 'Browser action completed', + { text: 'page content' } + ) + } + ) + it('suppresses completion when scope cancellation outlives the stream AbortController', async () => { let resolveTool: (value: unknown) => void = () => {} let markCancelled: (() => void) | undefined @@ -245,9 +548,12 @@ describe('executeBrowserToolOnClient', () => { it('reshapes a screenshot into an image attachment the model can see', async () => { mockExecuteBrowserTool.mockResolvedValue({ dataUrl: 'data:image/jpeg;base64,/9j/4AAQ', - url: 'https://example.com/pricing', - width: 1024, - height: 640, + viewport: { + url: 'https://example.com/pricing', + title: 'Pricing', + width: 1024, + height: 640, + }, }) const toolCallId = nextToolCallId() @@ -261,7 +567,7 @@ describe('executeBrowserToolOnClient', () => { }) expect(reported.content).toContain('https://example.com/pricing') expect(reported.dataUrl).toBeUndefined() - expect(reported.width).toBe(1024) + expect(reported.viewport).toMatchObject({ width: 1024, height: 640 }) }) it('falls back to a note when a screenshot is not a usable data URL', async () => { @@ -276,16 +582,18 @@ describe('executeBrowserToolOnClient', () => { expect(reported.note).toContain('could not be encoded') }) - // The desktop driver parses browser_wait_for.timeoutMs with a lenient num() - // that coerces numeric strings, then clamps to 120s. This side has to match: - // budgeting less time than the desktop actually waits makes the renderer - // abort first, which strands the native promise on the serialized tool queue - // and stalls every later browser call behind it. + /** + * Shared normalization coerces numeric strings and caps the requested wait + * at 120 seconds. The renderer adds delivery grace so it cannot abandon the + * native queue while the desktop is still honoring that same wait. + */ it.each([ ['number', 30_000, 45_000], ['numeric string', '30000', 45_000], ['absent', undefined, 25_000], ['non-numeric', 'soon', 25_000], + ['zero', 0, 25_000], + ['negative', -5_000, 25_000], ['above the desktop clamp', 500_000, 135_000], ])( 'budgets browser_wait_for above the desktop wait (%s)', @@ -379,6 +687,25 @@ describe('executeBrowserToolOnClient', () => { }) }) + it('lists known sessions without restoring a closed page scope', async () => { + useBrowserSessionStore.getState().setSessionAlive(false, CHAT_SCOPE) + mockExecuteBrowserTool.mockResolvedValue({ sessions: [] }) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_list_sessions', {}) + await flush() + + expect(mockRestoreBrowserScope).not.toHaveBeenCalled() + expect(mockExecuteBrowserTool).toHaveBeenCalledWith( + toolCallId, + 'browser_list_sessions', + {}, + 30_000, + CHAT_SCOPE, + expect.any(Function) + ) + }) + it('tags a failure with sessionClosed when the session died mid-call', async () => { mockExecuteBrowserTool.mockImplementation(async () => { useBrowserSessionStore.getState().setSessionAlive(false, CHAT_SCOPE) @@ -412,6 +739,27 @@ describe('executeBrowserToolOnClient', () => { }) }) + it('preserves structured do-not-retry guidance for an outcome-unknown timeout', async () => { + mockExecuteBrowserTool.mockRejectedValue( + Object.assign(new Error('The browser outcome is unknown.'), { outcomeUnknown: true }) + ) + const toolCallId = nextToolCallId() + + executeBrowserToolOnClient(toolCallId, 'browser_click', { ref: 'e12' }) + await flush() + + expect(mockReportCompletion).toHaveBeenCalledWith( + toolCallId, + 'error', + 'The browser outcome is unknown.', + { + error: 'The browser outcome is unknown.', + outcomeUnknown: true, + doNotRetry: true, + } + ) + }) + it('checks and executes against the originating chat rather than the active projection', async () => { const store = useBrowserSessionStore.getState() store.setSessionAlive(false, 'chat-a') diff --git a/apps/sim/lib/copilot/tools/client/browser-tool-execution.ts b/apps/sim/lib/copilot/tools/client/browser-tool-execution.ts index 4e1f4476822..a2174c6df9e 100644 --- a/apps/sim/lib/copilot/tools/client/browser-tool-execution.ts +++ b/apps/sim/lib/copilot/tools/client/browser-tool-execution.ts @@ -7,45 +7,49 @@ * browser and reports the outcome via the confirm endpoint, which wakes the * server-side waiter. */ -import type { BrowserToolName } from '@sim/browser-protocol' +import { + BROWSER_WAIT_FOR_RENDERER_GRACE_MS, + type BrowserToolName, + normalizeBrowserWaitForTimeoutMs, +} from '@sim/browser-protocol' import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { isRecordLike } from '@sim/utils/object' +import { truncate } from '@sim/utils/string' import { cancelBrowserTool, executeBrowserTool, restoreBrowserScope, } from '@/lib/browser-agent/transport' -import { ASYNC_TOOL_CONFIRMATION_STATUS } from '@/lib/copilot/async-runs/lifecycle' +import { + ASYNC_TOOL_CONFIRMATION_STATUS, + type AsyncCompletionData, + type AsyncConfirmationStatus, +} from '@/lib/copilot/async-runs/lifecycle' import { COPILOT_CONFIRM_API_PATH } from '@/lib/copilot/constants' -import { reportClientToolCompletion } from '@/lib/copilot/tools/client/completion' +import { + reportClientToolCompletion, + reportClientToolCompletionOnPageExit, +} from '@/lib/copilot/tools/client/completion' import { getBrowserSession, useBrowserSessionStore } from '@/stores/browser-session/store' const logger = createLogger('CopilotBrowserToolExecution') const DEFAULT_TOOL_TIMEOUT_MS = 30_000 const NAVIGATION_TOOL_TIMEOUT_MS = 45_000 -const WAIT_FOR_TIMEOUT_GRACE_MS = 15_000 -// Mirror the desktop driver's parse of browser_wait_for.timeoutMs exactly. It -// coerces numeric strings and clamps to a maximum; reading the value more -// strictly here would budget less time than the desktop actually waits, and the -// renderer aborting first strands the native promise on the serialized tool -// queue so every later browser call stalls behind it. -const DEFAULT_WAIT_FOR_TIMEOUT_MS = 10_000 -const MAX_WAIT_FOR_TIMEOUT_MS = 120_000 /** - * Tools that can revive a closed browser session by opening a fresh tab. - * Everything else requires a live page and is rejected up front when the - * session is closed, instead of burning the full IPC timeout per call — a - * dead session used to answer every tool with an indistinguishable generic - * ~30s timeout, which the agent retried indefinitely. + * Tools that do not require an existing live page. Most create a new page; + * `browser_list_sessions` reads the desktop's profile-level session registry. + * Everything else is rejected up front when a closed scope cannot be restored, + * instead of burning the full IPC timeout per call. */ -const SESSION_REVIVAL_TOOLS: ReadonlySet = new Set([ +const LIVE_PAGE_OPTIONAL_TOOLS: ReadonlySet = new Set([ 'browser_navigate', 'browser_open_url', 'browser_open_tab', 'browser_list_tabs', + 'browser_list_sessions', ]) const SESSION_CLOSED_MESSAGE = @@ -55,6 +59,36 @@ const SESSION_CLOSED_MESSAGE = /** Tool events older than this are replays, not live instructions — never act on them. */ const MAX_EVENT_AGE_MS = 120_000 const EXECUTED_STORAGE_PREFIX = 'sim:copilot:browser-tool-executed:' +const PAGE_EXIT_COMPLETION_MAX_BYTES = 48 * 1024 +const OUTCOME_UNKNOWN_MESSAGE = + 'The Sim window closed while this browser action was in flight. It may already have taken effect. Do not retry it automatically; take a fresh browser snapshot before deciding what to do.' + +interface PendingTerminalCompletion { + status: AsyncConfirmationStatus + message: string + data?: AsyncCompletionData +} + +function compactCompletionForPageExit( + toolCallId: string, + completion: PendingTerminalCompletion +): PendingTerminalCompletion { + const serialized = JSON.stringify({ toolCallId, ...completion }) + if (new Blob([serialized]).size <= PAGE_EXIT_COMPLETION_MAX_BYTES) return completion + + const data = isRecordLike(completion.data) ? completion.data : {} + return { + status: completion.status, + message: truncate(completion.message, 1024), + data: { + ...(data.outcomeUnknown === true ? { outcomeUnknown: true } : {}), + ...(data.doNotRetry === true ? { doNotRetry: true } : {}), + ...(data.sessionClosed === true ? { sessionClosed: true } : {}), + resultOmittedDuringPageExit: true, + note: 'The browser action reached a known terminal state, but its full result was too large for unload-safe delivery. Do not repeat a side-effecting action. Take a fresh browser snapshot to recover current page state.', + }, + } +} /** * Exactly-once guard. Stream recovery and tab reloads replay persisted tool @@ -91,6 +125,15 @@ function eventAgeMs(eventTs: string | undefined): number | null { return Number.isNaN(emitted) ? null : Date.now() - emitted } +function isOutcomeUnknownError(error: unknown): boolean { + return ( + typeof error === 'object' && + error !== null && + 'outcomeUnknown' in error && + error.outcomeUnknown === true + ) +} + function timeoutForTool(toolName: BrowserToolName, params: Record): number | null { if (toolName === 'browser_request_takeover') return null if ( @@ -103,12 +146,8 @@ function timeoutForTool(toolName: BrowserToolName, params: Record 0 - ? Math.min(raw, MAX_WAIT_FOR_TIMEOUT_MS) - : DEFAULT_WAIT_FOR_TIMEOUT_MS - return requested + WAIT_FOR_TIMEOUT_GRACE_MS + const requested = normalizeBrowserWaitForTimeoutMs(params.timeoutMs) + return requested + BROWSER_WAIT_FOR_RENDERER_GRACE_MS } return DEFAULT_TOOL_TIMEOUT_MS } @@ -145,7 +184,14 @@ function sanitizeResultForModel( note: 'The screenshot could not be encoded. Use browser_snapshot or browser_read_text instead.', } } - const location = typeof rest.url === 'string' && rest.url ? ` of ${rest.url}` : '' + const viewport = isRecordLike(rest.viewport) ? rest.viewport : null + const screenshotUrl = + typeof rest.url === 'string' && rest.url + ? rest.url + : viewport && typeof viewport.url === 'string' + ? viewport.url + : '' + const location = screenshotUrl ? ` of ${screenshotUrl}` : '' return { ...rest, content: `Screenshot${location}. This is the rendered viewport only — it carries no element ids, so use browser_snapshot before interacting.`, @@ -239,6 +285,45 @@ async function doExecuteBrowserTool( abortSignal?: AbortSignal ): Promise { let cancelled = abortSignal?.aborted === true + let nativeActionPending = true + let nativeDispatchStarted = false + let pendingTerminalCompletion: PendingTerminalCompletion | null = null + const reportTerminalCompletion = async ( + completion: PendingTerminalCompletion, + failureLog: string + ): Promise => { + pendingTerminalCompletion = completion + try { + await reportClientToolCompletion( + toolCallId, + completion.status, + completion.message, + completion.data + ) + pendingTerminalCompletion = null + } catch (error) { + logger.error(failureLog, { + toolCallId, + error: toError(error).message, + }) + const compactCompletion = compactCompletionForPageExit(toolCallId, completion) + pendingTerminalCompletion = compactCompletion + try { + await reportClientToolCompletionOnPageExit( + toolCallId, + compactCompletion.status, + compactCompletion.message, + compactCompletion.data + ) + pendingTerminalCompletion = null + } catch (fallbackError) { + logger.error('Failed to enqueue browser completion with unload-safe fallback', { + toolCallId, + error: toError(fallbackError).message, + }) + } + } + } const cancelNativeTool = async () => { cancelled = true try { @@ -252,121 +337,174 @@ async function doExecuteBrowserTool( } } const onAbort = () => { + if (!nativeActionPending) return void cancelNativeTool() } - if (cancelled) { - void cancelNativeTool() - } else { - abortSignal?.addEventListener('abort', onAbort, { once: true }) - } - - const needsLivePage = !SESSION_REVIVAL_TOOLS.has(toolName) - if (needsLivePage && isSessionClosed(scopeId)) { - try { - await restoreBrowserScope(scopeId) - } catch (err) { - logger.warn('Could not restore the scoped browser session before tool execution', { + const onPageHide = () => { + if (cancelled) return + const pendingCompletion = + pendingTerminalCompletion ?? + (() => { + if (!nativeActionPending) return null + const message = nativeDispatchStarted + ? OUTCOME_UNKNOWN_MESSAGE + : 'The Sim window closed before this browser action started. Its result was lost.' + return { + status: ASYNC_TOOL_CONFIRMATION_STATUS.error, + message, + data: { + error: message, + outcomeUnknown: nativeDispatchStarted, + doNotRetry: nativeDispatchStarted, + }, + } + })() + if (!pendingCompletion) return + const completion = compactCompletionForPageExit(toolCallId, pendingCompletion) + if (typeof window !== 'undefined') { + window.removeEventListener('pagehide', onPageHide) + } + const reportFallback = () => { + void reportClientToolCompletionOnPageExit( toolCallId, - toolName, - error: toError(err).message, + completion.status, + completion.message, + completion.data + ).catch((error) => { + logger.error('Failed to report browser page-exit completion fallback', { + toolCallId, + toolName, + error: toError(error).message, + }) }) } - } - if (needsLivePage && isSessionClosed(scopeId)) { - logger.warn('Rejecting browser tool: agent browser session is closed', { - toolCallId, - toolName, - }) - if (cancelled) { - abortSignal?.removeEventListener('abort', onAbort) - return + if (nativeActionPending) { + void cancelNativeTool() } - await reportClientToolCompletion( - toolCallId, - ASYNC_TOOL_CONFIRMATION_STATUS.error, - SESSION_CLOSED_MESSAGE, - { error: SESSION_CLOSED_MESSAGE, sessionClosed: true } - ).catch((reportErr) => { - logger.error('Failed to report browser session-closed error', { + try { + const accepted = navigator.sendBeacon( + COPILOT_CONFIRM_API_PATH, + new Blob( + [ + JSON.stringify({ + toolCallId, + status: completion.status, + message: completion.message, + ...(completion.data !== undefined ? { data: completion.data } : {}), + }), + ], + { type: 'application/json' } + ) + ) + if (!accepted) { + logger.warn('Browser page-exit completion beacon was not accepted', { + toolCallId, + toolName, + }) + reportFallback() + } + } catch (error) { + logger.warn('Browser page-exit completion beacon failed', { toolCallId, - error: toError(reportErr).message, + toolName, + error: toError(error).message, }) - }) - abortSignal?.removeEventListener('abort', onAbort) - return + reportFallback() + } } - // A restore can outlive the stream that requested it. Do not dispatch the - // tool afterward—older shells have no cancellation tombstone to catch a - // takeover-done signal that arrived before the takeover itself existed. if (cancelled) { - abortSignal?.removeEventListener('abort', onAbort) - return - } - // If the user leaves the page mid-action the awaited result is lost; tell - // the waiter so the turn fails fast instead of hanging until its timeout. - const onPageHide = () => { - if (cancelled) return - navigator.sendBeacon( - COPILOT_CONFIRM_API_PATH, - new Blob( - [ - JSON.stringify({ - toolCallId, - status: ASYNC_TOOL_CONFIRMATION_STATUS.error, - message: - 'The user left the Sim window while this browser action was running, so its result was lost.', - }), - ], - { type: 'application/json' } - ) - ) + void cancelNativeTool() + } else { + abortSignal?.addEventListener('abort', onAbort, { once: true }) } if (typeof window !== 'undefined') { window.addEventListener('pagehide', onPageHide) } - logger.info('Executing browser tool via the desktop agent browser', { toolCallId, toolName }) - try { - const result = await executeBrowserTool( - toolCallId, - toolName, - params, - timeoutForTool(toolName, params), - scopeId, - () => { - cancelled = true + const needsLivePage = !LIVE_PAGE_OPTIONAL_TOOLS.has(toolName) + if (needsLivePage && isSessionClosed(scopeId)) { + try { + await restoreBrowserScope(scopeId) + } catch (err) { + logger.warn('Could not restore the scoped browser session before tool execution', { + toolCallId, + toolName, + error: toError(err).message, + }) } - ) - if (cancelled) return - await reportClientToolCompletion( - toolCallId, - ASYNC_TOOL_CONFIRMATION_STATUS.success, - 'Browser action completed', - sanitizeResultForModel(toolName, result) - ) - } catch (err) { - if (cancelled) return - // The session dying mid-call (e.g. during a takeover) surfaces as a - // generic timeout; tag it so the model learns the real, terminal cause - // instead of retrying against a dead session. - const sessionClosed = isSessionClosed(scopeId) - const message = sessionClosed - ? `${toError(err).message} ${SESSION_CLOSED_MESSAGE}` - : toError(err).message - logger.warn('Browser tool failed', { toolCallId, toolName, error: message, sessionClosed }) - await reportClientToolCompletion(toolCallId, ASYNC_TOOL_CONFIRMATION_STATUS.error, message, { - error: message, - ...(sessionClosed ? { sessionClosed: true } : {}), - }).catch((reportErr) => { - logger.error('Failed to report browser tool error', { + } + if (needsLivePage && isSessionClosed(scopeId)) { + nativeActionPending = false + logger.warn('Rejecting browser tool: agent browser session is closed', { toolCallId, - error: toError(reportErr).message, + toolName, }) - }) + if (cancelled) return + await reportTerminalCompletion( + { + status: ASYNC_TOOL_CONFIRMATION_STATUS.error, + message: SESSION_CLOSED_MESSAGE, + data: { error: SESSION_CLOSED_MESSAGE, sessionClosed: true }, + }, + 'Failed to report browser session-closed error' + ) + return + } + + if (cancelled) return + + logger.info('Executing browser tool via the desktop agent browser', { toolCallId, toolName }) + + let result: unknown + try { + nativeDispatchStarted = true + result = await executeBrowserTool( + toolCallId, + toolName, + params, + timeoutForTool(toolName, params), + scopeId, + () => { + cancelled = true + } + ) + } catch (err) { + nativeActionPending = false + if (cancelled) return + const sessionClosed = isSessionClosed(scopeId) + const outcomeUnknown = isOutcomeUnknownError(err) + const message = sessionClosed + ? `${toError(err).message} ${SESSION_CLOSED_MESSAGE}` + : toError(err).message + logger.warn('Browser tool failed', { toolCallId, toolName, error: message, sessionClosed }) + await reportTerminalCompletion( + { + status: ASYNC_TOOL_CONFIRMATION_STATUS.error, + message, + data: { + error: message, + ...(outcomeUnknown ? { outcomeUnknown: true, doNotRetry: true } : {}), + ...(sessionClosed ? { sessionClosed: true } : {}), + }, + }, + 'Failed to report browser tool error' + ) + return + } + nativeActionPending = false + if (cancelled) return + await reportTerminalCompletion( + { + status: ASYNC_TOOL_CONFIRMATION_STATUS.success, + message: 'Browser action completed', + data: sanitizeResultForModel(toolName, result), + }, + 'Failed to report successful browser tool completion' + ) } finally { abortSignal?.removeEventListener('abort', onAbort) - if (typeof window !== 'undefined') { + if (typeof window !== 'undefined' && !pendingTerminalCompletion) { window.removeEventListener('pagehide', onPageHide) } } diff --git a/apps/sim/lib/copilot/tools/client/completion.test.ts b/apps/sim/lib/copilot/tools/client/completion.test.ts new file mode 100644 index 00000000000..00fd511e67b --- /dev/null +++ b/apps/sim/lib/copilot/tools/client/completion.test.ts @@ -0,0 +1,50 @@ +/** + * @vitest-environment node + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + CompletionReportError, + reportClientToolCompletionOnPageExit, +} from '@/lib/copilot/tools/client/completion' + +describe('reportClientToolCompletionOnPageExit', () => { + const fetchMock = vi.fn() + + beforeEach(() => { + fetchMock.mockResolvedValue(new Response(null, { status: 200 })) + vi.stubGlobal('fetch', fetchMock) + }) + + afterEach(() => { + vi.unstubAllGlobals() + vi.clearAllMocks() + }) + + it('uses a keepalive request with the exact terminal payload', async () => { + await reportClientToolCompletionOnPageExit('tool-1', 'success', 'Browser action completed', { + url: 'https://example.com', + }) + + expect(fetchMock).toHaveBeenCalledWith( + '/api/copilot/confirm', + expect.objectContaining({ + method: 'POST', + keepalive: true, + body: JSON.stringify({ + toolCallId: 'tool-1', + status: 'success', + message: 'Browser action completed', + data: { url: 'https://example.com' }, + }), + }) + ) + }) + + it('rejects a non-success response', async () => { + fetchMock.mockResolvedValue(new Response(null, { status: 503 })) + + await expect( + reportClientToolCompletionOnPageExit('tool-1', 'error', 'Browser failed') + ).rejects.toBeInstanceOf(CompletionReportError) + }) +}) diff --git a/apps/sim/lib/copilot/tools/client/completion.ts b/apps/sim/lib/copilot/tools/client/completion.ts index 691cb1699f4..7457de9e14e 100644 --- a/apps/sim/lib/copilot/tools/client/completion.ts +++ b/apps/sim/lib/copilot/tools/client/completion.ts @@ -96,3 +96,30 @@ export async function reportClientToolCompletion( }) throw new CompletionReportError(lastError?.message ?? 'Failed to report tool completion') } + +/** + * Makes one unload-safe attempt to deliver a compact terminal result. The + * caller must keep the serialized payload below the browser's keepalive quota. + */ +export async function reportClientToolCompletionOnPageExit( + toolCallId: string, + status: AsyncConfirmationStatus, + message: string, + data?: AsyncCompletionData +): Promise { + // boundary-raw-fetch: keepalive is required so a terminal desktop result survives page unload + const response = await fetch(COPILOT_CONFIRM_API_PATH, { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...traceparentHeader() }, + body: JSON.stringify({ + toolCallId, + status, + message, + ...(data !== undefined ? { data } : {}), + }), + keepalive: true, + }) + if (!response.ok) { + throw new CompletionReportError(`Page-exit completion failed with status ${response.status}`) + } +} diff --git a/apps/sim/lib/copilot/tools/client/terminal-tool-execution.test.ts b/apps/sim/lib/copilot/tools/client/terminal-tool-execution.test.ts new file mode 100644 index 00000000000..3421f3903dc --- /dev/null +++ b/apps/sim/lib/copilot/tools/client/terminal-tool-execution.test.ts @@ -0,0 +1,56 @@ +/** + * @vitest-environment jsdom + */ +import { Blob as NodeBlob } from 'node:buffer' +import { sleep } from '@sim/utils/helpers' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { executeTerminalTool, reportClientToolCompletion } = vi.hoisted(() => ({ + executeTerminalTool: vi.fn(), + reportClientToolCompletion: vi.fn(), +})) + +vi.mock('@/lib/terminal/transport', () => ({ executeTerminalTool })) +vi.mock('@/lib/copilot/tools/client/completion', () => ({ reportClientToolCompletion })) + +import { executeTerminalToolOnClient } from '@/lib/copilot/tools/client/terminal-tool-execution' + +describe('terminal client execution', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.stubGlobal('Blob', NodeBlob) + window.sessionStorage.clear() + Object.defineProperty(navigator, 'sendBeacon', { + configurable: true, + value: vi.fn(() => true), + }) + reportClientToolCompletion.mockResolvedValue(undefined) + }) + + it('marks a page-exit result as indeterminate and unsafe to retry', async () => { + let resolveExecution: (result: unknown) => void = () => {} + executeTerminalTool.mockImplementation( + () => + new Promise((resolve) => { + resolveExecution = resolve + }) + ) + + executeTerminalToolOnClient('terminal-page-exit', { operation: 'read', args: {} }, 'chat-1') + window.dispatchEvent(new Event('pagehide')) + + const beacon = vi.mocked(navigator.sendBeacon) + expect(beacon).toHaveBeenCalledOnce() + const payload = beacon.mock.calls[0]?.[1] + expect(JSON.parse(await (payload as NodeBlob).text())).toMatchObject({ + toolCallId: 'terminal-page-exit', + status: 'error', + data: { outcomeUnknown: true, doNotRetry: true }, + }) + + resolveExecution({ output: 'done' }) + await sleep(0) + window.dispatchEvent(new Event('pagehide')) + expect(beacon).toHaveBeenCalledOnce() + }) +}) diff --git a/apps/sim/lib/copilot/tools/client/terminal-tool-execution.ts b/apps/sim/lib/copilot/tools/client/terminal-tool-execution.ts index 378f56fd133..caa1acf86a9 100644 --- a/apps/sim/lib/copilot/tools/client/terminal-tool-execution.ts +++ b/apps/sim/lib/copilot/tools/client/terminal-tool-execution.ts @@ -151,6 +151,7 @@ async function doExecuteTerminalTool( status: ASYNC_TOOL_CONFIRMATION_STATUS.error, message: 'The user left the Sim window while this terminal command was running, so its result was lost.', + data: { outcomeUnknown: true, doNotRetry: true }, }), ], { type: 'application/json' } diff --git a/packages/browser-protocol/src/index.ts b/packages/browser-protocol/src/index.ts index ff93d2661bd..cc6689eff14 100644 --- a/packages/browser-protocol/src/index.ts +++ b/packages/browser-protocol/src/index.ts @@ -8,16 +8,19 @@ * that is embedded INSIDE the main Sim window, positioned exactly over the * chat's browser panel. The panel is therefore natively interactive — the * user clicks and types into the real page, no frame streaming or synthetic - * input. Both sides consume this package so tool names, parameter shapes, - * and result shapes cannot drift. + * input. Both sides consume this package for tool identity, shared timeout + * policy, and bridge envelopes. Individual tool parameters and results are + * still validated by the desktop driver rather than statically mapped here. * - * Tool names and parameter shapes mirror the mothership tool catalog - * (`copilot/internal/tools/catalog/browser` in the mothership repo) — that - * catalog is the source of truth for what the model can call; this package is - * the source of truth for how those calls travel to the desktop main process. + * Current tool names mirror the mothership tool catalog + * (`copilot/internal/tools/catalog/browser` in the mothership repo). This + * package also retains retired names needed to replay persisted chat history. + * The catalog is the source of truth for what the model can call; this package + * is the source of truth for how current and compatible legacy calls travel to + * the desktop main process. */ -export const BROWSER_TOOL_NAMES = [ +export const CURRENT_BROWSER_TOOL_NAMES = [ 'browser_navigate', 'browser_open_url', 'browser_go_back', @@ -41,11 +44,38 @@ export const BROWSER_TOOL_NAMES = [ 'browser_select_option', 'browser_hover', 'browser_drag', - 'browser_request_takeover', +] as const + +export type CurrentBrowserToolName = (typeof CURRENT_BROWSER_TOOL_NAMES)[number] + +export const RETIRED_BROWSER_TOOL_NAMES = ['browser_request_takeover'] as const + +export const BROWSER_TOOL_NAMES = [ + ...CURRENT_BROWSER_TOOL_NAMES, + ...RETIRED_BROWSER_TOOL_NAMES, ] as const export type BrowserToolName = (typeof BROWSER_TOOL_NAMES)[number] +export const BROWSER_WAIT_FOR_DEFAULT_TIMEOUT_MS = 10_000 +export const BROWSER_WAIT_FOR_MAX_TIMEOUT_MS = 120_000 +export const BROWSER_WAIT_FOR_RENDERER_GRACE_MS = 15_000 + +/** + * Normalizes the model-visible `browser_wait_for.timeoutMs` consistently in + * the renderer and desktop main process. + */ +export function normalizeBrowserWaitForTimeoutMs(value: unknown): number { + const parsed = + typeof value === 'number' + ? value + : typeof value === 'string' && value.trim() !== '' + ? Number(value) + : Number.NaN + if (!Number.isFinite(parsed) || parsed <= 0) return BROWSER_WAIT_FOR_DEFAULT_TIMEOUT_MS + return Math.min(parsed, BROWSER_WAIT_FOR_MAX_TIMEOUT_MS) +} + export const BROWSER_THEMES = ['system', 'light', 'dark'] as const /** Sim appearance preference mirrored into browser-tab media queries. */ @@ -55,12 +85,18 @@ export type BrowserTheme = (typeof BROWSER_THEMES)[number] export type BrowserOmniboxFocusMode = 'select' | 'clear' const BROWSER_TOOL_NAME_SET: ReadonlySet = new Set(BROWSER_TOOL_NAMES) +const CURRENT_BROWSER_TOOL_NAME_SET: ReadonlySet = new Set(CURRENT_BROWSER_TOOL_NAMES) const BROWSER_THEME_SET: ReadonlySet = new Set(BROWSER_THEMES) export function isBrowserToolName(name: string): name is BrowserToolName { return BROWSER_TOOL_NAME_SET.has(name) } +/** True only for browser tools the current model catalog may execute. */ +export function isCurrentBrowserToolName(name: string): name is CurrentBrowserToolName { + return CURRENT_BROWSER_TOOL_NAME_SET.has(name) +} + export function isBrowserTheme(value: unknown): value is BrowserTheme { return typeof value === 'string' && BROWSER_THEME_SET.has(value) } @@ -139,11 +175,10 @@ export interface BrowserPanelSnapshot { /** * Browser-chrome commands from the panel header (URL bar, back/forward, - * reload) plus `takeover-done`, sent by the question card on the chat's - * `browser_request_takeover` tool row when the user finishes a - * hand-control-back request. Page interactions need no protocol — the user - * acts on the real embedded page directly, and its right-click menu is native - * and lives entirely in the shell. + * reload) plus the legacy `takeover-done` action retained for persisted + * `browser_request_takeover` cards. Page interactions need no protocol — the + * user acts on the real embedded page directly, and its right-click menu is + * native and lives entirely in the shell. */ export interface BrowserPanelAction { action: @@ -308,8 +343,8 @@ export const BROWSER_DATA_KINDS = ['cookies', 'site-data', 'cache'] as const /** * A kind of browsing data the user can clear independently. * - * Download history is deliberately absent: the built-in browser cancels every - * download, so there is none to clear and offering the option would be a lie. + * Downloads are deliberately absent because their files and per-chat transfer + * history have a separate lifecycle from Chromium browsing-data removal. * Saved passwords are absent too — they are a separate, explicit action. */ export type BrowserDataKind = (typeof BROWSER_DATA_KINDS)[number] diff --git a/packages/desktop-bridge/contract-snapshot.ts b/packages/desktop-bridge/contract-snapshot.ts index 2cad883150e..8b61c52b0e2 100644 --- a/packages/desktop-bridge/contract-snapshot.ts +++ b/packages/desktop-bridge/contract-snapshot.ts @@ -23,16 +23,19 @@ * that is embedded INSIDE the main Sim window, positioned exactly over the * chat's browser panel. The panel is therefore natively interactive — the * user clicks and types into the real page, no frame streaming or synthetic - * input. Both sides consume this package so tool names, parameter shapes, - * and result shapes cannot drift. + * input. Both sides consume this package for tool identity, shared timeout + * policy, and bridge envelopes. Individual tool parameters and results are + * still validated by the desktop driver rather than statically mapped here. * - * Tool names and parameter shapes mirror the mothership tool catalog - * (`copilot/internal/tools/catalog/browser` in the mothership repo) — that - * catalog is the source of truth for what the model can call; this package is - * the source of truth for how those calls travel to the desktop main process. + * Current tool names mirror the mothership tool catalog + * (`copilot/internal/tools/catalog/browser` in the mothership repo). This + * package also retains retired names needed to replay persisted chat history. + * The catalog is the source of truth for what the model can call; this package + * is the source of truth for how current and compatible legacy calls travel to + * the desktop main process. */ -export const BROWSER_TOOL_NAMES = [ +export const CURRENT_BROWSER_TOOL_NAMES = [ 'browser_navigate', 'browser_open_url', 'browser_go_back', @@ -56,11 +59,38 @@ export const BROWSER_TOOL_NAMES = [ 'browser_select_option', 'browser_hover', 'browser_drag', - 'browser_request_takeover', +] as const + +export type CurrentBrowserToolName = (typeof CURRENT_BROWSER_TOOL_NAMES)[number] + +export const RETIRED_BROWSER_TOOL_NAMES = ['browser_request_takeover'] as const + +export const BROWSER_TOOL_NAMES = [ + ...CURRENT_BROWSER_TOOL_NAMES, + ...RETIRED_BROWSER_TOOL_NAMES, ] as const export type BrowserToolName = (typeof BROWSER_TOOL_NAMES)[number] +export const BROWSER_WAIT_FOR_DEFAULT_TIMEOUT_MS = 10_000 +export const BROWSER_WAIT_FOR_MAX_TIMEOUT_MS = 120_000 +export const BROWSER_WAIT_FOR_RENDERER_GRACE_MS = 15_000 + +/** + * Normalizes the model-visible `browser_wait_for.timeoutMs` consistently in + * the renderer and desktop main process. + */ +export function normalizeBrowserWaitForTimeoutMs(value: unknown): number { + const parsed = + typeof value === 'number' + ? value + : typeof value === 'string' && value.trim() !== '' + ? Number(value) + : Number.NaN + if (!Number.isFinite(parsed) || parsed <= 0) return BROWSER_WAIT_FOR_DEFAULT_TIMEOUT_MS + return Math.min(parsed, BROWSER_WAIT_FOR_MAX_TIMEOUT_MS) +} + export const BROWSER_THEMES = ['system', 'light', 'dark'] as const /** Sim appearance preference mirrored into browser-tab media queries. */ @@ -70,12 +100,18 @@ export type BrowserTheme = (typeof BROWSER_THEMES)[number] export type BrowserOmniboxFocusMode = 'select' | 'clear' const BROWSER_TOOL_NAME_SET: ReadonlySet = new Set(BROWSER_TOOL_NAMES) +const CURRENT_BROWSER_TOOL_NAME_SET: ReadonlySet = new Set(CURRENT_BROWSER_TOOL_NAMES) const BROWSER_THEME_SET: ReadonlySet = new Set(BROWSER_THEMES) export function isBrowserToolName(name: string): name is BrowserToolName { return BROWSER_TOOL_NAME_SET.has(name) } +/** True only for browser tools the current model catalog may execute. */ +export function isCurrentBrowserToolName(name: string): name is CurrentBrowserToolName { + return CURRENT_BROWSER_TOOL_NAME_SET.has(name) +} + export function isBrowserTheme(value: unknown): value is BrowserTheme { return typeof value === 'string' && BROWSER_THEME_SET.has(value) } @@ -154,11 +190,10 @@ export interface BrowserPanelSnapshot { /** * Browser-chrome commands from the panel header (URL bar, back/forward, - * reload) plus `takeover-done`, sent by the question card on the chat's - * `browser_request_takeover` tool row when the user finishes a - * hand-control-back request. Page interactions need no protocol — the user - * acts on the real embedded page directly, and its right-click menu is native - * and lives entirely in the shell. + * reload) plus the legacy `takeover-done` action retained for persisted + * `browser_request_takeover` cards. Page interactions need no protocol — the + * user acts on the real embedded page directly, and its right-click menu is + * native and lives entirely in the shell. */ export interface BrowserPanelAction { action: @@ -323,8 +358,8 @@ export const BROWSER_DATA_KINDS = ['cookies', 'site-data', 'cache'] as const /** * A kind of browsing data the user can clear independently. * - * Download history is deliberately absent: the built-in browser cancels every - * download, so there is none to clear and offering the option would be a lie. + * Downloads are deliberately absent because their files and per-chat transfer + * history have a separate lifecycle from Chromium browsing-data removal. * Saved passwords are absent too — they are a separate, explicit action. */ export type BrowserDataKind = (typeof BROWSER_DATA_KINDS)[number]