Skip to content

Commit 71b2c39

Browse files
jackwenerjackwener
authored
fix: harden browser automation pipeline (resolves #249) (#251)
- resolveTabId: validate URL even for explicit tabId, fall through to auto-resolve when tab is not debuggable or has been closed - handleNavigate: wait for URL change before checking 'complete' status to avoid race condition with stale about:blank - ensureAttached: pre-check tab URL, verify cached attach with probe, invalidate cache on URL change via onUpdated listener - daemon-client: recognize transient extension errors (disconnected, attach failed) as retryable with 1500ms delay; fresh command ID per attempt - pipeline executor: add per-step retry for browser steps (up to 2 retries on transient errors); cleanup automation window on pipeline failure - page.ts: selectTab/newTab/closeTab properly update/invalidate _tabId - daemon.ts: add WebSocket ping/pong heartbeat (15s interval, 2-miss disconnect) - Increase automation window idle timeout from 30s to 120s - Fix timeout param edge cases in BrowserBridge._ensureDaemon - Remove unused chalk import; fix trailing import placement Closes #249
1 parent b3b9892 commit 71b2c39

File tree

8 files changed

+311
-66
lines changed

8 files changed

+311
-66
lines changed

‎extension/dist/background.js‎

Lines changed: 91 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,33 @@ const WS_RECONNECT_BASE_DELAY = 2e3;
55
const WS_RECONNECT_MAX_DELAY = 6e4;
66

77
const attached = /* @__PURE__ */ new Set();
8+
function isDebuggableUrl$1(url) {
9+
if (!url) return false;
10+
return !url.startsWith("chrome://") && !url.startsWith("chrome-extension://");
11+
}
812
async function ensureAttached(tabId) {
9-
if (attached.has(tabId)) return;
13+
try {
14+
const tab = await chrome.tabs.get(tabId);
15+
if (!isDebuggableUrl$1(tab.url)) {
16+
attached.delete(tabId);
17+
throw new Error(`Cannot debug tab ${tabId}: URL is ${tab.url ?? "unknown"}`);
18+
}
19+
} catch (e) {
20+
if (e instanceof Error && e.message.startsWith("Cannot debug tab")) throw e;
21+
attached.delete(tabId);
22+
throw new Error(`Tab ${tabId} no longer exists`);
23+
}
24+
if (attached.has(tabId)) {
25+
try {
26+
await chrome.debugger.sendCommand({ tabId }, "Runtime.evaluate", {
27+
expression: "1",
28+
returnByValue: true
29+
});
30+
return;
31+
} catch {
32+
attached.delete(tabId);
33+
}
34+
}
1035
try {
1136
await chrome.debugger.attach({ tabId }, "1.3");
1237
} catch (e) {
@@ -89,6 +114,17 @@ function registerListeners() {
89114
chrome.debugger.onDetach.addListener((source) => {
90115
if (source.tabId) attached.delete(source.tabId);
91116
});
117+
chrome.tabs.onUpdated.addListener((tabId, info) => {
118+
if (info.url && !isDebuggableUrl$1(info.url)) {
119+
if (attached.has(tabId)) {
120+
attached.delete(tabId);
121+
try {
122+
chrome.debugger.detach({ tabId });
123+
} catch {
124+
}
125+
}
126+
}
127+
});
92128
}
93129

94130
let ws = null;
@@ -161,7 +197,7 @@ function scheduleReconnect() {
161197
}, delay);
162198
}
163199
const automationSessions = /* @__PURE__ */ new Map();
164-
const WINDOW_IDLE_TIMEOUT = 3e4;
200+
const WINDOW_IDLE_TIMEOUT = 12e4;
165201
function getWorkspaceKey(workspace) {
166202
return workspace?.trim() || "default";
167203
}
@@ -265,17 +301,30 @@ async function handleCommand(cmd) {
265301
};
266302
}
267303
}
268-
function isWebUrl(url) {
304+
function isDebuggableUrl(url) {
269305
if (!url) return false;
270306
return !url.startsWith("chrome://") && !url.startsWith("chrome-extension://");
271307
}
272308
async function resolveTabId(tabId, workspace) {
273-
if (tabId !== void 0) return tabId;
309+
if (tabId !== void 0) {
310+
try {
311+
const tab = await chrome.tabs.get(tabId);
312+
if (isDebuggableUrl(tab.url)) return tabId;
313+
console.warn(`[opencli] Tab ${tabId} URL is not debuggable (${tab.url}), re-resolving`);
314+
} catch {
315+
console.warn(`[opencli] Tab ${tabId} no longer exists, re-resolving`);
316+
}
317+
}
274318
const windowId = await getAutomationWindow(workspace);
275319
const tabs = await chrome.tabs.query({ windowId });
276-
const webTab = tabs.find((t) => t.id && isWebUrl(t.url));
277-
if (webTab?.id) return webTab.id;
278-
if (tabs.length > 0 && tabs[0]?.id) return tabs[0].id;
320+
const debuggableTab = tabs.find((t) => t.id && isDebuggableUrl(t.url));
321+
if (debuggableTab?.id) return debuggableTab.id;
322+
const reuseTab = tabs.find((t) => t.id);
323+
if (reuseTab?.id) {
324+
await chrome.tabs.update(reuseTab.id, { url: "about:blank" });
325+
await new Promise((resolve) => setTimeout(resolve, 200));
326+
return reuseTab.id;
327+
}
279328
const newTab = await chrome.tabs.create({ windowId, url: "about:blank", active: true });
280329
if (!newTab.id) throw new Error("Failed to create tab in automation window");
281330
return newTab.id;
@@ -292,7 +341,7 @@ async function listAutomationTabs(workspace) {
292341
}
293342
async function listAutomationWebTabs(workspace) {
294343
const tabs = await listAutomationTabs(workspace);
295-
return tabs.filter((tab) => isWebUrl(tab.url));
344+
return tabs.filter((tab) => isDebuggableUrl(tab.url));
296345
}
297346
async function handleExec(cmd, workspace) {
298347
if (!cmd.code) return { id: cmd.id, ok: false, error: "Missing code" };
@@ -307,28 +356,47 @@ async function handleExec(cmd, workspace) {
307356
async function handleNavigate(cmd, workspace) {
308357
if (!cmd.url) return { id: cmd.id, ok: false, error: "Missing url" };
309358
const tabId = await resolveTabId(cmd.tabId, workspace);
310-
await chrome.tabs.update(tabId, { url: cmd.url });
359+
const beforeTab = await chrome.tabs.get(tabId);
360+
const beforeUrl = beforeTab.url ?? "";
361+
const targetUrl = cmd.url;
362+
await chrome.tabs.update(tabId, { url: targetUrl });
363+
let timedOut = false;
311364
await new Promise((resolve) => {
312-
chrome.tabs.get(tabId).then((tab2) => {
313-
if (tab2.status === "complete") {
365+
let urlChanged = false;
366+
const listener = (id, info, tab2) => {
367+
if (id !== tabId) return;
368+
if (info.url && info.url !== beforeUrl) {
369+
urlChanged = true;
370+
}
371+
if (urlChanged && info.status === "complete") {
372+
chrome.tabs.onUpdated.removeListener(listener);
314373
resolve();
315-
return;
316374
}
317-
const listener = (id, info) => {
318-
if (id === tabId && info.status === "complete") {
375+
};
376+
chrome.tabs.onUpdated.addListener(listener);
377+
setTimeout(async () => {
378+
try {
379+
const currentTab = await chrome.tabs.get(tabId);
380+
if (currentTab.url !== beforeUrl && currentTab.status === "complete") {
319381
chrome.tabs.onUpdated.removeListener(listener);
320382
resolve();
321383
}
322-
};
323-
chrome.tabs.onUpdated.addListener(listener);
324-
setTimeout(() => {
325-
chrome.tabs.onUpdated.removeListener(listener);
326-
resolve();
327-
}, 15e3);
328-
});
384+
} catch {
385+
}
386+
}, 100);
387+
setTimeout(() => {
388+
chrome.tabs.onUpdated.removeListener(listener);
389+
timedOut = true;
390+
console.warn(`[opencli] Navigate to ${targetUrl} timed out after 15s`);
391+
resolve();
392+
}, 15e3);
329393
});
330394
const tab = await chrome.tabs.get(tabId);
331-
return { id: cmd.id, ok: true, data: { title: tab.title, url: tab.url, tabId } };
395+
return {
396+
id: cmd.id,
397+
ok: true,
398+
data: { title: tab.title, url: tab.url, tabId, timedOut }
399+
};
332400
}
333401
async function handleTabs(cmd, workspace) {
334402
switch (cmd.op) {
@@ -425,7 +493,7 @@ async function handleSessions(cmd) {
425493
const data = await Promise.all([...automationSessions.entries()].map(async ([workspace, session]) => ({
426494
workspace,
427495
windowId: session.windowId,
428-
tabCount: (await chrome.tabs.query({ windowId: session.windowId })).filter((tab) => isWebUrl(tab.url)).length,
496+
tabCount: (await chrome.tabs.query({ windowId: session.windowId })).filter((tab) => isDebuggableUrl(tab.url)).length,
429497
idleMsRemaining: Math.max(0, session.idleDeadlineAt - now)
430498
})));
431499
return { id: cmd.id, ok: true, data };

‎extension/src/background.ts‎

Lines changed: 64 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ type AutomationSession = {
9797
};
9898

9999
const automationSessions = new Map<string, AutomationSession>();
100-
const WINDOW_IDLE_TIMEOUT = 30000; // 30s
100+
const WINDOW_IDLE_TIMEOUT = 120000; // 120s — longer to survive slow pipelines
101101

102102
function getWorkspaceKey(workspace?: string): string {
103103
return workspace?.trim() || 'default';
@@ -238,7 +238,20 @@ function isDebuggableUrl(url?: string): boolean {
238238
* Otherwise, find or create a tab in the dedicated automation window.
239239
*/
240240
async function resolveTabId(tabId: number | undefined, workspace: string): Promise<number> {
241-
if (tabId !== undefined) return tabId;
241+
// Even when an explicit tabId is provided, validate it is still debuggable.
242+
// This prevents issues when extensions hijack the tab URL to chrome-extension://
243+
// or when the tab has been closed by the user.
244+
if (tabId !== undefined) {
245+
try {
246+
const tab = await chrome.tabs.get(tabId);
247+
if (isDebuggableUrl(tab.url)) return tabId;
248+
// Tab exists but URL is not debuggable — fall through to auto-resolve
249+
console.warn(`[opencli] Tab ${tabId} URL is not debuggable (${tab.url}), re-resolving`);
250+
} catch {
251+
// Tab was closed — fall through to auto-resolve
252+
console.warn(`[opencli] Tab ${tabId} no longer exists, re-resolving`);
253+
}
254+
}
242255

243256
// Get (or create) the automation window
244257
const windowId = await getAutomationWindow(workspace);
@@ -255,6 +268,8 @@ async function resolveTabId(tabId: number | undefined, workspace: string): Promi
255268
const reuseTab = tabs.find(t => t.id);
256269
if (reuseTab?.id) {
257270
await chrome.tabs.update(reuseTab.id, { url: 'about:blank' });
271+
// Wait briefly for the navigation to take effect
272+
await new Promise(resolve => setTimeout(resolve, 200));
258273
return reuseTab.id;
259274
}
260275

@@ -294,31 +309,62 @@ async function handleExec(cmd: Command, workspace: string): Promise<Result> {
294309
async function handleNavigate(cmd: Command, workspace: string): Promise<Result> {
295310
if (!cmd.url) return { id: cmd.id, ok: false, error: 'Missing url' };
296311
const tabId = await resolveTabId(cmd.tabId, workspace);
297-
await chrome.tabs.update(tabId, { url: cmd.url });
298312

299-
// Wait for page to finish loading, checking current status first to avoid race
313+
// Capture the current URL before navigation to detect actual URL change
314+
const beforeTab = await chrome.tabs.get(tabId);
315+
const beforeUrl = beforeTab.url ?? '';
316+
const targetUrl = cmd.url;
317+
318+
await chrome.tabs.update(tabId, { url: targetUrl });
319+
320+
// Wait for: 1) URL to change from the old URL, 2) tab.status === 'complete'
321+
// This avoids the race where 'complete' fires for the OLD URL (e.g. about:blank)
322+
let timedOut = false;
300323
await new Promise<void>((resolve) => {
301-
// Check if already complete (e.g. cached pages)
302-
chrome.tabs.get(tabId).then(tab => {
303-
if (tab.status === 'complete') { resolve(); return; }
324+
let urlChanged = false;
325+
326+
const listener = (id: number, info: chrome.tabs.TabChangeInfo, tab: chrome.tabs.Tab) => {
327+
if (id !== tabId) return;
328+
329+
// Track URL change (new URL differs from the one before navigation)
330+
if (info.url && info.url !== beforeUrl) {
331+
urlChanged = true;
332+
}
304333

305-
const listener = (id: number, info: chrome.tabs.TabChangeInfo) => {
306-
if (id === tabId && info.status === 'complete') {
334+
// Only resolve when both URL has changed AND status is complete
335+
if (urlChanged && info.status === 'complete') {
336+
chrome.tabs.onUpdated.removeListener(listener);
337+
resolve();
338+
}
339+
};
340+
chrome.tabs.onUpdated.addListener(listener);
341+
342+
// Also check if the tab already navigated (e.g. instant cache hit)
343+
setTimeout(async () => {
344+
try {
345+
const currentTab = await chrome.tabs.get(tabId);
346+
if (currentTab.url !== beforeUrl && currentTab.status === 'complete') {
307347
chrome.tabs.onUpdated.removeListener(listener);
308348
resolve();
309349
}
310-
};
311-
chrome.tabs.onUpdated.addListener(listener);
312-
// Timeout fallback
313-
setTimeout(() => {
314-
chrome.tabs.onUpdated.removeListener(listener);
315-
resolve();
316-
}, 15000);
317-
});
350+
} catch { /* tab gone */ }
351+
}, 100);
352+
353+
// Timeout fallback with warning
354+
setTimeout(() => {
355+
chrome.tabs.onUpdated.removeListener(listener);
356+
timedOut = true;
357+
console.warn(`[opencli] Navigate to ${targetUrl} timed out after 15s`);
358+
resolve();
359+
}, 15000);
318360
});
319361

320362
const tab = await chrome.tabs.get(tabId);
321-
return { id: cmd.id, ok: true, data: { title: tab.title, url: tab.url, tabId } };
363+
return {
364+
id: cmd.id,
365+
ok: true,
366+
data: { title: tab.title, url: tab.url, tabId, timedOut },
367+
};
322368
}
323369

324370
async function handleTabs(cmd: Command, workspace: string): Promise<Result> {

‎extension/src/cdp.ts‎

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,40 @@
88

99
const attached = new Set<number>();
1010

11+
/** Check if a URL can be attached via CDP */
12+
function isDebuggableUrl(url?: string): boolean {
13+
if (!url) return false;
14+
return !url.startsWith('chrome://') && !url.startsWith('chrome-extension://');
15+
}
16+
1117
async function ensureAttached(tabId: number): Promise<void> {
12-
if (attached.has(tabId)) return;
18+
// Verify the tab URL is debuggable before attempting attach
19+
try {
20+
const tab = await chrome.tabs.get(tabId);
21+
if (!isDebuggableUrl(tab.url)) {
22+
// Invalidate cache if previously attached
23+
attached.delete(tabId);
24+
throw new Error(`Cannot debug tab ${tabId}: URL is ${tab.url ?? 'unknown'}`);
25+
}
26+
} catch (e) {
27+
// Re-throw our own error, catch only chrome.tabs.get failures
28+
if (e instanceof Error && e.message.startsWith('Cannot debug tab')) throw e;
29+
attached.delete(tabId);
30+
throw new Error(`Tab ${tabId} no longer exists`);
31+
}
32+
33+
if (attached.has(tabId)) {
34+
// Verify the debugger is still actually attached by sending a harmless command
35+
try {
36+
await chrome.debugger.sendCommand({ tabId }, 'Runtime.evaluate', {
37+
expression: '1', returnByValue: true,
38+
});
39+
return; // Still attached and working
40+
} catch {
41+
// Stale cache entry — need to re-attach
42+
attached.delete(tabId);
43+
}
44+
}
1345

1446
try {
1547
await chrome.debugger.attach({ tabId }, '1.3');
@@ -122,4 +154,13 @@ export function registerListeners(): void {
122154
chrome.debugger.onDetach.addListener((source) => {
123155
if (source.tabId) attached.delete(source.tabId);
124156
});
157+
// Invalidate attached cache when tab URL changes to non-debuggable
158+
chrome.tabs.onUpdated.addListener((tabId, info) => {
159+
if (info.url && !isDebuggableUrl(info.url)) {
160+
if (attached.has(tabId)) {
161+
attached.delete(tabId);
162+
try { chrome.debugger.detach({ tabId }); } catch { /* ignore */ }
163+
}
164+
}
165+
});
125166
}

0 commit comments

Comments
 (0)