Count Every Unresolved Review Thread in pr_review.py

scripts/README.md

@@ -182,7 +182,7 @@ python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout
 
 **Do not infer one field's spelling from another's.** Several states print upper-case, `merge=` reproduces GitHub's own enum verbatim, and a field can arrive behind a prefix, so a pattern built by analogy with a neighboring field silently never fires. `review_on_head` is the one that catches a watcher, rendering `yes` against `NO`, so a pattern written for `YES` waits out its whole bound over a review that landed. A watcher that cannot fire reads exactly like a review that has not landed, so run the command once, read a real line, and write the pattern against that. Anchor the pattern on what is unique to the field being read, meaning that field's own name with its value, and for a pair nested in a parenthesized group the group's name ahead of it, since values repeat across fields and the groups carry the same inner names as each other. A watcher testing for a bare negative matches a line a covered review wrote, so it reports no review over a review that landed and waits out its whole bound saying so. That one matches on the wrong evidence rather than failing to match, which is worse than the casing trap above, since its answer is the digest's own inverted rather than absent. Give the watcher a branch that exits non-zero where the digest cannot be read at all, or a pattern that never fires and a command that never ran are equally silent.
 
-`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the four mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, and `resolveReviewThread`, so a fifth arriving is a write nobody reviewed as one.
+`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, an unknown author's open thread left uncounted, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the four mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, and `resolveReviewThread`, so a fifth arriving is a write nobody reviewed as one.
 
 `wait` exits `64` when the target is out of scope, the same refusal `comment` and `reply` raise, checked before either the auto-request or the backoff loop reaches GitHub. The `reply` paragraph below states what that refusal covers.
 

scripts/pr_review.py

@@ -95,11 +95,12 @@
            file-count refusal is spent by coverage of the same head. `wait` is where that state gets
            its own exit codes, 46 and 47 below, because only `wait` is the command a caller might
            otherwise poll out a timeout on.
-           `unresolved` counts every tracked reviewer's own open thread, not only Copilot's:
-           CodeRabbit (`coderabbitai`) and qodo (`qodo-free-for-open-source-projects`) are
-           tracked at the identity and thread-resolution level, since an open thread blocks a
-           ruleset-gated merge whoever opened it and `unresolved=0` once hid one of theirs that
-           still did.
+           `unresolved` counts every open thread whoever opened it, since an open thread blocks
+           a ruleset-gated merge whatever its author, and a filter on known reviewer logins
+           once read `unresolved=0` over a qodo thread posted under a login it did not know.
+           The known logins, Copilot, CodeRabbit (`coderabbitai`), and qodo
+           (`qodo-free-for-open-source-projects`), only attribute a thread in the breakdown
+           beside the count, where any other author reads as `other`.
            Both `threads=` and `unresolved=` are read from a single 100-thread page with no
            further pagination, so a pull request carrying more than that undercounts silently
            past that point: both fields print a trailing `+` and a `THREADS TRUNCATED` block
@@ -332,8 +333,6 @@
 # Other review bots this repository has trialed alongside Copilot.
 # Tracked at the identity level only, login and commit oid, never body prose, except where a reader below names one explicitly.
 # Each format read here is its own reader, and doing that well is a separate task per bot.
-# What generalizes without reading any of their prose is thread resolution.
-# An open thread blocks a ruleset-gated merge whoever opened it, and `status`'s `unresolved=0` once silently hid a CodeRabbit/qodo thread that did block one.
 CODERABBIT_LOGIN = "coderabbitai"
 QODO_LOGIN = "qodo-free-for-open-source-projects"
 # Named rather than inlined at each of their own readers below, so a login rename updates one spelling instead of silently leaving a hardcoded copy matching nothing.
@@ -1304,7 +1303,7 @@ def threads_truncated(pr: dict) -> bool:
     open. That inversion is why this is its own guard rather than a second use of `window_blind`:
     that one settles the question from what is already in view, and there is no such settling
     available here, only the fact that something was cut, undercounted ever since `unresolved`
