review: trim the sub-agent tool surface to Read/Grep/Bash, and window Read

.changeset/review-pi-tool-surface.md

@@ -0,0 +1,25 @@
+---
+"review": minor
+---
+
+review: trim the sub-agent tool surface to Read/Grep/Bash, and window Read
+
+Two tools leave `createReviewTools`, raised in review on #305: LS wrapped
+`ls -la` verbatim and added nothing over sandboxed Bash, and Glob's
+`find -path` emulation was wrong rather than merely limited (`*` matched
+across `/`, so a reviewer asking for `src/*.ts` silently received nested
+files too). Directory listing and file finding go through Bash, where the
+model owns the semantics of its own command. Every tool runs through the
+same sandboxed executor, so the named tools that remain earn their place on
+model ergonomics, not containment: Read for windowed, line-numbered file
+views, Grep because structured params avoid the shell-quoting failure class.
+
+Read gains `offset`/`limit` windowing. Previously a large file was silently
+truncated at the output cap and its tail was unreachable, a recall defect;
+the window keeps `cat -n` line numbers so findings still anchor on real
+lines, and a partial view says which lines of how many it shows.
+
+The eval's measured arms now run this production surface unrestricted
+(previously they were pinned to Read/Grep/Glob, a surface production never
+ran), so the A/B measures what production ships, by construction. The
+re-anchoring run for the new surface is on the PR.

workflows/review/eval/harness-probe.ts

@@ -59,8 +59,14 @@ import {matchCase} from "./live-match";
 import {produceLive, type LiveAgentRunner} from "./live-producer";
 import {runCase} from "./runner";
 
-/** The eval's measured tool surface; held fixed so the probe varies one thing. */
-const ALLOWED_TOOLS = ["Read", "Grep", "Glob"];
+/**
+ * The tool surface the lost-finding A/B ran on, held fixed so the probe
+ * varies one thing. Glob has since been removed from `createReviewTools`
+ * (its `find -path` emulation was wrong), so the closest reproduction the
+ * current lib can offer is Read/Grep; the probe's question (system prompt
+ * vs structured final) does not turn on Glob.
+ */
+const ALLOWED_TOOLS = ["Read", "Grep"];
 
 const CASE_ID = "incident-sql-missing-index";
 

workflows/review/eval/live-runner.ts

@@ -8,10 +8,14 @@
  * runtime. `live-producer.ts` stays runner-free behind its seam, so unit
  * tests never load Pi's libraries.
  *
- * Tool policy: read-only investigation (Read/Grep/Glob), cwd pinned to the
- * staged checkout, no network. The investigation-cap CLI the prompts mention
- * is not runnable under this policy; the prompts' own fallback applies (a
- * denied budget request stops investigation, findings still report).
+ * Tool policy: the production surface, unrestricted (Read/Grep/Bash from
+ * `createReviewTools`), cwd pinned to the staged checkout, no network. The
+ * eval measures the surface production runs, by construction; the old
+ * three-tool restriction (Read/Grep/Glob) measured a surface production
+ * never used. The investigation-cap CLI the prompts mention has no staged
+ * routing in the corpus checkouts, so a reviewer that tries it gets the
+ * prompts' own fallback (a denied budget request stops investigation,
+ * findings still report).
  *
  * Run one case end to end (requires ANTHROPIC_API_KEY):
  *
@@ -32,12 +36,9 @@ import {extractAgents} from "./agent-extract";
 import {loadLiveCorpus} from "./corpus/loader";
 import {produceLive, type LiveAgentRunner} from "./live-producer";
 
