Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .agents/skills/pr-review-conduct/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,9 @@ visible comments, routinely still carries a finding nobody has answered. Treatin
`T` reads `?` where no total was found, which is a round stating none and equally one the digest
could not locate, so a `?` leaves this item unsatisfied and sends you to the body exactly as a
shortfall does. The digest's `FINDINGS WITH NO THREAD` block also counts an open-findings entry
linking no thread, whatever the totals say. A round with a verdict other than the clean one
that counts no finding prints its headline under `VERDICT WITH NO COUNTED FINDING`, and
`status` and `wait` exit `50`. The
whose title links no thread, whatever the totals say. A round with a verdict other than the
clean one that counts no finding prints its headline under `VERDICT WITH NO COUNTED FINDING`,
and `status` and `wait` exit `50`. The
headline is the one place that round names what it flags, so answer it as a suppressed finding.
The exit code repeats on that head after the answer.
The body read in that format so far carried no `Suppressed comments` heading, so `suppressed=`
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
7d6c9820b5e14bd8
7407d2839e84376f
6 changes: 3 additions & 3 deletions .claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,9 @@ visible comments, routinely still carries a finding nobody has answered. Treatin
`T` reads `?` where no total was found, which is a round stating none and equally one the digest
could not locate, so a `?` leaves this item unsatisfied and sends you to the body exactly as a
shortfall does. The digest's `FINDINGS WITH NO THREAD` block also counts an open-findings entry
linking no thread, whatever the totals say. A round with a verdict other than the clean one
that counts no finding prints its headline under `VERDICT WITH NO COUNTED FINDING`, and
`status` and `wait` exit `50`. The
whose title links no thread, whatever the totals say. A round with a verdict other than the
clean one that counts no finding prints its headline under `VERDICT WITH NO COUNTED FINDING`,
and `status` and `wait` exit `50`. The
headline is the one place that round names what it flags, so answer it as a suppressed finding.
The exit code repeats on that head after the answer.
The body read in that format so far carried no `Suppressed comments` heading, so `suppressed=`
Expand Down
6 changes: 3 additions & 3 deletions .github/skills/pr-review-conduct/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,9 @@ visible comments, routinely still carries a finding nobody has answered. Treatin
`T` reads `?` where no total was found, which is a round stating none and equally one the digest
could not locate, so a `?` leaves this item unsatisfied and sends you to the body exactly as a
shortfall does. The digest's `FINDINGS WITH NO THREAD` block also counts an open-findings entry
linking no thread, whatever the totals say. A round with a verdict other than the clean one
that counts no finding prints its headline under `VERDICT WITH NO COUNTED FINDING`, and
`status` and `wait` exit `50`. The
whose title links no thread, whatever the totals say. A round with a verdict other than the
clean one that counts no finding prints its headline under `VERDICT WITH NO COUNTED FINDING`,
and `status` and `wait` exit `50`. The
headline is the one place that round names what it flags, so answer it as a suppressed finding.
The exit code repeats on that head after the answer.
The body read in that format so far carried no `Suppressed comments` heading, so `suppressed=`
Expand Down
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. A count-first open section supplying the total is one exception, as the next paragraph says. Open entries standing for an earlier round's threads are the other, as stated below. 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 total counts what is still open, not only what the round raised. So a round can state findings an earlier round raised and left open. An open entry whose title links an earlier round's thread, and which links none the round opened, is counted beside `M` first. Where a block still prints, it names how many it counted that way. A carried finding no entry links still reads as a shortfall. Confirm that one against the body rather than acting on 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. 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 the entries it lists with a link 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. Entries are counted rather than links, so a back-reference beside an entry's own link covers no other entry. 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 whose title links 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 the entries it lists with a link 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. Entries are counted rather than links, so a back-reference beside an entry's own link covers no other entry. 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
20 changes: 11 additions & 9 deletions scripts/pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,9 @@
the reader's to judge.
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. So is each
entry an `Open (N)` section counts beyond its linked entries, the bullets at its
Each entry an open section lists links its thread, and an entry whose title links no
thread on 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 the
entries it lists with a link rather than against `M`, so an entry linking an earlier
Expand Down Expand Up @@ -2144,9 +2144,11 @@ def unthreaded_entries(pr: dict) -> int:
"""How many findings the head round's open sections link to no thread on this pull request.

Each entry links the database id of its thread's first comment, so an id no thread carries is
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.
a finding named only in the review body, which no thread poll reaches. An entry is read by its
first link, its title's own anchor, as `carried_open_threads` reads it, so a back-reference
beside a title anchor that is a thread adds nothing. Distinct title anchors are counted, and a
resolved section's entries are not, being threads an earlier round raised. A title anchor
past the hundred threads 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.
Expand Down Expand Up @@ -2227,12 +2229,12 @@ def total_section_entries(pr: dict) -> int:


def open_entry_ids(pr: dict) -> set[str]:
"""The distinct thread ids the head round's open sections link, resolved sections left out."""
"""The distinct title anchors the head round's open entries link, resolved sections left out."""
newest = second_format_head(pr)
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 entry in ids for i in entry}
return {entry[0] for role, _, ids, _ in sections if role != RESOLVED for entry in ids}


PROSE_FINDINGS = re.compile(
Expand Down Expand Up @@ -4017,7 +4019,7 @@ def digest(
)
+ (
f", and {unthreaded} of the entries its open sections count "
f"{'carries' if unthreaded == 1 else 'carry'} no link this script reads to a "
f"{'has' if unthreaded == 1 else 'have'} no title link this script reads to a "
"thread on this pull request"
if unthreaded
else ""
Expand Down
30 changes: 27 additions & 3 deletions tests/test_pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -4417,7 +4417,7 @@ def test_an_unthreaded_entry_under_no_stated_total_says_so(self) -> None:
self.assertIn("overview=?/0", out)
self.assertIn(
"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",
"has no title link this script reads to a thread",
out,
)

Expand All @@ -4431,7 +4431,7 @@ def test_an_open_list_entry_carrying_no_link_is_counted(self) -> None:
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)
self.assertIn("open sections count has no title 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."""
Expand Down Expand Up @@ -4526,7 +4526,31 @@ 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("open sections count carries no link this script reads to a thread", out)
self.assertIn("open sections count has no title link this script reads to a thread", out)

def back_referenced(self, cid: str) -> dict:
"""One open entry whose title anchors 6000000001 beside a back-reference to 6000000009,
on a pull request whose one thread read carries `cid`."""
title = "(#discussion_r6000000001) \u00b7 New"
body = revised_with(open_section(("6000000001",)))
self.assertIn(title, body)
body = body.replace(title, f"{title}, see [earlier](#discussion_r6000000009)")
return payload([review(body=body, rid="PRR_head")], [thread("T1", rid="PRR_head", cid=cid)])

def test_a_back_reference_beside_a_threaded_title_adds_no_unthreaded_entry(self) -> None:
"""The entry is read by its title anchor, which is a thread, so a back-reference to an id
no thread read carries, such as one past the hundred threads read, adds nothing."""
pr = self.back_referenced("6000000001")
self.assertEqual(0, pr_review.unthreaded_entries(pr))
self.answer(pr)
out, _ = pr_review.digest("o", "r", 7)
self.assertNotIn("FINDINGS WITH NO THREAD", out)

def test_a_threaded_back_reference_beside_an_unthreaded_title_cancels_nothing(self) -> None:
"""A title anchor naming no thread is still one entry with no thread, whatever thread a
back-reference beside it names."""
pr = self.back_referenced("6000000009")
self.assertEqual(1, pr_review.unthreaded_entries(pr))

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