fix: address review feedback for PR #505

Author: shadowfax92

packages/browseros-agent/.llm/tmp_review_feedback.md

@@ -0,0 +1,13 @@
+## Review Comment Evaluation
+
+### Comment 1: [packages/browseros-agent/apps/server/src/api/services/sdk/browser.ts:128] — greptile-apps[bot]
+> Silent fall-through when `windowId` navigation fails completely.
+**Verdict: ACCEPT**
+**Reasoning:** The comment identifies a real behavioral regression. If the caller specifies `windowId`, `newPage(url, { windowId })` fails, and there is no fallback page, the current code falls through to the generic `newPage(url)` path and silently drops the window constraint.
+**Action:** Preserve the failure instead of silently ignoring the requested window. Re-throw the underlying window-targeted error when no fallback page exists, and add a focused unit test for that path.
+
+### Comment 2: [packages/browseros-agent/apps/server/tests/api/routes/health.test.ts:12] — greptile summary
+> Test asserts default fallback, not real CDP connectivity.
+**Verdict: ACCEPT**
+**Reasoning:** This is a valid test-quality concern even though it appeared in the summary rather than as a standalone inline comment. The existing test only exercised the `?? true` fallback and would miss a regression in the actual `browser.isCdpConnected()` path.
+**Action:** Update the route test to pass a mock browser and cover both connected and disconnected CDP states.

packages/browseros-agent/apps/eval/scripts/debug-mcp.ts

@@ -16,6 +16,22 @@ interface TestResult {
   error?: string
 }
 
+interface TextContentItem {
+  type: string
+  text?: string
+}
+
+interface StructuredWindowResult {
+  windowId?: number
+  tabId?: number
+}
+
+interface McpToolResult {
+  isError?: boolean
+  content?: TextContentItem[]
+  structuredContent?: StructuredWindowResult
+}
+
 const results: TestResult[] = []
 
 async function checkHealth(): Promise<boolean> {
@@ -67,13 +83,15 @@ async function callMcpTool(
       ),
     )
 
-    const result = await Promise.race([toolPromise, timeoutPromise])
+    const result = (await Promise.race([
+      toolPromise,
+      timeoutPromise,
+    ])) as McpToolResult
     const duration = Date.now() - start
 
-    if ((result as any).isError) {
+    if (result.isError) {
       const errorText =
-        (result as any).content?.find((c: any) => c.type === 'text')?.text ||
-        'Unknown error'
+        result.content?.find((c) => c.type === 'text')?.text || 'Unknown error'
       return { success: false, error: errorText, duration }
     }
 
@@ -161,15 +179,16 @@ async function main() {
     })
     if (!res.success) throw new Error(res.error)
 
