fix(core): screen every tool call through one sandbox ruleset

Two holes, one shape: a tool call could reach the filesystem or the shell
without the sandbox ever having an opinion about it.

`editPathOf` only read `file_path`, so `NotebookEdit` — which carries its target
in `notebook_path` — passed the confinement screen and the diff-capture hook
alike with an undefined path. Under `--safe` that meant a notebook edit anywhere
on disk. It reads both now, and `EDIT_TOOLS` keeps listing `MultiEdit` as
defence in depth even though claude no longer offers it.

`screenTool` replaces the per-call-site branching with one total, fail-open
dispatch from a tool name to `screenEdit` or `screenBash`. Fail-open is
deliberate: a read-only tool, an MCP tool, or a name this version has never
heard of is not the sandbox's business, and a screen that guessed would break
more than it protected. On allow it returns `{}` rather than a decision, so
`canUseTool` still runs behind it.

On the claude side `--safe` was asking for `permissionMode: "acceptEdits"`,
which pre-approves exactly the in-project edits `canUseTool` exists to inspect —
so the callback never ran for them. It is `"default"` now, and Bash goes through
the same `screenTool` rather than being waved through unscreened.
`allowDangerouslySkipPermissions` moves to the non-safe path, where skipping is
the point.

Separately the toolset stops being advisory. `allowedTools` only auto-approves;
it never removes anything, so the whole builtin set stayed reachable. `tools`
genuinely narrows it: `MultiEdit` and the MCP entry drop out, `BashOutput` and
`KillShell` come in because a backgrounded command is unreadable without them,
and `WebFetch`/`WebSearch` are excluded under `--safe`. That last one is why the
CLI banner changed — `--safe` disables the builtin web tools on both backends
now, not only on opencode.

apps/cli/src/index.ts

