Stop the reviewer re-posting inline findings it already delivered

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

@@ -58,11 +58,31 @@ are safe solely because they were filtered out of the incremental artifact.
 Do not use local git history for incremental review. The local checkout is the current
 PR head tree, not the previous reviewed tree.
 
-### Step 3 — Note pre-resolved threads
+### Step 3 — Note what you have already said on this PR
 
-Read `.github/resolved-threads.json` — it contains a summary of outdated bot review threads
-that were automatically resolved before this review started. Use `resolved_count` from this
-file when reporting "Threads Resolved" in the summary.
+Read `.github/resolved-threads.json`. It summarizes the review threads you own on this PR:
+
+- `resolved_count`: outdated threads auto-resolved before this review started. Use this
+  number for "Threads Resolved" in the summary.
+- `open_findings`: inline findings you posted on an earlier run that are **still open**. Each
+  entry has `path`, `line`, the finding `body`, and `has_human_reply`.
+- `settled_findings`: inline findings that are now resolved, whether by this run or by a human.
+- `findings_truncated`: true when either list was capped; if so, be more conservative about
+  claiming a finding is new.
+
+**Every entry in either list is a finding you have already delivered. Do not post it again.**
+This is not a style preference — re-posting is the single most common defect in this review's
+output. The author already sees the earlier comment on that line; a byte-identical second copy
+adds nothing and buries the findings that are actually new.
+
+If the underlying issue still exists and still matters, say so **once** in the Review Summary
+section of the summary comment (e.g. "the pagination finding from the previous run is still
+open"), and leave the original inline thread to carry the detail. Post a new inline comment
+only when you have something genuinely new to say about that line: a different defect, or the
+same defect whose shape changed because the code changed.
+
+An entry with `has_human_reply: true` has been discussed. Do not re-raise it at all; if you
+believe it is unresolved, the summary is the place to say so.
 
 ### Step 4 — Use Trusted Repo-Local Review Criteria
 
@@ -135,8 +155,12 @@ 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.
 
-Skip any issue that was already raised in an existing PR comment or inline review comment.
-Do not re-flag issues on unchanged code that were pre-resolved (see step 3).
+Skip any issue already raised on this PR. Check candidates against all three sources before
+posting: `existing_findings` (previous summary lines) and `comments` from
+`.github/pr-context.json`, and `open_findings` / `settled_findings` from
+`.github/resolved-threads.json` (see Step 3). A candidate that matches an entry in any of them
+by file, line and substance has already been reported — drop it from posted output, whether or
+not it is fixed. Do not re-flag issues on unchanged code that were pre-resolved.
 
 ### Step 7 — Post results directly (new findings only)
 

.github/actions/pr-review/scripts/resolve-outdated-threads.py

@@ -144,6 +144,37 @@ def should_resolve(thread: dict) -> bool:
     return any(body.startswith(prefix) for prefix in REVIEW_PREFIXES)
 
 
+MAX_FINDING_BODY = 600
+MAX_FINDINGS_PER_BUCKET = 60
+
+
+def is_bot_finding_thread(thread: dict) -> bool:
+    """True when the thread was opened by the reviewer with a review-prefixed body."""
+    comments = thread["comments"]["nodes"]
+    if not comments:
+        return False
+    first = comments[0]
+    if (first.get("author") or {}).get("login", "") not in BOT_LOGINS:
+        return False
+    body = first.get("body", "")
+    return any(body.startswith(prefix) for prefix in REVIEW_PREFIXES)
+
+
+def finding_digest(thread: dict) -> dict:
+    """Compact record of one reviewer finding, for dedup on the next run."""
+    comments = thread["comments"]["nodes"]
+    body = comments[0].get("body", "") if comments else ""
+    return {
+        "path": thread["path"],
+        "line": thread.get("line"),
+        "body": body[:MAX_FINDING_BODY],
+        "has_human_reply": any(
+            (c.get("author") or {}).get("login", "") not in BOT_LOGINS
+            for c in comments[1:]
+        ),
+    }
+
+
 def resolve_thread(thread_id: str) -> bool:
     """Resolve a single review thread. Returns True on success."""
     try:
@@ -201,15 +232,40 @@ def main():
                 "body_preview": body_preview,
             })
 
+    # Everything the reviewer has already said on this PR, so the next run does not
+    # say it again. Inline findings never reached the review prompt before: the PR
+    # context only carried summary-comment lines, so an open inline thread on
+    # unchanged code was invisible and got re-posted verbatim after every push.
+    resolved_ids = {t["id"] for t in to_resolve}
+    open_findings, settled_findings = [], []
+    for thread in threads:
+        if not is_bot_finding_thread(thread):
+            continue
+        if thread["isResolved"] or thread["id"] in resolved_ids:
+            settled_findings.append(finding_digest(thread))
+        else:
+            open_findings.append(finding_digest(thread))
+
     summary = {
         "total_threads": len(threads),
         "outdated_bot_threads": len(to_resolve),
         "resolved_count": len(resolved),
         "resolved": resolved,
+        "open_findings": open_findings[:MAX_FINDINGS_PER_BUCKET],
+        "settled_findings": settled_findings[:MAX_FINDINGS_PER_BUCKET],
+        "findings_truncated": (
+            len(open_findings) > MAX_FINDINGS_PER_BUCKET
+            or len(settled_findings) > MAX_FINDINGS_PER_BUCKET
+        ),
     }
 
     write_summary(summary)
 
