refactor: unify browser error classification and deduplicate retry logic

src/browser.test.ts

@@ -3,7 +3,7 @@ import { BrowserBridge, generateStealthJs } from './browser/index.js';
 import { extractTabEntries, diffTabIndexes, appendLimited } from './browser/tabs.js';
 import { withTimeoutMs } from './runtime.js';
 import { __test__ as cdpTest } from './browser/cdp.js';
-import { isRetryableSettleError } from './browser/page.js';
+import { classifyBrowserError } from './browser/errors.js';
 import * as daemonClient from './browser/daemon-client.js';
 
 describe('browser helpers', () => {
@@ -49,10 +49,20 @@ describe('browser helpers', () => {
     await expect(withTimeoutMs(new Promise(() => {}), 10, 'timeout')).rejects.toThrow('timeout');
   });
 
-  it('retries settle only for target-invalidated errors', () => {
-    expect(isRetryableSettleError(new Error('{"code":-32000,"message":"Inspected target navigated or closed"}'))).toBe(true);
-    expect(isRetryableSettleError(new Error('attach failed: target no longer exists'))).toBe(false);
-    expect(isRetryableSettleError(new Error('malformed exec payload'))).toBe(false);
+  it('classifies browser errors with correct kind and retry advice', () => {
+    // CDP target navigation — page-level settle retry
+    const nav = classifyBrowserError(new Error('{"code":-32000,"message":"Inspected target navigated or closed"}'));
+    expect(nav.kind).toBe('target-navigation');
+    expect(nav.delayMs).toBe(200);
+
+    // Extension transient — daemon-client retry only, NOT page-level
+    const ext = classifyBrowserError(new Error('Extension disconnected'));
+    expect(ext.kind).toBe('extension-transient');
+    expect(ext.delayMs).toBe(1500);
+
+    // Non-transient errors — not retryable
+    expect(classifyBrowserError(new Error('malformed exec payload')).kind).toBe('non-retryable');
+    expect(classifyBrowserError(new Error('Permission denied')).kind).toBe('non-retryable');
   });
 
   it('prefers the real Electron app target over DevTools and blank pages', () => {

src/browser/daemon-client.ts

@@ -7,7 +7,7 @@
 import { DEFAULT_DAEMON_PORT } from '../constants.js';
 import type { BrowserSessionInfo } from '../types.js';
 import { sleep } from '../utils.js';
-import { isTransientBrowserError } from './errors.js';
+import { classifyBrowserError } from './errors.js';
 
 const DAEMON_PORT = parseInt(process.env.OPENCLI_DAEMON_PORT ?? String(DEFAULT_DAEMON_PORT), 10);
 const DAEMON_URL = `http://127.0.0.1:${DAEMON_PORT}`;
@@ -122,18 +122,21 @@ export async function requestDaemonShutdown(opts?: { timeout?: number }): Promis
 }
 
 /**
- * Send a command to the daemon and wait for a result.
- * Retries up to 4 times: network errors retry at 500ms,
- * transient extension errors retry at 1500ms.
+ * Internal: send a command to the daemon with retry logic.
+ * Returns the raw DaemonResult. All retry policy lives here — callers
+ * (sendCommand, sendCommandFull) only shape the return value.
+ *
+ * Retries up to 4 times:
+ * - Network errors (TypeError, AbortError): retry at 500ms
+ * - Transient browser errors: retry at the delay suggested by classifyBrowserError()
  */
-export async function sendCommand(
+async function sendCommandRaw(
   action: DaemonCommand['action'],
-  params: Omit<DaemonCommand, 'id' | 'action'> = {},
-): Promise<unknown> {
+  params: Omit<DaemonCommand, 'id' | 'action'>,
+): Promise<DaemonResult> {
   const maxRetries = 4;
 
   for (let attempt = 1; attempt <= maxRetries; attempt++) {
-    // Generate a fresh ID per attempt to avoid daemon-side duplicate detection
     const id = generateId();
     const command: DaemonCommand = { id, action, ...params };
     try {
@@ -147,30 +150,39 @@ export async function sendCommand(
       const result = (await res.json()) as DaemonResult;
 
       if (!result.ok) {
-        // Check if error is a transient extension issue worth retrying
-        if (isTransientBrowserError(new Error(result.error ?? '')) && attempt < maxRetries) {
-          // Longer delay for extension recovery (service worker restart)
-          await sleep(1500);
+        const advice = classifyBrowserError(new Error(result.error ?? ''));
+        if (advice.retryable && attempt < maxRetries) {
+          await sleep(advice.delayMs);
           continue;
         }
         throw new Error(result.error ?? 'Daemon command failed');
       }
 
-      return result.data;
+      return result;
     } catch (err) {
-      const isRetryable = err instanceof TypeError  // fetch network error
+      const isNetworkError = err instanceof TypeError
         || (err instanceof Error && err.name === 'AbortError');
-      if (isRetryable && attempt < maxRetries) {
+      if (isNetworkError && attempt < maxRetries) {
         await sleep(500);
         continue;
       }
       throw err;
     }
   }
-  // Unreachable — the loop always returns or throws
   throw new Error('sendCommand: max retries exhausted');
 }
 
+/**
+ * Send a command to the daemon and return the result data.
+ */
+export async function sendCommand(
+  action: DaemonCommand['action'],
+  params: Omit<DaemonCommand, 'id' | 'action'> = {},
+): Promise<unknown> {
+  const result = await sendCommandRaw(action, params);
+  return result.data;
+}
+
 /**
  * Like sendCommand, but returns both data and page identity (targetId).
  * Use this for page-scoped commands where the caller needs the page identity.
@@ -179,41 +191,8 @@ export async function sendCommandFull(
   action: DaemonCommand['action'],
   params: Omit<DaemonCommand, 'id' | 'action'> = {},
 ): Promise<{ data: unknown; page?: string }> {
-  const maxRetries = 4;
-
-  for (let attempt = 1; attempt <= maxRetries; attempt++) {
-    const id = generateId();
-    const command: DaemonCommand = { id, action, ...params };
-    try {
-      const res = await requestDaemon('/command', {
-        method: 'POST',
-        headers: { 'Content-Type': 'application/json' },
-        body: JSON.stringify(command),
-        timeout: 30000,
-      });
-
-      const result = (await res.json()) as DaemonResult;
-
-      if (!result.ok) {
-        if (isTransientBrowserError(new Error(result.error ?? '')) && attempt < maxRetries) {
-          await sleep(1500);
-          continue;
-        }
-        throw new Error(result.error ?? 'Daemon command failed');
-      }
-
-      return { data: result.data, page: result.page };
-    } catch (err) {
-      const isRetryable = err instanceof TypeError
-        || (err instanceof Error && err.name === 'AbortError');
-      if (isRetryable && attempt < maxRetries) {
-        await sleep(500);
-        continue;
-      }
-      throw err;
-    }
-  }
-  throw new Error('sendCommandFull: max retries exhausted');
+  const result = await sendCommandRaw(action, params);
+  return { data: result.data, page: result.page };
 }
 
 export async function listSessions(): Promise<BrowserSessionInfo[]> {

src/browser/errors.test.ts

@@ -1,13 +1,59 @@
 import { describe, expect, it } from 'vitest';
 
-import { isTransientBrowserError } from './errors.js';
+import { classifyBrowserError, isTransientBrowserError } from './errors.js';
 
-describe('isTransientBrowserError', () => {
-  it('treats "No window with id" as transient', () => {
+describe('classifyBrowserError', () => {
+  it('classifies extension transient errors with 1500ms delay', () => {
+    for (const msg of [
+      'Extension disconnected',
+      'Extension not connected',
+      'attach failed',
+      'no longer exists',
+      'CDP connection reset',
+      'Daemon command failed',
+      'No window with id: 123',
+    ]) {
+      const advice = classifyBrowserError(new Error(msg));
+      expect(advice.kind, `expected "${msg}" → extension-transient`).toBe('extension-transient');
+      expect(advice.retryable).toBe(true);
+      expect(advice.delayMs).toBe(1500);
+    }
+  });
+
+  it('classifies CDP target navigation errors with 200ms delay', () => {
+    const advice = classifyBrowserError(new Error('Inspected target navigated or closed'));
+    expect(advice.kind).toBe('target-navigation');
+    expect(advice.retryable).toBe(true);
+    expect(advice.delayMs).toBe(200);
+  });
+
+  it('classifies CDP -32000 target errors with 200ms delay', () => {
+    const advice = classifyBrowserError(new Error('{"code":-32000,"message":"Target closed"}'));
+    expect(advice.kind).toBe('target-navigation');
+    expect(advice.retryable).toBe(true);
+    expect(advice.delayMs).toBe(200);
+  });
+
+  it('returns non-retryable for unrelated errors', () => {
+    for (const msg of ['Permission denied', 'malformed exec payload', 'SyntaxError']) {
+      const advice = classifyBrowserError(new Error(msg));
+      expect(advice.kind).toBe('non-retryable');
+      expect(advice.retryable).toBe(false);
+    }
+  });
+
+  it('handles non-Error values', () => {
+    expect(classifyBrowserError('Extension disconnected').kind).toBe('extension-transient');
+    expect(classifyBrowserError(42).kind).toBe('non-retryable');
+  });
+});
+
+describe('isTransientBrowserError (convenience wrapper)', () => {
+  it('returns true for transient errors', () => {
     expect(isTransientBrowserError(new Error('No window with id: 123'))).toBe(true);
   });
 
-  it('does not classify unrelated browser errors as transient', () => {
+  it('returns false for non-transient errors', () => {
     expect(isTransientBrowserError(new Error('Permission denied'))).toBe(false);
   });
 });

src/browser/errors.ts

@@ -9,11 +9,35 @@ import { BrowserConnectError, type BrowserConnectKind } from '../errors.js';
 import { DEFAULT_DAEMON_PORT } from '../constants.js';
 
 /**
- * Transient browser error patterns — shared across daemon-client, pipeline executor,
- * and page retry logic. These errors indicate temporary conditions (extension restart,
- * service worker cycle, tab navigation) that are worth retrying.
+ * Unified browser error classification.
+ *
+ * All transient error detection lives here — daemon-client, pipeline executor,
+ * and page retry logic all use this single system instead of maintaining
+ * separate pattern lists.
+ */
+
+/** Error category — determines which layer should retry. */
+export type BrowserErrorKind =
+  | 'extension-transient'   // daemon/extension hiccup — daemon-client retries
+  | 'target-navigation'     // CDP target invalidated by SPA nav — page-level settle retry
+  | 'non-retryable';        // permanent error — no retry
+
+/** How the caller should handle the error. */
+export interface RetryAdvice {
+  /** Error category — callers use this to decide whether *they* should retry. */
+  kind: BrowserErrorKind;
+  /** Whether the error is transient and worth retrying. */
+  retryable: boolean;
+  /** Suggested delay before retry (ms). */
+  delayMs: number;
+}
+
+/**
+ * Extension/daemon transient patterns — service worker restarts, attach races,
+ * tab closure, daemon hiccups. These warrant a longer retry delay (~1500ms)
+ * because the extension needs time to recover.
  */
-const TRANSIENT_ERROR_PATTERNS = [
+const EXTENSION_TRANSIENT_PATTERNS = [
   'Extension disconnected',
   'Extension not connected',
   'attach failed',
@@ -24,11 +48,50 @@ const TRANSIENT_ERROR_PATTERNS = [
 ] as const;
 
 /**
- * Check if an error message indicates a transient browser error worth retrying.
+ * CDP target navigation patterns — SPA client-side redirects can invalidate the
+ * CDP target after chrome.tabs reports 'complete'. These warrant a shorter retry
+ * delay (~200ms) because the new document is usually available quickly.
+ */
+const TARGET_NAVIGATION_PATTERNS = [
+  'Inspected target navigated or closed',
+] as const;
+
+function errorMessage(err: unknown): string {
+  return err instanceof Error ? err.message : String(err);
+}
+
+/**
+ * Classify a browser error and return retry advice.
+ *
+ * Single source of truth for "is this error transient?" across all layers.
+ */
+export function classifyBrowserError(err: unknown): RetryAdvice {
+  const msg = errorMessage(err);
+
+  // Extension/daemon transient errors — longer recovery time
+  if (EXTENSION_TRANSIENT_PATTERNS.some(p => msg.includes(p))) {
+    return { kind: 'extension-transient', retryable: true, delayMs: 1500 };
+  }
+
+  // CDP target navigation errors — shorter recovery time
+  if (TARGET_NAVIGATION_PATTERNS.some(p => msg.includes(p))) {
+    return { kind: 'target-navigation', retryable: true, delayMs: 200 };
+  }
+
+  // CDP protocol error with target context (e.g., -32000 "target closed")
+  if (msg.includes('-32000') && msg.toLowerCase().includes('target')) {
+    return { kind: 'target-navigation', retryable: true, delayMs: 200 };
+  }
+
+  return { kind: 'non-retryable', retryable: false, delayMs: 0 };
+}
+
+/**
+ * Check if an error is a transient browser error worth retrying.
+ * Convenience wrapper around classifyBrowserError().
  */
 export function isTransientBrowserError(err: unknown): boolean {
-  const msg = err instanceof Error ? err.message : String(err);
-  return TRANSIENT_ERROR_PATTERNS.some(pattern => msg.includes(pattern));
+  return classifyBrowserError(err).retryable;
 }
 
 // Re-export so callers don't need to import from two places

src/browser/page.ts

@@ -16,12 +16,7 @@ import { saveBase64ToFile } from '../utils.js';
 import { generateStealthJs } from './stealth.js';
 import { waitForDomStableJs } from './dom-helpers.js';
 import { BasePage } from './base-page.js';
-
-export function isRetryableSettleError(err: unknown): boolean {
-  const message = err instanceof Error ? err.message : String(err);
-  return message.includes('Inspected target navigated or closed')
-    || (message.includes('-32000') && message.toLowerCase().includes('target'));
-}
+import { classifyBrowserError } from './errors.js';
 
 /**
  * Page — implements IPage by talking to the daemon via HTTP.
@@ -69,15 +64,16 @@ export class Page extends BasePage {
       try {
         await sendCommand('exec', combinedOpts);
       } catch (err) {
-        if (!isRetryableSettleError(err)) throw err;
-        // SPA client-side redirects can invalidate the CDP target after
-        // chrome.tabs reports 'complete'. Wait briefly for the new document
-        // to load, then retry the settle probe once.
+        const advice = classifyBrowserError(err);
+        // Only settle-retry on target navigation (SPA client-side redirects).
+        // Extension/daemon errors are already retried by sendCommandRaw —
+        // retrying them here would silently swallow real failures.
+        if (advice.kind !== 'target-navigation') throw err;
         try {
-          await new Promise((r) => setTimeout(r, 200));
+          await new Promise((r) => setTimeout(r, advice.delayMs));
           await sendCommand('exec', combinedOpts);
         } catch (retryErr) {
-          if (!isRetryableSettleError(retryErr)) throw retryErr;
+          if (classifyBrowserError(retryErr).kind !== 'target-navigation') throw retryErr;
         }
       }
     } else {
@@ -108,8 +104,9 @@ export class Page extends BasePage {
     try {
       return await sendCommand('exec', { code, ...this._cmdOpts() });
     } catch (err) {
-      if (!isRetryableSettleError(err)) throw err;
-      await new Promise((resolve) => setTimeout(resolve, 200));
+      const advice = classifyBrowserError(err);
+      if (advice.kind !== 'target-navigation') throw err;
+      await new Promise((resolve) => setTimeout(resolve, advice.delayMs));
       return sendCommand('exec', { code, ...this._cmdOpts() });
     }
   }