diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index bcf2dc9768ef..352803fb41a4 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -77,12 +77,49 @@ const CANONICAL_REVIEW_HEADING_RE = /^## Ally — Consolidated PR Review[ \t]*$/ const BLOCKING_SECTION_RE = /^#+[ \t]*(critical|important)[^\n]*\((?!0\))\d+\)/im; +/** + * Leading whitespace that CommonMark would render as an indented code block, + * i.e. quoted text rather than emitted structure. Four spaces reach column + * four, and so does a tab however few spaces precede it. + * + * Must stay equivalent to NOT_INDENTED_CODE in + * server/src/services/ally-review-detection.ts. BLO-31730 is a bug about two + * parsers disagreeing on this exact line, so the auditor and the merge gate + * must not disagree about which indentation counts. + * + * The two constants are deliberately not byte-identical, so compare the + * *composed* forms rather than these lines. The module's is the bare pair of + * lookaheads and each of its three use sites appends its own ` {0,3}`; this + * one folds that quantifier in, because both of its use sites want it. What + * must match is the composition — `(?! *\t)(?! {4}) {0,3}` on either side. A + * future edit that reads this as a literal-identity claim and "restores" it + * by deleting the ` {0,3}` here would silently stop allowing the up-to-three + * spaces CommonMark still treats as a paragraph, which is the divergence this + * 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. + */ +const NOT_INDENTED_CODE = String.raw`(?! *\t)(?! {4}) {0,3}`; + /** A prior-finding disposition that says the blocker is still present. */ -const STILL_PRESENT_DISPOSITION_RE = - /^[ \t]*-[ \t]*\*\*prior:[^\n]*\*\*[ \t]*(?:—|-)[ \t]*still-present[ \t]*(?:—|-)/im; +const STILL_PRESENT_DISPOSITION_RE = new RegExp( + String.raw`^${NOT_INDENTED_CODE}-[ \t]*\*\*prior:[^\n]*\*\*[ \t]*(?:—|-)[ \t]*still-present[ \t]*(?:—|-)`, + "im", +); /** The single standalone attestation line Ally is required to emit. */ -const ATTESTED_HEAD_RE = /^[ \t]*(?:[_*]+)?[ \t]*reviewed head:[ \t]*`?([0-9a-f]{40})`?[ \t]*(?:[_*]+)?[ \t]*$/im; +const ATTESTED_HEAD_RE = new RegExp( + String.raw`^${NOT_INDENTED_CODE}(?:[_*]+)?[ \t]*reviewed head:[ \t]*\`?([0-9a-f]{40})\`?[ \t]*(?:[_*]+)?[ \t]*$`, + "im", +); const ATTESTED_HEAD_GLOBAL_RE = new RegExp(ATTESTED_HEAD_RE.source, "gim"); const ALLY_REVIEW_LANES = ["app", "seat"]; diff --git a/server/src/__tests__/pr-comment-review-gate.test.ts b/server/src/__tests__/pr-comment-review-gate.test.ts index 9b116e3d44eb..9dbe86f47f0e 100644 --- a/server/src/__tests__/pr-comment-review-gate.test.ts +++ b/server/src/__tests__/pr-comment-review-gate.test.ts @@ -4,6 +4,13 @@ import { describe, expect, it } from "vitest"; // the retirement description is checked against the real thing, not a copy. import { admitsNothingEvaluated } from "../../../scripts/check-comment-review-gate-census.mjs"; +import { + extractAllyPriorFindingDispositions, + extractAllyReportedFindingRefs, + extractAllyReviewedHeadSha, + hasActionablePrReviewFeedback, + hasAllyConsolidatedReviewHeading, +} from "../services/ally-review-detection.js"; import { commentReviewGateRetirementDescription, commentReviewGateRetirementStatus, @@ -632,6 +639,267 @@ describe("evaluateCommentReviewGate", () => { }); }); +/** + * Quoting a review must never be mistaken for emitting one. + * + * The gate's only identity check is the author login, and every agent in the + * fleet comments as that same App. So before this suite existed, an agent + * pasting the review it was replying to published a merge-visible verdict + * about a head nothing had examined — in both directions. + */ +describe("evaluateCommentReviewGate — quoted review bodies", () => { + const fenced = (body: string, info = ""): string => + ["Quoting the review I am replying to:", "", `\`\`\`${info}`, body, "```", "", "Nothing addressed yet."].join("\n"); + + it("does not let a fenced paste of a clean review attest the head", () => { + const verdict = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(fenced(cleanReview(CURRENT_HEAD)), "2026-09-05T00:00:00Z")], + }); + + // Not merely "not clean": `clean` is the one outcome that asserts positive + // evidence of review, which is exactly what a quote is not. + expect(verdict).toMatchObject({ state: "success", outcome: "not_evaluated" }); + }); + + it("does not let a fenced paste of a finding redden a head Ally never reviewed", () => { + const verdict = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(fenced(blockingReview(CURRENT_HEAD), "markdown"), "2026-09-05T00:00:00Z")], + }); + + expect(verdict).toMatchObject({ state: "success", outcome: "not_evaluated" }); + }); + + it("does not let a quoted ledger entry retire a live finding", () => { + const verdict = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [ + allyComment(blockingReview(OLD_HEAD), "2026-09-05T00:00:00Z"), + allyComment(fenced(dispositioningReview(CURRENT_HEAD, OLD_HEAD, "fixed")), "2026-09-05T01:00:00Z"), + ], + }); + + expect(verdict).toMatchObject({ state: "failure", outcome: "carried_finding" }); + }); + + it("still reads a genuine review that itself contains a fenced code block", () => { + const withSuggestion = reviewBody(CURRENT_HEAD, [ + "### Critical Issues (0)", + "### Important Issues (1)", + "- Prefer the guarded form:", + "```ts", + "if (!ok) return;", + "```", + "### Recommended Action", + "Fix the guard before merge.", + ]); + + const verdict = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(withSuggestion, "2026-09-05T00:00:00Z")], + }); + + expect(verdict).toMatchObject({ state: "failure", outcome: "blocking_finding" }); + }); + + it("keeps a finding visible when an unbalanced fence would blank the rest of the body", () => { + // Fail-closed guard: hasActionablePrReviewFeedback reads the raw body too, + // so a malformed fence cannot silently clear a PR. + const unbalanced = reviewBody(CURRENT_HEAD, [ + "### Critical Issues (0)", + "```ts", + "const oops = true;", + "### Important Issues (1)", + "- The unterminated fence above swallows this line when rendered.", + ]); + + expect(hasActionablePrReviewFeedback(unbalanced)).toBe(true); + }); + + /** + * The unit assertion above passes while the gate still goes green, because + * detecting a finding and enumerating which findings exist are separate + * predicates. Enumerating from fence-stripped text alone dropped the bucket + * that followed an unbalanced fence, so retiring the surviving one retired + * the whole head — a silent green with a live finding on it. + */ + it("does not drop a finding bucket that an unbalanced fence swallows", () => { + const swallowed = reviewBody(OLD_HEAD, [ + "### Critical Issues (1)", + "- **[code]** the terminator is missing.", + "```ts", + "const unterminated = true;", + "### Important Issues (1)", + "- **[code]** this bucket follows the unbalanced fence.", + ]); + + // Both buckets are enumerated, so a ledger must name both to retire the head. + expect(extractAllyReportedFindingRefs(swallowed)).toEqual([ + { severity: "critical", index: 1 }, + { severity: "important", index: 1 }, + ]); + + const retiresOnlyTheFirst = reviewBody(INTERMEDIATE_HEAD, [ + "### Prior Findings Dispositioned (1)", + `- **prior:${OLD_HEAD.slice(0, 7)} critical 1** — fixed — the terminator is back.`, + "### Critical Issues (0)", + "### Important Issues (0)", + ]); + + const verdict = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [ + allyComment(swallowed, "2026-09-05T00:00:00Z"), + allyComment(retiresOnlyTheFirst, "2026-09-05T01:00:00Z"), + ], + }); + + expect(verdict).toMatchObject({ state: "failure", outcome: "carried_finding" }); + }); + + it("does not let a 4-space-indented ledger entry retire a live finding", () => { + // Indentation is the other way to quote a ledger, and stripping fenced + // spans alone left it readable as emitted structure. + const quotesLedgerByIndent = reviewBody(CURRENT_HEAD, [ + "The earlier review's ledger read:", + "", + ` - **prior:${OLD_HEAD.slice(0, 7)} important 1** — fixed — re-checked.`, + "", + "### Critical Issues (0)", + "### Important Issues (0)", + ]); + + expect(extractAllyPriorFindingDispositions(quotesLedgerByIndent)).toEqual([]); + + const verdict = evaluateCommentReviewGate({ + headSha: INTERMEDIATE_HEAD, + comments: [ + allyComment(blockingReview(OLD_HEAD), "2026-09-05T00:00:00Z"), + allyComment(quotesLedgerByIndent, "2026-09-05T01:00:00Z"), + ], + }); + + expect(verdict).toMatchObject({ state: "failure", outcome: "carried_finding" }); + }); + + it("still reads the unindented ledger entry Ally actually emits", () => { + // The guard above must not cost a real retirement: every ledger entry in + // the sampled corpus is unindented. + expect( + extractAllyPriorFindingDispositions(dispositioningReview(CURRENT_HEAD, OLD_HEAD, "fixed")), + ).toMatchObject([{ shortSha: OLD_HEAD.slice(0, 7), disposition: "fixed", kind: "retires" }]); + }); + + it("ignores a 4-space-indented paste, which the heading and attestation once disagreed about", () => { + const indented = [ + "For reference, the earlier review said:", + "", + " ## Ally — Consolidated PR Review", + ` Reviewed head: ${CURRENT_HEAD}`, + " ### Critical Issues (0)", + " ### Important Issues (0)", + ].join("\n"); + + expect(extractAllyReviewedHeadSha(indented)).toBeNull(); + expect( + evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(indented, "2026-09-05T00:00:00Z")], + }), + ).toMatchObject({ state: "success", outcome: "not_evaluated" }); + }); +}); + +/** + * Ally wraps the attested SHA in whatever emphasis it happens to choose. The + * suite previously built every fixture with a bare SHA, so it asserted the + * parser correct only on the one shape it already handled (BLO-31730). + */ +describe("extractAllyReviewedHeadSha — attestation delimiters", () => { + // Verbatim from paperclip#1637's `cce8d6b0` review, the review whose + // invisibility carried a resolved finding forward against a dead head. + it("parses the backticked form that made a real review invisible", () => { + expect( + extractAllyReviewedHeadSha( + ["## Ally — Consolidated PR Review", `Reviewed head: \`${CURRENT_HEAD}\``, ""].join("\n"), + ), + ).toBe(CURRENT_HEAD); + }); + + it.each([ + ["bare", `Reviewed head: ${CURRENT_HEAD}`], + ["backticked sha", `Reviewed head: \`${CURRENT_HEAD}\``], + ["bold sha", `Reviewed head: **${CURRENT_HEAD}**`], + ["bold and backticked", `Reviewed head: **\`${CURRENT_HEAD}\`**`], + ["italicized line", `_Reviewed head: ${CURRENT_HEAD}_`], + ])("accepts the %s attestation", (_label, line) => { + expect(extractAllyReviewedHeadSha(`## Ally — Consolidated PR Review\n${line}\n`)).toBe(CURRENT_HEAD); + }); + + it("preserves the ambiguity guard that keeps a check from being set on a guess", () => { + expect( + extractAllyReviewedHeadSha( + [`Reviewed head: \`${CURRENT_HEAD}\``, `Reviewed head: **${OLD_HEAD}**`].join("\n"), + ), + ).toBeNull(); + expect(extractAllyReviewedHeadSha("## Ally — Consolidated PR Review\nno attestation\n")).toBeNull(); + }); + + it("does not treat a mid-line prose mention as an attestation", () => { + expect(extractAllyReviewedHeadSha(`The status says Reviewed head: ${CURRENT_HEAD} which is stale.`)).toBeNull(); + }); + + it("does not treat a fenced SHA as an attestation", () => { + expect( + extractAllyReviewedHeadSha(["```", `Reviewed head: ${CURRENT_HEAD}`, "```"].join("\n")), + ).toBeNull(); + }); + + /** + * A tab advances to the next four-column stop, so it starts an indented code + * block however few spaces precede it. The heading pattern already rejected + * this shape; the attestation accepted it, which is the same two-parsers + * disagreement in miniature. + */ + it.each([ + ["four spaces", " "], + ["a tab", "\t"], + ["spaces then a tab", " \t"], + ])("rejects an attestation indented by %s", (_label, indent) => { + expect(extractAllyReviewedHeadSha(`context\n${indent}Reviewed head: ${CURRENT_HEAD}\n`)).toBeNull(); + }); + + it("still accepts the up-to-three-space indentation Markdown treats as a paragraph", () => { + expect(extractAllyReviewedHeadSha(`context\n Reviewed head: ${CURRENT_HEAD}\n`)).toBe(CURRENT_HEAD); + }); + + /** + * The converse of the quoting tests above, and the one direction that fails + * open: blanking can also remove a *genuine* attestation. A review whose + * attestation is swallowed attests no head, so it is never an attesting + * comment and its findings go untracked. + * + * This is the residual the module header accepts rather than closes — Ally's + * template leaves nothing fenceable above these lines, so reaching it needs a + * malformed body. Pinning it keeps the residual executable instead of merely + * described, and fails loudly if the attestation is ever moved below a + * fenceable region. + */ + it.each([ + ["an unbalanced backtick fence", "```ts"], + ["a stray tilde fence", "~~~ts"], + ])("loses a genuine attestation to %s above it", (_label, fence) => { + const body = ["## Ally — Consolidated PR Review", fence, "", `Reviewed head: ${CURRENT_HEAD}`].join( + "\n", + ); + expect(extractAllyReviewedHeadSha(body)).toBeNull(); + // The body still reads as an actionable Ally review — only the attestation + // is lost, which is precisely what makes this direction fail open. + expect(hasAllyConsolidatedReviewHeading(body)).toBe(true); + }); +}); + // BLO-29711 AC#1. The deployed context moved out of the `review/` namespace so // a green can no longer be misread as review evidence. Because commit statuses // cannot be deleted, the pre-rename rows have to be superseded in place. diff --git a/server/src/services/ally-review-detection.ts b/server/src/services/ally-review-detection.ts index ef8913f3c849..0896eed1aecb 100644 --- a/server/src/services/ally-review-detection.ts +++ b/server/src/services/ally-review-detection.ts @@ -5,28 +5,148 @@ * the comment-review gate uses the same parsing to publish a merge-visible * status. Keeping the detection in one dependency-free module avoids subtle * differences in which comments wake an author versus block a PR. + * + * The distinction between *emitted* review structure and *quoted* text is + * load-bearing because the only identity check upstream is the author login, + * and every agent in the fleet comments as that same App + * (`allyblockcast[bot]`). So an agent quoting a review it is replying to was + * previously indistinguishable from Ally emitting that review: the quote set + * the merge-visible status, in either direction. + * + * The invariant is *not* that every predicate ignores quoted text, and it is + * not unconditional. It is directional, and it holds only *once a review has + * been recognised*: + * + * Quoted text may never *reduce* what the gate blocks on; only emitted text + * may retire a finding. + * + * That asymmetry decides which side each predicate reads, and the three groups + * are not interchangeable: + * + * - Retiring (extractAllyPriorFindingDispositions) reads only emitted text. + * A quote that reached it would clear a live finding. + * - Detecting and enumerating findings (hasActionablePrReviewFeedback, + * extractAllyReportedFindingRefs) read emitted *and* raw text and keep + * whichever blocks more. Ignoring quotes there would fail open, because an + * unbalanced fence blanks the rest of the body and would drop the findings + * after it. + * - Deciding whether a review exists at all (hasAllyConsolidatedReviewHeading, + * extractAllyReviewedHeadSha) reads only emitted text — and here blanking + * fails *open*, not closed. A review whose heading or attestation is + * swallowed attests no head, so it is never an attesting comment and its + * findings are never tracked: the gate reaches not_evaluated instead of + * blocking, even though the body still reads as actionable. + * + * So for the first two groups quoted text costs at most a false red, which is + * visible and recoverable. For the third it can cost a false green. That + * direction is accepted rather than closed, for two reasons: Ally's template + * puts the heading and `Reviewed head:` in the opening lines with nothing + * fenceable above them, so reaching it needs Ally to emit a malformed body; + * and the resulting state is the already-known fail-open that + * commentReviewGateVerdictIsMisreadable (pr-comment-review-gate.ts) reports + * under BLO-29711, not a novel silent green. A discriminator does exist — an + * emitted heading with no emitted attestation but exactly one raw attestation + * separates a malformed genuine review from a fenced paste, which blanks both + * lines together — but it is subtle enough to become its own footgun, so it is + * deliberately not used. + * + * The rule for a predicate added later: emitted-only is the safe default for + * anything that retires or dispositions, and the *wrong* default for anything + * that decides whether a review is recognised at all. */ +// A fenced span is quoted content, not emitted structure. Blank the lines +// rather than deleting them so line geometry is preserved exactly: every +// pattern below is line-anchored, and hasNonNegatedMatch's lookback walks +// back to the previous newline, so collapsing lines here would silently +// re-point those anchors at unrelated text. +const FENCE_DELIMITER_PATTERN = /^ {0,3}(`{3,}|~{3,})(.*)$/; +const FENCE_CLOSE_PATTERN = /^ {0,3}(`{3,}|~{3,})[ \t]*$/; + +function withoutFencedCodeBlocks(body: string): string { + if (!body.includes("```") && !body.includes("~~~")) return body; + const lines = body.split("\n"); + let open: { char: string; length: number } | null = 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 itself contain a + // backtick. Honoring that keeps an inline span from opening a phantom + // fence that would blank the rest of a genuine review. + if (fence && !(fence[1]![0] === "`" && fence[2]!.includes("`"))) { + open = { char: fence[1]![0]!, length: fence[1]!.length }; + lines[i] = ""; + } + } + // An unclosed fence blanks to end of body, matching how GitHub renders it. + return lines.join("\n"); +} + +/** Review text with quoted (fenced) spans removed, or null for a non-string. */ +function emittedReviewText(body: string | null | undefined): string | null { + return typeof body === "string" ? withoutFencedCodeBlocks(body) : null; +} + +// CommonMark starts an indented code block at four columns, and a tab always +// advances to the next multiple of four — so a tab anywhere in the leading run +// reaches column four regardless of how few spaces precede it. Both shapes are +// rejected by one lookahead, shared by every line-anchored pattern below, +// because two of them disagreeing about what counts as code is exactly how a +// 4-space paste attested a head while not registering as a review at all. The +// bound is a lookahead rather than a counted run because the emphasis and +// spacing that follow would otherwise absorb the fourth space and re-open the +// hole. +const NOT_INDENTED_CODE = String.raw`(?! *\t)(?! {4})`; + // Ally's own output has this heading on its own line, as a Markdown heading // or bold run — optionally indented up to three spaces (still a paragraph, // rather than a code block). A prose mention or quoted heading must not count // as the review itself. -const ALLY_CONSOLIDATED_REVIEW_HEADING_PATTERN = - /^[ \t]{0,3}(?:#{1,6}[ \t]+|\*\*[ \t]*)?Ally[ \t]*(?:—|–|-|:)[ \t]*Consolidated[ \t]+PR[ \t]+Review\b/im; +const ALLY_CONSOLIDATED_REVIEW_HEADING_PATTERN = new RegExp( + String.raw`^${NOT_INDENTED_CODE} {0,3}(?:#{1,6}[ \t]+|\*\*[ \t]*)?Ally[ \t]*(?:—|–|-|:)[ \t]*Consolidated[ \t]+PR[ \t]+Review\b`, + "im", +); export function hasAllyConsolidatedReviewHeading(body: string | null | undefined): boolean { - return typeof body === "string" && ALLY_CONSOLIDATED_REVIEW_HEADING_PATTERN.test(body); + const text = emittedReviewText(body); + return text !== null && ALLY_CONSOLIDATED_REVIEW_HEADING_PATTERN.test(text); } // A comment-shaped review attests to the exact head it examined. Require one // complete standalone SHA: an absent or ambiguous attestation must not be // guessed at when a required check is being set. -const REVIEWED_HEAD_ATTESTATION_PATTERN = /(?:^|\n)\s*_?\s*reviewed head:\s*([0-9a-f]{40})\s*_?\s*(?=\n|$)/gi; +// +// Ally wraps this line's SHA in whatever emphasis it happens to choose, and a +// strict bare-SHA match made an entire real review invisible when it chose +// backticks (BLO-31730) — the review that *resolved* a finding, so the finding +// carried forward against a head that no longer existed and could never be +// re-reviewed. Delimiters are matched as an unbalanced run rather than as +// pairs: what protects a required check from being set on a guess is the +// exactly-one rule below, not delimiter symmetry, and demanding symmetry only +// reintroduces the brittleness this is widening away from. +const MARKDOWN_EMPHASIS_RUN = "[*_`]{0,3}"; + +// Indentation is bounded to agree with the heading pattern above — see +// NOT_INDENTED_CODE. +const REVIEWED_HEAD_ATTESTATION_PATTERN = new RegExp( + `(?:^|\\n)${NOT_INDENTED_CODE} {0,3}${MARKDOWN_EMPHASIS_RUN}[ \\t]{0,3}reviewed head:[ \\t]*` + + `${MARKDOWN_EMPHASIS_RUN}([0-9a-f]{40})${MARKDOWN_EMPHASIS_RUN}[ \\t]*` + + `${MARKDOWN_EMPHASIS_RUN}[ \\t]*(?=\\n|$)`, + "gi", +); export function extractAllyReviewedHeadSha(body: string | null | undefined): string | null { - if (typeof body !== "string") return null; + const text = emittedReviewText(body); + if (text === null) return null; const attestations = Array.from( - body.matchAll(REVIEWED_HEAD_ATTESTATION_PATTERN), + text.matchAll(REVIEWED_HEAD_ATTESTATION_PATTERN), (match) => match[1]!.toLowerCase(), ); return attestations.length === 1 ? attestations[0]! : null; @@ -70,8 +190,18 @@ function hasNonNegatedMatch(text: string, pattern: RegExp): boolean { // `[prior:...]` references inside a Critical/Important bucket use bracket // syntax and deliberately do not match: those are open findings, not // dispositions. -const PRIOR_FINDING_DISPOSITION_PATTERN = - /^[ \t]*-[ \t]*\*\*[ \t]*prior:([0-9a-f]{7,40})[ \t]+([a-z]+)[ \t]+(\d+)[ \t]*\*\*[ \t]*(?:—|–|-)[ \t]*([a-z][a-z-]*)[ \t]*(?:—|–|-)/gim; +// +// Indentation is bounded like the heading and attestation patterns. Stripping +// fenced spans alone left this predicate reading a 4-space-indented paste as +// emitted structure, so quoting an earlier ledger retired a live finding — +// fail-open, and the one direction this module must not fail in. All 90 +// ledger entries across the 40 most recent PRs' Ally reviews are unindented, +// so the bound excludes no observed real entry; and an entry it did exclude +// would leave a visible red rather than a silent green. +const PRIOR_FINDING_DISPOSITION_PATTERN = new RegExp( + String.raw`^${NOT_INDENTED_CODE} {0,3}-[ \t]*\*\*[ \t]*prior:([0-9a-f]{7,40})[ \t]+([a-z]+)[ \t]+(\d+)[ \t]*\*\*[ \t]*(?:—|–|-)[ \t]*([a-z][a-z-]*)[ \t]*(?:—|–|-)`, + "gim", +); // The counted finding buckets a review reports, e.g. `### Important Issues (2)`. // Ally numbers findings within a bucket from 1, and its ledger entries name @@ -153,9 +283,10 @@ export function classifyPriorDisposition(disposition: string): PriorDispositionK export function extractAllyPriorFindingDispositions( body: string | null | undefined, ): AllyPriorFindingDisposition[] { - if (typeof body !== "string") return []; + const text = emittedReviewText(body); + if (text === null) return []; const entries: AllyPriorFindingDisposition[] = []; - for (const [, shortSha, severity, index, disposition] of body.matchAll( + for (const [, shortSha, severity, index, disposition] of text.matchAll( PRIOR_FINDING_DISPOSITION_PATTERN, )) { const verb = disposition!.toLowerCase(); @@ -179,30 +310,55 @@ export function extractAllyPriorFindingDispositions( * `changes requested`, neither of which yields identities a ledger could name. * A caller deciding whether every finding has been retired must treat that as * "unknown", not as "none". + * + * Reads the raw body as well as the fence-stripped one and keeps whichever + * bucket is larger, for the same reason hasActionablePrReviewFeedback does. + * Enumerating from stripped text alone fails open: an unbalanced fence blanks + * everything after it, so a bucket *following* one disappears, and + * isFullyDispositioned then retires the whole head once the surviving subset is + * retired — silently clearing the findings the fence swallowed. + * + * Neither source subsumes the other, so both are read rather than just the raw + * one. Blanking a line can only remove a bucket, but the pattern's `\s+` spans + * newlines, so blanking an interposed line can also *join* two lines into a + * bucket that the raw text does not contain: + * + * Critical + * ```ts <- blanked, along with its closing fence + * x + * ``` + * Issues (2) + * + * Raw finds no bucket there; stripped finds `Critical Issues (2)`. The fence + * has to be *closed* for this: left open it swallows `Issues (2)` as well and + * both readings find nothing, which is why the open-fence version of this + * example does not demonstrate the join it was meant to. */ export function extractAllyReportedFindingRefs( body: string | null | undefined, ): AllyFindingRef[] | null { if (typeof body !== "string") return null; - const refs: AllyFindingRef[] = []; - let sawBucket = false; - for (const [, severity, count] of body.matchAll(COUNTED_FINDINGS_BUCKET_PATTERN)) { - sawBucket = true; - for (let index = 1; index <= Number(count); index += 1) { - refs.push({ severity: severity!.toLowerCase(), index }); + + // Highest count seen per severity, across both readings. Findings are + // identified by (severity, index), so a bucket of N contributes indices + // 1..N; taking the maximum yields a superset of either reading alone. + const highestCount = new Map(); + for (const text of [body, withoutFencedCodeBlocks(body)]) { + for (const [, severity, count] of text.matchAll(COUNTED_FINDINGS_BUCKET_PATTERN)) { + const key = severity!.toLowerCase(); + highestCount.set(key, Math.max(highestCount.get(key) ?? 0, Number(count))); } } - return sawBucket ? refs : null; -} + if (highestCount.size === 0) return null; -/** Return whether a formal or comment-shaped review contains blocking feedback. */ -export function hasActionablePrReviewFeedback(body: string | null | undefined, state?: string | null): boolean { - const normalizedState = state?.trim().toLowerCase(); - if (normalizedState === "changes_requested" || normalizedState === "changes-requested") return true; - if (typeof body !== "string") return false; - const text = body.trim(); - if (!text) return false; + const refs: AllyFindingRef[] = []; + for (const [severity, count] of highestCount) { + for (let index = 1; index <= count; index += 1) refs.push({ severity, index }); + } + return refs; +} +function carriesBlockingFeedback(text: string): boolean { for (const bucket of text.matchAll(/\b(?:Critical|Important)\s+Issues\b[*_]*\s*\((\d+)\)/gi)) { if (Number(bucket[1]) > 0) return true; } @@ -212,3 +368,21 @@ export function hasActionablePrReviewFeedback(body: string | null | undefined, s if (hasNonNegatedMatch(text, /\brequest(?:ed|s)?\s+changes\b/i)) return true; return /\bRecommended\s+Action\b[\s\S]{0,400}\bfix\b[\s\S]{0,400}\bbefore\s+merg(?:e|es|ed|ing)\b/i.test(text); } + +/** Return whether a formal or comment-shaped review contains blocking feedback. */ +export function hasActionablePrReviewFeedback(body: string | null | undefined, state?: string | null): boolean { + const normalizedState = state?.trim().toLowerCase(); + if (normalizedState === "changes_requested" || normalizedState === "changes-requested") return true; + if (typeof body !== "string") return false; + const text = body.trim(); + if (!text) return false; + + // Deliberately the one predicate that reads the raw body as well as the + // fence-stripped one, and blocks if *either* says so. Everywhere else, + // ignoring quoted text fails safe; here it would fail open — an unbalanced + // fence blanks the rest of the body, and a dropped finding silently clears a + // PR. A quoted finding costs a false red, which is visible and recoverable; + // a missed one is neither. Same asymmetry that keeps an unrecognized ledger + // verb from retiring a finding. + return carriesBlockingFeedback(text) || carriesBlockingFeedback(withoutFencedCodeBlocks(text)); +}