diff --git a/.changeset/review-pi-tool-surface.md b/.changeset/review-pi-tool-surface.md new file mode 100644 index 00000000..8247b61b --- /dev/null +++ b/.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. diff --git a/workflows/review/eval/harness-probe.ts b/workflows/review/eval/harness-probe.ts index 8296b870..6ac44c0d 100644 --- a/workflows/review/eval/harness-probe.ts +++ b/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"; diff --git a/workflows/review/eval/live-runner.ts b/workflows/review/eval/live-runner.ts index a1ab034b..102902fd 100644 --- a/workflows/review/eval/live-runner.ts +++ b/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 | undefined; return async (request) => { - runner ??= createPiRunner({allowedTools: ALLOWED_TOOLS}); + runner ??= createPiRunner(); return (await runner)(request); }; }; diff --git a/workflows/review/eval/sandbox-smoke.ts b/workflows/review/eval/sandbox-smoke.ts index 7be83246..7258d33e 100644 --- a/workflows/review/eval/sandbox-smoke.ts +++ b/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: * diff --git a/workflows/review/lib/dispatch-runner-pi.test.ts b/workflows/review/lib/dispatch-runner-pi.test.ts index 0598df9c..6c7c633c 100644 --- a/workflows/review/lib/dispatch-runner-pi.test.ts +++ b/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 () => { diff --git a/workflows/review/lib/dispatch-runner-pi.ts b/workflows/review/lib/dispatch-runner-pi.ts index f3c32eb3..bf70c640 100644 --- a/workflows/review/lib/dispatch-runner-pi.ts +++ b/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 => { */ 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[]; /**