diff --git a/apps/desktop/src/main/browser-agent/cdp.test.ts b/apps/desktop/src/main/browser-agent/cdp.test.ts index 2718792bed5..a579b13855f 100644 --- a/apps/desktop/src/main/browser-agent/cdp.test.ts +++ b/apps/desktop/src/main/browser-agent/cdp.test.ts @@ -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() }) diff --git a/apps/desktop/src/main/browser-agent/cdp.ts b/apps/desktop/src/main/browser-agent/cdp.ts index a7115c9d352..e5b2b4143d1 100644 --- a/apps/desktop/src/main/browser-agent/cdp.ts +++ b/apps/desktop/src/main/browser-agent/cdp.ts @@ -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. @@ -997,6 +997,7 @@ export async function clickAt( click: PointerClick = PRIMARY_CLICK, signal?: AbortSignal ): Promise { + signal?.throwIfAborted() if (moveBeforePress) await moveMouse(contents, x, y) signal?.throwIfAborted() const { button, clickCount, modifiers, holdMs } = click diff --git a/apps/desktop/src/main/browser-agent/driver.test.ts b/apps/desktop/src/main/browser-agent/driver.test.ts index 46ea744e861..84290af3c75 100644 --- a/apps/desktop/src/main/browser-agent/driver.test.ts +++ b/apps/desktop/src/main/browser-agent/driver.test.ts @@ -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' }) @@ -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: { @@ -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 }) diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index 38de2aef97a..7bf2eff96f5 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -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 { const state = driverScopeState() const tab = session.requireAutomationTab() @@ -2326,7 +2328,7 @@ async function captureSnapshot( await execInPage( contents, collectSnapshot, - elementId === undefined ? [mainStartingElementId] : [mainStartingElementId, elementId], + [mainStartingElementId, elementId ?? null, markNew], false, notAfter ) @@ -2383,7 +2385,7 @@ async function captureSnapshot( const frameSnapshot = await execInPage( frame, collectSnapshot, - [frameStartingElementId], + [frameStartingElementId, null, markNew], false, notAfter ) @@ -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() diff --git a/apps/desktop/src/main/browser-agent/page-functions.test.ts b/apps/desktop/src/main/browser-agent/page-functions.test.ts index f148da6dbcb..a8e61266783 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -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 = '' + 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 = '
' const control = visible(document.querySelector('div') as HTMLDivElement) diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index 244ff9b6e1d..bc2089e70ab 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -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' } } @@ -238,9 +244,13 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn window.__simAgentElements = registry const previouslyShown = window.__simAgentShownElements const shown = previouslyShown ?? new WeakSet() - 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 @@ -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) } } @@ -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 => { diff --git a/apps/desktop/src/main/browser-agent/session.test.ts b/apps/desktop/src/main/browser-agent/session.test.ts index fe710012a3a..02a4f52bb10 100644 --- a/apps/desktop/src/main/browser-agent/session.test.ts +++ b/apps/desktop/src/main/browser-agent/session.test.ts @@ -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() + 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> }> = [] diff --git a/apps/desktop/src/main/browser-agent/session.ts b/apps/desktop/src/main/browser-agent/session.ts index 4dc99479661..b709510f0e1 100644 --- a/apps/desktop/src/main/browser-agent/session.ts +++ b/apps/desktop/src/main/browser-agent/session.ts @@ -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 @@ -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, @@ -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 @@ -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 })