feat(dsh-plugin-browserskill): strict ownership boundary — foreign sessions are invisible

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

@@ -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`):

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<TrackedSession, "owned">): 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;
   }
 }

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,
           })),
         };
       },

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()));
 

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"]);
   });
 });

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/);
   });
 });