From 01b8b6b5bf4dca44c094efb9c9ed9ac0ca562e1a Mon Sep 17 00:00:00 2001 From: jackwener Date: Thu, 19 Mar 2026 15:58:14 +0800 Subject: [PATCH] refactor: fix 14 issues from deep code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 2 - extension/manifest.json | 11 ---- extension/src/background.ts | 51 ++++++++-------- extension/src/cdp.ts | 12 +++- src/browser.test.ts | 9 --- src/browser/daemon-client.ts | 39 +++++++++---- src/browser/index.ts | 3 - src/browser/mcp.ts | 7 +-- src/browser/page.ts | 110 +++++++++++++++++------------------ src/daemon.ts | 4 ++ 10 files changed, 127 insertions(+), 121 deletions(-) delete mode 100644 extension/content/noop.js diff --git a/extension/content/noop.js b/extension/content/noop.js deleted file mode 100644 index 2698aae..0000000 --- a/extension/content/noop.js +++ /dev/null @@ -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. diff --git a/extension/manifest.json b/extension/manifest.json index d0c84c9..c2a2d6c 100644 --- a/extension/manifest.json +++ b/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": [ - "" - ], - "content_scripts": [ - { - "matches": ["http://*/*", "https://*/*"], - "js": ["content/noop.js"], - "run_at": "document_idle" - } - ], "background": { "service_worker": "dist/background.js", "type": "module" diff --git a/extension/src/background.ts b/extension/src/background.ts index 2dcebde..30ec020 100644 --- a/extension/src/background.ts +++ b/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 { // ─── 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 { 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 { 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((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 { 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 { } 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 { 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); diff --git a/extension/src/cdp.ts b/extension/src/cdp.ts index 8d4409a..390133d 100644 --- a/extension/src/cdp.ts +++ b/extension/src/cdp.ts @@ -15,7 +15,17 @@ async function ensureAttached(tabId: number): Promise { 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}`); } } diff --git a/src/browser.test.ts b/src/browser.test.ts index ca679a5..dba6826 100644 --- a/src/browser.test.ts +++ b/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'); diff --git a/src/browser/daemon-client.ts b/src/browser/daemon-client.ts index d7810cf..c12689b 100644 --- a/src/browser/daemon-client.ts +++ b/src/browser/daemon-client.ts @@ -65,6 +65,7 @@ export async function isExtensionConnected(): Promise { /** * 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 { 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; } diff --git a/src/browser/index.ts b/src/browser/index.ts index adec262..0c08f08 100644 --- a/src/browser/index.ts +++ b/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, diff --git a/src/browser/mcp.ts b/src/browser/mcp.ts index d40a9a8..e3bbf49 100644 --- a/src/browser/mcp.ts +++ b/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 = {}): { id: number; message: string } { - const id = ++_jsonRpcId; - return { id, message: JSON.stringify({ id, method, params }) }; -} + /** * Browser factory: manages daemon lifecycle and provides IPage instances. diff --git a/src/browser/page.ts b/src/browser/page.ts index 2ef766b..3097f56 100644 --- a/src/browser/page.ts +++ b/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 { + return this._tabId !== undefined ? { tabId: this._tabId } : {}; + } + async goto(url: string): Promise { 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 { - 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 { - // 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 { @@ -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 { @@ -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 { @@ -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 { - // 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 { - // 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 { + 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 { + 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; +} diff --git a/src/daemon.ts b/src/daemon.ts index 568eda7..21b29db 100644 --- a/src/daemon.ts +++ b/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.' });