diff --git a/apps/desktop/e2e/browser-tools.spec.ts b/apps/desktop/e2e/browser-tools.spec.ts index 92e5e18d429..bf3d323751f 100644 --- a/apps/desktop/e2e/browser-tools.spec.ts +++ b/apps/desktop/e2e/browser-tools.spec.ts @@ -1365,7 +1365,7 @@ test.describe('browser tools', () => { expect(await formState()).toMatchObject({ name: 'Observed value' }) const result = fill.result as { observation: { result: { matches: { elementId: number }[] } } } const freshId = result.observation.result.matches[0].elementId - expect(freshId).not.toBe(ref('Name')) + expect(freshId).toBe(ref('Name')) const clear = await execute('browser_type', { elementId: freshId, text: '', observe: {} }) expect(clear.ok, clear.error).toBe(true) expect(clear.result).toMatchObject({ observation: { ok: true } }) diff --git a/apps/desktop/src/main/browser-agent/driver.test.ts b/apps/desktop/src/main/browser-agent/driver.test.ts index fccd280a9ec..2bac3457ae9 100644 --- a/apps/desktop/src/main/browser-agent/driver.test.ts +++ b/apps/desktop/src/main/browser-agent/driver.test.ts @@ -3098,40 +3098,120 @@ describe('credential protection', () => { }) }) + it('accepts a later snapshot that keeps refs this scope issued for the same page', 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', {}) + respondWith(contents, { collectSnapshot: snapshot([4, 6], 7) }) + + const kept = await driver.executeTool('chat-test', 'browser_snapshot', {}) + expect(kept).toMatchObject({ ok: true }) + respondWith(contents, { collectSnapshot: snapshot([2], 7) }) + + const forged = await driver.executeTool('chat-test', 'browser_snapshot', {}) + expect(forged).toMatchObject({ ok: false }) + }) + + it("forgets a page's oldest refs once its history passes the retention bound", 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, + }) + let next = 0 + for (let round = 0; round < 19; round++) { + const refIds = Array.from({ length: 300 }, (_, index) => next + index) + next += 300 + respondWith(contents, { collectSnapshot: snapshot(refIds, next) }) + expect(await driver.executeTool('chat-test', 'browser_snapshot', {})).toMatchObject({ + ok: true, + }) + } + respondWith(contents, { collectSnapshot: snapshot([next - 1], next) }) + expect(await driver.executeTool('chat-test', 'browser_snapshot', {})).toMatchObject({ + ok: true, + }) + respondWith(contents, { collectSnapshot: snapshot([0], next) }) + + expect(await driver.executeTool('chat-test', 'browser_snapshot', {})).toMatchObject({ + ok: false, + }) + }) + + it('acts on a ref an earlier snapshot of the same page issued', async () => { + const contents = await openPage() + respondWith(contents, { + collectSnapshot: { + url: 'https://example.com/', + title: 'Example', + outline: '- button "Pins" [ref=4]\n- button "Files" [ref=5]', + truncated: false, + refIds: [4, 5], + refLineIndexes: { 4: 0, 5: 1 }, + nextElementId: 6, + }, + }) + await driver.executeTool('chat-test', 'browser_snapshot', {}) + respondWith(contents, { + collectSnapshot: { + url: 'https://example.com/', + title: 'Example', + outline: '- dialog "Pick a time" [ref=6]', + truncated: false, + refIds: [6], + refLineIndexes: { 6: 0 }, + nextElementId: 7, + }, + }) + await driver.executeTool('chat-test', 'browser_find', { query: 'time' }) + + const clicked = await driver.executeTool('chat-test', 'browser_click', { elementId: 4 }) + expect(clicked).toMatchObject({ ok: true, result: { dispatched: true } }) + }) + it.each([ ['browser_snapshot', 'true'], ['browser_find', 'false'], - ] as const)( - 'passes the current root ref to %s and invalidates previous refs', - async (tool, markNew) => { - const contents = await openPage() - respondWith(contents, { - collectSnapshot: { - url: 'https://example.com/login', - title: 'Scoped', - scoped: true, - outline: '- button "Save" [ref=1]', - truncated: false, - refIds: [1], - refLineIndexes: { 1: 0 }, - nextElementId: 2, - }, - }) - const result = await driver.executeTool('chat-test', tool, { elementId: 0, query: 'Save' }) - expect(result).toMatchObject({ ok: true, result: { scoped: true } }) - expect( - vi - .mocked(contents.executeJavaScript) - .mock.calls.some( - ([expression]) => - isPageCall(expression, 'collectSnapshot') && - expression.includes(`.apply(null, [1,0,${markNew}])`) - ) - ).toBe(true) - const stale = await driver.executeTool('chat-test', 'browser_click', { elementId: 0 }) - expect(stale).toMatchObject({ ok: false }) - } - ) + ] as const)('passes the current root ref to %s', async (tool, markNew) => { + const contents = await openPage() + respondWith(contents, { + collectSnapshot: { + url: 'https://example.com/login', + title: 'Scoped', + scoped: true, + outline: '- button "Save" [ref=1]', + truncated: false, + refIds: [1], + refLineIndexes: { 1: 0 }, + nextElementId: 2, + }, + }) + const result = await driver.executeTool('chat-test', tool, { elementId: 0, query: 'Save' }) + expect(result).toMatchObject({ ok: true, result: { scoped: true } }) + expect( + vi + .mocked(contents.executeJavaScript) + .mock.calls.some( + ([expression]) => + isPageCall(expression, 'collectSnapshot') && + expression.includes(`.apply(null, [1,0,${markNew},`) + ) + ).toBe(true) + }) it.each([ { kind: 'input:checkbox', checked: true, disabled: false, readOnly: false }, diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index d0ada80bf5f..da2a3b8918e 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -88,6 +88,7 @@ import { } from '@/main/browser-agent/page-prompts' import { isPanelVisible, panelWindow } from '@/main/browser-agent/panel' import { + observeRequest, withFailedPostActionObservation, withPostActionObservation, } from '@/main/browser-agent/post-action-observation' @@ -144,6 +145,24 @@ function isBrowserWaitElementState(value: string): value is BrowserWaitElementSt type PageExecutionTarget = WebContents | WebFrameMain +/** + * Refs remembered per page or frame. Above the page's own 5000-ref store by + * one snapshot's 300 refs, and refreshed by the same snapshots, so the driver + * never forgets a ref the page can still hand back. + */ +const ISSUED_REFS_PER_TARGET = 5300 + +/** Whether two targets are the same page or frame; Electron may hand out separate wrappers for one frame. */ +function samePageTarget( + left: PageExecutionTarget | undefined, + right: PageExecutionTarget +): boolean { + if (left === right) return true + return ( + left !== undefined && 'routingId' in left && 'routingId' in right && sameWebFrame(left, right) + ) +} + type BrowserActionOutcome = { status: 'pending' } | { status: 'acknowledged'; result: unknown } type FormField = @@ -326,6 +345,10 @@ interface DriverScopeState { snapshotTargets: Map /** Monotonic ref floor shared by every frame in this browser scope. */ nextElementRefId: number + /** Every ref this scope issued and its frame: a later snapshot may reuse it there, and an action may still name it. */ + issuedRefTargets: Map + /** The same refs per page or frame, oldest first, so each page's history stays bounded. */ + issuedRefsByTarget: Map> /** Invalidates captures that finish after a tab/navigation/timeout race. */ snapshotCaptureEpoch: number /** Cancels a timed-out multi-step action before any later native dispatch. */ @@ -367,6 +390,8 @@ function createDriverScopeState(): DriverScopeState { snapshotTabId: null, snapshotTargets: new Map(), nextElementRefId: 0, + issuedRefTargets: new Map(), + issuedRefsByTarget: new Map(), snapshotCaptureEpoch: 0, toolExecutionEpoch: 0, } @@ -1766,9 +1791,44 @@ function requireSnapshotForElementAction(): void { ) } +function rememberIssuedRef( + state: DriverScopeState, + ref: number, + target: PageExecutionTarget +): void { + const previous = state.issuedRefTargets.get(ref) + if (previous && previous !== target) state.issuedRefsByTarget.get(previous)?.delete(ref) + state.issuedRefTargets.set(ref, target) + const refs = state.issuedRefsByTarget.get(target) ?? new Set() + state.issuedRefsByTarget.set(target, refs) + refs.delete(ref) + refs.add(ref) + const oldest = refs.values().next() + if (refs.size > ISSUED_REFS_PER_TARGET && !oldest.done) { + refs.delete(oldest.value) + state.issuedRefTargets.delete(oldest.value) + } +} + +/** + * The frame that owns a ref: the latest snapshot's, or for a ref an earlier + * snapshot issued, its frame while that frame still belongs to the tab the + * latest snapshot covered. The page then confirms the element itself. + */ +function elementTarget(elementId: number): PageExecutionTarget | undefined { + const state = driverScopeState() + const latest = state.snapshotTargets.get(elementId) + if (latest) return latest + const issued = state.issuedRefTargets.get(elementId) + const contents = session.automationTab()?.view.webContents + if (!issued || !contents || contents.isDestroyed()) return undefined + if (!('routingId' in issued)) return issued === contents ? contents : undefined + return liveFrameInTab(contents, issued) +} + function pageTargetForElement(elementId: number): PageExecutionTarget { requireSnapshotForElementAction() - const target = driverScopeState().snapshotTargets.get(elementId) + const target = elementTarget(elementId) if (!target || ('isDestroyed' in target && target.isDestroyed())) { throw new ToolError( 'That element id is not present in the current snapshot. Call browser_snapshot and use a visible ref from that result.' @@ -1865,7 +1925,7 @@ function assertElementActionCurrent( assertActiveContents(contents) const state = driverScopeState() const active = session.automationTab() - if (state.snapshotTabId !== active?.id || state.snapshotTargets.get(elementId) !== target) { + if (state.snapshotTabId !== active?.id || !samePageTarget(elementTarget(elementId), target)) { throw new ToolError( 'The page changed before input could be dispatched. Take a fresh browser_snapshot and try again.' ) @@ -1877,6 +1937,13 @@ function focusedPageTarget(contents: WebContents): PageExecutionTarget { return focused && focused !== contents.mainFrame && !focused.isDestroyed() ? focused : contents } +/** The tab's current wrapper for a frame, if the frame is still live in that tab. */ +function liveFrameInTab(contents: WebContents, frame: WebFrameMain): WebFrameMain | undefined { + return contents.mainFrame.framesInSubtree.find( + (candidate) => !candidate.isDestroyed() && sameWebFrame(candidate, frame) + ) +} + function sameWebFrame(left: WebFrameMain, right: WebFrameMain): boolean { if (left === right) return true if (Number.isSafeInteger(left.frameTreeNodeId) && Number.isSafeInteger(right.frameTreeNodeId)) { @@ -1899,11 +1966,7 @@ function assertFocusedTargetUnchanged( expectedFrameNavigationEpoch?: number ): void { const focused = focusedPageTarget(contents) - const unchanged = - target === contents - ? focused === contents - : focused !== contents && sameWebFrame(focused as WebFrameMain, target as WebFrameMain) - if (!unchanged) { + if (!samePageTarget(focused, target)) { throw new ToolError( 'Focus moved to a different page or frame before the keystroke. No retry was attempted; inspect the page first.' ) @@ -1915,7 +1978,7 @@ function assertFocusedTargetUnchanged( frame.detached || (expectedFrameNavigationEpoch !== undefined && frameNavigationEpoch(contents, frame) !== expectedFrameNavigationEpoch) || - !contents.mainFrame.framesInSubtree.some((candidate) => sameWebFrame(candidate, frame)) + !liveFrameInTab(contents, frame) ) { throw new ToolError( 'The focused frame navigated or was replaced before the keystroke. Inspect the page before continuing.' @@ -2338,7 +2401,8 @@ async function readWholePageText( function validateSnapshotRefs( snapshot: Record, - startingElementId: number + startingElementId: number, + issuedHere: (ref: number) => boolean ): { refs: number[]; nextElementId: number; lineIndexes: Map } | null { if (typeof snapshot.outline !== 'string' || snapshot.outline.length > 500_000) return null if (!Array.isArray(snapshot.refIds) || snapshot.refIds.length > 300) return null @@ -2362,7 +2426,7 @@ function validateSnapshotRefs( if ( typeof value !== 'number' || !Number.isSafeInteger(value) || - value < startingElementId || + (value < startingElementId && !issuedHere(value)) || value >= nextElementId || unique.has(value) ) { @@ -2436,7 +2500,7 @@ async function captureSnapshot( await execInPage( contents, collectSnapshot, - [mainStartingElementId, elementId ?? null, markNew], + [mainStartingElementId, elementId ?? null, markNew, state.generation], false, notAfter ) @@ -2451,7 +2515,9 @@ async function captureSnapshot( ) { throw new ToolError('The page returned an incomplete snapshot. Try browser_snapshot again.') } - const mainRefs = validateSnapshotRefs(mainSnapshot, mainStartingElementId) + const mainRefs = validateSnapshotRefs(mainSnapshot, mainStartingElementId, (ref) => + samePageTarget(state.issuedRefTargets.get(ref), contents) + ) if (!mainRefs) { throw new ToolError('The page returned invalid element ids. Try browser_snapshot again.') } @@ -2493,7 +2559,7 @@ async function captureSnapshot( const frameSnapshot = await execInPage( frame, collectSnapshot, - [frameStartingElementId, null, markNew], + [frameStartingElementId, null, markNew, state.generation], false, notAfter ) @@ -2512,7 +2578,9 @@ async function captureSnapshot( unreadableCrossOriginFrames++ continue } - const frameRefs = validateSnapshotRefs(frameSnapshot, frameStartingElementId) + const frameRefs = validateSnapshotRefs(frameSnapshot, frameStartingElementId, (ref) => + samePageTarget(state.issuedRefTargets.get(ref), frame) + ) if (!frameRefs) { unreadableCrossOriginFrames++ continue @@ -2565,6 +2633,12 @@ async function captureSnapshot( (targetLineIndexes.get(elementId) ?? Number.POSITIVE_INFINITY) < visibleOutlineLines.length ) ) + for (const [target, refs] of state.issuedRefsByTarget) { + if (!('isDestroyed' in target && target.isDestroyed())) continue + for (const ref of refs) state.issuedRefTargets.delete(ref) + state.issuedRefsByTarget.delete(target) + } + for (const [ref, target] of targets) rememberIssuedRef(state, ref, target) state.snapshotTabId = capturedTabId return { ...omit(mainSnapshot, ['refIds', 'refLineIndexes', 'nextElementId']), @@ -4042,6 +4116,11 @@ async function executeToolInner( }, } const notes: string[] = [] + if (typeof initialSurface.coveredBy === 'string') { + notes.push( + `The field is covered by ${initialSurface.coveredBy}, so the text went in through keyboard focus.` + ) + } if (submitNote) notes.push(submitNote) if (!combinedObservation.effectObserved) { notes.push( @@ -5347,7 +5426,9 @@ export async function executeTool( actionOutcome = outcome } ) - if (params.observe !== undefined) actionOutcome = { status: 'acknowledged', result } + if (observeRequest(tool, params) !== undefined) { + actionOutcome = { status: 'acknowledged', result } + } return result }, (query) => @@ -5372,7 +5453,7 @@ export async function executeTool( tool === 'browser_snapshot' || tool === 'browser_open_url' || tool === 'browser_find' || - params.observe !== undefined + observeRequest(tool, params) !== undefined ) { invalidateSnapshot(state) } @@ -5382,7 +5463,7 @@ export async function executeTool( observedResult = await guardedExecution } catch (error) { if (!actionOutcome) { - if (params.observe !== undefined) cancelPendingSnapshotCapture(state) + if (observeRequest(tool, params) !== undefined) cancelPendingSnapshotCapture(state) throw error } invalidateSnapshot(state) 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 c9b6ef73e33..7f30104ff9d 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -334,8 +334,8 @@ describe('combobox typing surfaces', () => { expect(input.value).toBe('Mondu') }) - it('refuses mixed or unrelated blockers instead of treating them as suggestions', () => { - const { option } = composeField() + it('does not treat mixed or unrelated blockers as suggestions, and types through keyboard focus', () => { + const { input, option } = composeField() const overlay = visible(document.createElement('div')) overlay.setAttribute('aria-label', 'Unrelated overlay') document.body.append(overlay) @@ -344,11 +344,53 @@ describe('combobox typing surfaces', () => { value: (x: number) => (x > 70 ? overlay : option), }) - expect(focusElementForTyping(0)).toEqual({ - error: 'obstructed', - blocker: 'Mondu', - blockerControls: [], + expect(focusElementForTyping(0)).toMatchObject({ + focused: true, + coveredBy: 'Mondu', + coveredByRelatedPopup: false, }) + expect(document.activeElement).toBe(input) + }) + + it('types into a top-page field a promo card covers, naming the cover', () => { + document.body.innerHTML = + '
Shop for your home
' + const input = visible(document.querySelector('input') as HTMLInputElement) + const card = visible(document.querySelector('div') as HTMLDivElement) + register(input) + Object.defineProperty(document, 'elementFromPoint', { configurable: true, value: () => card }) + + expect(focusElementForTyping(0)).toMatchObject({ + focused: true, + coveredBy: 'Shop for your home', + }) + expect(typeIntoElement(0, 'cooking pot', false)).toMatchObject({ dispatched: true }) + expect(input.value).toBe('cooking pot') + }) + + it('still refuses a top-page field a modal dialog covers', () => { + document.body.innerHTML = + '

Sign in

' + const input = visible(document.querySelector('input') as HTMLInputElement) + const cover = visible(document.querySelector('p') as HTMLParagraphElement) + register(input) + Object.defineProperty(document, 'elementFromPoint', { configurable: true, value: () => cover }) + + expect(focusElementForTyping(0)).toMatchObject({ error: 'obstructed' }) + }) + + it('still refuses a covered field inside a frame', () => { + const frame = document.createElement('iframe') + document.body.append(frame) + const inner = frame.contentDocument as Document + inner.body.innerHTML = '
Cover
' + const field = visible(inner.querySelector('input') as HTMLInputElement) + const cover = visible(inner.querySelector('div') as HTMLDivElement) + field.scrollIntoView = () => {} + register(field) + Object.defineProperty(inner, 'elementFromPoint', { configurable: true, value: () => cover }) + + expect(focusElementForTyping(0)).toMatchObject({ error: 'obstructed', blocker: 'Cover' }) }) it('refuses ambiguous composite fields and descendant passwords', () => { @@ -673,16 +715,119 @@ describe('collectSnapshot', () => { expect(clickElement(ref)).toMatchObject({ error: 'stale' }) }) - it('never recycles numeric refs across snapshots', () => { + it('keeps a connected ref whose name grew when its own submenu opened', () => { + document.body.innerHTML = + '
Products ▾
    ' + const menu = visible(document.querySelector('[role="button"]') as HTMLDivElement) + const ref = refFor(outlineOf(collectSnapshot()), 'Products ▾') + + ;(menu.querySelector('ul') as HTMLUListElement).innerHTML = + '
  • Starter
  • Business
  • ' + + expect(clickElement(ref)).toMatchObject({ dispatched: true }) + }) + + it('acts on a text line whose accessible name differs from its rendered text', () => { + document.body.innerHTML = '

    i

    ' + visible(document.querySelector('span') as HTMLSpanElement) + const ref = refFor(outlineOf(collectSnapshot()), 'i') + + expect(clickElement(ref)).toMatchObject({ dispatched: true }) + }) + + it('acts on a text line longer than the identity name budget', () => { + const text = 'Terms of service '.repeat(10).trim() + document.body.innerHTML = `

    ${text}

    ` + visible(document.querySelector('p') as HTMLParagraphElement) + const ref = refFor(outlineOf(collectSnapshot()), text.slice(0, 160)) + + expect(clickElement(ref)).toMatchObject({ dispatched: true }) + }) + + it('invalidates a recycled row whose new name merely extends the old one', () => { + document.body.innerHTML = '
    Alice
    ' + const row = visible(document.querySelector('[role="listitem"]') as HTMLDivElement) + const ref = refFor(outlineOf(collectSnapshot()), 'Alice') + + row.textContent = 'Alice Smith' + + expect(clickElement(ref)).toMatchObject({ error: 'stale' }) + }) + + it('gives a row control a new ref when its row now shows a different record', () => { + document.body.innerHTML = '
    Order 1
    ' + for (const el of Array.from(document.querySelectorAll('span, button'))) visible(el) + const first = refFor(outlineOf(collectSnapshot()), 'Delete') + ;(document.querySelector('span') as HTMLSpanElement).textContent = 'Order 2' + + expect(refFor(outlineOf(collectSnapshot()), 'Delete')).not.toBe(first) + }) + + 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) + const ref = refFor(outlineOf(collectSnapshot()), 'Item 1') + + row.textContent = 'Item 12' + + expect(clickElement(ref)).toMatchObject({ error: 'stale' }) + }) + + it('keeps an unchanged element on the same ref across snapshots', () => { + document.body.innerHTML = '' + for (const button of Array.from(document.querySelectorAll('button'))) visible(button) + const first = outlineOf(collectSnapshot()) + const second = outlineOf(collectSnapshot()) + + expect(refFor(second, 'Pins')).toBe(refFor(first, 'Pins')) + expect(refFor(second, 'Files')).toBe(refFor(first, 'Files')) + expect(clickElement(refFor(first, 'Pins'))).toMatchObject({ dispatched: true }) + }) + + it('acts on a ref from an earlier snapshot that a later scoped snapshot left out', () => { + document.body.innerHTML = '
    ' + for (const button of Array.from(document.querySelectorAll('button'))) visible(button) + const pins = refFor(outlineOf(collectSnapshot()), 'Pins') + const section = refFor(outlineOf(collectSnapshot()), 'Files') + collectSnapshot(100, section) + + expect(clickElement(pins)).toMatchObject({ dispatched: true }) + }) + + it('refuses a ref from an earlier snapshot once its element left the DOM', () => { + document.body.innerHTML = '' + for (const button of Array.from(document.querySelectorAll('button'))) visible(button) + const pins = refFor(outlineOf(collectSnapshot()), 'Pins') + ;(document.querySelector('button') as HTMLButtonElement).remove() + collectSnapshot() + + expect(clickElement(pins)).toMatchObject({ error: 'stale' }) + }) + + it('never gives a ref to a different element across snapshots', () => { document.body.innerHTML = '' - visible(document.querySelector('button') as HTMLButtonElement) + const original = visible(document.querySelector('button') as HTMLButtonElement) const firstRef = refFor(outlineOf(collectSnapshot()), 'Pins') - const secondRef = refFor(outlineOf(collectSnapshot()), 'Pins') + original.replaceWith(visible(document.createElement('button'))) + ;(document.querySelector('button') as HTMLButtonElement).textContent = 'Files' + const secondRef = refFor(outlineOf(collectSnapshot()), 'Files') expect(secondRef).toBeGreaterThan(firstRef) expect(clickElement(firstRef)).toMatchObject({ error: 'stale' }) expect(clickElement(secondRef)).toMatchObject({ dispatched: true }) }) + + it('gives a recycled row a new ref when its identity changed in place', () => { + document.body.innerHTML = '
    eng-bugs
    ' + const row = visible(document.querySelector('[role="listitem"]') as HTMLDivElement) + const firstRef = refFor(outlineOf(collectSnapshot()), 'eng-bugs') + row.textContent = 'random' + row.dataset.key = 'channel-2' + const secondRef = refFor(outlineOf(collectSnapshot()), 'random') + + expect(secondRef).not.toBe(firstRef) + expect(clickElement(firstRef)).toMatchObject({ error: 'stale' }) + }) }) describe('semantic control state', () => { diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index 3cfa32e0a8a..1fe3dc93da3 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -28,6 +28,19 @@ * the keystroke refusal that read it. */ +/** What a snapshot recorded about an element, so a later action can confirm it is the same one. */ +interface SnapshotRefLocator { + url: string + tag: string + role: string + name: string + attributes: Record + ancestor: string + context: string + /** Hash of the element's DOM text, to tell revealed content from replaced content. */ + textHash: number +} + declare global { interface Window { __simAgentElements?: Element[] @@ -43,6 +56,13 @@ declare global { observedRoots: WeakSet }> __simAgentNextElementId?: number + /** Every ref this document issued, resolvable while its element keeps that identity. */ + __simAgentIssuedRefs?: Map< + number, + { element: WeakRef; locator: SnapshotRefLocator; namespace: number } + > + /** The ref each element was last issued, so an unchanged element keeps it. */ + __simAgentRefOfElement?: WeakMap /** Elements an earlier snapshot of this document listed; a later snapshot marks the rest `new`. */ __simAgentShownElements?: WeakSet /** Installed by {@link installPageHelpers} before every page function; cached per call. */ @@ -210,7 +230,8 @@ export function installPageHelpers(): void { export function collectSnapshot( startingElementId = 0, elementId: number | null = null, - markNew = true + markNew = true, + refNamespace = 0 ): unknown { const resolver = window.__simAgentResolveElement const scopedRoot = @@ -308,15 +329,7 @@ export function collectSnapshot( const registry: Element[] = [] const refLineIndexes: Record = {} - const locators: Array<{ - url: string - tag: string - role: string - name: string - attributes: Record - ancestor: string - context: string - }> = [] + const locators: SnapshotRefLocator[] = [] window.__simAgentElements = registry const previouslyShown = window.__simAgentShownElements const shown = previouslyShown ?? new WeakSet() @@ -588,10 +601,23 @@ export function collectSnapshot( return '' } + // 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() + window.__simAgentIssuedRefs = issuedRefs + const refOfElement = window.__simAgentRefOfElement ?? new WeakMap() + window.__simAgentRefOfElement = refOfElement + const issuedRefCap = 5000 + const textHashOf = (el: Element): number => { + const text = el.textContent || '' + let hash = 0 + for (let index = 0; index < text.length; index++) { + hash = (hash * 31 + text.charCodeAt(index)) | 0 + } + return hash + } const registerElement = (el: Element, role: string, name: string): number => { - const id = nextElementId++ - registry[id] = el - locators[id] = { + const locator = { url: pageUrlFor(el), tag: el.tagName.toUpperCase(), role, @@ -599,7 +625,32 @@ export function collectSnapshot( attributes: locatorAttributes(el), ancestor: ancestorSignature(el), context: contextSignature(el), - } + textHash: textHashOf(el), + } + const priorId = refOfElement.get(el) + const prior = priorId === undefined ? undefined : issuedRefs.get(priorId) + const unchanged = + prior !== undefined && + prior.namespace === refNamespace && + prior.locator.tag === locator.tag && + prior.locator.role === locator.role && + prior.locator.name === locator.name && + prior.locator.ancestor === locator.ancestor && + prior.locator.context === locator.context && + Object.keys({ ...prior.locator.attributes, ...locator.attributes }).every( + (key) => prior.locator.attributes[key] === locator.attributes[key] + ) + const id = + unchanged && priorId !== undefined && registry[priorId] === undefined + ? priorId + : nextElementId++ + refOfElement.set(el, id) + registry[id] = el + locators[id] = locator + issuedRefs.delete(id) + issuedRefs.set(id, { element: new WeakRef(el), locator, namespace: refNamespace }) + const oldest = issuedRefs.keys().next() + if (issuedRefs.size > issuedRefCap && !oldest.done) issuedRefs.delete(oldest.value) refCount++ window.__simAgentNextElementId = nextElementId return id @@ -691,7 +742,8 @@ export function collectSnapshot( 160 ) if (!text) return - const id = registerElement(el, roleFor(el), text) + // The resolver recomputes nameFor, which differs from the shown text for aria-labelled or long leaves. + const id = registerElement(el, roleFor(el), nameFor(el)) textLineCount++ const marker = isNew(el) ? ' new' : '' const lineIndex = lines.length @@ -850,11 +902,15 @@ export function collectSnapshot( * real stale ref, never permission to click something nearby. */ window.__simAgentResolveElement = (id: number, allowRecovery = true) => { - if (scopedRoot && !scopedRoot.isConnected) { + // A ref from an earlier snapshot resolves only to its own live node: no + // lookalike recovery, and the latest snapshot's scope does not apply to it. + const inLatestSnapshot = locators[id] !== undefined + if (inLatestSnapshot && scopedRoot && !scopedRoot.isConnected) { window.__simAgentStaleReason = 'the scoped snapshot root left the DOM' return null } - const locator = locators[id] + const issued = issuedRefs.get(id) + const locator = issued?.locator if (!locator) { window.__simAgentStaleReason = `id ${id} is not in the current snapshot's registry` return null @@ -897,7 +953,26 @@ export function collectSnapshot( ) { return false } - if (nameFor(candidate) !== locator.name) return false + // A mounted control whose own menu opened (often by the hover or click + // being validated) appends that menu to its name: either hidden content + // 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 || + ['aria-haspopup', 'aria-expanded', 'aria-controls'].some( + (attribute) => (candidate.getAttribute(attribute) || 'false') !== 'false' + ) || + candidate.querySelector( + '[role="menu"], [role="menubar"], [role="listbox"], [role="tooltip"], [role="dialog"], [role="tree"]' + ) !== null + const nameGrewInPlace = + connected && + opensPopup && + locator.name.length > 0 && + name.startsWith(locator.name) && + (locator.name.length >= 120 || /\s/.test(name.charAt(locator.name.length))) + if (name !== locator.name && !nameGrewInPlace) return false const candidateAttributes = locatorAttributes(candidate) const attributes = connected ? connectedIdentityAttributes : stableAttributes const genericName = @@ -976,7 +1051,7 @@ export function collectSnapshot( return true } - const current = registry[id] + const current = registry[id] ?? issued?.element.deref() 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)}")` @@ -988,7 +1063,7 @@ export function collectSnapshot( } } - if (!allowRecovery) { + if (!allowRecovery || !inLatestSnapshot) { window.__simAgentStaleReason = 'the original snapshot node is detached or hidden' return null } @@ -1866,6 +1941,33 @@ export function focusElementForTyping(id: number, moveFocus = true): unknown { coveredByRelatedPopup = true } } + // Top-page typing goes through keyboard focus, so a non-modal overlay does not block it; a modal + // or dialog cover still refuses, and framed fields keep the check for their mapped point. + const topPageField = view === window && window.top === window + const insideModal = (node: Element | null): boolean => { + for (let current = node; current; current = composedParent(current)) { + const role = current.getAttribute('role') + if ( + current.getAttribute('aria-modal') === 'true' || + role === 'dialog' || + role === 'alertdialog' || + tagFor(current) === 'DIALOG' + ) { + return true + } + } + return false + } + let coveredBy: string | undefined + if ( + !chosenPoint && + topPageField && + blockedPoints.length > 0 && + !blockedPoints.some((point) => insideModal(point.hit)) + ) { + chosenPoint = { x: blockedPoints[0].x, y: blockedPoints[0].y } + coveredBy = blockerLabel(firstBlocker) + } if (!chosenPoint) { return { error: 'obstructed', @@ -1876,6 +1978,7 @@ export function focusElementForTyping(id: number, moveFocus = true): unknown { return { focused: true, + ...(coveredBy ? { coveredBy } : {}), kind: editableTag === 'INPUT' ? 'input' diff --git a/apps/desktop/src/main/browser-agent/post-action-observation.test.ts b/apps/desktop/src/main/browser-agent/post-action-observation.test.ts index 3938df6c7fb..cff8f98322a 100644 --- a/apps/desktop/src/main/browser-agent/post-action-observation.test.ts +++ b/apps/desktop/src/main/browser-agent/post-action-observation.test.ts @@ -2,6 +2,34 @@ import { describe, expect, it, vi } from 'vitest' import { withPostActionObservation } from '@/main/browser-agent/post-action-observation' describe('post-action observation', () => { + it('shows the page after a wait without being asked, since a wait exists to see what changed', async () => { + const result = await withPostActionObservation( + 'browser_wait_for', + { timeoutMs: 500 }, + async (params) => { + expect(params).toEqual({ timeoutMs: 500 }) + return { found: false } + }, + async (query) => { + expect(query).toBeUndefined() + return { outline: '- heading "Order confirmed"' } + }, + vi.fn() + ) + expect(result).toMatchObject({ + found: false, + observation: { ok: true, result: { outline: '- heading "Order confirmed"' } }, + }) + }) + + it('rejects an explicit null observe on a wait instead of treating it as omitted', async () => { + const action = vi.fn(async () => ({ found: true })) + await expect( + withPostActionObservation('browser_wait_for', { observe: null }, action, vi.fn(), vi.fn()) + ).rejects.toThrow('observe must be') + expect(action).not.toHaveBeenCalled() + }) + it('observes after the action, preserving partial form results and replacing refs', async () => { const order: string[] = [] const result = await withPostActionObservation( diff --git a/apps/desktop/src/main/browser-agent/post-action-observation.ts b/apps/desktop/src/main/browser-agent/post-action-observation.ts index 3b2d7554d64..39a6df35d79 100644 --- a/apps/desktop/src/main/browser-agent/post-action-observation.ts +++ b/apps/desktop/src/main/browser-agent/post-action-observation.ts @@ -17,8 +17,15 @@ const OBSERVABLE_ACTIONS: ReadonlySet = new Set([ 'browser_hover', 'browser_drag', 'browser_batch', + 'browser_wait_for', ]) +/** The call's post-action observation request; a wait observes by default, to show what the page became. */ +export function observeRequest(tool: BrowserToolName, params: Record): unknown { + if (params.observe !== undefined) return params.observe + return tool === 'browser_wait_for' ? {} : undefined +} + /** Preserves a dispatched action when its acknowledgement or observation is interrupted. */ export function withFailedPostActionObservation(result: unknown, error: unknown): unknown { const observation = { @@ -38,7 +45,7 @@ export async function withPostActionObservation( observe: (query: string | undefined) => Promise, assertCurrent: () => void ): Promise { - const request = params.observe + const request = observeRequest(tool, params) if (request === undefined) return action(params) if ( !OBSERVABLE_ACTIONS.has(tool) ||