Skip to content
2 changes: 1 addition & 1 deletion scripts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ The digest reports the **previously missed findings** the second body format col

Copilot's reviews arrive in two body formats. None of the ten bodies read here in the second carried a `Suppressed comments` heading, so the reading above found nothing in it, and that format states its own finding total instead, as a count per severity that is summed across the badges the line pairs them with, since reading its first number alone reported two findings for a round stating two of one severity and three of another. The markup is masked to one sentinel before any count is read, a badge carrying digits of its own in its attributes, and a run of more than four digits is an identifier rather than a count, which is the second guard under a mask that can terminate early on a bracket a URL carries itself. Every count the mask left is then summed, rather than each being paired with markup beside it, since which side of a count that markup sits on and whether the severity carries any is presentation rather than structure: a bold split, a bare prose split, and a reference-style link each read as one severity under a paired reading. Summing overstates and never understates, which is the direction chosen deliberately, and its named cost is that a line stating a total and then its context, `12 findings across 4 files`, sums to sixteen. A shortfall overstated prints a block a reader checks against the body and blocks no merge, where one suppressed closes the gate on findings nobody answered. `overview=T/M` reports that total beside the number of review threads the round actually opened, `?` for `T` where no total is found in the overview preamble or a count-first open-findings section, since a total absent and a total of zero are different readings. A `?` covers a round stating no total, a round stating one only after its first collapsed section in anything but the open-findings section described below, and a round stating one as a bullet, that last blocking at exit 43 as an unvetted metadata label. None of them says the round withheld nothing. `M` comes from the API rather than the review's prose: that format enumerates the findings it opened threads for, and the anchor each entry links is the database id of that thread's own comment, so the threads are the same set already structured. Reading the enumeration instead took six attempts and four of them cancelled a shortfall by counting something that was not an entry, which needs a Markdown parser to tell apart rather than a line scan. A total larger than the thread count is findings raised where polling threads cannot see them, and a `FINDINGS WITH NO THREAD` block follows naming the shortfall, except where a count-first open section supplied the total, as the next paragraph says. A thread past the hundred `reviewThreads` reads would count in `M` too, so a cut page overstates the shortfall and `threads=` carries the trailing `+` that says so. The comparison holds while the round's enumeration lists what that round raised, and a format listing findings still open from earlier rounds states a total `M` undercounts, which is a shortfall to confirm against the body rather than to act on from the number. The field is present only where the round covering the head is written in that format, and it is head-scoped where `suppressed=` is not, because a total is one round's statement about one diff and the push that changes that diff raises a round stating its own, where a suppressed block carries the finding text an answer is still owed to.

One reader, `read_overview`, reads every section of that format's overview. Each section it added arrived as a spelling nothing read, and each stopped the loop at exit `43` in turn. The `OVERVIEW_SECTIONS` table in the script names each collapsible section that lists thread links, with its role. The open-findings section states the later revision's total, since that revision writes the bare zero line only for a zero. So where the preamble states no total, that section's count is `T`. `Open (N)` is not read as a total, since it listed an earlier round's thread in 2 of the 41 rounds measured. Each open-section entry links the database id of its thread's first comment. An entry linking no thread on the pull request is a finding named only in the body. `FINDINGS WITH NO THREAD` counts it whatever the totals say. Where that section supplied `T`, the block is measured against its entries rather than against `M`. So an entry linking an earlier round's thread is no finding without one, while a finding the count names and no entry links is still counted. A resolved section's entries link threads an earlier round raised, so they are read as information rather than findings.
One reader, `read_overview`, reads every section of that format's overview. Each section it added arrived as a spelling nothing read, and each stopped the loop at exit `43` in turn. The `OVERVIEW_SECTIONS` table in the script names each collapsible section that lists thread links, with its role. The open-findings section states the later revision's total, since that revision writes the bare zero line only for a zero. So where the preamble states no total, that section's count is `T`. `Open (N)` is not read as a total, since it listed an earlier round's thread in 2 of the 41 rounds measured. Each open-section entry links the database id of its thread's first comment. An entry linking no thread on the pull request is a finding named only in the body. `FINDINGS WITH NO THREAD` counts it whatever the totals say. It also counts each entry an `Open (N)` section counts beyond those it lists with a link. A linked entry is a bullet at the section's margin carrying a link this reader recognizes. A bullet inside any block collapsed within the section is not one. Any other entry counts here too. Where the open-findings section supplied `T`, the block is measured against its entries rather than against `M`. So an entry linking an earlier round's thread is no finding without one. A finding that section's count names and no entry links is still counted. A resolved section's entries link threads an earlier round raised, so they are read as information rather than findings.

