Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/desktop/src/main/browser-agent/cdp.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ describe('browser-agent CDP instrumentation', () => {
controller.abort()

await expect(
clickAt(contents, 5, 6, false, PRIMARY_CLICK, controller.signal)
clickAt(contents, 5, 6, true, PRIMARY_CLICK, controller.signal)
).rejects.toMatchObject({ name: 'AbortError' })
expect(contents.debugger.sendCommand).not.toHaveBeenCalled()
})
Expand Down
3 changes: 2 additions & 1 deletion apps/desktop/src/main/browser-agent/cdp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -984,7 +984,7 @@ export function clearAgentContextMenu(contents: WebContents): void {
}

/**
* Clicks at viewport coordinates. An already-aborted `signal` rejects before anything is pressed.
* Clicks at viewport coordinates. An already-aborted `signal` rejects before any input is sent.
* During a press-and-hold it ends the hold early: the click rejects with the abort reason and the
* button is released at once, so a cancelled or timed-out click cannot stay held into the next
* action. That release can still activate the control under the pointer.
Expand All @@ -997,6 +997,7 @@ export async function clickAt(
click: PointerClick = PRIMARY_CLICK,
signal?: AbortSignal
): Promise<void> {
signal?.throwIfAborted()
if (moveBeforePress) await moveMouse(contents, x, y)
signal?.throwIfAborted()
const { button, clickCount, modifiers, holdMs } = click
Expand Down
20 changes: 13 additions & 7 deletions apps/desktop/src/main/browser-agent/driver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4661,9 +4661,12 @@ describe('credential protection', () => {
})
})

it.each(['browser_snapshot', 'browser_find'] as const)(
'omits an absent scope from the serialized %s page call',
async (tool) => {
it.each([
['browser_snapshot', 'true'],
['browser_find', 'false'],
] as const)(
'passes an absent scope as null to the serialized %s page call',
async (tool, markNew) => {
const contents = await openPage()
vi.mocked(contents.executeJavaScript).mockClear()
await driver.executeTool('chat-test', tool, { query: 'Continue' })
Expand All @@ -4672,13 +4675,16 @@ describe('credential protection', () => {
.mock.calls.map(([expression]) => expression)
.filter((expression) => isPageCall(expression, 'collectSnapshot'))
expect(expressions).toHaveLength(1)
expect(expressions[0]).toContain('.apply(null, [1])')
expect(expressions[0]).toContain(`.apply(null, [1,null,${markNew}])`)
}
)

it.each(['browser_snapshot', 'browser_find'] as const)(
it.each([
['browser_snapshot', 'true'],
['browser_find', 'false'],
] as const)(
'passes the current root ref to %s and invalidates previous refs',
async (tool) => {
async (tool, markNew) => {
const contents = await openPage()
respondWith(contents, {
collectSnapshot: {
Expand All @@ -4700,7 +4706,7 @@ describe('credential protection', () => {
.mock.calls.some(
([expression]) =>
isPageCall(expression, 'collectSnapshot') &&
expression.includes('.apply(null, [1,0])')
expression.includes(`.apply(null, [1,0,${markNew}])`)
)
).toBe(true)
const stale = await driver.executeTool('chat-test', 'browser_click', { elementId: 0 })
Expand Down
12 changes: 7 additions & 5 deletions apps/desktop/src/main/browser-agent/driver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2285,12 +2285,14 @@ function validateSnapshotRefs(
* Captures the top page plus each cross-origin boundary frame in its own
* isolated world. CDP is the privileged bridge the top document's same-origin
* policy intentionally lacks; password redaction still runs inside every frame
* before any result crosses back to the driver.
* before any result crosses back to the driver. `markNew` is false for reads the
* model never sees as an outline, so they neither carry nor consume `new` markers.
*/
async function captureSnapshot(
contents: WebContents,
notAfter?: number,
elementId?: number
elementId?: number,
markNew = true
): Promise<unknown> {
const state = driverScopeState()
const tab = session.requireAutomationTab()
Expand Down Expand Up @@ -2326,7 +2328,7 @@ async function captureSnapshot(
await execInPage(
contents,
collectSnapshot,
elementId === undefined ? [mainStartingElementId] : [mainStartingElementId, elementId],
[mainStartingElementId, elementId ?? null, markNew],
false,
notAfter
)
Expand Down Expand Up @@ -2383,7 +2385,7 @@ async function captureSnapshot(
const frameSnapshot = await execInPage(
frame,
collectSnapshot,
[frameStartingElementId],
[frameStartingElementId, null, markNew],
false,
notAfter
)
Expand Down Expand Up @@ -2867,7 +2869,7 @@ async function executeToolInner(
const maxResults = Math.min(50, Math.max(1, Math.floor(requestedMax ?? 20)))
const contents = session.requireAutomationTab().view.webContents
const snapshot = toRecord(
await captureSnapshot(contents, executionDeadline, num(params, 'elementId'))
await captureSnapshot(contents, executionDeadline, num(params, 'elementId'), false)
)
const outline = typeof snapshot.outline === 'string' ? snapshot.outline : ''
const needle = query.toLowerCase()
Expand Down
14 changes: 14 additions & 0 deletions apps/desktop/src/main/browser-agent/page-functions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,20 @@ describe('collectSnapshot', () => {
expect(outlineOf(collectSnapshot())).not.toContain(' new')
})

it('leaves new markers out of an unmarked read without consuming them', () => {
document.body.innerHTML = '<button>Compose</button>'
visible(document.querySelector('button') as HTMLButtonElement)
collectSnapshot()

const added = document.createElement('button')
added.textContent = 'Send'
document.body.append(visible(added))

expect(outlineOf(collectSnapshot(0, null, false))).not.toContain(' new')
const lines = outlineOf(collectSnapshot()).split('\n')
expect(lines.find((line) => line.includes('"Send"'))).toMatch(/ new$/)
})

it('sanitizes a malicious role so it cannot forge a second snapshot line', () => {
document.body.innerHTML = '<div tabindex="0" aria-label="Safe control"></div>'
const control = visible(document.querySelector('div') as HTMLDivElement)
Expand Down
28 changes: 20 additions & 8 deletions apps/desktop/src/main/browser-agent/page-functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,16 +128,22 @@ export function installPageHelpers(): void {
* Builds the page snapshot: a structural outline (headings, landmarks) with
* interactive elements carrying numeric ids, walking open shadow roots and
* same-origin iframes. Rebuilds the element registry as a side effect.
* `markNew` false leaves the `new` markers out and records nothing as shown,
* for internal reads (such as a text search) whose outline the model never sees.
*/
export function collectSnapshot(startingElementId = 0, elementId?: number): unknown {
export function collectSnapshot(
startingElementId = 0,
elementId: number | null = null,
markNew = true
): unknown {
const resolver = window.__simAgentResolveElement
const scopedRoot =
elementId === undefined
elementId === null
? undefined
: resolver
? resolver(elementId, false)?.element
: window.__simAgentElements?.[elementId]
if (elementId !== undefined) {
if (elementId !== null) {
if (!scopedRoot?.isConnected) return { error: 'stale', reason: window.__simAgentStaleReason }
if (scopedRoot.ownerDocument !== document) return { error: 'framed-snapshot' }
}
Expand Down Expand Up @@ -238,9 +244,13 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn
window.__simAgentElements = registry
const previouslyShown = window.__simAgentShownElements
const shown = previouslyShown ?? new WeakSet<Element>()
window.__simAgentShownElements = shown
if (markNew) window.__simAgentShownElements = shown
/** Whether no earlier snapshot of this document listed the element; the first snapshot marks nothing. */
const isNew = (el: Element): boolean => previouslyShown !== undefined && !shown.has(el)
const isNew = (el: Element): boolean => markNew && previouslyShown !== undefined && !shown.has(el)
/** Records an element whose line made it into the outline, so the next snapshot knows it. */
const recordShown = (el: Element): void => {
if (markNew) shown.add(el)
}
const lines: string[] = []
let truncated = false
let refCount = 0
Expand Down Expand Up @@ -585,11 +595,11 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn
}
}
if (isNew(el)) parts.push('new')
shown.add(el)
const suffix = parts.length > 0 ? ` ${parts.join(' ')}` : ''
const lineIndex = lines.length
if (push(`${indent}- ${role} ${quote(name)} [ref=${id}]${suffix}`)) {
refLineIndexes[id] = lineIndex
recordShown(el)
}
}

Expand All @@ -608,9 +618,11 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn
const id = registerElement(el, roleFor(el), text)
textLineCount++
const marker = isNew(el) ? ' new' : ''
shown.add(el)
const lineIndex = lines.length
if (push(`${indent}- text ${quote(text)} [ref=${id}]${marker}`)) refLineIndexes[id] = lineIndex
if (push(`${indent}- text ${quote(text)} [ref=${id}]${marker}`)) {
refLineIndexes[id] = lineIndex
recordShown(el)
}
}

const headingLevel = (el: Element): number | null => {
Expand Down
33 changes: 33 additions & 0 deletions apps/desktop/src/main/browser-agent/session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4221,6 +4221,39 @@ describe('browser-agent session', () => {
})
})

it('stops a download whose placeholder claim hangs and removes the late placeholder', async () => {
vi.useFakeTimers()
try {
const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
const gate = deferred<void>()
const claimFile = vi.fn(async (path: string) => {
await gate.promise
writeFileSync(path, '', { flag: 'wx' })
})
session = freshSession(win, {}, undefined, {
getDirectory: () => directory,
getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER,
pathExists: () => false,
claimFile,
})
const contents = (session.ensureTab().view as unknown as MockView).webContents
const download = mockDownloadItem({ filename: 'hung-claim.bin', totalBytes: 100 })

startMockDownload(contents, download)
await vi.advanceTimersByTimeAsync(0)
expect(claimFile).toHaveBeenCalledOnce()
await vi.advanceTimersByTimeAsync(5_000)

expect(download.item.cancel).toHaveBeenCalledOnce()
expect(download.item.resume).not.toHaveBeenCalled()
download.emitDone('cancelled')
gate.resolve()
await vi.waitFor(() => expect(readdirSync(directory)).toEqual([]))
} finally {
vi.useRealTimers()
}
})

it('keeps a torn-down download name reserved until its pending claim settles', async () => {
const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
const claims: Array<{ path: string; gate: ReturnType<typeof deferred<void>> }> = []
Expand Down
49 changes: 23 additions & 26 deletions apps/desktop/src/main/browser-agent/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,8 @@ async function moveStagedBrowserDownload(active: ActiveBrowserDownload): Promise
for (let attempt = 1; ; attempt++) {
try {
await moveFile(active.stagingPath, destination)
// The name now holds the finished file, which no cleanup may remove as a placeholder.
active.placeholderPath = undefined
return destination
} catch (error) {
const code = (error as NodeJS.ErrnoException).code
Expand Down Expand Up @@ -1737,27 +1739,33 @@ function configureBrowserDownloads(ses: Session): void {
})
})
let allocationExpired = false
const allocationLive = () =>
!allocationExpired &&
!active.terminal &&
!active.limitReason &&
activeBrowserDownloads.has(active)
const allocation = uniqueDownloadPath(directory, filename, {
isActive: () =>
!allocationExpired &&
!active.terminal &&
!active.limitReason &&
activeBrowserDownloads.has(active),
isActive: allocationLive,
pathExists: browserDownloadSettings?.pathExists,
reservePath: (candidate) => {
if (
allocationExpired ||
active.terminal ||
active.limitReason ||
!activeBrowserDownloads.has(active) ||
activeDownloadPaths.has(candidate)
) {
return false
}
if (!allocationLive() || activeDownloadPaths.has(candidate)) return false
activeDownloadPaths.set(candidate, active)
active.savePath = candidate
return true
},
}).then(async (savePath) => {
if (!savePath || !allocationLive()) return savePath
active.claimingDestination = true
try {
await claimBrowserDownloadDestination(active, savePath)
} finally {
active.claimingDestination = false
if (!allocationLive()) {
removeBrowserDownloadPlaceholder(active)
releaseActiveBrowserDownloadPath(active, savePath)
}
}
return savePath
})
active.destination = withBrowserDownloadTimeout(
allocation,
Expand All @@ -1767,7 +1775,7 @@ function configureBrowserDownloads(ses: Session): void {
allocationExpired = true
}
)
.then(async (savePath) => {
.then((savePath) => {
if (active.terminal || !activeBrowserDownloads.has(active)) {
releaseActiveBrowserDownloadPath(active, savePath ?? undefined)
return null
Expand All @@ -1780,17 +1788,6 @@ function configureBrowserDownloads(ses: Session): void {
publishActiveBrowserDownload(active)
return null
}
active.claimingDestination = true
try {
await claimBrowserDownloadDestination(active, savePath)
} finally {
active.claimingDestination = false
if (active.terminal || !activeBrowserDownloads.has(active)) {
removeBrowserDownloadPlaceholder(active)
releaseActiveBrowserDownloadPath(active, savePath)
}
}
if (active.terminal || !activeBrowserDownloads.has(active)) return null
checkBrowserDownloadDiskSpace(active, 'admission')
return savePath
})
Expand Down
Loading