Repository navigation
[CXH-3051] Stop re-raising findings a human already closed - #156
Conversation
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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Superseded — see the current review report for commit
|
…swered 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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Superseded by a completed review with no blocking issues; current audit evidence persisted in CI.
…lved 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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Superseded by a completed review with no blocking issues; current audit evidence persisted in CI.
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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
sergiocorral-conductorone
left a comment
sergiocorral-conductorone
left a comment
There was a problem hiding this comment.
Automated review. drafted 1 → confirmed 0, low-confidence 1, dropped 0, added 0 (panel: opus-5.5 / gpt-6-astra / gemini-4-argon, all high). The logic is sound and tests cover the new paths; one suggestion inline. Linked ticket CXH-3051 body could not be read, so its Fix-list coverage was not checked.
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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
…-reposting-resolved
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) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
General PR Review: [CXH-3051] Stop re-raising findings a human already closedBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review Summary
I traced the convergence cases and found none that adds a thread on every run:
There are no repo-local criteria, so the base criteria plus the SDK/shared-library compatibility checks were applied. The Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Resolved prior findings
Reviewed commit: |
A resolved review thread gets re-posted as a new inline thread on the next run, and again on the one after that. #147 ended with one finding spread across six threads, five of them redundant, out of 21 threads on the PR — roughly a quarter of its review conversation was the same accepted comment repeated.
Why it happens
The reviewer was not malfunctioning. It was following Step 3 of the base prompt literally, which gave it exactly two cases:
A resolved thread is neither. It falls through to "post a fresh comment".
And what makes it recur rather than happen once: an outdated thread stays outdated forever. Every later run sees the same permanently-outdated original, takes the same branch, and opens another thread — which then becomes another prior finding that can re-post in turn.
The tell that this is not ordinary anchor drift: one of the re-posts on #147 landed when
action.ymlhad not changed at all — identical md5, three README-only commits in between — on a different line of the same untouched block. Each time, the bot explained itself accurately: "I'm re-posting only because the earlier thread is outdated."flowchart TD T["Thread for finding X:<br/>resolved, and outdated because the code moved"] T --> S["Next run. Step 3 sees 'still present' and outdated,<br/>so it says post a fresh inline comment."] S --> N["A NEW thread for X.<br/>The original is still resolved AND still outdated."] N --> T2["The run after that sees both,<br/>and the original never stops being outdated."]The fix
The rule now turns on whether a person has dealt with the thread, not on whether it is resolved.
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 own doing rather than anyone's decision. Keying on that read a bot sweep as "someone closed this deliberately", while a thread a person had actually replied to — but not resolved — stayed outdated and unresolved forever and kept re-posting.
So
collect_prior_findingsreportshuman_engaged: a person wrote in the thread, or resolved it themselves. Both say "I have dealt with this", and neither should be answered by opening the same thread again.resolvedBy { login }is new in the query; everything else comes from comments it already fetched. The bot's own resolutions carry aBOT_LOGINSresolver, so they stay false and keep the path that re-anchors a finding whose thread was closed with no replacement posted.Step 3's cases, in order:
human_engagedThat second case is what stops accumulation by any route: prior findings are one entry per thread, not per issue, so after a re-post the old entry kept matching "post fresh" while the new one matched "do not duplicate", adding another thread every run.
The finding is never silenced — only its duplicate inline thread is. It is still audited against current code and still counted in the summary at its severity, so a resolved-but-unfixed blocker still blocks. Step 6 now points at these cases rather than restating them; two copies of one rule is how the original bug got here.
The prompt is also told about
human_engagedandresolution_blocked, neither of which it previously saw — the second meaning the reviewer could not tell it was in the degraded mode where the token cannot resolve threads at all.Tests
184, up from 175. New coverage: a reply counts as engagement, and still counts when a bot comment lands after it; a silent human resolution counts; a bot resolution does not, so the re-anchor path survives; an open thread's null resolver does not read as a person; the truncation boundary at 20-of-20 and 20-of-21, which is the shape
comments(first: 20)can actually return; and the pre-mutation snapshotmain()depends on, 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.Prompt rules remain untested by construction — nothing in the repo asserts prompt content, and the functional change here lives in the prompt.
Scope
Deliberately not included: deduplicating the threads that already exist. This stops new ones accumulating; it does not retroactively collapse a PR that already has six.
🤖 Generated with Claude Code