diff --git a/scripts/pr_review.py b/scripts/pr_review.py index c79538ac..604913c9 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. @@ -2878,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() + ) + ) ] @@ -3616,6 +3625,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 +3661,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 +3702,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 +3728,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 +4348,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 +4484,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 +4529,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 +4575,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 +4591,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 +4608,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 +4631,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..8bb0fda9 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. @@ -5562,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", @@ -5761,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",