fix: browser_download no longer hijacks the user's own downloads (#33)

Browser.setDownloadBehavior was sent without a browserContextId, so the override applied to the
user's default context and outlived the session. browser_download now takes restore:true, tracks
the override this process set, and a clean session shutdown restores Chrome's default with a
bounded call that is skipped when the browser is already gone.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

6 files changed, 273 insertions, 4 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index aaf0a11..d85ef79 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@ All notable changes to pi-browser-harness will be documented in this file.
 
 ### Fixed
 
+- **`browser_download` no longer redirects the user's own downloads for good.** `Browser.setDownloadBehavior` was sent without a `browserContextId`, which Chrome applies to its default context — the one the user's own tabs are in — and the override lives on the browser connection, which the daemon keeps open across pi sessions. Every later download in that Chrome, the user's own included, went to the tool's temp directory, and nothing undid it when the session ended. The tool now takes `restore: true` to reset the behavior, and a clean session shutdown restores the default whenever this session's override is still in force (a killed session cannot, so call `restore: true` as soon as you are done).
 - **Browser state is no longer appended to the session on every callback.** Every ownership change, every `session_tree` event (even one that merely restored the entry already on the branch) and every shutdown appended a `browser-harness-state` entry, so the append-only session file and Pi Web's `+N custom` chain grew without bound. A snapshot is now written only when it differs from the last one persisted or restored; tab order does not count as a difference. (#26)
 - **The daemon starts from a pi-managed install without `typebox`.** pi installs packages with dev and peer dependencies omitted and maps host-provided packages such as `typebox` only inside the extension host, but the daemon is a separate Node process that resolved `typebox` through `src/cdp/discovery.ts` and died with `Cannot find module 'typebox'` before binding its socket, so `/browser-setup` reported `Could not start the browser daemon`. The daemon's `/json/version` parsing no longer uses `typebox`, and a test walks the daemon's import graph to keep every reachable package a runtime dependency. (#34)
 - **The daemon starts from a Bun-compiled pi binary.** Inside `pi.exe` (and any other `bun build --compile` host), `createRequire().resolve("tsx/cli")` is rooted at the binary's embedded virtual filesystem and cannot see the extension's real `node_modules`, so `browser_setup` failed instantly with `Could not start the browser daemon`. The resolver result is now checked on disk and, when it fails, the tsx CLI is found by walking up the `node_modules` chain the way Node does. Under Bun the daemon is launched with `node` from `PATH` instead of `process.execPath` (which is the pi binary itself); `PI_BROWSER_NODE` overrides the executable. (#27)
diff --git a/README.md b/README.md
index a1fd9b2..b99535b 100644
--- a/README.md
+++ b/README.md
@@ -173,7 +173,7 @@ tabs or navigating the tab to another origin.
 | Tool | Purpose |
 |---|---|
 | `browser_upload_file` | Set files on a file input, with a DataTransfer fallback for stubborn pages. |
-| `browser_download` | Set the download directory and suppress the save-as prompt. |
+| `browser_download` | Set the download directory and suppress the save-as prompt, or restore the default. Applies to the user's own downloads until restored. |
 | `browser_print_to_pdf` | Print the page to PDF via `Page.printToPDF`. |
 | `browser_viewport_resize` | Override viewport size and device pixel ratio for responsive checks. |
 
diff --git a/skills/pi-browser-harness/SKILL.md b/skills/pi-browser-harness/SKILL.md
index f0a3324..e3bf7dc 100644
--- a/skills/pi-browser-harness/SKILL.md
+++ b/skills/pi-browser-harness/SKILL.md
@@ -76,6 +76,14 @@ it by opening tabs elsewhere — a different profile means different accounts.
 
 You're attached to the user's real Chrome — never launch your own. If auth is required, stop and ask the user. If `browser_page_info` returns a dialog, handle it first with `browser_handle_dialog`.
 
+## Downloads land in the user's context
+
+`browser_download` calls `Browser.setDownloadBehavior` with no `browserContextId`, which Chrome applies to its **default browser context** — the same one the user's own tabs are in. The override redirects the user's manual downloads too, and it lives on the browser connection the daemon keeps open, so it outlives the tab and the pi session. Symptom the user reports: *"I download things in Chrome and nothing appears in Downloads."*
+
+A clean session shutdown restores the default; a killed session does not. Call `browser_download({ restore: true })` as soon as you are done with a download path, and if a user reports the symptom, call it first.
+
+`browser_run_script` is the same hazard: a script that sets download behavior and then throws partway leaves Chrome mutated. Reset it in a `finally`, not after the last line.
+
 ## Diagnosing a "nothing happened" moment
 
 When an action runs but the page didn't change, capture `browser_console`'s `nextCursor` *before* the action, take the action, then call `browser_console({ sinceSeq: <cursor> })` after — this isolates what your action caused from what was already there. Pair with `browser_network_requests({ sinceMs: 5000 })` to see if an API call fired and failed. The console buffer is page-scoped: it clears on tab switch, capacity 500.
diff --git a/src/domains/files.ts b/src/domains/files.ts
index 9a3a705..9213c1b 100644
--- a/src/domains/files.ts
+++ b/src/domains/files.ts
@@ -145,22 +145,70 @@ export const uploadFileTool = defineBrowserTool({
   },
 });
 
+// Chrome stores the override in its default browser context, so it outlives this client and
+// silently redirects the user's own downloads. Track it, so session teardown can undo it.
+// The flag only records what THIS process set: the daemon is shared by every pi process on the
+// machine and the CDP primitive is last-writer-wins, so another session may have reset or
+// re-pointed it since. A crashed or killed session never reaches teardown, so the override can
+// also outlive the process that set it.
+let activeDownloadOverride: string | undefined;
+
+// Shutdown must not block on a browser that is already gone: a pending restore against a
+// disconnected daemon would otherwise trigger the one-shot reconnect (and Chrome's consent
+// prompt) that 0.11.1 confined to explicit tool calls, and wait up to the connect timeout.
+const SHUTDOWN_RESTORE_TIMEOUT_MS = 2_000;
+
+export const restoreDownloadBehavior = async (client: BrowserClient): Promise<Result<void, ToolErr>> => {
+  const pending = activeDownloadOverride;
+  if (pending === undefined) return ok(undefined);
+  if (!client.status().alive) {
+    return err({ kind: "not_connected", message: `browser not connected; downloads may keep going to ${pending} until Chrome restarts` });
+  }
+  const r = await cdpCallBrowser(client, "Browser.setDownloadBehavior", { behavior: "default" }, { timeoutMs: SHUTDOWN_RESTORE_TIMEOUT_MS });
+  if (!r.success) {
+    return err({ ...r.error, message: `${r.error.message} (downloads may keep going to ${pending})` });
+  }
+  activeDownloadOverride = undefined;
+  return ok(undefined);
+};
+
 const DownloadArgs = Type.Object({
-  downloadPath: Type.String({ description: "Absolute path to a writable directory where downloads should be saved" }),
+  downloadPath: Type.Optional(
+    Type.String({ description: "Absolute path to a writable directory where downloads should be saved" }),
+  ),
+  restore: Type.Optional(
+    Type.Boolean({
+      description:
+        "Undo a previous browser_download: reset Chrome's download behavior back to the user's own default.",
+    }),
+  ),
 });
 
 export const downloadTool = defineBrowserTool({
   name: "browser_download",
   label: "Browser Download",
-  description: "Configure Chrome's download behavior: set the save directory and disable the save-as prompt.",
+  description:
+    "Configure Chrome's download behavior: set the save directory and disable the save-as prompt, or restore the user's default. Restored automatically when this session shuts down cleanly.",
   promptSnippet: "Configure download directory",
   promptGuidelines: [
     "Pass an absolute path to an existing writable directory.",
-    "Affects all subsequent downloads on this browser.",
+    "The override applies to the user's own downloads too, not just yours, and only a clean session shutdown undoes it. Call browser_download({restore:true}) as soon as you are done.",
   ],
   parameters: DownloadArgs,
   concurrency: "serialized",
   async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> {
+    if (args.restore === true) {
+      if (args.downloadPath !== undefined) {
+        return err({ kind: "invalid_state", message: "Pass either downloadPath or restore, not both" });
+      }
+      const r = await cdpCallBrowser(client, "Browser.setDownloadBehavior", { behavior: "default" });
+      if (!r.success) return r;
+      activeDownloadOverride = undefined;
+      return ok({ text: "Downloads restored to Chrome's default location", details: { restored: true } });
+    }
+    if (args.downloadPath === undefined) {
+      return err({ kind: "invalid_state", message: "Provide downloadPath to set a directory, or restore:true to undo" });
+    }
     try {
       await mkdir(args.downloadPath, { recursive: true });
     } catch (e) {
@@ -187,6 +235,7 @@ export const downloadTool = defineBrowserTool({
       eventsEnabled: true,
     });
     if (!r.success) return r;
+    activeDownloadOverride = args.downloadPath;
     return ok({
       text: `Downloads will save to: ${args.downloadPath}`,
       details: { downloadPath: args.downloadPath },
diff --git a/src/index.ts b/src/index.ts
index a2e0c11..5818a5d 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -7,6 +7,7 @@ import { readPin } from "./profile/store";
 import { type BrowserState, createStatePersister, defaultState, restoreState } from "./state";
 import { registerAllTools } from "./registry";
 import { cleanupTempDirs } from "./util/truncate";
+import { restoreDownloadBehavior } from "./domains/files";
 import { createDaemonTransport } from "./daemon/transport";
 import { setDebugClicks } from "./util/debug";
 import { asBoolean, asString } from "./util/guards";
@@ -132,6 +133,11 @@ export default function browserHarnessExtension(pi: ExtensionAPI): void {
   pi.on("session_shutdown", async () => {
     if (client) {
       try {
+        // Warn but keep going: detach and tab cleanup still have to run, and a throw here would skip both.
+        const restored = await restoreDownloadBehavior(client);
+        if (!restored.success) {
+          console.warn(`[pi-browser-harness] ${restored.error.message}`);
+        }
         await client.detach();
         await client.closeOwnedTabs();
       } catch (e) {
diff --git a/test/domains/download-test.ts b/test/domains/download-test.ts
new file mode 100644
index 0000000..929356e
--- /dev/null
+++ b/test/domains/download-test.ts
@@ -0,0 +1,205 @@
+import { test, describe, beforeEach } from "node:test";
+import assert from "node:assert/strict";
+import { mkdtemp, rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { createFakeClient, type FakeClient } from "./fake-client";
+import { downloadTool, restoreDownloadBehavior } from "../../src/domains/files";
+import type { HandlerContext } from "../../src/util/tool";
+import { cdpError } from "../../src/cdp/errors";
+import { err } from "../../src/util/result";
+import { Value } from "typebox/value";
+
+const SET_BEHAVIOR = "Browser.setDownloadBehavior";
+
+const ctxFor = (fake: FakeClient): HandlerContext => ({
+  client: fake.client,
+  signal: undefined,
+  onUpdate: () => {},
+  extensionCtx: undefined as never,
+});
+
+const behaviorCalls = (fake: FakeClient): ReadonlyArray<Record<string, unknown>> =>
+  fake.callsTo(SET_BEHAVIOR).map((c) => c.params);
+
+const withTmp = async (fn: (dir: string) => Promise<void>): Promise<void> => {
+  const dir = await mkdtemp(join(tmpdir(), "bh-download-test-"));
+  try {
+    await fn(dir);
+  } finally {
+    await rm(dir, { recursive: true, force: true });
+  }
+};
+
+// The override flag is module state, so every test starts from "nothing set".
+beforeEach(async () => {
+  await downloadTool.handler({ restore: true }, ctxFor(await createFakeClient()));
+});
+
+describe("browser_download sets and restores Chrome's download behavior", () => {
+  test("restore resets to the browser default without a path", async () => {
+    const fake = await createFakeClient();
+    const r = await downloadTool.handler({ restore: true }, ctxFor(fake));
+    assert.equal(r.success, true);
+    const calls = behaviorCalls(fake);
+    assert.equal(calls.length, 1);
+    assert.equal(calls[0]?.["behavior"], "default");
+    assert.equal(calls[0]?.["downloadPath"], undefined);
+  });
+
+  test("a download path sets an allow behavior pointing at it", async () => {
+    await withTmp(async (dir) => {
+      const fake = await createFakeClient();
+      const r = await downloadTool.handler({ downloadPath: dir }, ctxFor(fake));
+      assert.equal(r.success, true);
+      const calls = behaviorCalls(fake);
+      assert.equal(calls.length, 1);
+      assert.equal(calls[0]?.["behavior"], "allow");
+      assert.equal(calls[0]?.["downloadPath"], dir);
+    });
+  });
+
+  test("restore and downloadPath together are refused before any CDP call", async () => {
+    const fake = await createFakeClient();
+    const r = await downloadTool.handler({ restore: true, downloadPath: "/tmp/whatever" }, ctxFor(fake));
+    assert.equal(r.success, false);
+    if (r.success) return;
+    assert.equal(r.error.kind, "invalid_state");
+    assert.match(r.error.message, /not both/);
+    assert.equal(fake.callsTo(SET_BEHAVIOR).length, 0);
+  });
+
+  test("neither argument is refused before any CDP call", async () => {
+    const fake = await createFakeClient();
+    const r = await downloadTool.handler({}, ctxFor(fake));
+    assert.equal(r.success, false);
+    if (r.success) return;
+    assert.equal(r.error.kind, "invalid_state");
+    assert.match(r.error.message, /restore:true to undo/);
+    assert.equal(fake.callsTo(SET_BEHAVIOR).length, 0);
+  });
+
+  test("an explicit restore:false is not a restore", async () => {
+    await withTmp(async (dir) => {
+      const fake = await createFakeClient();
+      const r = await downloadTool.handler({ restore: false, downloadPath: dir }, ctxFor(fake));
+      assert.equal(r.success, true);
+      assert.equal(behaviorCalls(fake)[0]?.["behavior"], "allow");
+    });
+  });
+
+  test("restore:false without a path is refused rather than defaulting to a restore", async () => {
+    const fake = await createFakeClient();
+    const r = await downloadTool.handler({ restore: false }, ctxFor(fake));
+    assert.equal(r.success, false);
+    if (r.success) return;
+    assert.equal(r.error.kind, "invalid_state");
+    assert.equal(fake.callsTo(SET_BEHAVIOR).length, 0);
+  });
+
+  test("a failed restore CDP call is reported, not swallowed", async () => {
+    const fake = await createFakeClient({
+      canned: { [SET_BEHAVIOR]: err(cdpError("remote_error", "not allowed in this context")) },
+    });
+    const r = await downloadTool.handler({ restore: true }, ctxFor(fake));
+    assert.equal(r.success, false);
+    if (r.success) return;
+    assert.equal(r.error.kind, "cdp_error");
+    assert.match(r.error.message, /not allowed in this context/);
+  });
+
+  test("a path that is an existing file is refused", async () => {
+    await withTmp(async (dir) => {
+      const file = join(dir, "not-a-dir");
+      await writeFile(file, "x");
+      const fake = await createFakeClient();
+      const r = await downloadTool.handler({ downloadPath: file }, ctxFor(fake));
+      assert.equal(r.success, false);
+      if (r.success) return;
+      assert.equal(r.error.kind, "io_error");
+      assert.equal(fake.callsTo(SET_BEHAVIOR).length, 0);
+    });
+  });
+});
+
+describe("the schema is permissive and the handler is the guard", () => {
+  const schema = downloadTool.parameters;
+
+  test("wrong types are refused by the schema, so they never reach the handler", () => {
+    assert.equal(Value.Check(schema, { restore: "yes" }), false);
+    assert.equal(Value.Check(schema, { downloadPath: 5 }), false);
+  });
+
+  test("both-optional means the ambiguous combinations pass the schema and are refused in the handler", () => {
+    assert.equal(Value.Check(schema, {}), true);
+    assert.equal(Value.Check(schema, { restore: true, downloadPath: "/tmp/x" }), true);
+  });
+
+  test("the accepted shapes are the ones the handler acts on", () => {
+    assert.equal(Value.Check(schema, { restore: true }), true);
+    assert.equal(Value.Check(schema, { downloadPath: "/tmp/x" }), true);
+  });
+});
+
+describe("session teardown undoes an override that is still in force", () => {
+  test("a teardown with no override in force sends nothing", async () => {
+    const fake = await createFakeClient();
+    const r = await restoreDownloadBehavior(fake.client);
+    assert.equal(r.success, true);
+    assert.equal(fake.callsTo(SET_BEHAVIOR).length, 0);
+  });
+
+  test("a teardown after a set restores the default exactly once", async () => {
+    await withTmp(async (dir) => {
+      const fake = await createFakeClient();
+      const set = await downloadTool.handler({ downloadPath: dir }, ctxFor(fake));
+      assert.equal(set.success, true);
+
+      const first = await restoreDownloadBehavior(fake.client);
+      const second = await restoreDownloadBehavior(fake.client);
+      assert.equal(first.success, true);
+      assert.equal(second.success, true);
+
+      const calls = behaviorCalls(fake);
+      assert.equal(calls.length, 2);
+      assert.equal(calls[0]?.["behavior"], "allow");
+      assert.equal(calls[1]?.["behavior"], "default");
+    });
+  });
+
+  test("an explicit restore clears the override, so teardown is a no-op", async () => {
+    await withTmp(async (dir) => {
+      const fake = await createFakeClient();
+      await downloadTool.handler({ downloadPath: dir }, ctxFor(fake));
+      await downloadTool.handler({ restore: true }, ctxFor(fake));
+      const r = await restoreDownloadBehavior(fake.client);
+      assert.equal(r.success, true);
+      assert.equal(fake.callsTo(SET_BEHAVIOR).length, 2);
+    });
+  });
+
+  test("a failed teardown reports the failure and keeps the override pending for a retry", async () => {
+    await withTmp(async (dir) => {
+      const setFake = await createFakeClient();
+      await downloadTool.handler({ downloadPath: dir }, ctxFor(setFake));
+
+      const failing = await createFakeClient({
+        canned: { [SET_BEHAVIOR]: err(cdpError("transport_closed", "socket closed")) },
+      });
+      const failed = await restoreDownloadBehavior(failing.client);
+      assert.equal(failed.success, false);
+      if (failed.success) return;
+      assert.equal(failed.error.kind, "cdp_error");
+      assert.match(failed.error.message, /socket closed/);
+      assert.ok(
+        failed.error.message.includes(dir),
+        `the warning has to name the directory that is still hijacked: ${failed.error.message}`,
+      );
+
+      const retry = await createFakeClient();
+      const second = await restoreDownloadBehavior(retry.client);
+      assert.equal(second.success, true);
+      assert.equal(behaviorCalls(retry)[0]?.["behavior"], "default");
+    });
+  });
+});