fix(browser): close the edge cases review found in the desktop agent changes

- A held right-click renews its agent context-menu marker before release, so
  Windows' release-time menu stays suppressed after a hold over one second.
- A failed chord releases only the keys whose press was attempted.
- A disk probe that resolves after a download completed can no longer cancel it.
- A disablePortal modal nested inside another open modal is picked as topmost.
- holdMs is an integer bounded to 0-10000 in the tool contract.

Author: waleedlatif1

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

@@ -14,6 +14,7 @@ import {
 import {
   captureScreenshot,
   clickAt,
+  consumeAgentContextMenu,
   ensureInstrumented,
   evaluateInIsolatedFrame,
   insertText,
@@ -275,6 +276,28 @@ describe('browser-agent CDP instrumentation', () => {
     }
   })
 
+  it('keeps a held right-click marked as the agent context menu until release', async () => {
+    const contents = new WebContentsView().webContents
+    vi.useFakeTimers()
+    try {
+      const rightHold = { ...PRIMARY_CLICK, button: 'right' as const, holdMs: 1500 }
+      await Promise.all([
+        clickAt(contents, 5, 6, false, rightHold),
+        vi.advanceTimersByTimeAsync(1500),
+      ])
+      expect(consumeAgentContextMenu(contents)).toBe(true)
+
+      const click = clickAt(contents, 5, 6, false, rightHold)
+      await vi.advanceTimersByTimeAsync(0)
+      expect(consumeAgentContextMenu(contents)).toBe(true)
+      await vi.advanceTimersByTimeAsync(1500)
+      await click
+      expect(consumeAgentContextMenu(contents)).toBe(false)
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+
   it('releases the mouse after a partial click failure', async () => {
     const contents = new WebContentsView().webContents
     vi.mocked(contents.debugger.sendCommand)

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

@@ -1012,7 +1012,14 @@ export async function clickAt(
         modifiers,
         clickCount: count,
       })
-      if (holdMs > 0) await sleep(holdMs)
+      if (holdMs > 0) {
+        await sleep(holdMs)
+        // Windows opens the context menu on release, after the hold; renew a marker a
+        // press-time menu has not already consumed.
+        if (button === 'right' && agentContextClicks.has(contents)) {
+          agentContextClicks.set(contents, Date.now())
+        }
+      }
       await sendInput(contents, 'Input.dispatchMouseEvent', {
         type: 'mouseReleased',
         x,

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

@@ -1,3 +1,4 @@
+import { toRecord } from '@sim/utils/object'
 import { describe, expect, it, vi } from 'vitest'
 
 vi.mock('electron', () => import('@/test/electron-mock'))
@@ -312,6 +313,23 @@ describe('dispatchKeyCombo', () => {
     ])
   })
 
+  it('releases only the chord keys whose press was attempted', async () => {
+    const contents = new WebContentsView().webContents
+    vi.mocked(contents.debugger.sendCommand)
+      .mockResolvedValueOnce({})
+      .mockRejectedValueOnce(new Error('shift-down response lost'))
+
+    await expect(dispatchKeyCombo(contents, parseKeyCombo('Ctrl+Shift+K'))).rejects.toMatchObject({
+      name: KeyDispatchError.name,
+      keyDownDispatched: true,
+    })
+
+    const keys = vi
+      .mocked(contents.debugger.sendCommand)
+      .mock.calls.map(([, params]) => `${toRecord(params).type} ${toRecord(params).key}`)
+    expect(keys).toEqual(['rawKeyDown Control', 'rawKeyDown Shift', 'keyUp Shift', 'keyUp Control'])
+  })
+
   it('does not turn menu-restoration cleanup failure into a duplicate key retry signal', async () => {
     const contents = new WebContentsView().webContents
     vi.mocked(contents.setIgnoreMenuShortcuts).mockImplementation((ignored: boolean) => {

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

@@ -372,28 +372,37 @@ export async function dispatchKeyCombo(contents: WebContents, combo: ParsedCombo
     if (depth === 0) contents.setIgnoreMenuShortcuts(true)
     applicationMenuIsolationDepth.set(contents, depth + 1)
   }
-  let keyDownDispatched = false
+  const presses = [...modifierKeys.downs, down]
+  let pressesAttempted = 0
   try {
-    // Mark before awaiting: Blink may receive the key-down and then lose the
+    // Count before awaiting: Blink may receive a key-down and then lose the
     // CDP acknowledgement during navigation/process swap. In that ambiguous
     // case cleanup is required and a synthetic retry could double-act.
-    keyDownDispatched = true
-    for (const event of modifierKeys.downs) await cdp.dispatchKeyEvent(contents, event)
-    await cdp.dispatchKeyEvent(contents, down)
+    for (const press of presses) {
+      pressesAttempted++
+      await cdp.dispatchKeyEvent(contents, press)
+    }
     await cdp.dispatchKeyEvent(contents, up)
     for (const event of modifierKeys.ups) await cdp.dispatchKeyEvent(contents, event)
   } catch (error) {
-    if (keyDownDispatched && !contents.isDestroyed()) {
+    if (pressesAttempted > 0 && !contents.isDestroyed()) {
       // Like pointer cleanup, this is best effort. The original key-up may
       // have reached Blink before its CDP response was lost; a duplicate
       // release is harmless, while omitting it can leave input state stuck.
-      for (const release of [up, ...modifierKeys.ups]) {
+      // Only keys whose press was attempted are released: a key-up for a key
+      // that never went down can itself trigger a page's keyup handler.
+      const heldModifiers = Math.min(pressesAttempted, modifierKeys.downs.length)
+      const releases = [
+        ...(pressesAttempted === presses.length ? [up] : []),
+        ...modifierKeys.ups.slice(modifierKeys.ups.length - heldModifiers),
+      ]
+      for (const release of releases) {
         await cdp.dispatchKeyEvent(contents, release).catch(() => {})
       }
     }
     throw new KeyDispatchError(
       getErrorMessage(error, 'Trusted key dispatch failed'),
-      keyDownDispatched
+      pressesAttempted > 0
     )
   } finally {
     if (isolatesApplicationMenu) {

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

@@ -2536,4 +2536,19 @@ describe('modal hidden together with its own app root', () => {
     expect(outline).not.toContain('Discard')
     expect((readPageActionState() as { dialogs: string[] }).dialogs).toEqual(['Upper'])
   })
+
+  it('exposes a disablePortal modal nested inside another open modal', () => {
+    document.body.innerHTML = `
+      <div id="__next" aria-hidden="true"><main>
+        <div role="dialog" aria-modal="true" aria-label="Outer"><button>Discard</button>
+          <div role="dialog" aria-modal="true" aria-label="Inner"><button>Confirm</button></div>
+        </div>
+      </main></div>`
+    showAll()
+
+    const outline = outlineOf(collectSnapshot())
+
+    expect(outline).toContain('Confirm')
+    expect(outline).not.toContain('Discard')
+  })
 })

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

@@ -71,7 +71,7 @@ export function installPageHelpers(): void {
    * mounts inside the app root it just hid, so its own ancestor carries aria-hidden. Visibility
    * checks skip only the aria-hidden test on ancestors above this modal. Null when any modal is
    * visible unmodified (the portaled case) or no single topmost modal is contained by all of its
-   * hidden ancestors.
+   * hidden ancestors. A modal nested inside another open modal is the topmost of the two.
    */
   let exemptModal: Element | null | undefined
   window.__simAgentFindExemptModal = (): Element | null => {
@@ -109,8 +109,10 @@ export function installPageHelpers(): void {
       if (visible) rendered.push({ modal, hidden })
     }
     if (rendered.some(({ hidden }) => hidden.length === 0)) return null
-    const topmost = rendered.filter(({ hidden }) =>
-      hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal)))
+    const topmost = rendered.filter(
+      ({ modal, hidden }) =>
+        hidden.every((ancestor) => rendered.every((other) => ancestor.contains(other.modal))) &&
+        !rendered.some((other) => other.modal !== modal && modal.contains(other.modal))
     )
     return topmost.length === 1 ? topmost[0].modal : null
   }

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

@@ -3495,6 +3495,44 @@ describe('browser-agent session', () => {
     })
   })
 
+  it('ignores a progress disk probe that resolves after the download completed', async () => {
+    vi.useFakeTimers({ toFake: ['Date'] })
+    try {
+      const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
+      const progressProbe = deferred<number>()
+      const getFreeDiskBytes = vi
+        .fn<(directory: string) => number | Promise<number>>()
+        .mockReturnValueOnce(Number.MAX_SAFE_INTEGER)
+        .mockReturnValueOnce(progressProbe.promise)
+      session = freshSession(win, {}, undefined, {
+        getDirectory: () => directory,
+        getFreeDiskBytes,
+      })
+      const contents = (session.ensureTab().view as unknown as MockView).webContents
+      const download = mockDownloadItem({ filename: 'late-probe.bin', totalBytes: 100 })
+
+      startMockDownload(contents, download)
+      await vi.waitFor(() => expect(download.item.resume).toHaveBeenCalledOnce())
+      vi.setSystemTime(Date.now() + 1_000)
+      download.emitUpdated()
+      expect(getFreeDiskBytes).toHaveBeenCalledTimes(2)
+
+      download.emitDone('completed')
+      progressProbe.resolve(0)
+
+      await vi.waitFor(() =>
+        expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({
+          filename: 'late-probe.bin',
+          state: 'completed',
+        })
+      )
+      expect(download.item.cancel).not.toHaveBeenCalled()
+      expect(finishedDownloadFiles(directory)).toEqual(['late-probe.bin'])
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+
   it('interrupts a completed download whose staging file cannot be moved into place', async () => {
     const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
     session = freshSession(win, {}, undefined, {

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

@@ -647,6 +647,16 @@ function publishActiveBrowserDownload(active: ActiveBrowserDownload): void {
   publishBrowserDownloads(liveScopeId)
 }
 
+/** A disk probe that resolves after its download finished or stopped must not cancel it. */
+function isDiskCheckStale(active: ActiveBrowserDownload): boolean {
+  return (
+    active.terminal ||
+    active.finished ||
+    Boolean(active.limitReason) ||
+    !activeBrowserDownloads.has(active)
+  )
+}
+
 function checkBrowserDownloadDiskSpace(
   active: ActiveBrowserDownload,
   check: 'admission' | 'progress',
@@ -664,9 +674,7 @@ function checkBrowserDownloadDiskSpace(
   active.diskCheckInFlight = true
   void browserDownloadFreeDiskBytes(active.directory)
     .then((freeDiskBytes) => {
-      if (active.terminal || active.limitReason || !activeBrowserDownloads.has(active)) {
-        return
-      }
+      if (isDiskCheckStale(active)) return
       const requiredFreeDiskBytes =
         MIN_BROWSER_DOWNLOAD_FREE_DISK_BYTES +
         activeDownloadReservations(check === 'admission' ? active : undefined)
@@ -683,7 +691,7 @@ function checkBrowserDownloadDiskSpace(
       if (check === 'admission' && active.download.state === 'progressing') active.item.resume()
     })
     .catch((error) => {
-      if (active.terminal || active.limitReason || !activeBrowserDownloads.has(active)) return
+      if (isDiskCheckStale(active)) return
       logger.warn('Could not complete an agent browser download disk-space check', {
         error: getErrorMessage(error),
       })

apps/sim/lib/mothership/generated/tool-catalog-v1.ts

@@ -530,9 +530,11 @@ export const BrowserClick: ToolCatalogEntry = {
           "The element id to act on (from the current tab's most recent browser_snapshot). Treat refs as invalid across tab switches or later snapshots.",
       },
       holdMs: {
-        type: 'number',
+        type: 'integer',
         description:
           'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.',
+        minimum: 0,
+        maximum: 10000,
       },
       modifiers: {
         type: 'array',
@@ -673,9 +675,11 @@ export const BrowserClickAt: ToolCatalogEntry = {
         required: ['accept'],
       },
       holdMs: {
-        type: 'number',
+        type: 'integer',
         description:
           'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.',
+        minimum: 0,
+        maximum: 10000,
       },
       modifiers: {
         type: 'array',

apps/sim/lib/mothership/generated/tool-schemas-v1.ts

@@ -219,9 +219,11 @@ export const TOOL_RUNTIME_SCHEMAS: Record<string, ToolRuntimeSchemaEntry> = {
             "The element id to act on (from the current tab's most recent browser_snapshot). Treat refs as invalid across tab switches or later snapshots.",
         },
         holdMs: {
-          type: 'number',
+          type: 'integer',
           description:
             'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.',
+          minimum: 0,
+          maximum: 10000,
         },
         modifiers: {
           type: 'array',
@@ -396,9 +398,11 @@ export const TOOL_RUNTIME_SCHEMAS: Record<string, ToolRuntimeSchemaEntry> = {
           required: ['accept'],
         },
         holdMs: {
-          type: 'number',
+          type: 'integer',
           description:
             'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.',
+          minimum: 0,
+          maximum: 10000,
         },
         modifiers: {
           type: 'array',