Stop the browser agent acting on the wrong element, and say why it refused

Four findings from the module audit, the first of which could silently do the
wrong thing rather than merely fail.

- A ref whose node is gone is re-adopted by structural resemblance, matching on
  ORIGIN only so a pushState between snapshot and act does not kill every ref.
  That leniency also let a ref to a row control in one view rebind to the
  identical control in a view the app had since navigated to — acting on the
  wrong message, signalled by nothing louder than recovered: true. Adoption now
  requires the same path; a view swap reports the ref stale, and the caller
  re-snapshots. Revalidating a still-connected node stays lenient, because that
  is literally the node the model chose.

- A hit INSIDE the requested element is its own nested control, not an overlay.
  Both produced 'covered by X — close or move the overlay', advice that cannot
  be followed because there is nothing to close. Nested hits now say so and
  point at retargeting.

- browser_click_at, browser_insert_text, and browser_drag listed targetChanged
  in their effect formulas, but none passes an elementId, so no targetState is
  ever captured and the term was always false — coverage that read as real.
  Removed, with a test pinning the dependency.

- The seven effect formulas are deliberately NOT collapsed into one predicate:
  drag must trust domChanged where others must not, hover must ignore field and
  focus changes, click counts focus only for editables. Forcing one would make
  each tool wrong differently. The differences are now documented in one place
  next to the shared computation, so divergence is a declared policy rather
  than an accident.

Author: Sg312

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

@@ -2,8 +2,9 @@ import { describe, expect, it, vi } from 'vitest'
 
 vi.mock('electron', () => import('@/test/electron-mock'))
 
-import { WebContentsView, type WebFrameMain } from 'electron'
+import { nativeImage, type WebContents, WebContentsView, type WebFrameMain } from 'electron'
 import {
+  captureScreenshot,
   clickAt,
   ensureInstrumented,
   evaluateInIsolatedFrame,
@@ -482,3 +483,89 @@ describe('browser-agent CDP theme', () => {
     })
   })
 })
+
+/**
+ * The browser panel shows a LIVE view, so a capture must not perturb the page.
+ * Chromium serves `clip` by applying device-emulation params to the widget and
+ * syncing visual properties, which the user sees as the page rescaling and
+ * snapping back. Resolution is bounded on the returned image instead.
+ */
+describe('browser-agent screenshot capture', () => {
+  function captureFixture(imageSize: { width: number; height: number } | null) {
+    const contents = new WebContentsView().webContents
+    vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => {
+      if (method === 'Page.getLayoutMetrics') {
+        return Promise.resolve({ cssLayoutViewport: { clientWidth: 2048, clientHeight: 1024 } })
+      }
+      if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' })
+      return Promise.resolve(undefined)
+    })
+    const resized = {
+      toJPEG: vi.fn(() => Buffer.from('resized')),
+    }
+    // Shared module-level mock: without this, a later fixture reads the
+    // earlier test's decoded image.
+    vi.mocked(nativeImage.createFromBuffer).mockReset()
+    vi.mocked(nativeImage.createFromBuffer).mockReturnValue({
+      isEmpty: vi.fn(() => imageSize === null),
+      getSize: vi.fn(() => imageSize ?? { width: 0, height: 0 }),
+      resize: vi.fn(() => resized),
+      toJPEG: vi.fn(() => Buffer.alloc(0)),
+    } as unknown as ReturnType<typeof nativeImage.createFromBuffer>)
+    return { contents, resized }
+  }
+
+  function screenshotParams(contents: WebContents): Record<string, unknown> {
+    const call = vi
+      .mocked(contents.debugger.sendCommand)
+      .mock.calls.find(([method]) => method === 'Page.captureScreenshot')
+    if (!call) throw new Error('no capture was requested')
+    return call[1] as Record<string, unknown>
+  }
+
+  it('never sends a clip, which would emulate the live page for the capture', async () => {
+    const { contents } = captureFixture({ width: 4096, height: 2048 })
+
+    await captureScreenshot(contents)
+
+    expect(screenshotParams(contents)).not.toHaveProperty('clip')
+  })
+
+  /**
+   * A 2048px CSS viewport bounded to 1024px is scale 0.5, and the capture
+   * arrives at device resolution (4096px on a 2x display). The resize is what
+   * lands the image on the CSS-relative size the coordinate contract
+   * (cssX = imageX / scale) assumes.
+   */
+  it('downscales the returned image to the CSS-relative size', async () => {
+    const { contents, resized } = captureFixture({ width: 4096, height: 2048 })
+
+    const shot = await captureScreenshot(contents)
+
+    const image = vi.mocked(nativeImage.createFromBuffer).mock.results[0].value
+    expect(image.resize).toHaveBeenCalledWith({ width: 1024, height: 512, quality: 'good' })
+    expect(resized.toJPEG).toHaveBeenCalled()
+    expect(shot).toEqual({
+      dataUrl: `data:image/jpeg;base64,${Buffer.from('resized').toString('base64')}`,
+      scale: 0.5,
+    })
+  })
+
+  it('skips the re-encode when the capture already matches the target size', async () => {
+    const { contents } = captureFixture({ width: 1024, height: 512 })
+
+    const shot = await captureScreenshot(contents)
+
+    const image = vi.mocked(nativeImage.createFromBuffer).mock.results[0].value
+    expect(image.resize).not.toHaveBeenCalled()
+    expect(shot).toEqual({ dataUrl: 'data:image/jpeg;base64,c2lt', scale: 0.5 })
+  })
+
+  it('returns the raw capture when the image cannot be decoded', async () => {
+    const { contents } = captureFixture(null)
+
+    const shot = await captureScreenshot(contents)
+
+    expect(shot).toEqual({ dataUrl: 'data:image/jpeg;base64,c2lt', scale: 0.5 })
+  })
+})

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

