fix: address network command review feedback

Co-authored-by: Cursor <cursoragent@cursor.com>

apps/extension/src/tools/__tests__/console.test.ts

@@ -72,7 +72,7 @@ describe("handleConsole", () => {
     expect(deps.consoleEntriesSince).toHaveBeenCalledWith(7, undefined, 50, 1000, false);
   });
 
-  it("reads an explicit tab and forwards bounded options", async () => {
+  it("reads an explicit tab and forwards cursor options", async () => {
     const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) });
     await sm.start("aa11");
     const deps = makeDeps({
@@ -85,27 +85,15 @@ describe("handleConsole", () => {
         session_id: "aa11",
         tab_id: 9,
         since: 12,
-        limit: 500,
-        max_text_chars: 9999,
+        limit: 75,
+        max_text_chars: 1500,
         include_stack: true,
       },
       deps,
     );
 
     expect(deps.ensureConsoleCapture).toHaveBeenCalledWith(9);
-    expect(deps.consoleEntriesSince).toHaveBeenCalledWith(9, 12, 200, 4096, true);
-  });
-
-  it("rejects invalid bounds before touching CDP", async () => {
-    const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) });
-    await sm.start("aa11");
-    const deps = makeDeps();
-
-    const res = await handleConsole(sm, { session_id: "aa11", limit: 0 }, deps);
-
-    expect(res).toMatchObject({ code: "invalid_params", message: /limit/ });
-    expect(deps.ensureConsoleCapture).not.toHaveBeenCalled();
-    expect(deps.consoleEntriesSince).not.toHaveBeenCalled();
+    expect(deps.consoleEntriesSince).toHaveBeenCalledWith(9, 12, 75, 1500, true);
   });
 
   it("hides other sessions' Agent Window tabs", async () => {

apps/extension/src/tools/__tests__/dispatcher.test.ts

@@ -106,6 +106,13 @@ describe("ToolDispatcher", () => {
       send: vi.fn(),
       detachSession: vi.fn(async () => {}),
       ensureConsoleCapture: vi.fn(async () => {}),
+      ensureNetworkCapture: vi.fn(async () => {}),
+      networkEntriesSince: vi.fn(() => ({
+        tab_id: 7,
+        entries: [],
+        next_since: 0,
+        truncated: false,
+      })),
       consoleEntriesSince: vi.fn(
         () =>
           ({
@@ -149,6 +156,13 @@ describe("ToolDispatcher", () => {
     const cdp = {
       send: vi.fn(),
       detachSession: vi.fn(async () => {}),
+      ensureNetworkCapture: vi.fn(async () => {}),
+      networkEntriesSince: vi.fn(() => ({
+        tab_id: 7,
+        entries: [],
+        next_since: 0,
+        truncated: false,
+      })),
     };
     const dispatcher = new ToolDispatcher({ transport, sessions, cdp });
     dispatcher.start();

apps/extension/src/tools/__tests__/network.test.ts

@@ -1,8 +1,7 @@
 import { describe, expect, it, vi } from "vitest";
 import { SessionManager } from "@/session-manager/manager";
-import type { CdpRunner } from "@/tools/shared";
 import type { NetworkResult } from "@/transport/types";
-import { handleNetwork } from "../network";
+import { handleNetwork, type NetworkCdpRunner } from "../network";
 
 function fakeAgentWindow(ids: number[]) {
   let i = 0;
@@ -22,6 +21,7 @@ function makeDeps(
     get?: (tabId: number) => Promise<chrome.tabs.Tab>;
     query?: (query: chrome.tabs.QueryInfo) => Promise<chrome.tabs.Tab[]>;
     result?: NetworkResult;
+    captureError?: Error;
   } = {},
 ) {
   const result =
@@ -32,13 +32,16 @@ function makeDeps(
       next_since: 0,
       truncated: false,
     } satisfies NetworkResult);
-  const ensureNetworkCapture = vi.fn(async () => {});
+  const ensureNetworkCapture = vi.fn(async () => {
+    if (opts.captureError) throw opts.captureError;
+  });
   const networkEntriesSince = vi.fn(() => result);
+  const trackSessionTab = vi.fn();
   const cdp = {
-    send: vi.fn(),
+    trackSessionTab,
     ensureNetworkCapture,
     networkEntriesSince,
-  } as unknown as CdpRunner;
+  } satisfies NetworkCdpRunner;
   const tabsApi = {
     get:
       opts.get ??
@@ -48,7 +51,7 @@ function makeDeps(
     query:
       opts.query ?? vi.fn(async () => [{ id: 7, windowId: 100, active: true } as chrome.tabs.Tab]),
   };
-  return { cdp, tabsApi, ensureNetworkCapture, networkEntriesSince };
+  return { cdp, tabsApi, ensureNetworkCapture, networkEntriesSince, trackSessionTab };
 }
 
 describe("handleNetwork", () => {
@@ -74,7 +77,7 @@ describe("handleNetwork", () => {
     expect(deps.networkEntriesSince).toHaveBeenCalledWith(7, undefined, 50, 1000);
   });
 
-  it("reads an explicit tab and forwards bounded options", async () => {
+  it("reads an explicit tab and forwards cursor options", async () => {
     const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) });
     await sm.start("aa11");
     const deps = makeDeps({
@@ -87,25 +90,25 @@ describe("handleNetwork", () => {
         session_id: "aa11",
         tab_id: 9,
         since: 12,
-        limit: 500,
-        max_text_chars: 9999,
+        limit: 75,
+        max_text_chars: 1500,
       },
       deps,
     );
 
     expect(deps.ensureNetworkCapture).toHaveBeenCalledWith(9);
-    expect(deps.networkEntriesSince).toHaveBeenCalledWith(9, 12, 200, 4096);
+    expect(deps.networkEntriesSince).toHaveBeenCalledWith(9, 12, 75, 1500);
   });
 
-  it("rejects invalid bounds before touching CDP", async () => {
+  it("surfaces network capture failures as cdp_failed", async () => {
     const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) });
     await sm.start("aa11");
-    const deps = makeDeps();
+    const deps = makeDeps({ captureError: new Error("Network.enable denied") });
 
-    const res = await handleNetwork(sm, { session_id: "aa11", limit: 0 }, deps);
+    const res = await handleNetwork(sm, { session_id: "aa11" }, deps);
 
-    expect(res).toMatchObject({ code: "invalid_params", message: /limit/ });
-    expect(deps.ensureNetworkCapture).not.toHaveBeenCalled();
+    expect(res).toEqual({ code: "cdp_failed", message: "Network.enable denied" });
+    expect(deps.trackSessionTab).toHaveBeenCalledWith("aa11", 7);
     expect(deps.networkEntriesSince).not.toHaveBeenCalled();
   });
 

apps/extension/src/tools/__tests__/shared.test.ts

@@ -1,6 +1,6 @@
 import { describe, expect, it, vi } from "vitest";
 import { SessionManager } from "@/session-manager/manager";
-import { lookupSession } from "../shared";
+import { lookupSession, parseBufferedReadBounds } from "../shared";
 
 function fakeAgentWindow() {
   return {
@@ -40,3 +40,30 @@ describe("lookupSession", () => {
     expect(result).toBe(ctx);
   });
 });
+
+describe("parseBufferedReadBounds", () => {
+  it("applies shared defaults and caps", () => {
+    expect(parseBufferedReadBounds({})).toEqual({
+      since: undefined,
+      limit: 50,
+      maxTextChars: 1000,
+    });
+    expect(parseBufferedReadBounds({ since: 12, limit: 500, max_text_chars: 9999 })).toEqual({
+      since: 12,
+      limit: 200,
+      maxTextChars: 4096,
+    });
+  });
+
+  it.each([
+    [{ since: -1 }, "since"],
+    [{ since: 1.5 }, "since"],
+    [{ limit: 0 }, "limit"],
+    [{ max_text_chars: 0 }, "max_text_chars"],
+  ])("rejects invalid bounds in %o", (params, field) => {
+    expect(parseBufferedReadBounds(params)).toMatchObject({
+      code: "invalid_params",
+      message: expect.stringContaining(field),
+    });
+  });
+});

apps/extension/src/tools/console.ts

@@ -7,14 +7,10 @@ import {
   chromeTabsApi,
   isRpcError,
   lookupSession,
+  parseBufferedReadBounds,
   resolveTargetTab,
 } from "./shared";
 
-const DEFAULT_CONSOLE_LIMIT = 50;
-const MAX_CONSOLE_LIMIT = 200;
-const DEFAULT_MAX_TEXT_CHARS = 1000;
-const MAX_TEXT_CHARS = 4096;
-
 export interface ConsoleDeps {
   cdp: CdpRunner;
   tabsApi: ChromeTabsApi;
@@ -34,7 +30,7 @@ export async function handleConsole(
 ): Promise<ConsoleResult | RpcError> {
   const ctxOrErr = lookupSession(manager, params, "console");
   if (isRpcError(ctxOrErr)) return ctxOrErr;
-  const bounds = parseConsoleBounds(params);
+  const bounds = parseBufferedReadBounds(params);
   if (isRpcError(bounds)) return bounds;
   const target = await resolveTargetTab(manager, ctxOrErr, params.tab_id, deps.tabsApi);
   if (isRpcError(target)) return target;
@@ -50,7 +46,7 @@ export async function handleConsole(
       bounds.since,
       bounds.limit,
       bounds.maxTextChars,
-      bounds.includeStack,
+      params.include_stack === true,
     );
   } catch (err) {
     return {
@@ -59,50 +55,3 @@ export async function handleConsole(
     };
   }
 }
-
-function parseConsoleBounds(params: ConsoleParams):
-  | {
-      since: number | undefined;
-      limit: number;
-      maxTextChars: number;
-      includeStack: boolean;
-    }
-  | RpcError {
-  const since = params.since;
-  if (since !== undefined && (!Number.isSafeInteger(since) || since < 0)) {
-    return { code: "invalid_params", message: "since must be a non-negative integer" };
-  }
-  const limit = boundedOptionalInteger(
-    params.limit,
-    DEFAULT_CONSOLE_LIMIT,
-    MAX_CONSOLE_LIMIT,
-    "limit",
-  );
-  if (isRpcError(limit)) return limit;
-  const maxTextChars = boundedOptionalInteger(
-    params.max_text_chars,
-    DEFAULT_MAX_TEXT_CHARS,
-    MAX_TEXT_CHARS,
-    "max_text_chars",
-  );
-  if (isRpcError(maxTextChars)) return maxTextChars;
-  return {
-    since,
-    limit,
-    maxTextChars,
-    includeStack: params.include_stack === true,
-  };
-}
-
-function boundedOptionalInteger(
-  value: number | undefined,
-  defaultValue: number,
-  maxValue: number,
-  field: string,
-): number | RpcError {
-  if (value === undefined) return defaultValue;
-  if (!Number.isSafeInteger(value) || value <= 0) {
-    return { code: "invalid_params", message: `${field} must be a positive integer` };
-  }
-  return Math.min(value, maxValue);
-}

apps/extension/src/tools/dispatcher.ts

@@ -34,7 +34,7 @@ import {
   handleNavigateForward,
   handleReload,
 } from "./navigation";
-import { handleNetwork } from "./network";
+import { handleNetwork, type NetworkCdpRunner } from "./network";
 import {
   type CdpRunner,
   chromeTabsCaptureApi,
@@ -66,12 +66,15 @@ import {
 } from "./tabs";
 import { handleWaitForNavigation } from "./waits";
 
+type DispatcherCdpRunner = CdpRunner &
+  NetworkCdpRunner & {
+    detachSession(sessionId: string): Promise<void>;
+  };
+
 export interface DispatcherDeps {
   transport: Transport;
   sessions: SessionManager;
-  cdp?: CdpRunner & {
-    detachSession(sessionId: string): Promise<void>;
-  };
+  cdp?: DispatcherCdpRunner;
   /**
    * Invoked whenever a dispatched RPC may have changed the live
    * session set (currently `tool.session_start` and
@@ -105,9 +108,7 @@ export interface DispatcherDeps {
 export class ToolDispatcher {
   private readonly transport: Transport;
   private readonly sessions: SessionManager;
-  private readonly cdp?: CdpRunner & {
-    detachSession(sessionId: string): Promise<void>;
-  };
+  private readonly cdp?: DispatcherCdpRunner;
   private readonly onSessionsChanged?: () => void;
   private readonly approveBorrow?: BorrowConfirmationApprover;
   private readonly helpNotificationCopy?: () => { title: string; body: string };

apps/extension/src/tools/network.ts

@@ -2,21 +2,27 @@ import { ChromiumCdp } from "@/browser-driver/chromium-cdp";
 import type { SessionManager } from "@/session-manager/manager";
 import type { NetworkParams, NetworkResult, RpcError } from "@/transport/types";
 import {
-  type CdpRunner,
   type ChromeTabsApi,
   chromeTabsApi,
   isRpcError,
   lookupSession,
+  parseBufferedReadBounds,
   resolveTargetTab,
 } from "./shared";
 
-const DEFAULT_NETWORK_LIMIT = 50;
-const MAX_NETWORK_LIMIT = 200;
-const DEFAULT_MAX_TEXT_CHARS = 1000;
-const MAX_TEXT_CHARS = 4096;
+export interface NetworkCdpRunner {
+  trackSessionTab?(sessionId: string, tabId: number): void;
+  ensureNetworkCapture(tabId: number): Promise<void>;
+  networkEntriesSince(
+    tabId: number,
+    since: number | undefined,
+    limit: number,
+    maxTextChars: number,
+  ): NetworkResult;
+}
 
 export interface NetworkDeps {
-  cdp: CdpRunner;
+  cdp: NetworkCdpRunner;
   tabsApi: ChromeTabsApi;
 }
 
@@ -34,17 +40,14 @@ export async function handleNetwork(
 ): Promise<NetworkResult | RpcError> {
   const ctxOrErr = lookupSession(manager, params, "network");
   if (isRpcError(ctxOrErr)) return ctxOrErr;
-  const bounds = parseNetworkBounds(params);
+  const bounds = parseBufferedReadBounds(params);
   if (isRpcError(bounds)) return bounds;
   const target = await resolveTargetTab(manager, ctxOrErr, params.tab_id, deps.tabsApi);
   if (isRpcError(target)) return target;
-  if (!deps.cdp.ensureNetworkCapture || !deps.cdp.networkEntriesSince) {
-    return { code: "cdp_failed", message: "network capture requires CDP network support" };
-  }
 
   try {
-    await deps.cdp.ensureNetworkCapture(target.tabId);
     deps.cdp.trackSessionTab?.(ctxOrErr.sessionId, target.tabId);
+    await deps.cdp.ensureNetworkCapture(target.tabId);
     return deps.cdp.networkEntriesSince(
       target.tabId,
       bounds.since,
@@ -58,48 +61,3 @@ export async function handleNetwork(
     };
   }
 }
-
-function parseNetworkBounds(params: NetworkParams):
-  | {
-      since: number | undefined;
-      limit: number;
-      maxTextChars: number;
-    }
-  | RpcError {
-  const since = params.since;
-  if (since !== undefined && (!Number.isSafeInteger(since) || since < 0)) {
-    return { code: "invalid_params", message: "since must be a non-negative integer" };
-  }
-  const limit = boundedOptionalInteger(
-    params.limit,
-    DEFAULT_NETWORK_LIMIT,
-    MAX_NETWORK_LIMIT,
-    "limit",
-  );
-  if (isRpcError(limit)) return limit;
-  const maxTextChars = boundedOptionalInteger(
-    params.max_text_chars,
-    DEFAULT_MAX_TEXT_CHARS,
-    MAX_TEXT_CHARS,
-    "max_text_chars",
-  );
-  if (isRpcError(maxTextChars)) return maxTextChars;
-  return {
-    since,
-    limit,
-    maxTextChars,
-  };
-}
-
-function boundedOptionalInteger(
-  value: number | undefined,
-  defaultValue: number,
-  maxValue: number,
-  field: string,
-): number | RpcError {
-  if (value === undefined) return defaultValue;
-  if (!Number.isSafeInteger(value) || value <= 0) {
-    return { code: "invalid_params", message: `${field} must be a positive integer` };
-  }
-  return Math.min(value, maxValue);
-}

apps/extension/src/tools/shared.ts

@@ -9,14 +9,14 @@
 import type { DialogCursor } from "@/browser-driver/chromium-cdp";
 import type { SessionContext, SessionManager } from "@/session-manager/manager";
 import { normaliseRef } from "@/session-manager/ref-store";
-import type {
-  ConsoleResult,
-  JavaScriptDialogInfo,
-  NetworkResult,
-  RpcError,
-} from "@/transport/types";
+import type { ConsoleResult, JavaScriptDialogInfo, RpcError } from "@/transport/types";
 import { rpcError } from "./errors";
 
+const DEFAULT_BUFFERED_READ_LIMIT = 50;
+const MAX_BUFFERED_READ_LIMIT = 200;
+const DEFAULT_MAX_TEXT_CHARS = 1000;
+const MAX_TEXT_CHARS = 4096;
+
 /**
  * Subset of `chrome.tabs` we depend on across tool handlers. Kept on
  * a thin interface so vitest can inject a fake without monkey-patching
@@ -58,13 +58,52 @@ export interface CdpRunner {
     maxTextChars: number,
     includeStack: boolean,
   ): ConsoleResult;
-  ensureNetworkCapture?(tabId: number): Promise<void>;
-  networkEntriesSince?(
-    tabId: number,
-    since: number | undefined,
-    limit: number,
-    maxTextChars: number,
-  ): NetworkResult;
+}
+
+export interface BufferedReadBounds {
+  since: number | undefined;
+  limit: number;
+  maxTextChars: number;
+}
+
+/** Parse the common cursor and output bounds used by buffered read tools. */
+export function parseBufferedReadBounds(params: {
+  since?: number;
+  limit?: number;
+  max_text_chars?: number;
+}): BufferedReadBounds | RpcError {
+  const since = params.since;
+  if (since !== undefined && (!Number.isSafeInteger(since) || since < 0)) {
+    return { code: "invalid_params", message: "since must be a non-negative integer" };
+  }
+  const limit = boundedOptionalInteger(
+    params.limit,
+    DEFAULT_BUFFERED_READ_LIMIT,
+    MAX_BUFFERED_READ_LIMIT,
+    "limit",
+  );
+  if (isRpcError(limit)) return limit;
+  const maxTextChars = boundedOptionalInteger(
+    params.max_text_chars,
+    DEFAULT_MAX_TEXT_CHARS,
+    MAX_TEXT_CHARS,
+    "max_text_chars",
+  );
+  if (isRpcError(maxTextChars)) return maxTextChars;
+  return { since, limit, maxTextChars };
+}
+
+function boundedOptionalInteger(
+  value: number | undefined,
+  defaultValue: number,
+  maxValue: number,
+  field: string,
+): number | RpcError {
+  if (value === undefined) return defaultValue;
+  if (!Number.isSafeInteger(value) || value <= 0) {
+    return { code: "invalid_params", message: `${field} must be a positive integer` };
+  }
+  return Math.min(value, maxValue);
 }
 
 /**