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()