-    widened from Copilot's own threads to every tracked reviewer's.
+    widened from Copilot's own threads to every open thread.
     """
     return bool(((pr.get("reviewThreads") or {}).get("pageInfo") or {}).get("hasNextPage"))
 
@@ -2338,14 +2337,18 @@ def uncounted_verdict(pr: dict) -> tuple[str, str] | None:
         return None
     if suppressed_blocks(body) or previously_missed_blocks(body):
         return None
-    if open_reviewer_threads((pr.get("reviewThreads") or {}).get("nodes") or []):
+    if open_threads((pr.get("reviewThreads") or {}).get("nodes") or []):
         return None
     return verdict, headline
 
 
-def open_reviewer_threads(threads: list[dict]) -> list[dict]:
-    """The unresolved threads a known reviewer opened, which block a ruleset-gated merge."""
-    return [t for t in threads if not t.get("isResolved") and thread_author(t) in KNOWN_REVIEWERS]
+def open_threads(threads: list[dict]) -> list[dict]:
+    """Every unresolved thread, whoever opened it, since each one blocks a ruleset-gated merge.
+
+    The author decides only how `status` attributes a thread, never whether it counts, because a
+    login set read off past history misses the next spelling a reviewer posts under.
+    """
+    return [t for t in threads if not t.get("isResolved")]
 
 
 def unlisted_findings(manifest: tuple[int | None, int] | None) -> int:
@@ -3794,20 +3797,15 @@ def digest(
     truncated = threads_truncated(pr)
     # Same reasoning, the `reviews` connection rather than `reviewThreads`, feeding `suppressed=` and `cr_outside_diff=` below.
     revs_truncated = reviews_truncated(pr)
-    # Any known reviewer's own thread, not only Copilot's.
-    # An open thread blocks a ruleset-gated merge whoever opened it, and counting Copilot's alone hid a CodeRabbit/qodo thread that did block one.
-    # `thread_author` carries the deleted-account default this needs.
-    unresolved = open_reviewer_threads(threads)
-    # A breakdown beside the raw count, but only where more than one reviewer contributes to it.
-    # A single reviewer's own count is what `unresolved=N` already meant before this generalized.
-    # Printing one name beside its own total says nothing the number did not already say.
+    unresolved = open_threads(threads)
     by_login = {
         login: sum(1 for t in unresolved if thread_author(t) == login) for login in KNOWN_REVIEWERS
     }
+    by_login["other"] = sum(1 for t in unresolved if thread_author(t) not in KNOWN_REVIEWERS)
     contributors = {login: n for login, n in by_login.items() if n}
     breakdown = (
         " (" + " ".join(f"{login}={n}" for login, n in contributors.items()) + ")"
-        if len(contributors) > 1
+        if len(contributors) > 1 or "other" in contributors
         else ""
     )
 
@@ -4379,7 +4377,7 @@ def unresolved_threads(owner: str, repo: str, num: int) -> list[dict]:
         conn = gh_graphql(Q_THREADS, o=owner, r=repo, n=num, **extra)["repository"]["pullRequest"][
             "reviewThreads"
         ]
-        out += [t for t in conn["nodes"] if not t["isResolved"]]
+        out += open_threads(conn["nodes"])
         page = conn.get("pageInfo") or {}
         if not page.get("hasNextPage"):
             return out

tests/test_pr_review.py

@@ -736,21 +736,26 @@ def test_digest_reports_effective_effort_without_changing_the_verdict(self) -> N
         self.assertIn("coverage=full", out)
 
     def test_a_thread_from_a_deleted_account_does_not_crash_the_digest(self) -> None:
-        """GraphQL sends `author` present and null, which a defaulted lookup returns as None."""
+        """GraphQL sends `author` present and null, which a defaulted lookup returns as None.
+
+        The thread still blocks the merge, so it counts, attributed as `other`.
+        """
         orphan = thread("T1")
         orphan["comments"]["nodes"][0]["author"] = None
         self.answer(payload([review()], [orphan, thread("T2")]))
         out, unresolved = pr_review.digest("o", "r", 7)
-        self.assertEqual(1, unresolved)
+        self.assertEqual(2, unresolved)
+        self.assertIn(f"unresolved=2 ({pr_review.REVIEWER}=1 other=1)", out)
+        self.assertIn("T1", out)
         self.assertIn("T2", out)
 
-    def test_only_the_reviewer_s_own_unresolved_threads_are_listed(self) -> None:
-        """A maintainer's own open thread is not a review finding to answer."""
+    def test_every_open_thread_is_listed_whoever_opened_it(self) -> None:
+        """An open thread blocks a ruleset-gated merge whatever its author."""
         self.answer(payload([review()], [thread("T1", login="ptr727"), thread("T2")]))
         out, unresolved = pr_review.digest("o", "r", 7)
