refactor: fix 14 issues from deep code review

P0 Critical:
- #1 Fix double IIFE wrapping: unified wrapForEval() replaces
  normalizeEvaluateSource + ad-hoc wrap in page.evaluate()
- #2 Fix navigate race: check tab.status before addListener,
  reduced timeout 30s→15s

P1 Should Fix:
- #8 Remove unused permissions (scripting, host_permissions, content_scripts)
- #10 Add retry (3x, 500ms) + timeout (30s) to sendCommand()

P2 Cleanup:
- #3 Extract isWebUrl() to safely handle undefined tab.url
- #4 Sanitize maxDepth with Math.max/min bounds
- #6 Delete empty src/daemon/ directory
- #7 Remove dead createJsonRpcRequest + its test
- #9 Remove stale IIFE-mode comment
- #11 Validate body.id in daemon request handler
- #12 Guard ensureAttached: detach+re-attach on 'already attached'
- #14 Extract _tabOpt() helper (removes 13x spread duplication)
- #15 Add verbose warning for unsupported consoleMessages()

All 35 unit tests pass.

extension/content/noop.js

@@ -1,2 +0,0 @@
-// Minimal content script — required by manifest to auto-grant host permissions.
-// No actual logic needed; chrome.scripting.executeScript handles all injection.

extension/manifest.json

@@ -5,22 +5,11 @@
   "description": "Bridge between opencli CLI and your browser — execute commands, read cookies, manage tabs.",
   "permissions": [
     "debugger",
-    "scripting",
     "tabs",
     "cookies",
     "activeTab",
     "alarms"
   ],
