From 1c524a48d50389d243c31d0c473b36a0bba3013c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 16:35:33 -0700 Subject: [PATCH 1/3] Read Every Section of a Kind When Sections Alternate in One Wrapper in pr_review.py marker_blocks followed its chain of bounds only while each bound matched its own marker, so a suppressed section, then a previously-missed one, then a second suppressed one in the same wrapper left the third section read by nobody. The walk now visits every bound, opening a block only where the marker matches and skipping a sibling of the other kind to its own bound. Prose past a bound is still never scanned. Fixes #3180 Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 7 ++++--- tests/test_pr_review.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index e8dd3fb3..e0256edd 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -4534,7 +4534,7 @@ 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 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. + Past a region's first section, only a sibling heading some section 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 @@ -4565,9 +4565,10 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F 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): start = i - while start < len(scan_lines) and marker.search(scan_lines[start]): + while start < len(scan_lines): end = sibling_heading(scan_lines, start) - blocks.append("\n".join(raw_lines[start:end])) + if marker.search(scan_lines[start]): + blocks.append("\n".join(raw_lines[start:end])) start = end break return blocks diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 1382e7e6..cfaadddc 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1603,6 +1603,41 @@ 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_sections_alternating_in_one_wrapper_are_each_read(self) -> None: + """The chain of bounds runs through a section of the other kind rather than stopping there. + + Stopped at the first sibling of the other kind, the second suppressed section reached no + block. The digest counted one suppressed finding of three, under `shapes=ok`. + """ + shared = ( + f"{OVERVIEW}\n
\nReview details\n\n" + "### Suppressed comments (1)\n\nThe first suppressed finding.\n\n" + "### Previously missed (1)\n\nThe previously missed finding.\n\n" + "### Suppressed comments (2)\n\nThe second suppressed finding.\n\n" + "The third suppressed finding.\n
\n" + ) + suppressed = pr_review.suppressed_blocks(shared) + self.assertEqual(2, len(suppressed)) + self.assertIn("The first suppressed finding.", suppressed[0]) + self.assertIn("The third suppressed finding.", suppressed[1]) + self.assertNotIn("The previously missed finding.", "".join(suppressed)) + missed = pr_review.previously_missed_blocks(shared) + self.assertEqual(1, len(missed)) + self.assertNotIn("suppressed finding.", missed[0]) + self.answer(payload([review(body=shared)])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("suppressed=3", out) + for finding in ("first suppressed", "previously missed", "third suppressed"): + self.assertEqual(1, out.count(f"The {finding} finding.")) + swapped = ( + f"{OVERVIEW}\n
\nReview details\n\n" + "Previously missed (1)\n\nThe first.\n\n" + "Suppressed comments (1)\n\nThe second.\n\n" + "Previously missed (1)\n\nThe third.\n
\n" + ) + self.assertEqual(2, len(pr_review.previously_missed_blocks(swapped))) + self.assertEqual([], pr_review.unrecognized_in(swapped)) + 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. From fcbc3dee409b3cce730367075879d2cce9912381 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 16:39:30 -0700 Subject: [PATCH 2/3] Spell the Alternating-Sections Test in Vetted Summaries in pr_review.py The Markdown previously-missed heading the test used is no vetted spelling, so the old digest reported that body as unrecognized rather than silently undercounting it. Spelled as vetted summaries, the body is the silent shape the test describes, and it now asserts shapes=ok. Co-Authored-By: Claude Opus 5.5 --- tests/test_pr_review.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index cfaadddc..4bd5952a 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1611,9 +1611,9 @@ def test_sections_alternating_in_one_wrapper_are_each_read(self) -> None: """ shared = ( f"{OVERVIEW}\n
\nReview details\n\n" - "### Suppressed comments (1)\n\nThe first suppressed finding.\n\n" - "### Previously missed (1)\n\nThe previously missed finding.\n\n" - "### Suppressed comments (2)\n\nThe second suppressed finding.\n\n" + "Suppressed comments (1)\n\nThe first suppressed finding.\n\n" + "Previously missed (1)\n\nThe previously missed finding.\n\n" + "Suppressed comments (2)\n\nThe second suppressed finding.\n\n" "The third suppressed finding.\n
\n" ) suppressed = pr_review.suppressed_blocks(shared) @@ -1627,6 +1627,7 @@ def test_sections_alternating_in_one_wrapper_are_each_read(self) -> None: self.answer(payload([review(body=shared)])) out, _ = pr_review.digest("o", "r", 7) self.assertIn("suppressed=3", out) + self.assertIn("shapes=ok", out) for finding in ("first suppressed", "previously missed", "third suppressed"): self.assertEqual(1, out.count(f"The {finding} finding.")) swapped = ( From d8ef9608a36e511a9709e96a81f16c924d8ed2f6 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 17 Oct 2026 16:46:21 -0700 Subject: [PATCH 3/3] Assert the Second Suppressed Finding in the Alternating-Sections Test in pr_review.py The test checked the first and third suppressed findings but never the second, which shares the third's block. It now asserts the second sits in that block and prints once in the digest. Co-Authored-By: Claude Opus 5.5 --- tests/test_pr_review.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 4bd5952a..1f3a4206 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1619,6 +1619,7 @@ def test_sections_alternating_in_one_wrapper_are_each_read(self) -> None: suppressed = pr_review.suppressed_blocks(shared) self.assertEqual(2, len(suppressed)) self.assertIn("The first suppressed finding.", suppressed[0]) + self.assertIn("The second suppressed finding.", suppressed[1]) self.assertIn("The third suppressed finding.", suppressed[1]) self.assertNotIn("The previously missed finding.", "".join(suppressed)) missed = pr_review.previously_missed_blocks(shared) @@ -1628,7 +1629,12 @@ def test_sections_alternating_in_one_wrapper_are_each_read(self) -> None: out, _ = pr_review.digest("o", "r", 7) self.assertIn("suppressed=3", out) self.assertIn("shapes=ok", out) - for finding in ("first suppressed", "previously missed", "third suppressed"): + for finding in ( + "first suppressed", + "previously missed", + "second suppressed", + "third suppressed", + ): self.assertEqual(1, out.count(f"The {finding} finding.")) swapped = ( f"{OVERVIEW}\n
\nReview details\n\n"