Security: replace Bash LLM tool with read-only, shell-free tools

.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/

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 <branch-name>`
+  (plain `git push` fails here — the SSH key isn't in the default agent).

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 <url>`).
@@ -79,7 +79,7 @@ docker run --rm -t --env-file .env -v "$PWD:/workspace" -w /workspace \
 
 `local://<base>` diffs `<base>...HEAD` (base defaults to `main`). Use `local://<branch-or-sha>` 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.

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");

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)}.` };
+        }
+    },
+};

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 });
+        });
+    });
+}

dist/llm/tools/index.js

@@ -10,9 +10,17 @@
 // Add a new tool: create tools/<name>.ts exporting `<name>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);