-/** Read-only investigation tools; see the module doc for the rationale. */
-const ALLOWED_TOOLS = ["Read", "Grep", "Glob"];
-
 /**
- * The eval runner: the production Pi harness pinned to the eval's three-tool
- * surface (the corpus was measured on Read/Grep/Glob). Lazily constructed so
+ * The eval runner: the production Pi harness on the production tool surface
+ * (no `allowedTools` restriction; see the module doc). Lazily constructed so
  * importing this module never requires Pi's libraries; both A/B arms share
  * one instance, which also shares its lazy sandbox initialization.
  *
@@ -57,7 +58,7 @@ export const piRunner = (): LiveAgentRunner => {
     // of every finder but one on the first case.
     let runner: ReturnType<typeof createPiRunner> | undefined;
     return async (request) => {
-        runner ??= createPiRunner({allowedTools: ALLOWED_TOOLS});
+        runner ??= createPiRunner();
         return (await runner)(request);
     };
 };

workflows/review/eval/sandbox-smoke.ts

@@ -2,15 +2,15 @@
  * The sandbox smoke: does the PRODUCTION tool surface actually work inside the
  * srt sandbox?
  *
- * Why this is separate from the A/B arms. The measured arms run on
- * Read/Grep/Glob (`live-runner.ts`), because the corpus was calibrated on that
- * three-tool surface and changing it would move every quality number. But
- * production grants the full surface — Read/Grep/Glob/LS/Bash, with Bash
- * running the investigation-cap CLI, which is the one thing in the review that
- * must WRITE inside a sandbox whose whole point is that writes are denied. So
- * the A/B measures quality on a surface production does not run, and nothing
- * measured the surface production DOES run. This job closes that gap: it
- * proves the sandbox, never quality, and reports no metrics.
+ * Why this is separate from the A/B arms. The measured arms now run the
+ * production surface too (`live-runner.ts` grants `createReviewTools`
+ * unrestricted), so the A/B owns quality-on-the-production-surface. This job
+ * keeps a different franchise: it exercises the sandbox BOUNDARY itself
+ * (deny-side probes, the cap journal's one allowed write, Bash reached
+ * live), proves the sandbox rather than quality, and reports no metrics.
+ * Bash matters most: it runs the investigation-cap CLI, the one thing in the
+ * review that must WRITE inside a sandbox whose whole point is that writes
+ * are denied.
  *
  * Two phases, deliberately split by determinism:
  *

workflows/review/lib/dispatch-runner-pi.test.ts

@@ -15,6 +15,7 @@ import {
     rejectStaleRunnerSelection,
     resolveModelId,
     shellQuote,
+    windowLines,
 } from "./dispatch-runner-pi";
 
 /**
@@ -243,8 +244,6 @@ describe("createReviewTools", () => {
         expect(createReviewTools("/tmp").map((tool) => tool.name)).toEqual([
             "Read",
             "Grep",
-            "Glob",
-            "LS",
             "Bash",
         ]);
     });
@@ -268,6 +267,30 @@ describe("createReviewTools", () => {
         expect(result?.content[0].text).toContain("1");
     });
 
+    it("windows a Read with offset and limit, keeping real line numbers", async () => {
+        const dir = mkdtempSync(join(tmpdir(), "pi-runner-"));
+        const body = Array.from({length: 50}, (_, i) => `line ${i + 1}`).join(
+            "\n",
+        );
+        writeFileSync(join(dir, "big.ts"), `${body}\n`);
+        const read = createReviewTools(dir).find(
+            (tool) => tool.name === "Read",
+        );
+        const result = await read?.execute("1", {
+            path: "big.ts",
+            offset: 10,
+            limit: 3,
+        });
+        const text = result?.content[0].text ?? "";
+        expect(text).toContain("line 10");
+        expect(text).toContain("line 12");
+        expect(text).not.toContain("line 13");
+        // `cat -n` numbering survives the window: the model can anchor
+        // findings on real line numbers, not window-relative ones.
+        expect(text).toMatch(/10\tline 10/);
+        expect(text).toContain("[showing lines 10-12 of 50]");
+    });
+
     it("reports a grep miss as an ordinary result, not a tool failure", async () => {
         const dir = mkdtempSync(join(tmpdir(), "pi-runner-"));
         writeFileSync(join(dir, "a.ts"), "const a = 1;\n");
@@ -280,6 +303,38 @@ describe("createReviewTools", () => {
     });
 });
 
+describe("windowLines", () => {
+    const numbered = "     1\ta\n     2\tb\n     3\tc\n     4\td\n";
+
+    it("returns the text untouched when no window is asked for", () => {
+        expect(windowLines(numbered)).toBe(numbered);
+        expect(windowLines(numbered, undefined, undefined)).toBe(numbered);
+    });
+
+    it("slices from offset and notes what was left out", () => {
+        expect(windowLines(numbered, 2, 2)).toBe(
+            "     2\tb\n     3\tc\n[showing lines 2-3 of 4]",
+        );
+    });
+
+    it("omits the note when the window covers the whole file", () => {
+        expect(windowLines(numbered, 1, 100)).toBe(
+            "     1\ta\n     2\tb\n     3\tc\n     4\td",
+        );
+    });
+
+    it("says so when the offset is past the end of the file", () => {
+        expect(windowLines(numbered, 99)).toBe(
+            "(no lines in window: the file has 4 lines, offset was 99)",
+        );
+    });
+
+    it("ignores non-numeric and non-positive window params", () => {
+        expect(windowLines(numbered, "2", "1")).toBe(numbered);
+        expect(windowLines(numbered, 0, -5)).toBe(numbered);
+    });
+});
+
 describe("shellQuote", () => {
     it("single-quotes each argv part", () => {
         expect(shellQuote(["grep", "-n", "a b"])).toBe("'grep' '-n' 'a b'");
@@ -497,15 +552,14 @@ describe("createPiRunner", () => {
             return Promise.resolve([]);
         };
         const runner = await createPiRunner({
-            allowedTools: ["Read", "Grep", "Glob"],
+            allowedTools: ["Read", "Grep"],
         });
         await runner(request());
-        expect(names).toEqual(["Read", "Grep", "Glob"]);
-        // The excluded tools must not reach the agent at all: an unregistered
+        expect(names).toEqual(["Read", "Grep"]);
+        // The excluded tool must not reach the agent at all: an unregistered
         // tool cannot be called, which is the guarantee this seam rests on
         // (Pi has no permission layer to fall back to).
         expect(names).not.toContain("Bash");
-        expect(names).not.toContain("LS");
     });
 
     it("grants the full surface when allowedTools is omitted", async () => {
@@ -516,7 +570,7 @@ describe("createPiRunner", () => {
         };
         const runner = await createPiRunner();
         await runner(request());
-        expect(names).toEqual(["Read", "Grep", "Glob", "LS", "Bash"]);
+        expect(names).toEqual(["Read", "Grep", "Bash"]);
     });
 
     it("keeps submit_result available under a restricted surface", async () => {

workflows/review/lib/dispatch-runner-pi.ts

@@ -207,6 +207,43 @@ const ok = (text: string): PiToolResult => ({
     details: {},
 });
 
+/**
+ * Window a `cat -n` capture to `limit` lines starting at the 1-indexed
+ * `offset`, saying what was left out. The subprocess always reads the whole
+ * file (the sandbox boundary lives on the subprocess, so the window is about
+ * the model's view, not about I/O). Without this, a large file was silently
+ * truncated at MAX_TOOL_OUTPUT_CHARS and its tail was unreachable — a recall
+ * defect in a reviewer, not a nicety.
+ */
+export const windowLines = (
+    text: string,
+    offset?: unknown,
+    limit?: unknown,
+): string => {
+    const start =
+        typeof offset === "number" && offset > 0 ? Math.floor(offset) : 1;
+    const max =
+        typeof limit === "number" && limit > 0 ? Math.floor(limit) : undefined;
+    if (start === 1 && max === undefined) {
+        return text;
+    }
+    const lines = text.replace(/\n$/, "").split("\n");
+    const total = lines.length;
+    const window = lines.slice(
+        start - 1,
+        max === undefined ? undefined : start - 1 + max,
+    );
+    if (window.length === 0) {
+        return `(no lines in window: the file has ${total} lines, offset was ${start})`;
+    }
+    const end = start + window.length - 1;
+    const note =
+        start > 1 || end < total
+            ? `\n[showing lines ${start}-${end} of ${total}]`
+            : "";
+    return window.join("\n") + note;
+};
+
 /**
  * Spawn one argv, resolving with its combined output. A non-zero exit is
  * NOT an error here: `grep` exits 1 on no-match, and the model needs to see
@@ -305,10 +342,20 @@ const schema = (
 const str = (description: string): unknown => ({type: "string", description});
 
 /**
- * The reviewer tool surface: read-only investigation plus Bash (the
+ * The reviewer tool surface: Read and Grep for investigation, plus Bash (the
  * investigation-cap CLI the sub-agent prompts invoke runs through it). No
  * edit, no write — and with the sandboxed executor that is a mount-level
  * boundary on Bash too, not just a tool-surface promise.
+ *
+ * Deliberately small. Every tool here runs through the same sandboxed
+ * executor as Bash, so a named tool earns its place on model ergonomics, not
+ * on containment: Read gives windowed, line-numbered file views, and Grep's
+ * structured params avoid the shell-quoting failure class (a model quoting a
+ * regex into `bash -lc` botches it often enough to add noise). Two former
+ * tools were removed as adding nothing over Bash: LS (`ls -la` verbatim) and
+ * Glob, whose `find -path` emulation was wrong, not just limited (`*`
+ * matched across `/`, so reviewers got a wider file list than they asked
+ * for). Raised by mojadem on #305.
  */
 export const createReviewTools = (
     cwd: string,
@@ -317,19 +364,26 @@ export const createReviewTools = (
     {
         name: "Read",
         label: "Read",
-        // KNOWN LIMIT: no offset/limit windowing; the whole file is read and
-        // truncated at MAX_TOOL_OUTPUT_CHARS, so a sub-agent reading a large
-        // file gets a silently narrower view than a windowing Read would
-        // give. Relevant when a reviewer "misses" something in a big file.
         description:
-            "Read a file from the repository. Returns the file with 1-indexed line numbers.",
+            "Read a file from the repository. Returns the file with 1-indexed line numbers. Use offset and limit to window large files.",
         parameters: schema(
-            {path: str("Path to the file, relative to the repository root.")},
+            {
+                path: str("Path to the file, relative to the repository root."),
+                offset: {
+                    type: "number",
+                    description: "1-indexed line number to start reading from.",
+                },
+                limit: {
+                    type: "number",
+                    description: "Maximum number of lines to return.",
+                },
+            },
             ["path"],
         ),
         execute: async (_id, params, signal) => {
             const path = String(params["path"] ?? "");
-            return ok(await exec(["cat", "-n", "--", path], cwd, signal));
+            const out = await exec(["cat", "-n", "--", path], cwd, signal);
+            return ok(windowLines(out, params["offset"], params["limit"]));
         },
     },
     {
@@ -364,55 +418,11 @@ export const createReviewTools = (
             );
         },
     },
-    {
-        name: "Glob",
-        label: "Glob",
-        // KNOWN LIMIT: `find -path` matches `*` ACROSS `/`, so `src/*.ts`
-        // also matches nested files that a real glob library would exclude,
-        // and `**` differs too. Documented rather than normalized because it
-        // widens rather than narrows the view.
-        description: "Find files whose path matches a glob pattern.",
-        parameters: schema(
-            {pattern: str("Glob pattern, e.g. 'src/**/*.ts'.")},
-            ["pattern"],
-        ),
-        execute: async (_id, params, signal) => {
-            const pattern = String(params["pattern"] ?? "");
-            return ok(
-                await exec(
-                    [
-                        "find",
-                        ".",
-                        "-not",
-                        "-path",
-                        "./.git/*",
-                        "-path",
-                        `./${pattern}`,
-                    ],
-                    cwd,
-                    signal,
-                ),
-            );
-        },
-    },
-    {
-        name: "LS",
-        label: "LS",
-        description: "List the entries of a directory.",
-        parameters: schema(
-            {path: str("Directory to list, relative to the repository root.")},
-            ["path"],
-        ),
-        execute: async (_id, params, signal) => {
-            const path = String(params["path"] ?? ".");
-            return ok(await exec(["ls", "-la", "--", path], cwd, signal));
-        },
-    },
     {
         name: "Bash",
         label: "Bash",
         description:
-            "Run a shell command in the repository. Use for the investigation-cap CLI and other read-only checks. Commands run inside an OS sandbox: the repository is read-only and there is no network access.",
+            "Run a shell command in the repository. Use for the investigation-cap CLI, listing or finding files, and other read-only checks. Commands run inside an OS sandbox: the repository is read-only and there is no network access.",
         parameters: schema({command: str("The shell command to run.")}, [
             "command",
         ]),
@@ -598,10 +608,10 @@ export const createToolExec = async (): Promise<ToolExec> => {
  */
 export type PiRunnerOptions = {
     /**
-     * Restrict the tool surface to these names. Used by the eval harness,
-     * which allows only Read/Grep/Glob (the corpus was measured on that
-     * three-tool surface). Omitted in production, where the full surface
-     * (including Bash, which the investigation-cap CLI needs) is granted.
+     * Restrict the tool surface to these names. Production and the eval's
+     * measured arms both omit it (the eval measures the surface production
+     * runs, by construction); the harness probe uses it to reproduce a
+     * historical configuration.
      */
     allowedTools?: string[];
     /**