From d7404a86022329e964fcd9961ebbe6d5a452fe40 Mon Sep 17 00:00:00 2001 From: Luca Giordano Date: Tue, 30 Jun 2026 09:12:28 +0000 Subject: [PATCH] fix: make reviewer produce pass single-pass (maxIterations: 1) (#80) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract buildReviewerPassOneConfig() pure function from ReviewerAdapter that returns pass-1 run options with maxIterations: 1. The reviewer produces a complete review in a single sandcastle iteration — re-running up to 8× wasted tokens and widened the window for transient failures. Unit-tests the pure config builder in reviewer-adapter.test.ts, mirroring the buildAgentInput pattern from sandbox-runner.test.ts. Pass-2 extraction, fail-safe, and reduce.ts are untouched. --- .sandcastle/package.json | 2 +- .sandcastle/reviewer-adapter.test.ts | 57 ++++++++++++++++++++++++++++ .sandcastle/reviewer-adapter.ts | 49 ++++++++++++++++++------ 3 files changed, 95 insertions(+), 13 deletions(-) create mode 100644 .sandcastle/reviewer-adapter.test.ts diff --git a/.sandcastle/package.json b/.sandcastle/package.json index 1293cb0..877e328 100644 --- a/.sandcastle/package.json +++ b/.sandcastle/package.json @@ -7,7 +7,7 @@ "scripts": { "start": "tsx main.ts", "typecheck": "tsc --noEmit", - "test": "node --import tsx --test reduce.test.ts sandbox-runner.test.ts run-sh.test.ts init-sh.test.ts up-sh.test.ts afk-cmd.test.ts", + "test": "node --import tsx --test reduce.test.ts sandbox-runner.test.ts reviewer-adapter.test.ts run-sh.test.ts init-sh.test.ts up-sh.test.ts afk-cmd.test.ts", "test:integration": "SANDCASTLE_INTEGRATION=1 node --import tsx --test integration.test.ts" }, "devDependencies": { diff --git a/.sandcastle/reviewer-adapter.test.ts b/.sandcastle/reviewer-adapter.test.ts new file mode 100644 index 0000000..423851b --- /dev/null +++ b/.sandcastle/reviewer-adapter.test.ts @@ -0,0 +1,57 @@ +/** + * Unit tests for buildReviewerPassOneConfig — the pure function that returns + * the produce-pass run options for the reviewer adapter. + * No Docker, GitHub, or network required. + * + * Run: npm test + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { buildReviewerPassOneConfig } from "./reviewer-adapter.ts"; + +const STUB_INPUT = { + issueNumber: 42, + issueTitle: "Fix the bug", + issueBody: "Detailed description", + branch: "agent/issue-42", + prNumber: 7, +}; + +test("produce pass is capped at a single iteration", () => { + const config = buildReviewerPassOneConfig({}, STUB_INPUT); + assert.equal(config.maxIterations, 1); +}); + +test("produce pass uses review-prompt.md template", () => { + const config = buildReviewerPassOneConfig({}, STUB_INPUT); + assert.ok(config.promptFile.endsWith("review-prompt.md"), `expected review-prompt.md, got ${config.promptFile}`); +}); + +test("promptArgs include issue number, title, body, branch, and base branch", () => { + const config = buildReviewerPassOneConfig({}, STUB_INPUT); + assert.equal(config.promptArgs["ISSUE_NUMBER"], "42"); + assert.equal(config.promptArgs["ISSUE_TITLE"], "Fix the bug"); + assert.equal(config.promptArgs["ISSUE_BODY"], "Detailed description"); + assert.equal(config.promptArgs["BRANCH"], "agent/issue-42"); + assert.equal(config.promptArgs["BASE_BRANCH"], "main"); +}); + +test("baseBranch defaults to main", () => { + const config = buildReviewerPassOneConfig({}, STUB_INPUT); + assert.equal(config.promptArgs["BASE_BRANCH"], "main"); +}); + +test("baseBranch is overridable via options", () => { + const config = buildReviewerPassOneConfig({ baseBranch: "develop" }, STUB_INPUT); + assert.equal(config.promptArgs["BASE_BRANCH"], "develop"); +}); + +test("name includes the issue number", () => { + const config = buildReviewerPassOneConfig({}, STUB_INPUT); + assert.ok(config.name.includes("42"), `expected name to include issue number, got ${config.name}`); +}); + +test("ISSUE_BODY defaults to (no body) when empty", () => { + const config = buildReviewerPassOneConfig({}, { ...STUB_INPUT, issueBody: "" }); + assert.equal(config.promptArgs["ISSUE_BODY"], "(no body)"); +}); diff --git a/.sandcastle/reviewer-adapter.ts b/.sandcastle/reviewer-adapter.ts index a609672..9a0b540 100644 --- a/.sandcastle/reviewer-adapter.ts +++ b/.sandcastle/reviewer-adapter.ts @@ -109,6 +109,41 @@ export function filterInlineComments( return comments.filter((c) => diffLines.has(`${c.path}:${c.line}`)); } +/** Pass-1 (produce) run options returned by buildReviewerPassOneConfig. */ +export interface ReviewerPassOneConfig { + readonly promptFile: string; + readonly promptArgs: Record; + readonly name: string; + // A review is a single-pass job: read diff → emit verdict → done. + // maxIterations: 1 prevents sandcastle re-invoking the agent toward a + // spurious second pass (observed at 8× in issue #80). + readonly maxIterations: 1; +} + +/** + * Pure function: resolve ReviewerOptions + ReviewInput to the produce-pass + * run options — no Docker, no network. Mirrors buildAgentInput in + * sandbox-runner.ts; the caller wires in agent/sandbox/cwd before calling run(). + */ +export function buildReviewerPassOneConfig( + opts: ReviewerOptions, + input: ReviewInput, +): ReviewerPassOneConfig { + const baseBranch = opts.baseBranch ?? "main"; + return { + promptFile: join(_dir, "review-prompt.md"), + promptArgs: { + ISSUE_NUMBER: String(input.issueNumber), + ISSUE_TITLE: input.issueTitle, + ISSUE_BODY: input.issueBody || "(no body)", + BRANCH: input.branch, + BASE_BRANCH: baseBranch, + }, + name: `review-issue-${input.issueNumber}`, + maxIterations: 1, + }; +} + export class ReviewerAdapter { private readonly opts: ReviewerOptions; @@ -123,16 +158,9 @@ export class ReviewerAdapter { */ async review(input: ReviewInput): Promise { const model = this.opts.model ?? "claude-sonnet-4-6"; - const baseBranch = this.opts.baseBranch ?? "main"; const cwd = this.opts.cwd; - const promptArgs = { - ISSUE_NUMBER: String(input.issueNumber), - ISSUE_TITLE: input.issueTitle, - ISSUE_BODY: input.issueBody || "(no body)", - BRANCH: input.branch, - BASE_BRANCH: baseBranch, - }; + const passOneConfig = buildReviewerPassOneConfig(this.opts, input); // Pass 1: produce — the reviewer reads the diff and forms a free-form review. let reviewResult; @@ -141,10 +169,7 @@ export class ReviewerAdapter { agent: claudeCode(model, { permissionMode: "auto" }), sandbox: noSandbox(), cwd, - promptFile: join(_dir, "review-prompt.md"), - promptArgs, - name: `review-issue-${input.issueNumber}`, - maxIterations: 8, + ...passOneConfig, }); } catch (err) { console.warn(`[reviewer] pass-1 (produce) failed for #${input.issueNumber}:`, err);