+    print(
+        f"  {len(open_findings)} open and {len(settled_findings)} settled reviewer "
+        "findings recorded for dedup"
+    )
+
     print(f"\nDone: resolved {len(resolved)}/{len(to_resolve)} threads")
 
 

.github/actions/pr-review/scripts/test_resolve_outdated_threads.py

@@ -0,0 +1,109 @@
+#!/usr/bin/env python3
+"""Unit tests for reviewer-finding bookkeeping in resolve-outdated-threads.py.
+
+The module file name contains hyphens, so it is loaded by path via importlib
+rather than imported normally. Run with:
+
+    python3 -m unittest discover -s .github/actions/pr-review/scripts -p 'test_*.py'
+
+or directly:
+
+    python3 .github/actions/pr-review/scripts/test_resolve_outdated_threads.py
+"""
+
+import importlib.util
+import os
+import unittest
+
+_SCRIPT = os.path.join(os.path.dirname(__file__), "resolve-outdated-threads.py")
+_spec = importlib.util.spec_from_file_location("resolve_outdated_threads", _SCRIPT)
+rot = importlib.util.module_from_spec(_spec)
+_spec.loader.exec_module(rot)
+
+
+def thread(body="🟡 Suggestion: something", author="github-actions[bot]",
+           replies=(), path="pkg/x.go", line=10, resolved=False, outdated=False):
+    nodes = [{"body": body, "author": {"login": author}}]
+    nodes.extend({"body": b, "author": {"login": a}} for a, b in replies)
+    return {
+        "id": f"T{path}{line}",
+        "isResolved": resolved,
+        "isOutdated": outdated,
+        "path": path,
+        "line": line,
+        "comments": {"totalCount": len(nodes), "nodes": nodes},
+    }
+
+
+class IsBotFindingThread(unittest.TestCase):
+    def test_accepts_review_prefixed_bot_thread(self):
+        self.assertTrue(rot.is_bot_finding_thread(thread()))
+
+    def test_rejects_human_authored_thread(self):
+        self.assertFalse(rot.is_bot_finding_thread(thread(author="felipe")))
+
+    def test_rejects_bot_thread_without_review_prefix(self):
+        self.assertFalse(rot.is_bot_finding_thread(thread(body="deploy preview ready")))
+
+    def test_accepts_bot_thread_a_human_replied_to(self):
+        # A human reply must not make the thread invisible: that is exactly the
+        # thread we most need to avoid re-posting.
+        self.assertTrue(
+            rot.is_bot_finding_thread(thread(replies=[("felipe", "fixed, thanks")]))
+        )
+
+    def test_rejects_empty_thread(self):
+        empty = thread()
+        empty["comments"] = {"totalCount": 0, "nodes": []}
+        self.assertFalse(rot.is_bot_finding_thread(empty))
+
+
+class FindingDigest(unittest.TestCase):
+    def test_carries_location_and_body(self):
+        d = rot.finding_digest(thread(body="🔴 Security: leak", path="a/b.go", line=42))
+        self.assertEqual(d["path"], "a/b.go")
+        self.assertEqual(d["line"], 42)
+        self.assertEqual(d["body"], "🔴 Security: leak")
+
+    def test_truncates_long_bodies(self):
+        d = rot.finding_digest(thread(body="🟠 Bug: " + "x" * 5000))
+        self.assertEqual(len(d["body"]), rot.MAX_FINDING_BODY)
+
+    def test_flags_human_reply(self):
+        self.assertFalse(rot.finding_digest(thread())["has_human_reply"])
+        self.assertTrue(
+            rot.finding_digest(
+                thread(replies=[("felipe", "disagree")])
+            )["has_human_reply"]
+        )
+
+    def test_bot_reply_is_not_a_human_reply(self):
+        self.assertFalse(
+            rot.finding_digest(
+                thread(replies=[("github-actions[bot]", "still open")])
+            )["has_human_reply"]
+        )
+
+    def test_tolerates_missing_line(self):
+        t = thread()
+        del t["line"]
+        self.assertIsNone(rot.finding_digest(t)["line"])
+
+
+class ShouldResolveUnchanged(unittest.TestCase):
+    """The dedup bookkeeping must not widen what gets auto-resolved."""
+
+    def test_outdated_bot_thread_still_resolves(self):
+        self.assertTrue(rot.should_resolve(thread(outdated=True)))
+
+    def test_outdated_thread_with_human_reply_still_does_not_resolve(self):
+        self.assertFalse(
+            rot.should_resolve(thread(outdated=True, replies=[("felipe", "no")]))
+        )
+
+    def test_current_bot_thread_does_not_resolve(self):
+        self.assertFalse(rot.should_resolve(thread(outdated=False)))
+
+
+if __name__ == "__main__":
+    unittest.main()

Makefile

@@ -24,6 +24,7 @@ test-scripts:
 	bash scripts/test-release-workflow-tag-pin.sh
 	bash scripts/test-s3-release-uploads.sh
 	if command -v pwsh >/dev/null 2>&1; then pwsh -NoProfile -File scripts/test-s3-release-uploads.ps1; else echo "pwsh not found; skipping PowerShell S3 release upload tests"; fi
+	python3 -m unittest discover -s .github/actions/pr-review/scripts -p 'test_*.py'
 
 .PHONY: workflow-validate
 workflow-validate: