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.
This commit is contained in:
BB-fat
2026-08-14 11:56:02 +00:00
parent 52aa356153
commit a4e668542d
13 changed files with 784 additions and 104 deletions
@@ -111,3 +111,87 @@ describe("ObservationClientStore", () => {
expect(store.getSnapshot().sessions).toHaveLength(0);
});
});
describe("thumbnail blob URL lifecycle", () => {
function withRevokeSpy() {
const revoke = vi.fn();
(URL as unknown as { revokeObjectURL: unknown }).revokeObjectURL = revoke;
return revoke;
}
it("replacing a frame revokes the old blob URL and drops its entry", async () => {
const revoke = withRevokeSpy();
const { store, esInstances } = harness([{ ...OBS_IDLE, thumbnailAttachmentId: "a1" }]);
store.start();
await vi.waitFor(() => expect(store.getSnapshot().sessions).toHaveLength(1));
store.ensureThumbnail("a1");
await vi.waitFor(() =>
expect(store.getSnapshot().thumbnails["a1"]).toEqual({ status: "ready", url: "blob:url-a1" }),
);
emit(esInstances[0], {
type: "upsert",
session: { ...OBS_IDLE, thumbnailAttachmentId: "a2" },
});
expect(store.getSnapshot().thumbnails["a1"]).toBeUndefined();
expect(revoke).toHaveBeenCalledWith("blob:url-a1");
store.stop();
});
it("remove and reset revoke the session's tracked frame", async () => {
const revoke = withRevokeSpy();
const { store, esInstances } = harness([{ ...OBS_IDLE, thumbnailAttachmentId: "a1" }]);
store.start();
await vi.waitFor(() => expect(store.getSnapshot().sessions).toHaveLength(1));
store.ensureThumbnail("a1");
await vi.waitFor(() => expect(store.getSnapshot().thumbnails["a1"].status).toBe("ready"));
emit(esInstances[0], {
type: "remove",
session: { sessionId: "s1", action: "idle", since: 0 },
});
expect(revoke).toHaveBeenCalledWith("blob:url-a1");
expect(store.getSnapshot().thumbnails["a1"]).toBeUndefined();
store.stop();
});
it("a load that finishes after replacement never resurrects the old frame", async () => {
const revoke = withRevokeSpy();
const { store, esInstances, loadImage } = harness([
{ ...OBS_IDLE, thumbnailAttachmentId: "a1" },
]);
let resolveA1!: (url: string) => void;
loadImage.mockImplementation((id: string) =>
id === "a1"
? new Promise<string>((resolve) => {
resolveA1 = resolve;
})
: Promise.resolve(`blob:url-${id}`),
);
store.start();
await vi.waitFor(() => expect(store.getSnapshot().sessions).toHaveLength(1));
store.ensureThumbnail("a1");
// the frame is replaced while a1 is still loading
emit(esInstances[0], {
type: "upsert",
session: { ...OBS_IDLE, thumbnailAttachmentId: "a2" },
});
resolveA1("blob:url-a1-late");
await new Promise((resolve) => setImmediate(resolve));
expect(store.getSnapshot().thumbnails["a1"]).toBeUndefined();
expect(revoke).toHaveBeenCalledWith("blob:url-a1-late");
store.stop();
});
});
describe("SSE (re)open resync", () => {
it("refetches the full state when the stream (re)opens", async () => {
const { store, fetches, esInstances } = harness([OBS_IDLE]);
store.start();
await vi.waitFor(() => expect(store.getSnapshot().sessions).toHaveLength(1));
const before = fetches.filter((f) => f.url === "/bsk-observation/state").length;
esInstances[0].onopen?.();
await vi.waitFor(() =>
expect(fetches.filter((f) => f.url === "/bsk-observation/state").length).toBe(before + 1),
);
store.stop();
});
});
@@ -2,7 +2,7 @@
// traffic isolation, interrupt routing, the owned boundary, and the HTTP/SSE
// interface. All bsk runs are faked; the scheduler is a manual timer queue.
import { mkdtempSync, writeFileSync } from "node:fs";
import { existsSync, mkdtempSync, writeFileSync } from "node:fs";
import type { IncomingMessage, ServerResponse } from "node:http";
import { tmpdir } from "node:os";
import { join } from "node:path";
@@ -344,6 +344,7 @@ describe("HTTP/SSE interface", () => {
function fakeRes() {
const chunks: string[] = [];
const closeHandlers: (() => void)[] = [];
const res = {
status: 0,
writeHead(status: number) {
@@ -356,14 +357,35 @@ describe("HTTP/SSE interface", () => {
end(chunk?: string) {
if (chunk !== undefined) chunks.push(chunk);
},
on(event: string, fn: () => void) {
if (event === "close") closeHandlers.push(fn);
},
close: () => closeHandlers.forEach((fn) => fn()),
body: () => chunks.join(""),
};
return {
res: res as unknown as ServerResponse & { status: number; body: () => string },
res: res as unknown as ServerResponse & {
status: number;
body: () => string;
close: () => void;
},
chunks,
};
}
/** Loopback GET/POST request stubs the fence accepts. */
function fakeReq(overrides: Record<string, unknown> = {}): IncomingMessage {
return {
method: "GET",
headers: { host: "127.0.0.1:3999" },
on: (event: string, fn: (chunk?: string) => void) => {
if (event === "data") fn(JSON.stringify({ sessionId: "s1" }));
if (event === "end") fn();
},
...overrides,
} as unknown as IncomingMessage;
}
it("serves state, streams events, and routes interrupt", async () => {
const registry = new SessionRegistry(5);
own(registry, "s1");
@@ -379,35 +401,25 @@ describe("HTTP/SSE interface", () => {
const stateRoute = routes.get("/bsk-observation/state");
expect(stateRoute).toBeDefined();
const stateRes = fakeRes();
await stateRoute?.handler({ method: "GET" } as IncomingMessage, stateRes.res as never);
await stateRoute?.handler(fakeReq(), stateRes.res as never);
expect(stateRes.res.status).toBe(200);
expect(JSON.parse(stateRes.res.body()).sessions[0].sessionId).toBe("s1");
// events (SSE): one write per service event
const eventsRoute = routes.get("/bsk-observation/events");
const eventsRes = fakeRes();
const closeHandlers: (() => void)[] = [];
const req = {
method: "GET",
on: (event: string, fn: () => void) => {
if (event === "close") closeHandlers.push(fn);
},
} as unknown as IncomingMessage;
await eventsRoute?.handler(req, eventsRes.res as never);
await eventsRoute?.handler(fakeReq(), eventsRes.res as never);
service.beginAction("s1", "clicking");
expect(eventsRes.res.body()).toContain('"action":"clicking"');
closeHandlers.forEach((fn) => fn());
eventsRes.res.close();
// interrupt (POST {sessionId})
const interruptRoute = routes.get("/bsk-observation/interrupt");
const interruptRes = fakeRes();
const postReq = {
const postReq = fakeReq({
method: "POST",
on: (event: string, fn: (chunk?: string) => void) => {
if (event === "data") fn(JSON.stringify({ sessionId: "s1" }));
if (event === "end") fn();
},
} as unknown as IncomingMessage;
headers: { host: "127.0.0.1:3999", "content-type": "application/json" },
});
await interruptRoute?.handler(postReq, interruptRes.res as never);
expect(JSON.parse(interruptRes.res.body())).toEqual({ interrupted: true });
@@ -416,6 +428,57 @@ describe("HTTP/SSE interface", () => {
service.dispose();
});
it("fences off non-loopback, cross-site, and simple-request traffic", async () => {
const registry = new SessionRegistry(5);
own(registry, "s1");
const { service } = setup({ registry });
service.addSession("s1");
const { routes, webServer } = routeHarness();
const ctx = { get: (key: string) => (key === "webServer" ? webServer : undefined) } as never;
const dispose = registerObservationRoutes(ctx, service);
const stateRoute = routes.get("/bsk-observation/state");
const interruptRoute = routes.get("/bsk-observation/interrupt");
const getState = async (headers: Record<string, string>) => {
const res = fakeRes();
await stateRoute?.handler(fakeReq({ headers }), res.res as never);
return res.res.status;
};
// Host must be a loopback authority.
expect(await getState({ host: "192.168.1.20:3999" })).toBe(403);
expect(await getState({ host: "attacker.example" })).toBe(403);
expect(await getState({ host: "localhost:3999" })).toBe(200);
expect(await getState({ host: "[::1]:3999" })).toBe(200);
// A present Origin must match the Host (DNS-rebinding reads die here).
expect(await getState({ host: "127.0.0.1:3999", origin: "http://evil.example" })).toBe(403);
expect(await getState({ host: "127.0.0.1:3999", origin: "http://127.0.0.1:3999" })).toBe(200);
// Explicitly cross-site fetches are refused even without an Origin.
expect(await getState({ host: "127.0.0.1:3999", "sec-fetch-site": "cross-site" })).toBe(403);
expect(await getState({ host: "127.0.0.1:3999", "sec-fetch-site": "same-origin" })).toBe(200);
// POST must be application/json — a cross-site simple request can never comply.
const post = async (contentType: string | undefined) => {
const res = fakeRes();
const req = fakeReq({
method: "POST",
headers: {
host: "127.0.0.1:3999",
...(contentType !== undefined ? { "content-type": contentType } : {}),
},
});
await interruptRoute?.handler(req, res.res as never);
return res.res.status;
};
expect(await post("text/plain")).toBe(403);
expect(await post("application/x-www-form-urlencoded")).toBe(403);
expect(await post(undefined)).toBe(403);
expect(await post("application/json; charset=utf-8")).toBe(200);
dispose();
service.dispose();
});
it("registers nothing when no webServer is mounted", () => {
const { service } = setup({});
const ctx = { get: () => undefined } as never;
@@ -434,7 +497,12 @@ describe("availability and dead sessions", () => {
service.addSession("s1");
for (let i = 1; i <= 3; i++) {
scheduler.runNext();
await waitFor(() => runner.calls.filter((c) => c.args[0] === "screenshot").length === i);
// the frame's scratch-file unlink lands before the next timer is queued
await waitFor(
() =>
runner.calls.filter((c) => c.args[0] === "screenshot").length === i &&
scheduler.pending().length === 1,
);
}
expect(service.isAvailable()).toBe(false);
expect(events.some((e) => e.type === "availability" && e.available === false)).toBe(true);
@@ -498,3 +566,79 @@ describe("actionForLabel", () => {
expect(actionForLabel("session start")).toBe("starting");
});
});
describe("scratch files and thumbnail references", () => {
it("reuses one scratch path per session and deletes it after reading", async () => {
const scheduler = fakeScheduler();
const runner = fakeRunner();
const { service } = setup({ runner, scheduler });
service.addSession("s1");
// first frame (scheduled with delay 0)
scheduler.runNext();
await waitFor(
() =>
service.getState()[0]?.thumbnailAttachmentId !== undefined &&
scheduler.pending().length === 1,
);
const firstOut = runner.calls.filter((c) => c.args[0] === "screenshot").at(-1)?.args;
const pathOf = (args: string[]) => args[args.indexOf("--out") + 1];
expect(pathOf(firstOut ?? [])).toBe(join(tmpdir(), "bsk-obs-s1.png"));
expect(existsSync(pathOf(firstOut ?? []))).toBe(false);
// second frame: same path, deleted again
service.endAction("s1");
scheduler.runNext();
await waitFor(
() =>
runner.calls.filter((c) => c.args[0] === "screenshot").length >= 2 &&
scheduler.pending().length === 1,
);
const secondOut = runner.calls.filter((c) => c.args[0] === "screenshot").at(-1)?.args;
expect(pathOf(secondOut ?? [])).toBe(join(tmpdir(), "bsk-obs-s1.png"));
await waitFor(() => !existsSync(pathOf(secondOut ?? [])));
service.dispose();
});
it("drops the replaced frame reference and clears it on remove", async () => {
let counter = 0;
const attachments = {
saveImage: async (input: { data: Uint8Array; name?: string }) => {
counter += 1;
return {
attachmentId: `att-${counter}`,
mediaType: "image/png",
bytes: input.data.byteLength,
width: 4,
height: 4,
name: input.name,
};
},
readImage: async (ref: { attachmentId: unknown }) => ({
data: new Uint8Array([1, 2, 3]),
attachment: ref,
}),
};
const scheduler = fakeScheduler();
const { service } = setup({ scheduler, attachments });
service.addSession("s1");
scheduler.runNext();
await waitFor(
() =>
service.getState()[0]?.thumbnailAttachmentId === "att-1" &&
scheduler.pending().length === 1,
);
service.endAction("s1");
scheduler.runNext();
await waitFor(
() =>
service.getState()[0]?.thumbnailAttachmentId === "att-2" &&
scheduler.pending().length === 1,
);
// the replaced frame's read path is gone
expect(await service.readThumbnail("att-1")).toBeUndefined();
expect((await service.readThumbnail("att-2"))?.mediaType).toBe("image/png");
// removing the session clears its live reference too
service.removeSession("s1");
expect(await service.readThumbnail("att-2")).toBeUndefined();
service.dispose();
});
});
@@ -0,0 +1,85 @@
// KeyedExecutor: per-key FIFO guarantees, including the abort-while-queued
// race that used to release the tail early (letting a later task overlap the
// one still running on the same session).
import { describe, expect, it } from "vitest";
import { KeyedExecutor } from "../src/queue";
describe("KeyedExecutor", () => {
it("runs same-key tasks in FIFO order", async () => {
const queue = new KeyedExecutor();
const order: string[] = [];
const gate = new Promise<void>((resolve) => setTimeout(resolve, 5));
await Promise.all([
queue.run("k", async () => {
await gate;
order.push("a");
}),
queue.run("k", async () => {
order.push("b");
}),
queue.run("k", async () => {
order.push("c");
}),
]);
expect(order).toEqual(["a", "b", "c"]);
});
it("a task aborted while queued does not let the next one overlap the runner", async () => {
const queue = new KeyedExecutor();
let releaseA!: () => void;
const aRuns = new Promise<void>((resolve) => {
releaseA = resolve;
});
const events: string[] = [];
// A holds the session until releaseA.
const a = queue.run("s1", async () => {
events.push("a:start");
await aRuns;
events.push("a:end");
});
// B is queued behind A and aborted before A finishes.
const abortB = new AbortController();
const b = queue.run(
"s1",
async () => {
events.push("b:start");
},
abortB.signal,
);
// C is queued behind B.
const c = queue.run("s1", async () => {
events.push("c:start");
});
abortB.abort();
await expect(b).rejects.toMatchObject({ name: "AbortError" });
// Give C every chance to (wrongly) start while A is still in flight.
await new Promise((resolve) => setTimeout(resolve, 20));
expect(events).toEqual(["a:start"]);
releaseA();
await a;
await c;
expect(events).toEqual(["a:start", "a:end", "c:start"]);
});
it("a failed task never strands the queue", async () => {
const queue = new KeyedExecutor();
await expect(
queue.run("k", async () => {
throw new Error("boom");
}),
).rejects.toThrow("boom");
await expect(queue.run("k", async () => "ok")).resolves.toBe("ok");
});
it("drops the tail entry once the queue drains", async () => {
const queue = new KeyedExecutor();
const internals = queue as unknown as { tails: Map<string, Promise<void>> };
await queue.run("k", async () => {});
await new Promise((resolve) => setImmediate(resolve));
expect(internals.tails.size).toBe(0);
});
});
@@ -1,8 +1,8 @@
import { mkdtempSync, writeFileSync } from "node:fs";
import { existsSync, mkdtempSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import type { ToolDefinition, ToolRunContext } from "@deepseek-ai/dsh-tools";
import { describe, expect, it } from "vitest";
import { describe, expect, it, vi } from "vitest";
import { ObservationService } from "../src/observation";
import { KeyedExecutor } from "../src/queue";
import type { BskRunner, BskRunOptions, BskRunResult } from "../src/runner";
@@ -621,3 +621,180 @@ describe("presentation", () => {
expect(view).toMatchObject({ card: "terminal", output: "[session s1] screenshot of tab 7" });
});
});
describe("screenshot scratch file lifecycle", () => {
/** Runner that honors --out like the real bsk (writes the PNG there). */
function outWritingRunner() {
const calls: FakeCall[] = [];
return {
calls,
runner: {
async run(args: string[], options: BskRunOptions = {}): Promise<BskRunResult> {
calls.push({ args, options });
if (args[0] === "session") {
return {
code: 0,
stdout: JSON.stringify({ session_id: "s1", browser_instance_id: "chrome-1" }),
stderr: "",
timedOut: false,
aborted: false,
};
}
if (args[0] === "screenshot") {
const out = args[args.indexOf("--out") + 1];
writeFileSync(out, Buffer.from([0x89, 0x50, 0x4e, 0x47]));
return {
code: 0,
stdout: JSON.stringify({
tab_id: 7,
width: 4,
height: 4,
format: "png",
path: out,
byte_size: 4,
}),
stderr: "",
timedOut: false,
aborted: false,
};
}
return { code: 2, stdout: "{}", stderr: "", timedOut: false, aborted: false };
},
killAll() {},
killFor: () => 0,
} as BskRunner,
};
}
function setupWithRunner(runner: BskRunner, services: Record<string, unknown> = {}) {
const { ctx, tools } = makeCtx(services);
const registry = new SessionRegistry(5);
registerTools({
ctx: ctx as never,
runner,
registry,
config: CONFIG,
observation: disabledObservation({ ctx, runner, registry }),
queue: new KeyedExecutor(),
});
return { tools, registry };
}
const IMAGE_SERVICES = {
attachments: {
imageLimits: { mediaTypes: ["image/png"], maxImageBytes: 1e7, maxMessageImageBytes: 1e7 },
saveImage: async (input: { data: Uint8Array; mediaType: string }) => ({
attachmentId: "att-1",
mediaType: input.mediaType,
bytes: input.data.byteLength,
width: 4,
height: 4,
}),
},
llm: { resolveModelInfo: async () => ({ inputModalities: ["text", "image"] }) },
};
it("deletes the scratch PNG once the bytes are in the attachment store", async () => {
const { runner, calls } = outWritingRunner();
const { tools } = setupWithRunner(runner, IMAGE_SERVICES);
await startSession(tools);
const exec = makeExec({
agent: {
session: { requestHeader: () => ({ config: { provider: "p", model: "vl" } }) },
options: {},
},
});
const screenshot = tools.get("browser_screenshot");
const value = (await screenshot?.execute({}, exec)) as { image?: unknown; path: string };
expect(value.image).toBeDefined();
const out = calls.find((c) => c.args[0] === "screenshot")?.args.at(-1);
expect(out).toBeDefined();
expect(existsSync(out ?? "")).toBe(false);
});
it("keeps the PNG only when it is the model-facing artifact (no store)", async () => {
const { runner, calls } = outWritingRunner();
const { tools } = setupWithRunner(runner);
await startSession(tools);
const screenshot = tools.get("browser_screenshot");
const value = (await screenshot?.execute({}, makeExec())) as { image?: unknown; path: string };
expect(value.image).toBeUndefined();
const out = calls.find((c) => c.args[0] === "screenshot")?.args.at(-1);
expect(existsSync(out ?? "")).toBe(true);
});
});
describe("observation action instrumentation timing", () => {
it("a queued call never overwrites the in-flight action label", async () => {
const { ctx, tools } = makeCtx();
const registry = new SessionRegistry(5);
let releaseNavigate!: () => void;
const navigateGate = new Promise<void>((resolve) => {
releaseNavigate = resolve;
});
const runner: BskRunner = {
async run(args: string[]): Promise<BskRunResult> {
if (args[0] === "session") {
return {
code: 0,
stdout: JSON.stringify({ session_id: "s1", browser_instance_id: "chrome-1" }),
stderr: "",
timedOut: false,
aborted: false,
};
}
if (args[0] === "navigate") {
await navigateGate;
return {
code: 0,
stdout: JSON.stringify({ tab_id: 7, url: "http://x" }),
stderr: "",
timedOut: false,
aborted: false,
};
}
return {
code: 0,
stdout: JSON.stringify({ text: "ok", ref_count: 0, tab_id: 7, truncated: false }),
stderr: "",
timedOut: false,
aborted: false,
};
},
killAll() {},
killFor: () => 0,
};
const queue = new KeyedExecutor();
const observation = new ObservationService({
ctx: ctx as never,
runner,
registry,
queue,
options: { enabled: true, thumbnailIntervalMs: 1500, idleIntervalMs: 8000 },
});
registerTools({ ctx: ctx as never, runner, registry, config: CONFIG, observation, queue });
await startSession(tools);
const navigate = tools.get("browser_navigate");
const snapshot = tools.get("browser_snapshot");
const a = navigate?.execute({ url: "http://x" }, makeExec()) as Promise<unknown>;
// A starts immediately (empty queue) and blocks on the gate.
await vi.waitFor(() =>
expect(observation.getState().find((s) => s.sessionId === "s1")?.action).toBe("navigating"),
);
// B queues behind A: the label must stay "navigating" until A settles.
const b = snapshot?.execute({}, makeExec()) as Promise<unknown>;
await new Promise((resolve) => setTimeout(resolve, 30));
expect(observation.getState().find((s) => s.sessionId === "s1")?.action).toBe("navigating");
releaseNavigate();
await a;
await vi.waitFor(() =>
expect(observation.getState().find((s) => s.sessionId === "s1")?.action).toBe("snapshotting"),
);
await b;
await vi.waitFor(() =>
expect(observation.getState().find((s) => s.sessionId === "s1")?.action).toBe("idle"),
);
observation.dispose();
});
});