From a4fed136e36ef8174499aeaf0b7d8315e830e1f5 Mon Sep 17 00:00:00 2001 From: Ljy-0827 Date: Thu, 6 Aug 2026 14:36:38 +0800 Subject: [PATCH] fix(extension): keep user-opened tabs free & fix agent-tab leak on session stop (issue #57) - session_stop: exclude tabs still tracked in agentCreatedTabs from the release decision so a tab that failed to close is no longer mistaken for a user tab (which previously kept the window open and leaked the tab). - Distinguish user-created tabs from agent-created tabs in the Agent Window via a pending-tab counter consumed by chrome.tabs.onCreated, so tabs the user opens themselves (new-tab button / Cmd+T / open in new tab) are never taken over by the BrowserSkill control mask. - background overlay: decide per-tab state BEFORE showing controls (overlayStateForTab + pushOverlayStateForTab) so a user tab receives hidden on its first overlay.ready ping instead of flashing control then hiding. - AgentWindowApi.ensureActiveTab now resolves with the activated/created tab id instead of void, so SessionManager can record homeTabId. This fixes the tsc failure flagged in review (the interface previously returned Promise while callers treated the result as number | null). - Wire the production tabManagement.tabs / tabsQuery deps into tool.session_stop, both in the dispatcher and in the disconnect cleanup. They were never injected, which left the agent-tab cleanup and the window-release path as dead code outside tests (behaviour identical to main). Added a dispatcher-level integration test that fails without the wiring: a surviving user tab must release the window, not close it. --- apps/extension/src/entrypoints/background.ts | 82 +++++- .../__tests__/disconnect-cleanup.test.ts | 4 +- .../__tests__/event-handler.test.ts | 8 +- .../session-manager/__tests__/manager.test.ts | 51 +++- .../src/session-manager/agent-window.ts | 17 +- apps/extension/src/session-manager/manager.ts | 84 ++++++- .../src/tools/__tests__/console.test.ts | 2 +- .../src/tools/__tests__/dispatcher.test.ts | 88 +++++-- .../src/tools/__tests__/emulate.test.ts | 2 +- .../src/tools/__tests__/evaluate.test.ts | 2 +- .../src/tools/__tests__/interaction.test.ts | 2 +- .../src/tools/__tests__/navigation.test.ts | 2 +- .../src/tools/__tests__/network.test.ts | 2 +- .../src/tools/__tests__/observation.test.ts | 2 +- .../src/tools/__tests__/session.test.ts | 237 +++++++++++++++++- .../src/tools/__tests__/shared.test.ts | 2 +- .../src/tools/__tests__/snapshot-ref.test.ts | 2 +- .../src/tools/__tests__/tabs.test.ts | 2 +- .../src/tools/__tests__/waits.test.ts | 2 +- .../src/tools/__tests__/window.test.ts | 2 +- apps/extension/src/tools/dispatcher.ts | 6 + apps/extension/src/tools/session.ts | 92 ++++++- apps/extension/src/tools/tabs.ts | 12 + 23 files changed, 645 insertions(+), 60 deletions(-) diff --git a/apps/extension/src/entrypoints/background.ts b/apps/extension/src/entrypoints/background.ts index 4edd0a2..a3f773b 100644 --- a/apps/extension/src/entrypoints/background.ts +++ b/apps/extension/src/entrypoints/background.ts @@ -41,6 +41,8 @@ import { attachRecordStepListener, type RecordRuntimeDeps, } from "@/tools/record"; +import { chromeTabsApi } from "@/tools/shared"; +import { chromeTabMutationApi } from "@/tools/tabs"; import { detectBrowserMeta } from "@/transport/handshake"; import type { Transport } from "@/transport/transport"; import { WSTransport } from "@/transport/ws-transport"; @@ -80,6 +82,29 @@ export default defineBackground(() => { }; } + /** + * Authoritative overlay state for a *specific* tab. A user-created tab + * (`ctx.userTabs`) inside the Agent Window must never show the control + * mask — return hidden immediately. This is what lets a freshly-mounted + * content script receive the correct state on its first `overlay.ready` + * ping instead of flashing control and then hiding (design: decide before + * showing, never show then correct). + */ + function overlayStateForTab(tabId?: number, windowId?: number): OverlayAgentStateMessage { + if (typeof tabId === "number" && typeof windowId === "number") { + const ctx = sessions.findByWindowId(windowId); + if (ctx && ctx.userTabs.has(tabId)) { + return { + type: OVERLAY_AGENT_STATE, + sessionId: null, + mode: "hidden", + generation: overlayGeneration, + }; + } + } + return overlayStateForWindow(windowId); + } + async function pushOverlayStateToTab( tabId: number, state: OverlayAgentStateMessage, @@ -91,13 +116,27 @@ export default defineBackground(() => { } } + async function pushOverlayStateForTab(tabId: number, windowId?: number): Promise { + const state = overlayStateForTab(tabId, windowId); + await pushOverlayStateToTab(tabId, state); + } + async function pushOverlayStateForWindow(windowId: number): Promise { - const state = overlayStateForWindow(windowId); + const ctx = sessions.findByWindowId(windowId); + const baseState = overlayStateForWindow(windowId); const tabs = await chrome.tabs.query({ windowId }); await Promise.all( - tabs.map((tab) => - typeof tab.id === "number" ? pushOverlayStateToTab(tab.id, state) : Promise.resolve(), - ), + tabs.map((tab) => { + if (typeof tab.id !== "number") return Promise.resolve(); + // A user-created tab in the Agent Window must stay free for the user + // to operate — never show the control mask over it. Override the + // window-level control state with a hidden state for those tabs. + const isUserTab = ctx ? ctx.userTabs.has(tab.id) : false; + const state: OverlayAgentStateMessage = isUserTab + ? { ...baseState, sessionId: null, mode: "hidden" } + : baseState; + return pushOverlayStateToTab(tab.id, state); + }), ); } @@ -136,13 +175,40 @@ export default defineBackground(() => { if (typeof tab.windowId !== "number") return; pushOverlayStateForAgentWindow(tab.windowId); }); + // A new tab inside an Agent Window is either agent-created (via + // tool.tab_create, flagged by the pending count) or user-created (via + // Chrome UI). Classify it so user-opened tabs are kept free and can be + // pushed a hidden overlay immediately. See SessionManager.classifyNewTab. + chrome.tabs.onCreated.addListener((tab) => { + if (typeof tab.windowId !== "number" || typeof tab.id !== "number") return; + const kind = sessions.classifyNewTab(tab.id, tab.windowId); + if (kind === "user") { + // User tab: explicitly free it from the agent control mask. + void pushOverlayStateToTab(tab.id, { + type: OVERLAY_AGENT_STATE, + sessionId: null, + mode: "hidden", + generation: overlayGeneration, + }); + } else if (kind === "agent" || kind === "initializing") { + // Agent tab (or home tab): make sure it reflects the session's + // current control mode. + void pushOverlayStateForAgentWindow(tab.windowId); + } + }); // Re-sync the storage.session flag on SW startup so a previous SW's // stale `true` does not keep waking us on every page load until the // first mutation (review M4/M5 round 3 m-R3-1). void sessionsLive.refresh(); const cleanupAfterDisconnect = createDisconnectCleanup({ manager: sessions, - sessionStopDeps: { cdp }, + // Same contract as the dispatcher's `tool.session_stop`: without these + // deps the agent-tab cleanup and the window-release path are dead code. + sessionStopDeps: { + cdp, + tabManagement: { tabs: chromeTabMutationApi }, + tabsQuery: chromeTabsApi, + }, onSessionsChanged: () => { void sessionsLive.syncFromManager(); }, @@ -309,7 +375,11 @@ export default defineBackground(() => { } if (msg.kind === OVERLAY_MSG_READY) { - sendResponse(overlayStateForWindow(sender.tab?.windowId)); + // Decide per-tab *before* sending: a user-created tab gets hidden + // immediately so the content script never flashes the control mask. + if (typeof sender.tab?.id === "number") { + void pushOverlayStateForTab(sender.tab.id, sender.tab.windowId); + } return false; } diff --git a/apps/extension/src/session-manager/__tests__/disconnect-cleanup.test.ts b/apps/extension/src/session-manager/__tests__/disconnect-cleanup.test.ts index 3ca893a..f35a2a2 100644 --- a/apps/extension/src/session-manager/__tests__/disconnect-cleanup.test.ts +++ b/apps/extension/src/session-manager/__tests__/disconnect-cleanup.test.ts @@ -9,7 +9,7 @@ describe("disconnect session cleanup", () => { const manager = new SessionManager({ agentWindow: { create: vi.fn(async () => nextWindowId++), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), remove, }, }); @@ -41,7 +41,7 @@ describe("disconnect session cleanup", () => { const manager = new SessionManager({ agentWindow: { create: vi.fn(async () => 100), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), remove: vi.fn(() => removeGate), }, }); diff --git a/apps/extension/src/session-manager/__tests__/event-handler.test.ts b/apps/extension/src/session-manager/__tests__/event-handler.test.ts index 7ccae33..5aaf338 100644 --- a/apps/extension/src/session-manager/__tests__/event-handler.test.ts +++ b/apps/extension/src/session-manager/__tests__/event-handler.test.ts @@ -39,7 +39,7 @@ describe("attachSessionEventHandler", () => { agentWindow: { create: vi.fn(async () => 4242), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); await manager.start("aa11"); @@ -73,7 +73,7 @@ describe("attachSessionEventHandler", () => { agentWindow: { create: vi.fn(async () => 4242), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const ctx = await manager.start("aa11"); @@ -113,7 +113,7 @@ describe("attachSessionEventHandler", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const transport = fakeTransport(); @@ -129,7 +129,7 @@ describe("attachSessionEventHandler", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const transport = fakeTransport(); diff --git a/apps/extension/src/session-manager/__tests__/manager.test.ts b/apps/extension/src/session-manager/__tests__/manager.test.ts index 966693e..cd166b4 100644 --- a/apps/extension/src/session-manager/__tests__/manager.test.ts +++ b/apps/extension/src/session-manager/__tests__/manager.test.ts @@ -13,7 +13,7 @@ function fakeAgentWindow(): AgentWindowApi & { return id; }); const removeMock = vi.fn(async (_id: number) => {}); - const ensureActiveTabMock = vi.fn(async (_windowId: number, _url: string) => {}); + const ensureActiveTabMock = vi.fn(async (_windowId: number, _url: string) => 0); return { create: createMock, remove: removeMock, @@ -151,6 +151,55 @@ describe("SessionManager", () => { expect(sm.list()).toEqual([]); }); + describe("classifyNewTab (user vs agent tab freedom)", () => { + it("classifies a tab born during window init as agent (home tab)", async () => { + const aw = fakeAgentWindow(); + const sm = new SessionManager({ agentWindow: aw }); + const ctx = await sm.start("aa11"); + // Simulate the onCreated event for the home tab while still initializing. + // (start() already clears the flag, so re-add to emulate the race.) + (sm as unknown as { windowInitializing: Set }).windowInitializing.add( + ctx.agentWindowId, + ); + const kind = sm.classifyNewTab(0, ctx.agentWindowId); + expect(kind).toBe("initializing"); + expect(ctx.agentCreatedTabs.has(0)).toBe(true); + expect(ctx.userTabs.has(0)).toBe(false); + }); + + it("classifies a pending tab_create tab as agent", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow() }); + const ctx = await sm.start("aa11"); + sm.markAgentTabPending(ctx.agentWindowId); + const kind = sm.classifyNewTab(11, ctx.agentWindowId); + expect(kind).toBe("agent"); + expect(ctx.agentCreatedTabs.has(11)).toBe(true); + expect(ctx.userTabs.has(11)).toBe(false); + expect(ctx.pendingAgentTabCount).toBe(0); + }); + + it("classifies a user-opened tab (no pending) as user and keeps it free", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow() }); + const ctx = await sm.start("aa11"); + const kind = sm.classifyNewTab(99, ctx.agentWindowId); + expect(kind).toBe("user"); + expect(ctx.userTabs.has(99)).toBe(true); + expect(ctx.agentCreatedTabs.has(99)).toBe(false); + }); + + it("matches multiple pending agent tabs to multiple onCreated events", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow() }); + const ctx = await sm.start("aa11"); + sm.markAgentTabPending(ctx.agentWindowId); + sm.markAgentTabPending(ctx.agentWindowId); + expect(sm.classifyNewTab(11, ctx.agentWindowId)).toBe("agent"); + // Second agent tab still pending → agent; not mistaken for user. + expect(sm.classifyNewTab(12, ctx.agentWindowId)).toBe("agent"); + // No more pending → a later user tab is user. + expect(sm.classifyNewTab(99, ctx.agentWindowId)).toBe("user"); + }); + }); + describe("findBorrowingSession", () => { it("returns null when no session has borrowed the tab", async () => { const sm = new SessionManager({ agentWindow: fakeAgentWindow() }); diff --git a/apps/extension/src/session-manager/agent-window.ts b/apps/extension/src/session-manager/agent-window.ts index 73f634b..1336dc7 100644 --- a/apps/extension/src/session-manager/agent-window.ts +++ b/apps/extension/src/session-manager/agent-window.ts @@ -14,8 +14,11 @@ export interface AgentWindowApi { * Guarantee the Agent Window has an active, CDP-navigable tab. * `chrome://` pages (including the New Tab page) reject `Page.navigate`, * so sessions bootstrap with `about:blank` instead. + * + * Resolves with the id of the activated (or newly created) tab, so callers + * can track the session's home tab without re-querying Chrome. */ - ensureActiveTab(windowId: number, url: string): Promise; + ensureActiveTab(windowId: number, url: string): Promise; } /** Creation hints for a new Agent Window. */ @@ -49,15 +52,19 @@ export const chromeAgentWindowApi: AgentWindowApi = { // cancellation success while the Agent Window remains open. await chrome.windows.remove(windowId); }, - async ensureActiveTab(windowId: number, url: string): Promise { + async ensureActiveTab(windowId: number, url: string): Promise { const tabs = await chrome.tabs.query({ windowId }); const first = tabs.find((t) => typeof t.id === "number"); - if (first?.id) { + if (first?.id !== undefined) { if (!first.active) { await chrome.tabs.update(first.id, { active: true }); } - return; + return first.id; } - await chrome.tabs.create({ windowId, url, active: true }); + const created = await chrome.tabs.create({ windowId, url, active: true }); + if (typeof created?.id !== "number") { + throw new Error("[bh] chrome.tabs.create returned no tab id"); + } + return created.id; }, }; diff --git a/apps/extension/src/session-manager/manager.ts b/apps/extension/src/session-manager/manager.ts index 2fef0d7..f8557dd 100644 --- a/apps/extension/src/session-manager/manager.ts +++ b/apps/extension/src/session-manager/manager.ts @@ -6,6 +6,38 @@ export interface SessionContext { agentWindowId: number; refStore: RefStore; borrowedTabs: Map; + /** + * Tabs created by the agent via `tool.tab_create` in this session's + * Agent Window. Tracked so `session_stop` can close them before + * releasing the window (design §3.1). User-created tabs (via Chrome UI) + * never enter this set. + */ + agentCreatedTabs: Set; + /** + * Tabs the user opened themselves inside the Agent Window via Chrome UI + * (new-tab button, Cmd+T, right-click → open in new tab, …). These must + * NOT be controlled by the agent overlay and must be left free for the + * user to operate. Distinguishing them from agent-created tabs is done + * by the `chrome.tabs.onCreated` listener in background.ts, which consults + * `pendingAgentTabCount` to tell "opened via tool.tab_create" apart from + * "opened by the user". + */ + userTabs: Set; + /** + * Number of agent tabs currently being created via `tool.tab_create` but + * whose `onCreated` event has not yet been observed. `handleTabCreate` + * increments this *before* calling `chrome.tabs.create`; the + * `onCreated` listener decrements it once per new tab in the Agent Window. + * This lets us tell agent-created tabs from user-created tabs even though + * both fire `onCreated` (design §3.1, user-tab freedom fix). + */ + pendingAgentTabCount: number; + /** + * Id of the home tab created/activated when the session started + * (`ensureActiveTab`). Used to clean up the home tab precisely on + * stop instead of matching by URL (design §3.2). + */ + homeTabId: number | null; createdAtMs: number; } @@ -81,6 +113,13 @@ export class SessionManager { private readonly sessions = new Map(); private readonly windowIndex = new Map(); private readonly borrowReservations = new Map(); + /** + * Windows that are still being initialised by `start()` (the home tab is + * being created). Tabs born into one of these windows via `onCreated` + * belong to the agent (the home tab), not the user, and must not be + * mistaken for user-created tabs. + */ + private readonly windowInitializing = new Set(); private readonly agentWindow: AgentWindowApi; private readonly now: () => number; @@ -179,7 +218,10 @@ export class SessionManager { const { signal: _signal, ...createOptions } = opts; windowId = await this.agentWindow.create(AGENT_WINDOW_HOME, createOptions); throwIfSessionStartAborted(opts.signal); - await this.agentWindow.ensureActiveTab(windowId, AGENT_WINDOW_HOME); + // Mark the window as initialising so `onCreated` (driven by the home + // tab) does not mistake the home tab for a user-created tab. + this.windowInitializing.add(windowId); + const homeTabId = await this.agentWindow.ensureActiveTab(windowId, AGENT_WINDOW_HOME); throwIfSessionStartAborted(opts.signal); const ctx: SessionContext = { @@ -187,13 +229,19 @@ export class SessionManager { agentWindowId: windowId, refStore: new RefStore(), borrowedTabs: new Map(), + agentCreatedTabs: new Set(), + userTabs: new Set(), + pendingAgentTabCount: 0, + homeTabId, createdAtMs: this.now(), }; this.sessions.set(sessionId, ctx); this.windowIndex.set(windowId, sessionId); + this.windowInitializing.delete(windowId); return ctx; } catch (startupError) { if (windowId !== null) { + this.windowInitializing.delete(windowId); try { await this.agentWindow.remove(windowId); } catch (cleanupError) { @@ -204,6 +252,40 @@ export class SessionManager { } } + /** + * Record that a `tool.tab_create` is about to open a tab. Called *before* + * `chrome.tabs.create` so the pending count is visible to the + * `onCreated` listener that will fire for the new tab. + */ + markAgentTabPending(windowId: number): void { + const ctx = this.findByWindowId(windowId); + if (!ctx) return; + ctx.pendingAgentTabCount += 1; + } + + /** + * Called by the `onCreated` listener for each new tab in an Agent Window. + * Returns `"agent"` when the tab corresponds to a pending `tool.tab_create` + * (and registers it), `"user"` when the user opened it via Chrome UI, or + * `"initializing"` when the window is still booting its home tab. + */ + classifyNewTab(tabId: number, windowId: number): "agent" | "user" | "initializing" | "unknown" { + const ctx = this.findByWindowId(windowId); + if (!ctx) return "unknown"; + if (this.windowInitializing.has(windowId)) { + // Home tab / window boot — belongs to the agent, not the user. + ctx.agentCreatedTabs.add(tabId); + return "initializing"; + } + if (ctx.pendingAgentTabCount > 0) { + ctx.pendingAgentTabCount -= 1; + ctx.agentCreatedTabs.add(tabId); + return "agent"; + } + ctx.userTabs.add(tabId); + return "user"; + } + /** * Tear down a session: close its Agent Window and drop the context. * diff --git a/apps/extension/src/tools/__tests__/console.test.ts b/apps/extension/src/tools/__tests__/console.test.ts index a84e2be..650bf0d 100644 --- a/apps/extension/src/tools/__tests__/console.test.ts +++ b/apps/extension/src/tools/__tests__/console.test.ts @@ -13,7 +13,7 @@ function fakeAgentWindow(ids: number[]) { return id; }), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }; } diff --git a/apps/extension/src/tools/__tests__/dispatcher.test.ts b/apps/extension/src/tools/__tests__/dispatcher.test.ts index c05cdb2..dc5e421 100644 --- a/apps/extension/src/tools/__tests__/dispatcher.test.ts +++ b/apps/extension/src/tools/__tests__/dispatcher.test.ts @@ -55,7 +55,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 4242), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); await sessions.start("aa11"); @@ -117,7 +117,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 4242), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); @@ -140,7 +140,7 @@ describe("ToolDispatcher", () => { agentWindow: { create, remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); @@ -158,7 +158,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 4242), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); await sessions.start("aa11"); @@ -172,6 +172,48 @@ describe("ToolDispatcher", () => { expect(sent[0]).toEqual({ id: "r-1", result: {} }); }); + it("wires the production tab APIs into tool.session_stop so a surviving user tab releases the window", async () => { + // Regression guard for the deps that session_stop reads directly: when + // `tabManagement.tabs` / `tabsQuery` are not injected by the dispatcher, + // the agent-tab cleanup and the release decision silently no-op and the + // Agent Window is closed even though a user tab is still open (issue #57). + const removeTab = vi.fn(async () => {}); + const closeWindow = vi.fn(async () => {}); + // After cleanup the window still holds one tab (id 99) that is neither + // the home tab nor an agent-created tab — i.e. a genuine user tab. + vi.stubGlobal("chrome", { + tabs: { + remove: removeTab, + query: vi.fn(async () => [{ id: 99, windowId: 4242, active: true }]), + }, + }); + const { transport, sent, deliver } = fakeTransport(); + const sessions = new SessionManager({ + agentWindow: { + create: vi.fn(async () => 4242), + remove: closeWindow, + ensureActiveTab: vi.fn(async () => 1), + }, + }); + await sessions.start("aa11"); + // Register tab 7 as agent-created (pending counter + onCreated classify). + sessions.markAgentTabPending(4242); + sessions.classifyNewTab(7, 4242); + + const dispatcher = new ToolDispatcher({ transport, sessions }); + dispatcher.start(); + + deliver(makeRequest("tool.session_stop", { session_id: "aa11" })); + await flushMicrotasks(); + + // The agent tab is closed through the real chrome.tabs surface... + expect(removeTab).toHaveBeenCalledWith(7); + // ...and the window is released rather than closed, so the user's tab survives. + expect(closeWindow).not.toHaveBeenCalled(); + expect(sessions.has("aa11")).toBe(false); + expect(sent[0]).toEqual({ id: "r-1", result: { window_released: true } }); + }); + it("routes tool.console through the CDP console buffer", async () => { vi.stubGlobal("chrome", { tabs: { @@ -183,7 +225,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 4242), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); await sessions.start("aa11"); @@ -238,7 +280,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 4242), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); await sessions.start("aa11"); @@ -274,7 +316,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 4242), remove, - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); await sessions.start("aa11"); @@ -322,7 +364,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); @@ -342,7 +384,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); @@ -366,7 +408,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); @@ -383,7 +425,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); @@ -400,7 +442,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const onSessionsChanged = vi.fn(); @@ -422,7 +464,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const onBrowserControlResumed = vi.fn(); @@ -456,7 +498,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const order: string[] = []; @@ -538,7 +580,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const cdp = { @@ -599,7 +641,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); @@ -636,7 +678,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 4242), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); await sessions.start("aa11"); @@ -688,7 +730,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); @@ -722,7 +764,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 4242), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); transport.send = () => { @@ -744,7 +786,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 5555), remove, - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); transport.send = () => { @@ -772,7 +814,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(() => createPromise), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); @@ -823,7 +865,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(async () => 1), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); @@ -844,7 +886,7 @@ describe("ToolDispatcher", () => { agentWindow: { create: vi.fn(() => createPromise), remove: vi.fn(), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }, }); const dispatcher = new ToolDispatcher({ transport, sessions }); diff --git a/apps/extension/src/tools/__tests__/emulate.test.ts b/apps/extension/src/tools/__tests__/emulate.test.ts index 37b375b..d8e27af 100644 --- a/apps/extension/src/tools/__tests__/emulate.test.ts +++ b/apps/extension/src/tools/__tests__/emulate.test.ts @@ -20,7 +20,7 @@ function fakeAgentWindow(ids: number[]) { return id; }), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }; } diff --git a/apps/extension/src/tools/__tests__/evaluate.test.ts b/apps/extension/src/tools/__tests__/evaluate.test.ts index 5b4df70..0065499 100644 --- a/apps/extension/src/tools/__tests__/evaluate.test.ts +++ b/apps/extension/src/tools/__tests__/evaluate.test.ts @@ -12,7 +12,7 @@ function fakeAgentWindow(ids: number[]) { return id; }), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }; } diff --git a/apps/extension/src/tools/__tests__/interaction.test.ts b/apps/extension/src/tools/__tests__/interaction.test.ts index b78877a..45ab74e 100644 --- a/apps/extension/src/tools/__tests__/interaction.test.ts +++ b/apps/extension/src/tools/__tests__/interaction.test.ts @@ -21,7 +21,7 @@ function fakeAgentWindow(ids: number[]) { return id; }), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }; } diff --git a/apps/extension/src/tools/__tests__/navigation.test.ts b/apps/extension/src/tools/__tests__/navigation.test.ts index f2a6bbe..921771e 100644 --- a/apps/extension/src/tools/__tests__/navigation.test.ts +++ b/apps/extension/src/tools/__tests__/navigation.test.ts @@ -21,7 +21,7 @@ function fakeAgentWindow(ids: number[]) { return id; }), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }; } diff --git a/apps/extension/src/tools/__tests__/network.test.ts b/apps/extension/src/tools/__tests__/network.test.ts index ed6f80b..8af1793 100644 --- a/apps/extension/src/tools/__tests__/network.test.ts +++ b/apps/extension/src/tools/__tests__/network.test.ts @@ -12,7 +12,7 @@ function fakeAgentWindow(ids: number[]) { return id; }), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }; } diff --git a/apps/extension/src/tools/__tests__/observation.test.ts b/apps/extension/src/tools/__tests__/observation.test.ts index 2e18a41..0763262 100644 --- a/apps/extension/src/tools/__tests__/observation.test.ts +++ b/apps/extension/src/tools/__tests__/observation.test.ts @@ -28,7 +28,7 @@ function fakeAgentWindow(ids: number[]) { return id; }), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }; } diff --git a/apps/extension/src/tools/__tests__/session.test.ts b/apps/extension/src/tools/__tests__/session.test.ts index 118c18e..57e53e3 100644 --- a/apps/extension/src/tools/__tests__/session.test.ts +++ b/apps/extension/src/tools/__tests__/session.test.ts @@ -1,8 +1,26 @@ import { describe, expect, it, vi } from "vitest"; import { SessionManager } from "@/session-manager/manager"; import { handleSessionStop } from "../session"; +import type { ChromeTabsApi } from "../shared"; import { type AgentOverlayResetApi, type ChromeWindowsApi, type TabMutationApi } from "../tabs"; +/** Build a read-only query api over a FakeState. */ +function makeQuery(state: FakeState): ChromeTabsApi { + return { + get: vi.fn(async (id) => { + const t = state.tabs.get(id); + if (!t) throw new Error(`tab ${id} not found`); + return t; + }), + query: vi.fn(async (q: chrome.tabs.QueryInfo) => { + const w = q.windowId; + return Array.from(state.tabs.values()).filter( + (t) => typeof w !== "number" || t.windowId === w, + ); + }), + }; +} + function fakeAgentWindow(ids: number[]) { let i = 0; const create = vi.fn(async () => { @@ -11,7 +29,7 @@ function fakeAgentWindow(ids: number[]) { return id; }); const remove = vi.fn(async () => {}); - const ensureActiveTab = vi.fn(async () => {}); + const ensureActiveTab = vi.fn(async () => 0); return { create, remove, ensureActiveTab }; } @@ -30,7 +48,9 @@ function makeApis( } { const tabs: TabMutationApi = { create: vi.fn(), - remove: vi.fn(async () => {}), + remove: vi.fn(async (id: number) => { + state.tabs.delete(id); + }), update: vi.fn(async (_id, _p) => undefined), get: vi.fn(async (id) => { const t = state.tabs.get(id); @@ -268,3 +288,216 @@ describe("handleSessionStop with auto-return", () => { expect(ctx.refStore.isEmpty()).toBe(true); }); }); + +describe("handleSessionStop window release (issue #57)", () => { + const agentWindowId = 100; + + it("releases the window when user-created tabs remain", async () => { + const aw = fakeAgentWindow([agentWindowId]); + // ensureActiveTab returns the home tab id (10). We override the fake to + // return a known id. + aw.ensureActiveTab = vi.fn(async () => 10); + const sm = new SessionManager({ agentWindow: aw }); + const ctx = await sm.start("aa11"); + ctx.agentCreatedTabs.add(11); + ctx.agentCreatedTabs.add(12); + // home tab id from ensureActiveTab override + ctx.homeTabId = 10; + // user tab 99 (created via Chrome UI, NOT in agentCreatedTabs) + const state: FakeState = { + tabs: new Map([ + [10, { id: 10, windowId: agentWindowId } as chrome.tabs.Tab], + [11, { id: 11, windowId: agentWindowId } as chrome.tabs.Tab], + [12, { id: 12, windowId: agentWindowId } as chrome.tabs.Tab], + [99, { id: 99, windowId: agentWindowId } as chrome.tabs.Tab], + ]), + windowsClosed: new Set(), + moves: [], + }; + const { tabs, windows } = makeApis(state); + const query = makeQuery(state); + + const res = await handleSessionStop( + sm, + { session_id: "aa11" }, + { tabManagement: { tabs, windows }, tabsQuery: query }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + expect(res.window_released).toBe(true); + // agent tabs + home removed, user tab 99 kept + expect((tabs.remove as ReturnType).mock.calls.map((c) => c[0]).sort()).toEqual([ + 10, 11, 12, + ]); + expect(state.tabs.has(99)).toBe(true); + // window released (dropOnly), not closed + expect(aw.remove).not.toHaveBeenCalled(); + expect(sm.has("aa11")).toBe(false); + }); + + it("closes the window (not release) when no user tabs remain", async () => { + const aw = fakeAgentWindow([agentWindowId]); + aw.ensureActiveTab = vi.fn(async () => 10); + const sm = new SessionManager({ agentWindow: aw }); + const ctx = await sm.start("aa11"); + ctx.agentCreatedTabs.add(11); + ctx.homeTabId = 10; + const state: FakeState = { + tabs: new Map([ + [10, { id: 10, windowId: agentWindowId } as chrome.tabs.Tab], + [11, { id: 11, windowId: agentWindowId } as chrome.tabs.Tab], + ]), + windowsClosed: new Set(), + moves: [], + }; + const { tabs, windows } = makeApis(state); + const query = makeQuery(state); + + const res = await handleSessionStop( + sm, + { session_id: "aa11" }, + { tabManagement: { tabs, windows }, tabsQuery: query }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + expect(res.window_released).toBeFalsy(); + expect(aw.remove).toHaveBeenCalledWith(agentWindowId); + expect(sm.has("aa11")).toBe(false); + }); + + it("is non-fatal when an agent tab is already gone", async () => { + const aw = fakeAgentWindow([agentWindowId]); + aw.ensureActiveTab = vi.fn(async () => 10); + const sm = new SessionManager({ agentWindow: aw }); + const ctx = await sm.start("aa11"); + ctx.agentCreatedTabs.add(11); // no longer in state → remove throws + ctx.homeTabId = 10; + const state: FakeState = { + tabs: new Map([[10, { id: 10, windowId: agentWindowId } as chrome.tabs.Tab]]), + windowsClosed: new Set(), + moves: [], + }; + const { tabs, windows } = makeApis(state); + (tabs.remove as ReturnType).mockImplementation(async (id: number) => { + if (id === 11) throw new Error("tab 11 already closed"); + state.tabs.delete(id); // home (10) removed normally + }); + const query = makeQuery(state); + + const res = await handleSessionStop( + sm, + { session_id: "aa11" }, + { tabManagement: { tabs, windows }, tabsQuery: query }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + // user tab 10? no — 10 is home, removed. window would be empty → close. + expect(res.window_released).toBeFalsy(); + expect(aw.remove).toHaveBeenCalled(); + }); + + it("degrades to close-window when tabsQuery is missing", async () => { + const aw = fakeAgentWindow([agentWindowId]); + aw.ensureActiveTab = vi.fn(async () => 10); + const sm = new SessionManager({ agentWindow: aw }); + const ctx = await sm.start("aa11"); + ctx.agentCreatedTabs.add(11); + ctx.homeTabId = 10; + const state: FakeState = { + tabs: new Map([ + [10, { id: 10, windowId: agentWindowId } as chrome.tabs.Tab], + [11, { id: 11, windowId: agentWindowId } as chrome.tabs.Tab], + ]), + windowsClosed: new Set(), + moves: [], + }; + const { tabs, windows } = makeApis(state); + + const res = await handleSessionStop( + sm, + { session_id: "aa11" }, + { tabManagement: { tabs, windows } }, // no tabsQuery + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + // No query available → conservatively close the window. + expect(res.window_released).toBeFalsy(); + expect(aw.remove).toHaveBeenCalledWith(agentWindowId); + }); + + it("degrades to close-window when tabsQuery.query throws", async () => { + const aw = fakeAgentWindow([agentWindowId]); + aw.ensureActiveTab = vi.fn(async () => 10); + const sm = new SessionManager({ agentWindow: aw }); + const ctx = await sm.start("aa11"); + ctx.agentCreatedTabs.add(11); + ctx.homeTabId = 10; + const state: FakeState = { + tabs: new Map([[11, { id: 11, windowId: agentWindowId } as chrome.tabs.Tab]]), + windowsClosed: new Set(), + moves: [], + }; + const { tabs, windows } = makeApis(state); + const query = makeQuery(state); + (query.query as ReturnType).mockImplementation(async () => { + throw new Error("query failed"); + }); + + const res = await handleSessionStop( + sm, + { session_id: "aa11" }, + { tabManagement: { tabs, windows }, tabsQuery: query }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + expect(res.window_released).toBeFalsy(); + expect(aw.remove).toHaveBeenCalled(); + }); + + it("tracks agentCreatedTabs across tab_create / tab_close", async () => { + const aw = fakeAgentWindow([agentWindowId]); + aw.ensureActiveTab = vi.fn(async () => 10); + const sm = new SessionManager({ agentWindow: aw }); + const ctx = await sm.start("aa11"); + // Simulate handleTabCreate adding, then handleTabClose removing. + ctx.agentCreatedTabs.add(11); + expect(ctx.agentCreatedTabs.has(11)).toBe(true); + ctx.agentCreatedTabs.delete(11); // as handleTabClose does on success + expect(ctx.agentCreatedTabs.has(11)).toBe(false); + // Unknown id delete is a safe no-op (user-closed tab). + expect(() => ctx.agentCreatedTabs.delete(999)).not.toThrow(); + }); + + it("closes the window (not release) when an agent tab fails to close", async () => { + // Regression: a leaked agent tab (still in agentCreatedTabs, but + // still present because Step 4 remove() threw) must NOT be mistaken + // for a user tab. Otherwise the window would be released (dropOnly) + // and the agent tab would leak (issue #57 regression). + const aw = fakeAgentWindow([agentWindowId]); + aw.ensureActiveTab = vi.fn(async () => 10); + const sm = new SessionManager({ agentWindow: aw }); + const ctx = await sm.start("aa11"); + ctx.agentCreatedTabs.add(11); // Step 4 remove() will throw for this tab + ctx.homeTabId = 10; + const state: FakeState = { + tabs: new Map([ + [10, { id: 10, windowId: agentWindowId } as chrome.tabs.Tab], + [11, { id: 11, windowId: agentWindowId } as chrome.tabs.Tab], + ]), + windowsClosed: new Set(), + moves: [], + }; + const { tabs, windows } = makeApis(state); + (tabs.remove as ReturnType).mockImplementation(async (id: number) => { + if (id === 11) throw new Error("tab 11 failed to close"); + state.tabs.delete(id); // home (10) removed normally; 11 lingers + }); + const query = makeQuery(state); + + const res = await handleSessionStop( + sm, + { session_id: "aa11" }, + { tabManagement: { tabs, windows }, tabsQuery: query }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + // The leaked agent tab must NOT keep the window open. + expect(res.window_released).toBeFalsy(); + expect(aw.remove).toHaveBeenCalledWith(agentWindowId); + expect(sm.has("aa11")).toBe(false); + }); +}); diff --git a/apps/extension/src/tools/__tests__/shared.test.ts b/apps/extension/src/tools/__tests__/shared.test.ts index ab44ad6..680c951 100644 --- a/apps/extension/src/tools/__tests__/shared.test.ts +++ b/apps/extension/src/tools/__tests__/shared.test.ts @@ -12,7 +12,7 @@ function fakeAgentWindow() { return { create: vi.fn(async () => 100), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }; } diff --git a/apps/extension/src/tools/__tests__/snapshot-ref.test.ts b/apps/extension/src/tools/__tests__/snapshot-ref.test.ts index cc9459d..8c9b0ae 100644 --- a/apps/extension/src/tools/__tests__/snapshot-ref.test.ts +++ b/apps/extension/src/tools/__tests__/snapshot-ref.test.ts @@ -11,7 +11,7 @@ function fakeAgentWindow(ids: number[]) { return id; }, remove: async () => {}, - ensureActiveTab: async () => {}, + ensureActiveTab: async () => 1, }; } diff --git a/apps/extension/src/tools/__tests__/tabs.test.ts b/apps/extension/src/tools/__tests__/tabs.test.ts index f8fbce2..aa47fad 100644 --- a/apps/extension/src/tools/__tests__/tabs.test.ts +++ b/apps/extension/src/tools/__tests__/tabs.test.ts @@ -23,7 +23,7 @@ function fakeAgentWindow(ids: number[]) { return id; }), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }; } diff --git a/apps/extension/src/tools/__tests__/waits.test.ts b/apps/extension/src/tools/__tests__/waits.test.ts index 3f82750..ce0f3aa 100644 --- a/apps/extension/src/tools/__tests__/waits.test.ts +++ b/apps/extension/src/tools/__tests__/waits.test.ts @@ -12,7 +12,7 @@ function fakeAgentWindow(ids: number[]) { return id; }), remove: vi.fn(async () => {}), - ensureActiveTab: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => 1), }; } diff --git a/apps/extension/src/tools/__tests__/window.test.ts b/apps/extension/src/tools/__tests__/window.test.ts index 3d86504..adab158 100755 --- a/apps/extension/src/tools/__tests__/window.test.ts +++ b/apps/extension/src/tools/__tests__/window.test.ts @@ -11,7 +11,7 @@ function fakeAgentWindow(ids: number[]) { return id; }); const remove = vi.fn(async () => {}); - const ensureActiveTab = vi.fn(async () => {}); + const ensureActiveTab = vi.fn(async () => 1); return { create, remove, ensureActiveTab }; } diff --git a/apps/extension/src/tools/dispatcher.ts b/apps/extension/src/tools/dispatcher.ts index e0a5ac4..8590478 100644 --- a/apps/extension/src/tools/dispatcher.ts +++ b/apps/extension/src/tools/dispatcher.ts @@ -66,6 +66,7 @@ import { import { chromeTabsApi, lookupSession, resolveTargetTab } from "./shared"; import { type BorrowConfirmationApprover, + chromeTabMutationApi, handleTabBorrow, handleTabClose, handleTabCreate, @@ -298,6 +299,11 @@ export class ToolDispatcher { await this.releaseHoverLatch((req.params as SessionStopParams).session_id); return handleSessionStop(this.sessions, req.params as SessionStopParams, { cdp: this.cdp, + // Must be wired in production: the agent-tab cleanup and the + // window-release decision (issue #57) read these deps directly + // and silently no-op when they are absent. + tabManagement: { tabs: chromeTabMutationApi }, + tabsQuery: chromeTabsApi, signal, }); } diff --git a/apps/extension/src/tools/session.ts b/apps/extension/src/tools/session.ts index 9bd1b27..165f874 100644 --- a/apps/extension/src/tools/session.ts +++ b/apps/extension/src/tools/session.ts @@ -2,6 +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 { isRpcError } from "./shared"; import { returnBorrowedTab, type TabManagementDeps } from "./tabs"; @@ -73,6 +74,12 @@ export interface SessionStopResult { /** Tab ids whose return path failed; those entries remain borrowed so * shutdown can be retried without closing the Agent Window. */ return_failures?: Array<{ tab_id: number; code: string; message: string }>; + /** + * True when the Agent Window was *released* to the user (instead of being + * closed) because it still contained user-created tabs after the agent's own + * tabs were closed. Single-session scope. + */ + window_released?: boolean; } export interface SessionStopDeps { @@ -87,6 +94,13 @@ export interface SessionStopDeps { * without a real browser. */ tabManagement?: TabManagementDeps; + /** + * Read-only Chrome tabs API used to *query* remaining tabs before deciding + * whether to release the window. Kept separate from `tabManagement.tabs` + * (a `TabMutationApi` that has no `query`) so the mutation interface stays + * pure. Tests can inject a fake `ChromeTabsApi` too. + */ + tabsQuery?: ChromeTabsApi; } /** @@ -152,8 +166,10 @@ export async function handleSessionStart( * cleanly (review parity with M6). * 3. Detach CDP sessions the extension still holds for this * session (no-op if M6/M7 didn't attach to any tab). - * 4. Close the Agent Window. SessionManager.stop() removes the - * Chrome window and forgets the context. + * 4. Close the agent-created tabs and the home tab (design §3.2). + * 5. If any tab remains — those are user-created tabs — release the + * window to the user (dropOnly) instead of closing it, so user + * tabs survive. Otherwise close the window as before. * * Failures in step 1 keep the Agent Window open: a failed borrowed tab * may still be there, so closing the window would risk losing user @@ -253,8 +269,76 @@ export async function handleSessionStop( return { code: "cancelled", message: "session_stop aborted before window close" }; } - // Step 4: close the Agent Window and drop the context. - await manager.stop(params.session_id); + // Step 4: close the agent-created tabs and the home tab. `tabsApi` is a + // TabMutationApi (remove only); `queryApi` is a separate read-only + // ChromeTabsApi. If neither is injected, we conservatively fall back to + // closing the window (see Step 5). + const tabsApi = deps.tabManagement?.tabs; + const queryApi = deps.tabsQuery; + + if (tabsApi) { + // 4a: close each agent-created tab that still exists. + const agentCreatedTabIds = Array.from(ctx.agentCreatedTabs); + for (const tabId of agentCreatedTabIds) { + try { + await tabsApi.remove(tabId); + ctx.agentCreatedTabs.delete(tabId); + } catch (err) { + // Tab may already be gone (closed by the user). Non-fatal. + console.warn(`[bsk session_stop] failed to close agent tab ${tabId}`, err); + } + } + + // 4b: close the home tab by id (not by URL — avoids deleting a user's + // `about:blank` tab). Non-fatal if it's already gone. + if (ctx.homeTabId != null) { + try { + await tabsApi.remove(ctx.homeTabId); + } catch (err) { + console.warn(`[bsk session_stop] failed to close home tab`, err); + } + } + } + + // Step 5: decide whether to release (keep) the window or close it. + let shouldRelease = false; + if (queryApi) { + try { + const liveWindowTabs = await queryApi.query({ windowId: ctx.agentWindowId }); + // Only genuine *user* tabs count toward keeping the window open. + // An agent tab that failed to close in Step 4 may still be present + // here; if we counted it as a reason to release (dropOnly), the + // window would be kept and that agent tab would leak (issue #57 + // regression). Exclude any id still tracked in agentCreatedTabs. + const userTabs = liveWindowTabs.filter((t) => { + if (t.id === undefined) return false; + if (t.id === ctx.homeTabId) return false; + return !ctx.agentCreatedTabs.has(t.id); + }); + const leakedAgentTabs = liveWindowTabs.filter( + (t) => t.id !== undefined && ctx.agentCreatedTabs.has(t.id), + ); + if (leakedAgentTabs.length > 0) { + console.warn( + `[bsk session_stop] ${leakedAgentTabs.length} agent tab(s) failed to close; forcing window close instead of release`, + leakedAgentTabs.map((t) => t.id), + ); + } + shouldRelease = userTabs.length > 0; + } catch { + // Query failed (e.g. window already gone) — conservatively close it. + shouldRelease = false; + } + } + + if (shouldRelease) { + // Keep the window + its user tabs; only drop the session binding. + await manager.stop(params.session_id, { dropOnly: true }); + result.window_released = true; + } else { + // Window is empty (or we couldn't verify state) — close it. + await manager.stop(params.session_id); + } return result; } diff --git a/apps/extension/src/tools/tabs.ts b/apps/extension/src/tools/tabs.ts index 04e46b8..102a0ef 100644 --- a/apps/extension/src/tools/tabs.ts +++ b/apps/extension/src/tools/tabs.ts @@ -426,9 +426,18 @@ export async function handleTabCreate( const paramErr = validateTabCreateParams(params); if (paramErr) return paramErr; + // Flag the pending agent tab *before* create so the chrome.tabs.onCreated + // listener (background.ts) can tell this tab apart from a user-opened tab. + manager.markAgentTabPending(ctx.agentWindowId); + const tab = await createTabAndCleanup(deps, buildCreateProps(ctx, params)); if (isRpcError(tab)) return tab; + // Track agent-created tabs so session_stop can clean them up (design §3.1). + // The onCreated listener also adds this id; this is an idempotent fallback + // in case the listener has not fired yet. + ctx.agentCreatedTabs.add(tab.id); + return { tab_id: tab.id, window_id: ctx.agentWindowId, @@ -526,6 +535,9 @@ export async function handleTabClose( } try { await getTabsApi(deps).remove(params.tab_id); + // Keep the tracking set accurate so session_stop won't try to close a + // tab that's already gone (design §3.1). + ctx.agentCreatedTabs.delete(params.tab_id); } catch (err) { return { code: "protocol_error",