fix(desktop): harden semantic browser control edge cases

Author: waleedlatif1

apps/desktop/src/main/browser-agent/cdp.ts

@@ -468,10 +468,8 @@ function sameScreenshotViewport(
  * snapshot capture refuses to scale a visible surface for the same reason.
  *
  * Bounding resolution therefore happens here instead, on the returned image.
- * The output keeps the dimensions the clipped capture produced, so `scale`
- * still maps image pixels back to CSS pixels for the coordinate tools
- * (cssX = imageX / scale) — including on a 2x display, where an unclipped
- * capture arrives at device resolution and this is what brings it back down.
+ * Optional element crops also happen in memory. Convert output coordinates
+ * with cssX = (clip?.x ?? 0) + imageX / scale, and the equivalent Y formula.
  */
 export async function captureScreenshot(
   contents: WebContents,

apps/desktop/src/main/browser-agent/driver.test.ts

@@ -212,6 +212,28 @@ describe('executeTool', () => {
     }
   })
 
+  it('uses the shared failed-page recovery path when reloading', async () => {
+    await driver.executeTool('chat-test', 'browser_open_tab', {})
+    const tab = session.requireTab()
+    tab.pageIssue = {
+      kind: 'load-error',
+      url: 'https://example.com/failed',
+      code: -102,
+      description: 'ERR_CONNECTION_REFUSED',
+    }
+    vi.useFakeTimers()
+    try {
+      const result = driver.executeTool('chat-test', 'browser_reload', {})
+      await vi.advanceTimersByTimeAsync(500)
+
+      await expect(result).resolves.toMatchObject({ ok: true })
+      expect(tab.view.webContents.loadURL).toHaveBeenCalledWith('https://example.com/failed')
+      expect(tab.view.webContents.reload).not.toHaveBeenCalled()
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+
   it('keeps a takeover pending when the clock advances beyond twelve hours', async () => {
     await driver.executeTool('chat-test', 'browser_open_tab', {})
     vi.useFakeTimers()
@@ -1623,62 +1645,48 @@ describe('executeTool', () => {
     )
   })
 
-  it('does not let a snapshot that resolves after timeout overwrite newer refs', async () => {
-    vi.useFakeTimers()
-    try {
-      await driver.executeTool('chat-test', 'browser_open_tab', {})
-      const contents = session.requireTab().view.webContents
-      vi.mocked(contents.getURL).mockReturnValue('https://example.com/')
-      let resolveLate: ((value: unknown) => void) | undefined
-      let snapshotCalls = 0
-      vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => {
-        if (!isPageCall(expression, 'collectSnapshot')) return Promise.resolve(undefined)
-        snapshotCalls++
-        if (snapshotCalls === 1) {
+  it.each(['browser_snapshot', 'browser_find'] as const)(
+    'does not let a timed-out %s restore refs',
+    async (tool) => {
+      vi.useFakeTimers()
+      try {
+        await driver.executeTool('chat-test', 'browser_open_tab', {})
+        const contents = session.requireTab().view.webContents
+        vi.mocked(contents.getURL).mockReturnValue('https://example.com/')
+        let resolveLate: ((value: unknown) => void) | undefined
+        vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => {
+          if (!isPageCall(expression, 'collectSnapshot')) return Promise.resolve(undefined)
           return new Promise((resolve) => {
             resolveLate = resolve
           })
-        }
-        return Promise.resolve({
+        })
+
+        const late = driver.executeTool('chat-test', tool, { query: 'Late' })
+        await vi.advanceTimersByTimeAsync(20_000)
+        await expect(late).resolves.toMatchObject({ ok: false })
+        resolveLate?.({
           url: 'https://example.com/',
-          title: 'Fresh',
-          outline: '- button "Fresh" [ref=10]',
+          title: 'Late',
+          outline: '- button "Late" [ref=0]',
           truncated: false,
-          refIds: [10],
-          refLineIndexes: { 10: 0 },
-          nextElementId: 11,
+          refIds: [0],
+          refLineIndexes: { 0: 0 },
+          nextElementId: 1,
         })
-      })
-
-      const late = driver.executeTool('chat-test', 'browser_snapshot', {})
-      await vi.advanceTimersByTimeAsync(20_000)
-      await expect(late).resolves.toMatchObject({ ok: false })
-      await expect(driver.executeTool('chat-test', 'browser_snapshot', {})).resolves.toMatchObject({
-        ok: true,
-      })
-
-      resolveLate?.({
-        url: 'https://example.com/',
-        title: 'Late',
-        outline: '- button "Late" [ref=0]',
-        truncated: false,
-        refIds: [0],
-        refLineIndexes: { 0: 0 },
-        nextElementId: 1,
-      })
-      await Promise.resolve()
-      await Promise.resolve()
-
-      await expect(
-        driver.executeTool('chat-test', 'browser_click', { elementId: 0 })
-      ).resolves.toMatchObject({
-        ok: false,
-        error: expect.stringContaining('not present in the current snapshot'),
-      })
-    } finally {
-      vi.useRealTimers()
+        await Promise.resolve()
+        await Promise.resolve()
+
+        await expect(
+          driver.executeTool('chat-test', 'browser_click', { elementId: 0 })
+        ).resolves.toMatchObject({
+          ok: false,
+          error: expect.stringContaining('Call browser_snapshot'),
+        })
+      } finally {
+        vi.useRealTimers()
+      }
     }
-  })
+  )
 
   it('merges cross-origin structure and routes its refs through production frame isolation', async () => {
     const win = new BrowserWindow()
@@ -1777,7 +1785,7 @@ describe('executeTool', () => {
         if (isPageCall(expression, 'collectSnapshot')) {
           return Promise.resolve({
             url: 'https://ogs.google.com/u/0/widget/app',
-            title: 'Google apps',
+            title: 'Google apps [ref=0]',
             outline: '- link "Drive" [ref=1]\n- textbox "Search apps" [ref=2]',
             truncated: false,
             refIds: [1, 2],
@@ -1863,7 +1871,7 @@ describe('executeTool', () => {
     expect(snapshot).toMatchObject({
       ok: true,
       result: {
-        outline: expect.stringContaining('cross-origin iframe "Google apps"'),
+        outline: expect.stringContaining('cross-origin iframe "Google apps [ref\u200b=0]"'),
         capturedCrossOriginFrames: 1,
         unreadableCrossOriginFrames: 1,
         hiddenCrossOriginFrames: 1,
@@ -3249,6 +3257,65 @@ describe('credential protection', () => {
     })
   })
 
+  it.each([
+    [1, false, 1],
+    [100, false, 50],
+    [100, true, 50],
+  ])(
+    'bounds find results for maxResults=%s and snapshot truncation=%s',
+    async (maxResults, truncated, expectedCount) => {
+      const contents = await openPage()
+      const ids = Array.from({ length: 60 }, (_, index) => index + 1)
+      respondWith(contents, {
+        collectSnapshot: {
+          url: 'https://example.com/login',
+          title: 'Example',
+          outline: ids.map((id) => `- button "Continue ${id}" [ref=${id}]`).join('\n'),
+          truncated,
+          refIds: ids,
+          refLineIndexes: Object.fromEntries(ids.map((id, index) => [id, index])),
+          nextElementId: 61,
+        },
+      })
+      const response = await driver.executeTool('chat-test', 'browser_find', {
+        query: 'Continue',
+        maxResults,
+      })
+      expect(response).toMatchObject({ ok: true, result: { totalMatches: 60, truncated: true } })
+      expect((response.result as { matches: unknown[] }).matches).toHaveLength(expectedCount)
+    }
+  )
+
+  it('reports incomplete search coverage even when no ref matches', async () => {
+    const contents = await openPage()
+    respondWith(contents, {
+      collectSnapshot: {
+        url: 'https://example.com/login',
+        title: 'Example',
+        outline: '- button "Other" [ref=1]',
+        truncated: true,
+        refIds: [1],
+        refLineIndexes: { 1: 0 },
+        nextElementId: 2,
+      },
+    })
+    const response = await driver.executeTool('chat-test', 'browser_find', { query: 'Missing' })
+    expect(response).toMatchObject({
+      ok: true,
+      result: { matches: [], totalMatches: 0, truncated: true },
+    })
+  })
+
+  it('rejects oversized search text before taking a snapshot', async () => {
+    const contents = await openPage()
+    vi.mocked(contents.executeJavaScript).mockClear()
+    const response = await driver.executeTool('chat-test', 'browser_find', {
+      query: 'x'.repeat(4097),
+    })
+    expect(response).toMatchObject({ ok: false })
+    expect(contents.executeJavaScript).not.toHaveBeenCalled()
+  })
+
   it('does not click a checkable control already in the requested state', async () => {
     const contents = await openPage()
     respondWith(contents, {
@@ -3272,47 +3339,50 @@ describe('credential protection', () => {
     expect(cdpCalls(contents, 'Input.dispatchMouseEvent')).toHaveLength(0)
   })
 
-  it('uses the trusted click path and verifies a changed checkable control', async () => {
-    const contents = await openPage()
-    let stateReads = 0
-    vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => {
-      if (isPageCall(expression, 'readCheckableElementState')) {
-        stateReads++
-        return Promise.resolve({
-          checked: stateReads > 1,
-          disabled: false,
-          readOnly: false,
-          kind: 'input:checkbox',
-        })
-      }
-      if (isPageCall(expression, 'clickElement')) {
-        return Promise.resolve({ dispatched: false, x: 24, y: 48, element: 'Checkbox' })
-      }
-      if (isPageCall(expression, 'readPageActionState')) {
-        return Promise.resolve({
-          url: 'https://example.com/login',
-          title: 'Example',
-          focus: 'body',
-          mutationRevision: 0,
-          dialogs: [],
-          scroll: [0],
-        })
-      }
-      if (isPageCall(expression, 'readActiveElementState')) return Promise.resolve({})
-      return Promise.resolve(undefined)
-    })
+  it.each([false, 'mixed'])(
+    'uses the trusted click path from %s and verifies a changed checkable control',
+    async (initialState) => {
+      const contents = await openPage()
+      let stateReads = 0
+      vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => {
+        if (isPageCall(expression, 'readCheckableElementState')) {
+          stateReads++
+          return Promise.resolve({
+            checked: stateReads > 1 ? true : initialState,
+            disabled: false,
+            readOnly: false,
+            kind: 'input:checkbox',
+          })
+        }
+        if (isPageCall(expression, 'clickElement')) {
+          return Promise.resolve({ dispatched: false, x: 24, y: 48, element: 'Checkbox' })
+        }
+        if (isPageCall(expression, 'readPageActionState')) {
+          return Promise.resolve({
+            url: 'https://example.com/login',
+            title: 'Example',
+            focus: 'body',
+            mutationRevision: 0,
+            dialogs: [],
+            scroll: [0],
+          })
+        }
+        if (isPageCall(expression, 'readActiveElementState')) return Promise.resolve({})
+        return Promise.resolve(undefined)
+      })
 
-    const result = await driver.executeTool('chat-test', 'browser_set_checked', {
-      elementId: 0,
-      checked: true,
-    })
+      const result = await driver.executeTool('chat-test', 'browser_set_checked', {
+        elementId: 0,
+        checked: true,
+      })
 
-    expect(result).toMatchObject({
-      ok: true,
-      result: { checked: true, changed: true, dispatched: true, trusted: true },
-    })
-    expect(cdpCalls(contents, 'Input.dispatchMouseEvent')).toHaveLength(3)
-  })
+      expect(result).toMatchObject({
+        ok: true,
+        result: { checked: true, changed: true, dispatched: true, trusted: true },
+      })
+      expect(cdpCalls(contents, 'Input.dispatchMouseEvent')).toHaveLength(3)
+    }
+  )
 
   it('waits for URL and semantic element state together', async () => {
     const contents = await openPage()
@@ -3335,6 +3405,59 @@ describe('credential protection', () => {
     })
   })
 
+  it.each(['hidden', 'detached'])(
+    'does not treat a failed probe as element state %s',
+    async (state) => {
+      const contents = await openPage()
+      vi.mocked(contents.executeJavaScript).mockRejectedValue(
+        new Error('Execution context destroyed')
+      )
+      const response = await driver.executeTool('chat-test', 'browser_wait_for', {
+        elementId: 0,
+        state,
+        timeoutMs: 1000,
+      })
+      expect(response).toMatchObject({ ok: false })
+      expect(response.error).toContain('Execution context destroyed')
+    }
+  )
+
+  it('rejects navigation while inspecting an element wait condition', async () => {
+    const contents = await openPage()
+    vi.mocked(contents.executeJavaScript).mockImplementation(async () => {
+      vi.mocked(contents.getURL).mockReturnValue('https://example.com/next')
+      return { targetState: { present: false, rendered: false } }
+    })
+    const response = await driver.executeTool('chat-test', 'browser_wait_for', {
+      elementId: 0,
+      state: 'detached',
+      timeoutMs: 1000,
+    })
+    expect(response).toMatchObject({ ok: false })
+    expect(response.error).toContain('page changed')
+  })
+
+  it.each([
+    ['detached', { present: true, rendered: false }],
+    ['unchecked', { present: true, rendered: true, checked: 'mixed' }],
+    ['unchecked', { present: true, rendered: true }],
+  ])('does not satisfy %s from an incompatible element state', async (state, targetState) => {
+    const contents = await openPage()
+    respondWith(contents, { readPageActionState: { targetState } })
+    vi.useFakeTimers()
+    try {
+      const response = driver.executeTool('chat-test', 'browser_wait_for', {
+        elementId: 0,
+        state,
+        timeoutMs: 100,
+      })
+      await vi.advanceTimersByTimeAsync(1000)
+      await expect(response).resolves.toMatchObject({ ok: true, result: { found: false } })
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+
   it('crops an element screenshot without changing the live viewport', async () => {
     const contents = await openPage()
     respondWith(contents, {
@@ -3367,6 +3490,25 @@ describe('credential protection', () => {
     }
   })
 
+  it('rejects navigation during an element screenshot measurement', async () => {
+    const contents = await openPage()
+    vi.mocked(contents.executeJavaScript).mockImplementation(async (expression: string) => {
+      if (isPageCall(expression, 'getElementScreenshotRect')) {
+        vi.mocked(contents.getURL).mockReturnValue('https://example.com/next')
+        return { x: 20, y: 30, width: 200, height: 100 }
+      }
+    })
+    const capture = vi.spyOn(cdp, 'captureScreenshot')
+    try {
+      const result = await driver.executeTool('chat-test', 'browser_screenshot', { elementId: 0 })
+      expect(result).toMatchObject({ ok: false })
+      expect(result.error).toMatch(/page changed/)
+      expect(capture).not.toHaveBeenCalled()
+    } finally {
+      capture.mockRestore()
+    }
+  })
+
   it('zooms by a standard step and invalidates existing element refs', async () => {
     const contents = await openPage()