From f70978bfcfd0bf0e8b2053ee156afbbb8e81958f Mon Sep 17 00:00:00 2001 From: drakezhang Date: Mon, 7 Sep 2026 11:30:01 +0800 Subject: [PATCH 1/2] fix(extension): release automation state when returning borrowed tabs --- .../__tests__/chromium-cdp.test.ts | 149 ++++++++++++++++++ .../src/browser-driver/chromium-cdp.ts | 41 +++-- apps/extension/src/entrypoints/background.ts | 7 +- .../src/tools/__tests__/dispatcher.test.ts | 72 +++++++++ .../src/tools/__tests__/session.test.ts | 8 +- .../src/tools/__tests__/tabs.test.ts | 52 ++++++ apps/extension/src/tools/dispatcher.ts | 10 +- apps/extension/src/tools/session.ts | 5 +- apps/extension/src/tools/shared.ts | 1 + apps/extension/src/tools/tabs.ts | 18 ++- 10 files changed, 343 insertions(+), 20 deletions(-) diff --git a/apps/extension/src/browser-driver/__tests__/chromium-cdp.test.ts b/apps/extension/src/browser-driver/__tests__/chromium-cdp.test.ts index ed26fee..a11c718 100644 --- a/apps/extension/src/browser-driver/__tests__/chromium-cdp.test.ts +++ b/apps/extension/src/browser-driver/__tests__/chromium-cdp.test.ts @@ -651,6 +651,155 @@ describe("ChromiumCdp", () => { expect(cdp.networkEntriesSince(33, 0, 50, 1000).entries[0].url).toBeUndefined(); }); + it("releases only the returning session's claim on a tab", async () => { + const { api } = fakeApi(); + const cdp = new ChromiumCdp(api); + await cdp.ensureAttached(1); + await cdp.ensureAttached(2); + cdp.trackSessionTab("aa11", 1); + cdp.trackSessionTab("bb22", 1); + cdp.trackSessionTab("aa11", 2); + + await cdp.releaseSessionTab("unknown", 1); + await cdp.releaseSessionTab("aa11", 1); + expect(api.detach).not.toHaveBeenCalled(); + + await cdp.releaseSessionTab("bb22", 1); + await cdp.releaseSessionTab("bb22", 1); + expect(api.detach).toHaveBeenCalledExactlyOnceWith({ tabId: 1 }); + expect(cdp.isAttached(2)).toBe(true); + + await cdp.detachSession("aa11"); + expect(api.detach).toHaveBeenCalledTimes(2); + expect(api.detach).toHaveBeenLastCalledWith({ tabId: 2 }); + }); + + it.each([ + false, + true, + ])("releases a tab with an attachment in flight (new owner: %s)", async (newOwner) => { + const { api } = fakeApi(); + let finishAttach!: () => void; + vi.mocked(api.attach).mockImplementationOnce( + () => + new Promise((resolve) => { + finishAttach = resolve; + }), + ); + const cdp = new ChromiumCdp(api); + cdp.trackSessionTab("aa11", 1); + const attaching = cdp.ensureAttached(1); + const releasing = cdp.releaseSessionTab("aa11", 1); + if (newOwner) cdp.trackSessionTab("bb22", 1); + finishAttach(); + await Promise.all([attaching, releasing]); + + expect(cdp.isAttached(1)).toBe(newOwner); + expect(api.detach).toHaveBeenCalledTimes(newOwner ? 0 : 1); + }); + + it("stops accepting dialogs on a returned tab even when another session keeps CDP attached", async () => { + const { api, onEvent } = fakeApi(); + let controlled = true; + const cdp = new ChromiumCdp(api, { shouldAutoAcceptDialog: () => controlled }); + cdp.trackSessionTab("aa11", 1); + cdp.trackSessionTab("bb22", 1); + await cdp.ensureAttached(1); + onEvent.fire({ tabId: 1 }, "Page.javascriptDialogOpening", { + type: "confirm", + message: "before return", + }); + await vi.waitFor(() => expect(cdp.dialogsSince(1, 0)).toHaveLength(1)); + vi.mocked(api.sendCommand).mockClear(); + + controlled = false; + await cdp.releaseSessionTab("aa11", 1); + expect(cdp.isAttached(1)).toBe(true); + onEvent.fire({ tabId: 1 }, "Page.javascriptDialogOpening", { + type: "confirm", + message: "after return", + }); + await Promise.resolve(); + expect(api.sendCommand).not.toHaveBeenCalled(); + expect(cdp.dialogsSince(1, 0)).toHaveLength(1); + + controlled = true; + onEvent.fire({ tabId: 1 }, "Page.javascriptDialogOpening", { + type: "alert", + message: "borrowed again", + }); + await vi.waitFor(() => expect(cdp.dialogsSince(1, 0)).toHaveLength(2)); + expect(api.sendCommand).toHaveBeenCalledExactlyOnceWith( + { tabId: 1 }, + "Page.handleJavaScriptDialog", + { accept: true }, + ); + }); + + it("ignores dialog events after detach, including a pending eligibility check", async () => { + const { api, onEvent } = fakeApi(); + let resolveEligibility!: (value: boolean) => void; + const shouldAutoAcceptDialog = vi.fn( + () => + new Promise((resolve) => { + resolveEligibility = resolve; + }), + ); + const cdp = new ChromiumCdp(api, { shouldAutoAcceptDialog }); + await cdp.ensureAttached(1); + onEvent.fire({ tabId: 1 }, "Page.javascriptDialogOpening", { type: "alert" }); + await cdp.detach(1); + resolveEligibility(true); + await Promise.resolve(); + onEvent.fire({ tabId: 1 }, "Page.javascriptDialogOpening", { type: "alert" }); + + expect(shouldAutoAcceptDialog).toHaveBeenCalledOnce(); + expect(api.sendCommand).not.toHaveBeenCalledWith( + expect.anything(), + "Page.handleJavaScriptDialog", + expect.anything(), + ); + expect(cdp.dialogsSince(1, 0)).toEqual([]); + }); + + it("does not accept dialogs when the tab's current scope cannot be determined", async () => { + const { api, onEvent } = fakeApi(); + const shouldAutoAcceptDialog = vi.fn(async () => { + throw new Error("tab is gone"); + }); + const cdp = new ChromiumCdp(api, { shouldAutoAcceptDialog }); + await cdp.ensureAttached(1); + vi.mocked(api.sendCommand).mockClear(); + + onEvent.fire({ tabId: 1 }, "Page.javascriptDialogOpening", { type: "confirm" }); + await Promise.resolve(); + + expect(shouldAutoAcceptDialog).toHaveBeenCalledWith(1); + expect(api.sendCommand).not.toHaveBeenCalled(); + expect(cdp.dialogsSince(1, 0)).toEqual([]); + }); + + it("does not restore dialog state when an acceptance completes after detach", async () => { + const { api, onEvent } = fakeApi(); + const cdp = new ChromiumCdp(api); + await cdp.ensureAttached(1); + let finishAccept!: () => void; + vi.mocked(api.sendCommand).mockImplementationOnce( + () => + new Promise((resolve) => { + finishAccept = resolve; + }), + ); + + onEvent.fire({ tabId: 1 }, "Page.javascriptDialogOpening", { type: "alert" }); + await cdp.detach(1); + finishAccept(); + await Promise.resolve(); + + expect(cdp.dialogsSince(1, 0)).toEqual([]); + expect(cdp.dialogCursor(1)).toBe(0); + }); + it("detachSession only detaches tabs no other session owns", async () => { const { api } = fakeApi(); const cdp = new ChromiumCdp(api); diff --git a/apps/extension/src/browser-driver/chromium-cdp.ts b/apps/extension/src/browser-driver/chromium-cdp.ts index 2a24a25..4ebcad1 100644 --- a/apps/extension/src/browser-driver/chromium-cdp.ts +++ b/apps/extension/src/browser-driver/chromium-cdp.ts @@ -182,7 +182,13 @@ export class ChromiumCdp { private networkSubscription: { dispose(): void } | null = null; private frameTargetSubscription: { dispose(): void } | null = null; - constructor(api: CdpDebuggerApi = chromeDebuggerApi) { + constructor( + api: CdpDebuggerApi = chromeDebuggerApi, + private readonly options: { + /** CDP observation alone does not authorize dismissing user dialogs. */ + shouldAutoAcceptDialog?: (tabId: number) => boolean | Promise; + } = {}, + ) { this.api = api; this.bindAutoDetach(); this.bindDialogHandler(); @@ -453,6 +459,17 @@ export class ChromiumCdp { this.tabOwners.set(tabId, owners); } + /** Release one session's claim, preserving attachments still used by another. */ + async releaseSessionTab(sessionId: string, tabId: number): Promise { + const owners = this.tabOwners.get(tabId); + if (!owners?.delete(sessionId) || owners.size > 0) return; + this.tabOwners.delete(tabId); + // An observation may still be attaching when the tab is returned. Wait + // for it so detach cannot miss the attachment or remove a new owner's claim. + await this.attachInFlight.get(tabId)?.catch(() => {}); + if (!this.tabOwners.has(tabId)) await this.detach(tabId); + } + /** Subscribe to all CDP events. Returned disposable removes the listener. */ onEvent(handler: (source: CdpDebuggee, method: string, params: unknown) => void): { dispose(): void; @@ -639,13 +656,23 @@ export class ChromiumCdp { } private async onJavaScriptDialogOpening(tabId: number, params: unknown): Promise { + if (!this.attachedTabs.has(tabId) && !this.attachInFlight.has(tabId)) return; const parsed = parseDialogOpeningParams(params); try { + if ( + this.options.shouldAutoAcceptDialog && + !(await this.options.shouldAutoAcceptDialog(tabId)) + ) { + return; + } + // The tab may have been returned while its current scope was checked. + if (!this.attachedTabs.has(tabId) && !this.attachInFlight.has(tabId)) return; const handleParams: { accept: boolean; promptText?: string } = { accept: true }; if (parsed.type === "prompt") { handleParams.promptText = parsed.defaultPrompt ?? ""; } await this.api.sendCommand({ tabId }, "Page.handleJavaScriptDialog", handleParams); + if (!this.attachedTabs.has(tabId) && !this.attachInFlight.has(tabId)) return; const sequence = (this.dialogSequences.get(tabId) ?? 0) + 1; this.dialogSequences.set(tabId, sequence); this.appendDialog(tabId, { @@ -835,15 +862,9 @@ export class ChromiumCdp { /** Detach tabs only when no other live session has claimed them. */ async detachSession(sessionId: string): Promise { - const tabsToDetach: number[] = []; - for (const [tabId, owners] of this.tabOwners) { - owners.delete(sessionId); - if (owners.size === 0) { - this.tabOwners.delete(tabId); - tabsToDetach.push(tabId); - } - } - await Promise.all(tabsToDetach.map((tabId) => this.detach(tabId))); + await Promise.all( + Array.from(this.tabOwners.keys(), (tabId) => this.releaseSessionTab(sessionId, tabId)), + ); } } diff --git a/apps/extension/src/entrypoints/background.ts b/apps/extension/src/entrypoints/background.ts index 34d67a3..e9ee4fe 100644 --- a/apps/extension/src/entrypoints/background.ts +++ b/apps/extension/src/entrypoints/background.ts @@ -51,7 +51,12 @@ export default defineBackground(() => { const controller = new ConnectionController(); const transport = new WSTransport({ url: __BSK_DAEMON_WS_URL__ }); const sessions = new SessionManager(); - const cdp = new ChromiumCdp(); + const cdp = new ChromiumCdp(undefined, { + shouldAutoAcceptDialog: async (tabId) => { + const tab = await chrome.tabs.get(tabId); + return sessions.findByWindowId(tab.windowId) !== null; + }, + }); const sessionsLive = attachSessionsLiveFlag({ manager: sessions }); let overlayGeneration = 0; const controlModes = new Map(); diff --git a/apps/extension/src/tools/__tests__/dispatcher.test.ts b/apps/extension/src/tools/__tests__/dispatcher.test.ts index 98175f8..94cf4ee 100644 --- a/apps/extension/src/tools/__tests__/dispatcher.test.ts +++ b/apps/extension/src/tools/__tests__/dispatcher.test.ts @@ -747,6 +747,78 @@ describe("ToolDispatcher", () => { ); }); + it("releases the returned tab's hover before moving it and leaves other tabs alone", async () => { + const sendMessage = vi.fn(async () => undefined); + const move = vi.fn(async () => { + expect(sendMessage).toHaveBeenCalledWith(7, expect.objectContaining({ enabled: false })); + return { id: 7, windowId: 200, index: 4 }; + }); + vi.stubGlobal("chrome", { + tabs: { sendMessage, move }, + windows: { get: vi.fn(async () => ({ id: 200 })) }, + }); + const { transport, sent, deliver } = fakeTransport(); + const sessions = new SessionManager({ + agentWindow: { + create: vi.fn(async () => 100), + remove: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), + }, + }); + const ctx = await sessions.start("aa11"); + ctx.borrowedTabs.set(7, { tabId: 7, originalWindowId: 200, originalIndex: 4 }); + const cdp = { + send: vi.fn(async () => ({}) as T), + detachSession: vi.fn(async () => {}), + releaseSessionTab: vi.fn(async () => {}), + }; + const dispatcher = new ToolDispatcher({ + transport, + sessions, + cdp: cdp as unknown as TestDispatcherCdp, + }); + const helpers = dispatcher as unknown as { + rememberHover: (sessionId: string, result: unknown) => unknown; + setHoverBypass: (sessionId: string, tabId: number, enabled: boolean) => Promise; + withHoverReassert: ( + params: { session_id: string; tab_id?: number }, + work: () => Promise, + ) => Promise; + }; + for (const [sessionId, tabId] of [ + ["aa11", 7], + ["aa11", 8], + ["bb22", 9], + ] as const) { + helpers.rememberHover(sessionId, { tab_id: tabId, x: 10, y: 20 }); + await helpers.setHoverBypass(sessionId, tabId, true); + } + dispatcher.start(); + deliver(makeRequest("tool.tab_return", { session_id: "aa11", tab_id: 7 })); + await vi.waitFor(() => expect(sent).toHaveLength(1)); + + expect(sent[0]).toMatchObject({ result: { tab_id: 7, returned_to_window_id: 200 } }); + expect(cdp.releaseSessionTab).toHaveBeenCalledExactlyOnceWith("aa11", 7); + await helpers.withHoverReassert({ session_id: "aa11", tab_id: 7 }, async () => ({})); + expect(cdp.send).not.toHaveBeenCalled(); + for (const [sessionId, tabId] of [ + ["aa11", 8], + ["bb22", 9], + ] as const) { + expect(sendMessage).not.toHaveBeenCalledWith( + tabId, + expect.objectContaining({ enabled: false }), + ); + await helpers.withHoverReassert({ session_id: sessionId, tab_id: tabId }, async () => ({})); + expect(cdp.send).toHaveBeenLastCalledWith(tabId, "Input.dispatchMouseEvent", { + type: "mouseMoved", + x: 10, + y: 20, + }); + } + dispatcher.stop(); + }); + it("transfers hover overlay bypass ownership between sessions", async () => { vi.stubGlobal("chrome", { tabs: { diff --git a/apps/extension/src/tools/__tests__/session.test.ts b/apps/extension/src/tools/__tests__/session.test.ts index f575c93..e87e01a 100644 --- a/apps/extension/src/tools/__tests__/session.test.ts +++ b/apps/extension/src/tools/__tests__/session.test.ts @@ -257,11 +257,15 @@ describe("handleSessionStop with auto-return", () => { moves: [], }; const { tabs, windows } = makeApis(state, { moveThrowsFor: new Set([1]) }); + const cdp = { + detachSession: vi.fn(async () => {}), + releaseSessionTab: vi.fn(async () => {}), + }; const res = await handleSessionStop( sm, { session_id: "aa11" }, - { tabManagement: { tabs, windows } }, + { cdp, tabManagement: { tabs, windows } }, ); if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); expect(res.return_failures?.map((f) => f.tab_id)).toEqual([1]); @@ -269,6 +273,8 @@ describe("handleSessionStop with auto-return", () => { expect(sm.has("aa11")).toBe(true); expect(ctx.borrowedTabs.has(1)).toBe(true); expect(ctx.borrowedTabs.has(2)).toBe(false); + expect(cdp.releaseSessionTab).toHaveBeenCalledExactlyOnceWith("aa11", 2); + expect(cdp.detachSession).not.toHaveBeenCalled(); expect(aw.remove).not.toHaveBeenCalled(); }); diff --git a/apps/extension/src/tools/__tests__/tabs.test.ts b/apps/extension/src/tools/__tests__/tabs.test.ts index ffc6444..b6b3764 100644 --- a/apps/extension/src/tools/__tests__/tabs.test.ts +++ b/apps/extension/src/tools/__tests__/tabs.test.ts @@ -547,6 +547,58 @@ describe("handleTabBorrow", () => { }); describe("handleTabReturn", () => { + it.each([ + "original", + "fallback", + "cleanup_failed", + "failed", + "cancelled", + ])("releases CDP only after a successful return (%s)", async (mode) => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + const ctx = await sm.start("aa11"); + ctx.borrowedTabs.set(7, { tabId: 7, originalWindowId: 200, originalIndex: 4 }); + const state: FakeTabState = { + tabs: new Map([[7, { id: 7, windowId: 100 } as chrome.tabs.Tab]]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api, spies } = makeTabMutationApi(state); + const { api: windowsApi } = makeWindowsApi(state, { lastFocused: 500 }); + const success = mode !== "failed" && mode !== "cancelled"; + if (mode === "fallback") spies.move.mockRejectedValueOnce(new Error("original move failed")); + if (mode === "failed") spies.move.mockRejectedValue(new Error("move failed")); + let windowAtRelease: number | undefined; + const cdp = { + releaseSessionTab: vi.fn(async () => { + windowAtRelease = state.tabs.get(7)?.windowId; + if (mode === "cleanup_failed") throw new Error("debugger release failed"); + }), + }; + const controller = new AbortController(); + if (mode === "cancelled") controller.abort(); + + const result = await handleTabReturn( + sm, + { session_id: "aa11", tab_id: 7 }, + { + tabs: api, + windows: windowsApi, + cdp, + signal: controller.signal, + agentOverlayReset: { resetAgentOverlays: vi.fn(async () => {}) }, + }, + ); + + expect("code" in result).toBe(!success); + expect(ctx.borrowedTabs.has(7)).toBe(!success); + expect(cdp.releaseSessionTab).toHaveBeenCalledTimes(success ? 1 : 0); + if (success) { + expect(cdp.releaseSessionTab).toHaveBeenCalledWith("aa11", 7); + expect(windowAtRelease).toBe(mode === "fallback" ? 500 : 200); + expect(spies.move).toHaveBeenCalledTimes(mode === "fallback" ? 2 : 1); + } + }); + it("returns the tab to its original window/index and clears borrowedTabs", async () => { const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); const ctx = await sm.start("aa11"); diff --git a/apps/extension/src/tools/dispatcher.ts b/apps/extension/src/tools/dispatcher.ts index 8202186..c396af8 100644 --- a/apps/extension/src/tools/dispatcher.ts +++ b/apps/extension/src/tools/dispatcher.ts @@ -345,7 +345,15 @@ export class ToolDispatcher { return result; } case "tool.tab_return": - return handleTabReturn(this.sessions, req.params as TabReturnParams, { signal }); + return this.withHoverReleaseForRequest( + req.params as TabReturnParams, + () => + handleTabReturn(this.sessions, req.params as TabReturnParams, { + signal, + cdp: this.cdp, + }), + signal, + ); case "tool.window_resize": return handleWindowResize( this.sessions, diff --git a/apps/extension/src/tools/session.ts b/apps/extension/src/tools/session.ts index 338e5c2..b6fe38f 100644 --- a/apps/extension/src/tools/session.ts +++ b/apps/extension/src/tools/session.ts @@ -2,7 +2,7 @@ import { type SessionManager, SessionStartCleanupError } from "@/session-manager import type { RpcError } from "@/transport/types"; import { rpcError } from "./errors"; import { clearRecordingForSession } from "./record"; -import type { ChromeTabsApi } from "./shared"; +import type { CdpRunner, ChromeTabsApi } from "./shared"; import { isRpcError } from "./shared"; import { returnBorrowedTab, type TabManagementDeps } from "./tabs"; @@ -84,7 +84,7 @@ export interface SessionStopResult { export interface SessionStopDeps { signal?: AbortSignal; - cdp?: { + cdp?: Pick & { detachSession(sessionId: string): Promise; }; /** @@ -209,6 +209,7 @@ export async function handleSessionStop( try { const tabManagement = { ...(deps.tabManagement ?? {}), + cdp: deps.cdp ?? deps.tabManagement?.cdp, signal: deps.signal, isAgentWindowId: deps.tabManagement?.isAgentWindowId ?? diff --git a/apps/extension/src/tools/shared.ts b/apps/extension/src/tools/shared.ts index 9483abf..82aed79 100644 --- a/apps/extension/src/tools/shared.ts +++ b/apps/extension/src/tools/shared.ts @@ -57,6 +57,7 @@ export interface CdpRunner { getFrameGraph?(tabId: number): Promise; ensureAttachedToUrl?(tabId: number, expectedUrl: string | undefined): Promise; trackSessionTab?(sessionId: string, tabId: number): void; + releaseSessionTab?(sessionId: string, tabId: number): Promise; onEvent?(handler: (source: CdpDebuggee, method: string, params: unknown) => void): { dispose(): void; }; diff --git a/apps/extension/src/tools/tabs.ts b/apps/extension/src/tools/tabs.ts index 9057354..80fca96 100644 --- a/apps/extension/src/tools/tabs.ts +++ b/apps/extension/src/tools/tabs.ts @@ -10,7 +10,7 @@ import { import type { SessionContext, SessionManager } from "@/session-manager/manager"; import type { RpcError } from "@/transport/types"; import { rpcError } from "./errors"; -import { isRpcError, lookupSession } from "./shared"; +import { type CdpRunner, isRpcError, lookupSession } from "./shared"; export type TabScope = "user" | "agent" | "all"; @@ -287,6 +287,8 @@ export interface TabManagementDeps { approveBorrow?: BorrowConfirmationApprover; /** Clears Agent-scoped overlays after a borrowed tab is returned. */ agentOverlayReset?: AgentOverlayResetApi; + /** Releases this session's CDP claim after a borrowed tab is returned. */ + cdp?: Pick; /** * Reports whether `windowId` is any live session's Agent Window. * `tab_return`'s fallback window picker uses this to avoid moving a @@ -945,16 +947,22 @@ async function cleanupUnusedFallbackWindow( } } -function resetAgentOverlaysInReturnedTab( +async function releaseReturnedTabState( ctx: SessionContext, tabId: number, deps: TabManagementDeps, -): void { +): Promise { void getAgentOverlayResetApi(deps) .resetAgentOverlays(tabId, ctx.sessionId) .catch((err) => { console.debug("[bsk tab_return] agent overlay reset failed", err); }); + try { + await deps.cdp?.releaseSessionTab?.(ctx.sessionId, tabId); + } catch (err) { + // A successful move must not be retried because debugger cleanup failed. + console.debug("[bsk tab_return] CDP release failed", err); + } } /** @@ -1028,7 +1036,7 @@ export async function returnBorrowedTab( }); const movedTab = Array.isArray(moved) ? moved[0] : moved; const finalIndex = typeof movedTab?.index === "number" ? movedTab.index : targetIndex; - resetAgentOverlaysInReturnedTab(ctx, tabId, deps); + await releaseReturnedTabState(ctx, tabId, deps); return { tabId, toWindowId: targetWindowId, @@ -1070,7 +1078,7 @@ export async function returnBorrowedTab( }); const movedTab = Array.isArray(moved) ? moved[0] : moved; const finalIndex = typeof movedTab?.index === "number" ? movedTab.index : target.index; - resetAgentOverlaysInReturnedTab(ctx, tabId, deps); + await releaseReturnedTabState(ctx, tabId, deps); return { tabId, toWindowId: target.windowId, From cef59137e4ce1b9dc007278791913d1103b842c3 Mon Sep 17 00:00:00 2001 From: drakezhang Date: Mon, 7 Sep 2026 11:44:11 +0800 Subject: [PATCH 2/2] fix(extension): handle tab return cleanup edge cases --- .../__tests__/chromium-cdp.test.ts | 63 +++++++++++++ .../src/browser-driver/chromium-cdp.ts | 30 ++++-- .../src/tools/__tests__/dispatcher.test.ts | 57 ++++++++++++ .../src/tools/__tests__/tabs.test.ts | 92 +++++++++++++++++++ apps/extension/src/tools/dispatcher.ts | 12 +-- apps/extension/src/tools/interaction.ts | 2 + apps/extension/src/tools/tabs.ts | 3 + 7 files changed, 244 insertions(+), 15 deletions(-) diff --git a/apps/extension/src/browser-driver/__tests__/chromium-cdp.test.ts b/apps/extension/src/browser-driver/__tests__/chromium-cdp.test.ts index a11c718..fccd4e2 100644 --- a/apps/extension/src/browser-driver/__tests__/chromium-cdp.test.ts +++ b/apps/extension/src/browser-driver/__tests__/chromium-cdp.test.ts @@ -254,6 +254,69 @@ describe("ChromiumCdp", () => { expect(cdp.isAttached(7)).toBe(false); }); + it.each([ + false, + true, + ])("waits for detach before reconnecting a returned tab (detach rejects: %s)", async (rejectDetach) => { + const { api } = fakeApi(); + const attached = new Set(); + let finishDetach!: () => void; + vi.mocked(api.attach).mockImplementation(async ({ tabId }) => { + if (attached.has(tabId!)) throw new Error("Another debugger is already attached"); + attached.add(tabId!); + }); + vi.mocked(api.detach).mockImplementation(async ({ tabId }) => { + attached.delete(tabId!); + }); + vi.mocked(api.detach).mockImplementationOnce( + ({ tabId }) => + new Promise((resolve, reject) => { + finishDetach = () => { + attached.delete(tabId!); + if (rejectDetach) reject(new Error("tab already detached")); + else resolve(); + }; + }), + ); + const cdp = new ChromiumCdp(api); + cdp.trackSessionTab("aa11", 7); + await cdp.ensureAttached(7); + const returning = cdp.releaseSessionTab("aa11", 7); + await vi.waitFor(() => expect(api.detach).toHaveBeenCalledOnce()); + let duplicateDetachFinished = false; + const duplicateDetach = cdp.detach(7).then(() => { + duplicateDetachFinished = true; + }); + + cdp.trackSessionTab("bb22", 7); + // Register rejection handlers immediately; the pre-fix driver rejects + // both commands while the browser still has the previous attachment. + const pending = Promise.allSettled([ + cdp.send(7, "Runtime.evaluate", { expression: "document.title" }), + cdp.send(7, "DOM.getDocument"), + ]); + await cdp.send(8, "DOM.getDocument"); + const finishedEarly = duplicateDetachFinished; + finishDetach(); + await Promise.all([returning, duplicateDetach]); + + expect(await pending).toEqual([ + { status: "fulfilled", value: { ok: true } }, + { status: "fulfilled", value: { ok: true } }, + ]); + expect(finishedEarly).toBe(false); + expect(vi.mocked(api.attach).mock.calls.filter(([target]) => target.tabId === 7)).toHaveLength( + 2, + ); + expect(cdp.isAttached(7)).toBe(true); + expect(cdp.isAttached(8)).toBe(true); + await cdp.detachSession("bb22"); + expect(api.detach).toHaveBeenCalledTimes(2); + expect(cdp.isAttached(7)).toBe(false); + expect(attached.has(7)).toBe(false); + expect(cdp.isAttached(8)).toBe(true); + }); + it("detachAll() iterates every cached tab", async () => { const { api } = fakeApi(); const cdp = new ChromiumCdp(api); diff --git a/apps/extension/src/browser-driver/chromium-cdp.ts b/apps/extension/src/browser-driver/chromium-cdp.ts index 4ebcad1..8af2abc 100644 --- a/apps/extension/src/browser-driver/chromium-cdp.ts +++ b/apps/extension/src/browser-driver/chromium-cdp.ts @@ -165,6 +165,7 @@ export class ChromiumCdp { private readonly api: CdpDebuggerApi; private readonly attachedTabs = new Set(); private readonly attachInFlight = new Map>(); + private readonly detachInFlight = new Map>(); private readonly tabOwners = new Map>(); private readonly dialogBuffers = new Map(); private readonly dialogSequences = new Map(); @@ -199,6 +200,10 @@ export class ChromiumCdp { /** Attach to `tabId` if we haven't already in this driver. */ async ensureAttached(tabId: number): Promise { + // Returning a tab clears the cache before Chrome finishes detaching. + // New observers must wait before opening the next connection to that tab. + const detaching = this.detachInFlight.get(tabId); + if (detaching) await detaching; if (this.attachedTabs.has(tabId)) return; const existing = this.attachInFlight.get(tabId); if (existing) { @@ -431,6 +436,11 @@ export class ChromiumCdp { /** Detach if attached; never throws. */ async detach(tabId: number): Promise { + const existing = this.detachInFlight.get(tabId); + if (existing) { + await existing; + return; + } this.attachInFlight.delete(tabId); if (!this.attachedTabs.has(tabId)) return; this.attachedTabs.delete(tabId); @@ -438,13 +448,19 @@ export class ChromiumCdp { this.clearConsoleState(tabId); this.clearNetworkState(tabId); this.clearFrameState(tabId); - try { - await this.api.detach({ tabId }); - } catch (err) { - // Tab may already be gone — Chrome auto-detaches on close. Log - // at debug so production builds aren't noisy. - console.debug("[bsk cdp] detach failed (likely tab already closed)", err); - } + const detach = (async () => { + try { + await this.api.detach({ tabId }); + } catch (err) { + // Tab may already be gone — Chrome auto-detaches on close. Log + // at debug so production builds aren't noisy. + console.debug("[bsk cdp] detach failed (likely tab already closed)", err); + } + })().finally(() => { + this.detachInFlight.delete(tabId); + }); + this.detachInFlight.set(tabId, detach); + await detach; } /** True iff `ensureAttached(tabId)` has succeeded since the last detach. */ diff --git a/apps/extension/src/tools/__tests__/dispatcher.test.ts b/apps/extension/src/tools/__tests__/dispatcher.test.ts index 94cf4ee..bb0216f 100644 --- a/apps/extension/src/tools/__tests__/dispatcher.test.ts +++ b/apps/extension/src/tools/__tests__/dispatcher.test.ts @@ -747,6 +747,63 @@ describe("ToolDispatcher", () => { ); }); + it.each([ + { tab_id: 7, code: "not_found" }, + { tab_id: undefined, code: "invalid_params" }, + ])("keeps hover when tab_return is rejected with $code", async ({ tab_id, code }) => { + const sendMessage = vi.fn(async () => undefined); + vi.stubGlobal("chrome", { + tabs: { + sendMessage, + get: vi.fn(async () => ({ id: 7, windowId: 100, active: true })), + query: vi.fn(async () => [{ id: 7, windowId: 100, active: true }]), + }, + }); + const { transport, sent, deliver } = fakeTransport(); + const sessions = new SessionManager({ + agentWindow: { + create: vi.fn(async () => 100), + remove: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), + }, + }); + await sessions.start("aa11"); + const cdp = { + send: vi.fn(async () => ({}) as T), + detachSession: vi.fn(async () => {}), + releaseSessionTab: vi.fn(async () => {}), + }; + const dispatcher = new ToolDispatcher({ + transport, + sessions, + cdp: cdp as unknown as TestDispatcherCdp, + }); + const helpers = dispatcher as unknown as { + rememberHover: (sessionId: string, result: unknown) => unknown; + setHoverBypass: (sessionId: string, tabId: number, enabled: boolean) => Promise; + withHoverReassert: ( + params: { session_id: string; tab_id?: number }, + work: () => Promise, + ) => Promise; + }; + helpers.rememberHover("aa11", { tab_id: 7, x: 10, y: 20 }); + await helpers.setHoverBypass("aa11", 7, true); + dispatcher.start(); + deliver(makeRequest("tool.tab_return", { session_id: "aa11", tab_id })); + await vi.waitFor(() => expect(sent).toHaveLength(1)); + + expect(sent[0]).toMatchObject({ error: { code } }); + expect(cdp.releaseSessionTab).not.toHaveBeenCalled(); + expect(sendMessage).not.toHaveBeenCalledWith(7, expect.objectContaining({ enabled: false })); + await helpers.withHoverReassert({ session_id: "aa11", tab_id: 7 }, async () => ({})); + expect(cdp.send).toHaveBeenCalledExactlyOnceWith(7, "Input.dispatchMouseEvent", { + type: "mouseMoved", + x: 10, + y: 20, + }); + dispatcher.stop(); + }); + it("releases the returned tab's hover before moving it and leaves other tabs alone", async () => { const sendMessage = vi.fn(async () => undefined); const move = vi.fn(async () => { diff --git a/apps/extension/src/tools/__tests__/tabs.test.ts b/apps/extension/src/tools/__tests__/tabs.test.ts index b6b3764..9e1270e 100644 --- a/apps/extension/src/tools/__tests__/tabs.test.ts +++ b/apps/extension/src/tools/__tests__/tabs.test.ts @@ -1,5 +1,7 @@ import { describe, expect, it, vi } from "vitest"; +import { type CdpDebuggerApi, ChromiumCdp } from "@/browser-driver/chromium-cdp"; import { SessionManager } from "@/session-manager/manager"; +import { handleHover } from "../interaction"; import { type AgentOverlayResetApi, type ChromeWindowsApi, @@ -547,6 +549,96 @@ describe("handleTabBorrow", () => { }); describe("handleTabReturn", () => { + it.each([ + "not_found", + "cdp_failed", + ])("detaches after selector resolution fails with %s", async (code) => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + const ctx = await sm.start("aa11"); + ctx.borrowedTabs.set(7, { tabId: 7, originalWindowId: 200, originalIndex: 4 }); + const state: FakeTabState = { + tabs: new Map([[7, { id: 7, windowId: 100 } as chrome.tabs.Tab]]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api } = makeTabMutationApi(state); + const { api: windows } = makeWindowsApi(state); + const debuggerApi = { + attach: vi.fn(async () => {}), + detach: vi.fn(async () => {}), + sendCommand: vi.fn(async (_target: unknown, method: string) => { + if (method === "DOM.getDocument") return { root: { nodeId: 1 } }; + if (method === "DOM.querySelector") { + if (code === "cdp_failed") throw new Error("selector query failed"); + return { nodeId: 0 }; + } + return {}; + }), + onEvent: { addListener: vi.fn(), removeListener: vi.fn() }, + onDetach: { addListener: vi.fn(), removeListener: vi.fn() }, + }; + const cdp = new ChromiumCdp(debuggerApi as unknown as CdpDebuggerApi); + + const hover = await handleHover( + sm, + { session_id: "aa11", tab_id: 7, selector: "#missing" }, + { + cdp, + tabsApi: { get: api.get, query: vi.fn(async () => []) }, + }, + ); + expect(hover).toMatchObject({ code }); + expect(cdp.isAttached(7)).toBe(true); + + const returned = await handleTabReturn( + sm, + { session_id: "aa11", tab_id: 7 }, + { + tabs: api, + windows, + cdp, + agentOverlayReset: { resetAgentOverlays: vi.fn(async () => {}) }, + }, + ); + expect(returned).toMatchObject({ returned_to_window_id: 200 }); + expect(ctx.borrowedTabs.has(7)).toBe(false); + expect(cdp.isAttached(7)).toBe(false); + expect(debuggerApi.detach).toHaveBeenCalledExactlyOnceWith({ tabId: 7 }); + cdp.dispose(); + }); + + it("honours cancellation while preparing a validated return", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + const ctx = await sm.start("aa11"); + ctx.borrowedTabs.set(7, { tabId: 7, originalWindowId: 200, originalIndex: 4 }); + const state: FakeTabState = { + tabs: new Map([[7, { id: 7, windowId: 100 } as chrome.tabs.Tab]]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api, spies } = makeTabMutationApi(state); + const { api: windows } = makeWindowsApi(state); + const controller = new AbortController(); + const beforeReturn = vi.fn(async () => { + controller.abort(); + }); + const result = await handleTabReturn( + sm, + { session_id: "aa11", tab_id: 7 }, + { + tabs: api, + windows, + signal: controller.signal, + beforeReturn, + }, + ); + + expect(result).toMatchObject({ code: "cancelled" }); + expect(beforeReturn).toHaveBeenCalledExactlyOnceWith("aa11", 7); + expect(spies.move).not.toHaveBeenCalled(); + expect(ctx.borrowedTabs.has(7)).toBe(true); + }); + it.each([ "original", "fallback", diff --git a/apps/extension/src/tools/dispatcher.ts b/apps/extension/src/tools/dispatcher.ts index c396af8..64e451f 100644 --- a/apps/extension/src/tools/dispatcher.ts +++ b/apps/extension/src/tools/dispatcher.ts @@ -345,15 +345,11 @@ export class ToolDispatcher { return result; } case "tool.tab_return": - return this.withHoverReleaseForRequest( - req.params as TabReturnParams, - () => - handleTabReturn(this.sessions, req.params as TabReturnParams, { - signal, - cdp: this.cdp, - }), + return handleTabReturn(this.sessions, req.params as TabReturnParams, { signal, - ); + cdp: this.cdp, + beforeReturn: (sessionId, tabId) => this.releaseHoverLatch(sessionId, tabId), + }); case "tool.window_resize": return handleWindowResize( this.sessions, diff --git a/apps/extension/src/tools/interaction.ts b/apps/extension/src/tools/interaction.ts index 54646ed..18608ca 100644 --- a/apps/extension/src/tools/interaction.ts +++ b/apps/extension/src/tools/interaction.ts @@ -183,6 +183,8 @@ export async function resolveBackendNode( } // selector path try { + // Selector lookup itself attaches CDP, even when no element is found. + cdp.trackSessionTab?.(ctx.sessionId, target.tabId); const doc = await cdp.send<{ root?: { nodeId?: number } }>(target.tabId, "DOM.getDocument", { depth: 0, }); diff --git a/apps/extension/src/tools/tabs.ts b/apps/extension/src/tools/tabs.ts index 80fca96..feb9763 100644 --- a/apps/extension/src/tools/tabs.ts +++ b/apps/extension/src/tools/tabs.ts @@ -289,6 +289,8 @@ export interface TabManagementDeps { agentOverlayReset?: AgentOverlayResetApi; /** Releases this session's CDP claim after a borrowed tab is returned. */ cdp?: Pick; + /** Runs after tab_return validation, before moving the borrowed tab. */ + beforeReturn?: (sessionId: string, tabId: number) => Promise; /** * Reports whether `windowId` is any live session's Agent Window. * `tab_return`'s fallback window picker uses this to avoid moving a @@ -1124,6 +1126,7 @@ export async function handleTabReturn( message: `tab_return: tab ${params.tab_id} is not borrowed by this session`, }; } + await deps.beforeReturn?.(ctx.sessionId, params.tab_id); const outcome = await returnBorrowedTab(ctx, params.tab_id, { ...deps, isAgentWindowId: