fix(pr-review): re-read the whole diff after a fix instead of holding confidence at 8 forever

apps/ai/src/chat/code-mode.test.ts

@@ -228,7 +228,11 @@ describe("run_code", () => {
 				submitDiagnosis: () => Effect.die("no investigation in a review session"),
 				submitReview: (_org, _id, request) => Effect.sync(() => filed.push(request)),
 				review: {
-					coverage: { observe: (tool) => observed.push(tool), unread: () => [] },
+					coverage: {
+						observe: (tool) => observed.push(tool),
+						unread: () => [],
+						wholeDiff: () => true,
+					},
 					ledger: makeReviewLedger(),
 				},
 				codeExecutor: inProcessCodeExecutorLayer,
@@ -327,7 +331,10 @@ describe("run_code", () => {
 					model,
 					submitDiagnosis: () => Effect.die("no investigation in a review session"),
 					submitReview: () => Effect.void,
-					review: { coverage: { observe: () => {}, unread: () => [] }, ledger: makeReviewLedger() },
+					review: {
+						coverage: { observe: () => {}, unread: () => [], wholeDiff: () => true },
+						ledger: makeReviewLedger(),
+					},
 					codeExecutor: inProcessCodeExecutorLayer,
 					codeOnly,
 					text: "Review pull request #1.",

apps/ai/src/chat/review-coverage.test.ts

@@ -69,6 +69,32 @@ describe("makeReviewCoverage", () => {
 		)
 		nothing.observe("pr_changed_files", listing)
 		assert.deepEqual(nothing.unread(), [])
+		assert.isFalse(coverage.wholeDiff())
+	})
+
+	it("owes the pass after an unchecked fix every file, whatever changed since", () => {
+		const coverage = makeReviewCoverage(
+			[
+				"Review pull request #7.",
+				...renderFollowUp({
+					previousSha: "abcdef1234",
+					changes: { paths: ["src/b.ts"], rewritten: false },
+					open: [],
+					recheck: true,
+				}),
+			].join("\n"),
+		)
+		coverage.observe("pr_changed_files", listing)
+		assert.lengthOf(coverage.unread(), 3)
+		assert.isTrue(coverage.wholeDiff())
+	})
+
+	// Without a listing nothing is due, so an empty unread list proves nothing was checked.
+	it("is no whole-diff read until a file listing arrives", () => {
+		const coverage = makeReviewCoverage("Review pull request #7.")
+		assert.isFalse(coverage.wholeDiff())
+		coverage.observe("pr_changed_files", listing)
+		assert.isTrue(coverage.wholeDiff())
 	})
 })
 

apps/ai/src/chat/review-coverage.ts

@@ -13,6 +13,11 @@ export interface ReviewCoverage {
 	readonly observe: (tool: string, answer: string) => void
 	/** Reviewed files with a patch whose diff no `pr_file_diff` answer has shown yet, in listing order. */
 	readonly unread: () => ReadonlyArray<string>
+	/**
+	 * The kickoff names no narrower scope and a file listing arrived: every reviewed file was due,
+	 * not only what changed since. Without a listing nothing is due, so nothing was checked.
+	 */
+	readonly wholeDiff: () => boolean
 }
 
 /**
@@ -26,15 +31,18 @@ export const makeReviewCoverage = (kickoff: string): ReviewCoverage => {
 		(scope === undefined || scope.includes(path)) && !pathIgnored(path, ignored)
 	const due = new Set<string>()
 	const read = new Set<string>()
+	let listed = false
 	return {
 		observe: (tool, answer) => {
 			if (tool === "pr_changed_files") {
+				listed = true
 				for (const path of reviewablePathsInListing(answer)) if (inScope(path)) due.add(path)
 			} else if (tool === "pr_file_diff") {
 				for (const path of pathsInDiffAnswer(answer)) read.add(path)
 			}
 		},
 		unread: () => [...due].filter((path) => !read.has(path)),
+		wholeDiff: () => scope === undefined && listed,
 	}
 }
 

apps/ai/src/chat/run-review-fanout.test.ts

@@ -113,7 +113,10 @@ describe("runChatTurn review_files children", () => {
 				model,
 				submitDiagnosis: () => Effect.die("no investigation in a review session"),
 				submitReview: () => Effect.void,
-				review: { coverage: { observe: () => {}, unread: () => [] }, ledger: makeReviewLedger() },
+				review: {
+					coverage: { observe: () => {}, unread: () => [], wholeDiff: () => true },
+					ledger: makeReviewLedger(),
+				},
 				text: "Review pull request #7 of acme/shop.",
 				history: [],
 				usage: makeRunUsage(),
@@ -146,7 +149,7 @@ describe("runChatTurn review_files children", () => {
 				Effect.succeed({ content: [{ type: "text" as const, text: "src/a.ts (source, +3 -1)" }] }),
 		}
 		const review = {
-			coverage: { observe: () => {}, unread: () => [] },
+			coverage: { observe: () => {}, unread: () => [], wholeDiff: () => true },
 			ledger: makeReviewLedger(),
 			childFindings: makeChildFindings(),
 		}
@@ -237,7 +240,10 @@ describe("runChatTurn review_files children", () => {
 				model,
 				submitDiagnosis: () => Effect.die("no investigation in a review session"),
 				submitReview: () => Effect.void,
-				review: { coverage: { observe: () => {}, unread: () => [] }, ledger: makeReviewLedger() },
+				review: {
+					coverage: { observe: () => {}, unread: () => [], wholeDiff: () => true },
+					ledger: makeReviewLedger(),
+				},
 				text: "Review pull request #7 of acme/shop.",
 				history: [],
 				usage: makeRunUsage(),

apps/ai/src/chat/run-tool-failures.test.ts

@@ -394,7 +394,7 @@ describe("runChatTurn review refusals", () => {
 			submitDiagnosis: () => Effect.die("no investigation in a review session"),
 			submitReview: (_org, _id, request) => Effect.sync(() => filed.push(request)),
 			review: {
-				coverage: { observe: () => {}, unread: () => ["src/b.ts"] },
+				coverage: { observe: () => {}, unread: () => ["src/b.ts"], wholeDiff: () => true },
 				ledger: makeReviewLedger(),
 			},
 			text: "Review pull request #1.",

apps/ai/src/chat/tools.test.ts

@@ -168,16 +168,22 @@ describe("buildReviewCompletion", () => {
 	})
 
 	describe("with files unread", () => {
-		const coverage = (unread: ReadonlyArray<string>): ReviewCoverage => ({
+		const coverage = (unread: ReadonlyArray<string>, wholeDiff = true): ReviewCoverage => ({
 			observe: () => {},
 			unread: () => unread,
+			wholeDiff: () => wholeDiff,
 		})
 		const recording = () => {
 			const filed: Array<unknown> = []
 			const submit: SubmitReview = (_org, _id, request) => Effect.sync(() => filed.push(request))
 			return { filed, submit }
 		}
-		const submitWith = (submit: SubmitReview, unread: ReadonlyArray<string>, partial = false) => {
+		const submitWith = (
+			submit: SubmitReview,
+			unread: ReadonlyArray<string>,
+			partial = false,
+			wholeDiff = true,
+		) => {
 			const completion = buildReviewCompletion(
 				REVIEW_SESSION,
 				tenantFor(human),
@@ -187,7 +193,7 @@ describe("buildReviewCompletion", () => {
 				MODEL_NAME,
 				partial,
 				undefined,
-				coverage(unread),
+				coverage(unread, wholeDiff),
 			)
 			assert.isDefined(completion)
 			return {
@@ -220,6 +226,39 @@ describe("buildReviewCompletion", () => {
 			assert.lengthOf(filed, 1)
 		})
 
+		// Only a pass that read every file re-checks the fixes since the last one; the service lifts its cap on it.
+		it("marks the report a whole-diff read only when every file was due and read", async () => {
+			const filedWholeDiff = async (
+				unread: ReadonlyArray<string>,
+				partial: boolean,
+				wholeDiff: boolean,
+				submission: PrReviewSubmission = CLEAN,
+			) => {
+				const filed: Array<SubmitPrReviewRequest> = []
+				const { call } = submitWith(
+					(_org, _id, request) => Effect.sync(() => filed.push(request)),
+					unread,
+					partial,
+					wholeDiff,
+				)
+				// Unread files are refused once; the second call records.
+				await call(submission)
+				if (filed.length === 0) await call(submission)
+				assert.lengthOf(filed, 1)
+				return filed[0]?.report.wholeDiff
+			}
+			assert.isTrue(await filedWholeDiff([], false, true))
+			assert.isUndefined(await filedWholeDiff([], false, false))
+			assert.isUndefined(await filedWholeDiff(["src/b.ts"], false, true))
+			assert.isUndefined(await filedWholeDiff([], true, true))
+			assert.isUndefined(
+				await filedWholeDiff(["src/b.ts"], false, true, {
+					verdict: "not_applicable",
+					summary: "Only docs changed.",
+				}),
+			)
+		})
+
 		it("never holds a close-out or a not_applicable verdict", async () => {
 			const closeOut = recording()
 			assert.isTrue(Result.isSuccess(await submitWith(closeOut.submit, ["src/b.ts"], true).call(CLEAN)))
@@ -425,7 +464,7 @@ describe("record_finding", () => {
 			MODEL_NAME,
 			partial,
 			undefined,
-			{ observe: () => {}, unread: () => unread },
+			{ observe: () => {}, unread: () => unread, wholeDiff: () => true },
 			ledger,
 		)
 		assert.isDefined(completion)

apps/ai/src/chat/tools.ts

@@ -513,7 +513,16 @@ export const buildReviewCompletion = (
 			const saved = [...ledger.findings(), ...childFiled]
 			const unreviewed =
 				normalized.report.verdict === "not_applicable" ? [] : (coverage?.unread() ?? [])
-			const report = withSavedFindings(normalized.report, saved, unreviewed)
+			const merged = withSavedFindings(normalized.report, saved, unreviewed)
+			// A close-out files what a stopped pass saved, and not_applicable owes no reads, so
+			// neither counts as a whole-diff read.
+			const report =
+				coverage?.wholeDiff() === true &&
+				!partial &&
+				unreviewed.length === 0 &&
+				normalized.report.verdict !== "not_applicable"
+					? new PrReviewReport({ ...merged, wholeDiff: true })
+					: merged
 			const childPosted = childFiled.filter((child) =>
 				report.findings.some(
 					(finding) =>

packages/backend/src/services/pr-review/PrReviewService.test.ts

@@ -33,6 +33,7 @@ import { PrReviewFindingEmbeddings, PrReviewFindings, PrReviewMergeSteps, PrRevi
 import { fakeChatSessionsLayer } from "@maple/backend/platform/chat-sessions-fake"
 import { envPorts } from "@maple/backend/platform/env-ports"
 import { Effect, Layer, Option, Schema } from "effect"
+import { TestClock } from "effect/testing"
 import { Database } from "@maple/backend/platform/DatabaseLive"
 import { cleanupTestDbs, createTestDb, type TestDb } from "@maple/backend/platform/test-pglite"
 import {
@@ -73,6 +74,8 @@ afterEach(() => cleanupTestDbs(trackedDbs))
 const sha = Schema.decodeUnknownSync(GitCommitSha)
 const HEAD = sha("1111111111111111111111111111111111111111")
 const HEAD_2 = sha("2222222222222222222222222222222222222222")
+const HEAD_3 = sha("3333333333333333333333333333333333333333")
+const HEAD_4 = sha("4444444444444444444444444444444444444444")
 const BASE = sha("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
 
 const orgId = asOrgId("org_review")
@@ -1071,6 +1074,79 @@ describe("PrReviewService.submitReview", () => {
 		}).pipe(Effect.provide(layerFor(testDb, { published, comments })))
 	})
 
+	// A re-review reads only what changed, so a fix holds confidence at 8 until a pass re-reads every file.
+	it.effect("re-reads the whole diff after a fix, and a clean whole-diff pass lifts the cap", () => {
+		const testDb = createTestDb(trackedDbs)
+		const published: Array<PullRequestReviewPublication> = []
+		const begun: Array<Begun> = []
+		const bug = {
+			path: "a.ts",
+			line: 10,
+			category: "correctness" as const,
+			severity: "warn" as const,
+			title: "off by one",
+			body: "b",
+		}
+		return Effect.gen(function* () {
+			yield* seed(true)
+			const reviews = yield* PrReviewService
+			// Each review finishes a minute after the last, so "newest" is well defined.
+			const push = (headSha: GitCommitSha) =>
+				TestClock.adjust("1 minute").pipe(
+					Effect.andThen(
+						reviews.onPullRequestEvent(orgId, job({ action: "synchronize", headSha })),
+					),
+				)
+			const confidenceOf = (reviewId: PrReviewId) =>
+				reviews
+					.getReview(orgId, reviewId)
+					.pipe(Effect.map((stored) => Option.getOrThrow(stored).report?.confidenceReason))
+
+			const first = yield* reviews.onPullRequestEvent(orgId, job())
+			yield* reviews.submitReview(
+				orgId,
+				first.reviewId!,
+				new SubmitPrReviewRequest({
+					report: new PrReviewReport({ ...report([bug]), wholeDiff: true }),
+				}),
+			)
+
+			// Fixed in a pass that read only what changed: held at 8.
+			const fixed = yield* push(HEAD_2)
+			assert.notInclude(begun.at(-1)!.text, "review the whole diff")
+			yield* reviews.submitReview(
+				orgId,
+				fixed.reviewId!,
+				new SubmitPrReviewRequest({ resolved: ["F1"], report: report([]) }),
+			)
+			assert.equal(
+				yield* confidenceOf(fixed.reviewId!),
+				"Held at 8 because a fix has not been re-checked against the whole diff yet.",
+			)
+
+			// The next push re-reads every file; read clean, the cap is gone.
+			const recheck = yield* push(HEAD_3)
+			assert.include(begun.at(-1)!.text, "review the whole diff")
+			assert.notInclude(begun.at(-1)!.text, "Files changed since then")
+			yield* reviews.submitReview(
+				orgId,
+				recheck.reviewId!,
+				new SubmitPrReviewRequest({ report: new PrReviewReport({ ...report([]), wholeDiff: true }) }),
+			)
+			assert.notInclude((yield* confidenceOf(recheck.reviewId!)) ?? "", "Held at")
+
+			// After that, a push is reviewed incrementally again and nothing holds it.
+			const later = yield* push(HEAD_4)
+			assert.include(begun.at(-1)!.text, "Files changed since then")
+			yield* reviews.submitReview(
+				orgId,
+				later.reviewId!,
+				new SubmitPrReviewRequest({ report: report([]) }),
+			)
+			assert.notInclude((yield* confidenceOf(later.reviewId!)) ?? "", "Held at")
+		}).pipe(Effect.provide(layerFor(testDb, { published, begun })))
+	})
+
 	it.effect("follows findings across pushes: resolves fixed ones, never reposts open ones", () => {
 		const testDb = createTestDb(trackedDbs)
 		const published: Array<PullRequestReviewPublication> = []
@@ -1143,7 +1219,7 @@ describe("PrReviewService.submitReview", () => {
 			assert.include(publication.summaryComment.body, "~~F1 · off by one~~")
 			// Two open correctness warnings hold it at "needs attention", and F1, fixed, counts as history.
 			assert.equal(publication.title, "Confidence 6/10 · 2 issues to address")
-			assert.include(publication.summary, "1 defect fixed in earlier rounds")
+			assert.include(publication.summary, "1 fix not yet re-checked against the whole diff")
 			assert.deepEqual(resolvedThreads, ["T1:Fixed in `2222222`."])
 			// Every published review folds away the summaries before it and keeps its own.
 			assert.deepEqual(