diff --git a/src/connectors/gerritSshReviewProvider.ts b/src/connectors/gerritSshReviewProvider.ts index 301fd17c..277c0b8a 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 { @@ -295,7 +296,76 @@ 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( + details: ReviewChangeDetails, + fromPatchset: number, + toPatchset: number + ): Promise { + const baseDir = this.config.workspaceBaseDir ?? "/tmp/ve-review-diffs"; + await mkdir(baseDir, { recursive: true }); + 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: details.changeId, fromPatchset, toPatchset, fileCount: files.length }, + "computed inter-patchset diff from SSH fetch" + ); + return { changeId: details.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, @@ -533,4 +603,3 @@ function groupCommentsByFile( } return grouped; } - diff --git a/src/interfaces.ts b/src/interfaces.ts index e390c77f..7629caf5 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?( + details: ReviewChangeDetails, + 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 85c51927..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"; @@ -453,6 +453,36 @@ 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: SinceLastReviewDelta | 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( + details, + 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 +498,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 }); @@ -601,10 +632,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 +645,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/src/review/reviewPromptBuilder.ts b/src/review/reviewPromptBuilder.ts index 24253a99..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; @@ -28,6 +31,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 +59,15 @@ 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 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`, @@ -55,14 +83,24 @@ 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, + ]; + + // 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`, truncateCommitMessage(description)); + } + + sections.push( ``, `## User Instructions`, userPrompt, - ]; + ); if (priorComments && priorComments.length > 0) { sections.push(``, `## Already reported (do not repeat)`, renderPriorComments(priorComments)); @@ -76,6 +114,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, deltaBudget), + ); + } + sections.push( ``, `## Files in this patchset`, @@ -152,3 +198,26 @@ 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 + * (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..0e63b576 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)), })); @@ -115,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", () => { @@ -285,6 +299,71 @@ 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 () => { + // 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(SAMPLE_DETAILS, 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 () => { + setGitResults([new Error("git init failed")]); + + 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 () => { + // 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(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 () => { + setGitResults(["", "", "", "sha1\n", "", "sha2\n", ""]); + + await makeProvider().getInterPatchsetDiff({ ...SAMPLE_DETAILS, changeNumber: 7 }, 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", () => { 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 b6c58f41..acc6df11 100644 --- a/tests/unit/reviewOrchestrator.test.ts +++ b/tests/unit/reviewOrchestrator.test.ts @@ -878,6 +878,152 @@ 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( + 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"); + }); + + 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"); + }); + + 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"); + }); + + 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", () => { it("marks task REVIEW_FAILED and rethrows on runReviewInDocker failure", async () => { const initial = makeTask({ state: "REVIEW_PENDING" }); @@ -1168,6 +1314,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 () => { diff --git a/tests/unit/reviewPromptBuilder.test.ts b/tests/unit/reviewPromptBuilder.test.ts index a8761f48..a0f53fa7 100644 --- a/tests/unit/reviewPromptBuilder.test.ts +++ b/tests/unit/reviewPromptBuilder.test.ts @@ -148,6 +148,50 @@ 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("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: "" }, + 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." }); @@ -198,3 +242,102 @@ 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"); + }); + + 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, + 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"); + }); +}); 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);