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.