Stop browser evals from hanging a tool call forever

resources/mcp-bridge.js

@@ -85,9 +85,18 @@ function send(msg) {
   process.stdout.write(JSON.stringify(msg) + '\n')
 }
 
-function callControl(method, path, body) {
+// Backstop only. Node's http client has no default request timeout, so a
+// control-server handler that never responds would hang the tool call — and
+// therefore the agent — forever. Sits well above every server-side timeout so
+// the server's own (more specific) error wins the race in the normal case.
+const CALL_TIMEOUT_MS = Number(process.env.HARNESS_CALL_TIMEOUT_MS) || 60_000
+// Worktree creation legitimately blocks on git fetch / PR checkout.
+const WORKTREE_CREATE_TIMEOUT_MS = 300_000
+
+function callControl(method, path, body, timeoutMs) {
   return new Promise((resolve, reject) => {
     const data = body ? JSON.stringify(body) : undefined
+    const limit = timeoutMs || CALL_TIMEOUT_MS
     const req = http.request(
       {
         host: '127.0.0.1',
@@ -117,12 +126,40 @@ function callControl(method, path, body) {
         })
       }
     )
+    req.setTimeout(limit, () => {
+      // The 'timeout' event only fires — it doesn't abort — so destroy first.
+      req.destroy(new Error('Ness did not respond within ' + limit + 'ms: ' + method + ' ' + path))
+    })
     req.on('error', reject)
     if (data) req.write(data)
     req.end()
   })
 }
 
+// Every other browser tool bounds its output (console logs at 200 entries,
+// clickables at 500 items, screenshots at JPEG q70). Raw outerHTML is 1-5MB on
+// a heavy page, which is a context-blowing tool result.
+const DOM_DEFAULT_MAX_BYTES = 100_000
+const DOM_HARD_MAX_BYTES = 2_000_000
+
+function truncateDom(html, maxBytes) {
+  const requested = Number(maxBytes)
+  const cap = Number.isFinite(requested) && requested > 0
+    ? Math.min(Math.round(requested), DOM_HARD_MAX_BYTES)
+    : DOM_DEFAULT_MAX_BYTES
+  const total = Buffer.byteLength(html, 'utf-8')
+  if (total <= cap) return html
+  const head = Buffer.from(html, 'utf-8').subarray(0, cap).toString('utf-8')
+  return (
+    head +
+    '\n<!-- truncated by Ness: showing the first ' +
+    cap +
+    ' of ' +
+    total +
+    ' bytes. Pass max_bytes to raise the cap, or use get_tab_clickables for a compact view. -->'
+  )
+}
+
 // Appended to create_worktree's description, and removed again by
 // stripForkAffordance when the feature is off. Kept as its own constant so the
 // two stay in sync — a literal that drifts would silently stop being stripped.
@@ -334,11 +371,15 @@ const TOOLS = [
   {
     name: 'get_tab_dom',
     description:
-      "Return the serialized outer HTML of the tab's document. Useful for inspecting rendered DOM that an HTTP fetch wouldn't see.",
+      "Return the serialized outer HTML of the tab's document. Useful for inspecting rendered DOM that an HTTP fetch wouldn't see. Truncated to 100KB by default — a heavy page's markup will blow your context otherwise, so prefer get_tab_clickables when you just need something to click.",
     inputSchema: {
       type: 'object',
       properties: {
-        tab_id: { type: 'string', description: 'Browser tab id from list_browser_tabs.' }
+        tab_id: { type: 'string', description: 'Browser tab id from list_browser_tabs.' },
+        max_bytes: {
+          type: 'number',
+          description: 'Truncate the markup to this many bytes. Default 100000, max 2000000.'
+        }
       },
       required: ['tab_id']
     }
@@ -656,18 +697,23 @@ async function handleToolCall(name, args) {
     ) {
       throw new Error('agentKind must be "claude" or "codex"')
     }
-    const r = await callControl('POST', '/worktrees', {
-      terminalId: TERMINAL_ID,
-      repoRoot: args.repoRoot,
-      branchName: args.branchName,
-      prNumber: prNumber,
-      baseBranch: args.baseBranch,
-      initialPrompt: args.initialPrompt,
-      agentKind: args.agentKind,
-      model: args.model,
-      alias: args.alias,
-      forkConversation: args.forkConversation === true
-    })
+    const r = await callControl(
+      'POST',
+      '/worktrees',
+      {
+        terminalId: TERMINAL_ID,
+        repoRoot: args.repoRoot,
+        branchName: args.branchName,
+        prNumber: prNumber,
+        baseBranch: args.baseBranch,
+        initialPrompt: args.initialPrompt,
+        agentKind: args.agentKind,
+        model: args.model,
+        alias: args.alias,
+        forkConversation: args.forkConversation === true
+      },
+      WORKTREE_CREATE_TIMEOUT_MS
+    )
     const agentLabel = args.agentKind === 'codex' ? 'Codex' : 'Claude'
     const modelSuffix = args.model ? ` (model: ${args.model})` : ''
     const aliasSuffix = args.alias && args.alias.trim() ? ` (alias: "${args.alias.trim()}")` : ''
@@ -785,7 +831,7 @@ async function handleToolCall(name, args) {
       '/browser/dom?tabId=' + encodeURIComponent(args.tab_id)
     )
     if (r == null || r.html == null) throw new Error(r && r.error ? r.error : 'dom read failed')
-    return r.html
+    return truncateDom(r.html, args.max_bytes)
   }
   if (name === 'get_tab_url') {
     if (!args || !args.tab_id) throw new Error('tab_id is required')

resources/mcp-bridge.test.js

@@ -40,13 +40,14 @@ function startStub(handler) {
   })
 }
 
