From 2f0543d14a14308b76d1d711e09d2a416edbcfad Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 16 Oct 2026 19:20:52 -0700 Subject: [PATCH 1/6] Read Copilot's Count-First Overview Sections in pr_review.py A later revision of Copilot's second overview format opens each section summary on its count and renames findings to issues. pr_review.py stopped on each of these shapes with exit 43: - `2 previously missed issues` is the previously-missed section, whose findings open no thread. It is now vetted, matched by PREVIOUSLY_MISSED, and counted by finding_count from its leading count rather than falling to the floor of one. - `1 open issue` and `N open issues` are the open-findings section under a new noun. They map to the same role, so the section states the round's total and each entry is checked against the threads. - `**No open issues**` followed by an emoji is the bare stated zero, which printed `?` before. The vetted-list size comment is removed with the test that kept it in sync, since its count went stale on every new spelling. Refs #3167 Refs #3170 --- scripts/pr_review.py | 25 ++++++-- tests/test_pr_review.py | 122 ++++++++++++++++++++++++++++++++++------ 2 files changed, 123 insertions(+), 24 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index a2d5ae1a..731d3ec3 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -448,7 +448,7 @@ # What this still admits is non-ASCII punctuation, an em dash or a typographic quote opening a sentence, which reads that sentence as the section. # It is narrower than the spelling before it, which admitted ASCII punctuation as well, and the remedy for the rest is a class of non-ASCII symbols rather than the whole of non-ASCII. PREVIOUSLY_MISSED = re.compile( - r"^(?:]*>|[ \t>#]|[*_](?![ \t])|[^\x00-\x7f])*Previously missed\b", + r"^(?:]*>|[ \t>#]|[*_](?![ \t])|[^\x00-\x7f])*(?:\d{1,4}[ \t]+)?Previously missed\b", re.IGNORECASE, ) # CodeRabbit's own equivalent, collapsed into the review body like `SUPPRESSED` rather than raised as an inline comment. @@ -664,7 +664,6 @@ def strip_fences( # A body is read for these rather than trusted, because every reader below keys on one of them. # 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: 10 headings, 13 summaries and 4 labels. # Two overview formats are carried rather than one, the second arriving after that measurement. # Its own markers are listed beside the first's, since a round in either format can land next. # Counts are normalized to `(N)` and non-ASCII is dropped before comparing, and `unvetted` folds letter case at the comparison itself. @@ -693,6 +692,8 @@ def strip_fences( "Open (N)": OPEN, "(N) open findings": TOTAL, "(N) open finding": TOTAL, + "(N) open issues": TOTAL, + "(N) open issue": TOTAL, "Resolved since last review (N)": RESOLVED, "(N) resolved since last review": RESOLVED, } @@ -705,6 +706,8 @@ def strip_fences( "Suppressed comments (N)", "Comments suppressed due to low confidence (N)", "Previously missed (N)", + "(N) previously missed issues", + "(N) previously missed issue", } VETTED_LABELS = {"Files reviewed", "Comments generated", "Review effort level", "Review effort"} # Inline emphasis around a marker is presentation rather than identity. @@ -746,7 +749,8 @@ def strip_fences( # The format states a count per severity, so a round stating `2 . 3 ` raised five. CCR_FINDINGS = re.compile(r"^ {0,3}\*\*Findings:\*\*(.*)$", re.MULTILINE) CCR_OPEN_FINDINGS = re.compile( - r"^ {0,3}\*\*(\d+) open findings?\*\*[ \t]*\r?$", re.MULTILINE | re.IGNORECASE + r"^ {0,3}\*\*(\d+|No) open (?:findings?|issues?)\*\*[ \t]*(?:[^\x00-\x7f\s]+[ \t]*)?\r?$", + re.MULTILINE | re.IGNORECASE, ) # The markup the format writes a count beside, replaced by one sentinel before a count is read. # Masked rather than matched around, because a badge carries digits of its own in its attributes. @@ -817,6 +821,7 @@ def strip_fences( SUMMARY = re.compile(r"]*)?>(.*?)", re.DOTALL | re.IGNORECASE) TAGS = re.compile(r"", re.IGNORECASE) COUNT = re.compile(r"\((\d+)\)") +LEADING_COUNT = re.compile(r"(?:[ \t#*_]|[^\x00-\x7f])*(\d+)[ \t]") # What makes a line a heading rather than prose mentioning the phrase, in either markup. # A line that is neither still qualifies where it carries a count. # The section has appeared as a bare line too, and a count is what prose does not carry. @@ -2418,7 +2423,8 @@ def stated_total(body: str, preamble_only: bool = False) -> int | None: A later revision of the format drops the `**Findings:**` line and states its total as a bare `**0 open findings**` line, read here the same way, since left unread it printed `?` and - suppressed the shortfall the total is compared for. + suppressed the shortfall the total is compared for. A later one still writes that zero as + `**No open issues**` followed by an emoji, which is the same stated zero. That revision writes the bare line only for a zero, measured over every body carrying it, and states any other total as the count opening its open-findings section, so where the preamble @@ -2430,7 +2436,10 @@ def stated_total(body: str, preamble_only: bool = False) -> int | None: opener = DETAILS_OPEN.search(CODE_SPAN.sub(MARKUP_MASK, stripped)) preamble = plain[: opener.start()] if opener else plain totals = [findings_on(m.group(1)) for m in CCR_FINDINGS.finditer(preamble)] - totals += [counted(m.group(1)) for m in CCR_OPEN_FINDINGS.finditer(preamble)] + totals += [ + 0 if m.group(1).casefold() == "no" else counted(m.group(1)) + for m in CCR_OPEN_FINDINGS.finditer(preamble) + ] found = [t for t in totals if t is not None] if not found and not preamble_only: found = [c for role, c, *_ in read_overview(body)[2] if role == TOTAL and c is not None] @@ -4510,10 +4519,14 @@ def outside_diff_blocks(body: str) -> list[str]: def finding_count(block: str) -> int: """The heading's `(N)`, floored at one, since a block reported as zero reads as a clean pass. + A heading stating no `(N)` is read for a count opening it, `2 previously missed issues`, which + left unread fell to the floor and reported one finding where the round raised two. + A run longer than a count carries falls to that same floor rather than crashing the digest, the block existing being what says there is at least one finding in it. """ - m = COUNT.search(heading_of(block)) + heading = heading_of(block) + m = COUNT.search(heading) or LEADING_COUNT.match(EMPHASIS.sub("", heading)) stated = counted(m.group(1)) if m else None return max(stated, 1) if stated is not None else 1 diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index ed1061fd..eae7afc2 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1366,6 +1366,10 @@ def test_prose_naming_the_section_is_not_the_section(self) -> None: line = f"{opener}Previously missed (2) findings were re-raised this round." self.assertIsNone(pr_review.PREVIOUSLY_MISSED.match(line)) self.assertEqual([], pr_review.previously_missed_blocks(f"{OVERVIEW}\n{line}\n")) + counted = f"{opener}2 previously missed issues were re-raised this round." + self.assertIsNone(pr_review.PREVIOUSLY_MISSED.match(counted)) + bare = "2 previously missed issues were re-raised this round." + self.assertEqual([], pr_review.previously_missed_blocks(f"{OVERVIEW}\n{bare}\n")) def test_the_heading_is_read_through_whatever_markup_it_wears(self) -> None: """What the anchor must admit, beside the prose above that it must not. @@ -1386,10 +1390,48 @@ def test_the_heading_is_read_through_whatever_markup_it_wears(self) -> None: # An emphasis marker is heading markup and a bullet is prose, and a space is what separates them, so both spellings are held here rather than only the one. ("italic, with no tags", "_Previously missed (1)_"), ("bold and italic", "***Previously missed (1)***"), + ( + "the count-first spelling", + "2 previously missed issues", + ), + ("a count-first markdown heading", "### 2 previously missed issues"), ): with self.subTest(case=label): self.assertIsNotNone(pr_review.PREVIOUSLY_MISSED.match(heading)) + def test_a_count_first_heading_is_the_section_and_states_its_count(self) -> None: + """A later revision opens the heading on its count, `2 previously missed issues`. + + Unread, the round exited 43 on an unvetted summary, and its findings, which open no thread, + reached the agent only through the raw body. Read as the section without its count, the + block fell to the floor of one and reported one finding where the round raised two. + """ + body = counted_missed(2) + self.assertEqual([], pr_review.unrecognized_in(body)) + blocks = pr_review.previously_missed_blocks(body) + self.assertEqual(1, len(blocks)) + self.assertEqual(2, pr_review.finding_count(blocks[0])) + self.assertEqual( + 1, pr_review.finding_count(pr_review.previously_missed_blocks(counted_missed(1))[0]) + ) + self.answer(payload([review(body=body)])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("previously_missed=2 (on_head=2 earlier=0)", out) + self.assertIn("shapes=ok", out) + self.assertIn("Low severity Constructed missed finding 2", out) + + def test_a_count_first_section_cancels_the_flagging_verdict_reading(self) -> None: + """The round flags a closer look and states no open issue, and its findings are the + previously missed ones, which the digest now counts, so the round is not a verdict naming + nothing.""" + self.answer(payload([review(body=counted_missed(2))])) + self.assertIsNone(pr_review.uncounted_verdict(payload([review(body=counted_missed(2))]))) + with contextlib.redirect_stdout(io.StringIO()) as printed: + code = pr_review.main(["status", "7", "--repo", "o/r"]) + self.assertNotIn("VERDICT WITH NO COUNTED FINDING", printed.getvalue()) + self.assertNotIn("UNRECOGNIZED", printed.getvalue()) + self.assertEqual(0, code) + def test_a_quoted_heading_is_not_a_section_this_round_raised(self) -> None: """Every other reader in this file strips a quotation before looking, and this one did not. @@ -1879,6 +1921,29 @@ def previously_missed( ) +def counted_missed(entries: int) -> str: + """The later revision's previously-missed section, its summary opening on the count. + + The round flags a closer look, states its zero as `**No open issues**`, and nests one badged + entry per finding under the section, each a `FILE:LINE` anchor and a priority-tagged paragraph. + That nesting is the observed one, and every title, path, and sentence is constructed, per + GOVERNANCE.md "Representative Data in Agent-Authored Text". + """ + noun = "issue" if entries == 1 else "issues" + found = "".join( + f"
\n{BADGE} Constructed missed finding {i}\n\n" + f"`a.py:{10 * i}`\n\n[P1] A constructed paragraph about the finding.\n
\n" + for i in range(1, entries + 1) + ) + block = ( + f"
\n{entries} previously missed {noun}\n\n" + f"In code that hasn't changed since last review\n\n{found}
\n\n
" + ) + return revised_with( + "**No open issues** \U0001f389", "### \U0001f535 Needs a closer look" + ).replace("
", block, 1) + + class TestCoverage(GqlCase): """The round that covered the head and read part of the diff, which is a clean pass elsewhere. @@ -4109,6 +4174,22 @@ def test_the_revision_s_open_findings_line_is_its_stated_total(self) -> None: ) self.assertIsNone(pr_review.stated_total(inside)) + def test_the_revision_s_no_open_issues_line_is_a_stated_zero(self) -> None: + """A later revision writes its zero as `**No open issues**` closed by an emoji, and unread + it printed `?` over a round stating nothing open. The line anchor still holds, so prose + naming the words, or a line going on past the emoji, states nothing.""" + for line, total in ( + ("**No open issues** \U0001f389", 0), + ("**No open issues**", 0), + ("**1 open issue**", 1), + ("**2 open issues**", 2), + ): + with self.subTest(line=line): + self.assertEqual(total, pr_review.stated_total(overview_v2_revised(line))) + for line in ("It had **No open issues** before.", "**No open issues** \U0001f389 yet"): + with self.subTest(line=line): + self.assertIsNone(pr_review.stated_total(overview_v2_revised(line))) + def test_a_total_split_across_severities_is_summed(self) -> None: """The format states a count per severity, so the first number alone undercounts. @@ -4651,23 +4732,6 @@ def test_a_phrase_rewrapped_across_comment_lines_reads_as_one_sentence(self) -> comment_flowed_text(wrapped), ) - def test_the_vetted_lists_hold_what_the_comment_beside_them_counts(self) -> None: - """The comment states the sizes, and adding an entry without it is how it goes stale.""" - source = comment_flowed_text(Path(pr_review.__file__).read_text(encoding="utf-8")) - stated = re.search( - r"the output is regular: (\d+) headings, (\d+) summaries and (\d+) labels", source - ) - if stated is None: - self.fail("the comment stating the vetted list sizes is no longer in the source") - self.assertEqual( - ( - len(pr_review.VETTED_HEADINGS), - len(pr_review.VETTED_SUMMARIES), - len(pr_review.VETTED_LABELS), - ), - tuple(int(g) for g in stated.groups()), - ) - COVERS_ONE = "" @@ -4728,6 +4792,24 @@ def test_the_open_findings_section_states_the_round_s_total(self) -> None: body = revised_with(open_section(("6000000001", "6000000002", "6000000003"))) self.assertEqual(3, pr_review.stated_total(body)) + def test_the_open_issues_spelling_is_the_same_section(self) -> None: + """A later revision writes `1 open issue` where it wrote `1 open finding`, and exited 43. + + Its entries link the round's own threads as the earlier spelling's do, so it is read as the + same section: vetted, stating the total, and each entry checked against the threads. + """ + for ids in (("6000000001",), ("6000000001", "6000000002")): + noun = "issue" if len(ids) == 1 else "issues" + with self.subTest(count=len(ids)): + body = revised_with(open_section(ids, noun=noun)) + self.assertEqual([], pr_review.unrecognized_in(body)) + self.assertEqual(len(ids), pr_review.stated_total(body)) + rd = review(body=body, rid="PRR_head") + threads = [thread(f"T{i}", rid="PRR_head", cid=c) for i, c in enumerate(ids)] + self.assertEqual(0, pr_review.unthreaded_entries(payload([rd], threads))) + lone = revised_with(open_section(("6000000009",), noun="issue")) + self.assertEqual(1, pr_review.unthreaded_entries(payload([review(body=lone)]))) + def test_a_count_first_summary_outside_the_table_is_still_unvetted(self) -> None: """The table vets spellings, not any summary that opens on a count.""" body = revised_with(open_section(("6000000001",), noun="questions")) @@ -5015,7 +5097,11 @@ def test_fifty_outranks_a_stuck_check(self) -> None: def test_a_collapsed_finding_cancels_the_reading(self) -> None: """A suppressed or previously-missed block is a counted finding the digest already prints.""" - for summary in ("Previously missed (1)", "Suppressed comments (1)"): + for summary in ( + "Previously missed (1)", + "2 previously missed issues", + "Suppressed comments (1)", + ): with self.subTest(summary=summary): block = ( f"
\n{summary}\n\n" From ed66727d03ca9866f7256ca0224fa044dfa4ed1e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 16 Oct 2026 19:29:06 -0700 Subject: [PATCH 2/6] Read a Heading's Count in One Place in pr_review.py finding_count, section_entries and sibling_heading each read a section heading's count their own way, and only finding_count learned the count-first spelling. sibling_heading still bounded a section on `(N)` alone, so a suppressed block sharing a wrapper with a count-first previously-missed sibling ran into it, and the digest printed that finding twice. heading_count now serves all three, reading `(N)` or a count opening the heading. marker_blocks keeps requiring `(N)` for a line that is not a heading, since a count opening a line cannot tell a heading from prose. The count patterns take ASCII digits only, so a run of non-ASCII digits no longer overlaps the non-ASCII opener alternative. Refs #3167 Refs #3170 --- scripts/pr_review.py | 35 ++++++++++++++++++++------------- tests/test_pr_review.py | 43 +++++++++++++++++++++++++++++++---------- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 731d3ec3..8a7df9b5 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -448,7 +448,7 @@ # What this still admits is non-ASCII punctuation, an em dash or a typographic quote opening a sentence, which reads that sentence as the section. # It is narrower than the spelling before it, which admitted ASCII punctuation as well, and the remedy for the rest is a class of non-ASCII symbols rather than the whole of non-ASCII. PREVIOUSLY_MISSED = re.compile( - r"^(?:]*>|[ \t>#]|[*_](?![ \t])|[^\x00-\x7f])*(?:\d{1,4}[ \t]+)?Previously missed\b", + r"^(?:]*>|[ \t>#]|[*_](?![ \t])|[^\x00-\x7f])*(?:[0-9]{1,4}[ \t]+)?Previously missed\b", re.IGNORECASE, ) # CodeRabbit's own equivalent, collapsed into the review body like `SUPPRESSED` rather than raised as an inline comment. @@ -821,7 +821,7 @@ def strip_fences( SUMMARY = re.compile(r"]*)?>(.*?)", re.DOTALL | re.IGNORECASE) TAGS = re.compile(r"", re.IGNORECASE) COUNT = re.compile(r"\((\d+)\)") -LEADING_COUNT = re.compile(r"(?:[ \t#*_]|[^\x00-\x7f])*(\d+)[ \t]") +LEADING_COUNT = re.compile(r"(?:[ \t#*_]|[^\x00-\x7f])*([0-9]+)[ \t]") # What makes a line a heading rather than prose mentioning the phrase, in either markup. # A line that is neither still qualifies where it carries a count. # The section has appeared as a bare line too, and a count is what prose does not carry. @@ -2400,8 +2400,7 @@ def section_entries(body: str) -> list[tuple[str, int | None, list[str]]]: role = roles.get(normal(summary).casefold()) if role is None: continue - digits = re.search(r"\d+", EMPHASIS.sub("", summary)) - count = counted(digits.group()) if digits else None + count = heading_count(summary) text = masked[start:end] own = "\n".join(text[a:b] for a, b in details_spans(tags[start:end])[1]) found.append((role, count, [ln for ln in own.splitlines() if ENTRY_LINE.match(ln)])) @@ -2422,9 +2421,9 @@ def stated_total(body: str, preamble_only: bool = False) -> int | None: `MARKUP_MASK`, for the reason `details_tags` gives, which keeps every offset the preamble needs. A later revision of the format drops the `**Findings:**` line and states its total as a bare - `**0 open findings**` line, read here the same way, since left unread it printed `?` and - suppressed the shortfall the total is compared for. A later one still writes that zero as - `**No open issues**` followed by an emoji, which is the same stated zero. + `**0 open findings**` line. That line is read here the same way, since left unread it printed + `?` and suppressed the shortfall the total is compared for. A later one still writes that zero as + `**No open issues**` and an emoji. That is the same stated zero. That revision writes the bare line only for a zero, measured over every body carrying it, and states any other total as the count opening its open-findings section, so where the preamble @@ -4275,6 +4274,18 @@ def heading_of(block: str) -> str: return m.group(1) if m and head.lower().startswith(" int | None: + """The count the block's own heading states, closing it as `(N)` or opening it. + + One reader serves every caller, since the format moved its count to the front of a summary. + A caller reading only one of the two places misses the other. A section bounded on `(N)` alone + ran into a count-first sibling and printed that sibling's findings twice. + """ + heading = heading_of(block) + m = COUNT.search(heading) or LEADING_COUNT.match(EMPHASIS.sub("", heading)) + return counted(m.group(1)) if m else None + + # `
(.*?)
` lazily pairs each open with the *next* close, which is the innermost one once a shape nests, silently losing everything the outer wrapper still carries after it. # CodeRabbit's outside-diff section does exactly that: a file wrapper nested inside the section heading, itself wrapping a per-finding "Prompt for AI Agents" block three levels deep. DETAILS_TAG = re.compile( @@ -4458,7 +4469,7 @@ def sibling_heading(scan_lines: list[str], start: int, marker: re.Pattern[str]) """ for j in range(start + 1, len(scan_lines)): line = scan_lines[j] - if not (HEADING.match(line) and COUNT.search(line)): + if not (HEADING.match(line) and heading_count(line) is not None): continue if any(o is not marker and o.search(line) for o in SECTION_MARKERS): return j @@ -4519,15 +4530,13 @@ def outside_diff_blocks(body: str) -> list[str]: def finding_count(block: str) -> int: """The heading's `(N)`, floored at one, since a block reported as zero reads as a clean pass. - A heading stating no `(N)` is read for a count opening it, `2 previously missed issues`, which - left unread fell to the floor and reported one finding where the round raised two. + A heading stating its count first, `2 previously missed issues`, is read by `heading_count` too. + Left unread, it fell to the floor and reported one finding where the round raised two. A run longer than a count carries falls to that same floor rather than crashing the digest, the block existing being what says there is at least one finding in it. """ - heading = heading_of(block) - m = COUNT.search(heading) or LEADING_COUNT.match(EMPHASIS.sub("", heading)) - stated = counted(m.group(1)) if m else None + stated = heading_count(block) return max(stated, 1) if stated is not None else 1 diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index eae7afc2..1d0385cd 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1402,17 +1402,18 @@ def test_the_heading_is_read_through_whatever_markup_it_wears(self) -> None: def test_a_count_first_heading_is_the_section_and_states_its_count(self) -> None: """A later revision opens the heading on its count, `2 previously missed issues`. - Unread, the round exited 43 on an unvetted summary, and its findings, which open no thread, - reached the agent only through the raw body. Read as the section without its count, the - block fell to the floor of one and reported one finding where the round raised two. + Unread, the round exited 43 on an unvetted summary. Its findings open no thread, so they + reached the agent only through the raw body. Read without its count, the block fell to the + floor of one. It then reported one finding where the round raised two. """ body = counted_missed(2) self.assertEqual([], pr_review.unrecognized_in(body)) blocks = pr_review.previously_missed_blocks(body) self.assertEqual(1, len(blocks)) self.assertEqual(2, pr_review.finding_count(blocks[0])) + self.assertEqual([], pr_review.unrecognized_in(counted_missed(1))) self.assertEqual( - 1, pr_review.finding_count(pr_review.previously_missed_blocks(counted_missed(1))[0]) + 3, pr_review.finding_count(pr_review.previously_missed_blocks(counted_missed(3))[0]) ) self.answer(payload([review(body=body)])) out, _ = pr_review.digest("o", "r", 7) @@ -1420,10 +1421,32 @@ def test_a_count_first_heading_is_the_section_and_states_its_count(self) -> None self.assertIn("shapes=ok", out) self.assertIn("Low severity Constructed missed finding 2", out) + def test_a_section_ends_where_its_count_first_sibling_starts(self) -> None: + """A sibling bounds a section only where its heading states a count. + + A bound reading only `(N)` ran a suppressed block into a count-first previously-missed + sibling. The digest then printed that finding twice, once under the suppressed banner. + """ + shared = ( + f"{OVERVIEW}\n
\nReview details\n\n" + "### Suppressed comments (1)\n\nThe suppressed finding.\n\n" + "
\n2 previously missed issues\n\n" + "The previously missed finding.\n
\n
\n" + ) + suppressed = pr_review.suppressed_blocks(shared) + self.assertEqual(1, len(suppressed)) + self.assertIn("The suppressed finding.", suppressed[0]) + self.assertNotIn("The previously missed finding.", suppressed[0]) + self.answer(payload([review(body=shared)])) + out, _ = pr_review.digest("o", "r", 7) + self.assertEqual(1, out.count("The previously missed finding.")) + def test_a_count_first_section_cancels_the_flagging_verdict_reading(self) -> None: - """The round flags a closer look and states no open issue, and its findings are the - previously missed ones, which the digest now counts, so the round is not a verdict naming - nothing.""" + """The round flags a closer look and states no open issue. + + Its findings are the previously missed ones, which the digest now counts. So the round is + not a verdict naming nothing. + """ self.answer(payload([review(body=counted_missed(2))])) self.assertIsNone(pr_review.uncounted_verdict(payload([review(body=counted_missed(2))]))) with contextlib.redirect_stdout(io.StringIO()) as printed: @@ -1924,8 +1947,8 @@ def previously_missed( def counted_missed(entries: int) -> str: """The later revision's previously-missed section, its summary opening on the count. - The round flags a closer look, states its zero as `**No open issues**`, and nests one badged - entry per finding under the section, each a `FILE:LINE` anchor and a priority-tagged paragraph. + The round flags a closer look and states its zero as `**No open issues**`. The section nests + one badged entry per finding, each a `FILE:LINE` anchor and a priority-tagged paragraph. That nesting is the observed one, and every title, path, and sentence is constructed, per GOVERNANCE.md "Representative Data in Agent-Authored Text". """ @@ -4795,7 +4818,7 @@ def test_the_open_findings_section_states_the_round_s_total(self) -> None: def test_the_open_issues_spelling_is_the_same_section(self) -> None: """A later revision writes `1 open issue` where it wrote `1 open finding`, and exited 43. - Its entries link the round's own threads as the earlier spelling's do, so it is read as the + Its entries link the round's own threads, as the earlier spelling's do. So it is read as the same section: vetted, stating the total, and each entry checked against the threads. """ for ids in (("6000000001",), ("6000000001", "6000000002")): From 91707541b0ee79a8ec5972fb99c5fcffbfc45cf1 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 16 Oct 2026 19:36:53 -0700 Subject: [PATCH 3/6] Keep Count-First Prose Out of the Previously Missed Section A line that is not a heading qualified as a section on any `(N)` it carried. With the count-first spelling admitted by PREVIOUSLY_MISSED, a sentence such as `2 previously missed (3) rounds ago` read as a section. marker_blocks now accepts such a line only where it does not open on a count, since a leading count is how a heading states one and also how a sentence opens. PREVIOUSLY_MISSED takes a count of any length, so counted() stays the one place the digit limit applies, and a run longer than a count reads as the section at the floor of one rather than as no section at all. Refs #3167 --- scripts/pr_review.py | 11 ++++++++--- tests/test_pr_review.py | 14 ++++++++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 8a7df9b5..9890989f 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -448,7 +448,7 @@ # What this still admits is non-ASCII punctuation, an em dash or a typographic quote opening a sentence, which reads that sentence as the section. # It is narrower than the spelling before it, which admitted ASCII punctuation as well, and the remedy for the rest is a class of non-ASCII symbols rather than the whole of non-ASCII. PREVIOUSLY_MISSED = re.compile( - r"^(?:]*>|[ \t>#]|[*_](?![ \t])|[^\x00-\x7f])*(?:[0-9]{1,4}[ \t]+)?Previously missed\b", + r"^(?:]*>|[ \t>#]|[*_](?![ \t])|[^\x00-\x7f])*(?:[0-9]+[ \t]+)?Previously missed\b", re.IGNORECASE, ) # CodeRabbit's own equivalent, collapsed into the review body like `SUPPRESSED` rather than raised as an inline comment. @@ -4417,6 +4417,10 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F Shared by `suppressed_blocks` and `outside_diff_blocks`. `strip_blockquote` affects heading detection only, never the returned content. + + A line that is not a heading qualifies on a `(N)` it carries, and only where it does not open + on a count. A count opening a line is how a heading states it and also how a sentence opens, + so `2 previously missed (3) rounds ago` is prose, not a section. """ if not body: return [] @@ -4435,7 +4439,8 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F if strip_blockquote: scan_lines = [BLOCKQUOTE.sub("", ln) for ln in scan_lines] for i, line in enumerate(scan_lines): - if marker.search(line) and (HEADING.match(line) or COUNT.search(line)): + bare = COUNT.search(line) and not LEADING_COUNT.match(line) + if marker.search(line) and (HEADING.match(line) or bare): blocks.append("\n".join(raw_lines[i : sibling_heading(scan_lines, i, marker)])) break return blocks @@ -4528,7 +4533,7 @@ def outside_diff_blocks(body: str) -> list[str]: def finding_count(block: str) -> int: - """The heading's `(N)`, floored at one, since a block reported as zero reads as a clean pass. + """The heading's stated count, floored at one, since a block reported as zero reads as clean. A heading stating its count first, `2 previously missed issues`, is read by `heading_count` too. Left unread, it fell to the floor and reported one finding where the round raised two. diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 1d0385cd..c3a94313 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1368,8 +1368,12 @@ def test_prose_naming_the_section_is_not_the_section(self) -> None: self.assertEqual([], pr_review.previously_missed_blocks(f"{OVERVIEW}\n{line}\n")) counted = f"{opener}2 previously missed issues were re-raised this round." self.assertIsNone(pr_review.PREVIOUSLY_MISSED.match(counted)) - bare = "2 previously missed issues were re-raised this round." - self.assertEqual([], pr_review.previously_missed_blocks(f"{OVERVIEW}\n{bare}\n")) + for bare in ( + "2 previously missed issues were re-raised this round.", + "2 previously missed (3) rounds ago, the guard is back.", + ): + with self.subTest(bare=bare): + self.assertEqual([], pr_review.previously_missed_blocks(f"{OVERVIEW}\n{bare}\n")) def test_the_heading_is_read_through_whatever_markup_it_wears(self) -> None: """What the anchor must admit, beside the prose above that it must not. @@ -1415,6 +1419,12 @@ def test_a_count_first_heading_is_the_section_and_states_its_count(self) -> None self.assertEqual( 3, pr_review.finding_count(pr_review.previously_missed_blocks(counted_missed(3))[0]) ) + long_run = counted_missed(1).replace( + "1 previously missed issue", "12345 previously missed issue" + ) + long_blocks = pr_review.previously_missed_blocks(long_run) + self.assertEqual(1, len(long_blocks)) + self.assertEqual(1, pr_review.finding_count(long_blocks[0])) self.answer(payload([review(body=body)])) out, _ = pr_review.digest("o", "r", 7) self.assertIn("previously_missed=2 (on_head=2 earlier=0)", out) From 9722c346a2295fa5a0b359e63175ca30aebff326 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 16 Oct 2026 19:51:37 -0700 Subject: [PATCH 4/6] Read a Leading Count Past the Same Opener the Anchor Admits PREVIOUSLY_MISSED admitted a leading count after a tag or a `>`, which LEADING_COUNT did not strip, so `> 2 previously missed (3) rounds ago` escaped the prose check in marker_blocks and read as a section. Both patterns now share HEADING_OPENER, so what the anchor admits the prose check strips. heading_count takes the heading itself and reads all of it. Cut to its first line, a summary split as `Open` then `(2)` lost the count its role was vetted with. marker_blocks folds a `` split across lines onto its opening line before scanning, so a count-first summary whose text sits on lines of its own is read as the section its vetted spelling says it is. The comments that said a count is what prose does not carry now say which count, and the prose test's docstring says the same. Refs #3167 --- scripts/pr_review.py | 60 +++++++++++++++++++++++++++++++++-------- tests/test_pr_review.py | 42 +++++++++++++++++++++++++++-- 2 files changed, 89 insertions(+), 13 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 9890989f..3ee601d2 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -433,8 +433,12 @@ # The second overview format's own section for a finding against code the pull request did not change. # Anchored to the opening of its line, past whatever markup a heading wears, rather than matched anywhere in it. # The wording is ordinary English, so a body-wide match reads a sentence such as `restores a guard previously missed (2) rounds ago` as this section. -# `marker_blocks` accepts a line on a count alone, which is what lets that sentence through, so the anchor is what separates a heading from prose naming one. +# `marker_blocks` accepts a line that is not a heading on a `(N)` alone, which is what lets that sentence through. +# So the anchor is what separates a heading from prose naming one. # The opener admits heading markup and anything outside ASCII, and nothing else. +# One count may follow it, since a later revision opens the heading on its count, as `2 previously missed issues`. +# A count opening a line is how a sentence opens too, so `marker_blocks` reads such a line as prose unless it is a heading. +# `LEADING_COUNT` reads that count past this same opener, so whatever markup the anchor admits, the prose check strips. # An emoji is what this format already puts on its verdict headings, and a spelling that admitted no such character read a heading wearing one as no section at all. # That failure is silent rather than loud, the heading itself being vetted and `normal` dropping the emoji before the comparison, so the digest reports every shape read while the findings reach nobody. # Widened to any non-word character instead, ordinary punctuation opened the run and a sentence such as `- Previously missed (2) findings were re-raised` became the section, printing the prose around it as the finding. @@ -443,12 +447,14 @@ # Two alternatives able to consume the same character make the run ambiguous, and the parse then doubles per character of it on a line that does not go on to say this, which is a hang rather than a slow read. # A body carrying 30 non-breaking spaces in a line took minutes, and every alternative here now opens on a character the others cannot. # An emphasis marker is admitted only where no space follows it, which is what tells `**Previously missed (1)**` from the list item `* Previously missed (2) findings`. -# A bullet is the one prose opener that is also heading markup, and `marker_blocks` accepts a line on a count alone, so the list item read as the section and printed the prose around it as findings. +# A bullet is the one prose opener that is also heading markup, and `marker_blocks` accepts a line on a `(N)` alone. +# So the list item read as the section and printed the prose around it as findings. # One character per iteration rather than a run of them, since a repeated group that can split a run of stars more than one way is ambiguous and parses exponentially in it. # What this still admits is non-ASCII punctuation, an em dash or a typographic quote opening a sentence, which reads that sentence as the section. # It is narrower than the spelling before it, which admitted ASCII punctuation as well, and the remedy for the rest is a class of non-ASCII symbols rather than the whole of non-ASCII. +HEADING_OPENER = r"(?:]*>|[ \t\r\n>#]|[*_](?![ \t])|[^\x00-\x7f])*" PREVIOUSLY_MISSED = re.compile( - r"^(?:]*>|[ \t>#]|[*_](?![ \t])|[^\x00-\x7f])*(?:[0-9]+[ \t]+)?Previously missed\b", + rf"^{HEADING_OPENER}(?:[0-9]+[ \t]+)?Previously missed\b", re.IGNORECASE, ) # CodeRabbit's own equivalent, collapsed into the review body like `SUPPRESSED` rather than raised as an inline comment. @@ -821,10 +827,12 @@ def strip_fences( SUMMARY = re.compile(r"]*)?>(.*?)", re.DOTALL | re.IGNORECASE) TAGS = re.compile(r"", re.IGNORECASE) COUNT = re.compile(r"\((\d+)\)") -LEADING_COUNT = re.compile(r"(?:[ \t#*_]|[^\x00-\x7f])*([0-9]+)[ \t]") +# A count opening a heading, past the opener `PREVIOUSLY_MISSED` admits, a summary's line breaks included. +LEADING_COUNT = re.compile(rf"{HEADING_OPENER}([0-9]+)[ \t\r\n]") # What makes a line a heading rather than prose mentioning the phrase, in either markup. -# A line that is neither still qualifies where it carries a count. -# The section has appeared as a bare line too, and a count is what prose does not carry. +# A line that is neither still qualifies where it carries a `(N)` and does not open on a count. +# The section has appeared as a bare line too, and a closing `(N)` is what prose does not carry. +# A count opening the line is not that signal, since a sentence opens on one as readily as a heading does. HEADING = re.compile(r"\s*(?:#{1,6}\s| str: return m.group(1) if m and head.lower().startswith(" int | None: - """The count the block's own heading states, closing it as `(N)` or opening it. +def heading_count(heading: str) -> int | None: + """The count a heading states, closing it as `(N)` or opening it. One reader serves every caller, since the format moved its count to the front of a summary. A caller reading only one of the two places misses the other. A section bounded on `(N)` alone ran into a count-first sibling and printed that sibling's findings twice. + + It takes the heading itself rather than the block, and reads all of it. Cut to its first line, + a summary split as `Open` and then `(2)` lost the count its role was vetted with. """ - heading = heading_of(block) - m = COUNT.search(heading) or LEADING_COUNT.match(EMPHASIS.sub("", heading)) + m = COUNT.search(heading) or LEADING_COUNT.match(heading) return counted(m.group(1)) if m else None @@ -4436,6 +4446,7 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F # Scanned line by line rather than read from a wrapper's own summary. # A heading can sit in the wrapper's body instead, where reading only the summary would miss it. scan_lines = "".join(masked[a:b] for a, b in spans).splitlines() + scan_lines, raw_lines = fold_summaries(scan_lines, raw_lines) if strip_blockquote: scan_lines = [BLOCKQUOTE.sub("", ln) for ln in scan_lines] for i, line in enumerate(scan_lines): @@ -4446,6 +4457,33 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F return blocks +def fold_summaries(scan_lines: list[str], raw_lines: list[str]) -> tuple[list[str], list[str]]: + """Both line lists with each `` split across lines folded onto its opening line. + + A heading is found line by line, so a summary whose text sits on lines of its own read as no + heading at all. `unrecognized_in` vetted its spelling meanwhile and reported nothing. The groups + come from the scanned copy and apply to both, so the two keep addressing the same lines. + """ + scan: list[str] = [] + raw: list[str] = [] + i = 0 + while i < len(scan_lines): + low = scan_lines[i].lower() + opened = low.rfind("= 0 and "" not in low[opened:]: + close = next( + (j for j in range(i + 1, len(scan_lines)) if "" in scan_lines[j].lower()), + None, + ) + end = close if close is not None else i + for source, target in ((scan_lines, scan), (raw_lines, raw)): + parts = [source[i].rstrip()] + [ln.strip() for ln in source[i + 1 : end + 1]] + target.append(" ".join(parts)) + i = end + 1 + return scan, raw + + def sibling_heading(scan_lines: list[str], start: int, marker: re.Pattern[str]) -> int: """Where this block ends, which is the next sibling section's heading or the region's end. @@ -4541,7 +4579,7 @@ def finding_count(block: str) -> int: A run longer than a count carries falls to that same floor rather than crashing the digest, the block existing being what says there is at least one finding in it. """ - stated = heading_count(block) + stated = heading_count(heading_of(block)) return max(stated, 1) if stated is not None else 1 diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index c3a94313..3b3157a8 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1337,8 +1337,8 @@ def test_no_exit_code_rides_on_it(self) -> None: self.assertEqual(0, code) def test_prose_naming_the_section_is_not_the_section(self) -> None: - """`marker_blocks` accepts a line on a count alone, so an unanchored marker reads an - ordinary English sentence carrying a number as a section this round raised. + """`marker_blocks` accepts a line that is not a heading on a `(N)` alone. An unanchored + marker then reads an ordinary English sentence carrying a number as a section this round raised. The observed wording is common enough to write by accident, and the result is a banner telling the reader a clean round raised a finding against code the branch never touched. @@ -1371,6 +1371,9 @@ def test_prose_naming_the_section_is_not_the_section(self) -> None: for bare in ( "2 previously missed issues were re-raised this round.", "2 previously missed (3) rounds ago, the guard is back.", + "> 2 previously missed (3) rounds ago, the guard is back.", + "2 previously missed (3) rounds ago", + "2 previously missed (3) rounds ago, the guard is back.", ): with self.subTest(bare=bare): self.assertEqual([], pr_review.previously_missed_blocks(f"{OVERVIEW}\n{bare}\n")) @@ -1431,6 +1434,29 @@ def test_a_count_first_heading_is_the_section_and_states_its_count(self) -> None self.assertIn("shapes=ok", out) self.assertIn("Low severity Constructed missed finding 2", out) + def test_a_summary_split_across_lines_is_still_the_section(self) -> None: + """A summary whose text sits on lines of its own is vetted by its spelling. A heading + found only within one line read it as no section, while `shapes=ok` reported nothing.""" + for label, summary in ( + ( + "its text on a line of its own", + "\n2 previously missed issues\n", + ), + ( + "its wording broken across lines", + "2 previously\nmissed issues", + ), + ): + with self.subTest(case=label): + body = counted_missed(2).replace( + "2 previously missed issues", summary + ) + self.assertEqual([], pr_review.unrecognized_in(body)) + blocks = pr_review.previously_missed_blocks(body) + self.assertEqual(1, len(blocks)) + self.assertEqual(2, pr_review.finding_count(blocks[0])) + self.assertIn("Constructed missed finding 2", blocks[0]) + def test_a_section_ends_where_its_count_first_sibling_starts(self) -> None: """A sibling bounds a section only where its heading states a count. @@ -4843,6 +4869,18 @@ def test_the_open_issues_spelling_is_the_same_section(self) -> None: lone = revised_with(open_section(("6000000009",), noun="issue")) self.assertEqual(1, pr_review.unthreaded_entries(payload([review(body=lone)]))) + def test_a_summary_split_across_lines_keeps_its_count(self) -> None: + """The role is vetted on the whole summary, so its count is read from the whole summary + too. Read from its first line alone, `Open` then `(2)` stated no count.""" + ids = ("6000000001", "6000000002", "6000000003") + split = revised_with(open_section(ids)).replace( + "3 open findings", "\n3 open findings\n" + ) + self.assertEqual([3], [c for r, c, _ in pr_review.section_entries(split) if r == "total"]) + self.assertEqual(3, pr_review.stated_total(split)) + closing = overview_v2().replace("Open (2)", "Open\n(2)") + self.assertEqual([2], [c for r, c, _ in pr_review.section_entries(closing) if r == "open"]) + def test_a_count_first_summary_outside_the_table_is_still_unvetted(self) -> None: """The table vets spellings, not any summary that opens on a count.""" body = revised_with(open_section(("6000000001",), noun="questions")) From e1d9f2618469313626649ae7061e281fa084aa1c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 16 Oct 2026 19:59:44 -0700 Subject: [PATCH 5/6] Read a Heading's Count Past Emphasis Alone and Bound the Summary Fold Sharing the wide opener with the count reader made a finding entry's severity badge read as heading markup, so an entry titled with a number stated a count and bounded its section there, dropping the entries after it. LEADING_COUNT reads past emphasis tags only again, and the prose check in marker_blocks uses OPENING_COUNT, which keeps the wide opener the anchor admits. fold_summaries folds only a line opening on `]*)?>(.*?)", re.DOTALL | re.IGNORECASE) TAGS = re.compile(r"", re.IGNORECASE) COUNT = re.compile(r"\((\d+)\)") -# A count opening a heading, past the opener `PREVIOUSLY_MISSED` admits, a summary's line breaks included. -LEADING_COUNT = re.compile(rf"{HEADING_OPENER}([0-9]+)[ \t\r\n]") +# A count opening a line past any opener `PREVIOUSLY_MISSED` admits, which is how `marker_blocks` tells prose. +OPENING_COUNT = re.compile(rf"{HEADING_OPENER}[0-9]+[ \t]") +# A count opening a heading's own text, past emphasis and nothing else that is a tag, a summary's line breaks included. +# Narrower than the opener above, since a finding entry's summary opens on its severity badge. +# A badge read as heading markup made an entry titled `2 low confidence results` state a count, and bound a section there. +LEADING_COUNT = re.compile(r"(?:|[ \t\r\n#*_]|[^\x00-\x7f])*([0-9]+)[ \t\r\n]") # What makes a line a heading rather than prose mentioning the phrase, in either markup. # A line that is neither still qualifies where it carries a `(N)` and does not open on a count. # The section has appeared as a bare line too, and a closing `(N)` is what prose does not carry. @@ -4425,8 +4429,9 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F """Return the review body's sections whose own heading matches `marker`, each sliced from that heading through to the end of its own region. - Shared by `suppressed_blocks` and `outside_diff_blocks`. `strip_blockquote` affects heading - detection only, never the returned content. + Shared by `suppressed_blocks`, `previously_missed_blocks`, and `outside_diff_blocks`. + `strip_blockquote` affects heading detection only, never the returned content. A summary split + across lines is folded onto its opening line in the returned content too, per `fold_summaries`. A line that is not a heading qualifies on a `(N)` it carries, and only where it does not open on a count. A count opening a line is how a heading states it and also how a sentence opens, @@ -4450,36 +4455,45 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F if strip_blockquote: scan_lines = [BLOCKQUOTE.sub("", ln) for ln in scan_lines] for i, line in enumerate(scan_lines): - bare = COUNT.search(line) and not LEADING_COUNT.match(line) + bare = COUNT.search(line) and not OPENING_COUNT.match(line) if marker.search(line) and (HEADING.match(line) or bare): blocks.append("\n".join(raw_lines[i : sibling_heading(scan_lines, i, marker)])) break return blocks +FOLD_OPENER = re.compile(r"[>\s]*]|$)", re.IGNORECASE) +FOLD_STOP = re.compile(r"\s*<(?:details|summary)(?=[\s>/]|$)", re.IGNORECASE) + + def fold_summaries(scan_lines: list[str], raw_lines: list[str]) -> tuple[list[str], list[str]]: """Both line lists with each `` split across lines folded onto its opening line. A heading is found line by line, so a summary whose text sits on lines of its own read as no heading at all. `unrecognized_in` vetted its spelling meanwhile and reported nothing. The groups come from the scanned copy and apply to both, so the two keep addressing the same lines. + + Only a line opening on `= 0 and "" not in low[opened:]: - close = next( - (j for j in range(i + 1, len(scan_lines)) if "" in scan_lines[j].lower()), - None, - ) - end = close if close is not None else i + if FOLD_OPENER.match(scan_lines[i]) and "" not in scan_lines[i].lower(): + for j in range(i + 1, len(scan_lines)): + bare = BLOCKQUOTE.sub("", scan_lines[j].strip()) + if not bare.strip() or FOLD_STOP.match(bare): + break + if "" in bare.lower(): + end = j + break for source, target in ((scan_lines, scan), (raw_lines, raw)): - parts = [source[i].rstrip()] + [ln.strip() for ln in source[i + 1 : end + 1]] - target.append(" ".join(parts)) + parts = [BLOCKQUOTE.sub("", ln.strip()) for ln in source[i + 1 : end + 1]] + target.append(" ".join([source[i].rstrip(), *parts])) i = end + 1 return scan, raw @@ -4512,7 +4526,7 @@ def sibling_heading(scan_lines: list[str], start: int, marker: re.Pattern[str]) """ for j in range(start + 1, len(scan_lines)): line = scan_lines[j] - if not (HEADING.match(line) and heading_count(line) is not None): + if not (HEADING.match(line) and heading_count(heading_of(line)) is not None): continue if any(o is not marker and o.search(line) for o in SECTION_MARKERS): return j diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 3b3157a8..05af2c5a 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1457,6 +1457,37 @@ def test_a_summary_split_across_lines_is_still_the_section(self) -> None: self.assertEqual(2, pr_review.finding_count(blocks[0])) self.assertIn("Constructed missed finding 2", blocks[0]) + def test_an_entry_title_opening_on_a_number_bounds_no_section(self) -> None: + """An entry's summary opens on its severity badge, which is not heading markup. + + Read as markup, an entry titled `2 low confidence results are dropped` stated a count. That + entry then bounded the section, and the entries after it were dropped from it. + """ + body = counted_missed(3).replace( + "Constructed missed finding 2", "2 low confidence results are dropped" + ) + blocks = pr_review.previously_missed_blocks(body) + self.assertEqual(1, len(blocks)) + self.assertIn("Constructed missed finding 3", blocks[0]) + self.assertEqual(3, pr_review.finding_count(blocks[0])) + + def test_a_tag_named_in_prose_folds_nothing(self) -> None: + """Only a line opening on `\nReview details\n\n" + "### Suppressed comments (1)\n\n" + "The SUMMARY pattern misses a opener.\n\n" + "2 previously missed issues\n\n" + "The previously missed finding.\n
\n" + ) + missed = pr_review.previously_missed_blocks(shared) + self.assertEqual(1, len(missed)) + self.assertIn("The previously missed finding.", missed[0]) + suppressed = pr_review.suppressed_blocks(shared) + self.assertEqual(1, len(suppressed)) + self.assertNotIn("The previously missed finding.", suppressed[0]) + def test_a_section_ends_where_its_count_first_sibling_starts(self) -> None: """A sibling bounds a section only where its heading states a count. From b994110227fa8387360572ed2006297028ef8dda Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 16 Oct 2026 20:09:06 -0700 Subject: [PATCH 6/6] Read a Heading's Count From the Text Its Role Is Vetted On heading_count now reduces the badge and the emphasis the way normal does before vetting, then reads the count. A count wrapped in its own emphasis tag, or emphasis in capitals, reads as it did on develop, and a badge reduces to its word, so an entry titled with a number still states no count. LEADING_COUNT skips no tag of its own any more. fold_summaries no longer stops at a blank line, which left a summary set off by blank lines unread. It stops at a line opening another wrapper, summary, or Markdown heading. unrecognized_in reports a previously-missed summary that no reader finds as a section, since its spelling is vetted wherever it appears while the reader keys on how its line opens. A layout the reader misses is then a reported shape rather than findings nobody reads. HEADING_OPENER drops the line breaks only the count reader needed. Refs #3167 --- scripts/pr_review.py | 58 ++++++++++++++++++++++++++++------------- tests/test_pr_review.py | 29 +++++++++++++++++++++ 2 files changed, 69 insertions(+), 18 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index e4d9e236..c79538ac 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -452,7 +452,7 @@ # One character per iteration rather than a run of them, since a repeated group that can split a run of stars more than one way is ambiguous and parses exponentially in it. # What this still admits is non-ASCII punctuation, an em dash or a typographic quote opening a sentence, which reads that sentence as the section. # It is narrower than the spelling before it, which admitted ASCII punctuation as well, and the remedy for the rest is a class of non-ASCII symbols rather than the whole of non-ASCII. -HEADING_OPENER = r"(?:]*>|[ \t\r\n>#]|[*_](?![ \t])|[^\x00-\x7f])*" +HEADING_OPENER = r"(?:]*>|[ \t>#]|[*_](?![ \t])|[^\x00-\x7f])*" PREVIOUSLY_MISSED = re.compile( rf"^{HEADING_OPENER}(?:[0-9]+[ \t]+)?Previously missed\b", re.IGNORECASE, @@ -703,18 +703,24 @@ def strip_fences( "Resolved since last review (N)": RESOLVED, "(N) resolved since last review": RESOLVED, } -VETTED_SUMMARIES = set(OVERVIEW_SECTIONS) | { - "Pull request overview", - NARRATIVE_SUMMARY, - "Show a summary per file", - "File summaries", - "Review details", - "Suppressed comments (N)", - "Comments suppressed due to low confidence (N)", +PREVIOUSLY_MISSED_SUMMARIES = { "Previously missed (N)", "(N) previously missed issues", "(N) previously missed issue", } +VETTED_SUMMARIES = ( + set(OVERVIEW_SECTIONS) + | { + "Pull request overview", + NARRATIVE_SUMMARY, + "Show a summary per file", + "File summaries", + "Review details", + "Suppressed comments (N)", + "Comments suppressed due to low confidence (N)", + } + | PREVIOUSLY_MISSED_SUMMARIES +) VETTED_LABELS = {"Files reviewed", "Comments generated", "Review effort level", "Review effort"} # Inline emphasis around a marker is presentation rather than identity. # The second overview format wraps every `` in `` where the first wrapped none. @@ -829,10 +835,10 @@ def strip_fences( COUNT = re.compile(r"\((\d+)\)") # A count opening a line past any opener `PREVIOUSLY_MISSED` admits, which is how `marker_blocks` tells prose. OPENING_COUNT = re.compile(rf"{HEADING_OPENER}[0-9]+[ \t]") -# A count opening a heading's own text, past emphasis and nothing else that is a tag, a summary's line breaks included. -# Narrower than the opener above, since a finding entry's summary opens on its severity badge. -# A badge read as heading markup made an entry titled `2 low confidence results` state a count, and bound a section there. -LEADING_COUNT = re.compile(r"(?:|[ \t\r\n#*_]|[^\x00-\x7f])*([0-9]+)[ \t\r\n]") +# A count opening a heading's text, read once `heading_count` has reduced its badge and emphasis as `normal` does. +# No tag is skipped here, since a finding entry's summary opens on a severity badge rather than on heading markup. +# A badge read as markup made an entry titled `2 low confidence results` state a count, and bound a section there. +LEADING_COUNT = re.compile(r"(?:[ \t\r\n#*_]|[^\x00-\x7f])*([0-9]+)[ \t\r\n]") # What makes a line a heading rather than prose mentioning the phrase, in either markup. # A line that is neither still qualifies where it carries a `(N)` and does not open on a count. # The section has appeared as a bare line too, and a closing `(N)` is what prose does not carry. @@ -3667,6 +3673,10 @@ def unrecognized_in(body: str) -> list[str]: quoting a heading is not a review carrying one, and this script's own pull requests quote these lists in full. + A previously-missed summary that no reader finds as a section is reported too. Its spelling is + vetted wherever it appears, while the reader keys on how its line opens. A layout the reader + misses would otherwise hand its findings to nobody under `shapes=ok`. + A body carrying no heading at all is reported rather than passed, since every one of the 333 measured opens on a heading and a body with none is a format nothing here has seen. A refusal is the exemption, being a bare paragraph by design and already classified as one. @@ -3689,6 +3699,13 @@ def unrecognized_in(body: str) -> list[str]: for marker in (normal(s) for s in dict.fromkeys(SUMMARY.findall(plain))) if unvetted(marker, VETTED_SUMMARIES) and not finding_entry(marker) ] + missed = [ + marker + for marker in (normal(s) for s in SUMMARY.findall(plain)) + if not unvetted(marker, PREVIOUSLY_MISSED_SUMMARIES) + ] + if len(missed) > len(previously_missed_blocks(body)): + found.append(f"summary: {missed[0]}, vetted while no reader found its section") found += [ f"metadata label: {la}" for la in dict.fromkeys(labels) if unvetted(la, VETTED_LABELS) ] @@ -4295,8 +4312,13 @@ def heading_count(heading: str) -> int | None: It takes the heading itself rather than the block, and reads all of it. Cut to its first line, a summary split as `Open` and then `(2)` lost the count its role was vetted with. + + The badge and the emphasis are reduced first, as `normal` reduces them before vetting. The + count is then read from the same text the role was vetted on. A badge reduces to its word, which is + what keeps an entry titled `2 low confidence results` from stating a count. """ - m = COUNT.search(heading) or LEADING_COUNT.match(heading) + text = EMPHASIS.sub("", SEVERITY_BADGE.sub(badge_text, heading)) + m = COUNT.search(text) or LEADING_COUNT.match(text) return counted(m.group(1)) if m else None @@ -4473,9 +4495,9 @@ def fold_summaries(scan_lines: list[str], raw_lines: list[str]) -> tuple[list[st heading at all. `unrecognized_in` vetted its spelling meanwhile and reported nothing. The groups come from the scanned copy and apply to both, so the two keep addressing the same lines. - Only a line opening on ` tuple[list[st if FOLD_OPENER.match(scan_lines[i]) and "" not in scan_lines[i].lower(): for j in range(i + 1, len(scan_lines)): bare = BLOCKQUOTE.sub("", scan_lines[j].strip()) - if not bare.strip() or FOLD_STOP.match(bare): + if FOLD_STOP.match(bare) or MARKDOWN_HEADING.match(bare): break if "" in bare.lower(): end = j diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 05af2c5a..b06e1270 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1442,6 +1442,14 @@ def test_a_summary_split_across_lines_is_still_the_section(self) -> None: "its text on a line of its own", "\n2 previously missed issues\n", ), + ( + "its text set off by blank lines", + "\n\n2 previously missed issues\n\n", + ), + ( + "its emphasis in capitals", + "2 previously missed issues", + ), ( "its wording broken across lines", "2 previously\nmissed issues", @@ -1457,6 +1465,21 @@ def test_a_summary_split_across_lines_is_still_the_section(self) -> None: self.assertEqual(2, pr_review.finding_count(blocks[0])) self.assertIn("Constructed missed finding 2", blocks[0]) + def test_a_vetted_summary_no_reader_finds_is_reported(self) -> None: + """The spelling is vetted wherever it appears, while the reader keys on how its line opens. + + A layout the reader misses would hand its findings to nobody under `shapes=ok`. So the + shortfall between the two is reported as an unread shape instead. + """ + body = counted_missed(2).replace( + "2 previously missed issues", + "2 previously missed issues", + ) + self.assertEqual([], pr_review.previously_missed_blocks(body)) + found = pr_review.unrecognized_in(body) + self.assertEqual(1, len(found)) + self.assertIn("no reader found its section", found[0]) + def test_an_entry_title_opening_on_a_number_bounds_no_section(self) -> None: """An entry's summary opens on its severity badge, which is not heading markup. @@ -4909,6 +4932,12 @@ def test_a_summary_split_across_lines_keeps_its_count(self) -> None: ) self.assertEqual([3], [c for r, c, _ in pr_review.section_entries(split) if r == "total"]) self.assertEqual(3, pr_review.stated_total(split)) + for spelled in ("3 open findings", "3 open findings"): + with self.subTest(spelled=spelled): + body = revised_with(open_section(ids)).replace( + "3 open findings", spelled + ) + self.assertEqual(3, pr_review.stated_total(body)) closing = overview_v2().replace("Open (2)", "Open\n(2)") self.assertEqual([2], [c for r, c, _ in pr_review.section_entries(closing) if r == "open"])