From f4eb405656ca5547b7b22af1a29e4b3d4c0dc2dc Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 08:31:06 -0700 Subject: [PATCH 1/2] Read a Section Heading Through One Reduction in pr_review.py (#3179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Resolves #3174 and #3173. Both decide what counts as a section heading in `pr_review.py`, so they land together. - **One reduction, one leading-count rule (#3174).** `normal`, `unwrap`, and `heading_count` now share `reduced`, which turns a badge into its word and drops emphasis. `normal` and `heading_count` also share `spaced` and one `LEADING_COUNT`. As a result, the role a summary is vetted with and the count read from it cannot disagree. Before, `2\fopen findings` was vetted as the open-findings section while its count read as none. - **Which summaries, not how many (#3174).** `unrecognized_in` now checks which vetted previously-missed summaries appear inside the blocks it read. A bare `(N)` line read in one region can no longer stand in for a summary that no reader found in another. - **One block per section.** A section ends at the next counted heading of either kind, its own kind included. So two previously-missed sections in one wrapper are two blocks, each counted by its own heading. Past a region's first section, only the sibling heading that section ends at can open another one, so prose after a section boundary never opens a section. - **A finding entry is never a section (#3173).** An entry whose summary opens on a severity badge is never read as a section heading and never ends a section, whatever its title says. ## Verification - `python3 -m unittest test_pr_review`: 855 tests pass. All 850 existing tests pass unchanged. - Each of the 5 new tests fails against the code it fixes. The four first-commit tests fail on `develop`, and `test_prose_past_a_bound_opens_no_second_section` fails on the previous branch head. - `ruff format --check`, `ruff check`, `mypy`, `spec/validate.py`, and the commit hooks' prose and EOL gates are clean. - One report string changes, on a shape that was already unrecognized: a count-first Markdown heading such as `### 2 foo` now reports as `### (N) foo`. ## Local review findings still open The edit budget for the local strict review was spent over three recorded passes. These remain for this PR's review loop: - **introduced:** the `sibling_heading` docstring says "A second section of the same kind bounds too, so each is its own block with its own count". That holds only when the two sections are adjacent. With sections interleaved in one region (suppressed, then previously missed, then suppressed again), the third section is read by nobody. The gap itself is **pre-existing**, since `develop` read only the first section per region, and no observed body interleaves this way. - **style:** the docstring of `test_prose_past_a_bound_opens_no_second_section` holds one 26-word sentence that only the opt-in `--check sentence-length` flags. The default prose gate passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- scripts/pr_review.py | 107 ++++++++++++++++++++++++++---------- tests/test_pr_review.py | 118 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 188 insertions(+), 37 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index c79538ac..4e3ef75d 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 @@ -835,10 +836,7 @@ 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"(?:[ \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 +3614,26 @@ 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. + + `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() + + def normal(text: str) -> str: """A marker reduced toward what a vetted list compares: emphasis markup dropped, ASCII, single spaces, counts as `(N)`. @@ -3632,11 +3650,9 @@ def normal(text: str) -> str: 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)`. + `heading_count` reads that count by the same `LEADING_COUNT`. """ - 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 +3691,9 @@ 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. 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 @@ -3699,13 +3717,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 +4337,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,9 +4473,23 @@ 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. + 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. + """ + 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. + """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 @@ -4478,8 +4518,12 @@ 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): - blocks.append("\n".join(raw_lines[i : sibling_heading(scan_lines, i, marker)])) + if marker.search(line) and (HEADING.match(line) or bare) and not entry_heading(line): + 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 @@ -4520,7 +4564,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 @@ -4536,9 +4580,12 @@ 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 - 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 narrows the bound on a CodeRabbit block, whose own marker is in neither of the two. + + 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 @@ -4550,7 +4597,9 @@ 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 any(o is not marker and o.search(line) for o in SECTION_MARKERS): + if entry_heading(line): + continue + if any(o.search(line) for o in SECTION_MARKERS): return j return len(scan_lines) @@ -4571,7 +4620,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..f21cc7fb 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. The previously missed finding then printed 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. @@ -1584,6 +1601,66 @@ 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 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" + ) + 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 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 (2)", "Constructed finding 2", "Constructed 3") + + "
\n" + ) + 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" + "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 +2111,23 @@ def previously_missed( ) +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\n{paragraph}\n
\n" + for i, title in enumerate(titles, 1) + ) + return ( + f"
\n{heading}\n\n{preamble}{found}
\n" + ) + + def counted_missed(entries: int) -> str: """The later revision's previously-missed section, its summary opening on the count. @@ -2043,15 +2137,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) @@ -4095,6 +4187,16 @@ 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 vetted the open-findings section while reading no count. + """ + 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 1a511c805c8be8d513e54c22af7e90b8bd7f891b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 09:00:49 -0700 Subject: [PATCH 2/2] Pair a Review Findings Entry With Its Thread Past the Line Suffix in pr_review.py (#3181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Resolves #3176. `entry_paths` now drops a trailing `:LINE` or `:START-END` from a path a list entry names in a code span. So `` `src/a.py:340` `` pairs with a thread on `src/a.py`. Before, a `Review findings:` entry in that shape never paired, and the digest reported every such entry under `FINDINGS WITH NO THREAD`, including the ones that had opened a thread. The narrative `Open findings remain:` reader shares `entry_paths`, so it gets the same pairing. A test pins it there too. ## Verification - `python3 -m unittest test_pr_review`: all pass. - Both new cases fail on `develop` at f4eb4056: the prose subtest naming `lib/reader.py:340` and `writer.py:12-18`, and the narrative test naming `pkg/guard.py:12`. - `ruff format --check`, `ruff check`, `mypy`, and the commit hooks' prose and EOL gates are clean. - Two recorded local strict-review passes. The first raised 3 findings, all fixed. The second raised none. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- scripts/pr_review.py | 15 +++++++++++++-- tests/test_pr_review.py | 9 ++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 4e3ef75d..604913c9 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -2876,12 +2876,23 @@ def labelled_entries(body: str, label: re.Pattern[str]) -> list[str]: return entries +LINE_SUFFIX = re.compile(r":[0-9]+(?:-[0-9]+)?$") + + def entry_paths(entry: str) -> list[str]: - """The paths one list entry names, read from its own code spans.""" + """The paths one list entry names, read from its own code spans. + + A span written as `FILE:LINE` or `FILE:START-END` names the file, so the suffix is dropped. + A thread carries the path alone, which is what the entry's path is paired against. + """ return [ named for span in CODE_SPAN.finditer(entry) - if (named := bare_path(span.group()[len(span.group(1)) : -len(span.group(1))]).strip()) + if ( + named := LINE_SUFFIX.sub( + "", bare_path(span.group()[len(span.group(1)) : -len(span.group(1))]).strip() + ) + ) ] diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index f21cc7fb..8bb0fda9 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5664,9 +5664,12 @@ def test_an_entry_on_a_file_no_thread_is_on_is_counted_where_the_totals_balance( ) def test_every_entry_on_a_threaded_file_counts_none(self) -> None: - """A trailing part, a zero-width space after a slash, and a directory each name a file.""" + """A trailing part, a zero-width space after a slash, a directory, and a path carrying a + line or a line range each name a file. + """ for listed in ( PROSE_LISTED, + "- [P1] `lib/reader.py:340` drops an entry.\n- [P2] `writer.py:12-18` is stale.\n", "- `lib/\u200breader.py` and `lib/writer.py`: one issue each (2 votes each).\n", "- `lib/`: moderate retry issue (2 votes).\n", "- A retry issue\n in `lib/reader.py` (2 votes).\n", @@ -5863,6 +5866,10 @@ def test_entries_restating_threads_count_none(self) -> None: self.assertEqual(0, self.count(with_narrative(listed))) self.assertNotIn("FINDINGS WITH NO THREAD", self.digest(with_narrative(listed))) + def test_an_entry_naming_its_file_with_a_line_pairs_with_the_thread_on_it(self) -> None: + listed = "- `pkg/guard.py:12`: a failed read skips the check (**critical**, 1 vote).\n" + self.assertEqual(0, self.count(with_narrative(listed))) + def test_an_entry_stating_no_severity_this_reads_is_counted(self) -> None: for listed in ( "- `pkg/guard.py`: a failed read skips the check (1 vote).\n",