diff --git a/.github/scripts/sweep-stalled-ally-reviews.py b/.github/scripts/sweep-stalled-ally-reviews.py index 1749bae9ace1..1b68a7912536 100755 --- a/.github/scripts/sweep-stalled-ally-reviews.py +++ b/.github/scripts/sweep-stalled-ally-reviews.py @@ -71,25 +71,428 @@ 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, +) + + +# 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( + # `[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 | ASCII_RE, +) +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_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. + + 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 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. + + 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_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 + # 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 + ) + ) + + +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 + ) + + +class TestPatternCharacterClassesAreAsciiOnly(unittest.TestCase): + """Peer review of #1721 at d05a49f3 -- `\\b` and IGNORECASE, the two members + of the class above that the `[0-9]` fix did not reach. + + The gate builds its regexes with "gm"/"gim"/"gi" and never the `u` flag, so + every character class in them is ASCII-only. Python's are not. Measured + exhaustively over U+0000..U+10FFFF 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. + + Both harms run in the *silent* direction, which is why they are pinned + rather than documented: the sweep records the head as reviewed, so the one + automatic route back from a red gate never fires. + """ + + HEAD = "c" * 40 + + def test_a_non_ascii_word_char_after_the_prefix_still_counts_an_opener(self): + # `\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() 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 }); } @@ -162,6 +172,58 @@ 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 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}", tsNotIndented[1]), "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", () => { @@ -173,6 +235,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); }); @@ -180,6 +275,150 @@ 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); + }); + + // 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); + }); + + 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); + } + }); + + // 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", () => { @@ -320,6 +559,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 +1429,71 @@ 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("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 + // 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..615cc68ab10e 100644 --- a/server/src/__tests__/pr-comment-review-gate.test.ts +++ b/server/src/__tests__/pr-comment-review-gate.test.ts @@ -10,6 +10,8 @@ import { extractAllyReviewedHeadSha, hasActionablePrReviewFeedback, hasAllyConsolidatedReviewHeading, + isPublishableToken, + parseAllyVerdictBlock, } from "../services/ally-review-detection.js"; import { commentReviewGateCheckConclusion, @@ -461,6 +463,154 @@ 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 + // 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("the publisher's alphabet is the parser's alphabet", () => { + // `[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 + // 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( + 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) => 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", () => { const verdict = evaluateCommentReviewGate({ headSha: CURRENT_HEAD, @@ -1278,6 +1428,160 @@ 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); + }); + + // 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. + 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 @@ -1365,3 +1669,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..1b226a5221ab 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,536 @@ 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) { + // 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; + 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 }; + + const ledgerDisagreement = proseDispositionContradicting(text, ledger); + if (ledgerDisagreement !== null) return { kind: "unreadable", reason: ledgerDisagreement }; + + 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) { + // `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; +} + +/** + * 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. @@ -209,6 +740,63 @@ function hasNonNegatedMatch(text: string, pattern: RegExp): boolean { return false; } +// 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. 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 +// 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 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 model-authored token be quoted into a public commit-status + * description? + * + * 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 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. // - **prior:731ced5 critical 1** — fixed — the terminator is gone. // Anchored to the bold list-item form Ally emits, matching the shape @@ -224,6 +812,22 @@ function hasNonNegatedMatch(text: string, pattern: RegExp): 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 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]*([a-z][a-z-]*)[ \t]*(?:—|–|-)`, "gim", @@ -254,6 +858,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 +998,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 +1069,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 +1098,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 +1238,73 @@ 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 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 + // 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..8ce997bd6c20 100644 --- a/server/src/services/pr-comment-review-gate.ts +++ b/server/src/services/pr-comment-review-gate.ts @@ -18,8 +18,11 @@ import { extractAllyPriorFindingDispositions, extractAllyReportedFindingRefs, extractAllyReviewedHeadSha, + allyClaimedReviewHead, hasActionablePrReviewFeedback, hasAllyConsolidatedReviewHeading, + asPublishableToken, + parseAllyVerdictBlock, type AllyFindingRef, type AllyPriorFindingDisposition, } from "./ally-review-detection.js"; @@ -72,6 +75,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 +89,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 +102,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 +211,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 +221,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 +267,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 @@ -275,29 +394,109 @@ 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 asPublishableToken. + verbs.add(asPublishableToken(prior.entry.disposition)); + } } } 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 +518,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 +603,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 +700,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"; }