From 3c27b2535b2ccb484ff37cee396932324d89da95 Mon Sep 17 00:00:00 2001 From: Fadi Shehadeh Date: Mon, 29 Jun 2026 15:00:44 -0400 Subject: [PATCH 01/13] feat(prompts): inject commit message into review prompt Render the change's commit-message body ("the why") as a dedicated `## Commit message` section in the review prompt so the agent reasons about intent, not just the diff. Omitted when the description is empty or whitespace-only to avoid injecting a blank section. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/review/reviewPromptBuilder.ts | 12 +++++++++- tests/unit/reviewPromptBuilder.test.ts | 31 ++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/review/reviewPromptBuilder.ts b/src/review/reviewPromptBuilder.ts index 24253a99..317b10a3 100644 --- a/src/review/reviewPromptBuilder.ts +++ b/src/review/reviewPromptBuilder.ts @@ -59,10 +59,20 @@ export function buildReviewPrompt(input: ReviewPromptInput): string { const sections = [ header, + ]; + + // Commit message body ("the why"). Omitted when the change has no description + // beyond its subject so we never inject an empty section. + const description = details.description.trim(); + if (description.length > 0) { + sections.push(``, `## Commit message`, description); + } + + sections.push( ``, `## User Instructions`, userPrompt, - ]; + ); if (priorComments && priorComments.length > 0) { sections.push(``, `## Already reported (do not repeat)`, renderPriorComments(priorComments)); diff --git a/tests/unit/reviewPromptBuilder.test.ts b/tests/unit/reviewPromptBuilder.test.ts index a8761f48..ebbfd53b 100644 --- a/tests/unit/reviewPromptBuilder.test.ts +++ b/tests/unit/reviewPromptBuilder.test.ts @@ -148,6 +148,37 @@ describe("buildReviewPrompt", () => { }); }); +describe("buildReviewPrompt commit message", () => { + it("renders the commit message body when the description is non-empty", () => { + const prompt = buildReviewPrompt({ + details: { ...details, description: "Implements rate limiting.\n\nCloses #42." }, + diff, + userPrompt: "Review this.", + }); + expect(prompt).toContain("## Commit message"); + expect(prompt).toContain("Implements rate limiting."); + expect(prompt).toContain("Closes #42."); + }); + + it("omits the commit message section when the description is empty", () => { + const prompt = buildReviewPrompt({ + details: { ...details, description: "" }, + diff, + userPrompt: "Review this.", + }); + expect(prompt).not.toContain("## Commit message"); + }); + + it("omits the commit message section when the description is whitespace only", () => { + const prompt = buildReviewPrompt({ + details: { ...details, description: " \n \n" }, + diff, + userPrompt: "Review this.", + }); + expect(prompt).not.toContain("## Commit message"); + }); +}); + describe("buildReviewPrompt discussion threads", () => { it("omits the open-threads section when none are provided", () => { const prompt = buildReviewPrompt({ details, diff, userPrompt: "Review this." }); From 9e171095e11729ba284ac8e39906d32c8dffdf5c Mon Sep 17 00:00:00 2001 From: Fadi Shehadeh Date: Mon, 29 Jun 2026 15:02:08 -0400 Subject: [PATCH 02/13] fix(review): decouple verdict re-post from discussion replies The silent-re-review gate previously required `repliesToPost.length === 0`, so delivering a discussion reply on an otherwise-unchanged re-review forced the summary + vote to be re-posted, spamming the change. Drop that clause so the verdict is skipped purely on whether anything new was found and the vote is unchanged, while pending replies are always delivered through their own independent path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/review/reviewOrchestrator.ts | 13 ++++++------ tests/unit/reviewOrchestrator.test.ts | 29 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/review/reviewOrchestrator.ts b/src/review/reviewOrchestrator.ts index 85c51927..eb91d79b 100644 --- a/src/review/reviewOrchestrator.ts +++ b/src/review/reviewOrchestrator.ts @@ -601,10 +601,12 @@ export class ReviewOrchestrator { repliesToPost.push({ threadId: reply.threadId, message, handledHash: entry.handledHash }); } - // Avoid re-posting an identical verdict on re-reviews. When a pass - // finds nothing new to say (no inline comments, no folded notes, no - // replies) and the overall vote matches the last review cycle, stay - // silent instead of spamming another summary + vote notification. + // Avoid re-posting an identical verdict on re-reviews. When a pass finds + // nothing new to say (no inline comments, no folded notes) and the overall + // vote matches the last review cycle, stay silent instead of spamming + // another summary + vote notification. This gate is decoupled from + // discussion replies: a pending reply is always delivered through its own + // path below and never forces the verdict to be re-posted. const hasNothingNew = commentsToPost.length === 0 && folded.length === 0; const previousVote = await this.getLastReviewVote(taskId); const skipPosting = @@ -612,8 +614,7 @@ export class ReviewOrchestrator { cycleNumber > 1 && hasNothingNew && previousVote !== null && - previousVote === vote && - repliesToPost.length === 0; + previousVote === vote; emitReviewEvent("review.posting_comments", { commentCount: commentsToPost.length, diff --git a/tests/unit/reviewOrchestrator.test.ts b/tests/unit/reviewOrchestrator.test.ts index b6c58f41..483f39a6 100644 --- a/tests/unit/reviewOrchestrator.test.ts +++ b/tests/unit/reviewOrchestrator.test.ts @@ -1168,6 +1168,35 @@ describe("ReviewOrchestrator.runReview - discussion replies", () => { expect(postThreadReply).toHaveBeenCalledWith(CHANGE_ID, 2, "disc-1", "Replying here."); expect(mocks.store.markThreadReplyPosted).toHaveBeenCalledOnce(); + // The verdict (summary + vote) is unchanged, so it must NOT be re-posted just + // because a reply was delivered — replies and the verdict are decoupled. + expect(mocks.provider.postReviewComments).not.toHaveBeenCalled(); + expect(mocks.provider.vote).not.toHaveBeenCalled(); + }); + + it("re-posts the verdict alongside a reply when a genuinely new finding exists", async () => { + const initial = makeTask({ state: "REVIEW_WATCHING", cycleCount: 1 }); + const mocks = makeMocks(initial); + // Prior vote was 0; this pass surfaces a new blocking comment, so the + // verdict genuinely changed and must be posted even though a reply also goes out. + (mocks.store.getAgentCycles as ReturnType).mockResolvedValue([ + { result: { metadata: { vote: 0 } } }, + ]); + const postThreadReply = withThreads(mocks, [makeThread()]); + const { runner } = makeWorkspaceRunner( + rawWithReplies([{ threadId: "disc-1", message: "Replying here." }], { + comments: [{ file: "src/a.ts", line: 1, message: "Brand new bug", severity: "error" }], + summary: "blocking", + score: -1, + }) + ); + const orch = new ReviewOrchestrator(makeDeps(mocks, runner)); + + await orch.runReview(initial.taskId); + + expect(postThreadReply).toHaveBeenCalledOnce(); + // A new finding → the verdict IS posted. + expect(mocks.provider.vote).toHaveBeenCalledWith(CHANGE_ID, 2, -1, "blocking"); }); it("ignores discussion threads when the provider lacks thread support", async () => { From 26477e983a47a0b11c8bab105081851b94459488 Mon Sep 17 00:00:00 2001 From: Fadi Shehadeh Date: Mon, 29 Jun 2026 15:02:29 -0400 Subject: [PATCH 03/13] feat(review): add inter-patchset delta in re-reviews On a re-review, surface the diff between the patchset VE last reviewed and the current one as a focused "Changes since last reviewed patchset" section so the agent concentrates new findings on the delta while still seeing the full change. - Add optional ReviewProvider.getInterPatchsetDiff(changeId, from, to). - Implement it for Gerrit (SSH): shallow-fetch both patchset refs, diff tips. - Render the delta section in the prompt (capped per-section, rebase caveat); omitted on first reviews and when the delta is empty. - Wire the orchestrator to fetch the delta on re-reviews, degrading gracefully when the provider lacks the method or the fetch fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/connectors/gerritSshReviewProvider.ts | 72 +++++++++++++++- src/interfaces.ts | 12 +++ src/review/reviewOrchestrator.ts | 33 ++++++++ src/review/reviewPromptBuilder.ts | 45 +++++++++- tests/unit/gerritSshReviewProvider.test.ts | 45 ++++++++++ tests/unit/reviewOrchestrator.test.ts | 95 ++++++++++++++++++++++ tests/unit/reviewPromptBuilder.test.ts | 42 ++++++++++ 7 files changed, 342 insertions(+), 2 deletions(-) diff --git a/src/connectors/gerritSshReviewProvider.ts b/src/connectors/gerritSshReviewProvider.ts index 301fd17c..c8b4e24d 100644 --- a/src/connectors/gerritSshReviewProvider.ts +++ b/src/connectors/gerritSshReviewProvider.ts @@ -295,7 +295,77 @@ export class GerritSshReviewProvider implements ReviewProvider { } } - /** Post inline comments together with a Code-Review vote via SSH `gerrit review --json`. */ + /** + * Diff two patchsets of the same change (`fromPatchset` → `toPatchset`) by + * fetching both refs and comparing their tips. Used on a re-review to surface + * what changed since VE last reviewed. + */ + async getInterPatchsetDiff( + changeId: ExternalChangeId, + fromPatchset: number, + toPatchset: number + ): Promise { + const details = await this.getChangeDetails(changeId); + + const baseDir = this.config.workspaceBaseDir ?? "/tmp/ve-review-diffs"; + const dir = await mkdtemp(join(baseDir, `delta-${details.changeNumber}-`)); + + try { + const sshUrl = `ssh://${this.config.sshUser}@${this.config.sshHost}:${this.config.sshPort}/${details.project}`; + const hostKeyOpts = buildSshHostKeyOptions(this.config.sshKnownHostsPath).join(" "); + const sshCmd = `ssh -p ${this.config.sshPort} -i ${this.config.sshKeyPath} ${hostKeyOpts}`; + const gitEnv = { + ...process.env, + GIT_TERMINAL_PROMPT: "0", + GIT_SSH_COMMAND: sshCmd, + }; + + await execFileAsync("git", ["init"], { cwd: dir, timeout: GIT_TIMEOUT_MS }); + await execFileAsync("git", ["remote", "add", "origin", sshUrl], { cwd: dir, timeout: GIT_TIMEOUT_MS }); + + const fromSha = await this.fetchPatchsetTip(dir, gitEnv, details.changeNumber, fromPatchset); + const toSha = await this.fetchPatchsetTip(dir, gitEnv, details.changeNumber, toPatchset); + + // Tree-level comparison between the two patchset tips. This does not need + // shared history, so shallow (depth=1) fetches of each ref are sufficient. + const { stdout: diffOutput } = await execFileAsync( + "git", + ["diff", `${fromSha}..${toSha}`, "--no-color", "--unified=5"], + { cwd: dir, timeout: GIT_TIMEOUT_MS, maxBuffer: 10 * 1024 * 1024 } + ); + + const files = parseDiffOutput(diffOutput); + log.info( + { changeId, fromPatchset, toPatchset, fileCount: files.length }, + "computed inter-patchset diff from SSH fetch" + ); + return { changeId, patchset: toPatchset, files }; + } finally { + await rm(dir, { recursive: true, force: true }).catch(() => undefined); + } + } + + /** Shallow-fetch a single patchset ref and return its tip SHA. */ + private async fetchPatchsetTip( + dir: string, + gitEnv: NodeJS.ProcessEnv, + changeNumber: number, + patchset: number + ): Promise { + const nn = String(changeNumber % 100).padStart(2, "0"); + const ref = `refs/changes/${nn}/${changeNumber}/${patchset}`; + await execFileAsync("git", ["fetch", "--depth=1", "origin", ref], { + cwd: dir, + timeout: GIT_TIMEOUT_MS, + env: gitEnv, + }); + const { stdout } = await execFileAsync("git", ["rev-parse", "FETCH_HEAD"], { + cwd: dir, + timeout: GIT_TIMEOUT_MS, + }); + return stdout.trim(); + } + async postReviewWithComments( changeId: ExternalChangeId, revision: number, diff --git a/src/interfaces.ts b/src/interfaces.ts index e390c77f..b999e13c 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -842,6 +842,18 @@ export interface ReviewProvider { /** Fetch the diff for a specific patchset (defaults to current). */ getChangeDiff(changeId: ExternalChangeId, patchset?: number): Promise; + /** + * Fetch the diff between two patchsets of the same change (`fromPatchset` → + * `toPatchset`). Used on a re-review to surface "what changed since my last + * review". Optional — providers that cannot compute an inter-patchset diff + * omit it and the orchestrator falls back to the full diff only. + */ + getInterPatchsetDiff?( + changeId: ExternalChangeId, + fromPatchset: number, + toPatchset: number + ): Promise; + /** * Post inline comments and a summary on the given revision. * diff --git a/src/review/reviewOrchestrator.ts b/src/review/reviewOrchestrator.ts index eb91d79b..214707c0 100644 --- a/src/review/reviewOrchestrator.ts +++ b/src/review/reviewOrchestrator.ts @@ -453,6 +453,38 @@ export class ReviewOrchestrator { } } + // On a re-review, surface what changed since the patchset VE last + // reviewed so the agent focuses new findings on the delta. Guarded: + // providers without inter-patchset diff support skip this entirely, and + // empty deltas are dropped by the prompt builder. + let sinceLastReview: + | { fromPatchset: number; toPatchset: number; diff: ReviewChangeDiff } + | undefined; + const previousReviewed = task.reviewedPatchset; + if ( + previousReviewed !== null && + previousReviewed < details.currentPatchset && + typeof this.deps.reviewProvider.getInterPatchsetDiff === "function" + ) { + try { + const deltaDiff = await this.deps.reviewProvider.getInterPatchsetDiff( + changeId, + previousReviewed, + details.currentPatchset + ); + sinceLastReview = { + fromPatchset: previousReviewed, + toPatchset: details.currentPatchset, + diff: deltaDiff, + }; + } catch (err) { + log.warn( + { err, taskId, fromPatchset: previousReviewed, toPatchset: details.currentPatchset }, + "failed to compute inter-patchset delta; continuing with full diff only" + ); + } + } + const prompt = buildReviewPrompt({ details, diff, @@ -468,6 +500,7 @@ export class ReviewOrchestrator { : {}), ...(eligibleThreads.length > 0 ? { discussionThreads: eligibleThreads } : {}), ...(this.deps.maxDiffChars !== undefined ? { maxDiffChars: this.deps.maxDiffChars } : {}), + ...(sinceLastReview !== undefined ? { sinceLastReview } : {}), }); emitReviewEvent("review.prompt_built", { promptLength: prompt.length, patchset: details.currentPatchset }); diff --git a/src/review/reviewPromptBuilder.ts b/src/review/reviewPromptBuilder.ts index 317b10a3..d374bb23 100644 --- a/src/review/reviewPromptBuilder.ts +++ b/src/review/reviewPromptBuilder.ts @@ -28,6 +28,23 @@ export interface ReviewPromptInput { * `threadId` the agent must echo back in its `replies[]` output to address it. */ discussionThreads?: ReviewDiscussionThread[] | undefined; + /** + * On a re-review, the diff between the last patchset VE reviewed and the + * current one. Surfaced as a focused "what changed since my last review" + * section so the agent concentrates new findings on the delta while still + * seeing the full change. Omitted on the first review pass. + */ + sinceLastReview?: SinceLastReviewDelta | undefined; +} + +/** Inter-patchset delta surfaced to the agent on a re-review. */ +export interface SinceLastReviewDelta { + /** Patchset VE last reviewed. */ + fromPatchset: number; + /** Current patchset under review. */ + toPatchset: number; + /** Diff between `fromPatchset` and `toPatchset`. */ + diff: ReviewChangeDiff; } /** A previously-posted review comment surfaced back to the agent as memory. */ @@ -39,7 +56,8 @@ export interface PriorReviewComment { /** Build the full prompt sent to the review agent for a single change. */ export function buildReviewPrompt(input: ReviewPromptInput): string { - const { details, diff, userPrompt, maxDiffChars, priorComments, discussionThreads } = input; + const { details, diff, userPrompt, maxDiffChars, priorComments, discussionThreads, sinceLastReview } = + input; const header = [ `# Code Review Task`, @@ -86,6 +104,14 @@ export function buildReviewPrompt(input: ReviewPromptInput): string { ); } + if (sinceLastReview && sinceLastReview.diff.files.length > 0) { + sections.push( + ``, + `## Changes since last reviewed patchset (PS ${sinceLastReview.fromPatchset} \u2192 ${sinceLastReview.toPatchset})`, + renderSinceLastReview(sinceLastReview, maxDiffChars ?? DEFAULT_MAX_DIFF_CHARS), + ); + } + sections.push( ``, `## Files in this patchset`, @@ -162,3 +188,20 @@ function renderDiffSections(diff: ReviewChangeDiff, maxDiffChars: number): strin } return parts.join("\n\n"); } + +/** + * Render the inter-patchset delta as a guidance note followed by the delta diff. + * Caps the diff with the same per-section budget as the full diff. Rebase noise + * (upstream changes pulled in between patchsets) may appear here; the note tells + * the agent to treat it as such. + */ +function renderSinceLastReview(delta: SinceLastReviewDelta, maxDiffChars: number): string { + return [ + "These are the changes between the patchset you last reviewed and the current", + "one. Focus genuinely new findings on this delta. If the change was rebased,", + "some hunks here may be upstream churn rather than author edits — judge", + "accordingly. The full change is still provided below for context.", + "", + renderDiffSections(delta.diff, maxDiffChars), + ].join("\n"); +} diff --git a/tests/unit/gerritSshReviewProvider.test.ts b/tests/unit/gerritSshReviewProvider.test.ts index 64660bad..1f00845c 100644 --- a/tests/unit/gerritSshReviewProvider.test.ts +++ b/tests/unit/gerritSshReviewProvider.test.ts @@ -285,6 +285,51 @@ describe("GerritSshReviewProvider", () => { }); }); + describe("getInterPatchsetDiff", () => { + const DELTA_OUTPUT = [ + "diff --git a/src/main.ts b/src/main.ts", + "--- a/src/main.ts", + "+++ b/src/main.ts", + "@@ -1,3 +1,4 @@", + " import { foo } from './foo';", + "+import { baz } from './baz';", + " ", + " console.log(foo);", + ].join("\n"); + + it("fetches both patchset refs and diffs their tips", async () => { + mockQuery.mockResolvedValueOnce(sshNdjson(SAMPLE_CHANGE)); + // init, remote add, fetch(from), rev-parse(from), fetch(to), rev-parse(to), diff + setGitResults(["", "", "", "shaFrom\n", "", "shaTo\n", DELTA_OUTPUT]); + + const result = await makeProvider().getInterPatchsetDiff(CHANGE_ID, 2, 3); + + expect(result.changeId).toBe(CHANGE_ID); + expect(result.patchset).toBe(3); + expect(result.files).toHaveLength(1); + expect(result.files[0]!.path).toBe("src/main.ts"); + expect(result.files[0]!.patch).toContain("+import { baz }"); + + // Verify it fetched the from-ref, the to-ref, then diffed the resolved SHAs. + const fetchArgs = gitFileCalls.filter((c) => c.args[0] === "fetch").map((c) => c.args); + expect(fetchArgs).toHaveLength(2); + expect(fetchArgs[0]).toContain("refs/changes/42/42/2"); + expect(fetchArgs[1]).toContain("refs/changes/42/42/3"); + const diffCall = gitFileCalls.find((c) => c.args[0] === "diff"); + expect(diffCall!.args).toContain("shaFrom..shaTo"); + }); + + it("cleans up temp directory on error", async () => { + mockQuery.mockResolvedValueOnce(sshNdjson(SAMPLE_CHANGE)); + setGitResults([new Error("git init failed")]); + + await expect(makeProvider().getInterPatchsetDiff(CHANGE_ID, 2, 3)).rejects.toThrow( + "git init failed" + ); + expect(rm).toHaveBeenCalledWith("/tmp/test-diffs/diff-42-abc", { recursive: true, force: true }); + }); + }); + describe("postReviewWithComments", () => { it("posts review via SSH gerrit review --json", async () => { mockQuery.mockResolvedValueOnce(sshNdjson(SAMPLE_CHANGE)); diff --git a/tests/unit/reviewOrchestrator.test.ts b/tests/unit/reviewOrchestrator.test.ts index 483f39a6..86badd9e 100644 --- a/tests/unit/reviewOrchestrator.test.ts +++ b/tests/unit/reviewOrchestrator.test.ts @@ -878,6 +878,101 @@ describe("ReviewOrchestrator.runReview — happy path", () => { }); }); +describe("ReviewOrchestrator.runReview - inter-patchset delta", () => { + it("fetches the delta and injects it into the prompt on a re-review", async () => { + const initial = makeTask({ + state: "REVIEW_WATCHING", + cycleCount: 1, + reviewedPatchset: 2, + currentPatchset: 3, + }); + const mocks = makeMocks(initial); + const { runner } = makeWorkspaceRunner(); + + (mocks.provider.getChangeDetails as ReturnType).mockResolvedValue( + makeDetails({ currentPatchset: 3 }) + ); + const getInterPatchsetDiff = vi.fn(async () => + makeDiff({ + patchset: 3, + files: [{ path: "src/a.ts", status: "modified", patch: "+delta-line" }], + }) + ); + mocks.provider.getInterPatchsetDiff = + getInterPatchsetDiff as NonNullable; + + const orch = new ReviewOrchestrator(makeDeps(mocks, runner)); + await orch.runReview(initial.taskId); + + expect(getInterPatchsetDiff).toHaveBeenCalledWith(CHANGE_ID, 2, 3); + const prompt = runner.runReviewInDocker.mock.calls[0]?.[1]?.prompt as string; + expect(prompt).toContain("## Changes since last reviewed patchset (PS 2 \u2192 3)"); + expect(prompt).toContain("+delta-line"); + }); + + it("does not fetch a delta on the first review (no prior reviewed patchset)", async () => { + const initial = makeTask({ + state: "REVIEW_PENDING", + reviewedPatchset: null, + currentPatchset: 2, + }); + const mocks = makeMocks(initial); + const { runner } = makeWorkspaceRunner(); + const getInterPatchsetDiff = vi.fn(async () => makeDiff()); + mocks.provider.getInterPatchsetDiff = + getInterPatchsetDiff as NonNullable; + + const orch = new ReviewOrchestrator(makeDeps(mocks, runner)); + await orch.runReview(initial.taskId); + + expect(getInterPatchsetDiff).not.toHaveBeenCalled(); + const prompt = runner.runReviewInDocker.mock.calls[0]?.[1]?.prompt as string; + expect(prompt).not.toContain("## Changes since last reviewed patchset"); + }); + + it("does not fetch a delta when the provider lacks getInterPatchsetDiff", async () => { + const initial = makeTask({ + state: "REVIEW_WATCHING", + cycleCount: 1, + reviewedPatchset: 2, + currentPatchset: 3, + }); + const mocks = makeMocks(initial); + const { runner } = makeWorkspaceRunner(); + (mocks.provider.getChangeDetails as ReturnType).mockResolvedValue( + makeDetails({ currentPatchset: 3 }) + ); + // Provider has no getInterPatchsetDiff (default mock omits it). + const orch = new ReviewOrchestrator(makeDeps(mocks, runner)); + await orch.runReview(initial.taskId); + + const prompt = runner.runReviewInDocker.mock.calls[0]?.[1]?.prompt as string; + expect(prompt).not.toContain("## Changes since last reviewed patchset"); + }); + + it("degrades gracefully when the delta fetch fails", async () => { + const initial = makeTask({ + state: "REVIEW_WATCHING", + cycleCount: 1, + reviewedPatchset: 2, + currentPatchset: 3, + }); + const mocks = makeMocks(initial); + const { runner } = makeWorkspaceRunner(); + (mocks.provider.getChangeDetails as ReturnType).mockResolvedValue( + makeDetails({ currentPatchset: 3 }) + ); + mocks.provider.getInterPatchsetDiff = vi.fn(async () => { + throw new Error("fetch failed"); + }) as NonNullable; + + const orch = new ReviewOrchestrator(makeDeps(mocks, runner)); + await expect(orch.runReview(initial.taskId)).resolves.toBeUndefined(); + const prompt = runner.runReviewInDocker.mock.calls[0]?.[1]?.prompt as string; + expect(prompt).not.toContain("## Changes since last reviewed patchset"); + }); +}); + describe("ReviewOrchestrator.runReview — failure paths", () => { it("marks task REVIEW_FAILED and rethrows on runReviewInDocker failure", async () => { const initial = makeTask({ state: "REVIEW_PENDING" }); diff --git a/tests/unit/reviewPromptBuilder.test.ts b/tests/unit/reviewPromptBuilder.test.ts index ebbfd53b..643b0d82 100644 --- a/tests/unit/reviewPromptBuilder.test.ts +++ b/tests/unit/reviewPromptBuilder.test.ts @@ -229,3 +229,45 @@ describe("buildReviewPrompt discussion threads", () => { }); }); +describe("buildReviewPrompt since-last-review delta", () => { + const deltaDiff: ReviewChangeDiff = { + changeId: CHANGE_ID, + patchset: 3, + files: [ + { + path: "src/foo.ts", + status: "modified", + patch: "--- a/src/foo.ts\n+++ b/src/foo.ts\n@@ -1 +1 @@\n-new\n+newer", + }, + ], + }; + + it("omits the delta section when sinceLastReview is not provided", () => { + const prompt = buildReviewPrompt({ details, diff, userPrompt: "Review this." }); + expect(prompt).not.toContain("## Changes since last reviewed patchset"); + }); + + it("omits the delta section when the delta has no files", () => { + const prompt = buildReviewPrompt({ + details, + diff, + userPrompt: "Review this.", + sinceLastReview: { fromPatchset: 2, toPatchset: 3, diff: { ...deltaDiff, files: [] } }, + }); + expect(prompt).not.toContain("## Changes since last reviewed patchset"); + }); + + it("renders the delta section with the PS range and the delta diff when provided", () => { + const prompt = buildReviewPrompt({ + details, + diff, + userPrompt: "Review this.", + sinceLastReview: { fromPatchset: 2, toPatchset: 3, diff: deltaDiff }, + }); + expect(prompt).toContain("## Changes since last reviewed patchset (PS 2 → 3)"); + expect(prompt).toContain("+newer"); + // The full diff is still present alongside the delta. + expect(prompt).toContain("## Unified diffs"); + }); +}); + From e1db783cde5fe58e5f4dac9b7eabe9674e6cf383 Mon Sep 17 00:00:00 2001 From: Charles Wenger Date: Fri, 3 Jul 2026 10:48:18 +0200 Subject: [PATCH 04/13] test(gerrit): single-digit change number pads shard ref correctly --- tests/unit/gerritSshReviewProvider.test.ts | 25 ++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/unit/gerritSshReviewProvider.test.ts b/tests/unit/gerritSshReviewProvider.test.ts index 1f00845c..988e4f1f 100644 --- a/tests/unit/gerritSshReviewProvider.test.ts +++ b/tests/unit/gerritSshReviewProvider.test.ts @@ -328,6 +328,31 @@ describe("GerritSshReviewProvider", () => { ); expect(rm).toHaveBeenCalledWith("/tmp/test-diffs/diff-42-abc", { recursive: true, force: true }); }); + + it("returns an empty file list when the two patchsets are identical (empty diff output)", async () => { + mockQuery.mockResolvedValueOnce(sshNdjson(SAMPLE_CHANGE)); + // init, remote add, fetch(from), rev-parse(from), fetch(to), rev-parse(to), diff → empty + setGitResults(["", "", "", "shaA\n", "", "shaA\n", ""]); + + const result = await makeProvider().getInterPatchsetDiff(CHANGE_ID, 3, 3); + + expect(result.patchset).toBe(3); + expect(result.files).toHaveLength(0); + }); + + it("pads the two-digit shard correctly for a single-digit change number", async () => { + const singleDigitChange = { ...SAMPLE_CHANGE, number: 7 }; + mockQuery.mockResolvedValueOnce(sshNdjson(singleDigitChange)); + setGitResults(["", "", "", "sha1\n", "", "sha2\n", ""]); + + await makeProvider().getInterPatchsetDiff(CHANGE_ID, 1, 2); + + const fetchRefs = gitFileCalls + .filter((c) => c.args[0] === "fetch") + .map((c) => c.args.find((a) => a.startsWith("refs/changes/"))); + expect(fetchRefs[0]).toBe("refs/changes/07/7/1"); + expect(fetchRefs[1]).toBe("refs/changes/07/7/2"); + }); }); describe("postReviewWithComments", () => { From e363a8c6a21a7bb6cc243037e0a07d730bc14245 Mon Sep 17 00:00:00 2001 From: Charles Wenger Date: Fri, 3 Jul 2026 10:48:51 +0200 Subject: [PATCH 05/13] test(review): no delta fetched when reviewedPatchset equals currentPatchset --- tests/unit/reviewOrchestrator.test.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/unit/reviewOrchestrator.test.ts b/tests/unit/reviewOrchestrator.test.ts index 86badd9e..35b5cdc7 100644 --- a/tests/unit/reviewOrchestrator.test.ts +++ b/tests/unit/reviewOrchestrator.test.ts @@ -971,6 +971,30 @@ describe("ReviewOrchestrator.runReview - inter-patchset delta", () => { const prompt = runner.runReviewInDocker.mock.calls[0]?.[1]?.prompt as string; expect(prompt).not.toContain("## Changes since last reviewed patchset"); }); + + it("does not fetch a delta when reviewedPatchset equals currentPatchset", async () => { + const initial = makeTask({ + state: "REVIEW_WATCHING", + cycleCount: 1, + reviewedPatchset: 3, + currentPatchset: 3, + }); + const mocks = makeMocks(initial); + const { runner } = makeWorkspaceRunner(); + (mocks.provider.getChangeDetails as ReturnType).mockResolvedValue( + makeDetails({ currentPatchset: 3 }) + ); + const getInterPatchsetDiff = vi.fn(async () => makeDiff()); + mocks.provider.getInterPatchsetDiff = + getInterPatchsetDiff as NonNullable; + + const orch = new ReviewOrchestrator(makeDeps(mocks, runner)); + await orch.runReview(initial.taskId); + + expect(getInterPatchsetDiff).not.toHaveBeenCalled(); + const prompt = runner.runReviewInDocker.mock.calls[0]?.[1]?.prompt as string; + expect(prompt).not.toContain("## Changes since last reviewed patchset"); + }); }); describe("ReviewOrchestrator.runReview — failure paths", () => { From 8ee40bf137d758c590d0dab05153673e48aa4e9f Mon Sep 17 00:00:00 2001 From: Charles Wenger Date: Fri, 3 Jul 2026 10:49:16 +0200 Subject: [PATCH 06/13] test(review): delta section omitted in prompt when delta has no files --- tests/unit/reviewOrchestrator.test.ts | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/unit/reviewOrchestrator.test.ts b/tests/unit/reviewOrchestrator.test.ts index 35b5cdc7..983a92d0 100644 --- a/tests/unit/reviewOrchestrator.test.ts +++ b/tests/unit/reviewOrchestrator.test.ts @@ -995,6 +995,29 @@ describe("ReviewOrchestrator.runReview - inter-patchset delta", () => { const prompt = runner.runReviewInDocker.mock.calls[0]?.[1]?.prompt as string; expect(prompt).not.toContain("## Changes since last reviewed patchset"); }); + + it("omits the delta section in the prompt when the delta contains no files", async () => { + const initial = makeTask({ + state: "REVIEW_WATCHING", + cycleCount: 1, + reviewedPatchset: 2, + currentPatchset: 3, + }); + const mocks = makeMocks(initial); + const { runner } = makeWorkspaceRunner(); + (mocks.provider.getChangeDetails as ReturnType).mockResolvedValue( + makeDetails({ currentPatchset: 3 }) + ); + mocks.provider.getInterPatchsetDiff = vi.fn(async () => + makeDiff({ patchset: 3, files: [] }) + ) as NonNullable; + + const orch = new ReviewOrchestrator(makeDeps(mocks, runner)); + await orch.runReview(initial.taskId); + + const prompt = runner.runReviewInDocker.mock.calls[0]?.[1]?.prompt as string; + expect(prompt).not.toContain("## Changes since last reviewed patchset"); + }); }); describe("ReviewOrchestrator.runReview — failure paths", () => { From 48f685bda8aa5f25ca090b931e4aaf7053f63024 Mon Sep 17 00:00:00 2001 From: Charles Wenger Date: Fri, 3 Jul 2026 10:50:03 +0200 Subject: [PATCH 07/13] test(review): delta diff is truncated when it exceeds maxDiffChars --- tests/unit/reviewPromptBuilder.test.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/unit/reviewPromptBuilder.test.ts b/tests/unit/reviewPromptBuilder.test.ts index 643b0d82..97c553dc 100644 --- a/tests/unit/reviewPromptBuilder.test.ts +++ b/tests/unit/reviewPromptBuilder.test.ts @@ -269,5 +269,25 @@ describe("buildReviewPrompt since-last-review delta", () => { // The full diff is still present alongside the delta. expect(prompt).toContain("## Unified diffs"); }); + + it("truncates the delta diff when it exceeds maxDiffChars", () => { + const hugeDeltaDiff: ReviewChangeDiff = { + changeId: CHANGE_ID, + patchset: 3, + files: Array.from({ length: 5 }, (_, i) => ({ + path: `src/delta-big-${i}.ts`, + status: "modified" as const, + patch: "+delta\n".repeat(20_000), + })), + }; + const prompt = buildReviewPrompt({ + details, + diff, + userPrompt: "Review this.", + sinceLastReview: { fromPatchset: 2, toPatchset: 3, diff: hugeDeltaDiff }, + }); + expect(prompt).toContain("## Changes since last reviewed patchset"); + expect(prompt).toContain("diff truncated"); + }); }); From f54b1dfc84a80f16eab947bb030001730440a872 Mon Sep 17 00:00:00 2001 From: Charles Wenger Date: Fri, 3 Jul 2026 11:06:54 +0200 Subject: [PATCH 08/13] test(worker): disable git hooks in commit protocol test repos A global Gerrit commit-msg hook on the host was injecting Change-Ids into every commit, causing assertion failures (changeId unexpectedly non-empty) and timeouts (hook made slow network calls per commit). Set core.hooksPath to an empty directory in initRepo() to isolate test repos from any host-level git hooks. --- tests/unit/workerCommitProtocol.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/unit/workerCommitProtocol.test.ts b/tests/unit/workerCommitProtocol.test.ts index 8fd0665f..7ca200bd 100644 --- a/tests/unit/workerCommitProtocol.test.ts +++ b/tests/unit/workerCommitProtocol.test.ts @@ -38,6 +38,9 @@ function initRepo(): string { git(["config", "user.name", "Test"], dir); git(["config", "user.email", "test@test.local"], dir); git(["config", "commit.gpgsign", "false"], dir); + const noHooksDir = join(dir, ".git", "no-hooks"); + mkdirSync(noHooksDir, { recursive: true }); + git(["config", "core.hooksPath", noHooksDir], dir); writeFileSync(join(dir, "README.md"), "# Test\n"); git(["add", "README.md"], dir); git(["commit", "-m", "chore: initial commit"], dir); From 0d5681ad27e7843342666dfd794a7484a9909f27 Mon Sep 17 00:00:00 2001 From: Charles Wenger Date: Fri, 3 Jul 2026 11:11:38 +0200 Subject: [PATCH 09/13] test(review): add failing test for double-budget bug in prompt builder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When both a sinceLastReview delta and a full diff are present, renderSinceLastReview and renderDiffSections each independently consume maxDiffChars, allowing the prompt to reach 2×maxDiffChars. Test: given two ~600-char patches with maxDiffChars=1000, at least one section must be truncated after the shared-budget fix is applied. --- tests/unit/reviewPromptBuilder.test.ts | 38 ++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/unit/reviewPromptBuilder.test.ts b/tests/unit/reviewPromptBuilder.test.ts index 97c553dc..d44e7d7f 100644 --- a/tests/unit/reviewPromptBuilder.test.ts +++ b/tests/unit/reviewPromptBuilder.test.ts @@ -270,6 +270,44 @@ describe("buildReviewPrompt since-last-review delta", () => { expect(prompt).toContain("## Unified diffs"); }); + it("delta and full diff share the maxDiffChars budget — at least one is truncated when both are large", () => { + // Each patch is ~600 chars. With maxDiffChars=1000, each fits individually + // (600 < 1000) but together they would exceed the budget (1200 > 1000). + // The fix: split the budget so the combined diff content stays within maxDiffChars. + // Without the fix both sections render fully → neither is truncated (bug). + const maxDiffChars = 1000; + const patch600 = "+" + "x".repeat(600); + + const bigFullDiff: ReviewChangeDiff = { + changeId: CHANGE_ID, + patchset: 3, + files: [{ path: "src/full.ts", status: "modified" as const, patch: patch600 }], + }; + const bigDeltaDiff: ReviewChangeDiff = { + changeId: CHANGE_ID, + patchset: 3, + files: [{ path: "src/delta.ts", status: "modified" as const, patch: patch600 }], + }; + + const prompt = buildReviewPrompt({ + details, + diff: bigFullDiff, + userPrompt: "Review.", + maxDiffChars, + sinceLastReview: { fromPatchset: 2, toPatchset: 3, diff: bigDeltaDiff }, + }); + + const deltaIdx = prompt.indexOf("## Changes since last reviewed patchset"); + const fullDiffIdx = prompt.indexOf("## Unified diffs"); + const deltaSection = prompt.slice(deltaIdx, fullDiffIdx); + const diffSection = prompt.slice(fullDiffIdx); + + // At least one section must be truncated to respect the shared budget. + const atLeastOneTruncated = + deltaSection.includes("diff truncated") || diffSection.includes("diff truncated"); + expect(atLeastOneTruncated).toBe(true); + }); + it("truncates the delta diff when it exceeds maxDiffChars", () => { const hugeDeltaDiff: ReviewChangeDiff = { changeId: CHANGE_ID, From 48c820dbd3f6ab5fc2e5bf3d4632f8af83356398 Mon Sep 17 00:00:00 2001 From: Charles Wenger Date: Fri, 3 Jul 2026 11:11:45 +0200 Subject: [PATCH 10/13] fix(review): split maxDiffChars budget between delta and full diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When sinceLastReview is present, both renderSinceLastReview and renderDiffSections were independently allowed to consume the full maxDiffChars budget, letting the prompt grow to 2×maxDiffChars. Fix: detect whether a delta will be rendered and split the budget equally (floor(max/2) for delta, remainder for full diff). When no delta is present the full budget is unchanged. --- src/review/reviewPromptBuilder.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/review/reviewPromptBuilder.ts b/src/review/reviewPromptBuilder.ts index d374bb23..6e2f49b8 100644 --- a/src/review/reviewPromptBuilder.ts +++ b/src/review/reviewPromptBuilder.ts @@ -59,6 +59,13 @@ export function buildReviewPrompt(input: ReviewPromptInput): string { const { details, diff, userPrompt, maxDiffChars, priorComments, discussionThreads, sinceLastReview } = input; + const effectiveMax = maxDiffChars ?? DEFAULT_MAX_DIFF_CHARS; + const hasDelta = sinceLastReview !== undefined && sinceLastReview.diff.files.length > 0; + // When both a delta section and a full diff are present, split the budget + // equally so the combined diff content stays within effectiveMax chars. + const deltaBudget = hasDelta ? Math.floor(effectiveMax / 2) : effectiveMax; + const fullDiffBudget = hasDelta ? effectiveMax - deltaBudget : effectiveMax; + const header = [ `# Code Review Task`, ``, @@ -73,7 +80,7 @@ export function buildReviewPrompt(input: ReviewPromptInput): string { .map((f) => `- ${f.status.toUpperCase().padEnd(8)} ${f.path}`) .join("\n"); - const diffSections = renderDiffSections(diff, maxDiffChars ?? DEFAULT_MAX_DIFF_CHARS); + const diffSections = renderDiffSections(diff, fullDiffBudget); const sections = [ header, @@ -108,7 +115,7 @@ export function buildReviewPrompt(input: ReviewPromptInput): string { sections.push( ``, `## Changes since last reviewed patchset (PS ${sinceLastReview.fromPatchset} \u2192 ${sinceLastReview.toPatchset})`, - renderSinceLastReview(sinceLastReview, maxDiffChars ?? DEFAULT_MAX_DIFF_CHARS), + renderSinceLastReview(sinceLastReview, deltaBudget), ); } From f98033d12b2935a52d82ba562cddd022690f6080 Mon Sep 17 00:00:00 2001 From: Charles Wenger Date: Fri, 3 Jul 2026 13:15:41 +0200 Subject: [PATCH 11/13] fix(gerrit): ensure baseDir exists before calling mkdtemp mkdtemp(join(baseDir, ...)) throws ENOENT when baseDir does not yet exist. Both getDiff and getInterPatchsetDiff assumed /tmp/ve-review-diffs (or workspaceBaseDir) was already present, which could silently break delta computation on host/dev runs. Add mkdir(baseDir, { recursive: true }) before mkdtemp in both methods. Update the node:fs/promises mock in the test suite to include mkdir. --- src/connectors/gerritSshReviewProvider.ts | 6 ++++-- tests/unit/gerritSshReviewProvider.test.ts | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/connectors/gerritSshReviewProvider.ts b/src/connectors/gerritSshReviewProvider.ts index c8b4e24d..72b882f7 100644 --- a/src/connectors/gerritSshReviewProvider.ts +++ b/src/connectors/gerritSshReviewProvider.ts @@ -7,7 +7,7 @@ */ import { execFile } from "node:child_process"; import { promisify } from "node:util"; -import { mkdtemp, rm } from "node:fs/promises"; +import { mkdir, mkdtemp, rm } from "node:fs/promises"; import { join } from "node:path"; import { z } from "zod"; import { @@ -105,7 +105,7 @@ export interface GerritSshReviewProviderConfig { reviewerAccountId?: string | undefined; /** * Base directory for temporary git checkouts (used to compute diffs). - * Defaults to /tmp/virtual-engineer/review-diffs. + * Defaults to /tmp/ve-review-diffs. */ workspaceBaseDir?: string | undefined; } @@ -250,6 +250,7 @@ export class GerritSshReviewProvider implements ReviewProvider { const changeRef = `refs/changes/${nn}/${details.changeNumber}/${ps}`; const baseDir = this.config.workspaceBaseDir ?? "/tmp/ve-review-diffs"; + await mkdir(baseDir, { recursive: true }); const dir = await mkdtemp(join(baseDir, `diff-${details.changeNumber}-`)); try { @@ -308,6 +309,7 @@ export class GerritSshReviewProvider implements ReviewProvider { const details = await this.getChangeDetails(changeId); const baseDir = this.config.workspaceBaseDir ?? "/tmp/ve-review-diffs"; + await mkdir(baseDir, { recursive: true }); const dir = await mkdtemp(join(baseDir, `delta-${details.changeNumber}-`)); try { diff --git a/tests/unit/gerritSshReviewProvider.test.ts b/tests/unit/gerritSshReviewProvider.test.ts index 988e4f1f..3030ef43 100644 --- a/tests/unit/gerritSshReviewProvider.test.ts +++ b/tests/unit/gerritSshReviewProvider.test.ts @@ -53,6 +53,7 @@ vi.mock("node:child_process", () => ({ })); vi.mock("node:fs/promises", () => ({ + mkdir: vi.fn().mockResolvedValue(undefined), mkdtemp: vi.fn().mockResolvedValue("/tmp/test-diffs/diff-42-abc"), rm: vi.fn().mockReturnValue(Promise.resolve(undefined)), })); From 11256463c008386a44bbb089343d7203bbbffd81 Mon Sep 17 00:00:00 2001 From: Charles Wenger Date: Mon, 13 Jul 2026 10:19:46 +0200 Subject: [PATCH 12/13] fix(review): cap commit message in prompts The review prompt already limits rendered diffs to avoid blowing up the model context, but the Gerrit commit message was inserted without any size guard. A very large commit body could therefore consume most of the prompt before the diff and review instructions are added. Truncate oversized commit messages and add a clear truncation marker, so the review agent keeps enough context for the actual code changes. --- src/review/reviewPromptBuilder.ts | 11 ++++++++++- tests/unit/reviewPromptBuilder.test.ts | 14 +++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/review/reviewPromptBuilder.ts b/src/review/reviewPromptBuilder.ts index 6e2f49b8..3a4da56d 100644 --- a/src/review/reviewPromptBuilder.ts +++ b/src/review/reviewPromptBuilder.ts @@ -8,8 +8,11 @@ import type { ReviewChangeDetails, ReviewChangeDiff, ReviewDiscussionThread } fr /** Default upper bound on diff size injected into the prompt to avoid token blow-ups. */ const DEFAULT_MAX_DIFF_CHARS = 60_000; +const DEFAULT_MAX_COMMIT_MESSAGE_CHARS = 8_000; const TRUNCATION_NOTE = "\n\n[... diff truncated to fit in the model context — review the rest from the repository if needed ...]"; +const COMMIT_MESSAGE_TRUNCATION_NOTE = + "\n\n[... commit message truncated to fit in the model context ...]"; export interface ReviewPromptInput { details: ReviewChangeDetails; @@ -90,7 +93,7 @@ export function buildReviewPrompt(input: ReviewPromptInput): string { // beyond its subject so we never inject an empty section. const description = details.description.trim(); if (description.length > 0) { - sections.push(``, `## Commit message`, description); + sections.push(``, `## Commit message`, truncateCommitMessage(description)); } sections.push( @@ -196,6 +199,12 @@ function renderDiffSections(diff: ReviewChangeDiff, maxDiffChars: number): strin return parts.join("\n\n"); } +function truncateCommitMessage(description: string): string { + if (description.length <= DEFAULT_MAX_COMMIT_MESSAGE_CHARS) return description; + const bodyLimit = DEFAULT_MAX_COMMIT_MESSAGE_CHARS - COMMIT_MESSAGE_TRUNCATION_NOTE.length; + return description.slice(0, bodyLimit) + COMMIT_MESSAGE_TRUNCATION_NOTE; +} + /** * Render the inter-patchset delta as a guidance note followed by the delta diff. * Caps the diff with the same per-section budget as the full diff. Rebase noise diff --git a/tests/unit/reviewPromptBuilder.test.ts b/tests/unit/reviewPromptBuilder.test.ts index d44e7d7f..a0f53fa7 100644 --- a/tests/unit/reviewPromptBuilder.test.ts +++ b/tests/unit/reviewPromptBuilder.test.ts @@ -160,6 +160,19 @@ describe("buildReviewPrompt commit message", () => { expect(prompt).toContain("Closes #42."); }); + it("truncates very large commit message bodies", () => { + const prompt = buildReviewPrompt({ + details: { ...details, description: "x".repeat(9_000) }, + diff, + userPrompt: "Review this.", + }); + expect(prompt).toContain("## Commit message"); + expect(prompt).toContain("commit message truncated"); + expect(prompt).not.toContain("x".repeat(8_500)); + const commitMessageSection = prompt.split("\n## User Instructions")[0]!.split("## Commit message\n")[1]!.trimEnd(); + expect(commitMessageSection.length).toBeLessThanOrEqual(8_000); + }); + it("omits the commit message section when the description is empty", () => { const prompt = buildReviewPrompt({ details: { ...details, description: "" }, @@ -328,4 +341,3 @@ describe("buildReviewPrompt since-last-review delta", () => { expect(prompt).toContain("diff truncated"); }); }); - From f2f82a601a69e68197722d34a26606ce4349646e Mon Sep 17 00:00:00 2001 From: Charles Wenger Date: Mon, 13 Jul 2026 10:19:56 +0200 Subject: [PATCH 13/13] fix(review): reuse change details for patchset deltas ReviewOrchestrator already fetches Gerrit change details before requesting the inter-patchset delta. The Gerrit SSH provider was fetching the same details again only to recover the change number, project and change id. Pass the existing ReviewChangeDetails into getInterPatchsetDiff instead. This removes the redundant Gerrit SSH query and keeps the delta path using the same change metadata as the rest of the review run. --- src/connectors/gerritSshReviewProvider.ts | 9 +++----- src/interfaces.ts | 2 +- src/review/reviewOrchestrator.ts | 8 +++---- tests/unit/gerritSshReviewProvider.test.ts | 26 ++++++++++++++-------- tests/unit/reviewOrchestrator.test.ts | 6 ++++- 5 files changed, 29 insertions(+), 22 deletions(-) diff --git a/src/connectors/gerritSshReviewProvider.ts b/src/connectors/gerritSshReviewProvider.ts index 72b882f7..277c0b8a 100644 --- a/src/connectors/gerritSshReviewProvider.ts +++ b/src/connectors/gerritSshReviewProvider.ts @@ -302,12 +302,10 @@ export class GerritSshReviewProvider implements ReviewProvider { * what changed since VE last reviewed. */ async getInterPatchsetDiff( - changeId: ExternalChangeId, + details: ReviewChangeDetails, fromPatchset: number, toPatchset: number ): Promise { - const details = await this.getChangeDetails(changeId); - const baseDir = this.config.workspaceBaseDir ?? "/tmp/ve-review-diffs"; await mkdir(baseDir, { recursive: true }); const dir = await mkdtemp(join(baseDir, `delta-${details.changeNumber}-`)); @@ -338,10 +336,10 @@ export class GerritSshReviewProvider implements ReviewProvider { const files = parseDiffOutput(diffOutput); log.info( - { changeId, fromPatchset, toPatchset, fileCount: files.length }, + { changeId: details.changeId, fromPatchset, toPatchset, fileCount: files.length }, "computed inter-patchset diff from SSH fetch" ); - return { changeId, patchset: toPatchset, files }; + return { changeId: details.changeId, patchset: toPatchset, files }; } finally { await rm(dir, { recursive: true, force: true }).catch(() => undefined); } @@ -605,4 +603,3 @@ function groupCommentsByFile( } return grouped; } - diff --git a/src/interfaces.ts b/src/interfaces.ts index b999e13c..7629caf5 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -849,7 +849,7 @@ export interface ReviewProvider { * omit it and the orchestrator falls back to the full diff only. */ getInterPatchsetDiff?( - changeId: ExternalChangeId, + details: ReviewChangeDetails, fromPatchset: number, toPatchset: number ): Promise; diff --git a/src/review/reviewOrchestrator.ts b/src/review/reviewOrchestrator.ts index 214707c0..88023a96 100644 --- a/src/review/reviewOrchestrator.ts +++ b/src/review/reviewOrchestrator.ts @@ -20,7 +20,7 @@ import { makeTaskId, makeTicketId, } from "../interfaces.js"; -import { buildReviewPrompt } from "./reviewPromptBuilder.js"; +import { buildReviewPrompt, type SinceLastReviewDelta } from "./reviewPromptBuilder.js"; import { computeVote, parseReviewResult } from "./reviewResultParser.js"; import { filterCommentsByAllowedFiles } from "./commentFilter.js"; import { computeCommentHash, computeThreadReplyHash } from "./commentHash.js"; @@ -457,9 +457,7 @@ export class ReviewOrchestrator { // reviewed so the agent focuses new findings on the delta. Guarded: // providers without inter-patchset diff support skip this entirely, and // empty deltas are dropped by the prompt builder. - let sinceLastReview: - | { fromPatchset: number; toPatchset: number; diff: ReviewChangeDiff } - | undefined; + let sinceLastReview: SinceLastReviewDelta | undefined; const previousReviewed = task.reviewedPatchset; if ( previousReviewed !== null && @@ -468,7 +466,7 @@ export class ReviewOrchestrator { ) { try { const deltaDiff = await this.deps.reviewProvider.getInterPatchsetDiff( - changeId, + details, previousReviewed, details.currentPatchset ); diff --git a/tests/unit/gerritSshReviewProvider.test.ts b/tests/unit/gerritSshReviewProvider.test.ts index 3030ef43..0e63b576 100644 --- a/tests/unit/gerritSshReviewProvider.test.ts +++ b/tests/unit/gerritSshReviewProvider.test.ts @@ -116,6 +116,19 @@ const SAMPLE_CHANGE = { lastUpdated: Math.floor(Date.now() / 1000), }; +const SAMPLE_DETAILS = { + changeId: CHANGE_ID, + changeNumber: 42, + subject: "Add feature X", + description: "This is the commit body.", + ownerAccountId: "42", + currentPatchset: 3, + status: "OPEN" as const, + project: "jami-client-qt", + targetBranch: "master", + url: "https://gerrit.test/c/42", +}; + // ─── Tests ──────────────────────────────────────────────────────────────────── describe("GerritSshReviewProvider", () => { @@ -299,11 +312,10 @@ describe("GerritSshReviewProvider", () => { ].join("\n"); it("fetches both patchset refs and diffs their tips", async () => { - mockQuery.mockResolvedValueOnce(sshNdjson(SAMPLE_CHANGE)); // init, remote add, fetch(from), rev-parse(from), fetch(to), rev-parse(to), diff setGitResults(["", "", "", "shaFrom\n", "", "shaTo\n", DELTA_OUTPUT]); - const result = await makeProvider().getInterPatchsetDiff(CHANGE_ID, 2, 3); + const result = await makeProvider().getInterPatchsetDiff(SAMPLE_DETAILS, 2, 3); expect(result.changeId).toBe(CHANGE_ID); expect(result.patchset).toBe(3); @@ -321,32 +333,28 @@ describe("GerritSshReviewProvider", () => { }); it("cleans up temp directory on error", async () => { - mockQuery.mockResolvedValueOnce(sshNdjson(SAMPLE_CHANGE)); setGitResults([new Error("git init failed")]); - await expect(makeProvider().getInterPatchsetDiff(CHANGE_ID, 2, 3)).rejects.toThrow( + await expect(makeProvider().getInterPatchsetDiff(SAMPLE_DETAILS, 2, 3)).rejects.toThrow( "git init failed" ); expect(rm).toHaveBeenCalledWith("/tmp/test-diffs/diff-42-abc", { recursive: true, force: true }); }); it("returns an empty file list when the two patchsets are identical (empty diff output)", async () => { - mockQuery.mockResolvedValueOnce(sshNdjson(SAMPLE_CHANGE)); // init, remote add, fetch(from), rev-parse(from), fetch(to), rev-parse(to), diff → empty setGitResults(["", "", "", "shaA\n", "", "shaA\n", ""]); - const result = await makeProvider().getInterPatchsetDiff(CHANGE_ID, 3, 3); + const result = await makeProvider().getInterPatchsetDiff(SAMPLE_DETAILS, 3, 3); expect(result.patchset).toBe(3); expect(result.files).toHaveLength(0); }); it("pads the two-digit shard correctly for a single-digit change number", async () => { - const singleDigitChange = { ...SAMPLE_CHANGE, number: 7 }; - mockQuery.mockResolvedValueOnce(sshNdjson(singleDigitChange)); setGitResults(["", "", "", "sha1\n", "", "sha2\n", ""]); - await makeProvider().getInterPatchsetDiff(CHANGE_ID, 1, 2); + await makeProvider().getInterPatchsetDiff({ ...SAMPLE_DETAILS, changeNumber: 7 }, 1, 2); const fetchRefs = gitFileCalls .filter((c) => c.args[0] === "fetch") diff --git a/tests/unit/reviewOrchestrator.test.ts b/tests/unit/reviewOrchestrator.test.ts index 983a92d0..acc6df11 100644 --- a/tests/unit/reviewOrchestrator.test.ts +++ b/tests/unit/reviewOrchestrator.test.ts @@ -904,7 +904,11 @@ describe("ReviewOrchestrator.runReview - inter-patchset delta", () => { const orch = new ReviewOrchestrator(makeDeps(mocks, runner)); await orch.runReview(initial.taskId); - expect(getInterPatchsetDiff).toHaveBeenCalledWith(CHANGE_ID, 2, 3); + expect(getInterPatchsetDiff).toHaveBeenCalledWith( + expect.objectContaining({ changeId: CHANGE_ID, currentPatchset: 3 }), + 2, + 3 + ); const prompt = runner.runReviewInDocker.mock.calls[0]?.[1]?.prompt as string; expect(prompt).toContain("## Changes since last reviewed patchset (PS 2 \u2192 3)"); expect(prompt).toContain("+delta-line");