diff --git a/scripts/ally-review-consistency-baseline.json b/scripts/ally-review-consistency-baseline.json index 683ae9de240..9e2fbc2291f 100644 --- a/scripts/ally-review-consistency-baseline.json +++ b/scripts/ally-review-consistency-baseline.json @@ -13,7 +13,10 @@ "Empty as of 2026-09-20 (PEN-2847). The six founding entries (#1525 x2, #1360, #1316 x2,", "#1304) had all self-expired by head movement, exactly as the fingerprint design intends,", "and the guard warned on every hourly run asking for their removal. An empty list is not a", - "disabled ratchet: with no entries, every finding on a mergeable PR fails the run." + "disabled ratchet: with no entries, every finding on a mergeable PR fails the run.", + "Separately (BLO-25764): the I1 shape four of them suppressed is no longer a violation when", + "the bodies differ - a re-review supersedes. That exemption is I1-only and does not reach", + "I3, which asserts one canonical body and a matching attestation." ], "entries": [] } diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index 5b7e039d66e..4fbd888413b 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -15,12 +15,34 @@ * Observed on Blockcast/paperclip#876 (BLO-19778): two runs dispatched 43 ms * apart both submitted at head ff1c72db, 34 s apart, with opposite verdicts. * - * I1 At most one operative review per lane per (PR, head SHA). A same-lane - * duplicate also reports whether the bodies are identical or differ + * I1 At most one operative review per lane per (PR, head SHA), EXCEPT where + * the App lane's duplicates all carry distinct bodies — see below. A + * same-lane duplicate reports whether the bodies are identical or differ * (`sameLaneBodyRelation`), because that — not the gap between * submissions — is what says whether the missing control is submit * idempotency or reviewer exclusion. * + * On the App-lane `recompute` exemption (BLO-25764). Ally may re-review + * an unchanged head: a finding whose remedy is not a code change (a + * wrong PR description, a rebase that moved nothing) is addressed + * without moving the head, so the re-review lands at the same SHA and + * supersedes its predecessor. That is correct behaviour, and it is + * observationally identical to the concurrent-run race in the paragraph + * above: both yield N canonical App verdicts at one head with differing + * bodies and possibly differing dispositions. Measured over every + * same-head App duplicate pair on the open PRs (2026-09-23, n=15), the + * gap between submissions runs 3 s → 33.6 h with no separation, so no + * time threshold distinguishes them either. Asserting `at most 1` over + * that shape is therefore unsatisfiable while re-review is permitted — + * which is why this guard failed 99/99 scheduled runs from 2026-08-07. + * Differing App bodies at one head are reported as a notice + * (`findPrNotices`) and the latest submission is the standing verdict; + * I2/I3/I4 still evaluate EVERY operative review, so a superseded review + * that approves over a blocker is still fatal. Identical bodies keep + * failing: one verdict submitted twice has no legitimate explanation. + * The exclusion control this gave up belongs at dispatch, where the + * concurrency is visible — see BLO-20074. + * * Three arms here can only fire when an operative seat review exists — * I1 over the seat lane, I1 for one body submitted under two * credentials, and I2b — so I6 already fires wherever they do. They are @@ -29,8 +51,10 @@ * lane still carries a permitted shape. * I2 No operative APPROVED review whose own body reports a Critical or * Important finding, no User-seat APPROVED review coexisting with a - * blocking App review, and no App approval without a `Reviewed head:` - * attestation. + * blocking App review, no App APPROVED coexisting with a different + * blocking App review at one head unless it follows every such blocker + * and retires, by name, a finding raised against that head (I2e), and no + * App approval without a `Reviewed head:` attestation. * I3 An operative App review has exactly one canonical body and its * body-attested `Reviewed head:` matches the commit GitHub recorded it * against. @@ -113,23 +137,129 @@ const BLOCKING_SECTION_RE = * comment exists to prevent. * * Residual, stated rather than implied: the gate additionally blanks fenced - * spans before matching, and this script does not, so a *fenced* paste is - * still read here as an attestation while the gate ignores it. The extra - * attestation is not quietly absorbed — canonicalReviewHead requires exactly - * one, so it returns null and the review is reported as an I3 "not canonical" - * violation. (I3, not I1: I1 caps operative reviews per lane, not attestations - * within a body.) The direction is still the safe one for an auditor, because - * the consequence is a false red against an otherwise-valid review rather than - * a missed one, but it is a real remaining divergence, not parity. + * spans before matching. For the attestation and bucket sites this script + * does not, so a *fenced* paste is still read here as an attestation while the + * gate ignores it. The extra attestation is not quietly absorbed — + * canonicalReviewHead requires exactly one, so it returns null and the review + * is reported as an I3 "not canonical" violation. (I3, not I1: I1 caps + * operative reviews per lane, not attestations within a body.) For those sites + * the direction is the safe one for an auditor, because the consequence is a + * false red against an otherwise-valid review rather than a missed one. + * + * It is NOT safe for the retiring site. A fenced retiring entry read here but + * not by the gate retires a finding the gate still counts, which lets + * supersedesBlocker exempt an approval the gate holds: fail-open. So + * retiredFindingKeys strips fences first with withoutFencedCodeBlocks, as the + * gate's extractAllyPriorFindingDispositions does. + * + * hasStillPresentDisposition deliberately does NOT strip. It mirrors a + * blocking predicate, and the gate's rule (ally-review-detection.ts header: + * "Quoted text may never *reduce* what the gate blocks on; only emitted text + * may retire a finding") puts blocking predicates in the detecting group, + * which reads emitted and raw text and blocks if either does + * (hasActionablePrReviewFeedback). A fenced still-present entry therefore + * blocks at the gate, and must block here; stripping it would go silent on a + * PR the gate holds red. */ const NOT_INDENTED_CODE = String.raw`(?! *\t)(?! {4}) {0,3}`; +/** + * Blanks fenced code spans, line for line. A verbatim mirror of + * withoutFencedCodeBlocks in server/src/services/ally-review-detection.ts, + * which the gate applies before reading the prior-finding ledger; mirrored + * rather than imported for the same unpinned-`node` reason as the patterns + * here. Lines are blanked, not removed, so line-anchored patterns keep their + * anchors. An unclosed fence blanks to end of body, as GitHub renders it. + */ +const FENCE_DELIMITER_PATTERN = /^ {0,3}(`{3,}|~{3,})(.*)$/; +const FENCE_CLOSE_PATTERN = /^ {0,3}(`{3,}|~{3,})[ \t]*$/; + +function withoutFencedCodeBlocks(body) { + if (!body.includes("```") && !body.includes("~~~")) return body; + const lines = body.split("\n"); + let open = null; + for (let i = 0; i < lines.length; i += 1) { + const line = lines[i]; + if (open) { + const close = FENCE_CLOSE_PATTERN.exec(line); + const closes = close && close[1][0] === open.char && close[1].length >= open.length; + lines[i] = ""; + if (closes) open = null; + continue; + } + const fence = FENCE_DELIMITER_PATTERN.exec(line); + // Per CommonMark a backtick fence's info string may not contain a backtick. + if (fence && !(fence[1][0] === "`" && fence[2].includes("`"))) { + open = { char: fence[1][0], length: fence[1].length }; + lines[i] = ""; + } + } + return lines.join("\n"); +} + +/** + * Every counted bucket, with its severity and count. The `(0)` case is kept + * here — unlike BLOCKING_SECTION_RE, which asks "does this block?" — so that + * enumerating a body's findings sees an explicit empty bucket and simply + * contributes no indices for it. + * + * Must stay equivalent to COUNTED_FINDINGS_BUCKET_PATTERN in + * server/src/services/ally-review-detection.ts, which decides what a review + * raised for the merge gate. Mirrored rather than imported because the + * scheduled guard runs this file under the runner's unpinned `node`, which is + * not guaranteed to load a `.ts` module. The module's leading ` {0,3}` is + * folded into NOT_INDENTED_CODE here, as described above. A narrower copy + * under-counts a blocker, which lets supersedesBlocker exempt an approval that + * retired only part of it: the fail-open direction. The suite runs both + * against the shapes where they used to diverge. + * + * The gate reads the raw and the fence-stripped body and keeps the higher + * count per severity. Reading raw alone is equivalent: every part of this + * pattern is line-local, and fence stripping only blanks whole lines, so the + * stripped reading's matches are a subset of the raw reading's. + */ +const COUNTED_SECTION_GLOBAL_RE = new RegExp( + String.raw`^${NOT_INDENTED_CODE}(?:[#>][ \t]*)*(?:(?:[-*+]|\d+[.)])[ \t]+)?[*_]*(critical|important)[ \t]+Issues\b[*_]*[ \t]*\((\d+)\)`, + "gim", +); + /** A prior-finding disposition that says the blocker is still present. */ const STILL_PRESENT_DISPOSITION_RE = new RegExp( - String.raw`^${NOT_INDENTED_CODE}-[ \t]*\*\*prior:[^\n]*\*\*[ \t]*(?:—|-)[ \t]*still-present[ \t]*(?:—|-)`, + String.raw`^${NOT_INDENTED_CODE}-[ \t]*\*\*prior:[^\n]*\*\*[ \t]*(?:—|-)[ \t]*still-present(?![a-z-])[ \t]*(?:—|-)`, "im", ); +/** + * A prior-finding disposition, capturing the head it was raised against, the + * `(severity, index)` pair naming it, and the whole verb. The verb is captured + * the way the gate's PRIOR_FINDING_DISPOSITION_PATTERN captures it, + * `([a-z][a-z-]*)`, and retiredFindingKeys tests it for membership in + * RETIRING_DISPOSITIONS. Alternating the retiring verbs inside the pattern + * instead let a hyphenated verb that merely begins with one (`fixed-upstream`) + * retire a finding the gate does not, because the trailing separator class + * absorbed the rest of the verb: fail-open. An unrecognized verb is captured + * but is not a member, so I2e fails closed on it. + * + * Severity and index are required rather than skipped over, because I2e checks + * ledger *coverage* of the blocker's counted findings, not merely that the head + * was named once. An entry too malformed to identify a finding therefore + * retires nothing — a false red on an otherwise-valid supersession, which is + * the direction an auditor may fail in. + * + * The separator alternation and the space allowed after `**` track the gate's + * PRIOR_FINDING_DISPOSITION_PATTERN. Both shapes are ones the gate accepts, so + * omitting them here would let the gate read an entry as retiring while I2e did + * not — a fatal red on a supersession the gate is happy with, which is the + * failure class this script exists to remove. + */ +const RETIRING_DISPOSITION_GLOBAL_RE = new RegExp( + String.raw`^${NOT_INDENTED_CODE}-[ \t]*\*\*[ \t]*prior:([0-9a-f]{7,40})[ \t]+([a-z]+)[ \t]+(\d+)[ \t]*\*\*[ \t]*(?:—|–|-)[ \t]*([a-z][a-z-]*)[ \t]*(?:—|–|-)`, + "gim", +); + +/** The retiring verbs: the gate's RESOLVED_PRIOR_DISPOSITIONS, verbatim. */ +const RETIRING_DISPOSITIONS = new Set(["fixed", "no-longer-applicable"]); + /** The single standalone attestation line Ally is required to emit. */ const ATTESTED_HEAD_RE = new RegExp( String.raw`^${NOT_INDENTED_CODE}(?:[_*]+)?[ \t]*reviewed head:[ \t]*\`?([0-9a-f]{40})\`?[ \t]*(?:[_*]+)?[ \t]*$`, @@ -168,6 +298,14 @@ function hasBlockingVerdict(body) { return hasBlockingFindings(body) || hasStillPresentDisposition(body); } +// submitted_at has 1 s resolution, so ties fall back to the monotonic id. +function bySubmission(a, b) { + return ( + String(a?.submitted_at ?? "").localeCompare(String(b?.submitted_at ?? "")) || + Number(a?.id ?? 0) - Number(b?.id ?? 0) + ); +} + function reviewDetails(reviews) { return reviews.map((review) => `${reviewState(review)}/${review.id}`).join(", "); } @@ -246,6 +384,72 @@ export function hasStillPresentDisposition(body) { return STILL_PRESENT_DISPOSITION_RE.test(String(body ?? "")); } +/** + * The findings a body declares in its own counted buckets, as `severity index` + * keys. A bucket of N contributes indices 1..N, the same `(severity, index)` + * identity the merge gate enumerates in extractAllyReportedFindingRefs. The + * two agree on what a review raised only because COUNTED_SECTION_GLOBAL_RE + * mirrors the gate's bucket pattern; see the note there. + */ +export function countedFindingKeys(body) { + const keys = new Set(); + for (const [, severity, count] of String(body ?? "").matchAll(COUNTED_SECTION_GLOBAL_RE)) { + for (let index = 1; index <= Number(count); index += 1) { + keys.add(`${severity.toLowerCase()} ${index}`); + } + } + return keys; +} + +/** The findings a body retires by name against `head`, in the same key space. */ +export function retiredFindingKeys(body, head) { + const normalizedHead = String(head ?? "").toLowerCase(); + const keys = new Set(); + for (const [, prefix, severity, index, verb] of withoutFencedCodeBlocks( + String(body ?? ""), + ).matchAll(RETIRING_DISPOSITION_GLOBAL_RE)) { + if (!RETIRING_DISPOSITIONS.has(verb.toLowerCase())) continue; + if (normalizedHead.startsWith(prefix.toLowerCase())) { + keys.add(`${severity.toLowerCase()} ${Number(index)}`); + } + } + return keys; +} + +/** + * True when `approval` retires, by name and against this head, EVERY finding + * `blocker` counted. + * + * Coverage rather than presence: a blocker may raise several findings at one + * head, and an approval retiring 1 of N would otherwise stand green over the + * N-1 nobody dispositioned — I2e's own harm class, reached through its exemption. + * + * A blocker with no counted findings is never superseded. That is a blocker + * blocking solely on a `still-present` entry, which asserts a finding raised at + * an *earlier* head; it has no (severity, index) at this head for a ledger to + * name, and it is enumerated on its own account at the head that raised it. + * Fail closed. + * + * A blocker that MIRRORS that still-present finding into its counted bucket, as + * the reviewer contract asks, keeping its original `prior: ...` label, + * is keyed here by position at THIS head all the same. That is deliberate, and + * it is a known false red: an approval retiring the finding only under its + * original name does not supersede the blocker. Keying the slot on the name it + * carries would clear it, and would also clear the #876 / #1220 race, because an + * earlier head's finding is exactly the name both racing runs can produce. It + * proves nothing about having read the blocker. A this-head name does, so an + * approval that also retires `prior: ` still + * supersedes; otherwise a new head is the exit. Ally's audit at 37522699 found + * the shape in none of 64 real bodies. + */ +function supersedesBlocker(approval, blocker, head) { + const raised = countedFindingKeys(blocker?.body); + if (raised.size === 0) return false; + const retired = retiredFindingKeys(approval?.body, head); + for (const key of raised) if (!retired.has(key)) return false; + return true; +} + export function attestedHead(body) { const match = ATTESTED_HEAD_RE.exec(String(body ?? "")); return match ? match[1].toLowerCase() : null; @@ -351,6 +555,46 @@ export function sameLaneBodyRelation(operative) { return anyPairIdentical ? "mixed" : "recompute"; } +/** + * True when a same-head App-lane duplicate is a re-review superseding its + * predecessor rather than a defect. + * + * Scoped to the App lane deliberately: the User seat may not submit a verdict + * at all (I6/R4), so a seat duplicate has no legitimate reading and keeps + * failing. `recompute` — every body distinct — is the only exempt relation. + * `resubmit` and `mixed` both contain a byte-identical pair, which is one + * verdict delivered more than once and is always a submit-side defect, and a + * `null` relation means an empty body, which is an attestation defect. + * + * This exempts the shape from I1 only. Every review in the set is still + * carried through I2/I3/I4/I5, so a superseded review that approves over a + * blocking finding remains fatal. + */ +export function isSupersedingAppRereview(lane, operative) { + return lane === "app" && sameLaneBodyRelation(operative) === "recompute"; +} + +/** + * Non-fatal observations. Supersession is legitimate but it is still two runs + * doing one PR's work, so it is reported rather than dropped: silence here + * would make a re-review storm indistinguishable from a quiet week. + * + * @returns {string[]} + */ +export function findPrNotices(pr) { + const head = pr.headSha; + const short = String(head ?? "").slice(0, 8); + const reviews = operativeAllyReviews(pr.reviews, head, "app"); + if (!isSupersedingAppRereview("app", reviews)) return []; + const latest = [...reviews].sort(bySubmission).at(-1); + return [ + `PR #${pr.number} @${short}: ${reviews.length} operative Ally App reviews (${reviewDetails(reviews)}) with distinct bodies — ` + + `treating the latest (${latest?.id}, ${latest?.submitted_at}) as the standing verdict. Legitimate for a re-review of an ` + + `unchanged head; also the signature of two concurrent runs, which review data cannot distinguish (BLO-25764). ` + + `Exclusion belongs at dispatch — see BLO-20074.`, + ]; +} + const SAME_LANE_RELATION_NOTES = { resubmit: "the bodies are identical — one verdict submitted more than once, so the submit step is at-least-once", @@ -389,7 +633,7 @@ export function findPrViolations(pr) { const reviews = reviewsByLane.get(lane); const label = laneLabel(lane); - if (reviews.length > 1) { + if (reviews.length > 1 && !isSupersedingAppRereview(lane, reviews)) { const relation = SAME_LANE_RELATION_NOTES[sameLaneBodyRelation(reviews)]; violations.push( `I1 PR #${pr.number} @${short}: ${reviews.length} operative ${label} reviews (${reviewDetails(reviews)}) — expected at most 1 in the ${lane} lane` + @@ -485,6 +729,42 @@ export function findPrViolations(pr) { `I2b PR #${pr.number} @${short}: User-seat APPROVED (${seatApprovals.map((review) => review.id).join(", ")}) coexists with a blocking Ally App review (${appBlockers.map((review) => review.id).join(", ")}) — the User seat cannot mask the App blocker`, ); } + // I2e: the I1 supersession exemption lets differing App bodies at one head + // stand as a re-review, so I1 no longer catches the BLO-19778 shape: a clean + // App APPROVED beside a DIFFERENT App review that blocks. I2a sees a blocker + // only inside the approving body itself. An undismissed APPROVED counts + // toward reviewDecision and a COMMENTED blocker does not, so the approval + // would outrank it. A re-review that supersedes a blocker dismisses the stale + // approval, which leaves it non-operative, so this does not fire there. + // + // The other order has no such exit: a COMMENTED blocker cannot be dismissed, + // so a clean approval that supersedes it at an unchanged head would fail here + // forever. That approval is exempt from a given blocker when it retires, by + // name, every finding that blocker counted at this head, and lands after it. + // Naming is the test: only a run that read the blocker can name its findings, + // and a racing run never saw them. Merely carrying a ledger is not enough, + // because both racing reviews on #876 (ff1c72db) and on #1220 (a9ee094a) + // carried one, for findings raised at an earlier head. Naming the head once is + // not enough either: a blocker may raise several findings at it, and retiring + // 1 of N would leave the approval standing over the rest. Order alone is not + // the test (a race can land its approval last); it only keeps a blocker that + // follows the approval fatal, since dismissing the approval is the exit there. + // Residual: two runs racing after a same-head predecessor can both name it, so + // this cannot separate them; that exclusion belongs at dispatch (BLO-20074). + const appApprovals = appReviews.filter(isApproved); + const otherAppBlockers = appBlockers.filter((review) => !appApprovals.includes(review)); + const unsupersedingApprovals = appApprovals.filter((review) => + otherAppBlockers.some( + (blocker) => + bySubmission(blocker, review) > 0 || !supersedesBlocker(review, blocker, head), + ), + ); + + if (unsupersedingApprovals.length > 0 && otherAppBlockers.length > 0) { + violations.push( + `I2e PR #${pr.number} @${short}: Ally App APPROVED (${unsupersedingApprovals.map((review) => review.id).join(", ")}) coexists with a different blocking Ally App review (${otherAppBlockers.map((review) => review.id).join(", ")}) at one head; the standing approval outranks the blocker`, + ); + } return violations; } @@ -852,6 +1132,12 @@ function main() { const { failing, deferred } = partitionByMergeEligibility(baselined.failing, prs); const liveCount = prs.filter((pr) => prDormancy(pr) === null).length; + for (const pr of prs) { + for (const notice of findPrNotices(pr)) { + console.log(`::notice title=Superseded Ally review at one head::${notice}`); + } + } + for (const entry of staleEntries) { console.log( `::warning title=Stale ally-review-consistency baseline entry::` + diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index 5aef980b94b..1e956052b33 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -4,6 +4,11 @@ import { resolve } from "node:path"; import { describe, it } from "node:test"; import { pathToFileURL } from "node:url"; +import { + extractAllyPriorFindingDispositions, + extractAllyReportedFindingRefs, + hasActionablePrReviewFeedback, +} from "../server/src/services/ally-review-detection.ts"; import { ALLY_APP_REVIEWER_ID, ALLY_APP_REVIEWER_LOGIN, @@ -15,7 +20,9 @@ import { assertLiveScopeNonVacuous, assertPrListComplete, attestedHead, + countedFindingKeys, duplicateBodyAcrossIdentities, + findPrNotices, findPrViolations, findViolations, hasBlockingFindings, @@ -30,6 +37,7 @@ import { parseBaseline, partitionByMergeEligibility, prDormancy, + retiredFindingKeys, sameLaneBodyRelation, violationFingerprint, } from "./check-ally-review-consistency.mjs"; @@ -165,6 +173,23 @@ describe("hasStillPresentDisposition", () => { false, ); }); + + // Must agree with the merge gate, which treats still-present as a blocking + // predicate and so reads it from the raw body as well as the fence-stripped + // one: a fenced still-present entry blocks there and must fire here. + const entry = "- **prior:354d5b9 important 1** — still-present — the issue remains"; + for (const [shape, body] of [ + ["a plain entry (control)", entry], + ["an entry inside a closed backtick fence", `\`\`\`\n${entry}\n\`\`\``], + ["an entry inside a closed tilde fence", `~~~\n${entry}\n~~~`], + ["an entry inside an md-tagged fence", `\`\`\`md\n${entry}\n\`\`\``], + ["an entry after an unclosed fence", `\`\`\`\n${entry}`], + ]) { + it(`matches the merge gate on ${shape}`, () => { + assert.equal(hasActionablePrReviewFeedback(body), true, "gate fixture drifted"); + assert.equal(hasStillPresentDisposition(body), true); + }); + } }); describe("attestedHead", () => { @@ -614,14 +639,228 @@ describe("I1 names the mechanism a same-lane duplicate implies", () => { assert.match(violation, /submit step is at-least-once/); }); - it("calls differing bodies a double-compute needing exclusion, not idempotency", () => { - // paperclip#1220: two reviews 10 s apart carried different bodies, so a - // timing threshold misfiles this case as a retry. - const violation = findPrViolations( + it("exempts differing bodies: a re-review of one head supersedes, it does not duplicate", () => { + // BLO-25764. paperclip#1220 (10 s apart) and #1972 (33.6 h apart) are the + // same observable shape, and #1972 is a legitimate re-review after a + // description-only fix that could not move the head. Measured over every + // same-head App duplicate pair on the open PRs (n=15) the gap runs + // 3 s → 33.6 h with no separation, so nothing here distinguishes the race + // from the re-review. Asserting at-most-1 over it can never pass. + const violations = findPrViolations( + duplicatePr([canonicalBody(HEAD, "first pass"), canonicalBody(HEAD, "second pass")]), + ); + assert.deepEqual(violations.filter((v) => v.startsWith("I1")), []); + }); + + it("still reports the superseded pair as a notice rather than dropping the signal", () => { + const notices = findPrNotices( duplicatePr([canonicalBody(HEAD, "first pass"), canonicalBody(HEAD, "second pass")]), + ); + assert.equal(notices.length, 1); + assert.match(notices[0], /2 operative Ally App reviews/); + assert.match(notices[0], /standing verdict/); + }); + + it("keeps failing when only SOME bodies repeat — a mixed set still contains a repeated submit", () => { + const violation = findPrViolations( + duplicatePr([canonicalBody(), canonicalBody(), canonicalBody(HEAD, "third")]), ).find((v) => v.startsWith("I1")); - assert.match(violation, /bodies differ/); - assert.match(violation, /exclusion, not submit idempotency/); + assert.match(violation, /some bodies are identical and some differ/); + }); + + it("does not exempt the User seat, which may not submit a verdict at all", () => { + const pr = { + number: 1193, + headSha: HEAD, + reviews: [ + seatReview({ id: 3, body: canonicalBody(HEAD, "a") }), + seatReview({ id: 4, body: canonicalBody(HEAD, "b") }), + ], + }; + assert.match( + findPrViolations(pr).find((v) => v.startsWith("I1")) ?? "", + /^I1 PR #1193 @ff1c72db: 2 operative Ally User seat reviews/, + ); + }); + + it("exempts I1 only — a superseded review that approves over a blocker is still fatal", () => { + // The exemption must not become a hiding place: every review in the set is + // still carried through I2/I3/I4. + const violations = findPrViolations( + duplicatePr([ + canonicalBody(HEAD, "### Important Issues (1)\n- boom"), + canonicalBody(HEAD, "clean second pass"), + ]), + ); + assert.deepEqual(violations.filter((v) => v.startsWith("I1")), []); + assert.match(violations.find((v) => v.startsWith("I2a")) ?? "", /APPROVED but its body reports a Critical\/Important finding/); + }); + + for (const order of ["approval first", "blocker first"]) { + it(`fires I2e when a clean App APPROVED coexists with a different blocking App review (${order})`, () => { + // The BLO-19778 shape across two reviews with DIFFERENT states: the + // approval's own body is clean, so I2a cannot see the blocker, and the + // I1 supersession exemption lets both stand. + const approval = appReview({ id: DUPLICATE_IDS[0], state: "APPROVED", body: canonicalBody(HEAD, "### Critical Issues (0)\n### Important Issues (0)") }); + const blocker = appReview({ id: DUPLICATE_IDS[1], state: "COMMENTED", body: canonicalBody(HEAD, "### Critical Issues (1)\n- boom") }); + const reviews = order === "approval first" ? [approval, blocker] : [blocker, approval]; + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews }); + assert.deepEqual(violations.filter((v) => v.startsWith("I1")), []); + assert.deepEqual(violations.filter((v) => v.startsWith("I2a")), []); + assert.match( + violations.find((v) => v.startsWith("I2e")) ?? "", + new RegExp(`^I2e PR #1220 @ff1c72db: Ally App APPROVED \\(${DUPLICATE_IDS[0]}\\) coexists with a different blocking Ally App review \\(${DUPLICATE_IDS[1]}\\)`), + ); + }); + } + + it("does not fire I2e once the stale approval is dismissed", () => { + const approval = appReview({ id: DUPLICATE_IDS[0], state: "DISMISSED", body: canonicalBody(HEAD, "clean") }); + const blocker = appReview({ id: DUPLICATE_IDS[1], state: "COMMENTED", body: canonicalBody(HEAD, "### Critical Issues (1)\n- boom") }); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [approval, blocker] }); + assert.deepEqual(violations.filter((v) => v.startsWith("I2e")), []); + }); + + // The other order: a COMMENTED blocker cannot be dismissed, so a clean + // re-review that supersedes it at an unchanged head needs its own exit. + const blockerAtHead = () => + appReview({ id: DUPLICATE_IDS[0], state: "COMMENTED", submitted_at: "2026-09-23T10:00:00Z", body: canonicalBody(HEAD, "### Critical Issues (0)\n### Important Issues (1)\n- wrong PR description") }); + const approvalWithLedger = (ledger) => + appReview({ + id: DUPLICATE_IDS[1], + state: "APPROVED", + submitted_at: "2026-09-23T12:00:00Z", + body: canonicalBody(HEAD, `### Prior Findings Dispositioned (1)\n${ledger}\n### Critical Issues (0)\n### Important Issues (0)`), + }); + + it("does not fire I2e when the approval retires, by name, the finding raised at this head", () => { + const approval = approvalWithLedger(`- **prior:${HEAD.slice(0, 7)} important 1** — fixed — description corrected`); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [blockerAtHead(), approval] }); + assert.deepEqual(violations, []); + }); + + for (const [why, ledger] of [ + // #876 and #1220: both racing reviews carried a ledger for an earlier head. + ["retires a finding raised at an earlier head", `- **prior:${OTHER.slice(0, 7)} important 1** — fixed — gone`], + ["names this head with an unrecognized verb", `- **prior:${HEAD.slice(0, 7)} important 1** — superseded — gone`], + ["quotes a same-head entry as indented code", ` - **prior:${HEAD.slice(0, 7)} important 1** — fixed — gone`], + ]) { + it(`still fires I2e when the approval only ${why}`, () => { + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [blockerAtHead(), approvalWithLedger(ledger)] }); + assert.match(violations.find((v) => v.startsWith("I2e")) ?? "", new RegExp(`APPROVED \\(${DUPLICATE_IDS[1]}\\)`)); + }); + } + + it("still fires I2e for a blocker submitted after the approval that retired its predecessor", () => { + // Dismissing the approval is the exit in this order, so the ledger buys nothing. + const approval = approvalWithLedger(`- **prior:${HEAD.slice(0, 7)} important 1** — fixed — description corrected`); + const laterBlocker = appReview({ id: 5124950999, state: "COMMENTED", submitted_at: "2026-09-23T13:00:00Z", body: canonicalBody(HEAD, "### Critical Issues (1)\n- new") }); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [blockerAtHead(), approval, laterBlocker] }); + assert.match(violations.find((v) => v.startsWith("I2e")) ?? "", new RegExp(`APPROVED \\(${DUPLICATE_IDS[1]}\\)`)); + }); + + it("still fires I2e for a later blocker whose findings the approval's ledger happens to name", () => { + // Coverage alone would exempt this: the approval retired `important 1` + // against the EARLIER blocker, and the later one raises `important 1` too. + // An approval cannot have read a blocker submitted after it, so order — + // not coverage — is what keeps this fatal. + const approval = approvalWithLedger(`- **prior:${HEAD.slice(0, 7)} important 1** — fixed — description corrected`); + const laterBlocker = appReview({ id: 5124950999, state: "COMMENTED", submitted_at: "2026-09-23T13:00:00Z", body: canonicalBody(HEAD, "### Critical Issues (0)\n### Important Issues (1)\n- raised after the approval") }); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [blockerAtHead(), approval, laterBlocker] }); + assert.match(violations.find((v) => v.startsWith("I2e")) ?? "", new RegExp(`APPROVED \\(${DUPLICATE_IDS[1]}\\)`)); + }); + + // The exemption is coverage, not presence: an approval retiring 1 of the N + // findings its blocker raised would otherwise stand green over the other N-1. + const multiFindingBlocker = () => + appReview({ id: DUPLICATE_IDS[0], state: "COMMENTED", submitted_at: "2026-09-23T10:00:00Z", body: canonicalBody(HEAD, "### Critical Issues (0)\n### Important Issues (2)\n- one\n- two") }); + + it("still fires I2e when the approval retires only some of the findings raised at this head", () => { + const approval = approvalWithLedger(`- **prior:${HEAD.slice(0, 7)} important 1** — fixed — one done`); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [multiFindingBlocker(), approval] }); + assert.match(violations.find((v) => v.startsWith("I2e")) ?? "", new RegExp(`APPROVED \\(${DUPLICATE_IDS[1]}\\)`)); + }); + + it("does not fire I2e when the approval retires every finding raised at this head", () => { + const approval = approvalWithLedger( + `- **prior:${HEAD.slice(0, 7)} important 1** — fixed — one done\n- **prior:${HEAD.slice(0, 7)} important 2** — no-longer-applicable — two moot`, + ); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [multiFindingBlocker(), approval] }); + assert.deepEqual(violations, []); + }); + + it("counts a bucket by the number after its heading, not a later parenthesized one", () => { + // The merge gate reads the count straight after `Issues`, so this heading + // raises two findings. A greedy capture would read `(1)` and let one + // retirement stand as full coverage. + const blocker = appReview({ id: DUPLICATE_IDS[0], state: "COMMENTED", submitted_at: "2026-09-23T10:00:00Z", body: canonicalBody(HEAD, "### Critical Issues (0)\n### Important Issues (2), was (1)\n- one\n- two") }); + const approval = approvalWithLedger(`- **prior:${HEAD.slice(0, 7)} important 1** - fixed - one done`); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [blocker, approval] }); + assert.match(violations.find((v) => v.startsWith("I2e")) ?? "", new RegExp(`APPROVED \\(${DUPLICATE_IDS[1]}\\)`)); + }); + + it("still fires I2e when a second, emphasised bucket raises more than the heading", () => { + // The merge gate reads `**Important Issues (3)**` as a bucket too and keeps + // the higher count, so this blocker raised three findings, not one. + const blocker = appReview({ id: DUPLICATE_IDS[0], state: "COMMENTED", submitted_at: "2026-09-23T10:00:00Z", body: canonicalBody(HEAD, "### Critical Issues (0)\n### Important Issues (1)\n- one\n**Important Issues (3)**\n- two\n- three") }); + const approval = approvalWithLedger(`- **prior:${HEAD.slice(0, 7)} important 1** — fixed — one done`); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [blocker, approval] }); + assert.match(violations.find((v) => v.startsWith("I2e")) ?? "", new RegExp(`APPROVED \\(${DUPLICATE_IDS[1]}\\)`)); + }); + + it("still fires I2e when the blocker blocks only on a still-present entry", () => { + // No counted bucket at this head, so no (severity, index) a ledger can name. + const blocker = appReview({ id: DUPLICATE_IDS[0], state: "COMMENTED", submitted_at: "2026-09-23T10:00:00Z", body: canonicalBody(HEAD, `- **prior:${OTHER.slice(0, 7)} important 1** — still-present — stands`) }); + const approval = approvalWithLedger(`- **prior:${HEAD.slice(0, 7)} important 1** — fixed — done`); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [blocker, approval] }); + assert.match(violations.find((v) => v.startsWith("I2e")) ?? "", new RegExp(`APPROVED \\(${DUPLICATE_IDS[1]}\\)`)); + }); + + // The contract mirrors a still-standing finding into the counted bucket under + // its ORIGINAL id. Keying that slot on the earlier head's name would reopen the + // #876 / #1220 race, since both racing runs can name an earlier head's finding, + // so the slot keeps its position at this head: retiring it by the earlier name + // alone stays fatal (a known false red), and a this-head name still clears it. + const mirroredBlocker = () => + appReview({ + id: DUPLICATE_IDS[0], + state: "COMMENTED", + submitted_at: "2026-09-23T10:00:00Z", + body: canonicalBody( + HEAD, + `### Prior Findings Dispositioned (1)\n- **prior:${OTHER.slice(0, 7)} important 1** — still-present — stands\n### Critical Issues (0)\n### Important Issues (1)\n- **prior:${OTHER.slice(0, 7)} important 1** — stands`, + ), + }); + + it("still fires I2e when the approval retires a mirrored finding only by its earlier-head name", () => { + const approval = approvalWithLedger(`- **prior:${OTHER.slice(0, 7)} important 1** — fixed — gone`); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [mirroredBlocker(), approval] }); + assert.match(violations.find((v) => v.startsWith("I2e")) ?? "", new RegExp(`APPROVED \\(${DUPLICATE_IDS[1]}\\)`)); + }); + + it("does not fire I2e when the approval retires a mirrored finding by its position at this head", () => { + const approval = approvalWithLedger(`- **prior:${HEAD.slice(0, 7)} important 1** — fixed — gone`); + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [mirroredBlocker(), approval] }); + assert.deepEqual(violations.filter((v) => v.startsWith("I2e")), []); + }); + + // Shapes the merge gate's PRIOR_FINDING_DISPOSITION_PATTERN accepts. Reading + // either as non-retiring here would fail a supersession the gate allows. + for (const [shape, ledger] of [ + ["an en dash separator", `- **prior:${HEAD.slice(0, 7)} important 1** – fixed – description corrected`], + ["a space after the opening `**`", `- ** prior:${HEAD.slice(0, 7)} important 1** — fixed — description corrected`], + ]) { + it(`does not fire I2e when the retiring entry uses ${shape}`, () => { + const violations = findPrViolations({ number: 1220, headSha: HEAD, reviews: [blockerAtHead(), approvalWithLedger(ledger)] }); + assert.deepEqual(violations, []); + }); + } + + it("names the latest by id when two reviews share a submitted_at second", () => { + const at = (id) => appReview({ id, state: "COMMENTED", submitted_at: "2026-09-06T09:35:17Z", body: canonicalBody(HEAD, `pass ${id}`) }); + for (const reviews of [[at(DUPLICATE_IDS[0]), at(DUPLICATE_IDS[1])], [at(DUPLICATE_IDS[1]), at(DUPLICATE_IDS[0])]]) { + assert.match(findPrNotices({ number: 1220, headSha: HEAD, reviews })[0], new RegExp(`the latest \\(${DUPLICATE_IDS[1]},`)); + } }); it("omits the clause rather than guessing when a body is empty", () => { @@ -636,17 +875,91 @@ describe("I1 names the mechanism a same-lane duplicate implies", () => { const identical = findPrViolations(duplicatePr([canonicalBody(), canonicalBody()])).find((v) => v.startsWith("I1"), ); - const differing = findPrViolations( - duplicatePr([canonicalBody(HEAD, "a"), canonicalBody(HEAD, "b")]), - ).find((v) => v.startsWith("I1")); const bodiless = findPrViolations(duplicatePr(["", ""])).find((v) => v.startsWith("I1")); assert.equal(violationFingerprint(identical), "I1:1220:ff1c72db:5124949902,5124950225"); - assert.equal(violationFingerprint(identical), violationFingerprint(differing)); assert.equal(violationFingerprint(identical), violationFingerprint(bodiless)); }); }); +describe("countedFindingKeys", () => { + // Must agree with the merge gate on what a review raised, or supersedesBlocker + // can exempt an approval the gate would still hold (BLO-25764). Each shape is + // one where the two used to disagree, plus controls on either side. + const gateKeys = (body) => + new Set((extractAllyReportedFindingRefs(body) ?? []).map(({ severity, index }) => `${severity} ${index}`)); + const upTo = (n) => new Set(Array.from({ length: n }, (_, i) => `important ${i + 1}`)); + + for (const [shape, body, expected] of [ + ["a bold bucket beside a smaller heading bucket", "### Important Issues (1)\n**Important Issues (3)**", 3], + ["a blockquoted bucket beside a smaller heading bucket", "### Important Issues (1)\n> Important Issues (4)", 4], + ["a list-marker bucket beside a smaller heading bucket", "### Important Issues (1)\n- Important Issues (5)", 5], + ["a heading without the word Issues", "### Important findings (2)", 0], + ["a bucket heading indented one space", " ### Important Issues (2)", 2], + ["a bucket heading indented three spaces", " ### Important Issues (2)", 2], + ["a bucket heading indented four spaces (code)", " ### Important Issues (2)", 0], + ["a bucket inside a closed fence", "```\n### Important Issues (2)\n```", 2], + ["a bucket after an unclosed fence", "```\n### Important Issues (2)", 2], + ]) { + it(`matches the merge gate on ${shape}`, () => { + assert.deepEqual(gateKeys(body), upTo(expected), "gate fixture drifted"); + assert.deepEqual(countedFindingKeys(body), upTo(expected)); + }); + } +}); + +describe("retiredFindingKeys", () => { + // Must agree with the merge gate on which ledger entries retire a finding, or + // supersedesBlocker can exempt an approval the gate would still hold + // (BLO-25764). A hyphenated verb that only begins with a retiring one is the + // shape the two used to disagree on. + const PRIOR = "e3e84e2644aacaf68a0ad61eeb513882b7ee35b3"; + const gateKeys = (body) => + new Set( + extractAllyPriorFindingDispositions(body) + .filter(({ shortSha, kind }) => kind === "retires" && PRIOR.startsWith(shortSha)) + .map(({ severity, index }) => `${severity} ${index}`), + ); + const entry = (verb, separator = "—") => + `### Prior Findings Dispositioned (1)\n- **prior:e3e84e2 important 1** ${separator} ${verb} ${separator} reason`; + + const line = entry("fixed").split("\n")[1]; + for (const [shape, body, retires] of [ + ["a plain entry (control)", entry("fixed"), true], + ["an entry inside a closed backtick fence", `\`\`\`\n${line}\n\`\`\``, false], + ["an entry inside a closed tilde fence", `~~~\n${line}\n~~~`, false], + ["an entry inside an md-tagged fence", `\`\`\`md\n${line}\n\`\`\``, false], + ["an entry after an unclosed fence", `\`\`\`\n${line}`, false], + ["an entry after a closed fence (control)", `\`\`\`\nx\n\`\`\`\n${line}`, true], + ]) { + it(`matches the merge gate on ${shape}`, () => { + const expected = retires ? new Set(["important 1"]) : new Set(); + assert.deepEqual(gateKeys(body), expected, "gate fixture drifted"); + assert.deepEqual(retiredFindingKeys(body, PRIOR), expected); + }); + } + + for (const separator of ["—", "–", "-"]) { + for (const [verb, retires] of [ + ["fixed", true], + ["no-longer-applicable", true], + ["fixed-upstream", false], + ["no-longer-applicable-here", false], + ["still-present", false], + ["deferred", false], + ["partially-fixed", false], + ["fixedx", false], + ]) { + it(`matches the merge gate on \`${verb}\` with a ${JSON.stringify(separator)} separator`, () => { + const expected = retires ? new Set(["important 1"]) : new Set(); + const body = entry(verb, separator); + assert.deepEqual(gateKeys(body), expected, "gate fixture drifted"); + assert.deepEqual(retiredFindingKeys(body, PRIOR), expected); + }); + } + } +}); + describe("duplicateBodyAcrossIdentities", () => { const at = (id, uid, body) => ({ id, user: { id: uid }, body });