fix(claude-review): don't let a partial post's landed findings rename the rest

.github/scripts/claude_pr_review.py

@@ -296,6 +296,7 @@ def select_findings(
     pr_lines: DiffLines,
     interdiff_lines: DiffLines,
     suppress: set[str],
+    live: set[str] = frozenset(),
 ) -> tuple[list[Finding], list[Resolution], list[str]]:
     """Validate the agent's output and apply the posting rules.
 
@@ -305,6 +306,10 @@ def select_findings(
     - Its fp must not already be suppressed. Two findings in one run that
       share an fp are distinct issues on the same symbol (the agent does not
       repeat itself within a run), so later ones get a numeric suffix.
+      Suffixes skip only `suppress`, the threads seen when the review began,
+      so a re-run of the post job names each finding the same way. `live`
+      holds the threads on the PR now; a finding whose final fp is among
+      them already landed (in an earlier attempt of this job) and is dropped.
     - Incremental reviews post no nits, and post should-fix findings only on
       lines this revision changed; blocking findings may land anywhere in the
       PR diff, since a missed blocker is worth raising late. Old-side line
@@ -364,6 +369,9 @@ def select_findings(
             while fp in seen or fp in suppress:
                 fp, n = f"{base}-{n}", n + 1
             seen.add(fp)
+            if fp in live:
+                dropped.append(f"{fp} ({path}:{line}): already posted")
+                continue
             findings.append(Finding(path=path, line=line, severity=severity, fp=fp, body=body, side=side))
     return findings, resolutions, dropped
 
@@ -689,9 +697,9 @@ def close_addressed(env: Env, threads: list[Thread], resolutions: list[Resolutio
 def post(env: Env, *, agent_ok: bool, agent_output: str, mode: str, prior_sha: str | None) -> None:
     state_path = env.context_dir / "prior-review-state.json"
     suppress = set(json.loads(state_path.read_text(encoding="utf-8"))["suppress"]) if state_path.exists() else set()
-    # Also suppress what is on the PR now: a re-run of this job after a
-    # partial post must not post the findings that did land a second time.
-    suppress |= suppressed_fps(fetch_threads(env.repo, env.pr))
+    # What is on the PR now: a re-run of this job after a partial post must not
+    # post the findings that did land a second time.
+    live = suppressed_fps(fetch_threads(env.repo, env.pr))
     pr_lines = diff_lines((env.context_dir / "pr.diff").read_text(encoding="utf-8"))
     interdiff_path = env.context_dir / "interdiff.diff"
     interdiff_lines = diff_lines(interdiff_path.read_text(encoding="utf-8") if interdiff_path.exists() else "")
@@ -704,7 +712,7 @@ def post(env: Env, *, agent_ok: bool, agent_output: str, mode: str, prior_sha: s
         parsed = ([], [])
     records, errors = parsed
     findings, resolutions, dropped = select_findings(
-        records, mode=mode, pr_lines=pr_lines, interdiff_lines=interdiff_lines, suppress=suppress
+        records, mode=mode, pr_lines=pr_lines, interdiff_lines=interdiff_lines, suppress=suppress, live=live
     )
     for msg in errors + dropped:
         print(f"skipped: {msg}")

test/test_claude_pr_review.py

@@ -271,6 +271,21 @@ def test_same_fp_in_one_run_is_disambiguated(self):
         )
         self.assertEqual(dropped, [])
 
+    def test_suffix_never_collides_with_a_literal_symbol(self):
+        findings, _, _ = self._select([_finding(), _finding(body="Other."), _finding(symbol="new-2", body="Literal.")])
+        self.assertEqual(
+            [f.fp for f in findings],
+            ["src/app.py::correctness::new", "src/app.py::correctness::new-2", "src/app.py::correctness::new-2-2"],
+        )
+
+    def test_suffixed_finding_survives_when_only_the_first_already_landed(self):
+        findings, _, dropped = review.select_findings(
+            [_finding(), _finding(body="Other bug.")], mode="full", pr_lines=self.PR_LINES,
+            interdiff_lines=self.INTERDIFF, suppress=set(), live={"src/app.py::correctness::new"},
+        )
+        self.assertEqual([f.fp for f in findings], ["src/app.py::correctness::new-2"])
+        self.assertEqual(len(dropped), 1)
+
     def test_incremental_rules(self):
         findings, _, dropped = self._select(
             [
@@ -374,6 +389,7 @@ def setUp(self):
         self.live = []  # thread nodes on the PR
         self.calls = []
         self.fail_posts = set()  # paths whose comments GitHub rejects
+        self.fail_bodies = set()  # comments GitHub rejects when the body contains one of these
         self.summary_body = None
 
         def fake_gh(*args, input_json=None, check=True):
@@ -385,13 +401,13 @@ def fake_gh(*args, input_json=None, check=True):
                 return {"data": {"repository": {"pullRequest": {"reviewThreads": {
                     "pageInfo": {"hasNextPage": False, "endCursor": None}, "nodes": list(self.live)}}}}}
             if path.endswith("/reviews"):
-                if any(c["path"] in self.fail_posts for c in input_json["comments"]):
+                if any(self._rejects(c) for c in input_json["comments"]):
                     return None
                 for c in input_json["comments"]:
                     self._land(c)
                 return {}
             if path.endswith("/pulls/1/comments"):
-                if input_json["path"] in self.fail_posts:
+                if self._rejects(input_json):
                     return None
                 self._land(input_json)
                 return {}
@@ -413,6 +429,9 @@ def tearDown(self):
         review.gh = self._orig
         self.tmp.cleanup()
 
+    def _rejects(self, comment):
+        return comment["path"] in self.fail_posts or any(b in comment["body"] for b in self.fail_bodies)
+
     def _land(self, comment):
         self.live.append({
             "id": f"T{len(self.live)}", "isResolved": False, "isOutdated": False,
@@ -459,6 +478,16 @@ def test_rerun_after_partial_post_does_not_duplicate(self):
         self.assertEqual(self._posted_paths(), ["src/app.py", "new.py"])
         self.assertIn(f"reviewed={SHA_A}", self.summary_body)
 
+    def test_rerun_after_partial_post_keeps_a_suffixed_finding(self):
+        findings = [_finding(), _finding(body="Second bug on the same symbol.")]
+        self.fail_bodies = {"Second bug"}
+        self._post(findings)
+        self.fail_bodies = set()
+        self._post(findings)
+        fps = [review.parse_fp_marker(n["comments"]["nodes"][0]["body"])[0] for n in self.live]
+        self.assertEqual(fps, ["src/app.py::correctness::new", "src/app.py::correctness::new-2"])
+        self.assertIn(f"reviewed={SHA_A}", self.summary_body)
+
 
 if __name__ == "__main__":
     unittest_main()