-    const structured = (res.result as any)?.structuredContent
+    const structured = res.result as StructuredWindowResult | undefined
     windowId = structured?.windowId
     tabId = structured?.tabId
 
     if (!windowId || !tabId) {
       // Try parsing from text
       const text =
-        (res.result as any)?.content?.find((c: any) => c.type === 'text')
-          ?.text || ''
+        (res.result as McpToolResult | undefined)?.content?.find(
+          (c) => c.type === 'text',
+        )?.text || ''
       const windowMatch = text.match(/window\s+(\d+)/i)
       const tabMatch = text.match(/tab\s+(?:ID:\s*)?(\d+)/i)
       if (windowMatch) windowId = parseInt(windowMatch[1], 10)

packages/browseros-agent/apps/eval/scripts/test-lifecycle.ts

@@ -28,6 +28,16 @@ const MCP_URL = `http://127.0.0.1:${EVAL_PORTS.server}/mcp`
 
 let currentServerPid: number | null = null
 
+interface TextContentItem {
+  type: string
+  text?: string
+}
+
+interface McpToolResult {
+  isError?: boolean
+  content?: TextContentItem[]
+}
+
 // ============================================================================
 // Utility Functions (same as parallel-executor)
 // ============================================================================
@@ -193,7 +203,7 @@ async function callMcpTool(
   name: string,
   args: Record<string, unknown> = {},
   timeoutMs = 60000,
-): Promise<{ success: boolean; result?: any; error?: string }> {
+): Promise<{ success: boolean; result?: McpToolResult; error?: string }> {
   const client = new Client({ name: 'lifecycle-test', version: '1.0.0' })
   const transport = new StreamableHTTPClientTransport(new URL(MCP_URL))
 
@@ -206,12 +216,14 @@ async function callMcpTool(
         timeoutMs,
       ),
     )
-    const result = await Promise.race([toolPromise, timeoutPromise])
+    const result = (await Promise.race([
+      toolPromise,
+      timeoutPromise,
+    ])) as McpToolResult
 
-    if ((result as any).isError) {
+    if (result.isError) {
       const errorText =
-        (result as any).content?.find((c: any) => c.type === 'text')?.text ||
-        'Unknown error'
+        result.content?.find((c) => c.type === 'text')?.text || 'Unknown error'
       return { success: false, error: errorText }
     }
     return { success: true, result }

packages/browseros-agent/apps/eval/src/agents/single-agent.ts

@@ -4,6 +4,7 @@ import type { ResolvedAgentConfig } from '@browseros/server/agent/types'
 import { Browser } from '@browseros/server/browser'
 import { CdpBackend } from '@browseros/server/browser/backends/cdp'
 import { registry } from '@browseros/server/tools/registry'
+import type { UIMessageStreamEvent } from '@browseros/shared/schemas/ui-stream'
 import { DEFAULT_TIMEOUT_MS } from '../constants'
 import type { EvalConfig, TaskMetadata } from '../types'
 import { resolveProviderConfig } from '../utils/resolve-provider-config'
@@ -114,38 +115,44 @@ export class SingleAgentEvaluator implements AgentEvaluator {
             onStepFinish: async ({ toolCalls, toolResults, text }) => {
               if (toolCalls) {
                 for (const tc of toolCalls) {
-                  const inputEvent = {
+                  const inputEvent: UIMessageStreamEvent = {
                     type: 'tool-input-available',
                     toolCallId: tc.toolCallId,
                     toolName: tc.toolName,
                     input: tc.input,
-                  } as any
+                  }
                   await capture.messageLogger.logStreamEvent(inputEvent)
                   capture.emitEvent(task.query_id, inputEvent)
                 }
               }
 
               if (toolResults) {
                 for (const tr of toolResults) {
-                  const outputEvent = {
+                  const outputEvent: UIMessageStreamEvent = {
                     type: 'tool-output-available',
                     toolCallId: tr.toolCallId,
                     output: tr.output,
-                  } as any
+                  }
                   await capture.messageLogger.logStreamEvent(outputEvent)
                   capture.emitEvent(task.query_id, outputEvent)
                 }
               }
 
               if (text) {
                 const textId = randomUUID()
-                const startEvent = { type: 'text-start', id: textId } as any
-                const deltaEvent = {
+                const startEvent: UIMessageStreamEvent = {
+                  type: 'text-start',
+                  id: textId,
+                }
+                const deltaEvent: UIMessageStreamEvent = {
                   type: 'text-delta',
                   id: textId,
                   delta: text,
-                } as any
-                const endEvent = { type: 'text-end', id: textId } as any
+                }
+                const endEvent: UIMessageStreamEvent = {
+                  type: 'text-end',
+                  id: textId,
+                }
                 await capture.messageLogger.logStreamEvent(startEvent)
                 await capture.messageLogger.logStreamEvent(deltaEvent)
                 await capture.messageLogger.logStreamEvent(endEvent)

packages/browseros-agent/apps/server/src/api/services/sdk/browser.ts

@@ -125,7 +125,7 @@ export class BrowserService {
           tabId: createdPage.tabId,
           windowId: createdPage.windowId ?? windowId,
         }
-      } catch {
+      } catch (error) {
         const fallbackPage = await this.getFallbackPage()
         if (fallbackPage) {
           await this.browser.goto(fallbackPage.pageId, url)
@@ -134,6 +134,13 @@ export class BrowserService {
             windowId: fallbackPage.windowId ?? 0,
           }
         }
+        throw error instanceof SdkError
+          ? error
+          : new SdkError(
+              error instanceof Error
+                ? error.message
+                : 'Failed to navigate in specified window',
+            )
       }
     }
 

packages/browseros-agent/apps/server/tests/api/routes/health.test.ts

@@ -9,12 +9,25 @@ import assert from 'node:assert'
 import { createHealthRoute } from '../../../src/api/routes/health'
 
 describe('createHealthRoute', () => {
-  it('returns status ok', async () => {
-    const route = createHealthRoute()
+  it('returns status ok with connected CDP state', async () => {
+    const route = createHealthRoute({
+      browser: { isCdpConnected: () => true } as never,
+    })
     const response = await route.request('/')
 
     assert.strictEqual(response.status, 200)
     const body = await response.json()
     assert.deepStrictEqual(body, { status: 'ok', cdpConnected: true })
   })
+
+  it('returns disconnected CDP state when browser is disconnected', async () => {
+    const route = createHealthRoute({
+      browser: { isCdpConnected: () => false } as never,
+    })
+    const response = await route.request('/')
+
+    assert.strictEqual(response.status, 200)
+    const body = await response.json()
+    assert.deepStrictEqual(body, { status: 'ok', cdpConnected: false })
+  })
 })

packages/browseros-agent/apps/server/tests/api/services/sdk/browser.test.ts

@@ -0,0 +1,21 @@
+import { describe, expect, it } from 'bun:test'
+import { BrowserService } from '../../../../src/api/services/sdk/browser'
+
+describe('BrowserService.navigate', () => {
+  it('throws when a window-targeted navigation cannot create or find any page', async () => {
+    const browser = {
+      listPages: async () => [],
+      getActivePage: async () => null,
+      newPage: async () => {
+        throw new Error('Window not found')
+      },
+      goto: async () => {},
+    }
+
+    const service = new BrowserService(browser as never)
+
+    await expect(
+      service.navigate('https://example.com', undefined, 12345),
+    ).rejects.toThrow('Window not found')
+  })
+})