[CXH-3051] Stop re-raising findings a human already closed

.github/actions/pr-review/prompts/base-pr-review.md

@@ -127,10 +127,15 @@ 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 `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
+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
@@ -140,11 +145,48 @@ 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.
+  Work down these in order and stop at the first that matches:
+  - **`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 thread for this same issue is open and accurate, OR has
+    `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. 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
+    thread was resolved automatically rather than by a person.
+  - **Otherwise** (thread open and accurate) — do not post a duplicate.
+
+  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.
+
+  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.
@@ -252,10 +294,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, 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.
+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

.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) {
@@ -31,13 +35,14 @@
           id
           isResolved
           isOutdated
+          resolvedBy { login __typename }
           path
           line
           comments(first: 20) {
             totalCount
             nodes {
               body
-              author { login }
+              author { login __typename }
             }
           }
         }
@@ -173,6 +178,38 @@ def severity_of(body: str) -> str:
     return "unknown"
 
 
+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.
 
@@ -181,6 +218,26 @@ 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.
+
+    `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 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.
+
+    "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:
@@ -200,6 +257,26 @@ 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 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".
+            #
+            # 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.
+            "human_engaged": (
+                thread["comments"]["totalCount"] > len(comments)
+                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))
     return findings
@@ -286,6 +363,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")