diff --git a/apps/ai/src/chat/code-mode.test.ts b/apps/ai/src/chat/code-mode.test.ts index 61b1dbb676..6925df55bf 100644 --- a/apps/ai/src/chat/code-mode.test.ts +++ b/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.", diff --git a/apps/ai/src/chat/review-coverage.test.ts b/apps/ai/src/chat/review-coverage.test.ts index d09ec63b35..3cad22d8df 100644 --- a/apps/ai/src/chat/review-coverage.test.ts +++ b/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()) }) }) diff --git a/apps/ai/src/chat/review-coverage.ts b/apps/ai/src/chat/review-coverage.ts index 3aaeef2a2e..1ba7ee8f1f 100644 --- a/apps/ai/src/chat/review-coverage.ts +++ b/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 + /** + * 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() const read = new Set() + 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, } } diff --git a/apps/ai/src/chat/run-review-fanout.test.ts b/apps/ai/src/chat/run-review-fanout.test.ts index 940c051005..478a839e55 100644 --- a/apps/ai/src/chat/run-review-fanout.test.ts +++ b/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(), diff --git a/apps/ai/src/chat/run-tool-failures.test.ts b/apps/ai/src/chat/run-tool-failures.test.ts index 451e2bf104..7df591d61a 100644 --- a/apps/ai/src/chat/run-tool-failures.test.ts +++ b/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.", diff --git a/apps/ai/src/chat/tools.test.ts b/apps/ai/src/chat/tools.test.ts index a9ccb3de7a..670f18f71c 100644 --- a/apps/ai/src/chat/tools.test.ts +++ b/apps/ai/src/chat/tools.test.ts @@ -168,16 +168,22 @@ describe("buildReviewCompletion", () => { }) describe("with files unread", () => { - const coverage = (unread: ReadonlyArray): ReviewCoverage => ({ + const coverage = (unread: ReadonlyArray, wholeDiff = true): ReviewCoverage => ({ observe: () => {}, unread: () => unread, + wholeDiff: () => wholeDiff, }) const recording = () => { const filed: Array = [] const submit: SubmitReview = (_org, _id, request) => Effect.sync(() => filed.push(request)) return { filed, submit } } - const submitWith = (submit: SubmitReview, unread: ReadonlyArray, partial = false) => { + const submitWith = ( + submit: SubmitReview, + unread: ReadonlyArray, + 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, + partial: boolean, + wholeDiff: boolean, + submission: PrReviewSubmission = CLEAN, + ) => { + const filed: Array = [] + 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) diff --git a/apps/ai/src/chat/tools.ts b/apps/ai/src/chat/tools.ts index 5700a8e574..d1f887b1db 100644 --- a/apps/ai/src/chat/tools.ts +++ b/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) => diff --git a/packages/backend/src/services/pr-review/PrReviewService.test.ts b/packages/backend/src/services/pr-review/PrReviewService.test.ts index 17e19a46f9..af351dee96 100644 --- a/packages/backend/src/services/pr-review/PrReviewService.test.ts +++ b/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 = [] + const begun: Array = [] + 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 = [] @@ -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( diff --git a/packages/backend/src/services/pr-review/PrReviewService.ts b/packages/backend/src/services/pr-review/PrReviewService.ts index d1771eda80..262ea83672 100644 --- a/packages/backend/src/services/pr-review/PrReviewService.ts +++ b/packages/backend/src/services/pr-review/PrReviewService.ts @@ -128,6 +128,7 @@ import { renderFollowUp, renderIgnoredPaths, resolvedByHandle, + uncheckedFixes, SEVERITY_RANK, threadForFinding, type TrackedFinding, @@ -451,8 +452,8 @@ const severityLevel = (severity: PrReviewFinding["severity"]): PullRequestCheckA export interface CarriedFindings { readonly open: ReadonlyArray readonly resolved: ReadonlyArray - /** Defects earlier reviews of the pull request found and saw fixed; they hold confidence at 8. */ - readonly priorDefects?: number + /** Defects fixed since the last whole-diff pass; they hold confidence at 8. */ + readonly uncheckedFixes?: number } const NO_CARRIED: CarriedFindings = { open: [], resolved: [] } @@ -752,7 +753,7 @@ export const renderReviewMarkdown = (input: ReviewMarkdownInput & { readonly hea .join("/")}${line === undefined ? "" : `#L${line}`}` const observable = report.coverage.filter((unit) => unit.instrumented).length - const confidence = confidencePrReview(report, carried.open, input.partial, carried.priorDefects ?? 0) + const confidence = confidencePrReview(report, carried.open, input.partial, carried.uncheckedFixes ?? 0) // The comment is what people read on every push; inventories and evidence stay on the check run. const comment = input.heading @@ -1000,7 +1001,7 @@ export const buildPublication = (input: { const { report } = input const marker = prReviewCommentMarker(input.reviewId, input.commentAttempt) const carried = input.carried ?? NO_CARRIED - const confidence = confidencePrReview(report, carried.open, input.partial, carried.priorDefects ?? 0) + const confidence = confidencePrReview(report, carried.open, input.partial, carried.uncheckedFixes ?? 0) const threshold = SEVERITY_RANK[input.minInlineSeverity ?? "warn"] const annotations: Array = report.findings.map((finding) => ({ path: finding.path, @@ -1125,6 +1126,7 @@ export class PrReviewService extends Context.Service rows.map(toTracked))) + /** The pull request's completed reviews, newest first, and whether each read the whole diff. */ + const loadReviewHistory = (orgId: OrgId, repositoryId: VcsRepositoryId, number: number) => + dbExecute((db) => + db.run( + PG.from(PrReviews) + // The flag alone, read in SQL, so a long-lived pull request never loads its reports. + .select(($) => ({ + headSha: $.headSha, + wholeDiff: PG.sql( + PG.bool, + )`coalesce((${$.reportJson}->>'wholeDiff')::boolean, false)`, + })) + .where(($) => [ + $.orgId.eq(orgId), + $.repositoryId.eq(repositoryId), + $.number.eq(number), + $.status.eq("completed"), + ]) + .orderBy(($) => [[$.finishedAt, "desc"]]), + ), + ) + const setFindingStatus = ( orgId: OrgId, ids: ReadonlyArray, @@ -1598,23 +1622,12 @@ export class PrReviewService extends Context.Service - db.run( - PG.from(PrReviews) - .select("headSha") - .where(($) => [ - $.repositoryId.eq(repo.id), - $.number.eq(number), - $.status.eq("completed"), - $.headSha.neq(headSha), - ]) - .orderBy(($) => [[$.finishedAt, "desc"]]) - .limit(1), - ), - ) - const previousSha = previous[0]?.headSha + const history = yield* loadReviewHistory(orgId, repo.id, number) + const previousSha = history.find((review) => review.headSha !== headSha)?.headSha if (previousSha === undefined) return undefined const tracked = yield* loadTracked(orgId, repo.id, number) + // A defect fixed since the last whole-diff pass: this pass re-reads every file. + const recheck = uncheckedFixes(history, tracked) > 0 const upstream = yield* providerFor(orgId, repo) let changes: PullRequestDelta | undefined if (Option.isSome(upstream)) { @@ -1638,8 +1651,9 @@ export class PrReviewService extends Context.Service finding.status === "resolved" && isPrReviewDefectHistory(finding), - ).length + resolved.filter(isPrReviewDefectHistory).length - const confidence = confidencePrReview( - settled, - stillOpen, - request.partial === true, - priorDefects, - ) + // Defects fixed since the last whole-diff pass: nothing has re-read the pull request + // around them. A pass that read the whole diff checked every fix up to this head. + const unchecked = + settled.wholeDiff === true + ? 0 + : uncheckedFixes( + yield* loadReviewHistory(orgId, review.repositoryId, review.number), + tracked, + ) + resolved.filter(isPrReviewDefectHistory).length + const confidence = confidencePrReview(settled, stillOpen, request.partial === true, unchecked) const { confidenceReason: _reason, ...rest } = settled const report = confidence === undefined @@ -2645,7 +2658,7 @@ export class PrReviewService extends Context.Service { assert.include(text, "the rest was already reviewed: src/b.ts.") }) + it("asks for the whole diff after an unchecked fix, whatever changed since", () => { + const followUp = renderFollowUp({ + previousSha: "abcdef1234", + changes: { paths: ["src/a.ts"], rewritten: false }, + open: [], + recheck: true, + }) + assert.include(followUp.join("\n"), "review the whole diff") + assert.isUndefined(followUpScope(["Review pull request #7.", ...followUp].join("\n"))) + }) + it("asks for the whole diff when the change could not be compared", () => { for (const changes of [undefined, { paths: undefined, rewritten: true }]) { const text = renderFollowUp({ previousSha: "abcdef1234", changes, open: [] }).join("\n") @@ -201,6 +213,42 @@ describe("renderFollowUp", () => { }) }) +describe("uncheckedFixes", () => { + const fixedAt = (resolvedSha: string, overrides: Partial = {}) => + tracked({ status: "resolved", resolvedSha, ...overrides }) + const delta = (headSha: string) => ({ headSha, wholeDiff: false }) + const whole = (headSha: string) => ({ headSha, wholeDiff: true }) + + it("counts defects fixed by passes since the last whole-diff read", () => { + const reviews = [delta("c"), delta("b"), whole("a")] + assert.equal(uncheckedFixes(reviews, [fixedAt("b"), fixedAt("c", { handle: "F2" })]), 2) + }) + + it("drops fixes a whole-diff pass has read since, including one it fixed itself", () => { + assert.equal(uncheckedFixes([delta("c"), whole("b"), delta("a")], [fixedAt("a")]), 0) + assert.equal(uncheckedFixes([whole("b"), delta("a")], [fixedAt("b")]), 0) + }) + + it("counts every fix when no pass read the whole diff, as on reviews stored before the flag", () => { + assert.equal( + uncheckedFixes([delta("b"), delta("a")], [fixedAt("a"), fixedAt("b", { handle: "F2" })]), + 2, + ) + }) + + it("ignores open, dismissed and non-defect findings", () => { + const reviews = [delta("b")] + assert.equal( + uncheckedFixes(reviews, [ + tracked({ resolvedSha: "b" }), + tracked({ status: "dismissed", resolvedSha: "b" }), + fixedAt("b", { category: "maintainability" }), + ]), + 0, + ) + }) +}) + describe("followUpScope", () => { const kickoff = (paths: ReadonlyArray | undefined, rewritten = false) => [ diff --git a/packages/backend/src/services/pr-review/findings.ts b/packages/backend/src/services/pr-review/findings.ts index f1185f732f..afde2722d6 100644 --- a/packages/backend/src/services/pr-review/findings.ts +++ b/packages/backend/src/services/pr-review/findings.ts @@ -11,6 +11,7 @@ import type { PrReviewSeverity, PullRequestReviewThread, } from "@maple/domain/http" +import { isPrReviewDefectHistory } from "@maple/domain/http" import type { PullRequestDelta } from "@maple/backend/services/integrations/vcs/VcsProviderClient" import { isPrivacyScanFinding } from "./telemetry/privacy" @@ -31,6 +32,30 @@ export interface TrackedFinding { readonly title: string readonly status: PrReviewFindingStatus readonly commentId: string | null + /** The head that fixed it, once resolved. */ + readonly resolvedSha?: string | null +} + +/** + * Defects fixed since the last pass that read the whole diff. A later push's pass reads only what + * changed, so nothing has re-read the pull request around these fixes. `reviews` are the pull + * request's completed reviews, newest first; one stored before `wholeDiff` existed counts as partial. + */ +export const uncheckedFixes = ( + reviews: ReadonlyArray<{ readonly headSha: string; readonly wholeDiff: boolean }>, + tracked: ReadonlyArray, +): number => { + const lastWhole = reviews.findIndex((review) => review.wholeDiff) + const since = new Set( + (lastWhole === -1 ? reviews : reviews.slice(0, lastWhole)).map((review) => review.headSha), + ) + return tracked.filter( + (finding) => + finding.status === "resolved" && + isPrReviewDefectHistory(finding) && + finding.resolvedSha != null && + since.has(finding.resolvedSha), + ).length } /** Lines a finding may drift between pushes and still be a restatement of the same issue. */ @@ -188,15 +213,22 @@ const renderChanges = (changes: PullRequestDelta | undefined): ReadonlyArray + readonly recheck?: boolean }): ReadonlyArray => { const lines = [ `This pull request was reviewed before, at ${input.previousSha.slice(0, 7)}.`, - ...renderChanges(input.changes), + ...(input.recheck === true ? [RECHECK] : renderChanges(input.changes)), ] if (input.open.length === 0) { lines.push("No finding from earlier reviews is still open.") diff --git a/packages/backend/src/services/pr-review/review-signal-corpus.ts b/packages/backend/src/services/pr-review/review-signal-corpus.ts index f565102c02..92414d94ad 100644 --- a/packages/backend/src/services/pr-review/review-signal-corpus.ts +++ b/packages/backend/src/services/pr-review/review-signal-corpus.ts @@ -470,7 +470,8 @@ export const scoreCase = ( ...review.report.findings.flatMap((finding) => finding.handle ?? []), ]) // Defects earlier reviews filed that are no longer open here: fixed (or dismissed) before this head. - const priorDefects = earlier.filter( + // The corpus predates whole-diff passes, so no fix in it was re-checked. + const uncheckedFixes = earlier.filter( (finding) => isPrReviewDefectHistory(finding) && !openHandles.has(finding.handle ?? ""), ).length const resolved = review.resolved.flatMap((handle) => { @@ -484,7 +485,7 @@ export const scoreCase = ( report: review.report, partial: review.partial, repositoryUrl: `https://github.com/${item.repo}`, - carried: { open, resolved, priorDefects }, + carried: { open, resolved, uncheckedFixes }, }) const next = parsed[index + 1] return { @@ -492,7 +493,7 @@ export const scoreCase = ( published, parsed: review, open, - confidence: confidencePrReview(review.report, open, review.partial, priorDefects)?.confidence, + confidence: confidencePrReview(review.report, open, review.partial, uncheckedFixes)?.confidence, confidenceWithoutHistory: confidencePrReview(review.report, open, review.partial)?.confidence, openDefect: [...review.report.findings, ...open].some(isPrReviewDefect), nextFound: diff --git a/packages/domain/src/http/pr-review.test.ts b/packages/domain/src/http/pr-review.test.ts index 5f185a984e..de773d8c86 100644 --- a/packages/domain/src/http/pr-review.test.ts +++ b/packages/domain/src/http/pr-review.test.ts @@ -393,15 +393,22 @@ describe("confidencePrReview", () => { assert.equal(score(many, { tests: "missing", risk: "high" }), 5) }) - // A re-review reads what changed since; on a pull request that has had defects it can miss more. - it("holds a pull request whose earlier reviews found defects at 8", () => { + // A re-review reads what changed since; around a fix nobody re-read it can miss more. + it("holds fixes not yet re-checked against the whole diff at 8", () => { const clean = reportWith([], { tests: "covered", risk: "low" }) assert.equal(confidencePrReview(clean)?.confidence, 10) const held = confidencePrReview(clean, [], false, 2) assert.equal(held?.confidence, 8) assert.equal(held?.cappedBy, "history") - assert.equal(held?.reason, "Held at 8 because earlier reviews of this pull request found 2 defects.") - assert.include(held?.factors ?? [], "2 defects fixed in earlier rounds") + assert.equal( + held?.reason, + "Held at 8 because 2 fixes have not been re-checked against the whole diff yet.", + ) + assert.include(held?.factors ?? [], "2 fixes not yet re-checked against the whole diff") + assert.equal( + confidencePrReview(clean, [], false, 1)?.reason, + "Held at 8 because a fix has not been re-checked against the whole diff yet.", + ) // A tighter cap still binds. assert.equal(confidencePrReview(reportWith([finding("warn")]), [], false, 1)?.confidence, 6) }) diff --git a/packages/domain/src/http/pr-review.ts b/packages/domain/src/http/pr-review.ts index a7d46edd03..04c4998a9f 100644 --- a/packages/domain/src/http/pr-review.ts +++ b/packages/domain/src/http/pr-review.ts @@ -453,6 +453,8 @@ export class PrReviewReport extends Schema.Class("PrReviewReport findings: Schema.Array(PrReviewFinding), /** Reviewed files whose diff the pass never read; set by the runner, never by the model. */ unreviewed: Schema.optionalKey(Schema.Array(Schema.String)), + /** The pass read every file of the diff, not only what changed since the last review; set by the runner. */ + wholeDiff: Schema.optionalKey(Schema.Boolean), /** What production telemetry says about the change; set by the service, never by the model. */ telemetry: Schema.optionalKey(PrReviewTelemetry), /** What has to happen outside the diff before merge; diff steps are set by the service. */ @@ -928,8 +930,9 @@ export const isPrReviewDefectHistory = (finding: { readonly category?: string }) * high-risk area and new work that cannot be observed. Caps hold it down: a critical at 4 (2 with * more than one), a security warning at 4, a correctness or performance warning at 6, any other * warning at 8, a review that ended early at 6, reviewed files left unread at 8 (6 from three - * files), and a pull request whose earlier reviews found defects at 8: a re-review reads only what - * changed, and on such pull requests the next pass found another defect about a quarter of the time. + * files), and defects fixed since the last pass that read the whole diff at 8: a re-review reads + * only what changed, and after a fix the next pass found another defect about a quarter of the time. + * The pass after a fix is due the whole diff, so a clean one lifts this cap. * Without a critical, a security warning, an early end or three unread files it never reads under * 5. The reviewer's own number is not an input: a concern that should move it is a finding. * `undefined` for a pull request with nothing to review. @@ -938,8 +941,8 @@ export const confidencePrReview = ( report: PrReviewReport, carriedOpen: ReadonlyArray<{ readonly severity: PrReviewSeverity; readonly category?: string }> = [], partial = false, - /** Defects ({@link isPrReviewDefectHistory}) earlier reviews of this pull request found and saw fixed. */ - priorDefects = 0, + /** Defects ({@link isPrReviewDefectHistory}) fixed since the last pass that read the whole diff. */ + uncheckedFixes = 0, ): PrReviewConfidenceResult | undefined => { if (report.verdict === "not_applicable" && report.findings.length === 0 && carriedOpen.length === 0) { return undefined @@ -980,7 +983,11 @@ export const confidencePrReview = ( const factors = [ ...deductions.map(([label, points]) => factor(label, points)), ...(unread > 0 ? [`${unread} ${unread === 1 ? "file" : "files"} not read`] : []), - ...(priorDefects > 0 ? [`${findingPhrase(priorDefects, "defect")} fixed in earlier rounds`] : []), + ...(uncheckedFixes > 0 + ? [ + `${uncheckedFixes} ${uncheckedFixes === 1 ? "fix" : "fixes"} not yet re-checked against the whole diff`, + ] + : []), ] const total = deductions.reduce((sum, [, points]) => sum + points, 0) @@ -1025,12 +1032,12 @@ export const confidencePrReview = ( ] : []), ...(warns > 0 ? [{ at: 8, by: "warn" as const, why: "a warning is open" }] : []), - ...(priorDefects > 0 + ...(uncheckedFixes > 0 ? [ { at: 8, by: "history" as const, - why: `earlier reviews of this pull request found ${findingPhrase(priorDefects, "defect")}`, + why: `${uncheckedFixes === 1 ? "a fix has" : `${uncheckedFixes} fixes have`} not been re-checked against the whole diff yet`, }, ] : []),