diff --git a/llp/0003-model-runtimes-and-credentials.explainer.md b/llp/0003-model-runtimes-and-credentials.explainer.md index cb9abf7..7b1025c 100644 --- a/llp/0003-model-runtimes-and-credentials.explainer.md +++ b/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 `///**` 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 diff --git a/llp/0004-diff-noise-and-prompts.explainer.md b/llp/0004-diff-noise-and-prompts.explainer.md index 35fa256..2f8e7a5 100644 --- a/llp/0004-diff-noise-and-prompts.explainer.md +++ b/llp/0004-diff-noise-and-prompts.explainer.md @@ -186,17 +186,27 @@ 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 @@ -204,7 +214,7 @@ 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. diff --git a/src/__tests__/claude-code.test.ts b/src/__tests__/claude-code.test.ts index b1c119f..a3a2a77 100644 --- a/src/__tests__/claude-code.test.ts +++ b/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", diff --git a/src/core/claude-code.ts b/src/core/claude-code.ts index 7430047..1bf47f9 100644 --- a/src/core/claude-code.ts +++ b/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; + /** 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 (`///**`, 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,6 +310,12 @@ 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; @@ -313,8 +323,10 @@ export function buildClaudeArgs(opts: { // 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 { const cliPath = await resolveOnPath("claude"); if (!cliPath) { @@ -1222,5 +1236,6 @@ export async function startClaudeCode( childEnv, ...(research ? { researchMcpConfigPath: research.claudeConfigPath } : {}), researchAgents, + ...(patchDir ? { patchDir } : {}), }; } diff --git a/src/core/noise.ts b/src/core/noise.ts index dd54c16..2b62d05 100644 --- a/src/core/noise.ts +++ b/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 { - const patchDir = path.join(rootDir, "patches"); + const patchDir = patchDirFor(rootDir); await mkdir(patchDir, { recursive: true }); const files: PatchWorkspaceFile[] = []; diff --git a/src/core/review.ts b/src/core/review.ts index b99266d..e058e79 100644 --- a/src/core/review.ts +++ b/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();