From cbbefe5df3c74a450839326f4540457b1e8a0a56 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Mon, 21 Sep 2026 14:33:33 -0300 Subject: [PATCH] Stop the reviewer re-posting inline findings it already delivered The review prompt tells the model to skip issues already raised in an inline review comment, but nothing ever gave it those comments. fetch-pr-context.py builds existing_findings by scanning bot summary comments for lines starting with "- " that contain backticks; inline threads are never collected. So an open inline finding on unchanged code was invisible to the next run and got posted again, verbatim. On ConductorOne/baton-admin#928 that produced 19 inline comments for 11 distinct findings: after each push the reviewer re-posted every still-open finding byte-for-byte, burying the ones that were actually new. resolve-outdated-threads.py already fetches every thread with its path, line, bodies, authors and resolution state, and throws all of it away except the outdated ones it resolves. It now also writes open_findings and settled_findings, and the prompt's Step 3 and Step 6 dedup against them. A finding whose thread has a human reply is marked so the reviewer leaves it alone entirely. Step 3 says where a still-relevant repeat belongs instead: one line in the summary, not a second inline copy. Auto-resolution behavior is unchanged; tests cover that it did not widen. Also wire the pr-review script tests into make test-scripts. Both existing test files were run by nothing -- not the Makefile, not any workflow. Co-Authored-By: Claude Opus 5 (1M context) --- .../pr-review/prompts/base-pr-review.md | 36 +++++- .../scripts/resolve-outdated-threads.py | 56 +++++++++ .../scripts/test_resolve_outdated_threads.py | 109 ++++++++++++++++++ Makefile | 1 + 4 files changed, 196 insertions(+), 6 deletions(-) create mode 100644 .github/actions/pr-review/scripts/test_resolve_outdated_threads.py diff --git a/.github/actions/pr-review/prompts/base-pr-review.md b/.github/actions/pr-review/prompts/base-pr-review.md index 7e79704..d160106 100644 --- a/.github/actions/pr-review/prompts/base-pr-review.md +++ b/.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) diff --git a/.github/actions/pr-review/scripts/resolve-outdated-threads.py b/.github/actions/pr-review/scripts/resolve-outdated-threads.py index d4cc802..640df25 100644 --- a/.github/actions/pr-review/scripts/resolve-outdated-threads.py +++ b/.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") diff --git a/.github/actions/pr-review/scripts/test_resolve_outdated_threads.py b/.github/actions/pr-review/scripts/test_resolve_outdated_threads.py new file mode 100644 index 0000000..f1964ca --- /dev/null +++ b/.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() diff --git a/Makefile b/Makefile index 6bc157c..707cd66 100644 --- a/Makefile +++ b/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: