fix(dsh-plugin-browserskill): address review findings (leaks, trust fence, queue race)

Blocking:
- B1: delete screenshot scratch PNGs — observation frames reuse one fixed
  per-session path and unlink after every read (finally); browser_screenshot
  unlinks once the bytes are in the attachment store (kept only when the file
  itself is the model-facing artifact).
- B2: replicate dsh's browser-trust fence on /bsk-observation/*: loopback
  Host only, Origin must match Host, sec-fetch-site: cross-site refused, POST
  requires application/json; README documents the loopback trust premise and
  the 0.0.0.0 warning.

Major:
- M1: KeyedExecutor tail chains previous+task (allSettled) so a task aborted
  while queued cannot release the next one into the running session.
- M2: client tracks the live frame per session — replacing/removing/reset
  revokes the old blob URL at once; loads settling after replacement never
  resurrect it.

Minor: drop replaced thumbRefs (+on remove); beginAction fires inside the
queue (no label overwrite while queued, no idle flash); half-initialized
session cleanup stops through the queue with one retry; SSE (re)open refetches
/state (+ snapshot flag renamed subscribed); install probe uses --version (no
daemon spawn); emulate mobile documents the width+height requirement (the
daemon refuses it alone — verified).

Nit: single BskRunOptions declaration; tails map entry dropped on drain; SSE
cleanup on res close; popOut catches requestWindow rejections; resize handle
gains keyboard control (arrows, 16px steps, aria value attrs).

Tests: +14 (fence rules, scratch lifecycle, queue race, blob revocation,
instrumentation timing, SSE resync). 113/113 green.

Author: BB-fat

packages/dsh-plugin-browserskill/README.md

@@ -107,6 +107,14 @@ shell's theme cannot bleed back in.
   the dsh `webServer` route seam (dsh 0.1's Typert Remote pipeline is closed to out-of-tree
   packages). All commands for one session — tool calls and frame captures alike — run through a
   per-session FIFO, because the daemon accepts only one unfinished command per session.
+- **Trust model**: these routes expose live screenshots (and an interrupt write), so they
+  replicate the browser-trust fence dsh applies to its own `/api` routes: the request Host
+  must be a loopback authority (`localhost`, `127.0.0.0/8`, `[::1]`), a present Origin must
+  match the Host, `sec-fetch-site: cross-site` is refused, and POST requires an
+  `application/json` body (cross-site simple requests can never satisfy that). The channel is
+  therefore built for **loopback-only serving** — binding the dsh web server to `0.0.0.0` and
+  reaching it through a LAN address will (deliberately) fail the fence; do not put these
+  routes behind a non-loopback reverse proxy without adding your own authentication.
 - Configure with `observationEnabled` / `thumbnailIntervalMs` / `idleIntervalMs`.
 
 ## Behavior notes

packages/dsh-plugin-browserskill/src/client/ObservationOverlay.tsx

@@ -434,10 +434,15 @@ export function ObservationOverlay({ store }: { store: ObservationClientStore })
   const popOut = async (): Promise<void> => {
     const pip = pipApi();
     if (pip === undefined) return;
-    const win = await pip.requestWindow({ width: size.w, height: size.h });
-    cloneStylesInto(win);
-    win.addEventListener("pagehide", () => setPipWindow(null));
-    setPipWindow(win);
+    try {
+      const win = await pip.requestWindow({ width: size.w, height: size.h });
+      cloneStylesInto(win);
+      win.addEventListener("pagehide", () => setPipWindow(null));
+      setPipWindow(win);
+    } catch {
+      // requestWindow rejects without a user gesture (or when the window was
+      // closed mid-request): stay on the in-page card, no state change.
+    }
   };
 
   // Hidden while no owned session exists (and no PiP is up).
@@ -496,7 +501,28 @@ export function ObservationOverlay({ store }: { store: ObservationClientStore })
         data-testid="obs-resize"
         aria-label="Resize overlay"
         role="separator"
+        aria-valuenow={size.w}
+        aria-valuetext={`${Math.round(size.w)} by ${Math.round(size.h)} pixels`}
+        aria-valuemin={MIN_SIZE.w}
+        aria-valuemax={Math.round(viewport().w * 0.8)}
+        tabIndex={0}
         onPointerDown={beginDrag("resize")}
+        onKeyDown={(event) => {
+          const step = 16;
+          const delta =
+            event.key === "ArrowRight"
+              ? { w: step, h: 0 }
+              : event.key === "ArrowLeft"
+                ? { w: -step, h: 0 }
+                : event.key === "ArrowDown"
+                  ? { w: 0, h: step }
+                  : event.key === "ArrowUp"
+                    ? { w: 0, h: -step }
+                    : undefined;
+          if (delta === undefined) return;
+          event.preventDefault();
+          setSize(clampSize({ w: size.w + delta.w, h: size.h + delta.h }, viewport()));
+        }}
       />
     </div>
   );

packages/dsh-plugin-browserskill/src/client/observation-store.ts

@@ -1,14 +1,17 @@
 /**
  * Client-side data layer for the observation overlay: initial state fetch,
- * SSE increment stream, on-demand thumbnail blob loading through the
- * session-authorized readAttachment path, and the interrupt call. All I/O is
- * injected so tests never touch a network.
+ * SSE increment stream (with a state refetch on every (re)open — events that
+ * fired while the stream was down are otherwise lost forever), on-demand
+ * thumbnail blob loading through the session-authorized readAttachment path,
+ * and the interrupt call. All I/O is injected so tests never touch a network.
  */
 
 import type { ObservationEvent, SessionObservation } from "../observation";
 
 export interface EventSourceLike {
   onmessage: ((event: { data: string }) => void) | null;
+  /** Fires on the initial connect AND on every automatic reconnect. */
+  onopen?: (() => void) | null;
   close(): void;
 }
 