-        self.assertEqual(1, unresolved)
+        self.assertEqual(2, unresolved)
+        self.assertIn("T1", out)
         self.assertIn("T2", out)
-        self.assertNotIn("T1", out)
 
     def test_a_seen_set_marks_each_thread_new_exactly_once(self) -> None:
         """The seen set is what tells a second round's findings from the ones already answered."""
@@ -784,10 +789,11 @@ class TestOtherReviewers(GqlCase):
     in `TestCodeRabbitOutsideDiff` and `TestQodoOpenFindings` below, not here.
 
     `status`'s `unresolved=0` used to hide a CodeRabbit/qodo thread that still blocked a
-    ruleset-gated merge, since only Copilot's own threads
-    counted. Coverage and refusal reading stay Copilot-only: `review_on_head` above names
-    Copilot's own coverage specifically, the reviewer this script requests and waits for, not
-    "no review of any kind covers this head".
+    ruleset-gated merge, first since only Copilot's own threads counted and then since only known
+    reviewer logins did, so every open thread now counts and a login only attributes it. Coverage
+    and refusal reading stay Copilot-only: `review_on_head` above names Copilot's own coverage
+    specifically, the reviewer this script requests and waits for, not "no review of any kind
+    covers this head".
     """
 
     def other_review(self, login: str, oid: str = HEAD, body: str = "") -> dict:
@@ -818,14 +824,34 @@ def test_a_coderabbit_and_a_qodo_thread_both_count_toward_unresolved(self) -> No
         self.assertIn("T2", out)
         self.assertNotIn("T3", out)
 
-    def test_a_human_thread_still_does_not_count(self) -> None:
-        """Generalizing past Copilot means the other tracked bots, not every account."""
-        self.answer(payload([review()], [thread("T1", login="ptr727")]))
+    def test_a_thread_from_an_unknown_login_counts_and_is_named_other(self) -> None:
+        """A reviewer posting under a login missing from the known set once read `unresolved=0`.
+
+        The known logins attribute a thread and never decide whether it counts, and `other` is
+        named even where it is the only contributor, since it is the count a reader cannot assume.
+        """
+        self.answer(payload([review()], [thread("T1", login="example-review-bot")]))
         out, unresolved = pr_review.digest("o", "r", 7)
-        self.assertEqual(0, unresolved)
-        self.assertNotIn("T1", out)
+        self.assertEqual(1, unresolved)
+        self.assertIn("unresolved=1 (other=1)", out)
+        self.assertIn("T1", out)
+
+    def test_status_and_wait_print_an_unknown_login_s_thread_in_the_digest(self) -> None:
+        """`status` and `wait` print the digest a Merge Gate decision is read from."""
+        unknown = thread("T1", login="example-review-bot")
+        for command in (["status", "7"], ["wait", "7"]):
+            with self.subTest(command=command[0]):
+                self.answer(payload([review()], [unknown]))
+                out = io.StringIO()
+                with (
+                    contextlib.redirect_stdout(out),
+                    mock.patch.object(pr_review.time, "sleep"),
+                ):
+                    pr_review.main([*command, "--repo", "o/r"])
+                self.assertIn("unresolved=1 (other=1)", out.getvalue())
+                self.assertIn("T1", out.getvalue())
 
-    def test_the_breakdown_appears_only_once_more_than_one_reviewer_contributes(self) -> None:
+    def test_one_known_reviewer_alone_prints_no_breakdown_and_two_print_one(self) -> None:
         self.answer(payload([review()], [thread("T1"), thread("T2")]))
         out, unresolved = pr_review.digest("o", "r", 7)
         self.assertEqual(2, unresolved)
@@ -4678,6 +4704,8 @@ def test_an_open_thread_defers_the_reading_until_it_is_resolved(self) -> None:
         """The headline may name that thread, which the loop already waits on."""
         body = self.headline()
         self.assertIsNone(pr_review.uncounted_verdict(payload([review(body=body)], [thread("T1")])))
+        unknown = thread("T1", login="example-review-bot")
+        self.assertIsNone(pr_review.uncounted_verdict(payload([review(body=body)], [unknown])))
         resolved = payload([review(body=body)], [thread("T1", resolved=True)])
         self.assertIsNotNone(pr_review.uncounted_verdict(resolved))