@@ -52,7 +52,7 @@ const SECTIONS = [
   {
     body: `By default the agent runs unsandboxed — it can write anywhere you can and reach the network. Pass --safe to confine edits to the project directory, cut network access, and screen destructive shell commands.
 
-Only codex confines writes at the OS level. On claude and opencode --safe screens edits by path and commands by pattern, which does not parse shell — a write redirected out of the project is not caught. Neither cuts raw network access; opencode additionally disables its network tools.`,
+Only codex confines writes at the OS level. On claude and opencode --safe screens edits by path and commands by pattern, which does not parse shell — a write redirected out of the project is not caught. Neither cuts raw network access, though both disable their built-in web tools.`,
     title: "Sandboxing",
   },
   {

packages/core/src/providers/claude.ts

@@ -9,7 +9,7 @@
  */
 import { existsSync } from "node:fs";
 import { homedir } from "node:os";
-import { join } from "node:path";
+import { join, resolve } from "node:path";
 import {
   EDIT_OUTPUT_JSON_SCHEMA,
   type EditStructuredOutput,
@@ -30,28 +30,49 @@ import {
   type AgentRunOutcome,
   failureText,
 } from "../agent";
-import { isPathInside } from "../paths";
 import { systemPrompt } from "../prompt";
-import { EDIT_TOOLS, makeSandboxHook } from "../sandbox";
+import { editPathOf, makeSandboxHook, screenTool } from "../sandbox";
 import type { TimelineRecorder } from "../timeline";
 import { describeTool } from "../tool-summary";
 import { buildAirshipMcpServer } from "../tools";
 
-const ALLOWED_TOOLS = [
+/**
+ * The built-in toolset the agent runs with, passed as the SDK's `tools`
+ * option — which genuinely restricts availability (the CLI narrows via deny
+ * rules, in every permission mode) — not as `allowedTools`, which only
+ * auto-approves and restricts nothing. Grep and Glob stay listed: native
+ * builds otherwise fall back to Bash `find`/`grep`. BashOutput and KillShell
+ * ride along with Bash so a background command can still be read and
+ * stopped. The `mcp__airship__*` tools need no entry: MCP tools are exempt
+ * from narrowing, and their permission is `canUseTool`'s to grant.
+ *
+ * Deliberately absent: Task/agent spawning, Skill, SlashCommand, plan-mode
+ * tools, and AskUserQuestion — there is no interactive channel back to the
+ * user from inside a job, so a model that asks would hang the turn.
+ */
+const BUILTIN_TOOLS = [
   "Read",
   "Glob",
   "Grep",
   "Edit",
   "Write",
-  "MultiEdit",
   "NotebookEdit",
   "TodoWrite",
   "Bash",
+  "BashOutput",
+  "KillShell",
   "WebFetch",
   "WebSearch",
-  "mcp__airship__get_element_context",
 ];
 
+/**
+ * Under `--safe` the built-in web tools go too, matching opencode's posture
+ * (its `--safe` denies webfetch/websearch) so the launch banner's "same
+ * guards" claim holds. Raw network access via the shell is still not cut on
+ * either backend; the banner says that too.
+ */
+const SAFE_EXCLUDED_TOOLS = new Set(["WebFetch", "WebSearch"]);
+
 const EDIT_TOOL_MATCHER = "Write|Edit|MultiEdit|NotebookEdit";
 
 /** Claude has no `minimal`; it is the closest thing to Codex's floor. */
@@ -169,9 +190,13 @@ async function run(ctx: AgentRunContext): Promise<AgentRunOutcome> {
     hooks: [
       (i: { hook_event_name?: string; tool_input?: unknown }) => {
         if (i.hook_event_name === "PreToolUse") {
-          const fp = (i.tool_input as Record<string, unknown> | undefined)
-            ?.file_path;
-          if (typeof fp === "string") {
+          // `editPathOf`, not `.file_path`: NotebookEdit names its target
+          // `notebook_path`, and a notebook edit captured under the wrong key
+          // is a notebook edit with no diff and no undo.
+          const fp = editPathOf(
+            (i.tool_input as Record<string, unknown> | undefined) ?? {}
+          );
+          if (fp !== undefined) {
             diffCapture.recordBefore(fp);
           }
         }
@@ -190,7 +215,11 @@ async function run(ctx: AgentRunContext): Promise<AgentRunOutcome> {
 
   const options: Options = {
     abortController: input.abortController,
-    allowedTools: ALLOWED_TOOLS,
+    // The SDK requires this alongside `bypassPermissions`. It also matters
+    // when bypass is downgraded (background sessions, hardened settings):
+    // without it, non-safe mode would run permission asks with no canUseTool
+    // installed, and every ask would become a terminal auto-deny.
+    allowDangerouslySkipPermissions: !safe,
     cwd: input.cwd,
     effort: toClaudeEffort(input.effort),
     enableFileCheckpointing: true,
@@ -207,9 +236,10 @@ async function run(ctx: AgentRunContext): Promise<AgentRunOutcome> {
       schema: EDIT_OUTPUT_JSON_SCHEMA as unknown as Record<string, unknown>,
       type: "json_schema",
     },
-    // Under `--safe`, edits are auto-accepted but the sandbox hook and
-    // canUseTool still screen them. Otherwise nothing screens anything.
-    permissionMode: safe ? "acceptEdits" : "bypassPermissions",
+    // Under `--safe`, "default" routes every tool decision through
+    // `canUseTool` below — `acceptEdits` would auto-accept in-project edits
+    // before the callback ever ran. Otherwise nothing screens anything.
+    permissionMode: safe ? "default" : "bypassPermissions",
     persistSession: true,
     settingSources: ["project"],
     stderr: (data) => {
@@ -222,23 +252,29 @@ async function run(ctx: AgentRunContext): Promise<AgentRunOutcome> {
       preset: "claude_code",
       type: "preset",
     },
+    tools: safe
+      ? BUILTIN_TOOLS.filter((t) => !SAFE_EXCLUDED_TOOLS.has(t))
+      : BUILTIN_TOOLS,
   };
 
-  // Programmatic permission catch-all. The sandbox hook is the primary guard
-  // (hooks run first); this is defense-in-depth in case ordering ever changes.
-  // Both belong to the same posture, so both are gated together.
+  // Two screens, one `screenTool` ruleset. The sandbox hook is the primary
+  // guard: hooks run ahead of permission evaluation, and unlike this callback
+  // they cannot be shadowed by a target project's own `permissions.allow`
+  // settings (which `settingSources: ["project"]` loads). This is the second,
+  // genuinely reachable layer — under `permissionMode: "default"` every tool
+  // decision funnels through it, Bash included. It must resolve on every
+  // path: an unresolved permission request parks the tool call forever.
   if (safe) {
+    const root = resolve(input.cwd);
     options.canUseTool = (toolName, toolInput) => {
-      if (EDIT_TOOLS.has(toolName)) {
-        const fp = (toolInput as Record<string, unknown>).file_path;
-        if (typeof fp === "string" && !isPathInside(input.cwd, fp)) {
-          return Promise.resolve({
-            behavior: "deny",
-            message: `Refusing to modify a path outside the project: ${fp}`,
-          });
-        }
+      const verdict = screenTool(root, toolName, toolInput);
+      if (verdict.allowed) {
+        return Promise.resolve({ behavior: "allow", updatedInput: toolInput });
       }
-      return Promise.resolve({ behavior: "allow", updatedInput: toolInput });
+      return Promise.resolve({
+        behavior: "deny",
+        message: verdict.reason ?? "denied",
+      });
     };
   }
 

packages/core/src/sandbox.test.ts

@@ -11,8 +11,9 @@
  * or PowerShell — the screen was a no-op while the banner still said it was on.
  * (Path containment lives in ./paths.test.ts.)
  */
+import { resolve } from "node:path";
 import { describe, expect, it } from "vitest";
-import { screenBash } from "./sandbox";
+import { screenBash, screenTool } from "./sandbox";
 
 const blocked = (command: string) => screenBash(command).allowed === false;
 
@@ -67,3 +68,91 @@ describe("screenBash allows ordinary commands", () => {
     expect(blocked(command)).toBe(false);
   });
 });
+
+// `screenTool` is the single dispatch both the PreToolUse hook and Claude's
+// `canUseTool` callback run — under `--safe` every permission decision
+// funnels through it, so it must be total: a verdict on every input, never a
+// throw. An unresolved `canUseTool` parks the tool call forever.
+const ROOT = resolve("/project");
+const INSIDE = resolve("/project/src/app.ts");
+const OUTSIDE = resolve("/somewhere/else.ts");
+
+describe("screenTool confines edit tools to the project", () => {
+  it.each(["Write", "Edit", "NotebookEdit"])(
+    "%s outside the root is denied, and the reason names the path",
+    (tool) => {
+      const verdict = screenTool(ROOT, tool, { file_path: OUTSIDE });
+      expect(verdict.allowed).toBe(false);
+      expect(verdict.reason).toContain(OUTSIDE);
+    }
+  );
+
+  it.each(["Write", "Edit", "NotebookEdit"])(
+    "%s inside the root is allowed",
+    (tool) => {
+      expect(screenTool(ROOT, tool, { file_path: INSIDE }).allowed).toBe(true);
+    }
+  );
+
+  // NotebookEdit names its target `notebook_path`, not `file_path`. Keying on
+  // `file_path` alone is exactly the hole that let notebook writes anywhere on
+  // disk sail past the screen.
+  it("NotebookEdit is screened through notebook_path", () => {
+    const inside = screenTool(ROOT, "NotebookEdit", {
+      notebook_path: resolve("/project/analysis.ipynb"),
+    });
+    expect(inside.allowed).toBe(true);
+    const outside = screenTool(ROOT, "NotebookEdit", {
+      notebook_path: resolve("/somewhere/else.ipynb"),
+    });
+    expect(outside.allowed).toBe(false);
+  });
+});
+
+describe("screenTool screens Bash commands", () => {
+  it("denies a destructive command", () => {
+    expect(screenTool(ROOT, "Bash", { command: "rm -rf /" }).allowed).toBe(
+      false
+    );
+  });
+
+  it("allows an ordinary command", () => {
+    expect(screenTool(ROOT, "Bash", { command: "git status" }).allowed).toBe(
+      true
+    );
+  });
+});
+
+describe("screenTool fails open for everything it does not screen", () => {
+  // Unknown and MCP names must pass: under `--safe` there is no other route
+  // to an allow, so a fail-closed default would cut off the tools the flow
+  // depends on rather than guard anything.
+  it.each([
+    "Read",
+    "Grep",
+    "Glob",
+    "TodoWrite",
+    "WebFetch",
+    "mcp__airship__get_element_context",
+    "mcp__airship__get_design_tokens",
+    "SomeFutureTool",
+  ])("allows %s", (tool) => {
+    expect(screenTool(ROOT, tool, {}).allowed).toBe(true);
+  });
+});
+
+describe("screenTool is total on malformed input", () => {
+  it.each([
+    ["missing path", "Edit", {}],
+    ["non-string file_path", "Write", { file_path: 42 }],
+    ["non-string notebook_path", "NotebookEdit", { notebook_path: null }],
+    ["missing command", "Bash", {}],
+    ["non-string command", "Bash", { command: ["rm", "-rf"] }],
+  ] as const)("returns a verdict for %s", (_label, tool, input) => {
+    const verdict = screenTool(ROOT, tool, input as Record<string, unknown>);
+    expect(typeof verdict.allowed).toBe("boolean");
+    // Malformed edits are the SDK's problem to reject, not a deny; a
+    // command that is not a string screens as the empty command.
+    expect(verdict.allowed).toBe(true);
+  });
+});

packages/core/src/sandbox.ts

@@ -2,10 +2,14 @@
  * The `--safe` guards, and the Claude hook that installs them.
  *
  * The two screens — `screenEdit` and `screenBash` — are the policy, expressed
- * without reference to any backend. `makeSandboxHook` wraps them as a Claude
- * `PreToolUse` hook, where a deny hard-blocks the tool because hooks run
- * ahead of permission rules and `canUseTool`; that is the safety layer which
- * replaces the reference tools' blanket `--dangerously-skip-permissions`.
+ * without reference to any backend, and `screenTool` is the single dispatch
+ * that routes a named tool call through them. `makeSandboxHook` wraps that
+ * dispatch as a Claude `PreToolUse` hook, where a deny hard-blocks the tool
+ * because hooks run ahead of permission rules and `canUseTool` — and unlike
+ * `canUseTool`, a hook cannot be shadowed by a target project's own
+ * `permissions.allow` settings, which makes it the primary guard. The Claude
+ * adapter's `canUseTool` calls the same `screenTool`, so the second layer
+ * enforces an identical rule set rather than a drifting copy.
  *
  * OpenCode has no hook of any kind, but it does emit a permission request and
  * accept a reply, so its adapter answers each request from these same two
@@ -94,31 +98,66 @@ export function screenBash(command: string): ScreenResult {
   return ALLOWED;
 }
 
+/**
+ * The path an edit tool is about to write. `NotebookEdit` names its target
+ * `notebook_path` where every other edit tool says `file_path` — keying on
+ * `file_path` alone let notebook writes anywhere on disk sail past both
+ * screens, silently.
+ */
+export function editPathOf(
+  toolInput: Record<string, unknown>
+): string | undefined {
+  const fp = toolInput.file_path ?? toolInput.notebook_path;
+  return typeof fp === "string" ? fp : undefined;
+}
+
+/**
+ * Route one tool call through the policy. Everything that is neither an edit
+ * nor a shell command — read-only built-ins and the `mcp__airship__*` tools —
+ * passes. The screen must fail open for unknown names: under `--safe` every
+ * permission decision funnels through here, and a fail-closed default would
+ * cut off tools the flow depends on rather than guard anything.
+ *
+ * Total by construction: every branch returns a verdict, never throws. A
+ * caller that turns this into `canUseTool` relies on that — an unresolved
+ * permission request parks the tool call forever.
+ */
+export function screenTool(
+  root: string,
+  toolName: string,
+  toolInput: Record<string, unknown>
+): ScreenResult {
+  if (EDIT_TOOLS.has(toolName)) {
+    const fp = editPathOf(toolInput);
+    // A missing or malformed path is the SDK's problem to reject, not a
+    // security decision to make on its behalf.
+    return fp === undefined ? ALLOWED : screenEdit(root, fp);
+  }
+  if (toolName === "Bash") {
+    return screenBash(
+      typeof toolInput.command === "string" ? toolInput.command : ""
+    );
+  }
+  return ALLOWED;
+}
+
 export function makeSandboxHook(cwd: string): HookCallback {
   const root = resolve(cwd);
   return (input) => {
     if (input.hook_event_name !== "PreToolUse") {
       return Promise.resolve({});
     }
-    const name = input.tool_name;
-    const ti = (input.tool_input ?? {}) as Record<string, unknown>;
-
-    if (EDIT_TOOLS.has(name) && typeof ti.file_path === "string") {
-      const verdict = screenEdit(root, ti.file_path);
-      if (!verdict.allowed) {
-        return Promise.resolve(deny(verdict.reason ?? "denied"));
-      }
+    const verdict = screenTool(
+      root,
+      input.tool_name,
+      (input.tool_input ?? {}) as Record<string, unknown>
+    );
+    if (!verdict.allowed) {
+      return Promise.resolve(deny(verdict.reason ?? "denied"));
     }
-
-    if (name === "Bash") {
-      const verdict = screenBash(
-        typeof ti.command === "string" ? ti.command : ""
-      );
-      if (!verdict.allowed) {
-        return Promise.resolve(deny(verdict.reason ?? "denied"));
-      }
-    }
-
+    // An allow stays `{}` — fall through to permission evaluation — never an
+    // explicit hook allow, which would skip `canUseTool` and silently delete
+    // the second screen.
     return Promise.resolve({});
   };
 }