-function spawnBridge(port, token) {
+function spawnBridge(port, token, extraEnv) {
   const proc = spawn(process.execPath, [BRIDGE], {
     env: {
       ...process.env,
       HARNESS_PORT: String(port),
       HARNESS_TOKEN: token,
-      HARNESS_TERMINAL_ID: 'test-terminal'
+      HARNESS_TERMINAL_ID: 'test-terminal',
+      ...extraEnv
     },
     stdio: ['pipe', 'pipe', 'pipe']
   })
@@ -307,6 +308,76 @@ describe('mcp-bridge create_worktree', () => {
   })
 })
 
+describe('mcp-bridge get_tab_dom', () => {
+  let stub
+  let bridge
+
+  afterEach(async () => {
+    if (bridge) await bridge.kill()
+    if (stub) await stub.close()
+  })
+
+  async function callGetDom(args, extraEnv) {
+    bridge.send({ jsonrpc: '2.0', id: 1, method: 'initialize', params: {} })
+    await bridge.next()
+    bridge.send({
+      jsonrpc: '2.0',
+      id: 2,
+      method: 'tools/call',
+      params: { name: 'get_tab_dom', arguments: args }
+    })
+    return bridge.next()
+  }
+
+  function serveDom(html) {
+    return startStub((req, body, res) => {
+      if (req.url === '/scope') {
+        res.writeHead(200, { 'Content-Type': 'application/json' })
+        return res.end(JSON.stringify({ scope: null, browser: { enabled: true, mode: 'full' } }))
+      }
+      res.writeHead(200, { 'Content-Type': 'application/json' })
+      res.end(JSON.stringify({ html }))
+    })
+  }
+
+  it('passes a small document through untouched', async () => {
+    stub = await serveDom('<html><body>hi</body></html>')
+    bridge = spawnBridge(stub.port, 'tok')
+    const response = await callGetDom({ tab_id: 'tab-1' })
+    expect(response.result.content[0].text).toBe('<html><body>hi</body></html>')
+  })
+
+  it('caps a heavy page so the markup cannot blow the caller context', async () => {
+    stub = await serveDom('<p>' + 'x'.repeat(500_000) + '</p>')
+    bridge = spawnBridge(stub.port, 'tok')
+    const text = (await callGetDom({ tab_id: 'tab-1' })).result.content[0].text
+    expect(text.length).toBeLessThan(101_000)
+    expect(text).toMatch(/truncated by Ness: showing the first 100000 of 500007 bytes/)
+  })
+
+  it('honours an explicit max_bytes', async () => {
+    stub = await serveDom('y'.repeat(5000))
+    bridge = spawnBridge(stub.port, 'tok')
+    const text = (await callGetDom({ tab_id: 'tab-1', max_bytes: 1000 })).result.content[0].text
+    expect(text.startsWith('y'.repeat(1000))).toBe(true)
+    expect(text).toMatch(/first 1000 of 5000 bytes/)
+  })
+
+  it('reports a timeout instead of hanging when the server never responds', async () => {
+    stub = await startStub((req, body, res) => {
+      if (req.url === '/scope') {
+        res.writeHead(200, { 'Content-Type': 'application/json' })
+        return res.end(JSON.stringify({ scope: null, browser: { enabled: true, mode: 'full' } }))
+      }
+      // Deliberately never respond — the pre-fix hang.
+    })
+    bridge = spawnBridge(stub.port, 'tok', { HARNESS_CALL_TIMEOUT_MS: '400' })
+    const response = await callGetDom({ tab_id: 'tab-1' })
+    expect(response.result.isError).toBe(true)
+    expect(response.result.content[0].text).toMatch(/did not respond within 400ms/)
+  }, 10_000)
+})
+
 describe('mcp-bridge log-size cap on startup', () => {
   let tmpDir
 

src/main/browser-eval.test.ts

@@ -0,0 +1,89 @@
+import { describe, expect, it, vi } from 'vitest'
+import { EVAL_TIMEOUT_MS, evalWithTimeout, evalBlockedReason } from './browser-eval'
+
+const LIVE = { hasDocument: true, lastLoadError: null, crashed: false, crashReason: null }
+
+describe('evalWithTimeout', () => {
+  it('resolves with the value when the eval settles', async () => {
+    await expect(evalWithTimeout(async () => 'html', 'getDom')).resolves.toBe('html')
+  })
+
+  it('rejects rather than hanging when the eval never settles', async () => {
+    vi.useFakeTimers()
+    try {
+      const pending = evalWithTimeout(() => new Promise<string>(() => {}), 'getDom tab=t1', 5000)
+      const assertion = expect(pending).rejects.toThrow(/getDom tab=t1 timed out after 5000ms/)
+      await vi.advanceTimersByTimeAsync(5000)
+      await assertion
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+
+  it('propagates a real eval failure unchanged', async () => {
+    await expect(
+      evalWithTimeout(async () => {
+        throw new Error('SyntaxError')
+      }, 'getDom')
+    ).rejects.toThrow('SyntaxError')
+  })
+
+  it('clears the timer on the success path so it cannot hold the event loop open', async () => {
+    vi.useFakeTimers()
+    try {
+      await evalWithTimeout(async () => 'ok', 'getDom')
+      expect(vi.getTimerCount()).toBe(0)
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+
+  it('defaults to a bound short enough to beat the caller running out of patience', () => {
+    expect(EVAL_TIMEOUT_MS).toBeLessThanOrEqual(10_000)
+  })
+})
+
+describe('evalBlockedReason', () => {
+  it('names the reload for a tab whose renderer died', () => {
+    expect(evalBlockedReason({ ...LIVE, crashed: true, crashReason: 'crashed' })).toBe(
+      'tab renderer crashed (reason: crashed) — reload the tab'
+    )
+  })
+
+  it('still reports a crash when the reason is unknown', () => {
+    expect(evalBlockedReason({ ...LIVE, crashed: true })).toBe(
+      'tab renderer crashed — reload the tab'
+    )
+  })
+
+  it('prefers the crash over the ERR_FAILED the crash also produced', () => {
+    expect(
+      evalBlockedReason({
+        hasDocument: false,
+        lastLoadError: 'ERR_FAILED (-2)',
+        crashed: true,
+        crashReason: 'oom'
+      })
+    ).toMatch(/renderer crashed \(reason: oom\)/)
+  })
+
+  it('reports the load failure for a tab that never committed a document', () => {
+    expect(
+      evalBlockedReason({
+        ...LIVE,
+        hasDocument: false,
+        lastLoadError: "ERR_FAILED (-2) loading 'http://localhost:8765/local.html'"
+      })
+    ).toBe(
+      "tab has no document loaded (last load failed: ERR_FAILED (-2) loading 'http://localhost:8765/local.html')"
+    )
+  })
+
+  it('allows the eval once a document has committed, even after an earlier failure', () => {
+    expect(evalBlockedReason({ ...LIVE, lastLoadError: 'ERR_FAILED (-2)' })).toBeNull()
+  })
+
+  it('allows the eval for a tab still on its first load, so it queues as before', () => {
+    expect(evalBlockedReason({ ...LIVE, hasDocument: false })).toBeNull()
+  })
+})

src/main/browser-eval.ts

@@ -0,0 +1,60 @@
+// Guards for evaluating JavaScript inside a browser tab. Shared by both
+// BrowserManagerLike implementations (Electron WebContentsView and
+// Playwright), so it deliberately imports nothing runtime-specific.
+
+/** Long enough for a slow real page's eval, far shorter than the patience of
+ * whoever is waiting on the MCP tool call. */
+export const EVAL_TIMEOUT_MS = 5_000
+
+export interface TabEvalState {
+  /** True once any document has committed in the main frame. */
+  hasDocument: boolean
+  /** Description of the last main-frame load failure, cleared on commit. */
+  lastLoadError: string | null
+  /** True while the tab's renderer process is gone. */
+  crashed: boolean
+  /** Why the renderer died, from `render-process-gone`. */
+  crashReason: string | null
+}
+
+/** `webContents.executeJavaScript` on a view with no live renderer — one whose
+ * main frame never committed, or whose renderer process died — neither
+ * resolves nor rejects. It queues for a frame that never arrives, so a
+ * try/catch around it can't rescue the caller. Race it against a timer. */
+export async function evalWithTimeout<T>(
+  run: () => Promise<T>,
+  what: string,
+  timeoutMs: number = EVAL_TIMEOUT_MS
+): Promise<T> {
+  let timer: NodeJS.Timeout | undefined
+  try {
+    return await Promise.race([
+      run(),
+      new Promise<never>((_, reject) => {
+        timer = setTimeout(
+          () => reject(new Error(`${what} timed out after ${timeoutMs}ms`)),
+          timeoutMs
+        )
+      })
+    ])
+  } finally {
+    clearTimeout(timer)
+  }
+}
+
+/** The actionable reason a tab can never be evaluated, or null when evaluating
+ * is worth attempting. Beats waiting out the timeout: it names the fix
+ * (reload the tab / the URL was dead) instead of just the symptom.
+ *
+ * A tab mid-first-load has no document yet but no error either — its eval
+ * queues until the frame commits, which is the behaviour callers want. */
+export function evalBlockedReason(state: TabEvalState): string | null {
+  if (state.crashed) {
+    const why = state.crashReason ? ` (reason: ${state.crashReason})` : ''
+    return `tab renderer crashed${why} — reload the tab`
+  }
+  if (!state.hasDocument && state.lastLoadError) {
+    return `tab has no document loaded (last load failed: ${state.lastLoadError})`
+  }
+  return null
+}