-  "host_permissions": [
-    "<all_urls>"
-  ],
-  "content_scripts": [
-    {
-      "matches": ["http://*/*", "https://*/*"],
-      "js": ["content/noop.js"],
-      "run_at": "document_idle"
-    }
-  ],
   "background": {
     "service_worker": "dist/background.js",
     "type": "module"

extension/src/background.ts

@@ -2,11 +2,7 @@
  * opencli Browser Bridge — Service Worker (background script).
  *
  * Connects to the opencli daemon via WebSocket, receives commands,
- * dispatches them to Chrome APIs (scripting/tabs/cookies), returns results.
- *
- * IMPORTANT: In IIFE mode (non-module), all chrome.* API registrations
- * must happen inside lifecycle events (onInstalled/onStartup), NOT at
- * the top level, or the service worker registration will fail.
+ * dispatches them to Chrome APIs (debugger/tabs/cookies), returns results.
  */
 
 import type { Command, Result } from './protocol';
@@ -66,7 +62,6 @@ function scheduleReconnect(): void {
 }
 
 // ─── Lifecycle events ────────────────────────────────────────────────
-// All chrome.* API registrations must be in these listeners, not top-level.
 
 function initialize(): void {
   chrome.alarms.create('keepalive', { periodInMinutes: 0.4 }); // ~24 seconds
@@ -114,20 +109,25 @@ async function handleCommand(cmd: Command): Promise<Result> {
 
 // ─── Action handlers ─────────────────────────────────────────────────
 
+/** Check if a URL is a debuggable web page (not chrome:// or extension page) */
+function isWebUrl(url?: string): boolean {
+  if (!url) return false;
+  return !url.startsWith('chrome://') && !url.startsWith('chrome-extension://');
+}
+
 /** Resolve target tab: use specified tabId or fall back to active web page tab */
 async function resolveTabId(tabId?: number): Promise<number> {
   if (tabId !== undefined) return tabId;
 
   // Try the active tab first
   const [activeTab] = await chrome.tabs.query({ active: true, currentWindow: true });
-  if (activeTab?.id && activeTab.url && !activeTab.url.startsWith('chrome://') && !activeTab.url.startsWith('chrome-extension://')) {
+  if (activeTab?.id && isWebUrl(activeTab.url)) {
     return activeTab.id;
   }
 
-  // Active tab is not debuggable (chrome:// or chrome-extension://),
-  // try to find any open web page tab
+  // Active tab is not debuggable — try to find any open web page tab
   const allTabs = await chrome.tabs.query({ currentWindow: true });
-  const webTab = allTabs.find(t => t.id && t.url && !t.url.startsWith('chrome://') && !t.url.startsWith('chrome-extension://'));
+  const webTab = allTabs.find(t => t.id && isWebUrl(t.url));
   if (webTab?.id) {
     await chrome.tabs.update(webTab.id, { active: true });
     return webTab.id;
@@ -155,20 +155,25 @@ async function handleNavigate(cmd: Command): Promise<Result> {
   const tabId = await resolveTabId(cmd.tabId);
   await chrome.tabs.update(tabId, { url: cmd.url });
 
-  // Wait for page to finish loading
+  // Wait for page to finish loading, checking current status first to avoid race
   await new Promise<void>((resolve) => {
-    const listener = (id: number, info: chrome.tabs.TabChangeInfo) => {
-      if (id === tabId && info.status === 'complete') {
+    // Check if already complete (e.g. cached pages)
+    chrome.tabs.get(tabId).then(tab => {
+      if (tab.status === 'complete') { resolve(); return; }
+
+      const listener = (id: number, info: chrome.tabs.TabChangeInfo) => {
+        if (id === tabId && info.status === 'complete') {
+          chrome.tabs.onUpdated.removeListener(listener);
+          resolve();
+        }
+      };
+      chrome.tabs.onUpdated.addListener(listener);
+      // Timeout fallback
+      setTimeout(() => {
         chrome.tabs.onUpdated.removeListener(listener);
         resolve();
-      }
-    };
-    chrome.tabs.onUpdated.addListener(listener);
-    // Timeout fallback
-    setTimeout(() => {
-      chrome.tabs.onUpdated.removeListener(listener);
-      resolve();
-    }, 30000);
+      }, 15000);
+    });
   });
 
   const tab = await chrome.tabs.get(tabId);
@@ -180,7 +185,7 @@ async function handleTabs(cmd: Command): Promise<Result> {
     case 'list': {
       const tabs = await chrome.tabs.query({});
       const data = tabs
-        .filter((t) => t.url && !t.url.startsWith('chrome://') && !t.url.startsWith('chrome-extension://'))
+        .filter((t) => isWebUrl(t.url))
         .map((t, i) => ({
           index: i,
           tabId: t.id,
@@ -196,7 +201,6 @@ async function handleTabs(cmd: Command): Promise<Result> {
     }
     case 'close': {
       if (cmd.index !== undefined) {
-        // Close by index
         const tabs = await chrome.tabs.query({});
         const target = tabs[cmd.index];
         if (!target?.id) return { id: cmd.id, ok: false, error: `Tab index ${cmd.index} not found` };
@@ -204,7 +208,6 @@ async function handleTabs(cmd: Command): Promise<Result> {
         cdp.detach(target.id);
         return { id: cmd.id, ok: true, data: { closed: target.id } };
       }
-      // Close by tabId or active tab
       const tabId = await resolveTabId(cmd.tabId);
       await chrome.tabs.remove(tabId);
       cdp.detach(tabId);

extension/src/cdp.ts

@@ -15,7 +15,17 @@ async function ensureAttached(tabId: number): Promise<void> {
     await chrome.debugger.attach({ tabId }, '1.3');
   } catch (e: unknown) {
     const msg = e instanceof Error ? e.message : String(e);
-    if (!msg.includes('Another debugger is already attached')) {
+    // "Already attached" from OUR previous session (service worker restart) — OK, track it
+    if (msg.includes('Another debugger is already attached')) {
+      // Don't add to attached set — we can't sendCommand on someone else's session
+      // Try to detach and re-attach to claim ownership
+      try { await chrome.debugger.detach({ tabId }); } catch { /* ignore */ }
+      try {
+        await chrome.debugger.attach({ tabId }, '1.3');
+      } catch {
+        throw new Error(`attach failed: ${msg}`);
+      }
+    } else {
       throw new Error(`attach failed: ${msg}`);
     }
   }

src/browser.test.ts

@@ -2,15 +2,6 @@ import { afterEach, describe, it, expect, vi } from 'vitest';
 import { PlaywrightMCP, __test__ } from './browser/index.js';
 
 describe('browser helpers', () => {
-  it('creates JSON-RPC requests with unique ids', () => {
-    const first = __test__.createJsonRpcRequest('tools/call', { name: 'browser_tabs' });
-    const second = __test__.createJsonRpcRequest('tools/call', { name: 'browser_snapshot' });
-
-    expect(second.id).toBe(first.id + 1);
-    expect(first.message).toContain(`"id":${first.id}`);
-    expect(second.message).toContain(`"id":${second.id}`);
-  });
-
   it('extracts tab entries from string snapshots', () => {
     const entries = __test__.extractTabEntries('Tab 0 https://example.com\nTab 1 Chrome Extension');
 

src/browser/daemon-client.ts

@@ -65,6 +65,7 @@ export async function isExtensionConnected(): Promise<boolean> {
 
 /**
  * Send a command to the daemon and wait for a result.
+ * Retries up to 3 times with 500ms delay for transient failures.
  */
 export async function sendCommand(
   action: DaemonCommand['action'],
@@ -72,18 +73,36 @@ export async function sendCommand(
 ): Promise<unknown> {
   const id = generateId();
   const command: DaemonCommand = { id, action, ...params };
+  const maxRetries = 3;
 
-  const res = await fetch(`${DAEMON_URL}/command`, {
-    method: 'POST',
-    headers: { 'Content-Type': 'application/json' },
-    body: JSON.stringify(command),
-  });
+  for (let attempt = 1; attempt <= maxRetries; attempt++) {
+    try {
+      const controller = new AbortController();
+      const timer = setTimeout(() => controller.abort(), 30000);
 
-  const result = (await res.json()) as DaemonResult;
+      const res = await fetch(`${DAEMON_URL}/command`, {
+        method: 'POST',
+        headers: { 'Content-Type': 'application/json' },
+        body: JSON.stringify(command),
+        signal: controller.signal,
+      });
+      clearTimeout(timer);
 
-  if (!result.ok) {
-    throw new Error(result.error ?? 'Daemon command failed');
+      const result = (await res.json()) as DaemonResult;
+
+      if (!result.ok) {
+        throw new Error(result.error ?? 'Daemon command failed');
+      }
+
+      return result.data;
+    } catch (err) {
+      const isRetryable = err instanceof TypeError  // fetch network error
+        || (err instanceof Error && err.name === 'AbortError');
+      if (isRetryable && attempt < maxRetries) {
+        await new Promise(r => setTimeout(r, 500));
+        continue;
+      }
+      throw err;
+    }
   }
-
-  return result.data;
 }

src/browser/index.ts

@@ -14,13 +14,10 @@ export function getTokenFingerprint(_token: string | undefined): string | null {
   return null;
 }
 
-// Test-only helpers
-import { createJsonRpcRequest } from './mcp.js';
 import { extractTabEntries, diffTabIndexes, appendLimited } from './tabs.js';
 import { withTimeoutMs } from '../runtime.js';
 
 export const __test__ = {
-  createJsonRpcRequest,
   extractTabEntries,
   diffTabIndexes,
   appendLimited,

src/browser/mcp.ts

@@ -17,12 +17,7 @@ const DAEMON_SPAWN_TIMEOUT = 10000; // 10s to wait for daemon + extension
 
 export type PlaywrightMCPState = 'idle' | 'connecting' | 'connected' | 'closing' | 'closed';
 
-// Re-export for __test__ compatibility
-let _jsonRpcId = 0;
-export function createJsonRpcRequest(method: string, params: Record<string, unknown> = {}): { id: number; message: string } {
-  const id = ++_jsonRpcId;
-  return { id, message: JSON.stringify({ id, method, params }) };
-}
+
 
 /**
  * Browser factory: manages daemon lifecycle and provides IPage instances.

src/browser/page.ts

@@ -11,8 +11,6 @@
  */
 
 import { formatSnapshot } from '../snapshotFormatter.js';
-import { normalizeEvaluateSource } from '../pipeline/template.js';
-import { generateInterceptorJs, generateReadInterceptedJs } from '../interceptor.js';
 import type { IPage } from '../types.js';
 import { sendCommand } from './daemon-client.js';
 
@@ -23,10 +21,15 @@ export class Page implements IPage {
   /** Active tab ID, set after navigate and used in all subsequent commands */
   private _tabId: number | undefined;
 
+  /** Helper: spread tabId into command params if we have one */
+  private _tabOpt(): { tabId: number } | Record<string, never> {
+    return this._tabId !== undefined ? { tabId: this._tabId } : {};
+  }
+
   async goto(url: string): Promise<void> {
     const result = await sendCommand('navigate', {
       url,
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
+      ...this._tabOpt(),
     }) as { tabId?: number };
     // Remember the tabId for subsequent exec calls
     if (result?.tabId) {
@@ -35,27 +38,16 @@ export class Page implements IPage {
   }
 
   async evaluate(js: string): Promise<any> {
-    const normalized = normalizeEvaluateSource(js);
-    // Wrap function-style code: `() => { ... }` or `async () => { ... }` → IIFE
-    const trimmed = normalized.trim();
-    const code = trimmed.startsWith('async')
-      ? `(${trimmed})()`
-      : trimmed.startsWith('function') || trimmed.startsWith('(')
-        ? `(${trimmed})()`
-        : trimmed;
-    return sendCommand('exec', {
-      code,
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
-    });
+    const code = wrapForEval(js);
+    return sendCommand('exec', { code, ...this._tabOpt() });
   }
 
   async snapshot(opts: { interactive?: boolean; compact?: boolean; maxDepth?: number; raw?: boolean } = {}): Promise<any> {
-    // Use CDP Accessibility.getFullAXTree via exec
+    const maxDepth = Math.max(1, Math.min(Number(opts.maxDepth) || 50, 200));
     const code = `
       (async () => {
-        // Build a simplified accessibility tree from the DOM
-        function buildTree(node, depth = 0) {
-          if (depth > ${opts.maxDepth ?? 50}) return '';
+        function buildTree(node, depth) {
+          if (depth > ${maxDepth}) return '';
           const role = node.getAttribute?.('role') || node.tagName?.toLowerCase() || 'generic';
           const name = node.getAttribute?.('aria-label') || node.getAttribute?.('alt') || node.textContent?.trim().slice(0, 80) || '';
           const isInteractive = ['a', 'button', 'input', 'select', 'textarea'].includes(node.tagName?.toLowerCase()) || node.getAttribute?.('tabindex') != null;
@@ -64,7 +56,7 @@ export class Page implements IPage {
 
           let indent = '  '.repeat(depth);
           let line = indent + role;
-          if (name) line += ' "' + name.replace(/"/g, '\\"') + '"';
+          if (name) line += ' "' + name.replace(/"/g, '\\\\"') + '"';
           if (node.tagName?.toLowerCase() === 'a' && node.href) line += ' [' + node.href + ']';
           if (node.tagName?.toLowerCase() === 'input') line += ' [' + (node.type || 'text') + ']';
 
@@ -76,13 +68,10 @@ export class Page implements IPage {
           }
           return result;
         }
-        return buildTree(document.body);
+        return buildTree(document.body, 0);
       })()
     `;
-    const raw = await sendCommand('exec', {
-      code,
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
-    });
+    const raw = await sendCommand('exec', { code, ...this._tabOpt() });
     if (opts.raw) return raw;
     if (typeof raw === 'string') return formatSnapshot(raw, opts);
     return raw;
@@ -101,10 +90,7 @@ export class Page implements IPage {
         return 'clicked';
       })()
     `;
-    await sendCommand('exec', {
-      code,
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
-    });
+    await sendCommand('exec', { code, ...this._tabOpt() });
   }
 
   async typeText(ref: string, text: string): Promise<void> {
@@ -123,10 +109,7 @@ export class Page implements IPage {
         return 'typed';
       })()
     `;
-    await sendCommand('exec', {
-      code,
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
-    });
+    await sendCommand('exec', { code, ...this._tabOpt() });
   }
 
   async pressKey(key: string): Promise<void> {
@@ -138,10 +121,7 @@ export class Page implements IPage {
         return 'pressed';
       })()
     `;
-    await sendCommand('exec', {
-      code,
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
-    });
+    await sendCommand('exec', { code, ...this._tabOpt() });
   }
 
   async wait(options: number | { text?: string; time?: number; timeout?: number }): Promise<void> {
@@ -166,10 +146,7 @@ export class Page implements IPage {
           check();
         })
       `;
-      await sendCommand('exec', {
-        code,
-        ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
-      });
+      await sendCommand('exec', { code, ...this._tabOpt() });
     }
   }
 
@@ -190,7 +167,6 @@ export class Page implements IPage {
   }
 
   async networkRequests(includeStatic: boolean = false): Promise<any> {
-    // Use performance API to get network entries
     const code = `
       (() => {
         const entries = performance.getEntriesByType('resource');
@@ -204,15 +180,15 @@ export class Page implements IPage {
           }));
       })()
     `;
-    return sendCommand('exec', {
-      code,
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
-    });
+    return sendCommand('exec', { code, ...this._tabOpt() });
   }
 
   async consoleMessages(level: string = 'info'): Promise<any> {
-    // Console messages can't be retrospectively read via exec.
-    // Return empty for now — users should use networkRequests or evaluate.
+    // Console messages can't be retrospectively read via CDP Runtime.evaluate.
+    // Would need Runtime.consoleAPICalled event listener, which is not yet implemented.
+    if (process.env.OPENCLI_VERBOSE) {
+      console.error('[page] consoleMessages() not supported in lightweight mode — returning empty');
+    }
     return [];
   }
 
@@ -221,7 +197,7 @@ export class Page implements IPage {
     const dy = direction === 'up' ? -amount : direction === 'down' ? amount : 0;
     await sendCommand('exec', {
       code: `window.scrollBy(${dx}, ${dy})`,
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
+      ...this._tabOpt(),
     });
   }
 
@@ -248,27 +224,51 @@ export class Page implements IPage {
         }
       })()
     `;
-    await sendCommand('exec', {
-      code,
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
-    });
+    await sendCommand('exec', { code, ...this._tabOpt() });
   }
 
   async installInterceptor(pattern: string): Promise<void> {
+    const { generateInterceptorJs } = await import('../interceptor.js');
     await sendCommand('exec', {
       code: generateInterceptorJs(JSON.stringify(pattern), {
         arrayName: '__opencli_xhr',
         patchGuard: '__opencli_interceptor_patched',
       }),
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
+      ...this._tabOpt(),
     });
   }
 
   async getInterceptedRequests(): Promise<any[]> {
+    const { generateReadInterceptedJs } = await import('../interceptor.js');
     const result = await sendCommand('exec', {
       code: generateReadInterceptedJs('__opencli_xhr'),
-      ...(this._tabId !== undefined ? { tabId: this._tabId } : {}),
+      ...this._tabOpt(),
     });
     return (result as any[]) || [];
   }
 }
+
+// ─── Helpers ─────────────────────────────────────────────────────────
+
+/**
+ * Wrap JS code for CDP Runtime.evaluate:
+ * - Already an IIFE `(...)()` → send as-is
+ * - Arrow/function literal → wrap as IIFE `(code)()`
+ * - `new Promise(...)` or raw expression → send as-is (expression)
+ */
+function wrapForEval(js: string): string {
+  const code = js.trim();
+  if (!code) return 'undefined';
+
+  // Already an IIFE: `(async () => { ... })()` or `(function() {...})()`
+  if (/^\([\s\S]*\)\s*\(.*\)\s*$/.test(code)) return code;
+
+  // Arrow function: `() => ...` or `async () => ...`
+  if (/^(async\s+)?(\([^)]*\)|[A-Za-z_]\w*)\s*=>/.test(code)) return `(${code})()`;
+
+  // Function declaration: `function ...` or `async function ...`
+  if (/^(async\s+)?function[\s(]/.test(code)) return `(${code})()`;
+
+  // Everything else: bare expression, `new Promise(...)`, etc. → evaluate directly
+  return code;
+}

src/daemon.ts

@@ -74,6 +74,10 @@ async function handleRequest(req: IncomingMessage, res: ServerResponse): Promise
     resetIdleTimer();
     try {
       const body = JSON.parse(await readBody(req));
+      if (!body.id) {
+        jsonResponse(res, 400, { ok: false, error: 'Missing command id' });
+        return;
+      }
 
       if (!extensionWs || extensionWs.readyState !== WebSocket.OPEN) {
         jsonResponse(res, 503, { id: body.id, ok: false, error: 'Extension not connected. Please install the opencli Browser Bridge extension.' });