Make the browser tools agree with each other

An audit of the module found the frame-descent bug was one instance of a
pattern: six independent definitions of 'is this editable' and seven of 'what
is focused', disagreeing with each other. A tool refusing what its sibling
accepts on identical page state is invisible at runtime — the agent follows a
snapshot that says one thing into a tool that says another.

- browser_type now accepts role="textbox" like browser_insert_text does. The
  snapshot advertises those elements as [textbox] with a ref, so refusing them
  meant rejecting exactly what the outline told the model to type into. Both
  the native and synthetic paths, and their descendant scans.
- pressKeyOnPage descends shadow roots and frames like every other focus
  reader. It was dispatching synthetic keys at the shadow host or <iframe>
  element, where they bubble but never reach the editor, while reporting
  success — and contradicting the activeElement reported beside it.
- not-editable and ambiguous-editable name what was found: the element's tag
  and role, and the candidate fields. Both had the data and discarded it, which
  is what turns one blocked step into twenty rounds of guessing.
- obstructedAfterNavigation requires a dialog that ARRIVED with the
  navigation. It compared against nothing, so every SPA route change under a
  persistent role=dialog reported a successful click as obstructed. The test
  that covered this asserted the false positive; it now pins both directions.
- browser_insert_text observes the top document when typing inside a frame,
  like every other input tool. A submit that navigates the top page was
  invisible to its frame-scoped observation.

Author: Sg312

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

@@ -1660,7 +1660,11 @@ describe('credential protection', () => {
     expect(mainFrame.executeJavaScript).not.toHaveBeenCalled()
   })
 