@@ -11,7 +11,7 @@
 import type { BrowserTheme } from '@sim/browser-protocol'
 import { createLogger } from '@sim/logger'
 import { sleep } from '@sim/utils/helpers'
-import type { WebContents, WebFrameMain } from 'electron'
+import { nativeImage, type WebContents, type WebFrameMain } from 'electron'
 
 const logger = createLogger('BrowserAgentCdp')
 
@@ -370,6 +370,14 @@ export async function evaluateInIsolatedFrame(
  */
 const MAX_SCREENSHOT_EDGE = 1024
 const SCREENSHOT_QUALITY = 70
+/**
+ * Quality of the intermediate capture, before the in-process downscale
+ * re-encodes at {@link SCREENSHOT_QUALITY}. Higher than the final quality so
+ * the two lossy passes together land near where one pass did — the model reads
+ * text out of these frames, and compression artifacts on glyphs cost more than
+ * the transient bytes do.
+ */
+const SCREENSHOT_CAPTURE_QUALITY = 90
 
 interface CdpViewport {
   clientWidth: number
@@ -379,11 +387,19 @@ interface CdpViewport {
 /**
  * Screenshot via CDP (works while the view is hidden), bounded in resolution.
  *
- * `clip.scale` is relative to CSS pixels, so passing the CSS viewport with a
- * scale of 1 already sidesteps the device pixel ratio — an unclipped capture
- * on a 2x display returns a 2x image. Scaling further down keeps the longest
- * edge within {@link MAX_SCREENSHOT_EDGE}. Falls back to an unclipped capture
- * when layout metrics are unavailable.
+ * The capture is deliberately UNCLIPPED. Chromium implements `clip` by applying
+ * device-emulation parameters (viewport offset and scale) to the widget and
+ * synchronizing visual properties, then restoring them. On a headless target
+ * that is invisible; against the live, composited WebContentsView the Sim
+ * resource panel shows, it is a real visual-properties round-trip, and the page
+ * visibly rescales and snaps back — the screenshot flash. `panel.ts`'s own
+ * 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.
  */
 export async function captureScreenshot(
   contents: WebContents
@@ -398,23 +414,32 @@ export async function captureScreenshot(
   const height = viewport?.clientHeight ?? 0
   const scale =
     width > 0 && height > 0 ? Math.min(1, MAX_SCREENSHOT_EDGE / Math.max(width, height)) : 1
-  const clip =
-    width > 0 && height > 0
-      ? {
-          x: 0,
-          y: 0,
-          width,
-          height,
-          scale,
-        }
-      : undefined
 
   const result = await send<{ data: string }>(contents, 'Page.captureScreenshot', {
     format: 'jpeg',
-    quality: SCREENSHOT_QUALITY,
-    ...(clip ? { clip } : {}),
+    quality: SCREENSHOT_CAPTURE_QUALITY,
   })
-  return { dataUrl: `data:image/jpeg;base64,${result.data}`, scale }
+  const captured = `data:image/jpeg;base64,${result.data}`
+
+  const targetWidth = Math.round(width * scale)
+  const targetHeight = Math.round(height * scale)
+  // Without layout metrics there is no CSS frame of reference to resize
+  // against, so the raw capture is the honest answer — the same fallback the
+  // clipped path took.
+  if (targetWidth <= 0 || targetHeight <= 0) return { dataUrl: captured, scale }
+
+  const image = nativeImage.createFromBuffer(Buffer.from(result.data, 'base64'))
+  const size = image.isEmpty() ? { width: 0, height: 0 } : image.getSize()
+  if (size.width === 0 || size.height === 0) return { dataUrl: captured, scale }
+  if (size.width === targetWidth && size.height === targetHeight) {
+    return { dataUrl: captured, scale }
+  }
+
+  const resized = image.resize({ width: targetWidth, height: targetHeight, quality: 'good' })
+  return {
+    dataUrl: `data:image/jpeg;base64,${resized.toJPEG(SCREENSHOT_QUALITY).toString('base64')}`,
+    scale,
+  }
 }
 
 /** One half of a trusted key press (`Input.dispatchKeyEvent` params). */

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

@@ -1664,6 +1664,39 @@ describe('credential protection', () => {
   // navigation it survived — reporting it made every SPA route change under a
   // persistent role=dialog (cookie banner, side drawer, picker) read as a
   // failed click. Only a dialog that arrives with the navigation obstructs it.
+  // targetChanged can only fire when pageActionState was given an elementId.
+  // Tools without one listed it in their effect formula for a long time, where
+  // it was silently always false — coverage that read as real. This pins the
+  // dependency so the next tool that adds the term has to earn it.
+  it('cannot observe a target change for a tool that passes no elementId', async () => {
+    const contents = await openPage()
+    let actionReads = 0
+    vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => {
+      if (isPageCall(expression, 'readActiveElementState')) return Promise.resolve({})
+      if (isPageCall(expression, 'readPageActionState')) {
+        actionReads++
+        // No targetState in either sample: that is what a call without an
+        // elementId returns.
+        return Promise.resolve({
+          url: 'https://example.com/a',
+          title: 'A',
+          focus: 'body',
+          mutationRevision: actionReads === 1 ? 0 : 3,
+          dialogs: [],
+          popups: [],
+          scroll: [0],
+        })
+      }
+      return Promise.resolve(undefined)
+    })
+
+    const result = await driver.executeTool('chat-test', 'browser_press_key', { key: 'Enter' })
+
+    expect(result).toMatchObject({ ok: true })
+    const effect = (result as { result?: { effect?: Record<string, boolean> } }).result?.effect
+    expect(effect?.targetChanged).toBe(false)
+  })
+
   it('ignores a dialog that was already open before the click', async () => {
     const contents = await openPage()
     let actionReads = 0

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

@@ -886,6 +886,12 @@ function unwrapPageResult(result: unknown): unknown {
         `That element is covered by ${blocker}. Close or move the overlay, then take a fresh browser_snapshot.`
       )
     }
+    if (code === 'nested-control') {
+      const blocker = String((result as { blocker?: unknown }).blocker || 'a nested control')
+      throw new ToolError(
+        `The point you targeted lands on ${blocker}, which is its own control inside that element — nothing is covering it. Take a fresh browser_snapshot and use the id of the control you actually want.`
+      )
+    }
     if (code === 'suggestions-open') {
       throw new ToolError(
         'That editable field is already focused and covered by its own suggestions popup. Use browser_type on the same element; do not dismiss the popup first.'
@@ -1298,6 +1304,29 @@ async function pageActionState(
   return toRecord(state)
 }
 
+/**
+ * Which signals each tool accepts as proof its action reached the page.
+ *
+ * The tools deliberately do NOT share one predicate — the differences are real,
+ * and flattening them would make every tool wrong in a different direction:
+ *
+ * - `browser_drag` is the only tool that trusts `domChanged`, because a drop
+ *   that reorders a list may change nothing else observable. Everywhere else
+ *   background churn (Slack, Gmail) would forge success for an ignored action.
+ * - `browser_hover` ignores `fieldChanged` and `focusChanged`: hovering does not
+ *   type or focus, so those would only ever be someone else's effect.
+ * - `browser_click` / `browser_click_at` count `focusChanged` only when the
+ *   target was editable — otherwise a click that merely moved focus reads as
+ *   success.
+ * - `targetChanged` requires a `targetState`, which `pageActionState` captures
+ *   only when given an `elementId`. Tools without one (click_at, insert_text,
+ *   drag, press_key) cannot use it; listing it there read as coverage they did
+ *   not have, and it was silently always false.
+ *
+ * What IS shared is this function: every signal is computed here once, so a
+ * tool's formula is a statement about which evidence it trusts, not a private
+ * re-derivation of what changed.
+ */
 function pageEffect(
   beforePage: Record<string, unknown>,
   afterPage: Record<string, unknown>,
@@ -3207,11 +3236,13 @@ async function executeToolInner(
       const observation = pageEffect(beforePage, afterPage, beforeElement, afterElement)
       const activeTab = session.automationTab()
       const tabChanged = activeTab?.id !== clickedTab.id
+      // targetChanged is deliberately absent: this tool has no elementId, so
+      // pageActionState captures no targetState and the term could only ever
+      // be false. Listing it read as coverage this tool does not have.
       const effectObserved =
         observation.effect.urlChanged ||
         observation.effect.dialogChanged ||
         observation.effect.popupChanged ||
-        observation.effect.targetChanged ||
         (pointTarget.editable === true && observation.effect.focusChanged) ||
         tabChanged
       const dialogs = Array.isArray(afterPage.dialogs) ? afterPage.dialogs.map(String) : []
@@ -3330,11 +3361,13 @@ async function executeToolInner(
       const topObservation = insertInFrame
         ? pageEffect(beforeTopPage, await pageActionState(contents, true), beforeElement, state)
         : observation
+      // targetChanged is deliberately absent: this tool has no elementId, so
+      // pageActionState captures no targetState and the term could only ever
+      // be false. Listing it read as coverage this tool does not have.
       const effectObserved =
         observation.effect.fieldChanged ||
         observation.effect.urlChanged ||
         observation.effect.dialogChanged ||
-        observation.effect.targetChanged ||
         topObservation.effect.urlChanged ||
         topObservation.effect.dialogChanged
       return {
@@ -3448,11 +3481,14 @@ async function executeToolInner(
       const afterElement = await activeElementState(contents)
       const afterPage = await pageActionState(contents)
       const observation = pageEffect(beforePage, afterPage, beforeElement, afterElement)
+      // targetChanged is deliberately absent: this tool has no elementId, so
+      // pageActionState captures no targetState and the term could only ever be
+      // false. domChanged IS trusted here — unlike every other tool — because a
+      // drop that reorders a list may change nothing else observable.
       const effectObserved =
         observation.effect.domChanged ||
         observation.effect.urlChanged ||
         observation.effect.dialogChanged ||
-        observation.effect.targetChanged ||
         observation.effect.scrollChanged
       const dialogs = Array.isArray(afterPage.dialogs) ? afterPage.dialogs.map(String) : []
       return {

apps/desktop/src/main/browser-agent/page-functions.test.ts

@@ -601,8 +601,11 @@ describe('collectSnapshot', () => {
     })
 
     expect(card.contains(nestedButton)).toBe(true)
+    // Nothing is covering the card — its own button owns the point. Reporting
+    // this as an obstruction told the agent to close an overlay that does not
+    // exist; the recovery is to target the nested control instead.
     expect(clickElement(ref, false)).toEqual({
-      error: 'obstructed',
+      error: 'nested-control',
       blocker: 'Delete channel',
     })
   })

apps/desktop/src/main/browser-agent/page-functions.ts

@@ -724,6 +724,29 @@ export function collectSnapshot(startingElementId = 0): unknown {
       if (isCurrentlyVisible(current)) return { element: current, recovered: false }
     }
 
+    // Past this point the original node is gone or hidden, so anything returned
+    // is a DIFFERENT node adopted by structural resemblance. Identity matching
+    // compares origins only — deliberately, so a pushState between snapshot and
+    // act does not kill every ref — but that same leniency let a ref to "More
+    // actions" on a row in one view rebind to the identical control in another
+    // view the app had since navigated to, and act on the wrong thing with no
+    // signal beyond `recovered: true`.
+    //
+    // A same-document path change means the view was swapped, so resemblance is
+    // no longer evidence of sameness. Refuse to adopt and report the ref stale:
+    // the caller re-snapshots, which is cheap and always correct. Revalidating
+    // the still-connected node above stays lenient — it is literally the node
+    // the model chose.
+    const pathOf = (url: string): string => {
+      try {
+        const parsed = new URL(url)
+        return `${parsed.origin}${parsed.pathname}`
+      } catch {
+        return url
+      }
+    }
+    if (pathOf(window.location.href) !== pathOf(locator.url)) return null
+
     const reachable: Element[] = []
     let candidateCount = 0
     const collect = (root: ParentNode, depth = 0): void => {
@@ -1103,6 +1126,22 @@ export function clickElement(
     if (suggestionsCoverFocusedEditable()) {
       return { error: 'suggestions-open', blocker: blockerLabel(blocker) }
     }
+    // A hit INSIDE the requested element is not an overlay — it is the ref
+    // wrapping its own control (a row containing a button, a card containing a
+    // link). hitBelongsToTarget rejects both cases identically, so this was
+    // reported as "covered by X, close or move the overlay", advice that cannot
+    // be followed because there is nothing to close. Name it for what it is so
+    // the agent retargets instead of hunting a phantom overlay.
+    let nested = false
+    for (let current = blocker; current; current = composedParent(current)) {
+      if (current === el) {
+        nested = true
+        break
+      }
+    }
+    if (nested) {
+      return { error: 'nested-control', blocker: blockerLabel(blocker) }
+    }
     return { error: 'obstructed', blocker: blockerLabel(blocker) }
   }