From e83940fd64950289e9e4cd6cb556b82e5b431d80 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 7 Oct 2026 10:37:08 -0700 Subject: [PATCH] fix(claude-review): don't let a partial post's landed findings rename the rest Two findings that share a base fingerprint post as base and base-2. If base posted but base-2 failed, a re-run of the post job added the live threads to the suppress list it names findings against. base was now suppressed, so both findings were dropped; the re-run reported nothing unposted and advanced the baseline, losing base-2 for good. Suffixes now skip only the suppress list captured when the review began, which a re-run reads from the same artifact, so each finding gets the same name in every attempt. Threads live on the PR at post time are checked after naming and drop only an exact match. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .github/scripts/claude_pr_review.py | 16 ++++++++++---- test/test_claude_pr_review.py | 33 +++++++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/.github/scripts/claude_pr_review.py b/.github/scripts/claude_pr_review.py index d05f7252..54019094 100644 --- a/.github/scripts/claude_pr_review.py +++ b/.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}") diff --git a/test/test_claude_pr_review.py b/test/test_claude_pr_review.py index 934cb21c..dc2ef80a 100644 --- a/test/test_claude_pr_review.py +++ b/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()