Skip to content

Commit 74f6e48

Browse files
committed
fix(browser): isolate actions and bound file transfers
1 parent ee60695 commit 74f6e48

25 files changed

Lines changed: 30190 additions & 72 deletions

‎.github/workflows/test-build.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -239,14 +239,16 @@ jobs:
239239
if-no-files-found: ignore
240240
retention-days: 7
241241

242-
- name: Verify durable provenance, concurrent memory writes, and attachment replay
242+
- name: Verify durable provenance, concurrent memory writes, and browser download admission
243243
working-directory: apps/sim
244244
env:
245+
BROWSER_FILE_TRANSFER_TEST_DATABASE_URL: postgresql://postgres:postgres@127.0.0.1:5432/sim_auth_scim
245246
TABLE_PROVENANCE_TEST_DATABASE_URL: postgresql://postgres:postgres@127.0.0.1:5432/sim_auth_scim
246247
MEMORY_PROVENANCE_TEST_DATABASE_URL: postgresql://postgres:postgres@127.0.0.1:5432/sim_auth_scim
247248
AGENT_MEMORY_TEST_DATABASE_URL: postgresql://postgres:postgres@127.0.0.1:5432/sim_auth_scim
248249
run: >-
249250
bunx vitest run
251+
lib/mothership/async-runs/browser-download-claim.postgres.test.ts
250252
lib/table/rows/secret-provenance.postgres.test.ts
251253
lib/memory/message-provenance.postgres.test.ts
252254
lib/memory/conversation-store.postgres.test.ts

