fix: let Claude Code review passes read the run's patch files in CI

llp/0003-model-runtimes-and-credentials.explainer.md

@@ -165,7 +165,10 @@ attempt and accounts for both attempts; it never accepts unvalidated fallback te
 `--allowedTools` list default-**allows** reads; it does not default-deny. So two things
 happen together: every **granted** read tool (`Read`/`Grep`/`Glob`) is path-scoped to
 the review tree with a `//<cwd>/**` allow rule, and every read tool **not** granted is
-denied explicitly **by name** via `--disallowedTools`. Assuming standard allow-list
+denied explicitly **by name** via `--disallowedTools`. The one scope beyond the tree is
+the run's patch directory (`extraReadRoots`): in CI it sits under the base checkout,
+outside the PR-head tree, and holds only patch text ecr wrote itself. The rest of
+`.runs/` stays unreadable (see LLP 0004 "Patch Workspace"). Assuming standard allow-list
 semantics (unlisted = denied) would silently reopen every unscoped read tool — a bare
 `Grep` no less than a bare `Read` would let an injected instruction read
 `~/.claude/.credentials.json`, `.env*`, or SSH keys and emit them into a finding. This

llp/0004-diff-noise-and-prompts.explainer.md

@@ -186,25 +186,35 @@ decides which reviewer agents run [observed] (`src/core/agent-paths.ts`).
 ## Patch Workspace
 
 `writePatchWorkspace` materializes the kept diffs as one `.patch` file per
-changed file plus a `context.md` manifest, all written **inside** the repo run
-directory so the agent's file-read tool can reach them [observed]
-(`src/core/noise.ts:199-231`). Agents are pointed at these paths instead of
-having the full diff inlined into every prompt [observed]
-(`src/core/noise.ts:200-202` docstring).
+changed file plus a `context.md` manifest under the run directory [observed]
+(`src/core/noise.ts:215-245`). Agents are pointed at these paths for files whose
+diff is not inlined into their prompt: a reviewer's context-only files, and the
+cross-cutting pass's files past its inline budget [observed]
+(`src/core/prompts.ts` `buildReviewerTask`, `buildCrossCuttingTask`).
+
+The run directory is **not** inside the tree the passes read in CI. `.runs/` lives
+under the trusted base checkout, while the passes run in the PR-head tree that
+`ecr ci` materializes in a temp directory. The Claude Code engine scopes every read
+tool to that tree, so it grants the patch directory (`patchDirFor(runDir)`) as a
+second read scope, and only that directory [observed] (`src/core/noise.ts:205`,
+`src/core/claude-code.ts` `buildClaudeArgs` `extraReadRoots`). Before that grant,
+every patch read in CI was denied, verified 2026-10-20 against `claude 2.1.280`.
+The OpenCode engine sets no read permissions of its own, so whether it can read
+outside the tree depends on OpenCode's defaults [assumed].
 
 The token/context saving is the stated motive. There is likely a second motive:
 delivering diffs as files the agent reads on demand also keeps untrusted PR
 content out of the prompt text that must be sanitized inline, shrinking the
 injection surface [inferred] — the diff/noise code states only the
-context-budget reason (`src/core/noise.ts:200-202`), and AGENTS.md states diffs
+context-budget reason (`src/core/noise.ts:211-213`), and AGENTS.md states diffs
 are untrusted and must go through `prompts.ts`, but the code never states
 file-delivery as an isolation choice. Note the cross-cutting pass reverses this
 default and inlines diffs anyway (see [Prompt Layout for Cache and
 Coverage](#prompt-layout-for-cache-and-coverage)), which is why the isolation
 motive is a hypothesis, not a stated rule.
 
 Patch filenames are sanitized to `[a-zA-Z0-9._-]` (everything else → `__`) and
-zero-padded index-prefixed [observed] (`src/core/noise.ts:214-217`). This is a
+zero-padded index-prefixed [observed] (`src/core/noise.ts:226-229`). This is a
 security control, not cosmetics: `entry.path` comes from untrusted PR diff
 content, and an unsanitized path could be used for traversal or collision when
 writing files to disk.

src/__tests__/claude-code.test.ts

@@ -627,6 +627,61 @@ test("buildClaudeArgs: empty tools (coordinator/no-tools fallback) deny every re
   );
 });
 
