From 9fe58f4f24be90ad581e0c2f663166561ac29d22 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 30 Aug 2026 15:13:57 -0700 Subject: [PATCH 1/3] Compare Vetted Reviewer Markers Without Regard to Letter Case A review body carrying `### Reviewed Changes` reported as a shape this script had never seen, while `### Reviewed changes` sat in `VETTED_HEADINGS`. Case carries no meaning in any of these markers, so a one-letter drift blocked a review loop on a round every reader had parsed correctly, `coverage=full` among them, since `SUPPRESSED`, `CR_OUTSIDE_DIFF` and `REFUSAL` are all case-insensitive already and only the membership test was not. A vetting list whose entries stop matching for a reason that means nothing is the silent narrowing the verification rules name. It arrived loudly here, which is better and still wrong, and the cost of that shape is an override asked of the maintainer that nothing warranted. `unvetted()` folds at the membership test for all three lists, with a case per list so a one-arm revert cannot stay green. `normal()` is unchanged, because the report strings carry its value and a reported shape's remedy names the shape beside the body it quotes, so a folded name would not match the body printed next to it. Its docstring, the comment above the three lists, and the `scripts/README.md` paragraph each stated the comparison as a closed set that no longer held. That paragraph also carried a heading count correct when it was written and stale from the eighth entry onward, which is the count a maintainer reads when deciding whether a reported marker is a drift or a missing entry. Closes #1132 --- scripts/README.md | 2 +- scripts/pr_review.py | 33 ++++++++++++++++++++++++----- scripts/tests/test_pr_review.py | 37 +++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 6 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 15f4a5f8..32080066 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -183,7 +183,7 @@ A partial round is reported and handed over rather than retried into. Measured o `status` and `wait` both exit `43` where the reviewer sent a **shape this script has no reader for**, and that outcome outranks every other reading here, because a reader that does not understand the output cannot be believed about what it found in it. Every rule in this script keys on a structural marker, so a marker that changes spelling is a section the reader stops finding and reports as absent. All three failures on record have that exact shape: the suppressed heading reworded and the count went to zero, the suppressed section moved inside another wrapper and the count went to zero again, and the coverage line was never parsed at all. Each reported a clean pass over a review it had misread, and each was found by the maintainer afterwards rather than by the gate. The digest carries `shapes=UNRECOGNIZED`, lists each shape it could not place, and names the remedy in two parts: **file an issue on the hub repository that hosts this script**, quoting the body the shape came from, and leave the merge decision to the maintainer, since an unrecognized shape does not say the pull request is bad, only that nothing here can vouch for the review of it. -The vetted inventory is measured rather than imagined, and it is small because the output is regular. Across those 332 bodies, with fenced blocks dropped and text reduced to ASCII, the whole corpus is seven headings, six `` texts and three metadata labels. Counts are normalized to `(N)` and the verdict headings' colored circle is dropped before comparing, since both change on every review without the section having changed, and dropping the emoji is also what keeps this repository's charset rule satisfied. A body carrying **no** heading at all is itself unrecognized, which is what catches a rewrite that changes every marker at once, and a **refusal is exempt** because it is a bare paragraph by design and `REFUSAL` is its vetted spelling. That exemption is the pattern rather than a carve-out, so a refusal reworded stops being exempt and blocks, which is the refusal check's own failure mode caught one rewording later. The last reading is the quietest: a reviewer **login** that reads as this reviewer without being the spelling every query filters on, since a rename leaves every filter matching nothing and the digest then reports a review that landed as no review at all. A case runs the whole inventory over the measured corpus, where it raises nothing. +The vetted inventory is measured rather than imagined, and it is small because the output is regular. Across those 332 bodies, with fenced blocks dropped and text reduced to ASCII, the whole corpus is nine headings, six `` texts and three metadata labels. Counts are normalized to `(N)` and the verdict headings' colored circle is dropped before comparing, since both change on every review without the section having changed, and dropping the emoji is also what keeps this repository's charset rule satisfied. Letter case is folded at the comparison rather than in the reduction, so a marker keeps the case the reviewer wrote while a drift of case alone stops blocking a loop, which it did once, on a heading already in the inventory. A body carrying **no** heading at all is itself unrecognized, which is what catches a rewrite that changes every marker at once, and a **refusal is exempt** because it is a bare paragraph by design and `REFUSAL` is its vetted spelling. That exemption is the pattern rather than a carve-out, so a refusal reworded stops being exempt and blocks, which is the refusal check's own failure mode caught one rewording later. The last reading is the quietest: a reviewer **login** that reads as this reviewer without being the spelling every query filters on, since a rename leaves every filter matching nothing and the digest then reports a review that landed as no review at all. A case runs the whole inventory over the measured corpus, where it raises nothing. `wait` exits `44` when the review loop has closed but a required check sits in a shape no wait clears. The digest carries `checks=N/M` beside the merge word and names the shape as `stuck=`, because `mergeStateStatus` reports one word, `BLOCKED`, for a red check, a check nothing is running, an unresolved thread, and a missing approval alike, and a run in this repository spent twenty-five minutes polling that word on a pull request whose only unfinished check was an aggregator job no runner ever took. The cause came from the maintainer rather than from any field here, which is the whole defect: the digest named the state it could not explain and stopped there. Four shapes are told apart because each wants a different response. `NOT_POSTED` is a required status whose poster has not spoken, which is a `StatusContext`'s `EXPECTED` and only ever that. It is deliberately not folded in with the starved shape, since no runner is owed a status nothing has posted, so re-running a workflow clears nothing and the starved wording would send a reader at the runner pool over a missing poster. `NOT_PICKED_UP` is a check GitHub dispatched and assigned no runner, read from the queued state rather than from a runner name GraphQL does not carry, and the state suffices because a job held behind a `needs:` dependency does not enter the rollup until that dependency finishes, so there is no dependency-blocked queue to mistake for a starved one. Nothing agent-side starts it, since the pool is GitHub-hosted, so the remedy is a re-run or that capacity. `RUNNING_LONG` is deliberately the weaker reading and its wording says so, because duration alone cannot separate a hung job from a slow one: this repository's lint job legitimately runs nine to eleven minutes while its aggregator is a single shell conditional, so the threshold is generous, the elapsed time prints for the reader to judge against what the job costs, and nothing asserts a fault. `FAILED` is a verdict rather than a stuck check, reported so no reader deduces a red check from `BLOCKED`. A check merely still running normally is **not** any of these and exits `0`. That boundary is the whole design, because `wait` returns the moment coverage lands and on almost every pull request the checks are still going at that instant, so taking `44` for a pending check would make `44` the ordinary outcome and a code that fires always carries nothing. `44` additionally requires `mergeStateStatus: BLOCKED`, which the module docstring's own list of shapes has to name rather than only implying, because a reader who sees `stuck=FAILED` and exit `0` on a merge that is `UNSTABLE` should find the condition written down rather than infer the field is unreliable. It is because a rollup carries checks the ruleset does not require and four of the six on a green pull request here are exactly that, so the code borrows GitHub's own reading of which checks gate a merge rather than reading the ruleset's contexts over another call. `CLEAN` proves no required gate is outstanding whatever else the rollup is doing, and without that condition a stuck check nothing requires returns `44` on a mergeable pull request. The digest names the check either way, so the narrower code costs the reader nothing. Both of those came out of this change's own review. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 489abdbb..afbfee45 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -297,7 +297,7 @@ # A heading this script has no spelling for is a section it will not find, reported as absent. # That is the shape of all three failures already on record here, each caught after it landed. # The lists are small because the output is regular: 9 headings, 6 summaries and 3 labels. -# Counts are normalized to `(N)` and non-ASCII is dropped before comparing. +# Counts are normalized to `(N)` and non-ASCII is dropped before comparing, and `unvetted` folds letter case at the comparison itself. # The verdict headings carry a colored circle, so the emoji is what would drift most cheaply. # Dropping it also keeps this file inside the charset rule that governs the repository. VETTED_HEADINGS = { @@ -1083,7 +1083,10 @@ def table_against_diff(pr: dict, counts: tuple[int, int] | None) -> str: def normal(text: str) -> str: - """A marker reduced to what a vetted list compares: ASCII, single spaces, counts as `(N)`. + """A marker reduced toward what a vetted list compares: ASCII, single spaces, counts as `(N)`. + + Letter case is not reduced here and is folded at the membership test in `unvetted` instead, + so this value keeps the case the reviewer wrote while the comparison ignores it. The verdict headings carry a colored circle and the suppressed heading carries its finding count, so both drift on every review without the section having changed at all. @@ -1092,6 +1095,24 @@ def normal(text: str) -> str: return re.sub(r"\s+", " ", re.sub(r"\(\d+\)", "(N)", ascii_only)).strip() +def unvetted(marker: str, vetted: set[str]) -> bool: + """Whether `marker` is absent from `vetted`, comparing without regard to letter case. + + Case carries no meaning in any of these markers, and the reviewer has drifted one: a review + body carrying `### Reviewed Changes` reported as a shape this script had never seen while + `### Reviewed changes` sat in the vetted list, which blocked a review loop over a letter. A + vetting list whose entries stop matching for a reason that means nothing is the silent + narrowing the verification rules name, arriving loudly rather than quietly and still wrong. + + Folded here rather than in `normal`, whose value the report strings also carry. A reported + shape's remedy names the shape beside the body it quotes, so a folded name would not match the + body printed next to it. Case is the one reduction with no reason to apply: the emoji and the + count are dropped because they drift on every review without the section having changed, and + case has drifted the same way once. + """ + return marker.casefold() not in {v.casefold() for v in vetted} + + def unrecognized_in(body: str) -> list[str]: """Every marker in one review body this script has no vetted spelling for. @@ -1112,13 +1133,15 @@ def unrecognized_in(body: str) -> list[str]: plain = FENCE.sub("", body or "") headings = [normal(ln) for ln in plain.splitlines() if MARKDOWN_HEADING.match(ln)] labels = [normal(m.group(1)) for m in map(LABEL_LINE.match, plain.splitlines()) if m] - found = [f"heading: {h}" for h in dict.fromkeys(headings) if h not in VETTED_HEADINGS] + found = [f"heading: {h}" for h in dict.fromkeys(headings) if unvetted(h, VETTED_HEADINGS)] found += [ f"summary: {normal(s)}" for s in dict.fromkeys(SUMMARY.findall(plain)) - if normal(s) not in VETTED_SUMMARIES + if unvetted(normal(s), VETTED_SUMMARIES) + ] + found += [ + f"metadata label: {la}" for la in dict.fromkeys(labels) if unvetted(la, VETTED_LABELS) ] - found += [f"metadata label: {la}" for la in dict.fromkeys(labels) if la not in VETTED_LABELS] found += [ f"coverage line: {ln}" for ln in coverage_statements(body) if read_coverage(ln) is None ] diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 6df4ed4c..ee1e48ed 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -1594,6 +1594,43 @@ def digest_for(self, *reviews: dict) -> str: out, _ = pr_review.digest("o", "r", 7) return out + def test_a_vetted_marker_survives_a_change_of_letter_case(self) -> None: + """Case carries no meaning in these markers, and the reviewer has drifted one. + + A body carrying `### Reviewed Changes` reported as a shape never seen while + `### Reviewed changes` sat in the vetted list, which blocked a review loop over one letter + on a round every reader had parsed correctly. + """ + for vetted in ("### Reviewed changes", "## Pull request overview"): + drifted = vetted.title() + self.assertNotEqual(drifted, vetted, "this case pair no longer differs") + self.assertEqual( + [], pr_review.unrecognized_in(OVERVIEW + "\n" + drifted), f"blocked on {drifted}" + ) + # One case per vetted list, since each arm folds separately. + # A one-arm revert would otherwise stay green while that arm blocks a loop on the next drift. + # Not the files-reviewed label, which is also read as a coverage line and judged separately. + self.assertEqual([], pr_review.unrecognized_in(OVERVIEW + "\n- **comments generated:** 2")) + self.assertEqual( + [], + pr_review.unrecognized_in(OVERVIEW + "\n
Review Details"), + ) + + def test_a_genuinely_unknown_marker_is_still_reported(self) -> None: + """The floor under the case above, since folding could have turned the check off. + + A vetting list that stops reporting is indistinguishable from a body with nothing to + report, which is the failure the list exists to make loud. + """ + for shape, unknown in ( + ("heading", "### Reviewed Chances"), + ("metadata label", "- **Review depth level:** deep"), + ): + found = pr_review.unrecognized_in(OVERVIEW + "\n" + unknown) + self.assertTrue( + any(f.startswith(shape) for f in found), f"{unknown!r} passed as vetted: {found}" + ) + def test_every_vetted_marker_together_reads_as_recognized(self) -> None: """The corpus shape in one body, so the lists are held against what they were built from.""" self.assertEqual([], pr_review.unrecognized_in(nested())) From 109a9970f99b849e0b7849c86f394af0b7cb12ed Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 30 Aug 2026 15:49:31 -0700 Subject: [PATCH 2/3] Cut the unvetted Docstring to Its Contract The docstring narrated the incident, cited the silent-narrowing rule, and explained where the fold sits, which is a historic narrative and a rule citation in a place the fleet's comment rules keep for the behavior contract. It is now one line, and the two-line why that survives sits inside the function beside the comparison it explains, matching where `unrecognized_in` keeps its own. The `normal` addition loses the same restatement. That why names no count. "The three report strings" would have been the one stale-able claim left in it, in the same commit that fixes a heading count stale from the eighth entry onward. The reviewer also read three prose lines in the new test comment, which is one past what a constraint earns. --- scripts/pr_review.py | 20 ++++---------------- scripts/tests/test_pr_review.py | 3 +-- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index afbfee45..ea5385b9 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1085,8 +1085,7 @@ def table_against_diff(pr: dict, counts: tuple[int, int] | None) -> str: def normal(text: str) -> str: """A marker reduced toward what a vetted list compares: ASCII, single spaces, counts as `(N)`. - Letter case is not reduced here and is folded at the membership test in `unvetted` instead, - so this value keeps the case the reviewer wrote while the comparison ignores it. + Letter case survives, and `unvetted` folds it at the membership test instead. The verdict headings carry a colored circle and the suppressed heading carries its finding count, so both drift on every review without the section having changed at all. @@ -1096,20 +1095,9 @@ def normal(text: str) -> str: def unvetted(marker: str, vetted: set[str]) -> bool: - """Whether `marker` is absent from `vetted`, comparing without regard to letter case. - - Case carries no meaning in any of these markers, and the reviewer has drifted one: a review - body carrying `### Reviewed Changes` reported as a shape this script had never seen while - `### Reviewed changes` sat in the vetted list, which blocked a review loop over a letter. A - vetting list whose entries stop matching for a reason that means nothing is the silent - narrowing the verification rules name, arriving loudly rather than quietly and still wrong. - - Folded here rather than in `normal`, whose value the report strings also carry. A reported - shape's remedy names the shape beside the body it quotes, so a folded name would not match the - body printed next to it. Case is the one reduction with no reason to apply: the emoji and the - count are dropped because they drift on every review without the section having changed, and - case has drifted the same way once. - """ + """Whether `marker` is absent from `vetted`, compared without regard to letter case.""" + # Folded here rather than in `normal`, whose value the report strings also carry. + # A report naming a folded shape would not match the body it asks the reader to quote beside it. return marker.casefold() not in {v.casefold() for v in vetted} diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index ee1e48ed..ba30021b 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -1607,8 +1607,7 @@ def test_a_vetted_marker_survives_a_change_of_letter_case(self) -> None: self.assertEqual( [], pr_review.unrecognized_in(OVERVIEW + "\n" + drifted), f"blocked on {drifted}" ) - # One case per vetted list, since each arm folds separately. - # A one-arm revert would otherwise stay green while that arm blocks a loop on the next drift. + # One case per vetted list, since a one-arm revert would otherwise stay green. # Not the files-reviewed label, which is also read as a coverage line and judged separately. self.assertEqual([], pr_review.unrecognized_in(OVERVIEW + "\n- **comments generated:** 2")) self.assertEqual( From 1e44b26c8824d902bf9e3591aab115105058a825 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 30 Aug 2026 16:23:17 -0700 Subject: [PATCH 3/3] Normalize a Summary Marker Once Rather Than Twice The summaries arm computed `normal(s)` for the membership test and again for the string it renders. Hoisting it into a generator leaves the deduplication on the raw text, where it was, so a marker pair that differs only in what `normal` strips still reads as two rather than collapsing to one. --- scripts/pr_review.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index ea5385b9..e8b7d6b8 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1123,9 +1123,10 @@ def unrecognized_in(body: str) -> list[str]: labels = [normal(m.group(1)) for m in map(LABEL_LINE.match, plain.splitlines()) if m] found = [f"heading: {h}" for h in dict.fromkeys(headings) if unvetted(h, VETTED_HEADINGS)] found += [ - f"summary: {normal(s)}" - for s in dict.fromkeys(SUMMARY.findall(plain)) - if unvetted(normal(s), VETTED_SUMMARIES) + f"summary: {marker}" + # Deduplicated on the raw text before normalizing, which is what it did before the hoist. + for marker in (normal(s) for s in dict.fromkeys(SUMMARY.findall(plain))) + if unvetted(marker, VETTED_SUMMARIES) ] found += [ f"metadata label: {la}" for la in dict.fromkeys(labels) if unvetted(la, VETTED_LABELS)