‎apps/desktop/src/main/browser-agent/cdp.ts‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -796,10 +796,18 @@ const BUTTON_MASKS: Record<PointerClick['button'], number> = { left: 1, right: 2
796796
const agentContextClicks = new WeakMap<WebContents, number>()
797797
const AGENT_CONTEXT_CLICK_WINDOW_MS = 1000
798798

799-
/** True while a context menu event would be the echo of an agent right-click. */
800-
export function isAgentContextMenu(contents: WebContents): boolean {
799+
/** Consumes the single context menu echo expected from an agent right-click. */
800+
export function consumeAgentContextMenu(contents: WebContents): boolean {
801801
const at = agentContextClicks.get(contents)
802-
return at !== undefined && Date.now() - at < AGENT_CONTEXT_CLICK_WINDOW_MS
802+
agentContextClicks.delete(contents)
803+
if (at === undefined) return false
804+
const elapsed = Date.now() - at
805+
return elapsed >= 0 && elapsed < AGENT_CONTEXT_CLICK_WINDOW_MS
806+
}
807+
808+
/** A real user gesture supersedes an agent click whose page prevented its native menu. */
809+
export function clearAgentContextMenu(contents: WebContents): void {
810+
agentContextClicks.delete(contents)
803811
}
804812

805813
export async function clickAt(
@@ -812,7 +820,6 @@ export async function clickAt(
812820
if (moveBeforePress) await moveMouse(contents, x, y)
813821
const { button, clickCount, modifiers } = click
814822
const buttons = BUTTON_MASKS[button]
815-
if (button === 'right') agentContextClicks.set(contents, Date.now())
816823
let pressed = false
817824
try {
818825
// Set before awaiting: CDP can deliver the press and then lose/reject the
@@ -822,6 +829,7 @@ export async function clickAt(
822829
// A multi-click is a sequence of press/release pairs with an increasing
823830
// clickCount — Blink synthesizes dblclick from the pair whose count is 2.
824831
for (let count = 1; count <= clickCount; count++) {
832+
if (button === 'right') agentContextClicks.set(contents, Date.now())
825833
await sendInput(contents, 'Input.dispatchMouseEvent', {
826834
type: 'mousePressed',
827835
x,

‎apps/desktop/src/main/browser-agent/context-menu.test.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { describe, expect, it, vi } from 'vitest'
44
vi.mock('electron', () => import('@/test/electron-mock'))
55

66
import { Menu, WebContentsView } from 'electron'
7+
import { clickAt } from '@/main/browser-agent/cdp'
78
import {
89
attachAgentContextMenu,
910
BASE_ZOOM_FACTOR,
@@ -215,6 +216,51 @@ describe('buildAgentContextMenuTemplate', () => {
215216
describe('attachAgentContextMenu', () => {
216217
type ContextMenuListener = (event: unknown, params: Params) => void
217218

219+
it('suppresses one agent context menu and immediately allows the next menu', async () => {
220+
const contents = new WebContentsView().webContents
221+
attachAgentContextMenu(contents, {
222+
addToChat: vi.fn(),
223+
openTab: vi.fn(),
224+
defaultZoomFactor: () => BASE_ZOOM_FACTOR,
225+
})
226+
const listeners = vi.mocked(contents.on).mock.calls as unknown as [
227+
string,
228+
ContextMenuListener,
229+
][]
230+
const onContextMenu = listeners.find(([event]) => event === 'context-menu')![1]
231+
await clickAt(contents, 10, 20, false, { button: 'right', clickCount: 1, modifiers: 0 })
232+
vi.mocked(Menu.buildFromTemplate).mockClear()
233+
234+
onContextMenu({}, params())
235+
expect(Menu.buildFromTemplate).not.toHaveBeenCalled()
236+
onContextMenu({}, params())
237+
expect(Menu.buildFromTemplate).toHaveBeenCalledTimes(1)
238+
})
239+
240+
it.each(['mouseDown', 'keyDown', 'touchStart'])(
241+
'allows human %s when a page prevents the agent context menu event',
242+
async (inputEvent) => {
243+
const contents = new WebContentsView().webContents
244+
attachAgentContextMenu(contents, {
245+
addToChat: vi.fn(),
246+
openTab: vi.fn(),
247+
defaultZoomFactor: () => BASE_ZOOM_FACTOR,
248+
})
249+
const listeners = vi.mocked(contents.on).mock.calls as unknown as [
250+
string,
251+
(event: unknown, params: unknown) => void,
252+
][]
253+
const onInput = listeners.find(([event]) => event === 'input-event')?.[1]
254+
const onContextMenu = listeners.find(([event]) => event === 'context-menu')![1]
255+
await clickAt(contents, 10, 20, false, { button: 'right', clickCount: 1, modifiers: 0 })
256+
vi.mocked(Menu.buildFromTemplate).mockClear()
257+
258+
onInput?.({}, { type: inputEvent })
259+
onContextMenu({}, params())
260+
expect(Menu.buildFromTemplate).toHaveBeenCalledTimes(1)
261+
}
262+
)
263+
218264
it('pops a menu built from the page that was right-clicked', () => {
219265
const contents = new WebContentsView().webContents
220266
vi.mocked(contents.navigationHistory.canGoBack).mockReturnValue(true)

‎apps/desktop/src/main/browser-agent/context-menu.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,11 @@
1717
import { resolveDesktopZoom } from '@sim/desktop-bridge'
1818
import type { ContextMenuParams, MenuItemConstructorOptions, WebContents } from 'electron'
1919
import { clipboard, Menu } from 'electron'
20-
import { isAgentContextMenu } from '@/main/browser-agent/cdp'
20+
import {
21+
clearAgentContextMenu,
22+
consumeAgentContextMenu,
23+
isDispatchingAgentInput,
24+
} from '@/main/browser-agent/cdp'
2125

2226
/**
2327
* Page-zoom ladder for the embedded browser, in Chromium's absolute zoom
@@ -181,8 +185,16 @@ export function buildAgentContextMenuTemplate(
181185

182186
/** Gives one agent tab its page menu; an agent right-click reaches only the page's own handlers. */
183187
export function attachAgentContextMenu(contents: WebContents, host: AgentContextMenuHost): void {
188+
contents.on('input-event', (_event, input) => {
189+
if (
190+
['mouseDown', 'keyDown', 'rawKeyDown', 'char', 'touchStart'].includes(input.type) &&
191+
!isDispatchingAgentInput(contents)
192+
) {
193+
clearAgentContextMenu(contents)
194+
}
195+
})
184196
contents.on('context-menu', (_event, params) => {
185-
if (isAgentContextMenu(contents)) return
197+
if (consumeAgentContextMenu(contents)) return
186198
const template = buildAgentContextMenuTemplate(
187199
params,
188200
{

‎apps/desktop/src/main/browser-agent/driver.test.ts‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2474,6 +2474,75 @@ describe('credential protection', () => {
24742474
expect(cdpCalls(contents, 'Input.dispatchKeyEvent').length).toBeGreaterThan(0)
24752475
})
24762476

2477+
it.each(['cancelled', 'timed out'] as const)(
2478+
'retains a completed action when its observation is %s',
2479+
async (stop) => {
2480+
const contents = await openPage()
2481+
respondWith(contents, {
2482+
activeElementSecrecy: 'safe',
2483+
readActiveElementState: {},
2484+
readPageActionState: {},
2485+
})
2486+
const pageCall = vi.mocked(contents.executeJavaScript).getMockImplementation()
2487+
let releaseObservation: (value: unknown) => void = () => {}
2488+
const observation = new Promise<unknown>((resolve) => {
2489+
releaseObservation = resolve
2490+
})
2491+
let observing = false
2492+
vi.mocked(contents.executeJavaScript).mockImplementation((expression, ...args) => {
2493+
if (isPageCall(expression, 'collectSnapshot')) {
2494+
observing = true
2495+
return observation
2496+
}
2497+
return pageCall?.(expression, ...args) ?? Promise.resolve(undefined)
2498+
})
2499+
vi.useFakeTimers()
2500+
try {
2501+
const timersBefore = vi.getTimerCount()
2502+
const pending = driver.executeTool(
2503+
'chat-test',
2504+
'browser_press_key',
2505+
{ key: 'a', observe: {} },
2506+
'observed-action'
2507+
)
2508+
await vi.advanceTimersByTimeAsync(200)
2509+
expect(observing).toBe(true)
2510+
2511+
if (stop === 'cancelled') driver.cancelTool('chat-test', 'observed-action')
2512+
else
2513+
await vi.advanceTimersByTimeAsync(driver.browserToolWatchdogMs('browser_press_key', {})!)
2514+
2515+
await expect(pending).resolves.toMatchObject({
2516+
ok: true,
2517+
result: {
2518+
pressed: 'a',
2519+
trusted: true,
2520+
observation: {
2521+
ok: false,
2522+
doNotRetry: true,
2523+
note: expect.stringContaining('The action already ran'),
2524+
},
2525+
},
2526+
})
2527+
await expect(
2528+
driver.executeTool('chat-test', 'browser_list_tabs', {})
2529+
).resolves.toMatchObject({
2530+
ok: true,
2531+
})
2532+
expect(vi.getTimerCount()).toBe(timersBefore)
2533+
expect(
2534+
cdpCalls(contents, 'Input.dispatchKeyEvent').filter(([, event]) =>
2535+
['keyDown', 'rawKeyDown'].includes((event as { type: string }).type)
2536+
)
2537+
).toHaveLength(1)
2538+
} finally {
2539+
releaseObservation({ outline: 'Late snapshot', refIds: [], nextElementId: 1 })
2540+
await vi.advanceTimersByTimeAsync(0)
2541+
vi.useRealTimers()
2542+
}
2543+
}
2544+
)
2545+
24772546
it('reports when a platform-mismatched shortcut produces no observable effect', async () => {
24782547
const contents = await openPage()
24792548
respondWith(contents, {
@@ -3641,6 +3710,47 @@ describe('credential protection', () => {
36413710
expect(dismissed).toMatchObject({ ok: true })
36423711
})
36433712

3713+
it('dismisses background tab dialogs while the action target accepts its dialog', async () => {
3714+
const background = await openPage()
3715+
await driver.executeTool('chat-test', 'browser_open_tab', {})
3716+
const contents = session.requireAutomationTab().view.webContents
3717+
vi.mocked(contents.getURL).mockReturnValue('https://example.com/target')
3718+
respondWith(contents, {
3719+
describePointTarget: { found: true, element: 'button "Delete"', editable: false },
3720+
readActiveElementState: {},
3721+
readPageActionState: {},
3722+
})
3723+
const listeners = [background, contents].map(
3724+
(tab) =>
3725+
vi.mocked(tab.debugger.on).mock.calls.find(([event]) => event === 'message')?.[1] as
3726+
| ((event: unknown, method: string, params: unknown) => void)
3727+
| undefined
3728+
)
3729+
vi.mocked(contents.debugger.sendCommand).mockImplementation(async (method, params) => {
3730+
if (method === 'Input.dispatchMouseEvent' && params?.type === 'mouseReleased') {
3731+
for (const listener of listeners) {
3732+
listener?.({}, 'Page.javascriptDialogOpening', { type: 'confirm', message: 'Delete?' })
3733+
}
3734+
}
3735+
return {}
3736+
})
3737+
3738+
await expect(
3739+
driver.executeTool('chat-test', 'browser_click_at', {
3740+
x: 10,
3741+
y: 20,
3742+
dialog: { accept: true },
3743+
})
3744+
).resolves.toMatchObject({ ok: true })
3745+
3746+
expect(cdpCalls(background, 'Page.handleJavaScriptDialog').map(([, answer]) => answer)).toEqual(
3747+
[{ accept: false }]
3748+
)
3749+
expect(cdpCalls(contents, 'Page.handleJavaScriptDialog').map(([, answer]) => answer)).toEqual([
3750+
{ accept: true },
3751+
])
3752+
})
3753+
36443754
it('rejects a malformed dialog response before dispatch', async () => {
36453755
const contents = await openPage()
36463756

‎apps/desktop/src/main/browser-agent/driver.ts‎

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,10 @@ import {
8282
typeIntoElement,
8383
} from '@/main/browser-agent/page-functions'
8484
import { isPanelVisible, panelWindow } from '@/main/browser-agent/panel'
85-
import { withPostActionObservation } from '@/main/browser-agent/post-action-observation'
85+
import {
86+
withFailedPostActionObservation,
87+
withPostActionObservation,
88+
} from '@/main/browser-agent/post-action-observation'
8689
import * as session from '@/main/browser-agent/session'
8790
import { checkAgentUrl } from '@/main/browser-agent/url-guard'
8891
import { clearCredentials, fillCoordinator, initFillCoordinator } from '@/main/browser-credentials'
@@ -233,8 +236,8 @@ interface DriverScopeState {
233236
/** Unique state generation so teardown cannot suffer an epoch ABA race. */
234237
generation: number
235238
pendingNotices: string[]
236-
/** Answer for JavaScript dialogs opened while the running action executes. */
237-
dialogResponse: cdp.DialogResponse | null
239+
/** Answer for JavaScript dialogs from the running action's target tab only. */
240+
dialogResponse: { contents: WebContents; response: cdp.DialogResponse } | null
238241
takeoverActive: boolean
239242
takeoverDone: boolean
240243
takeoverResponse: string | null
@@ -544,7 +547,10 @@ function instrumentTab(contents: WebContents): void {
544547
)
545548
}),
546549
dialogResponse: () =>
547-
session.withBrowserScope(scopeId, () => driverScopeState().dialogResponse),
550+
session.withBrowserScope(scopeId, () => {
551+
const requested = driverScopeState().dialogResponse
552+
return requested?.contents === contents ? requested.response : null
553+
}),
548554
}
549555
void (async () => {
550556
let lastError: unknown
@@ -4938,7 +4944,10 @@ export async function executeTool(
49384944
session.setAutomationActive(true)
49394945
}
49404946
try {
4941-
state.dialogResponse = dialogResponse(tool, params)
4947+
const response = dialogResponse(tool, params)
4948+
state.dialogResponse = response
4949+
? { contents: session.requireAutomationTab().view.webContents, response }
4950+
: null
49424951
const executionEpoch = ++state.toolExecutionEpoch
49434952
const watchdogMs = browserToolWatchdogMs(tool, params)
49444953
const executionDeadline = watchdogMs === null ? undefined : Date.now() + watchdogMs
@@ -4947,18 +4956,22 @@ export async function executeTool(
49474956
throw new ToolError('This browser action expired before it could dispatch input.')
49484957
}
49494958
}
4959+
let completedAction: { result: unknown } | undefined
49504960
const execution = withPostActionObservation(
49514961
tool,
49524962
params,
4953-
(actionParams) =>
4954-
executeToolInner(
4963+
async (actionParams) => {
4964+
const result = await executeToolInner(
49554965
tool,
49564966
actionParams,
49574967
assertCurrentExecution,
49584968
executionDeadline,
49594969
invocationEpoch,
49604970
executionController.signal
4961-
),
4971+
)
4972+
if (params.observe !== undefined) completedAction = { result }
4973+
return result
4974+
},
49624975
(query) =>
49634976
executeToolInner(
49644977
query === undefined ? 'browser_snapshot' : 'browser_find',
@@ -4970,10 +4983,11 @@ export async function executeTool(
49704983
),
49714984
assertCurrentExecution
49724985
)
4986+
const cancellableExecution = Promise.race([execution, cancellation])
49734987
const guardedExecution =
49744988
watchdogMs === null
4975-
? execution
4976-
: raceAgainstWatchdog(execution, watchdogMs, () => {
4989+
? cancellableExecution
4990+
: raceAgainstWatchdog(cancellableExecution, watchdogMs, () => {
49774991
executionController.abort()
49784992
if (state.toolExecutionEpoch === executionEpoch) state.toolExecutionEpoch++
49794993
if (
@@ -4985,7 +4999,15 @@ export async function executeTool(
49854999
invalidateSnapshot(state)
49865000
}
49875001
})
4988-
const result = withNotices(await Promise.race([guardedExecution, cancellation]))
5002+
let observedResult: unknown
5003+
try {
5004+
observedResult = await guardedExecution
5005+
} catch (error) {
5006+
if (!completedAction) throw error
5007+
invalidateSnapshot(state)
5008+
observedResult = withFailedPostActionObservation(completedAction.result, error)
5009+
}
5010+
const result = withNotices(observedResult)
49895011
logger.info('Browser tool completed', {
49905012
tool,
49915013
toolCallId,

0 commit comments

Comments
 (0)