From 48feb16cc92e565226060f8243c4024c70ddba2f Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Fri, 18 Sep 2026 21:23:51 +0000 Subject: [PATCH 01/13] feat(review-gate): read Ally's verdict from a structured block, not prose (BLO-32695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Squash-linearization of the 28 commits on re/BLO-32695-ally-verdict-block. The branch carried three `Merge origin/master` commits and this repo's merge queue is REBASE/ALLGREEN, so the queue fails the rebase at head-of-queue and dequeues before creating any build (BLO-22300) — the branch could never merge however green it got. Re-merging master would have added a fourth. Linearizing onto master is the only topology the queue accepts, so the history is squashed rather than the tree re-merged. Ally now emits its verdict as an `` block carrying the attested head, per-severity counts and one entry per prior-finding disposition. All four Ally-body readers consume it as the primary source; the prose regexes remain a fallback, and an unreadable block is reported as its own `unreadable_verdict` outcome rather than as a finding or as review evidence. Reconciled with BLO-31446, which landed on master after this branch forked and reworked the same function. The two changes are orthogonal and BOTH are kept — this is not a "newest wins" resolution, and each half is mutation-tested: * `carriesBlockingFeedback` — master's `declaresNoFindings` (every counted severity declared zero) supersedes this branch's `sawCountedBucket` (any counted bucket, all zero). Both implement the same precedence-over- boilerplate fix; master's is strictly narrower, so the branch's form was a fail-open relative to it. Reverting to `sawCountedBucket` fails "keeps the prose fallback live when only one bucket is declared". Master's still-present-ledger blocking clause is kept in full. * `headsWithUndispositionedFinding` — this branch's `topTiedBy` tied-set rewrite is kept (master's `newestPerHead` max-scan let array order decide the verdict), and master's `countInheritedLedgerAssertion: false` is threaded into the tied-set predicate. The option narrows what a candidate asserts; the tie-break decides which candidates may assert it; neither reads the other. Dropping the option fails "does not let a still-present disposition clear the finding it reports". * `check-ally-review-consistency.mjs` — still-present detection keeps master's I3 title and gains the structured-block precedence (`structuredBlocking(body, "stillPresent") ?? hasStillPresentDisposition`). Carries 6e8f37d5d (CTO) unchanged: the `compareKeys` tripwire comment now names the reachable data-dependent-key-length trigger, and the unreadable-verdict scan is exhaustive with `block.reason` as its final axis. Verified: pr-comment-review-gate.test.ts 99/99 (77 branch + 22 BLO-31446), six sibling review-gate suites 241/241, check-ally-review-consistency.test.mjs 140/140, `tsc --noEmit` clean. Co-Authored-By: Claude --- .github/scripts/sweep-stalled-ally-reviews.py | 270 ++- .../test_sweep_stalled_ally_reviews.py | 314 ++++ .planning/ally-agent/AGENTS.md | 26 +- .../ally-agent-idempotency-contract.test.mjs | 209 +++ scripts/check-ally-review-consistency.mjs | 264 ++- .../check-ally-review-consistency.test.mjs | 196 ++ .../check-comment-review-gate-census.test.mjs | 4 +- .../ally-review-verdict-block.test.ts | 1573 +++++++++++++++++ ...lockcast-gh-io-pr126-2026-09-14T020111Z.md | 24 + .../ally-review-pr1675-2026-09-07T154142Z.md | 146 ++ server/src/__tests__/github-app-auth.test.ts | 63 + .../__tests__/pr-comment-review-gate.test.ts | 237 +++ server/src/services/ally-review-detection.ts | 618 ++++++- server/src/services/github-app-auth.ts | 14 + server/src/services/pr-comment-review-gate.ts | 337 +++- 15 files changed, 4236 insertions(+), 59 deletions(-) create mode 100644 server/src/__tests__/ally-review-verdict-block.test.ts create mode 100644 server/src/__tests__/fixtures/ally-review-blockcast-gh-io-pr126-2026-09-14T020111Z.md create mode 100644 server/src/__tests__/fixtures/ally-review-pr1675-2026-09-07T154142Z.md diff --git a/.github/scripts/sweep-stalled-ally-reviews.py b/.github/scripts/sweep-stalled-ally-reviews.py index 1749bae9ace1..e8989cb88540 100755 --- a/.github/scripts/sweep-stalled-ally-reviews.py +++ b/.github/scripts/sweep-stalled-ally-reviews.py @@ -79,17 +79,271 @@ def parse_list(value, fallback): ) +# Ally's structured verdict block -- the primary source, mirroring +# server/src/services/ally-review-detection.ts so this reader and the merge gate +# cannot disagree about which tree was reviewed. The prose line above is the +# fallback for a body that carries no block. +VERDICT_BLOCK_PATTERN = re.compile( + r"^(?! *\t)(?! {4}) {0,3}(?![ \t]*>)", + re.MULTILINE | re.DOTALL, +) +VERDICT_OPENER_PATTERN = re.compile( + r"^(?! *\t)(?! {4}) {0,3}(?![ \t]*>)" % head + ) + + def test_block_wins_when_prose_is_unparseable(self): + body = "%s\n\n## Ally — Consolidated PR Review\nReviewed head: %s (unchanged since my last pass)\n" % ( + self.block(self.HEAD), + self.HEAD, + ) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + + def test_block_alone_attests(self): + self.assertEqual(sweep.parse_reviewed_head(self.block(self.HEAD)), self.HEAD) + + def test_prose_alone_still_attests(self): + self.assertEqual( + sweep.parse_reviewed_head("Reviewed head: %s" % self.HEAD), self.HEAD + ) + + def test_disagreement_fails_closed(self): + body = "%s\nReviewed head: %s" % (self.block(self.HEAD), "d" * 40) + self.assertIsNone(sweep.parse_reviewed_head(body)) + + def test_two_blocks_fail_closed_without_falling_back_to_prose(self): + body = "%s\n%s\nReviewed head: %s" % ( + self.block(self.HEAD), + self.block(self.HEAD), + self.HEAD, + ) + self.assertIsNone(sweep.parse_reviewed_head(body)) + + def test_unterminated_block_fails_closed_without_falling_back_to_prose(self): + body = '' % self.HEAD) + ) + + def test_partial_sha_in_block_fails_closed(self): + self.assertIsNone( + sweep.parse_reviewed_head( + '' % self.HEAD[:7] + ) + ) + + def test_a_zero_padded_version_reads_the_same_here_as_in_the_two_js_readers(self): + """Peer review of #1721, Suggestion 1 -- the version compare diverged. + + This compared `raw_version != str(1)` while ally-review-detection.ts and + check-ally-review-consistency.mjs both use `Number(raw) !== 1`, so + `ally-verdict:01` was readable to the merge gate and unreadable here. + The sweep then treats the review as no signal for that head and + re-requests a review that already happened. Two parsers disagreeing + about one body is the BLO-31730 failure, not a formatting nicety. + """ + body = '' % self.HEAD + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + + def test_a_space_after_the_colon_is_the_block_it_plainly_is(self): + """Peer review of #1721, Important 2. + + The emitter is a model transcribing a template out of a fenced example, + so pretty-printing a space here is the likeliest single drift. It used + to match neither the block nor the opener pattern, so it read `absent` + and fell through to the prose path this row retires. + """ + body = '' % self.HEAD + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + + def test_a_garbled_version_fails_closed_rather_than_vanishing(self): + """The opener is version-agnostic so the strict pattern can be the only + reader of the version. `:v1` and a missing version previously missed + both patterns and degraded silently to prose.""" + for opener in ('\nReviewed head: %s' % ( + opener, + self.HEAD, + self.HEAD, + ) + self.assertIsNone(sweep.parse_reviewed_head(body), opener) + + def test_quoted_block_is_a_body_discussing_one_not_emitting_one(self): + body = "> \nReviewed head: %s" % ( + "d" * 40, + self.HEAD, + ) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + + +class TestVerdictCountsMirrorTheGate(unittest.TestCase): + """Peer review of #1721, Important 2 -- the count rule landed in one reader + of three. + + A block whose counts the merge gate rejects is red there with + `unreadable_verdict`, whose only escape is one more review. This sweep is + what asks for that review, and it used to read the same body as a perfectly + good attestation -- so the red had no escape route at all. + """ + + HEAD = "c" * 40 + + def body(self, findings, *prose): + return "\n".join( + [ + '' % (self.HEAD, findings), + "", + "## Ally — Consolidated PR Review", + ] + + list(prose) + ) + + def test_a_positive_bucket_against_a_stated_zero_is_unreadable(self): + body = self.body('{"critical":0,"important":0}', "### Critical Issues (2)") + self.assertIsNone(sweep.parse_reviewed_head(body)) + + def test_control_agreeing_counts_still_attest(self): + body = self.body('{"critical":0,"important":0}', "### Critical Issues (0)") + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + + def test_a_sentence_referencing_a_prior_pass_does_not_fail_it_closed(self): + body = self.body( + '{"critical":0,"important":0}', + "### Critical Issues (0)", + "", + "Both Critical Issues (2) from the previous pass are fixed.", + ) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + + def test_a_quoted_bucket_does_not_fail_it_closed(self): + for quoted in ("> ### Critical Issues (2)", "```\n### Critical Issues (2)\n```"): + body = self.body('{"critical":0,"important":0}', quoted) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD, quoted) + + def test_findings_the_gate_rejects_are_rejected_here_too(self): + # Absent counts are not zero counts; an unknown severity is not a key to + # drop; and `true` is not 1, however Python spells its bools. + for findings in ('{}', '{"critical":0}', '{"critical":0,"important":0,"typo":0}', + '{"critical":true,"important":0}', '{"critical":-1,"important":0}'): + self.assertIsNone(sweep.parse_reviewed_head(self.body(findings)), findings) + + +class TestVerdictBlockMirrorsTheGateOnFencesAndLedgers(unittest.TestCase): + """Peer review of #1721 at 97b4ddd1 -- the remaining two reader divergences. + + Both are the same defect as TestVerdictCountsMirrorTheGate: a rule landed in + some readers and not this one, so the gate and this sweep disagree about one + body. Disagreeing in this direction is the expensive one -- with + ally_has_reviewed_head false the sweep re-fires a request on a head Ally + already reviewed, and each duplicate is a COMMENTED review that cannot be + dismissed. + """ + + HEAD = "d" * 40 + + def block(self, extra=""): + return '' % ( + self.HEAD, + extra, + ) + + def body(self, *rest): + return "\n".join([self.block(), "", "## Ally — Consolidated PR Review"] + list(rest)) + + def test_a_fenced_example_of_the_marker_is_not_a_second_block(self): + # The gate counts blocks over fence-stripped text (parseAllyVerdictBlock + # reads emittedReviewText), so a quoted marker is invisible there: + # blocks=1, openers=1 -> ok. Read raw, this saw blocks=2, openers=2 -> + # unreadable. It fires first on a review quoting the template, which is + # the likeliest shape for a review *of this feature*. + body = self.body("Here is the emitted form:", "", "```markdown", self.block(), "```") + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + + def test_control_a_real_second_block_is_still_unreadable(self): + # Without this the test above passes for a reader that stopped counting + # blocks at all. + body = self.body("", self.block()) + self.assertIsNone(sweep.parse_reviewed_head(body)) + + def test_a_malformed_ledger_is_rejected_here_too(self): + # Validation had covered one of the two fields the payload carries. TS + # rejects these via asDispositions and the mjs via stillPresentIn, while + # this dropped through to ("ok", head) -- so the gate went red on + # `unreadable_verdict` with no mechanism left to clear it. + for dispositions in ( + '"nope"', + "42", + "null", + '[{"head":"deadbee","severity":"critical","verb":"fixed"}]', + '[{"head":"deadbee","severity":"critical","index":0,"verb":"fixed"}]', + '[{"head":"deadbee","severity":"critical","index":true,"verb":"fixed"}]', + '[{"head":"xyz","severity":"critical","index":1,"verb":"fixed"}]', + '[{"head":"deadbee","severity":"","index":1,"verb":"fixed"}]', + '[{"head":"deadbee","severity":"critical","index":1,"verb":" "}]', + '["not an object"]', + ): + body = self.body_with_dispositions(dispositions) + self.assertIsNone(sweep.parse_reviewed_head(body), dispositions) + + def test_control_a_well_formed_or_absent_ledger_still_attests(self): + # `null` is deliberately absent from this list: both JS readers key on + # `undefined`, so an explicit null fails Array.isArray and is unreadable + # there. Collapsing the two is why this takes the payload rather than + # the field. + self.assertEqual(sweep.parse_reviewed_head(self.body()), self.HEAD) + for dispositions in ( + "[]", + '[{"head":"deadbee","severity":"critical","index":1,"verb":"fixed"}]', + '[{"head":"deadbee","severity":"recommended-action","index":4,"verb":"withdrawn"}]', + ): + body = self.body_with_dispositions(dispositions) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD, dispositions) + + def body_with_dispositions(self, dispositions): + return "\n".join( + [self.block(',"dispositions":%s' % dispositions), "", "## Ally — Consolidated PR Review"] + ) + + +class TestFloatFormattedIntegersMirrorNumberIsInteger(unittest.TestCase): + """Ally review of #1721 at bbe6d640 -- `isinstance(_, int)` is not that. + + `json.loads` yields floats for `0.0`, `0e0` and `1e3`; every one is an + integer to `Number.isInteger`, so the gate and the mjs read these bodies + `ok` while this reader read them `unreadable`. Same harm direction as the + two divergences above: the sweep re-requests a review of a head Ally + already reviewed, and a COMMENTED duplicate cannot be dismissed. + + The pre-existing `true` cases pin the other direction and cannot catch + this, which is why these are separate rather than added to that list. + """ + + HEAD = "e" * 40 + + def body(self, payload): + return "\n".join( + ['' % (self.HEAD, payload), "", + "## Ally — Consolidated PR Review"] + ) + + def test_float_formatted_counts_still_attest(self): + for findings in ('{"critical":0.0,"important":0.0}', + '{"critical":0e0,"important":0}', + '{"critical":1e3,"important":0}'): + body = self.body('"findings":%s' % findings) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD, findings) + + def test_a_float_formatted_ledger_index_still_attests(self): + body = self.body( + '"findings":{"critical":0,"important":0},' + '"dispositions":[{"head":"deadbee","severity":"critical","index":1.0,"verb":"fixed"}]' + ) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + + def test_control_non_integral_and_bool_are_still_rejected(self): + # Number.isInteger(0.5) is false and a bool is not a number in JS, so + # widening to floats must not have widened past integral ones. + for payload in ('"findings":{"critical":0.5,"important":0}', + '"findings":{"critical":true,"important":0}', + '"findings":{"critical":1e4,"important":0}', + '"findings":{"critical":0,"important":0},' + '"dispositions":[{"head":"deadbee","severity":"critical",' + '"index":1.5,"verb":"fixed"}]'): + self.assertIsNone(sweep.parse_reviewed_head(self.body(payload)), payload) + + +class TestIsConsolidatedAllyCommentForHead(unittest.TestCase): + HEAD = "c" * 40 + + def test_block_before_heading_still_matches(self): + """Ally emits the verdict block first, so the heading is not byte 0. + + `body.startswith("## Ally")` rejected Ally's own emitted bodies. + """ + body = '\n\n## Ally — Consolidated PR Review\nReviewed head: %s\n' % ( + self.HEAD, + self.HEAD, + ) + self.assertTrue(sweep.is_consolidated_ally_comment_for_head(body, self.HEAD)) + + def test_heading_first_still_matches(self): + body = "## Ally — Consolidated PR Review\nReviewed head: %s\n" % self.HEAD + self.assertTrue(sweep.is_consolidated_ally_comment_for_head(body, self.HEAD)) + + def test_no_heading_does_not_match(self): + body = "Reviewed head: %s\n" % self.HEAD + self.assertFalse(sweep.is_consolidated_ally_comment_for_head(body, self.HEAD)) + + def test_heading_without_attestation_does_not_match(self): + self.assertFalse( + sweep.is_consolidated_ally_comment_for_head( + "## Ally — Consolidated PR Review\n", self.HEAD + ) + ) + + if __name__ == "__main__": unittest.main() diff --git a/.planning/ally-agent/AGENTS.md b/.planning/ally-agent/AGENTS.md index b1b3d29b627d..c16bcc9bbeb9 100644 --- a/.planning/ally-agent/AGENTS.md +++ b/.planning/ally-agent/AGENTS.md @@ -175,7 +175,19 @@ If either pipeline errors out (model unavailable, tool failure, etc.), continue Merge findings from both pipelines into one consolidated review. Follow this structure: ```markdown -## 🔍 Automated Review — PR # @ +## Ally — Consolidated PR Review + + + +_🔍 Automated Review — PR # @ _ Reviewed head: @@ -206,8 +218,20 @@ Reviewed head: **Dedup rule**: if both pipelines flag the same line for similar reasons, merge into one bullet with both `[pipeline]` tags. Don't double-count. +**The `Ally — Consolidated PR Review` heading is mandatory, and it gates everything below.** `hasAllyConsolidatedReviewHeading` (`server/src/services/ally-review-detection.ts`) is the first thing the gate applies, and a body that fails it is not treated as a review at all — so a perfectly well-formed `ally-verdict:1` block inside it is never even looked for. That makes this the one field whose mismatch is silent on *both* sides: the block still parses in isolation, and the gate simply never sees the comment. Keep any friendlier title as secondary prose underneath, not in place of it. + +**Emit the heading FIRST, before the `ally-verdict:1` block.** Every reader in this repo is line-anchored, so either order parses here — which is what makes getting it wrong silent. Ally's live one-review-per-head guard is a managed bundle *outside* this repo and matches the heading at the **first byte**, so a body that leads with the block reads as "not yet reviewed" and the next wake re-reviews the same head. A `COMMENTED` review cannot be dismissed, so each duplicate is permanent until the head moves. Measured on paperclip#1721 (2026-09-15): of 17 reviews, the 4 that led with the block produced same-head duplicates at 2 heads (`a8096107`, `d40c450b`); the 13 that led with the heading produced 0. Pinned by `scripts/ally-agent-idempotency-contract.test.mjs`. + **`Reviewed head:` is mandatory, in every state.** It is the immutable attestation Step 2 reads, and it is the *only* thing that makes the skip work — the heading's `` is not a substitute. Emit the full 40-character lowercase SHA on its own line. Omit it and Step 2 counts zero forever, so every wake re-reviews the same head; that is one of the two ways this guard has previously gone inert, and it is invisible until duplicate reviews pile up. +**The `ally-verdict:1` block is mandatory, and it is ADDITIVE — it never replaces the `Reviewed head:` line.** It is the machine-readable source the comment-review gate reads first (`parseAllyVerdictBlock` in `server/src/services/ally-review-detection.ts`, BLO-32695). Prose parsing survives only as the fallback for bodies posted before the block existed, and that fallback is why the family of parser widenings kept growing: one clean review of paperclip#1675 (2026-09-07T15:41:42Z, 0 Critical / 0 Important, two findings explicitly retired) defeated **four** independent prose patterns at once — a parenthetical after the attested SHA, a bolded ledger verb, a comma where a dash was required, and a hyphenated severity — so the gate published a finding you had already withdrawn. Fields, not sentences, is the exit. + +Three rules bind: + +- **Emit exactly one block per review.** Two blocks, an unknown version, malformed JSON, a `head` that is not a complete 40-hex SHA, or a **missing `findings` object** all resolve to a *fail-closed* `unreadable_verdict` red. That red is scoped to your newest review only, so posting one more readable review always clears it — but it is a red, not a green. `findings` is required even when you found nothing: emit `{ "critical": 0, "important": 0, "suggestions": 0 }`. Omitting the key is not the same as zero — zero is a clean verdict, so a block that never stated its counts would clear a head it made no claim about, and the parser refuses to read it rather than default it. +- **Keep the prose `Reviewed head:` line.** Four independent readers parse that line and only the gate understands the block: this module, `commentAttestsHead` in `server/src/services/github-app-auth.ts`, `ATTESTED_HEAD_RE` in `scripts/check-ally-review-consistency.mjs`, and `REVIEWED_HEAD_PATTERN` in `.github/scripts/sweep-stalled-ally-reviews.py`. Drop the prose and readers 2–4 attest nothing — reader 2 raises `pr_review_output_missing` and posts a false "reviewer never finished". +- **`findings` counts what you actually found; `dispositions` retires prior findings by name.** `blocking_finding` is now reachable only from a counted finding, so a zero-count block plus your usual "Recommended Action" boilerplate no longer reads as actionable. Verb vocabulary is unchanged — an unrecognised verb still fails closed rather than retiring anything. + **Severity rule**: trust the higher of the two pipelines' severities. If pr-review-toolkit's `code-reviewer` sub-agent marks something Critical and codex marks the same thing Suggestion, treat it as Critical. ### Step 5 — Post the review diff --git a/scripts/ally-agent-idempotency-contract.test.mjs b/scripts/ally-agent-idempotency-contract.test.mjs index 18c48b86d2f4..ca660569b976 100644 --- a/scripts/ally-agent-idempotency-contract.test.mjs +++ b/scripts/ally-agent-idempotency-contract.test.mjs @@ -253,6 +253,123 @@ test("Step 4 emits the attestation Step 2 consumes", () => { + " short SHA is not a substitute for the 40-hex attestation"); }); +// The producer half of BLO-32695. The gate reads `ally-verdict:1` as its +// primary source, so the same consumer-with-no-producer failure applies: a +// reader that understands blocks against an emitter that never posts one falls +// back to the prose patterns on every review, which is the state that produced +// the paperclip#1675 false red in the first place. +function step4Template() { + const start = agentsDoc.indexOf("### Step 4"); + assert.notEqual(start, -1, "Step 4 must exist"); + const step4 = agentsDoc.slice(start, agentsDoc.indexOf("### Step 5", start)); + const fence = /```markdown\n([\s\S]*?)```/.exec(step4); + assert.ok(fence, "Step 4 must retain its markdown review template"); + return { step4, template: fence[1] }; +} + +test("Step 4 emits the structured verdict block the gate reads first", () => { + const { template } = step4Template(); + + // Matched against the parser's own opener, not against loose prose: the + // regex in ally-review-detection.ts is ``, + "gm", +); +const VERDICT_OPENER_RE = new RegExp( + String.raw`^${NOT_INDENTED_CODE}(?![ \t]*>) {0,3}\n${extra}`; +} + function appReview(overrides = {}) { return review({ user: { login: "allyblockcast[bot]", id: ALLY_APP_REVIEWER_ID, type: "Bot" }, ...overrides }); } @@ -180,6 +190,87 @@ describe("attestedHead", () => { it("ignores a SHA mentioned mid-sentence", () => { assert.equal(attestedHead(`I reviewed head: ${HEAD} earlier today`), null); }); + + // The structured block is the primary source here exactly as it is in + // server/src/services/ally-review-detection.ts. Before this, a body carrying + // a block plus a #1675-shaped prose line read as "no attestation" to this + // script while the merge gate read it as attesting — the readers disagreed + // about which tree was reviewed, which is the BLO-32695 finding. + const block = (head) => + ``; + + it("reads the structured block when the prose line is unparseable (#1675)", () => { + const body = `${block(HEAD)}\n\n## Ally — Consolidated PR Review\nReviewed head: ${HEAD} (unchanged since my last pass — no new commits)\n`; + assert.equal(attestedHead(body), HEAD); + }); + + it("reads the structured block when no prose line is present", () => { + assert.equal(attestedHead(block(HEAD)), HEAD); + }); + + it("fails closed when the block and the prose line name different heads", () => { + const other = "a".repeat(40); + assert.equal(attestedHead(`${block(HEAD)}\nReviewed head: ${other}`), null); + }); + + it("fails closed on two blocks rather than falling back to prose", () => { + const body = `${block(HEAD)}\n${block(HEAD)}\nReviewed head: ${HEAD}`; + assert.equal(attestedHead(body), null); + }); + + it("fails closed on an unterminated block rather than falling back to prose", () => { + const body = ``), null); + }); + + it("fails closed on a block whose head is not a complete SHA", () => { + assert.equal(attestedHead(``), null); + }); + + it("ignores a quoted block — that is a body discussing one, not emitting one", () => { + const body = `> ${block(HEAD).split("\n").join("\n> ")}\nReviewed head: ${HEAD}`; + assert.equal(attestedHead(body), HEAD); + }); + + // Peer review of #1721, Important 2 — the count rule landed in one reader of + // three. The merge gate treats a block contradicted by its own emitted + // buckets as unreadable; this script read the same body as a good + // attestation, so the two disagreed about the field that decides whether a + // merge is blocked. + const counted = (findings, ...prose) => + [ + ``, + "", + "## Ally — Consolidated PR Review", + ...prose, + ].join("\n"); + + it("fails closed when an emitted bucket contradicts the block's zero", () => { + assert.equal(attestedHead(counted('{"critical":0,"important":0}', "### Critical Issues (2)")), null); + }); + + it("control: agreeing counts still attest", () => { + assert.equal( + attestedHead(counted('{"critical":0,"important":0}', "### Critical Issues (0)")), + HEAD, + ); + }); + + it("does not fail closed on a referenced, quoted or fenced bucket", () => { + // Over-matching here reds a clean review, which is the false red this row + // retires — so the cross-check reads only the emitted heading form. + for (const prose of [ + "Both Critical Issues (2) from the previous pass are fixed.", + "> ### Critical Issues (2)", + "```\n### Critical Issues (2)\n```", + ]) { + assert.equal(attestedHead(counted('{"critical":0,"important":0}', prose)), HEAD, prose); + } + }); }); describe("operativeAllyReviews", () => { @@ -320,6 +411,80 @@ describe("findPrViolations", () => { assert.match(violations[0], /^I2c PR #5 @ff1c72db: Ally App review 12 is APPROVED/); }); + // The producer's own template heads its buckets `### 🚨 Critical` with no + // `(N)`, so every prose reader here sees a blocking review as clean. The + // structured counts are the only place the finding is actually stated. + it("I2a: catches a structured blocking verdict whose prose carries no counted headings", () => { + const pr = { + number: 1721, + headSha: HEAD, + reviews: [ + appReview({ + id: 21, + state: "APPROVED", + body: verdictBody({ critical: 0, important: 1 }, "\n### ⚠️ Important\n- **[codex]** something real\n"), + }), + ], + }; + const violations = findPrViolations(pr); + assert.equal(violations.length, 1); + assert.match(violations[0], /^I2a PR #1721 @ff1c72db: Ally App review 21 is APPROVED/); + }); + + it("I2c: catches a structured still-present disposition with no prose ledger line", () => { + const pr = { + number: 1722, + headSha: HEAD, + reviews: [ + appReview({ + id: 22, + state: "APPROVED", + body: verdictBody({ critical: 0, important: 0 }, "\n### ✅ Strengths\n- clean\n", [ + { head: "d40c450", severity: "important", index: 1, verb: "still-present" }, + ]), + }), + ], + }; + const violations = findPrViolations(pr); + assert.equal(violations.length, 1); + assert.match(violations[0], /^I2c PR #1722 @ff1c72db: Ally App review 22 is APPROVED/); + }); + + // The control for the two above: the same uncounted prose with a verdict that + // explicitly reports nothing must stay clean, or the fix is just a blanket red. + it("allows an APPROVED whose structured verdict explicitly reports zero findings", () => { + const pr = { + number: 1723, + headSha: HEAD, + reviews: [ + appReview({ + id: 23, + state: "APPROVED", + body: verdictBody({ critical: 0, important: 0 }, "\n### 🚨 Critical\n### ⚠️ Important\n"), + }), + ], + }; + assert.deepEqual(findPrViolations(pr), []); + }); + + // A block Ally tried and failed to state is not a review that predates the + // block, so it must not reach the prose path the block exists to replace. + it("I2a: fails closed on an APPROVED whose verdict block omits a blocking count", () => { + const pr = { + number: 1724, + headSha: HEAD, + reviews: [ + appReview({ + id: 24, + state: "APPROVED", + body: verdictBody({ suggestions: 0 }, "\n### ✅ Strengths\n- clean\n"), + }), + ], + }; + const violations = findPrViolations(pr); + assert.ok(violations.some((v) => /^I2a PR #1724 /.test(v)), violations.join("\n")); + }); + it("I3: catches an App review whose body attests a head other than the recorded commit", () => { const pr = { number: 870, @@ -1116,3 +1281,34 @@ describe("the committed baseline", () => { assert.match(failing[0].violation, /PR #1601/); }); }); + +/** + * Peer review of #1721 at 97b4ddd1 — this reader counted verdict blocks over + * the raw body while the gate counts them over fence-stripped text. + * + * Same body, different verdict across two of the four readers the PR's central + * invariant names. It fires first on a review that quotes the template inside a + * fence, which is the likeliest shape for a review *of this feature* — the same + * self-referential trigger the block's own line anchoring exists for. + */ +describe("BLO-32695 — a fenced example of the marker is not a second block", () => { + const block = (head) => + ``; + const body = (...rest) => + [block(HEAD), "", "## Ally — Consolidated PR Review", `Reviewed head: ${HEAD}`, ...rest].join("\n"); + + it("reads the head through a fenced quote of the marker", () => { + assert.equal(attestedHead(body("As emitted:", "", "```markdown", block(HEAD), "```")), HEAD); + }); + + it("control: a real second block is still unreadable", () => { + // Without this the test above passes for a reader that stopped counting. + assert.equal(attestedHead(body("", block(HEAD))), null); + }); + + it("control: a fenced opener alone does not mint a truncated-payload red", () => { + // openers > blocks is the fail-closed branch; stripping fences has to move + // both counts together or it trades one divergence for another. + assert.equal(attestedHead(body("```markdown", "`; +} + +/** The structured verdict Ally *would* emit for the fixture's own review. */ +const PR1675_VERDICT = { + head: PR1675_HEAD, + findings: { critical: 0, important: 0 }, + dispositions: [ + { head: "583085ded", severity: "important", index: 1, verb: "fixed" }, + { head: "583085ded", severity: "recommended-action", index: 4, verb: "withdrawn" }, + ], +}; + +/** + * BLO-32695's measurement, pinned so it cannot silently stop being true. + * + * These four assertions are negative controls, not aspirations: they document + * that one clean review — 0 Critical, 0 Important, two findings explicitly + * retired — defeated four independent prose patterns for four unrelated + * reasons. They are what makes the structured block a replacement rather than + * a fifth widening. If a future prose fix makes one of them parse, that is + * fine and the case should be updated to say so; what must not happen is the + * set quietly shrinking because nobody noticed the shapes changed. + */ +describe("BLO-32695 — the #1675 clean review against prose parsing", () => { + it("is recognised as an Ally review, so nothing here is an author-identity miss", () => { + expect(hasAllyConsolidatedReviewHeading(PR1675_CLEAN_REVIEW_BODY)).toBe(true); + }); + + it("attests no head: the parenthetical after the SHA closes the attestation pattern", () => { + expect(PR1675_CLEAN_REVIEW_BODY).toContain( + `Reviewed head: ${PR1675_HEAD} (unchanged since my last pass`, + ); + expect(extractAllyReviewedHeadSha(PR1675_CLEAN_REVIEW_BODY)).toBeNull(); + + // The control that isolates the parenthetical as the sole cause: drop it + // and the very same body attests cleanly. + const withoutParenthetical = PR1675_CLEAN_REVIEW_BODY.replace( + new RegExp(`(Reviewed head: ${PR1675_HEAD}) \\(.*?\\)`), + "$1", + ); + expect(extractAllyReviewedHeadSha(withoutParenthetical)).toBe(PR1675_HEAD); + }); + + it("dispositions nothing: both ledger bullets are dropped whole", () => { + expect(ledgerBullets()).toHaveLength(2); + expect(extractAllyPriorFindingDispositions(PR1675_CLEAN_REVIEW_BODY)).toEqual([]); + }); + + it("drops bullet 1 for a bolded verb AND a missing second dash, not either alone", () => { + const [bullet] = ledgerBullets(); + expect(bullet).toContain("— **fixed, and my finding was stale when I filed it.**"); + + // Progressive repair. Un-bolding alone still yields nothing — the pattern + // also requires a second dash after the verb, and Ally wrote a comma then + // prose. This is the step that separates BLO-32695 from BLO-31947: that + // row's repair addresses the bold, and the bullet still does not parse. + const unbolded = bullet!.replace(/— \*\*fixed,.*$/, "— fixed, and my finding was stale."); + expect(extractAllyPriorFindingDispositions(unbolded)).toEqual([]); + + const unboldedWithDash = bullet!.replace(/— \*\*fixed,.*$/, "— fixed — my finding was stale."); + expect(extractAllyPriorFindingDispositions(unboldedWithDash)).toHaveLength(1); + }); + + it("drops bullet 2 for a hyphenated severity the pattern cannot express", () => { + const [, bullet] = ledgerBullets(); + expect(bullet).toContain("prior:583085ded recommended-action 4"); + + // `recommended-action` cannot match the pattern's `([a-z]+)` severity, so + // even the fully-repaired punctuation still yields nothing. + const fullyRepunctuated = + "- **prior:583085ded recommended-action 4** — withdrawn — I was wrong."; + expect(extractAllyPriorFindingDispositions(fullyRepunctuated)).toEqual([]); + + // Same bullet with a single-word severity parses, isolating the hyphen. + const singleWordSeverity = "- **prior:583085ded important 4** — fixed — I was wrong."; + expect(extractAllyPriorFindingDispositions(singleWordSeverity)).toHaveLength(1); + }); + + it("is itself clean, so the red came from a stale review of the same head", () => { + // The body carries no blocking feedback at all. That matters for the + // diagnosis: `blocking_finding` was not manufactured out of this review's + // prose — this review was simply *invisible*, so an older review of the + // same head stayed authoritative and its finding was published instead. + expect(hasActionablePrReviewFeedback(PR1675_CLEAN_REVIEW_BODY)).toBe(false); + // Both buckets are present and both read (0), so the findings *are* + // enumerable — they just enumerate to nothing. That is a different state + // from `null` ("no bucket at all"), and the difference decides whether a + // head can ever be fully dispositioned. + expect(extractAllyReportedFindingRefs(PR1675_CLEAN_REVIEW_BODY)).toEqual([]); + }); + + it("reproduces the incident end to end: the superseded verdict is the one published", () => { + // The earlier review is synthesised rather than stored verbatim: the four + // pinned failures all live in the 15:41:42Z body, and what this case needs + // from the 03:46:19Z one is only that it attested the same head and + // reported a finding, which is exactly what it did. + const earlierSameHead = [ + "## Ally — Consolidated PR Review", + `Reviewed head: ${PR1675_HEAD}`, + "### Critical Issues (0)", + "### Important Issues (1)", + "- The PR title still reads `docs(agents):`.", + ].join("\n"); + + const verdict = evaluateCommentReviewGate({ + headSha: PR1675_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [ + allyComment(earlierSameHead, "2026-09-07T03:46:19Z"), + allyComment(PR1675_CLEAN_REVIEW_BODY, "2026-09-07T15:41:42Z"), + ], + }); + + expect(verdict).toMatchObject({ state: "failure", outcome: "blocking_finding" }); + }); +}); + +describe("BLO-32695 — the structured verdict block as the primary source", () => { + const withBlock = `${verdictBlock(PR1675_VERDICT)}\n${PR1675_CLEAN_REVIEW_BODY}`; + + it("attests the head as a field, unmoved by the prose that defeated the pattern", () => { + expect(extractAllyReviewedHeadSha(withBlock)).toBe(PR1675_HEAD); + }); + + it("carries both dispositions, including the hyphenated severity", () => { + expect(extractAllyPriorFindingDispositions(withBlock)).toEqual([ + { + shortSha: "583085ded", + severity: "important", + index: 1, + disposition: "fixed", + kind: "retires", + }, + { + shortSha: "583085ded", + severity: "recommended-action", + index: 4, + disposition: "withdrawn", + // Unchanged vocabulary: `withdrawn` is not in the retiring set, so it + // still fails closed. Widening the verb list is a separate decision + // about what Ally *decides*, which BLO-32695 puts out of scope; the + // point here is that the verb now arrives intact instead of the whole + // bullet being dropped. + kind: "unrecognized", + }, + ]); + }); + + it("resolves the same body to clean/success", () => { + const verdict = evaluateCommentReviewGate({ + headSha: PR1675_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [allyComment(withBlock, "2026-09-07T15:41:42Z")], + }); + expect(verdict).toMatchObject({ state: "success", outcome: "clean" }); + }); + + it("still publishes a real counted finding as blocking_finding", () => { + const blocking = `${verdictBlock({ + head: PR1675_HEAD, + findings: { critical: 0, important: 2 }, + })}\n## Ally — Consolidated PR Review\nNo prose buckets at all.`; + + expect(extractAllyReportedFindingRefs(blocking)).toEqual([ + { severity: "important", index: 1 }, + { severity: "important", index: 2 }, + ]); + expect( + evaluateCommentReviewGate({ + headSha: PR1675_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [allyComment(blocking, "2026-09-07T15:41:42Z")], + }), + ).toMatchObject({ state: "failure", outcome: "blocking_finding" }); + }); + + it("reads an explicitly zeroed findings object as a stated zero", () => { + // The boundary of the missing-count tightening below, pinned from the + // permissive side so the fix cannot quietly grow into rejecting a review + // that legitimately found nothing. Stating both blocking counts as `0` is + // Ally saying "I counted, the answer was nothing"; omitting either — or + // the whole object — is Ally saying nothing at all. Only the second may + // clear no head. + const emptyCounts = `${verdictBlock({ head: PR1675_HEAD, findings: { critical: 0, important: 0 } })}\n## Ally — Consolidated PR Review`; + const parsed = parseAllyVerdictBlock(emptyCounts); + expect(parsed.kind).toBe("ok"); + expect(hasActionablePrReviewFeedback(emptyCounts)).toBe(false); + expect( + evaluateCommentReviewGate({ + headSha: PR1675_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [allyComment(emptyCounts, "2026-09-07T15:41:42Z")], + }), + ).toMatchObject({ state: "success", outcome: "clean" }); + }); + + it("ignores a block inside a fence, so a quoted verdict cannot clear a head", () => { + const quoted = [ + "## Ally — Consolidated PR Review", + "Quoting the review I am replying to:", + "```", + verdictBlock(PR1675_VERDICT), + "```", + ].join("\n"); + expect(parseAllyVerdictBlock(quoted)).toEqual({ kind: "absent" }); + }); +}); + +/** + * Cross-reader agreement about which tree was examined. + * + * Four readers parse `Reviewed head:` and only this module understands the + * block — commentAttestsHead (github-app-auth.ts), ATTESTED_HEAD_RE + * (check-ally-review-consistency.mjs) and HEAD_ATTESTATION_RE + * (sweep-stalled-ally-reviews.py) are the other three. So a body whose block + * and prose name different heads would set the merge gate against one tree + * while the retry sweep reasoned about another. + * + * The asymmetry is the whole design and is easy to get backwards. Requiring a + * *matching* prose attestation before the block may be trusted would have been + * the obvious reading of the finding, and it would have reverted BLO-32695 + * outright: the #1675 body's prose attestation is exactly the one the retired + * regex cannot read, so the block would have been unusable on the very review + * that motivated it. Only a readable *disagreement* is fatal. + */ +describe("BLO-32695 — the block and the prose line must not name different heads", () => { + const OTHER_HEAD = "1111111111111111111111111111111111111111"; + + it("fails closed when a clean prose attestation contradicts the block", () => { + const conflicting = [ + verdictBlock(PR1675_VERDICT), + "## Ally — Consolidated PR Review", + `Reviewed head: ${OTHER_HEAD}`, + ].join("\n"); + + expect(parseAllyVerdictBlock(conflicting)).toMatchObject({ kind: "unreadable" }); + expect(extractAllyReviewedHeadSha(conflicting)).toBeNull(); + }); + + it("does not resolve a contradicting body to success", () => { + const conflicting = [ + verdictBlock(PR1675_VERDICT), + "## Ally — Consolidated PR Review", + `Reviewed head: ${OTHER_HEAD}`, + ].join("\n"); + + const verdict = evaluateCommentReviewGate({ + headSha: PR1675_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [allyComment(conflicting, "2026-09-07T15:41:42Z")], + }); + expect(verdict.state).not.toBe("success"); + }); + + it("still trusts the block when the prose agrees", () => { + const agreeing = [ + verdictBlock(PR1675_VERDICT), + "## Ally — Consolidated PR Review", + `Reviewed head: ${PR1675_HEAD}`, + ].join("\n"); + + expect(extractAllyReviewedHeadSha(agreeing)).toBe(PR1675_HEAD); + }); + + it("still trusts the block when the prose attestation is the unreadable #1675 shape", () => { + // The regression guard for the obvious-but-wrong fix. This body's prose + // line carries the trailing parenthetical that yields zero matches, so a + // rule demanding a matching prose attestation would null it out — the + // exact false red BLO-32695 was filed to end. + const withBlock = `${verdictBlock(PR1675_VERDICT)}\n${PR1675_CLEAN_REVIEW_BODY}`; + // Control: without the block that same prose attests nothing at all. + expect(extractAllyReviewedHeadSha(PR1675_CLEAN_REVIEW_BODY)).toBeNull(); + expect(extractAllyReviewedHeadSha(withBlock)).toBe(PR1675_HEAD); + }); + + it("still trusts the block when the prose is ambiguous rather than contradicting", () => { + // Two attestations are not a competing claim, they are noise — precisely + // what the block exists to speak over. Failing closed here would let any + // review that *quotes* a head defeat its own verdict. + const ambiguous = [ + verdictBlock(PR1675_VERDICT), + "## Ally — Consolidated PR Review", + `Reviewed head: ${PR1675_HEAD}`, + `Reviewed head: ${OTHER_HEAD}`, + ].join("\n"); + + expect(extractAllyReviewedHeadSha(ambiguous)).toBe(PR1675_HEAD); + }); +}); + +/** + * A truncated block must not fall through to the prose parser. + * + * The fail-closed branch had a hole shaped like its own entry condition. An + * unterminated `\n## Ally — Consolidated PR Review`, + reason: /not valid JSON/, + }, + { + name: "an unsupported version", + body: `${verdictBlock({ head: PR1675_HEAD, findings: {} }, 2)}\n## Ally — Consolidated PR Review`, + reason: /unsupported ally-verdict version 2/, + }, + { + name: "non-integer finding counts", + body: `${verdictBlock({ head: PR1675_HEAD, findings: { important: "two" } })}\n## Ally — Consolidated PR Review`, + reason: /not severity counts/, + }, + { + // The dangerous shape, and the reason it needs its own case: every other + // entry here is malformed in a way that is obvious on sight. This one is + // a *valid* block — good head, good version, parseable JSON — that simply + // never states what it found. Defaulting the absent counts to an empty + // map made it read as 0 Critical / 0 Important, i.e. a clean verdict, so + // a payload making no claim could clear a head. That is the fail-open + // direction BLO-29711 closed and AC-5 forbids re-opening. + name: "a valid head with no findings counts at all", + body: `${verdictBlock({ head: PR1675_HEAD })}\n## Ally — Consolidated PR Review`, + reason: /states no findings counts/, + }, + { + // Same fail-open one level down, and strictly harder to spot: the object + // is present, well-typed and internally consistent, it simply never + // states the two counts that decide the gate. The blocking loop reads + // the absent keys as zero, so this cleared a head while claiming only + // that it found no suggestions. + name: "a findings object that omits the blocking counts", + body: `${verdictBlock({ head: PR1675_HEAD, findings: { suggestions: 0 } })}\n## Ally — Consolidated PR Review`, + reason: /omit the `critical` count/, + }, + { + name: "a findings object that omits only one blocking count", + body: `${verdictBlock({ head: PR1675_HEAD, findings: { critical: 0 } })}\n## Ally — Consolidated PR Review`, + reason: /omit the `important` count/, + }, + { + // `Number.isInteger(1e100)` is true, so this passed every type check and + // then hung extractAllyReportedFindingRefs, which enumerates 1..count. + // A malformed block must fail the gate, never stall the worker that + // evaluates it. + name: "a finding count past the tracking ceiling", + body: `${verdictBlock({ head: PR1675_HEAD, findings: { critical: 1e100, important: 0 } })}\n## Ally — Consolidated PR Review`, + reason: /exceeds 1000/, + }, + { + name: "a disposition missing its index", + body: `${verdictBlock({ + head: PR1675_HEAD, + findings: { critical: 0, important: 0 }, + dispositions: [{ head: "583085d", severity: "important", verb: "fixed" }], + })}\n## Ally — Consolidated PR Review`, + reason: /dispositions are malformed/, + }, + ]; + + for (const { name, body, reason } of cases) { + it(`reports ${name} as unreadable rather than resolving to success`, () => { + const parsed = parseAllyVerdictBlock(body); + expect(parsed.kind).toBe("unreadable"); + expect(parsed.kind === "unreadable" && parsed.reason).toMatch(reason); + + const verdict = evaluateCommentReviewGate({ + headSha: PR1675_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [allyComment(body, "2026-09-07T15:41:42Z")], + }); + expect(verdict.state).toBe("failure"); + expect(verdict.outcome).toBe("unreadable_verdict"); + }); + } + + it("does not report an unreadable block as carrying a finding", () => { + // AC-3. The distinction is the whole point: "I could not read this" and + // "this carries an unresolved finding" are different claims, and the gate + // may only make the second one when a finding was actually counted. + const body = `\n## Ally — Consolidated PR Review`; + expect(hasActionablePrReviewFeedback(body)).toBe(false); + expect(extractAllyReportedFindingRefs(body)).toBeNull(); + expect( + evaluateCommentReviewGate({ + headSha: PR1675_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [allyComment(body, "2026-09-07T15:41:42Z")], + }), + ).toMatchObject({ outcome: "unreadable_verdict" }); + }); + + it("clears once Ally posts a readable review, so the red is never a dead end", () => { + // Bounded to the newest review on purpose. An unreadable block anywhere in + // history would be unretirable — the trap BLO-31446 and BLO-31947 both + // describe — so the escape route has to be one more review. + const unreadable = `\n## Ally — Consolidated PR Review`; + const readable = `${verdictBlock(PR1675_VERDICT)}\n## Ally — Consolidated PR Review`; + + expect( + evaluateCommentReviewGate({ + headSha: PR1675_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [ + allyComment(unreadable, "2026-09-07T15:41:42Z"), + allyComment(readable, "2026-09-08T00:39:55Z"), + ], + }), + ).toMatchObject({ state: "success", outcome: "clean" }); + }); +}); + +/** + * The block is additive. A body without one must behave exactly as it did + * before, because every review already posted is such a body — if `absent` + * changed behaviour at all, this change would red-wedge the whole open-PR + * population the moment it shipped. + */ +describe("BLO-32695 — block-less bodies keep the prose fallback", () => { + const prose = [ + "## Ally — Consolidated PR Review", + `Reviewed head: ${PR1675_HEAD}`, + "### Prior Findings Dispositioned (1)", + "- **prior:abc1234 important 1** — fixed — re-checked at this head.", + "### Critical Issues (0)", + "### Important Issues (0)", + ].join("\n"); + + it("takes the absent branch", () => { + expect(parseAllyVerdictBlock(prose)).toEqual({ kind: "absent" }); + }); + + it("still parses the attestation, the ledger and the buckets from prose", () => { + expect(extractAllyReviewedHeadSha(prose)).toBe(PR1675_HEAD); + expect(extractAllyPriorFindingDispositions(prose)).toHaveLength(1); + expect(extractAllyReportedFindingRefs(prose)).toEqual([]); + expect(hasActionablePrReviewFeedback(prose)).toBe(false); + }); + + it("still resolves to clean/success", () => { + expect( + evaluateCommentReviewGate({ + headSha: PR1675_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [allyComment(prose, "2026-09-07T15:41:42Z")], + }), + ).toMatchObject({ state: "success", outcome: "clean" }); + }); +}); + +/** + * BLO-33818's instance, and the reason it is a precedence rule and not a guard. + * + * This body carries explicit `Critical Issues (0)` / `Important Issues (0)` and + * an APPROVED state, yet the live gate reported "carries an unresolved + * finding". The trip is the `Recommended Action` template heuristic, whose + * matched span here is: + * + * "Recommended Action\n1. No Critical issues to fix before merge" + * + * Ally negated the boilerplate and it matched anyway — `fix` and `before merge` + * both survive the negation. The negation also sits *inside* the matched span, + * and `hasNonNegatedMatch` only inspects text *preceding* a match, so wrapping + * this clause would not have helped. That is the whole argument for keying on + * the count instead: the template is emitted unconditionally, so no rewording + * of it is separable from a real finding by pattern alone. + */ +describe("BLO-32695 — an explicit zero count outranks the Recommended Action template", () => { + it("does not read the negated boilerplate as a finding", () => { + expect(parseAllyVerdictBlock(PR126_CLEAN_REVIEW_BODY)).toEqual({ kind: "absent" }); + expect(hasActionablePrReviewFeedback(PR126_CLEAN_REVIEW_BODY, "APPROVED")).toBe(false); + }); + + it("still reads the attestation and retires the prior finding", () => { + expect(extractAllyReviewedHeadSha(PR126_CLEAN_REVIEW_BODY)).toBe(PR126_HEAD); + expect(extractAllyPriorFindingDispositions(PR126_CLEAN_REVIEW_BODY)).toEqual([ + { shortSha: "da2b878", severity: "important", index: 1, disposition: "fixed", kind: "retires" }, + ]); + expect(extractAllyReportedFindingRefs(PR126_CLEAN_REVIEW_BODY)).toEqual([]); + }); + + it("resolves to clean/success end to end", () => { + expect( + evaluateCommentReviewGate({ + headSha: PR126_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [allyComment(PR126_CLEAN_REVIEW_BODY, "2026-09-14T02:01:11Z")], + }), + ).toMatchObject({ state: "success", outcome: "clean" }); + }); + + /** + * The fail-closed half. Skipping the template must cost nothing that + * actually carries signal, so each of these still blocks with `(0)` buckets + * present. Without them the change would be a fail-open regression of + * BLO-29711 rather than a precedence rule. + */ + it.each([ + ["a counted bucket above zero", "### Critical Issues (1)\n- boom"], + ["an uncounted findings heading", "### Critical Issues (0)\n### Important Issues\n- boom"], + ["a decision line", "### Critical Issues (0)\n### Important Issues (0)\ndecision: changes_requested"], + ["a changes-requested assertion", "### Critical Issues (0)\n### Important Issues (0)\nChanges requested."], + ])("still blocks on %s", (_label, tail) => { + const body = `## Ally — Consolidated PR Review\nReviewed head: ${PR126_HEAD}\n\n${tail}\n\n### Recommended Action\n1. No Critical issues to fix before merge`; + expect(hasActionablePrReviewFeedback(body)).toBe(true); + }); + + it("keeps the template as a last resort when no bucket is counted at all", () => { + const body = `## Ally — Consolidated PR Review\nReviewed head: ${PR126_HEAD}\n\n### Recommended Action\n1. Fix Critical issues before merge`; + expect(hasActionablePrReviewFeedback(body)).toBe(true); + }); + + /** + * A quoted clean review must not disarm a real one. The fence-stripped pass + * loses the quoted buckets, so the template is consulted there and the OR in + * hasActionablePrReviewFeedback still blocks. + */ + it("does not let a fenced quote of zero counts clear a real boilerplate finding", () => { + const body = [ + "## Ally — Consolidated PR Review", + `Reviewed head: ${PR126_HEAD}`, + "", + "```", + "### Critical Issues (0)", + "### Important Issues (0)", + "```", + "", + "### Recommended Action", + "1. Fix Critical issues before merge", + ].join("\n"); + expect(hasActionablePrReviewFeedback(body)).toBe(true); + }); +}); + +/** + * The producer/consumer heading contract. + * + * Every other reader in this file is gated behind `hasAllyConsolidatedReviewHeading` + * — a body that fails it is not treated as a review at all, so a correct verdict + * block inside it is never even looked for. That makes the heading the one field + * where a producer/consumer mismatch is *silent on both sides*: the block parses + * fine in isolation, and the gate simply never sees the comment. + * + * Asserted against the real exported function rather than a transcribed regex. + * A copy here would be one more prose pattern drifting from its consumer, which + * is the failure mode this whole row exists to retire. + */ +describe("BLO-32695 — the Step 4 template satisfies the heading the gate requires", () => { + const agentsDoc = readFileSync( + fileURLToPath(new URL("../../../.planning/ally-agent/AGENTS.md", import.meta.url)), + "utf8", + ); + + /** The emitted template itself — the fenced block, not the prose around it. */ + function step4Template(): string { + const start = agentsDoc.indexOf("### Step 4"); + expect(start).not.toBe(-1); + const step4 = agentsDoc.slice(start, agentsDoc.indexOf("### Step 5", start)); + const fence = /```markdown\n([\s\S]*?)```/.exec(step4); + expect(fence, "Step 4 must retain its markdown review template").not.toBeNull(); + return fence![1]; + } + + it("is recognised as an Ally consolidated review", () => { + expect(hasAllyConsolidatedReviewHeading(step4Template())).toBe(true); + }); + + it("would catch a template that emits only a friendlier title", () => { + // The regression control. `.planning/ally-agent/AGENTS.md` carried exactly + // this heading and no canonical one, so a review produced from it would have + // been invisible to the gate, to the carried-finding ledger and to same-head + // idempotency. Without this case the assertion above could pass vacuously. + expect( + hasAllyConsolidatedReviewHeading("## 🔍 Automated Review — PR #1721 @ bd489d5"), + ).toBe(false); + }); +}); + +/** + * Peer review of #1721 at 11a52e9a, Critical 1 — the unreadable branch was not + * head-scoped. + * + * `newestAllyConsolidatedReviewComments` has no head filter, so a malformed + * block on a review of head A decided the gate for head B — while the same PR + * carrying no comments at all resolved `not_evaluated`/success. A stale broken + * block was strictly worse for an author than no review. + * + * The scoping added for it is asymmetric, and these cases pin both sides. Only + * a review that *positively names some other tree* is exempt. "Cannot tell + * which head this examined" is an ambiguity, not an exemption, so every case + * in the fail-closed suite above — a sole review of this head whose verdict is + * unreadable — stays red. Relaxing that instead would have satisfied the + * Critical by re-opening AC-5. + */ +describe("BLO-32695 — an unreadable block reds only the head it concerns", () => { + const HEAD_A = PR1675_HEAD; + const HEAD_B = "a".repeat(40); + + /** Unreadable — truncated payload — but it still says which tree it read. */ + const brokenNamingHeadA = [ + ``, + ...PROSE, + ].join("\n"); + const malformedBlock = [``, + "## Ally — Consolidated PR Review", + `Reviewed head: ${HEAD_B}`, + ].join("\n"); + expect(parseAllyVerdictBlock(retiringReadable).kind).toBe("ok"); + const c = "c".repeat(40); + expect( + gateAt(c, [ + allyComment(proseOnly, "2026-09-07T03:46:19Z"), + allyComment(retiringReadable, "2026-09-07T15:41:42Z"), + ]), + ).toMatchObject({ state: "success", outcome: "not_evaluated" }); + }); + + it("does not invent a finding from a malformed block over clean prose", () => { + // AC-3 still holds: falling through reads prose that positively states a + // count, it does not treat the parse failure itself as a finding. + const cleanProse = [ + ``, so the four likeliest prefix drifts missed both patterns and read + * `absent`, silently degrading to the prose path this row retires. + * + * The producer is a model transcribing a template out of a fenced markdown + * example. Pretty-printing a space after the colon is the likeliest single + * drift there is; `v1` is the second. The opener is now version-agnostic, so + * the strict block pattern stays the only reader of the version and every way + * of garbling it lands on `openers > blocks`. + */ +describe("BLO-32695 — prefix drift fails closed rather than vanishing", () => { + const prose = [ + "", + "## Ally — Consolidated PR Review", + `Reviewed head: ${PR1675_HEAD}`, + "### Critical Issues (0)", + "### Important Issues (0)", + ].join("\n"); + + const payload = JSON.stringify(PR1675_VERDICT, null, 2); + + it("reads a space after the colon as the block it plainly is", () => { + const spaced = `${prose}`; + expect(parseAllyVerdictBlock(spaced)).toMatchObject({ kind: "ok" }); + expect(extractAllyReviewedHeadSha(spaced)).toBe(PR1675_HEAD); + }); + + it.each([ + ["a `v`-prefixed version", ``], + ["no version at all", ``], + ])("fails closed on %s rather than falling through to prose", (_label, opener) => { + // Before the fix each of these matched neither pattern, read `absent`, and + // cleared the gate off the very prose the block exists to stop trusting. + expect(parseAllyVerdictBlock(`${opener}${prose}`).kind).toBe("unreadable"); + expect( + evaluateCommentReviewGate({ + headSha: PR1675_HEAD, + reviewerBotLogin: ALLY_BOT_LOGIN, + comments: [ + allyComment(`${opener}${prose}`, "2026-09-07T15:41:42Z"), + // Shadowed, so the unreadable branch is reachable at all — see the + // head-scoping suite above. + allyComment(`## Ally — Consolidated PR Review\nReviewed head: ${PR1675_HEAD}\n### Critical Issues (0)`, "2026-09-07T03:46:19Z"), + ], + }).state, + ).not.toBe("success"); + }); + + it("reads a zero-padded version, so the three readers cannot split on it", () => { + // Suggestion 1 of the same review: the Python sweep compared the version + // as a string while both JS readers use Number(), so `:01` was readable + // here and unreadable there — and the sweep would then re-request a review + // that had already happened. Pinned on both sides. + const padded = `${prose}`; + expect(parseAllyVerdictBlock(padded)).toMatchObject({ kind: "ok" }); + }); + + it("keeps the line anchor: an inline marker is still absent, not an opener", () => { + // Deliberate and unchanged. Un-anchoring would let a review *of this file* + // mint a phantom opener out of a quoted marker and wedge its own gate, + // which is the worse of the two failures. + expect(parseAllyVerdictBlock(`see ", + ].join("\n"), + }, + ], + }); + await expect(githubHasReviewerEvidenceForPr({ repoFullName, prNumber, headSha })).resolves.toEqual({ + found: true, + via: "comment", + }); + }); + + it("BLO-32695: still fails closed when the block and a clean prose line disagree", async () => { + setCreds(); + // The asymmetry is the design: an unreadable prose line falls back to the + // block, but a *readable* one naming another tree is two conflicting + // claims and must not credit either. + stubGithub({ + reviews: [], + comments: [ + { + user: { login: "allyblockcast[bot]" }, + body: [ + "## Ally — Consolidated PR Review", + "", + `Reviewed head: ${"1".repeat(40)}`, + "", + `", + ].join("\n"), + }, + ], + }); + await expect(githubHasReviewerEvidenceForPr({ repoFullName, prNumber, headSha })).resolves.toEqual({ + found: false, + }); + }); + it("accepts the App-prefixed reviewer identity variant", async () => { setCreds(); stubGithub({ diff --git a/server/src/__tests__/pr-comment-review-gate.test.ts b/server/src/__tests__/pr-comment-review-gate.test.ts index 4c1519c480a4..1df9d1c6eb64 100644 --- a/server/src/__tests__/pr-comment-review-gate.test.ts +++ b/server/src/__tests__/pr-comment-review-gate.test.ts @@ -1365,3 +1365,240 @@ describe("clean-review precedence over the Recommended Action prose fallback", ( expect(verdict).toMatchObject({ state: "success", outcome: "clean" }); }); }); + +describe("commit-status description budget", () => { + // GitHub caps a commit-status description at 140 characters and + // `githubPostCommitStatusDetailed` slices to that before the POST, so an + // overlong reason is never rejected — it is silently cut. What gets cut is + // the tail, and the tail of the clean reason is the source attribution, the + // one field that says whether the structured block or the prose fallback + // produced the green (BLO-32695). Losing it is exactly the silent regression + // the attribution exists to make visible. + const MAX = 140; + + // Both branches of the `source` ternary, driven through the real evaluator + // rather than re-rendered here: a copy of the sentence would keep passing + // after the real one grew. + const structured = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [ + allyComment( + [ + "## Ally — Consolidated PR Review", + "", + "", + "", + `Reviewed head: ${CURRENT_HEAD}`, + ].join("\n"), + "2026-08-04T21:09:19Z", + ), + ], + }); + const prose = evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(cleanReview(CURRENT_HEAD), "2026-08-04T21:09:19Z")], + }); + + it("keeps both clean descriptions inside GitHub's cap", () => { + // Positive control: assert we actually exercised both branches, so a + // regression that collapses them to one phrasing cannot pass vacuously. + expect(structured).toMatchObject({ state: "success", outcome: "clean" }); + expect(prose).toMatchObject({ state: "success", outcome: "clean" }); + expect(structured.reason).not.toBe(prose.reason); + + for (const verdict of [structured, prose]) { + expect(verdict.reason.length).toBeLessThanOrEqual(MAX); + } + }); +}); + +/** + * Equal-timestamp ties at each of the three newest-wins scans. + * + * GitHub's created_at is second-resolution and + * executeCommentReviewGateCheck concatenates two independently-ordered + * surfaces, so nothing makes the array order meaningful. Each scan used to end + * in `time >= best`, so a tie handed the verdict to whichever comment the array + * happened to list last — and all three flipped *open*. Every case asserts both + * orders, because a case aimed at one site cannot reach the others: the + * pre-existing multi-comment cases all use distinct timestamps, and the one at + * "an unattested newer review does not displace" pins the attested/unattested + * axis with a 12-hour gap. + */ +describe("equal-timestamp ties resolve to the conservative verdict", () => { + const TIE = "2026-08-04T20:09:19Z"; + const LATER = "2026-08-04T20:09:20Z"; + + const bothOrders = (a: ReturnType, b: ReturnType) => + [ + [a, b], + [b, a], + ].map((comments) => evaluateCommentReviewGate({ headSha: CURRENT_HEAD, comments })); + + it("latestAttestingAllyComment: a finding at this head beats a clean review of the same second", () => { + for (const verdict of bothOrders( + allyComment(blockingReview(CURRENT_HEAD), TIE), + allyComment(cleanReview(CURRENT_HEAD), TIE), + )) { + expect(verdict).toMatchObject({ state: "failure", outcome: "blocking_finding" }); + } + }); + + it("latestAttestingAllyComment: a strictly later clean review still clears it", () => { + // Control. The tie rule must not turn "newest wins" into "a finding always + // wins", which would wedge every PR whose finding was later cleared. + for (const verdict of bothOrders( + allyComment(blockingReview(CURRENT_HEAD), TIE), + allyComment(cleanReview(CURRENT_HEAD), LATER), + )) { + expect(verdict).toMatchObject({ state: "success", outcome: "clean" }); + } + }); + + it("headsWithUndispositionedFinding: a carried finding beats a clean review of the same second", () => { + for (const verdict of bothOrders( + allyComment(blockingReview(OLD_HEAD), TIE), + allyComment(cleanReview(OLD_HEAD), TIE), + )) { + expect(verdict).toMatchObject({ state: "failure", outcome: "carried_finding" }); + } + }); + + it("headsWithUndispositionedFinding: a strictly later clean review of that head still clears it", () => { + for (const verdict of bothOrders( + allyComment(blockingReview(OLD_HEAD), TIE), + allyComment(cleanReview(OLD_HEAD), LATER), + )) { + expect(verdict).toMatchObject({ state: "success", outcome: "not_evaluated" }); + } + }); + + it("headsWithUndispositionedFinding: a tied finding the ledger retires loses nothing", () => { + // The reason the tie is resolved on the final verdict rather than by + // preferring the blocking body inside the loop: preferring it there hands + // the slot to a body isFullyDispositioned filters back out, and the head + // goes green off a candidate that was never consulted. Here the tied + // blocking body is retired by name, so the head clears on its own terms + // rather than because the clean body happened to take the slot. + const comments = [ + allyComment(blockingReview(OLD_HEAD), TIE), + allyComment(cleanReview(OLD_HEAD), TIE), + allyComment(dispositioningReview(INTERMEDIATE_HEAD, OLD_HEAD, "fixed"), LATER), + ]; + + for (const order of [comments, [...comments].reverse()]) { + expect(evaluateCommentReviewGate({ headSha: CURRENT_HEAD, comments: order })).toMatchObject({ + state: "success", + outcome: "not_evaluated", + }); + } + }); + + /** An unreadable verdict block, claiming `headSha` in prose, or no head. */ + const unreadableReview = (headSha: string | null) => + [ + "## Ally — Consolidated PR Review", + "", + ...(headSha ? [`Reviewed head: ${headSha}`] : []), + ].join("\n"); + + it("newestAllyConsolidatedReviewComments: an unreadable block beats a clean review of the same second", () => { + for (const verdict of bothOrders( + allyComment(unreadableReview(CURRENT_HEAD), TIE), + allyComment(cleanReview(CURRENT_HEAD), TIE), + )) { + expect(verdict).toMatchObject({ state: "failure", outcome: "unreadable_verdict" }); + } + }); + + /** + * The tie-break must be the caller's *whole* predicate, not a subset of it. + * + * newestAllyConsolidatedReviewComments picked one unreadable review out of the + * tied set, but evaluateCommentReviewGate then requires that review to also + * be in scope for this head. Where two unreadable reviews tie, the subset + * predicate could hand the slot to the out-of-scope one, whose claim the + * caller discards — and the in-scope unreadable review is never examined. + * Both flips fail open, which is the BLO-29711 direction (Ally, #1721 at + * 9fd4b499). A null claim is in scope: "cannot tell which head this + * examined" is an ambiguity, not an exemption. + */ + it("newestAllyConsolidatedReviewComments: an out-of-scope unreadable review cannot take the slot", () => { + for (const inScope of [unreadableReview(CURRENT_HEAD), unreadableReview(null)]) { + for (const verdict of bothOrders( + allyComment(unreadableReview(OLD_HEAD), TIE), + allyComment(inScope, TIE), + )) { + expect(verdict).toMatchObject({ state: "failure", outcome: "unreadable_verdict" }); + } + } + }); + + it("newestAllyConsolidatedReviewComments: two unreadable reviews of other heads stay out of scope", () => { + for (const verdict of bothOrders( + allyComment(unreadableReview(OLD_HEAD), TIE), + allyComment(unreadableReview(INTERMEDIATE_HEAD), TIE), + )) { + expect(verdict).toMatchObject({ state: "success", outcome: "not_evaluated" }); + } + }); + + /** An unreadable block of an unsupported version — a *different* reason. */ + const unsupportedVersionReview = (headSha: string) => + [ + "## Ally — Consolidated PR Review", + "", + `Reviewed head: ${headSha}`, + ].join("\n"); + + /** + * The last place array order was visible in output. Two in-scope unreadable + * reviews tied at one second agree on {state, outcome} but not on + * `block.reason`, and that string is the commit-status description and the + * check-run summary — so the same input named a different cause on each run + * (Ally, #1721 at 31532b48). Asserts the *property* (order-independence) + * rather than a chosen string, so reversing the comparator's sense cannot + * silently keep this green. + */ + it("the unreadable cause is named deterministically", () => { + const reasons = bothOrders( + allyComment(unreadableReview(CURRENT_HEAD), TIE), + allyComment(unsupportedVersionReview(CURRENT_HEAD), TIE), + ).map((verdict) => { + expect(verdict).toMatchObject({ state: "failure", outcome: "unreadable_verdict" }); + return verdict.reason; + }); + + expect(reasons[0]).toBe(reasons[1]); + // Both causes are genuinely reachable from this input, so the assertion + // above is not vacuously satisfied by one of them never being produced. + expect(reasons[0]).toMatch(/not valid JSON|unsupported ally-verdict version/); + }); + + /** + * The cross-head sort is not a verdict, but it is output. Both entries carry + * a finding, so `failure/carried_finding` is stable either way — the head + * named in the commit-status text was not (Ally, #1721 at 9fd4b499). + */ + it("headsWithUndispositionedFinding: the carried head is named deterministically", () => { + const named = bothOrders( + allyComment(blockingReview(OLD_HEAD), TIE), + allyComment(blockingReview(INTERMEDIATE_HEAD), TIE), + ).map((verdict) => { + expect(verdict).toMatchObject({ state: "failure", outcome: "carried_finding" }); + return verdict.outcome === "carried_finding" ? verdict.carriedFromHeadSha : null; + }); + + expect(named[0]).toBe(named[1]); + }); +}); diff --git a/server/src/services/ally-review-detection.ts b/server/src/services/ally-review-detection.ts index 04bccbf7897e..d149b8ce1d9b 100644 --- a/server/src/services/ally-review-detection.ts +++ b/server/src/services/ally-review-detection.ts @@ -168,9 +168,10 @@ const REVIEWED_HEAD_ATTESTATION_PATTERN = new RegExp( "gi", ); -export function extractAllyReviewedHeadSha(body: string | null | undefined): string | null { - const text = emittedReviewText(body); - if (text === null) return null; +// The prose attestation, when the body states exactly one. Ambiguity — none, +// or several — is not an answer, because this decides which tree a required +// check is set against. +function soleProseAttestedHead(text: string): string | null { const attestations = Array.from( text.matchAll(REVIEWED_HEAD_ATTESTATION_PATTERN), (match) => match[1]!.toLowerCase(), @@ -178,6 +179,463 @@ export function extractAllyReviewedHeadSha(body: string | null | undefined): str return attestations.length === 1 ? attestations[0]! : null; } +export function extractAllyReviewedHeadSha(body: string | null | undefined): string | null { + // The structured block wins outright when present: it says which tree was + // examined as a field, so no amount of prose around it can move the answer. + const block = parseAllyVerdictBlock(body); + if (block.kind === "ok") return block.verdict.head; + // An unreadable block attests nothing, and unlike its two sibling readers + // this one does *not* fall through to prose. Attesting is how a review + // retires a prior head's finding, so reading prose here would let a body we + // failed to parse dispose of a live finding — the one direction that loses + // information. Carrying is the opposite trade and falls through; see + // hasActionablePrReviewFeedback. + if (block.kind === "unreadable") return null; + const text = emittedReviewText(body); + if (text === null) return null; + return soleProseAttestedHead(text); +} + +/** + * Ally's structured verdict block — the primary source, with the prose parsers + * below retained only as the fallback for a body that carries no block. + * + * Why a block at all (BLO-32695). Every prose pattern in this file was widened + * in response to a real review it could not read, each widening was correct, + * and the family still grew: BLO-29711, BLO-31730, BLO-31947, BLO-31446. The + * measurement that ended it was a single clean review — paperclip#1675 at + * 2026-09-07T15:41:42Z, 0 Critical / 0 Important — failing *four* independent + * patterns at once, for four unrelated reasons: a parenthetical after the + * attested SHA, a bolded ledger verb, a comma where a dash was required, and a + * hyphenated severity. Two separate clearing paths existed and prose + * formatting closed both, so the gate reported a finding Ally had already + * withdrawn. The space of English an author might write is unbounded; the + * space this file can enumerate is not. + * + * An HTML comment rather than a fenced block, deliberately. A fenced + * ```ally-verdict payload would be blanked by withoutFencedCodeBlocks before + * any parser saw it — the block would be invisible to exactly the predicates + * it exists to serve. The marker is read from *emitted* text for the same + * reason the retiring predicates are: a review quoted inside a fence must + * never retire a live finding, and fencing is how a body gets quoted. + * + * `ally-verdict:` is not a new token. The bundled github-pr-workflow skill + * already reserves it to the reviewer service and forbids ordinary agents from + * posting one (SKILL.md), so the marker namespace this reads was spoken for + * before it was parsed. + * + * ⚠ THE BLOCK MUST BE ADDITIVE, NOT A REPLACEMENT. Four independent readers + * parse the `Reviewed head:` attestation and only this one understands the + * block: + * + * 1. this module + * 2. `commentAttestsHead` in server/src/services/github-app-auth.ts + * 3. `ATTESTED_HEAD_RE` in scripts/check-ally-review-consistency.mjs + * 4. `REVIEWED_HEAD_PATTERN` in .github/scripts/sweep-stalled-ally-reviews.py + * + * A review carrying a block *and* the prose line reads identically to all + * four, so adding the block breaks nothing. A review carrying only a block + * would attest nothing to readers 2-4 — reader 2 would raise + * `pr_review_output_missing` and post a false "reviewer never finished". So + * whoever changes Ally's emitting side must keep the prose attestation until + * all four read the block; BLO-31730 was already one instance of two of these + * parsers disagreeing, and this is the same hazard with more copies. + * + * Line-anchored and guarded like every prose pattern in this file, and for a + * sharper reason than they have. Fencing is not the only way to quote: an + * indented example, an inline-code mention, and a blockquoted prior review all + * survive withoutFencedCodeBlocks, and an unanchored opener reads each of them + * as a *second* block — which is the fail-closed two-blocks red. So the + * quoting forms this pattern must reject are the ones a reviewer reaches for + * when discussing the block format itself, on a parser whose own reviews are + * the likeliest place that discussion happens. Left unanchored, a review of + * this file wedges its own gate. + * + * Recoverable rather than a wedge — it fails closed and the unreadable check + * is scoped to the newest review, so one more readable review clears it — but + * the round trip would be a confusing one to debug, and the anchor is free. + * + * ⚠ A payload may not contain `-->`: the capture is non-greedy, so an + * embedded terminator truncates the JSON and the block reads `unreadable`. No + * current field can carry one; a future free-text field (a `reason`, a `file` + * holding a diff hunk or a regex) could, and would have to encode it. + */ +const ALLY_VERDICT_BLOCK_PATTERN = new RegExp( + String.raw`^${NOT_INDENTED_CODE}(?![ \t]*>) {0,3}`, + "gm", +); + +// The opener alone, anchored identically to the block above so the two agree +// on what they are looking at. Counting openers is what distinguishes "Ally +// tried to state a verdict and the payload is broken" from "this review +// predates the block" — the pattern above cannot tell them apart, because an +// unterminated marker simply fails to match and reads as `absent`. +// +// It matters because `absent` falls through to the prose parser. A body whose +// block is truncated but whose prose happens to read clean would clear the +// gate on the strength of the very prose the block exists to stop trusting, +// which is a fail-open path through the fail-closed branch. +// +// So the opener is deliberately laxer than the block: everything after +// `ally-verdict` is dropped, including the version and its colon. The block +// pattern is the strict reader, and every way of garbling the prefix that the +// opener still recognizes — `ally-verdict:v1`, a version-less +// `ally-verdict {…}` — lands on `openers > blocks` and fails closed, rather +// than missing both patterns and vanishing into `absent`. That matters because +// the emitter is a model transcribing a template out of a fenced example, so +// prefix drift is the likeliest drift there is; the two patterns previously +// shared the `:(\d+)` prefix, which meant any drift in it moved them together +// and the guard could not fire. +// +// Still anchored to the line start, and that bound is kept: an inline +// `… prose. ` reads `absent`. Un-anchoring would let +// a review *of this file* mint a phantom opener out of a quoted marker and +// wedge its own gate, which is the worse failure. +const ALLY_VERDICT_OPENER_PATTERN = new RegExp( + String.raw`^${NOT_INDENTED_CODE}(?![ \t]*>) {0,3}` terminator. A payload + * Ally failed to serialize is not the same fact as a review that predates + * the block — and only the latter may use the prose path. + * + * Note the asymmetry with the prose fallback: an unreadable *block* is red, + * whereas an unreadable *body* with no block keeps the historical behavior. + * That is intentional. Every review posted before this shipped carries no + * block, so `absent` must stay non-blocking or the gate would red-wedge the + * whole open-PR population on arrival. + */ +export function parseAllyVerdictBlock(body: string | null | undefined): AllyVerdictBlockParse { + const text = emittedReviewText(body); + if (text === null) return { kind: "absent" }; + const blocks = [...text.matchAll(ALLY_VERDICT_BLOCK_PATTERN)]; + // Openers without a matching complete block mean Ally tried to state a + // verdict and the payload did not survive — a missing `-->`, or a version + // the strict pattern rejects (`:v1`, or none at all). Not an older review. + // Checked before the `absent` return so a broken block can never fall + // through to the prose parser it exists to replace. + const openers = [...text.matchAll(ALLY_VERDICT_OPENER_PATTERN)]; + if (openers.length > blocks.length) { + return { + kind: "unreadable", + reason: `${openers.length - blocks.length} ally-verdict opener(s) state no readable version or have no \`-->\` terminator`, + }; + } + if (blocks.length === 0) return { kind: "absent" }; + if (blocks.length > 1) { + return { kind: "unreadable", reason: `${blocks.length} ally-verdict blocks; expected exactly one` }; + } + + const [, rawVersion, rawPayload] = blocks[0]!; + if (Number(rawVersion) !== SUPPORTED_ALLY_VERDICT_VERSION) { + return { kind: "unreadable", reason: `unsupported ally-verdict version ${rawVersion}` }; + } + + let parsed: unknown; + try { + parsed = JSON.parse(rawPayload!.trim()); + } catch { + return { kind: "unreadable", reason: "ally-verdict payload is not valid JSON" }; + } + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { + return { kind: "unreadable", reason: "ally-verdict payload is not a JSON object" }; + } + + const { head, findings, dispositions } = parsed as Record; + if (typeof head !== "string" || !/^[0-9a-f]{40}$/i.test(head.trim())) { + return { kind: "unreadable", reason: "ally-verdict block attests no complete head SHA" }; + } + if (findings === undefined) { + return { kind: "unreadable", reason: "ally-verdict block states no findings counts" }; + } + const counts = asSeverityCounts(findings); + if (typeof counts === "string") return { kind: "unreadable", reason: counts }; + const ledger = asDispositions(dispositions); + if (!ledger) return { kind: "unreadable", reason: "ally-verdict dispositions are malformed" }; + + // A readable prose attestation naming a *different* head is two claims about + // which tree was examined. Fail closed instead of silently picking one. + // + // Asymmetric on purpose: only a *disagreement* is fatal. An unreadable or + // absent prose line is not, because that is the #1675 case this block exists + // to survive — requiring the prose to parse would put the retired regex back + // on the critical path and undo the whole change. The measurement is in this + // repo: `extractAllyReviewedHeadSha` on the verbatim #1675 body is `null` + // (ally-review-verdict-block.test.ts), so under a stricter rule here a review + // carrying a block *and* a #1675-shaped prose line would read `unreadable` — + // the exact false red this row retires, reintroduced one layer down. + // + // This rule is no longer this module's alone. All four readers of an Ally + // body now apply it, so none of them can attest a tree the others do not: + // `commentAttestsHead` in github-app-auth.ts delegates here outright, + // and `attestedHead`/`canonicalReviewHead` in + // scripts/check-ally-review-consistency.mjs plus `parse_reviewed_head` in + // .github/scripts/sweep-stalled-ally-reviews.py mirror it in their own + // languages, pinned by tests alongside each. Block additivity stays a + // producer invariant ("the verdict block is additive, never a replacement + // for the prose line", scripts/ally-agent-idempotency-contract.test.mjs), + // but the readers no longer *depend* on the producer honouring it — which + // matters, because the producer is a model following a prompt rather than a + // serializer, and #1675 is the existence proof that its prose drifts. + const attestedHead = head.trim().toLowerCase(); + const proseHead = soleProseAttestedHead(text); + if (proseHead !== null && proseHead !== attestedHead) { + return { + kind: "unreadable", + reason: + `ally-verdict head ${attestedHead.slice(0, 7)} disagrees with the prose ` + + `attestation ${proseHead.slice(0, 7)}`, + }; + } + + const countDisagreement = proseCountContradicting(text, counts); + if (countDisagreement !== null) return { kind: "unreadable", reason: countDisagreement }; + + return { + kind: "ok", + verdict: { head: attestedHead, findings: counts, dispositions: ledger }, + }; +} + +/** + * Best-effort: the head a body *claims* to have examined, even when its + * verdict block is unreadable. + * + * Deliberately not `extractAllyReviewedHeadSha`, which returns null for an + * unreadable block and must keep doing so — an unreadable verdict attests + * nothing, and letting one attest a head would let a broken block clear or + * carry findings. This answers a different, weaker question: *which tree was + * this review looking at*, for the sole purpose of deciding whether an + * unreadable verdict is relevant to the head being evaluated. + * + * Weaker on purpose, so the two cannot be confused at a call site: the result + * is never used as an attestation, only to establish that a review is about + * some *other* tree. A null answer therefore means "cannot tell", and the + * caller must fail closed on it. + * + * Reads every head claim the body makes — each block's `head` field, plus the + * prose line — and answers only when they agree. Unanimity rather than + * first-wins because two of the `unreadable` reasons above literally *are* + * "this body makes more than one head claim" (a block disagreeing with the + * prose line, and two blocks), and picking a winner among claims the parse + * deliberately declined to pick among invents an answer the body never gave. + * When the invented answer is some other head, the caller stands down and a + * review carrying a structured Critical goes invisible at the head it + * concerns — #1675 again, in the fail-open direction. + * + * A disagreeing body therefore reds every head, including ones it has nothing + * to do with. That is the same rule already applied to a body that claims no + * head at all, and for the same reason: a review that will not say which tree + * it examined might have examined this one. + */ +export function allyClaimedReviewHead(body: string | null | undefined): string | null { + const text = emittedReviewText(body); + if (text === null) return null; + const claims = new Set(); + for (const [, , rawPayload] of text.matchAll(ALLY_VERDICT_BLOCK_PATTERN)) { + try { + const parsed: unknown = JSON.parse(rawPayload!.trim()); + if (parsed !== null && typeof parsed === "object" && !Array.isArray(parsed)) { + const { head } = parsed as Record; + if (typeof head === "string" && /^[0-9a-f]{40}$/i.test(head.trim())) { + claims.add(head.trim().toLowerCase()); + } + } + } catch { + // A payload that does not parse states no head. The claims it does not + // make cannot disagree with anything; the rest of the body still decides. + } + } + const proseHead = soleProseAttestedHead(text); + if (proseHead !== null) claims.add(proseHead); + return claims.size === 1 ? claims.values().next().value! : null; +} + +/** + * The head rule above, applied to the field that actually decides + * `blocking_finding`. + * + * The counts outrank every prose clause in `hasActionablePrReviewFeedback` — + * that is the point of the block — so without this a body whose block states + * zero while its own prose enumerates `### Critical Issues (2)` resolves + * `clean`/`success`. That is the BLO-29711 direction arriving through the + * structured path, and it arrives silently: a green, not a red. + * + * The premise is the same one the head rule rests on. The producer is a model + * following a prompt rather than a serializer, so a prompt edit that renumbers + * or renames a count field lands before the parser that understands it does + * (the prompt is `.planning/ally-agent/AGENTS.md`, which takes effect on + * merge; this file ships on the server's own rollout). Disagreement between + * the two things the same review says is the only signal available in that + * window. + * + * Asymmetric exactly like the head rule, and for the same reason: only a + * *positive* prose count against a stated zero is fatal. An absent or + * unparseable bucket is the #1675 case the block exists to survive, and a + * block reporting more than the prose does cannot fail open. The #1675 body + * reads `Critical Issues (0)` / `Important Issues (0)`, so this never fires on + * it — the fixture is the control. + * + * Reads the emitted text, so a quoted or fenced bucket cannot fail a block + * closed. Unlike `hasActionablePrReviewFeedback`, which reads the raw body + * too, the fail-open direction here is already covered: the block itself is + * the claim, and this only cross-checks it. + */ +function proseCountContradicting(text: string, counts: Map): string | null { + for (const [, severity, count] of text.matchAll(EMITTED_COUNTED_FINDINGS_BUCKET_PATTERN)) { + const key = severity!.toLowerCase(); + if (!BLOCKING_SEVERITIES.has(key)) continue; + if (Number(count) > 0 && counts.get(key) === 0) { + return `ally-verdict states 0 \`${key}\` but the review enumerates ${count}`; + } + } + return null; +} + // Negation cues flip an otherwise-actionable bare phrase into a confirmation // that no follow-up is required. Limit the lookback to the local sentence so // an unrelated earlier negation does not mask a real later finding. @@ -254,6 +712,73 @@ const COUNTED_FINDINGS_BUCKET_PATTERN = new RegExp( "gi", ); +// The same buckets, but only where the review *emits* one as a heading of its +// own — the form that states what this review found, as opposed to a sentence +// mentioning what some earlier pass found. +// +// proseCountContradicting must not share the unanchored pattern above. +// `extractAllyReportedFindingRefs` wants a superset and over-matching there +// only carries extra findings forward; here over-matching fails a *clean* +// review closed, which is the false red this row exists to retire. A sentence +// such as "the previous pass reported Critical Issues (2)" reads as a +// contradiction of a block stating zero, and so does a blockquoted or +// inline-code bucket — which is why every sibling pattern in this file carries +// its own `(?![ \t]*>)` and indentation bound, and this one now does too. +// +// Strict because the emitted form measured strict: across six recent PRs' +// Ally bodies, all 64 genuinely emitted buckets are `### Issues +// (N)` — heading, line-anchored, bucket ending the line — and every match that +// was not one of those was prose, a fenced example, a blockquote, or inline +// code. Emphasis is allowed around the heading because Ally has chosen it +// elsewhere. If the emitted form ever grows decoration this does not cover, +// the rule stops firing and the block is trusted as it was before this +// cross-check existed; that degrades to the prior behaviour rather than +// opening something new, whereas a loose pattern reds clean reviews. +const EMITTED_COUNTED_FINDINGS_BUCKET_PATTERN = new RegExp( + String.raw`^${NOT_INDENTED_CODE}(?![ \t]*>) {0,3}(?:#{1,6}[ \t]*)?[*_]{0,3}` + + String.raw`(Critical|Important)[ \t]+Issues[ \t]*[*_]{0,3}[ \t]*\((\d+)\)[*_]{0,3}[ \t]*$`, + "gim", +); + +// The severities that block a merge, named once so the structured path and the +// prose path cannot disagree about the vocabulary. The prose readers get this +// bound for free from COUNTED_FINDINGS_BUCKET_PATTERN's alternation, which +// enumerates the two blocking buckets and no others; the block reader has no +// such pattern to inherit it from, so it consults this set explicitly. +// +// Naming it matters because the two paths acquire the bound by different +// means. Ally's template mandates a third count, `suggestions`, so a block +// reader that blocks on "any positive count" reds the *most common* review +// shape — clean, with suggestions — while the prose reader it replaces calls +// the same review clean. The divergence also reaches extractAllyReportedFindingRefs: +// a `suggestions` ref can never be retired, because the ledger vocabulary only +// ever dispositions Critical/Important, so the head would carry forever. That +// is the unretirable trap BLO-31446/BLO-31947 exist for, and reintroducing it +// through the new path would make this replacement worse than the prose +// parsing it retires. +// +// A severity added here must be one Ally's ledger can name in a disposition, +// or it re-opens the unretirable carry from the other direction. +const BLOCKING_SEVERITIES: ReadonlySet = new Set(["critical", "important"]); + +// A finding count is a review's tally of one bucket, not an arbitrary integer. +// Both ref loops in extractAllyReportedFindingRefs enumerate 1..count, so an +// unbounded count is a hang: `1e100` from a structured block, `(99999999999)` +// from a prose bucket heading. The structured path rejects anything past this +// (fail closed, the producer is ours); the prose path clamps, because there the +// refs are already a deliberate superset and a review with 1000 open findings +// in one bucket is not a shape worth reddening a PR over. +const MAX_VERDICT_FINDING_COUNT = 1000; + +// The full vocabulary a structured `findings` object may name. Derived from +// BLOCKING_SEVERITIES so the subset relation cannot drift: adding a blocking +// severity above automatically makes it readable here, and the only extra is +// `suggestions`, which reads but never blocks. +// +// Anything outside this set makes the block `unreadable` — see asSeverityCounts +// for why dropping an unknown key is a fail-open rather than a nicety. +const VERDICT_SEVERITIES: ReadonlySet = new Set([...BLOCKING_SEVERITIES, "suggestions"]); + // Ally's disposition vocabulary is three words: `fixed` and // `no-longer-applicable` retire a prior finding, `still-present` asserts it // stands. That matches scripts/check-ally-review-consistency.mjs, which treats @@ -327,6 +852,23 @@ export function classifyPriorDisposition(disposition: string): PriorDispositionK export function extractAllyPriorFindingDispositions( body: string | null | undefined, ): AllyPriorFindingDisposition[] { + // Structured dispositions carry head/severity/index/verb as discrete fields, + // so the four prose shapes that silently dropped whole ledger bullets + // (bolded verb, comma instead of a dash, hyphenated severity, a trailing + // parenthetical) cannot arise. The verb vocabulary is unchanged: a verb + // arriving as a field is still classified by classifyPriorDisposition, so an + // unknown one still fails closed rather than retiring anything. + const block = parseAllyVerdictBlock(body); + if (block.kind === "ok") { + return block.verdict.dispositions.map((entry) => ({ + shortSha: entry.head, + severity: entry.severity, + index: entry.index, + disposition: entry.verb, + kind: classifyPriorDisposition(entry.verb), + })); + } + if (block.kind === "unreadable") return []; const text = emittedReviewText(body); if (text === null) return []; const entries: AllyPriorFindingDisposition[] = []; @@ -381,6 +923,26 @@ export function extractAllyPriorFindingDispositions( export function extractAllyReportedFindingRefs( body: string | null | undefined, ): AllyFindingRef[] | null { + // Counts as fields, so the raw/stripped max below — which exists purely to + // stop a fence from swallowing a bucket — has nothing to guard against. + const block = parseAllyVerdictBlock(body); + if (block.kind === "ok") { + const refs: AllyFindingRef[] = []; + for (const [severity, count] of block.verdict.findings) { + // Only the blocking severities have finding identities a ledger entry + // can retire. Minting a ref for `suggestions` would carry the head + // forever, since no disposition verb ever names one. + if (!BLOCKING_SEVERITIES.has(severity)) continue; + for (let index = 1; index <= count; index += 1) refs.push({ severity, index }); + } + return refs; + } + // `unreadable` deliberately falls through to the prose enumeration below, for + // the reason spelled out in hasActionablePrReviewFeedback: a block we cannot + // read must not reduce what the gate blocks on. These two have to move + // together — the carried-finding path asks *both* whether a review blocks and + // which identities it raised, so a body that blocks here while enumerating + // `null` there would carry a head no ledger entry could ever retire. if (typeof body !== "string") return null; // Highest count seen per severity, across both readings. Findings are @@ -390,7 +952,10 @@ export function extractAllyReportedFindingRefs( 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))); + highestCount.set( + key, + Math.min(Math.max(highestCount.get(key) ?? 0, Number(count)), MAX_VERDICT_FINDING_COUNT), + ); } } if (highestCount.size === 0) return null; @@ -527,6 +1092,51 @@ export function hasActionablePrReviewFeedback( const normalizedState = state?.trim().toLowerCase(); if (normalizedState === "changes_requested" || normalizedState === "changes-requested") return true; if (typeof body !== "string") return false; + + // With a structured block, the blocking-severity counts decide and nothing + // else is consulted. This is the AC-3 half of BLO-32695: `blocking_finding` + // becomes reachable only from a finding Ally actually counted, never from + // prose that merely reads as actionable. The clauses below stay for + // block-less bodies, where dropping them would fail open. + const block = parseAllyVerdictBlock(body); + if (block.kind === "ok") { + for (const [severity, count] of block.verdict.findings) { + if (BLOCKING_SEVERITIES.has(severity) && count > 0) return true; + } + return false; + } + // An unreadable block falls through to prose rather than answering "no + // finding". Returning false here was a fail-open regression against master, + // found in peer review of #1721 at 97b4ddd1: `evaluateCommentReviewGate` + // catches an unreadable block under its own outcome, but that branch is + // head-scoped, so a review of an *earlier* head became invisible and the + // finding it carried was not carried. Same body, same prose, same finding, + // with only the block varying: + // + // prose only (master) -> failure/carried_finding + // prose + well-formed block -> failure/carried_finding + // prose + malformed block -> success/not_evaluated <- the hole + // + // The trigger is the upgrade path this file documents, not contrived damage: + // a version bump makes every body unreadable until the rollout catches up, + // and then one push past the reviewed head turns the red green with the + // Critical still open — BLO-29711's direction, in the direction the author + // benefits from. So this is the module's own rule ("quoted text may never + // reduce what the gate blocks on") one layer out: an unreadable block may not + // reduce it either. Falling through costs at most a false red, which is + // visible and recoverable, and it is what master already does with the same + // prose. + // + // Asymmetric with extractAllyReviewedHeadSha, which still returns null here, + // and the asymmetry is the point: an unreadable verdict may still *carry* a + // finding, but it may never *retire* one. Attesting is how a review disposes + // of a prior head, so a body we could not parse must not be able to. + // + // AC-3 is intact. It forbids reaching `blocking_finding` from a failure to + // parse *prose* — a clean body the regex could not read. This path is the + // opposite: prose that positively states a count, on a body whose structured + // block is the part that failed. At the head under evaluation the + // unreadable_verdict branch still runs first and still wins. const text = body.trim(); if (!text) return false; diff --git a/server/src/services/github-app-auth.ts b/server/src/services/github-app-auth.ts index 974950a7ba1b..20542caec959 100644 --- a/server/src/services/github-app-auth.ts +++ b/server/src/services/github-app-auth.ts @@ -485,6 +485,20 @@ export async function githubFetchPrAuthorLogin(input: { * would let a bare `Reviewed head:` line bind to an unrelated 40-hex on the * following line — setting a required check on a guess, which is the one thing * the exactly-one rule exists to prevent. + * + * Sharing the grammar is also what BLO-32695 needs one layer down. While this + * file carried its own heading and attestation regexes, a block-backed review + * could be credited by the merge gate while reading as "no review at head" + * here — the #1675 false red, reproduced in a second parser. The shared heading + * pattern is the wider of the two (it admits the bold and alternate-dash forms + * Ally actually emits); forgery is not the risk it guards, because + * `githubHasReviewerEvidenceForPr` credits a body only after matching the + * reviewer App identity. + * + * An unreadable verdict block yields false here, exactly as it yields a + * non-`success` verdict in the gate: no evidence re-runs the reviewer, whereas + * crediting a verdict nothing could parse would let a run that died mid-flow + * self-attest. */ const commentAttestsHead = (body: string, head: string): boolean => hasAllyConsolidatedReviewHeading(body) && extractAllyReviewedHeadSha(body) === head; diff --git a/server/src/services/pr-comment-review-gate.ts b/server/src/services/pr-comment-review-gate.ts index 8b280c71303f..ac7d1226c345 100644 --- a/server/src/services/pr-comment-review-gate.ts +++ b/server/src/services/pr-comment-review-gate.ts @@ -18,8 +18,10 @@ import { extractAllyPriorFindingDispositions, extractAllyReportedFindingRefs, extractAllyReviewedHeadSha, + allyClaimedReviewHead, hasActionablePrReviewFeedback, hasAllyConsolidatedReviewHeading, + parseAllyVerdictBlock, type AllyFindingRef, type AllyPriorFindingDisposition, } from "./ally-review-detection.js"; @@ -72,6 +74,13 @@ export type CommentReviewGateOutcome = | "blocking_finding" /** No comment attests this head, but a finding from an earlier head stands undispositioned. */ | "carried_finding" + /** + * The newest Ally review carries a structured verdict block this parser + * cannot read. Distinct from every other outcome on purpose: it is neither + * evidence of review nor evidence of a finding, and conflating it with + * either is the misreport BLO-32695 exists to end. + */ + | "unreadable_verdict" /** Nothing established a comment-shaped review of this head. Not evidence of review. */ | "not_evaluated"; @@ -79,6 +88,7 @@ export type CommentReviewGateVerdict = | { state: "success"; outcome: "clean"; reason: string } | { state: "success"; outcome: "not_evaluated"; reason: string } | { state: "failure"; outcome: "blocking_finding"; reason: string; commentCreatedAt: string } + | { state: "failure"; outcome: "unreadable_verdict"; reason: string; commentCreatedAt: string } | { state: "failure"; outcome: "carried_finding"; @@ -91,6 +101,78 @@ function toEpochMs(value: string | Date): number { return value instanceof Date ? value.getTime() : Date.parse(value); } +/** + * Every candidate tied at the top of a newest-wins scan, by a lexicographic + * numeric precedence key. + * + * Shared by all three scans below, which is the point of it (CTO ruling on + * BLO-32695). Each of them used to end in `time >= best`, and each therefore + * let array order decide the verdict whenever two comments shared a second: + * latestAttestingAllyComment flipped clean/blocking_finding, + * headsWithUndispositionedFinding flipped not_evaluated/carried_finding, + * newestAllyConsolidatedReviewComments flipped clean/unreadable_verdict — all + * three reproduced in both orders, all three fail *open*. Nothing establishes + * that order: executeCommentReviewGateCheck concatenates two independently + * ordered GitHub surfaces, and GitHub's created_at is second-resolution, so a + * tie is not a corner case of the data, it is the expected collision. + * + * A precedence tuple is order-independent only while its final axis is a strict + * total order, and a second-resolution timestamp is not one. So the comparison + * is strict (`>`) and the tie is not resolved here at all: this returns the + * whole tied set and each caller then picks the more conservative candidate — + * `unreadable` > `finding` > `clean`, which is order-independent by + * construction and is what AC-5 already requires. Resolving it here instead + * would need a per-site notion of "conservative" threaded through as a + * parameter, which is the three-comparators-that-drift shape this replaces. + * + * A stable sort is not an alternative: executeCommentReviewGateCheck's `.map()` + * keeps only authorLogin/body/createdAt, so no comment id reaches these scans. + */ +function topTiedBy(items: T[], key: (item: T) => number[]): T[] { + let best: number[] | null = null; + let tied: T[] = []; + for (const item of items) { + const candidate = key(item); + const cmp = best === null ? 1 : compareKeys(candidate, best); + if (cmp > 0) { + best = candidate; + tied = [item]; + } else if (cmp === 0) { + tied.push(item); + } + } + return tied; +} + +function compareKeys(a: number[], b: number[]): number { + // Equal length is the invariant, not an assumption: this loop runs to + // `a.length`, so a shorter key with an equal prefix reports a *tie* against a + // longer one that outranks it, while the reverse order compares `undefined` + // and reports `-1`. Both are silent — in a helper whose whole purpose is + // making a tie decidable. + // + // Exactly one shape reaches this throw: a `key` whose output length is + // *data-dependent*, e.g. `(c) => c.attested ? [1, c.timeMs] : [c.timeMs]`. + // That is one edit away from the live key at :412, which keeps its ternary + // *inside* the tuple precisely so the length cannot vary with the item. + // + // Lengthening one call site's key does NOT reach it, and believing otherwise + // misreads the helper: compareKeys has a single caller (:136), where both + // arguments are outputs of the same `key` within the same topTiedBy + // invocation. Keys from different call sites are never compared, so a longer + // tuple changes that site's own comparisons uniformly and meets no other. + // topTiedBy does not relate its call sites to one another (Ally, #1721 at + // 31532b48). Unreachable today: all three keys return unconditional array + // literals. + if (a.length !== b.length) { + throw new Error(`compareKeys requires equal-length keys, got ${a.length} and ${b.length}`); + } + for (let i = 0; i < a.length; i += 1) { + if (a[i]! !== b[i]!) return a[i]! > b[i]! ? 1 : -1; + } + return 0; +} + function isAllyConsolidatedReviewComment( comment: CommentReviewGateComment, reviewerBotLogin: string, @@ -128,8 +210,7 @@ function latestAttestingAllyComment( reviewerBotLogin: string, matchHeadSha: string, ): AttestingComment | null { - let latest: AttestingComment | null = null; - let latestTime = -Infinity; + const candidates: { attesting: AttestingComment; timeMs: number }[] = []; for (const comment of comments) { if (!isAllyConsolidatedReviewComment(comment, reviewerBotLogin)) continue; @@ -139,14 +220,18 @@ function latestAttestingAllyComment( const commentTime = toEpochMs(comment.createdAt); if (!Number.isFinite(commentTime)) continue; - // GitHub's issue-comment endpoint is chronological. Prefer the later item - // when two comments share its second-resolution created_at timestamp. - if (commentTime >= latestTime) { - latest = { comment, attestedHeadSha }; - latestTime = commentTime; - } + candidates.push({ attesting: { comment, attestedHeadSha }, timeMs: commentTime }); } - return latest; + + // Strictly newest wins; on an exact tie the finding wins over the clean + // review. Two comments at the same second, one carrying a finding and one + // clean, used to go green with the finding open whenever the finding + // happened to come first in the array. + const tied = topTiedBy(candidates, (candidate) => [candidate.timeMs]); + const chosen = + tied.find((candidate) => hasActionablePrReviewFeedback(candidate.attesting.comment.body)) ?? + tied[0]; + return chosen?.attesting ?? null; } /** @@ -181,26 +266,59 @@ function headsWithUndispositionedFinding( comments: CommentReviewGateComment[], reviewerBotLogin: string, ): CarriedFinding[] { - const newestPerHead = new Map(); + const byHead = new Map(); const ledger: { entry: AllyPriorFindingDisposition; timeMs: number; attestedHeadSha: string }[] = []; for (const comment of comments) { if (!isAllyConsolidatedReviewComment(comment, reviewerBotLogin)) continue; const attestedHeadSha = extractAllyReviewedHeadSha(comment.body); - if (!attestedHeadSha) continue; + // A review whose verdict block we cannot read still says which tree it + // examined, and it may still *raise* a finding here — it just may never + // retire one. Without this the whole comment was skipped at the `continue` + // below, so a finding its prose carried vanished the moment the author + // pushed past the reviewed head: master red that head, this branch greened + // it (peer review of #1721 at 97b4ddd1, TrafficOpsEngineer). The unreadable + // branch in evaluateCommentReviewGate catches it only at its own head. + // + // Scoped to `unreadable` deliberately. allyClaimedReviewHead is laxer than + // the attestation parser by design, and letting it stand in for every body + // that fails to attest would newly carry findings off ambiguous prose — a + // change to the block-less population this row never measured. Only the + // population the block created gets the new path. + const claimedHeadSha = + attestedHeadSha ?? + (parseAllyVerdictBlock(comment.body).kind === "unreadable" + ? allyClaimedReviewHead(comment.body) + : null); + if (!claimedHeadSha) continue; const commentTime = toEpochMs(comment.createdAt); if (!Number.isFinite(commentTime)) continue; - for (const entry of extractAllyPriorFindingDispositions(comment.body)) { - ledger.push({ entry, timeMs: commentTime, attestedHeadSha }); + // Ledger authority requires a real attestation, which is the asymmetry this + // whole path turns on: an unreadable verdict may carry a finding forward + // and may not dispose of one. Retiring is the direction that loses + // information, so it stays gated on a body we could actually parse. + // + // Belt and braces today rather than the mechanism: the extractor already + // returns `[]` for an unreadable block, so this guard changes nothing on + // its own. It is here because the loop now admits comments on a *claimed* + // head, and without it the code would read as though a claimed head + // conferred ledger authority — which it must not, and which would become + // true the moment that extractor grew a prose fallback of its own. + if (attestedHeadSha) { + for (const entry of extractAllyPriorFindingDispositions(comment.body)) { + ledger.push({ entry, timeMs: commentTime, attestedHeadSha }); + } } - const existing = newestPerHead.get(attestedHeadSha); - // Ties prefer the later item, matching latestAttestingAllyComment: the - // comment endpoint is chronological but its timestamps are second-resolution. - if (!existing || commentTime >= existing.timeMs) { - newestPerHead.set(attestedHeadSha, { attesting: { comment, attestedHeadSha }, timeMs: commentTime }); - } + const perHead = byHead.get(claimedHeadSha); + const candidate = { + attesting: { comment, attestedHeadSha: claimedHeadSha }, + timeMs: commentTime, + attested: Boolean(attestedHeadSha), + }; + if (perHead) perHead.push(candidate); + else byHead.set(claimedHeadSha, [candidate]); } // A ledger entry speaks only to findings that already existed when it was @@ -281,23 +399,98 @@ function headsWithUndispositionedFinding( return [...verbs]; }; + // Newest statement per head wins, with one precedence above recency: an + // unreadable review may not *displace* an attested one, however much newer it + // is, because displacing is retiring by another name. "Newest per head" means + // the newest statement about that tree, and a verdict we could not read makes + // no statement. Letting it win drops the older review's finding on the + // strength of prose that merely happens not to mention one — the same + // fail-open this branch exists to close, arriving through the fix for it + // (peer review of #1721 at bbe6d640, TrafficOpsEngineer). Caught by the + // pre-existing case at "leaves a finding carried from the head it names". + // Among two unreadable reviews neither is evidence, so there is nothing to + // lose between them. + // + // The remaining tie — same head, same attestation class, same second — is + // resolved here on the *final* verdict rather than on a mid-loop proxy, which + // is what keeps it from trading one fail-open for a quieter one. Preferring + // the blocking body up in the loop would have handed the slot to a body that + // isFullyDispositioned then filters straight back out, losing the tied + // candidate that would have carried. Asking "does any tied candidate still + // carry a finding?" is order-independent and cannot lose one. + // // `countInheritedLedgerAssertion: false` keeps this enumeration answering // "which findings did *this head* raise?". A `still-present` entry names an // earlier head's finding, and that head is enumerated in its own right, so // counting it here would name a review whose own buckets are empty — and // permanently, since a 0/0 body reports no identities for any later ledger - // entry to retire. The current-head branch below deliberately does count it. - return [...newestPerHead.values()] - .filter( + // entry to retire. The current-head branch deliberately does count it. That + // asymmetry is BLO-31446's, and it survives the tied-set rewrite unchanged: + // the option narrows what each candidate *asserts*, the tie-break decides + // *which* candidates are eligible to assert it, and neither reads the other. + const carried: { attesting: AttestingComment; timeMs: number }[] = []; + for (const candidates of byHead.values()) { + const tied = topTiedBy(candidates, (candidate) => [candidate.attested ? 1 : 0, candidate.timeMs]); + const blocking = tied.find( (entry) => hasActionablePrReviewFeedback(entry.attesting.comment.body, undefined, { countInheritedLedgerAssertion: false, }) && !isFullyDispositioned(entry), + ); + if (blocking) carried.push(blocking); + } + + // Cross-head, and the last `timeMs`-only comparison left. The verdict does + // not turn on it — every entry here already carries a finding — but + // evaluateCommentReviewGate takes `[carried]` and puts that head's short SHA + // in the commit-status text, so two heads carrying at the same second named + // a different one on each run. The head is the final axis because it is a + // strict total order and a second-resolution timestamp is not. + return carried + .sort( + (a, b) => + b.timeMs - a.timeMs || + a.attesting.attestedHeadSha.localeCompare(b.attesting.attestedHeadSha), ) - .sort((a, b) => b.timeMs - a.timeMs) .map((entry) => ({ ...entry.attesting, unrecognizedVerbs: unrecognizedVerbsBlocking(entry) })); } +/** + * The newest Ally consolidated-review comments, whatever they attest — the + * whole set tied at that second, not a winner among them. + * + * Deliberately not filtered by attestation: the point is to reach a review + * whose head could not be established, which is precisely the case + * latestAttestingAllyComment skips. + * + * The tie is returned rather than resolved because the only caller's predicate + * is strictly wider than the one this function could apply: it wants an + * unreadable review that is *also* in scope for the head being evaluated, and + * scope is not known here. Picking on the narrower predicate alone let `find` + * hand the slot to an unreadable review naming some other tree, which the + * caller then scopes out — so the in-scope unreadable review was never + * examined and the gate went green off a candidate nobody consulted (Ally, + * peer review of #1721 at 9fd4b499; the same shape as the mid-loop proxy at + * headsWithUndispositionedFinding, and it fails open the same way). A + * tie-break is order-independent only when its predicate is the caller's whole + * predicate. + */ +function newestAllyConsolidatedReviewComments( + comments: CommentReviewGateComment[], + reviewerBotLogin: string, +): CommentReviewGateComment[] { + const candidates: { comment: CommentReviewGateComment; timeMs: number }[] = []; + for (const comment of comments) { + if (!isAllyConsolidatedReviewComment(comment, reviewerBotLogin)) continue; + const commentTime = toEpochMs(comment.createdAt); + if (!Number.isFinite(commentTime)) continue; + candidates.push({ comment, timeMs: commentTime }); + } + return topTiedBy(candidates, (candidate) => [candidate.timeMs]).map( + (candidate) => candidate.comment, + ); +} + /** * Evaluate only the comment-shaped review surface for one exact PR head. * Formal reviews remain owned by GitHub's normal reviewDecision path. @@ -319,6 +512,79 @@ export function evaluateCommentReviewGate(input: { const comments = input.comments ?? []; const normalizedHead = headSha.toLowerCase(); + + // Checked before anything else, scoped to the newest review, and scoped to + // this head. + // + // Scoped to the newest review, because an unreadable block anywhere in + // history would wedge the PR permanently with no route out — the same + // unretirable trap BLO-31446 and BLO-31947 document. + // + // Checked first, because the alternative is silence: an unreadable block + // attests no head, so without this branch the newest review is invisible and + // an *older* review of the same head stays authoritative. That is not + // hypothetical — it is exactly how paperclip#1675 reported a finding Ally + // had withdrawn. The 15:41:42Z clean review failed to attest, so the + // 03:46:19Z review of the same head kept its `Important Issues (1)`, and the + // gate published `blocking_finding` against a superseded verdict. + // + // Scoped to this head, because "newest" is not "at this head" and the + // difference is a real red on a tree nobody reviewed. + // newestAllyConsolidatedReviewComments has no head filter, so unscoped this + // branch lets a malformed block from three pushes ago decide the current + // head — where the same PR with no comments at all is `not_evaluated`, i.e. + // green. A stale broken block must not be worse for an author than no review + // (found in peer review of #1721 at 11a52e9a). It matters most on the + // designed upgrade path: SUPPORTED_ALLY_VERDICT_VERSION is bumped by a + // server rollout, but the producer is a prompt that takes effect the moment + // it merges, so between those two moments an unscoped branch reds every open + // PR at once — including PRs whose current head was never reviewed. + // + // The scoping test is asymmetric and fails closed, which is what keeps AC-5: + // the branch is skipped only when the review *positively* names some other + // tree. allyClaimedReviewHead returning null means "cannot tell which head + // this examined", and that is an ambiguity, not an exemption — a review of + // this head whose verdict we could not read is precisely the case that must + // not resolve to success. Only a head we can read, and that is not this one, + // makes the unreadable verdict somebody else's problem. + // Applied as one predicate over the whole tied set, because scope and + // readability are both parts of the question and splitting them across the + // helper and here is what let a tie go green (see + // newestAllyConsolidatedReviewComments). Among reviews we cannot order, + // "does any of them make this claim?" is order-independent. + // + // The verdict is settled by that existential, but the *cause* is not: every + // tied candidate yields the same {state, outcome} while `block.reason` + // differs between them, and that string becomes the commit-status + // description and the check-run summary. Returning on the first match let + // array order name a different cause on each run. So the scan is exhaustive + // and the reason is the final axis — lexicographically smallest, mirroring + // the cross-head tie-break at :429, which is a strict total order where a + // second-resolution timestamp is not. commentCreatedAt travels with the + // chosen candidate so the two can never describe different comments + // (Ally, #1721 at 31532b48). + let unreadable: { reason: string; commentCreatedAt: string } | null = null; + for (const review of newestAllyConsolidatedReviewComments(comments, reviewerBotLogin)) { + const claimedHead = allyClaimedReviewHead(review.body); + if (claimedHead !== null && claimedHead !== normalizedHead) continue; + const block = parseAllyVerdictBlock(review.body); + if (block.kind !== "unreadable") continue; + if (unreadable === null || block.reason.localeCompare(unreadable.reason) < 0) { + unreadable = { + reason: block.reason, + commentCreatedAt: new Date(toEpochMs(review.createdAt)).toISOString(), + }; + } + } + if (unreadable) { + return { + state: "failure", + outcome: "unreadable_verdict", + reason: `Ally's newest review carries an unreadable verdict block: ${unreadable.reason}.`, + commentCreatedAt: unreadable.commentCreatedAt, + }; + } + const forHead = latestAttestingAllyComment(comments, reviewerBotLogin, normalizedHead); if (forHead) { @@ -331,11 +597,25 @@ export function evaluateCommentReviewGate(input: { commentCreatedAt: new Date(toEpochMs(forHead.comment.createdAt)).toISOString(), }; } + // Name the source that decided this, because "clean" from a counted + // structured block and "clean" from the prose fallback are different + // claims with different failure modes, and the whole point of BLO-32695 + // is being able to tell which one you are looking at. Without this the + // gate description is identical either way, so a silent regression back + // onto the prose path — the exact thing this change retires — would be + // invisible on the PR. + // Both phrasings are kept inside MAX_COMMIT_STATUS_DESCRIPTION. The writer + // in github-app-auth.ts slices at 140 before the POST, so an overlong + // description is never rejected — it is silently cut, and what it cuts is + // the tail, which is where the source attribution lives. Pinned by test. + const source = + parseAllyVerdictBlock(forHead.comment.body).kind === "ok" + ? "its structured ally-verdict block" + : "prose fallback (no ally-verdict block)"; return { state: "success", outcome: "clean", - reason: - "Ally's most recent consolidated-review comment for this head reports no unresolved findings.", + reason: `Ally's most recent consolidated-review comment for this head reports no unresolved findings, per ${source}.`, }; } @@ -414,6 +694,13 @@ export function commentReviewGateCheckTitle( return "Unresolved finding at this head"; case "carried_finding": return "Unresolved finding carried from an earlier head"; + // Deliberately does not say "finding": this outcome is neither evidence of + // review nor evidence of a finding, and the title is the surface a reader + // sees before opening the check. Calling it a finding here would re-commit + // the misreport BLO-32695 exists to end, on the one line most likely to be + // read in isolation. + case "unreadable_verdict": + return "Verdict block unreadable — no finding asserted"; case "not_evaluated": return "Not evaluated — no comment-shaped review attests this head"; } From d3412cce9601f3b38dca6c3f005c3f4378985701 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Fri, 18 Sep 2026 23:26:03 +0000 Subject: [PATCH 02/13] fix(review-gate): mirror the gate's fence stripper in both peer readers (BLO-32695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ally's Important at 48feb16cc: sweep-stalled-ally-reviews.py and check-ally-review-consistency.mjs stripped fences with a naive ``` line toggle while the gate's withoutFencedCodeBlocks handles ~~~, fence-length matching and the backtick info-string rule. All three compute block and opener counts over that stripped text, so they disagreed about how many blocks a body contains — the BLO-31730 cross-reader divergence this row exists to retire, one layer down. Its measured trigger is a review that quotes the marker template, i.e. the likeliest shape for a review of this feature: the sweep then re-requests a review of a head Ally already read. Ported the gate's algorithm verbatim to both rather than widening the delimiter, which would have left fence-length as a live residual. Each of the three rules is pinned by a case that fails when the rule alone is reverted, per BLO-34263 — zero ~~~ fixtures existed on either side before, which is why the class stayed open. sweep 115/115, consistency 143/143, idempotency-contract 17/17, pr-comment-review-gate 99/99. check-ally-review-consistency.mjs output is byte-identical to HEAD's (same 3 unrelated I1 fingerprints). Ally's Suggestion (parse_reviewed_head reads the raw body) is deliberately not folded in: it is unreachable from a contract-conforming body and needs a producer change that ally-agent-idempotency-contract.test.mjs pins against. --- .github/scripts/sweep-stalled-ally-reviews.py | 38 +++++++++++++------ .../test_sweep_stalled_ally_reviews.py | 30 +++++++++++++++ scripts/check-ally-review-consistency.mjs | 33 +++++++++++----- .../check-ally-review-consistency.test.mjs | 37 ++++++++++++++++++ 4 files changed, 116 insertions(+), 22 deletions(-) diff --git a/.github/scripts/sweep-stalled-ally-reviews.py b/.github/scripts/sweep-stalled-ally-reviews.py index e8989cb88540..c3907bdfda28 100755 --- a/.github/scripts/sweep-stalled-ally-reviews.py +++ b/.github/scripts/sweep-stalled-ally-reviews.py @@ -114,30 +114,44 @@ def parse_list(value, fallback): r"(Critical|Important)[ \t]+Issues[ \t]*[*_]{0,3}[ \t]*\((\d+)\)[*_]{0,3}[ \t]*$", re.IGNORECASE | re.MULTILINE, ) -FENCE_PATTERN = re.compile(r"^ {0,3}```") +FENCE_OPEN_PATTERN = re.compile(r"^ {0,3}(`{3,}|~{3,})(.*)$") +FENCE_CLOSE_PATTERN = re.compile(r"^ {0,3}(`{3,}|~{3,})[ \t]*$") def without_fenced_spans(text): """Blank fenced spans so a quoted bucket cannot fail a block closed. - Deliberately simpler than withoutFencedCodeBlocks in the gate: a line-level - toggle on ``` only, with no tilde fences, no fence-length matching and no - info-string rule. Stated rather than implied -- a body using those forms is - read here as emitted structure and by the gate as a quote. Applied to the - count cross-check and to the block and opener counts in + Mirrors withoutFencedCodeBlocks in the gate exactly -- tilde fences, fence + length matching and the backtick info-string rule. A simpler toggle here is + not a scoping choice but a divergence: the two readers then disagree about + how many blocks a body contains, which is the BLO-31730 cross-reader + failure one layer down, and it fires first on a review that quotes the + marker template -- the likeliest shape for a review of this feature. + Applied to the count cross-check and to the block and opener counts in parse_verdict_block_head, not to the prose attestation pattern, whose own - fence handling is unchanged by this. + fence handling is the residual documented on parse_reviewed_head. """ - if "```" not in text: + if "```" not in text and "~~~" not in text: return text lines = [] - fenced = False + open_fence = None for line in text.split("\n"): - if FENCE_PATTERN.match(line): - fenced = not fenced + if open_fence: + close = FENCE_CLOSE_PATTERN.match(line) + lines.append("") + if close and close.group(1)[0] == open_fence[0] and len(close.group(1)) >= open_fence[1]: + open_fence = None + continue + fence = FENCE_OPEN_PATTERN.match(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 and not (fence.group(1)[0] == "`" and "`" in fence.group(2)): + open_fence = (fence.group(1)[0], len(fence.group(1))) lines.append("") else: - lines.append("" if fenced else line) + lines.append(line) + # An unclosed fence blanks to end of body, matching how GitHub renders it. return "\n".join(lines) diff --git a/.github/scripts/test_sweep_stalled_ally_reviews.py b/.github/scripts/test_sweep_stalled_ally_reviews.py index a4892a7bcbf2..eed7f187b34d 100644 --- a/.github/scripts/test_sweep_stalled_ally_reviews.py +++ b/.github/scripts/test_sweep_stalled_ally_reviews.py @@ -1206,6 +1206,36 @@ def test_control_a_real_second_block_is_still_unreadable(self): body = self.body("", self.block()) self.assertIsNone(sweep.parse_reviewed_head(body)) + def test_a_tilde_or_longer_fenced_example_is_not_a_second_block_either(self): + # The test above pinned the ``` form only, so the same harm reopened + # under every other CommonMark fence the gate handles: a ~~~ opener, and + # a longer backtick run wrapping a ``` fence. Both read `ok` at the + # gate and `unreadable` here until this reader matched it. + for opener, closer in (("~~~", "~~~"), ("````markdown", "````")): + body = self.body("Here is the emitted form:", "", opener, self.block(), closer) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD, opener) + + def test_only_a_same_char_run_at_least_as_long_closes_a_fence(self): + # Fence-length and fence-char matching are the halves a delimiter + # widening leaves behind. If ``` closed a ```` fence, or ~~~ closed a + # ``` one, the quoted block after it would re-appear as a second block + # and the body would read `unreadable` again. + for opener, inner, closer in ( + ("````markdown", "```", "````"), + ("```markdown", "~~~", "```"), + ): + body = self.body("As emitted:", "", opener, self.block(), inner, self.block(), closer) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD, opener) + + def test_an_inline_backtick_span_does_not_open_a_phantom_fence(self): + # CommonMark bars a backtick from a backtick fence's info string. Without + # that rule this line opens a fence that never closes, blanking the rest + # of the body -- so the second block below goes unseen and a genuinely + # unreadable body reads `ok`. The assertion is the same as the + # real-second-block control precisely because the harm is masking it. + body = self.body("``` `example` is prose, not a fence opener", self.block()) + self.assertIsNone(sweep.parse_reviewed_head(body)) + def test_a_malformed_ledger_is_rejected_here_too(self): # Validation had covered one of the two fields the payload carries. TS # rejects these via asDispositions and the mjs via stillPresentIn, while diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index b47bd75ed756..13d8e10e9b36 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -203,26 +203,39 @@ const EMITTED_BUCKET_RE = new RegExp( /** * Fenced spans blanked, so a quoted bucket cannot fail a block closed. * - * Deliberately simpler than withoutFencedCodeBlocks in the gate: a line-level - * toggle on ``` only, with no tilde fences, no fence-length matching and no - * info-string rule. The bound is stated rather than implied — a body using - * those forms is read here as emitted structure and by the gate as a quote. - * Applied to this cross-check and to the block and opener counts in + * Mirrors withoutFencedCodeBlocks in the gate exactly — tilde fences, fence + * length matching and the backtick info-string rule. A simpler toggle here is + * not a scoping choice but a divergence: the two readers then disagree about + * how many blocks a body contains, which is the BLO-31730 cross-reader failure + * one layer down, and it fires first on a review that quotes the marker + * template. Applied to this cross-check and to the block and opener counts in * structuredVerdict, not to the prose attestation pattern above, whose own * fence divergence is the documented residual on NOT_INDENTED_CODE and is * unchanged by this. */ +const FENCE_OPEN_RE = /^ {0,3}(`{3,}|~{3,})(.*)$/; +const FENCE_CLOSE_RE = /^ {0,3}(`{3,}|~{3,})[ \t]*$/; + function withoutFencedSpans(text) { - if (!text.includes("```")) return text; - let fenced = false; + if (!text.includes("```") && !text.includes("~~~")) return text; + let open = null; return text .split("\n") .map((line) => { - if (/^ {0,3}```/.test(line)) { - fenced = !fenced; + if (open) { + const close = FENCE_CLOSE_RE.exec(line); + if (close && close[1][0] === open.char && close[1].length >= open.length) open = null; + return ""; + } + const fence = FENCE_OPEN_RE.exec(line); + // Per CommonMark a backtick fence's info string may not itself contain a + // backtick, so an inline span cannot open 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 }; return ""; } - return fenced ? "" : line; + return line; }) .join("\n"); } diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index 6e6749c95bab..2965b082d876 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -1311,4 +1311,41 @@ describe("BLO-32695 — a fenced example of the marker is not a second block", ( // both counts together or it trades one divergence for another. assert.equal(attestedHead(body("```markdown", "", + # `[0-9]`, never `\d`: Python's `\d` is Unicode-aware and JavaScript's is + # ASCII-only, so `ally-verdict:١` (U+0661) parses as version 1 here and + # matches nothing in either JS reader. The gate then counts an opener with + # no block and goes `unreadable_verdict` while this sweep records the head + # as reviewed -- verbatim the divergence `parse_verdict_block_head` below + # exists to prevent. Same rule at EMITTED_BUCKET_PATTERN, where it inverts. + r"^(?! *\t)(?! {4}) {0,3}(?![ \t]*>)", re.MULTILINE | re.DOTALL, ) VERDICT_OPENER_PATTERN = re.compile( @@ -111,7 +117,12 @@ def parse_list(value, fallback): # counts, and over-matching fails a clean review closed. EMITTED_BUCKET_PATTERN = re.compile( r"^(?! *\t)(?! {4}) {0,3}(?![ \t]*>)(?:#{1,6}[ \t]*)?[*_]{0,3}" - r"(Critical|Important)[ \t]+Issues[ \t]*[*_]{0,3}[ \t]*\((\d+)\)[*_]{0,3}[ \t]*$", + # `[0-9]` for the reason given at VERDICT_BLOCK_PATTERN, and here the harm + # runs the other way: `### Critical Issues (١)` over a block stating 0 is + # no bucket at all to the JS readers -- no contradiction, gate green -- + # while Unicode `\d` would make it a contradiction here, sending the sweep + # to re-request review on a head Ally already reviewed. + r"(Critical|Important)[ \t]+Issues[ \t]*[*_]{0,3}[ \t]*\(([0-9]+)\)[*_]{0,3}[ \t]*$", re.IGNORECASE | re.MULTILINE, ) FENCE_OPEN_PATTERN = re.compile(r"^ {0,3}(`{3,}|~{3,})(.*)$") @@ -155,6 +166,42 @@ def without_fenced_spans(text): return "\n".join(lines) +# The exact character set ECMAScript's String.prototype.trim removes: +# WhiteSpace (TAB, VT, FF, ZWNBSP, and the Space_Separator category) plus +# LineTerminator (LF, CR, LS, PS). Python's str.strip() is a different set in +# *both* directions -- it removes U+001C..U+001F and U+0085, which JS keeps, +# and it keeps U+FEFF, which JS removes -- so a bare .strip() on any field this +# reader shares with the JS readers is the same class of divergence as `\d`. +JS_WHITESPACE = ( + # WhiteSpace: TAB, VT, FF, ZWNBSP ... + "\u0009\u000b\u000c\ufeff" + # ... and the Space_Separator (Zs) category. + "\u0020\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005" + "\u2006\u2007\u2008\u2009\u200a\u202f\u205f\u3000" + # LineTerminator: LF, CR, LS, PS. + "\u000a\u000d\u2028\u2029" +) + + +def js_trim(value): + """`String.prototype.trim`, not `str.strip`. See JS_WHITESPACE.""" + return value.strip(JS_WHITESPACE) + + +def reject_js_nonfinite(literal): + """Make json.loads refuse what JSON.parse refuses. + + Python accepts the bare `NaN`/`Infinity`/`-Infinity` literals as a JSON + extension; `JSON.parse` raises on all three. The counts and ledger indices + are already held to `is_js_integer`, so the gap is only reachable through a + key no reader validates -- but the block's own comment anticipates a future + free-text field, and one extra key carrying `NaN` would read `ok` here and + `unreadable` at the gate. Closing it at the parser costs one argument and + cannot rot as fields are added. + """ + raise ValueError("JSON.parse rejects the %s literal" % literal) + + def is_js_integer(value): """Whether `Number.isInteger` would accept this JSON value. @@ -196,7 +243,7 @@ def severity_counts(raw): return None if value < 0 or value > MAX_VERDICT_FINDING_COUNT: return None - key = severity.strip().lower() if isinstance(severity, str) else None + key = js_trim(severity).lower() if isinstance(severity, str) else None if key not in VERDICT_SEVERITIES: return None counts[key] = value @@ -239,11 +286,11 @@ def dispositions_ok(payload): if not isinstance(item, dict): return False head = item.get("head") - if not isinstance(head, str) or not ABBREV_SHA_PATTERN.match(head.strip()): + if not isinstance(head, str) or not ABBREV_SHA_PATTERN.match(js_trim(head)): return False for field in ("severity", "verb"): value = item.get(field) - if not isinstance(value, str) or not value.strip(): + if not isinstance(value, str) or not js_trim(value): return False index = item.get("index") # Same JS-integer predicate as the counts above, for the same reason: @@ -311,13 +358,13 @@ def parse_verdict_block_head(body): if int(raw_version) != SUPPORTED_VERDICT_VERSION: return ("unreadable", None) try: - parsed = json.loads(raw_payload.strip()) + parsed = json.loads(js_trim(raw_payload), parse_constant=reject_js_nonfinite) except ValueError: return ("unreadable", None) if not isinstance(parsed, dict): return ("unreadable", None) head = parsed.get("head") - if not isinstance(head, str) or not FULL_SHA_PATTERN.match(head.strip()): + if not isinstance(head, str) or not FULL_SHA_PATTERN.match(js_trim(head)): return ("unreadable", None) # The counts the gate reads, read here too. A block whose findings that # reader rejects -- or whose own emitted buckets contradict it -- is a @@ -332,7 +379,7 @@ def parse_verdict_block_head(body): return ("unreadable", None) if prose_count_contradicts(text, counts): return ("unreadable", None) - return ("ok", head.strip().lower()) + return ("ok", js_trim(head).lower()) def parse_reviewed_head(body): diff --git a/.github/scripts/test_sweep_stalled_ally_reviews.py b/.github/scripts/test_sweep_stalled_ally_reviews.py index eed7f187b34d..d31ca33f57ed 100644 --- a/.github/scripts/test_sweep_stalled_ally_reviews.py +++ b/.github/scripts/test_sweep_stalled_ally_reviews.py @@ -1353,5 +1353,117 @@ def test_heading_without_attestation_does_not_match(self): ) +class TestVerdictBlockMirrorsJsCharacterSemantics(unittest.TestCase): + """Peer review of #1721 at d3412cce -- the same reader split one layer down. + + The fence and ledger rules above made this reader agree with the gate about + *which constructs* it recognises. These pin the *character sets* underneath + them: Python's `\\d`, `str.strip` and `json.loads` are each a superset of + the JavaScript primitive they mirror, so a body could still be read by one + reader and refused by the other with every construct-level rule in place. + + Both harms named on the divergent rows are the ones this module's own + docstrings already describe, and they run in opposite directions -- which + is why neither masks the other and each needs its own case. + """ + + HEAD = "e" * 40 + + def block(self, version="1", head=None, extra=""): + return '' % ( + version, + self.HEAD if head is None else head, + extra, + ) + + def body(self, *rest, **kw): + return "\n".join( + [self.block(**kw), "", "## Ally - Consolidated PR Review"] + list(rest) + ) + + def test_a_unicode_digit_version_is_unreadable_not_version_one(self): + # int("\u0661") == 1 in Python, so a Unicode-aware `\d` would accept + # this as a supported version and record the head as reviewed -- while + # both JS readers match no block at all, count the opener, and go + # `unreadable_verdict`. Gate red, sweep silent, and the sweep is the + # only automatic route back. + for digit in ("\u0661", "\uff11"): + self.assertIsNone(sweep.parse_reviewed_head(self.body(version=digit)), digit) + + def test_control_an_ascii_digit_version_is_still_read(self): + # Without this the test above passes for a reader that stopped parsing + # versions entirely. `01` is pinned for the same reason it is elsewhere: + # the JS readers use Number(), so a string compare would split them. + for digit in ("1", "01"): + self.assertEqual( + sweep.parse_reviewed_head(self.body(version=digit)), self.HEAD, digit + ) + + def test_a_unicode_digit_bucket_count_is_not_a_bucket(self): + # The mirror image, and the reason a single `\d` rule is not enough: + # here Unicode-awareness makes this reader see a contradiction the JS + # readers cannot see. The gate stays green while the sweep re-requests + # review on a head Ally already reviewed -- spam, and each duplicate is + # a COMMENTED review that cannot be dismissed. + body = self.body("### Critical Issues (\u0661)") + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + + def test_control_an_ascii_bucket_count_still_contradicts(self): + body = self.body("### Critical Issues (1)") + self.assertIsNone(sweep.parse_reviewed_head(body)) + + def test_head_padding_is_trimmed_exactly_as_javascript_trims_it(self): + # str.strip() differs from String.prototype.trim in *both* directions, + # so a bare strip splits the readers either way round, and each + # direction needs its own case. + # + # U+0085 is Python-only whitespace: strip() removes it, trim() keeps + # it, so the JS SHA test fails and the gate reads `unreadable` while + # this read a clean attestation. U+FEFF is the exact reverse -- trim() + # removes it, strip() keeps it -- so before js_trim this reader alone + # refused a head both JS readers accept. + # + # U+001C..U+001F are the other Python-only whitespace and are + # deliberately NOT here: they are JSON control characters, so both + # json.loads and JSON.parse refuse the payload before any trim runs. + # Asserting on them would pass with or without js_trim. + self.assertIsNone( + sweep.parse_reviewed_head( + self.body(head="\u0085" + self.HEAD + "\u0085") + ) + ) + self.assertEqual( + sweep.parse_reviewed_head( + self.body(head="\ufeff" + self.HEAD + "\ufeff") + ), + self.HEAD, + ) + # Agreement controls -- whitespace both runtimes trim. These fail if + # js_trim is narrowed to just the two characters above. + for pad in ("\u0020", "\u00a0", "\u2028", "\u3000"): + self.assertEqual( + sweep.parse_reviewed_head(self.body(head=pad + self.HEAD + pad)), + self.HEAD, + repr(pad), + ) + + def test_a_json_literal_javascript_rejects_is_unreadable(self): + # json.loads accepts the bare NaN/Infinity literals as an extension; + # JSON.parse raises on all three. Reachable only through a key no + # reader validates today, but the block's own comment anticipates a + # future free-text field, and closing it at the parser cannot rot as + # fields are added. + for literal in ("NaN", "Infinity", "-Infinity"): + body = self.body(extra=',"note":%s' % literal) + self.assertIsNone(sweep.parse_reviewed_head(body), literal) + + def test_control_an_unvalidated_extra_key_is_otherwise_ignored(self): + # The gate destructures head/findings/dispositions and ignores the + # rest, so rejecting every extra key would be its own divergence. + self.assertEqual( + sweep.parse_reviewed_head(self.body(extra=',"note":"anything"')), self.HEAD + ) + + if __name__ == "__main__": unittest.main() From 56f71d1bb353b1f4a1d2d3d2af66572bffa7c0b1 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sat, 19 Sep 2026 23:16:29 +0000 Subject: [PATCH 04/13] fix(review-gate): make every sweep pattern ASCII-only, as JS is (BLO-32695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Peer review of #1721 at d05a49f3: `\b` and IGNORECASE are the two members of the character-semantics class the `[0-9]` fix did not reach. The gate builds its regexes with "gm"/"gim"/"gi" and never the `u` flag, so its classes are ASCII-only; Python's are Unicode-aware. Measured exhaustively over U+0000..U+10FFFF: 138495 code points are word characters to Python's `\b` and not to JavaScript's, and exactly three -- U+0130, U+0131, U+017F -- fold into the ASCII letters these patterns spell. `re.ASCII` on every compiled pattern closes both halves at once. Applied as a blanket rule rather than per site: `\b` was missed because the prior fix enumerated the constructs it had seen, and a rule that needs no per-site reachability argument cannot be reasoned wrong by the next editor. Also fixes a site the finding's enumeration classed as safe. REVIEWED_HEAD_ PATTERN was read as a `[0-9a-f]` pattern, but it also spells `Reviewed head:` under IGNORECASE, so `Revıewed head:` (U+0131) attested here and not at the gate -- the same silent direction, on the fallback the opener rows route to. Takes the Suggestion in the same edit, which closes the last prose-path divergence: the gate's MARKDOWN_EMPHASIS_RUN / ATTESTATION_WRAPPER_RUN (3 of 25 attesting bodies on this PR wrap the SHA in backticks, which the gate reads and this reader did not) and, with them, the gate's NOT_INDENTED_CODE bound, so widening the wrappers does not introduce a divergence in the other direction. Six guards, six failing mutations, each reverted alone. The structural guard is killed by dropping `re.ASCII` from FENCE_OPEN_PATTERN, which no behavioural test covers -- that is the case that makes it a guard and not documentation. 130/130 pass. --- .github/scripts/sweep-stalled-ally-reviews.py | 47 ++++++-- .../test_sweep_stalled_ally_reviews.py | 102 ++++++++++++++++++ 2 files changed, 140 insertions(+), 9 deletions(-) diff --git a/.github/scripts/sweep-stalled-ally-reviews.py b/.github/scripts/sweep-stalled-ally-reviews.py index 34c1b5619756..eb234bd41e6a 100755 --- a/.github/scripts/sweep-stalled-ally-reviews.py +++ b/.github/scripts/sweep-stalled-ally-reviews.py @@ -71,11 +71,39 @@ def parse_list(value, fallback): return [item.strip() for item in raw.split(",") if item.strip()] +# Every pattern in this file carries re.ASCII, and that is a parity rule rather +# than a style one. Python's `\b`, `\w` and IGNORECASE folding are all +# Unicode-aware; the gate's regexes are built with "gm"/"gim"/"gi" and never the +# `u` flag, so JavaScript's are ASCII-only. Measured over the whole Unicode +# range at this head: 138495 code points are word characters to Python's `\b` +# and not to JavaScript's, and exactly three -- U+0130, U+0131, U+017F -- fold +# into the ASCII letters these patterns spell. re.ASCII closes both at once and +# cannot be reasoned wrong per site, which an explicit character class can. +# TestPatternCharacterClassesAreAsciiOnly pins the rule for patterns added +# later, including ones using a construct nobody has hit yet. +ASCII_RE = re.ASCII + +# Mirrors MARKDOWN_EMPHASIS_RUN / ATTESTATION_WRAPPER_RUN in the gate. Without +# them this reader is *narrower* than the gate on the prose path: of the 25 +# attesting bodies on #1721, 3 wrap the SHA in backticks, which the gate reads +# and a bare pattern does not. The indent bound is the gate's NOT_INDENTED_CODE +# for the same reason in the other direction -- an attestation inside an +# indented code block is prose to a bare `[ \t]*` and code to the gate. +MARKDOWN_EMPHASIS_RUN = r"[*_`]{0,3}" +ATTESTATION_WRAPPER_RUN = r"[*_`\t ]{0,6}" + # The immutable head attestation Ally writes into every consolidated body: # a standalone "Reviewed head: <40 lowercase hex>" line. This is what binds a # signal to a revision -- NOT review.commit_id, and NOT a substring scan. REVIEWED_HEAD_PATTERN = re.compile( - r"^[ \t]*Reviewed head:[ \t]*([0-9a-f]{40})[ \t]*$", re.IGNORECASE | re.MULTILINE + r"^(?! *\t)(?! {4}) {0,3}" + + MARKDOWN_EMPHASIS_RUN + + r"[ \t]{0,3}Reviewed head:[ \t]*" + + ATTESTATION_WRAPPER_RUN + + r"([0-9a-f]{40})" + + ATTESTATION_WRAPPER_RUN + + r"[ \t]*$", + re.IGNORECASE | re.MULTILINE | ASCII_RE, ) @@ -91,16 +119,16 @@ def parse_list(value, fallback): # as reviewed -- verbatim the divergence `parse_verdict_block_head` below # exists to prevent. Same rule at EMITTED_BUCKET_PATTERN, where it inverts. r"^(?! *\t)(?! {4}) {0,3}(?![ \t]*>)", - re.MULTILINE | re.DOTALL, + re.MULTILINE | re.DOTALL | ASCII_RE, ) VERDICT_OPENER_PATTERN = re.compile( - r"^(?! *\t)(?! {4}) {0,3}(?![ \t]*>)\n\nReviewed head: %s" % (drift, self.HEAD) + self.assertEqual( + len(sweep.VERDICT_OPENER_PATTERN.findall(body)), 1, repr(drift) + ) + self.assertIsNone(sweep.parse_reviewed_head(body), repr(drift)) + + def test_control_an_ascii_non_word_char_after_the_prefix_is_unchanged(self): + # Without this the test above passes for a pattern that dropped `\b` + # and matched the bare prefix unconditionally -- which would also count + # an opener for a marker that never drifted at all. + ok = ( + '' + % self.HEAD + ) + self.assertEqual(sweep.parse_reviewed_head(ok), self.HEAD) + for control in ("\n\nReviewed head: %s" % (control, self.HEAD) + self.assertEqual( + len(sweep.VERDICT_OPENER_PATTERN.findall(body)), 1, control + ) + self.assertIsNone(sweep.parse_reviewed_head(body), control) + + def test_a_folded_bucket_heading_is_no_bucket_here_either(self): + # Python folds U+017F into `s`, so `### Critical Iſſues (1)` over a + # block stating 0 reads as a contradiction here and as no bucket at all + # to the gate -- gate green, sweep re-requesting a head Ally reviewed. + # The mirror image of the row above, so neither masks the other. + block = '' % self.HEAD + body = "%s\n\n### Critical Iſſues (1)" % block + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + + def test_control_an_ascii_bucket_heading_still_contradicts(self): + block = '' % self.HEAD + self.assertIsNone(sweep.parse_reviewed_head("%s\n\n### Critical Issues (1)" % block)) + + def test_a_folded_attestation_label_is_not_an_attestation(self): + # Not in the reported finding, and reachable by the same flag: the + # prose pattern's IGNORECASE folds U+0131 into `i`, so `Revıewed head:` + # attests here and not at the gate. Silent direction again, and it is + # the fallback the two rows above route *to*. + self.assertIsNone(sweep.parse_reviewed_head("Revıewed head: %s" % self.HEAD)) + self.assertEqual(sweep.parse_reviewed_head("REVIEWED HEAD: %s" % self.HEAD), self.HEAD) + + def test_the_prose_fallback_reads_the_wrapped_forms_the_gate_reads(self): + # 3 of the 25 attesting bodies on #1721 wrap the SHA in backticks. The + # gate carries ATTESTATION_WRAPPER_RUN / MARKDOWN_EMPHASIS_RUN for + # exactly these; without them this reader is narrower than the gate and + # re-requests a review that already attests. + for form in ( + "Reviewed head: `%s`", + "**Reviewed head:** `%s`", + "_Reviewed head:_ %s", + ): + self.assertEqual( + sweep.parse_reviewed_head(form % self.HEAD), self.HEAD, form + ) + + def test_control_an_indented_code_attestation_is_not_read(self): + # The wrapper runs widen the pattern; the gate's NOT_INDENTED_CODE bound + # comes with them. A four-space-indented line is code to the gate, so + # reading it here would be a new divergence introduced by the fix. + self.assertIsNone(sweep.parse_reviewed_head(" Reviewed head: %s" % self.HEAD)) + + def test_every_compiled_pattern_in_the_module_is_ascii_only(self): + # The rule, not the four instances of it. `\b` was missed by the `[0-9]` + # fix because that fix enumerated the constructs it had seen; this + # fails for any pattern added later, including one using a construct + # nobody has hit yet. + import re as _re + + offenders = [ + name + for name, value in vars(sweep).items() + if isinstance(value, _re.Pattern) and not value.flags & _re.ASCII + ] + self.assertEqual(offenders, []) + + if __name__ == "__main__": unittest.main() From f48111f2f8ac49be906eca7c5f8fea6dff9cb2c0 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sun, 20 Sep 2026 01:14:06 +0000 Subject: [PATCH 05/13] fix(review-gate): stop publishing an unbounded ledger verb (BLO-32695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate quotes an unrecognized disposition verb verbatim into the commit-status description, and githubPostCommitStatusDetailed POSTs that description unscrubbed — github-egress-outbound-coverage.test.ts classifies it so under PEN-3157. Until this branch, the only route to that text was PRIOR_FINDING_DISPOSITION_PATTERN, whose `[a-z][a-z-]*` verb group bounded what model-authored text could reach the boundary. The structured block types `verb` as any non-empty string, so the bound was gone. Measured on one fixture, both arms: structured -> ...unrecognized ledger verb "ghp_abcdef...0123456789". prose -> ...is still undispositioned; no comment attests... Guarded at the publisher, not the parser. An unknown verb already fails closed as `unrecognized`, so rejecting the whole block over a cosmetic one would only manufacture a red — and it would put the gate out of step with the two peer readers that accept any non-empty string (`dispositions_ok`, `stillPresentIn`), which is the gate-red/peers-silent divergence this file keeps closing. The drift is still named; only its payload is withheld. The alphabet is now one constant, used to build the prose pattern and to test publishability, so the two cannot drift. Mutation-tested: reverting the guard alone turns the new test red. 259/259 pass across every suite depending on either module; tsc clean. --- .../__tests__/pr-comment-review-gate.test.ts | 53 +++++++++++++++++++ server/src/services/ally-review-detection.ts | 34 +++++++++++- server/src/services/pr-comment-review-gate.ts | 17 +++++- 3 files changed, 102 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/pr-comment-review-gate.test.ts b/server/src/__tests__/pr-comment-review-gate.test.ts index 1df9d1c6eb64..491650463160 100644 --- a/server/src/__tests__/pr-comment-review-gate.test.ts +++ b/server/src/__tests__/pr-comment-review-gate.test.ts @@ -461,6 +461,59 @@ describe("evaluateCommentReviewGate", () => { expect(verdict.reason).toContain(OLD_HEAD.slice(0, 7)); }); + it("never publishes a ledger verb outside the prose parser's alphabet (PEN-3157)", () => { + // An unrecognized verb is quoted verbatim into the commit-status + // description, and githubPostCommitStatusDetailed POSTs that description + // unscrubbed — github-egress-outbound-coverage.test.ts classifies it so. + // PRIOR_FINDING_DISPOSITION_PATTERN's `[a-z][a-z-]*` bounded what + // model-authored text could reach that boundary. Typing the structured + // field as a non-empty string dropped the bound, so a credential-shaped + // token in the JSON ledger was published where the identical token in + // prose was refused. Measured on this fixture before the bound: + // structured -> ...unrecognized ledger verb "ghp_abcdef...0123456789". + // prose -> ...is still undispositioned; no comment attests... + // Guarded at the publisher rather than the parser: an unknown verb already + // fails closed as `unrecognized`, so rejecting the block would only + // manufacture a red — and would put the gate out of step with the two peer + // readers that accept any non-empty string. + const token = "ghp_abcdefghijklmnopqrstuvwxyz0123456789"; + const structuredLedger = (verb: string) => + [ + "## Ally — Consolidated PR Review", + "", + `Reviewed head: ${INTERMEDIATE_HEAD}`, + "### Critical Issues (0)", + "### Important Issues (0)", + ].join("\n"); + + const carriedFor = (verb: string) => + evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [ + allyComment(blockingReview(OLD_HEAD), "2026-08-04T20:09:19Z"), + allyComment(structuredLedger(verb), "2026-08-04T21:09:19Z"), + ], + }); + + const leaked = carriedFor(token); + expect(leaked).toMatchObject({ state: "failure", outcome: "carried_finding" }); + expect(leaked.reason).not.toContain(token); + expect(leaked.reason).not.toContain("ghp_"); + // Withheld, not dropped: the reader must still learn that the red is + // vocabulary drift rather than a genuinely open finding. + expect(leaked.reason).toContain("unrecognized ledger verb"); + + // Positive control. Without it the assertions above would also pass on a + // fixture the block parser never read at all — a false all-clear. + expect(carriedFor("deferred").reason).toContain('unrecognized ledger verb "deferred"'); + }); + it("keeps the ordinary reason when no unrecognized verb is involved", () => { const verdict = evaluateCommentReviewGate({ headSha: CURRENT_HEAD, diff --git a/server/src/services/ally-review-detection.ts b/server/src/services/ally-review-detection.ts index d149b8ce1d9b..29a6e1598789 100644 --- a/server/src/services/ally-review-detection.ts +++ b/server/src/services/ally-review-detection.ts @@ -667,6 +667,38 @@ function hasNonNegatedMatch(text: string, pattern: RegExp): boolean { return false; } +// The alphabet a prose ledger entry may spell its verb in — the single source +// for the pattern below and for isConformingDispositionVerb. +// +// The structured block deliberately does NOT enforce it. An unknown verb +// already fails closed as `unrecognized`, so rejecting the whole block over a +// cosmetic one (`fixed (partially)`) would only manufacture a red, and it would +// put the gate out of step with the two peer readers that type `verb` as any +// non-empty string (`dispositions_ok` in sweep-stalled-ally-reviews.py, +// `stillPresentIn` in check-ally-review-consistency.mjs) — gate red, peers +// silent. +// +// What the alphabet is needed for is publication: see +// isConformingDispositionVerb. +const DISPOSITION_VERB_ALPHABET = String.raw`[a-z][a-z-]*`; +const DISPOSITION_VERB_PATTERN = new RegExp(`^${DISPOSITION_VERB_ALPHABET}$`); + +/** + * May this verb be quoted into a public commit-status description? + * + * The gate names an unrecognized verb verbatim so a reader can tell vocabulary + * drift from a genuinely open finding. That description is POSTed by + * githubPostCommitStatusDetailed, which github-egress-outbound-coverage.test.ts + * classifies `unscrubbed` under PEN-3157 — so the text it carries is bounded + * only by whatever produced it. Prose was bounded by the pattern above; + * `verb` arriving as a JSON field is typed as any non-empty string, so a + * credential-shaped token in the ledger was published where the identical token + * in prose was refused. Callers name the drift, not its payload. + */ +export function isConformingDispositionVerb(verb: string): boolean { + return DISPOSITION_VERB_PATTERN.test(verb); +} + // A "Prior Findings Dispositioned" ledger entry, e.g. // - **prior:731ced5 critical 1** — fixed — the terminator is gone. // Anchored to the bold list-item form Ally emits, matching the shape @@ -683,7 +715,7 @@ function hasNonNegatedMatch(text: string, pattern: RegExp): boolean { // 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]*(?:—|–|-)`, + String.raw`^${NOT_INDENTED_CODE} {0,3}-[ \t]*\*\*[ \t]*prior:([0-9a-f]{7,40})[ \t]+([a-z]+)[ \t]+(\d+)[ \t]*\*\*[ \t]*(?:—|–|-)[ \t]*(${DISPOSITION_VERB_ALPHABET})[ \t]*(?:—|–|-)`, "gim", ); diff --git a/server/src/services/pr-comment-review-gate.ts b/server/src/services/pr-comment-review-gate.ts index ac7d1226c345..50d2c7d64f94 100644 --- a/server/src/services/pr-comment-review-gate.ts +++ b/server/src/services/pr-comment-review-gate.ts @@ -21,6 +21,7 @@ import { allyClaimedReviewHead, hasActionablePrReviewFeedback, hasAllyConsolidatedReviewHeading, + isConformingDispositionVerb, parseAllyVerdictBlock, type AllyFindingRef, type AllyPriorFindingDisposition, @@ -44,6 +45,11 @@ const DEFAULT_PR_REVIEWER_BOT_LOGIN = "allyblockcast[bot]"; // the red actionable. const UNRECOGNIZED_VERB_BUDGET = 48; +// Stands in for a ledger verb that must not be published verbatim. It is not a +// verb Ally can emit — the alphabet excludes `<` — so it cannot be confused +// for one, and it keeps the drift visible while withholding its text. +const NON_CONFORMING_VERB = ""; + export interface CommentReviewGateComment { authorLogin: string | null | undefined; body: string | null | undefined; @@ -393,7 +399,16 @@ function headsWithUndispositionedFinding( if (isExplicitlyBlocked(headSha, entry.timeMs, finding)) continue; for (const prior of ledger) { if (prior.entry.kind !== "unrecognized") continue; - if (namesFinding(prior, headSha, entry.timeMs, finding)) verbs.add(prior.entry.disposition); + if (namesFinding(prior, headSha, entry.timeMs, finding)) { + // Name the drift, not its payload. This set is quoted verbatim into + // the commit-status description below, which is POSTed unscrubbed + // (PEN-3157) — see isConformingDispositionVerb. + verbs.add( + isConformingDispositionVerb(prior.entry.disposition) + ? prior.entry.disposition + : NON_CONFORMING_VERB, + ); + } } } return [...verbs]; From ba478a007a842c67fa483b685d4ce3b7537bad57 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sun, 20 Sep 2026 05:43:04 +0000 Subject: [PATCH 06/13] fix(review-gate): keep the PEN-3157 verb alphabet literal in the pattern (BLO-32695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `General tests (server 4/4)` went red at f48111f2f on a test this branch does not contain. master landed 43c3875de (PEN-3157) on 09-19, 94 commits ahead, adding github-write-egress-scrub.test.ts — which pins the verb alphabet by reading `([a-z][a-z-]*)` out of PRIOR_FINDING_DISPOSITION_PATTERN's own SOURCE TEXT. f48111f2f had folded that literal into a shared DISPOSITION_VERB_ALPHABET constant, so a refactor that widened nothing read to that test as a widening of a security bound. Zero file overlap, real interaction: CI builds refs/pull/N/merge, my worktree does not have the file. Restore the literal in the pattern and give the publisher its own copy. The two cannot drift: "the publisher's alphabet is the parser's alphabet" drives the real prose parser over a 13-verb corpus and asserts isConformingDispositionVerb agrees on every one, with a both-answers-present control so a one-sided corpus cannot pass vacuously. Deliberately NOT fixed by merging master and rewriting their assertion into an invariant pin: master's merge queue is merge_method REBASE, so a merge commit is dequeued unbuilt (rebaseable: false), and it is the CTO's freshly-landed security test. Mutation-tested: widening the publisher's copy alone ([a-z][a-z_-]*) turns the new test red on "fixed_ok" and nothing else. Master's pin re-checked against the restored source with a negative control (widening the verb group to [a-z]+ makes it fail). 231/231 across the 4 suites depending on either module; 130/130 python peer-reader tests; pnpm typecheck exit 0. --- .../__tests__/pr-comment-review-gate.test.ts | 50 +++++++++++++++++++ server/src/services/ally-review-detection.ts | 18 +++++-- 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/server/src/__tests__/pr-comment-review-gate.test.ts b/server/src/__tests__/pr-comment-review-gate.test.ts index 491650463160..a27aa66331c9 100644 --- a/server/src/__tests__/pr-comment-review-gate.test.ts +++ b/server/src/__tests__/pr-comment-review-gate.test.ts @@ -10,6 +10,7 @@ import { extractAllyReviewedHeadSha, hasActionablePrReviewFeedback, hasAllyConsolidatedReviewHeading, + isConformingDispositionVerb, } from "../services/ally-review-detection.js"; import { commentReviewGateCheckConclusion, @@ -514,6 +515,55 @@ describe("evaluateCommentReviewGate", () => { expect(carriedFor("deferred").reason).toContain('unrecognized ledger verb "deferred"'); }); + it("the publisher's alphabet is the parser's alphabet", () => { + // `[a-z][a-z-]*` is spelled TWICE in ally-review-detection.ts: once as the + // verb group of PRIOR_FINDING_DISPOSITION_PATTERN, once as + // DISPOSITION_VERB_PATTERN behind isConformingDispositionVerb. That is + // deliberate — the PEN-3157 pin in github-write-egress-scrub.test.ts reads + // the first out of the pattern's own SOURCE TEXT, so folding them into a + // shared constant makes a pure refactor read to that test as a widening of + // a security bound. This is what stops the two copies drifting instead. + // + // The bound only holds if the publisher's copy is no WIDER than the + // parser's: a verb the parser will extract from prose and the publisher + // then refuses is merely withheld, but the reverse is the PEN-3157 leak. + const verbs = [ + "fixed", + "wontfix", + "not-reproducible", + "a", + "fixed (partially)", + "Fixed", + "ghp_abcdefghijklmnopqrstuvwxyz0123456789", + "AKIAIOSFODNN7EXAMPLE", + "eyJhbGciOiJIUzI1NiJ9", + "-leading", + "fixed1", + "fixed_ok", + "", + ]; + + for (const verb of verbs) { + // Drive the real prose parser rather than re-spelling its regex here. + const body = [ + "## Ally — Consolidated PR Review", + `- **prior:731ced5 important 1** — ${verb} — note.`, + ].join("\n"); + const parserAdmits = extractAllyPriorFindingDispositions(body).some( + (entry) => entry.disposition === verb, + ); + expect( + isConformingDispositionVerb(verb), + `publisher vs parser disagree on ${JSON.stringify(verb)}`, + ).toBe(parserAdmits); + } + + // Positive control: the corpus must actually exercise both answers, or the + // loop above passes on a corpus that is entirely one-sided. + expect(verbs.filter((v) => isConformingDispositionVerb(v)).length).toBeGreaterThan(0); + expect(verbs.filter((v) => !isConformingDispositionVerb(v)).length).toBeGreaterThan(0); + }); + it("keeps the ordinary reason when no unrecognized verb is involved", () => { const verdict = evaluateCommentReviewGate({ headSha: CURRENT_HEAD, diff --git a/server/src/services/ally-review-detection.ts b/server/src/services/ally-review-detection.ts index 29a6e1598789..04906846db1c 100644 --- a/server/src/services/ally-review-detection.ts +++ b/server/src/services/ally-review-detection.ts @@ -667,8 +667,14 @@ function hasNonNegatedMatch(text: string, pattern: RegExp): boolean { return false; } -// The alphabet a prose ledger entry may spell its verb in — the single source -// for the pattern below and for isConformingDispositionVerb. +// The alphabet a prose ledger entry may spell its verb in. Deliberately a +// SECOND literal copy of the verb group in PRIOR_FINDING_DISPOSITION_PATTERN +// below, rather than a shared constant: the PEN-3157 pin in +// github-write-egress-scrub.test.ts reads `([a-z][a-z-]*)` out of that +// pattern's own source text, so interpolating a constant there makes a +// refactor read as a widening of a security bound. The two copies cannot drift +// — "the publisher's alphabet is the parser's alphabet" in +// pr-comment-review-gate.test.ts holds them equal. // // The structured block deliberately does NOT enforce it. An unknown verb // already fails closed as `unrecognized`, so rejecting the whole block over a @@ -680,8 +686,7 @@ function hasNonNegatedMatch(text: string, pattern: RegExp): boolean { // // What the alphabet is needed for is publication: see // isConformingDispositionVerb. -const DISPOSITION_VERB_ALPHABET = String.raw`[a-z][a-z-]*`; -const DISPOSITION_VERB_PATTERN = new RegExp(`^${DISPOSITION_VERB_ALPHABET}$`); +const DISPOSITION_VERB_PATTERN = /^[a-z][a-z-]*$/; /** * May this verb be quoted into a public commit-status description? @@ -714,8 +719,11 @@ export function isConformingDispositionVerb(verb: string): boolean { // 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. +// +// The `([a-z][a-z-]*)` verb group is pinned as source text by the PEN-3157 +// test in github-write-egress-scrub.test.ts — keep it literal here. 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]*(${DISPOSITION_VERB_ALPHABET})[ \t]*(?:—|–|-)`, + 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", ); From 8e6e84bd01ee6e849a950da17f0d1ee321ea2ac3 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sun, 20 Sep 2026 07:29:01 +0000 Subject: [PATCH 07/13] fix(review-gate): bound every model-authored value on the unreadable path (BLO-32695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit f48111f2f bounded the ledger `verb` at the publisher. The guard was applied per-site, so the sibling field on the same JSON object stayed unbounded: asSeverityCounts interpolated a model-authored `findings` key verbatim into the block's `reason`, which reaches the same unscrubbed PEN-3157 boundary — the commit-status description, and the check-run summary, which has no 140-character cap of its own. Guard the boundary rather than the field. PUBLISHABLE_TOKEN_ALPHABET is now one source of truth shared by the prose parser's verb group and the publisher predicate, asPublishableToken() names the drift and withholds the payload for anything outside it, and both the severity key and the version digits go through it. The duplication that commit ba478a007 existed to preserve is dropped: its justification named github-write-egress-scrub.test.ts in three places and that file does not exist at any revision — 0 hits across the tree. No test reads the verb group out of the pattern's source text, so sharing the constant is a pure refactor. "the publisher's alphabet is the parser's alphabet" still pins the equality, and is still non-trivial because the two sites embed the alphabet differently (anchored vs. inside the list-item match). Each guard has a failing mutation, checked one at a time: reverting the severity guard fails 2 tests, the version slice 1, widening the shared alphabet 18. Co-Authored-By: Claude --- .../ally-review-verdict-block.test.ts | 51 ++++++++++++ .../__tests__/pr-comment-review-gate.test.ts | 64 ++++++++++++--- server/src/services/ally-review-detection.ts | 79 ++++++++++++------- server/src/services/pr-comment-review-gate.ts | 15 +--- 4 files changed, 159 insertions(+), 50 deletions(-) diff --git a/server/src/__tests__/ally-review-verdict-block.test.ts b/server/src/__tests__/ally-review-verdict-block.test.ts index d9767d4875e7..9036ddba231a 100644 --- a/server/src/__tests__/ally-review-verdict-block.test.ts +++ b/server/src/__tests__/ally-review-verdict-block.test.ts @@ -748,6 +748,57 @@ describe("BLO-32695 — fail-closed on an unreadable block", () => { }); } + it("bounds every model-authored value it quotes into an unreadable reason", () => { + // The reason reaches the commit-status description AND the check-run + // summary, which has no 140-character cap of its own, so an unbounded + // value publishes in full there. Two values on this path are written by + // the model: the version digits and an unsupported severity key. Both are + // enumerated here together, because the leak that prompted this was the + // guarded field's neighbour — the bound was applied per-site, so the next + // site was unbounded by default. + const reasonFor = (body: string) => { + const parsed = parseAllyVerdictBlock(body); + expect(parsed.kind).toBe("unreadable"); + return parsed.kind === "unreadable" ? parsed.reason : ""; + }; + + const longVersion = "9".repeat(400); + expect( + reasonFor(`${verdictBlock({ head: PR1675_HEAD, findings: {} }, longVersion as never)} +## Ally — Consolidated PR Review`).length, + ).toBeLessThan(120); + + const token = "ghp_abcdefghijklmnopqrstuvwxyz0123456789"; + const severityReason = reasonFor( + `${verdictBlock({ head: PR1675_HEAD, findings: { critical: 0, important: 0, [token]: 1 } })} +## Ally — Consolidated PR Review`, + ); + expect(severityReason).not.toContain("ghp_"); + expect(severityReason).toContain("unsupported severity"); + + // A lowercase-alphabet key is not mangled by `.toLowerCase()`, so the + // guard has to be the alphabet rather than the case fold. + expect( + reasonFor(`${verdictBlock({ + head: PR1675_HEAD, + findings: { critical: 0, important: 0, "https://hooks.example.io/s3cr3t": 1 }, + })} +## Ally — Consolidated PR Review`), + ).not.toContain("s3cr3t"); + + // Positive control on both surfaces: bounding must not turn into + // silencing. A conforming version and a conforming typo are still named, + // or the assertions above pass on a guard that reports nothing at all. + expect( + reasonFor(`${verdictBlock({ head: PR1675_HEAD, findings: {} }, 2)} +## Ally — Consolidated PR Review`), + ).toContain("version 2"); + expect( + reasonFor(`${verdictBlock({ head: PR1675_HEAD, findings: { critcal: 1, important: 0 } })} +## Ally — Consolidated PR Review`), + ).toContain("critcal"); + }); + it("does not report an unreadable block as carrying a finding", () => { // AC-3. The distinction is the whole point: "I could not read this" and // "this carries an unresolved finding" are different claims, and the gate diff --git a/server/src/__tests__/pr-comment-review-gate.test.ts b/server/src/__tests__/pr-comment-review-gate.test.ts index a27aa66331c9..e0b58173c8be 100644 --- a/server/src/__tests__/pr-comment-review-gate.test.ts +++ b/server/src/__tests__/pr-comment-review-gate.test.ts @@ -10,7 +10,7 @@ import { extractAllyReviewedHeadSha, hasActionablePrReviewFeedback, hasAllyConsolidatedReviewHeading, - isConformingDispositionVerb, + isPublishableToken, } from "../services/ally-review-detection.js"; import { commentReviewGateCheckConclusion, @@ -462,6 +462,49 @@ describe("evaluateCommentReviewGate", () => { expect(verdict.reason).toContain(OLD_HEAD.slice(0, 7)); }); + it("never publishes an unsupported severity key verbatim (PEN-3157)", () => { + // Sibling of the ledger-verb case below, one JSON object over: an + // unsupported `findings` key is model-authored, and asSeverityCounts quotes + // it into the `unreadable` reason, which becomes the commit-status + // description and the *uncapped* check-run summary. The verb guard did not + // cover it, and the verb test would not have caught it — the leak was the + // same token one field away from the one that was fixtured. + const token = "ghp_abcdefghijklmnopqrstuvwxyz0123456789"; + const verdictFor = (severityKey: string) => + evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [ + allyComment( + [ + "## Ally — Consolidated PR Review", + "", + `Reviewed head: ${CURRENT_HEAD}`, + ].join("\n"), + "2026-08-04T21:09:19Z", + ), + ], + }); + + const leaked = verdictFor(token); + expect(leaked.reason ?? "").not.toContain(token); + expect(leaked.reason ?? "").not.toContain("ghp_"); + // A URL is alphabet-conforming nowhere near the verb alphabet, so it must + // be withheld too — `.toLowerCase()` mangles case but is not a guard. + expect(verdictFor("https://hooks.example.io/s3cr3t-9f2a").reason ?? "").not.toContain("s3cr3t"); + // Withheld, not dropped: the reader must still learn which defect this is. + expect(leaked.reason ?? "").toContain("unsupported severity"); + + // Positive control. A conforming typo is still named, or the assertions + // above would pass on a guard that simply stopped reporting the key — and + // naming the rejected key is the whole point of this reason string. + expect(verdictFor("critcal").reason ?? "").toContain("critcal"); + }); + it("never publishes a ledger verb outside the prose parser's alphabet (PEN-3157)", () => { // An unrecognized verb is quoted verbatim into the commit-status // description, and githubPostCommitStatusDetailed POSTs that description @@ -516,13 +559,12 @@ describe("evaluateCommentReviewGate", () => { }); it("the publisher's alphabet is the parser's alphabet", () => { - // `[a-z][a-z-]*` is spelled TWICE in ally-review-detection.ts: once as the - // verb group of PRIOR_FINDING_DISPOSITION_PATTERN, once as - // DISPOSITION_VERB_PATTERN behind isConformingDispositionVerb. That is - // deliberate — the PEN-3157 pin in github-write-egress-scrub.test.ts reads - // the first out of the pattern's own SOURCE TEXT, so folding them into a - // shared constant makes a pure refactor read to that test as a widening of - // a security bound. This is what stops the two copies drifting instead. + // `[a-z][a-z-]*` is PUBLISHABLE_TOKEN_ALPHABET in ally-review-detection.ts, + // interpolated into both the verb group of PRIOR_FINDING_DISPOSITION_PATTERN + // and the anchored pattern behind isPublishableToken. Sharing the source + // text is what stops them drifting; this test is what proves sharing it + // actually yields the same answers, since the two embed it differently — + // one anchored `^…$`, one inside a larger bold-list-item match. // // The bound only holds if the publisher's copy is no WIDER than the // parser's: a verb the parser will extract from prose and the publisher @@ -553,15 +595,15 @@ describe("evaluateCommentReviewGate", () => { (entry) => entry.disposition === verb, ); expect( - isConformingDispositionVerb(verb), + isPublishableToken(verb), `publisher vs parser disagree on ${JSON.stringify(verb)}`, ).toBe(parserAdmits); } // Positive control: the corpus must actually exercise both answers, or the // loop above passes on a corpus that is entirely one-sided. - expect(verbs.filter((v) => isConformingDispositionVerb(v)).length).toBeGreaterThan(0); - expect(verbs.filter((v) => !isConformingDispositionVerb(v)).length).toBeGreaterThan(0); + expect(verbs.filter((v) => isPublishableToken(v)).length).toBeGreaterThan(0); + expect(verbs.filter((v) => !isPublishableToken(v)).length).toBeGreaterThan(0); }); it("keeps the ordinary reason when no unrecognized verb is involved", () => { diff --git a/server/src/services/ally-review-detection.ts b/server/src/services/ally-review-detection.ts index 04906846db1c..f923e9956f5a 100644 --- a/server/src/services/ally-review-detection.ts +++ b/server/src/services/ally-review-detection.ts @@ -366,7 +366,10 @@ function asSeverityCounts(raw: unknown): Map | string { // the gate description is the only place Ally sees which key we rejected. // Reporting `exceeds 1000` for it names the wrong defect. if (!VERDICT_SEVERITIES.has(key)) { - return `ally-verdict findings name unsupported severity \`${key}\``; + // Named, not quoted: `key` is model-authored and this reason reaches the + // same unscrubbed boundary as the ledger verb (PEN-3157) — see + // asPublishableToken. + return `ally-verdict findings name unsupported severity \`${asPublishableToken(key)}\``; } // `Number.isInteger(1e100)` is true, and the ref loops in // extractAllyReportedFindingRefs enumerate 1..count. Without a ceiling a @@ -472,7 +475,12 @@ export function parseAllyVerdictBlock(body: string | null | undefined): AllyVerd const [, rawVersion, rawPayload] = blocks[0]!; if (Number(rawVersion) !== SUPPORTED_ALLY_VERDICT_VERSION) { - return { kind: "unreadable", reason: `unsupported ally-verdict version ${rawVersion}` }; + // Digits only, so it cannot carry a credential — but it is unbounded in + // length and lands in the uncapped check-run summary with everything else. + return { + kind: "unreadable", + reason: `unsupported ally-verdict version ${rawVersion!.slice(0, PUBLISHABLE_TOKEN_BUDGET)}`, + }; } let parsed: unknown; @@ -667,14 +675,12 @@ function hasNonNegatedMatch(text: string, pattern: RegExp): boolean { return false; } -// The alphabet a prose ledger entry may spell its verb in. Deliberately a -// SECOND literal copy of the verb group in PRIOR_FINDING_DISPOSITION_PATTERN -// below, rather than a shared constant: the PEN-3157 pin in -// github-write-egress-scrub.test.ts reads `([a-z][a-z-]*)` out of that -// pattern's own source text, so interpolating a constant there makes a -// refactor read as a widening of a security bound. The two copies cannot drift -// — "the publisher's alphabet is the parser's alphabet" in -// pr-comment-review-gate.test.ts holds them equal. +// The alphabet a prose ledger entry may spell its verb in, and — because it is +// the same question — the alphabet any model-authored token may be quoted in +// when the gate names it publicly. Shared as source text with the verb group of +// PRIOR_FINDING_DISPOSITION_PATTERN below so the parser's alphabet and the +// publisher's cannot drift; "the publisher's alphabet is the parser's alphabet" +// in pr-comment-review-gate.test.ts drives both and pins that they agree. // // The structured block deliberately does NOT enforce it. An unknown verb // already fails closed as `unrecognized`, so rejecting the whole block over a @@ -684,24 +690,43 @@ function hasNonNegatedMatch(text: string, pattern: RegExp): boolean { // `stillPresentIn` in check-ally-review-consistency.mjs) — gate red, peers // silent. // -// What the alphabet is needed for is publication: see -// isConformingDispositionVerb. -const DISPOSITION_VERB_PATTERN = /^[a-z][a-z-]*$/; +// What the alphabet is needed for is publication: see asPublishableToken. +const PUBLISHABLE_TOKEN_ALPHABET = String.raw`[a-z][a-z-]*`; +const PUBLISHABLE_TOKEN_PATTERN = new RegExp(`^${PUBLISHABLE_TOKEN_ALPHABET}$`); + +// Characters of a single model-authored token the gate will quote. The +// alphabet already rules out a credential; this only keeps one token from +// crowding out the phrase that makes the red actionable, in the check-run +// summary that — unlike the commit status — has no cap of its own. +const PUBLISHABLE_TOKEN_BUDGET = 48; + +// Stands in for a token that must not be published verbatim. It is not a value +// the alphabet admits — `<` is outside it — so it cannot be mistaken for one, +// and it keeps the drift visible while withholding its text. +export const NON_CONFORMING_TOKEN = ""; /** - * May this verb be quoted into a public commit-status description? + * May this model-authored token be quoted into a public commit-status + * description? * - * The gate names an unrecognized verb verbatim so a reader can tell vocabulary - * drift from a genuinely open finding. That description is POSTed by - * githubPostCommitStatusDetailed, which github-egress-outbound-coverage.test.ts - * classifies `unscrubbed` under PEN-3157 — so the text it carries is bounded - * only by whatever produced it. Prose was bounded by the pattern above; - * `verb` arriving as a JSON field is typed as any non-empty string, so a - * credential-shaped token in the ledger was published where the identical token - * in prose was refused. Callers name the drift, not its payload. + * The gate names an unrecognized ledger verb, and an unsupported severity key, + * verbatim so a reader can tell drift from a genuinely open finding. That + * description is POSTed by githubPostCommitStatusDetailed, which + * github-egress-outbound-coverage.test.ts classifies `unscrubbed` under + * PEN-3157 — so the text it carries is bounded only by whatever produced it. + * Prose was bounded by the pattern above; the same token arriving as a JSON + * field or object key is bounded by nothing, so it was published where the + * identical token in prose was refused. Callers name the drift, not its payload. */ -export function isConformingDispositionVerb(verb: string): boolean { - return DISPOSITION_VERB_PATTERN.test(verb); +export function isPublishableToken(token: string): boolean { + return PUBLISHABLE_TOKEN_PATTERN.test(token); +} + +/** The token itself when that is safe, otherwise a stand-in naming the drift. */ +export function asPublishableToken(token: string): string { + return isPublishableToken(token) + ? token.slice(0, PUBLISHABLE_TOKEN_BUDGET) + : NON_CONFORMING_TOKEN; } // A "Prior Findings Dispositioned" ledger entry, e.g. @@ -720,10 +745,10 @@ export function isConformingDispositionVerb(verb: string): boolean { // so the bound excludes no observed real entry; and an entry it did exclude // would leave a visible red rather than a silent green. // -// The `([a-z][a-z-]*)` verb group is pinned as source text by the PEN-3157 -// test in github-write-egress-scrub.test.ts — keep it literal here. +// The verb group is PUBLISHABLE_TOKEN_ALPHABET, shared with the publisher +// guard above so the two cannot drift. 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]*(?:—|–|-)`, + String.raw`^${NOT_INDENTED_CODE} {0,3}-[ \t]*\*\*[ \t]*prior:([0-9a-f]{7,40})[ \t]+([a-z]+)[ \t]+(\d+)[ \t]*\*\*[ \t]*(?:—|–|-)[ \t]*(${PUBLISHABLE_TOKEN_ALPHABET})[ \t]*(?:—|–|-)`, "gim", ); diff --git a/server/src/services/pr-comment-review-gate.ts b/server/src/services/pr-comment-review-gate.ts index 50d2c7d64f94..8ce997bd6c20 100644 --- a/server/src/services/pr-comment-review-gate.ts +++ b/server/src/services/pr-comment-review-gate.ts @@ -21,7 +21,7 @@ import { allyClaimedReviewHead, hasActionablePrReviewFeedback, hasAllyConsolidatedReviewHeading, - isConformingDispositionVerb, + asPublishableToken, parseAllyVerdictBlock, type AllyFindingRef, type AllyPriorFindingDisposition, @@ -45,11 +45,6 @@ const DEFAULT_PR_REVIEWER_BOT_LOGIN = "allyblockcast[bot]"; // the red actionable. const UNRECOGNIZED_VERB_BUDGET = 48; -// Stands in for a ledger verb that must not be published verbatim. It is not a -// verb Ally can emit — the alphabet excludes `<` — so it cannot be confused -// for one, and it keeps the drift visible while withholding its text. -const NON_CONFORMING_VERB = ""; - export interface CommentReviewGateComment { authorLogin: string | null | undefined; body: string | null | undefined; @@ -402,12 +397,8 @@ function headsWithUndispositionedFinding( if (namesFinding(prior, headSha, entry.timeMs, finding)) { // Name the drift, not its payload. This set is quoted verbatim into // the commit-status description below, which is POSTed unscrubbed - // (PEN-3157) — see isConformingDispositionVerb. - verbs.add( - isConformingDispositionVerb(prior.entry.disposition) - ? prior.entry.disposition - : NON_CONFORMING_VERB, - ); + // (PEN-3157) — see asPublishableToken. + verbs.add(asPublishableToken(prior.entry.disposition)); } } } From 1d6f37853e1e238af15791a655af006660c09ca4 Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sun, 20 Sep 2026 13:20:23 +0000 Subject: [PATCH 08/13] fix(review-gate): read the ledger on the structured path, not just the counts (BLO-32695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Peer review of #1721 at 8e6e84bd0 — 1 Critical, 2 Important, 1 Suggestion. Critical: the structured `ok` branch of hasActionablePrReviewFeedback decided from `verdict.findings` alone, so a block stating {"critical":0} beside a ledger entry saying a prior Critical is `still-present` returned false. evaluateCommentReviewGate short-circuits on a current-head attestation before consulting the carry-forward, so nothing downstream re-examined it: forHead truthy -> false -> success/clean. A fail-open regression against master, where the identical body without a block blocks via the prose clause, and a live reader disagreement besides — check-ally-review-consistency.mjs read the same block as blocking and raised I2c. Gated on the same option as the prose twin so the carry-forward enumeration keeps asking its narrower question. Important 1: two JSON keys normalizing to one severity silently overwrote, last wins, so {"critical":1,"Critical":0} parsed ok with critical: 0. Reachable unlike an exact duplicate, which JSON.parse collapses first. All three readers shared it identically, so no peer reader caught it — guarded in all three. Important 2: check-ally-review-consistency.mjs was left on the narrow attestation form while the gate and the Python sweep were widened, dropping `**Reviewed head:** ` — the exact shape this repo's own comment names, and the BLO-31730 failure mode. Masked on block-carrying bodies; the harm landed on the pre-block population, in a file whose purpose is reader parity. Suggestion: bound the `(\d+)` prose count quoted back into the check-run summary. A noise bound, not a security one. Also reverts 8e6e84bd0's interpolation of PUBLISHABLE_TOKEN_ALPHABET into PRIOR_FINDING_DISPOSITION_PATTERN. That commit measured the file as absent and concluded the PEN-3157 reference was false; the measurement was scoped to this branch's worktree. github-write-egress-scrub.test.ts exists on origin/master and asserts `([a-z][a-z-]*)` out of the pattern's own source text (:423) — it landed after this branch diverged, and CI builds refs/pull/N/merge. Ally's "prior important 2 — fixed" disposition repeats the same branch-scoped read. The verb group stays a literal; the two copies are held equal by an existing test that drives both. Every guard mutation-tested alone: Critical 2 failures, duplicate-key ts 3, mjs regex 2, duplicate-key python 1, count bound 1. The count bound initially had none — dropping it left all 219 tests green — so it was a comment rather than a guard until pinned. tsc clean; 230/230 server, 151/151 mjs, 132/132 python. check-ally-review-consistency.mjs no worse (its one violation is #1220, unrelated). --- .github/scripts/sweep-stalled-ally-reviews.py | 6 ++ .../test_sweep_stalled_ally_reviews.py | 30 +++++++ scripts/check-ally-review-consistency.mjs | 33 ++++++- .../check-ally-review-consistency.test.mjs | 63 ++++++++++++++ .../ally-review-verdict-block.test.ts | 72 ++++++++++++++++ .../__tests__/pr-comment-review-gate.test.ts | 86 +++++++++++++++++-- server/src/services/ally-review-detection.ts | 83 +++++++++++++++--- 7 files changed, 352 insertions(+), 21 deletions(-) diff --git a/.github/scripts/sweep-stalled-ally-reviews.py b/.github/scripts/sweep-stalled-ally-reviews.py index eb234bd41e6a..11dd239a4282 100755 --- a/.github/scripts/sweep-stalled-ally-reviews.py +++ b/.github/scripts/sweep-stalled-ally-reviews.py @@ -274,6 +274,12 @@ def severity_counts(raw): key = js_trim(severity).lower() if isinstance(severity, str) else None if key not in VERDICT_SEVERITIES: return None + # Two keys normalizing to one severity: last-wins would let + # {"critical":1,"Critical":0} read clean. Mirrors the guard in + # ally-review-detection.ts and check-ally-review-consistency.mjs -- all + # three readers shared the bug identically, so none of them caught it. + if key in counts: + return None counts[key] = value if any(severity not in counts for severity in BLOCKING_SEVERITIES): return None diff --git a/.github/scripts/test_sweep_stalled_ally_reviews.py b/.github/scripts/test_sweep_stalled_ally_reviews.py index ef765408fd63..9174b8abc265 100644 --- a/.github/scripts/test_sweep_stalled_ally_reviews.py +++ b/.github/scripts/test_sweep_stalled_ally_reviews.py @@ -1073,6 +1073,36 @@ def test_partial_sha_in_block_fails_closed(self): ) ) + def test_two_keys_normalizing_to_one_severity_fail_closed(self): + """Peer review of #1721 at 8e6e84bd -- shared by all three readers. + + `json.loads` keeps "critical" and "Critical" as distinct keys; they + become one severity only at the `.lower()` in severity_counts, where an + unconditional assignment let the last one win. So a block stating a + Critical could read clean. Reachable precisely because the keys differ + in case -- an exact duplicate is collapsed by the parser before this + code sees it. Both orders, because last-wins made the verdict depend on + key order and a guard catching only one order leaves the dangerous one. + """ + for payload in ( + '{"critical":0,"Critical":1,"important":0}', + '{"Critical":1,"critical":0,"important":0}', + ): + body = '' % ( + self.HEAD, + payload, + ) + self.assertIsNone(sweep.parse_reviewed_head(body), payload) + + def test_distinct_severities_are_still_accepted(self): + """Control: without it the guard would reject every honest verdict.""" + body = ( + '" % self.HEAD + ) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD) + def test_a_zero_padded_version_reads_the_same_here_as_in_the_two_js_readers(self): """Peer review of #1721, Suggestion 1 -- the version compare diverged. diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index 13d8e10e9b36..c73fb02f9176 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -127,9 +127,33 @@ const STILL_PRESENT_DISPOSITION_RE = new RegExp( "im", ); -/** The single standalone attestation line Ally is required to emit. */ +/** + * The single standalone attestation line Ally is required to emit. + * + * The emphasis runs mirror MARKDOWN_EMPHASIS_RUN / ATTESTATION_WRAPPER_RUN in + * ally-review-detection.ts:166-167 and sweep-stalled-ally-reviews.py:92-104, + * character for character. This reader was left on the narrow + * `(?:[_*]+)?` / `` \`? `` form while the other two were widened, and the shape + * it dropped is the one this repo's own comment names at + * ally-review-detection.ts:138-143 — ``**Reviewed head:** `` `` — where the + * single permitted run is consumed by `**` and cannot then cross the space to + * reach the backtick. Measured on the same bodies before this change: gate 1, + * python 1, mjs 0, for both that form and `_Reviewed head:_ `. + * + * Block-carrying bodies masked it, because attestedHeadFrom falls through to + * block.head. The harm landed on the entire pre-block population, where + * canonicalReviewHead returned null and operativeAllyReviews dropped a review + * the gate reads fine — in a file whose stated purpose is reader parity. + * + * The inter-run bound is `[ \t]{0,3}` rather than `[ \t]*` deliberately: the + * widening has a converse, and `*` here would accept a line the gate rejects, + * which is the same divergence one delimiter out. + */ +const MARKDOWN_EMPHASIS_RUN = String.raw`[*_\`]{0,3}`; +const ATTESTATION_WRAPPER_RUN = String.raw`[*_\`\t ]{0,6}`; const ATTESTED_HEAD_RE = new RegExp( - String.raw`^${NOT_INDENTED_CODE} {0,3}(?:[_*]+)?[ \t]*reviewed head:[ \t]*\`?([0-9a-f]{40})\`?[ \t]*(?:[_*]+)?[ \t]*$`, + String.raw`^${NOT_INDENTED_CODE} {0,3}${MARKDOWN_EMPHASIS_RUN}[ \t]{0,3}reviewed head:[ \t]*` + + String.raw`${ATTESTATION_WRAPPER_RUN}([0-9a-f]{40})${ATTESTATION_WRAPPER_RUN}[ \t]*$`, "im", ); const ATTESTED_HEAD_GLOBAL_RE = new RegExp(ATTESTED_HEAD_RE.source, "gim"); @@ -174,6 +198,11 @@ function severityCountsIn(raw) { if (value > MAX_VERDICT_FINDING_COUNT) return null; const key = severity.trim().toLowerCase(); if (!VERDICT_SEVERITIES.has(key)) return null; + // Two keys normalizing to one severity: last-wins would let + // `{"critical":1,"Critical":0}` read clean. Mirrors the guard in + // ally-review-detection.ts and sweep-stalled-ally-reviews.py — all three + // readers shared the bug identically, so none of them caught it. + if (counts.has(key)) return null; counts.set(key, value); } if (!BLOCKING_SEVERITIES.every((severity) => counts.has(severity))) return null; diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index 2965b082d876..e2dc4055167d 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -183,6 +183,39 @@ describe("attestedHead", () => { assert.equal(attestedHead(`_Reviewed head: \`${HEAD}\`_`), HEAD); }); + // This reader was left on the narrow `(?:[_*]+)?` / `` \`? `` form while the + // gate and the Python sweep were both widened, so the emphasis forms below + // measured gate 1 / python 1 / mjs 0 on the same bodies. The first is named + // verbatim in ally-review-detection.ts's own comment as the BLO-31730 shape + // — the single permitted run is consumed by `**` and cannot then cross the + // space to reach the backtick. + // + // Block-carrying bodies masked it, because attestedHead falls through to the + // block's head. The harm landed on the whole pre-block population, where + // operativeAllyReviews dropped a review the gate reads fine — in a script + // whose stated purpose is reader parity. Found by Ally reviewing #1721 at + // 8e6e84bd. No block in these fixtures, deliberately: with one they pass + // whether or not the prose regex works. + for (const [label, line] of [ + ["emphasis closing after the colon", `**Reviewed head:** \`${HEAD}\``], + ["underscore emphasis closing after the colon", `_Reviewed head:_ ${HEAD}`], + ["a bold wrapper around the whole line", `**Reviewed head: ${HEAD}**`], + ["a backticked SHA with no emphasis", `Reviewed head: \`${HEAD}\``], + ]) { + it(`parses ${label}, as the gate and the Python sweep do`, () => { + assert.equal(attestedHead(line), HEAD); + }); + } + + it("does not widen past the gate's own indent bound", () => { + // The converse check: the widening has a direction, and accepting a line + // the gate rejects is the same divergence one delimiter out. The gate + // bounds the run between emphasis and the label at `[ \t]{0,3}`, and + // treats four leading spaces as indented code. + assert.equal(attestedHead(`** Reviewed head:** \`${HEAD}\``), null); + assert.equal(attestedHead(` Reviewed head: ${HEAD}`), null); + }); + it("returns null when no attestation is present", () => { assert.equal(attestedHead("## Ally — Consolidated PR Review"), null); }); @@ -213,6 +246,36 @@ describe("attestedHead", () => { assert.equal(attestedHead(`${block(HEAD)}\nReviewed head: ${other}`), null); }); + // Peer review of #1721 at 8e6e84bd -- shared identically by all three + // readers, so none of them caught it. `JSON.parse` keeps "critical" and + // "Critical" as distinct keys; they become one severity only at the + // `toLowerCase` in severityCountsIn, where an unconditional `set` let the + // last one win, so a block stating a Critical could read clean. Reachable + // because the keys differ in CASE -- an exact duplicate is collapsed by the + // parser first. Both orders, because last-wins made the verdict depend on + // key order and a guard catching one order leaves the dangerous one live. + for (const findings of [ + `{"critical":0,"Critical":1,"important":0}`, + `{"Critical":1,"critical":0,"important":0}`, + ]) { + it(`fails closed on two keys normalizing to one severity: ${findings}`, () => { + assert.equal( + attestedHead(``), + null, + ); + }); + } + + it("still accepts distinct severities", () => { + // Control: without it the guard would reject every honest verdict. + assert.equal( + attestedHead( + ``, + ), + HEAD, + ); + }); + it("fails closed on two blocks rather than falling back to prose", () => { const body = `${block(HEAD)}\n${block(HEAD)}\nReviewed head: ${HEAD}`; assert.equal(attestedHead(body), null); diff --git a/server/src/__tests__/ally-review-verdict-block.test.ts b/server/src/__tests__/ally-review-verdict-block.test.ts index 9036ddba231a..ce1c84856a41 100644 --- a/server/src/__tests__/ally-review-verdict-block.test.ts +++ b/server/src/__tests__/ally-review-verdict-block.test.ts @@ -586,6 +586,57 @@ describe("BLO-32695 — an unknown severity key fails closed, not open", () => { ).toMatchObject({ kind: "ok" }); }); + // The same fail-open arriving through a key that IS recognized. + // + // `{"critcal":1}` above fails closed; `{"Critical":1,"critical":0}` did not. + // The keys are distinct to `JSON.parse` — it keeps both — and only become one + // severity at the `trim().toLowerCase()` in the count loop, where an + // unconditional `set` let the last one win. So a block could state a Critical + // and clear the head, which is the BLO-29711 direction the unknown-key guard + // was written to close, entering through the door that guard holds open. An + // exact duplicate key is not reachable — `JSON.parse` collapses those before + // this code sees them — which is why case is the shape that matters. + // + // Found by Ally in review of #1721 at 8e6e84bd, who also established that + // `proseCountContradicting` cannot rescue it: the producer template heads + // buckets `### 🚨 Critical` with no `(N)`, which the emitted-bucket pattern + // requires. All three readers shared the bug identically, so no peer reader + // caught it either; the mjs and Python guards are pinned in their own suites. + describe("two keys normalizing to one severity fail closed", () => { + it("reads a case-variant duplicate as unreadable, not as the last value written", () => { + expect(parseAllyVerdictBlock(agreeing({ critical: 0, Critical: 1, important: 0 }))).toMatchObject({ + kind: "unreadable", + reason: /same severity twice/, + }); + }); + + it("fails closed whichever order the block states them in", () => { + // Order-independence is the whole point: last-wins is what made the + // verdict depend on key order, so a guard that only catches one order + // leaves the dangerous one — non-zero first, zero second — still live. + expect(parseAllyVerdictBlock(agreeing({ Critical: 1, critical: 0, important: 0 }))).toMatchObject({ + kind: "unreadable", + }); + }); + + it("names the repetition without quoting the model-authored key", () => { + // PEN-3157: this reason reaches the same unscrubbed boundary as the + // ledger verb. The defect is the repetition, not which severity + // repeated, so there is nothing to quote. + const parsed = parseAllyVerdictBlock(agreeing({ critical: 0, Critical: 1, important: 0 })); + expect(parsed).toMatchObject({ kind: "unreadable" }); + expect((parsed as { reason: string }).reason).not.toMatch(/`/); + }); + + it("control: distinct severities are still accepted", () => { + // Without this the guard could be satisfied by any second key at all, + // which would reject every honest verdict. + expect( + parseAllyVerdictBlock(agreeing({ critical: 0, important: 0, suggestions: 1 })), + ).toMatchObject({ kind: "ok" }); + }); + }); + it("does not constrain disposition severities, which are open by design", () => { // #1675's ledger retires `prior:583085ded recommended-action 4` — failure #4 // of this row. The findings vocabulary is closed because a typo there clears @@ -1331,6 +1382,27 @@ describe("BLO-32695 — the block and the prose must not name different counts", }); }); + // Peer review of #1721, Suggestion 1. The count is a `(\d+)` capture over + // model-authored text: digits-only, but unbounded in length, and this reason + // reaches the check-run summary, which has no cap of its own. A noise bound + // rather than a security one — digits cannot carry a credential — so this + // pins the same call already made for `rawVersion` and for token length. + // + // Without this the bound had no failing mutation: dropping the `.slice` left + // all 219 tests in the two gate suites green, which made it a comment rather + // than a guard. + it("bounds the enumerated count it quotes back", () => { + const flood = "9".repeat(400); + const result = parseAllyVerdictBlock( + body({ head: PR1675_HEAD, findings: { critical: 0, important: 0 } }, `### Critical Issues (${flood})`), + ); + expect(result).toMatchObject({ kind: "unreadable" }); + const reason = (result as { reason: string }).reason; + expect(reason).not.toContain(flood); + expect(reason).toContain("9".repeat(48)); + expect(reason).not.toContain("9".repeat(49)); + }); + it("does not resolve that body to success", () => { expect( evaluateCommentReviewGate({ diff --git a/server/src/__tests__/pr-comment-review-gate.test.ts b/server/src/__tests__/pr-comment-review-gate.test.ts index e0b58173c8be..25bf03e1d5b5 100644 --- a/server/src/__tests__/pr-comment-review-gate.test.ts +++ b/server/src/__tests__/pr-comment-review-gate.test.ts @@ -559,12 +559,16 @@ describe("evaluateCommentReviewGate", () => { }); it("the publisher's alphabet is the parser's alphabet", () => { - // `[a-z][a-z-]*` is PUBLISHABLE_TOKEN_ALPHABET in ally-review-detection.ts, - // interpolated into both the verb group of PRIOR_FINDING_DISPOSITION_PATTERN - // and the anchored pattern behind isPublishableToken. Sharing the source - // text is what stops them drifting; this test is what proves sharing it - // actually yields the same answers, since the two embed it differently — - // one anchored `^…$`, one inside a larger bold-list-item match. + // `[a-z][a-z-]*` appears TWICE in ally-review-detection.ts: as + // PUBLISHABLE_TOKEN_ALPHABET behind isPublishableToken, and as a literal + // verb group inside PRIOR_FINDING_DISPOSITION_PATTERN. The duplication is + // deliberate — the PEN-3157 pin in master's github-write-egress-scrub.test + // reads that group out of the pattern's own source text, so interpolating + // the constant there reads as a widening of a security bound. THIS TEST IS + // WHAT MAKES THE DUPLICATION SAFE, so it is load-bearing rather than + // belt-and-braces: it drives both copies and pins that they agree, which is + // non-trivial because they embed the alphabet differently — one anchored + // `^…$`, one inside a larger bold-list-item match. // // The bound only holds if the publisher's copy is no WIDER than the // parser's: a verb the parser will extract from prose and the publisher @@ -1423,6 +1427,76 @@ describe("clean-review precedence over the Recommended Action prose fallback", ( ); }); + // The same assertion, carried by the STRUCTURED block instead of prose. + // + // Every block-carrying still-present fixture above attests INTERMEDIATE_HEAD + // while evaluating CURRENT_HEAD, so all of them exercise the carried path and + // none placed the entry at the head under evaluation — the case one head over + // from the one written. That gap hid a fail-open: the `ok` branch of + // hasActionablePrReviewFeedback decided from `verdict.findings` alone, so a + // block stating `{"critical":0,"important":0}` beside a ledger entry saying a + // prior Critical is `still-present` returned false. evaluateCommentReviewGate + // short-circuits on a current-head attestation before consulting the + // carry-forward, so nothing downstream re-examined it and the gate published + // `success`/`clean`. A regression against master, where the identical body + // without a block blocks via the prose clause. Found by Ally in review of + // #1721 at 8e6e84bd. + const blockCarryingStillPresent = (headSha: string, verb: string) => + [ + "## Ally — Consolidated PR Review", + "", + "", + "", + `Reviewed head: ${headSha}`, + "### Critical Issues (0)", + "### Important Issues (0)", + "### Recommended Action", + "1. No Critical issues to fix before merge.", + ].join("\n"); + + it("still blocks a structured 0/0 block whose ledger asserts still-present at this head", () => { + expect(hasActionablePrReviewFeedback(blockCarryingStillPresent(CURRENT_HEAD, "still-present"))).toBe( + true, + ); + }); + + it("does not clear the head when a structured still-present block is evaluated end to end", () => { + // Driven through the real evaluator, because the unit above passes while + // the gate still goes green if the short-circuit ordering is what leaks. + // This is the assertion that would have caught the regression. + expect( + evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(blockCarryingStillPresent(CURRENT_HEAD, "still-present"), "2026-08-04T21:09:19Z")], + }), + ).not.toMatchObject({ state: "success" }); + }); + + it("clears a structured block whose ledger only retires prior findings", () => { + // Control for both cases above: `fixed` classifies as `retires`. Without + // it the new guard could be satisfied by any ledger entry at all, which + // would red-wedge every review that correctly reports a fix. + expect(hasActionablePrReviewFeedback(blockCarryingStillPresent(CURRENT_HEAD, "fixed"))).toBe(false); + }); + + it("does not count a structured still-present entry when the caller asks the narrower question", () => { + // The carry-forward enumeration asks "which findings did *this head* + // raise?" and passes false, for the reason ActionableFeedbackOptions + // documents. The structured clause is gated on the same option as its + // prose twin, so the two cannot drift on which question they answer. + expect( + hasActionablePrReviewFeedback(blockCarryingStillPresent(CURRENT_HEAD, "still-present"), undefined, { + countInheritedLedgerAssertion: false, + }), + ).toBe(false); + }); + it("clears a 0/0 review whose ledger only retires prior findings", () => { // The control for the case above: `fixed` classifies as `retires`, so it // must not block. Without this, the still-present guard could be satisfied diff --git a/server/src/services/ally-review-detection.ts b/server/src/services/ally-review-detection.ts index f923e9956f5a..d11340b9c95c 100644 --- a/server/src/services/ally-review-detection.ts +++ b/server/src/services/ally-review-detection.ts @@ -377,6 +377,19 @@ function asSeverityCounts(raw: unknown): Map | string { if (value > MAX_VERDICT_FINDING_COUNT) { return `ally-verdict findings count \`${key}\` exceeds ${MAX_VERDICT_FINDING_COUNT}`; } + // Two JSON keys that normalize to the same severity are distinct until the + // `toLowerCase` above, so `JSON.parse` keeps both and an unconditional set + // would let the last one win: `{"critical":1,"Critical":0}` would parse ok + // with `critical: 0` and clear the head. This is the BLO-29711 hole the + // unsupported-key guard at :368 was written to close, arriving through a + // key that *is* recognized — `{"critcal":1}` already fails closed, + // `{"Critical":1}` did not. Unlike an exact duplicate key, which JSON.parse + // collapses before we see it, this is reachable. The key is not quoted for + // the PEN-3157 reason above, and naming it is not needed: the defect is the + // repetition, not which severity repeated. + if (counts.has(key)) { + return "ally-verdict findings state the same severity twice"; + } counts.set(key, value); } // Absent counts are not zero counts — the same rule the caller applies to a @@ -638,7 +651,13 @@ function proseCountContradicting(text: string, counts: Map): str const key = severity!.toLowerCase(); if (!BLOCKING_SEVERITIES.has(key)) continue; if (Number(count) > 0 && counts.get(key) === 0) { - return `ally-verdict states 0 \`${key}\` but the review enumerates ${count}`; + // `count` is the `(\d+)` capture over model-authored review text, so it + // is digits-only but unbounded in length, and this reason reaches the + // check-run summary, which has no cap of its own. A noise bound, not a + // security one — digits cannot carry a credential — so it is the same + // call already made for `rawVersion` at :495 and for token length in + // asPublishableToken, applied here for the symmetry. + return `ally-verdict states 0 \`${key}\` but the review enumerates ${count!.slice(0, PUBLISHABLE_TOKEN_BUDGET)}`; } } return null; @@ -677,10 +696,13 @@ function hasNonNegatedMatch(text: string, pattern: RegExp): boolean { // The alphabet a prose ledger entry may spell its verb in, and — because it is // the same question — the alphabet any model-authored token may be quoted in -// when the gate names it publicly. Shared as source text with the verb group of -// PRIOR_FINDING_DISPOSITION_PATTERN below so the parser's alphabet and the -// publisher's cannot drift; "the publisher's alphabet is the parser's alphabet" -// in pr-comment-review-gate.test.ts drives both and pins that they agree. +// when the gate names it publicly. Deliberately NOT interpolated into the verb +// group of PRIOR_FINDING_DISPOSITION_PATTERN below: that group must stay a +// literal for the PEN-3157 source-text pin, for the reason set out there. The +// two copies are held equal by "the publisher's alphabet is the parser's +// alphabet" in pr-comment-review-gate.test.ts, which drives both and is +// non-trivial because they embed the alphabet differently — anchored here, +// inside the list-item match there. // // The structured block deliberately does NOT enforce it. An unknown verb // already fails closed as `unrecognized`, so rejecting the whole block over a @@ -745,10 +767,23 @@ export function asPublishableToken(token: string): string { // so the bound excludes no observed real entry; and an entry it did exclude // would leave a visible red rather than a silent green. // -// The verb group is PUBLISHABLE_TOKEN_ALPHABET, shared with the publisher -// guard above so the two cannot drift. +// The verb group is a deliberate SECOND literal copy of +// PUBLISHABLE_TOKEN_ALPHABET, not an interpolation of it. The PEN-3157 pin in +// server/src/__tests__/github-write-egress-scrub.test.ts (master, added by +// 1a895b2f8, last touched 43c3875de) reads `([a-z][a-z-]*)` out of this +// pattern's own SOURCE TEXT, so interpolating the constant here makes a +// refactor that widens nothing read to that test as a widening of a security +// bound. The file is absent from this branch — it landed on master after this +// branch diverged, and CI builds `refs/pull/N/merge` — so `grep` in a worktree +// reports it missing. Commit 8e6e84bd0 made exactly that measurement and +// concluded the reference was false, which re-opened this red; the file exists +// and has never been deleted. +// +// The two copies cannot drift: "the publisher's alphabet is the parser's +// alphabet" in pr-comment-review-gate.test.ts drives the real parser over a +// verb corpus and asserts isPublishableToken agrees on every one. 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]*(${PUBLISHABLE_TOKEN_ALPHABET})[ \t]*(?:—|–|-)`, + 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", ); @@ -1158,16 +1193,38 @@ export function hasActionablePrReviewFeedback( if (normalizedState === "changes_requested" || normalizedState === "changes-requested") return true; if (typeof body !== "string") return false; - // With a structured block, the blocking-severity counts decide and nothing - // else is consulted. This is the AC-3 half of BLO-32695: `blocking_finding` - // becomes reachable only from a finding Ally actually counted, never from - // prose that merely reads as actionable. The clauses below stay for - // block-less bodies, where dropping them would fail open. + // With a structured block, the counts and the ledger decide and nothing else + // is consulted. This is the AC-3 half of BLO-32695: `blocking_finding` + // becomes reachable only from a finding Ally actually counted or a ledger + // entry Ally actually wrote, never from prose that merely reads as + // actionable. The clauses below stay for block-less bodies, where dropping + // them would fail open. const block = parseAllyVerdictBlock(body); if (block.kind === "ok") { for (const [severity, count] of block.verdict.findings) { if (BLOCKING_SEVERITIES.has(severity) && count > 0) return true; } + // The structured twin of the prose ledger clause at :1102, and it is load + // bearing for the same reason that one is: the contract says a + // still-standing finding is mirrored into the current buckets, and this is + // the defence for when that mirroring is omitted. Deciding from `findings` + // alone made a block stating `{"critical":0}` beside a ledger entry saying + // a prior Critical is `still-present` return false, and + // `evaluateCommentReviewGate` short-circuits on a current-head attestation + // before consulting the carry-forward, so nothing downstream re-examined + // it: `pr-comment-review-gate.ts:596` forHead truthy -> here -> false -> + // `success`/`clean`. That was a fail-open regression against master, where + // the identical body without a block blocks via :1102, and a live reader + // disagreement besides — `check-ally-review-consistency.mjs` reads the + // same block as blocking and raises I2c. Found in peer review of #1721 at + // 8e6e84bd. Gated on the same option as the prose clause so the + // carry-forward enumeration keeps asking its narrower question. + if ( + options?.countInheritedLedgerAssertion !== false && + block.verdict.dispositions.some((entry) => classifyPriorDisposition(entry.verb) === "blocks") + ) { + return true; + } return false; } // An unreadable block falls through to prose rather than answering "no From dd61a4a2d7fb49ff708b69b1094ee53f757c8eec Mon Sep 17 00:00:00 2001 From: Release Engineer Date: Sun, 20 Sep 2026 23:50:40 +0000 Subject: [PATCH 09/13] fix(review-gate): fail a block closed when its prose ledger still stands (BLO-32695) The counts axis already fails a verdict block closed when the review's own prose contradicts it (proseCountContradicting). The ledger axis had no twin, so a block whose `dispositions` are absent, `[]`, or merely missing the entry took the `ok` branch, found zero counts, found no blocking disposition, and returned clean -- while the identical body *without* a block blocks via the prose ledger clause in carriesBlockingFeedback. A fail-open against master of the same shape as the one fixed at 1d6f3785, one axis over, and the producer template mandates emitting both the block and the prose ledger, which is exactly how the two come to disagree on this field. Found by Ally in review of #1721 at 1d6f3785 -- the last uncovered quadrant of the block/prose drift matrix this PR exists to close. Ported to all three readers, not just the gate. The py sweep is the gate's only automatic escape route: with it disagreeing, the gate goes red on `unreadable_verdict` while the sweep sees a review that already happened and never re-requests the one that would clear it. check-ally-review-consistency.mjs carried the identical fail-open in its own precedence (`structuredBlocking(...) ?? hasStillPresentDisposition(...)`) -- the auditor whose job is to notice this class of divergence had it too. Narrower than the reported shape on purpose, and the controls pin it: only a prose entry classifying as `blocks` can fail a block closed. A prose `fixed` entry the block omits clears either way, so reddening it would be a false red with no fail-open behind it -- the #1675 direction. Symmetrically, a block already carrying a blocking verb cannot fail open, so the prose is not consulted and a contract-compliant still-present review stays readable. Each guard has a failing mutation: reverting the TS call site reds 5 tests, the mjs 1, the py 2. Verified one mutation at a time, since two at once lets one mask the other. `node scripts/check-ally-review-consistency.mjs` returns the same 2 unbaselined violations before and after (both on other PRs). Comments cite symbols rather than line numbers -- the two `:1102` references this diff would itself have shifted are now `carriesBlockingFeedback`. --- .github/scripts/sweep-stalled-ally-reviews.py | 53 +++++++++++ .../test_sweep_stalled_ally_reviews.py | 74 +++++++++++++++ scripts/check-ally-review-consistency.mjs | 11 +++ .../check-ally-review-consistency.test.mjs | 33 +++++++ .../__tests__/pr-comment-review-gate.test.ts | 89 ++++++++++++++++++- server/src/services/ally-review-detection.ts | 46 ++++++++++ 6 files changed, 304 insertions(+), 2 deletions(-) diff --git a/.github/scripts/sweep-stalled-ally-reviews.py b/.github/scripts/sweep-stalled-ally-reviews.py index 11dd239a4282..1b68a7912536 100755 --- a/.github/scripts/sweep-stalled-ally-reviews.py +++ b/.github/scripts/sweep-stalled-ally-reviews.py @@ -153,6 +153,31 @@ def parse_list(value, fallback): r"(Critical|Important)[ \t]+Issues[ \t]*[*_]{0,3}[ \t]*\(([0-9]+)\)[*_]{0,3}[ \t]*$", re.IGNORECASE | re.MULTILINE | ASCII_RE, ) + +# A prose ledger entry, e.g. `- **prior:abc1234 critical 1** - still-present -`. +# Composed character-for-character as PRIOR_FINDING_DISPOSITION_PATTERN in +# ally-review-detection.ts, including the `(?! *\t)(?! {4})` indentation bound +# and the em/en/hyphen alternation. `[0-9]` rather than `\d` for the reason +# given at EMITTED_BUCKET_PATTERN, and the harm runs the same way here: a +# Unicode digit is no ledger entry at all to the JS readers, so widening it +# would send this sweep to re-request a review Ally already gave. +# +# Strict rather than reusing the looser `**prior:[^\n]***` form in +# check-ally-review-consistency.mjs. That reader is an auditor, where +# over-matching costs a reported violation; here it costs a duplicate review +# request, which is the BLO-22892/BLO-28203 loop. +PRIOR_FINDING_DISPOSITION_PATTERN = re.compile( + r"^(?! *\t)(?! {4}) {0,3}-[ \t]*\*\*[ \t]*prior:[0-9a-f]{7,40}[ \t]+[a-z]+[ \t]+[0-9]+" + r"[ \t]*\*\*[ \t]*(?:—|–|-)[ \t]*([a-z][a-z-]*)[ \t]*(?:—|–|-)", + re.IGNORECASE | re.MULTILINE | ASCII_RE, +) + +# The verb that asserts a prior finding still stands. Mirrors +# BLOCKING_PRIOR_DISPOSITIONS in check-ally-review-consistency.mjs and the +# `blocks` arm of classifyPriorDisposition in ally-review-detection.ts. An +# unrecognized verb is deliberately not blocking in any of the three. +BLOCKING_PRIOR_DISPOSITIONS = frozenset(("still-present",)) + FENCE_OPEN_PATTERN = re.compile(r"^ {0,3}(`{3,}|~{3,})(.*)$", ASCII_RE) FENCE_CLOSE_PATTERN = re.compile(r"^ {0,3}(`{3,}|~{3,})[ \t]*$", ASCII_RE) @@ -343,6 +368,31 @@ def prose_count_contradicts(text, counts): return True return False + +def prose_disposition_contradicts(text, payload): + """A prose ledger entry that still stands against a block retiring them all. + + Mirrors proseDispositionContradicting in ally-review-detection.ts. The gate + reads `dispositions` for a blocking verb exactly as it reads `findings` for + a non-zero count, so a block whose ledger is absent, `[]`, or merely missing + the entry suppresses a prose entry saying a prior finding stands. Without + this the gate goes red on `unreadable_verdict` while this sweep sees a + review that already happened and never re-requests the one that would clear + it -- the same asymmetry the `findings` check above exists for. + + Asymmetric like that one: a block already carrying a blocking verb cannot + fail open, so the prose is not consulted, and a prose entry that only + retires clears either way. + """ + for entry in payload.get("dispositions") or (): + verb = entry.get("verb") if isinstance(entry, dict) else None + if isinstance(verb, str) and js_trim(verb).lower() in BLOCKING_PRIOR_DISPOSITIONS: + return False + for verb in PRIOR_FINDING_DISPOSITION_PATTERN.findall(without_fenced_spans(text)): + if verb.lower() in BLOCKING_PRIOR_DISPOSITIONS: + return True + return False + # Same latitude the previous `startswith("## Ally") and "Consolidated PR Review" # in body` pair allowed, minus the first-byte anchor. CONSOLIDATED_HEADING_PATTERN = re.compile( @@ -414,6 +464,9 @@ def parse_verdict_block_head(body): return ("unreadable", None) if prose_count_contradicts(text, counts): return ("unreadable", None) + # The same rule on the other field the payload carries. + if prose_disposition_contradicts(text, parsed): + return ("unreadable", None) return ("ok", js_trim(head).lower()) diff --git a/.github/scripts/test_sweep_stalled_ally_reviews.py b/.github/scripts/test_sweep_stalled_ally_reviews.py index 9174b8abc265..7e08237b3158 100644 --- a/.github/scripts/test_sweep_stalled_ally_reviews.py +++ b/.github/scripts/test_sweep_stalled_ally_reviews.py @@ -1199,6 +1199,80 @@ def test_findings_the_gate_rejects_are_rejected_here_too(self): self.assertIsNone(sweep.parse_reviewed_head(self.body(findings)), findings) +class TestVerdictLedgerMirrorsTheGate(unittest.TestCase): + """Peer review of #1721 at 1d6f3785 -- the count rule's twin on the other + field the gate decides from. + + `carriesBlockingFeedback` reads `dispositions` for a blocking verb exactly + as it reads `findings` for a non-zero count, so a block whose ledger is + absent, `[]`, or merely missing the entry suppressed a prose ledger entry + saying a prior finding still stands. Same asymmetry as the counts class + above: the gate goes red on `unreadable_verdict` and this sweep is the only + automatic route back, so a divergence here leaves the red with no escape. + """ + + HEAD = "e" * 40 + + def body(self, dispositions, verb): + return "\n".join( + [ + '' + % (self.HEAD, dispositions), + "", + "## Ally — Consolidated PR Review", + "### Critical Issues (0)", + "- **prior:abc1234 critical 1** — %s — the guard is unchanged." % verb, + ] + ) + + def test_a_standing_prose_ledger_against_a_block_retiring_everything(self): + for dispositions in ("", ',"dispositions":[]'): + self.assertIsNone( + sweep.parse_reviewed_head(self.body(dispositions, "still-present")), + dispositions or "absent", + ) + + def test_a_partially_drifted_ledger_is_the_same_hole(self): + dispositions = ',"dispositions":[{"head":"abc1234","severity":"important","index":1,"verb":"fixed"}]' + self.assertIsNone(sweep.parse_reviewed_head(self.body(dispositions, "still-present"))) + + def test_control_a_prose_ledger_that_only_retires_still_attests(self): + # Keeps this fail-closed rather than a widening: a `fixed` entry the + # block omits clears either way, so reddening it buys nothing and costs + # the #1675 direction. + self.assertEqual( + sweep.parse_reviewed_head(self.body(',"dispositions":[]', "fixed")), self.HEAD + ) + + def test_control_a_block_carrying_the_standing_entry_still_attests(self): + dispositions = ( + ',"dispositions":[{"head":"abc1234","severity":"critical","index":1,' + '"verb":"still-present"}]' + ) + self.assertEqual( + sweep.parse_reviewed_head(self.body(dispositions, "still-present")), self.HEAD + ) + + def test_a_quoted_or_fenced_ledger_does_not_fail_it_closed(self): + # Over-matching here re-requests a review Ally already gave, which is + # the duplicate-COMMENTED loop this file exists downstream of. + for quoted in ( + "> - **prior:abc1234 critical 1** — still-present — stands.", + "```\n- **prior:abc1234 critical 1** — still-present — stands.\n```", + " - **prior:abc1234 critical 1** — still-present — stands.", + ): + body = "\n".join( + [ + '' % self.HEAD, + "", + "## Ally — Consolidated PR Review", + quoted, + ] + ) + self.assertEqual(sweep.parse_reviewed_head(body), self.HEAD, quoted) + + class TestVerdictBlockMirrorsTheGateOnFencesAndLedgers(unittest.TestCase): """Peer review of #1721 at 97b4ddd1 -- the remaining two reader divergences. diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index c73fb02f9176..b99beb3fdd2a 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -341,6 +341,17 @@ function structuredVerdict(rawText) { const stillPresent = stillPresentIn(parsed?.dispositions); if (counts === null || stillPresent === null) return { kind: "unreadable" }; if (proseCountContradicts(text, counts)) return { kind: "unreadable" }; + // The same rule on the other field, mirroring proseDispositionContradicting + // in ally-review-detection.ts. `structuredBlocking(body, "stillPresent") ?? + // hasStillPresentDisposition(body)` in reportsStillPresent gives the block + // precedence, so a block stating no standing prior finding suppressed the + // prose ledger entirely — this reader carried the identical fail-open the + // gate did. + // + // Asymmetric like the count rule: only a *blocking* prose entry against a + // block that retires everything is fatal, so `stillPresent` true short- + // circuits and a `fixed` prose entry the block omits still reads clean. + if (!stillPresent && hasStillPresentDisposition(text)) return { kind: "unreadable" }; return { kind: "ok", head: head.trim().toLowerCase(), diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index e2dc4055167d..4966adfa9f73 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -334,6 +334,39 @@ describe("attestedHead", () => { assert.equal(attestedHead(counted('{"critical":0,"important":0}', prose)), HEAD, prose); } }); + + // Peer review of #1721, Important at 1d6f3785 — the same rule on the other + // field. `structuredBlocking(body, "stillPresent") ?? hasStillPresentDisposition(body)` + // gives the block precedence, so a block retiring everything suppressed a + // prose ledger entry saying a prior finding stands. Identical fail-open to + // the gate's, in the reader whose job is to notice the gate's. + const ledgered = (dispositions, verb) => + [ + ``, + "", + "## Ally — Consolidated PR Review", + "### Critical Issues (0)", + `- **prior:abc1234 critical 1** — ${verb} — the guard is unchanged.`, + ].join("\n"); + + it("fails closed when a prose ledger still stands against a block retiring everything", () => { + for (const dispositions of ["", ',"dispositions":[]']) { + assert.equal(attestedHead(ledgered(dispositions, "still-present")), null, dispositions || "absent"); + } + }); + + it("control: a prose ledger that only retires still attests", () => { + // Keeps this a fail-closed rule rather than a widening: a `fixed` entry + // the block omits clears either way, so reddening it buys nothing. + assert.equal(attestedHead(ledgered(',"dispositions":[]', "fixed")), HEAD); + }); + + it("control: a block that already carries the standing entry still attests", () => { + // It blocks — but as a structured verdict, not as an unreadable one, or + // every contract-compliant still-present review reads broken. + const dispositions = ',"dispositions":[{"head":"abc1234","severity":"critical","index":1,"verb":"still-present"}]'; + assert.equal(attestedHead(ledgered(dispositions, "still-present")), HEAD); + }); }); describe("operativeAllyReviews", () => { diff --git a/server/src/__tests__/pr-comment-review-gate.test.ts b/server/src/__tests__/pr-comment-review-gate.test.ts index 25bf03e1d5b5..615cc68ab10e 100644 --- a/server/src/__tests__/pr-comment-review-gate.test.ts +++ b/server/src/__tests__/pr-comment-review-gate.test.ts @@ -11,6 +11,7 @@ import { hasActionablePrReviewFeedback, hasAllyConsolidatedReviewHeading, isPublishableToken, + parseAllyVerdictBlock, } from "../services/ally-review-detection.js"; import { commentReviewGateCheckConclusion, @@ -1485,8 +1486,92 @@ describe("clean-review precedence over the Recommended Action prose fallback", ( expect(hasActionablePrReviewFeedback(blockCarryingStillPresent(CURRENT_HEAD, "fixed"))).toBe(false); }); - it("does not count a structured still-present entry when the caller asks the narrower question", () => { - // The carry-forward enumeration asks "which findings did *this head* + // The last uncovered quadrant of the block/prose drift matrix. The counts + // axis fails a block closed when prose contradicts it (proseCountContradicting); + // the ledger axis had no twin, so a block whose `dispositions` are absent, + // `[]`, or merely missing the entry took the `ok` branch, found zero counts, + // found an empty ledger, and returned false -- while the identical body + // *without* a block blocks via the prose ledger clause in + // carriesBlockingFeedback. The producer + // template mandates emitting both the block and the prose ledger, which is + // exactly how the two come to disagree on this field. Found by Ally in + // review of #1721 at 1d6f3785. + const proseLedgerAgainstBlock = (verb: string, dispositions?: unknown[]) => + [ + "## Ally — Consolidated PR Review", + "", + "", + "", + `Reviewed head: ${CURRENT_HEAD}`, + "### Critical Issues (0)", + "### Important Issues (0)", + "### Prior Findings Dispositioned (1)", + `- **prior:${OLD_HEAD.slice(0, 7)} critical 1** — ${verb} — the guard is unchanged.`, + "### Recommended Action", + "1. No Critical issues to fix before merge.", + ].join("\n"); + + for (const [label, dispositions] of [ + ["omits `dispositions`", undefined], + ["states `dispositions: []`", []], + ["names only a retired entry", [{ head: OLD_HEAD.slice(0, 7), severity: "important", index: 1, verb: "fixed" }]], + ] as const) { + it(`fails a block closed when it ${label} and the prose ledger still stands`, () => { + // Both shapes matter: absence is legitimately "this review retires + // nothing", and a partially-drifted ledger is the same hole one entry in. + const body = proseLedgerAgainstBlock("still-present", dispositions); + expect(parseAllyVerdictBlock(body)).toMatchObject({ kind: "unreadable" }); + expect(hasActionablePrReviewFeedback(body)).toBe(true); + }); + } + + it("does not clear the head when a block contradicting its prose ledger is evaluated end to end", () => { + // The load-bearing assertion. The unit above passes while the gate still + // goes green if evaluateCommentReviewGate reaches `success` by another + // route, which is how this family of fail-opens has escaped every time. + expect( + evaluateCommentReviewGate({ + headSha: CURRENT_HEAD, + comments: [allyComment(proseLedgerAgainstBlock("still-present", []), "2026-08-04T21:09:19Z")], + }), + ).not.toMatchObject({ state: "success" }); + }); + + it("reports a block contradicting its prose ledger as unreadable, never as carrying a finding", () => { + // The acceptance criterion this row exists for: `blocking_finding` must be + // reachable only from a structured finding. A body the parser cannot + // reconcile is unreadable, which is a distinguishable red. + expect(parseAllyVerdictBlock(proseLedgerAgainstBlock("still-present", []))).toMatchObject({ + reason: expect.stringContaining("prose ledger"), + }); + }); + + it("clears a block whose prose ledger only retires prior findings", () => { + // Control, and the one that keeps this from being a widening: a prose + // `fixed` entry the block omits clears either way, so failing closed on it + // would be a false red with no fail-open behind it -- the #1675 direction. + expect(parseAllyVerdictBlock(proseLedgerAgainstBlock("fixed", []))).toMatchObject({ kind: "ok" }); + expect(hasActionablePrReviewFeedback(proseLedgerAgainstBlock("fixed", []))).toBe(false); + }); + + it("stays readable when the block's ledger agrees with its prose ledger", () => { + // The real contract-compliant shape: both state the same standing entry. + // It must block, but as a *structured* finding rather than as an + // unreadable verdict, or every genuine still-present review reads broken. + const agreeing = proseLedgerAgainstBlock("still-present", [ + { head: OLD_HEAD.slice(0, 7), severity: "critical", index: 1, verb: "still-present" }, + ]); + expect(parseAllyVerdictBlock(agreeing)).toMatchObject({ kind: "ok" }); + expect(hasActionablePrReviewFeedback(agreeing)).toBe(true); + }); + + it("does not count a structured still-present entry when the caller asks the narrower question", () => { // The carry-forward enumeration asks "which findings did *this head* // raise?" and passes false, for the reason ActionableFeedbackOptions // documents. The structured clause is gated on the same option as its // prose twin, so the two cannot drift on which question they answer. diff --git a/server/src/services/ally-review-detection.ts b/server/src/services/ally-review-detection.ts index d11340b9c95c..1b226a5221ab 100644 --- a/server/src/services/ally-review-detection.ts +++ b/server/src/services/ally-review-detection.ts @@ -556,6 +556,9 @@ export function parseAllyVerdictBlock(body: string | null | undefined): AllyVerd const countDisagreement = proseCountContradicting(text, counts); if (countDisagreement !== null) return { kind: "unreadable", reason: countDisagreement }; + const ledgerDisagreement = proseDispositionContradicting(text, ledger); + if (ledgerDisagreement !== null) return { kind: "unreadable", reason: ledgerDisagreement }; + return { kind: "ok", verdict: { head: attestedHead, findings: counts, dispositions: ledger }, @@ -663,6 +666,49 @@ function proseCountContradicting(text: string, counts: Map): str return null; } +/** + * The count rule above, applied to the other field the gate decides from. + * + * `carriesBlockingFeedback` reads `dispositions` for a `blocks` verb exactly as + * it reads `findings` for a non-zero count, so the same block/prose drift fails + * open one axis over. A block whose ledger is absent, `[]`, or merely missing + * the entry takes the `ok` branch, finds zero counts, finds no blocking + * disposition, and returns `false` — while the identical body *without* a block + * blocks via the prose ledger clause in carriesBlockingFeedback. Found in + * peer review of #1721 at + * 1d6f3785; the producer template mandates emitting both the block and the + * prose ledger (`.planning/ally-agent/AGENTS.md`), which is precisely how the + * two come to disagree on this field. + * + * Asymmetric exactly like the count rule, and narrower than the reported shape + * on purpose: only a prose entry classifying as `blocks` can fail a block + * closed. A prose `fixed` entry the block omits clears either way, so failing + * on it would be a false red with no fail-open behind it — the #1675 direction. + * Symmetrically, a block already carrying a `blocks` entry cannot fail open, so + * the prose is not consulted at all. + * + * Shares PRIOR_FINDING_DISPOSITION_PATTERN and classifyPriorDisposition with + * the prose clause it defends, so by construction it fires exactly when that + * clause would have blocked — the two cannot drift into disagreeing about what + * "still stands" means. + * + * Quotes nothing. The severity and index captures are model-authored, and the + * reason reaches the uncapped check-run summary; there is no information in + * them the operator needs that "the prose ledger retains one" does not carry. + */ +function proseDispositionContradicting( + text: string, + ledger: AllyStructuredDisposition[], +): string | null { + if (ledger.some((entry) => classifyPriorDisposition(entry.verb) === "blocks")) return null; + for (const match of text.matchAll(PRIOR_FINDING_DISPOSITION_PATTERN)) { + if (classifyPriorDisposition(match[4]!) === "blocks") { + return "ally-verdict retires every prior finding but the review's prose ledger retains one"; + } + } + return null; +} + // Negation cues flip an otherwise-actionable bare phrase into a confirmation // that no follow-up is required. Limit the lookback to the local sentence so // an unrelated earlier negation does not mask a real later finding. From a412b4d8e7af3c8406d7dd1c9a01257cf7554965 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Tue, 22 Sep 2026 01:21:34 +0000 Subject: [PATCH 10/13] fix(scripts): read the prose ledger with the gate's own pattern (BLO-32695) The auditor decided "does the prose ledger still stand?" with a regex that was not composed character-for-character with PRIOR_FINDING_DISPOSITION_PATTERN in ally-review-detection.ts and sweep-stalled-ally-reviews.py, so the three readers disagreed in both directions (trailing parenthetical accepted here only; en dash and a space after ** accepted there only). Compose the pattern identically, decide the verb against BLOCKING_PRIOR_DISPOSITIONS, and add a cross-reader corpus that drives the same ledger strings through the patterns read out of all three committed files. Co-Authored-By: Claude Fable 5.1 --- scripts/check-ally-review-consistency.mjs | 21 ++++++-- .../check-ally-review-consistency.test.mjs | 48 +++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index b99beb3fdd2a..3ca023a7b56d 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -121,10 +121,18 @@ const BLOCKING_SECTION_RE = */ const NOT_INDENTED_CODE = String.raw`(?! *\t)(?! {4})`; -/** A prior-finding disposition that says the blocker is still present. */ -const STILL_PRESENT_DISPOSITION_RE = new RegExp( - String.raw`^${NOT_INDENTED_CODE} {0,3}-[ \t]*\*\*prior:[^\n]*\*\*[ \t]*(?:—|-)[ \t]*still-present[ \t]*(?:—|-)`, - "im", +/** + * A prior-finding ledger entry. Composed character-for-character with + * PRIOR_FINDING_DISPOSITION_PATTERN in server/src/services/ally-review-detection.ts + * (mirrored verbatim in .github/scripts/sweep-stalled-ally-reviews.py), so the + * three readers accept exactly the same entries. Group 4 is the disposition + * verb; whether it blocks is decided against BLOCKING_PRIOR_DISPOSITIONS rather + * than embedded in the pattern. The cross-reader corpus in the test file drives + * the same ledger strings through all three sources. + */ +const PRIOR_FINDING_DISPOSITION_RE = 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", ); /** @@ -521,7 +529,10 @@ export function hasBlockingFindings(body) { } export function hasStillPresentDisposition(body) { - return STILL_PRESENT_DISPOSITION_RE.test(String(body ?? "")); + for (const match of String(body ?? "").matchAll(PRIOR_FINDING_DISPOSITION_RE)) { + if (BLOCKING_PRIOR_DISPOSITIONS.has(match[4].toLowerCase())) return true; + } + return false; } export function attestedHead(body) { diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index 4966adfa9f73..679d0515cd17 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -172,6 +172,54 @@ describe("hasStillPresentDisposition", () => { false, ); }); + + // The gate (ally-review-detection.ts) and the sweep (sweep-stalled-ally-reviews.py) + // read the prose ledger with PRIOR_FINDING_DISPOSITION_PATTERN; this auditor + // must accept exactly the entries they accept, or the gate goes red on + // `unreadable_verdict` while the auditor reads a cleanly-attesting review. + // Drive the same ledger strings through all three sources, taken from the + // committed files rather than retyped. + it("accepts exactly the ledger entries the gate and the sweep accept", () => { + const notIndentedCode = String.raw`(?! *\t)(?! {4})`; + const tsSource = readFileSync( + new URL("../server/src/services/ally-review-detection.ts", import.meta.url), + "utf8", + ); + const tsRaw = tsSource.match( + /PRIOR_FINDING_DISPOSITION_PATTERN = new RegExp\(\n\s*String\.raw`([^`]+)`,\n\s*"gim",/, + ); + assert.ok(tsRaw, "ally-review-detection.ts still defines PRIOR_FINDING_DISPOSITION_PATTERN"); + const gatePattern = new RegExp(tsRaw[1].replace("${NOT_INDENTED_CODE}", notIndentedCode), "gim"); + + const pySource = readFileSync( + new URL("../.github/scripts/sweep-stalled-ally-reviews.py", import.meta.url), + "utf8", + ); + const pyRaw = pySource.match( + /PRIOR_FINDING_DISPOSITION_PATTERN = re\.compile\(\n\s*r"([^"]+)"\n\s*r"([^"]+)",/, + ); + assert.ok(pyRaw, "sweep-stalled-ally-reviews.py still defines PRIOR_FINDING_DISPOSITION_PATTERN"); + const sweepPattern = new RegExp(pyRaw[1] + pyRaw[2], "gim"); + + const blocksUnder = (pattern, verbGroup, text) => + [...text.matchAll(pattern)].some((m) => m[verbGroup].toLowerCase() === "still-present"); + + const corpus = [ + ["canonical", "- **prior:354d5b9 important 1** — still-present — not mirrored", true], + ["en dash separator", "- **prior:354d5b9 important 1** – still-present – not mirrored", true], + ["space after the emphasis", "- ** prior:354d5b9 important 1** — still-present — not mirrored", true], + ["trailing parenthetical after the index", "- **prior:354d5b9 important 1 (see below)** — still-present — not mirrored", false], + ["fixed verb", "- **prior:354d5b9 important 1** — fixed — closed", false], + ["verb in prose only", "still-present in quoted prose\n- prior:354d5b9 important 1 still-present", false], + ]; + for (const [name, text, expected] of corpus) { + const gate = blocksUnder(gatePattern, 4, text); + const sweep = blocksUnder(sweepPattern, 1, text); + assert.equal(gate, expected, `gate reader: ${name}`); + assert.equal(sweep, expected, `sweep reader: ${name}`); + assert.equal(hasStillPresentDisposition(text), expected, `auditor: ${name}`); + } + }); }); describe("attestedHead", () => { From cfb8f07e39120149c00af9c2990e4636b68957d6 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Tue, 22 Sep 2026 09:21:11 +0000 Subject: [PATCH 11/13] test(scripts): read the gate's not-indented-code pattern from its source (BLO-32695) The consistency test retyped NOT_INDENTED_CODE from ally-review-detection.ts by hand, so a change to the gate's pattern would leave the test agreeing with a stale copy instead of the gate. Extract the literal from the TypeScript source at test time and fail loudly when the constant is no longer found. Negative control: mutating the source constant to `(?!-)` now fails the ledger-entries case; the retyped copy passed unchanged. Co-Authored-By: Claude Fable 5.1 --- scripts/check-ally-review-consistency.test.mjs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index 679d0515cd17..c99b391f5cda 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -180,16 +180,20 @@ describe("hasStillPresentDisposition", () => { // Drive the same ledger strings through all three sources, taken from the // committed files rather than retyped. it("accepts exactly the ledger entries the gate and the sweep accept", () => { - const notIndentedCode = String.raw`(?! *\t)(?! {4})`; const tsSource = readFileSync( new URL("../server/src/services/ally-review-detection.ts", import.meta.url), "utf8", ); + // The interpolated sub-pattern comes out of the same source, not a retyped + // copy: a retyped copy would keep this test green after an edit to the + // constant every line-anchored gate pattern shares. + const tsNotIndented = tsSource.match(/NOT_INDENTED_CODE = String\.raw`([^`]+)`/); + assert.ok(tsNotIndented, "ally-review-detection.ts still defines NOT_INDENTED_CODE"); const tsRaw = tsSource.match( /PRIOR_FINDING_DISPOSITION_PATTERN = new RegExp\(\n\s*String\.raw`([^`]+)`,\n\s*"gim",/, ); assert.ok(tsRaw, "ally-review-detection.ts still defines PRIOR_FINDING_DISPOSITION_PATTERN"); - const gatePattern = new RegExp(tsRaw[1].replace("${NOT_INDENTED_CODE}", notIndentedCode), "gim"); + const gatePattern = new RegExp(tsRaw[1].replace("${NOT_INDENTED_CODE}", tsNotIndented[1]), "gim"); const pySource = readFileSync( new URL("../.github/scripts/sweep-stalled-ally-reviews.py", import.meta.url), From 2b6860bf4a436d21a84af862cf89d251268979c7 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Thu, 24 Sep 2026 07:12:48 +0000 Subject: [PATCH 12/13] test(ally): add the 3-space indent row that discriminates NOT_INDENTED_CODE (BLO-32695) Every corpus row began at column 0, so moving the indent boundary in NOT_INDENTED_CODE from (?! {4}) to (?! {3}) left the suite at 155/155. A 3-space-indented ledger entry is the one input where the two differ. With the row: 155/155 unmutated; with (?! {3}) in ally-review-detection.ts the suite fails "gate reader: 3-space indent". Co-Authored-By: Claude Opus 5.5 (1M context) --- scripts/check-ally-review-consistency.test.mjs | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index c99b391f5cda..d598d4ffcdc5 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -212,6 +212,7 @@ describe("hasStillPresentDisposition", () => { ["canonical", "- **prior:354d5b9 important 1** — still-present — not mirrored", true], ["en dash separator", "- **prior:354d5b9 important 1** – still-present – not mirrored", true], ["space after the emphasis", "- ** prior:354d5b9 important 1** — still-present — not mirrored", true], + ["3-space indent", " - **prior:354d5b9 important 1** — still-present — not mirrored", true], ["trailing parenthetical after the index", "- **prior:354d5b9 important 1 (see below)** — still-present — not mirrored", false], ["fixed verb", "- **prior:354d5b9 important 1** — fixed — closed", false], ["verb in prose only", "still-present in quoted prose\n- prior:354d5b9 important 1 still-present", false], From dd6966566da82b94dace7646a0516525606a6924 Mon Sep 17 00:00:00 2001 From: CTO Date: Thu, 24 Sep 2026 10:01:35 +0000 Subject: [PATCH 13/13] test(ally): splice NOT_INDENTED_CODE with a function replacement (BLO-32695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit String.prototype.replace with a *string* replacement interprets $&, $`, $' and $$ in the replacement text. tsNotIndented[1] is source text spliced in unescaped, so a future NOT_INDENTED_CODE containing `$` would be silently mangled instead of failing loudly. Measured, not reasoned: (?!$&) -> rebuilds the literal ${NOT_INDENTED_CODE} into the pattern (?!a$$b) -> collapses to (?!a$b) (?!x$`y) -> collapses to (?!x^y) A function replacement disables $ interpretation entirely. Latent today — the constant is $-free — so this closes the hazard, not a live defect. Verified the guard still binds after the change: suite is 155/155 at this head, and with (?! {4}) -> (?! {3}) in ally-review-detection.ts it fails "gate reader: 3-space indent" (154/1). Co-Authored-By: Claude --- scripts/check-ally-review-consistency.test.mjs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/check-ally-review-consistency.test.mjs b/scripts/check-ally-review-consistency.test.mjs index d598d4ffcdc5..28d12a92518d 100644 --- a/scripts/check-ally-review-consistency.test.mjs +++ b/scripts/check-ally-review-consistency.test.mjs @@ -193,7 +193,14 @@ describe("hasStillPresentDisposition", () => { /PRIOR_FINDING_DISPOSITION_PATTERN = new RegExp\(\n\s*String\.raw`([^`]+)`,\n\s*"gim",/, ); assert.ok(tsRaw, "ally-review-detection.ts still defines PRIOR_FINDING_DISPOSITION_PATTERN"); - const gatePattern = new RegExp(tsRaw[1].replace("${NOT_INDENTED_CODE}", tsNotIndented[1]), "gim"); + // Function replacement, not a string one: a string replacement interprets + // `$&`/`` $` ``/`$'`/`$$` in the spliced-in source text, so a future + // NOT_INDENTED_CODE containing `$` would be silently mangled rather than + // failing loudly. `$&` alone would rebuild the literal `${NOT_INDENTED_CODE}`. + const gatePattern = new RegExp( + tsRaw[1].replace("${NOT_INDENTED_CODE}", () => tsNotIndented[1]), + "gim", + ); const pySource = readFileSync( new URL("../.github/scripts/sweep-stalled-ally-reviews.py", import.meta.url),