`status` and `wait` both exit `50` where the head's round opens on a verdict other than the clean one and counts no finding. It states none, lists none in an open section, opened no thread, and collapsed none, and no reviewer's thread is open. Every count reads that round as a clean pass, so the digest prints its headline under `VERDICT WITH NO COUNTED FINDING`. Over the rounds read here in that format, most such headlines named a finding and the rest a caution about the change. Only reading the headline tells the two apart, so it takes the triage a suppressed finding gets, fixed or answered with `comment`. The code repeats on that head after the answer, since nothing here reads one. An open thread defers the reading rather than cancelling it, since the headline may name that thread. The code ranks under `42`, `43`, and `45`, a part-reviewed diff being the larger gap. It outranks `wait`'s `44`, so on such a head a stuck check shows only in the digest.

Expand Down
61 changes: 46 additions & 15 deletions scripts/pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,13 @@
Where the preamble states no total, the count opening the open-findings section is
`T`, since that format's later revision writes the bare zero line only for a zero.
Each entry an open section lists links its thread, and an entry linking no thread on
the pull request is counted in that same block whatever the totals say. Where that
section supplied `T`, the block is measured against its entries rather than against
`M`, so an entry linking an earlier round's thread is no finding without one, while a
finding the count names and no entry links is still counted.
the pull request is counted in that same block whatever the totals say. So is each
entry an `Open (N)` section counts beyond its linked entries, the bullets at its
margin carrying a link this script reads, outside any block collapsed within it.
Where the open-findings section supplied `T`, the block is measured against its
entries rather than against `M`, so an entry linking an earlier round's thread is no
finding without one, while a finding that section's count names and no entry links is
still counted.
Ten bodies in that format have been read here, so what follows describes those rather
than the format in general. None carried a `Suppressed comments` heading, which is
why this field rather than `suppressed=` is what finds a withheld finding in one.
Expand Down Expand Up @@ -1946,7 +1949,8 @@ def findings_on(tail: str) -> int | None:


DISCUSSION_LINK = re.compile(r"\]\(#discussion_r(\d+)\)")
OverviewSection = tuple[str, int | None, list[str]]
ENTRY_LINE = re.compile(r"[-*+]\s")
OverviewSection = tuple[str, int | None, list[str], int]


def read_overview(body: str) -> tuple[str, str, list[OverviewSection]]:
Expand All @@ -1961,9 +1965,12 @@ def read_overview(body: str) -> tuple[str, str, list[OverviewSection]]:
is the prose under it up to the next heading or bold line, taken from the body unmasked so a
code span the headline names survives. Each section is a top-level
`<details>` whose own summary is a row of `OVERVIEW_SECTIONS`, as its role, the count that
summary states, and the thread id each of its entries links, which is the database id of that
thread's first comment. Quotations are masked first, so a body quoting a section is not read
as carrying one.
summary states, the thread id each of its entries links, which is the database id of that
thread's first comment, and its linked entry count, the bullets at its margin carrying a link
this reader recognizes, outside any block collapsed within it. One entry can link a thread
beside its own, hold an indented back-reference, or collapse a list of its own, so a section's
links overstate its linked entries. Quotations are masked first, so a body quoting a section
is not read as carrying one.

An open section's entries are findings, each checked against the threads, and a resolved
section's are threads an earlier round raised. The count-first open section also states the
Expand Down Expand Up @@ -1994,7 +2001,14 @@ def read_overview(body: str) -> tuple[str, str, list[OverviewSection]]:
continue
digits = re.search(r"\d+", EMPHASIS.sub("", summary))
count = counted(digits.group()) if digits else None
sections.append((role, count, DISCUSSION_LINK.findall(masked[start:end])))
text = masked[start:end]
own = "\n".join(text[a:b] for a, b in details_spans(tags[start:end])[1])
linked = sum(
1
for line in own.splitlines()
if ENTRY_LINE.match(line) and DISCUSSION_LINK.search(line)
)
sections.append((role, count, DISCUSSION_LINK.findall(text), linked))
return verdict, " ".join(headline), sections


