fix(desktop-browser): ask the user before a page's alert, confirm, or leave-site prompt is answered

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

@@ -61,11 +61,22 @@ function createOopifFrameFixture() {
   }
 }
 
+/** Callbacks for a page with no user to ask, so the shell answers every dialog. */
+const shellAnswersDialogs = {
+  claimUserDialog: () => false,
+  onDialogClosed: () => {},
+  claimUserLeave: () => false,
+}
+
 describe('browser-agent CDP instrumentation', () => {
   it('leaves file chooser dialogs native so users can upload files', async () => {
     const contents = new WebContentsView().webContents
 
-    await ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
+    await ensureInstrumented(contents, {
+      onDialog: vi.fn(),
+      dialogResponse: () => null,
+      ...shellAnswersDialogs,
+    })
 
     expect(contents.debugger.sendCommand).toHaveBeenCalledWith('Page.enable', undefined)
     expect(contents.debugger.sendCommand).not.toHaveBeenCalledWith(
@@ -86,10 +97,18 @@ describe('browser-agent CDP instrumentation', () => {
     })
 
     await expect(
-      ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
+      ensureInstrumented(contents, {
+        onDialog: vi.fn(),
+        dialogResponse: () => null,
+        ...shellAnswersDialogs,
+      })
     ).rejects.toThrow('setup acknowledgement lost')
     await expect(
-      ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
+      ensureInstrumented(contents, {
+        onDialog: vi.fn(),
+        dialogResponse: () => null,
+        ...shellAnswersDialogs,
+      })
     ).resolves.toBeUndefined()
 
     expect(autoAttachAttempts).toBe(2)
@@ -98,7 +117,11 @@ describe('browser-agent CDP instrumentation', () => {
   it('dismisses an OOPIF dialog on the flattened child session', async () => {
     const contents = new WebContentsView().webContents
     const onDialog = vi.fn()
-    await ensureInstrumented(contents, { onDialog, dialogResponse: () => null })
+    await ensureInstrumented(contents, {
+      onDialog,
+      dialogResponse: () => null,
+      ...shellAnswersDialogs,
+    })
     const listener = vi
       .mocked(contents.debugger.on)
       .mock.calls.find(([event]) => event === 'message')?.[1] as
@@ -132,7 +155,7 @@ describe('browser-agent CDP instrumentation', () => {
     const contents = new WebContentsView().webContents
     const onDialog = vi.fn()
     const dialogResponse = vi.fn(() => ({ accept: true }))
-    await ensureInstrumented(contents, { onDialog, dialogResponse })
+    await ensureInstrumented(contents, { onDialog, dialogResponse, ...shellAnswersDialogs })
     const listener = vi
       .mocked(contents.debugger.on)
       .mock.calls.find(([event]) => event === 'message')?.[1] as
@@ -275,7 +298,11 @@ describe('browser-agent CDP instrumentation', () => {
     async (treeKind) => {
       const contents = new WebContentsView().webContents
       const { child, frameTree } = createOopifFrameFixture()
-      await ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
+      await ensureInstrumented(contents, {
+        onDialog: vi.fn(),
+        dialogResponse: () => null,
+        ...shellAnswersDialogs,
+      })
       const listener = vi
         .mocked(contents.debugger.on)
         .mock.calls.find(([event]) => event === 'message')?.[1] as
@@ -378,7 +405,11 @@ describe('browser-agent CDP instrumentation', () => {
   it('falls back to the root target when OOPIF isolated-world creation fails', async () => {
     const contents = new WebContentsView().webContents
     const { child, frameTree } = createOopifFrameFixture()
-    await ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
+    await ensureInstrumented(contents, {
+      onDialog: vi.fn(),
+      dialogResponse: () => null,
+      ...shellAnswersDialogs,
+    })
     const listener = vi
       .mocked(contents.debugger.on)
       .mock.calls.find(([event]) => event === 'message')?.[1] as
@@ -458,7 +489,11 @@ describe('browser-agent file input handles', () => {
   async function fileInputFixture(childSession = false) {
     const contents = new WebContentsView().webContents
     const { child, frameTree } = createOopifFrameFixture()
-    await ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
+    await ensureInstrumented(contents, {
+      onDialog: vi.fn(),
+      dialogResponse: () => null,
+      ...shellAnswersDialogs,
+    })
     if (childSession) {
       const onMessage = vi.mocked(contents.debugger.on).mock.calls[0]?.[1] as
         | ((event: unknown, method: string, params: unknown, sessionId?: string) => void)

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

@@ -13,6 +13,7 @@ import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
 import { interruptibleSleep } from '@sim/utils/helpers'
 import { isRecordLike } from '@sim/utils/object'
+import { truncateAtCodePoint } from '@sim/utils/string'
 import type { NativeImage, WebContents, WebFrameMain } from 'electron'
 
 const logger = createLogger('BrowserAgentCdp')
@@ -38,8 +39,14 @@ export interface DialogResponse {
 export interface CdpCallbacks {
   /** A JS dialog was handled; the driver surfaces it to the model. */
   onDialog: (dialog: PageDialog) => void
-  /** The running action's requested answer; dialogs are dismissed when it has none. */
+  /** The running action's requested answer; defaults to declining the dialog. */
   dialogResponse: () => DialogResponse | null
+  /** Leaves a visible user-owned alert or confirm to Electron's native dialog. */
+  claimUserDialog: () => boolean
+  /** The dialog ended through a user answer, a CDP answer, or page teardown. */
+  onDialogClosed: () => void
+  /** True when the user, not the shell, decides this beforeunload. */
+  claimUserLeave: () => boolean
 }
 
 /** Per-tab callbacks, so a background tab's events reach ITS driver, not the
@@ -189,37 +196,55 @@ function handleDebuggerEvent(
     return
   }
   const callbacks = callbacksByContents.get(contents)
+  if (method === 'Page.javascriptDialogClosed') {
+    callbacks?.onDialogClosed()
+    return
+  }
   if (method === 'Page.javascriptDialogOpening') {
     const type = String(params.type ?? 'dialog')
-    const message = String(params.message ?? '').slice(0, 500)
-    // Dialogs never stay open: beforeunload is accepted (navigation proceeds),
-    // and alert/confirm follow the running action's requested answer, defaulting
-    // to dismissal so an unexpected dialog can never block the page.
-    const accept = type === 'beforeunload' || callbacks?.dialogResponse()?.accept === true
-    const answer = { accept }
-    void (async () => {
-      let handled = false
-      try {
-        await send(contents, 'Page.handleJavaScriptDialog', answer, parentSessionId)
-        handled = true
-      } catch {
-        // Some Chromium builds surface an OOPIF's tab-modal dialog on its
-        // flattened session but accept the answer only on the root target.
-        if (parentSessionId) {
-          try {
-            await send(contents, 'Page.handleJavaScriptDialog', answer)
-            handled = true
-          } catch {}
-        }
-      }
+    const rawMessage = String(params.message ?? '')
+    const message = truncateAtCodePoint(rawMessage, 500, '')
+    const requested = callbacks?.dialogResponse() ?? null
+    if (!requested && (type === 'alert' || type === 'confirm') && callbacks?.claimUserDialog()) {
+      return
+    }
+    if (type === 'beforeunload' && callbacks?.claimUserLeave()) {
+      // The user's Leave replays the navigation; this unload stays cancelled.
+      void answerDialog(contents, { accept: false }, parentSessionId)
+      return
+    }
+    // CDP unblocks JavaScript, but Electron may retain the native dialog until navigation.
+    const accept = type === 'beforeunload' || requested?.accept === true
+    void answerDialog(contents, { accept }, parentSessionId).then((handled) => {
       if (handled) logger.info('Handled page dialog', { type, accept })
       else logger.warn('Could not handle page dialog', { type })
       callbacks?.onDialog({ type, message, handled, accepted: handled && accept })
-    })()
+    })
     return
   }
 }
 
+async function answerDialog(
+  contents: WebContents,
+  answer: { accept: boolean },
+  parentSessionId: string | undefined
+): Promise<boolean> {
+  try {
+    await send(contents, 'Page.handleJavaScriptDialog', answer, parentSessionId)
+    return true
+  } catch {
+    // Some Chromium builds surface an OOPIF's tab-modal dialog on its
+    // flattened session but accept the answer only on the root target.
+    if (!parentSessionId) return false
+    try {
+      await send(contents, 'Page.handleJavaScriptDialog', answer)
+      return true
+    } catch {
+      return false
+    }
+  }
+}
+
 interface ProtocolFrame {
   id: string
   parentId?: string

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

@@ -554,6 +554,7 @@ function recordNotice(notice: string): void {
 function pageStateFor(contents: WebContents, tabId: string): BrowserPageState {
   const issue = session.pageIssueForContents(contents)
   const mediaPermissionRequest = session.mediaPermissionRequestForContents(contents)
+  const dialog = session.pageDialogForContents(contents)
   return {
     scopeId: session.getBrowserScopeId(),
     tabId,
@@ -564,6 +565,7 @@ function pageStateFor(contents: WebContents, tabId: string): BrowserPageState {
     canGoForward: session.canGoForward(contents),
     ...(issue ? { issue } : {}),
     ...(mediaPermissionRequest ? { mediaPermissionRequest } : {}),
+    ...(dialog ? { dialog } : {}),
   }
 }
 
@@ -615,6 +617,10 @@ function instrumentTab(contents: WebContents): void {
         const requested = driverScopeState().dialogResponse
         return requested?.contents === contents ? requested.response : null
       }),
+    claimUserDialog: () =>
+      session.withBrowserScope(scopeId, () => session.claimUserDialog(contents)),
+    onDialogClosed: inScope(() => session.notePageDialogClosed(contents)),
+    claimUserLeave: () => session.withBrowserScope(scopeId, () => session.claimUserLeave(contents)),
   }
   void (async () => {
     let lastError: unknown
@@ -5221,6 +5227,14 @@ export async function executeTool(
       ) {
         throw new ToolError('This browser action was cancelled before it started.')
       }
+      session.withBrowserScope(resolvedScopeId, () => {
+        const automation = session.automationTab()
+        if (automation && session.hasPendingPageDialog(automation.view.webContents)) {
+          throw new ToolError(
+            'The user is answering a dialog on this page. Wait for their answer before using it.'
+          )
+        }
+      })
       state.activeToolCallId = toolCallId ?? null
       const executionController = new AbortController()
       let cancelActiveExecution: () => void = () => {}
@@ -5456,6 +5470,16 @@ export async function handlePanelAction(
       }
       return
     }
+    if (action.action === 'enable-page-dialogs') {
+      session.enablePageDialogs()
+      return
+    }
+    if (action.action === 'respond-dialog') {
+      if (typeof action.requestId === 'string' && typeof action.allowed === 'boolean') {
+        session.respondToPageDialog(action.requestId, action.allowed)
+      }
+      return
+    }
     if (action.action === 'respond-site-permission') {
       /** Older renderers can still send a response to the retired task-navigation prompt. */
       return
@@ -5471,8 +5495,11 @@ export async function handlePanelAction(
           action.url,
           { agentOwned: false }
         )
-        session.prepareExplicitNavigation(contents)
-        void contents.loadURL(action.url).catch(() => {})
+        const url = action.url
+        session.navigateForUser(contents, () => {
+          session.prepareExplicitNavigation(contents)
+          void contents.loadURL(url).catch(() => {})
+        })
         session.focusPageForUser(contents)
       }
       return
@@ -5495,13 +5522,13 @@ export async function handlePanelAction(
     const contents = tab.view.webContents
     switch (action.action) {
       case 'reload':
-        session.reloadPage(contents)
+        session.navigateForUser(contents, () => session.reloadPage(contents))
         return
       case 'back':
-        session.goBack(contents)
+        session.navigateForUser(contents, () => session.goBack(contents))
         return
       case 'forward':
-        session.goForward(contents)
+        session.navigateForUser(contents, () => session.goForward(contents))
         return
       case 'print':
         contents.print({ printBackground: true })