diff --git a/apps/extension/src/tools/__tests__/tabs.test.ts b/apps/extension/src/tools/__tests__/tabs.test.ts index 277384a..732ff41 100644 --- a/apps/extension/src/tools/__tests__/tabs.test.ts +++ b/apps/extension/src/tools/__tests__/tabs.test.ts @@ -613,6 +613,65 @@ describe("handleTabReturn", () => { expect(winSpies.create).toHaveBeenCalledOnce(); }); + it("does not fall back into another session's Agent Window (creates a new window instead)", async () => { + // Regression: a returned borrowed tab whose original window is gone + // must never be parked in *another* session's Agent Window. Agent + // Windows are `type: "normal"`, so getLastFocused({windowTypes: + // ["normal"]}) can return session bb22's window (200). Without the + // isAgentWindowId guard the tab would be moved into window 200, + // letting bb22 write to it and destroying it when bb22 stops. + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100, 200]) }); + const ctx = await sm.start("aa11"); + await sm.start("bb22"); // owns Agent Window 200 + ctx.borrowedTabs.set(7, { tabId: 7, originalWindowId: 300, originalIndex: 4 }); + const state: FakeTabState = { + tabs: new Map([[7, { id: 7, windowId: 100 } as chrome.tabs.Tab]]), + nextTabId: 50, + windowsClosed: new Set([300]), + }; + const { api, spies } = makeTabMutationApi(state); + const { api: windowsApi, spies: winSpies } = makeWindowsApi(state, { + lastFocused: 200, // session bb22's Agent Window + createWindowId: 777, + }); + const res = await handleTabReturn( + sm, + { session_id: "aa11", tab_id: 7 }, + { tabs: api, windows: windowsApi }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + expect(res.returned_to_window_id).toBe(777); + expect(res.fallback).toBe(true); + expect(winSpies.create).toHaveBeenCalledOnce(); + expect(spies.move).not.toHaveBeenCalledWith(7, expect.objectContaining({ windowId: 200 })); + }); + + it("returnBorrowedTab honours an explicit isAgentWindowId predicate", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + const ctx = await sm.start("aa11"); + ctx.borrowedTabs.set(7, { tabId: 7, originalWindowId: 300, originalIndex: 4 }); + const state: FakeTabState = { + tabs: new Map([[7, { id: 7, windowId: 100 } as chrome.tabs.Tab]]), + nextTabId: 50, + windowsClosed: new Set([300]), + }; + const { api, spies } = makeTabMutationApi(state); + const { api: windowsApi, spies: winSpies } = makeWindowsApi(state, { + lastFocused: 555, + createWindowId: 777, + }); + const res = await returnBorrowedTab(ctx, 7, { + tabs: api, + windows: windowsApi, + isAgentWindowId: (windowId) => windowId === 555, + }); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + expect(res.toWindowId).toBe(777); + expect(res.fallback).toBe(true); + expect(winSpies.create).toHaveBeenCalledOnce(); + expect(spies.move).not.toHaveBeenCalledWith(7, expect.objectContaining({ windowId: 555 })); + }); + it("returns not_found when the tab is not borrowed by this session", async () => { const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); await sm.start("aa11"); diff --git a/apps/extension/src/tools/session.ts b/apps/extension/src/tools/session.ts index cd2f416..6c14d7e 100644 --- a/apps/extension/src/tools/session.ts +++ b/apps/extension/src/tools/session.ts @@ -113,7 +113,13 @@ export async function handleSessionStop( const borrowedIds = Array.from(ctx.borrowedTabs.keys()); for (const tabId of borrowedIds) { try { - const outcome = await returnBorrowedTab(ctx, tabId, deps.tabManagement ?? {}); + const tabManagement = { + ...(deps.tabManagement ?? {}), + isAgentWindowId: + deps.tabManagement?.isAgentWindowId ?? + ((windowId: number) => manager.findByWindowId(windowId) !== null), + }; + const outcome = await returnBorrowedTab(ctx, tabId, tabManagement); if (typeof outcome === "object" && "code" in outcome) { console.warn(`[bsk session_stop] auto-return failed for tab ${tabId}`, outcome); returnFailures?.push({ diff --git a/apps/extension/src/tools/tabs.ts b/apps/extension/src/tools/tabs.ts index 748b071..6bad610 100644 --- a/apps/extension/src/tools/tabs.ts +++ b/apps/extension/src/tools/tabs.ts @@ -282,6 +282,16 @@ export interface TabManagementDeps { approveBorrow?: BorrowConfirmationApprover; /** Clears Agent-scoped overlays after a borrowed tab is returned. */ agentOverlayReset?: AgentOverlayResetApi; + /** + * Reports whether `windowId` is any live session's Agent Window. + * `tab_return`'s fallback window picker uses this to avoid moving a + * returned borrowed tab into *another* session's Agent Window (Agent + * Windows are created with `type: "normal"`, so they otherwise match + * the `getLastFocused({ windowTypes: ["normal"] })` query). Production + * callers derive this from the `SessionManager`; the `() => false` + * default only preserves legacy behaviour for direct/test callers. + */ + isAgentWindowId?: (windowId: number) => boolean; } function getTabsApi(deps: TabManagementDeps): TabMutationApi { @@ -292,6 +302,10 @@ function getWindowsApi(deps: TabManagementDeps): ChromeWindowsApi { return deps.windows ?? chromeWindowsApi; } +function getIsAgentWindowId(deps: TabManagementDeps): (windowId: number) => boolean { + return deps.isAgentWindowId ?? (() => false); +} + function getAgentOverlayResetApi(deps: TabManagementDeps): AgentOverlayResetApi { return deps.agentOverlayReset ?? chromeAgentOverlayResetApi; } @@ -818,12 +832,20 @@ function describeError(err: unknown): string { async function chooseFallbackWindow( ctx: SessionContext, windowsApi: ChromeWindowsApi, + isAgentWindowId: (windowId: number) => boolean, ): Promise<{ windowId: number; index: number } | RpcError> { let lastFocusedError: unknown; try { const last = await windowsApi.getLastFocused({ windowTypes: ["normal"] }); const lastId = typeof last?.id === "number" ? last.id : null; - if (lastId !== null && lastId !== ctx.agentWindowId) { + // Never relocate a user tab into *any* session's Agent Window: this + // session's is excluded explicitly, and other sessions' are excluded + // via `isAgentWindowId` (Agent Windows are `type: "normal"`, so they + // otherwise survive the `windowTypes: ["normal"]` filter). Parking a + // returned tab in another session's Agent Window would let that + // session write to it and, worse, see it destroyed when that session + // stops and closes its window. + if (lastId !== null && lastId !== ctx.agentWindowId && !isAgentWindowId(lastId)) { return { windowId: lastId, index: -1 }; } } catch (err) { @@ -888,6 +910,7 @@ export async function returnBorrowedTab( } const tabsApi = getTabsApi(deps); const windowsApi = getWindowsApi(deps); + const isAgentWindowId = getIsAgentWindowId(deps); let targetWindowId = entry.originalWindowId; let targetIndex = entry.originalIndex; @@ -903,7 +926,7 @@ export async function returnBorrowedTab( } if (!originalAlive) { fallback = true; - const target = await chooseFallbackWindow(ctx, windowsApi); + const target = await chooseFallbackWindow(ctx, windowsApi, isAgentWindowId); if ("code" in target) return target; targetWindowId = target.windowId; targetIndex = target.index; @@ -925,7 +948,7 @@ export async function returnBorrowedTab( }; } catch (err) { if (!fallback) { - const target = await chooseFallbackWindow(ctx, windowsApi); + const target = await chooseFallbackWindow(ctx, windowsApi, isAgentWindowId); if ("code" in target) { return { code: "cdp_failed", @@ -979,7 +1002,11 @@ export async function handleTabReturn( message: `tab_return: tab ${params.tab_id} is not borrowed by this session`, }; } - const outcome = await returnBorrowedTab(ctx, params.tab_id, deps); + const outcome = await returnBorrowedTab(ctx, params.tab_id, { + ...deps, + isAgentWindowId: + deps.isAgentWindowId ?? ((windowId) => manager.findByWindowId(windowId) !== null), + }); if (isRpcError(outcome)) return outcome; ctx.borrowedTabs.delete(params.tab_id); const result: TabReturnResult = {