Expand Down Expand Up @@ -2028,7 +2042,7 @@ def stated_total(body: str, preamble_only: bool = False) -> int | None:
totals += [counted(m.group(1)) for m in CCR_OPEN_FINDINGS.finditer(preamble)]
found = [t for t in totals if t is not None]
if not found and not preamble_only:
found = [c for role, c, _ in read_overview(body)[2] if role == TOTAL and c is not None]
found = [c for role, c, *_ in read_overview(body)[2] if role == TOTAL and c is not None]
return max(found, default=None)


Expand Down Expand Up @@ -2111,13 +2125,29 @@ def unthreaded_entries(pr: dict) -> int:
a finding named only in the review body, which no thread poll reaches. Distinct ids are
counted, and a resolved section's entries are not, being threads an earlier round raised. A
thread past the hundred the query reads counts here too, overstating rather than hiding.
An `Open (N)` section counting more entries than it has linked entries adds the difference.
An entry carrying a link this reader does not recognize lands there too, overstating rather
than hiding.
"""
known = {
str(first_comment(thread)["fullDatabaseId"])
for thread in ((pr.get("reviewThreads") or {}).get("nodes") or [])
if first_comment(thread).get("fullDatabaseId") is not None
}
return len(open_entry_ids(pr) - known)
return len(open_entry_ids(pr) - known) + unlinked_open_entries(pr)


def unlinked_open_entries(pr: dict) -> int:
"""How many entries the head round's `Open (N)` sections count beyond their linked entries."""
newest = second_format_head(pr)
if newest is None:
return 0
sections = read_overview(newest.get("body") or "")[2]
return sum(
max(count - linked, 0)
for role, count, _, linked in sections
if role == OPEN and count is not None
)


def open_entry_ids(pr: dict) -> set[str]:
Expand All @@ -2126,7 +2156,7 @@ def open_entry_ids(pr: dict) -> set[str]:
if newest is None:
return set()
sections = read_overview(newest.get("body") or "")[2]
return {i for role, _, ids in sections if role != RESOLVED for i in ids}
return {i for role, _, ids, _ in sections if role != RESOLVED for i in ids}


def uncounted_verdict(pr: dict) -> tuple[str, str] | None:
Expand All @@ -2149,7 +2179,7 @@ def uncounted_verdict(pr: dict) -> tuple[str, str] | None:
verdict, headline, sections = read_overview(body)
if not verdict or unvetted(verdict, FLAGGING_VERDICTS):
return None
listed = any(role != RESOLVED and (count or ids) for role, count, ids in sections)
listed = any(role != RESOLVED and (count or ids) for role, count, ids, _ in sections)
if listed or stated_total(body) or round_threads(pr, newest):
return None
if suppressed_blocks(body) or previously_missed_blocks(body):
Expand Down Expand Up @@ -3736,8 +3766,9 @@ def digest(
else f" and opened {listed} thread{'' if listed == 1 else 's'}"
)
+ (
f", and {unthreaded} of the entries its open section lists "
f"link{'s' if unthreaded == 1 else ''} no thread on this pull request"
f", and {unthreaded} of the entries its open sections count "
f"{'carries' if unthreaded == 1 else 'carry'} no link this script reads to a "
"thread on this pull request"
if unthreaded
else ""
)
Expand Down
83 changes: 80 additions & 3 deletions tests/test_pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -4218,11 +4218,88 @@ def test_an_unthreaded_entry_under_no_stated_total_says_so(self) -> None:
out, _ = pr_review.digest("o", "r", 7)
self.assertIn("overview=?/0", out)
self.assertIn(
"states no total and opened 0 threads, and 1 of the entries its open section lists "
"links no thread",
"states no total and opened 0 threads, and 1 of the entries its open sections count "
"carries no link this script reads to a thread",
out,
)

def test_an_open_list_entry_carrying_no_link_is_counted(self) -> None:
"""`Open (N)` is not a total, so an entry it counts with no thread link is counted here."""
linked = "- [Finding 1](#discussion_r4000000001) New"
body = overview_v2(findings="", entries=1)
self.assertIn(linked, body)
self.assertIn("Open (1)", body)
body = body.replace("Open (1)", "Open (2)").replace(linked, f"{linked}\n- Finding 2 New")
self.answer(payload([review(body=body)], [thread("T1", cid="4000000001")]))
out, _ = pr_review.digest("o", "r", 7)
self.assertIn("FINDINGS WITH NO THREAD (1)", out)
self.assertIn("open sections count carries no link this script reads to a thread", out)

