diff --git a/apps/desktop/src/main/browser-agent/driver.test.ts b/apps/desktop/src/main/browser-agent/driver.test.ts index 2bac3457ae9..c2b1cccb142 100644 --- a/apps/desktop/src/main/browser-agent/driver.test.ts +++ b/apps/desktop/src/main/browser-agent/driver.test.ts @@ -3152,6 +3152,33 @@ describe('credential protection', () => { }) }) + it('recaptures with fresh ids when the page reuses an id this scope never committed', async () => { + const contents = await openPage() + const snapshot = (refIds: number[], nextElementId: number) => ({ + url: 'https://example.com/', + title: 'Example', + outline: refIds.map((ref) => `- button "B${ref}" [ref=${ref}]`).join('\n'), + truncated: false, + refIds, + refLineIndexes: Object.fromEntries(refIds.map((ref, index) => [ref, index])), + nextElementId, + }) + respondWith(contents, { collectSnapshot: snapshot([4, 5], 6) }) + await driver.executeTool('chat-test', 'browser_snapshot', {}) + vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => { + if (!isPageCall(expression, 'collectSnapshot')) return Promise.resolve(undefined) + const forgets = expression.includes(',true])') + return Promise.resolve(forgets ? snapshot([6, 7], 8) : snapshot([2, 6], 7)) + }) + + const result = await driver.executeTool('chat-test', 'browser_snapshot', {}) + + expect(result).toMatchObject({ + ok: true, + result: { outline: expect.stringContaining('[ref=7]') }, + }) + }) + it('acts on a ref an earlier snapshot of the same page issued', async () => { const contents = await openPage() respondWith(contents, { diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index e89e96b4dc6..3ba31fa310d 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -2496,28 +2496,36 @@ async function captureSnapshot( } const mainStartingElementId = state.nextElementRefId - const mainSnapshot = unwrapPageResult( - await execInPage( - contents, - collectSnapshot, - [mainStartingElementId, elementId ?? null, markNew, state.generation], - false, - notAfter + // A page reuses ids it issued in a capture this scope never committed (an + // aborted or timed-out snapshot); when validation cannot vouch for one, the + // page forgets its issued refs and the capture runs once more. + const captureMain = async (forgetIssuedRefs: boolean) => { + const snapshot = unwrapPageResult( + await execInPage( + contents, + collectSnapshot, + [mainStartingElementId, elementId ?? null, markNew, state.generation, forgetIssuedRefs], + false, + notAfter + ) ) - ) - if (!stillCurrent()) { - throw new ToolError('The tab changed while its snapshot was being captured. Try again.') - } - if ( - !isRecordLike(mainSnapshot) || - typeof mainSnapshot.outline !== 'string' || - !Array.isArray(mainSnapshot.refIds) - ) { - throw new ToolError('The page returned an incomplete snapshot. Try browser_snapshot again.') + if (!stillCurrent()) { + throw new ToolError('The tab changed while its snapshot was being captured. Try again.') + } + if ( + !isRecordLike(snapshot) || + typeof snapshot.outline !== 'string' || + !Array.isArray(snapshot.refIds) + ) { + throw new ToolError('The page returned an incomplete snapshot. Try browser_snapshot again.') + } + const refs = validateSnapshotRefs(snapshot, mainStartingElementId, (ref) => + samePageTarget(state.issuedRefTargets.get(ref), contents) + ) + return { snapshot, refs } } - const mainRefs = validateSnapshotRefs(mainSnapshot, mainStartingElementId, (ref) => - samePageTarget(state.issuedRefTargets.get(ref), contents) - ) + let { snapshot: mainSnapshot, refs: mainRefs } = await captureMain(false) + if (!mainRefs) ({ snapshot: mainSnapshot, refs: mainRefs } = await captureMain(true)) if (!mainRefs) { throw new ToolError('The page returned invalid element ids. Try browser_snapshot again.') } @@ -2556,13 +2564,15 @@ async function captureSnapshot( } const frameUrl = frame.url const frameStartingElementId = nextElementId - const frameSnapshot = await execInPage( - frame, - collectSnapshot, - [frameStartingElementId, null, markNew, state.generation], - false, - notAfter - ) + const captureFrame = (forgetIssuedRefs: boolean) => + execInPage( + frame, + collectSnapshot, + [frameStartingElementId, null, markNew, state.generation, forgetIssuedRefs], + false, + notAfter + ) + let frameSnapshot = await captureFrame(false) if (!stillCurrent()) { throw new ToolError('The tab changed while its frames were being captured. Try again.') } @@ -2578,10 +2588,25 @@ async function captureSnapshot( unreadableCrossOriginFrames++ continue } - const frameRefs = validateSnapshotRefs(frameSnapshot, frameStartingElementId, (ref) => - samePageTarget(state.issuedRefTargets.get(ref), frame) - ) + const validateFrame = (snapshot: Record) => + validateSnapshotRefs(snapshot, frameStartingElementId, (ref) => + samePageTarget(state.issuedRefTargets.get(ref), frame) + ) + let frameRefs = validateFrame(frameSnapshot) if (!frameRefs) { + frameSnapshot = await captureFrame(true) + if (!stillCurrent()) { + throw new ToolError('The tab changed while its frames were being captured. Try again.') + } + if (frame.isDestroyed() || frame.detached || frame.url !== frameUrl) { + unreadableCrossOriginFrames++ + continue + } + if (isRecordLike(frameSnapshot) && Array.isArray(frameSnapshot.refIds)) { + frameRefs = validateFrame(frameSnapshot) + } + } + if (!frameRefs || !isRecordLike(frameSnapshot) || typeof frameSnapshot.outline !== 'string') { unreadableCrossOriginFrames++ continue } 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 c82eb044edc..2c394eba4b2 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -117,6 +117,7 @@ beforeEach(() => { window.__simAgentMutationStates = undefined window.__simAgentNextElementId = 0 window.__simAgentShownElements = undefined + window.__simAgentShownPending = undefined installPageHelpers() window.__simAgentResolveElement = undefined installDomShims() @@ -542,6 +543,19 @@ describe('collectSnapshot', () => { expect(outlineOf(collectSnapshot())).not.toContain(' new') }) + it('keeps new markers for a recapture that replaces a discarded capture', () => { + document.body.innerHTML = '' + visible(document.querySelector('button') as HTMLButtonElement) + collectSnapshot() + const added = document.createElement('button') + added.textContent = 'Send' + document.body.append(visible(added)) + collectSnapshot() + + const lines = outlineOf(collectSnapshot(0, null, true, 0, true)).split('\n') + expect(lines.find((line) => line.includes('"Send"'))).toMatch(/ new$/) + }) + it('leaves new markers out of an unmarked read without consuming them', () => { document.body.innerHTML = '' visible(document.querySelector('button') as HTMLButtonElement) @@ -814,6 +828,28 @@ describe('collectSnapshot', () => { expect(refFor(outlineOf(collectSnapshot()), 'Delete')).not.toBe(first) }) + it('refuses an earlier ref once a later snapshot reissued its recycled element', () => { + document.body.innerHTML = '
Order 1
' + for (const el of Array.from(document.querySelectorAll('span, button'))) visible(el) + const first = refFor(outlineOf(collectSnapshot()), 'Archive') + ;(document.querySelector('span') as HTMLSpanElement).textContent = 'Order 2' + const second = refFor(outlineOf(collectSnapshot()), 'Archive') + collectSnapshot(500, second) + + expect(second).not.toBe(first) + expect(clickElement(first)).toMatchObject({ error: 'stale' }) + }) + + it('does not take an icon-only control as revealing content when its label grows', () => { + document.body.innerHTML = '' + const button = visible(document.querySelector('button') as HTMLButtonElement) + const ref = refFor(outlineOf(collectSnapshot()), 'Remove') + + button.setAttribute('aria-label', 'Remove member') + + expect(clickElement(ref)).toMatchObject({ error: 'stale' }) + }) + it('invalidates a connected ref whose name changed to a longer, different word', () => { document.body.innerHTML = '
Item 1
' const row = visible(document.querySelector('[role="listitem"]') 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 794750a52f6..896163677c1 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -65,6 +65,8 @@ declare global { __simAgentRefOfElement?: WeakMap /** Elements an earlier snapshot of this document listed; a later snapshot marks the rest `new`. */ __simAgentShownElements?: WeakSet + /** Elements the latest capture listed, committed to the shown set by the next capture that keeps them. */ + __simAgentShownPending?: Element[] /** Installed by {@link installPageHelpers} before every page function; cached per call. */ __simAgentIsExemptModal: (element: Element) => boolean /** Installed by {@link installPageHelpers}: the element's id in the current snapshot. */ @@ -231,7 +233,8 @@ export function collectSnapshot( startingElementId = 0, elementId: number | null = null, markNew = true, - refNamespace = 0 + refNamespace = 0, + forgetIssuedRefs = false ): unknown { const resolver = window.__simAgentResolveElement const scopedRoot = @@ -331,14 +334,22 @@ export function collectSnapshot( const refLineIndexes: Record = {} const locators: SnapshotRefLocator[] = [] window.__simAgentElements = registry - const previouslyShown = window.__simAgentShownElements - const shown = previouslyShown ?? new WeakSet() - if (markNew) window.__simAgentShownElements = shown + // A capture the driver discards and recaptures must not use up `new` + // markers, so its listed elements commit only when the next capture keeps them. + const pendingShown = window.__simAgentShownPending + if (markNew && pendingShown && !forgetIssuedRefs) { + const committed = window.__simAgentShownElements ?? new WeakSet() + for (const el of pendingShown) committed.add(el) + window.__simAgentShownElements = committed + } + const shown = window.__simAgentShownElements + const listed: Element[] = [] + if (markNew) window.__simAgentShownPending = listed /** Whether no earlier snapshot of this document listed the element; the first snapshot marks nothing. */ - const isNew = (el: Element): boolean => markNew && previouslyShown !== undefined && !shown.has(el) + const isNew = (el: Element): boolean => markNew && shown !== 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) + if (markNew) listed.push(el) } const lines: string[] = [] let truncated = false @@ -603,9 +614,11 @@ export function collectSnapshot( // An unchanged element keeps its ref, and a ref stays resolvable after a // later snapshot leaves it out; a changed element gets a new ref. - const issuedRefs = window.__simAgentIssuedRefs ?? new Map() + // The driver asks the page to forget when it cannot vouch for a reused id + // (a capture it never committed handed it out), so every element gets a new one. + const issuedRefs = (!forgetIssuedRefs && window.__simAgentIssuedRefs) || new Map() window.__simAgentIssuedRefs = issuedRefs - const refOfElement = window.__simAgentRefOfElement ?? new WeakMap() + const refOfElement = (!forgetIssuedRefs && window.__simAgentRefOfElement) || new WeakMap() window.__simAgentRefOfElement = refOfElement const issuedRefCap = 5000 const textHashOf = (el: Element): number => { @@ -958,8 +971,10 @@ export function collectSnapshot( // was revealed (its DOM text is unchanged) or it declares a popup. A // recycled row changes its DOM text with neither. const name = nameFor(candidate) - const opensPopup = - textHashOf(candidate) === locator.textHash || + const revealedContent = (): boolean => + Boolean(candidate.textContent?.trim()) && textHashOf(candidate) === locator.textHash + const opensPopup = (): boolean => + revealedContent() || ['aria-haspopup', 'aria-expanded', 'aria-controls'].some( (attribute) => (candidate.getAttribute(attribute) || 'false') !== 'false' ) || @@ -968,10 +983,11 @@ export function collectSnapshot( ) !== null const nameGrewInPlace = connected && - opensPopup && + name !== locator.name && locator.name.length > 0 && name.startsWith(locator.name) && - (locator.name.length >= 120 || /\s/.test(name.charAt(locator.name.length))) + (locator.name.length >= 120 || /\s/.test(name.charAt(locator.name.length))) && + opensPopup() if (name !== locator.name && !nameGrewInPlace) return false const candidateAttributes = locatorAttributes(candidate) const attributes = connected ? connectedIdentityAttributes : stableAttributes @@ -1052,6 +1068,12 @@ export function collectSnapshot( } const current = registry[id] ?? issued?.element.deref() + // An earlier ref whose element has since been issued a new ref (it changed + // identity in between, e.g. a recycled row) no longer names it. + if (!inLatestSnapshot && current && refOfElement.get(current) !== id) { + window.__simAgentStaleReason = 'the element was given a new ref by a later snapshot' + return null + } if (current?.isConnected) { if (!identityMatches(current, true)) { window.__simAgentStaleReason = `the node is still mounted but its identity drifted (now <${String(current.tagName || '').toLowerCase()}> role=${roleFor(current) || 'none'} name="${nameFor(current).slice(0, 60)}")`