diff --git a/apps/desktop/src/main/browser-agent/driver.test.ts b/apps/desktop/src/main/browser-agent/driver.test.ts index f98083e1ad..5970afec61 100644 --- a/apps/desktop/src/main/browser-agent/driver.test.ts +++ b/apps/desktop/src/main/browser-agent/driver.test.ts @@ -1732,4 +1732,146 @@ describe('credential protection', () => { 'Element ids are not valid in this tab. Call browser_snapshot and use an id from that result.', }) }) + + /** Fires every instrumentation listener registered for a WebContents event. */ + function emitContentsEvent( + contents: Awaited>, + event: string, + ...args: unknown[] + ): void { + for (const [name, listener] of vi.mocked(contents.on).mock.calls) { + if (name === event) (listener as (...listenerArgs: unknown[]) => void)({}, ...args) + } + } + + it('keeps element ids across a same-document (SPA) navigation', async () => { + const contents = await openPage() + respondWith(contents, {}) + + emitContentsEvent(contents, 'did-navigate-in-page') + const result = await driver.executeTool('chat-test', 'browser_click', { elementId: 0 }) + + expect(result).toMatchObject({ ok: true, result: { dispatched: true } }) + }) + + it('still invalidates element ids on a cross-document navigation', async () => { + const contents = await openPage() + respondWith(contents, {}) + + emitContentsEvent(contents, 'did-navigate') + const result = await driver.executeTool('chat-test', 'browser_click', { elementId: 0 }) + + expect(result).toEqual({ + ok: false, + error: + 'Element ids are not valid in this tab. Call browser_snapshot and use an id from that result.', + }) + }) + + it('tolerates same-document URL churn during a keypress', async () => { + const contents = await openPage() + let urlReads = 0 + vi.mocked(contents.getURL).mockImplementation(() => + ++urlReads === 1 ? 'https://example.com/channel-a' : 'https://example.com/channel-b' + ) + respondWith(contents, { + activeElementSecrecy: 'safe', + readActiveElementState: {}, + readPageActionState: { + url: 'https://example.com/channel-a', + title: 'Example', + focus: 'body', + mutationRevision: 0, + dialogs: [], + scroll: [0], + }, + }) + + const result = await driver.executeTool('chat-test', 'browser_press_key', { key: 'Escape' }) + + expect(result.ok).toBe(true) + expect(cdpCalls(contents, 'Input.dispatchKeyEvent').length).toBeGreaterThan(0) + }) + + it('aborts a keypress when a cross-document navigation lands mid-flight', async () => { + const contents = await openPage() + let navigated = false + vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => { + if (isPageCall(expression, 'activeElementSecrecy')) return Promise.resolve('safe') + if (isPageCall(expression, 'readActiveElementState')) return Promise.resolve({}) + if (isPageCall(expression, 'readPageActionState')) { + if (!navigated) { + navigated = true + emitContentsEvent(contents, 'did-navigate') + } + return Promise.resolve({ + url: 'https://example.com/login', + title: 'Example', + focus: 'body', + mutationRevision: 0, + dialogs: [], + scroll: [0], + }) + } + return Promise.resolve(undefined) + }) + + const result = await driver.executeTool('chat-test', 'browser_press_key', { key: 'Escape' }) + + expect(result.ok).toBe(false) + expect(result.error).toMatch(/active tab or page changed/) + expect(cdpCalls(contents, 'Input.dispatchKeyEvent')).toHaveLength(0) + }) + + it('waits for a late-mounting editor before typing', async () => { + const contents = await openPage() + let focusReads = 0 + vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => { + if (isPageCall(expression, 'focusElementForTyping')) { + focusReads++ + return Promise.resolve( + focusReads === 1 + ? { error: 'not-editable' } + : { focused: true, kind: 'contenteditable', x: 24, y: 48 } + ) + } + if (isPageCall(expression, 'activeElementSecrecy')) return Promise.resolve('safe') + if (isPageCall(expression, 'readActiveElementState')) { + return Promise.resolve({ activeElement: 'div', valueLength: 5 }) + } + return Promise.resolve(undefined) + }) + + const result = await driver.executeTool('chat-test', 'browser_type', { + elementId: 0, + text: 'hello', + }) + + expect(result.ok).toBe(true) + expect(focusReads).toBeGreaterThan(1) + expect(cdpCalls(contents, 'Input.insertText')).toHaveLength(1) + }) + + it('reprobes a transiently stale click target before giving up', async () => { + const contents = await openPage() + let clickReads = 0 + vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => { + if (isPageCall(expression, 'clickElement')) { + clickReads++ + return Promise.resolve( + clickReads === 1 + ? { error: 'stale' } + : { dispatched: false, x: 24, y: 48, element: 'Channel row' } + ) + } + if (isPageCall(expression, 'readActiveElementState')) return Promise.resolve({}) + if (isPageCall(expression, 'readPageActionState')) return Promise.resolve({}) + return Promise.resolve(undefined) + }) + + const result = await driver.executeTool('chat-test', 'browser_click', { elementId: 0 }) + + expect(result).toMatchObject({ ok: true, result: { dispatched: true } }) + expect(clickReads).toBeGreaterThan(1) + }) }) diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index 8d3674cbd7..f6c43f21c6 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -174,6 +174,29 @@ function invalidateSnapshot(state = driverScopeState()): void { state.snapshotCaptureEpoch++ } +/** + * Cross-document navigation counters, bumped by tab instrumentation. A live + * SPA rewrites its URL with pushState/replaceState between a snapshot and the + * input dispatched from it, so URL equality cannot distinguish "the document + * the model saw is gone" from routine same-document churn — only a real + * document swap increments these. + */ +const crossDocumentNavigations = new WeakMap() +const crossDocumentFrameNavigations = new WeakMap>() + +function navigationEpoch(contents: WebContents): number { + return crossDocumentNavigations.get(contents) ?? 0 +} + +function frameEpochKey(processId: number, routingId: number): string { + return `${processId}:${routingId}` +} + +function frameNavigationEpoch(contents: WebContents, frame: WebFrameMain): number { + const key = frameEpochKey(frame.processId, frame.routingId) + return crossDocumentFrameNavigations.get(contents)?.get(key) ?? 0 +} + const driverScopeStates = new Map() const driverScopeAliases = new Map() const CANCELLED_TOOL_TTL_MS = 5 * 60_000 @@ -312,6 +335,7 @@ function instrumentTab(contents: WebContents): void { contents.on( 'did-navigate', inScope(() => { + crossDocumentNavigations.set(contents, navigationEpoch(contents) + 1) if (session.automationTab()?.view.webContents === contents) { invalidateSnapshot() } @@ -320,6 +344,21 @@ function instrumentTab(contents: WebContents): void { pushTabsState() }) ) + contents.on( + 'did-frame-navigate', + inScope( + (_event, _url, _httpResponseCode, _httpStatusText, _isMainFrame, processId, routingId) => { + const frames = crossDocumentFrameNavigations.get(contents) ?? new Map() + const key = frameEpochKey(processId, routingId) + frames.set(key, (frames.get(key) ?? 0) + 1) + crossDocumentFrameNavigations.set(contents, frames) + } + ) + ) + // Same-document navigation deliberately does NOT invalidate the snapshot: a + // live SPA (Slack) pushStates continuously, and invalidating here made every + // ref die between snapshot and act. Element-level identity checks and the + // in-page ref resolver keep individual actions honest instead. for (const event of [ 'did-navigate-in-page', 'page-title-updated', @@ -330,12 +369,6 @@ function instrumentTab(contents: WebContents): void { contents.on( event as 'did-navigate', inScope(() => { - if ( - event === 'did-navigate-in-page' && - session.automationTab()?.view.webContents === contents - ) { - invalidateSnapshot() - } pushPageState(contents) pushTabsState() }) @@ -1037,14 +1070,14 @@ function pageTargetForElement(contents: WebContents, elementId: number): PageExe return target } -function assertActiveContents(contents: WebContents, expectedUrl?: string): void { +function assertActiveContents(contents: WebContents, expectedNavigationEpoch?: number): void { const active = session.automationTab() if ( session.automationTabClaimedByUser() || !active || active.view.webContents !== contents || contents.isDestroyed() || - (expectedUrl !== undefined && contents.getURL() !== expectedUrl) + (expectedNavigationEpoch !== undefined && navigationEpoch(contents) !== expectedNavigationEpoch) ) { throw new ToolError('The active tab or page changed before input could be dispatched.') } @@ -1089,7 +1122,7 @@ function sameWebFrame(left: WebFrameMain, right: WebFrameMain): boolean { function assertFocusedTargetUnchanged( contents: WebContents, target: PageExecutionTarget, - expectedFrameUrl?: string + expectedFrameNavigationEpoch?: number ): void { const focused = focusedPageTarget(contents) const unchanged = @@ -1106,7 +1139,8 @@ function assertFocusedTargetUnchanged( if ( frame.isDestroyed() || frame.detached || - (expectedFrameUrl !== undefined && frame.url !== expectedFrameUrl) || + (expectedFrameNavigationEpoch !== undefined && + frameNavigationEpoch(contents, frame) !== expectedFrameNavigationEpoch) || !contents.mainFrame.framesInSubtree.some((candidate) => sameWebFrame(candidate, frame)) ) { throw new ToolError( @@ -1154,16 +1188,25 @@ async function prepareTypingSurface( target: PageExecutionTarget, elementId: number, moveFocus: boolean, - executionDeadline?: number + executionDeadline?: number, + settleGraceMs = 0 ): Promise> { const prepared = unwrapPageResult( - await execInPage( - target, - focusElementForTyping, - [elementId, moveFocus], - false, - executionDeadline - ) + settleGraceMs > 0 + ? await execInPageWithSettleGrace( + target, + focusElementForTyping, + [elementId, moveFocus], + executionDeadline, + settleGraceMs + ) + : await execInPage( + target, + focusElementForTyping, + [elementId, moveFocus], + false, + executionDeadline + ) ) if ( !isRecordLike(prepared) || @@ -1178,6 +1221,43 @@ async function prepareTypingSurface( return prepared } +const TRANSIENT_PROBE_ERRORS = new Set(['stale', 'not-visible', 'not-editable']) +const SETTLE_GRACE_MS = 1_000 +const SETTLE_PROBE_INTERVAL_MS = 250 + +/** + * First-touch page probe with a short settle grace. A live view re-rendering + * under the agent (Slack's virtualized sidebar, its late-mounting composer) + * can transiently report an element as stale, invisible, or not yet editable + * while its replacement mounts one frame later. These call sites run before + * anything is dispatched, so a bounded reprobe safely turns that churn into a + * recovery instead of a dead ref. + */ +async function execInPageWithSettleGrace( + target: PageExecutionTarget, + fn: (...args: Args) => Result, + args: Args, + executionDeadline?: number, + graceMs = SETTLE_GRACE_MS +): Promise { + const graceDeadline = Date.now() + graceMs + for (;;) { + const result = await execInPage(target, fn, args, false, executionDeadline) + const code = + isRecordLike(result) && typeof result.error === 'string' ? String(result.error) : null + if ( + code === null || + !TRANSIENT_PROBE_ERRORS.has(code) || + Date.now() + SETTLE_PROBE_INTERVAL_MS > graceDeadline || + (executionDeadline !== undefined && + Date.now() + SETTLE_PROBE_INTERVAL_MS >= executionDeadline) + ) { + return result + } + await sleep(SETTLE_PROBE_INTERVAL_MS) + } +} + function pointFromPrepared(prepared: Record): { x: number; y: number } { return { x: prepared.x as number, y: prepared.y as number } } @@ -1522,7 +1602,7 @@ async function captureSnapshot(contents: WebContents, notAfter?: number): Promis invalidateSnapshot(state) const captureEpoch = state.snapshotCaptureEpoch const capturedTabId = tab.id - const capturedUrl = contents.getURL() + const capturedNavigationEpoch = navigationEpoch(contents) const targets = new Map() const targetLineIndexes = new Map() const stillCurrent = (): boolean => { @@ -1532,7 +1612,9 @@ async function captureSnapshot(contents: WebContents, notAfter?: number): Promis active?.id === capturedTabId && active.view.webContents === contents && !contents.isDestroyed() && - contents.getURL() === capturedUrl + // Same-document URL drift (SPA pushState) must not abort the capture; + // only a cross-document navigation makes the collected refs meaningless. + navigationEpoch(contents) === capturedNavigationEpoch ) } @@ -1957,11 +2039,10 @@ async function executeToolInner( if (!targetFrame) { assertCurrentExecution() const first = unwrapPageResult( - await execInPage( + await execInPageWithSettleGrace( target, clickElement, [elementId, false, false], - false, executionDeadline ) ) @@ -2010,7 +2091,12 @@ async function executeToolInner( assertCurrentExecution() assertElementActionCurrent(contents, elementId, target) const focused = unwrapPageResult( - await execInPage(target, clickElement, [elementId, false, true], false, executionDeadline) + await execInPageWithSettleGrace( + target, + clickElement, + [elementId, false, true], + executionDeadline + ) ) if (!isRecordLike(focused)) throw new ToolError('Could not resolve that click target.') prepared = focused @@ -2292,7 +2378,16 @@ async function executeToolInner( // synthetic value-setter when CDP is unavailable. assertCurrentExecution() assertElementActionCurrent(contents, elementId, target) - const initialSurface = await prepareTypingSurface(target, elementId, true, executionDeadline) + // The settle grace covers a composer whose real contenteditable mounts a + // beat after the surrounding view renders (Slack); nothing has been + // dispatched yet, so reprobing is safe. + const initialSurface = await prepareTypingSurface( + target, + elementId, + true, + executionDeadline, + SETTLE_GRACE_MS + ) assertCurrentExecution() assertElementActionCurrent(contents, elementId, target) if (targetFrame) { @@ -2591,9 +2686,10 @@ async function executeToolInner( const requestedKey = requireStr(params, 'key') const combo = parseKeyCombo(requestedKey) const contents = session.requireAutomationTab().view.webContents - const pressedPageUrl = contents.getURL() + const pressedNavigationEpoch = navigationEpoch(contents) let target: PageExecutionTarget = focusedPageTarget(contents) - let pressedFrameUrl = target === contents ? undefined : (target as WebFrameMain).url + let pressedFrameEpoch = + target === contents ? undefined : frameNavigationEpoch(contents, target as WebFrameMain) // Pasting would move the user's clipboard into the page, where the next // snapshot reports it as an ordinary field value — clipboards routinely // hold a password copied out of a password manager. Copy and cut would @@ -2612,7 +2708,8 @@ async function executeToolInner( const dispatchTarget = focusedPageTarget(contents) if (dispatchTarget !== target) { target = dispatchTarget - pressedFrameUrl = target === contents ? undefined : (target as WebFrameMain).url + pressedFrameEpoch = + target === contents ? undefined : frameNavigationEpoch(contents, target as WebFrameMain) beforePage = await pageActionState(target, true) beforeElement = await activeElementState(target) } @@ -2648,8 +2745,8 @@ async function executeToolInner( let fallbackState: Record = {} try { assertCurrentExecution() - assertActiveContents(contents, pressedPageUrl) - assertFocusedTargetUnchanged(contents, target, pressedFrameUrl) + assertActiveContents(contents, pressedNavigationEpoch) + assertFocusedTargetUnchanged(contents, target, pressedFrameEpoch) await dispatchKeyCombo(contents, combo) } catch (error) { if (error instanceof KeyDispatchError && error.keyDownDispatched) { @@ -2661,8 +2758,8 @@ async function executeToolInner( // CDP unavailable (debugger detached): synthetic DOM fallback. It // cannot trigger default editing actions, so say so in the result. assertCurrentExecution() - assertActiveContents(contents, pressedPageUrl) - assertFocusedTargetUnchanged(contents, target, pressedFrameUrl) + assertActiveContents(contents, pressedNavigationEpoch) + assertFocusedTargetUnchanged(contents, target, pressedFrameEpoch) const fallbackSecrecy = await execInPage(target, activeElementSecrecy, []).catch( () => 'opaque' ) @@ -2673,8 +2770,8 @@ async function executeToolInner( ) } assertCurrentExecution() - assertActiveContents(contents, pressedPageUrl) - assertFocusedTargetUnchanged(contents, target, pressedFrameUrl) + assertActiveContents(contents, pressedNavigationEpoch) + assertFocusedTargetUnchanged(contents, target, pressedFrameEpoch) const fallback = unwrapPageResult( await execInPage( target, 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 52a84235db..a74f95bf9c 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -769,6 +769,38 @@ describe('collectSnapshot', () => { expect(focusElementForTyping(ref)).toEqual({ error: 'stale' }) }) + it('keeps a connected ref usable after a same-document URL change', () => { + document.body.innerHTML = '' + const button = visible(document.querySelector('button') as HTMLButtonElement) + const ref = refFor(outlineOf(collectSnapshot()), 'Send') + let clicked = false + button.addEventListener('click', () => { + clicked = true + }) + + window.history.pushState({}, '', '/client/T123/C456') + + expect(clickElement(ref)).toMatchObject({ dispatched: true, refRecovered: false }) + expect(clicked).toBe(true) + }) + + it('recovers a replaced ref after a same-document URL change', () => { + document.body.innerHTML = '' + const original = visible(document.querySelector('button') as HTMLButtonElement) + const ref = refFor(outlineOf(collectSnapshot()), 'Messages') + const replacement = visible(original.cloneNode(true) as HTMLButtonElement) + let clicked = false + replacement.addEventListener('click', () => { + clicked = true + }) + + window.history.pushState({}, '', '/client/T123/C456') + original.replaceWith(replacement) + + expect(clickElement(ref)).toMatchObject({ dispatched: true, refRecovered: true }) + expect(clicked).toBe(true) + }) + it('refuses to recover a ref when replacement is ambiguous', () => { document.body.innerHTML = '' const original = visible(document.querySelector('button') as HTMLButtonElement) diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index f5f4cb2f9a..3bdb88a64c 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -109,6 +109,7 @@ export function collectSnapshot(startingElementId = 0): unknown { '[onclick]', '[contenteditable="true"]', '[contenteditable=""]', + '[contenteditable="plaintext-only"]', ].join(', ') const landmarkSelector = [ 'nav', @@ -593,7 +594,7 @@ export function collectSnapshot(startingElementId = 0): unknown { /** * React commonly replaces a control's DOM node while preserving its - * semantics. Recover only when the old page URL and a strong semantic + * semantics. Recover only when the old page origin and a strong semantic * fingerprint still identify one candidate; a weak or ambiguous match is a * real stale ref, never permission to click something nearby. */ @@ -601,6 +602,19 @@ export function collectSnapshot(startingElementId = 0): unknown { const locator = locators[id] if (!locator) return null + // Origin, not full URL: a live SPA rewrites its path with pushState + // between snapshot and act (Slack does so continuously), while the + // element the model chose is often still the same mounted node. The + // origin still pins the document/frame; the role/name/attribute and + // ancestor/context signatures below pin the element itself. + const pageOriginOf = (url: string): string => { + try { + return new URL(url).origin + } catch { + return url + } + } + const stableAttributes = [ 'id', 'href', @@ -620,7 +634,7 @@ export function collectSnapshot(startingElementId = 0): unknown { const identityMatches = (candidate: Element, connected = false): boolean => { if ( candidate.tagName.toUpperCase() !== locator.tag || - pageUrlFor(candidate) !== locator.url || + pageOriginOf(pageUrlFor(candidate)) !== pageOriginOf(locator.url) || roleFor(candidate) !== locator.role ) { return false @@ -1028,7 +1042,7 @@ export function clickElement( addCandidate(el) for (const candidate of Array.from( el.querySelectorAll( - 'input, textarea, [contenteditable="true"], [contenteditable=""]' + 'input, textarea, [contenteditable="true"], [contenteditable=""], [contenteditable="plaintext-only"]' ) )) { addCandidate(candidate) @@ -1244,7 +1258,7 @@ export function focusElementForTyping(id: number, moveFocus = true): unknown { addEditable(el) for (const candidate of Array.from( el.querySelectorAll( - 'input, textarea, [contenteditable="true"], [contenteditable=""]' + 'input, textarea, [contenteditable="true"], [contenteditable=""], [contenteditable="plaintext-only"]' ) )) { addEditable(candidate) @@ -1770,7 +1784,7 @@ export function typeIntoElement(id: number, text: string, submit: boolean): unkn addEditable(el) for (const candidate of Array.from( el.querySelectorAll( - 'input, textarea, [contenteditable="true"], [contenteditable=""]' + 'input, textarea, [contenteditable="true"], [contenteditable=""], [contenteditable="plaintext-only"]' ) )) { addEditable(candidate)