@@ -32,7 +35,8 @@ export interface ThumbnailState {
 
 export interface OverlaySnapshot {
   readonly sessions: readonly SessionObservation[];
-  readonly connected: boolean;
+  /** Whether the SSE increment stream is currently subscribed. */
+  readonly subscribed: boolean;
   readonly thumbnails: Readonly<Record<string, ThumbnailState>>;
   /** False when the host reports the browser/daemon as unreachable. */
   readonly available: boolean;
@@ -42,14 +46,22 @@ const STATE_URL = "/bsk-observation/state";
 const EVENTS_URL = "/bsk-observation/events";
 const INTERRUPT_URL = "/bsk-observation/interrupt";
 
+function revoke(url: string | undefined): void {
+  if (url !== undefined && typeof URL.revokeObjectURL === "function") {
+    URL.revokeObjectURL(url);
+  }
+}
+
 export class ObservationClientStore {
   private sessions = new Map<string, SessionObservation>();
   private thumbs = new Map<string, ThumbnailState>();
+  /** sessionId → the attachment id currently displayed for it (leak guard). */
+  private readonly thumbBySession = new Map<string, string>();
   private listeners = new Set<() => void>();
   private events: EventSourceLike | undefined;
   private snapshot: OverlaySnapshot = {
     sessions: [],
-    connected: false,
+    subscribed: false,
     thumbnails: {},
     available: true,
   };
@@ -68,17 +80,15 @@ export class ObservationClientStore {
   private publish(): void {
     this.snapshot = {
       sessions: [...this.sessions.values()],
-      connected: this.events !== undefined,
+      subscribed: this.events !== undefined,
       thumbnails: Object.fromEntries(this.thumbs),
       available: this.available,
     };
     for (const listener of [...this.listeners]) listener();
   }
 
-  /** Initial fetch + SSE subscription. Idempotent. */
-  start(): void {
-    if (this.started) return;
-    this.started = true;
+  /** (Re)pull the full state: initial load and every SSE (re)open. */
+  private refreshState(): void {
     void this.deps
       .fetchFn(STATE_URL)
       .then(async (res) => {
@@ -87,11 +97,25 @@ export class ObservationClientStore {
           sessions?: SessionObservation[];
           available?: boolean;
         };
-        this.sessions = new Map((body.sessions ?? []).map((s) => [s.sessionId, s]));
+        const sessions = body.sessions ?? [];
+        this.sessions = new Map(sessions.map((s) => [s.sessionId, s]));
+        // Prune thumbnails of sessions that vanished while we were away.
+        const alive = new Set(sessions.map((s) => s.sessionId));
+        for (const sessionId of [...this.thumbBySession.keys()]) {
+          if (!alive.has(sessionId)) this.dropThumb(sessionId);
+        }
+        for (const s of sessions) this.trackThumb(s.sessionId, s.thumbnailAttachmentId);
         if (typeof body.available === "boolean") this.available = body.available;
         this.publish();
       })
       .catch(() => {});
+  }
+
+  /** Initial fetch + SSE subscription. Idempotent. */
+  start(): void {
+    if (this.started) return;
+    this.started = true;
+    this.refreshState();
     const events = this.deps.eventSourceFactory(EVENTS_URL);
     events.onmessage = (message) => {
       let event: ObservationEvent;
@@ -102,6 +126,10 @@ export class ObservationClientStore {
       }
       this.apply(event);
     };
+    events.onopen = () => {
+      // Reconnects lose every event fired during the outage — resync.
+      if (this.events === events) this.refreshState();
+    };
     this.events = events;
     this.publish();
   }
@@ -110,23 +138,45 @@ export class ObservationClientStore {
     this.events?.close();
     this.events = undefined;
     this.started = false;
-    for (const thumb of this.thumbs.values()) {
-      if (thumb.url !== undefined && typeof URL.revokeObjectURL === "function") {
-        URL.revokeObjectURL(thumb.url);
-      }
-    }
+    for (const thumb of this.thumbs.values()) revoke(thumb.url);
     this.thumbs.clear();
+    this.thumbBySession.clear();
     this.sessions.clear();
     this.publish();
   }
 
+  /** Forget one session's tracked thumbnail, revoking its blob URL. */
+  private dropThumb(sessionId: string): void {
+    const attachmentId = this.thumbBySession.get(sessionId);
+    if (attachmentId === undefined) return;
+    this.thumbBySession.delete(sessionId);
+    revoke(this.thumbs.get(attachmentId)?.url);
+    this.thumbs.delete(attachmentId);
+  }
+
+  /**
+   * Track the frame a session currently shows; replacing a frame revokes the
+   * old blob URL immediately — blob URLs must not accumulate one per frame.
+   */
+  private trackThumb(sessionId: string, attachmentId: string | undefined): void {
+    const previous = this.thumbBySession.get(sessionId);
+    if (previous === attachmentId) return;
+    this.dropThumb(sessionId);
+    if (attachmentId !== undefined) this.thumbBySession.set(sessionId, attachmentId);
+  }
+
   private apply(event: ObservationEvent): void {
     if (event.type === "reset") {
       this.sessions.clear();
+      for (const thumb of this.thumbs.values()) revoke(thumb.url);
+      this.thumbs.clear();
+      this.thumbBySession.clear();
     } else if (event.type === "remove" && event.session !== undefined) {
       this.sessions.delete(event.session.sessionId);
+      this.dropThumb(event.session.sessionId);
     } else if (event.type === "upsert" && event.session !== undefined) {
       this.sessions.set(event.session.sessionId, event.session);
+      this.trackThumb(event.session.sessionId, event.session.thumbnailAttachmentId);
     } else if (event.type === "availability") {
       this.available = event.available;
     }
@@ -144,6 +194,11 @@ export class ObservationClientStore {
     this.publish();
     this.deps.loadImage(attachmentId).then(
       (url) => {
+        if (!this.thumbs.has(attachmentId)) {
+          // Replaced or removed while loading: never resurrect (or leak) it.
+          revoke(url);
+          return;
+        }
         this.thumbs.set(attachmentId, { status: "ready", url });
         this.publish();
       },

packages/dsh-plugin-browserskill/src/index.ts

@@ -90,8 +90,10 @@ export function apply(
   });
 
   // Non-blocking install probe: warn early when bsk is missing instead of
-  // failing the first tool call with a bare spawn error.
-  runner.run(["status"], { timeoutMs: 10_000 }).then(
+  // failing the first tool call with a bare spawn error. Uses --version on
+  // purpose — it answers without starting the daemon (`bsk status` would
+  // ensure-spawn one, an expensive side effect for a probe).
+  runner.run(["--version"], { timeoutMs: 10_000 }).then(
     () => {},
     (error: unknown) => {
       const detail = error instanceof Error ? error.message : String(error);

packages/dsh-plugin-browserskill/src/observation-http.ts

@@ -35,6 +35,56 @@ function sendJson(res: ServerResponse, status: number, body: unknown): void {
   res.end(JSON.stringify(body));
 }
 
+/**
+ * Browser-trust fence, mirroring the one dsh applies to its /api routes (ours
+ * live outside that prefix, so the checks are replicated here):
+ * - Host must be a loopback authority (localhost / 127.0.0.0/8 / [::1]) — the
+ *   observation channel exposes live screenshots and must never answer a LAN
+ *   or DNS-rebound name;
+ * - a present Origin must be same-host with the Host header (blocks cross-site
+ *   reads), and `sec-fetch-site: cross-site` is refused outright;
+ * - POST must be `application/json` — anything a cross-site *simple request*
+ *   can send (form/plain) never reaches the handler, which kills CSRF.
+ */
+function fenceViolation(req: IncomingMessage): string | undefined {
+  const host = req.headers.host ?? "";
+  const hostname = /^\[.*\](?::\d+)?$/.test(host)
+    ? host.slice(1, host.indexOf("]"))
+    : host.split(":")[0];
+  const isLoopback =
+    hostname === "localhost" ||
+    hostname.endsWith(".localhost") ||
+    hostname === "::1" ||
+    /^127\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(hostname);
+  if (!isLoopback) return "host is not a loopback authority";
+  const origin = req.headers.origin;
+  if (origin !== undefined && origin !== "null") {
+    let originHost: string | undefined;
+    try {
+      originHost = new URL(origin).host;
+    } catch {
+      return "unparseable Origin header";
+    }
+    if (originHost !== host) return "Origin does not match Host";
+  }
+  if (req.headers["sec-fetch-site"] === "cross-site") return "sec-fetch-site: cross-site";
+  if (req.method === "POST") {
+    const contentType = req.headers["content-type"] ?? "";
+    if (!/^\s*application\/json\s*(;|$)/.test(contentType)) {
+      return "POST requires an application/json body";
+    }
+  }
+  return undefined;
+}
+
+/** Run the fence; returns true when the request was rejected (handled). */
+function fenceRejected(req: IncomingMessage, res: ServerResponse): boolean {
+  const violation = fenceViolation(req);
+  if (violation === undefined) return false;
+  sendJson(res, 403, { error: `forbidden: ${violation}` });
+  return true;
+}
+
 /**
  * Register the observation routes. No-op (with a console note) when the
  * composition has no web server.
@@ -57,6 +107,7 @@ export function registerObservationRoutes(
           sendJson(res, 405, { error: "method not allowed" });
           return;
         }
+        if (fenceRejected(req, res)) return;
         sendJson(res, 200, {
           sessions: observation.getState(),
           available: observation.isAvailable(),
@@ -71,6 +122,7 @@ export function registerObservationRoutes(
           sendJson(res, 405, { error: "method not allowed" });
           return;
         }
+        if (fenceRejected(req, res)) return;
         res.writeHead(200, {
           "content-type": "text/event-stream",
           "cache-control": "no-cache",
@@ -80,7 +132,7 @@ export function registerObservationRoutes(
           res.write(`data: ${JSON.stringify(event)}\n\n`);
         });
         const heartbeat = setInterval(() => res.write(": heartbeat\n\n"), SSE_HEARTBEAT_MS);
-        req.on("close", () => {
+        res.on("close", () => {
           clearInterval(heartbeat);
           unsubscribe();
         });
@@ -94,6 +146,7 @@ export function registerObservationRoutes(
           sendJson(res, 405, { error: "method not allowed" });
           return;
         }
+        if (fenceRejected(req, res)) return;
         let body = "";
         req.on("data", (chunk: Buffer | string) => {
           body += chunk;
@@ -121,6 +174,7 @@ export function registerObservationRoutes(
           sendJson(res, 405, { error: "method not allowed" });
           return;
         }
+        if (fenceRejected(req, res)) return;
         const pathname = decodeURIComponent(new URL(req.url ?? "/", "http://x").pathname);
         const attachmentId = pathname.slice(`${ROUTE_BASE}/thumbnail/`.length);
         const frame = await observation.readThumbnail(attachmentId);