From 9efc349ba42259681371511aabbbd12b59e8720f Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 21 Oct 2026 04:45:24 -0300 Subject: [PATCH 1/9] Stop re-raising findings a human already closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A resolved review thread was re-posted as a new inline thread on the next run, over and over. github-workflows#147 ended with ONE finding spread across SIX threads, five of them redundant, out of 21 threads on the PR. The reviewer was not malfunctioning; it was following Step 3 literally. The `still present` branch gave it two cases — thread outdated, so post a fresh comment, or thread open and accurate, so do not duplicate — and said nothing about a thread that is resolved. A resolved thread falls through to "post a fresh comment". What makes it recur rather than happen once: an outdated thread stays outdated forever. Every later run saw the same permanently-outdated original, took the same branch, and opened another thread. Each new thread then became another prior finding that could re-post in turn. One of the re-posts on #147 landed when action.yml had not changed at all — identical md5, three README-only commits in between — which is the tell that this is not anchor drift. So Step 3 now has a third case: a resolved thread does not get a new one, whether or not it is outdated. The finding is still audited against current code and still counted in the summary as "Prior — still present" — only the duplicate inline thread is suppressed. Re-opening a closed question belongs in the summary, where it is said once, not in a thread the author already answered. collect_prior_findings now also reports has_author_reply, computed from the comments the thread query already fetches, so no extra API calls. It is not evidence about the code and is documented as such: it distinguishes a human engaging with a finding from the bot talking to itself. Co-Authored-By: Claude Opus 5 (1M context) --- .../pr-review/prompts/base-pr-review.md | 21 ++++++-- .../scripts/resolve-outdated-threads.py | 10 ++++ .../scripts/test_verdict_scaffolding.py | 49 +++++++++++++++++-- 3 files changed, 71 insertions(+), 9 deletions(-) diff --git a/.github/actions/pr-review/prompts/base-pr-review.md b/.github/actions/pr-review/prompts/base-pr-review.md index 17a8de0..61f45ee 100644 --- a/.github/actions/pr-review/prompts/base-pr-review.md +++ b/.github/actions/pr-review/prompts/base-pr-review.md @@ -140,11 +140,22 @@ have been fixed since. Only the current code decides. For EACH entry in `prior_findings`, read the current code at (and around) the flagged location and assign exactly one verdict: -- `still present` — the issue exists in the current code. If the existing - thread is outdated (its line no longer matches the code), post a fresh inline - comment at the current location; if the thread is still open and accurate, do - not post a duplicate — the open thread already covers it. Either way, count - it in the summary's Blocking Issues or Suggestions at its severity. +- `still present` — the issue exists in the current code. Count it in the + summary's Blocking Issues or Suggestions at its severity in every case + below; what changes is only whether you open a NEW inline thread for it. + - Thread still open and accurate: do not post a duplicate — the open thread + already covers it. + - Thread outdated (its line no longer matches the code) and NOT resolved: + post a fresh inline comment at the current location, because nothing on + the PR currently points at the issue. + - Thread resolved, whether or not it is outdated: do NOT open a new thread. + Someone closed it deliberately, and `has_author_reply` tells you whether + they also wrote a reason. Say "Prior — still present" in the summary + instead. An outdated thread stays outdated forever, so re-posting on that + basis alone re-raises the same closed finding on every future run — each + new thread becoming another prior finding that re-posts in turn. + Re-opening the question belongs in the summary, where it is said once, not + in a thread the author already answered. - `fixed` — the current code resolves it. Cite the file:line that fixes it. - `obsolete` — the code it applied to was removed or rewritten so the issue no longer applies. Say what replaced it. diff --git a/.github/actions/pr-review/scripts/resolve-outdated-threads.py b/.github/actions/pr-review/scripts/resolve-outdated-threads.py index 09f4a47..cf2f150 100644 --- a/.github/actions/pr-review/scripts/resolve-outdated-threads.py +++ b/.github/actions/pr-review/scripts/resolve-outdated-threads.py @@ -181,6 +181,12 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: not mean the issue was fixed; an open one does not mean it is still present), so the reviewer re-verifies each entry against the current code. Unresolved entries sort first, then by path. + + `has_author_reply` records whether anyone other than this reviewer has + written in the thread. It is not evidence about the code either — it says + a human engaged with the finding and the thread is not just the bot + talking to itself. The audit uses it to decide whether re-raising the + finding as a NEW inline thread is warranted, never to skip verifying it. """ findings = [] for thread in threads: @@ -200,6 +206,10 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: "excerpt": body.splitlines()[0][:200], "thread_resolved": thread["isResolved"], "thread_outdated": thread["isOutdated"], + "has_author_reply": any( + (c.get("author") or {}).get("login", "") not in BOT_LOGINS + for c in comments[1:] + ), }) findings.sort(key=lambda f: (f["thread_resolved"], f["path"] or "", f["line"] or 0)) return findings diff --git a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py index 82c327d..9392c3a 100755 --- a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py +++ b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py @@ -1755,17 +1755,18 @@ def _thread( outdated: bool = False, path: str = "pkg/foo.go", line: int | None = 42, + replies: list[tuple[str, str]] | None = None, ) -> dict: + nodes = [{"body": body, "author": {"login": author}}] + for reply_author, reply_body in replies or []: + nodes.append({"body": reply_body, "author": {"login": reply_author}}) return { "id": "PRRT_x", "isResolved": resolved, "isOutdated": outdated, "path": path, "line": line, - "comments": { - "totalCount": 1, - "nodes": [{"body": body, "author": {"login": author}}], - }, + "comments": {"totalCount": len(nodes), "nodes": nodes}, } def test_collects_bot_findings_only(self): @@ -1796,6 +1797,46 @@ def test_outdated_state_preserved(self): findings = rot.collect_prior_findings([self._thread("🟠 Bug: x", outdated=True)]) self.assertTrue(findings[0]["thread_outdated"]) + def test_author_reply_detected(self): + # The signal the audit needs to tell "a human engaged with this" from + # "the bot has been talking to itself". + findings = rot.collect_prior_findings([ + self._thread("🟠 Bug: x", replies=[("octocat", "fixed in abc1234")]), + ]) + self.assertTrue(findings[0]["has_author_reply"]) + + def test_bot_only_thread_has_no_author_reply(self): + for replies in (None, [("github-actions[bot]", "re-posting, prior thread outdated")]): + with self.subTest(replies=replies): + findings = rot.collect_prior_findings([ + self._thread("🟠 Bug: x", replies=replies), + ]) + self.assertFalse(findings[0]["has_author_reply"]) + + def test_author_reply_survives_a_later_bot_comment(self): + # Ordering must not matter: the bot replying last does not erase the + # fact that a human wrote in the thread. + findings = rot.collect_prior_findings([ + self._thread("🟠 Bug: x", replies=[ + ("octocat", "accepted trade-off, see the docs"), + ("github-actions[bot]", "acknowledged"), + ]), + ]) + self.assertTrue(findings[0]["has_author_reply"]) + + def test_resolved_and_outdated_thread_still_reported(self): + # The entry is still produced — the audit must re-verify it against + # the code. What changed is the prompt'"'"'s instruction about opening a + # NEW thread for it, not whether the finding is surfaced at all. + findings = rot.collect_prior_findings([ + self._thread("🟠 Bug: x", resolved=True, outdated=True, + replies=[("octocat", "accepted")]), + ]) + self.assertEqual(len(findings), 1) + self.assertTrue(findings[0]["thread_resolved"]) + self.assertTrue(findings[0]["thread_outdated"]) + self.assertTrue(findings[0]["has_author_reply"]) + def test_severity_mapping(self): self.assertEqual(rot.severity_of("🔴 Security: s"), "security") self.assertEqual(rot.severity_of("🟠 Bug: b"), "bug") From 71b5be449e83464baf35ba9a83db47f13b542c5d Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 21 Oct 2026 04:51:54 -0300 Subject: [PATCH 2/9] Apply the same rule in Step 6, and stop a long thread reading as unanswered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 6 restates the prior-findings rule for the reviewer's final pass, and it still said to post a fresh inline comment whenever the old thread is outdated, with no exception for resolved ones. That is the instruction this branch set out to remove, left intact two hundred lines below the place it was removed — so a resolved and outdated thread could still be re-raised, and the fix was incomplete. Both steps now say the same thing. has_author_reply also read a truncated thread as unanswered. The query takes comments(first: 20), so on a longer thread a human reply can sit past the cut, and False there is the one direction that makes the audit MORE willing to re-raise a finding. Truncation now counts as replied-to, on the grounds that a thread that long has been engaged with. is_resolvable treats the same truncation as a reason not to act; both are the same instinct — do not act on what was not read. Also removes a shell-quoting artifact (`prompt'"'"'s`) that leaked out of the heredoc used to write the test comment. Swept the action directory; it was the only one. 181 tests, up from 179. Co-Authored-By: Claude Opus 5 (1M context) --- .../pr-review/prompts/base-pr-review.md | 6 +++-- .../scripts/resolve-outdated-threads.py | 16 +++++++++++--- .../scripts/test_verdict_scaffolding.py | 22 ++++++++++++++++++- 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/.github/actions/pr-review/prompts/base-pr-review.md b/.github/actions/pr-review/prompts/base-pr-review.md index 61f45ee..7051af9 100644 --- a/.github/actions/pr-review/prompts/base-pr-review.md +++ b/.github/actions/pr-review/prompts/base-pr-review.md @@ -265,8 +265,10 @@ not pre-filtering, decides what blocks merge. Handle prior findings per the Step 3 audit — never silently skip them. Do not post a duplicate inline comment for a still-present issue whose thread is open -and accurate, but DO count it in the summary counts, and DO post a fresh inline -comment when the old thread is outdated and no longer points at the code. +and accurate, and never open a new thread for one whose thread is resolved. DO +count every still-present issue in the summary counts, and DO post a fresh +inline comment when the old thread is outdated, unresolved, and no longer +points at the code. Finally, take stock of your own coverage. If part of the change could not be fully reviewed — truncated or dropped diff paths, unreadable generated content, areas you diff --git a/.github/actions/pr-review/scripts/resolve-outdated-threads.py b/.github/actions/pr-review/scripts/resolve-outdated-threads.py index cf2f150..befc274 100644 --- a/.github/actions/pr-review/scripts/resolve-outdated-threads.py +++ b/.github/actions/pr-review/scripts/resolve-outdated-threads.py @@ -206,9 +206,19 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: "excerpt": body.splitlines()[0][:200], "thread_resolved": thread["isResolved"], "thread_outdated": thread["isOutdated"], - "has_author_reply": any( - (c.get("author") or {}).get("login", "") not in BOT_LOGINS - for c in comments[1:] + # Truncated threads count as replied-to. The query takes only the + # first 20 comments, so on a longer thread a human reply can sit + # past the cut and read as "nobody ever answered" — the one + # direction that makes the audit MORE willing to re-raise a + # finding. A thread that long has been engaged with. (is_resolvable + # above treats the same truncation as a reason not to act, for the + # same underlying reason: do not act on what was not read.) + "has_author_reply": ( + thread["comments"]["totalCount"] != len(comments) + or any( + (c.get("author") or {}).get("login", "") not in BOT_LOGINS + for c in comments[1:] + ) ), }) findings.sort(key=lambda f: (f["thread_resolved"], f["path"] or "", f["line"] or 0)) diff --git a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py index 9392c3a..14ea5ba 100755 --- a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py +++ b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py @@ -1826,7 +1826,7 @@ def test_author_reply_survives_a_later_bot_comment(self): def test_resolved_and_outdated_thread_still_reported(self): # The entry is still produced — the audit must re-verify it against - # the code. What changed is the prompt'"'"'s instruction about opening a + # the code. What changed is the prompt's instruction about opening a # NEW thread for it, not whether the finding is surfaced at all. findings = rot.collect_prior_findings([ self._thread("🟠 Bug: x", resolved=True, outdated=True, @@ -1837,6 +1837,26 @@ def test_resolved_and_outdated_thread_still_reported(self): self.assertTrue(findings[0]["thread_outdated"]) self.assertTrue(findings[0]["has_author_reply"]) + def test_truncated_thread_counts_as_replied_to(self): + # The query takes comments(first: 20). On a longer thread a human + # reply can sit past the cut; reading that as "nobody replied" is the + # one direction that makes the audit more willing to re-raise. + thread = self._thread("\U0001f7e0 Bug: x") + thread["comments"]["totalCount"] = 25 # 1 fetched, 25 exist + findings = rot.collect_prior_findings([thread]) + self.assertTrue(findings[0]["has_author_reply"]) + + def test_untruncated_bot_only_thread_still_has_no_author_reply(self): + # The truncation rule must not swallow the ordinary case: a complete + # thread with only bot comments has no author reply. + thread = self._thread( + "\U0001f7e0 Bug: x", + replies=[("github-actions[bot]", "re-posting, prior thread outdated")], + ) + self.assertEqual(thread["comments"]["totalCount"], 2) + findings = rot.collect_prior_findings([thread]) + self.assertFalse(findings[0]["has_author_reply"]) + def test_severity_mapping(self): self.assertEqual(rot.severity_of("🔴 Security: s"), "security") self.assertEqual(rot.severity_of("🟠 Bug: b"), "bug") From b111dcd98c63be681dafa8d1aa84354d4eb2dea1 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 21 Oct 2026 06:05:23 -0300 Subject: [PATCH 3/9] Branch on whether a person engaged, not on whether the thread is resolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first pass fixed one path into the loop and left two open, because it keyed on the wrong signal. should_resolve auto-resolves every outdated, reply-free bot thread on each run, so thread_resolved is usually this action's own doing rather than a human decision — "Someone closed it deliberately" was false for the majority case. Meanwhile a thread a person REPLIED to but did not resolve stays outdated and unresolved forever, which is the branch that re-posts, so the most common human response to a finding kept looping. And where the token is denied resolveReviewThread — which this script documents as common — nothing is ever resolved and the rule could not fire at all. has_author_reply separates those populations exactly, and until now nothing branched on it. It does now: a thread a person has written in never gets a new one, whatever its resolution state; a thread that is only this bot talking to itself keeps the old behaviour, which is also what recovers a finding whose thread was auto-resolved before a replacement was posted. Added the case that stops accumulation regardless of path: do not open a new thread when another open, accurate thread already covers the same issue. prior_findings is one entry per thread, not per issue, so after any re-post the old entry kept matching the post-fresh branch while the new one matched do-not-duplicate — each run adding another. Also scoped the whole rule to the flagged location, since a distinct occurrence elsewhere is a New finding. Step 6 now points at Step 3's cases instead of restating them, keeping only the invariant it genuinely adds: every still-present issue is counted whatever happens to its thread. Two copies of one rule is how this bug got here. The prompt now also sees has_author_reply and resolution_blocked, neither of which it was told about — the second meaning the reviewer could not tell it was in the degraded mode where outdated threads never get cleared. Script: BOT_LOGINS comes from _review_state like everywhere else rather than a third private copy, where drift would be fail-open (an unlisted bot login reads as a human reply and suppresses re-raising). The truncation guard is `>` not `!=`, and its comment no longer claims True is the conservative direction — it buys silence, which for a reviewer is the unsafe one; it is chosen anyway and now says why. The cross-reference named is_resolvable, which does not exist; the function is should_resolve. Tests: the truncation fixture used one node with totalCount 25, a shape comments(first: 20) cannot return, and neither boundary was covered — now 20 of 20 and 20 of 21. And the pre-mutation snapshot that main() depends on to re-anchor outdated findings is documented and pinned, because "report the state we just set" reads as an obvious cleanup and would silently turn every outdated finding into a summary line with the suite still green. 181 tests, up from 179. Co-Authored-By: Claude Opus 5 (1M context) --- .../pr-review/prompts/base-pr-review.md | 62 ++++++++++++------- .../scripts/resolve-outdated-threads.py | 40 +++++++++--- .../scripts/test_verdict_scaffolding.py | 47 +++++++++----- 3 files changed, 100 insertions(+), 49 deletions(-) diff --git a/.github/actions/pr-review/prompts/base-pr-review.md b/.github/actions/pr-review/prompts/base-pr-review.md index 7051af9..4f6abb6 100644 --- a/.github/actions/pr-review/prompts/base-pr-review.md +++ b/.github/actions/pr-review/prompts/base-pr-review.md @@ -127,10 +127,14 @@ PR head tree, not the previous reviewed tree. ### Step 3 — Audit prior findings (mandatory) Read `.github/prior-findings.json` — it lists every finding this reviewer has -previously posted on this PR (path, line, severity, excerpt, and the thread's -`thread_resolved` / `thread_outdated` state). Also read -`.github/resolved-threads.json` and use its `resolved_count` when reporting -"Threads Resolved" in the summary. +previously posted on this PR: `path`, `line`, `severity`, `excerpt`, the +thread's `thread_resolved` / `thread_outdated` state, and `has_author_reply`, +which is true when anyone other than this reviewer has written in the thread. +Also read `.github/resolved-threads.json`: use its `resolved_count` when +reporting "Threads Resolved", and note `resolution_blocked` — when it is true +this run could not resolve its own outdated threads (the token is denied that +mutation in some repos), so expect stale `thread_outdated` entries that nobody +will ever clear. Thread state is not evidence of code state. A resolved or outdated thread does NOT mean the issue was fixed — anyone can resolve a thread without changing @@ -141,21 +145,32 @@ For EACH entry in `prior_findings`, read the current code at (and around) the flagged location and assign exactly one verdict: - `still present` — the issue exists in the current code. Count it in the - summary's Blocking Issues or Suggestions at its severity in every case + summary's Blocking Issues or Suggestions at its severity in EVERY case below; what changes is only whether you open a NEW inline thread for it. - - Thread still open and accurate: do not post a duplicate — the open thread - already covers it. - - Thread outdated (its line no longer matches the code) and NOT resolved: - post a fresh inline comment at the current location, because nothing on - the PR currently points at the issue. - - Thread resolved, whether or not it is outdated: do NOT open a new thread. - Someone closed it deliberately, and `has_author_reply` tells you whether - they also wrote a reason. Say "Prior — still present" in the summary - instead. An outdated thread stays outdated forever, so re-posting on that - basis alone re-raises the same closed finding on every future run — each - new thread becoming another prior finding that re-posts in turn. - Re-opening the question belongs in the summary, where it is said once, not - in a thread the author already answered. + Work down these in order and stop at the first that matches: + - **`has_author_reply` is true** — a person has written in that thread. Do + not open a new one, whether the thread is open, outdated or resolved. + Report it as "Prior — still present" in the summary. Re-raising a question + someone already answered belongs in the summary, where it is said once. + - **Another open, accurate thread already covers this same issue** — do not + post a duplicate. Prior findings are one entry per thread, not per issue, + so one issue can appear several times here; the open one covers it. + - **The thread is outdated and nothing open covers the issue** — post a + fresh inline comment at the current location. Nothing on the PR points at + it otherwise, and this is also the path that recovers a finding whose + thread was resolved automatically rather than by a person. + - **Otherwise** (thread open and accurate) — do not post a duplicate. + + This branches on `has_author_reply` rather than on `thread_resolved` because + a resolved thread is usually not a human decision: this action resolves its + own outdated, reply-free threads on every run. An outdated thread also stays + outdated forever, so keying on that alone re-raises the same finding on + every future run, each new thread becoming another prior finding that + re-posts in turn. + + Scope: all of this is about the flagged location. A distinct occurrence of + the same kind of issue somewhere else is a New finding and these rules do + not cover it. - `fixed` — the current code resolves it. Cite the file:line that fixes it. - `obsolete` — the code it applied to was removed or rewritten so the issue no longer applies. Say what replaced it. @@ -263,12 +278,11 @@ confident about is a validated finding at `suggestion` severity with its confide noted, not a dropped finding and not an unvalidated guess. The downstream verdict logic, not pre-filtering, decides what blocks merge. -Handle prior findings per the Step 3 audit — never silently skip them. Do not -post a duplicate inline comment for a still-present issue whose thread is open -and accurate, and never open a new thread for one whose thread is resolved. DO -count every still-present issue in the summary counts, and DO post a fresh -inline comment when the old thread is outdated, unresolved, and no longer -points at the code. +Handle prior findings per the Step 3 audit — never silently skip them, and +apply its four thread cases exactly as written there rather than re-deriving +them. The invariant those cases sit under, and the one thing to carry out of +Step 3 if you carry nothing else: EVERY still-present issue is counted in the +summary at its severity, whatever you decide about its thread. Finally, take stock of your own coverage. If part of the change could not be fully reviewed — truncated or dropped diff paths, unreadable generated content, areas you diff --git a/.github/actions/pr-review/scripts/resolve-outdated-threads.py b/.github/actions/pr-review/scripts/resolve-outdated-threads.py index befc274..377fb93 100644 --- a/.github/actions/pr-review/scripts/resolve-outdated-threads.py +++ b/.github/actions/pr-review/scripts/resolve-outdated-threads.py @@ -20,7 +20,11 @@ REVIEW_PREFIXES = ("🔴 Security:", "🟠 Bug:", "🟡 Suggestion:") DEFAULT_API_ATTEMPTS = 3 HTTP_STATUS_PATTERN = re.compile(r"HTTP\s+(\d{3})") -BOT_LOGINS = {"github-actions[bot]", "github-actions"} +# Shared with publish-review-report.py and fetch-pr-context.py so every side +# of the contract classifies comments identically. Thread classification is one +# of those sides: an unlisted bot login would read as a human reply here and +# suppress re-raising a finding, so a private copy drifting is fail-open. +from _review_state import BOT_LOGINS # noqa: E402 LIST_THREADS_QUERY = """ query($owner: String!, $repo: String!, $number: Int!, $after: String) { @@ -185,8 +189,11 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: `has_author_reply` records whether anyone other than this reviewer has written in the thread. It is not evidence about the code either — it says a human engaged with the finding and the thread is not just the bot - talking to itself. The audit uses it to decide whether re-raising the - finding as a NEW inline thread is warranted, never to skip verifying it. + talking to itself. Step 3 of the review prompt branches on it to decide + whether to open a NEW inline thread for a still-present finding, never to + skip verifying one. It is deliberately the axis rather than + `thread_resolved`, because most resolved threads here were resolved by + should_resolve above rather than by a person. """ findings = [] for thread in threads: @@ -206,15 +213,21 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: "excerpt": body.splitlines()[0][:200], "thread_resolved": thread["isResolved"], "thread_outdated": thread["isOutdated"], - # Truncated threads count as replied-to. The query takes only the + # A truncated thread counts as replied-to. The query takes the # first 20 comments, so on a longer thread a human reply can sit - # past the cut and read as "nobody ever answered" — the one - # direction that makes the audit MORE willing to re-raise a - # finding. A thread that long has been engaged with. (is_resolvable - # above treats the same truncation as a reason not to act, for the - # same underlying reason: do not act on what was not read.) + # past the cut and read as "nobody ever answered". + # + # Note which way this errs, because it is not the usual one: the + # audit suppresses a new inline thread when this is true, so True + # buys silence, and for a reviewer silence is the unsafe + # direction. It is chosen anyway because a thread past twenty + # comments has demonstrably been engaged with, and re-raising into + # a conversation that long is the worse failure. should_resolve + # above reads the same truncation as a reason not to auto-resolve + # — opposite literal value, same instinct: do not act on what was + # not read. "has_author_reply": ( - thread["comments"]["totalCount"] != len(comments) + thread["comments"]["totalCount"] > len(comments) or any( (c.get("author") or {}).get("login", "") not in BOT_LOGINS for c in comments[1:] @@ -306,6 +319,13 @@ def main(): } write_summary(summary) + # DELIBERATELY the pre-mutation snapshot. `resolve_thread` changes GitHub, + # not these dicts, so a thread resolved seconds ago is still reported + # unresolved and outdated — which is the one path that lets the review + # re-anchor that finding at its new location before the resolution takes + # effect for good. "Reporting the state we just set" looks like an obvious + # cleanup and would silently turn every outdated finding into a summary + # line forever. test_prior_findings_report_pre_resolution_state pins it. write_prior_findings(collect_prior_findings(threads)) print(f"\nDone: resolved {len(resolved)}/{len(to_resolve)} threads") diff --git a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py index 14ea5ba..ccbc61d 100755 --- a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py +++ b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py @@ -1813,6 +1813,19 @@ def test_bot_only_thread_has_no_author_reply(self): ]) self.assertFalse(findings[0]["has_author_reply"]) + def test_prior_findings_report_pre_resolution_state(self): + # main() resolves outdated bot-only threads and THEN collects prior + # findings from the same, un-refreshed list. That ordering is what + # lets the review re-anchor the finding once before the resolution + # takes effect; "report the state we just set" would look like a + # cleanup and would silently turn every outdated finding into a + # summary line forever. If this fails, read the comment in main(). + thread = self._thread("🟠 Bug: x", outdated=True) + self.assertTrue(rot.should_resolve(thread)) # main() would resolve it + finding = rot.collect_prior_findings([thread])[0] # ...and still reports it + self.assertFalse(finding["thread_resolved"]) # as unresolved + outdated, + self.assertTrue(finding["thread_outdated"]) # which is the re-anchor path + def test_author_reply_survives_a_later_bot_comment(self): # Ordering must not matter: the bot replying last does not erase the # fact that a human wrote in the thread. @@ -1837,25 +1850,29 @@ def test_resolved_and_outdated_thread_still_reported(self): self.assertTrue(findings[0]["thread_outdated"]) self.assertTrue(findings[0]["has_author_reply"]) - def test_truncated_thread_counts_as_replied_to(self): - # The query takes comments(first: 20). On a longer thread a human - # reply can sit past the cut; reading that as "nobody replied" is the - # one direction that makes the audit more willing to re-raise. - thread = self._thread("\U0001f7e0 Bug: x") - thread["comments"]["totalCount"] = 25 # 1 fetched, 25 exist - findings = rot.collect_prior_findings([thread]) - self.assertTrue(findings[0]["has_author_reply"]) + def _bot_thread_of_length(self, n: int, total: int | None = None) -> dict: + """A bot-only thread of n FETCHED comments, totalCount defaulting to n. - def test_untruncated_bot_only_thread_still_has_no_author_reply(self): - # The truncation rule must not swallow the ordinary case: a complete - # thread with only bot comments has no author reply. + The query is comments(first: 20), so a truncated thread really looks + like 20 fetched nodes with a higher totalCount — not one node and a + big number, which the API cannot produce. + """ thread = self._thread( "\U0001f7e0 Bug: x", - replies=[("github-actions[bot]", "re-posting, prior thread outdated")], + replies=[("github-actions[bot]", f"bot {i}") for i in range(n - 1)], ) - self.assertEqual(thread["comments"]["totalCount"], 2) - findings = rot.collect_prior_findings([thread]) - self.assertFalse(findings[0]["has_author_reply"]) + thread["comments"]["totalCount"] = n if total is None else total + return thread + + def test_truncation_boundary(self): + # 20 fetched of 20 is complete and must stay False; 20 of 21 is the + # first truncated shape the query can actually return. + complete = self._bot_thread_of_length(20) + self.assertEqual(len(complete["comments"]["nodes"]), 20) + self.assertFalse(rot.collect_prior_findings([complete])[0]["has_author_reply"]) + + truncated = self._bot_thread_of_length(20, total=21) + self.assertTrue(rot.collect_prior_findings([truncated])[0]["has_author_reply"]) def test_severity_mapping(self): self.assertEqual(rot.severity_of("🔴 Security: s"), "security") From 2e9a39a189d458977ae6bf48c9fed6c0ff1d6552 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 21 Oct 2026 06:11:02 -0300 Subject: [PATCH 4/9] Count clicking Resolve as engagement, not only replying MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third time this fix has been too narrow, each time by reading the signal I had already looked at rather than the one that decides. has_author_reply only saw replies. A person who resolves a thread without writing anything — far more common than replying — left it false, so a resolved, outdated, reply-free thread fell through to the post-fresh branch. Resolve the re-post too and nothing open covers the issue, so the original re-posts again next run, and the one after. That is the #147 loop by another route, and it contradicted this PR's own description. The query now asks for resolvedBy, and the field becomes human_engaged: a person wrote in the thread, or resolved it. Both are ways of saying "I have dealt with this", and neither should be answered by opening the same thread again. resolvedBy is also what separates the two populations the earlier version conflated. should_resolve closes this reviewer's own reply-free threads on every run, and those carry a BOT_LOGINS resolver, so they stay false and keep the re-anchor path that recovers a finding whose thread was closed with no replacement posted. Verified against the live API before relying on it: resolvedBy returns the resolving login and is null on an open thread. Renamed because "author" was wrong twice over — it was never limited to the PR author, and it is no longer limited to replies. 184 tests, up from 181: silent human resolution engages, bot resolution does not, and an open thread's null resolver does not read as a person. Co-Authored-By: Claude Opus 5 (1M context) --- .../pr-review/prompts/base-pr-review.md | 23 ++++++---- .../scripts/resolve-outdated-threads.py | 33 +++++++++----- .../scripts/test_verdict_scaffolding.py | 44 ++++++++++++++++--- 3 files changed, 74 insertions(+), 26 deletions(-) diff --git a/.github/actions/pr-review/prompts/base-pr-review.md b/.github/actions/pr-review/prompts/base-pr-review.md index 4f6abb6..b665f0a 100644 --- a/.github/actions/pr-review/prompts/base-pr-review.md +++ b/.github/actions/pr-review/prompts/base-pr-review.md @@ -128,8 +128,9 @@ PR head tree, not the previous reviewed tree. Read `.github/prior-findings.json` — it lists every finding this reviewer has previously posted on this PR: `path`, `line`, `severity`, `excerpt`, the -thread's `thread_resolved` / `thread_outdated` state, and `has_author_reply`, -which is true when anyone other than this reviewer has written in the thread. +thread's `thread_resolved` / `thread_outdated` state, and `human_engaged`, +which is true when a person has dealt with the thread at all — wrote in it, or +resolved it themselves. Also read `.github/resolved-threads.json`: use its `resolved_count` when reporting "Threads Resolved", and note `resolution_blocked` — when it is true this run could not resolve its own outdated threads (the token is denied that @@ -148,10 +149,11 @@ flagged location and assign exactly one verdict: summary's Blocking Issues or Suggestions at its severity in EVERY case below; what changes is only whether you open a NEW inline thread for it. Work down these in order and stop at the first that matches: - - **`has_author_reply` is true** — a person has written in that thread. Do - not open a new one, whether the thread is open, outdated or resolved. - Report it as "Prior — still present" in the summary. Re-raising a question - someone already answered belongs in the summary, where it is said once. + - **`human_engaged` is true** — a person replied in that thread or resolved + it. Do not open a new one, whether the thread is open, outdated or + resolved. Report it as "Prior — still present" in the summary. Re-raising + something a person already dealt with belongs in the summary, where it is + said once. - **Another open, accurate thread already covers this same issue** — do not post a duplicate. Prior findings are one entry per thread, not per issue, so one issue can appear several times here; the open one covers it. @@ -161,9 +163,12 @@ flagged location and assign exactly one verdict: thread was resolved automatically rather than by a person. - **Otherwise** (thread open and accurate) — do not post a duplicate. - This branches on `has_author_reply` rather than on `thread_resolved` because - a resolved thread is usually not a human decision: this action resolves its - own outdated, reply-free threads on every run. An outdated thread also stays + This branches on `human_engaged` rather than on `thread_resolved` because + resolution alone does not say who did it: this action resolves its own + outdated, reply-free threads on every run, so most resolved threads are its + doing rather than a decision. `human_engaged` covers both ways a person + signals they have dealt with a finding — replying, and simply clicking + Resolve, which is the more common of the two. An outdated thread also stays outdated forever, so keying on that alone re-raises the same finding on every future run, each new thread becoming another prior finding that re-posts in turn. diff --git a/.github/actions/pr-review/scripts/resolve-outdated-threads.py b/.github/actions/pr-review/scripts/resolve-outdated-threads.py index 377fb93..46d8bad 100644 --- a/.github/actions/pr-review/scripts/resolve-outdated-threads.py +++ b/.github/actions/pr-review/scripts/resolve-outdated-threads.py @@ -35,6 +35,7 @@ id isResolved isOutdated + resolvedBy { login } path line comments(first: 20) { @@ -186,14 +187,19 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: present), so the reviewer re-verifies each entry against the current code. Unresolved entries sort first, then by path. - `has_author_reply` records whether anyone other than this reviewer has - written in the thread. It is not evidence about the code either — it says - a human engaged with the finding and the thread is not just the bot - talking to itself. Step 3 of the review prompt branches on it to decide - whether to open a NEW inline thread for a still-present finding, never to - skip verifying one. It is deliberately the axis rather than - `thread_resolved`, because most resolved threads here were resolved by - should_resolve above rather than by a person. + `human_engaged` records whether a person has dealt with the thread at all: + wrote in it, or resolved it. It is not evidence about the code either — + it says the thread is not just this bot talking to itself. Step 3 of the + review prompt branches on it to decide whether to open a NEW inline thread + for a still-present finding, never to skip verifying one. + + It is deliberately the axis rather than `thread_resolved`, which does not + separate the populations: should_resolve above resolves this reviewer's + own outdated, reply-free threads on every run, so most resolved threads + are the action's doing. `resolvedBy` is what tells those apart from a + person clicking Resolve — which people do far more often than they reply, + and which an earlier version of this rule read as "nobody engaged" and + re-posted into. """ findings = [] for thread in threads: @@ -213,7 +219,7 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: "excerpt": body.splitlines()[0][:200], "thread_resolved": thread["isResolved"], "thread_outdated": thread["isOutdated"], - # A truncated thread counts as replied-to. The query takes the + # A truncated thread counts as engaged-with. The query takes the # first 20 comments, so on a longer thread a human reply can sit # past the cut and read as "nobody ever answered". # @@ -226,12 +232,19 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: # above reads the same truncation as a reason not to auto-resolve # — opposite literal value, same instinct: do not act on what was # not read. - "has_author_reply": ( + "human_engaged": ( thread["comments"]["totalCount"] > len(comments) or any( (c.get("author") or {}).get("login", "") not in BOT_LOGINS for c in comments[1:] ) + # Resolved by a person rather than by should_resolve. The bot's + # own resolutions carry a BOT_LOGINS resolver, so they stay + # false here and keep the re-anchor path. + or ( + (thread.get("resolvedBy") or {}).get("login", "") + not in BOT_LOGINS | {""} + ) ), }) findings.sort(key=lambda f: (f["thread_resolved"], f["path"] or "", f["line"] or 0)) diff --git a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py index ccbc61d..560d532 100755 --- a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py +++ b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py @@ -1756,6 +1756,7 @@ def _thread( path: str = "pkg/foo.go", line: int | None = 42, replies: list[tuple[str, str]] | None = None, + resolved_by: str | None = None, ) -> dict: nodes = [{"body": body, "author": {"login": author}}] for reply_author, reply_body in replies or []: @@ -1764,6 +1765,7 @@ def _thread( "id": "PRRT_x", "isResolved": resolved, "isOutdated": outdated, + "resolvedBy": {"login": resolved_by} if resolved_by else None, "path": path, "line": line, "comments": {"totalCount": len(nodes), "nodes": nodes}, @@ -1797,13 +1799,13 @@ def test_outdated_state_preserved(self): findings = rot.collect_prior_findings([self._thread("🟠 Bug: x", outdated=True)]) self.assertTrue(findings[0]["thread_outdated"]) - def test_author_reply_detected(self): + def test_reply_counts_as_engagement(self): # The signal the audit needs to tell "a human engaged with this" from # "the bot has been talking to itself". findings = rot.collect_prior_findings([ self._thread("🟠 Bug: x", replies=[("octocat", "fixed in abc1234")]), ]) - self.assertTrue(findings[0]["has_author_reply"]) + self.assertTrue(findings[0]["human_engaged"]) def test_bot_only_thread_has_no_author_reply(self): for replies in (None, [("github-actions[bot]", "re-posting, prior thread outdated")]): @@ -1811,7 +1813,7 @@ def test_bot_only_thread_has_no_author_reply(self): findings = rot.collect_prior_findings([ self._thread("🟠 Bug: x", replies=replies), ]) - self.assertFalse(findings[0]["has_author_reply"]) + self.assertFalse(findings[0]["human_engaged"]) def test_prior_findings_report_pre_resolution_state(self): # main() resolves outdated bot-only threads and THEN collects prior @@ -1835,7 +1837,7 @@ def test_author_reply_survives_a_later_bot_comment(self): ("github-actions[bot]", "acknowledged"), ]), ]) - self.assertTrue(findings[0]["has_author_reply"]) + self.assertTrue(findings[0]["human_engaged"]) def test_resolved_and_outdated_thread_still_reported(self): # The entry is still produced — the audit must re-verify it against @@ -1848,7 +1850,7 @@ def test_resolved_and_outdated_thread_still_reported(self): self.assertEqual(len(findings), 1) self.assertTrue(findings[0]["thread_resolved"]) self.assertTrue(findings[0]["thread_outdated"]) - self.assertTrue(findings[0]["has_author_reply"]) + self.assertTrue(findings[0]["human_engaged"]) def _bot_thread_of_length(self, n: int, total: int | None = None) -> dict: """A bot-only thread of n FETCHED comments, totalCount defaulting to n. @@ -1864,15 +1866,43 @@ def _bot_thread_of_length(self, n: int, total: int | None = None) -> dict: thread["comments"]["totalCount"] = n if total is None else total return thread + def test_silent_human_resolution_counts_as_engagement(self): + # Resolving without replying is the common case, and the earlier rule + # read it as "nobody engaged" and re-posted into it — the #147 loop by + # another route. + thread = self._thread( + "🟠 Bug: x", resolved=True, outdated=True, resolved_by="octocat", + ) + self.assertTrue(rot.collect_prior_findings([thread])[0]["human_engaged"]) + + def test_bot_resolution_is_not_engagement(self): + # should_resolve closes this reviewer's own reply-free threads every + # run. Those must stay false, or the re-anchor path disappears. + for resolver in ("github-actions[bot]", "github-actions"): + with self.subTest(resolver=resolver): + thread = self._thread( + "🟠 Bug: x", resolved=True, outdated=True, + resolved_by=resolver, + ) + self.assertFalse( + rot.collect_prior_findings([thread])[0]["human_engaged"] + ) + + def test_unresolved_thread_has_no_resolver(self): + # resolvedBy is null on an open thread; that must not read as a human. + thread = self._thread("🟠 Bug: x", outdated=True) + self.assertIsNone(thread["resolvedBy"]) + self.assertFalse(rot.collect_prior_findings([thread])[0]["human_engaged"]) + def test_truncation_boundary(self): # 20 fetched of 20 is complete and must stay False; 20 of 21 is the # first truncated shape the query can actually return. complete = self._bot_thread_of_length(20) self.assertEqual(len(complete["comments"]["nodes"]), 20) - self.assertFalse(rot.collect_prior_findings([complete])[0]["has_author_reply"]) + self.assertFalse(rot.collect_prior_findings([complete])[0]["human_engaged"]) truncated = self._bot_thread_of_length(20, total=21) - self.assertTrue(rot.collect_prior_findings([truncated])[0]["has_author_reply"]) + self.assertTrue(rot.collect_prior_findings([truncated])[0]["human_engaged"]) def test_severity_mapping(self): self.assertEqual(rot.severity_of("🔴 Security: s"), "security") From b677541b8458793f5fd2a666e6ed3df018041aa7 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 21 Oct 2026 06:19:46 -0300 Subject: [PATCH 5/9] Rename the two tests the term change missed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A blanket rename of has_author_reply left test_bot_only_thread_has_no_author_reply and test_author_reply_survives_a_later_bot_comment untouched, because neither contains that exact token — one reads has_no_author_reply, the other test_author_reply_survives. The term is gone from the tree now. Co-Authored-By: Claude Opus 5 (1M context) --- .github/actions/pr-review/scripts/test_verdict_scaffolding.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py index 560d532..d68556c 100755 --- a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py +++ b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py @@ -1807,7 +1807,7 @@ def test_reply_counts_as_engagement(self): ]) self.assertTrue(findings[0]["human_engaged"]) - def test_bot_only_thread_has_no_author_reply(self): + def test_bot_only_thread_is_not_engagement(self): for replies in (None, [("github-actions[bot]", "re-posting, prior thread outdated")]): with self.subTest(replies=replies): findings = rot.collect_prior_findings([ @@ -1828,7 +1828,7 @@ def test_prior_findings_report_pre_resolution_state(self): self.assertFalse(finding["thread_resolved"]) # as unresolved + outdated, self.assertTrue(finding["thread_outdated"]) # which is the re-anchor path - def test_author_reply_survives_a_later_bot_comment(self): + def test_reply_survives_a_later_bot_comment(self): # Ordering must not matter: the bot replying last does not erase the # fact that a human wrote in the thread. findings = rot.collect_prior_findings([ From 9f395db1d3ae6be44eb21d794d72111ac21ef96e Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 21 Oct 2026 19:21:18 -0300 Subject: [PATCH 6/9] Let any engaged thread settle an issue, not only an open one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The covering-thread case required another OPEN thread, which leaves the loop running through the bot's own superseded originals: 1. Code moves, thread A goes outdated. should_resolve closes A in the same run where the review re-anchors the finding as thread B. 2. A person resolves B without replying. 3. Next run: B is correctly skipped, human_engaged is true. But A is still in prior_findings — human_engaged false, because the bot resolved it, and outdated forever. Nothing OPEN covers the issue any more, since B is resolved. So A matches the outdated case and posts C. 4. The person resolves C. Same again next run, and the run after. Widened to "open and accurate, OR human_engaged" — one person dealing with any thread for an issue now settles every entry of that issue, including the bot's own superseded ones, which stay in prior_findings for the life of the PR. Also removed commentary that was about this PR's own iterations rather than about the code. It reads oddly once merged, and the history belongs in the description. Co-Authored-By: Claude Opus 5 (1M context) --- .../actions/pr-review/prompts/base-pr-review.md | 13 +++++++++---- .../scripts/resolve-outdated-threads.py | 16 +++++++--------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/.github/actions/pr-review/prompts/base-pr-review.md b/.github/actions/pr-review/prompts/base-pr-review.md index b665f0a..b2bff34 100644 --- a/.github/actions/pr-review/prompts/base-pr-review.md +++ b/.github/actions/pr-review/prompts/base-pr-review.md @@ -154,10 +154,15 @@ flagged location and assign exactly one verdict: resolved. Report it as "Prior — still present" in the summary. Re-raising something a person already dealt with belongs in the summary, where it is said once. - - **Another open, accurate thread already covers this same issue** — do not - post a duplicate. Prior findings are one entry per thread, not per issue, - so one issue can appear several times here; the open one covers it. - - **The thread is outdated and nothing open covers the issue** — post a + - **Another thread for this same issue is open and accurate, OR has + `human_engaged` true** — do not post a duplicate. Prior findings are one + entry per thread, not per issue, so one issue appears here once per thread + it has ever had, and the bot's own superseded threads stay in that list + forever. Without the `human_engaged` half, dealing with the newest thread + would still leave an older bot-resolved one matching the outdated case + below, and it would post again — and again after that one is dealt with + too. One person dealing with any thread for an issue settles all of them. + - **The thread is outdated and nothing above covers the issue** — post a fresh inline comment at the current location. Nothing on the PR points at it otherwise, and this is also the path that recovers a finding whose thread was resolved automatically rather than by a person. diff --git a/.github/actions/pr-review/scripts/resolve-outdated-threads.py b/.github/actions/pr-review/scripts/resolve-outdated-threads.py index 46d8bad..f0273b2 100644 --- a/.github/actions/pr-review/scripts/resolve-outdated-threads.py +++ b/.github/actions/pr-review/scripts/resolve-outdated-threads.py @@ -193,13 +193,11 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: review prompt branches on it to decide whether to open a NEW inline thread for a still-present finding, never to skip verifying one. - It is deliberately the axis rather than `thread_resolved`, which does not - separate the populations: should_resolve above resolves this reviewer's - own outdated, reply-free threads on every run, so most resolved threads - are the action's doing. `resolvedBy` is what tells those apart from a - person clicking Resolve — which people do far more often than they reply, - and which an earlier version of this rule read as "nobody engaged" and - re-posted into. + It is the axis rather than `thread_resolved`, which does not separate the + populations: should_resolve above resolves this reviewer's own outdated, + reply-free threads on every run, so most resolved threads are the action's + doing. `resolvedBy` is what tells those apart from a person clicking + Resolve, which people do far more often than they reply. """ findings = [] for thread in threads: @@ -229,8 +227,8 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: # direction. It is chosen anyway because a thread past twenty # comments has demonstrably been engaged with, and re-raising into # a conversation that long is the worse failure. should_resolve - # above reads the same truncation as a reason not to auto-resolve - # — opposite literal value, same instinct: do not act on what was + # above reads the same truncation as a reason not to auto-resolve: + # opposite literal value, same instinct — do not act on what was # not read. "human_engaged": ( thread["comments"]["totalCount"] > len(comments) From a5dbd76b7017fd4c0d22909e9e35c0b5938369c6 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Thu, 22 Oct 2026 10:42:11 -0300 Subject: [PATCH 7/9] Decide engagement by node type, not by login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit human_engaged treated every author outside BOT_LOGINS as a person, and BOT_LOGINS lists only this reviewer. Every other app that writes on these PRs — Copilot, CodeRabbit, linear-code, dependabot — therefore counted as human engagement, as did a resolvedBy that was any of them. Once that happened the finding was never re-anchored inline again, however far the code moved. Not hypothetical: linear-code comments on the PRs in this very series, and copilot-pull-request-reviewer has reviewed connector PRs alongside this bot. Now keyed on GraphQL's __typename: every GitHub App is a Bot, a person is a User. Matching on login instead would need a list of every app anyone ever installs and would miss them by default, which is the direction that silently retires a finding. Checked against the live API first — a reply from github-actions comes back as Bot, and its login carries no "[bot]" suffix, which is exactly why BOT_LOGINS has to list both spellings and why an endswith check would not work. BOT_LOGINS stays as it is and stays narrow. _review_state uses it as a trust allowlist for summary authorship, which is a different question from whether a commenter is a human, and widening it for this would weaken that. 186 tests, up from 184: three apps counted as not-a-person for both replying and resolving, a human login that looks bot-ish still a person, and the Actions bot still a bot despite the missing suffix. Co-Authored-By: Claude Opus 5 (1M context) --- .../scripts/resolve-outdated-threads.py | 40 +++++++++++------ .../scripts/test_verdict_scaffolding.py | 45 +++++++++++++++++-- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/.github/actions/pr-review/scripts/resolve-outdated-threads.py b/.github/actions/pr-review/scripts/resolve-outdated-threads.py index f0273b2..e643f9f 100644 --- a/.github/actions/pr-review/scripts/resolve-outdated-threads.py +++ b/.github/actions/pr-review/scripts/resolve-outdated-threads.py @@ -35,14 +35,14 @@ id isResolved isOutdated - resolvedBy { login } + resolvedBy { login __typename } path line comments(first: 20) { totalCount nodes { body - author { login } + author { login __typename } } } } @@ -178,6 +178,17 @@ def severity_of(body: str) -> str: return "unknown" +def _is_person(actor: Optional[dict]) -> bool: + """Whether a GraphQL Actor is a human, by node type rather than by login. + + Every GitHub App is a `Bot`, so this excludes the other apps that write on + these PRs as well as this reviewer. Matching on the login instead would + need a list of every app anyone ever installs, and would miss them by + default — the direction that silently retires a finding. + """ + return bool(actor) and actor.get("__typename") == "User" + + def collect_prior_findings(threads: list[dict]) -> list[dict]: """Build the prior-findings list the review prompt audits against. @@ -198,6 +209,16 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: reply-free threads on every run, so most resolved threads are the action's doing. `resolvedBy` is what tells those apart from a person clicking Resolve, which people do far more often than they reply. + + "A person" is GraphQL's `__typename == "User"`, not "a login outside + BOT_LOGINS". Plenty of other apps write on these PRs — Copilot, + CodeRabbit, linear-code, dependabot — and counting any of them as + engagement would retire the finding from inline reporting for good. The + login is no help: GraphQL reports the GitHub Actions bot as + `github-actions`, with no `[bot]` suffix, which is why BOT_LOGINS carries + both spellings. BOT_LOGINS stays narrow on purpose — _review_state uses it + as a trust allowlist for summary authorship, which is a different question + from "is this commenter a human". """ findings = [] for thread in threads: @@ -232,17 +253,10 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: # not read. "human_engaged": ( thread["comments"]["totalCount"] > len(comments) - or any( - (c.get("author") or {}).get("login", "") not in BOT_LOGINS - for c in comments[1:] - ) - # Resolved by a person rather than by should_resolve. The bot's - # own resolutions carry a BOT_LOGINS resolver, so they stay - # false here and keep the re-anchor path. - or ( - (thread.get("resolvedBy") or {}).get("login", "") - not in BOT_LOGINS | {""} - ) + or any(_is_person(c.get("author")) for c in comments[1:]) + # Resolved by a person rather than by should_resolve, whose + # resolutions come back as a Bot and so keep the re-anchor path. + or _is_person(thread.get("resolvedBy")) ), }) findings.sort(key=lambda f: (f["thread_resolved"], f["path"] or "", f["line"] or 0)) diff --git a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py index d68556c..bcc10cc 100755 --- a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py +++ b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py @@ -1758,14 +1758,21 @@ def _thread( replies: list[tuple[str, str]] | None = None, resolved_by: str | None = None, ) -> dict: - nodes = [{"body": body, "author": {"login": author}}] + def actor(login: str) -> dict: + # GraphQL reports every GitHub App as a Bot, and the Actions bot's + # login comes back without the "[bot]" suffix. + kind = "Bot" if login in ("github-actions", "github-actions[bot]") \ + or login.endswith("[bot]") else "User" + return {"login": login, "__typename": kind} + + nodes = [{"body": body, "author": actor(author)}] for reply_author, reply_body in replies or []: - nodes.append({"body": reply_body, "author": {"login": reply_author}}) + nodes.append({"body": reply_body, "author": actor(reply_author)}) return { "id": "PRRT_x", "isResolved": resolved, "isOutdated": outdated, - "resolvedBy": {"login": resolved_by} if resolved_by else None, + "resolvedBy": actor(resolved_by) if resolved_by else None, "path": path, "line": line, "comments": {"totalCount": len(nodes), "nodes": nodes}, @@ -1866,6 +1873,38 @@ def _bot_thread_of_length(self, n: int, total: int | None = None) -> dict: thread["comments"]["totalCount"] = n if total is None else total return thread + def test_other_apps_are_not_engagement(self): + # Copilot, CodeRabbit, linear-code and friends write on these PRs. + # Counting any of them as a person would retire the finding from + # inline reporting permanently. + for app in ("copilot-pull-request-reviewer", "linear-code", "dependabot"): + with self.subTest(app=app): + replied = self._thread("🟠 Bug: x", replies=[(app + "[bot]", "noted")]) + self.assertFalse( + rot.collect_prior_findings([replied])[0]["human_engaged"] + ) + resolved = self._thread( + "🟠 Bug: x", resolved=True, outdated=True, + resolved_by=app + "[bot]", + ) + self.assertFalse( + rot.collect_prior_findings([resolved])[0]["human_engaged"] + ) + + def test_engagement_is_decided_by_node_type_not_login(self): + # A human login is a User even if it looks bot-ish, and the Actions + # bot is a Bot even though its login carries no "[bot]" suffix. + human = self._thread("🟠 Bug: x") + human["comments"]["nodes"].append( + {"body": "mine", "author": {"login": "robot-dev", "__typename": "User"}} + ) + human["comments"]["totalCount"] = 2 + self.assertTrue(rot.collect_prior_findings([human])[0]["human_engaged"]) + + bot = self._thread("🟠 Bug: x", resolved=True, outdated=True) + bot["resolvedBy"] = {"login": "github-actions", "__typename": "Bot"} + self.assertFalse(rot.collect_prior_findings([bot])[0]["human_engaged"]) + def test_silent_human_resolution_counts_as_engagement(self): # Resolving without replying is the common case, and the earlier rule # read it as "nobody engaged" and re-posted into it — the #147 loop by From c8a748338eb71752ab5ebda98832037d7fcaec24 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Thu, 22 Oct 2026 10:46:18 -0300 Subject: [PATCH 8/9] Test the resolver by login, since GraphQL types it as a User MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit moved both halves of human_engaged onto __typename. That is right for comment authors and wrong for the resolver, and the wrong half silently disabled the re-anchor path. PullRequestReviewThread.resolvedBy is the User OBJECT, not the Actor interface — introspection returns {"kind":"OBJECT","name":"User"} — so an app that resolves a thread is coerced into a User and reports __typename "User" like anyone else. Every bot resolution therefore read as a person, and a finding whose thread the bot closed was never re-anchored again. It is not hypothetical and GitHub does not return null for it either: c1-squire-dev[bot] resolved 37 threads on baton-datadog#40, each reported as User. The tests did not catch it because they asserted a shape the API never returns, {"__typename": "Bot"} on resolvedBy. So the two halves now use different tests, because the schema types them differently: authors by node type, resolvers by login. Under the User coercion the login keeps the "[bot]" suffix that the Bot node omits — github-actions/Bot as an author, c1-squire-dev[bot]/User as a resolver — which is also the reason BOT_LOGINS has to list both spellings. Both facts are checked against the live API and written down beside the code, since the asymmetry invites exactly the simplification that caused this. 187 tests, up from 186. The resolver cases now use real shapes and cover the app that was observed doing it. Co-Authored-By: Claude Opus 5 (1M context) --- .../scripts/resolve-outdated-threads.py | 59 ++++++++++++------- .../scripts/test_verdict_scaffolding.py | 37 +++++++++--- 2 files changed, 67 insertions(+), 29 deletions(-) diff --git a/.github/actions/pr-review/scripts/resolve-outdated-threads.py b/.github/actions/pr-review/scripts/resolve-outdated-threads.py index e643f9f..aeae22f 100644 --- a/.github/actions/pr-review/scripts/resolve-outdated-threads.py +++ b/.github/actions/pr-review/scripts/resolve-outdated-threads.py @@ -178,17 +178,38 @@ def severity_of(body: str) -> str: return "unknown" -def _is_person(actor: Optional[dict]) -> bool: - """Whether a GraphQL Actor is a human, by node type rather than by login. - - Every GitHub App is a `Bot`, so this excludes the other apps that write on - these PRs as well as this reviewer. Matching on the login instead would - need a list of every app anyone ever installs, and would miss them by - default — the direction that silently retires a finding. +def _author_is_person(actor: Optional[dict]) -> bool: + """Whether a comment author is a human. + + `PullRequestReviewComment.author` is the `Actor` INTERFACE, so every + GitHub App arrives as a `Bot` node and the type alone settles it. Matching + on the login instead would need a list of every app anyone ever installs, + and would miss them by default — the direction that silently retires a + finding from inline reporting. """ return bool(actor) and actor.get("__typename") == "User" +def _resolver_is_person(resolver: Optional[dict]) -> bool: + """Whether a thread was resolved by a human. + + This cannot use the same test. `PullRequestReviewThread.resolvedBy` is the + `User` OBJECT, not `Actor`, so an app that resolves a thread is coerced + into a User and its `__typename` is ALWAYS "User" — checking the type here + would read every bot resolution as a person and permanently retire the + finding. Observed on a real PR: `c1-squire-dev[bot]` resolved 37 threads, + each reported as `User`. + + Under that coercion the login keeps the `[bot]` suffix (`c1-squire-dev[bot]`), + which it does NOT carry as a `Bot` node — which is also why BOT_LOGINS has + to list `github-actions` both ways. + """ + if not resolver: + return False + login = resolver.get("login", "") + return bool(login) and login not in BOT_LOGINS and not login.endswith("[bot]") + + def collect_prior_findings(threads: list[dict]) -> list[dict]: """Build the prior-findings list the review prompt audits against. @@ -210,15 +231,13 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: doing. `resolvedBy` is what tells those apart from a person clicking Resolve, which people do far more often than they reply. - "A person" is GraphQL's `__typename == "User"`, not "a login outside - BOT_LOGINS". Plenty of other apps write on these PRs — Copilot, - CodeRabbit, linear-code, dependabot — and counting any of them as - engagement would retire the finding from inline reporting for good. The - login is no help: GraphQL reports the GitHub Actions bot as - `github-actions`, with no `[bot]` suffix, which is why BOT_LOGINS carries - both spellings. BOT_LOGINS stays narrow on purpose — _review_state uses it - as a trust allowlist for summary authorship, which is a different question - from "is this commenter a human". + "A person" is decided differently for the two halves, because GraphQL + types them differently — see _author_is_person and _resolver_is_person. + Either way it is not "a login outside BOT_LOGINS": plenty of other apps + write on these PRs, and counting one as engagement retires the finding + from inline reporting for good. BOT_LOGINS stays narrow on purpose — + _review_state uses it as a trust allowlist for summary authorship, which + is a different question from "is this commenter a human". """ findings = [] for thread in threads: @@ -253,10 +272,10 @@ def collect_prior_findings(threads: list[dict]) -> list[dict]: # not read. "human_engaged": ( thread["comments"]["totalCount"] > len(comments) - or any(_is_person(c.get("author")) for c in comments[1:]) - # Resolved by a person rather than by should_resolve, whose - # resolutions come back as a Bot and so keep the re-anchor path. - or _is_person(thread.get("resolvedBy")) + or any(_author_is_person(c.get("author")) for c in comments[1:]) + # Resolved by a person rather than by should_resolve or any + # other automation, which keeps the re-anchor path for those. + or _resolver_is_person(thread.get("resolvedBy")) ), }) findings.sort(key=lambda f: (f["thread_resolved"], f["path"] or "", f["line"] or 0)) diff --git a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py index bcc10cc..21db83e 100755 --- a/.github/actions/pr-review/scripts/test_verdict_scaffolding.py +++ b/.github/actions/pr-review/scripts/test_verdict_scaffolding.py @@ -1772,7 +1772,11 @@ def actor(login: str) -> dict: "id": "PRRT_x", "isResolved": resolved, "isOutdated": outdated, - "resolvedBy": actor(resolved_by) if resolved_by else None, + # resolvedBy is the User OBJECT, so even an app comes back typed + # "User" — with the "[bot]" suffix the Bot node does not carry. + "resolvedBy": ( + {"login": resolved_by, "__typename": "User"} if resolved_by else None + ), "path": path, "line": line, "comments": {"totalCount": len(nodes), "nodes": nodes}, @@ -1874,10 +1878,11 @@ def _bot_thread_of_length(self, n: int, total: int | None = None) -> dict: return thread def test_other_apps_are_not_engagement(self): - # Copilot, CodeRabbit, linear-code and friends write on these PRs. + # Copilot, CodeRabbit, linear-code and friends write on these PRs, and + # c1-squire-dev[bot] has resolved 37 threads on a real connector PR. # Counting any of them as a person would retire the finding from # inline reporting permanently. - for app in ("copilot-pull-request-reviewer", "linear-code", "dependabot"): + for app in ("copilot-pull-request-reviewer", "linear-code", "c1-squire-dev"): with self.subTest(app=app): replied = self._thread("🟠 Bug: x", replies=[(app + "[bot]", "noted")]) self.assertFalse( @@ -1891,9 +1896,9 @@ def test_other_apps_are_not_engagement(self): rot.collect_prior_findings([resolved])[0]["human_engaged"] ) - def test_engagement_is_decided_by_node_type_not_login(self): - # A human login is a User even if it looks bot-ish, and the Actions - # bot is a Bot even though its login carries no "[bot]" suffix. + def test_author_engagement_is_decided_by_node_type(self): + # `author` is the Actor interface, so an app is a Bot node and a human + # login that looks bot-ish is still a User. human = self._thread("🟠 Bug: x") human["comments"]["nodes"].append( {"body": "mine", "author": {"login": "robot-dev", "__typename": "User"}} @@ -1901,9 +1906,23 @@ def test_engagement_is_decided_by_node_type_not_login(self): human["comments"]["totalCount"] = 2 self.assertTrue(rot.collect_prior_findings([human])[0]["human_engaged"]) - bot = self._thread("🟠 Bug: x", resolved=True, outdated=True) - bot["resolvedBy"] = {"login": "github-actions", "__typename": "Bot"} - self.assertFalse(rot.collect_prior_findings([bot])[0]["human_engaged"]) + def test_resolver_engagement_cannot_use_node_type(self): + # `resolvedBy` is the User OBJECT, so an app that resolves a thread is + # reported as "User" too — keying on the type here would read every + # bot resolution as a person and kill the re-anchor path. Shapes taken + # from the live API. + for login, engaged in ( + ("c1-squire-dev[bot]", False), # observed on baton-datadog#40 + ("github-actions[bot]", False), + ("github-actions", False), # BOT_LOGINS carries both spellings + ("octocat", True), + ): + with self.subTest(login=login): + t = self._thread("🟠 Bug: x", resolved=True, outdated=True) + t["resolvedBy"] = {"login": login, "__typename": "User"} + self.assertEqual( + rot.collect_prior_findings([t])[0]["human_engaged"], engaged + ) def test_silent_human_resolution_counts_as_engagement(self): # Resolving without replying is the common case, and the earlier rule From 12b1212e70872edd74823c0fecbc8b84633a7c4f Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Mon, 26 Oct 2026 08:28:31 -0300 Subject: [PATCH 9/9] Cap one issue to one fresh comment per audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The case list decided per entry, and nothing stopped one issue from getting several fresh comments in a single run. An issue with two stale threads: the first entry finds its sibling open but outdated, so "open and accurate" does not match and it posts; the second entry finds the first sibling resolved and posts too. Two threads for one issue in one run, four after the next change at that spot. Under resolution_blocked it is the same shape and permanent, since nothing is ever auto-resolved and the stale threads only pile up — the mode the prompt was just taught to recognise without being told what to do differently in it. Third clause on the covering case: do not post if you already posted for this issue earlier in this audit. No new data from the script — the audit knows what it has already done. The three clauses now cover the three ways the same issue gets raised twice: something live covers it, a person dealt with it, or you just covered it yourself. Not a regression — main behaves this way too — but the description claimed that case stops accumulation by any route, and now it does. Merging main also picks up scripts/test-check-image-locks.py from #157, which the required image-locks check runs and this branch predated. Co-Authored-By: Claude Opus 5 (1M context) --- .../actions/pr-review/prompts/base-pr-review.md | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/.github/actions/pr-review/prompts/base-pr-review.md b/.github/actions/pr-review/prompts/base-pr-review.md index b2bff34..73c0ad0 100644 --- a/.github/actions/pr-review/prompts/base-pr-review.md +++ b/.github/actions/pr-review/prompts/base-pr-review.md @@ -155,13 +155,19 @@ flagged location and assign exactly one verdict: something a person already dealt with belongs in the summary, where it is said once. - **Another thread for this same issue is open and accurate, OR has - `human_engaged` true** — do not post a duplicate. Prior findings are one + `human_engaged` true, OR you already posted a fresh comment for this issue + earlier in this audit** — do not post a duplicate. Prior findings are one entry per thread, not per issue, so one issue appears here once per thread it has ever had, and the bot's own superseded threads stay in that list - forever. Without the `human_engaged` half, dealing with the newest thread - would still leave an older bot-resolved one matching the outdated case - below, and it would post again — and again after that one is dealt with - too. One person dealing with any thread for an issue settles all of them. + forever. Each of the three clauses closes a different way the same issue + gets raised twice: a live thread already covers it, a person already dealt + with it, or you covered it yourself a moment ago. The last one matters + because the audit walks entries, not issues: without it, an issue with two + stale threads gets two fresh comments in a single run, four after the next + change, and so on. That is also the shape to expect when + `resolution_blocked` is true — nothing is ever auto-resolved, so stale + threads only accumulate, and this clause is the only thing holding the + line. - **The thread is outdated and nothing above covers the issue** — post a fresh inline comment at the current location. Nothing on the PR points at it otherwise, and this is also the path that recovers a finding whose