From 89d4b676b7362af424d26f9c04fb8004eecaae12 Mon Sep 17 00:00:00 2001 From: glatinone <93207632+glatinone@users.noreply.github.com> Date: Thu, 24 Sep 2026 22:15:27 +0800 Subject: [PATCH 1/2] Add bsk tab group: native tab-group management scoped to the Agent Window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `bsk tab group create|update|list|ungroup`, backed by chrome.tabGroups, following the same Agent Window sandbox rule as `tab close` / `tab select`: every tab (and, for `update`/`ungroup`, the group itself) must already live in the requesting session's Agent Window. - bsk-protocol: TabGroupColor + TabGroupCreate/Update/List/Ungroup params/results, four new Method variants classified for the pending-interrupt gate (create/update/ungroup mutate, list is a passive read). - CLI: `bsk tab group {create,update,list,ungroup}` subcommands with --json and human table output. - daemon: forwards the four new tool.* methods like the existing tab.* ones (no new daemon-side logic needed). - extension: handlers using chrome.tabs.group/ungroup and chrome.tabGroups.{get,query,update,move}, gated through the existing session/sandbox checks (authoriseAgentTab/authoriseAgentGroup). Real-browser testing (not just mocks) surfaced that a freshly created group can land outside the target window on some Chromium builds/hosts (most reliably reproduced with `session start --no-focus`, though the underlying trigger turned out to be a stale unpacked-extension reload during iteration, not focus itself). handleTabGroupCreate now verifies placement after creation and falls back from a group-level move to per-tab moves; if neither lands the tabs in the Agent Window, it ungroups them and returns a clear error instead of a hollow success with an empty tab_ids. Covered by new unit tests, including both fallback paths. Also documents (skill/references/tabs-and-profiles.md) that --title should come from what the grouped tabs are actually about, not a placeholder — read tab titles/URLs first, then name the group. Testing: - cargo test -p bsk-protocol / -p bsk --lib: all pass (162 + 361 tests) - pnpm vitest run (apps/extension): all pass (150 files / 2297 tests, 76 in tabs.test.ts covering the new handlers) - cargo clippy / cargo fmt --check: clean (pre-existing warnings only) - Manual end-to-end dogfooding against a real, isolated Edge profile (bsk CLI -> daemon -> unpacked extension -> real chrome.tabGroups): create/list/update/ungroup all verified against real tabs, in both focused and unfocused Agent Windows, plus the sandbox rejection path for a tab outside the Agent Window. Known scope limit: DSH plugin native tools (packages/dsh-plugin-browserskill) are not updated — this PR covers the bsk CLI only. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 11 + .../src/tools/__tests__/tabs.test.ts | 405 +++++++++++++++ apps/extension/src/tools/dispatcher.ts | 19 + apps/extension/src/tools/tabs.ts | 471 ++++++++++++++++++ apps/extension/wxt.config.ts | 1 + crates/bsk-cli/skill/SKILL.md | 2 +- .../skill/references/tabs-and-profiles.md | 25 + crates/bsk-cli/src/cli/tab.rs | 304 ++++++++++- crates/bsk-cli/src/daemon/ipc.rs | 4 + crates/bsk-protocol/src/method.rs | 16 + crates/bsk-protocol/src/tools/tabs.rs | 175 +++++++ 11 files changed, 1429 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2b542b3..69c66210 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Starting from 0.2.0, CLI / Extension / DSH Plugin share the same version number. ## [Unreleased] +<<<<<<< HEAD ### Fixed - Protocol: preserve explicit `null` results when deserializing `ResponseFrame`, @@ -60,6 +61,16 @@ Starting from 0.2.0, CLI / Extension / DSH Plugin share the same version number. `"staged"` status is gone. - README documents `BSK_AUTO_UPDATE=off`. +### Added + +- `bsk tab group create|update|list|ungroup`: native Chrome/Edge tab-group + management (`chrome.tabGroups`) scoped to the session's Agent Window, with + the same sandbox rule as `tab close` / `tab select`. Guards against browsers + that place a freshly created group outside the target window (most + reproducible with `session start --no-focus`) with a relocation fallback, + and reports an honest error instead of a silent partial group when no + relocation attempt lands the tabs in the Agent Window. + ## [0.3.1] - 2026-09-23 ### Added diff --git a/apps/extension/src/tools/__tests__/tabs.test.ts b/apps/extension/src/tools/__tests__/tabs.test.ts index 976d63ba..192180a0 100644 --- a/apps/extension/src/tools/__tests__/tabs.test.ts +++ b/apps/extension/src/tools/__tests__/tabs.test.ts @@ -5,15 +5,21 @@ import { handleHover } from "../interaction"; import { resolveTargetTab } from "../shared"; import { type AgentOverlayResetApi, + type ChromeTabGroupsApi, type ChromeWindowsApi, handleTabBorrow, handleTabClose, handleTabCreate, + handleTabGroupCreate, + handleTabGroupList, + handleTabGroupUngroup, + handleTabGroupUpdate, handleTabList, handleTabReturn, handleTabSelect, NEW_TAB_DEFAULT_URL, returnBorrowedTab, + type TabGroupMutationApi, type TabMutationApi, } from "../tabs"; @@ -1460,3 +1466,402 @@ it("keeps borrowed tabs as the default target and preserves explicit targeting", expect(ctx.borrowedTabs.has(7)).toBe(false); expect(cdp.releaseSessionTab).toHaveBeenCalledWith("aa11", 7); }); + +// --------------------------------------------------------------------------- +// tool.tab_group_create / _update / _list / _ungroup +// --------------------------------------------------------------------------- + +type FakeChromeTabGroup = chrome.tabGroups.TabGroup; + +interface FakeGroupState { + groups: Map; + tabGroupOf: Map; + nextGroupId: number; +} + +function makeTabGroupApis( + tabState: FakeTabState, + groupState: FakeGroupState, + /** + * Simulates the real-world Chromium quirk (observed manually against + * Edge 153) where a freshly created group lands in the tabs' *previous* + * window rather than the requested `createProperties.windowId`. Exists + * only to exercise `handleTabGroupCreate`'s self-heal path. + */ + options: { ignoreCreateWindowId?: boolean } = {}, +): { + tabGroups: TabGroupMutationApi; + tabGroupsApi: ChromeTabGroupsApi; + spies: { + group: ReturnType; + ungroup: ReturnType; + queryTabs: ReturnType; + get: ReturnType; + query: ReturnType; + update: ReturnType; + move: ReturnType; + }; +} { + const group = vi.fn(async (opts: chrome.tabs.GroupOptions) => { + const tabIds = Array.isArray(opts.tabIds) ? opts.tabIds : [opts.tabIds]; + let groupId = opts.groupId; + if (groupId === undefined) { + groupId = groupState.nextGroupId++; + // The "ignore" path always lands in a fixed window unrelated to the + // tabs' own windowId, mirroring the real quirk observed against Edge + // 153: the freshly created group escaped to the browser's regular + // window regardless of what `createProperties.windowId` requested. + const windowId = options.ignoreCreateWindowId + ? 999 + : (opts.createProperties?.windowId ?? tabState.tabs.get(tabIds[0])?.windowId ?? 1); + groupState.groups.set(groupId, { + id: groupId, + windowId, + title: "", + color: "grey", + collapsed: false, + } as FakeChromeTabGroup); + // A tab's own windowId always matches wherever its group physically + // lives (matches the real browser, and is what let this test suite + // catch a mismatch that a window-agnostic fake would have hidden). + for (const id of tabIds) { + const t = tabState.tabs.get(id); + if (t) (t as { windowId?: number }).windowId = windowId; + } + } + for (const id of tabIds) groupState.tabGroupOf.set(id, groupId); + return groupId; + }); + const ungroup = vi.fn(async (tabIds: number | number[]) => { + for (const id of Array.isArray(tabIds) ? tabIds : [tabIds]) groupState.tabGroupOf.delete(id); + }); + const queryTabs = vi.fn(async (q: chrome.tabs.QueryInfo) => + [...tabState.tabs.values()].filter( + (t) => + typeof t.id === "number" && + (q.windowId === undefined || t.windowId === q.windowId) && + (q.groupId === undefined || groupState.tabGroupOf.get(t.id) === q.groupId), + ), + ); + const get = vi.fn(async (groupId: number) => { + const g = groupState.groups.get(groupId); + if (!g) throw new Error(`group ${groupId} not found`); + return g; + }); + const query = vi.fn(async (q: chrome.tabGroups.QueryInfo) => + [...groupState.groups.values()].filter((g) => q.windowId === undefined || g.windowId === q.windowId), + ); + const update = vi.fn(async (groupId: number, props: chrome.tabGroups.UpdateProperties) => { + const g = groupState.groups.get(groupId); + if (!g) return undefined; + const updated = { ...g, ...props }; + groupState.groups.set(groupId, updated); + return updated; + }); + const move = vi.fn(async (groupId: number, props: chrome.tabGroups.MoveProperties) => { + const g = groupState.groups.get(groupId); + if (!g) throw new Error(`move: group ${groupId} not found`); + // Mirrors the real quirk this fallback exists for: `tabGroups.move` + // resolves without error but does not actually relocate anything + // against an unfocused window. + if (options.ignoreCreateWindowId) return g; + const updated = { ...g, ...(typeof props.windowId === "number" ? { windowId: props.windowId } : {}) }; + groupState.groups.set(groupId, updated); + return updated; + }); + return { + tabGroups: { group, ungroup, queryTabs }, + tabGroupsApi: { get, query, update, move }, + spies: { group, ungroup, queryTabs, get, query, update, move }, + }; +} + +function emptyGroupState(): FakeGroupState { + return { groups: new Map(), tabGroupOf: new Map(), nextGroupId: 1 }; +} + +describe("handleTabGroupCreate", () => { + it("groups Agent Window tabs into a new group and returns its info", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([ + [5, { id: 5, windowId: 100, index: 0 } as chrome.tabs.Tab], + [6, { id: 6, windowId: 100, index: 1 } as chrome.tabs.Tab], + ]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: tabs } = makeTabMutationApi(tabState); + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, emptyGroupState()); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [5, 6], title: "Research", color: "blue" }, + { tabs, tabGroups, tabGroupsApi }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + expect(res.tab_ids.sort()).toEqual([5, 6]); + expect(res.title).toBe("Research"); + expect(res.color).toBe("blue"); + expect(spies.group).toHaveBeenCalledWith({ + tabIds: [5, 6], + createProperties: { windowId: 100 }, + }); + expect(spies.update).toHaveBeenCalledWith(res.group_id, { title: "Research", color: "blue" }); + }); + + it("refuses a tab outside the Agent Window with permission_denied", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([[9, { id: 9, windowId: 200 } as chrome.tabs.Tab]]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: tabs } = makeTabMutationApi(tabState); + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, emptyGroupState()); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [9] }, + { tabs, tabGroups, tabGroupsApi }, + ); + expect(res).toMatchObject({ code: "permission_denied", data: { reason: "agent_window_scope" } }); + expect(spies.group).not.toHaveBeenCalled(); + }); + + it("adds tabs to an existing Agent Window group via group_id", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([ + [5, { id: 5, windowId: 100, index: 0 } as chrome.tabs.Tab], + [6, { id: 6, windowId: 100, index: 1 } as chrome.tabs.Tab], + ]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: tabs } = makeTabMutationApi(tabState); + const groupState = emptyGroupState(); + groupState.groups.set(1, { id: 1, windowId: 100, title: "Existing", color: "grey", collapsed: false }); + groupState.tabGroupOf.set(5, 1); + const { tabGroups, tabGroupsApi } = makeTabGroupApis(tabState, groupState); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [6], group_id: 1 }, + { tabs, tabGroups, tabGroupsApi }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + expect(res.group_id).toBe(1); + expect(res.tab_ids.sort()).toEqual([5, 6]); + }); + + it("rejects group_id belonging to another window with permission_denied", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([[5, { id: 5, windowId: 100 } as chrome.tabs.Tab]]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: tabs } = makeTabMutationApi(tabState); + const groupState = emptyGroupState(); + groupState.groups.set(1, { id: 1, windowId: 999, title: "", color: "grey", collapsed: false }); + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, groupState); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [5], group_id: 1 }, + { tabs, tabGroups, tabGroupsApi }, + ); + expect(res).toMatchObject({ code: "permission_denied", data: { reason: "agent_window_scope" } }); + expect(spies.group).not.toHaveBeenCalled(); + }); + + it("falls back to per-tab moves when the browser ignores both createProperties.windowId and tabGroups.move", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([ + [5, { id: 5, windowId: 100, index: 0 } as chrome.tabs.Tab], + [6, { id: 6, windowId: 100, index: 1 } as chrome.tabs.Tab], + ]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: tabs, spies: tabSpies } = makeTabMutationApi(tabState); + const { tabGroups, tabGroupsApi, spies: groupSpies } = makeTabGroupApis(tabState, emptyGroupState(), { + ignoreCreateWindowId: true, + }); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [5, 6] }, + { tabs, tabGroups, tabGroupsApi }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + // Group-level relocation was attempted first... + expect(groupSpies.move).toHaveBeenCalledWith(res.group_id, { windowId: 100, index: -1 }); + // ...but since it silently no-opped, each tab was moved back individually. + expect(tabSpies.move).toHaveBeenCalledWith(5, { windowId: 100, index: -1 }); + expect(tabSpies.move).toHaveBeenCalledWith(6, { windowId: 100, index: -1 }); + expect(tabState.tabs.get(5)?.windowId).toBe(100); + expect(tabState.tabs.get(6)?.windowId).toBe(100); + }); + + it("reports a clear error and ungroups the tabs when no relocation attempt lands them in the Agent Window", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([[5, { id: 5, windowId: 100, index: 0 } as chrome.tabs.Tab]]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: baseTabs } = makeTabMutationApi(tabState); + // Even the per-tab fallback fails to relocate here — worst case + // observed on some hosts for tabs that are members of a tab group. + const tabs: TabMutationApi = { ...baseTabs, move: vi.fn(async (id, p) => baseTabs.get(id)) }; + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, emptyGroupState(), { + ignoreCreateWindowId: true, + }); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [5] }, + { tabs, tabGroups, tabGroupsApi }, + ); + expect(res).toMatchObject({ code: "cdp_failed", data: { reason: "group_window_mismatch" } }); + expect(spies.ungroup).toHaveBeenCalledWith([5]); + }); + + it("rejects an empty tab_ids array", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { tabs: new Map(), nextTabId: 50, windowsClosed: new Set() }; + const { api: tabs } = makeTabMutationApi(tabState); + const { tabGroups, tabGroupsApi } = makeTabGroupApis(tabState, emptyGroupState()); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [] }, + { tabs, tabGroups, tabGroupsApi }, + ); + expect(res).toMatchObject({ code: "invalid_params" }); + }); +}); + +describe("handleTabGroupUpdate", () => { + it("renames, recolors, and collapses an Agent Window group", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { tabs: new Map(), nextTabId: 50, windowsClosed: new Set() }; + const groupState = emptyGroupState(); + groupState.groups.set(1, { id: 1, windowId: 100, title: "Old", color: "grey", collapsed: false }); + const { tabGroupsApi } = makeTabGroupApis(tabState, groupState); + const res = await handleTabGroupUpdate( + sm, + { session_id: "aa11", group_id: 1, title: "New", color: "red", collapsed: true }, + { tabGroupsApi }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + expect(res).toMatchObject({ group_id: 1, title: "New", color: "red", collapsed: true }); + }); + + it("refuses a group outside the Agent Window with permission_denied", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { tabs: new Map(), nextTabId: 50, windowsClosed: new Set() }; + const groupState = emptyGroupState(); + groupState.groups.set(1, { id: 1, windowId: 999, title: "", color: "grey", collapsed: false }); + const { tabGroupsApi, spies } = makeTabGroupApis(tabState, groupState); + const res = await handleTabGroupUpdate( + sm, + { session_id: "aa11", group_id: 1, title: "New" }, + { tabGroupsApi }, + ); + expect(res).toMatchObject({ code: "permission_denied", data: { reason: "agent_window_scope" } }); + expect(spies.update).not.toHaveBeenCalled(); + }); + + it("rejects an invalid color", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const res = await handleTabGroupUpdate( + sm, + { + session_id: "aa11", + group_id: 1, + color: "chartreuse" as unknown as Parameters[1]["color"], + }, + {}, + ); + expect(res).toMatchObject({ code: "invalid_params" }); + }); +}); + +describe("handleTabGroupList", () => { + it("lists only groups in the session's Agent Window", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100, 200]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([ + [5, { id: 5, windowId: 100 } as chrome.tabs.Tab], + [6, { id: 6, windowId: 200 } as chrome.tabs.Tab], + ]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const groupState = emptyGroupState(); + groupState.groups.set(1, { id: 1, windowId: 100, title: "Mine", color: "blue", collapsed: false }); + groupState.groups.set(2, { id: 2, windowId: 200, title: "Other window", color: "red", collapsed: false }); + groupState.tabGroupOf.set(5, 1); + groupState.tabGroupOf.set(6, 2); + const { tabGroups, tabGroupsApi } = makeTabGroupApis(tabState, groupState); + const res = await handleTabGroupList(sm, { session_id: "aa11" }, { tabGroups, tabGroupsApi }); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + expect(res.groups).toHaveLength(1); + expect(res.groups[0]).toMatchObject({ group_id: 1, title: "Mine", tab_ids: [5] }); + }); +}); + +describe("handleTabGroupUngroup", () => { + it("removes every tab from the group and leaves them open", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([ + [5, { id: 5, windowId: 100 } as chrome.tabs.Tab], + [6, { id: 6, windowId: 100 } as chrome.tabs.Tab], + ]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const groupState = emptyGroupState(); + groupState.groups.set(1, { id: 1, windowId: 100, title: "", color: "grey", collapsed: false }); + groupState.tabGroupOf.set(5, 1); + groupState.tabGroupOf.set(6, 1); + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, groupState); + const res = await handleTabGroupUngroup( + sm, + { session_id: "aa11", group_id: 1 }, + { tabGroups, tabGroupsApi }, + ); + if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); + expect(res.tab_ids.sort()).toEqual([5, 6]); + expect(spies.ungroup).toHaveBeenCalledWith([5, 6]); + expect(groupState.tabGroupOf.size).toBe(0); + expect(tabState.tabs.has(5)).toBe(true); + expect(tabState.tabs.has(6)).toBe(true); + }); + + it("refuses a group outside the Agent Window with permission_denied", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { tabs: new Map(), nextTabId: 50, windowsClosed: new Set() }; + const groupState = emptyGroupState(); + groupState.groups.set(1, { id: 1, windowId: 999, title: "", color: "grey", collapsed: false }); + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, groupState); + const res = await handleTabGroupUngroup( + sm, + { session_id: "aa11", group_id: 1 }, + { tabGroups, tabGroupsApi }, + ); + expect(res).toMatchObject({ code: "permission_denied", data: { reason: "agent_window_scope" } }); + expect(spies.ungroup).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/extension/src/tools/dispatcher.ts b/apps/extension/src/tools/dispatcher.ts index 0a658904..6ea568a4 100644 --- a/apps/extension/src/tools/dispatcher.ts +++ b/apps/extension/src/tools/dispatcher.ts @@ -100,12 +100,20 @@ import { handleTabBorrow, handleTabClose, handleTabCreate, + handleTabGroupCreate, + handleTabGroupList, + handleTabGroupUngroup, + handleTabGroupUpdate, handleTabList, handleTabReturn, handleTabSelect, type TabBorrowParams, type TabCloseParams, type TabCreateParams, + type TabGroupCreateParams, + type TabGroupListParams, + type TabGroupUngroupParams, + type TabGroupUpdateParams, type TabListParams, type TabReturnParams, type TabSelectParams, @@ -479,6 +487,14 @@ export class ToolDispatcher { ); case "tool.tab_select": return handleTabSelect(this.sessions, req.params as TabSelectParams, { signal }); + case "tool.tab_group_create": + return handleTabGroupCreate(this.sessions, req.params as TabGroupCreateParams, { signal }); + case "tool.tab_group_update": + return handleTabGroupUpdate(this.sessions, req.params as TabGroupUpdateParams, { signal }); + case "tool.tab_group_list": + return handleTabGroupList(this.sessions, req.params as TabGroupListParams, { signal }); + case "tool.tab_group_ungroup": + return handleTabGroupUngroup(this.sessions, req.params as TabGroupUngroupParams, { signal }); case "tool.tab_borrow": { const result = await handleTabBorrow(this.sessions, req.params as TabBorrowParams, { signal, @@ -1005,6 +1021,9 @@ function sessionIdForBrowserControlMethod(req: RequestFrame): string | null { case "tool.tab_select": case "tool.tab_borrow": case "tool.tab_return": + case "tool.tab_group_create": + case "tool.tab_group_update": + case "tool.tab_group_ungroup": case "tool.window_resize": case "tool.emulate": case "tool.navigate": diff --git a/apps/extension/src/tools/tabs.ts b/apps/extension/src/tools/tabs.ts index 5d30fa2b..201a0bc0 100644 --- a/apps/extension/src/tools/tabs.ts +++ b/apps/extension/src/tools/tabs.ts @@ -112,6 +112,88 @@ export interface TabReturnResult { const TAB_SCOPES = new Set(["user", "agent", "all"]); +// --- Tab group payload mirrors (bsk-protocol/src/tools/tabs.rs) --- + +/** Mirrors `chrome.tabGroups.ColorEnum`; the wire strings match verbatim. */ +export type TabGroupColor = + | "grey" + | "blue" + | "red" + | "yellow" + | "green" + | "pink" + | "purple" + | "cyan" + | "orange"; + +const TAB_GROUP_COLORS = new Set([ + "grey", + "blue", + "red", + "yellow", + "green", + "pink", + "purple", + "cyan", + "orange", +]); + +export interface TabGroupInfo { + group_id: number; + title?: string; + color: TabGroupColor; + collapsed: boolean; + tab_ids: number[]; +} + +export interface TabGroupCreateParams { + session_id: string; + tab_ids: number[]; + /** Add to this existing Agent Window group instead of creating a new one. */ + group_id?: number; + title?: string; + color?: TabGroupColor; +} + +export interface TabGroupCreateResult { + group_id: number; + title?: string; + color: TabGroupColor; + tab_ids: number[]; +} + +export interface TabGroupUpdateParams { + session_id: string; + group_id: number; + title?: string; + color?: TabGroupColor; + collapsed?: boolean; +} + +export interface TabGroupUpdateResult { + group_id: number; + title?: string; + color: TabGroupColor; + collapsed: boolean; +} + +export interface TabGroupListParams { + session_id: string; +} + +export interface TabGroupListResult { + groups: TabGroupInfo[]; +} + +export interface TabGroupUngroupParams { + session_id: string; + group_id: number; +} + +export interface TabGroupUngroupResult { + tab_ids: number[]; +} + /** Default URL used when `tool.tab_create` is called with no `url`. */ export const NEW_TAB_DEFAULT_URL = "chrome://newtab/"; @@ -154,6 +236,41 @@ export const chromeTabMutationApi: TabMutationApi = { move: (id, p) => chrome.tabs.move(id, p), }; +/** + * Subset of `chrome.tabs` used by the tab-group handlers: grouping, + * ungrouping, and querying tabs by group. Kept separate from + * `TabMutationApi` so its existing callers/tests are untouched. + */ +export interface TabGroupMutationApi { + group(options: chrome.tabs.GroupOptions): Promise; + ungroup(tabIds: number | number[]): Promise; + queryTabs(query: chrome.tabs.QueryInfo): Promise; +} + +export const chromeTabGroupMutationApi: TabGroupMutationApi = { + group: (o) => chrome.tabs.group(o), + ungroup: (ids) => chrome.tabs.ungroup(ids), + queryTabs: (q) => chrome.tabs.query(q), +}; + +/** Subset of `chrome.tabGroups` we depend on. */ +export interface ChromeTabGroupsApi { + get(groupId: number): Promise; + query(query: chrome.tabGroups.QueryInfo): Promise; + update( + groupId: number, + props: chrome.tabGroups.UpdateProperties, + ): Promise; + move(groupId: number, props: chrome.tabGroups.MoveProperties): Promise; +} + +export const chromeTabGroupsApi: ChromeTabGroupsApi = { + get: (id) => chrome.tabGroups.get(id), + query: (q) => chrome.tabGroups.query(q), + update: (id, p) => chrome.tabGroups.update(id, p), + move: (id, p) => chrome.tabGroups.move(id, p), +}; + /** * Subset of `chrome.windows` we use for tab_return's fallback path * (original window has been closed; pick another normal window or @@ -306,12 +423,24 @@ export interface TabManagementDeps { * default only preserves legacy behaviour for direct/test callers. */ isAgentWindowId?: (windowId: number) => boolean; + /** Tab-group mutation surface (`tab_group_create` / `_update` / `_ungroup`). */ + tabGroups?: TabGroupMutationApi; + /** `chrome.tabGroups` read/update surface. */ + tabGroupsApi?: ChromeTabGroupsApi; } function getTabsApi(deps: TabManagementDeps): TabMutationApi { return deps.tabs ?? chromeTabMutationApi; } +function getTabGroupMutationApi(deps: TabManagementDeps): TabGroupMutationApi { + return deps.tabGroups ?? chromeTabGroupMutationApi; +} + +function getTabGroupsApi(deps: TabManagementDeps): ChromeTabGroupsApi { + return deps.tabGroupsApi ?? chromeTabGroupsApi; +} + function getWindowsApi(deps: TabManagementDeps): ChromeWindowsApi { return deps.windows ?? chromeWindowsApi; } @@ -1301,3 +1430,345 @@ export async function handleTabReturn( if (outcome.fallback) result.fallback = true; return result; } + +// --------------------------------------------------------------------------- +// tool.tab_group_create / _update / _list / _ungroup +// --------------------------------------------------------------------------- + +/** + * Verify that `groupId` is a tab group living in the session's Agent + * Window. Mirrors `authoriseAgentTab`'s sandbox rule: groups outside + * the Agent Window (or belonging to another session's Agent Window) + * are not addressable. + */ +async function authoriseAgentGroup( + ctx: SessionContext, + groupId: number, + api: ChromeTabGroupsApi, + toolName: string, +): Promise { + let group: chrome.tabGroups.TabGroup; + try { + group = await api.get(groupId); + } catch (err) { + return { + code: "not_found", + message: err instanceof Error ? err.message : `group ${groupId} not found`, + }; + } + if (group.windowId !== ctx.agentWindowId) { + return rpcError( + "permission_denied", + "agent_window_scope", + `${toolName}: group ${groupId} is not in Agent Window ${ctx.agentWindowId}`, + ); + } + return group; +} + +/** Re-read a group's current title/color/collapsed state and member tabs. */ +async function buildGroupInfo( + groupsApi: ChromeTabGroupsApi, + queryTabs: TabGroupMutationApi["queryTabs"], + windowId: number, + groupId: number, +): Promise { + let group: chrome.tabGroups.TabGroup; + try { + group = await groupsApi.get(groupId); + } catch (err) { + return { + code: "protocol_error", + message: err instanceof Error ? err.message : String(err), + }; + } + let tabs: chrome.tabs.Tab[]; + try { + tabs = await queryTabs({ windowId, groupId }); + } catch (err) { + return { + code: "protocol_error", + message: err instanceof Error ? err.message : String(err), + }; + } + const tabIds = tabs + .filter((t): t is CreatedChromeTab => typeof t.id === "number") + .sort((a, b) => (a.index ?? 0) - (b.index ?? 0)) + .map((t) => t.id); + return { + group_id: groupId, + title: group.title || undefined, + color: group.color as TabGroupColor, + collapsed: group.collapsed, + tab_ids: tabIds, + }; +} + +function validateTabGroupCreateParams(params: TabGroupCreateParams): RpcError | null { + if (!Array.isArray(params.tab_ids) || params.tab_ids.length === 0) { + return { + code: "invalid_params", + message: "tab_group_create requires a non-empty tab_ids array", + }; + } + for (const tabId of params.tab_ids) { + const err = validatePositiveInt("tab_ids[]", tabId); + if (err) return err; + } + if (params.group_id !== undefined) { + const err = validatePositiveInt("group_id", params.group_id); + if (err) return err; + } + if (params.title !== undefined && typeof params.title !== "string") { + return { code: "invalid_params", message: "tab_group_create title must be a string" }; + } + if (params.color !== undefined && !TAB_GROUP_COLORS.has(params.color)) { + return { code: "invalid_params", message: "tab_group_create color must be a valid tab group color" }; + } + return null; +} + +/** + * Group one or more Agent Window tabs into a new tab group, or add them + * to an existing Agent Window group when `group_id` is given. Every tab + * (and, when given, the target group) must already live in the + * requesting session's Agent Window — same sandbox rule as `tab_close` + * / `tab_select` (design §6): agents never reach into a user window's + * tab strip directly. + */ +export async function handleTabGroupCreate( + manager: SessionManager, + params: TabGroupCreateParams, + deps: TabManagementDeps = {}, +): Promise { + const ctxOrErr = lookupSession(manager, params, "tab_group_create"); + if (isRpcError(ctxOrErr)) return ctxOrErr; + const ctx = ctxOrErr; + const paramErr = validateTabGroupCreateParams(params); + if (paramErr) return paramErr; + if (aborted(deps.signal, "tab_group_create")) { + return { code: "cancelled", message: "tab_group_create aborted" }; + } + + const tabsApi = getTabsApi(deps); + for (const tabId of params.tab_ids) { + const tabOrErr = await authoriseAgentTab(manager, ctx, tabId, tabsApi, "tab_group_create"); + if (isRpcError(tabOrErr)) return tabOrErr; + } + const groupsApi = getTabGroupsApi(deps); + if (params.group_id !== undefined) { + const groupOrErr = await authoriseAgentGroup(ctx, params.group_id, groupsApi, "tab_group_create"); + if (isRpcError(groupOrErr)) return groupOrErr; + } + if (aborted(deps.signal, "tab_group_create")) { + return { code: "cancelled", message: "tab_group_create aborted" }; + } + + const groupMutation = getTabGroupMutationApi(deps); + let groupId: number; + try { + groupId = await groupMutation.group({ + tabIds: params.tab_ids, + ...(params.group_id !== undefined + ? { groupId: params.group_id } + : // Chrome does not reliably keep a *new* group in the tabs' current + // window — observed moving grouped tabs into a regular user + // window instead. Pin it explicitly so a fresh group never + // escapes the Agent Window (design §6 sandbox rule). + { createProperties: { windowId: ctx.agentWindowId } }), + }); + } catch (err) { + return { code: "protocol_error", message: err instanceof Error ? err.message : String(err) }; + } + + // Belt-and-suspenders: some Chromium builds have been observed placing a + // freshly created group (and the tabs riding along with it) in a + // different window than `createProperties.windowId` requested — most + // reliably reproduced when the Agent Window is unfocused (the default + // for `session start --no-focus`). Try relocating the group, then each + // member tab individually, before giving up — never silently hand tabs + // to the user's regular browsing session (design §6 sandbox rule). + if (params.group_id === undefined) { + const misplaced = async () => (await groupsApi.get(groupId)).windowId !== ctx.agentWindowId; + try { + if (await misplaced()) { + await groupsApi.move(groupId, { windowId: ctx.agentWindowId, index: -1 }).catch(() => {}); + if (await misplaced()) { + for (const tabId of params.tab_ids) { + await tabsApi.move(tabId, { windowId: ctx.agentWindowId, index: -1 }); + } + } + } + } catch (err) { + return { code: "protocol_error", message: err instanceof Error ? err.message : String(err) }; + } + } + + if (params.title !== undefined || params.color !== undefined) { + try { + await groupsApi.update(groupId, { + ...(params.title !== undefined ? { title: params.title } : {}), + ...(params.color !== undefined ? { color: params.color } : {}), + }); + } catch (err) { + return { code: "protocol_error", message: err instanceof Error ? err.message : String(err) }; + } + } + + const infoOrErr = await buildGroupInfo(groupsApi, groupMutation.queryTabs, ctx.agentWindowId, groupId); + if (isRpcError(infoOrErr)) return infoOrErr; + + // Every relocation attempt above ran and the group's own metadata may + // still claim the Agent Window, but the only signal worth trusting is + // which requested tabs `buildGroupInfo` can actually see there. If none + // made it, don't report a hollow success: undo the stray group so it + // doesn't linger in the user's regular browsing session, and say so. + const requested = new Set(params.tab_ids); + const landed = infoOrErr.tab_ids.filter((id) => requested.has(id)); + if (landed.length === 0 && params.group_id === undefined) { + await groupMutation.ungroup(params.tab_ids).catch(() => {}); + return rpcError( + "cdp_failed", + "group_window_mismatch", + "tab_group_create: the browser did not keep the new group in the Agent Window " + + `${ctx.agentWindowId} after relocation was attempted; the tabs remain open, ungrouped`, + ); + } + return { + group_id: infoOrErr.group_id, + title: infoOrErr.title, + color: infoOrErr.color, + tab_ids: infoOrErr.tab_ids, + }; +} + +/** Rename, recolor, or (un)collapse an existing Agent Window tab group. */ +export async function handleTabGroupUpdate( + manager: SessionManager, + params: TabGroupUpdateParams, + deps: TabManagementDeps = {}, +): Promise { + const ctxOrErr = lookupSession(manager, params, "tab_group_update"); + if (isRpcError(ctxOrErr)) return ctxOrErr; + const ctx = ctxOrErr; + const bad = validatePositiveInt("group_id", params.group_id); + if (bad) return bad; + if (params.title !== undefined && typeof params.title !== "string") { + return { code: "invalid_params", message: "tab_group_update title must be a string" }; + } + if (params.color !== undefined && !TAB_GROUP_COLORS.has(params.color)) { + return { code: "invalid_params", message: "tab_group_update color must be a valid tab group color" }; + } + if (params.collapsed !== undefined && typeof params.collapsed !== "boolean") { + return { code: "invalid_params", message: "tab_group_update collapsed must be a boolean" }; + } + if (aborted(deps.signal, "tab_group_update")) { + return { code: "cancelled", message: "tab_group_update aborted" }; + } + + const groupsApi = getTabGroupsApi(deps); + const groupOrErr = await authoriseAgentGroup(ctx, params.group_id, groupsApi, "tab_group_update"); + if (isRpcError(groupOrErr)) return groupOrErr; + if (aborted(deps.signal, "tab_group_update")) { + return { code: "cancelled", message: "tab_group_update aborted" }; + } + + let updated: chrome.tabGroups.TabGroup | undefined; + try { + updated = await groupsApi.update(params.group_id, { + ...(params.title !== undefined ? { title: params.title } : {}), + ...(params.color !== undefined ? { color: params.color } : {}), + ...(params.collapsed !== undefined ? { collapsed: params.collapsed } : {}), + }); + } catch (err) { + return { code: "protocol_error", message: err instanceof Error ? err.message : String(err) }; + } + if (!updated) { + return { code: "not_found", message: `tab_group_update: group ${params.group_id} not found` }; + } + return { + group_id: params.group_id, + title: updated.title || undefined, + color: updated.color as TabGroupColor, + collapsed: updated.collapsed, + }; +} + +/** List tab groups living in the session's Agent Window. */ +export async function handleTabGroupList( + manager: SessionManager, + params: TabGroupListParams, + deps: TabManagementDeps = {}, +): Promise { + const ctxOrErr = lookupSession(manager, params, "tab_group_list"); + if (isRpcError(ctxOrErr)) return ctxOrErr; + const ctx = ctxOrErr; + if (aborted(deps.signal, "tab_group_list")) { + return { code: "cancelled", message: "tab_group_list aborted" }; + } + + const groupsApi = getTabGroupsApi(deps); + const groupMutation = getTabGroupMutationApi(deps); + let chromeGroups: chrome.tabGroups.TabGroup[]; + try { + chromeGroups = await groupsApi.query({ windowId: ctx.agentWindowId }); + } catch (err) { + return { code: "protocol_error", message: err instanceof Error ? err.message : String(err) }; + } + const groups: TabGroupInfo[] = []; + for (const g of chromeGroups) { + if (aborted(deps.signal, "tab_group_list")) { + return { code: "cancelled", message: "tab_group_list aborted" }; + } + const infoOrErr = await buildGroupInfo(groupsApi, groupMutation.queryTabs, ctx.agentWindowId, g.id); + // A group that vanished between query() and the follow-up get() is + // reported as empty rather than failing the whole listing. + if (isRpcError(infoOrErr)) continue; + groups.push(infoOrErr); + } + return { groups }; +} + +/** + * Remove every tab currently in `group_id` from that group. Tabs stay + * open in place; only their group membership is cleared. + */ +export async function handleTabGroupUngroup( + manager: SessionManager, + params: TabGroupUngroupParams, + deps: TabManagementDeps = {}, +): Promise { + const ctxOrErr = lookupSession(manager, params, "tab_group_ungroup"); + if (isRpcError(ctxOrErr)) return ctxOrErr; + const ctx = ctxOrErr; + const bad = validatePositiveInt("group_id", params.group_id); + if (bad) return bad; + if (aborted(deps.signal, "tab_group_ungroup")) { + return { code: "cancelled", message: "tab_group_ungroup aborted" }; + } + + const groupsApi = getTabGroupsApi(deps); + const groupOrErr = await authoriseAgentGroup(ctx, params.group_id, groupsApi, "tab_group_ungroup"); + if (isRpcError(groupOrErr)) return groupOrErr; + + const groupMutation = getTabGroupMutationApi(deps); + let tabs: chrome.tabs.Tab[]; + try { + tabs = await groupMutation.queryTabs({ windowId: ctx.agentWindowId, groupId: params.group_id }); + } catch (err) { + return { code: "protocol_error", message: err instanceof Error ? err.message : String(err) }; + } + const tabIds = tabs.filter((t): t is CreatedChromeTab => typeof t.id === "number").map((t) => t.id); + if (aborted(deps.signal, "tab_group_ungroup")) { + return { code: "cancelled", message: "tab_group_ungroup aborted" }; + } + if (tabIds.length === 0) { + return { tab_ids: [] }; + } + try { + await groupMutation.ungroup(tabIds); + } catch (err) { + return { code: "protocol_error", message: err instanceof Error ? err.message : String(err) }; + } + return { tab_ids: tabIds }; +} diff --git a/apps/extension/wxt.config.ts b/apps/extension/wxt.config.ts index e9c5bd04..f77283a5 100644 --- a/apps/extension/wxt.config.ts +++ b/apps/extension/wxt.config.ts @@ -48,6 +48,7 @@ export default defineConfig({ "scripting", "tabs", "storage", + "tabGroups", "webNavigation", "windows", ], diff --git a/crates/bsk-cli/skill/SKILL.md b/crates/bsk-cli/skill/SKILL.md index 55e9d808..6a3423df 100644 --- a/crates/bsk-cli/skill/SKILL.md +++ b/crates/bsk-cli/skill/SKILL.md @@ -116,7 +116,7 @@ A task may need more than one reference as it progresses. | When | Read | | --- | --- | | Website debugging, reproduction evidence, or request rules/replay | [Debugging](references/debugging.md) | -| Required profile, existing user tab, multiple/background tabs, or remote tab ownership | [Tabs and profiles](references/tabs-and-profiles.md) | +| Required profile, existing user tab, multiple/background tabs, grouping/organizing tabs, or remote tab ownership | [Tabs and profiles](references/tabs-and-profiles.md) | | Missing CLI, daemon startup failure, sandboxed startup, connection failure, or remote pairing | [Environment](references/environment.md) | | Hover menus/probing, scrolling, `next_cursor`/`@more`, console/network, emulation, evaluation, or recording | [Interaction details](references/interaction-details.md) | | Screenshot, full-page capture, or `[visual:screenshot]`/Canvas interaction | [Screenshots and Canvas](references/screenshots-and-canvas.md) | diff --git a/crates/bsk-cli/skill/references/tabs-and-profiles.md b/crates/bsk-cli/skill/references/tabs-and-profiles.md index 6497cc1e..0081e350 100644 --- a/crates/bsk-cli/skill/references/tabs-and-profiles.md +++ b/crates/bsk-cli/skill/references/tabs-and-profiles.md @@ -100,3 +100,28 @@ Serial execution does not restore a previous login state, so each task still verifies the expected account or tenant on its target site. When isolating with a separate profile, follow the profile-to-instance verification and explicit binding steps above, and preserve any profile the user required. + +## Tab groups + +```sh +bsk tab group create --session --tab [--tab ...] [--title ] [--color ] +bsk tab group create --session --tab --group-id +bsk tab group update --session [--title ] [--color ] [--collapsed|--expand] +bsk tab group list --session +bsk tab group ungroup --session +``` + +Only tabs already inside the session's Agent Window (own tab or borrowed tab) can +be grouped — same sandbox rule as `tab close` / `tab select`; a tab outside it is +rejected with `permission_denied`. `list` and `ungroup` are scoped to the Agent +Window's own groups the same way. `ungroup` removes tabs from the group without +closing them. + +**Choose `--title` from what the tabs are actually about, never a placeholder.** +Read the grouped tabs' titles/URLs first (`bsk tab list --scope agent`) and name +the group after the real topic — e.g. `AI Security Research` for a set of OWASP +LLM Top 10 / prompt-injection / red-team tabs, not `Group 1` or `New Tabs`. If the +tabs cover more than one topic, either pick the dominant one or split the grouping +calls so each group stays about one thing. `--color` is cosmetic; pick one that +fits the topic (e.g. `red` for security-review tabs) or omit it and let the +browser assign one. diff --git a/crates/bsk-cli/src/cli/tab.rs b/crates/bsk-cli/src/cli/tab.rs index d20d1869..567ec1d7 100644 --- a/crates/bsk-cli/src/cli/tab.rs +++ b/crates/bsk-cli/src/cli/tab.rs @@ -1,6 +1,7 @@ //! `bsk tab …` subcommands. M6.1 landed `list`; M8 adds create / close / //! select / borrow / return for Agent Window tab management and the -//! user-tab borrow ↔ return loop. +//! user-tab borrow ↔ return loop. `tab group` adds native tab-group +//! management (`chrome.tabGroups`) scoped to the Agent Window. use std::path::PathBuf; @@ -8,8 +9,10 @@ use anyhow::Context; use bsk_protocol::Method; use bsk_protocol::tools::{ TabBorrowParams, TabBorrowResult, TabCloseParams, TabCloseResult, TabCreateParams, - TabCreateResult, TabInfo, TabListParams, TabListResult, TabReturnParams, TabReturnResult, - TabScope, TabSelectParams, TabSelectResult, + TabCreateResult, TabGroupColor, TabGroupCreateParams, TabGroupCreateResult, TabGroupInfo, + TabGroupListParams, TabGroupListResult, TabGroupUngroupParams, TabGroupUngroupResult, + TabGroupUpdateParams, TabGroupUpdateResult, TabInfo, TabListParams, TabListResult, + TabReturnParams, TabReturnResult, TabScope, TabSelectParams, TabSelectResult, }; use clap::{Args, Subcommand, ValueEnum}; use serde::Serialize; @@ -38,6 +41,125 @@ pub enum TabSub { Borrow(TabBorrowArgs), /// Return a previously borrowed tab to its origin window. Return(TabReturnArgs), + /// Group, ungroup, list, or restyle tab groups in the Agent Window. + Group(TabGroupCmd), +} + +#[derive(Debug, Clone, Args)] +pub struct TabGroupCmd { + #[command(subcommand)] + pub sub: TabGroupSub, +} + +#[derive(Debug, Clone, Subcommand)] +pub enum TabGroupSub { + /// Group tabs into a new tab group, or add them to an existing one. + Create(TabGroupCreateArgs), + /// Rename, recolor, or (un)collapse an existing tab group. + Update(TabGroupUpdateArgs), + /// List tab groups in the session's Agent Window. + List(TabGroupListArgs), + /// Remove every tab in a group from that group (tabs stay open). + Ungroup(TabGroupUngroupArgs), +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, ValueEnum)] +#[value(rename_all = "lowercase")] +pub enum CliTabGroupColor { + Grey, + Blue, + Red, + Yellow, + Green, + Pink, + Purple, + Cyan, + Orange, +} + +impl From for TabGroupColor { + fn from(value: CliTabGroupColor) -> Self { + match value { + CliTabGroupColor::Grey => TabGroupColor::Grey, + CliTabGroupColor::Blue => TabGroupColor::Blue, + CliTabGroupColor::Red => TabGroupColor::Red, + CliTabGroupColor::Yellow => TabGroupColor::Yellow, + CliTabGroupColor::Green => TabGroupColor::Green, + CliTabGroupColor::Pink => TabGroupColor::Pink, + CliTabGroupColor::Purple => TabGroupColor::Purple, + CliTabGroupColor::Cyan => TabGroupColor::Cyan, + CliTabGroupColor::Orange => TabGroupColor::Orange, + } + } +} + +fn color_label(color: TabGroupColor) -> &'static str { + match color { + TabGroupColor::Grey => "grey", + TabGroupColor::Blue => "blue", + TabGroupColor::Red => "red", + TabGroupColor::Yellow => "yellow", + TabGroupColor::Green => "green", + TabGroupColor::Pink => "pink", + TabGroupColor::Purple => "purple", + TabGroupColor::Cyan => "cyan", + TabGroupColor::Orange => "orange", + } +} + +#[derive(Debug, Clone, Args)] +pub struct TabGroupCreateArgs { + /// Session id (must be active). + #[arg(long)] + pub session: String, + /// Tab ids to group (each must already be in the session's Agent + /// Window — own tab or borrowed tab). Repeatable or comma-separated. + #[arg(long = "tab", required = true, num_args = 1.., value_delimiter = ',')] + pub tabs: Vec, + /// Add to this existing group instead of creating a new one. + #[arg(long)] + pub group_id: Option, + /// Group title. + #[arg(long)] + pub title: Option, + /// Group color (Chrome picks one automatically when omitted). + #[arg(long, value_enum)] + pub color: Option, +} + +#[derive(Debug, Clone, Args)] +pub struct TabGroupUpdateArgs { + /// Group id to update. + pub group_id: i64, + #[arg(long)] + pub session: String, + /// New title. + #[arg(long)] + pub title: Option, + /// New color. + #[arg(long, value_enum)] + pub color: Option, + /// Collapse the group in the tab strip. + #[arg(long, conflicts_with = "expand")] + pub collapsed: bool, + /// Expand a previously collapsed group. + #[arg(long)] + pub expand: bool, +} + +#[derive(Debug, Clone, Args)] +pub struct TabGroupListArgs { + /// Session id (must be active). + #[arg(long)] + pub session: String, +} + +#[derive(Debug, Clone, Args)] +pub struct TabGroupUngroupArgs { + /// Group id to dissolve. + pub group_id: i64, + #[arg(long)] + pub session: String, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, ValueEnum)] @@ -135,6 +257,16 @@ pub fn dispatch(cmd: TabCmd, format: Format) -> Result<(), CliError> { TabSub::Select(args) => run_select(info.sock_path, args, format), TabSub::Borrow(args) => run_borrow(info.sock_path, args, format), TabSub::Return(args) => run_return(info.sock_path, args, format), + TabSub::Group(cmd) => dispatch_group(cmd, info.sock_path, format), + } +} + +fn dispatch_group(cmd: TabGroupCmd, sock: PathBuf, format: Format) -> Result<(), CliError> { + match cmd.sub { + TabGroupSub::Create(args) => run_group_create(sock, args, format), + TabGroupSub::Update(args) => run_group_update(sock, args, format), + TabGroupSub::List(args) => run_group_list(sock, args, format), + TabGroupSub::Ungroup(args) => run_group_ungroup(sock, args, format), } } @@ -368,3 +500,169 @@ fn call(sock: PathBuf, params: TabListParams) -> Result TOOL_IPC_TIMEOUT, ) } + +// --------------------------------------------------------------------------- +// tab group +// --------------------------------------------------------------------------- + +fn run_group_create( + sock: PathBuf, + args: TabGroupCreateArgs, + format: Format, +) -> Result<(), CliError> { + let params = TabGroupCreateParams { + session_id: args.session, + tab_ids: args.tabs, + group_id: args.group_id, + title: args.title, + color: args.color.map(Into::into), + }; + let reply: TabGroupCreateResult = ipc_call( + "tab-group-create-1", + Method::ToolTabGroupCreate, + sock, + params, + )?; + print_payload(&reply, format, || { + println!( + "group_id={} color={} title={} tabs={}", + reply.group_id, + color_label(reply.color), + reply.title.as_deref().unwrap_or(""), + format_tab_ids(&reply.tab_ids), + ); + }) +} + +fn run_group_update( + sock: PathBuf, + args: TabGroupUpdateArgs, + format: Format, +) -> Result<(), CliError> { + let collapsed = if args.collapsed { + Some(true) + } else if args.expand { + Some(false) + } else { + None + }; + let params = TabGroupUpdateParams { + session_id: args.session, + group_id: args.group_id, + title: args.title, + color: args.color.map(Into::into), + collapsed, + }; + let reply: TabGroupUpdateResult = ipc_call( + "tab-group-update-1", + Method::ToolTabGroupUpdate, + sock, + params, + )?; + print_payload(&reply, format, || { + println!( + "group_id={} color={} collapsed={} title={}", + reply.group_id, + color_label(reply.color), + reply.collapsed, + reply.title.as_deref().unwrap_or(""), + ); + }) +} + +fn run_group_list(sock: PathBuf, args: TabGroupListArgs, format: Format) -> Result<(), CliError> { + let params = TabGroupListParams { + session_id: args.session, + }; + let reply: TabGroupListResult = + ipc_call("tab-group-list-1", Method::ToolTabGroupList, sock, params)?; + match format { + Format::Json => { + let json = serde_json::to_string_pretty(&reply) + .map_err(|e| CliError::Local(anyhow::anyhow!(e)))?; + println!("{json}"); + } + Format::Human => render_groups_table(&reply.groups), + } + Ok(()) +} + +fn run_group_ungroup( + sock: PathBuf, + args: TabGroupUngroupArgs, + format: Format, +) -> Result<(), CliError> { + let params = TabGroupUngroupParams { + session_id: args.session, + group_id: args.group_id, + }; + let reply: TabGroupUngroupResult = ipc_call( + "tab-group-ungroup-1", + Method::ToolTabGroupUngroup, + sock, + params, + )?; + print_payload(&reply, format, || { + println!("ungrouped tabs={}", format_tab_ids(&reply.tab_ids)); + }) +} + +fn format_tab_ids(ids: &[i64]) -> String { + if ids.is_empty() { + return "".into(); + } + ids.iter().map(i64::to_string).collect::>().join(",") +} + +fn render_groups_table(groups: &[TabGroupInfo]) { + if groups.is_empty() { + println!("(no tab groups)"); + return; + } + let rows: Vec<[String; 5]> = groups + .iter() + .map(|g| { + [ + g.group_id.to_string(), + truncate(g.title.as_deref().unwrap_or("-"), 30), + color_label(g.color).to_string(), + g.collapsed.to_string(), + format_tab_ids(&g.tab_ids), + ] + }) + .collect(); + let headers = ["GROUP", "TITLE", "COLOR", "COLLAPSED", "TABS"]; + let widths: [usize; 5] = std::array::from_fn(|i| { + rows.iter() + .map(|r| r[i].len()) + .max() + .unwrap_or(0) + .max(headers[i].len()) + }); + println!( + "{:) -> RpcHandler | Method::ToolTabSelect | Method::ToolTabBorrow | Method::ToolTabReturn + | Method::ToolTabGroupCreate + | Method::ToolTabGroupUpdate + | Method::ToolTabGroupList + | Method::ToolTabGroupUngroup | Method::ToolWindowResize | Method::ToolEmulate | Method::ToolScreenshot diff --git a/crates/bsk-protocol/src/method.rs b/crates/bsk-protocol/src/method.rs index 9a6d6b5d..12a908dc 100644 --- a/crates/bsk-protocol/src/method.rs +++ b/crates/bsk-protocol/src/method.rs @@ -68,6 +68,14 @@ pub enum Method { ToolTabReturn, #[serde(rename = "tool.tab_select")] ToolTabSelect, + #[serde(rename = "tool.tab_group_create")] + ToolTabGroupCreate, + #[serde(rename = "tool.tab_group_update")] + ToolTabGroupUpdate, + #[serde(rename = "tool.tab_group_list")] + ToolTabGroupList, + #[serde(rename = "tool.tab_group_ungroup")] + ToolTabGroupUngroup, #[serde(rename = "tool.navigate")] ToolNavigate, #[serde(rename = "tool.navigate_back")] @@ -185,6 +193,9 @@ impl Method { | Method::ToolTabBorrow | Method::ToolTabReturn | Method::ToolTabSelect + | Method::ToolTabGroupCreate + | Method::ToolTabGroupUpdate + | Method::ToolTabGroupUngroup | Method::ToolWindowResize | Method::ToolEmulate | Method::ToolNavigate @@ -215,6 +226,7 @@ impl Method { // without driving new automation gestures, so they stay // ungated (teardown after interrupt must still work). Method::ToolTabList + | Method::ToolTabGroupList | Method::ToolSnapshot | Method::ToolGetHtml | Method::ToolScreenshot @@ -362,6 +374,7 @@ mod tests { #[test] fn is_mutating_classifies_read_only_tools_as_non_mutating() { assert!(!Method::ToolTabList.is_mutating()); + assert!(!Method::ToolTabGroupList.is_mutating()); assert!(!Method::ToolSnapshot.is_mutating()); assert!(!Method::ToolHover.is_mutating()); assert!(!Method::ToolObserve.is_mutating()); @@ -380,6 +393,9 @@ mod tests { assert!(Method::ToolTabBorrow.is_mutating()); assert!(Method::ToolTabReturn.is_mutating()); assert!(Method::ToolTabSelect.is_mutating()); + assert!(Method::ToolTabGroupCreate.is_mutating()); + assert!(Method::ToolTabGroupUpdate.is_mutating()); + assert!(Method::ToolTabGroupUngroup.is_mutating()); assert!(Method::ToolNavigate.is_mutating()); assert!(Method::ToolNavigateBack.is_mutating()); assert!(Method::ToolNavigateForward.is_mutating()); diff --git a/crates/bsk-protocol/src/tools/tabs.rs b/crates/bsk-protocol/src/tools/tabs.rs index 6e7921ae..67438962 100644 --- a/crates/bsk-protocol/src/tools/tabs.rs +++ b/crates/bsk-protocol/src/tools/tabs.rs @@ -178,6 +178,114 @@ pub struct TabReturnResult { pub fallback: bool, } +// --------------------------------------------------------------------------- +// tab_group_create / tab_group_update / tab_group_list / tab_group_ungroup +// --------------------------------------------------------------------------- + +/// Mirrors `chrome.tabGroups.ColorEnum`. Chrome assigns an arbitrary color +/// from this set when a group is created without one. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "lowercase")] +pub enum TabGroupColor { + Grey, + Blue, + Red, + Yellow, + Green, + Pink, + Purple, + Cyan, + Orange, +} + +/// A single tab group entry, as reported by `tool.tab_group_list`. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct TabGroupInfo { + pub group_id: i64, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub title: Option, + pub color: TabGroupColor, + pub collapsed: bool, + /// Tab ids currently in this group, in tab-strip order. + pub tab_ids: Vec, +} + +/// Params for `tool.tab_group_create`. Groups one or more tabs from the +/// requesting session's Agent Window into a new tab group, or adds them +/// to an existing group in that window when `group_id` is given (mirrors +/// `chrome.tabs.group`'s own create-or-add behavior). +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct TabGroupCreateParams { + pub session_id: String, + /// Tabs to group. Every tab must already be in the session's Agent + /// Window (own tab or borrowed tab) — same sandbox rule as `tab_close` + /// / `tab_select`. + pub tab_ids: Vec, + /// Add to this existing group instead of creating a new one. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub group_id: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub title: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub color: Option, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct TabGroupCreateResult { + pub group_id: i64, + pub title: Option, + pub color: TabGroupColor, + pub tab_ids: Vec, +} + +/// Params for `tool.tab_group_update`. Only the Agent Window's own +/// groups are addressable (enforced extension-side against +/// `chrome.tabGroups.query({ windowId })`). +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct TabGroupUpdateParams { + pub session_id: String, + pub group_id: i64, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub title: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub color: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub collapsed: Option, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct TabGroupUpdateResult { + pub group_id: i64, + pub title: Option, + pub color: TabGroupColor, + pub collapsed: bool, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct TabGroupListParams { + pub session_id: String, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct TabGroupListResult { + pub groups: Vec, +} + +/// Params for `tool.tab_group_ungroup`. Removes every tab currently in +/// `group_id` from that group; the tabs themselves are left open in +/// place. `group_id` must resolve to a group in the session's Agent +/// Window. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct TabGroupUngroupParams { + pub session_id: String, + pub group_id: i64, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct TabGroupUngroupResult { + pub tab_ids: Vec, +} + #[cfg(test)] mod tests { use super::*; @@ -306,4 +414,71 @@ mod tests { let v2 = serde_json::to_value(&r2).unwrap(); assert_eq!(v2["fallback"], true); } + + #[test] + fn tab_group_color_renders_as_lowercase() { + let v = serde_json::to_value(TabGroupColor::Cyan).unwrap(); + assert_eq!(v, "cyan"); + let round: TabGroupColor = serde_json::from_value(v).unwrap(); + assert_eq!(round, TabGroupColor::Cyan); + } + + #[test] + fn tab_group_create_params_optional_fields_omitted_by_default() { + let v = json!({ "session_id": "aa11", "tab_ids": [1, 2] }); + let p: TabGroupCreateParams = serde_json::from_value(v).unwrap(); + assert_eq!(p.tab_ids, vec![1, 2]); + assert!(p.group_id.is_none()); + assert!(p.title.is_none()); + assert!(p.color.is_none()); + } + + #[test] + fn tab_group_create_result_round_trip() { + let r = TabGroupCreateResult { + group_id: 42, + title: Some("Research".into()), + color: TabGroupColor::Blue, + tab_ids: vec![1, 2, 3], + }; + let v = serde_json::to_value(&r).unwrap(); + assert_eq!(v["group_id"], 42); + assert_eq!(v["color"], "blue"); + let round: TabGroupCreateResult = serde_json::from_value(v).unwrap(); + assert_eq!(round, r); + } + + #[test] + fn tab_group_update_params_all_optional_except_ids() { + let p: TabGroupUpdateParams = + serde_json::from_value(json!({ "session_id": "aa11", "group_id": 7 })).unwrap(); + assert!(p.title.is_none()); + assert!(p.color.is_none()); + assert!(p.collapsed.is_none()); + } + + #[test] + fn tab_group_info_round_trip() { + let info = TabGroupInfo { + group_id: 7, + title: None, + color: TabGroupColor::Grey, + collapsed: true, + tab_ids: vec![10, 11], + }; + let v = serde_json::to_value(&info).unwrap(); + assert!(v.get("title").is_none()); + let round: TabGroupInfo = serde_json::from_value(v).unwrap(); + assert_eq!(round, info); + } + + #[test] + fn tab_group_ungroup_result_round_trip() { + let r = TabGroupUngroupResult { + tab_ids: vec![1, 2], + }; + let v = serde_json::to_value(&r).unwrap(); + let round: TabGroupUngroupResult = serde_json::from_value(v).unwrap(); + assert_eq!(round, r); + } } From 00dbd3c4d6e5b2de4ac0b8de7f44bc79dfa2a66e Mon Sep 17 00:00:00 2001 From: glatinone <93207632+glatinone@users.noreply.github.com> Date: Tue, 29 Sep 2026 22:10:59 +0800 Subject: [PATCH 2/2] Address review: member authz, verified recovery, cancellation, types Review follow-up on #339 (Zhang GH): - Register `group_window_mismatch` in `RpcErrorReason` and fix the three potentially-undefined tab IDs in the test helper, so `tsc --noEmit` passes. - Authorise every affected tab with the direct-control rules, not just the group's window: adding to an existing group, `update` and `ungroup` now validate all current members via `authoriseAgentTab`, so an unowned or borrowed tab can no longer be affected through group operations while `tab_select` would reject controlling it directly. - Rebuild the cross-window recovery as a bounded ladder (group move, then per-tab moves plus an in-place group recreation), re-verifying placement from browser state at every step: per-tab moves across windows drop group membership and destroy the group in real Chromium, so the old group ID is no longer trusted. Failures report which tabs did not land and carry an honest `cleanup_state` instead of swallowing cleanup errors; the test double now models the membership loss. - Check cancellation after each async stage of group creation and before subsequent mutations, recovering stranded memberships before stopping. - Rebase onto latest main; resolve the tabs-and-profiles.md conflict preserving the shared-login-state guidance. Co-Authored-By: Claude Code --- CHANGELOG.md | 17 +- .../src/tools/__tests__/tabs.test.ts | 274 +++++++++++++++-- apps/extension/src/tools/tabs.ts | 277 +++++++++++++++--- apps/extension/src/transport/types.ts | 1 + .../skill/references/tabs-and-profiles.md | 13 +- 5 files changed, 523 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69c66210..200bdad1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,11 +65,18 @@ Starting from 0.2.0, CLI / Extension / DSH Plugin share the same version number. - `bsk tab group create|update|list|ungroup`: native Chrome/Edge tab-group management (`chrome.tabGroups`) scoped to the session's Agent Window, with - the same sandbox rule as `tab close` / `tab select`. Guards against browsers - that place a freshly created group outside the target window (most - reproducible with `session start --no-focus`) with a relocation fallback, - and reports an honest error instead of a silent partial group when no - relocation attempt lands the tabs in the Agent Window. + the same sandbox rule as `tab close` / `tab select`. Every affected tab — + not only the ones a request names, but every existing member of the group + too — is authorised with the direct-control rules, so unowned or borrowed + tabs cannot be renamed, regrouped or ungrouped through group operations. + Browsers that misplace a freshly created group (most reproducible with + `session start --no-focus`) get a bounded recovery ladder — group-level + move, then per-tab relocation plus an in-place group recreation, each + step re-verified against browser state — and an honest + `group_window_mismatch` error with `cleanup_state` instead of a silent + partial group when no attempt lands every tab. Group creation checks + cancellation between stages and stops with cleanup rather than + continuing to mutate after an abort. ## [0.3.1] - 2026-09-23 diff --git a/apps/extension/src/tools/__tests__/tabs.test.ts b/apps/extension/src/tools/__tests__/tabs.test.ts index 192180a0..12570924 100644 --- a/apps/extension/src/tools/__tests__/tabs.test.ts +++ b/apps/extension/src/tools/__tests__/tabs.test.ts @@ -1485,10 +1485,13 @@ function makeTabGroupApis( /** * Simulates the real-world Chromium quirk (observed manually against * Edge 153) where a freshly created group lands in the tabs' *previous* - * window rather than the requested `createProperties.windowId`. Exists - * only to exercise `handleTabGroupCreate`'s self-heal path. + * window rather than the requested `createProperties.windowId`, and + * `tabGroups.move` resolves without relocating anything. "first" applies + * the quirk to the first `tabs.group` call only (so a recreation attempt + * can succeed); "always" misplaces every creation. Exists only to + * exercise `handleTabGroupCreate`'s self-heal path. */ - options: { ignoreCreateWindowId?: boolean } = {}, + options: { misplaceCreatedGroups?: "first" | "always" } = {}, ): { tabGroups: TabGroupMutationApi; tabGroupsApi: ChromeTabGroupsApi; @@ -1502,18 +1505,28 @@ function makeTabGroupApis( move: ReturnType; }; } { + let groupCreateCalls = 0; + const hostIgnoresGroupMove = options.misplaceCreatedGroups !== undefined; const group = vi.fn(async (opts: chrome.tabs.GroupOptions) => { - const tabIds = Array.isArray(opts.tabIds) ? opts.tabIds : [opts.tabIds]; + const tabIds: number[] = ( + Array.isArray(opts.tabIds) ? opts.tabIds : opts.tabIds !== undefined ? [opts.tabIds] : [] + ).filter((id): id is number => typeof id === "number"); + const firstTabId = tabIds[0]; let groupId = opts.groupId; if (groupId === undefined) { groupId = groupState.nextGroupId++; - // The "ignore" path always lands in a fixed window unrelated to the + const quirkActive = + options.misplaceCreatedGroups === "always" || + (options.misplaceCreatedGroups === "first" && groupCreateCalls++ === 0); + // The quirk path always lands in a fixed window unrelated to the // tabs' own windowId, mirroring the real quirk observed against Edge // 153: the freshly created group escaped to the browser's regular // window regardless of what `createProperties.windowId` requested. - const windowId = options.ignoreCreateWindowId + const windowId = quirkActive ? 999 - : (opts.createProperties?.windowId ?? tabState.tabs.get(tabIds[0])?.windowId ?? 1); + : (opts.createProperties?.windowId ?? + (firstTabId !== undefined ? tabState.tabs.get(firstTabId)?.windowId : undefined) ?? + 1); groupState.groups.set(groupId, { id: groupId, windowId, @@ -1564,7 +1577,7 @@ function makeTabGroupApis( // Mirrors the real quirk this fallback exists for: `tabGroups.move` // resolves without error but does not actually relocate anything // against an unfocused window. - if (options.ignoreCreateWindowId) return g; + if (hostIgnoresGroupMove) return g; const updated = { ...g, ...(typeof props.windowId === "number" ? { windowId: props.windowId } : {}) }; groupState.groups.set(groupId, updated); return updated; @@ -1576,6 +1589,32 @@ function makeTabGroupApis( }; } +/** + * Wraps a tab API so per-tab moves model what real Chromium does during + * group relocation (verified in review of #339): moving a grouped tab + * across windows *drops its group membership*, and once the last member + * leaves, the group itself is destroyed. + */ +function makeChromiumStyleTabMove(base: TabMutationApi, groupState: FakeGroupState): TabMutationApi { + return { + ...base, + move: vi.fn(async (tabId: number, props: chrome.tabs.MoveProperties) => { + const tab = await base.get(tabId); + const fromWindowId = tab.windowId; + const moved = await base.move(tabId, props); + if (typeof props.windowId === "number" && props.windowId !== fromWindowId) { + const gid = groupState.tabGroupOf.get(tabId); + if (gid !== undefined) { + groupState.tabGroupOf.delete(tabId); + const stillHasMembers = [...groupState.tabGroupOf.values()].some((g) => g === gid); + if (!stillHasMembers) groupState.groups.delete(gid); + } + } + return moved; + }), + }; +} + function emptyGroupState(): FakeGroupState { return { groups: new Map(), tabGroupOf: new Map(), nextGroupId: 1 }; } @@ -1655,6 +1694,33 @@ describe("handleTabGroupCreate", () => { expect(res.tab_ids.sort()).toEqual([5, 6]); }); + it("refuses to add tabs to a group whose existing members the remote session does not own", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]), remote: () => true }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([ + // Tab 5 sits in the Agent Window but belongs to the user, not to + // this session — the same tab tab_select would refuse to control. + [5, { id: 5, windowId: 100 } as chrome.tabs.Tab], + [6, { id: 6, windowId: 100 } as chrome.tabs.Tab], + ]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: tabs } = makeTabMutationApi(tabState); + const groupState = emptyGroupState(); + groupState.groups.set(1, { id: 1, windowId: 100, title: "Existing", color: "grey", collapsed: false }); + groupState.tabGroupOf.set(5, 1); + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, groupState); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [6], group_id: 1 }, + { tabs, tabGroups, tabGroupsApi }, + ); + expect(res).toMatchObject({ code: "permission_denied" }); + expect(spies.group).not.toHaveBeenCalled(); + }); + it("rejects group_id belonging to another window with permission_denied", async () => { const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); await sm.start("aa11"); @@ -1676,7 +1742,7 @@ describe("handleTabGroupCreate", () => { expect(spies.group).not.toHaveBeenCalled(); }); - it("falls back to per-tab moves when the browser ignores both createProperties.windowId and tabGroups.move", async () => { + it("falls back to per-tab moves and recreates the group when the browser ignores both createProperties.windowId and tabGroups.move", async () => { const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); await sm.start("aa11"); const tabState: FakeTabState = { @@ -1687,10 +1753,17 @@ describe("handleTabGroupCreate", () => { nextTabId: 50, windowsClosed: new Set(), }; - const { api: tabs, spies: tabSpies } = makeTabMutationApi(tabState); - const { tabGroups, tabGroupsApi, spies: groupSpies } = makeTabGroupApis(tabState, emptyGroupState(), { - ignoreCreateWindowId: true, - }); + const { api: baseTabs, spies: tabSpies } = makeTabMutationApi(tabState); + const groupState = emptyGroupState(); + const { tabGroups, tabGroupsApi, spies: groupSpies } = makeTabGroupApis( + tabState, + groupState, + { misplaceCreatedGroups: "first" }, + ); + // Real Chromium drops group membership on cross-window per-tab moves, + // destroying the group once its last member leaves — the fallback must + // recreate the group instead of querying the dead ID. + const tabs = makeChromiumStyleTabMove(baseTabs, groupState); const res = await handleTabGroupCreate( sm, { session_id: "aa11", tab_ids: [5, 6] }, @@ -1698,15 +1771,23 @@ describe("handleTabGroupCreate", () => { ); if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); // Group-level relocation was attempted first... - expect(groupSpies.move).toHaveBeenCalledWith(res.group_id, { windowId: 100, index: -1 }); + expect(groupSpies.move).toHaveBeenCalledWith(1, { windowId: 100, index: -1 }); // ...but since it silently no-opped, each tab was moved back individually. expect(tabSpies.move).toHaveBeenCalledWith(5, { windowId: 100, index: -1 }); expect(tabSpies.move).toHaveBeenCalledWith(6, { windowId: 100, index: -1 }); expect(tabState.tabs.get(5)?.windowId).toBe(100); expect(tabState.tabs.get(6)?.windowId).toBe(100); + // The per-tab moves destroyed the misplaced group; the handler recreated + // the group in the Agent Window and reports the *new* ID. + expect(groupState.groups.has(1)).toBe(false); + expect(res.group_id).toBe(2); + expect(groupState.groups.get(2)?.windowId).toBe(100); + expect(groupState.tabGroupOf.get(5)).toBe(2); + expect(groupState.tabGroupOf.get(6)).toBe(2); + expect(res.tab_ids.sort()).toEqual([5, 6]); }); - it("reports a clear error and ungroups the tabs when no relocation attempt lands them in the Agent Window", async () => { + it("reports a clear error with cleanup state when no relocation attempt lands the tabs in the Agent Window", async () => { const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); await sm.start("aa11"); const tabState: FakeTabState = { @@ -1719,14 +1800,115 @@ describe("handleTabGroupCreate", () => { // observed on some hosts for tabs that are members of a tab group. const tabs: TabMutationApi = { ...baseTabs, move: vi.fn(async (id, p) => baseTabs.get(id)) }; const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, emptyGroupState(), { - ignoreCreateWindowId: true, + misplaceCreatedGroups: "always", }); const res = await handleTabGroupCreate( sm, { session_id: "aa11", tab_ids: [5] }, { tabs, tabGroups, tabGroupsApi }, ); - expect(res).toMatchObject({ code: "cdp_failed", data: { reason: "group_window_mismatch" } }); + expect(res).toMatchObject({ + code: "cdp_failed", + data: { reason: "group_window_mismatch", cleanup_state: "complete" }, + }); + expect(spies.ungroup).toHaveBeenCalledWith([5]); + }); + + it("reports a partial placement honestly when only some tabs land in the Agent Window", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([ + [5, { id: 5, windowId: 100, index: 0 } as chrome.tabs.Tab], + [6, { id: 6, windowId: 100, index: 1 } as chrome.tabs.Tab], + ]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: baseTabs } = makeTabMutationApi(tabState); + // Tab 5 relocates, tab 6 does not. + const tabs: TabMutationApi = { + ...baseTabs, + move: vi.fn(async (id, p) => (id === 5 ? baseTabs.move(id, p) : baseTabs.get(id))), + }; + const { tabGroups, tabGroupsApi } = makeTabGroupApis(tabState, emptyGroupState(), { + misplaceCreatedGroups: "always", + }); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [5, 6] }, + { tabs, tabGroups, tabGroupsApi }, + ); + expect(res).toMatchObject({ + code: "cdp_failed", + data: { reason: "group_window_mismatch", cleanup_state: "complete" }, + }); + expect(JSON.stringify(res)).toContain("6"); + // Only membership was cleaned up; the unlanded tab stays where the + // browser left it rather than being closed or force-moved further. + expect(tabState.tabs.get(6)?.windowId).toBe(999); + expect(tabState.tabs.get(5)?.windowId).toBe(100); + }); + + it("stops with cancelled when aborted during group creation, before further mutations", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([ + [5, { id: 5, windowId: 100, index: 0 } as chrome.tabs.Tab], + [6, { id: 6, windowId: 100, index: 1 } as chrome.tabs.Tab], + ]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: tabs } = makeTabMutationApi(tabState); + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, emptyGroupState()); + const controller = new AbortController(); + const abortingGroup = vi.fn(async (opts: chrome.tabs.GroupOptions) => { + controller.abort(); + return tabGroups.group(opts); + }); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [5, 6] }, + { tabs, tabGroups: { ...tabGroups, group: abortingGroup }, tabGroupsApi, signal: controller.signal }, + ); + expect(res).toMatchObject({ code: "cancelled" }); + // The group landed correctly, so no recovery mutation ran — but the + // handler must not continue to metadata updates or report success. + expect(spies.ungroup).not.toHaveBeenCalled(); + expect(spies.update).not.toHaveBeenCalled(); + }); + + it("ungroups stranded tabs and returns cancelled when aborted during relocation", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([[5, { id: 5, windowId: 100, index: 0 } as chrome.tabs.Tab]]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: baseTabs } = makeTabMutationApi(tabState); + const controller = new AbortController(); + const tabs: TabMutationApi = { + ...baseTabs, + move: vi.fn(async (id, p) => { + controller.abort(); + return baseTabs.move(id, p); + }), + }; + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, emptyGroupState(), { + misplaceCreatedGroups: "always", + }); + const res = await handleTabGroupCreate( + sm, + { session_id: "aa11", tab_ids: [5] }, + { tabs, tabGroups, tabGroupsApi, signal: controller.signal }, + ); + expect(res).toMatchObject({ + code: "cancelled", + data: { cleanup_state: "complete" }, + }); expect(spies.ungroup).toHaveBeenCalledWith([5]); }); @@ -1752,11 +1934,12 @@ describe("handleTabGroupUpdate", () => { const tabState: FakeTabState = { tabs: new Map(), nextTabId: 50, windowsClosed: new Set() }; const groupState = emptyGroupState(); groupState.groups.set(1, { id: 1, windowId: 100, title: "Old", color: "grey", collapsed: false }); - const { tabGroupsApi } = makeTabGroupApis(tabState, groupState); + const { api: tabs } = makeTabMutationApi(tabState); + const { tabGroups, tabGroupsApi } = makeTabGroupApis(tabState, groupState); const res = await handleTabGroupUpdate( sm, { session_id: "aa11", group_id: 1, title: "New", color: "red", collapsed: true }, - { tabGroupsApi }, + { tabs, tabGroups, tabGroupsApi }, ); if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); expect(res).toMatchObject({ group_id: 1, title: "New", color: "red", collapsed: true }); @@ -1778,6 +1961,28 @@ describe("handleTabGroupUpdate", () => { expect(spies.update).not.toHaveBeenCalled(); }); + it("refuses to rename a group containing a tab the remote session does not own", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]), remote: () => true }); + await sm.start("aa11"); + const tabState: FakeTabState = { + tabs: new Map([[5, { id: 5, windowId: 100 } as chrome.tabs.Tab]]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: tabs } = makeTabMutationApi(tabState); + const groupState = emptyGroupState(); + groupState.groups.set(1, { id: 1, windowId: 100, title: "Old", color: "grey", collapsed: false }); + groupState.tabGroupOf.set(5, 1); + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, groupState); + const res = await handleTabGroupUpdate( + sm, + { session_id: "aa11", group_id: 1, title: "New" }, + { tabs, tabGroups, tabGroupsApi }, + ); + expect(res).toMatchObject({ code: "permission_denied" }); + expect(spies.update).not.toHaveBeenCalled(); + }); + it("rejects an invalid color", async () => { const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); await sm.start("aa11"); @@ -1835,11 +2040,12 @@ describe("handleTabGroupUngroup", () => { groupState.groups.set(1, { id: 1, windowId: 100, title: "", color: "grey", collapsed: false }); groupState.tabGroupOf.set(5, 1); groupState.tabGroupOf.set(6, 1); + const { api: tabs } = makeTabMutationApi(tabState); const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, groupState); const res = await handleTabGroupUngroup( sm, { session_id: "aa11", group_id: 1 }, - { tabGroups, tabGroupsApi }, + { tabs, tabGroups, tabGroupsApi }, ); if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`); expect(res.tab_ids.sort()).toEqual([5, 6]); @@ -1849,6 +2055,34 @@ describe("handleTabGroupUngroup", () => { expect(tabState.tabs.has(6)).toBe(true); }); + it("refuses to ungroup when another session has borrowed a member", async () => { + const sm = new SessionManager({ agentWindow: fakeAgentWindow([100, 300]) }); + await sm.start("aa11"); + const ctxB = await sm.start("bb22"); + ctxB.borrowedTabs.set(6, { tabId: 6, originalWindowId: 300, originalIndex: 1 }); + const tabState: FakeTabState = { + tabs: new Map([ + [5, { id: 5, windowId: 100 } as chrome.tabs.Tab], + [6, { id: 6, windowId: 100 } as chrome.tabs.Tab], + ]), + nextTabId: 50, + windowsClosed: new Set(), + }; + const { api: tabs } = makeTabMutationApi(tabState); + const groupState = emptyGroupState(); + groupState.groups.set(1, { id: 1, windowId: 100, title: "", color: "grey", collapsed: false }); + groupState.tabGroupOf.set(5, 1); + groupState.tabGroupOf.set(6, 1); + const { tabGroups, tabGroupsApi, spies } = makeTabGroupApis(tabState, groupState); + const res = await handleTabGroupUngroup( + sm, + { session_id: "aa11", group_id: 1 }, + { tabs, tabGroups, tabGroupsApi }, + ); + expect(res).toMatchObject({ code: "permission_denied", data: { reason: "borrow_conflict" } }); + expect(spies.ungroup).not.toHaveBeenCalled(); + }); + it("refuses a group outside the Agent Window with permission_denied", async () => { const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); await sm.start("aa11"); diff --git a/apps/extension/src/tools/tabs.ts b/apps/extension/src/tools/tabs.ts index 201a0bc0..fac843c6 100644 --- a/apps/extension/src/tools/tabs.ts +++ b/apps/extension/src/tools/tabs.ts @@ -14,7 +14,7 @@ import { type SessionContext, type SessionManager, } from "@/session-manager/manager"; -import type { RpcError } from "@/transport/types"; +import type { RpcError, TransferCleanupState } from "@/transport/types"; import { rpcError } from "./errors"; import { type CdpRunner, cdpBlockedUrlReason, isRpcError, lookupSession } from "./shared"; @@ -1466,6 +1466,53 @@ async function authoriseAgentGroup( return group; } +/** + * Run `authoriseAgentTab` against every tab in `tabIds`, so that group + * operations can never affect a tab the direct-control tools would reject + * (remote-session ownership, cross-session borrows, Agent Window scope). + * Returns the first failure, or null when every tab is authorised. + */ +async function authoriseAgentTabIds( + manager: SessionManager, + ctx: SessionContext, + tabIds: number[], + api: TabMutationApi, + toolName: string, +): Promise { + for (const tabId of tabIds) { + const tabOrErr = await authoriseAgentTab(manager, ctx, tabId, api, toolName); + if (isRpcError(tabOrErr)) return tabOrErr; + } + return null; +} + +/** + * Authorise every tab currently in `groupId`. A group operation renames, + * recolors, collapses or ungroups *all* its members, so each member needs + * the same authorization as a direct call against it — an unowned or + * borrowed tab must not be affected through group operations. + */ +async function authoriseAgentGroupMembers( + manager: SessionManager, + ctx: SessionContext, + groupId: number, + tabsApi: TabMutationApi, + queryTabs: TabGroupMutationApi["queryTabs"], + toolName: string, +): Promise { + let members: chrome.tabs.Tab[]; + try { + members = await queryTabs({ windowId: ctx.agentWindowId, groupId }); + } catch (err) { + return { + code: "protocol_error", + message: err instanceof Error ? err.message : String(err), + }; + } + const memberIds = members.filter((t): t is CreatedChromeTab => typeof t.id === "number").map((t) => t.id); + return authoriseAgentTabIds(manager, ctx, memberIds, tabsApi, toolName); +} + /** Re-read a group's current title/color/collapsed state and member tabs. */ async function buildGroupInfo( groupsApi: ChromeTabGroupsApi, @@ -1504,6 +1551,137 @@ async function buildGroupInfo( }; } +/** + * Ensure a freshly created group lives in the session's Agent Window, with + * a bounded recovery ladder. Returns the group ID that is actually valid + * afterwards — it may not be the ID `tabs.group` returned — or an RpcError. + * + * Recovery stages, verified against real Chromium during review of #339: + * + * 1. Nothing to do: the group already reports the Agent Window. + * 2. Move the whole group (`tabGroups.move`). Some hosts accept this + * silently without relocating anything, which is why it cannot be + * trusted without re-checking. + * 3. Move each member tab individually. In real Chromium a per-tab move + * across windows *drops the tab's group membership*, and once the last + * member leaves, the group itself is destroyed — so `groupId` must not + * be trusted afterwards. Verify that every tab actually landed in the + * Agent Window, then recreate the group there and use the new ID. + * + * Every stage re-verifies placement from the browser's own state instead + * of assuming the previous call worked, and cleanup (ungrouping stranded + * tabs) reports honestly whether it succeeded rather than being silently + * swallowed. + */ +async function placeNewGroupInAgentWindow( + deps: TabManagementDeps, + ctx: SessionContext, + tabIds: number[], + groupId: number, + signal: AbortSignal | undefined, +): Promise { + const toolName = "tab_group_create"; + const tabsApi = getTabsApi(deps); + const groupsApi = getTabGroupsApi(deps); + const groupMutation = getTabGroupMutationApi(deps); + + const groupIsPlaced = async (gid: number): Promise => { + try { + return (await groupsApi.get(gid)).windowId === ctx.agentWindowId; + } catch { + // The group may no longer exist at all — e.g. Chromium destroyed it + // when the last member was moved out during stage 3. + return false; + } + }; + const landedTabs = async (ids: number[]): Promise => { + const landed: number[] = []; + for (const id of ids) { + try { + if ((await tabsApi.get(id)).windowId === ctx.agentWindowId) landed.push(id); + } catch { + // Tab closed mid-relocation: it certainly did not land. + } + } + return landed; + }; + /** Best-effort membership cleanup; the outcome is surfaced, never hidden. */ + const cleanupStrandedTabs = async (): Promise => { + try { + await groupMutation.ungroup(tabIds); + return "complete"; + } catch { + return "failed"; + } + }; + const mismatch = (message: string, cleanup?: TransferCleanupState): RpcError => + rpcError("cdp_failed", "group_window_mismatch", message, { + ...(cleanup ? { cleanup_state: cleanup } : {}), + }); + /** Cancel mid-recovery: still ungroup whatever is stranded, then stop. */ + const cancelledWithCleanup = async (): Promise => { + const cleanup = await cleanupStrandedTabs(); + return { code: "cancelled", message: `${toolName} aborted`, data: { cleanup_state: cleanup } }; + }; + + // Stage 1 — asked for this window at creation time. + if (await groupIsPlaced(groupId)) return groupId; + + // Stage 2 — relocate the whole group. Harmless when the host silently + // no-ops it; placement is re-verified rather than assumed. + if (aborted(signal, toolName)) return cancelledWithCleanup(); + await groupsApi.move(groupId, { windowId: ctx.agentWindowId, index: -1 }).catch(() => undefined); + if (await groupIsPlaced(groupId)) return groupId; + + // Stage 3 — per-tab relocation. The old group ID is dead once the last + // member crosses windows (see doc comment), so treat it as unusable. + for (const tabId of tabIds) { + if (aborted(signal, toolName)) return cancelledWithCleanup(); + try { + await tabsApi.move(tabId, { windowId: ctx.agentWindowId, index: -1 }); + } catch { + // Verification below decides the outcome; keep trying the rest. + } + } + // Re-grouping below is a mutation: a cancellation that arrived during + // the moves must stop here, after recovering stranded memberships. + if (aborted(signal, toolName)) return cancelledWithCleanup(); + const landed = await landedTabs(tabIds); + if (landed.length < tabIds.length) { + const cleanup = await cleanupStrandedTabs(); + return mismatch( + `tab_group_create: only ${landed.length} of ${tabIds.length} requested tabs reached ` + + `Agent Window ${ctx.agentWindowId} after relocation was attempted ` + + `(${tabIds.filter((id) => !landed.includes(id)).join(", ")} did not land); ` + + "the tabs remain open" + + (cleanup === "complete" ? ", ungrouped" : `, ungrouping failed (cleanup_state: ${cleanup})`), + cleanup, + ); + } + + // Recreate the group now that every member is back in the Agent Window, + // and verify the recreation itself actually landed there. + let recreatedGroupId: number; + try { + recreatedGroupId = await groupMutation.group({ + tabIds, + createProperties: { windowId: ctx.agentWindowId }, + }); + } catch (err) { + return { code: "protocol_error", message: err instanceof Error ? err.message : String(err) }; + } + if (!(await groupIsPlaced(recreatedGroupId))) { + const cleanup = await cleanupStrandedTabs(); + return mismatch( + "tab_group_create: the browser did not keep the recreated group in the Agent Window " + + `${ctx.agentWindowId}; the tabs remain open` + + (cleanup === "complete" ? ", ungrouped" : `, ungrouping failed (cleanup_state: ${cleanup})`), + cleanup, + ); + } + return recreatedGroupId; +} + function validateTabGroupCreateParams(params: TabGroupCreateParams): RpcError | null { if (!Array.isArray(params.tab_ids) || params.tab_ids.length === 0) { return { @@ -1551,20 +1729,35 @@ export async function handleTabGroupCreate( } const tabsApi = getTabsApi(deps); - for (const tabId of params.tab_ids) { - const tabOrErr = await authoriseAgentTab(manager, ctx, tabId, tabsApi, "tab_group_create"); - if (isRpcError(tabOrErr)) return tabOrErr; - } const groupsApi = getTabGroupsApi(deps); + const groupMutation = getTabGroupMutationApi(deps); + + // Authorise every tab the request names... + const namedErr = await authoriseAgentTabIds(manager, ctx, params.tab_ids, tabsApi, "tab_group_create"); + if (namedErr) return namedErr; + // ...and, when adding to an existing group, every tab already in it: + // they will be renamed/re-grouped alongside, so each needs the same + // authorization a direct tab_select against it would require. Without + // this, an unowned user tab or another session's borrowed tab could be + // affected through group operations while `tab_select` rejects direct + // control of the same tab. if (params.group_id !== undefined) { const groupOrErr = await authoriseAgentGroup(ctx, params.group_id, groupsApi, "tab_group_create"); if (isRpcError(groupOrErr)) return groupOrErr; + const membersErr = await authoriseAgentGroupMembers( + manager, + ctx, + params.group_id, + tabsApi, + groupMutation.queryTabs, + "tab_group_create", + ); + if (membersErr) return membersErr; } if (aborted(deps.signal, "tab_group_create")) { return { code: "cancelled", message: "tab_group_create aborted" }; } - const groupMutation = getTabGroupMutationApi(deps); let groupId: number; try { groupId = await groupMutation.group({ @@ -1585,23 +1778,23 @@ export async function handleTabGroupCreate( // freshly created group (and the tabs riding along with it) in a // different window than `createProperties.windowId` requested — most // reliably reproduced when the Agent Window is unfocused (the default - // for `session start --no-focus`). Try relocating the group, then each - // member tab individually, before giving up — never silently hand tabs - // to the user's regular browsing session (design §6 sandbox rule). + // for `session start --no-focus`). Run the bounded recovery ladder — + // never silently hand tabs to the user's regular browsing session + // (design §6 sandbox rule). if (params.group_id === undefined) { - const misplaced = async () => (await groupsApi.get(groupId)).windowId !== ctx.agentWindowId; - try { - if (await misplaced()) { - await groupsApi.move(groupId, { windowId: ctx.agentWindowId, index: -1 }).catch(() => {}); - if (await misplaced()) { - for (const tabId of params.tab_ids) { - await tabsApi.move(tabId, { windowId: ctx.agentWindowId, index: -1 }); - } - } - } - } catch (err) { - return { code: "protocol_error", message: err instanceof Error ? err.message : String(err) }; - } + const placedOrErr = await placeNewGroupInAgentWindow( + deps, + ctx, + params.tab_ids, + groupId, + deps.signal, + ); + if (isRpcError(placedOrErr)) return placedOrErr; + groupId = placedOrErr; + } + + if (aborted(deps.signal, "tab_group_create")) { + return { code: "cancelled", message: "tab_group_create aborted" }; } if (params.title !== undefined || params.color !== undefined) { @@ -1618,20 +1811,17 @@ export async function handleTabGroupCreate( const infoOrErr = await buildGroupInfo(groupsApi, groupMutation.queryTabs, ctx.agentWindowId, groupId); if (isRpcError(infoOrErr)) return infoOrErr; - // Every relocation attempt above ran and the group's own metadata may - // still claim the Agent Window, but the only signal worth trusting is - // which requested tabs `buildGroupInfo` can actually see there. If none - // made it, don't report a hollow success: undo the stray group so it - // doesn't linger in the user's regular browsing session, and say so. - const requested = new Set(params.tab_ids); - const landed = infoOrErr.tab_ids.filter((id) => requested.has(id)); - if (landed.length === 0 && params.group_id === undefined) { - await groupMutation.ungroup(params.tab_ids).catch(() => {}); + // Every relocation attempt above ran, but the only signal worth trusting + // is which requested tabs are actually members of the group the browser + // reports now. Anything less than all of them is not a success. + const missing = params.tab_ids.filter((id) => !infoOrErr.tab_ids.includes(id)); + if (missing.length > 0) { return rpcError( "cdp_failed", "group_window_mismatch", - "tab_group_create: the browser did not keep the new group in the Agent Window " + - `${ctx.agentWindowId} after relocation was attempted; the tabs remain open, ungrouped`, + `tab_group_create: tab(s) ${missing.join(", ")} did not join group ${groupId} ` + + `in Agent Window ${ctx.agentWindowId}; the group's current members are ` + + `${infoOrErr.tab_ids.join(", ") || "(none)"}`, ); } return { @@ -1669,6 +1859,17 @@ export async function handleTabGroupUpdate( const groupsApi = getTabGroupsApi(deps); const groupOrErr = await authoriseAgentGroup(ctx, params.group_id, groupsApi, "tab_group_update"); if (isRpcError(groupOrErr)) return groupOrErr; + // Renaming/recoloring/collapsing affects every member, so all of them + // need the same authorization a direct control call would require. + const membersErr = await authoriseAgentGroupMembers( + manager, + ctx, + params.group_id, + getTabsApi(deps), + getTabGroupMutationApi(deps).queryTabs, + "tab_group_update", + ); + if (membersErr) return membersErr; if (aborted(deps.signal, "tab_group_update")) { return { code: "cancelled", message: "tab_group_update aborted" }; } @@ -1759,6 +1960,16 @@ export async function handleTabGroupUngroup( return { code: "protocol_error", message: err instanceof Error ? err.message : String(err) }; } const tabIds = tabs.filter((t): t is CreatedChromeTab => typeof t.id === "number").map((t) => t.id); + // Ungrouping affects every member: authorise them the same way direct + // control would before touching any of them. + const membersErr = await authoriseAgentTabIds( + manager, + ctx, + tabIds, + getTabsApi(deps), + "tab_group_ungroup", + ); + if (membersErr) return membersErr; if (aborted(deps.signal, "tab_group_ungroup")) { return { code: "cancelled", message: "tab_group_ungroup aborted" }; } diff --git a/apps/extension/src/transport/types.ts b/apps/extension/src/transport/types.ts index 37c01491..18825afc 100644 --- a/apps/extension/src/transport/types.ts +++ b/apps/extension/src/transport/types.ts @@ -28,6 +28,7 @@ export type RpcErrorReason = | "ui_deadline" | "preview_busy" | "agent_window_scope" + | "group_window_mismatch" | "element_not_visible" | "input_not_ready" | "input_outcome_unknown" diff --git a/crates/bsk-cli/skill/references/tabs-and-profiles.md b/crates/bsk-cli/skill/references/tabs-and-profiles.md index 0081e350..ed2a11cf 100644 --- a/crates/bsk-cli/skill/references/tabs-and-profiles.md +++ b/crates/bsk-cli/skill/references/tabs-and-profiles.md @@ -113,10 +113,21 @@ bsk tab group ungroup --session Only tabs already inside the session's Agent Window (own tab or borrowed tab) can be grouped — same sandbox rule as `tab close` / `tab select`; a tab outside it is -rejected with `permission_denied`. `list` and `ungroup` are scoped to the Agent +rejected with `permission_denied`. A group operation touches *every* member, so +all members must satisfy those same rules: a group containing an unowned tab or +another session's borrowed tab is rejected outright, even when the request only +names tabs the session owns. `list` and `ungroup` are scoped to the Agent Window's own groups the same way. `ungroup` removes tabs from the group without closing them. +If the browser places a newly created group outside the Agent Window, the +handler relocates it (group move, then per-tab moves plus an in-place group +recreation), re-verifying placement after each step. If no step lands every +requested tab in the Agent Window, the call fails with `group_window_mismatch`, +reports which tabs did not land, and states whether the stranded tabs were +ungrouped (`cleanup_state`) — it never leaves a stray group in the user's +regular browsing session while claiming success. + **Choose `--title` from what the tabs are actually about, never a placeholder.** Read the grouped tabs' titles/URLs first (`bsk tab list --scope agent`) and name the group after the real topic — e.g. `AI Security Research` for a set of OWASP