From bfde2f2576f5533397c3287fedd25e68c1297b2c Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 1 Sep 2026 12:39:36 +0900 Subject: [PATCH 01/14] feat(githubbot): bound review risk epochs --- contrib/chart/Chart.yaml | 2 +- contrib/chart/templates/githubbot.yaml | 2 + contrib/chart/values.yaml | 4 + services/githubbot/README.md | 35 +- services/githubbot/src/index.ts | 8 + services/githubbot/src/pr-manager.ts | 463 +++++++++++++++++- services/githubbot/src/review-budget.ts | 322 +++++++++--- services/githubbot/src/review-findings.ts | 309 ++++++++++++ services/githubbot/src/server.ts | 3 + services/githubbot/src/types.ts | 6 +- services/githubbot/test/pr-manager.test.ts | 194 +++++++- services/githubbot/test/review-budget.test.ts | 211 +++++++- .../githubbot/test/review-findings.test.ts | 130 +++++ 13 files changed, 1574 insertions(+), 115 deletions(-) create mode 100644 services/githubbot/src/review-findings.ts create mode 100644 services/githubbot/test/review-findings.test.ts diff --git a/contrib/chart/Chart.yaml b/contrib/chart/Chart.yaml index f5a7ef5970..41856f64c5 100644 --- a/contrib/chart/Chart.yaml +++ b/contrib/chart/Chart.yaml @@ -2,7 +2,7 @@ apiVersion: v2 name: centaur description: Helm chart for the trusted Centaur control plane type: application -version: 0.1.135 +version: 0.1.136 appVersion: "0.1.0" dependencies: - name: connect diff --git a/contrib/chart/templates/githubbot.yaml b/contrib/chart/templates/githubbot.yaml index a0607b3309..17a65d6287 100644 --- a/contrib/chart/templates/githubbot.yaml +++ b/contrib/chart/templates/githubbot.yaml @@ -124,6 +124,8 @@ spec: value: {{ .Values.githubbot.reviewMaxTotalRoundsPerEpoch | quote }} - name: GITHUBBOT_REVIEW_MAX_EPOCHS value: {{ .Values.githubbot.reviewMaxEpochs | quote }} + - name: GITHUBBOT_REVIEW_MAX_SECURITY_INTERRUPTS_PER_PR + value: {{ .Values.githubbot.reviewMaxSecurityInterruptsPerPr | quote }} - name: GITHUBBOT_REVIEW_MATERIAL_CHANGE_LINES value: {{ .Values.githubbot.reviewMaterialChangeLines | quote }} - name: GITHUBBOT_REVIEW_MATERIAL_CHANGE_FILES diff --git a/contrib/chart/values.yaml b/contrib/chart/values.yaml index 8d8cfbe945..e6c048bd1b 100644 --- a/contrib/chart/values.yaml +++ b/contrib/chart/values.yaml @@ -698,6 +698,10 @@ githubbot: reviewMaxRoundsPerEpoch: 3 reviewMaxTotalRoundsPerEpoch: 6 reviewMaxEpochs: 3 + # One concrete inline P0/security finding may interrupt an exhausted PR-wide + # or inconclusive budget without resetting an epoch. Hard-capped at 16; + # further interrupts require a human reset. + reviewMaxSecurityInterruptsPerPr: 1 reviewMaterialChangeLines: 200 reviewMaterialChangeFiles: 8 # Exact, comma-separated GitHub logins for trusted reviewer bots whose diff --git a/services/githubbot/README.md b/services/githubbot/README.md index 20649b860c..48c9a3503b 100644 --- a/services/githubbot/README.md +++ b/services/githubbot/README.md @@ -84,8 +84,15 @@ back. It only ever acts on owned PRs, and on a dedicated management thread - **Address review.** A submitted review (`changes_requested` / `commented`) triggers one holistic turn that reads all the feedback, validates each finding against reachable code and enforced contracts, makes one minimal coherent commit, replies on each thread, resolves what - it addressed, and re-requests review only when code changed. Finding validation is prompt-level; - the loop limit and epoch transitions below are deterministic controller decisions. Review authors + it addressed, and re-requests review only when code changed. The controller fingerprints each + normalized finding independently of reviewer identity and retains machine-readable accepted or + evidence-rejected dispositions. A decided fingerprint is not re-opened by another bot or a moved + line; a still-pending finding remains actionable. Inline dispositions are accepted only from the + bot's reply to the original comment and must bind the original review ID. A rejection also needs + a concrete `Centaur-Finding-Evidence` line. An acceptance is persisted only after GitHub proves + the head advanced, the finding's exact path changed, and a complete descendant commit range + contains that finding's `Centaur-Review-Finding` trailer. Incomplete or capped comparisons fail + closed. Review authors must currently be an owner, organization member, or repository collaborator. Reviewer bots whose GitHub association is `NONE` require an exact login in `GITHUBBOT_REVIEW_AUTHOR_ALLOWLIST`; wildcards are rejected. @@ -94,12 +101,15 @@ back. It only ever acts on owned PRs, and on a dedicated management thread when available (with a normalized-login fallback), so one review bot cannot consume another's allowance. The epoch also has a six-round aggregate cap, so adding reviewers cannot create an unbounded side channel. Repeated reviews of the same head consume both counters. - For a new head, githubbot compares the cumulative change against the epoch anchor. Authorization, - policy, schema/migration, dependency/build, API-contract, CI/deployment changes are material, as - are changes that cross configured runtime-line or runtime-file thresholds. Non-linear or - unreadable comparisons pause instead of guessing. A material human-authored change starts a fresh - epoch until the PR-wide epoch cap; automated changes consume the current epoch and cannot award - themselves a reset. Admitting the final allowed round records the handoff pause before its repair + For a new head, githubbot compares the change since the last reviewed head. New runtime behavior, + meaningful runtime diff growth, dependency/build changes, migrations, authorization/data/API + boundaries, and CI/deployment changes are a new risk surface. Formatting-only changes, + tree-identical rebases, generated/docs/test-only diffs, and bounded repairs explicitly linked to + an accepted finding stay in the current epoch. Non-linear or unreadable comparisons pause instead + of guessing. A new-risk human-authored change starts a fresh epoch until the PR-wide epoch cap; + automated changes consume the current epoch and cannot award themselves a reset by widening the + diff. The accepted/rejected ledger survives epoch transitions. Admitting the final allowed round + records the handoff pause before its repair turn starts. Once exhausted, auto-merge remains paused across descendant heads until a reviewed, authorized transition clears the handoff; an already-running repair cannot push around the pause. Only while that handoff pause is active, a non-bot collaborator with write/admin @@ -116,6 +126,10 @@ back. It only ever acts on owned PRs, and on a dedicated management thread Approved repair heads are recorded under the same per-PR lock even when draft, CI, or a hold prevents immediate merge, preserving the correct authorship boundary for the next review. Active handoff pauses are stored without expiry; ordinary in-progress budget state retains its 90-day TTL. + One new inline P0/security finding may interrupt an exhausted budget by default, without resetting + an epoch. The interrupt requires exact `Centaur-Severity`, `Impact`, and `Evidence` fields plus a + concrete path and line/diff hunk, is keyed by the finding fingerprint, and leaves the PR paused + after that one repair turn. Ordinary severity prose cannot trigger it. - **Merge when ready.** Deterministic — no agent. When GitHub reports the PR `mergeable_state == clean` the bot merges it (`GITHUBBOT_MERGE_METHOD`, default squash) and deletes the branch. `dirty` → conflict-resolution turn; `behind` → branch update; anything else → wait. Enabled by default for @@ -216,8 +230,9 @@ requests**, **Pull request reviews**, **Check runs**, **Check suites**, and **Wo | `GITHUBBOT_REVIEW_MAX_ROUNDS_PER_EPOCH` | — | Review heads handled per reviewer within one epoch. Default 3 (initial review plus two validations). | | `GITHUBBOT_REVIEW_MAX_TOTAL_ROUNDS_PER_EPOCH` | — | Aggregate review heads handled across all reviewers in one epoch. Default 6. | | `GITHUBBOT_REVIEW_MAX_EPOCHS` | — | Material human-change epochs before explicit continuation is required. Default 3. | -| `GITHUBBOT_REVIEW_MATERIAL_CHANGE_LINES` | — | Cumulative changed runtime lines that start a new epoch for a human change. Default 200. | -| `GITHUBBOT_REVIEW_MATERIAL_CHANGE_FILES` | — | Cumulative changed runtime files that start a new epoch for a human change. Default 8. | +| `GITHUBBOT_REVIEW_MAX_SECURITY_INTERRUPTS_PER_PR` | — | Evidence-backed inline P0/security findings allowed to interrupt an exhausted or inconclusive PR-wide budget without resetting an epoch. Default 1; hard maximum 16. | +| `GITHUBBOT_REVIEW_MATERIAL_CHANGE_LINES` | — | Changed runtime lines since the last reviewed head that start a new epoch for a human change. Default 200. | +| `GITHUBBOT_REVIEW_MATERIAL_CHANGE_FILES` | — | Changed runtime files since the last reviewed head that start a new epoch for a human change. Default 8. | | `GITHUBBOT_REVIEW_AUTHOR_ALLOWLIST` | — | Comma-separated exact GitHub logins for trusted reviewer bots whose `author_association` is `NONE`. Empty by default; wildcards are rejected. Collaborator/organization/owner reviews are allowed without listing. | | `GITHUBBOT_REVIEW_RESET_LABEL` | — | One-shot, write-authorized human continuation label. Default `centaur-review-reset`. | | `GITHUBBOT_WORKFLOW_EVENTS` | — | Emit settled CI and submitted-review events to durable workflows. Default `false`. | diff --git a/services/githubbot/src/index.ts b/services/githubbot/src/index.ts index f6b337be81..d1c92e7284 100644 --- a/services/githubbot/src/index.ts +++ b/services/githubbot/src/index.ts @@ -33,6 +33,7 @@ import { extractMessageOverrides } from "./overrides"; import { handleCiEvent, handlePullRequestEvent, + handleReviewFindingDispositionComment, handleReviewEvent, isPrOwned, managementThreadKey, @@ -208,6 +209,13 @@ export function createGithubbot(options: GithubbotOptions): Githubbot { return new globalThis.Response("ok", { status: 200 }); } + if ( + isCommentEvent && + (await handleReviewFindingDispositionComment(prManagerCtx, rawBody)) + ) { + return new globalThis.Response("ok", { status: 200 }); + } + await ensureChatInitialized(); const context = { retryableErrors: [], diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index 849bd57ce8..a41b276055 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -10,14 +10,27 @@ import { DEFAULT_REVIEW_MATERIAL_CHANGE_LINES, DEFAULT_REVIEW_MAX_EPOCHS, DEFAULT_REVIEW_MAX_ROUNDS_PER_EPOCH, + DEFAULT_REVIEW_MAX_SECURITY_INTERRUPTS_PER_PR, DEFAULT_REVIEW_MAX_TOTAL_ROUNDS_PER_EPOCH, DEFAULT_REVIEW_RESET_LABEL, + MAX_REVIEW_SECURITY_INTERRUPTS_PER_PR, type ReviewAdmission, type ReviewChangeActor, type ReviewChangeAssessment, type ReviewChangeFile, type ReviewEpochState, } from "./review-budget"; +import { + acceptedFindingPaths, + applyReviewFindingDispositionMarkers, + isReviewFindingLedger, + makeReviewFinding, + mergeReviewFindings, + parseReviewFindingDispositionMarkers, + type ReviewFinding, + type ReviewFindingDispositionMarker, + type ReviewFindingLedger, +} from "./review-findings"; import { runTurnStream } from "./turn"; import { fetchCiEvaluation, @@ -235,6 +248,7 @@ function isReviewEpochState(value: unknown): value is ReviewEpochState { if (!value || typeof value !== "object" || Array.isArray(value)) return false; const candidate = value as Partial; const reviewerRounds = candidate.reviewerRoundsUsed; + const securityInterrupts = candidate.securityInterruptFingerprints; const validReviewerRounds = reviewerRounds === undefined || (reviewerRounds !== null && @@ -275,7 +289,18 @@ function isReviewEpochState(value: unknown): value is ReviewEpochState { "epoch_budget_exhausted", "reviewer_round_budget_exhausted", "round_budget_exhausted", - ].includes(candidate.pauseReason)) + ].includes(candidate.pauseReason)) && + (candidate.findingLedger === undefined || + isReviewFindingLedger(candidate.findingLedger)) && + (securityInterrupts === undefined || + (Array.isArray(securityInterrupts) && + securityInterrupts.length <= MAX_REVIEW_SECURITY_INTERRUPTS_PER_PR && + new Set(securityInterrupts).size === securityInterrupts.length && + securityInterrupts.every( + (fingerprint) => + typeof fingerprint === "string" && + /^sha256:[0-9a-f]{64}$/.test(fingerprint), + ))) ); } @@ -816,6 +841,38 @@ export async function handleReviewEvent( ); return; } + let findings: ReviewFinding[]; + try { + findings = await collectReviewFindings( + ctx, + repo.owner, + repo.repo, + number, + reviewId, + reviewNode, + reviewerKey, + effectiveHeadSha, + ); + } catch (error) { + logger(ctx).warn("githubbot_review_findings_load_failed", { + error: errorMessage(error), + pr: `${repo.owner}/${repo.repo}#${number}`, + review_id: reviewId, + }); + await release(ctx, reviewClaimKey); + return; + } + if (findings.length === 0) { + traceLog( + ctx.options, + "githubbot_review_without_findings_skipped", + makeTrace( + managementThreadKey(repo.owner, repo.repo, number), + `review-empty-${reviewId}`, + ), + ); + return; + } const admission = await runExclusive( reviewBudgetLockKey(repo.owner, repo.repo, number), () => @@ -826,12 +883,14 @@ export async function handleReviewEvent( pr, effectiveHeadSha, reviewerKey, + findings, ), ); if (!admission) { await release(ctx, reviewClaimKey); return; } + if (admission.decision === "skip") return; if (admission.decision === "pause") { await escalateReviewBudget( ctx, @@ -849,6 +908,7 @@ export async function handleReviewEvent( reviewerKey, reviewId, reviewNodeId: stringValue(reviewNode.node_id), + findings: admission.newFindings, }); if (admission.state.pausedHeadSha && admission.state.pauseReason) { await escalateReviewBudget( @@ -868,6 +928,193 @@ export async function handleReviewEvent( } } +/** + * Persist machine-readable dispositions posted by Centaur's own repair turn. + * The marker can update only an already-known fingerprint and never changes a + * budget, epoch, repository scope, or authorization decision. + */ +export async function handleReviewFindingDispositionComment( + ctx: PrManagerContext, + rawBody: string, +): Promise { + const payload = parseJson(rawBody); + if (!payload) return false; + const repo = repoFromPayload(payload); + const comment = isRecord(payload.comment) ? payload.comment : undefined; + if (!repo || !comment) return false; + const author = isRecord(comment.user) + ? stringValue(comment.user.login) + : undefined; + if ( + !author || + author.toLowerCase() !== + (ctx.botActorLogin ?? ctx.userName).toLowerCase() + ) { + return false; + } + const markers = parseReviewFindingDispositionMarkers( + stringValue(comment.body) ?? "", + ); + if (markers.length === 0) return false; + + const pullRequest = isRecord(payload.pull_request) + ? payload.pull_request + : undefined; + const issue = isRecord(payload.issue) ? payload.issue : undefined; + const number = + numberValue(pullRequest?.number) ?? + (issue && isRecord(issue.pull_request) + ? numberValue(issue.number) + : undefined); + if (number === undefined) return false; + + let changed = false; + await runExclusive(reviewBudgetLockKey(repo.owner, repo.repo, number), async () => { + const loaded = await retryingReviewBudgetLoad( + ctx, + repo.owner, + repo.repo, + number, + ); + if (!loaded.ok || !loaded.state) return; + const verifiedMarkers = await verifyReviewFindingDispositionMarkers( + ctx, + repo.owner, + repo.repo, + number, + stringValue(comment.body) ?? "", + loaded.state.findingLedger, + markers, + ); + const applied = applyReviewFindingDispositionMarkers( + loaded.state.findingLedger, + verifiedMarkers, + { + commentId: numberValue(comment.id), + replyToCommentId: numberValue(comment.in_reply_to_id), + }, + ); + if (!applied.changed) return; + changed = true; + await retryingReviewBudgetSave(ctx, repo.owner, repo.repo, number, { + ...loaded.state, + findingLedger: applied.ledger, + }); + }); + traceLog( + ctx.options, + "githubbot_review_finding_dispositions_recorded", + makeTrace( + managementThreadKey(repo.owner, repo.repo, number), + `review-disposition-${stringValue(comment.id) ?? "comment"}`, + ), + { changed, marker_count: markers.length }, + ); + return true; +} + +const REJECTED_FINDING_EVIDENCE = + /^Centaur-Finding-Evidence:\s*(\S.{19,})$/im; + +/** + * A model-authored marker is only a proposal until deterministic evidence + * verifies it. Rejections need an explicit bounded evidence statement. + * Acceptances additionally need a descendant head, a complete GitHub compare, + * an exact changed path, and the fingerprint trailer in that repair commit. + */ +async function verifyReviewFindingDispositionMarkers( + ctx: PrManagerContext, + owner: string, + repo: string, + pullNumber: number, + commentBody: string, + ledger: ReviewFindingLedger | undefined, + markers: readonly ReviewFindingDispositionMarker[], +): Promise { + const verified: ReviewFindingDispositionMarker[] = []; + const rejectedHasEvidence = REJECTED_FINDING_EVIDENCE.test(commentBody); + let currentHeadSha: string | undefined; + const acceptedEvidence = new Map(); + + for (const marker of markers) { + const finding = ledger?.[marker.fingerprint]; + if (!finding || finding.reviewId !== marker.reviewId) continue; + if (marker.disposition === "rejected") { + if (rejectedHasEvidence) verified.push(marker); + continue; + } + + if (currentHeadSha === undefined) { + currentHeadSha = + (await fetchPr(ctx, owner, repo, pullNumber))?.headSha ?? ""; + } + const evidenceKey = `${marker.fingerprint}:${currentHeadSha}`; + let hasEvidence = acceptedEvidence.get(evidenceKey); + if (hasEvidence === undefined) { + hasEvidence = await hasAcceptedFindingRepairEvidence( + ctx, + owner, + repo, + finding.reviewedHeadSha, + currentHeadSha, + marker.fingerprint, + finding.path, + ); + acceptedEvidence.set(evidenceKey, hasEvidence); + } + if (hasEvidence) verified.push(marker); + } + return verified; +} + +async function hasAcceptedFindingRepairEvidence( + ctx: PrManagerContext, + owner: string, + repo: string, + reviewedHeadSha: string, + currentHeadSha: string, + fingerprint: string, + findingPath?: string, +): Promise { + if (!currentHeadSha || currentHeadSha === reviewedHeadSha) return false; + try { + const { data } = await ctx.octokit.rest.repos.compareCommitsWithBasehead({ + basehead: `${reviewedHeadSha}...${currentHeadSha}`, + owner, + per_page: 100, + repo, + }); + if (stringValue(data.status)?.toLowerCase() !== "ahead") return false; + const commits = Array.isArray(data.commits) ? data.commits : []; + const totalCommits = + typeof data.total_commits === "number" ? data.total_commits : undefined; + if (totalCommits === undefined || totalCommits !== commits.length) return false; + const files = Array.isArray(data.files) ? data.files : undefined; + // The compare API caps this array at 300 files. Exactly 300 is therefore + // ambiguous and cannot prove an exact path was included. + if (!files || files.length === 0 || files.length >= 300) return false; + const changedFindingPath = findingPath + ? files.some( + (file) => + file.filename === findingPath || + ("previous_filename" in file && + file.previous_filename === findingPath), + ) + : true; + return ( + changedFindingPath && + findingFingerprintsFromCommits(commits).has(fingerprint) + ); + } catch (error) { + logger(ctx).warn("githubbot_review_disposition_evidence_failed", { + error: errorMessage(error), + fingerprint, + pr: `${owner}/${repo}#${reviewedHeadSha}...${currentHeadSha}`, + }); + return false; + } +} + async function maybeRecordReviewResetApproval( ctx: PrManagerContext, owner: string, @@ -1030,6 +1277,7 @@ async function compareReviewChange( repo: string, baseHeadSha: string, currentHeadSha: string, + findingLedger?: ReviewFindingLedger, ): Promise { try { const { data } = await ctx.octokit.rest.repos.compareCommitsWithBasehead({ @@ -1044,10 +1292,41 @@ async function compareReviewChange( changes: file.changes, deletions: file.deletions, filename: file.filename, + patch: file.patch, status: file.status, })); + const comparisonStatus = data.status; + let treeUnchanged = comparisonStatus === "identical"; + if ( + !treeUnchanged && + comparisonStatus && + !["ahead", "identical"].includes(comparisonStatus.toLowerCase()) + ) { + try { + const [before, after] = await Promise.all([ + ctx.octokit.rest.repos.getCommit({ owner, repo, ref: baseHeadSha }), + ctx.octokit.rest.repos.getCommit({ owner, repo, ref: currentHeadSha }), + ]); + treeUnchanged = + before.data.commit.tree.sha === after.data.commit.tree.sha; + } catch (error) { + logger(ctx).warn("githubbot_review_tree_compare_failed", { + error: errorMessage(error), + pr: `${owner}/${repo}`, + }); + } + } + const commits = Array.isArray(data.commits) ? data.commits : []; + const acceptedFingerprints = acceptedFindingFingerprintsFromCommits( + commits, + findingLedger, + ); const assessment = assessReviewChange({ - comparisonStatus: data.status, + acceptedFindingPaths: acceptedFindingPaths( + findingLedger, + acceptedFingerprints, + ), + comparisonStatus, files, fileThreshold: ctx.options.reviewMaterialChangeFiles ?? @@ -1055,8 +1334,8 @@ async function compareReviewChange( lineThreshold: ctx.options.reviewMaterialChangeLines ?? DEFAULT_REVIEW_MATERIAL_CHANGE_LINES, + treeUnchanged, }); - const commits = Array.isArray(data.commits) ? data.commits : []; const totalCommits = typeof data.total_commits === "number" ? data.total_commits : commits.length; const kinds = new Set( @@ -1083,6 +1362,95 @@ async function compareReviewChange( } } +function acceptedFindingFingerprintsFromCommits( + commits: unknown[], + ledger: ReviewFindingLedger | undefined, +): Set { + const accepted = findingFingerprintsFromCommits(commits); + return new Set( + [...accepted].filter( + (fingerprint) => ledger?.[fingerprint]?.disposition === "accepted", + ), + ); +} + +function findingFingerprintsFromCommits(commits: unknown[]): Set { + const fingerprints = new Set(); + for (const commit of commits) { + if (!isRecord(commit)) continue; + const commitNode = isRecord(commit.commit) ? commit.commit : undefined; + const message = stringValue(commitNode?.message) ?? ""; + for (const match of message.matchAll( + /^Centaur-Review-Finding:\s*(sha256:[0-9a-f]{64})\s*$/gim, + )) { + const fingerprint = match[1]?.toLowerCase(); + if (fingerprint) fingerprints.add(fingerprint); + } + } + return fingerprints; +} + +const MAX_REVIEW_COMMENT_PAGES = 10; + +async function collectReviewFindings( + ctx: PrManagerContext, + owner: string, + repo: string, + pullNumber: number, + reviewId: number, + reviewNode: JsonRecord, + reviewerKey: string, + reviewedHeadSha: string, +): Promise { + const findings: ReviewFinding[] = []; + for (let page = 1; page <= MAX_REVIEW_COMMENT_PAGES; page += 1) { + const response = await ctx.octokit.rest.pulls.listCommentsForReview({ + owner, + page, + per_page: 100, + pull_number: pullNumber, + repo, + review_id: reviewId, + }); + const comments = Array.isArray(response.data) ? response.data : []; + for (const comment of comments) { + const body = stringValue(comment.body)?.trim(); + if (!body) continue; + findings.push( + makeReviewFinding({ + body, + commentId: numberValue(comment.id), + diffHunk: stringValue(comment.diff_hunk), + line: + numberValue(comment.line) ?? numberValue(comment.original_line), + path: stringValue(comment.path), + reviewId, + reviewerKey, + reviewedHeadSha, + url: stringValue(comment.html_url), + }), + ); + } + if (comments.length < 100) break; + if (page === MAX_REVIEW_COMMENT_PAGES) { + throw new Error("review contains more than 1000 inline findings"); + } + } + + // GitHub reviews may carry one body-only finding and no inline comments. + // Do not fingerprint a summary body in addition to its inline findings: that + // would create a fresh pseudo-finding whenever a reviewer rewrites a summary. + if (findings.length === 0) { + const body = stringValue(reviewNode.body)?.trim(); + if (body) { + findings.push( + makeReviewFinding({ body, reviewId, reviewerKey, reviewedHeadSha }), + ); + } + } + return findings; +} + async function pendingReviewResetApproval( ctx: PrManagerContext, owner: string, @@ -1256,9 +1624,34 @@ async function admitReviewResponse( pr: PullRequestSummary, headSha: string, reviewerKey: string, -): Promise { + findings: readonly ReviewFinding[], +): Promise< + | (ReviewAdmission & { newFindings: ReviewFinding[] }) + | { decision: "skip"; state: ReviewEpochState } + | null +> { const loaded = await retryingReviewBudgetLoad(ctx, owner, repo, pr.number); if (!loaded.ok) return null; + const mergedFindings = mergeReviewFindings( + loaded.state?.findingLedger, + findings, + loaded.state?.epoch ?? 1, + ); + if (findings.length > 0 && mergedFindings.newFindings.length === 0) { + traceLog( + ctx.options, + "githubbot_review_findings_already_known", + makeTrace( + managementThreadKey(owner, repo, pr.number), + `review-findings-${headSha}`, + ), + { finding_count: findings.length, head_sha: headSha }, + ); + return { + decision: "skip", + state: loaded.state as ReviewEpochState, + }; + } const approval = await pendingReviewResetApproval( ctx, owner, @@ -1274,22 +1667,10 @@ async function admitReviewResponse( ctx, owner, repo, - loaded.state.anchorHeadSha, + loaded.state.lastReviewedHeadSha, headSha, + mergedFindings.ledger, ); - if ( - evidence.assessment.kind === "material" && - loaded.state.anchorHeadSha !== loaded.state.lastReviewedHeadSha - ) { - const latestRange = await compareReviewChange( - ctx, - owner, - repo, - loaded.state.lastReviewedHeadSha, - headSha, - ); - evidence = { ...evidence, actor: latestRange.actor }; - } if ( evidence.actor === "unknown" && loaded.state.automationPendingFromHeadSha === @@ -1308,16 +1689,24 @@ async function admitReviewResponse( maxRoundsPerEpoch: ctx.options.reviewMaxRoundsPerEpoch ?? DEFAULT_REVIEW_MAX_ROUNDS_PER_EPOCH, + maxSecurityInterruptsPerPr: + ctx.options.reviewMaxSecurityInterruptsPerPr ?? + DEFAULT_REVIEW_MAX_SECURITY_INTERRUPTS_PER_PR, maxTotalRoundsPerEpoch: ctx.options.reviewMaxTotalRoundsPerEpoch ?? DEFAULT_REVIEW_MAX_TOTAL_ROUNDS_PER_EPOCH, reviewerKey, + securityInterruptFingerprint: mergedFindings.newFindings.find( + (finding) => finding.severity === "p0" || finding.severity === "security", + )?.fingerprint, startsRepairTurn: true, state: loaded.state, }); - const state = approval - ? { ...admission.state, consumedResetApprovalId: approval.approvalId } - : admission.state; + const state = { + ...admission.state, + findingLedger: mergedFindings.ledger, + ...(approval ? { consumedResetApprovalId: approval.approvalId } : {}), + }; await retryingReviewBudgetSave(ctx, owner, repo, pr.number, state); if (admission.decision === "allow" && manualReset) { await cleanupReviewResetApproval(ctx, owner, repo, pr, true); @@ -1331,10 +1720,12 @@ async function admitReviewResponse( ), { assessment: admission.assessment?.kind, + change_class: admission.assessment?.changeClass, assessment_reasons: admission.assessment?.reasons, decision: admission.decision, epoch: state.epoch, head_sha: headSha, + new_finding_count: mergedFindings.newFindings.length, reset_epoch: admission.decision === "allow" ? admission.resetEpoch : undefined, reviewer_key: reviewerKey, @@ -1343,7 +1734,7 @@ async function admitReviewResponse( rounds_used: state.roundsUsed, }, ); - return { ...admission, state }; + return { ...admission, newFindings: mergedFindings.newFindings, state }; } async function escalateReviewBudget( @@ -1696,13 +2087,14 @@ function fireAddressReviewTurn( pr: PullRequestSummary, review: { budget: ReviewEpochState; + findings: ReviewFinding[]; reviewer: string; reviewerKey: string; reviewId: number; reviewNodeId?: string; }, ): void { - const { budget, reviewer, reviewerKey, reviewId, reviewNodeId } = review; + const { budget, findings, reviewer, reviewerKey, reviewId, reviewNodeId } = review; const maxReviewerRounds = ctx.options.reviewMaxRoundsPerEpoch ?? DEFAULT_REVIEW_MAX_ROUNDS_PER_EPOCH; @@ -1711,6 +2103,14 @@ function fireAddressReviewTurn( DEFAULT_REVIEW_MAX_TOTAL_ROUNDS_PER_EPOCH; const reviewerRounds = budget.reviewerRoundsUsed?.[reviewerKey] ?? budget.roundsUsed; + const findingManifest = findings + .map( + (finding) => + ` - ${finding.fingerprint} (${finding.path ?? "review body"}${ + finding.line ? `:${finding.line}` : "" + })`, + ) + .join("\n"); const preamble = `A review was submitted on pull request ${owner}/${repo}#${pr.number} ` + `(head ${pr.headSha}). This is review epoch ${budget.epoch}, reviewer round ` + @@ -1731,8 +2131,21 @@ function fireAddressReviewTurn( `- Put all agreed changes in one coherent commit on ${pr.headRef} and include ` + `the commit trailer \`Centaur-Automation: true\`, then push.\n` + `- Reply to every thread with the evidence and what changed. Where a finding ` + - `is invalid, explain the enforcing contract briefly. Resolve addressed or ` + - `evidence-rejected threads when authorized.\n` + + `is invalid, explain the enforcing contract briefly and include an exact ` + + `\`Centaur-Finding-Evidence: \` ` + + `line. Resolve addressed or ` + + `evidence-rejected threads when authorized. For each finding below, include ` + + `exactly one machine-readable disposition marker in your reply to that ` + + `finding's thread: ` + + `\`\` ` + + `when you validate and address it, or ` + + `\`\` ` + + `when concrete evidence disproves it. Never invent, alter, or reuse a ` + + `fingerprint. Push before replying. Add a ` + + `\`Centaur-Review-Finding: \` commit trailer for each accepted ` + + `finding that changed code; an accepted marker is ignored unless GitHub ` + + `proves a descendant head, that exact changed path, and its trailer.\n` + + `${findingManifest}\n` + `- Re-request review from @${reviewer} only if you pushed code.\n` + `- If a request is unclear or you can't address it, say so in the thread and ask.`; fireManagementTurn( diff --git a/services/githubbot/src/review-budget.ts b/services/githubbot/src/review-budget.ts index a235ddaea5..873e467d54 100644 --- a/services/githubbot/src/review-budget.ts +++ b/services/githubbot/src/review-budget.ts @@ -1,6 +1,10 @@ +import type { ReviewFindingLedger } from "./review-findings"; + export const DEFAULT_REVIEW_MAX_ROUNDS_PER_EPOCH = 3; export const DEFAULT_REVIEW_MAX_TOTAL_ROUNDS_PER_EPOCH = 6; export const DEFAULT_REVIEW_MAX_EPOCHS = 3; +export const DEFAULT_REVIEW_MAX_SECURITY_INTERRUPTS_PER_PR = 1; +export const MAX_REVIEW_SECURITY_INTERRUPTS_PER_PR = 16; export const DEFAULT_REVIEW_MATERIAL_CHANGE_LINES = 200; export const DEFAULT_REVIEW_MATERIAL_CHANGE_FILES = 8; export const DEFAULT_REVIEW_RESET_LABEL = "centaur-review-reset"; @@ -10,10 +14,12 @@ export type ReviewChangeFile = { changes?: number; deletions?: number; filename: string; + patch?: string; status?: string; }; export type ReviewChangeAssessment = { + changeClass: "maintenance" | "new_risk" | "repair" | "unknown"; changedFiles: number; changedLines: number; kind: "material" | "minor" | "unknown"; @@ -26,11 +32,13 @@ export type ReviewEpochState = { automationPendingFromHeadSha?: string; consumedResetApprovalId?: string; epoch: number; + findingLedger?: ReviewFindingLedger; lastReviewedHeadSha: string; pausedHeadSha?: string; pauseReason?: ReviewPauseReason; reviewerRoundsUsed?: Record; roundsUsed: number; + securityInterruptFingerprints?: string[]; version: 1; }; @@ -66,17 +74,20 @@ type ReviewAdmissionInput = { manualReset: boolean; maxEpochs: number; maxRoundsPerEpoch: number; + maxSecurityInterruptsPerPr?: number; maxTotalRoundsPerEpoch: number; reviewerKey: string; + securityInterruptFingerprint?: string; startsRepairTurn: boolean; state?: ReviewEpochState; }; const DEPENDENCY_OR_BUILD_FILE = /(^|\/)(?:Cargo\.(?:toml|lock)|Dockerfile(?:\.[^/]+)?|Gemfile(?:\.lock)?|go\.(?:mod|sum)|package(?:-lock)?\.json|pnpm-lock\.yaml|pyproject\.toml|requirements[^/]*\.txt|uv\.lock|yarn\.lock)$/i; -const CRITICAL_PATH = /(^|\/)(?:auth(?:entication|orization)?|permissions?|polic(?:y|ies)|security|migrations?|schema)(?:[._/-]|$)/i; +const MIGRATION_PATH = /(^|\/)(?:migrations?|schema)(?:[._\/-]|$)/i; +const AUTH_DATA_API_PATH = /(^|\/)(?:api|auth(?:entication|orization)?|data|permissions?|polic(?:y|ies)|security)(?:[._\/-]|$)/i; const DEPLOYMENT_PATH = /(^|\/)(?:\.github\/workflows|charts?|contrib\/chart|deploy|helm|k8s|kubernetes)(?:\/|$)/i; -const API_CONTRACT_FILE = /(^|\/)(?:openapi|asyncapi|[^/]+\.proto)(?:[._/-]|$)/i; -const NON_RUNTIME_PATH = /(^|\/)(?:docs?|examples?|fixtures?|snapshots?|tests?|testdata)(?:\/|$)|(?:\.md|\.mdx|\.rst|\.snap)$|(?:^|\.)test\.[^/]+$|(?:^|\.)spec\.[^/]+$/i; +const API_CONTRACT_FILE = /(^|\/)(?:openapi|asyncapi|[^/]+\.proto)(?:[._\/-]|$)/i; +const NON_RUNTIME_PATH = /(^|\/)(?:docs?|examples?|fixtures?|generated|snapshots?|tests?|testdata|vendor)(?:\/|$)|(?:\.md|\.mdx|\.rst|\.snap)$|(?:^|\.)test\.[^/]+$|(?:^|\.)spec\.[^/]+$/i; function nonNegative(value: number | undefined): number { return typeof value === "number" && Number.isFinite(value) && value > 0 @@ -93,38 +104,40 @@ function fileChanges(file: ReviewChangeFile): number { function isCriticalBoundary(filename: string): boolean { return ( DEPENDENCY_OR_BUILD_FILE.test(filename) || - CRITICAL_PATH.test(filename) || + MIGRATION_PATH.test(filename) || + AUTH_DATA_API_PATH.test(filename) || DEPLOYMENT_PATH.test(filename) || API_CONTRACT_FILE.test(filename) ); } export function assessReviewChange(input: { + acceptedFindingPaths?: ReadonlySet; comparisonStatus?: string; files?: ReviewChangeFile[]; fileThreshold?: number; lineThreshold?: number; + treeUnchanged?: boolean; }): ReviewChangeAssessment { const files = input.files; - if (!files) { + if (input.treeUnchanged === true) { return { - changedFiles: 0, + changeClass: "maintenance", + changedFiles: files?.length ?? 0, changedLines: 0, - kind: "unknown", - reasons: ["comparison_files_unavailable"], + kind: "minor", + reasons: ["tree_unchanged"], runtimeFiles: 0, }; } - - const comparisonStatus = input.comparisonStatus?.toLowerCase(); - if (comparisonStatus && !["ahead", "identical"].includes(comparisonStatus)) { + if (!files) { return { - changedFiles: files.length, - changedLines: files.reduce((sum, file) => sum + fileChanges(file), 0), + changeClass: "unknown", + changedFiles: 0, + changedLines: 0, kind: "unknown", - reasons: [`non_linear_comparison:${comparisonStatus}`], - runtimeFiles: files.filter((file) => !NON_RUNTIME_PATH.test(file.filename)) - .length, + reasons: ["comparison_files_unavailable"], + runtimeFiles: 0, }; } @@ -137,14 +150,102 @@ export function assessReviewChange(input: { (sum, file) => sum + fileChanges(file), 0, ); - const criticalFiles = runtime + + // GitHub caps comparison files at 300. The visible prefix cannot prove that + // an omitted file did not add a risk boundary, so this is inconclusive. + if (files.length >= 300) { + return { + changeClass: "unknown", + changedFiles: files.length, + changedLines, + kind: "unknown", + reasons: ["github_comparison_file_cap"], + runtimeFiles: runtime.length, + }; + } + + const comparisonStatus = input.comparisonStatus?.toLowerCase(); + if (comparisonStatus && !["ahead", "identical"].includes(comparisonStatus)) { + return { + changeClass: "unknown", + changedFiles: files.length, + changedLines, + kind: "unknown", + reasons: [`non_linear_comparison:${comparisonStatus}`], + runtimeFiles: runtime.length, + }; + } + + if (runtime.length === 0) { + return { + changeClass: "maintenance", + changedFiles: files.length, + changedLines: 0, + kind: "minor", + reasons: ["non_runtime_or_generated_only"], + runtimeFiles: runtime.length, + }; + } + + const semanticRuntime = runtime.filter((file) => !isFormattingOnly(file)); + if (semanticRuntime.length === 0) { + return { + changeClass: "maintenance", + changedFiles: files.length, + changedLines, + kind: "minor", + reasons: ["formatting_only"], + runtimeFiles: runtime.length, + }; + } + + const criticalFiles = semanticRuntime .map((file) => file.filename) .filter(isCriticalBoundary); - const reasons: string[] = []; + const acceptedPaths = input.acceptedFindingPaths ?? new Set(); + const boundedAcceptedRepair = + semanticRuntime.every((file) => acceptedPaths.has(file.filename)) && + criticalFiles.length === 0 && + changedLines < lineThreshold && + runtime.length < fileThreshold && + !runtime.some((file) => + ["added", "removed", "renamed"].includes(file.status?.toLowerCase() ?? ""), + ); + if (boundedAcceptedRepair) { + return { + changeClass: "repair", + changedFiles: files.length, + changedLines, + kind: "minor", + reasons: ["accepted_finding_repair"], + runtimeFiles: runtime.length, + }; + } - if (files.length >= 300) reasons.push("github_comparison_file_cap"); - if (criticalFiles.length > 0) { - reasons.push(`critical_boundary:${criticalFiles.slice(0, 3).join(",")}`); + const reasons: string[] = []; + const dependencyFiles = criticalFiles.filter((file) => + DEPENDENCY_OR_BUILD_FILE.test(file), + ); + const migrationFiles = criticalFiles.filter((file) => + MIGRATION_PATH.test(file), + ); + const authDataApiFiles = criticalFiles.filter((file) => + AUTH_DATA_API_PATH.test(file) || API_CONTRACT_FILE.test(file), + ); + const deploymentFiles = criticalFiles.filter((file) => + DEPLOYMENT_PATH.test(file), + ); + if (dependencyFiles.length > 0) { + reasons.push(`dependency_or_build:${dependencyFiles.slice(0, 3).join(",")}`); + } + if (migrationFiles.length > 0) { + reasons.push(`migration_or_schema:${migrationFiles.slice(0, 3).join(",")}`); + } + if (authDataApiFiles.length > 0) { + reasons.push(`auth_data_api_boundary:${authDataApiFiles.slice(0, 3).join(",")}`); + } + if (deploymentFiles.length > 0) { + reasons.push(`deployment_boundary:${deploymentFiles.slice(0, 3).join(",")}`); } if (changedLines >= lineThreshold) { reasons.push(`runtime_lines:${changedLines}>=${lineThreshold}`); @@ -152,19 +253,57 @@ export function assessReviewChange(input: { if (runtime.length >= fileThreshold) { reasons.push(`runtime_files:${runtime.length}>=${fileThreshold}`); } - if (runtime.some((file) => file.status?.toLowerCase() === "removed")) { - reasons.push("runtime_file_removed"); + if ( + runtime.some((file) => + ["added", "removed", "renamed"].includes(file.status?.toLowerCase() ?? ""), + ) + ) { + reasons.push("runtime_surface_changed"); } + if (reasons.length === 0) reasons.push("runtime_behavior_changed"); return { + changeClass: "new_risk", changedFiles: files.length, changedLines, - kind: reasons.length > 0 ? "material" : "minor", - reasons: reasons.length > 0 ? reasons : ["below_material_change_thresholds"], + kind: "material", + reasons, runtimeFiles: runtime.length, }; } +function isFormattingOnly(file: ReviewChangeFile): boolean { + if (!file.patch) return fileChanges(file) === 0; + const added = new Map(); + const removed = new Map(); + for (const line of file.patch.split("\n")) { + if (line.startsWith("+++ ") || line.startsWith("--- ")) continue; + const target = line.startsWith("+") + ? added + : line.startsWith("-") + ? removed + : undefined; + if (!target) continue; + // Only ignore blank lines and trailing whitespace. Leading indentation and + // whitespace inside strings can be behavioral, so treating all whitespace + // as cosmetic would allow real changes to masquerade as formatting. + const normalized = line.slice(1).trimEnd(); + if (!normalized) continue; + target.set(normalized, (target.get(normalized) ?? 0) + 1); + } + for (const [line, additions] of added) { + const cancellations = Math.min(additions, removed.get(line) ?? 0); + if (cancellations > 0) { + added.set(line, additions - cancellations); + removed.set(line, (removed.get(line) ?? 0) - cancellations); + } + } + return ( + Array.from(added.values()).every((count) => count === 0) && + Array.from(removed.values()).every((count) => count === 0) + ); +} + function firstEpoch( headSha: string, reviewerKey: string, @@ -186,18 +325,18 @@ function nextEpoch( headSha: string, reviewerKey: string, ): ReviewEpochState { - return firstEpoch(headSha, reviewerKey, state.epoch + 1); + return { + ...firstEpoch(headSha, reviewerKey, state.epoch + 1), + findingLedger: state.findingLedger, + securityInterruptFingerprints: state.securityInterruptFingerprints, + }; } function reviewerRounds( state: ReviewEpochState, reviewerKey: string, ): number { - if (!state.reviewerRoundsUsed) { - // Version-1 states written before reviewer attribution are conservatively - // charged to the first reviewer handled after upgrade. - return state.roundsUsed; - } + if (!state.reviewerRoundsUsed) return state.roundsUsed; return state.reviewerRoundsUsed[reviewerKey] ?? 0; } @@ -248,30 +387,71 @@ function withFinalRoundHandoff( return state; } +function securityInterruptAdmission( + input: ReviewAdmissionInput, + state: ReviewEpochState, + pauseReason: ReviewPauseReason, +): Extract | undefined { + const fingerprint = input.securityInterruptFingerprint; + if (!fingerprint) return undefined; + const consumed = state.securityInterruptFingerprints ?? []; + if (consumed.includes(fingerprint)) return undefined; + if ( + consumed.length >= + Math.min( + input.maxSecurityInterruptsPerPr ?? + DEFAULT_REVIEW_MAX_SECURITY_INTERRUPTS_PER_PR, + MAX_REVIEW_SECURITY_INTERRUPTS_PER_PR, + ) + ) { + return undefined; + } + const next = nextRound(state, input.headSha, input.reviewerKey); + return { + assessment: input.assessment, + decision: "allow", + resetEpoch: false, + state: { + ...next, + pausedHeadSha: input.headSha, + pauseReason, + securityInterruptFingerprints: [...consumed, fingerprint], + }, + }; +} + function exhaustedAdmission( input: ReviewAdmissionInput, state: ReviewEpochState, reviewerReason: ReviewPauseReason = "reviewer_round_budget_exhausted", -): Extract | undefined { +): ReviewAdmission | undefined { if (state.roundsUsed >= input.maxTotalRoundsPerEpoch) { - return { - assessment: input.assessment, - decision: "pause", - reason: "aggregate_round_budget_exhausted", - state: paused( + return ( + securityInterruptAdmission( + input, state, - input.headSha, "aggregate_round_budget_exhausted", - ), - }; + ) ?? { + assessment: input.assessment, + decision: "pause", + reason: "aggregate_round_budget_exhausted", + state: paused( + state, + input.headSha, + "aggregate_round_budget_exhausted", + ), + } + ); } if (reviewerRounds(state, input.reviewerKey) >= input.maxRoundsPerEpoch) { - return { - assessment: input.assessment, - decision: "pause", - reason: reviewerReason, - state: paused(state, input.headSha, reviewerReason), - }; + return ( + securityInterruptAdmission(input, state, reviewerReason) ?? { + assessment: input.assessment, + decision: "pause", + reason: reviewerReason, + state: paused(state, input.headSha, reviewerReason), + } + ); } return undefined; } @@ -330,23 +510,35 @@ export function decideReviewAdmission( } if (!input.assessment || input.assessment.kind === "unknown") { - return { - assessment: input.assessment, - decision: "pause", - reason: "change_significance_unknown", - state: paused(existing, input.headSha, "change_significance_unknown"), - }; + return ( + securityInterruptAdmission( + input, + existing, + "change_significance_unknown", + ) ?? { + assessment: input.assessment, + decision: "pause", + reason: "change_significance_unknown", + state: paused(existing, input.headSha, "change_significance_unknown"), + } + ); } if (input.assessment.kind === "material") { if (input.actor === "human") { if (existing.epoch >= input.maxEpochs) { - return { - assessment: input.assessment, - decision: "pause", - reason: "epoch_budget_exhausted", - state: paused(existing, input.headSha, "epoch_budget_exhausted"), - }; + return ( + securityInterruptAdmission( + input, + existing, + "epoch_budget_exhausted", + ) ?? { + assessment: input.assessment, + decision: "pause", + reason: "epoch_budget_exhausted", + state: paused(existing, input.headSha, "epoch_budget_exhausted"), + } + ); } return { assessment: input.assessment, @@ -363,12 +555,14 @@ export function decideReviewAdmission( }; } if (input.actor === "unknown") { - return { - assessment: input.assessment, - decision: "pause", - reason: "change_actor_unknown", - state: paused(existing, input.headSha, "change_actor_unknown"), - }; + return ( + securityInterruptAdmission(input, existing, "change_actor_unknown") ?? { + assessment: input.assessment, + decision: "pause", + reason: "change_actor_unknown", + state: paused(existing, input.headSha, "change_actor_unknown"), + } + ); } const exhausted = exhaustedAdmission( input, diff --git a/services/githubbot/src/review-findings.ts b/services/githubbot/src/review-findings.ts new file mode 100644 index 0000000000..6372c50091 --- /dev/null +++ b/services/githubbot/src/review-findings.ts @@ -0,0 +1,309 @@ +import { createHash } from "node:crypto"; + +export const MAX_REVIEW_FINDINGS = 256; + +export type ReviewFindingDisposition = "accepted" | "pending" | "rejected"; + +export type ReviewFinding = { + body: string; + commentId?: number; + diffHunk?: string; + fingerprint: string; + line?: number; + path?: string; + reviewId: number; + reviewerKey: string; + reviewedHeadSha: string; + severity: "normal" | "p0" | "security"; + url?: string; +}; + +export type ReviewFindingRecord = { + commentId?: number; + dispositionCommentId?: number; + disposition: ReviewFindingDisposition; + firstSeenEpoch: number; + path?: string; + reviewId: number; + reviewerKey: string; + reviewedHeadSha: string; + severity: ReviewFinding["severity"]; +}; + +export type ReviewFindingLedger = Record; + +export type ReviewFindingDispositionMarker = { + disposition: Exclude; + fingerprint: string; + reviewId: number; +}; + +const DISPOSITION_MARKER_SOURCE = + ""; +const EXPLICIT_P0 = /^\s*centaur-severity\s*:\s*p0\s*$/im; +const EXPLICIT_SECURITY = + /^\s*centaur-severity\s*:\s*security\s*$/im; +const EXPLICIT_IMPACT = /^\s*impact\s*:\s*\S.+$/im; +const EXPLICIT_EVIDENCE = /^\s*evidence\s*:\s*\S.+$/im; + +/** + * Build a reviewer-independent semantic fingerprint. Exact reviewer identity, + * line number, and mutable diff context are intentionally excluded so a bot + * cannot reopen the same normalized finding merely by changing accounts or + * pointing at a nearby line after a repair. + */ +export function fingerprintReviewFinding(input: { + body: string; + diffHunk?: string; + line?: number; + path?: string; +}): string { + const canonical = JSON.stringify({ + body: normalizeFindingText(input.body), + path: normalizePath(input.path), + }); + return `sha256:${createHash("sha256").update(canonical).digest("hex")}`; +} + +export function makeReviewFinding(input: { + body: string; + commentId?: number; + diffHunk?: string; + line?: number; + path?: string; + reviewId: number; + reviewerKey: string; + reviewedHeadSha: string; + url?: string; +}): ReviewFinding { + const body = input.body.trim().slice(0, 16_000); + const path = normalizePath(input.path) || undefined; + const diffHunk = input.diffHunk?.trim().slice(0, 16_000) || undefined; + const line = positiveInteger(input.line); + return { + body, + commentId: positiveInteger(input.commentId), + diffHunk, + fingerprint: fingerprintReviewFinding({ body, diffHunk, line, path }), + line, + path, + reviewId: input.reviewId, + reviewerKey: input.reviewerKey, + reviewedHeadSha: input.reviewedHeadSha, + severity: findingSeverity({ body, diffHunk, line, path }), + url: input.url?.trim().slice(0, 2_000) || undefined, + }; +} + +/** + * A budget interrupt needs a strict machine-readable severity declaration, + * bounded impact/evidence statements, and concrete inline code evidence. + * Ordinary prose containing words such as "critical" is never enough. + */ +export function findingSeverity(input: { + body: string; + diffHunk?: string; + line?: number; + path?: string; +}): ReviewFinding["severity"] { + const hasCodeEvidence = + Boolean(normalizePath(input.path)) && + (Boolean(input.diffHunk?.trim()) || positiveInteger(input.line) !== undefined); + if ( + !hasCodeEvidence || + !EXPLICIT_IMPACT.test(input.body) || + !EXPLICIT_EVIDENCE.test(input.body) + ) { + return "normal"; + } + if (EXPLICIT_P0.test(input.body)) return "p0"; + if (EXPLICIT_SECURITY.test(input.body)) return "security"; + return "normal"; +} + +export function mergeReviewFindings( + ledger: ReviewFindingLedger | undefined, + findings: readonly ReviewFinding[], + epoch: number, +): { + ledger: ReviewFindingLedger; + newFindings: ReviewFinding[]; +} { + const next: ReviewFindingLedger = { ...(ledger ?? {}) }; + const actionableFindings: ReviewFinding[] = []; + for (const finding of findings) { + const existing = next[finding.fingerprint]; + if ( + existing?.disposition === "accepted" || + existing?.disposition === "rejected" + ) { + continue; + } + // A repeated pending finding remains actionable. Only an evidence-backed + // accepted/rejected decision suppresses rediscovery. + actionableFindings.push(finding); + if (existing) continue; + next[finding.fingerprint] = { + commentId: finding.commentId, + disposition: "pending", + firstSeenEpoch: epoch, + path: finding.path, + reviewId: finding.reviewId, + reviewerKey: finding.reviewerKey, + reviewedHeadSha: finding.reviewedHeadSha, + severity: finding.severity, + }; + } + + const entries = Object.entries(next); + if (entries.length <= MAX_REVIEW_FINDINGS) { + return { ledger: next, newFindings: actionableFindings }; + } + // Retain all decided findings first, then the newest pending findings. This + // bounds durable state without letting noisy pending reviews evict decisions. + const decided = entries.filter( + ([, finding]) => finding.disposition !== "pending", + ); + const pending = entries.filter( + ([, finding]) => finding.disposition === "pending", + ); + const retainedDecided = decided.slice(-MAX_REVIEW_FINDINGS); + const remaining = MAX_REVIEW_FINDINGS - retainedDecided.length; + const retained = [ + ...retainedDecided, + ...(remaining > 0 ? pending.slice(-remaining) : []), + ]; + const retainedLedger = Object.fromEntries(retained); + return { + ledger: retainedLedger, + newFindings: actionableFindings.filter( + (finding) => retainedLedger[finding.fingerprint] !== undefined, + ), + }; +} + +export function parseReviewFindingDispositionMarkers( + body: string, +): ReviewFindingDispositionMarker[] { + const markers: ReviewFindingDispositionMarker[] = []; + const seen = new Set(); + for (const match of body.matchAll(new RegExp(DISPOSITION_MARKER_SOURCE, "gi"))) { + const fingerprint = match[1]?.toLowerCase(); + const reviewId = Number.parseInt(match[2] ?? "", 10); + const disposition = match[3]?.toLowerCase(); + if ( + !fingerprint || + !Number.isSafeInteger(reviewId) || + reviewId <= 0 || + (disposition !== "accepted" && disposition !== "rejected") + ) { + continue; + } + const key = `${fingerprint}:${reviewId}:${disposition}`; + if (seen.has(key)) continue; + seen.add(key); + markers.push({ fingerprint, reviewId, disposition }); + } + return markers; +} + +export function applyReviewFindingDispositionMarkers( + ledger: ReviewFindingLedger | undefined, + markers: readonly ReviewFindingDispositionMarker[], + source?: { commentId?: number; replyToCommentId?: number }, +): { changed: boolean; ledger: ReviewFindingLedger } { + const next: ReviewFindingLedger = { ...(ledger ?? {}) }; + let changed = false; + for (const marker of markers) { + const existing = next[marker.fingerprint]; + if ( + !existing || + existing.reviewId !== marker.reviewId || + (existing.commentId !== undefined && + source?.replyToCommentId !== existing.commentId) || + existing.disposition === marker.disposition + ) { + continue; + } + next[marker.fingerprint] = { + ...existing, + dispositionCommentId: positiveInteger(source?.commentId), + disposition: marker.disposition, + }; + changed = true; + } + return { changed, ledger: next }; +} + +export function acceptedFindingPaths( + ledger: ReviewFindingLedger | undefined, + fingerprints?: ReadonlySet, +): Set { + return new Set( + Object.entries(ledger ?? {}) + .filter( + ([fingerprint, finding]) => + finding.disposition === "accepted" && + finding.path && + (!fingerprints || fingerprints.has(fingerprint)), + ) + .map(([, finding]) => finding.path as string), + ); +} + +export function isReviewFindingLedger( + value: unknown, +): value is ReviewFindingLedger { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const entries = Object.entries(value); + if (entries.length > MAX_REVIEW_FINDINGS) return false; + return entries.every(([fingerprint, raw]) => { + if (!/^sha256:[0-9a-f]{64}$/.test(fingerprint)) return false; + if (!raw || typeof raw !== "object" || Array.isArray(raw)) return false; + const finding = raw as Partial; + return ( + ["accepted", "pending", "rejected"].includes( + finding.disposition ?? "", + ) && + typeof finding.firstSeenEpoch === "number" && + Number.isInteger(finding.firstSeenEpoch) && + finding.firstSeenEpoch > 0 && + typeof finding.reviewId === "number" && + Number.isInteger(finding.reviewId) && + finding.reviewId > 0 && + typeof finding.reviewerKey === "string" && + finding.reviewerKey.length > 0 && + typeof finding.reviewedHeadSha === "string" && + finding.reviewedHeadSha.length > 0 && + finding.reviewedHeadSha.length <= 100 && + ["normal", "p0", "security"].includes(finding.severity ?? "") && + (finding.commentId === undefined || + (Number.isInteger(finding.commentId) && finding.commentId > 0)) && + (finding.dispositionCommentId === undefined || + (Number.isInteger(finding.dispositionCommentId) && + finding.dispositionCommentId > 0)) && + (finding.path === undefined || + (typeof finding.path === "string" && finding.path.length > 0)) + ); + }); +} + +function normalizeFindingText(value: string): string { + return value + .replace(new RegExp(DISPOSITION_MARKER_SOURCE, "gi"), " ") + .replace(/https?:\/\/\S+/g, "") + .replace(/\s+/g, " ") + .trim() + .toLowerCase() + .slice(0, 16_000); +} + +function normalizePath(value: string | undefined): string { + return (value ?? "").trim().replace(/^\.\//, "").slice(0, 1_000); +} + +function positiveInteger(value: number | undefined): number | undefined { + return typeof value === "number" && Number.isInteger(value) && value > 0 + ? value + : undefined; +} diff --git a/services/githubbot/src/server.ts b/services/githubbot/src/server.ts index f527aacf60..cb8c14cf53 100644 --- a/services/githubbot/src/server.ts +++ b/services/githubbot/src/server.ts @@ -173,6 +173,9 @@ const options: GithubbotOptions = { "GITHUBBOT_REVIEW_MAX_TOTAL_ROUNDS_PER_EPOCH", ), reviewMaxEpochs: optionalNumberEnv("GITHUBBOT_REVIEW_MAX_EPOCHS"), + reviewMaxSecurityInterruptsPerPr: optionalNumberEnv( + "GITHUBBOT_REVIEW_MAX_SECURITY_INTERRUPTS_PER_PR", + ), reviewMaterialChangeLines: optionalNumberEnv( "GITHUBBOT_REVIEW_MATERIAL_CHANGE_LINES", ), diff --git a/services/githubbot/src/types.ts b/services/githubbot/src/types.ts index 646b13937f..f532d9652e 100644 --- a/services/githubbot/src/types.ts +++ b/services/githubbot/src/types.ts @@ -151,9 +151,11 @@ export type GithubbotOptions = { reviewMaxTotalRoundsPerEpoch?: number; /** Material-change epochs allowed before human continuation is required. Default 3. */ reviewMaxEpochs?: number; - /** Cumulative changed runtime lines that make a review change material. Default 200. */ + /** Evidence-backed P0/security budget interrupts allowed for the whole PR. Default 1. */ + reviewMaxSecurityInterruptsPerPr?: number; + /** Changed runtime lines since the last reviewed head that add risk. Default 200. */ reviewMaterialChangeLines?: number; - /** Cumulative changed runtime files that make a review change material. Default 8. */ + /** Changed runtime files since the last reviewed head that add risk. Default 8. */ reviewMaterialChangeFiles?: number; /** Exact GitHub logins for trusted reviewer bots whose association is NONE. */ reviewAuthorAllowlist?: readonly string[]; diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index 4ec48439bf..9522f1c02a 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -4,6 +4,7 @@ import { decideMerge, handleCiEvent, handlePullRequestEvent, + handleReviewFindingDispositionComment, handleReviewEvent, isBotAssignmentHandoff, isOwnedPr, @@ -531,6 +532,7 @@ describe("bounded review epochs", () => { function budgetCtx(input?: { actor?: "bot" | "human"; + comparisonCommitMessage?: string | (() => string); comparisonFile?: string; comments?: string[]; headSha?: string; @@ -540,6 +542,16 @@ describe("bounded review epochs", () => { permission?: string; removedLabels?: string[]; reviewAuthorAllowlist?: string[]; + reviewFindings?: Record< + number, + Array<{ + body: string; + diff_hunk?: string; + id: number; + line?: number; + path?: string; + }> + >; state?: ReturnType; }): PrManagerContext { let headSha = input?.headSha ?? "head-1"; @@ -570,6 +582,17 @@ describe("bounded review epochs", () => { if (input?.merges) input.merges.count += 1; return { data: {} }; }, + listCommentsForReview: async (request: { review_id: number }) => ({ + data: input?.reviewFindings?.[request.review_id] ?? [ + { + body: `finding-${request.review_id}`, + diff_hunk: "@@ -1 +1 @@\n-old\n+new", + id: request.review_id * 10, + line: 10, + path: "src/implementation.ts", + }, + ], + }), }, repos: { compareCommitsWithBasehead: async (request: { @@ -586,9 +609,12 @@ describe("bounded review epochs", () => { : { login: "alice", type: "User" }, commit: { message: - actor === "bot" - ? "fix review\n\nCentaur-Automation: true" - : "revise implementation", + typeof input?.comparisonCommitMessage === "function" + ? input.comparisonCommitMessage() + : input?.comparisonCommitMessage ?? + (actor === "bot" + ? "fix review\n\nCentaur-Automation: true" + : "revise implementation"), }, }, ], @@ -690,6 +716,149 @@ describe("bounded review epochs", () => { ).toMatchObject({ roundsUsed: 1 }); }); + test("does not spend another round rediscovering an evidence-rejected finding", async () => { + const state = makeState(); + const sharedFinding = { + body: "The repository allowlist is not checked before token minting.", + diff_hunk: "@@ -1 +1 @@\n-unchecked\n+checked", + line: 20, + path: "src/policy.ts", + }; + const ctx = budgetCtx({ + reviewFindings: { + 40: [{ ...sharedFinding, id: 400 }], + 41: [{ ...sharedFinding, id: 410, line: 25 }], + }, + state, + }); + + await handleReviewEvent(ctx, submittedReview(40, "head-1")); + const initial = (await state.get( + "centaur-githubbot:review-budget:base/repo#7", + )) as { findingLedger: Record; roundsUsed: number }; + const fingerprint = Object.keys(initial.findingLedger)[0]; + expect(fingerprint).toMatch(/^sha256:[0-9a-f]{64}$/); + if (!fingerprint) throw new Error("missing finding fingerprint"); + + expect( + await handleReviewFindingDispositionComment( + ctx, + JSON.stringify({ + action: "created", + comment: { + body: + "Centaur-Finding-Evidence: repository-token broker middleware rejects every unlisted repository ID.\n\n" + + ``, + id: 401, + in_reply_to_id: 400, + user: { login: "centaur-bot" }, + }, + pull_request: { number: 7 }, + repository: { full_name: "base/repo" }, + }), + ), + ).toBe(true); + + await handleReviewEvent( + ctx, + submittedReview(41, "head-1", { + id: 202, + login: "second-reviewer", + }), + ); + await drainBackgroundWork(5_000); + + expect( + await state.get("centaur-githubbot:review-budget:base/repo#7"), + ).toMatchObject({ + findingLedger: { + [fingerprint]: { + disposition: "rejected", + dispositionCommentId: 401, + }, + }, + reviewerRoundsUsed: { "github-user:101": 1 }, + roundsUsed: 1, + }); + }); + + test("ignores an accepted marker until a descendant repair is proven", async () => { + const state = makeState(); + const ctx = budgetCtx({ state }); + await handleReviewEvent(ctx, submittedReview(42, "head-1")); + const initial = (await state.get( + "centaur-githubbot:review-budget:base/repo#7", + )) as { findingLedger: Record }; + const fingerprint = Object.keys(initial.findingLedger)[0]; + if (!fingerprint) throw new Error("missing finding fingerprint"); + + expect( + await handleReviewFindingDispositionComment( + ctx, + JSON.stringify({ + action: "created", + comment: { + body: ``, + id: 421, + in_reply_to_id: 420, + user: { login: "centaur-bot" }, + }, + pull_request: { number: 7 }, + repository: { full_name: "base/repo" }, + }), + ), + ).toBe(true); + expect( + await state.get("centaur-githubbot:review-budget:base/repo#7"), + ).toMatchObject({ + findingLedger: { [fingerprint]: { disposition: "pending" } }, + }); + }); + + test("accepts a finding only after an exact-path repair with its trailer", async () => { + const state = makeState(); + let fingerprint = ""; + const ctx = budgetCtx({ + comparisonCommitMessage: () => + `fix review\n\nCentaur-Automation: true\nCentaur-Review-Finding: ${fingerprint}`, + comparisonFile: "src/implementation.ts", + state, + }); + await handleReviewEvent(ctx, submittedReview(43, "head-1")); + const initial = (await state.get( + "centaur-githubbot:review-budget:base/repo#7", + )) as { findingLedger: Record }; + fingerprint = Object.keys(initial.findingLedger)[0] ?? ""; + if (!fingerprint) throw new Error("missing finding fingerprint"); + setHeadSha(ctx, "head-2"); + + await handleReviewFindingDispositionComment( + ctx, + JSON.stringify({ + action: "created", + comment: { + body: ``, + id: 431, + in_reply_to_id: 430, + user: { login: "centaur-bot" }, + }, + pull_request: { number: 7 }, + repository: { full_name: "base/repo" }, + }), + ); + + expect( + await state.get("centaur-githubbot:review-budget:base/repo#7"), + ).toMatchObject({ + findingLedger: { + [fingerprint]: { + disposition: "accepted", + dispositionCommentId: 431, + }, + }, + }); + }); + test("admits the final review round but pauses merge before its descendant", async () => { const comments: string[] = []; const merges = { count: 0 }; @@ -932,7 +1101,7 @@ describe("bounded review epochs", () => { }); }); - test("uses the latest reviewed range for authorship while keeping cumulative materiality", async () => { + test("uses the latest reviewed range for risk and authorship", async () => { const state = makeState(); await state.set("centaur-githubbot:review-budget:base/repo#7", { anchorHeadSha: "head-1", @@ -984,7 +1153,7 @@ describe("bounded review epochs", () => { await handleReviewEvent(ctx, submittedReview(8, "head-3")); await drainBackgroundWork(5_000); - expect(compared).toEqual(["head-1...head-3", "head-2...head-3"]); + expect(compared).toEqual(["head-2...head-3"]); expect( await state.get("centaur-githubbot:review-budget:base/repo#7"), ).toMatchObject({ anchorHeadSha: "head-3", epoch: 2, roundsUsed: 1 }); @@ -1063,7 +1232,7 @@ describe("bounded review epochs", () => { await handleReviewEvent(ctx, submittedReview(34, "head-3")); await drainBackgroundWork(5_000); - expect(compared).toEqual(["head-1...head-3", "head-2...head-3"]); + expect(compared).toEqual(["head-2...head-3"]); expect( await state.get("centaur-githubbot:review-budget:base/repo#7"), ).toMatchObject({ anchorHeadSha: "head-3", epoch: 2, roundsUsed: 1 }); @@ -1121,7 +1290,7 @@ describe("bounded review epochs", () => { await state.get("centaur-githubbot:review-budget:base/repo#7"), ).toMatchObject({ pausedHeadSha: "head-2", - pauseReason: "reviewer_round_budget_exhausted", + pauseReason: "automation_material_change_requires_reset", }); }); @@ -2529,6 +2698,17 @@ describe("management turn reaction ack", () => { get: async () => ({ data: prPayload({ headRepoFullName: "base/repo" }), }), + listCommentsForReview: async () => ({ + data: [ + { + body: "A concrete review finding.", + diff_hunk: "@@ -1 +1 @@\n-old\n+new", + id: 550, + line: 1, + path: "src/implementation.ts", + }, + ], + }), merge: async () => ({ data: {} }), }, git: { deleteRef: async () => ({ data: {} }) }, diff --git a/services/githubbot/test/review-budget.test.ts b/services/githubbot/test/review-budget.test.ts index cd1edac3f1..542074052c 100644 --- a/services/githubbot/test/review-budget.test.ts +++ b/services/githubbot/test/review-budget.test.ts @@ -24,15 +24,17 @@ const epoch = (overrides: Partial = {}): ReviewEpochState => { }; describe("assessReviewChange", () => { - test("keeps a small runtime change in the current epoch", () => { + test("treats a small semantic runtime change as new behavior", () => { expect( assessReviewChange({ comparisonStatus: "ahead", files: [{ changes: 12, filename: "services/githubbot/src/turn.ts" }], }), ).toMatchObject({ + changeClass: "new_risk", changedLines: 12, - kind: "minor", + kind: "material", + reasons: ["runtime_behavior_changed"], runtimeFiles: 1, }); }); @@ -49,6 +51,7 @@ describe("assessReviewChange", () => { ], }), ).toMatchObject({ + changeClass: "maintenance", changedLines: 0, kind: "minor", runtimeFiles: 0, @@ -71,7 +74,7 @@ describe("assessReviewChange", () => { } }); - test("uses cumulative runtime size thresholds", () => { + test("uses runtime size thresholds for the reviewed delta", () => { const assessment = assessReviewChange({ comparisonStatus: "ahead", files: [ @@ -84,6 +87,99 @@ describe("assessReviewChange", () => { expect(assessment.reasons).toContain("runtime_lines:210>=200"); }); + test("keeps formatting-only patches in the current epoch", () => { + expect( + assessReviewChange({ + comparisonStatus: "ahead", + files: [ + { + changes: 2, + filename: "src/one.ts", + patch: "@@ -1 +1 @@\n-const answer = 42; \n+const answer = 42;", + }, + ], + }), + ).toMatchObject({ + changeClass: "maintenance", + kind: "minor", + reasons: ["formatting_only"], + }); + }); + + test("does not mistake whitespace inside a string for formatting", () => { + expect( + assessReviewChange({ + comparisonStatus: "ahead", + files: [ + { + changes: 2, + filename: "src/one.ts", + patch: '-const label = "a b";\n+const label = "ab";', + }, + ], + }), + ).toMatchObject({ changeClass: "new_risk", kind: "material" }); + }); + + test("keeps a tree-identical rebase in the current epoch", () => { + expect( + assessReviewChange({ + comparisonStatus: "diverged", + files: [{ changes: 40, filename: "src/one.ts" }], + treeUnchanged: true, + }), + ).toMatchObject({ + changeClass: "maintenance", + kind: "minor", + reasons: ["tree_unchanged"], + }); + }); + + test("keeps a bounded accepted-finding repair in the current epoch", () => { + expect( + assessReviewChange({ + acceptedFindingPaths: new Set(["src/one.ts"]), + comparisonStatus: "ahead", + files: [ + { + changes: 2, + filename: "src/one.ts", + patch: "@@ -1 +1 @@\n-return unsafe;\n+return checked;", + status: "modified", + }, + ], + }), + ).toMatchObject({ + changeClass: "repair", + kind: "minor", + reasons: ["accepted_finding_repair"], + }); + }); + + test("does not disguise widened or boundary-changing work as a repair", () => { + for (const files of [ + [ + { changes: 2, filename: "src/one.ts", patch: "-old\n+new" }, + { changes: 2, filename: "src/two.ts", patch: "-old\n+new" }, + ], + [ + { + changes: 2, + filename: "src/auth/policy.ts", + patch: "-old\n+new", + }, + ], + ]) { + expect( + assessReviewChange({ + acceptedFindingPaths: new Set([files[0]!.filename]), + comparisonStatus: "ahead", + files, + }), + ).toMatchObject({ changeClass: "new_risk", kind: "material" }); + } + }); + test("requires human judgment for a non-linear comparison", () => { expect( assessReviewChange({ @@ -91,16 +187,33 @@ describe("assessReviewChange", () => { files: [{ changes: 1, filename: "src/one.ts" }], }), ).toMatchObject({ + changeClass: "unknown", kind: "unknown", reasons: ["non_linear_comparison:diverged"], }); }); + + test("fails closed when GitHub's comparison file list is capped", () => { + expect( + assessReviewChange({ + comparisonStatus: "ahead", + files: Array.from({ length: 300 }, (_, index) => ({ + changes: 1, + filename: `generated/file-${index}.ts`, + })), + }), + ).toMatchObject({ + changeClass: "unknown", + kind: "unknown", + reasons: ["github_comparison_file_cap"], + }); + }); }); describe("decideReviewAdmission", () => { - const minor = assessReviewChange({ + const maintenance = assessReviewChange({ comparisonStatus: "ahead", - files: [{ changes: 5, filename: "src/one.ts" }], + files: [{ changes: 5, filename: "docs/review.md" }], }); const material = assessReviewChange({ comparisonStatus: "ahead", @@ -108,7 +221,7 @@ describe("decideReviewAdmission", () => { }); const base = { actor: "automation" as const, - assessment: minor, + assessment: maintenance, headSha: "head-2", manualReset: false, maxEpochs: 3, @@ -304,6 +417,92 @@ describe("decideReviewAdmission", () => { }); }); + test("preserves finding decisions across a new epoch", () => { + const fingerprint = `sha256:${"a".repeat(64)}`; + const findingLedger = { + [fingerprint]: { + disposition: "rejected" as const, + firstSeenEpoch: 1, + reviewId: 31, + reviewerKey: DEFAULT_REVIEWER_KEY, + reviewedHeadSha: "head-1", + severity: "normal" as const, + }, + }; + expect( + decideReviewAdmission({ + ...base, + actor: "human", + assessment: material, + state: epoch({ findingLedger }), + }), + ).toMatchObject({ + decision: "allow", + resetEpoch: true, + state: { epoch: 2, findingLedger }, + }); + }); + + test("allows one evidence-fingerprinted security interrupt without resetting", () => { + const fingerprint = `sha256:${"b".repeat(64)}`; + const interrupted = decideReviewAdmission({ + ...base, + headSha: "head-4", + securityInterruptFingerprint: fingerprint, + state: epoch({ roundsUsed: 3 }), + }); + expect(interrupted).toMatchObject({ + decision: "allow", + resetEpoch: false, + state: { + epoch: 1, + pausedHeadSha: "head-4", + pauseReason: "reviewer_round_budget_exhausted", + roundsUsed: 4, + securityInterruptFingerprints: [fingerprint], + }, + }); + expect( + decideReviewAdmission({ + ...base, + headSha: "head-5", + securityInterruptFingerprint: fingerprint, + state: interrupted.state, + }), + ).toMatchObject({ + decision: "pause", + reason: "reviewer_round_budget_exhausted", + }); + }); + + test("allows the bounded security interrupt when change evidence is inconclusive", () => { + const fingerprint = `sha256:${"c".repeat(64)}`; + expect( + decideReviewAdmission({ + ...base, + assessment: { + changeClass: "unknown", + changedFiles: 0, + changedLines: 0, + kind: "unknown", + reasons: ["comparison_files_unavailable"], + runtimeFiles: 0, + }, + securityInterruptFingerprint: fingerprint, + state: epoch({ roundsUsed: 2 }), + }), + ).toMatchObject({ + decision: "allow", + resetEpoch: false, + state: { + pausedHeadSha: "head-2", + pauseReason: "change_significance_unknown", + roundsUsed: 3, + securityInterruptFingerprints: [fingerprint], + }, + }); + }); + test("pauses when a human material change exceeds the PR epoch cap", () => { expect( decideReviewAdmission({ diff --git a/services/githubbot/test/review-findings.test.ts b/services/githubbot/test/review-findings.test.ts new file mode 100644 index 0000000000..231da48f83 --- /dev/null +++ b/services/githubbot/test/review-findings.test.ts @@ -0,0 +1,130 @@ +import { describe, expect, test } from "bun:test"; +import { + applyReviewFindingDispositionMarkers, + findingSeverity, + fingerprintReviewFinding, + makeReviewFinding, + mergeReviewFindings, + parseReviewFindingDispositionMarkers, +} from "../src/review-findings"; + +function finding(overrides: Partial[0]> = {}) { + return makeReviewFinding({ + body: "The unchecked value can escape the repository policy.", + commentId: 71, + diffHunk: "@@ -1 +1 @@\n-old\n+new", + line: 14, + path: "src/policy.ts", + reviewId: 31, + reviewerKey: "github-user:101", + reviewedHeadSha: "abc1234", + ...overrides, + }); +} + +describe("review finding fingerprints", () => { + test("are stable across reviewers, moved lines, mutable hunks, and URLs", () => { + const first = fingerprintReviewFinding({ + body: "Check https://example.test/one before use", + diffHunk: "+first implementation", + line: 10, + path: "./src/policy.ts", + }); + const second = fingerprintReviewFinding({ + body: "check https://elsewhere.test/two before use", + diffHunk: "+replacement implementation", + line: 99, + path: "src/policy.ts", + }); + expect(second).toBe(first); + expect( + fingerprintReviewFinding({ + body: "check https://elsewhere.test/two before use", + path: "src/other.ts", + }), + ).not.toBe(first); + }); + + test("requires structured impact and inline evidence for a budget interrupt", () => { + const evidence = { + diffHunk: "+untrusted(input)", + line: 22, + path: "src/auth.ts", + }; + expect( + findingSeverity({ + body: "This is a critical security problem.", + ...evidence, + }), + ).toBe("normal"); + expect( + findingSeverity({ + body: + "Centaur-Severity: security\nImpact: crosses the repository allowlist\nEvidence: untrusted input reaches token minting", + ...evidence, + }), + ).toBe("security"); + expect( + findingSeverity({ + body: + "Centaur-Severity: P0\nImpact: arbitrary deployment\nEvidence: scope is not checked", + }), + ).toBe("normal"); + }); +}); + +describe("review finding ledger", () => { + test("keeps pending repeats actionable but suppresses decided rediscovery", () => { + const first = finding(); + const initial = mergeReviewFindings(undefined, [first], 1); + expect(initial.newFindings).toHaveLength(1); + expect(initial.ledger[first.fingerprint]?.disposition).toBe("pending"); + + const repeated = finding({ + commentId: 88, + line: 40, + reviewId: 32, + reviewerKey: "github-user:202", + }); + expect(mergeReviewFindings(initial.ledger, [repeated], 1).newFindings).toHaveLength( + 1, + ); + + const marker = parseReviewFindingDispositionMarkers( + ``, + ); + const decided = applyReviewFindingDispositionMarkers(initial.ledger, marker, { + commentId: 72, + replyToCommentId: 71, + }); + expect(decided.changed).toBe(true); + expect(decided.ledger[first.fingerprint]).toMatchObject({ + disposition: "accepted", + dispositionCommentId: 72, + }); + expect(mergeReviewFindings(decided.ledger, [repeated], 2).newFindings).toEqual( + [], + ); + }); + + test("rejects a disposition detached from the original review thread", () => { + const first = finding(); + const ledger = mergeReviewFindings(undefined, [first], 1).ledger; + const wrongReview = parseReviewFindingDispositionMarkers( + ``, + ); + const wrongThread = parseReviewFindingDispositionMarkers( + ``, + ); + expect( + applyReviewFindingDispositionMarkers(ledger, wrongReview, { + replyToCommentId: 71, + }).changed, + ).toBe(false); + expect( + applyReviewFindingDispositionMarkers(ledger, wrongThread, { + replyToCommentId: 999, + }).changed, + ).toBe(false); + }); +}); From 7239b4b73da85704616814c77607f5f1159e78d0 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 09:09:54 +0900 Subject: [PATCH 02/14] Harden review finding budget evidence --- services/githubbot/src/pr-manager.ts | 16 +++-- services/githubbot/src/review-budget.ts | 30 +++++----- services/githubbot/src/review-findings.ts | 34 +++++++++-- services/githubbot/test/pr-manager.test.ts | 59 ++++++++++++++++++- services/githubbot/test/review-budget.test.ts | 15 +++++ .../githubbot/test/review-findings.test.ts | 21 ++++++- 6 files changed, 146 insertions(+), 29 deletions(-) diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index a41b276055..7d32e44e41 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -1014,7 +1014,15 @@ export async function handleReviewFindingDispositionComment( } const REJECTED_FINDING_EVIDENCE = - /^Centaur-Finding-Evidence:\s*(\S.{19,})$/im; + /^Centaur-Finding-Evidence:\s*(sha256:[0-9a-f]{64})\s+(\S.{19,})$/gim; + +function rejectedFindingEvidenceFingerprints(body: string): Set { + return new Set( + Array.from(body.matchAll(REJECTED_FINDING_EVIDENCE), (match) => + (match[1] ?? "").toLowerCase(), + ).filter(Boolean), + ); +} /** * A model-authored marker is only a proposal until deterministic evidence @@ -1032,7 +1040,7 @@ async function verifyReviewFindingDispositionMarkers( markers: readonly ReviewFindingDispositionMarker[], ): Promise { const verified: ReviewFindingDispositionMarker[] = []; - const rejectedHasEvidence = REJECTED_FINDING_EVIDENCE.test(commentBody); + const rejectedEvidence = rejectedFindingEvidenceFingerprints(commentBody); let currentHeadSha: string | undefined; const acceptedEvidence = new Map(); @@ -1040,7 +1048,7 @@ async function verifyReviewFindingDispositionMarkers( const finding = ledger?.[marker.fingerprint]; if (!finding || finding.reviewId !== marker.reviewId) continue; if (marker.disposition === "rejected") { - if (rejectedHasEvidence) verified.push(marker); + if (rejectedEvidence.has(marker.fingerprint)) verified.push(marker); continue; } @@ -2132,7 +2140,7 @@ function fireAddressReviewTurn( `the commit trailer \`Centaur-Automation: true\`, then push.\n` + `- Reply to every thread with the evidence and what changed. Where a finding ` + `is invalid, explain the enforcing contract briefly and include an exact ` + - `\`Centaur-Finding-Evidence: \` ` + + `\`Centaur-Finding-Evidence: \` ` + `line. Resolve addressed or ` + `evidence-rejected threads when authorized. For each finding below, include ` + `exactly one machine-readable disposition marker in your reply to that ` + diff --git a/services/githubbot/src/review-budget.ts b/services/githubbot/src/review-budget.ts index 873e467d54..46ffdad6db 100644 --- a/services/githubbot/src/review-budget.ts +++ b/services/githubbot/src/review-budget.ts @@ -274,14 +274,19 @@ export function assessReviewChange(input: { function isFormattingOnly(file: ReviewChangeFile): boolean { if (!file.patch) return fileChanges(file) === 0; - const added = new Map(); - const removed = new Map(); + const hunks: Array<{ added: string[]; removed: string[] }> = []; + let hunk = { added: [] as string[], removed: [] as string[] }; for (const line of file.patch.split("\n")) { if (line.startsWith("+++ ") || line.startsWith("--- ")) continue; + if (line.startsWith("@@")) { + if (hunk.added.length > 0 || hunk.removed.length > 0) hunks.push(hunk); + hunk = { added: [], removed: [] }; + continue; + } const target = line.startsWith("+") - ? added + ? hunk.added : line.startsWith("-") - ? removed + ? hunk.removed : undefined; if (!target) continue; // Only ignore blank lines and trailing whitespace. Leading indentation and @@ -289,18 +294,13 @@ function isFormattingOnly(file: ReviewChangeFile): boolean { // as cosmetic would allow real changes to masquerade as formatting. const normalized = line.slice(1).trimEnd(); if (!normalized) continue; - target.set(normalized, (target.get(normalized) ?? 0) + 1); - } - for (const [line, additions] of added) { - const cancellations = Math.min(additions, removed.get(line) ?? 0); - if (cancellations > 0) { - added.set(line, additions - cancellations); - removed.set(line, (removed.get(line) ?? 0) - cancellations); - } + target.push(normalized); } - return ( - Array.from(added.values()).every((count) => count === 0) && - Array.from(removed.values()).every((count) => count === 0) + if (hunk.added.length > 0 || hunk.removed.length > 0) hunks.push(hunk); + return hunks.every( + ({ added, removed }) => + added.length === removed.length && + added.every((line, index) => line === removed[index]), ); } diff --git a/services/githubbot/src/review-findings.ts b/services/githubbot/src/review-findings.ts index 6372c50091..984eb98209 100644 --- a/services/githubbot/src/review-findings.ts +++ b/services/githubbot/src/review-findings.ts @@ -142,7 +142,18 @@ export function mergeReviewFindings( // A repeated pending finding remains actionable. Only an evidence-backed // accepted/rejected decision suppresses rediscovery. actionableFindings.push(finding); - if (existing) continue; + if (existing) { + next[finding.fingerprint] = { + ...existing, + commentId: finding.commentId, + path: finding.path, + reviewId: finding.reviewId, + reviewerKey: finding.reviewerKey, + reviewedHeadSha: finding.reviewedHeadSha, + severity: finding.severity, + }; + continue; + } next[finding.fingerprint] = { commentId: finding.commentId, disposition: "pending", @@ -186,7 +197,10 @@ export function parseReviewFindingDispositionMarkers( body: string, ): ReviewFindingDispositionMarker[] { const markers: ReviewFindingDispositionMarker[] = []; - const seen = new Set(); + const byFinding = new Map< + string, + ReviewFindingDispositionMarker | "conflict" + >(); for (const match of body.matchAll(new RegExp(DISPOSITION_MARKER_SOURCE, "gi"))) { const fingerprint = match[1]?.toLowerCase(); const reviewId = Number.parseInt(match[2] ?? "", 10); @@ -199,10 +213,18 @@ export function parseReviewFindingDispositionMarkers( ) { continue; } - const key = `${fingerprint}:${reviewId}:${disposition}`; - if (seen.has(key)) continue; - seen.add(key); - markers.push({ fingerprint, reviewId, disposition }); + const key = `${fingerprint}:${reviewId}`; + const marker = { fingerprint, reviewId, disposition }; + const existing = byFinding.get(key); + if (existing === "conflict") continue; + if (existing && existing.disposition !== disposition) { + byFinding.set(key, "conflict"); + continue; + } + if (!existing) byFinding.set(key, marker); + } + for (const marker of byFinding.values()) { + if (marker !== "conflict") markers.push(marker); } return markers; } diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index 9522f1c02a..d91a2065af 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -747,7 +747,7 @@ describe("bounded review epochs", () => { action: "created", comment: { body: - "Centaur-Finding-Evidence: repository-token broker middleware rejects every unlisted repository ID.\n\n" + + `Centaur-Finding-Evidence: ${fingerprint} repository-token broker middleware rejects every unlisted repository ID.\n\n` + ``, id: 401, in_reply_to_id: 400, @@ -782,6 +782,63 @@ describe("bounded review epochs", () => { }); }); + test("requires rejection evidence for each exact finding fingerprint", async () => { + const state = makeState(); + const ctx = budgetCtx({ + reviewFindings: { + 44: [ + { + body: "The first repository boundary is unchecked.", + diff_hunk: "+first();", + line: 20, + path: "src/first.ts", + }, + { + body: "The second repository boundary is unchecked.", + diff_hunk: "+second();", + line: 30, + path: "src/second.ts", + }, + ], + }, + state, + }); + await handleReviewEvent(ctx, submittedReview(44, "head-1")); + const initial = (await state.get( + "centaur-githubbot:review-budget:base/repo#7", + )) as { findingLedger: Record }; + const fingerprints = Object.keys(initial.findingLedger); + expect(fingerprints).toHaveLength(2); + const [evidenced, unsupported] = fingerprints; + if (!evidenced || !unsupported) throw new Error("missing finding fingerprints"); + + await handleReviewFindingDispositionComment( + ctx, + JSON.stringify({ + action: "created", + comment: { + body: + `Centaur-Finding-Evidence: ${evidenced} deterministic policy rejects this exact path before token minting.\n\n` + + `\n` + + ``, + id: 442, + user: { login: "centaur-bot" }, + }, + pull_request: { number: 7 }, + repository: { full_name: "base/repo" }, + }), + ); + + expect( + await state.get("centaur-githubbot:review-budget:base/repo#7"), + ).toMatchObject({ + findingLedger: { + [evidenced]: { disposition: "rejected" }, + [unsupported]: { disposition: "pending" }, + }, + }); + }); + test("ignores an accepted marker until a descendant repair is proven", async () => { const state = makeState(); const ctx = budgetCtx({ state }); diff --git a/services/githubbot/test/review-budget.test.ts b/services/githubbot/test/review-budget.test.ts index 542074052c..42ab9e8ff5 100644 --- a/services/githubbot/test/review-budget.test.ts +++ b/services/githubbot/test/review-budget.test.ts @@ -121,6 +121,21 @@ describe("assessReviewChange", () => { ).toMatchObject({ changeClass: "new_risk", kind: "material" }); }); + test("does not mistake reordered lines for formatting", () => { + expect( + assessReviewChange({ + comparisonStatus: "ahead", + files: [ + { + changes: 4, + filename: "src/one.ts", + patch: "@@ -1,2 +1,2 @@\n-first();\n-second();\n+second();\n+first();", + }, + ], + }), + ).toMatchObject({ changeClass: "new_risk", kind: "material" }); + }); + test("keeps a tree-identical rebase in the current epoch", () => { expect( assessReviewChange({ diff --git a/services/githubbot/test/review-findings.test.ts b/services/githubbot/test/review-findings.test.ts index 231da48f83..4662f05fdb 100644 --- a/services/githubbot/test/review-findings.test.ts +++ b/services/githubbot/test/review-findings.test.ts @@ -86,9 +86,14 @@ describe("review finding ledger", () => { reviewId: 32, reviewerKey: "github-user:202", }); - expect(mergeReviewFindings(initial.ledger, [repeated], 1).newFindings).toHaveLength( - 1, - ); + const repeatedMerge = mergeReviewFindings(initial.ledger, [repeated], 1); + expect(repeatedMerge.newFindings).toHaveLength(1); + expect(repeatedMerge.ledger[first.fingerprint]).toMatchObject({ + commentId: 88, + firstSeenEpoch: 1, + reviewId: 32, + reviewerKey: "github-user:202", + }); const marker = parseReviewFindingDispositionMarkers( ``, @@ -127,4 +132,14 @@ describe("review finding ledger", () => { }).changed, ).toBe(false); }); + + test("discards contradictory dispositions for the same finding and review", () => { + const first = finding(); + expect( + parseReviewFindingDispositionMarkers( + `\n` + + ``, + ), + ).toEqual([]); + }); }); From 8ee5658ae280837f6ec93ad2b09bc2f1e44cb284 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 09:23:28 +0900 Subject: [PATCH 03/14] Fix review evidence type validation --- services/githubbot/src/review-findings.ts | 6 +++++- services/githubbot/test/pr-manager.test.ts | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/services/githubbot/src/review-findings.ts b/services/githubbot/src/review-findings.ts index 984eb98209..692ab06198 100644 --- a/services/githubbot/src/review-findings.ts +++ b/services/githubbot/src/review-findings.ts @@ -214,7 +214,11 @@ export function parseReviewFindingDispositionMarkers( continue; } const key = `${fingerprint}:${reviewId}`; - const marker = { fingerprint, reviewId, disposition }; + const marker: ReviewFindingDispositionMarker = { + disposition, + fingerprint, + reviewId, + }; const existing = byFinding.get(key); if (existing === "conflict") continue; if (existing && existing.disposition !== disposition) { diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index d91a2065af..9344a4f2e7 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -790,12 +790,14 @@ describe("bounded review epochs", () => { { body: "The first repository boundary is unchecked.", diff_hunk: "+first();", + id: 0, line: 20, path: "src/first.ts", }, { body: "The second repository boundary is unchecked.", diff_hunk: "+second();", + id: 0, line: 30, path: "src/second.ts", }, From a3ee438986b7a3aaaf7108e1d37f40904dbe48b8 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 09:47:58 +0900 Subject: [PATCH 04/14] Harden review budget edge cases --- services/githubbot/src/pr-manager.ts | 62 +++++++++++++++---- services/githubbot/src/review-budget.ts | 25 +++++--- services/githubbot/src/review-findings.ts | 48 +++++++++----- services/githubbot/test/pr-manager.test.ts | 8 ++- services/githubbot/test/review-budget.test.ts | 15 +++++ .../githubbot/test/review-findings.test.ts | 26 ++++++++ 6 files changed, 147 insertions(+), 37 deletions(-) diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index 7d32e44e41..7a86ed14e3 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -968,8 +968,8 @@ export async function handleReviewFindingDispositionComment( : undefined); if (number === undefined) return false; - let changed = false; - await runExclusive(reviewBudgetLockKey(repo.owner, repo.repo, number), async () => { + backgroundWaitUntil(runExclusive(reviewBudgetLockKey(repo.owner, repo.repo, number), async () => { + let changed = false; const loaded = await retryingReviewBudgetLoad( ctx, repo.owner, @@ -1000,16 +1000,16 @@ export async function handleReviewFindingDispositionComment( ...loaded.state, findingLedger: applied.ledger, }); - }); - traceLog( - ctx.options, - "githubbot_review_finding_dispositions_recorded", - makeTrace( - managementThreadKey(repo.owner, repo.repo, number), - `review-disposition-${stringValue(comment.id) ?? "comment"}`, - ), - { changed, marker_count: markers.length }, - ); + traceLog( + ctx.options, + "githubbot_review_finding_dispositions_recorded", + makeTrace( + managementThreadKey(repo.owner, repo.repo, number), + `review-disposition-${stringValue(comment.id) ?? "comment"}`, + ), + { changed, marker_count: markers.length }, + ); + })); return true; } @@ -1645,6 +1645,37 @@ async function admitReviewResponse( findings, loaded.state?.epoch ?? 1, ); + if (mergedFindings.droppedFindings > 0) { + const initialized = decideReviewAdmission({ + actor: "unknown", + headSha, + manualReset: false, + maxEpochs: ctx.options.reviewMaxEpochs ?? DEFAULT_REVIEW_MAX_EPOCHS, + maxRoundsPerEpoch: + ctx.options.reviewMaxRoundsPerEpoch ?? + DEFAULT_REVIEW_MAX_ROUNDS_PER_EPOCH, + maxTotalRoundsPerEpoch: + ctx.options.reviewMaxTotalRoundsPerEpoch ?? + DEFAULT_REVIEW_MAX_TOTAL_ROUNDS_PER_EPOCH, + reviewerKey, + startsRepairTurn: false, + state: loaded.state, + }); + const state: ReviewEpochState = { + ...initialized.state, + findingLedger: mergedFindings.ledger, + lastReviewedHeadSha: headSha, + pausedHeadSha: headSha, + pauseReason: "finding_ledger_capacity_exhausted", + }; + await retryingReviewBudgetSave(ctx, owner, repo, pr.number, state); + return { + decision: "pause", + newFindings: mergedFindings.newFindings, + reason: "finding_ledger_capacity_exhausted", + state, + }; + } if (findings.length > 0 && mergedFindings.newFindings.length === 0) { traceLog( ctx.options, @@ -1655,9 +1686,14 @@ async function admitReviewResponse( ), { finding_count: findings.length, head_sha: headSha }, ); + const state = { + ...(loaded.state as ReviewEpochState), + lastReviewedHeadSha: headSha, + }; + await retryingReviewBudgetSave(ctx, owner, repo, pr.number, state); return { decision: "skip", - state: loaded.state as ReviewEpochState, + state, }; } const approval = await pendingReviewResetApproval( diff --git a/services/githubbot/src/review-budget.ts b/services/githubbot/src/review-budget.ts index 46ffdad6db..44258b098c 100644 --- a/services/githubbot/src/review-budget.ts +++ b/services/githubbot/src/review-budget.ts @@ -49,6 +49,7 @@ export type ReviewPauseReason = | "change_actor_unknown" | "change_significance_unknown" | "epoch_budget_exhausted" + | "finding_ledger_capacity_exhausted" | "aggregate_round_budget_exhausted" | "reviewer_round_budget_exhausted" | "round_budget_exhausted"; @@ -274,21 +275,27 @@ export function assessReviewChange(input: { function isFormattingOnly(file: ReviewChangeFile): boolean { if (!file.patch) return fileChanges(file) === 0; - const hunks: Array<{ added: string[]; removed: string[] }> = []; - let hunk = { added: [] as string[], removed: [] as string[] }; + const blocks: Array<{ added: string[]; removed: string[] }> = []; + let block = { added: [] as string[], removed: [] as string[] }; + const flush = () => { + if (block.added.length > 0 || block.removed.length > 0) blocks.push(block); + block = { added: [], removed: [] }; + }; for (const line of file.patch.split("\n")) { if (line.startsWith("+++ ") || line.startsWith("--- ")) continue; if (line.startsWith("@@")) { - if (hunk.added.length > 0 || hunk.removed.length > 0) hunks.push(hunk); - hunk = { added: [], removed: [] }; + flush(); continue; } const target = line.startsWith("+") - ? hunk.added + ? block.added : line.startsWith("-") - ? hunk.removed + ? block.removed : undefined; - if (!target) continue; + if (!target) { + flush(); + continue; + } // Only ignore blank lines and trailing whitespace. Leading indentation and // whitespace inside strings can be behavioral, so treating all whitespace // as cosmetic would allow real changes to masquerade as formatting. @@ -296,8 +303,8 @@ function isFormattingOnly(file: ReviewChangeFile): boolean { if (!normalized) continue; target.push(normalized); } - if (hunk.added.length > 0 || hunk.removed.length > 0) hunks.push(hunk); - return hunks.every( + flush(); + return blocks.every( ({ added, removed }) => added.length === removed.length && added.every((line, index) => line === removed[index]), diff --git a/services/githubbot/src/review-findings.ts b/services/githubbot/src/review-findings.ts index 692ab06198..708e4b92b0 100644 --- a/services/githubbot/src/review-findings.ts +++ b/services/githubbot/src/review-findings.ts @@ -126,6 +126,7 @@ export function mergeReviewFindings( findings: readonly ReviewFinding[], epoch: number, ): { + droppedFindings: number; ledger: ReviewFindingLedger; newFindings: ReviewFinding[]; } { @@ -168,28 +169,47 @@ export function mergeReviewFindings( const entries = Object.entries(next); if (entries.length <= MAX_REVIEW_FINDINGS) { - return { ledger: next, newFindings: actionableFindings }; + return { droppedFindings: 0, ledger: next, newFindings: actionableFindings }; } - // Retain all decided findings first, then the newest pending findings. This - // bounds durable state without letting noisy pending reviews evict decisions. + // Current actionable findings must not disappear behind a full historical + // ledger. Retain them first, then older pending work, then the newest decided + // fingerprints. Old decisions are the safest entries to evict: rediscovery + // spends bounded budget, while dropping a current finding silently skips it. + const actionableFingerprints = [ + ...new Set(actionableFindings.map((finding) => finding.fingerprint)), + ]; + const actionableFingerprintSet = new Set(actionableFingerprints); + const actionable: Array<[string, ReviewFindingRecord]> = + actionableFingerprints.flatMap((fingerprint) => { + const record = next[fingerprint]; + return record ? [[fingerprint, record]] : []; + }); const decided = entries.filter( - ([, finding]) => finding.disposition !== "pending", + ([fingerprint, finding]) => + finding.disposition !== "pending" && + !actionableFingerprintSet.has(fingerprint), ); const pending = entries.filter( - ([, finding]) => finding.disposition === "pending", + ([fingerprint, finding]) => + finding.disposition === "pending" && + !actionableFingerprintSet.has(fingerprint), ); - const retainedDecided = decided.slice(-MAX_REVIEW_FINDINGS); - const remaining = MAX_REVIEW_FINDINGS - retainedDecided.length; - const retained = [ - ...retainedDecided, - ...(remaining > 0 ? pending.slice(-remaining) : []), - ]; + const retained = actionable.slice(0, MAX_REVIEW_FINDINGS); + let remaining = MAX_REVIEW_FINDINGS - retained.length; + if (remaining > 0) { + const retainedPending = pending.slice(-remaining); + retained.push(...retainedPending); + remaining -= retainedPending.length; + } + if (remaining > 0) retained.push(...decided.slice(-remaining)); const retainedLedger = Object.fromEntries(retained); + const retainedNewFindings = actionableFindings.filter( + (finding) => retainedLedger[finding.fingerprint] !== undefined, + ); return { + droppedFindings: actionableFindings.length - retainedNewFindings.length, ledger: retainedLedger, - newFindings: actionableFindings.filter( - (finding) => retainedLedger[finding.fingerprint] !== undefined, - ), + newFindings: retainedNewFindings, }; } diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index 9344a4f2e7..9831710bca 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -758,10 +758,12 @@ describe("bounded review epochs", () => { }), ), ).toBe(true); + await drainBackgroundWork(5_000); + setHeadSha(ctx, "head-2"); await handleReviewEvent( ctx, - submittedReview(41, "head-1", { + submittedReview(41, "head-2", { id: 202, login: "second-reviewer", }), @@ -779,6 +781,7 @@ describe("bounded review epochs", () => { }, reviewerRoundsUsed: { "github-user:101": 1 }, roundsUsed: 1, + lastReviewedHeadSha: "head-2", }); }); @@ -830,6 +833,7 @@ describe("bounded review epochs", () => { repository: { full_name: "base/repo" }, }), ); + await drainBackgroundWork(5_000); expect( await state.get("centaur-githubbot:review-budget:base/repo#7"), @@ -867,6 +871,7 @@ describe("bounded review epochs", () => { }), ), ).toBe(true); + await drainBackgroundWork(5_000); expect( await state.get("centaur-githubbot:review-budget:base/repo#7"), ).toMatchObject({ @@ -905,6 +910,7 @@ describe("bounded review epochs", () => { repository: { full_name: "base/repo" }, }), ); + await drainBackgroundWork(5_000); expect( await state.get("centaur-githubbot:review-budget:base/repo#7"), diff --git a/services/githubbot/test/review-budget.test.ts b/services/githubbot/test/review-budget.test.ts index 42ab9e8ff5..d75cd9ccb4 100644 --- a/services/githubbot/test/review-budget.test.ts +++ b/services/githubbot/test/review-budget.test.ts @@ -136,6 +136,21 @@ describe("assessReviewChange", () => { ).toMatchObject({ changeClass: "new_risk", kind: "material" }); }); + test("does not mistake a line moved across context for formatting", () => { + expect( + assessReviewChange({ + comparisonStatus: "ahead", + files: [ + { + changes: 2, + filename: "src/one.ts", + patch: "@@ -1,2 +1,2 @@\n-authorize();\n mutate();\n+authorize();", + }, + ], + }), + ).toMatchObject({ changeClass: "new_risk", kind: "material" }); + }); + test("keeps a tree-identical rebase in the current epoch", () => { expect( assessReviewChange({ diff --git a/services/githubbot/test/review-findings.test.ts b/services/githubbot/test/review-findings.test.ts index 4662f05fdb..423cfb455e 100644 --- a/services/githubbot/test/review-findings.test.ts +++ b/services/githubbot/test/review-findings.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { + MAX_REVIEW_FINDINGS, applyReviewFindingDispositionMarkers, findingSeverity, fingerprintReviewFinding, @@ -142,4 +143,29 @@ describe("review finding ledger", () => { ), ).toEqual([]); }); + + test("evicts old decisions before dropping a current finding", () => { + const ledger = Object.fromEntries( + Array.from({ length: MAX_REVIEW_FINDINGS }, (_, index) => { + const item = finding({ body: `Historical finding ${index}` }); + return [ + item.fingerprint, + { + disposition: "rejected" as const, + firstSeenEpoch: 1, + reviewId: item.reviewId, + reviewedHeadSha: item.reviewedHeadSha, + reviewerKey: item.reviewerKey, + severity: item.severity, + }, + ]; + }), + ); + const current = finding({ body: "A current actionable finding" }); + const merged = mergeReviewFindings(ledger, [current], 2); + expect(Object.keys(merged.ledger)).toHaveLength(MAX_REVIEW_FINDINGS); + expect(merged.ledger[current.fingerprint]?.disposition).toBe("pending"); + expect(merged.newFindings).toEqual([current]); + expect(merged.droppedFindings).toBe(0); + }); }); From c186c8d33792842ee74e799e20e73f550aff688a Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 10:11:42 +0900 Subject: [PATCH 05/14] Close review budget recovery gaps --- services/githubbot/src/pr-manager.ts | 31 +++-- services/githubbot/test/pr-manager.test.ts | 136 +++++++++++++++++++++ 2 files changed, 157 insertions(+), 10 deletions(-) diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index 7a86ed14e3..e1c9e4b064 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -287,6 +287,7 @@ function isReviewEpochState(value: unknown): value is ReviewEpochState { "change_actor_unknown", "change_significance_unknown", "epoch_budget_exhausted", + "finding_ledger_capacity_exhausted", "reviewer_round_budget_exhausted", "round_budget_exhausted", ].includes(candidate.pauseReason)) && @@ -1084,7 +1085,15 @@ async function hasAcceptedFindingRepairEvidence( fingerprint: string, findingPath?: string, ): Promise { - if (!currentHeadSha || currentHeadSha === reviewedHeadSha) return false; + // A body-only finding has no deterministic affected path. A commit trailer + // alone cannot prove that the finding was repaired, so keep it pending. + if ( + !findingPath || + !currentHeadSha || + currentHeadSha === reviewedHeadSha + ) { + return false; + } try { const { data } = await ctx.octokit.rest.repos.compareCommitsWithBasehead({ basehead: `${reviewedHeadSha}...${currentHeadSha}`, @@ -1101,14 +1110,12 @@ async function hasAcceptedFindingRepairEvidence( // The compare API caps this array at 300 files. Exactly 300 is therefore // ambiguous and cannot prove an exact path was included. if (!files || files.length === 0 || files.length >= 300) return false; - const changedFindingPath = findingPath - ? files.some( - (file) => - file.filename === findingPath || - ("previous_filename" in file && - file.previous_filename === findingPath), - ) - : true; + const changedFindingPath = files.some( + (file) => + file.filename === findingPath || + ("previous_filename" in file && + file.previous_filename === findingPath), + ); return ( changedFindingPath && findingFingerprintsFromCommits(commits).has(fingerprint) @@ -1741,7 +1748,11 @@ async function admitReviewResponse( DEFAULT_REVIEW_MAX_TOTAL_ROUNDS_PER_EPOCH, reviewerKey, securityInterruptFingerprint: mergedFindings.newFindings.find( - (finding) => finding.severity === "p0" || finding.severity === "security", + (finding) => + (finding.severity === "p0" || finding.severity === "security") && + !loaded.state?.securityInterruptFingerprints?.includes( + finding.fingerprint, + ), )?.fingerprint, startsRepairTurn: true, state: loaded.state, diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index 9831710bca..f98818b561 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -10,6 +10,7 @@ import { isOwnedPr, type PrManagerContext, } from "../src/pr-manager"; +import { fingerprintReviewFinding } from "../src/review-findings"; import { emitWorkflowEvent } from "../src/session-api"; import { evaluateCi, @@ -538,6 +539,7 @@ describe("bounded review epochs", () => { headSha?: string; merges?: { count: number }; maxRoundsPerEpoch?: number; + maxSecurityInterruptsPerPr?: number; maxTotalRoundsPerEpoch?: number; permission?: string; removedLabels?: string[]; @@ -645,6 +647,8 @@ describe("bounded review epochs", () => { fetch: () => Promise.resolve(new Response("no", { status: 400 })), logger: quietLogger, reviewMaxRoundsPerEpoch: input?.maxRoundsPerEpoch, + reviewMaxSecurityInterruptsPerPr: + input?.maxSecurityInterruptsPerPr, reviewMaxTotalRoundsPerEpoch: input?.maxTotalRoundsPerEpoch, reviewAuthorAllowlist: input?.reviewAuthorAllowlist, }, @@ -924,6 +928,47 @@ describe("bounded review epochs", () => { }); }); + test("keeps a body-only finding pending without exact repaired-path evidence", async () => { + const state = makeState(); + let fingerprint = ""; + const ctx = budgetCtx({ + comparisonCommitMessage: () => + `fix review\n\nCentaur-Automation: true\nCentaur-Review-Finding: ${fingerprint}`, + reviewFindings: { + 45: [{ body: "A body-only finding without a file path.", id: 0 }], + }, + state, + }); + await handleReviewEvent(ctx, submittedReview(45, "head-1")); + const initial = (await state.get( + "centaur-githubbot:review-budget:base/repo#7", + )) as { findingLedger: Record }; + fingerprint = Object.keys(initial.findingLedger)[0] ?? ""; + if (!fingerprint) throw new Error("missing finding fingerprint"); + setHeadSha(ctx, "head-2"); + + await handleReviewFindingDispositionComment( + ctx, + JSON.stringify({ + action: "created", + comment: { + body: ``, + id: 451, + user: { login: "centaur-bot" }, + }, + pull_request: { number: 7 }, + repository: { full_name: "base/repo" }, + }), + ); + await drainBackgroundWork(5_000); + + expect( + await state.get("centaur-githubbot:review-budget:base/repo#7"), + ).toMatchObject({ + findingLedger: { [fingerprint]: { disposition: "pending" } }, + }); + }); + test("admits the final review round but pauses merge before its descendant", async () => { const comments: string[] = []; const merges = { count: 0 }; @@ -962,6 +1007,67 @@ describe("bounded review epochs", () => { expect(comments[0]).toContain("centaur-review-reset"); }); + test("selects a new severe finding when an earlier interrupt was consumed", async () => { + const oldBody = + "Centaur-Severity: security\nImpact: old boundary remains exposed\nEvidence: exact old call site is shown"; + const newBody = + "Centaur-Severity: security\nImpact: new boundary permits widening\nEvidence: exact new call site is shown"; + const oldPath = "src/old-policy.ts"; + const newPath = "src/new-policy.ts"; + const oldFingerprint = fingerprintReviewFinding({ + body: oldBody, + path: oldPath, + }); + const newFingerprint = fingerprintReviewFinding({ + body: newBody, + path: newPath, + }); + const state = makeState(); + await state.set("centaur-githubbot:review-budget:base/repo#7", { + anchorHeadSha: "head-1", + epoch: 1, + findingLedger: { + [oldFingerprint]: { + commentId: 460, + disposition: "pending", + firstSeenEpoch: 1, + path: oldPath, + reviewId: 45, + reviewedHeadSha: "head-1", + reviewerKey: "github-user:101", + severity: "security", + }, + }, + lastReviewedHeadSha: "head-1", + pausedHeadSha: "head-1", + pauseReason: "reviewer_round_budget_exhausted", + reviewerRoundsUsed: { "github-user:101": 1 }, + roundsUsed: 1, + securityInterruptFingerprints: [oldFingerprint], + version: 1, + }); + const ctx = budgetCtx({ + maxRoundsPerEpoch: 1, + maxSecurityInterruptsPerPr: 2, + reviewFindings: { + 46: [ + { body: oldBody, diff_hunk: "+old();", id: 460, line: 20, path: oldPath }, + { body: newBody, diff_hunk: "+new();", id: 461, line: 30, path: newPath }, + ], + }, + state, + }); + + await handleReviewEvent(ctx, submittedReview(46, "head-1")); + + expect( + await state.get("centaur-githubbot:review-budget:base/repo#7"), + ).toMatchObject({ + roundsUsed: 2, + securityInterruptFingerprints: [oldFingerprint, newFingerprint], + }); + }); + test("stores an active handoff pause without expiration", async () => { const durableState = makeState(); await durableState.set("centaur-githubbot:review-budget:base/repo#7", { @@ -1662,6 +1768,36 @@ describe("bounded review epochs", () => { expect(removedLabels).toEqual(["centaur-review-reset"]); }); + test("round-trips a capacity pause so an authorized reset can recover it", async () => { + const state = makeState(); + await state.set("centaur-githubbot:review-budget:base/repo#7", { + anchorHeadSha: "head-1", + epoch: 1, + lastReviewedHeadSha: "head-4", + pausedHeadSha: "head-4", + pauseReason: "finding_ledger_capacity_exhausted", + roundsUsed: 1, + version: 1, + }); + const ctx = budgetCtx({ headSha: "head-4", state }); + + await handlePullRequestEvent( + ctx, + JSON.stringify({ + action: "labeled", + label: { name: "centaur-review-reset" }, + pull_request: { number: 7 }, + repository: { full_name: "base/repo" }, + sender: { login: "alice", type: "User" }, + }), + "capacity-reset", + ); + + expect( + await state.get("centaur-githubbot:review-reset:base/repo#7:head-4"), + ).toMatchObject({ approvalId: "capacity-reset" }); + }); + test("retries consumed reset-label removal before deleting approval", async () => { const removedLabels: string[] = []; const state = makeState(); From d50e264db1e1a27bdf6882eee8ba75bf10b11849 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 10:26:37 +0900 Subject: [PATCH 06/14] Treat patchless runtime changes as semantic --- services/githubbot/src/review-budget.ts | 4 +++- services/githubbot/test/review-budget.test.ts | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/services/githubbot/src/review-budget.ts b/services/githubbot/src/review-budget.ts index 44258b098c..2c051d3f62 100644 --- a/services/githubbot/src/review-budget.ts +++ b/services/githubbot/src/review-budget.ts @@ -274,7 +274,9 @@ export function assessReviewChange(input: { } function isFormattingOnly(file: ReviewChangeFile): boolean { - if (!file.patch) return fileChanges(file) === 0; + // GitHub omits patches for binaries and some oversized diffs. Absence of a + // textual patch is never evidence that a reported file change was cosmetic. + if (!file.patch) return false; const blocks: Array<{ added: string[]; removed: string[] }> = []; let block = { added: [] as string[], removed: [] as string[] }; const flush = () => { diff --git a/services/githubbot/test/review-budget.test.ts b/services/githubbot/test/review-budget.test.ts index d75cd9ccb4..f9d02570de 100644 --- a/services/githubbot/test/review-budget.test.ts +++ b/services/githubbot/test/review-budget.test.ts @@ -151,6 +151,22 @@ describe("assessReviewChange", () => { ).toMatchObject({ changeClass: "new_risk", kind: "material" }); }); + test("treats a patchless zero-line runtime file as a semantic change", () => { + expect( + assessReviewChange({ + comparisonStatus: "ahead", + files: [ + { + additions: 0, + deletions: 0, + filename: "src/model.bin", + status: "modified", + }, + ], + }), + ).toMatchObject({ changeClass: "new_risk", kind: "material" }); + }); + test("keeps a tree-identical rebase in the current epoch", () => { expect( assessReviewChange({ From 2f8cbf477ceaa8abeb09ff45aba7f087392cec25 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 10:36:27 +0900 Subject: [PATCH 07/14] Classify known review heads without spending rounds --- services/githubbot/src/pr-manager.ts | 54 +++++++++++++--------- services/githubbot/test/pr-manager.test.ts | 6 ++- 2 files changed, 36 insertions(+), 24 deletions(-) diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index e1c9e4b064..be88207991 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -1683,26 +1683,6 @@ async function admitReviewResponse( state, }; } - if (findings.length > 0 && mergedFindings.newFindings.length === 0) { - traceLog( - ctx.options, - "githubbot_review_findings_already_known", - makeTrace( - managementThreadKey(owner, repo, pr.number), - `review-findings-${headSha}`, - ), - { finding_count: findings.length, head_sha: headSha }, - ); - const state = { - ...(loaded.state as ReviewEpochState), - lastReviewedHeadSha: headSha, - }; - await retryingReviewBudgetSave(ctx, owner, repo, pr.number, state); - return { - decision: "skip", - state, - }; - } const approval = await pendingReviewResetApproval( ctx, owner, @@ -1731,6 +1711,20 @@ async function admitReviewResponse( } } + const findingsAlreadyKnown = + findings.length > 0 && mergedFindings.newFindings.length === 0; + if (findingsAlreadyKnown) { + traceLog( + ctx.options, + "githubbot_review_findings_already_known", + makeTrace( + managementThreadKey(owner, repo, pr.number), + `review-findings-${headSha}`, + ), + { finding_count: findings.length, head_sha: headSha }, + ); + } + const admission = decideReviewAdmission({ actor: evidence?.actor ?? "unknown", assessment: evidence?.assessment, @@ -1754,14 +1748,27 @@ async function admitReviewResponse( finding.fingerprint, ), )?.fingerprint, - startsRepairTurn: true, + startsRepairTurn: !findingsAlreadyKnown, state: loaded.state, }); - const state = { + let state = { ...admission.state, findingLedger: mergedFindings.ledger, ...(approval ? { consumedResetApprovalId: approval.approvalId } : {}), }; + // A repeated decided finding still has to classify the head transition and + // consume an authorized reset, but it must not spend another reviewer or + // aggregate repair round. A genuinely new human-risk epoch begins at zero; + // its first new-finding review will consume round one. + if (findingsAlreadyKnown && admission.decision === "allow" && loaded.state) { + state = { + ...state, + reviewerRoundsUsed: admission.resetEpoch + ? {} + : loaded.state.reviewerRoundsUsed, + roundsUsed: admission.resetEpoch ? 0 : loaded.state.roundsUsed, + }; + } await retryingReviewBudgetSave(ctx, owner, repo, pr.number, state); if (admission.decision === "allow" && manualReset) { await cleanupReviewResetApproval(ctx, owner, repo, pr, true); @@ -1789,6 +1796,9 @@ async function admitReviewResponse( rounds_used: state.roundsUsed, }, ); + if (findingsAlreadyKnown && admission.decision === "allow") { + return { decision: "skip", state }; + } return { ...admission, newFindings: mergedFindings.newFindings, state }; } diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index f98818b561..4a6ad3e2b1 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -729,6 +729,7 @@ describe("bounded review epochs", () => { path: "src/policy.ts", }; const ctx = budgetCtx({ + actor: "human", reviewFindings: { 40: [{ ...sharedFinding, id: 400 }], 41: [{ ...sharedFinding, id: 410, line: 25 }], @@ -783,8 +784,9 @@ describe("bounded review epochs", () => { dispositionCommentId: 401, }, }, - reviewerRoundsUsed: { "github-user:101": 1 }, - roundsUsed: 1, + epoch: 2, + reviewerRoundsUsed: {}, + roundsUsed: 0, lastReviewedHeadSha: "head-2", }); }); From 62c7f91c5d60a0f45d486d9b33d21a0d5c46e493 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 10:47:18 +0900 Subject: [PATCH 08/14] Accept durable zero-round review epochs --- services/githubbot/src/pr-manager.ts | 2 +- services/githubbot/test/pr-manager.test.ts | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index be88207991..d8590c27d8 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -272,7 +272,7 @@ function isReviewEpochState(value: unknown): value is ReviewEpochState { candidate.epoch > 0 && typeof candidate.roundsUsed === "number" && Number.isInteger(candidate.roundsUsed) && - candidate.roundsUsed > 0 && + candidate.roundsUsed >= 0 && (candidate.automationPendingFromHeadSha === undefined || typeof candidate.automationPendingFromHeadSha === "string") && (candidate.consumedResetApprovalId === undefined || diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index 4a6ad3e2b1..969168f5f9 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -733,6 +733,15 @@ describe("bounded review epochs", () => { reviewFindings: { 40: [{ ...sharedFinding, id: 400 }], 41: [{ ...sharedFinding, id: 410, line: 25 }], + 42: [ + { + body: "A new finding after the zero-round epoch was restored.", + diff_hunk: "+newRisk();", + id: 420, + line: 30, + path: "src/new-risk.ts", + }, + ], }, state, }); @@ -789,6 +798,12 @@ describe("bounded review epochs", () => { roundsUsed: 0, lastReviewedHeadSha: "head-2", }); + + await handleReviewEvent(ctx, submittedReview(42, "head-2")); + await drainBackgroundWork(5_000); + expect( + await state.get("centaur-githubbot:review-budget:base/repo#7"), + ).toMatchObject({ epoch: 2, roundsUsed: 1 }); }); test("requires rejection evidence for each exact finding fingerprint", async () => { From 87f44fbb6020d6f5937cb3531b5eec1bca367e9e Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 11:03:29 +0900 Subject: [PATCH 09/14] Close review budget bypasses --- services/githubbot/src/pr-manager.ts | 78 +++++++++++- services/githubbot/src/review-budget.ts | 17 +++ services/githubbot/src/review-findings.ts | 26 +++- services/githubbot/test/pr-manager.test.ts | 119 +++++++++++++++++- services/githubbot/test/review-budget.test.ts | 22 ++++ .../githubbot/test/review-findings.test.ts | 26 +++- 6 files changed, 273 insertions(+), 15 deletions(-) diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index d8590c27d8..f5011d8973 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -853,6 +853,7 @@ export async function handleReviewEvent( reviewNode, reviewerKey, effectiveHeadSha, + reviewState, ); } catch (error) { logger(ctx).warn("githubbot_review_findings_load_failed", { @@ -1116,10 +1117,16 @@ async function hasAcceptedFindingRepairEvidence( ("previous_filename" in file && file.previous_filename === findingPath), ); - return ( - changedFindingPath && - findingFingerprintsFromCommits(commits).has(fingerprint) - ); + if (!changedFindingPath) return false; + for (const commit of commits) { + if (!commitCarriesFindingFingerprint(commit, fingerprint)) continue; + if ( + await commitChangesFindingPath(ctx, owner, repo, commit, findingPath) + ) { + return true; + } + } + return false; } catch (error) { logger(ctx).warn("githubbot_review_disposition_evidence_failed", { error: errorMessage(error), @@ -1130,6 +1137,45 @@ async function hasAcceptedFindingRepairEvidence( } } +async function commitChangesFindingPath( + ctx: PrManagerContext, + owner: string, + repo: string, + commit: unknown, + findingPath: string, +): Promise { + if (!isRecord(commit)) return false; + const ref = stringValue(commit.sha); + if (!ref) return false; + const changedFiles: JsonRecord[] = []; + for (let page = 1; page <= 3; page += 1) { + const response = await ctx.octokit.rest.repos.getCommit({ + owner, + page, + per_page: 100, + ref, + repo, + }); + const files = Array.isArray(response.data.files) + ? response.data.files.filter(isRecord) + : []; + changedFiles.push(...files); + if (files.length < 100) break; + if (page === 3) return false; + } + return changedFiles.some( + (file) => + file.filename === findingPath || file.previous_filename === findingPath, + ); +} + +function commitCarriesFindingFingerprint( + commit: unknown, + fingerprint: string, +): boolean { + return findingFingerprintsFromCommits([commit]).has(fingerprint); +} + async function maybeRecordReviewResetApproval( ctx: PrManagerContext, owner: string, @@ -1416,6 +1462,7 @@ async function collectReviewFindings( reviewNode: JsonRecord, reviewerKey: string, reviewedHeadSha: string, + reviewState: string | undefined, ): Promise { const findings: ReviewFinding[] = []; for (let page = 1; page <= MAX_REVIEW_COMMENT_PAGES; page += 1) { @@ -1455,7 +1502,7 @@ async function collectReviewFindings( // GitHub reviews may carry one body-only finding and no inline comments. // Do not fingerprint a summary body in addition to its inline findings: that // would create a fresh pseudo-finding whenever a reviewer rewrites a summary. - if (findings.length === 0) { + if (findings.length === 0 || reviewState === "changes_requested") { const body = stringValue(reviewNode.body)?.trim(); if (body) { findings.push( @@ -1799,7 +1846,26 @@ async function admitReviewResponse( if (findingsAlreadyKnown && admission.decision === "allow") { return { decision: "skip", state }; } - return { ...admission, newFindings: mergedFindings.newFindings, state }; + const admittedFindings = findingsForReviewTurn( + mergedFindings.newFindings, + loaded.state?.securityInterruptFingerprints, + admission.state.securityInterruptFingerprints, + ); + return { ...admission, newFindings: admittedFindings, state }; +} + +export function findingsForReviewTurn( + findings: readonly ReviewFinding[], + priorInterrupts: readonly string[] | undefined, + currentInterrupts: readonly string[] | undefined, +): ReviewFinding[] { + const prior = new Set(priorInterrupts ?? []); + const consumed = currentInterrupts?.find( + (fingerprint) => !prior.has(fingerprint), + ); + return consumed + ? findings.filter((finding) => finding.fingerprint === consumed) + : [...findings]; } async function escalateReviewBudget( diff --git a/services/githubbot/src/review-budget.ts b/services/githubbot/src/review-budget.ts index 2c051d3f62..0c21293086 100644 --- a/services/githubbot/src/review-budget.ts +++ b/services/githubbot/src/review-budget.ts @@ -468,6 +468,23 @@ function exhaustedAdmission( export function decideReviewAdmission( input: ReviewAdmissionInput, ): ReviewAdmission { + if ( + input.assessment?.changeClass === "repair" && + input.actor !== "automation" + ) { + input = { + ...input, + assessment: { + ...input.assessment, + changeClass: "new_risk", + kind: "material", + reasons: [ + ...input.assessment.reasons, + `repair_actor_not_automation:${input.actor}`, + ], + }, + }; + } const existing = input.state; if (!existing) { const state = firstEpoch(input.headSha, input.reviewerKey); diff --git a/services/githubbot/src/review-findings.ts b/services/githubbot/src/review-findings.ts index 708e4b92b0..f220d1d005 100644 --- a/services/githubbot/src/review-findings.ts +++ b/services/githubbot/src/review-findings.ts @@ -48,9 +48,10 @@ const EXPLICIT_EVIDENCE = /^\s*evidence\s*:\s*\S.+$/im; /** * Build a reviewer-independent semantic fingerprint. Exact reviewer identity, - * line number, and mutable diff context are intentionally excluded so a bot - * cannot reopen the same normalized finding merely by changing accounts or - * pointing at a nearby line after a repair. + * line number and hunk coordinates are intentionally excluded so a bot cannot + * reopen the same normalized finding merely by changing accounts or pointing + * at a moved line. The coordinate-free hunk body remains part of the identity + * so identical prose at two distinct code sites cannot collapse into one row. */ export function fingerprintReviewFinding(input: { body: string; @@ -60,6 +61,7 @@ export function fingerprintReviewFinding(input: { }): string { const canonical = JSON.stringify({ body: normalizeFindingText(input.body), + context: normalizeDiffContext(input.diffHunk), path: normalizePath(input.path), }); return `sha256:${createHash("sha256").update(canonical).digest("hex")}`; @@ -151,7 +153,7 @@ export function mergeReviewFindings( reviewId: finding.reviewId, reviewerKey: finding.reviewerKey, reviewedHeadSha: finding.reviewedHeadSha, - severity: finding.severity, + severity: highestSeverity(existing.severity, finding.severity), }; continue; } @@ -213,6 +215,22 @@ export function mergeReviewFindings( }; } +function normalizeDiffContext(value: string | undefined): string { + return (value ?? "") + .split(/\r?\n/) + .filter((line) => !/^@@(?:\s|$)/.test(line)) + .join("\n") + .trim(); +} + +function highestSeverity( + left: ReviewFinding["severity"], + right: ReviewFinding["severity"], +): ReviewFinding["severity"] { + const rank = { normal: 0, security: 1, p0: 2 } as const; + return rank[left] >= rank[right] ? left : right; +} + export function parseReviewFindingDispositionMarkers( body: string, ): ReviewFindingDispositionMarker[] { diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index 969168f5f9..7302ea4a1b 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { drainBackgroundWork } from "../src/context"; import { decideMerge, + findingsForReviewTurn, handleCiEvent, handlePullRequestEvent, handleReviewFindingDispositionComment, @@ -10,7 +11,10 @@ import { isOwnedPr, type PrManagerContext, } from "../src/pr-manager"; -import { fingerprintReviewFinding } from "../src/review-findings"; +import { + fingerprintReviewFinding, + makeReviewFinding, +} from "../src/review-findings"; import { emitWorkflowEvent } from "../src/session-api"; import { evaluateCi, @@ -517,6 +521,8 @@ describe("bounded review epochs", () => { login: "reviewer", }, authorAssociation = "COLLABORATOR", + body?: string, + state = "commented", ) => JSON.stringify({ action: "submitted", @@ -526,7 +532,8 @@ describe("bounded review epochs", () => { author_association: authorAssociation, commit_id: headSha, id: reviewId, - state: "commented", + body, + state, user: reviewer, }, }); @@ -555,6 +562,7 @@ describe("bounded review epochs", () => { }> >; state?: ReturnType; + commitFilesByRef?: Record; }): PrManagerContext { let headSha = input?.headSha ?? "head-1"; const actor = input?.actor ?? "bot"; @@ -605,6 +613,7 @@ describe("bounded review epochs", () => { data: { commits: [ { + sha: "comparison-sha", author: actor === "bot" ? { login: "centaur-bot", type: "Bot" } @@ -634,6 +643,15 @@ describe("bounded review epochs", () => { }, }; }, + getCommit: async (request: { ref: string }) => ({ + data: { + files: ( + input?.commitFilesByRef?.[request.ref] ?? [ + input?.comparisonFile ?? "src/implementation.ts", + ] + ).map((filename) => ({ filename })), + }, + }), getCollaboratorPermissionLevel: async () => ({ data: { permission: input?.permission ?? "write" }, }), @@ -945,6 +963,47 @@ describe("bounded review epochs", () => { }); }); + test("keeps acceptance pending when the trailer commit did not change the finding path", async () => { + const state = makeState(); + let fingerprint = ""; + const ctx = budgetCtx({ + commitFilesByRef: { "comparison-sha": ["src/unrelated.ts"] }, + comparisonCommitMessage: () => + `fix review\n\nCentaur-Automation: true\nCentaur-Review-Finding: ${fingerprint}`, + comparisonFile: "src/implementation.ts", + state, + }); + await handleReviewEvent(ctx, submittedReview(47, "head-1")); + const initial = (await state.get( + "centaur-githubbot:review-budget:base/repo#7", + )) as { findingLedger: Record }; + fingerprint = Object.keys(initial.findingLedger)[0] ?? ""; + if (!fingerprint) throw new Error("missing finding fingerprint"); + setHeadSha(ctx, "head-2"); + + await handleReviewFindingDispositionComment( + ctx, + JSON.stringify({ + action: "created", + comment: { + body: ``, + id: 471, + in_reply_to_id: 470, + user: { login: "centaur-bot" }, + }, + pull_request: { number: 7 }, + repository: { full_name: "base/repo" }, + }), + ); + await drainBackgroundWork(5_000); + + expect( + await state.get("centaur-githubbot:review-budget:base/repo#7"), + ).toMatchObject({ + findingLedger: { [fingerprint]: { disposition: "pending" } }, + }); + }); + test("keeps a body-only finding pending without exact repaired-path evidence", async () => { const state = makeState(); let fingerprint = ""; @@ -1033,10 +1092,12 @@ describe("bounded review epochs", () => { const newPath = "src/new-policy.ts"; const oldFingerprint = fingerprintReviewFinding({ body: oldBody, + diffHunk: "+old();", path: oldPath, }); const newFingerprint = fingerprintReviewFinding({ body: newBody, + diffHunk: "+new();", path: newPath, }); const state = makeState(); @@ -1085,6 +1146,60 @@ describe("bounded review epochs", () => { }); }); + test("passes only the qualifying finding through a security interrupt", () => { + const securityBody = + "Centaur-Severity: security\nImpact: repository scope can widen\nEvidence: exact unchecked call is shown"; + const ordinary = makeReviewFinding({ + body: "An ordinary optional cleanup.", + commentId: 480, + diffHunk: "+cleanup();", + line: 10, + path: "src/cleanup.ts", + reviewId: 48, + reviewerKey: "github-user:101", + reviewedHeadSha: "head-1", + }); + const security = makeReviewFinding({ + body: securityBody, + commentId: 481, + diffHunk: "+unchecked();", + line: 20, + path: "src/auth.ts", + reviewId: 48, + reviewerKey: "github-user:101", + reviewedHeadSha: "head-1", + }); + expect( + findingsForReviewTurn( + [ordinary, security], + ["sha256:old"], + ["sha256:old", security.fingerprint], + ), + ).toEqual([security]); + }); + + test("retains a changes-requested review body alongside inline findings", async () => { + const state = makeState(); + const ctx = budgetCtx({ state }); + await handleReviewEvent( + ctx, + submittedReview( + 49, + "head-1", + undefined, + "COLLABORATOR", + "The review body identifies a separate deployment hazard.", + "changes_requested", + ), + ); + await drainBackgroundWork(5_000); + + const budget = (await state.get( + "centaur-githubbot:review-budget:base/repo#7", + )) as { findingLedger: Record }; + expect(Object.keys(budget.findingLedger)).toHaveLength(2); + }); + test("stores an active handoff pause without expiration", async () => { const durableState = makeState(); await durableState.set("centaur-githubbot:review-budget:base/repo#7", { diff --git a/services/githubbot/test/review-budget.test.ts b/services/githubbot/test/review-budget.test.ts index f9d02570de..ec4ae145a4 100644 --- a/services/githubbot/test/review-budget.test.ts +++ b/services/githubbot/test/review-budget.test.ts @@ -277,6 +277,28 @@ describe("decideReviewAdmission", () => { startsRepairTurn: true, }; + test("treats a mixed-author bounded repair as new risk", () => { + expect( + decideReviewAdmission({ + ...base, + actor: "unknown", + assessment: { + changeClass: "repair", + changedFiles: 1, + changedLines: 4, + kind: "minor", + reasons: ["accepted_finding_repair"], + runtimeFiles: 1, + }, + state: epoch(), + }), + ).toMatchObject({ + decision: "pause", + reason: "change_actor_unknown", + assessment: { changeClass: "new_risk", kind: "material" }, + }); + }); + test("starts the first epoch and counts its broad review", () => { expect(decideReviewAdmission({ ...base, state: undefined })).toEqual({ decision: "allow", diff --git a/services/githubbot/test/review-findings.test.ts b/services/githubbot/test/review-findings.test.ts index 423cfb455e..59b1753207 100644 --- a/services/githubbot/test/review-findings.test.ts +++ b/services/githubbot/test/review-findings.test.ts @@ -24,16 +24,16 @@ function finding(overrides: Partial[0]> = { } describe("review finding fingerprints", () => { - test("are stable across reviewers, moved lines, mutable hunks, and URLs", () => { + test("are stable across reviewers, moved hunk coordinates, and URLs", () => { const first = fingerprintReviewFinding({ body: "Check https://example.test/one before use", - diffHunk: "+first implementation", + diffHunk: "@@ -1 +1 @@\n-old\n+same implementation", line: 10, path: "./src/policy.ts", }); const second = fingerprintReviewFinding({ body: "check https://elsewhere.test/two before use", - diffHunk: "+replacement implementation", + diffHunk: "@@ -90 +99 @@\n-old\n+same implementation", line: 99, path: "src/policy.ts", }); @@ -44,6 +44,13 @@ describe("review finding fingerprints", () => { path: "src/other.ts", }), ).not.toBe(first); + expect( + fingerprintReviewFinding({ + body: "check https://elsewhere.test/two before use", + diffHunk: "@@ -90 +99 @@\n-old\n+different implementation", + path: "src/policy.ts", + }), + ).not.toBe(first); }); test("requires structured impact and inline evidence for a budget interrupt", () => { @@ -113,6 +120,19 @@ describe("review finding ledger", () => { ); }); + test("preserves the highest severity when a pending finding is rediscovered", () => { + const severe = finding({ + body: + "Centaur-Severity: security\nImpact: repository scope can widen\nEvidence: exact unchecked call is shown", + }); + const initial = mergeReviewFindings(undefined, [severe], 1); + expect(initial.ledger[severe.fingerprint]?.severity).toBe("security"); + + const downgraded = { ...severe, severity: "normal" as const, reviewId: 32 }; + const merged = mergeReviewFindings(initial.ledger, [downgraded], 1); + expect(merged.ledger[severe.fingerprint]?.severity).toBe("security"); + }); + test("rejects a disposition detached from the original review thread", () => { const first = finding(); const ledger = mergeReviewFindings(undefined, [first], 1).ledger; From e29ef2362e9501cece6808af9b869efddc66d245 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 11:15:29 +0900 Subject: [PATCH 10/14] Classify review heads and runtime paths safely --- services/githubbot/src/pr-manager.ts | 54 +++++++++++++++++-- services/githubbot/src/review-budget.ts | 2 +- services/githubbot/test/pr-manager.test.ts | 29 +++++----- services/githubbot/test/review-budget.test.ts | 30 +++++++++++ 4 files changed, 92 insertions(+), 23 deletions(-) diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index f5011d8973..4b8151c252 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -1627,10 +1627,47 @@ async function recordApprovedReview( ); if (!approval) { if (loaded.state && loaded.state.lastReviewedHeadSha !== headSha) { - const state = { - ...loaded.state, - automationPendingFromHeadSha: undefined, - lastReviewedHeadSha: headSha, + const evidence = await compareReviewChange( + ctx, + owner, + repo, + loaded.state.lastReviewedHeadSha, + headSha, + loaded.state.findingLedger, + ); + const admission = decideReviewAdmission({ + actor: evidence.actor, + assessment: evidence.assessment, + headSha, + manualReset: false, + maxEpochs: ctx.options.reviewMaxEpochs ?? DEFAULT_REVIEW_MAX_EPOCHS, + maxRoundsPerEpoch: + ctx.options.reviewMaxRoundsPerEpoch ?? + DEFAULT_REVIEW_MAX_ROUNDS_PER_EPOCH, + maxTotalRoundsPerEpoch: + ctx.options.reviewMaxTotalRoundsPerEpoch ?? + DEFAULT_REVIEW_MAX_TOTAL_ROUNDS_PER_EPOCH, + reviewerKey, + startsRepairTurn: false, + state: loaded.state, + }); + const state: ReviewEpochState = { + ...admission.state, + automationPendingFromHeadSha: + admission.decision === "allow" + ? undefined + : loaded.state.automationPendingFromHeadSha, + // An approval classifies the new head but does not start a repair turn. + // Preserve the current counters, or begin a genuine human-risk epoch at + // zero so its first finding receives round one. + reviewerRoundsUsed: + admission.decision === "allow" && admission.resetEpoch + ? {} + : loaded.state.reviewerRoundsUsed, + roundsUsed: + admission.decision === "allow" && admission.resetEpoch + ? 0 + : loaded.state.roundsUsed, }; await retryingReviewBudgetSave(ctx, owner, repo, pr.number, state); traceLog( @@ -1640,8 +1677,15 @@ async function recordApprovedReview( managementThreadKey(owner, repo, pr.number), `review-approved-${headSha}`, ), - { epoch: state.epoch, head_sha: headSha }, + { + assessment: admission.assessment?.kind, + change_class: admission.assessment?.changeClass, + decision: admission.decision, + epoch: state.epoch, + head_sha: headSha, + }, ); + return admission.decision === "allow"; } return true; } diff --git a/services/githubbot/src/review-budget.ts b/services/githubbot/src/review-budget.ts index 0c21293086..97717a9a9b 100644 --- a/services/githubbot/src/review-budget.ts +++ b/services/githubbot/src/review-budget.ts @@ -88,7 +88,7 @@ const MIGRATION_PATH = /(^|\/)(?:migrations?|schema)(?:[._\/-]|$)/i; const AUTH_DATA_API_PATH = /(^|\/)(?:api|auth(?:entication|orization)?|data|permissions?|polic(?:y|ies)|security)(?:[._\/-]|$)/i; const DEPLOYMENT_PATH = /(^|\/)(?:\.github\/workflows|charts?|contrib\/chart|deploy|helm|k8s|kubernetes)(?:\/|$)/i; const API_CONTRACT_FILE = /(^|\/)(?:openapi|asyncapi|[^/]+\.proto)(?:[._\/-]|$)/i; -const NON_RUNTIME_PATH = /(^|\/)(?:docs?|examples?|fixtures?|generated|snapshots?|tests?|testdata|vendor)(?:\/|$)|(?:\.md|\.mdx|\.rst|\.snap)$|(?:^|\.)test\.[^/]+$|(?:^|\.)spec\.[^/]+$/i; +const NON_RUNTIME_PATH = /(^|\/)(?:docs?|examples?|fixtures?|generated|snapshots?|tests?|testdata)(?:\/|$)|(?:\.md|\.mdx|\.rst|\.snap)$|(?:^|\.)test\.[^/]+$|(?:^|\.)spec\.[^/]+$|(?:^|\/)(?:test_[^/]+|[^/]+_(?:test|spec))\.(?:go|py|rb)$/i; function nonNegative(value: number | undefined): number { return typeof value === "number" && Number.isFinite(value) && value > 0 diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index 7302ea4a1b..772644c1f5 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -1462,7 +1462,7 @@ describe("bounded review epochs", () => { ).toMatchObject({ anchorHeadSha: "head-3", epoch: 2, roundsUsed: 1 }); }); - test("records an approved repair head before later authorship checks", async () => { + test("classifies an approved material human head before advancing the boundary", async () => { const state = makeState(); await state.set("centaur-githubbot:review-budget:base/repo#7", { anchorHeadSha: "head-1", @@ -1479,24 +1479,13 @@ describe("bounded review epochs", () => { basehead: string; }) => { compared.push(request.basehead); - const latestRange = request.basehead === "head-2...head-3"; const humanCommit = { author: { login: "alice", type: "User" }, commit: { message: "material human revision" }, }; return { data: { - commits: latestRange - ? [humanCommit] - : [ - { - author: { login: "centaur-bot", type: "Bot" }, - commit: { - message: "review fix\n\nCentaur-Automation: true", - }, - }, - humanCommit, - ], + commits: [humanCommit], files: [ { additions: 5, @@ -1507,7 +1496,7 @@ describe("bounded review epochs", () => { }, ], status: "ahead", - total_commits: latestRange ? 1 : 2, + total_commits: 1, }, }; }) as unknown as typeof ctx.octokit.rest.repos.compareCommitsWithBasehead; @@ -1529,16 +1518,22 @@ describe("bounded review epochs", () => { ); expect( await state.get("centaur-githubbot:review-budget:base/repo#7"), - ).toMatchObject({ lastReviewedHeadSha: "head-2" }); + ).toMatchObject({ + anchorHeadSha: "head-2", + epoch: 2, + lastReviewedHeadSha: "head-2", + reviewerRoundsUsed: {}, + roundsUsed: 0, + }); setHeadSha(ctx, "head-3"); await handleReviewEvent(ctx, submittedReview(34, "head-3")); await drainBackgroundWork(5_000); - expect(compared).toEqual(["head-2...head-3"]); + expect(compared).toEqual(["head-1...head-2", "head-2...head-3"]); expect( await state.get("centaur-githubbot:review-budget:base/repo#7"), - ).toMatchObject({ anchorHeadSha: "head-3", epoch: 2, roundsUsed: 1 }); + ).toMatchObject({ anchorHeadSha: "head-3", epoch: 3, roundsUsed: 1 }); }); test("serializes merge evaluation behind an in-flight review admission", async () => { diff --git a/services/githubbot/test/review-budget.test.ts b/services/githubbot/test/review-budget.test.ts index ec4ae145a4..d2b5cb83d5 100644 --- a/services/githubbot/test/review-budget.test.ts +++ b/services/githubbot/test/review-budget.test.ts @@ -58,6 +58,36 @@ describe("assessReviewChange", () => { }); }); + test("recognizes conventional Go, Python, and Ruby test filenames", () => { + for (const filename of [ + "pkg/client/client_test.go", + "services/sandbox/test_system_prompt.py", + "spec/models/user_spec.rb", + ]) { + expect( + assessReviewChange({ + comparisonStatus: "ahead", + files: [{ changes: 5, filename }], + }), + ).toMatchObject({ changeClass: "maintenance", runtimeFiles: 0 }); + } + }); + + test("treats vendored source as runtime behavior", () => { + expect( + assessReviewChange({ + comparisonStatus: "ahead", + files: [ + { + changes: 2, + filename: "vendor/example.com/dependency/client.go", + patch: "-return insecure()\n+return checked()", + }, + ], + }), + ).toMatchObject({ changeClass: "new_risk", kind: "material", runtimeFiles: 1 }); + }); + test("treats authorization, migration, dependency, and deployment files as material", () => { for (const filename of [ "services/api/src/authorization.ts", From 3ea06dba4c4ea5063877c313754a518ddec19782 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 11:33:41 +0900 Subject: [PATCH 11/14] Keep review dispositions durable across retries --- services/githubbot/src/pr-manager.ts | 98 +++++++++++++------ services/githubbot/src/review-findings.ts | 48 ++++++++- services/githubbot/test/pr-manager.test.ts | 14 +++ .../githubbot/test/review-findings.test.ts | 31 ++++++ 4 files changed, 157 insertions(+), 34 deletions(-) diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index 4b8151c252..2ac9560ac7 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -75,6 +75,7 @@ const STATE_TTL_MS = 90 * 24 * 60 * 60 * 1000; const CLAIM_TTL_MS = 7 * 24 * 60 * 60 * 1000; const DEFAULT_CI_FIX_MAX_ATTEMPTS = 3; export const DEFAULT_OWNERSHIP_LABEL = "centaur-managed"; +const REVIEW_GITHUB_EVIDENCE_RETRY_DELAYS_MS = [0, 100]; const REVIEW_STATE_RETRY_DELAYS_MS = [0, 100, 500, 1_000, 5_000, 10_000, 30_000]; // --------------------------------------------------------------------------- @@ -418,6 +419,36 @@ function isTransientGithubError(error: unknown): boolean { ); } +async function retryingGithubEvidenceOperation( + ctx: PrManagerContext, + pr: string, + fingerprint: string, + operation: () => Promise, +): Promise { + let failureCount = 0; + for (;;) { + try { + return await operation(); + } catch (error) { + const delayMs = REVIEW_GITHUB_EVIDENCE_RETRY_DELAYS_MS[failureCount]; + if (!isTransientGithubError(error) || delayMs === undefined) throw error; + failureCount += 1; + logger(ctx).warn("githubbot_review_disposition_evidence_retry", { + attempt: failureCount, + error: errorMessage(error), + fingerprint, + pr, + retry_in_ms: delayMs, + }); + if (delayMs > 0) { + await new Promise((resolve) => setTimeout(resolve, delayMs)); + } else { + await Promise.resolve(); + } + } + } +} + async function retryingReviewBudgetLoad( ctx: PrManagerContext, owner: string, @@ -1096,37 +1127,44 @@ async function hasAcceptedFindingRepairEvidence( return false; } try { - const { data } = await ctx.octokit.rest.repos.compareCommitsWithBasehead({ - basehead: `${reviewedHeadSha}...${currentHeadSha}`, - owner, - per_page: 100, - repo, - }); - if (stringValue(data.status)?.toLowerCase() !== "ahead") return false; - const commits = Array.isArray(data.commits) ? data.commits : []; - const totalCommits = - typeof data.total_commits === "number" ? data.total_commits : undefined; - if (totalCommits === undefined || totalCommits !== commits.length) return false; - const files = Array.isArray(data.files) ? data.files : undefined; - // The compare API caps this array at 300 files. Exactly 300 is therefore - // ambiguous and cannot prove an exact path was included. - if (!files || files.length === 0 || files.length >= 300) return false; - const changedFindingPath = files.some( - (file) => - file.filename === findingPath || - ("previous_filename" in file && - file.previous_filename === findingPath), - ); - if (!changedFindingPath) return false; - for (const commit of commits) { - if (!commitCarriesFindingFingerprint(commit, fingerprint)) continue; - if ( - await commitChangesFindingPath(ctx, owner, repo, commit, findingPath) - ) { - return true; + return await retryingGithubEvidenceOperation( + ctx, + `${owner}/${repo}#${reviewedHeadSha}...${currentHeadSha}`, + fingerprint, + async () => { + const { data } = await ctx.octokit.rest.repos.compareCommitsWithBasehead({ + basehead: `${reviewedHeadSha}...${currentHeadSha}`, + owner, + per_page: 100, + repo, + }); + if (stringValue(data.status)?.toLowerCase() !== "ahead") return false; + const commits = Array.isArray(data.commits) ? data.commits : []; + const totalCommits = + typeof data.total_commits === "number" ? data.total_commits : undefined; + if (totalCommits === undefined || totalCommits !== commits.length) return false; + const files = Array.isArray(data.files) ? data.files : undefined; + // The compare API caps this array at 300 files. Exactly 300 is therefore + // ambiguous and cannot prove an exact path was included. + if (!files || files.length === 0 || files.length >= 300) return false; + const changedFindingPath = files.some( + (file) => + file.filename === findingPath || + ("previous_filename" in file && + file.previous_filename === findingPath), + ); + if (!changedFindingPath) return false; + for (const commit of commits) { + if (!commitCarriesFindingFingerprint(commit, fingerprint)) continue; + if ( + await commitChangesFindingPath(ctx, owner, repo, commit, findingPath) + ) { + return true; + } + } + return false; } - } - return false; + ); } catch (error) { logger(ctx).warn("githubbot_review_disposition_evidence_failed", { error: errorMessage(error), diff --git a/services/githubbot/src/review-findings.ts b/services/githubbot/src/review-findings.ts index f220d1d005..1f0ae24143 100644 --- a/services/githubbot/src/review-findings.ts +++ b/services/githubbot/src/review-findings.ts @@ -14,6 +14,7 @@ export type ReviewFinding = { reviewId: number; reviewerKey: string; reviewedHeadSha: string; + semanticFingerprint: string; severity: "normal" | "p0" | "security"; url?: string; }; @@ -27,6 +28,7 @@ export type ReviewFindingRecord = { reviewId: number; reviewerKey: string; reviewedHeadSha: string; + semanticFingerprint?: string; severity: ReviewFinding["severity"]; }; @@ -67,6 +69,17 @@ export function fingerprintReviewFinding(input: { return `sha256:${createHash("sha256").update(canonical).digest("hex")}`; } +function semanticFingerprintReviewFinding(input: { + body: string; + path?: string; +}): string { + const canonical = JSON.stringify({ + body: normalizeFindingText(input.body), + path: normalizePath(input.path), + }); + return `sha256:${createHash("sha256").update(canonical).digest("hex")}`; +} + export function makeReviewFinding(input: { body: string; commentId?: number; @@ -92,6 +105,7 @@ export function makeReviewFinding(input: { reviewId: input.reviewId, reviewerKey: input.reviewerKey, reviewedHeadSha: input.reviewedHeadSha, + semanticFingerprint: semanticFingerprintReviewFinding({ body, path }), severity: findingSeverity({ body, diffHunk, line, path }), url: input.url?.trim().slice(0, 2_000) || undefined, }; @@ -134,8 +148,30 @@ export function mergeReviewFindings( } { const next: ReviewFindingLedger = { ...(ledger ?? {}) }; const actionableFindings: ReviewFinding[] = []; + const semanticCounts = new Map(); + for (const finding of findings) { + semanticCounts.set( + finding.semanticFingerprint, + (semanticCounts.get(finding.semanticFingerprint) ?? 0) + 1, + ); + } for (const finding of findings) { - const existing = next[finding.fingerprint]; + let fingerprint = finding.fingerprint; + if ( + !next[fingerprint] && + semanticCounts.get(finding.semanticFingerprint) === 1 + ) { + const matching = Object.entries(next).filter( + ([, record]) => + record.semanticFingerprint === finding.semanticFingerprint, + ); + if (matching.length === 1) fingerprint = matching[0]![0]; + } + const canonicalFinding = + fingerprint === finding.fingerprint + ? finding + : { ...finding, fingerprint }; + const existing = next[fingerprint]; if ( existing?.disposition === "accepted" || existing?.disposition === "rejected" @@ -144,20 +180,21 @@ export function mergeReviewFindings( } // A repeated pending finding remains actionable. Only an evidence-backed // accepted/rejected decision suppresses rediscovery. - actionableFindings.push(finding); + actionableFindings.push(canonicalFinding); if (existing) { - next[finding.fingerprint] = { + next[fingerprint] = { ...existing, commentId: finding.commentId, path: finding.path, reviewId: finding.reviewId, reviewerKey: finding.reviewerKey, reviewedHeadSha: finding.reviewedHeadSha, + semanticFingerprint: finding.semanticFingerprint, severity: highestSeverity(existing.severity, finding.severity), }; continue; } - next[finding.fingerprint] = { + next[fingerprint] = { commentId: finding.commentId, disposition: "pending", firstSeenEpoch: epoch, @@ -165,6 +202,7 @@ export function mergeReviewFindings( reviewId: finding.reviewId, reviewerKey: finding.reviewerKey, reviewedHeadSha: finding.reviewedHeadSha, + semanticFingerprint: finding.semanticFingerprint, severity: finding.severity, }; } @@ -340,6 +378,8 @@ export function isReviewFindingLedger( typeof finding.reviewedHeadSha === "string" && finding.reviewedHeadSha.length > 0 && finding.reviewedHeadSha.length <= 100 && + (finding.semanticFingerprint === undefined || + /^sha256:[0-9a-f]{64}$/.test(finding.semanticFingerprint)) && ["normal", "p0", "security"].includes(finding.severity ?? "") && (finding.commentId === undefined || (Number.isInteger(finding.commentId) && finding.commentId > 0)) && diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index 772644c1f5..e9fa9b1529 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -921,12 +921,25 @@ describe("bounded review epochs", () => { test("accepts a finding only after an exact-path repair with its trailer", async () => { const state = makeState(); let fingerprint = ""; + let comparisonAttempts = 0; const ctx = budgetCtx({ comparisonCommitMessage: () => `fix review\n\nCentaur-Automation: true\nCentaur-Review-Finding: ${fingerprint}`, comparisonFile: "src/implementation.ts", state, }); + const compare = ctx.octokit.rest.repos.compareCommitsWithBasehead; + ctx.octokit.rest.repos.compareCommitsWithBasehead = (async (request: { + basehead: string; + }) => { + comparisonAttempts += 1; + if (comparisonAttempts === 1) { + throw Object.assign(new Error("temporary GitHub failure"), { + status: 503, + }); + } + return compare(request as never); + }) as unknown as typeof ctx.octokit.rest.repos.compareCommitsWithBasehead; await handleReviewEvent(ctx, submittedReview(43, "head-1")); const initial = (await state.get( "centaur-githubbot:review-budget:base/repo#7", @@ -961,6 +974,7 @@ describe("bounded review epochs", () => { }, }, }); + expect(comparisonAttempts).toBe(2); }); test("keeps acceptance pending when the trailer commit did not change the finding path", async () => { diff --git a/services/githubbot/test/review-findings.test.ts b/services/githubbot/test/review-findings.test.ts index 59b1753207..cdcefca912 100644 --- a/services/githubbot/test/review-findings.test.ts +++ b/services/githubbot/test/review-findings.test.ts @@ -120,6 +120,37 @@ describe("review finding ledger", () => { ); }); + test("reconciles one moved rediscovery without collapsing simultaneous sites", () => { + const first = finding({ diffHunk: "@@ -1 +1 @@\n-old one\n+new one" }); + const initial = mergeReviewFindings(undefined, [first], 1); + const accepted = applyReviewFindingDispositionMarkers( + initial.ledger, + parseReviewFindingDispositionMarkers( + ``, + ), + { commentId: 72, replyToCommentId: 71 }, + ); + const moved = finding({ + commentId: 81, + diffHunk: "@@ -80 +90 @@\n-partially repaired\n+still unsafe", + line: 90, + reviewId: 32, + }); + expect(moved.fingerprint).not.toBe(first.fingerprint); + const reconciled = mergeReviewFindings(accepted.ledger, [moved], 2); + expect(reconciled.newFindings).toEqual([]); + expect(Object.keys(reconciled.ledger)).toEqual([first.fingerprint]); + + const otherSite = finding({ + commentId: 82, + diffHunk: "@@ -120 +120 @@\n-old other\n+new other", + line: 120, + }); + const simultaneous = mergeReviewFindings(undefined, [first, otherSite], 1); + expect(simultaneous.newFindings).toHaveLength(2); + expect(Object.keys(simultaneous.ledger)).toHaveLength(2); + }); + test("preserves the highest severity when a pending finding is rediscovered", () => { const severe = finding({ body: From 16075dc7416c4b5ebda7ce000c8f4cd62ddbf643 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 11:44:19 +0900 Subject: [PATCH 12/14] Distinguish review finding sites safely --- services/githubbot/src/review-findings.ts | 61 ++++++------------- services/githubbot/test/pr-manager.test.ts | 11 +++- .../githubbot/test/review-findings.test.ts | 34 ++++++++--- 3 files changed, 51 insertions(+), 55 deletions(-) diff --git a/services/githubbot/src/review-findings.ts b/services/githubbot/src/review-findings.ts index 1f0ae24143..f0257fd4a7 100644 --- a/services/githubbot/src/review-findings.ts +++ b/services/githubbot/src/review-findings.ts @@ -14,7 +14,6 @@ export type ReviewFinding = { reviewId: number; reviewerKey: string; reviewedHeadSha: string; - semanticFingerprint: string; severity: "normal" | "p0" | "security"; url?: string; }; @@ -28,7 +27,6 @@ export type ReviewFindingRecord = { reviewId: number; reviewerKey: string; reviewedHeadSha: string; - semanticFingerprint?: string; severity: ReviewFinding["severity"]; }; @@ -65,17 +63,7 @@ export function fingerprintReviewFinding(input: { body: normalizeFindingText(input.body), context: normalizeDiffContext(input.diffHunk), path: normalizePath(input.path), - }); - return `sha256:${createHash("sha256").update(canonical).digest("hex")}`; -} - -function semanticFingerprintReviewFinding(input: { - body: string; - path?: string; -}): string { - const canonical = JSON.stringify({ - body: normalizeFindingText(input.body), - path: normalizePath(input.path), + site: relativeDiffLine(input.diffHunk, input.line), }); return `sha256:${createHash("sha256").update(canonical).digest("hex")}`; } @@ -105,7 +93,6 @@ export function makeReviewFinding(input: { reviewId: input.reviewId, reviewerKey: input.reviewerKey, reviewedHeadSha: input.reviewedHeadSha, - semanticFingerprint: semanticFingerprintReviewFinding({ body, path }), severity: findingSeverity({ body, diffHunk, line, path }), url: input.url?.trim().slice(0, 2_000) || undefined, }; @@ -148,30 +135,8 @@ export function mergeReviewFindings( } { const next: ReviewFindingLedger = { ...(ledger ?? {}) }; const actionableFindings: ReviewFinding[] = []; - const semanticCounts = new Map(); for (const finding of findings) { - semanticCounts.set( - finding.semanticFingerprint, - (semanticCounts.get(finding.semanticFingerprint) ?? 0) + 1, - ); - } - for (const finding of findings) { - let fingerprint = finding.fingerprint; - if ( - !next[fingerprint] && - semanticCounts.get(finding.semanticFingerprint) === 1 - ) { - const matching = Object.entries(next).filter( - ([, record]) => - record.semanticFingerprint === finding.semanticFingerprint, - ); - if (matching.length === 1) fingerprint = matching[0]![0]; - } - const canonicalFinding = - fingerprint === finding.fingerprint - ? finding - : { ...finding, fingerprint }; - const existing = next[fingerprint]; + const existing = next[finding.fingerprint]; if ( existing?.disposition === "accepted" || existing?.disposition === "rejected" @@ -180,21 +145,20 @@ export function mergeReviewFindings( } // A repeated pending finding remains actionable. Only an evidence-backed // accepted/rejected decision suppresses rediscovery. - actionableFindings.push(canonicalFinding); + actionableFindings.push(finding); if (existing) { - next[fingerprint] = { + next[finding.fingerprint] = { ...existing, commentId: finding.commentId, path: finding.path, reviewId: finding.reviewId, reviewerKey: finding.reviewerKey, reviewedHeadSha: finding.reviewedHeadSha, - semanticFingerprint: finding.semanticFingerprint, severity: highestSeverity(existing.severity, finding.severity), }; continue; } - next[fingerprint] = { + next[finding.fingerprint] = { commentId: finding.commentId, disposition: "pending", firstSeenEpoch: epoch, @@ -202,7 +166,6 @@ export function mergeReviewFindings( reviewId: finding.reviewId, reviewerKey: finding.reviewerKey, reviewedHeadSha: finding.reviewedHeadSha, - semanticFingerprint: finding.semanticFingerprint, severity: finding.severity, }; } @@ -261,6 +224,18 @@ function normalizeDiffContext(value: string | undefined): string { .trim(); } +function relativeDiffLine( + diffHunk: string | undefined, + line: number | undefined, +): number | undefined { + const normalizedLine = positiveInteger(line); + if (normalizedLine === undefined) return undefined; + const header = diffHunk?.split(/\r?\n/, 1)[0]; + const rightStart = header?.match(/^@@\s+-\d+(?:,\d+)?\s+\+(\d+)/)?.[1]; + if (!rightStart) return normalizedLine; + return normalizedLine - Number.parseInt(rightStart, 10); +} + function highestSeverity( left: ReviewFinding["severity"], right: ReviewFinding["severity"], @@ -378,8 +353,6 @@ export function isReviewFindingLedger( typeof finding.reviewedHeadSha === "string" && finding.reviewedHeadSha.length > 0 && finding.reviewedHeadSha.length <= 100 && - (finding.semanticFingerprint === undefined || - /^sha256:[0-9a-f]{64}$/.test(finding.semanticFingerprint)) && ["normal", "p0", "security"].includes(finding.severity ?? "") && (finding.commentId === undefined || (Number.isInteger(finding.commentId) && finding.commentId > 0)) && diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index e9fa9b1529..9624a70bc1 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -750,7 +750,14 @@ describe("bounded review epochs", () => { actor: "human", reviewFindings: { 40: [{ ...sharedFinding, id: 400 }], - 41: [{ ...sharedFinding, id: 410, line: 25 }], + 41: [ + { + ...sharedFinding, + diff_hunk: "@@ -6 +6 @@\n-unchecked\n+checked", + id: 410, + line: 25, + }, + ], 42: [ { body: "A new finding after the zero-round epoch was restored.", @@ -1107,11 +1114,13 @@ describe("bounded review epochs", () => { const oldFingerprint = fingerprintReviewFinding({ body: oldBody, diffHunk: "+old();", + line: 20, path: oldPath, }); const newFingerprint = fingerprintReviewFinding({ body: newBody, diffHunk: "+new();", + line: 30, path: newPath, }); const state = makeState(); diff --git a/services/githubbot/test/review-findings.test.ts b/services/githubbot/test/review-findings.test.ts index cdcefca912..cdbbb85fbd 100644 --- a/services/githubbot/test/review-findings.test.ts +++ b/services/githubbot/test/review-findings.test.ts @@ -13,7 +13,7 @@ function finding(overrides: Partial[0]> = { return makeReviewFinding({ body: "The unchecked value can escape the repository policy.", commentId: 71, - diffHunk: "@@ -1 +1 @@\n-old\n+new", + diffHunk: "@@ -14 +14 @@\n-old\n+new", line: 14, path: "src/policy.ts", reviewId: 31, @@ -27,7 +27,7 @@ describe("review finding fingerprints", () => { test("are stable across reviewers, moved hunk coordinates, and URLs", () => { const first = fingerprintReviewFinding({ body: "Check https://example.test/one before use", - diffHunk: "@@ -1 +1 @@\n-old\n+same implementation", + diffHunk: "@@ -1 +10 @@\n-old\n+same implementation", line: 10, path: "./src/policy.ts", }); @@ -90,6 +90,7 @@ describe("review finding ledger", () => { const repeated = finding({ commentId: 88, + diffHunk: "@@ -40 +40 @@\n-old\n+new", line: 40, reviewId: 32, reviewerKey: "github-user:202", @@ -120,8 +121,11 @@ describe("review finding ledger", () => { ); }); - test("reconciles one moved rediscovery without collapsing simultaneous sites", () => { - const first = finding({ diffHunk: "@@ -1 +1 @@\n-old one\n+new one" }); + test("distinguishes sites while keeping an unchanged moved hunk stable", () => { + const first = finding({ + diffHunk: "@@ -14,2 +14,2 @@\n first line\n second line", + line: 14, + }); const initial = mergeReviewFindings(undefined, [first], 1); const accepted = applyReviewFindingDispositionMarkers( initial.ledger, @@ -130,25 +134,35 @@ describe("review finding ledger", () => { ), { commentId: 72, replyToCommentId: 71 }, ); - const moved = finding({ + const movedUnchanged = finding({ commentId: 81, - diffHunk: "@@ -80 +90 @@\n-partially repaired\n+still unsafe", + diffHunk: "@@ -90,2 +90,2 @@\n first line\n second line", line: 90, reviewId: 32, }); - expect(moved.fingerprint).not.toBe(first.fingerprint); - const reconciled = mergeReviewFindings(accepted.ledger, [moved], 2); + expect(movedUnchanged.fingerprint).toBe(first.fingerprint); + const reconciled = mergeReviewFindings(accepted.ledger, [movedUnchanged], 2); expect(reconciled.newFindings).toEqual([]); expect(Object.keys(reconciled.ledger)).toEqual([first.fingerprint]); const otherSite = finding({ commentId: 82, - diffHunk: "@@ -120 +120 @@\n-old other\n+new other", - line: 120, + diffHunk: first.diffHunk, + line: 15, }); const simultaneous = mergeReviewFindings(undefined, [first, otherSite], 1); expect(simultaneous.newFindings).toHaveLength(2); expect(Object.keys(simultaneous.ledger)).toHaveLength(2); + + const changedSite = finding({ + commentId: 83, + diffHunk: "@@ -14 +14 @@\n-partially repaired\n+still unsafe", + line: 14, + reviewId: 33, + }); + const reopened = mergeReviewFindings(accepted.ledger, [changedSite], 2); + expect(reopened.newFindings).toEqual([changedSite]); + expect(Object.keys(reopened.ledger)).toHaveLength(2); }); test("preserves the highest severity when a pending finding is rediscovered", () => { From 8ed571e85ae703c11a62d85b7abf82e27d319205 Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 11:55:38 +0900 Subject: [PATCH 13/14] Clarify blocking review body accounting --- services/githubbot/src/pr-manager.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index 2ac9560ac7..a2fde1a214 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -1537,9 +1537,10 @@ async function collectReviewFindings( } } - // GitHub reviews may carry one body-only finding and no inline comments. - // Do not fingerprint a summary body in addition to its inline findings: that - // would create a fresh pseudo-finding whenever a reviewer rewrites a summary. + // GitHub reviews may carry one body-only finding and no inline comments. A + // changes-requested body can also identify a distinct blocking finding, so + // retain it alongside inline findings; ordinary review summaries remain + // excluded to avoid creating a pseudo-finding when their wording changes. if (findings.length === 0 || reviewState === "changes_requested") { const body = stringValue(reviewNode.body)?.trim(); if (body) { From 9daa5444e65faabc2b0e6dd41df425858b22654b Mon Sep 17 00:00:00 2001 From: Michael Wu Date: Tue, 8 Sep 2026 12:04:26 +0900 Subject: [PATCH 14/14] Preserve review provenance and diff sides --- services/githubbot/src/pr-manager.ts | 15 +++--- services/githubbot/src/review-findings.ts | 31 +++++++++-- services/githubbot/test/pr-manager.test.ts | 51 +++++++++++++++++++ .../githubbot/test/review-findings.test.ts | 27 ++++++++++ 4 files changed, 110 insertions(+), 14 deletions(-) diff --git a/services/githubbot/src/pr-manager.ts b/services/githubbot/src/pr-manager.ts index a2fde1a214..84226cba09 100644 --- a/services/githubbot/src/pr-manager.ts +++ b/services/githubbot/src/pr-manager.ts @@ -1516,17 +1516,21 @@ async function collectReviewFindings( for (const comment of comments) { const body = stringValue(comment.body)?.trim(); if (!body) continue; + const currentLine = numberValue(comment.line); + const originalLine = numberValue(comment.original_line); findings.push( makeReviewFinding({ body, commentId: numberValue(comment.id), diffHunk: stringValue(comment.diff_hunk), - line: - numberValue(comment.line) ?? numberValue(comment.original_line), + line: currentLine ?? originalLine, path: stringValue(comment.path), reviewId, reviewerKey, reviewedHeadSha, + // GitHub retains `side` when a current line has become outdated; + // there is no `original_side` field in the REST response schema. + side: stringValue(comment.side), url: stringValue(comment.html_url), }), ); @@ -1832,13 +1836,6 @@ async function admitReviewResponse( headSha, mergedFindings.ledger, ); - if ( - evidence.actor === "unknown" && - loaded.state.automationPendingFromHeadSha === - loaded.state.lastReviewedHeadSha - ) { - evidence = { ...evidence, actor: "automation" }; - } } const findingsAlreadyKnown = diff --git a/services/githubbot/src/review-findings.ts b/services/githubbot/src/review-findings.ts index f0257fd4a7..400b9909d5 100644 --- a/services/githubbot/src/review-findings.ts +++ b/services/githubbot/src/review-findings.ts @@ -58,12 +58,17 @@ export function fingerprintReviewFinding(input: { diffHunk?: string; line?: number; path?: string; + side?: string; }): string { + const side = normalizeDiffSide(input.side); const canonical = JSON.stringify({ body: normalizeFindingText(input.body), context: normalizeDiffContext(input.diffHunk), path: normalizePath(input.path), - site: relativeDiffLine(input.diffHunk, input.line), + site: relativeDiffLine(input.diffHunk, input.line, side), + // Preserve existing right-side fingerprints while distinguishing a + // finding attached to the removed side of the same replacement hunk. + ...(side === "left" ? { side } : {}), }); return `sha256:${createHash("sha256").update(canonical).digest("hex")}`; } @@ -77,6 +82,7 @@ export function makeReviewFinding(input: { reviewId: number; reviewerKey: string; reviewedHeadSha: string; + side?: string; url?: string; }): ReviewFinding { const body = input.body.trim().slice(0, 16_000); @@ -87,7 +93,13 @@ export function makeReviewFinding(input: { body, commentId: positiveInteger(input.commentId), diffHunk, - fingerprint: fingerprintReviewFinding({ body, diffHunk, line, path }), + fingerprint: fingerprintReviewFinding({ + body, + diffHunk, + line, + path, + side: input.side, + }), line, path, reviewId: input.reviewId, @@ -227,13 +239,22 @@ function normalizeDiffContext(value: string | undefined): string { function relativeDiffLine( diffHunk: string | undefined, line: number | undefined, + side: "left" | "right" | undefined, ): number | undefined { const normalizedLine = positiveInteger(line); if (normalizedLine === undefined) return undefined; const header = diffHunk?.split(/\r?\n/, 1)[0]; - const rightStart = header?.match(/^@@\s+-\d+(?:,\d+)?\s+\+(\d+)/)?.[1]; - if (!rightStart) return normalizedLine; - return normalizedLine - Number.parseInt(rightStart, 10); + const starts = header?.match(/^@@\s+-(\d+)(?:,\d+)?\s+\+(\d+)/); + const start = side === "left" ? starts?.[1] : starts?.[2]; + if (!start) return normalizedLine; + return normalizedLine - Number.parseInt(start, 10); +} + +function normalizeDiffSide( + value: string | undefined, +): "left" | "right" | undefined { + const side = value?.trim().toLowerCase(); + return side === "left" || side === "right" ? side : undefined; } function highestSeverity( diff --git a/services/githubbot/test/pr-manager.test.ts b/services/githubbot/test/pr-manager.test.ts index 9624a70bc1..87f104dee0 100644 --- a/services/githubbot/test/pr-manager.test.ts +++ b/services/githubbot/test/pr-manager.test.ts @@ -1485,6 +1485,57 @@ describe("bounded review epochs", () => { ).toMatchObject({ anchorHeadSha: "head-3", epoch: 2, roundsUsed: 1 }); }); + test("does not treat mixed commits as automation because a repair was pending", async () => { + const state = makeState(); + await state.set("centaur-githubbot:review-budget:base/repo#7", { + anchorHeadSha: "head-1", + automationPendingFromHeadSha: "head-1", + epoch: 1, + lastReviewedHeadSha: "head-1", + reviewerRoundsUsed: { "github-user:101": 1 }, + roundsUsed: 1, + version: 1, + }); + const comments: string[] = []; + const ctx = budgetCtx({ headSha: "head-2", comments, state }); + ctx.octokit.rest.repos.compareCommitsWithBasehead = (async () => ({ + data: { + commits: [ + { + author: { login: "centaur-bot", type: "Bot" }, + commit: { message: "accepted repair\n\nCentaur-Automation: true" }, + }, + { + author: { login: "alice", type: "User" }, + commit: { message: "additional human change" }, + }, + ], + files: [ + { + additions: 4, + changes: 4, + deletions: 0, + filename: "src/implementation.ts", + status: "modified", + }, + ], + status: "ahead", + total_commits: 2, + }, + })) as unknown as typeof ctx.octokit.rest.repos.compareCommitsWithBasehead; + + await handleReviewEvent(ctx, submittedReview(81, "head-2")); + await drainBackgroundWork(5_000); + + expect( + await state.get("centaur-githubbot:review-budget:base/repo#7"), + ).toMatchObject({ + pausedHeadSha: "head-2", + pauseReason: "change_actor_unknown", + }); + expect(comments.at(-1)).toContain("change_actor_unknown"); + }); + test("classifies an approved material human head before advancing the boundary", async () => { const state = makeState(); await state.set("centaur-githubbot:review-budget:base/repo#7", { diff --git a/services/githubbot/test/review-findings.test.ts b/services/githubbot/test/review-findings.test.ts index cdbbb85fbd..0dcff48e55 100644 --- a/services/githubbot/test/review-findings.test.ts +++ b/services/githubbot/test/review-findings.test.ts @@ -53,6 +53,33 @@ describe("review finding fingerprints", () => { ).not.toBe(first); }); + test("distinguishes old and new sides while keeping moved side coordinates stable", () => { + const right = fingerprintReviewFinding({ + body: "The replacement remains unsafe.", + diffHunk: "@@ -14 +14 @@\n-old\n+new", + line: 14, + path: "src/policy.ts", + side: "RIGHT", + }); + const left = fingerprintReviewFinding({ + body: "The replacement remains unsafe.", + diffHunk: "@@ -14 +14 @@\n-old\n+new", + line: 14, + path: "src/policy.ts", + side: "LEFT", + }); + const movedLeft = fingerprintReviewFinding({ + body: "The replacement remains unsafe.", + diffHunk: "@@ -90 +99 @@\n-old\n+new", + line: 90, + path: "src/policy.ts", + side: "LEFT", + }); + + expect(left).not.toBe(right); + expect(movedLeft).toBe(left); + }); + test("requires structured impact and inline evidence for a budget interrupt", () => { const evidence = { diffHunk: "+untrusted(input)",