From 6e8df30774e341a4810bc1e2481ceebd6f1a40c6 Mon Sep 17 00:00:00 2001 From: bilikas Date: Wed, 10 Jun 2026 19:37:21 +0300 Subject: [PATCH] Security: replace Bash LLM tool with read-only, shell-free tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review input is untrusted PR content while the container holds live VCS + LLM credentials, so `bash -lc` was arbitrary command execution one prompt injection away. The LLM tool set is now Read (with start_line/lines paging), Grep, Ls, Glob, Tail — structured args, never a shell; Grep spawns the grep binary with an argv array so model input stays inert data. Decision recorded in ADR-0014; ARCHITECTURE.md pattern 4.11 carries the invariant for future tools. Also: - `pnpm build` now wipes dist/ before compiling, so deleted sources can't leave stale artifacts (flushes the orphaned dist bash.js / shared.js) - import shared docs contributed by the harness repo: ADR-0000 (ADR scope gate), conventions i18n / react / constants-and-identifiers, and the CLAUDE.md working procedure - ignore .claude/ local config Co-Authored-By: Claude Fable 5 --- .gitignore | 3 + CLAUDE.md | 65 +++++++++++ README.md | 10 +- dist/ctx.js | 14 +-- dist/llm/tools/bash.js | 62 ----------- dist/llm/tools/glob.js | 56 ++++++++++ dist/llm/tools/grep.js | 89 +++++++++++++++ dist/llm/tools/index.js | 12 ++- dist/llm/tools/ls.js | 42 ++++++++ dist/llm/tools/read.js | 69 +++++++----- dist/llm/tools/shared.js | 7 -- dist/llm/tools/tail.js | 64 +++++++++++ dist/stages/reconcile/prompt.md | 2 +- dist/stages/review/prompt.md | 2 +- docs/ARCHITECTURE.md | 23 +++- docs/adr/0000-adr-scope.md | 44 ++++++++ docs/adr/0014-llm-tools-read-only-no-shell.md | 55 ++++++++++ docs/adr/README.md | 6 +- docs/conventions/README.md | 7 ++ docs/conventions/constants-and-identifiers.md | 37 +++++++ docs/conventions/i18n.md | 39 +++++++ docs/conventions/react.md | 42 ++++++++ package.json | 2 +- src/ctx.ts | 14 +-- src/llm/client.ts | 2 +- src/llm/tools/bash.ts | 70 ------------ src/llm/tools/glob.ts | 60 +++++++++++ src/llm/tools/grep.ts | 101 ++++++++++++++++++ src/llm/tools/index.ts | 12 ++- src/llm/tools/ls.ts | 45 ++++++++ src/llm/tools/read.ts | 66 +++++++----- src/llm/tools/tail.ts | 69 ++++++++++++ src/stages/reconcile/prompt.md | 2 +- src/stages/review/prompt.md | 2 +- src/stages/types.ts | 2 +- 35 files changed, 968 insertions(+), 229 deletions(-) create mode 100644 CLAUDE.md delete mode 100644 dist/llm/tools/bash.js create mode 100644 dist/llm/tools/glob.js create mode 100644 dist/llm/tools/grep.js create mode 100644 dist/llm/tools/ls.js delete mode 100644 dist/llm/tools/shared.js create mode 100644 dist/llm/tools/tail.js create mode 100644 docs/adr/0000-adr-scope.md create mode 100644 docs/adr/0014-llm-tools-read-only-no-shell.md create mode 100644 docs/conventions/constants-and-identifiers.md create mode 100644 docs/conventions/i18n.md create mode 100644 docs/conventions/react.md delete mode 100644 src/llm/tools/bash.ts create mode 100644 src/llm/tools/glob.ts create mode 100644 src/llm/tools/grep.ts create mode 100644 src/llm/tools/ls.ts create mode 100644 src/llm/tools/tail.ts diff --git a/.gitignore b/.gitignore index 9dd09ac..db3bc09 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,6 @@ node_modules/ # Agent harness — developed here for context continuity, will be moved to its # own repo later. Has its own git; kept out of the reviewer repo. harness/ + +# Claude Code local config (permission grants etc.) — never committed +.claude/ diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..426e943 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,65 @@ +# How we work + +This repo documents itself in three layers (see [docs/conventions/documentation.md](docs/conventions/documentation.md)): + +- **Map** — [docs/ARCHITECTURE.md](docs/ARCHITECTURE.md): where things are, present tense. +- **Rules** — [docs/conventions/](docs/conventions/): portable engineering rules, one topic per file. +- **Decisions** — [docs/adr/](docs/adr/): dated decision log, Proposed → Accepted → Superseded. + +## Before starting work + +Check the current branch first. Starting from `main`/`master` is the normal case +(a feature branch gets created at commit time). On any other branch, ask the user +whether working on it is intended — they may have forgotten to switch back after +a merge. + +Read the docs: ARCHITECTURE.md for the area you're touching, the conventions +index ([docs/conventions/README.md](docs/conventions/README.md)), and any ADRs near +your change ([docs/adr/README.md](docs/adr/README.md)). Subagents get the same +instruction — point them at the specific files relevant to their task. + +## While working + +Build aligned with what is already settled: Accepted ADRs and the conventions as +written are binding. Don't invent a new pattern where a documented one fits, and +don't update the docs mid-session — note discoveries and deviations as you go and +carry them to the end-of-session step. + +## Settle, then move + +When a question, objection, or correction is raised — about code or docs — the +debate comes first and the edits come after. Don't touch files while a decision +is still open; discuss until it's settled or the user explicitly says go. This +applies symmetrically: code changes wait for settled decisions, doc updates wait +for finished debates (or the user's "time to write docs" signal). Nothing is +marked Accepted until the user has actually read and confirmed it. + +## When the user says it's time to write docs + +This step runs only on the user's signal, at the end of a session: + +1. **Update the map** — ARCHITECTURE.md reflects the code as it now is. +2. **Write new ADRs** for decisions made this session — status **Proposed**, one + decision per file, added to the ADR README index. Supersede, don't rewrite. + Gate every candidate through [ADR-0000](docs/adr/0000-adr-scope.md): + architectural decisions only — bug fixes don't get ADRs (commit message + + regression test + why-comment), and process changes amend this file instead. +3. **Add or amend conventions** for patterns discovered this session (new topic + file, or an addition to an existing one). +4. **Ask the user to confirm** the new ADRs and conventions. Confirmed → mark + Accepted. Corrected → settle the corrected version, mark it Accepted, and apply + it to the session's work so code and docs agree. + +The distinction that makes this work: existing docs are **settled** — follow them +without debate, fast. New ADRs and conventions are **fluid** — drafted from what +the session discovered, and only become settled once the user confirms them. Next +session (and every subagent) then starts from the upgraded baseline. + +## After everything is agreed + +Commit and push the session's work: + +- On `main`/`master`: create a new feature branch for the session's work first. +- On any other branch: stay on it — it's already the feature branch. +- Commit all the work (code + docs), then push with `./push.sh ` + (plain `git push` fails here — the SSH key isn't in the default agent). diff --git a/README.md b/README.md index 1907900..d5dbaec 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ Four stages, run in order by `reviewer run`: 1. **fetch** — PR metadata, changed files, diffs, prior bot threads, project standards 2. **reconcile** — re-judge each unresolved bot thread against the new head; relocate on renames, auto-address on deletion (skipped on first run) -3. **review** — LLM analyzes the diff with `Read` + `Bash` tools available, posts inline findings +3. **review** — LLM analyzes the diff with read-only tools available (`Read`/`Grep`/`Ls`/`Glob`/`Tail`), posts inline findings 4. **summarize** — derives final verdict (`approve` / `reject`), posts a summary comment Each stage is also addressable on its own (e.g. `reviewer review --pr `). @@ -79,7 +79,7 @@ docker run --rm -t --env-file .env -v "$PWD:/workspace" -w /workspace \ `local://` diffs `...HEAD` (base defaults to `main`). Use `local://` to pick a different base. -> **Must run inside a container.** Local review gives the LLM the `Bash`/`Read` tools against the mounted checkout; the container is the sandbox. The CLI refuses `local://` unless it detects a container (`/.dockerenv` / `/run/.containerenv`, created by the runtime — not a flag a caller can set), so it can't hand an LLM shell access to a bare host. +> **Must run inside a container.** Local review gives the LLM read-only file tools (`Read`/`Grep`/`Ls`/`Glob`/`Tail`) against the mounted checkout — no shell, but they can read any file the process can reach. The container is the sandbox. The CLI refuses `local://` unless it detects a container (`/.dockerenv` / `/run/.containerenv`, created by the runtime — not a flag a caller can set), so it can't hand an LLM file access to a bare host. ### Commands @@ -141,7 +141,7 @@ src/ reconcile/ re-judge prior bot threads review/ LLM review + inline findings summarize/ final verdict + summary comment - llm/ client (callLLM + resolveModel, schema healing), transport (HTTP + SSE), tools (Read/Bash) + llm/ client (callLLM + resolveModel, schema healing), transport (HTTP + SSE), tools (Read/Grep/Ls/Glob/Tail) providers/ Provider port + github / gitlab / bitbucket / local / mock adapters lib/ host-level helpers (git diffs, error normalization) logger/ Logger port + console / in-memory sinks @@ -151,7 +151,7 @@ tests/ docs/ ARCHITECTURE.md the map: structure, patterns, flows conventions/ portable engineering rules (naming, types placement, …) - adr/ dated architecture decision records + adr/ dated architecture decision records (scope: ADR-0000) ``` -Documentation is layered: [docs/ARCHITECTURE.md](docs/ARCHITECTURE.md) is this repo's map, [docs/conventions/](docs/conventions/) holds the portable rule set (reusable across projects), and [docs/adr/](docs/adr/) is the decision log that adopts them. +Documentation is layered: [docs/ARCHITECTURE.md](docs/ARCHITECTURE.md) is this repo's map, [docs/conventions/](docs/conventions/) holds the portable rule set (reusable across projects), and [docs/adr/](docs/adr/) is the decision log that adopts them. The working procedure that maintains these layers is the root [CLAUDE.md](CLAUDE.md) — agent sessions read it on start. diff --git a/dist/ctx.js b/dist/ctx.js index dd44b3d..445aa93 100644 --- a/dist/ctx.js +++ b/dist/ctx.js @@ -103,14 +103,14 @@ export function loadConfig(opts) { case "mock": break; case "local": - // Local review gives the LLM the Bash/Read tools against the real - // checkout. Those tools assume a throwaway sandbox; running on a bare - // host would hand an LLM unsandboxed shell + file access to your - // machine. Require an actual container — detect it rather than trust an - // env flag (which any caller could set on the host). + // Local review gives the LLM read-only file tools (Read/Grep/Ls/Glob/Tail) + // against the real checkout. They can't run shell, but they can read any + // file the process can reach; on a bare host that's your whole machine. + // Require an actual container — detect it rather than trust an env flag + // (which any caller could set on the host). if (!inContainer()) { - throw new Error("local review must run inside a container (the LLM gets shell + file access, " + - "and the container is the sandbox). Run:\n" + + throw new Error("local review must run inside a container (the LLM can read any file the " + + "process can reach, and the container is the sandbox). Run:\n" + " docker build -t reviewer .\n" + " docker run --rm -v \"$PWD:/workspace\" -w /workspace --env-file .env \\\n" + " reviewer review --pr local://main"); diff --git a/dist/llm/tools/bash.js b/dist/llm/tools/bash.js deleted file mode 100644 index 92e77d2..0000000 --- a/dist/llm/tools/bash.js +++ /dev/null @@ -1,62 +0,0 @@ -// Bash tool — run a shell command in the working directory. -// -// The container is throwaway and isolated, so this tool doesn't sandbox -// paths or restrict commands — the LLM has whatever the container ships -// with (git, grep, find, rg if installed, etc.). -import { spawn } from "node:child_process"; -import { cap } from "./types.js"; -export const bashTool = { - schema: { - type: "function", - function: { - name: "Bash", - description: "Run a shell command in the working directory. Returns combined stdout+stderr plus the exit code. Use for `git log`, `git blame`, `git show`, `grep -rn`, `find`, listing dirs, etc. The container is throwaway — no path restrictions, but state does not persist between calls.", - parameters: { - type: "object", - additionalProperties: false, - properties: { - command: { type: "string", description: "The shell command to run. Executed via `bash -lc`." }, - timeout_seconds: { type: "number", description: "Optional. Defaults to 60." }, - }, - required: ["command"], - }, - }, - }, - async execute(args) { - const command = String(args.command ?? ""); - const timeoutMs = typeof args.timeout_seconds === "number" ? args.timeout_seconds * 1000 : 60_000; - if (!command) { - return { - ok: false, - output: `Bash call rejected: missing required \`command\` argument. ` + - `Got: ${JSON.stringify(args)}. ` + - `Example: {"command": "ls -la"}`, - }; - } - return runBash(command, timeoutMs); - }, -}; -function runBash(command, timeoutMs) { - return new Promise((resolve) => { - const proc = spawn("bash", ["-lc", command], { stdio: ["ignore", "pipe", "pipe"] }); - let out = ""; - let timedOut = false; - const timer = setTimeout(() => { - timedOut = true; - proc.kill("SIGKILL"); - }, timeoutMs); - proc.stdout.on("data", (b) => { out += b.toString("utf-8"); }); - proc.stderr.on("data", (b) => { out += b.toString("utf-8"); }); - proc.on("close", (code) => { - clearTimeout(timer); - const suffix = timedOut - ? `\n[exit: killed after ${timeoutMs}ms timeout]` - : `\n[exit: ${code}]`; - resolve({ ok: !timedOut && code === 0, output: cap(out) + suffix }); - }); - proc.on("error", (e) => { - clearTimeout(timer); - resolve({ ok: false, output: `error spawning bash: ${e.message}` }); - }); - }); -} diff --git a/dist/llm/tools/glob.js b/dist/llm/tools/glob.js new file mode 100644 index 0000000..20c6332 --- /dev/null +++ b/dist/llm/tools/glob.js @@ -0,0 +1,56 @@ +// Glob tool — find files by path pattern, like a shell glob / `find -name`. +// +// Pure filesystem walk via node:fs/promises glob, no subprocess. Noise +// directories are pruned and results are capped so a broad pattern can't +// flood the model's context. +import { glob } from "node:fs/promises"; +import { errorMessage } from "../../lib/errors.js"; +import { cap } from "./types.js"; +const MAX_RESULTS = 500; +const EXCLUDE = ["node_modules", ".git", "dist"]; +export const globTool = { + schema: { + type: "function", + function: { + name: "Glob", + description: "Find files whose path matches a glob pattern, e.g. `src/**/*.ts` or " + + "`**/*.test.js`. Returns matching file paths.", + parameters: { + type: "object", + additionalProperties: false, + properties: { + pattern: { type: "string", description: "Glob pattern to match file paths against, e.g. `src/**/*.ts`." }, + }, + required: ["pattern"], + }, + }, + }, + async execute(args) { + const pattern = String(args.pattern ?? ""); + if (!pattern) { + return { + ok: false, + output: `Glob call rejected: missing required \`pattern\` argument. ` + + `Got: ${JSON.stringify(args)}. ` + + `Example: {"pattern": "src/**/*.ts"}`, + }; + } + try { + const matches = []; + const isExcluded = (p) => p.split(/[\\/]/).some((seg) => EXCLUDE.includes(seg)); + for await (const m of glob(pattern, { exclude: isExcluded })) { + matches.push(m); + if (matches.length >= MAX_RESULTS) + break; + } + if (matches.length === 0) + return { ok: true, output: `No files match ${JSON.stringify(pattern)}.` }; + matches.sort((a, b) => a.localeCompare(b)); + const note = matches.length >= MAX_RESULTS ? `\n[... truncated at ${MAX_RESULTS} matches]` : ""; + return { ok: true, output: cap(`# ${matches.length} match(es) for ${pattern}\n${matches.join("\n")}${note}`) }; + } + catch (e) { + return { ok: false, output: `error globbing ${pattern}: ${errorMessage(e)}.` }; + } + }, +}; diff --git a/dist/llm/tools/grep.js b/dist/llm/tools/grep.js new file mode 100644 index 0000000..da59ce5 --- /dev/null +++ b/dist/llm/tools/grep.js @@ -0,0 +1,89 @@ +// Grep tool — search file contents for a regex under a path, like `grep -rn`. +// +// Two mandatory params: `pattern` (regex) and `path` (where to search). We +// never go through a shell: runGrep spawns the `grep` binary with an ARGV +// ARRAY, so the model-supplied pattern/path are literal arguments, never +// tokenized by a shell — `|`, `;`, `$()`, quotes are inert data, not +// operators (this is exactly why there is no Bash tool: a shell would turn +// them into commands). NEVER rebuild this with `exec`, a command string, or +// `{ shell: true }` — that reintroduces arbitrary command execution. +import { spawn } from "node:child_process"; +import { existsSync } from "node:fs"; +import { cap } from "./types.js"; +const TIMEOUT_MS = 30_000; +export const grepTool = { + schema: { + type: "function", + function: { + name: "Grep", + description: "Search file contents for a pattern (regular expression), like `grep -rn`. " + + "Returns matching lines as `path:line:text` with a couple of lines of " + + "surrounding context. You must say where to search via `path` — scope it to " + + "the folder you expect the match in (e.g. `src`), or point at a specific " + + "file or dependency directory.", + parameters: { + type: "object", + additionalProperties: false, + properties: { + pattern: { type: "string", description: "Text or regular expression to search for." }, + path: { type: "string", description: "File or directory to search in, e.g. `src` or `src/foo.ts`." }, + }, + required: ["pattern", "path"], + }, + }, + }, + async execute(args) { + const pattern = String(args.pattern ?? ""); + const path = String(args.path ?? ""); + if (!pattern || !path) { + return { + ok: false, + output: `Grep call rejected: both \`pattern\` and \`path\` are required. ` + + `Got: ${JSON.stringify(args)}. ` + + `Example: {"pattern": "myFunction", "path": "src"}`, + }; + } + if (!existsSync(path)) { + return { + ok: false, + output: `Grep call rejected: path ${JSON.stringify(path)} does not exist. ` + + `Pass a file or directory that exists (use Ls or Glob to locate it).`, + }; + } + const r = await runGrep(pattern, path); + if (r.timedOut) + return { ok: false, output: `Grep timed out searching ${path}.` }; + if (r.code === 0) + return { ok: true, output: cap(r.output) }; + if (r.code === 1) + return { ok: true, output: `No matches for ${JSON.stringify(pattern)} in ${path}.` }; + return { ok: false, output: `Grep failed (exit ${r.code}): ${r.output.trim() || "unknown error"}` }; + }, +}; +// Run `grep` with an argv array (no shell — see the file header) and collect +// its output, killing it on a timeout. grep exit codes: 0 = matches, 1 = no +// matches, 2 = error; the caller translates them. +function runGrep(pattern, path) { + // `-e` marks the pattern so one starting with `-` is data, not a flag; + // -r recurses, -n adds line numbers, -C 2 gives a little context. + const args = ["-rn", "-C", "2", "-e", pattern, path]; + return new Promise((resolve) => { + const proc = spawn("grep", args, { stdio: ["ignore", "pipe", "pipe"] }); + let out = ""; + let timedOut = false; + const timer = setTimeout(() => { + timedOut = true; + proc.kill("SIGKILL"); + }, TIMEOUT_MS); + proc.stdout.on("data", (b) => { out += b.toString("utf-8"); }); + proc.stderr.on("data", (b) => { out += b.toString("utf-8"); }); + proc.on("close", (code) => { + clearTimeout(timer); + resolve({ code, output: out, timedOut }); + }); + proc.on("error", (e) => { + clearTimeout(timer); + resolve({ code: null, output: `error spawning grep: ${e.message}`, timedOut }); + }); + }); +} diff --git a/dist/llm/tools/index.js b/dist/llm/tools/index.js index c86c726..6fcc749 100644 --- a/dist/llm/tools/index.js +++ b/dist/llm/tools/index.js @@ -10,9 +10,17 @@ // Add a new tool: create tools/.ts exporting `Tool: Tool`, then // add it to the TOOLS array below. import { errorMessage } from "../../lib/errors.js"; -import { bashTool } from "./bash.js"; +import { globTool } from "./glob.js"; +import { grepTool } from "./grep.js"; +import { lsTool } from "./ls.js"; import { readTool } from "./read.js"; -const TOOLS = [readTool, bashTool]; +import { tailTool } from "./tail.js"; +// Read-only navigation only — no shell. A `Bash` tool was removed deliberately: +// `bash -lc` (and even `git -c core.pager=…`) is arbitrary command execution, +// and the review input is untrusted PR content while the container holds live +// VCS + LLM credentials. These tools take structured args and never spawn a +// shell, so the same input is inert data. Keep it that way. +const TOOLS = [readTool, grepTool, tailTool, lsTool, globTool]; const BY_NAME = new Map(TOOLS.map((t) => [t.schema.function.name, t])); const VALID_NAMES = TOOLS.map((t) => t.schema.function.name); export const TOOL_SCHEMAS = TOOLS.map((t) => t.schema); diff --git a/dist/llm/tools/ls.js b/dist/llm/tools/ls.js new file mode 100644 index 0000000..4a3f17b --- /dev/null +++ b/dist/llm/tools/ls.js @@ -0,0 +1,42 @@ +// Ls tool — list a directory, like `ls`. +// +// Pure filesystem read, no subprocess. Directories get a trailing `/`. +import { readdir } from "node:fs/promises"; +import { errorMessage } from "../../lib/errors.js"; +import { cap } from "./types.js"; +export const lsTool = { + schema: { + type: "function", + function: { + name: "Ls", + description: "List the entries of a directory, like `ls`. Directories are shown with a " + + "trailing `/`. Defaults to the working directory.", + parameters: { + type: "object", + additionalProperties: false, + properties: { + path: { type: "string", description: "Optional. Directory to list, relative to the working directory. Defaults to `.`." }, + }, + required: [], + }, + }, + }, + async execute(args) { + const path = args.path != null && String(args.path) ? String(args.path) : "."; + try { + const entries = await readdir(path, { withFileTypes: true }); + if (entries.length === 0) + return { ok: true, output: `# ${path} (empty)` }; + const names = entries + .map((e) => (e.isDirectory() ? `${e.name}/` : e.name)) + .sort((a, b) => a.localeCompare(b)); + return { ok: true, output: cap(`# ${path} (${entries.length} entries)\n${names.join("\n")}`) }; + } + catch (e) { + return { + ok: false, + output: `error listing ${path}: ${errorMessage(e)}. Check the path (relative to the working directory).`, + }; + } + }, +}; diff --git a/dist/llm/tools/read.js b/dist/llm/tools/read.js index 13fdf93..cfab3c8 100644 --- a/dist/llm/tools/read.js +++ b/dist/llm/tools/read.js @@ -1,30 +1,32 @@ -// Read tool — fetch a file's content from the working directory. +// Read tool — fetch a range of a file's lines from the working directory. // -// Returns the first MAX_LINES lines numbered (1-based), plus a header with -// the visible range and total count. For longer files, appends a footer -// with a ready-to-copy `sed` command for the next chunk. Smaller models -// loop on big files when they can't see where to ask next. +// Returns `lines` lines starting at `start_line` (1-based; defaults to the +// first 300 from line 1), each prefixed with its line number, plus a header +// showing the visible range and the file's total line count. When more lines +// remain below, the footer spells out the exact next Read call — so paging a +// long file is just repeated Reads with a higher start_line, no shell needed. import { readFile } from "node:fs/promises"; import { errorMessage } from "../../lib/errors.js"; import { cap } from "./types.js"; -const MAX_LINES = 300; +const DEFAULT_LINES = 300; +const MAX_LINES = 2000; export const readTool = { schema: { type: "function", function: { name: "Read", - description: "Read a file from the working directory. Returns up to the first 300 lines, " + - "each prefixed with its line number, plus a header showing the visible range " + - "and total line count. For files longer than 300 lines, use Bash to fetch " + - "the rest with `sed -n 'START,ENDp' path`, `tail -n N path`, or " + - "`grep -n 'pattern' path`. The footer of every Read result includes a " + - "ready-to-copy sed command for the next chunk. Path is relative to the " + - "working directory.", + description: "Read lines from a file in the working directory. By default returns the " + + "first 300 lines; pass `start_line` to begin elsewhere and `lines` to choose " + + "how many. Each line is prefixed with its line number, and the header shows " + + "the visible range and the file's total line count. To read further down, " + + "call Read again with a higher `start_line` (the footer shows the next call).", parameters: { type: "object", additionalProperties: false, properties: { path: { type: "string", description: "Path to the file, relative to the working directory." }, + start_line: { type: "number", description: "Optional. 1-based line to start from. Defaults to 1." }, + lines: { type: "number", description: "Optional. How many lines to return. Defaults to 300." }, }, required: ["path"], }, @@ -40,35 +42,48 @@ export const readTool = { `Example: {"path": "src/foo.ts"}`, }; } + let start = typeof args.start_line === "number" && Number.isFinite(args.start_line) ? Math.floor(args.start_line) : 1; + if (start < 1) + start = 1; + let count = typeof args.lines === "number" && Number.isFinite(args.lines) ? Math.floor(args.lines) : DEFAULT_LINES; + if (count <= 0) + count = DEFAULT_LINES; + if (count > MAX_LINES) + count = MAX_LINES; try { const content = await readFile(path, "utf-8"); - return { ok: true, output: cap(formatRead(path, content)) }; + return { ok: true, output: cap(formatRead(path, content, start, count)) }; } catch (e) { return { ok: false, output: `error reading ${path}: ${errorMessage(e)}. ` + - `Check the path (relative to the working directory) and try Bash ls/find if unsure.`, + `Check the path (relative to the working directory); use Ls or Glob if unsure.`, }; } }, }; -function formatRead(path, content) { - const lines = content.split(/\r?\n/); - const total = lines.length; - const visible = lines.slice(0, MAX_LINES); - const width = String(visible.length).length; - const numbered = visible.map((l, i) => `${String(i + 1).padStart(width, " ")}: ${l}`).join("\n"); - const header = `# ${path} (lines 1-${visible.length} of ${total})`; - if (total <= MAX_LINES) +function formatRead(path, content, start, count) { + const all = content.split(/\r?\n/); + const total = all.length; + if (start > total) { + return `# ${path} (${total} lines) — start_line ${start} is past the end of the file.`; + } + const from = start - 1; // 0-based slice index + const visible = all.slice(from, from + count); + const end = from + visible.length; // 1-based last line shown + const width = String(end).length; + const numbered = visible + .map((l, i) => `${String(start + i).padStart(width, " ")}: ${l}`) + .join("\n"); + const header = `# ${path} (lines ${start}-${end} of ${total})`; + if (end >= total) return `${header}\n${numbered}`; - const nextStart = MAX_LINES + 1; - const nextEnd = Math.min(total, MAX_LINES * 2); return [ header, numbered, ``, - `[... ${total - MAX_LINES} more lines]`, - `# Next chunk: Bash {"command": "sed -n '${nextStart},${nextEnd}p' ${path}"}`, + `[... ${total - end} more lines]`, + `# Continue with Read {"path": "${path}", "start_line": ${end + 1}}`, ].join("\n"); } diff --git a/dist/llm/tools/shared.js b/dist/llm/tools/shared.js deleted file mode 100644 index e70a2f8..0000000 --- a/dist/llm/tools/shared.js +++ /dev/null @@ -1,7 +0,0 @@ -// Shared types + helpers for LLM tools. -export const OUTPUT_CAP = 64 * 1024; // 64 KB per tool result is plenty; truncate beyond. -export function cap(s) { - if (s.length <= OUTPUT_CAP) - return s; - return s.slice(0, OUTPUT_CAP) + `\n\n[...output truncated; ${s.length - OUTPUT_CAP} more bytes dropped]`; -} diff --git a/dist/llm/tools/tail.js b/dist/llm/tools/tail.js new file mode 100644 index 0000000..e8f4e2d --- /dev/null +++ b/dist/llm/tools/tail.js @@ -0,0 +1,64 @@ +// Tail tool — the LAST lines of a file, like `tail -n`. +// +// Read shows the start of a file; Tail shows the end. Useful when a file is +// longer than Read's window and the relevant code is near the bottom. +import { readFile } from "node:fs/promises"; +import { errorMessage } from "../../lib/errors.js"; +import { cap } from "./types.js"; +const DEFAULT_LINES = 100; +const MAX_LINES = 1000; +export const tailTool = { + schema: { + type: "function", + function: { + name: "Tail", + description: "Read the last lines of a file, like `tail -n`. Returns the lines prefixed " + + "with their line numbers. Defaults to the last 100 lines; pass `lines` for more or fewer.", + parameters: { + type: "object", + additionalProperties: false, + properties: { + path: { type: "string", description: "Path to the file, relative to the working directory." }, + lines: { type: "number", description: "Optional. How many trailing lines to return. Defaults to 100." }, + }, + required: ["path"], + }, + }, + }, + async execute(args) { + const path = String(args.path ?? ""); + if (!path) { + return { + ok: false, + output: `Tail call rejected: missing required \`path\` argument. ` + + `Got: ${JSON.stringify(args)}. ` + + `Example: {"path": "src/foo.ts"}`, + }; + } + let n = typeof args.lines === "number" && Number.isFinite(args.lines) ? Math.floor(args.lines) : DEFAULT_LINES; + if (n <= 0) + n = DEFAULT_LINES; + if (n > MAX_LINES) + n = MAX_LINES; + try { + const content = await readFile(path, "utf-8"); + const all = content.split(/\r?\n/); + const total = all.length; + const start = Math.max(0, total - n); // 0-based index of first shown line + const visible = all.slice(start); + const width = String(total).length; + const numbered = visible + .map((l, i) => `${String(start + i + 1).padStart(width, " ")}: ${l}`) + .join("\n"); + const header = `# ${path} (lines ${start + 1}-${total} of ${total})`; + return { ok: true, output: cap(`${header}\n${numbered}`) }; + } + catch (e) { + return { + ok: false, + output: `error reading ${path}: ${errorMessage(e)}. ` + + `Check the path (relative to the working directory); use Ls or Glob if unsure.`, + }; + } + }, +}; diff --git a/dist/stages/reconcile/prompt.md b/dist/stages/reconcile/prompt.md index 1321b0a..b6cbdb4 100644 --- a/dist/stages/reconcile/prompt.md +++ b/dist/stages/reconcile/prompt.md @@ -18,7 +18,7 @@ You are running the verify pre-step of a follow-up code review. Your job: judge `inline.side: "head"` is anchored to a surviving head-side line. `inline.side: "base"` is anchored to a base-side (deleted) line. -You can call `Read` to inspect any file in the working directory (e.g. a caller, fixture, config) and `Bash` for things like `git log`, `git show`, `grep -rn`. Use them sparingly — most of what you need is already in ``. +You can call `Read`, `Tail`, `Grep`, `Ls`, and `Glob` to inspect the working tree (read a file, search for a symbol, list a directory, find files). Use them sparingly — most of what you need is already in ``. ## Emitting decisions diff --git a/dist/stages/review/prompt.md b/dist/stages/review/prompt.md index 22c1f35..ad408e3 100644 --- a/dist/stages/review/prompt.md +++ b/dist/stages/review/prompt.md @@ -35,7 +35,7 @@ Findings about an **absent** thing ("should also validate Y") anchor to the most ## Use your tools when unsure -You see only the **diff and the changed files** — not the whole repository. Whenever a judgment depends on something outside that view, use `Read` and `Bash` to check the real working tree instead of guessing: does a file / import / dependency / symbol exist, how is a function called elsewhere, what does a referenced value actually contain, is a case already handled in code you can't see. A finding asserted from the diff alone, when a quick `Read`/`grep`/`ls` could have confirmed or killed it, is a defect. +You see only the **diff and the changed files** — not the whole repository. Whenever a judgment depends on something outside that view, use `Read`, `Grep`, `Ls`, `Glob`, and `Tail` to check the real working tree instead of guessing: does a file / import / dependency / symbol exist, how is a function called elsewhere, what does a referenced value actually contain, is a case already handled in code you can't see. A finding asserted from the diff alone, when a quick `Read`/`Grep`/`Ls` could have confirmed or killed it, is a defect. This bites hardest with claims that something is **missing** — the diff shows only what changed, so it can never prove a thing is absent. Confirm before flagging ("not in the diff" is not "not in the repo"). And confirming a thing exists does not settle the finding — the real problem may still be there; investigate it. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b7fff06..23af576 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -6,7 +6,8 @@ process exit code *is* the verdict. This document is the map: structure as it is and the patterns the codebase commits to. Portable engineering rules live in [docs/conventions/](conventions/); dated decisions -(and their trade-offs) in [docs/adr/](adr/). +(and their trade-offs) in [docs/adr/](adr/). The working procedure that maintains all +three layers is the root [/CLAUDE.md](../CLAUDE.md) — agent sessions read it on start. --- @@ -37,7 +38,7 @@ flowchart TD direction LR client["client.ts
callLLM + resolveModel"] wire["transport.ts
HTTP + SSE wire"] - tools["tools/
Read · Bash"] + tools["tools/
Read · Grep · Ls · Glob · Tail"] client --> wire client --> tools end @@ -54,7 +55,7 @@ Two external worlds, each behind one port: posted. Five adapters: GitHub, GitLab, Bitbucket, `local://` (console echo), `mock://` (test recorder). - **LLM** (`llm/`) — any OpenAI-compatible `/chat/completions` endpoint, streamed, with a - local tool loop (`Read`, `Bash`) and schema-validated JSON output. + local tool loop (`Read`, `Grep`, `Ls`, `Glob`, `Tail` — read-only, no shell) and schema-validated JSON output. Everything else is orchestration around a single state object. @@ -172,6 +173,7 @@ One interface, five adapters. Key commitments: Tools follow a registry pattern: each tool is `{ schema, execute }` in its own file, collected in `tools/index.ts`. Tool errors are written *for the model* — explicit about what was received and what to do next, because weaker models loop on terse errors. +The tool set itself is constrained — read-only, no shell (4.11, ADR-0014). ### 4.5 Prompt assembly — colocated assets + tagged slots (ADR-0006) @@ -206,7 +208,7 @@ that genuinely needs reading code. ### 4.9 Safety gates are environmental, not flags -`local://` review (LLM gets unsandboxed `Bash`/`Read`) requires an actual container, +`local://` review (LLM gets read-only file tools `Read`/`Grep`/`Ls`/`Glob`/`Tail`, no shell) requires an actual container, detected via runtime-created files (`/.dockerenv`, `/run/.containerenv`) — not an env flag anyone could export. Same idea elsewhere: bot identity is derived from the token (`GET /user`), not from a CI-shaped env variable. @@ -220,6 +222,15 @@ the mock for assertions. Assertions favor structural guarantees (what got posted bucket a file landed in) over fragile text matching of LLM output. Pure logic (summarize) is unit-tested with hand-built state — no LLM, no fixture. +### 4.11 LLM tools: read-only, shell-free (ADR-0014) + +The model's input is untrusted PR content, and the container holds live VCS + LLM +credentials — so no tool gives the model a shell. `Read`/`Tail`/`Ls`/`Glob` are pure +`node:fs` operations; `Grep` spawns the `grep` binary with an argv array (`-e` keeps +a leading-dash pattern data, not a flag). The invariant for any future tool: it may +read; if it must spawn, it spawns a binary with an argv array — model input is never +tokenized by a shell, so `|`, `;`, `$()` stay inert data. + ## 5. Conventions The portable rule set (naming, types placement, consolidation, error handling, @@ -230,7 +241,9 @@ configuration, logging, testing, documentation) lives in **Language / compiler.** TypeScript, `strict` + `noUncheckedIndexedAccess`, ESM, Node ≥ 24. Source runs directly via `--experimental-transform-types` (Docker, `pnpm -start`); `dist/` is compiled output committed for npm consumers. Imports use explicit +start`); `dist/` is compiled output committed for npm consumers — `pnpm build` wipes +`dist/` before compiling, so deleted or renamed sources can't leave stale artifacts +behind. Imports use explicit `.ts` extensions (`rewriteRelativeImportExtensions` handles emit). `import type` for type-only imports. Node built-ins use the `node:` prefix. diff --git a/docs/adr/0000-adr-scope.md b/docs/adr/0000-adr-scope.md new file mode 100644 index 0000000..1c30341 --- /dev/null +++ b/docs/adr/0000-adr-scope.md @@ -0,0 +1,44 @@ +# ADR-0000: What lands in this log (root of all roots) + +Status: Accepted (2026-06-10) +Date: 2026-06-10 + +## Context + +Adopted from the sibling harness repo (which itself adopted this repo's docs +structure). Its log started accumulating entries that aren't architecture — a +bug fix dressed up as a decision, a working-procedure write-up restating +`/CLAUDE.md`. Each seemed reasonable alone; together they dilute the log — a +reader can no longer assume an ADR marks a structural choice that future work +depends on. Every log sharing this structure needs the same gate. + +## Decision + +An entry belongs here only when **all three** hold: + +1. **It's architectural** — it shapes the system's structure, contracts, or + boundaries (a port, a layer rule, a wire format, a gating model). +2. **A real choice existed** — alternatives with trade-offs were on the table, + and the why deserves to outlive the conversation that decided it. +3. **Future work depends on it** — someone building later acts differently + because this was decided. + +What does **not** land here, and where it goes instead: + +- **Bug fixes** — correctness has no alternatives. The record is the commit + message, the regression test, and (when the fix pins down a contract) a + why-comment at the code and a line in the map. +- **Process and procedures** — how we work lives in `/CLAUDE.md`, which is + updated directly when the workflow changes. No ADR mirrors it. +- **Restatements** — content that lives elsewhere is pointed at, never copied + (the [documentation convention](../conventions/documentation.md) already says + this for convention adoptions; it applies to everything). + +## Consequences + +- The log stays a trustworthy index of structural decisions; "is there an ADR?" + remains a meaningful question. +- Some genuinely useful records live only in commits, tests, and CLAUDE.md — + acceptable, because each has a defined home and the map links the layers. +- This ADR is numbered 0000 because it governs the log itself; entries predating + it were grandfathered without re-review. diff --git a/docs/adr/0014-llm-tools-read-only-no-shell.md b/docs/adr/0014-llm-tools-read-only-no-shell.md new file mode 100644 index 0000000..5b838de --- /dev/null +++ b/docs/adr/0014-llm-tools-read-only-no-shell.md @@ -0,0 +1,55 @@ +# ADR-0014: LLM tools are read-only and shell-free + +Status: Accepted (2026-06-10) + +## Context + +The review and reconcile stages hand the LLM a tool loop against the working +tree. Until now that included a `Bash` tool (`bash -lc `), justified by +the container being a throwaway sandbox. + +The threat model says otherwise. The model's input is **untrusted PR content** — +diffs, commit messages, prior comments — exactly the place a prompt injection +lives. And the container is not empty: it holds a live VCS token and an LLM API +key. A shell turns an injected instruction into command execution; container +isolation limits the blast radius to precisely the things the container must +contain to do its job — the credentials and the checkout. + +Three options were on the table: + +1. **Keep `Bash`, rely on the container.** Rejected: the sandbox boundary is in + the wrong place — the secrets are inside it. +2. **Keep `Bash` behind a command allowlist.** Rejected: deciding what a shell + string does requires parsing shell, and `git -c core.pager=…`-style flag + smuggling means even an "obviously safe" binary allowlist leaks back to + arbitrary execution. +3. **Replace the shell with structured read-only tools.** Chosen. + +## Decision + +The LLM's tool set is `Read`, `Grep`, `Ls`, `Glob`, `Tail` — structured +arguments, read-only, and **never a shell**: + +- `Read`/`Tail`/`Ls`/`Glob` are pure `node:fs` operations. `Read` pages via + `start_line`/`lines`, so long files no longer need a shell escape hatch. +- `Grep` spawns the `grep` binary with an **argv array** (and `-e` so a pattern + starting with `-` is data, not a flag). No `exec`, no command string, no + `{ shell: true }` — model-supplied text is never tokenized by a shell, so + `|`, `;`, `$()` stay inert data. + +The invariant for future tools: a tool may read; if it must spawn, it spawns a +binary with an argv array and model input only ever appears as an argument. +Nothing the model sends is interpreted by a shell. + +## Consequences + +- Injected instructions in PR content can no longer execute commands or write + anything; the worst case is reading files the process can already read. The + container gate for `local://` review (`inContainer()` in `ctx.ts`) remains, + now guarding read access rather than shell access. +- The review loses `git log` / `git blame` / `git show` as evidence sources. + Accepted for now; if history matters later, the path is a dedicated + structured `GitLog`-style tool obeying the invariant — not a shell. +- Each navigation primitive is a separate tool file in the registry, written + for the model (explicit errors, next-call hints in footers), per the + existing tool conventions. diff --git a/docs/adr/README.md b/docs/adr/README.md index 3c5f4a9..9f23c9b 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -1,12 +1,15 @@ # Architecture Decision Records Dated, immutable decision log. Format `NNNN-title.md`; statuses -Proposed → Accepted → Superseded. Current-state rules live in +Proposed → Accepted → Superseded. Scope is gated by +[ADR-0000](0000-adr-scope.md): architectural decisions only — bug fixes and +procedures don't land here. Current-state rules live in [../conventions/](../conventions/); the system map in [../ARCHITECTURE.md](../ARCHITECTURE.md). | # | Decision | | --- | --- | +| [0000](0000-adr-scope.md) | What lands in this log: architectural decisions only (bug fixes → commits; procedures → `/CLAUDE.md`) | | [0001](0001-ctx-dependency-injection-and-env-isolation.md) | Ctx DI container; `process.env` read only in `loadConfig` | | [0002](0002-stage-pipeline-uniform-stagestate.md) | Stage pipeline with uniform `StageState` | | [0003](0003-provider-port-with-static-async-factories.md) | `Provider` port, static async factories, semantic verdict ops | @@ -20,3 +23,4 @@ Proposed → Accepted → Superseded. Current-state rules live in | [0011](0011-canonical-open-bot-thread-predicate.md) | Canonical `isOpenBotThread` predicate | | [0012](0012-summary-comment-fragment-constants.md) | Shared fragment constants for summary detection | | [0013](0013-adopt-shared-conventions.md) | Adopt `docs/conventions/` (naming, types placement, consolidation, errors, config, logging, testing, docs) | +| [0014](0014-llm-tools-read-only-no-shell.md) | LLM tools are read-only and shell-free (no `Bash`; argv-only subprocess) | diff --git a/docs/conventions/README.md b/docs/conventions/README.md index 1c0f61a..ff9472a 100644 --- a/docs/conventions/README.md +++ b/docs/conventions/README.md @@ -18,6 +18,13 @@ This repo's instance-specific documentation lives in | [logging.md](logging.md) | Structured events with dot-scoped children; data objects, never interpolation | | [testing.md](testing.md) | Mock at the port with side-effect recorders; real engines; structural assertions | | [documentation.md](documentation.md) | Three doc layers (map / conventions / ADRs); Mermaid for diagrams; why-comments | +| [i18n.md](i18n.md) | Every user-facing string through `t()`; locale files stay key-for-key; constants store keys | +| [react.md](react.md) | Named function components; hooks-only state access; stable list keys; no floating rejections | +| [constants-and-identifiers.md](constants-and-identifiers.md) | Behavioral literals are named; one id generator (seeds are not ids); namespaced persisted keys | + +The UI-layer topics (i18n, react, constants-and-identifiers) were contributed by +the harness project (its ADR-0011) — this CLI repo doesn't exercise them, but +they ship with the set so any UI project adopting it gets them. ## Adopting in a new project diff --git a/docs/conventions/constants-and-identifiers.md b/docs/conventions/constants-and-identifiers.md new file mode 100644 index 0000000..ff83652 --- /dev/null +++ b/docs/conventions/constants-and-identifiers.md @@ -0,0 +1,37 @@ +# Constants and identifiers + +**Rule.** A value that changes behavior gets a name; a value that identifies an +entity comes from one generator; a value that persists gets a namespaced key. +Unnamed literals are where behavior hides. + +## Rules + +1. **Behavioral literals are named.** Any number that changes what the program + does — timeouts, poll intervals, retry counts, token budgets, size caps, + window minimums — is a named `UPPER_SNAKE` module-level const next to its + use site (`GREP_TIMEOUT_MS`, `OUTPUT_CAP`, `MAX_HEAL_ATTEMPTS`). The name + states the unit (`_MS`, `_BYTES`) when one exists; the comment states why + that value (see documentation.md — heuristics carry their rationale inline). +2. **Presentation literals stay inline.** Pixel sizes, spacing classes, icon + dimensions — naming them adds indirection without meaning. The test: would a + different value change *behavior* or just *looks*? +3. **One id generator.** Entity and call ids come from `crypto.randomUUID()` + (or the platform's single equivalent) — never `Math.random()`, timestamps, + or counters for identity. Mixed generators mean mixed collision properties + and unsearchable id formats. +4. **Seeds are not ids.** A numeric value fed to a generator *as randomness* + (an RNG seed for image/video generation) is not an identifier — it may use + `Math.random()` and stays numeric. Mark it as a seed in name and comment so + a cleanup pass doesn't "fix" it into a UUID. +5. **Persisted keys are namespaced and named.** Every localStorage/IndexedDB/ + file-cache key lives under one app prefix (`:`) and is a named + module-level constant, never an inline string. The prefix makes the app's + total persisted footprint greppable and prevents collisions with anything + else sharing the origin; the constant makes renames a one-line migration + site instead of a scavenger hunt. + +## Why + +Inline literals are invisible at review time and unfindable at debug time. The +naming cost is one line; the payoff is that grep answers "what are this app's +timeouts / keys / budgets" instead of a code-reading session. diff --git a/docs/conventions/i18n.md b/docs/conventions/i18n.md new file mode 100644 index 0000000..3b6f918 --- /dev/null +++ b/docs/conventions/i18n.md @@ -0,0 +1,39 @@ +# i18n: every user-facing string through the translation layer + +**Rule.** A user-facing string that bypasses the i18n layer is invisible debt: it +ships untranslated and nobody notices until a user in the other language hits it. +Locale files stay key-for-key identical; completeness must be diffable, not +discovered. + +## Rules + +1. **Everything user-facing goes through `t()`** — visible text, `title=` + tooltips, `placeholder=`, button labels, option labels and hints, status + lines. The only exemptions are developer-facing debug output, log events, + and **language-neutral example literals** (a placeholder showing a URL + shape, a dimension like `1024x1024`, a bare number) — translating those is + noise. A placeholder containing any prose word is not exempt. The exemption + list is deliberately tiny so the rule stays mechanical. +2. **Keys are `scope.key`**, scope = the owning feature (`provider.title`, + `workspace.modeAsk`). A key's scope tells the reader where it renders. +3. **Key parity is an invariant, not a goal.** Every locale file holds exactly + the same key set; adding a key to one file without the others is a defect. + Untranslated fallback text in a secondary locale is an acceptable stopgap — + a missing key is not. Parity is checked by diffing key sets, which makes + translation completeness a one-liner instead of a manual audit. +4. **Constants store keys, not strings.** A module-level option list (labels, + hints, modes) can't call `t()` at definition time — it stores i18n keys + (`labelKey`, `hintKey`) and translates at render. Never hardcode the label + into the constant "because it's just three words". +5. **Plurals and interpolation use the library's conventions** (`_other` + suffixes, `{{var}}` slots) — never string-concatenate around a translated + fragment, which breaks word order in other languages. +6. **Markup inside a translated sentence** goes through the library's component + interpolation (e.g. `Trans` with a components map), not by splitting the + sentence into fragments — fragments translate wrong. + +## Why + +Hardcoded strings are greppable only if you know to grep; key parity is +diffable by anyone. Both properties turn "is the app translated?" from a QA +pass in every language into a mechanical check. diff --git a/docs/conventions/react.md b/docs/conventions/react.md new file mode 100644 index 0000000..e10ad21 --- /dev/null +++ b/docs/conventions/react.md @@ -0,0 +1,42 @@ +# React components + +**Rule.** Components are plain named functions over typed props, reading state +through hooks. The component layer renders; it does not own engine state or +reach into store internals. + +## Rules + +1. **Named function declarations, exported by name** — `export function + SessionView(...)`. No `React.FC`, no default exports: named exports keep + imports greppable and rename-safe; `React.FC` adds nothing but an implicit + `children`. +2. **Props are typed inline or via a local interface.** Destructure in the + parameter list for small prop sets; destructure in the body (or use + `props.x`) when the list is long enough that the signature stops being + readable. +3. **State access is hooks-only.** Components call `use()` hooks; they + never import a store object or call its getters directly. The hook is the + subscription; direct reads silently skip re-renders. +4. **List keys use a stable identity when items have one** (an id, a unique + url). Index keys are acceptable only for immutable render-only lists — + content that is appended and re-rendered but never reordered or removed + item-by-item. An editable list with index keys is a reorder bug waiting. +5. **Event handlers are `on`** (`onPickFiles`, `onScroll`) — both the + prop name and the local function. +6. **Async handlers never leak rejections.** Wrap awaited work in try/catch or + call APIs that return result objects; deliberate fire-and-forget is an + explicit `void fn()`, so every floating promise is visibly intentional. +7. **Inline `style` only for values computed at runtime** (a percent width, a + measured height). Static styling belongs in classes — a hardcoded inline + style is a class that escaped review. +8. **Components stay small and extractable.** A page component that grows + nested function components (a card, a row, a block with its own state) ships + them as their own files — nested definitions can't be reused or tested and + re-create on every render of the parent. + +## Why + +These rules make the component layer boring on purpose: every component looks +the same shape, state flows one way through hooks, and nothing in the render +tree holds engine logic hostage. Boring components are the ones you can move, +split, and test without archaeology. diff --git a/package.json b/package.json index 409c307..e5cf483 100644 --- a/package.json +++ b/package.json @@ -11,7 +11,7 @@ "publishConfig": { "access": "public" }, "scripts": { "start": "node --experimental-transform-types --disable-warning=ExperimentalWarning src/cli.ts", - "build": "tsc -p tsconfig.build.json && node scripts/copy-assets.mjs", + "build": "node -e \"fs.rmSync('dist',{recursive:true,force:true})\" && tsc -p tsconfig.build.json && node scripts/copy-assets.mjs", "typecheck": "tsc --noEmit", "test:inner": "vitest run", "test": "docker build --build-arg BUILD_TARGET=test -t reviewer-test . && docker run --rm --entrypoint pnpm --env-file tests/.env -v \"$PWD/node_modules:/reviewer/node_modules\" -w /reviewer reviewer-test run test:inner" diff --git a/src/ctx.ts b/src/ctx.ts index c22a878..6e40fa8 100644 --- a/src/ctx.ts +++ b/src/ctx.ts @@ -186,15 +186,15 @@ export function loadConfig(opts: CliOverrides): Config { case "mock": break; case "local": - // Local review gives the LLM the Bash/Read tools against the real - // checkout. Those tools assume a throwaway sandbox; running on a bare - // host would hand an LLM unsandboxed shell + file access to your - // machine. Require an actual container — detect it rather than trust an - // env flag (which any caller could set on the host). + // Local review gives the LLM read-only file tools (Read/Grep/Ls/Glob/Tail) + // against the real checkout. They can't run shell, but they can read any + // file the process can reach; on a bare host that's your whole machine. + // Require an actual container — detect it rather than trust an env flag + // (which any caller could set on the host). if (!inContainer()) { throw new Error( - "local review must run inside a container (the LLM gets shell + file access, " + - "and the container is the sandbox). Run:\n" + + "local review must run inside a container (the LLM can read any file the " + + "process can reach, and the container is the sandbox). Run:\n" + " docker build -t reviewer .\n" + " docker run --rm -v \"$PWD:/workspace\" -w /workspace --env-file .env \\\n" + " reviewer review --pr local://main", diff --git a/src/llm/client.ts b/src/llm/client.ts index d29eebe..992448e 100644 --- a/src/llm/client.ts +++ b/src/llm/client.ts @@ -100,7 +100,7 @@ export interface CallLLMArgs { systemSections: Section[]; userSections: Section[]; schema: object; - // When true, expose Read + Bash tools. Default true. + // When true, expose the read-only tools (Read, Grep, Ls, Glob, Tail). Default true. tools?: boolean; } diff --git a/src/llm/tools/bash.ts b/src/llm/tools/bash.ts deleted file mode 100644 index 2a67779..0000000 --- a/src/llm/tools/bash.ts +++ /dev/null @@ -1,70 +0,0 @@ -// Bash tool — run a shell command in the working directory. -// -// The container is throwaway and isolated, so this tool doesn't sandbox -// paths or restrict commands — the LLM has whatever the container ships -// with (git, grep, find, rg if installed, etc.). - -import { spawn } from "node:child_process"; - -import { cap, type Tool, type ToolResult } from "./types.ts"; - -export const bashTool: Tool = { - schema: { - type: "function", - function: { - name: "Bash", - description: - "Run a shell command in the working directory. Returns combined stdout+stderr plus the exit code. Use for `git log`, `git blame`, `git show`, `grep -rn`, `find`, listing dirs, etc. The container is throwaway — no path restrictions, but state does not persist between calls.", - parameters: { - type: "object", - additionalProperties: false, - properties: { - command: { type: "string", description: "The shell command to run. Executed via `bash -lc`." }, - timeout_seconds: { type: "number", description: "Optional. Defaults to 60." }, - }, - required: ["command"], - }, - }, - }, - - async execute(args) { - const command = String(args.command ?? ""); - const timeoutMs = typeof args.timeout_seconds === "number" ? args.timeout_seconds * 1000 : 60_000; - if (!command) { - return { - ok: false, - output: - `Bash call rejected: missing required \`command\` argument. ` + - `Got: ${JSON.stringify(args)}. ` + - `Example: {"command": "ls -la"}`, - }; - } - return runBash(command, timeoutMs); - }, -}; - -function runBash(command: string, timeoutMs: number): Promise { - return new Promise((resolve) => { - const proc = spawn("bash", ["-lc", command], { stdio: ["ignore", "pipe", "pipe"] }); - let out = ""; - let timedOut = false; - const timer = setTimeout(() => { - timedOut = true; - proc.kill("SIGKILL"); - }, timeoutMs); - - proc.stdout.on("data", (b: Buffer) => { out += b.toString("utf-8"); }); - proc.stderr.on("data", (b: Buffer) => { out += b.toString("utf-8"); }); - proc.on("close", (code) => { - clearTimeout(timer); - const suffix = timedOut - ? `\n[exit: killed after ${timeoutMs}ms timeout]` - : `\n[exit: ${code}]`; - resolve({ ok: !timedOut && code === 0, output: cap(out) + suffix }); - }); - proc.on("error", (e) => { - clearTimeout(timer); - resolve({ ok: false, output: `error spawning bash: ${e.message}` }); - }); - }); -} diff --git a/src/llm/tools/glob.ts b/src/llm/tools/glob.ts new file mode 100644 index 0000000..e429051 --- /dev/null +++ b/src/llm/tools/glob.ts @@ -0,0 +1,60 @@ +// Glob tool — find files by path pattern, like a shell glob / `find -name`. +// +// Pure filesystem walk via node:fs/promises glob, no subprocess. Noise +// directories are pruned and results are capped so a broad pattern can't +// flood the model's context. + +import { glob } from "node:fs/promises"; + +import { errorMessage } from "../../lib/errors.ts"; +import { cap, type Tool } from "./types.ts"; + +const MAX_RESULTS = 500; +const EXCLUDE = ["node_modules", ".git", "dist"]; + +export const globTool: Tool = { + schema: { + type: "function", + function: { + name: "Glob", + description: + "Find files whose path matches a glob pattern, e.g. `src/**/*.ts` or " + + "`**/*.test.js`. Returns matching file paths.", + parameters: { + type: "object", + additionalProperties: false, + properties: { + pattern: { type: "string", description: "Glob pattern to match file paths against, e.g. `src/**/*.ts`." }, + }, + required: ["pattern"], + }, + }, + }, + + async execute(args) { + const pattern = String(args.pattern ?? ""); + if (!pattern) { + return { + ok: false, + output: + `Glob call rejected: missing required \`pattern\` argument. ` + + `Got: ${JSON.stringify(args)}. ` + + `Example: {"pattern": "src/**/*.ts"}`, + }; + } + try { + const matches: string[] = []; + const isExcluded = (p: string) => p.split(/[\\/]/).some((seg) => EXCLUDE.includes(seg)); + for await (const m of glob(pattern, { exclude: isExcluded })) { + matches.push(m); + if (matches.length >= MAX_RESULTS) break; + } + if (matches.length === 0) return { ok: true, output: `No files match ${JSON.stringify(pattern)}.` }; + matches.sort((a, b) => a.localeCompare(b)); + const note = matches.length >= MAX_RESULTS ? `\n[... truncated at ${MAX_RESULTS} matches]` : ""; + return { ok: true, output: cap(`# ${matches.length} match(es) for ${pattern}\n${matches.join("\n")}${note}`) }; + } catch (e) { + return { ok: false, output: `error globbing ${pattern}: ${errorMessage(e)}.` }; + } + }, +}; diff --git a/src/llm/tools/grep.ts b/src/llm/tools/grep.ts new file mode 100644 index 0000000..3d00b13 --- /dev/null +++ b/src/llm/tools/grep.ts @@ -0,0 +1,101 @@ +// Grep tool — search file contents for a regex under a path, like `grep -rn`. +// +// Two mandatory params: `pattern` (regex) and `path` (where to search). We +// never go through a shell: runGrep spawns the `grep` binary with an ARGV +// ARRAY, so the model-supplied pattern/path are literal arguments, never +// tokenized by a shell — `|`, `;`, `$()`, quotes are inert data, not +// operators (this is exactly why there is no Bash tool: a shell would turn +// them into commands). NEVER rebuild this with `exec`, a command string, or +// `{ shell: true }` — that reintroduces arbitrary command execution. + +import { spawn } from "node:child_process"; +import { existsSync } from "node:fs"; + +import { cap, type Tool } from "./types.ts"; + +const TIMEOUT_MS = 30_000; + +export const grepTool: Tool = { + schema: { + type: "function", + function: { + name: "Grep", + description: + "Search file contents for a pattern (regular expression), like `grep -rn`. " + + "Returns matching lines as `path:line:text` with a couple of lines of " + + "surrounding context. You must say where to search via `path` — scope it to " + + "the folder you expect the match in (e.g. `src`), or point at a specific " + + "file or dependency directory.", + parameters: { + type: "object", + additionalProperties: false, + properties: { + pattern: { type: "string", description: "Text or regular expression to search for." }, + path: { type: "string", description: "File or directory to search in, e.g. `src` or `src/foo.ts`." }, + }, + required: ["pattern", "path"], + }, + }, + }, + + async execute(args) { + const pattern = String(args.pattern ?? ""); + const path = String(args.path ?? ""); + if (!pattern || !path) { + return { + ok: false, + output: + `Grep call rejected: both \`pattern\` and \`path\` are required. ` + + `Got: ${JSON.stringify(args)}. ` + + `Example: {"pattern": "myFunction", "path": "src"}`, + }; + } + if (!existsSync(path)) { + return { + ok: false, + output: + `Grep call rejected: path ${JSON.stringify(path)} does not exist. ` + + `Pass a file or directory that exists (use Ls or Glob to locate it).`, + }; + } + const r = await runGrep(pattern, path); + if (r.timedOut) return { ok: false, output: `Grep timed out searching ${path}.` }; + if (r.code === 0) return { ok: true, output: cap(r.output) }; + if (r.code === 1) return { ok: true, output: `No matches for ${JSON.stringify(pattern)} in ${path}.` }; + return { ok: false, output: `Grep failed (exit ${r.code}): ${r.output.trim() || "unknown error"}` }; + }, +}; + +interface GrepResult { + code: number | null; + output: string; // combined stdout + stderr + timedOut: boolean; +} + +// Run `grep` with an argv array (no shell — see the file header) and collect +// its output, killing it on a timeout. grep exit codes: 0 = matches, 1 = no +// matches, 2 = error; the caller translates them. +function runGrep(pattern: string, path: string): Promise { + // `-e` marks the pattern so one starting with `-` is data, not a flag; + // -r recurses, -n adds line numbers, -C 2 gives a little context. + const args = ["-rn", "-C", "2", "-e", pattern, path]; + return new Promise((resolve) => { + const proc = spawn("grep", args, { stdio: ["ignore", "pipe", "pipe"] }); + let out = ""; + let timedOut = false; + const timer = setTimeout(() => { + timedOut = true; + proc.kill("SIGKILL"); + }, TIMEOUT_MS); + proc.stdout.on("data", (b: Buffer) => { out += b.toString("utf-8"); }); + proc.stderr.on("data", (b: Buffer) => { out += b.toString("utf-8"); }); + proc.on("close", (code) => { + clearTimeout(timer); + resolve({ code, output: out, timedOut }); + }); + proc.on("error", (e) => { + clearTimeout(timer); + resolve({ code: null, output: `error spawning grep: ${e.message}`, timedOut }); + }); + }); +} diff --git a/src/llm/tools/index.ts b/src/llm/tools/index.ts index 7fa4705..05e29e6 100644 --- a/src/llm/tools/index.ts +++ b/src/llm/tools/index.ts @@ -12,13 +12,21 @@ import { errorMessage } from "../../lib/errors.ts"; import type { ToolCall } from "../types.ts"; -import { bashTool } from "./bash.ts"; +import { globTool } from "./glob.ts"; +import { grepTool } from "./grep.ts"; +import { lsTool } from "./ls.ts"; import { readTool } from "./read.ts"; +import { tailTool } from "./tail.ts"; import type { Tool, ToolResult } from "./types.ts"; export type { ToolResult } from "./types.ts"; -const TOOLS: Tool[] = [readTool, bashTool]; +// Read-only navigation only — no shell. A `Bash` tool was removed deliberately: +// `bash -lc` (and even `git -c core.pager=…`) is arbitrary command execution, +// and the review input is untrusted PR content while the container holds live +// VCS + LLM credentials. These tools take structured args and never spawn a +// shell, so the same input is inert data. Keep it that way. +const TOOLS: Tool[] = [readTool, grepTool, tailTool, lsTool, globTool]; const BY_NAME = new Map(TOOLS.map((t) => [t.schema.function.name, t])); const VALID_NAMES = TOOLS.map((t) => t.schema.function.name); diff --git a/src/llm/tools/ls.ts b/src/llm/tools/ls.ts new file mode 100644 index 0000000..d2ff19b --- /dev/null +++ b/src/llm/tools/ls.ts @@ -0,0 +1,45 @@ +// Ls tool — list a directory, like `ls`. +// +// Pure filesystem read, no subprocess. Directories get a trailing `/`. + +import { readdir } from "node:fs/promises"; + +import { errorMessage } from "../../lib/errors.ts"; +import { cap, type Tool } from "./types.ts"; + +export const lsTool: Tool = { + schema: { + type: "function", + function: { + name: "Ls", + description: + "List the entries of a directory, like `ls`. Directories are shown with a " + + "trailing `/`. Defaults to the working directory.", + parameters: { + type: "object", + additionalProperties: false, + properties: { + path: { type: "string", description: "Optional. Directory to list, relative to the working directory. Defaults to `.`." }, + }, + required: [], + }, + }, + }, + + async execute(args) { + const path = args.path != null && String(args.path) ? String(args.path) : "."; + try { + const entries = await readdir(path, { withFileTypes: true }); + if (entries.length === 0) return { ok: true, output: `# ${path} (empty)` }; + const names = entries + .map((e) => (e.isDirectory() ? `${e.name}/` : e.name)) + .sort((a, b) => a.localeCompare(b)); + return { ok: true, output: cap(`# ${path} (${entries.length} entries)\n${names.join("\n")}`) }; + } catch (e) { + return { + ok: false, + output: `error listing ${path}: ${errorMessage(e)}. Check the path (relative to the working directory).`, + }; + } + }, +}; diff --git a/src/llm/tools/read.ts b/src/llm/tools/read.ts index 1b869ce..17fc02c 100644 --- a/src/llm/tools/read.ts +++ b/src/llm/tools/read.ts @@ -1,16 +1,18 @@ -// Read tool — fetch a file's content from the working directory. +// Read tool — fetch a range of a file's lines from the working directory. // -// Returns the first MAX_LINES lines numbered (1-based), plus a header with -// the visible range and total count. For longer files, appends a footer -// with a ready-to-copy `sed` command for the next chunk. Smaller models -// loop on big files when they can't see where to ask next. +// Returns `lines` lines starting at `start_line` (1-based; defaults to the +// first 300 from line 1), each prefixed with its line number, plus a header +// showing the visible range and the file's total line count. When more lines +// remain below, the footer spells out the exact next Read call — so paging a +// long file is just repeated Reads with a higher start_line, no shell needed. import { readFile } from "node:fs/promises"; import { errorMessage } from "../../lib/errors.ts"; import { cap, type Tool } from "./types.ts"; -const MAX_LINES = 300; +const DEFAULT_LINES = 300; +const MAX_LINES = 2000; export const readTool: Tool = { schema: { @@ -18,18 +20,18 @@ export const readTool: Tool = { function: { name: "Read", description: - "Read a file from the working directory. Returns up to the first 300 lines, " + - "each prefixed with its line number, plus a header showing the visible range " + - "and total line count. For files longer than 300 lines, use Bash to fetch " + - "the rest with `sed -n 'START,ENDp' path`, `tail -n N path`, or " + - "`grep -n 'pattern' path`. The footer of every Read result includes a " + - "ready-to-copy sed command for the next chunk. Path is relative to the " + - "working directory.", + "Read lines from a file in the working directory. By default returns the " + + "first 300 lines; pass `start_line` to begin elsewhere and `lines` to choose " + + "how many. Each line is prefixed with its line number, and the header shows " + + "the visible range and the file's total line count. To read further down, " + + "call Read again with a higher `start_line` (the footer shows the next call).", parameters: { type: "object", additionalProperties: false, properties: { path: { type: "string", description: "Path to the file, relative to the working directory." }, + start_line: { type: "number", description: "Optional. 1-based line to start from. Defaults to 1." }, + lines: { type: "number", description: "Optional. How many lines to return. Defaults to 300." }, }, required: ["path"], }, @@ -47,35 +49,45 @@ export const readTool: Tool = { `Example: {"path": "src/foo.ts"}`, }; } + let start = typeof args.start_line === "number" && Number.isFinite(args.start_line) ? Math.floor(args.start_line) : 1; + if (start < 1) start = 1; + let count = typeof args.lines === "number" && Number.isFinite(args.lines) ? Math.floor(args.lines) : DEFAULT_LINES; + if (count <= 0) count = DEFAULT_LINES; + if (count > MAX_LINES) count = MAX_LINES; try { const content = await readFile(path, "utf-8"); - return { ok: true, output: cap(formatRead(path, content)) }; + return { ok: true, output: cap(formatRead(path, content, start, count)) }; } catch (e) { return { ok: false, output: `error reading ${path}: ${errorMessage(e)}. ` + - `Check the path (relative to the working directory) and try Bash ls/find if unsure.`, + `Check the path (relative to the working directory); use Ls or Glob if unsure.`, }; } }, }; -function formatRead(path: string, content: string): string { - const lines = content.split(/\r?\n/); - const total = lines.length; - const visible = lines.slice(0, MAX_LINES); - const width = String(visible.length).length; - const numbered = visible.map((l, i) => `${String(i + 1).padStart(width, " ")}: ${l}`).join("\n"); - const header = `# ${path} (lines 1-${visible.length} of ${total})`; - if (total <= MAX_LINES) return `${header}\n${numbered}`; - const nextStart = MAX_LINES + 1; - const nextEnd = Math.min(total, MAX_LINES * 2); +function formatRead(path: string, content: string, start: number, count: number): string { + const all = content.split(/\r?\n/); + const total = all.length; + if (start > total) { + return `# ${path} (${total} lines) — start_line ${start} is past the end of the file.`; + } + const from = start - 1; // 0-based slice index + const visible = all.slice(from, from + count); + const end = from + visible.length; // 1-based last line shown + const width = String(end).length; + const numbered = visible + .map((l, i) => `${String(start + i).padStart(width, " ")}: ${l}`) + .join("\n"); + const header = `# ${path} (lines ${start}-${end} of ${total})`; + if (end >= total) return `${header}\n${numbered}`; return [ header, numbered, ``, - `[... ${total - MAX_LINES} more lines]`, - `# Next chunk: Bash {"command": "sed -n '${nextStart},${nextEnd}p' ${path}"}`, + `[... ${total - end} more lines]`, + `# Continue with Read {"path": "${path}", "start_line": ${end + 1}}`, ].join("\n"); } diff --git a/src/llm/tools/tail.ts b/src/llm/tools/tail.ts new file mode 100644 index 0000000..c11dd49 --- /dev/null +++ b/src/llm/tools/tail.ts @@ -0,0 +1,69 @@ +// Tail tool — the LAST lines of a file, like `tail -n`. +// +// Read shows the start of a file; Tail shows the end. Useful when a file is +// longer than Read's window and the relevant code is near the bottom. + +import { readFile } from "node:fs/promises"; + +import { errorMessage } from "../../lib/errors.ts"; +import { cap, type Tool } from "./types.ts"; + +const DEFAULT_LINES = 100; +const MAX_LINES = 1000; + +export const tailTool: Tool = { + schema: { + type: "function", + function: { + name: "Tail", + description: + "Read the last lines of a file, like `tail -n`. Returns the lines prefixed " + + "with their line numbers. Defaults to the last 100 lines; pass `lines` for more or fewer.", + parameters: { + type: "object", + additionalProperties: false, + properties: { + path: { type: "string", description: "Path to the file, relative to the working directory." }, + lines: { type: "number", description: "Optional. How many trailing lines to return. Defaults to 100." }, + }, + required: ["path"], + }, + }, + }, + + async execute(args) { + const path = String(args.path ?? ""); + if (!path) { + return { + ok: false, + output: + `Tail call rejected: missing required \`path\` argument. ` + + `Got: ${JSON.stringify(args)}. ` + + `Example: {"path": "src/foo.ts"}`, + }; + } + let n = typeof args.lines === "number" && Number.isFinite(args.lines) ? Math.floor(args.lines) : DEFAULT_LINES; + if (n <= 0) n = DEFAULT_LINES; + if (n > MAX_LINES) n = MAX_LINES; + try { + const content = await readFile(path, "utf-8"); + const all = content.split(/\r?\n/); + const total = all.length; + const start = Math.max(0, total - n); // 0-based index of first shown line + const visible = all.slice(start); + const width = String(total).length; + const numbered = visible + .map((l, i) => `${String(start + i + 1).padStart(width, " ")}: ${l}`) + .join("\n"); + const header = `# ${path} (lines ${start + 1}-${total} of ${total})`; + return { ok: true, output: cap(`${header}\n${numbered}`) }; + } catch (e) { + return { + ok: false, + output: + `error reading ${path}: ${errorMessage(e)}. ` + + `Check the path (relative to the working directory); use Ls or Glob if unsure.`, + }; + } + }, +}; diff --git a/src/stages/reconcile/prompt.md b/src/stages/reconcile/prompt.md index 1321b0a..b6cbdb4 100644 --- a/src/stages/reconcile/prompt.md +++ b/src/stages/reconcile/prompt.md @@ -18,7 +18,7 @@ You are running the verify pre-step of a follow-up code review. Your job: judge `inline.side: "head"` is anchored to a surviving head-side line. `inline.side: "base"` is anchored to a base-side (deleted) line. -You can call `Read` to inspect any file in the working directory (e.g. a caller, fixture, config) and `Bash` for things like `git log`, `git show`, `grep -rn`. Use them sparingly — most of what you need is already in ``. +You can call `Read`, `Tail`, `Grep`, `Ls`, and `Glob` to inspect the working tree (read a file, search for a symbol, list a directory, find files). Use them sparingly — most of what you need is already in ``. ## Emitting decisions diff --git a/src/stages/review/prompt.md b/src/stages/review/prompt.md index 22c1f35..ad408e3 100644 --- a/src/stages/review/prompt.md +++ b/src/stages/review/prompt.md @@ -35,7 +35,7 @@ Findings about an **absent** thing ("should also validate Y") anchor to the most ## Use your tools when unsure -You see only the **diff and the changed files** — not the whole repository. Whenever a judgment depends on something outside that view, use `Read` and `Bash` to check the real working tree instead of guessing: does a file / import / dependency / symbol exist, how is a function called elsewhere, what does a referenced value actually contain, is a case already handled in code you can't see. A finding asserted from the diff alone, when a quick `Read`/`grep`/`ls` could have confirmed or killed it, is a defect. +You see only the **diff and the changed files** — not the whole repository. Whenever a judgment depends on something outside that view, use `Read`, `Grep`, `Ls`, `Glob`, and `Tail` to check the real working tree instead of guessing: does a file / import / dependency / symbol exist, how is a function called elsewhere, what does a referenced value actually contain, is a case already handled in code you can't see. A finding asserted from the diff alone, when a quick `Read`/`Grep`/`Ls` could have confirmed or killed it, is a defect. This bites hardest with claims that something is **missing** — the diff shows only what changed, so it can never prove a thing is absent. Confirm before flagging ("not in the diff" is not "not in the repo"). And confirming a thing exists does not settle the finding — the real problem may still be there; investigate it. diff --git a/src/stages/types.ts b/src/stages/types.ts index 45f6499..04b35e9 100644 --- a/src/stages/types.ts +++ b/src/stages/types.ts @@ -61,7 +61,7 @@ export interface StageContext { projectContext: string; // CLAUDE.md sections + standards, rendered reviewChecklist: string; // review checklist file, noise-stripped - // Where the reviewer is running. LLM tool calls (Read, Bash) execute here. + // Where the reviewer is running. LLM tool calls (Read, Grep, Ls, Glob, Tail) execute here. workingDir: { cwd: string }; // Per-file content the LLM sees. fetch populates from diff classification;