+test("buildClaudeArgs: extraReadRoots add a second scope for each granted read tool only", () => {
+  const patchDir = "/work/base/.expo-code-review/.runs/r1/patches";
+  const args = buildClaudeArgs({
+    model: "claude-opus-5",
+    system: "SYS",
+    cwd: "/work/tree",
+    tools: ["read", "grep"],
+    extraReadRoots: [patchDir],
+  });
+  const joined = args.join(" ");
+  // In CI the patch files live under the base checkout, outside the PR-head tree, so
+  // without this scope every "read the patch file" instruction is denied by dontAsk.
+  expect(joined).toContain(
+    `--allowedTools Read(//work/tree/**) Read(/${patchDir}/**) Grep(//work/tree/**) Grep(/${patchDir}/**)`,
+  );
+  // Only the patch directory: the rest of the run directory stays unreadable.
+  expect(joined).not.toContain("/.runs/r1/**");
+  expect(joined).not.toContain("Glob(/");
+
+  // A pass with no read tools gets no rule for the extra root either.
+  const none = buildClaudeArgs({
+    model: "claude-opus-5",
+    system: "SYS",
+    cwd: "/work/tree",
+    tools: [],
+    extraReadRoots: [patchDir],
+  });
+  expect(none.join(" ")).not.toContain("--allowedTools");
+  expect(none.join(" ")).not.toContain(patchDir);
+});
+
+test("runClaudePrompt: the handle's patch directory reaches the CLI's read scope", async () => {
+  const body = JSON.stringify({
+    type: "result",
+    subtype: "success",
+    is_error: false,
+    result: "ok",
+  });
+  const { bin, argsLog, cleanup } = await sequencedFakeClaude([body]);
+  const patchDir = "/work/base/.expo-code-review/.runs/r1/patches";
+  try {
+    await runClaudePrompt(testHandle(bin, { patchDir }), {
+      agent: "reviewer",
+      system: "SYS",
+      text: "hi",
+      title: "t",
+    });
+    const args = (await argsLog(1)).split("\n");
+    expect(args).toContain(`Read(/${patchDir}/**)`);
+    expect(args).toContain(`Grep(/${patchDir}/**)`);
+  } finally {
+    await cleanup();
+  }
+});
+
 test("buildClaudeArgs: maxTurns is forwarded as the CLI --max-turns bound", () => {
   const args = buildClaudeArgs({
     model: "claude-opus-5",

src/core/claude-code.ts

@@ -53,6 +53,8 @@ export interface ClaudeCodeHandle extends OpencodeHandle {
   researchMcpConfigPath?: string;
   /** Only reviewer and cross-cutting roles may receive the documentation tools. */
   researchAgents?: ReadonlySet<string>;
+  /** The run's patch directory, readable by every pass that has read tools. */
+  patchDir?: string;
 }
 
 /** Prompt args, structurally shared with promptAgent/promptAndParse in opencode.ts. */
@@ -260,7 +262,9 @@ export function claudeModelMatches(requested: string, actualKey: string): boolea
  * (Glob withheld — directory crawling is what made them wander), and the coordinator
  * plus the no-tools fallback get none. Whatever the role, every GRANTED read tool is
  * path-scoped to the review tree (`//<cwd>/**`, Claude Code's absolute-path rule)
- * with `dontAsk` denying any call that matches no allow rule. This narrows the
+ * plus the run's own patch directory (`extraReadRoots` — ecr writes it, and it holds
+ * only patch text the passes are already shown), with `dontAsk` denying any call
+ * that matches no allow rule. This narrows the
  * prompt-injection exfil path for DIRECT out-of-tree reads: untrusted PR content is
  * the review input and findings are posted as PR comments, so any unscoped
  * read-capable tool (a bare `Grep` no less than a bare `Read`) would let an injected
@@ -306,15 +310,23 @@ export function buildClaudeArgs(opts: {
    * by omission.
    */
   tools?: readonly string[];
+  /**
+   * Absolute directories the granted read tools may ALSO read, beyond the tree: the
+   * run's patch directory, which in CI sits under the base checkout and outside the
+   * PR-head tree. Without it every "read the patch file" instruction is denied.
+   */
+  extraReadRoots?: readonly string[];
   researchMcpConfigPath?: string;
   maxTurns?: number;
   jsonSchema?: Record<string, unknown>;
 }): string[] {
   // Permission rules are gitignore-style with forward slashes; a Windows cwd
   // (`C:\Users\dev\repo`) must be normalized or every rule silently matches
   // nothing and dontAsk denies all reads.
-  const scopeRoot = opts.cwd.replace(/\\/g, "/");
-  const scope = (tool: string): string => `${tool}(/${scopeRoot}/**)`;
+  const scopeRoots = [opts.cwd, ...(opts.extraReadRoots ?? [])].map((root) =>
+    root.replace(/\\/g, "/"),
+  );
+  const scope = (tool: string): string[] => scopeRoots.map((root) => `${tool}(/${root}/**)`);
   const requested = opts.tools ?? ["read", "grep", "glob"];
   const enabled = ALL_READ_TOOLS.filter((claudeName) =>
     requested.some((name) => READ_TOOL_MAP[name] === claudeName),
@@ -323,7 +335,7 @@ export function buildClaudeArgs(opts: {
   // scoped allow rules alone don't deny them when the allow list is empty.
   const deniedReadTools = ALL_READ_TOOLS.filter((tool) => !enabled.includes(tool));
   const researchTools = opts.researchMcpConfigPath ? [...CLAUDE_RESEARCH_TOOLS] : [];
-  const allowedTools = [...enabled.map(scope), ...researchTools];
+  const allowedTools = [...enabled.flatMap(scope), ...researchTools];
   return [
     "-p",
     "--output-format",
@@ -822,6 +834,7 @@ export async function runClaudePrompt(
         system: args.system,
         cwd: process.cwd(),
         tools,
+        ...(handle.patchDir ? { extraReadRoots: [handle.patchDir] } : {}),
         researchMcpConfigPath,
         maxTurns,
         jsonSchema: args.jsonSchema,
@@ -1108,6 +1121,7 @@ export function claudeTokenCredential(
 export async function startClaudeCode(
   config: LoadedConfig,
   research?: ResearchMcpRuntime,
+  patchDir?: string,
 ): Promise<ClaudeCodeHandle> {
   const cliPath = await resolveOnPath("claude");
   if (!cliPath) {
@@ -1222,5 +1236,6 @@ export async function startClaudeCode(
     childEnv,
     ...(research ? { researchMcpConfigPath: research.claudeConfigPath } : {}),
     researchAgents,
+    ...(patchDir ? { patchDir } : {}),
   };
 }

src/core/noise.ts

@@ -197,18 +197,27 @@ export interface PatchWorkspace {
   files: PatchWorkspaceFile[];
 }
 
+/**
+ * Where a run's patch files live. In CI that is under the trusted base checkout,
+ * OUTSIDE the PR-head tree the review passes read, so the Claude Code engine grants
+ * this one directory as a second read scope (see buildClaudeArgs).
+ */
+export function patchDirFor(rootDir: string): string {
+  return path.join(rootDir, "patches");
+}
+
 // @ref LLP 0004#patch-workspace [implements] — filenames sanitized against traversal/collisions from untrusted diff paths
 /**
- * Write one patch file per changed file plus a shared manifest, all inside the
- * repo (so the OpenCode read tool can reach them). Agents are pointed at these
- * paths instead of having the full diff inlined into every prompt.
+ * Write one patch file per changed file plus a shared manifest under the run
+ * directory. Agents are pointed at these paths instead of having every diff
+ * inlined into every prompt.
  */
 export async function writePatchWorkspace(
   kept: DiffEntry[],
   metadata: ReviewMetadata,
   rootDir: string,
 ): Promise<PatchWorkspace> {
-  const patchDir = path.join(rootDir, "patches");
+  const patchDir = patchDirFor(rootDir);
   await mkdir(patchDir, { recursive: true });
 
   const files: PatchWorkspaceFile[] = [];

src/core/review.ts

@@ -12,7 +12,7 @@ import { prepareAuth } from "./auth.js";
 import { coordinate } from "./coordinator.js";
 import { writeRunLog } from "./log.js";
 import type { RunLogRecord } from "./log.js";
-import { filterNoise, writePatchWorkspace } from "./noise.js";
+import { filterNoise, patchDirFor, writePatchWorkspace } from "./noise.js";
 import type { PriorReview } from "./prior-review.js";
 import type { PatchWorkspaceFile } from "./noise.js";
 import {
@@ -456,7 +456,7 @@ export async function runReview(
   }
   try {
     if (usesClaude) {
-      claudeHandle = await startClaudeCode(config, researchRuntime);
+      claudeHandle = await startClaudeCode(config, researchRuntime, patchDirFor(runDir));
     }
   } catch (error) {
     opencodeHandle?.close();