From 3d050e0f2609e1b0eff840e995ed7698935c208d Mon Sep 17 00:00:00 2001 From: BB-fat <1056871944@qq.com> Date: Fri, 14 Aug 2026 04:12:38 +0000 Subject: [PATCH] =?UTF-8?q?feat(dsh-plugin-browserskill):=20strict=20owner?= =?UTF-8?q?ship=20boundary=20=E2=80=94=20foreign=20sessions=20are=20invisi?= =?UTF-8?q?ble?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tighten the shared-daemon boundary from 'cleanup only touches owned sessions' to 'the plugin only ever sees and operates on sessions it created': - browser_session_list no longer queries the daemon at all; it returns the plugin's own session table (with the current marker), so foreign sessions cannot even be enumerated through the plugin. - Every tool's optional session argument must name a plugin-created session; foreign/unknown ids are rejected with a clear error before any command reaches the daemon (the reference-adoption channel is gone). - Unchanged: dispose stops exactly the owned set, stop requires ownership, the start reservation protocol, and handle-precise child kills. Tests updated (59): explicit-foreign rejection without daemon contact, registry-only listing, reservation race, stale-handle-tolerant dispose. e2e: a manually started foreign session was rejected on reference, absent from session_list, refused on stop, and survived dispose while both plugin sessions were cleaned up. --- packages/dsh-plugin-browserskill/README.md | 10 ++- .../dsh-plugin-browserskill/src/sessions.ts | 82 +++++++++---------- packages/dsh-plugin-browserskill/src/tools.ts | 24 +++--- .../tests/dispose.test.ts | 6 +- .../tests/sessions.test.ts | 28 ++++--- .../tests/tools.test.ts | 55 ++++++++----- 6 files changed, 113 insertions(+), 92 deletions(-) diff --git a/packages/dsh-plugin-browserskill/README.md b/packages/dsh-plugin-browserskill/README.md index f47a4fe..2ca6592 100644 --- a/packages/dsh-plugin-browserskill/README.md +++ b/packages/dsh-plugin-browserskill/README.md @@ -12,8 +12,8 @@ extension keep owning the actual browser control — this package is a thin, wel | Tool | bsk command | Purpose | | --- | --- | --- | | `browser_session_start` | `bsk session start` | Open an Agent Window session; optional initial URL, window size, and device emulation preset. Returns the session id and makes it the current session. | -| `browser_session_stop` | `bsk session stop` | Stop a session (the current one by default) and close its Agent Window. | -| `browser_session_list` | `bsk session list` | List daemon-side sessions, marking the plugin's current session. | +| `browser_session_stop` | `bsk session stop` | Stop a session (the current one by default) and close its Agent Window. Only plugin-created sessions can be stopped. | +| `browser_session_list` | — (registry only) | List the sessions this plugin created, marking the current one. Foreign daemon sessions are never shown. | | `browser_navigate` | `bsk navigate` | Navigate the active tab, with `waitUntil` / timeout control. | | `browser_snapshot` | `bsk snapshot` | Indented aria-tree snapshot with `@eN` refs for interaction tools. | | `browser_observe` | `bsk observe` | Semantic VOM observation (read-only) with `@eN` refs. | @@ -34,6 +34,12 @@ One agent conversation can drive several browser sessions at once: - The number of concurrent sessions started through the plugin is capped (`maxSessions`, default 5). - Unloading the plugin stops every session it started and kills in-flight bsk processes. +**Ownership boundary**: the bsk daemon may be shared with other agents, terminals, or dsh +instances. The plugin therefore only ever sees and operates on sessions it created itself — +an explicit `session` argument naming a foreign or unknown id is rejected, `browser_session_list` +shows plugin-created sessions only (no daemon-wide view), and stop/unload cleanup can never touch +a session owned by another program. + ## Installation The plugin follows the standard dsh bundle layout (`dsh.bundle` manifest + `cordis.patch.yml`): diff --git a/packages/dsh-plugin-browserskill/src/sessions.ts b/packages/dsh-plugin-browserskill/src/sessions.ts index 4650d9b..a9aca98 100644 --- a/packages/dsh-plugin-browserskill/src/sessions.ts +++ b/packages/dsh-plugin-browserskill/src/sessions.ts @@ -1,23 +1,23 @@ /** - * Tracks the bsk sessions started through this plugin and the "current" - * session pointer used when a tool call omits its optional `session` arg. - * The pointer moves to whatever session was most recently started, stopped, - * or operated on, so a model can run several browsers side by side without - * repeating the id on every call. + * Tracks the bsk sessions this plugin created and the "current" session + * pointer used when a tool call omits its optional `session` arg. The pointer + * moves to whatever session was most recently started, stopped, or operated + * on, so a model can run several browsers side by side without repeating the + * id on every call. * - * Ownership discipline (the bsk daemon may be shared with other agents, - * terminals, or dsh instances): a session is **owned** only when it was - * created by this plugin's browser_session_start; an explicit `session` - * argument naming a foreign session is tracked as a **reference** so the - * current-session pointer keeps working, but referenced sessions are never - * stopped — not by browser_session_stop and not by unload cleanup. + * Strict ownership boundary (the bsk daemon may be shared with other agents, + * terminals, or dsh instances): the registry ONLY ever holds sessions created + * by this plugin's browser_session_start. Tools cannot see or act on any + * other session — an explicit `session` argument naming a foreign id is an + * error, the list tool shows owned sessions only, and stop/unload cleanup + * can never touch a session this plugin did not create. */ export interface TrackedSession { sessionId: string; browserInstanceId?: string; startedAtMs: number; - /** True only for sessions this plugin created (and therefore may stop). */ + /** Always true: only plugin-created sessions enter the registry at all. */ owned: boolean; } @@ -53,8 +53,8 @@ export class SessionRegistry { } /** - * Register a freshly started (owned) session, consuming its reservation, - * and make it current. + * Register a freshly started session, consuming its reservation, and make + * it current. */ completeStart(session: Omit): void { this.pendingStarts = Math.max(0, this.pendingStarts - 1); @@ -78,16 +78,12 @@ export class SessionRegistry { } } - /** Mark a session as most recently used. Unknown ids are adopted as references (never owned). */ - touch(sessionId: string): void { + /** Mark an owned session as most recently used (recency order refresh). */ + private touch(sessionId: string): void { const existing = this.sessions.get(sessionId); - if (existing !== undefined) { - // Re-insert to refresh recency order. - this.sessions.delete(sessionId); - this.sessions.set(sessionId, existing); - } else { - this.sessions.set(sessionId, { sessionId, startedAtMs: Date.now(), owned: false }); - } + if (existing === undefined) return; + this.sessions.delete(sessionId); + this.sessions.set(sessionId, existing); this.currentId = sessionId; } @@ -96,7 +92,7 @@ export class SessionRegistry { return this.currentId; } - /** Tracked sessions in least- to most-recently-used order. */ + /** Owned sessions in least- to most-recently-used order. */ list(): TrackedSession[] { return [...this.sessions.values()]; } @@ -108,7 +104,7 @@ export class SessionRegistry { .map((session) => session.sessionId); } - /** Whether the session was created by this plugin and may be stopped by it. */ + /** Whether the session was created by this plugin. */ isOwned(sessionId: string): boolean { return this.sessions.get(sessionId)?.owned === true; } @@ -117,22 +113,30 @@ export class SessionRegistry { return this.sessions.size; } + /** Shared not-yours error for foreign or unknown session ids. */ + private foreignError(sessionId: string, toolName: string): Error { + return new Error( + `${toolName}: session "${sessionId}" does not belong to this plugin — only sessions ` + + "created by browser_session_start are visible and operable here", + ); + } + /** - * Resolve the session an operation tool call acts on: an explicit `session` - * argument wins (and becomes current); otherwise fall back to the current - * session. Foreign ids are adopted as references. - * @throws when neither is available. + * Resolve the session a tool call acts on: an explicit `session` argument + * must name an owned session (and becomes current); omitted falls back to + * the current session. Foreign ids are rejected, never adopted. + * @throws on foreign/unknown ids, or when no session exists. */ resolve(explicit: string | undefined, toolName: string): string { if (explicit !== undefined && explicit.trim().length > 0) { + if (!this.isOwned(explicit)) throw this.foreignError(explicit, toolName); this.touch(explicit); return explicit; } const current = this.current(); if (current === undefined) { throw new Error( - `${toolName} needs a session but none is active — start one with browser_session_start ` + - "or pass an explicit `session` id", + `${toolName} needs a session but none is active — start one with browser_session_start`, ); } this.touch(current); @@ -140,26 +144,18 @@ export class SessionRegistry { } /** - * Resolve the session a STOP call acts on, without adopting foreign ids: - * only owned sessions may be stopped, and the current pointer is not moved - * by a rejected stop. - * @throws when the target is unknown, foreign, or no session exists. + * Resolve the session a STOP call acts on. Same ownership rule as every + * other tool; a rejected stop never moves the current pointer. */ resolveForStop(explicit: string | undefined): string { const candidate = explicit !== undefined && explicit.trim().length > 0 ? explicit : this.current(); if (candidate === undefined) { throw new Error( - "browser_session_stop needs a session but none is active — start one with browser_session_start " + - "or pass an explicit `session` id", - ); - } - if (!this.isOwned(candidate)) { - throw new Error( - `browser_session_stop refuses to stop session "${candidate}": it was not created by this plugin. ` + - "Only sessions returned by browser_session_start can be stopped (use `bsk session stop` yourself for others).", + "browser_session_stop needs a session but none is active — start one with browser_session_start", ); } + if (!this.isOwned(candidate)) throw this.foreignError(candidate, "browser_session_stop"); return candidate; } } diff --git a/packages/dsh-plugin-browserskill/src/tools.ts b/packages/dsh-plugin-browserskill/src/tools.ts index cc16cbe..869789f 100644 --- a/packages/dsh-plugin-browserskill/src/tools.ts +++ b/packages/dsh-plugin-browserskill/src/tools.ts @@ -44,7 +44,8 @@ const DEVICE_PRESETS = [ const SESSION_PARAM = { type: "string", description: - "bsk session id to act on. Omit to use the current session (the one most recently started or used).", + "bsk session id to act on; must be one created by this plugin's browser_session_start. " + + "Omit to use the current session (the one most recently started or used).", } as const; /** Run one bsk command and return its parsed JSON payload, or throw. */ @@ -266,8 +267,9 @@ export function registerTools(deps: ToolDeps): void { defineTool({ name: "browser_session_list", description: - "List active browser sessions known to the bsk daemon. The session marked `current` is " + - "the one browser_* tools act on when no explicit `session` is passed.", + "List the browser sessions created by this plugin. Sessions owned by other programs on the " + + "shared bsk daemon are not visible here. The session marked `current` is the one browser_* " + + "tools act on when no explicit `session` is passed.", parameters: {}, output: { schema: { @@ -305,17 +307,15 @@ export function registerTools(deps: ToolDeps): void { ], }, isConcurrencySafe: () => true, - async execute(_args, exec) { - const reply = (await runBsk(deps, exec, ["session", "list"], "session list")) as { - session_id: string; - browser_instance_id: string; - }[]; + // Registry-only by design: no daemon call, so foreign sessions on a + // shared daemon can never even be SEEN through this tool. + async execute() { const current = registry.current(); return { - sessions: reply.map((entry) => ({ - sessionId: entry.session_id, - browserInstanceId: entry.browser_instance_id, - current: entry.session_id === current, + sessions: registry.list().map((entry) => ({ + sessionId: entry.sessionId, + browserInstanceId: entry.browserInstanceId ?? "", + current: entry.sessionId === current, })), }; }, diff --git a/packages/dsh-plugin-browserskill/tests/dispose.test.ts b/packages/dsh-plugin-browserskill/tests/dispose.test.ts index 0962fbb..0163d05 100644 --- a/packages/dsh-plugin-browserskill/tests/dispose.test.ts +++ b/packages/dsh-plugin-browserskill/tests/dispose.test.ts @@ -75,8 +75,10 @@ describe("dispose cleanup ownership", () => { if (start === undefined || snapshot === undefined) throw new Error("tools not registered"); await start.execute({}, makeExec()); await start.execute({}, makeExec()); - // A foreign session gets referenced (current-pointer bookkeeping) but never owned. - await snapshot.execute({ session: "ext9" }, makeExec()); + // A foreign session cannot even be referenced any more. + await expect(snapshot.execute({ session: "ext9" }, makeExec())).rejects.toThrow( + /does not belong to this plugin/, + ); await Promise.all(disposers.map((dispose) => dispose())); diff --git a/packages/dsh-plugin-browserskill/tests/sessions.test.ts b/packages/dsh-plugin-browserskill/tests/sessions.test.ts index 63d8a50..b5f2512 100644 --- a/packages/dsh-plugin-browserskill/tests/sessions.test.ts +++ b/packages/dsh-plugin-browserskill/tests/sessions.test.ts @@ -45,13 +45,22 @@ describe("SessionRegistry", () => { expect(registry.ownedIds()).toEqual(["s1"]); }); - it("resolve prefers the explicit session and adopts it as a reference", () => { + it("resolve accepts an explicit owned session and makes it current", () => { const registry = new SessionRegistry(5); start(registry, "s1"); - expect(registry.resolve("foreign", "tool")).toBe("foreign"); - expect(registry.current()).toBe("foreign"); + start(registry, "s2"); + expect(registry.resolve("s1", "tool")).toBe("s1"); + expect(registry.current()).toBe("s1"); + }); + + it("resolve rejects foreign ids and never adopts them", () => { + const registry = new SessionRegistry(5); + start(registry, "s1"); + expect(() => registry.resolve("foreign", "tool")).toThrow(/does not belong to this plugin/); expect(registry.isOwned("foreign")).toBe(false); expect(registry.ownedIds()).toEqual(["s1"]); + // A rejected resolve must not move the current pointer. + expect(registry.current()).toBe("s1"); }); it("resolve falls back to the current session", () => { @@ -65,11 +74,11 @@ describe("SessionRegistry", () => { expect(() => registry.resolve(undefined, "browser_click")).toThrow(/browser_session_start/); }); - it("touch refreshes recency so fallback picks the most recent survivor", () => { + it("recency order survives remove of the current session", () => { const registry = new SessionRegistry(5); start(registry, "s1"); start(registry, "s2"); - registry.touch("s1"); + registry.resolve("s1", "tool"); registry.remove("s1"); expect(registry.current()).toBe("s2"); }); @@ -79,9 +88,8 @@ describe("SessionRegistry", () => { start(registry, "s1"); expect(registry.resolveForStop(undefined)).toBe("s1"); expect(registry.resolveForStop("s1")).toBe("s1"); - registry.resolve("foreign", "browser_snapshot"); - expect(() => registry.resolveForStop("foreign")).toThrow(/not created by this plugin/); - expect(() => registry.resolveForStop("never-seen")).toThrow(/not created by this plugin/); + expect(() => registry.resolveForStop("foreign")).toThrow(/does not belong to this plugin/); + expect(() => registry.resolveForStop("never-seen")).toThrow(/does not belong to this plugin/); }); it("a refused stop does not move the current pointer", () => { @@ -91,12 +99,10 @@ describe("SessionRegistry", () => { expect(registry.current()).toBe("s1"); }); - it("ownedIds excludes referenced sessions even after heavy interleaving", () => { + it("ownedIds always equals every registered session (foreign ids never enter)", () => { const registry = new SessionRegistry(5); start(registry, "own1"); - registry.resolve("ext1", "tool"); start(registry, "own2"); - registry.resolve("ext2", "tool"); expect(registry.ownedIds().sort()).toEqual(["own1", "own2"]); }); }); diff --git a/packages/dsh-plugin-browserskill/tests/tools.test.ts b/packages/dsh-plugin-browserskill/tests/tools.test.ts index eb2581c..8442c6f 100644 --- a/packages/dsh-plugin-browserskill/tests/tools.test.ts +++ b/packages/dsh-plugin-browserskill/tests/tools.test.ts @@ -208,16 +208,31 @@ describe("multi-session behavior", () => { expect(calls[1].args).toEqual(["snapshot", "--session", "s1"]); }); - it("an explicit session wins and becomes the new current session", async () => { - const { tools, calls, registry } = setup(responses); - await startSession(tools); + it("an explicit owned session wins and becomes the new current session", async () => { + const { tools, calls, registry } = setup({ + "session start": seq([START_REPLY("s1"), START_REPLY("s2")]), + snapshot: SNAPSHOT_REPLY, + }); + await startSession(tools, "s1"); + await startSession(tools, "s2"); const snapshot = tools.get("browser_snapshot"); - const value = (await snapshot?.execute({ session: "other" }, makeExec())) as { + const value = (await snapshot?.execute({ session: "s1" }, makeExec())) as { session: string; }; - expect(value.session).toBe("other"); - expect(calls[1].args).toEqual(["snapshot", "--session", "other"]); - expect(registry.current()).toBe("other"); + expect(value.session).toBe("s1"); + expect(calls[2].args).toEqual(["snapshot", "--session", "s1"]); + expect(registry.current()).toBe("s1"); + }); + + it("rejects an explicit foreign session without touching the daemon", async () => { + const { tools, calls } = setup(responses); + await startSession(tools); + const callsBefore = calls.length; + const snapshot = tools.get("browser_snapshot"); + await expect(snapshot?.execute({ session: "other" }, makeExec())).rejects.toThrow( + /does not belong to this plugin/, + ); + expect(calls.length).toBe(callsBefore); }); it("fails with guidance when no session exists", async () => { @@ -263,12 +278,9 @@ describe("multi-session behavior", () => { it("refuses to stop a session the plugin did not create", async () => { const { tools, calls } = setup(responses); await startSession(tools); - // Reference a foreign session (adopted for the current pointer, never owned). - const snapshot = tools.get("browser_snapshot"); - await snapshot?.execute({ session: "ext7" }, makeExec()); const stop = tools.get("browser_session_stop"); await expect(stop?.execute({ session: "ext7" }, makeExec())).rejects.toThrow( - /not created by this plugin/, + /does not belong to this plugin/, ); // No stop command may reach the daemon for a foreign session. expect(calls.some((c) => c.args.join(" ").startsWith("session stop"))).toBe(false); @@ -289,23 +301,22 @@ describe("browser_session_stop / list", () => { expect(registry.current()).toBeUndefined(); }); - it("lists daemon sessions with the current marker", async () => { - const { tools } = setup({ - "session start": START_REPLY("s1"), - "session list": [ - { session_id: "s1", browser_instance_id: "chrome-1" }, - { session_id: "s2", browser_instance_id: "chrome-1" }, - ], + it("lists only plugin-owned sessions with the current marker (no daemon view)", async () => { + const { tools, calls } = setup({ + "session start": seq([START_REPLY("s1"), START_REPLY("s2")]), }); await startSession(tools); + await startSession(tools); const list = tools.get("browser_session_list"); const value = (await list?.execute({}, makeExec())) as { sessions: { sessionId: string; current: boolean }[]; }; expect(value.sessions).toEqual([ - { sessionId: "s1", browserInstanceId: "chrome-1", current: true }, - { sessionId: "s2", browserInstanceId: "chrome-1", current: false }, + { sessionId: "s1", browserInstanceId: "chrome-1", current: false }, + { sessionId: "s2", browserInstanceId: "chrome-1", current: true }, ]); + // Registry-only: listing must not call the daemon at all. + expect(calls.some((c) => c.args.join(" ").startsWith("session list"))).toBe(false); }); }); @@ -533,8 +544,8 @@ describe("error and cancellation semantics", () => { killAll() {}, }; registerTools({ ctx: ctx as never, runner, registry, config: CONFIG }); - const list = tools.get("browser_session_list"); - await expect(list?.execute({}, makeExec())).rejects.toThrow(/BrowserSkill must be installed/); + const start = tools.get("browser_session_start"); + await expect(start?.execute({}, makeExec())).rejects.toThrow(/BrowserSkill must be installed/); }); });