-  it('reports navigation that remains obstructed by a DOM dialog', async () => {
+  // A dialog that was ALREADY open before the click is not obstructing the
+  // 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.
+  it('ignores a dialog that was already open before the click', async () => {
     const contents = await openPage()
     let actionReads = 0
     vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => {
@@ -1695,13 +1699,52 @@ describe('credential protection', () => {
 
     const result = await driver.executeTool('chat-test', 'browser_click', { elementId: 0 })
 
+    expect(result).toMatchObject({
+      ok: true,
+      result: { effectObserved: true, obstructedAfterNavigation: false, dialogs: ['Search'] },
+    })
+  })
+
+  it('reports navigation obstructed by a dialog that opened with it', async () => {
+    const contents = await openPage()
+    let actionReads = 0
+    vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => {
+      if (isPageCall(expression, 'clickElement')) {
+        return Promise.resolve({ dispatched: false, x: 24, y: 48, element: 'Search result' })
+      }
+      if (isPageCall(expression, 'readActiveElementState')) return Promise.resolve({})
+      if (isPageCall(expression, 'readPageActionState')) {
+        actionReads++
+        return Promise.resolve(
+          actionReads === 1
+            ? {
+                url: 'https://example.com/search',
+                title: 'Search',
+                focus: 'body',
+                mutationRevision: 0,
+                dialogs: [],
+                scroll: [0],
+              }
+            : {
+                url: 'https://example.com/channel/eng-bugs',
+                title: 'eng-bugs',
+                focus: 'body',
+                mutationRevision: 1,
+                dialogs: ['Open in the Slack app?'],
+                scroll: [0],
+              }
+        )
+      }
+      return Promise.resolve(undefined)
+    })
+
+    const result = await driver.executeTool('chat-test', 'browser_click', { elementId: 0 })
+
     expect(result).toMatchObject({
       ok: true,
       result: {
-        effectObserved: true,
         obstructedAfterNavigation: true,
-        dialogs: ['Search'],
-        note: expect.stringContaining('dialog is still open'),
+        note: expect.stringContaining('Open in the Slack app?'),
       },
     })
   })

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

@@ -892,16 +892,29 @@ function unwrapPageResult(result: unknown): unknown {
       )
     }
     if (code === 'not-editable') {
-      throw new ToolError('That element is not a text input — pick an editable element.')
+      const tag = isRecordLike(result) ? String(result.elementTag ?? '') : ''
+      const role = isRecordLike(result) ? String(result.elementRole ?? '') : ''
+      const described = [tag ? `<${tag}>` : '', role ? `role="${role}"` : '']
+        .filter(Boolean)
+        .join(' ')
+      throw new ToolError(
+        `That element is not a text input${described ? ` (it is ${described})` : ''} — take a fresh browser_snapshot and target the editable field itself.`
+      )
     }
     if (code === 'outside-viewport') {
       throw new ToolError(
         'That point is outside the visible viewport. Coordinates are CSS pixels within the current viewport — when reading them off a browser_screenshot, divide image pixels by its scale, and scroll the target into view first.'
       )
     }
     if (code === 'ambiguous-editable') {
+      const candidates =
+        isRecordLike(result) && Array.isArray(result.candidates)
+          ? result.candidates.map(String).filter(Boolean)
+          : []
       throw new ToolError(
-        'That composite control contains multiple editable fields. Take a fresh browser_snapshot and target the exact field.'
+        `That composite control contains multiple editable fields${
+          candidates.length > 0 ? ` (${candidates.join(', ')})` : ''
+        }. Take a fresh browser_snapshot and target the exact field.`
       )
     }
     if (code === 'different') {
@@ -2344,10 +2357,21 @@ async function executeToolInner(
       )
       const navigated =
         observation.effect.urlChanged || topObservation.effect.urlChanged || tabChanged
-      const obstructedAfterNavigation = navigated && dialogs.length > 0
+      // Only a dialog that ARRIVED with the navigation obstructs it. Comparing
+      // against the union of what was already open stops the false positive
+      // that fires on every SPA route change under a persistent `role=dialog`
+      // (a cookie banner, a side drawer, an emoji picker) — those are not
+      // blocking anything, and reporting them made successful clicks read as
+      // failures.
+      const dialogsBefore = new Set([
+        ...(Array.isArray(beforePage.dialogs) ? beforePage.dialogs.map(String) : []),
+        ...(Array.isArray(beforeTopPage.dialogs) ? beforeTopPage.dialogs.map(String) : []),
+      ])
+      const newDialogs = dialogs.filter((dialog) => !dialogsBefore.has(dialog))
+      const obstructedAfterNavigation = navigated && newDialogs.length > 0
       const notes: string[] = []
       if (obstructedAfterNavigation) {
-        notes.push('The page navigated, but a dialog is still open above it.')
+        notes.push(`The page navigated, but a dialog opened above it (${newDialogs.join(', ')}).`)
       }
       if (!effectObserved) {
         notes.push(
@@ -3249,6 +3273,12 @@ async function executeToolInner(
       }
       const beforePage = await pageActionState(target, true)
       const beforeElement = await activeElementState(target)
+      // Observe the TOP document too when typing inside a frame. A submit that
+      // navigates the top page is invisible to a frame-scoped observation, so a
+      // successful send reported effectObserved: false. Newly reachable now
+      // that the focus check descends into frames at all.
+      const insertInFrame = target !== contents
+      const beforeTopPage = insertInFrame ? await pageActionState(contents, true) : beforePage
       assertCurrentExecution()
       assertActiveContents(contents, insertNavigationEpoch)
       assertFocusedTargetUnchanged(contents, target)
@@ -3275,11 +3305,16 @@ async function executeToolInner(
       const state = await activeElementState(target)
       const afterPage = await pageActionState(target)
       const observation = pageEffect(beforePage, afterPage, beforeElement, state)
+      const topObservation = insertInFrame
+        ? pageEffect(beforeTopPage, await pageActionState(contents, true), beforeElement, state)
+        : observation
       const effectObserved =
         observation.effect.fieldChanged ||
         observation.effect.urlChanged ||
         observation.effect.dialogChanged ||
-        observation.effect.targetChanged
+        observation.effect.targetChanged ||
+        topObservation.effect.urlChanged ||
+        topObservation.effect.dialogChanged
       return {
         dispatched: true,
         trusted: true,

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

@@ -261,8 +261,11 @@ describe('secret-field detection', () => {
     expect(typeIntoElement(0, 'change', false)).toEqual({ error: 'readonly' })
     expect(focusElementForTyping(1)).toEqual({ error: 'disabled' })
     expect(typeIntoElement(1, 'change', false)).toEqual({ error: 'disabled' })
-    expect(focusElementForTyping(2)).toEqual({ error: 'not-editable' })
-    expect(typeIntoElement(2, 'change', false)).toEqual({ error: 'not-editable' })
+    expect(focusElementForTyping(2)).toMatchObject({ error: 'not-editable', elementTag: 'input' })
+    expect(typeIntoElement(2, 'change', false)).toMatchObject({
+      error: 'not-editable',
+      elementTag: 'input',
+    })
   })
 
   it('detects a password field reached through a same-origin iframe', () => {
@@ -383,7 +386,12 @@ describe('combobox typing surfaces', () => {
     for (const input of Array.from(document.querySelectorAll('input'))) visible(input)
     register(ambiguous, secret)
 
-    expect(focusElementForTyping(0)).toEqual({ error: 'ambiguous-editable' })
+    // The candidate list is the whole point of this error — it is the only
+    // thing that lets the agent pick a narrower target.
+    expect(focusElementForTyping(0)).toMatchObject({
+      error: 'ambiguous-editable',
+      candidates: expect.arrayContaining([expect.stringContaining('input')]),
+    })
     expect(focusElementForTyping(1)).toEqual({ error: 'password' })
     expect(typeIntoElement(1, 'nope', false)).toEqual({ error: 'password' })
   })

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

@@ -1042,7 +1042,7 @@ export function clickElement(
       addCandidate(el)
       for (const candidate of Array.from(
         el.querySelectorAll<HTMLElement>(
-          'input, textarea, [contenteditable="true"], [contenteditable=""], [contenteditable="plaintext-only"]'
+          'input, textarea, [contenteditable="true"], [contenteditable=""], [contenteditable="plaintext-only"], [role="textbox"]'
         )
       )) {
         addCandidate(candidate)
@@ -1250,22 +1250,47 @@ export function focusElementForTyping(id: number, moveFocus = true): unknown {
       tag === 'TEXTAREA' ||
       (tag === 'INPUT' &&
         ['text', 'search', 'email', 'url', 'tel', 'number', 'password'].includes(inputType)) ||
-      (node as HTMLElement).isContentEditable
+      (node as HTMLElement).isContentEditable ||
+      // An ARIA-only textbox. The snapshot already advertises these as
+      // `[textbox]` with a ref, and browser_insert_text accepts them, so
+      // refusing here meant one tool rejecting exactly what the outline told
+      // the model to type into and what its sibling would have accepted.
+      node.getAttribute('role') === 'textbox'
     ) {
       potentialEditables.push(node as HTMLElement)
     }
   }
   addEditable(el)
   for (const candidate of Array.from(
     el.querySelectorAll<HTMLElement>(
-      'input, textarea, [contenteditable="true"], [contenteditable=""], [contenteditable="plaintext-only"]'
+      'input, textarea, [contenteditable="true"], [contenteditable=""], [contenteditable="plaintext-only"], [role="textbox"]'
     )
   )) {
     addEditable(candidate)
   }
   const editables = Array.from(new Set(potentialEditables))
-  if (editables.length === 0) return { error: 'not-editable' }
-  if (editables.length > 1) return { error: 'ambiguous-editable' }
+  if (editables.length === 0) {
+    // Describe the element instead of only refusing it. Without this the agent
+    // cannot tell "wrong ref" from "this tool cannot type here" and retries
+    // variations of the same failing call.
+    return {
+      error: 'not-editable',
+      elementTag: String(el.tagName || '').toLowerCase(),
+      ...(el.getAttribute('role') ? { elementRole: el.getAttribute('role') } : {}),
+    }
+  }
+  if (editables.length > 1) {
+    // The candidate list is right here; discarding it left the agent unable to
+    // pick a narrower target, which is the only recovery this error allows.
+    return {
+      error: 'ambiguous-editable',
+      candidates: editables.slice(0, 5).map((field) => {
+        const fieldTag = String(field.tagName || '').toLowerCase()
+        const label = field.getAttribute('aria-label') || field.getAttribute('placeholder') || ''
+        return label ? `${fieldTag} "${label}"` : fieldTag
+      }),
+    }
+  }
   const editable = editables[0]
   const editableTag = tagFor(editable)
 
@@ -1776,22 +1801,47 @@ export function typeIntoElement(id: number, text: string, submit: boolean): unkn
       candidateTag === 'TEXTAREA' ||
       (candidateTag === 'INPUT' &&
         ['text', 'search', 'email', 'url', 'tel', 'number', 'password'].includes(inputType)) ||
-      (node as HTMLElement).isContentEditable
+      (node as HTMLElement).isContentEditable ||
+      // An ARIA-only textbox. The snapshot already advertises these as
+      // `[textbox]` with a ref, and browser_insert_text accepts them, so
+      // refusing here meant one tool rejecting exactly what the outline told
+      // the model to type into and what its sibling would have accepted.
+      node.getAttribute('role') === 'textbox'
     ) {
       potentialEditables.push(node as HTMLElement)
     }
   }
   addEditable(el)
   for (const candidate of Array.from(
     el.querySelectorAll<HTMLElement>(
-      'input, textarea, [contenteditable="true"], [contenteditable=""], [contenteditable="plaintext-only"]'
+      'input, textarea, [contenteditable="true"], [contenteditable=""], [contenteditable="plaintext-only"], [role="textbox"]'
     )
   )) {
     addEditable(candidate)
   }
   const editables = Array.from(new Set(potentialEditables))
-  if (editables.length === 0) return { error: 'not-editable' }
-  if (editables.length > 1) return { error: 'ambiguous-editable' }
+  if (editables.length === 0) {
+    // Describe the element instead of only refusing it. Without this the agent
+    // cannot tell "wrong ref" from "this tool cannot type here" and retries
+    // variations of the same failing call.
+    return {
+      error: 'not-editable',
+      elementTag: String(el.tagName || '').toLowerCase(),
+      ...(el.getAttribute('role') ? { elementRole: el.getAttribute('role') } : {}),
+    }
+  }
+  if (editables.length > 1) {
+    // The candidate list is right here; discarding it left the agent unable to
+    // pick a narrower target, which is the only recovery this error allows.
+    return {
+      error: 'ambiguous-editable',
+      candidates: editables.slice(0, 5).map((field) => {
+        const fieldTag = String(field.tagName || '').toLowerCase()
+        const label = field.getAttribute('aria-label') || field.getAttribute('placeholder') || ''
+        return label ? `${fieldTag} "${label}"` : fieldTag
+      }),
+    }
+  }
   const editable = editables[0]
   const tag = String(editable.tagName || '').toUpperCase()
   editable.scrollIntoView({ block: 'nearest', inline: 'nearest', behavior: 'instant' })
@@ -1873,9 +1923,37 @@ export function pressKeyOnPage(
       .some((token) => token === 'current-password' || token === 'new-password')
   }
 
-  const target = (document.activeElement as HTMLElement | null) ?? document.body
+  // Descend to what is really focused, like every other focus reader here.
+  // Without this the synthetic key lands on the shadow HOST or the <iframe>
+  // element: the event bubbles from there but never enters the shadow tree or
+  // the frame document, so the editor's keymap never sees it — and the result
+  // still reports success. It also made this function's `target` disagree with
+  // the `activeElement` reported alongside it in the same tool result.
+  let target = (document.activeElement as HTMLElement | null) ?? document.body
+  for (let depth = 0; depth < 10; depth++) {
+    const shadow = target.shadowRoot
+    if (shadow?.activeElement) {
+      target = shadow.activeElement as HTMLElement
+      continue
+    }
+    const targetTag = String(target.tagName || '').toUpperCase()
+    if (targetTag === 'IFRAME' || targetTag === 'FRAME') {
+      try {
+        const inner = (target as HTMLIFrameElement).contentDocument
+        if (inner?.activeElement && inner.activeElement !== inner.body) {
+          target = inner.activeElement as HTMLElement
+          continue
+        }
+      } catch {
+        // Cross-origin frame — not inspectable; dispatch to the frame itself.
+      }
+    }
+    break
+  }
   // The driver checks focus before taking the trusted CDP path; this covers
   // the synthetic fallback, which is reached independently when CDP is down.
+  // Descending first is what makes this guard reach a password field nested in
+  // a shadow root or frame, rather than only inspecting the host.
   if (isSecretField(target)) return { error: 'password' }
   const opts = {
     bubbles: true,