def test_a_second_link_on_one_entry_does_not_cover_an_unlinked_entry(self) -> None:
"""An entry linking a thread beside its own is still one entry, so the unlinked one counts."""
linked = "- [Finding 1](#discussion_r4000000001) New"
body = overview_v2(findings="", entries=1)
self.assertIn(linked, body)
body = body.replace("Open (1)", "Open (2)").replace(
linked, f"{linked}, see [earlier](#discussion_r4000000007)\n- Finding 2 New"
)
threads = [thread("T1", cid="4000000001"), thread("T7", cid="4000000007")]
self.answer(payload([review(body=body)], threads))
out, _ = pr_review.digest("o", "r", 7)
self.assertIn("FINDINGS WITH NO THREAD (1)", out)

def test_an_indented_back_reference_does_not_cover_an_unlinked_entry(self) -> None:
"""A back-reference on its own line under an entry is not another entry."""
linked = "- [Finding 1](#discussion_r4000000001) New"
body = overview_v2(findings="", entries=1)
self.assertIn(linked, body)
body = body.replace("Open (1)", "Open (2)").replace(
linked, f"{linked}\n - see [earlier](#discussion_r4000000007)\n- Finding 2 New"
)
threads = [thread("T1", cid="4000000001"), thread("T7", cid="4000000007")]
self.answer(payload([review(body=body)], threads))
out, _ = pr_review.digest("o", "r", 7)
self.assertIn("FINDINGS WITH NO THREAD (1)", out)

def test_a_bullet_collapsed_within_an_entry_does_not_cover_an_unlinked_entry(self) -> None:
"""A list collapsed inside an entry holds back-references rather than entries."""
linked = "- [Finding 1](#discussion_r4000000001) New"
body = overview_v2(findings="", entries=1)
self.assertIn(linked, body)
nested = (
"<details><summary>Related</summary>\n\n"
"- see [earlier](#discussion_r4000000007)\n\n</details>\n"
)
body = body.replace("Open (1)", "Open (2)").replace(
linked, f"{linked}\n{nested}- Finding 2 New"
)
threads = [thread("T1", cid="4000000001"), thread("T7", cid="4000000007")]
self.answer(payload([review(body=body)], threads))
out, _ = pr_review.digest("o", "r", 7)
self.assertIn("FINDINGS WITH NO THREAD (1)", out)

def test_more_linked_entries_than_counted_cancel_no_unthreaded_entry(self) -> None:
"""A section counting fewer entries than it links still counts an id no thread carries."""
linked = "- [Finding 1](#discussion_r4000000001) New"
body = overview_v2(findings="", entries=1)
self.assertIn(linked, body)
body = body.replace(linked, f"{linked}\n- [Finding 7](#discussion_r4000000007) New")
self.answer(payload([review(body=body)], [thread("T7", cid="4000000007")]))
out, _ = pr_review.digest("o", "r", 7)
self.assertIn("FINDINGS WITH NO THREAD (1)", out)

def test_every_bullet_marker_reads_as_a_linked_entry(self) -> None:
"""`*` and `+` open an entry as `-` does, so a list in either is fully linked."""
for marker in ("*", "+"):
with self.subTest(marker=marker):
linked = "- [Finding 1](#discussion_r4000000001) New"
body = overview_v2(findings="", entries=1)
self.assertIn(linked, body)
body = body.replace(linked, f"{marker}{linked[1:]}")
self.answer(payload([review(body=body)], [thread("T1", cid="4000000001")]))
out, _ = pr_review.digest("o", "r", 7)
self.assertNotIn("FINDINGS WITH NO THREAD", out)

def test_an_entry_linking_no_thread_is_counted_where_the_totals_balance(self) -> None:
"""The round opened as many threads as it states, and one entry links none of them, so
the totals alone pass a finding named only in the body."""
Expand All @@ -4235,7 +4312,7 @@ def test_an_entry_linking_no_thread_is_counted_where_the_totals_balance(self) ->
out, _ = pr_review.digest("o", "r", 7)
self.assertIn("overview=2/2", out)
self.assertIn("FINDINGS WITH NO THREAD (1)", out)
self.assertIn("and 1 of the entries its open section lists links no thread", out)
self.assertIn("open sections count carries no link this script reads to a thread", out)

def test_a_resolved_section_s_entries_are_not_findings(self) -> None:
"""Its entries link threads an earlier round raised, so none needs a thread of its own."""
Expand Down
Loading