From aa81e5e0f362d28bb8b1893476185161e826bcd7 Mon Sep 17 00:00:00 2001 From: jackwener Date: Tue, 24 Mar 2026 11:34:38 +0800 Subject: [PATCH] fix: resolve 6 critical bugs from deep code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. execution.ts: Guard lazy-loaded func commands against null page — if a lazy module incorrectly requires browser context, throw a clear error instead of a cryptic TypeError on page.goto(). 2. daemon.ts: Fix readBody race condition — add aborted flag to prevent req.destroy() from triggering both reject (via error) and resolve (via end event) on the same Promise, which could process truncated data. 3. browser/cdp.ts: Prevent CDPBridge.connect() reentry — throw if already connected instead of silently leaking the previous WebSocket and its message handlers. 4. interceptor.ts: Store intercept pattern in a separate global variable so subsequent installInterceptor calls with different patterns update the match condition without being blocked by the patchGuard. 5. record.ts: Always call cleanupEnter() after Promise.race — previously only called in the timeout path, leaving readline open when user pressed Enter, potentially blocking process exit. Also removed unused enterRace. 6. generate.ts: Fix undefined entering String.includes() — when c.name is undefined, toLowerCase() returns undefined which gets coerced to the string "undefined" by includes(), causing false positive matches. Co-Authored-By: Claude Opus 4.6 --- src/browser/cdp.ts | 2 ++ src/daemon.ts | 7 ++++--- src/execution.ts | 7 +++++-- src/generate.ts | 7 ++++--- src/interceptor.ts | 9 ++++++--- src/record.ts | 4 ++-- 6 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/browser/cdp.ts b/src/browser/cdp.ts index a776666a..2b34d366 100644 --- a/src/browser/cdp.ts +++ b/src/browser/cdp.ts @@ -51,6 +51,8 @@ export class CDPBridge { private _eventListeners = new Map void>>(); async connect(opts?: { timeout?: number; workspace?: string }): Promise { + if (this._ws) throw new Error('CDPBridge is already connected. Call close() before reconnecting.'); + const endpoint = process.env.OPENCLI_CDP_ENDPOINT; if (!endpoint) throw new Error('OPENCLI_CDP_ENDPOINT is not set'); diff --git a/src/daemon.ts b/src/daemon.ts index 01da631b..8b848d91 100644 --- a/src/daemon.ts +++ b/src/daemon.ts @@ -64,13 +64,14 @@ function readBody(req: IncomingMessage): Promise { return new Promise((resolve, reject) => { const chunks: Buffer[] = []; let size = 0; + let aborted = false; req.on('data', (c: Buffer) => { size += c.length; - if (size > MAX_BODY) { req.destroy(); reject(new Error('Body too large')); return; } + if (size > MAX_BODY) { aborted = true; req.destroy(); reject(new Error('Body too large')); return; } chunks.push(c); }); - req.on('end', () => resolve(Buffer.concat(chunks).toString('utf-8'))); - req.on('error', reject); + req.on('end', () => { if (!aborted) resolve(Buffer.concat(chunks).toString('utf-8')); }); + req.on('error', (err) => { if (!aborted) reject(err); }); }); } diff --git a/src/execution.ts b/src/execution.ts index 1ebbe4b1..4aa5a04b 100644 --- a/src/execution.ts +++ b/src/execution.ts @@ -98,11 +98,14 @@ async function runCommand( } // After loading, the module's cli() call will have updated the registry. const updated = getRegistry().get(fullName(cmd)); - if (updated?.func) return updated.func(page!, kwargs, debug); + if (updated?.func) { + if (!page) throw new CommandExecutionError(`Command ${fullName(cmd)} requires a browser session but none was provided`); + return updated.func(page, kwargs, debug); + } if (updated?.pipeline) return executePipeline(page, updated.pipeline, { args: kwargs, debug }); } - if (cmd.func) return cmd.func(page!, kwargs, debug); + if (cmd.func) return cmd.func(page as IPage, kwargs, debug); if (cmd.pipeline) return executePipeline(page, cmd.pipeline, { args: kwargs, debug }); throw new CommandExecutionError( `Command ${fullName(cmd)} has no func or pipeline`, diff --git a/src/generate.ts b/src/generate.ts index 9731f4f3..39126708 100644 --- a/src/generate.ts +++ b/src/generate.ts @@ -102,9 +102,10 @@ function selectCandidate(candidates: SynthesizeResult['candidates'], goal?: stri } const lower = (goal ?? '').trim().toLowerCase(); - const partial = candidates.find(c => - c.name?.toLowerCase().includes(lower) || lower.includes(c.name?.toLowerCase()) - ); + const partial = candidates.find(c => { + const cName = c.name?.toLowerCase() ?? ''; + return cName.includes(lower) || lower.includes(cName); + }); return partial ?? candidates[0]; } diff --git a/src/interceptor.ts b/src/interceptor.ts index 3dea6f40..115ab08c 100644 --- a/src/interceptor.ts +++ b/src/interceptor.ts @@ -24,15 +24,18 @@ export function generateInterceptorJs( const arr = opts.arrayName ?? '__opencli_intercepted'; const guard = opts.patchGuard ?? '__opencli_interceptor_patched'; + // Store the current pattern in a separate global so it can be updated + // without re-patching fetch/XHR (the patchGuard only prevents double-patching). + const patternVar = `${guard}_pattern`; + return ` () => { window.${arr} = window.${arr} || []; window.${arr}_errors = window.${arr}_errors || []; - const __pattern = ${patternExpr}; + window.${patternVar} = ${patternExpr}; + const __checkMatch = (url) => window.${patternVar} && url.includes(window.${patternVar}); if (!window.${guard}) { - const __checkMatch = (url) => __pattern && url.includes(__pattern); - // ── Patch fetch ── const __origFetch = window.fetch; window.fetch = async function(...args) { diff --git a/src/record.ts b/src/record.ts index f1f381b4..5e4bc6bb 100644 --- a/src/record.ts +++ b/src/record.ts @@ -397,10 +397,9 @@ export async function recordSession(opts: RecordOptions): Promise const stop = () => { stopped = true; }; const { promise: enterPromise, cleanup: cleanupEnter } = waitForEnter(); - const enterRace = enterPromise.then(stop); + enterPromise.then(stop); const timeoutPromise = new Promise(r => setTimeout(() => { stop(); - cleanupEnter(); // close readline to prevent process from hanging r(); }, timeoutMs)); @@ -428,6 +427,7 @@ export async function recordSession(opts: RecordOptions): Promise }, pollMs); await Promise.race([enterPromise, timeoutPromise]); + cleanupEnter(); // Always clean up readline to prevent process from hanging clearInterval(pollInterval); // Final drain from all known tabs