From 92430d4b1c278f58c1ad6f7b7926d34371696706 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 07:53:17 -0700 Subject: [PATCH 1/4] Read a Section Heading Through One Reduction in pr_review.py One reduction, `reduced`, now serves `normal`, `unwrap`, and `heading_count`, and one `LEADING_COUNT` rule serves both vetting and counting, so a summary's vetted role and its count cannot disagree. The vetted-but-unread check now compares which previously-missed summaries the blocks hold rather than how many blocks there are. A finding entry's own summary, which opens on a severity badge, is never a section heading and never bounds one. Fixes #3174 Fixes #3173 Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 79 +++++++++++++++++++++++++++++---------- tests/test_pr_review.py | 82 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 20 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index c79538ac..827a36d8 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -380,6 +380,7 @@ import time import unicodedata import urllib.parse +from collections import Counter from collections.abc import Callable, Iterable, Iterator from datetime import UTC, datetime from pathlib import Path @@ -838,7 +839,7 @@ def strip_fences( # 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]") +LEADING_COUNT = re.compile(r"^([#*_ ]*)([0-9]+)(?= )") # 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. @@ -3616,6 +3617,20 @@ def counted(digits: str) -> int | None: return int(digits) if len(digits) <= COUNT_DIGITS else None +def reduced(text: str) -> str: + """The text with each severity badge reduced to its word and its emphasis markup dropped. + + The one reduction `normal`, `unwrap`, and `heading_count` share, so a heading is vetted, + printed, and counted from the same text. + """ + return EMPHASIS.sub("", SEVERITY_BADGE.sub(badge_text, text)) + + +def spaced(text: str) -> str: + """The text `reduced`, then cut to ASCII and single spaces, which `LEADING_COUNT` reads.""" + return re.sub(r"\s+", " ", "".join(c for c in reduced(text) if ord(c) < 128)).strip() + + def normal(text: str) -> str: """A marker reduced toward what a vetted list compares: emphasis markup dropped, ASCII, single spaces, counts as `(N)`. @@ -3631,12 +3646,10 @@ def normal(text: str) -> str: icon set is rebuilt, and the text it carries is the only part a reader wants. A count opening the marker is rewritten the same way, since a later revision of the second - format writes `3 resolved since last review` where it once closed the summary on `(3)`. + format writes `3 resolved since last review` where it once closed the summary on `(3)`. It is + found by `LEADING_COUNT`, the rule `heading_count` reads the count by. """ - unbadged = SEVERITY_BADGE.sub(badge_text, text) - ascii_only = "".join(c for c in EMPHASIS.sub("", unbadged) if ord(c) < 128) - spaced = re.sub(r"\s+", " ", re.sub(r"\(\d+\)", "(N)", ascii_only)).strip() - return re.sub(r"^\d+(?= )", "(N)", spaced) + return LEADING_COUNT.sub(r"\1(N)", re.sub(r"\([0-9]+\)", "(N)", spaced(text))) def finding_entry(marker: str) -> bool: @@ -3675,7 +3688,10 @@ def unrecognized_in(body: str) -> list[str]: 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`. + misses would otherwise hand its findings to nobody under `shapes=ok`. The check compares which + summaries the blocks hold rather than how many blocks there are, since one region yields one + block, and a bare `(N)` line read in one region otherwise stood in for a summary missed in + another. 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 @@ -3699,13 +3715,19 @@ 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 = [ + missed = Counter( 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") + ) + read = Counter( + normal(s) + for block in previously_missed_blocks(body) + for s in SUMMARY.findall(CODE_SPAN.sub(" ", strip_fences(block))) + ) + unread = missed - read + if unread: + found.append(f"summary: {next(iter(unread))}, vetted while no reader found its section") found += [ f"metadata label: {la}" for la in dict.fromkeys(labels) if unvetted(la, VETTED_LABELS) ] @@ -4313,13 +4335,15 @@ 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. + The count is read from `spaced`, the text `normal` vets the role on, through the same + `LEADING_COUNT`. A badge reduces to its word, which is what keeps an entry titled + `2 low confidence results` from stating a count. """ - 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 + text = spaced(heading) + closing = COUNT.search(text) + opening = LEADING_COUNT.match(text) + digits = closing.group(1) if closing else opening.group(2) if opening else None + return counted(digits) if digits is not None 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. @@ -4447,6 +4471,18 @@ def mask_quotations(body: str, span: str = " ") -> str: SECTION_MARKERS = (SUPPRESSED, PREVIOUSLY_MISSED) +def entry_heading(line: str) -> bool: + """Whether this line is a finding entry's own heading, which opens on a severity badge. + + A finding entry is never a section heading, whatever words its title carries. `SUPPRESSED` + matches anywhere in a line, and `PREVIOUSLY_MISSED` reads past the badge as heading markup, so + an entry titled `2 low confidence results are dropped` otherwise opened a section of its own. + A summary opening on a severity and stating a count is still reported by `unrecognized_in`, + as an unvetted spelling. + """ + return bool(SEVERITY_TEXT.match(spaced(TAGS.sub("", heading_of(line))))) + + def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = False) -> list[str]: """Return the review body's sections whose own heading matches `marker`, each sliced from that heading through to the end of its own region. @@ -4478,7 +4514,7 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F scan_lines = [BLOCKQUOTE.sub("", ln) for ln in scan_lines] for i, line in enumerate(scan_lines): bare = COUNT.search(line) and not OPENING_COUNT.match(line) - if marker.search(line) and (HEADING.match(line) or bare): + if marker.search(line) and (HEADING.match(line) or bare) and not entry_heading(line): blocks.append("\n".join(raw_lines[i : sibling_heading(scan_lines, i, marker)])) break return blocks @@ -4536,7 +4572,8 @@ def sibling_heading(scan_lines: list[str], start: int, marker: re.Pattern[str]) opposite triage about them, while both counts come out wrong. A stated count is what separates them, the same signal `finding_entry` reads: every section - heading here states one and a finding's own title does not. Requiring it also keeps this off + heading here states one and a finding's own title does not. A title can still state one, so an + entry opening on its severity badge never bounds, per `entry_heading`. Requiring it also keeps this off the CodeRabbit blocks, whose own marker is in neither of the two and which were never bounded before this. @@ -4550,6 +4587,8 @@ def sibling_heading(scan_lines: list[str], start: int, marker: re.Pattern[str]) line = scan_lines[j] if not (HEADING.match(line) and heading_count(heading_of(line)) is not None): continue + if entry_heading(line): + continue if any(o is not marker and o.search(line) for o in SECTION_MARKERS): return j return len(scan_lines) @@ -4571,7 +4610,7 @@ def unwrap(block: str) -> str: nothing, and a severity badge says one word wrapped in two versioned icon URLs that bury the title following them. The word is kept and the markup is not. """ - return TAGS.sub("", EMPHASIS.sub("", SEVERITY_BADGE.sub(badge_text, block))) + return TAGS.sub("", reduced(block)) def previously_missed_blocks(body: str) -> list[str]: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index b06e1270..107c3dcd 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1584,6 +1584,64 @@ def test_a_section_ends_where_its_sibling_s_heading_starts(self) -> None: self.assertEqual(1, out.count("The suppressed finding.")) self.assertEqual(1, out.count("The previously missed finding.")) + def test_a_finding_entry_is_no_section_whatever_its_title_says(self) -> None: + """An entry's title is free text, and either section's marker words can appear in it. + + Read as a heading, an entry in one section opened the other section at that entry, so the + finding printed under both banners, which assert opposite triage, under `shapes=ok`. + """ + body = f"{OVERVIEW}\n{COVERED}\n\n" + entries_section( + "Previously missed (2)", "Constructed finding 1", "2 low confidence results are dropped" + ) + self.assertEqual([], pr_review.suppressed_blocks(body)) + self.assertEqual(2, pr_review.previously_missed_blocks(body)[0].count("paragraph")) + self.assertEqual([], pr_review.unrecognized_in(body)) + body = f"{OVERVIEW}\n{COVERED}\n\n" + entries_section( + "Comments suppressed due to low confidence (2)", + "Constructed finding 1", + "2 previously missed guards are re-added", + ) + self.assertEqual([], pr_review.previously_missed_blocks(body)) + self.assertEqual(2, pr_review.suppressed_blocks(body)[0].count("paragraph")) + + def test_a_finding_entry_stating_a_count_does_not_end_its_section(self) -> None: + """A title can state a count, which is what bounds a section on its sibling's heading. + + Bounded there, the entries after it left the section they belong to. + """ + body = f"{OVERVIEW}\n{COVERED}\n\n" + entries_section( + "Previously missed (3)", + "Constructed finding 1", + "Suppressed comments (3) are dropped", + "Constructed finding 3", + ) + self.assertEqual([], pr_review.suppressed_blocks(body)) + self.assertEqual(3, pr_review.previously_missed_blocks(body)[0].count("paragraph")) + + def test_the_vetted_but_unread_check_compares_which_summaries_were_read(self) -> None: + """Compared by how many, a count of blocks stood in for which summaries they hold. + + Two sections in one wrapper are one block holding both, which read as an unread section. + A bare line read in one region matched the count of a summary no reader found in another. + """ + nested_pair = ( + f"{OVERVIEW}\n{COVERED}\n\n
\nReview details\n\n" + + entries_section("Previously missed (1)", "Constructed finding 1") + + entries_section("Previously missed (1)", "Constructed finding 2") + + "
\n" + ) + self.assertEqual(2, pr_review.previously_missed_blocks(nested_pair)[0].count("paragraph")) + self.assertEqual([], pr_review.unrecognized_in(nested_pair)) + stand_in = ( + f"{OVERVIEW}\n{COVERED}\n\nPreviously missed (2)\n\n
\n" + "Review details\n\n" + "Note: Previously missed (1)\n\n
\n" + ) + self.assertEqual( + ["summary: Previously missed (N), vetted while no reader found its section"], + pr_review.unrecognized_in(stand_in), + ) + def test_every_no_thread_render_drops_the_same_presentation_markup(self) -> None: """All of these sections come from one format, so a badge in a suppressed block buries its title exactly as one in a previously-missed block does. @@ -2034,6 +2092,19 @@ def previously_missed( ) +def entries_section(heading: str, *titles: str) -> str: + """A collapsed section under `heading`, nesting one badged entry per title. + + The nesting is the observed one, and every title, path, and sentence is constructed. + """ + found = "".join( + f"
\n{BADGE} {title}\n\n" + f"`a.py:{10 * i}`\n\nA constructed paragraph.\n
\n" + for i, title in enumerate(titles, 1) + ) + return f"
\n{heading}\n\n{found}
\n" + + def counted_missed(entries: int) -> str: """The later revision's previously-missed section, its summary opening on the count. @@ -4095,6 +4166,17 @@ def test_emphasis_markup_around_a_marker_is_dropped_rather_than_vetted_with_it(s self.assertEqual("Open (N)", pr_review.normal("Open (2)")) self.assertEqual("Review details", pr_review.normal("Review details")) + def test_the_role_a_summary_is_vetted_with_and_its_count_come_from_one_rule(self) -> None: + """Defined once for vetting and once for counting, the leading count disagreed. + + A form feed after the count still vetted the open-findings section while reading no count, + so the section's role was known and its total was not. + """ + summary = "2\fopen findings" + self.assertEqual("(N) open findings", pr_review.normal(summary)) + self.assertEqual(2, pr_review.heading_count(summary)) + self.assertEqual(2, pr_review.heading_count("### 2 previously missed issues")) + def test_the_effort_line_reads_in_either_format_s_own_spelling(self) -> None: """The first states it as a `Review details` bullet and the second as a bare bold line. From c57f7d0edb01a6616e692ea97737454bed0831d4 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 08:02:38 -0700 Subject: [PATCH 2/4] Read Each Previously Missed Section in One Wrapper as Its Own Block A section now ends at the next counted heading of either kind, its own included, so two sections sharing one wrapper are two blocks and each is counted by its own heading. Read as one block, the digest counted only the first heading's findings. The leading-count rationale moves into `spaced`'s docstring, and `counted_missed` builds its section with `entries_section`. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 52 ++++++++++++++++++++++++----------------- tests/test_pr_review.py | 39 +++++++++++++++++-------------- 2 files changed, 52 insertions(+), 39 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 827a36d8..e3d1b7e3 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -836,9 +836,6 @@ 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 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"^([#*_ ]*)([0-9]+)(?= )") # 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. @@ -3627,7 +3624,13 @@ def reduced(text: str) -> str: def spaced(text: str) -> str: - """The text `reduced`, then cut to ASCII and single spaces, which `LEADING_COUNT` reads.""" + """The text `reduced`, then cut to ASCII and single spaces, which `LEADING_COUNT` reads. + + `LEADING_COUNT` is the one leading-count rule `normal` and `heading_count` share. It reads a + count past a Markdown heading or emphasis marker, and `normal` keeps that marker in place. It + skips no tag, since a finding entry 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. + """ return re.sub(r"\s+", " ", "".join(c for c in reduced(text) if ord(c) < 128)).strip() @@ -3646,8 +3649,8 @@ def normal(text: str) -> str: icon set is rebuilt, and the text it carries is the only part a reader wants. A count opening the marker is rewritten the same way, since a later revision of the second - format writes `3 resolved since last review` where it once closed the summary on `(3)`. It is - found by `LEADING_COUNT`, the rule `heading_count` reads the count by. + format writes `3 resolved since last review` where it once closed the summary on `(3)`. + `heading_count` reads that count by the same `LEADING_COUNT`. """ return LEADING_COUNT.sub(r"\1(N)", re.sub(r"\([0-9]+\)", "(N)", spaced(text))) @@ -3689,9 +3692,8 @@ def unrecognized_in(body: str) -> list[str]: 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`. The check compares which - summaries the blocks hold rather than how many blocks there are, since one region yields one - block, and a bare `(N)` line read in one region otherwise stood in for a summary missed in - another. + summaries the blocks hold rather than how many blocks there are. Compared by count, a bare + `(N)` line read in one region stood in for a summary missed in another. 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 @@ -4475,8 +4477,8 @@ def entry_heading(line: str) -> bool: """Whether this line is a finding entry's own heading, which opens on a severity badge. A finding entry is never a section heading, whatever words its title carries. `SUPPRESSED` - matches anywhere in a line, and `PREVIOUSLY_MISSED` reads past the badge as heading markup, so - an entry titled `2 low confidence results are dropped` otherwise opened a section of its own. + matches anywhere in a line, and `PREVIOUSLY_MISSED` reads past the badge as heading markup. + Read as a heading, an entry titled `2 low confidence results are dropped` opened a section. A summary opening on a severity and stating a count is still reported by `unrecognized_in`, as an unvetted spelling. """ @@ -4485,7 +4487,7 @@ def entry_heading(line: str) -> bool: def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = False) -> list[str]: """Return the review body's sections whose own heading matches `marker`, each sliced from - that heading through to the end of its own region. + that heading through to the next sibling heading or the end of its own region. Shared by `suppressed_blocks`, `previously_missed_blocks`, and `outside_diff_blocks`. `strip_blockquote` affects heading detection only, never the returned content. A summary split @@ -4512,11 +4514,16 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F 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): + i = 0 + while i < len(scan_lines): + line = scan_lines[i] bare = COUNT.search(line) and not OPENING_COUNT.match(line) if marker.search(line) and (HEADING.match(line) or bare) and not entry_heading(line): - blocks.append("\n".join(raw_lines[i : sibling_heading(scan_lines, i, marker)])) - break + end = sibling_heading(scan_lines, i) + blocks.append("\n".join(raw_lines[i:end])) + i = end + else: + i += 1 return blocks @@ -4556,7 +4563,7 @@ def fold_summaries(scan_lines: list[str], raw_lines: list[str]) -> tuple[list[st return scan, raw -def sibling_heading(scan_lines: list[str], start: int, marker: re.Pattern[str]) -> int: +def sibling_heading(scan_lines: list[str], start: int) -> int: """Where this block ends, which is the next sibling section's heading or the region's end. Two sections sharing one wrapper is a shape the corpus carries, a Markdown heading each inside @@ -4564,6 +4571,9 @@ def sibling_heading(scan_lines: list[str], start: int, marker: re.Pattern[str]) regardless, the first section's block contains the second whole, and the digest prints those findings twice, the second time under a banner asserting the opposite triage about them. + A second section of the same kind bounds too, so each is its own block with its own count. + Read as one block, the digest counted only the first heading's findings. + A heading is not enough to tell the two apart. In this format every finding entry is its own ``, so `HEADING` matches all of them, and the suppressed marker is ordinary English that an entry's title carries as readily as a heading does. Bounded on the heading alone, a @@ -4572,10 +4582,10 @@ def sibling_heading(scan_lines: list[str], start: int, marker: re.Pattern[str]) opposite triage about them, while both counts come out wrong. A stated count is what separates them, the same signal `finding_entry` reads: every section - heading here states one and a finding's own title does not. A title can still state one, so an - entry opening on its severity badge never bounds, per `entry_heading`. Requiring it also keeps this off - the CodeRabbit blocks, whose own marker is in neither of the two and which were never bounded - before this. + heading here states one and a finding's own title does not. A title can still state one, so + an entry opening on its severity badge never bounds, per `entry_heading`. Requiring a count + also keeps this off the CodeRabbit blocks, whose own marker is in neither of the two and which + were never bounded before this. The cost is named rather than hidden. A sibling heading stating no count does not bound, so a block runs into it, which is where this started before the bound existed at all. It is strictly @@ -4589,7 +4599,7 @@ def sibling_heading(scan_lines: list[str], start: int, marker: re.Pattern[str]) continue if entry_heading(line): continue - if any(o is not marker and o.search(line) for o in SECTION_MARKERS): + if any(o.search(line) for o in SECTION_MARKERS): return j return len(scan_lines) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 107c3dcd..4dca4caa 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1587,8 +1587,8 @@ def test_a_section_ends_where_its_sibling_s_heading_starts(self) -> None: def test_a_finding_entry_is_no_section_whatever_its_title_says(self) -> None: """An entry's title is free text, and either section's marker words can appear in it. - Read as a heading, an entry in one section opened the other section at that entry, so the - finding printed under both banners, which assert opposite triage, under `shapes=ok`. + Read as a heading, an entry opened the other section at that entry. Its finding then + printed under both banners, which assert opposite triage, under `shapes=ok`. """ body = f"{OVERVIEW}\n{COVERED}\n\n" + entries_section( "Previously missed (2)", "Constructed finding 1", "2 low confidence results are dropped" @@ -1621,16 +1621,18 @@ def test_a_finding_entry_stating_a_count_does_not_end_its_section(self) -> None: def test_the_vetted_but_unread_check_compares_which_summaries_were_read(self) -> None: """Compared by how many, a count of blocks stood in for which summaries they hold. - Two sections in one wrapper are one block holding both, which read as an unread section. + Two sections in one wrapper read as one block holding both, counted by the first heading. A bare line read in one region matched the count of a summary no reader found in another. """ nested_pair = ( f"{OVERVIEW}\n{COVERED}\n\n
\nReview details\n\n" + entries_section("Previously missed (1)", "Constructed finding 1") - + entries_section("Previously missed (1)", "Constructed finding 2") + + entries_section("Previously missed (2)", "Constructed finding 2", "Constructed 3") + "
\n" ) - self.assertEqual(2, pr_review.previously_missed_blocks(nested_pair)[0].count("paragraph")) + blocks = pr_review.previously_missed_blocks(nested_pair) + self.assertEqual([1, 2], [pr_review.finding_count(b) for b in blocks]) + self.assertEqual([1, 2], [b.count("paragraph") for b in blocks]) self.assertEqual([], pr_review.unrecognized_in(nested_pair)) stand_in = ( f"{OVERVIEW}\n{COVERED}\n\nPreviously missed (2)\n\n
\n" @@ -2092,17 +2094,21 @@ def previously_missed( ) -def entries_section(heading: str, *titles: str) -> str: +def entries_section( + heading: str, *titles: str, preamble: str = "", paragraph: str = "A constructed paragraph." +) -> str: """A collapsed section under `heading`, nesting one badged entry per title. The nesting is the observed one, and every title, path, and sentence is constructed. """ found = "".join( f"
\n{BADGE} {title}\n\n" - f"`a.py:{10 * i}`\n\nA constructed paragraph.\n
\n" + f"`a.py:{10 * i}`\n\n{paragraph}\n
\n" for i, title in enumerate(titles, 1) ) - return f"
\n{heading}\n\n{found}
\n" + return ( + f"
\n{heading}\n\n{preamble}{found}
\n" + ) def counted_missed(entries: int) -> str: @@ -2114,15 +2120,13 @@ def counted_missed(entries: int) -> str: 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
" + block = entries_section( + f"{entries} previously missed {noun}", + *(f"Constructed missed finding {i}" for i in range(1, entries + 1)), + preamble="In code that hasn't changed since last review\n\n", + paragraph="[P1] A constructed paragraph about the finding.", ) + block += "\n
" return revised_with( "**No open issues** \U0001f389", "### \U0001f535 Needs a closer look" ).replace("
", block, 1) @@ -4169,8 +4173,7 @@ def test_emphasis_markup_around_a_marker_is_dropped_rather_than_vetted_with_it(s def test_the_role_a_summary_is_vetted_with_and_its_count_come_from_one_rule(self) -> None: """Defined once for vetting and once for counting, the leading count disagreed. - A form feed after the count still vetted the open-findings section while reading no count, - so the section's role was known and its total was not. + A form feed after the count vetted the open-findings section while reading no count. """ summary = "2\fopen findings" self.assertEqual("(N) open findings", pr_review.normal(summary)) From 04a1841bc68e107b1f2dbeda24110570a03a7009 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 08:07:28 -0700 Subject: [PATCH 3/4] Open a Further Section in a Region Only at Its Sibling Heading Scanning on past a section's bound read prose naming the suppressed marker beside a `(N)` as a second section. Past a region's first section, only the sibling heading it ends at can now open another, so the nested pair still splits while prose past the bound opens nothing. The `sibling_heading` docstring keeps its "the two" bound to the paragraph that names them, and its CodeRabbit sentence states what the count requirement actually does. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 30 +++++++++++++++--------------- tests/test_pr_review.py | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index e3d1b7e3..45bb2f22 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -4486,8 +4486,10 @@ def entry_heading(line: str) -> bool: def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = False) -> list[str]: - """Return the review body's sections whose own heading matches `marker`, each sliced from - that heading through to the next sibling heading or the end of its own region. + """Return the review body's sections whose own heading matches `marker`. + + Each is sliced from its heading to the next sibling heading or the end of its own region. + Past a region's first section, only the sibling heading it ends at can open another. Shared by `suppressed_blocks`, `previously_missed_blocks`, and `outside_diff_blocks`. `strip_blockquote` affects heading detection only, never the returned content. A summary split @@ -4514,16 +4516,15 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F scan_lines, raw_lines = fold_summaries(scan_lines, raw_lines) if strip_blockquote: scan_lines = [BLOCKQUOTE.sub("", ln) for ln in scan_lines] - i = 0 - while i < len(scan_lines): - line = scan_lines[i] + for i, line in enumerate(scan_lines): bare = COUNT.search(line) and not OPENING_COUNT.match(line) if marker.search(line) and (HEADING.match(line) or bare) and not entry_heading(line): - end = sibling_heading(scan_lines, i) - blocks.append("\n".join(raw_lines[i:end])) - i = end - else: - i += 1 + start = i + while start < len(scan_lines) and marker.search(scan_lines[start]): + end = sibling_heading(scan_lines, start) + blocks.append("\n".join(raw_lines[start:end])) + start = end + break return blocks @@ -4571,9 +4572,6 @@ def sibling_heading(scan_lines: list[str], start: int) -> int: regardless, the first section's block contains the second whole, and the digest prints those findings twice, the second time under a banner asserting the opposite triage about them. - A second section of the same kind bounds too, so each is its own block with its own count. - Read as one block, the digest counted only the first heading's findings. - A heading is not enough to tell the two apart. In this format every finding entry is its own ``, so `HEADING` matches all of them, and the suppressed marker is ordinary English that an entry's title carries as readily as a heading does. Bounded on the heading alone, a @@ -4584,8 +4582,10 @@ def sibling_heading(scan_lines: list[str], start: int) -> int: A stated count is what separates them, the same signal `finding_entry` reads: every section heading here states one and a finding's own title does not. A title can still state one, so an entry opening on its severity badge never bounds, per `entry_heading`. Requiring a count - also keeps this off the CodeRabbit blocks, whose own marker is in neither of the two and which - were never bounded before this. + also narrows the bound on a CodeRabbit block, whose own marker is in neither of the two. + + A second section of the same kind bounds too, so each is its own block with its own count. + Read as one block, the digest counted only the first heading's findings. The cost is named rather than hidden. A sibling heading stating no count does not bound, so a block runs into it, which is where this started before the bound existed at all. It is strictly diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 4dca4caa..2a72d9af 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1531,6 +1531,23 @@ def test_a_section_ends_where_its_count_first_sibling_starts(self) -> None: out, _ = pr_review.digest("o", "r", 7) self.assertEqual(1, out.count("The previously missed finding.")) + def test_prose_past_a_bound_opens_no_second_section(self) -> None: + """Only the sibling heading a section ends at can open another section in its region. + + Scanned on past the bound, prose naming the marker beside a `(N)` opened a second + suppressed block, which printed the previously missed finding under both banners. + """ + shared = ( + f"{OVERVIEW}\n
\nReview details\n\n" + "### Suppressed comments (1)\n\nThe suppressed finding.\n\n" + "
\n2 previously missed issues\n\n" + "A fallback treats low confidence (2) results as clean.\n
\n
\n" + ) + suppressed = pr_review.suppressed_blocks(shared) + self.assertEqual(1, len(suppressed)) + self.assertNotIn("A fallback treats", suppressed[0]) + self.assertEqual(1, len(pr_review.previously_missed_blocks(shared))) + def test_a_count_first_section_cancels_the_flagging_verdict_reading(self) -> None: """The round flags a closer look and states no open issue. From aad58012c15499a5421c151224c13fa1395c8e81 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 08:15:30 -0700 Subject: [PATCH 4/4] Qualify the Same-Kind Sibling Claim to an Adjacent Section The `sibling_heading` docstring now says a same-kind section bounds where it follows directly, which is what the chain in `marker_blocks` does. The overlong sentence in the new test's docstring is split. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 4 ++-- tests/test_pr_review.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 45bb2f22..4e3ef75d 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -4584,8 +4584,8 @@ def sibling_heading(scan_lines: list[str], start: int) -> int: an entry opening on its severity badge never bounds, per `entry_heading`. Requiring a count also narrows the bound on a CodeRabbit block, whose own marker is in neither of the two. - A second section of the same kind bounds too, so each is its own block with its own count. - Read as one block, the digest counted only the first heading's findings. + A section of the same kind that follows directly bounds too, so each is its own block with + its own count. Read as one block, the digest counted only the first heading's findings. The cost is named rather than hidden. A sibling heading stating no count does not bound, so a block runs into it, which is where this started before the bound existed at all. It is strictly diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 2a72d9af..f21cc7fb 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1535,7 +1535,7 @@ def test_prose_past_a_bound_opens_no_second_section(self) -> None: """Only the sibling heading a section ends at can open another section in its region. Scanned on past the bound, prose naming the marker beside a `(N)` opened a second - suppressed block, which printed the previously missed finding under both banners. + suppressed block. The previously missed finding then printed under both banners. """ shared = ( f"{OVERVIEW}\n
\nReview details\n\n"