From 702019ddba04a8beca68bfe4473fb44594753116 Mon Sep 17 00:00:00 2001 From: William Aaron Cheung Date: Tue, 4 Aug 2026 11:03:11 +0800 Subject: [PATCH] Tolerate prior-finding dispositions for externally resolved findings A reviewer who resolves a bot thread on GitHub while a review round is queued or running makes prepare's sync_manifest_threads close that finding, but the manifest handed to the model still contains it. The model reads the thread, agrees it is fixed, and returns a `resolved` disposition -- and compile rejected the whole round because the ID was not in the open set, discarding an otherwise clean review pass. Validate prior_findings IDs against manifest membership rather than open status, so only a genuinely unknown ID is a contract violation. For a finding already resolved with `thread_resolution: confirmed`, ignore the model's disposition entirely: a concordant `resolved` is a no-op, and an `open` must not reopen a thread a human deliberately closed. Tell the model in the prompt to disposition only findings whose status is `open`, so the tolerance path is a backstop rather than the normal case. --- .github/actions/claude-pr-review/action.yml | 4 ++ .../claude-pr-review/review_pipeline.py | 22 ++++++-- .../claude-pr-review/test_review_pipeline.py | 54 ++++++++++++++++++- 3 files changed, 75 insertions(+), 5 deletions(-) diff --git a/.github/actions/claude-pr-review/action.yml b/.github/actions/claude-pr-review/action.yml index c6509f9..be537f8 100644 --- a/.github/actions/claude-pr-review/action.yml +++ b/.github/actions/claude-pr-review/action.yml @@ -214,6 +214,10 @@ runs: (`open` or `resolved`) for each finding you assessed. An omitted finding remains open; only an explicit `resolved` disposition closes one. Never resolve human-authored threads. + Disposition only findings whose manifest `status` is `open`. A finding + already carrying another status was closed outside this round; it appears + in the manifest as context so you do not re-raise it, and a disposition + for it is ignored. Do the same for every open manifest question: return a prior_questions disposition (`open`, `answered`, or `withdrawn`) with a one-line reason. An omitted question stays open. Disposition each question from the whole diff --git a/.github/actions/claude-pr-review/review_pipeline.py b/.github/actions/claude-pr-review/review_pipeline.py index 1c8f090..819b764 100644 --- a/.github/actions/claude-pr-review/review_pipeline.py +++ b/.github/actions/claude-pr-review/review_pipeline.py @@ -1543,10 +1543,10 @@ def compile_review( } model_output = validate_model_output(model_output) - expected_prior_ids = { + known_prior_ids = { item_id for item_id, item in manifest["findings"].items() - if isinstance(item, dict) and item.get("status") == "open" + if isinstance(item, dict) } returned_prior_ids = [ str(item.get("finding_id") or "") @@ -1558,10 +1558,10 @@ def compile_review( "prior_findings contains duplicate finding IDs", code="PRIOR_FINDING_INVALID", ) - unknown = sorted(set(returned_prior_ids) - expected_prior_ids) + unknown = sorted(set(returned_prior_ids) - known_prior_ids) if unknown: raise PipelineError( - "prior_findings references findings that are not open " + "prior_findings references findings that are not in the manifest " f"(unknown={unknown})", code="PRIOR_FINDING_INVALID", ) @@ -1598,6 +1598,20 @@ def compile_review( f"prior_findings has invalid disposition for {item_id}", code="PRIOR_FINDING_INVALID", ) + if ( + item.get("status") == "resolved" + and item.get("thread_resolution") == "confirmed" + ): + # Someone resolved the thread on GitHub between rounds, so prepare + # already closed this finding. That human action is authoritative: + # a concordant `resolved` is a no-op, and an `open` must not reopen + # a thread a reviewer deliberately closed. + print( + f"ignoring {status} disposition for {item_id}: already " + "resolved on GitHub", + file=sys.stderr, + ) + continue item["status"] = status item["last_checked_sha"] = head if status == "resolved": diff --git a/.github/actions/claude-pr-review/test_review_pipeline.py b/.github/actions/claude-pr-review/test_review_pipeline.py index 2aab686..b991e89 100644 --- a/.github/actions/claude-pr-review/test_review_pipeline.py +++ b/.github/actions/claude-pr-review/test_review_pipeline.py @@ -1000,10 +1000,62 @@ def test_unknown_prior_finding_still_fails(self): with self.assertRaisesRegex( pipeline.PipelineError, - "not open", + "not in the manifest", ): pipeline.compile_review(review_input(), output) + def test_disposition_for_externally_resolved_finding_is_tolerated(self): + value = review_input() + value["thread_resolution_enabled"] = True + value["manifest"]["findings"]["F-existing"] = { + "status": "resolved", + "severity": "major", + "thread_id": "THREAD", + "thread_resolution": "confirmed", + "resolved_sha": "a" * 40, + } + output = clean_output() + output["prior_findings"] = [ + { + "finding_id": "F-existing", + "disposition": "resolved", + "reason": "The head commit applies the requested change.", + } + ] + + payload = pipeline.compile_review(value, output) + + item = payload["manifest"]["findings"]["F-existing"] + self.assertEqual(item["status"], "resolved") + self.assertEqual(item["thread_resolution"], "confirmed") + self.assertEqual(payload["resolve_thread_ids"], []) + + def test_open_disposition_cannot_reopen_externally_resolved_finding(self): + value = review_input() + value["thread_resolution_enabled"] = True + value["manifest"]["findings"]["F-existing"] = { + "status": "resolved", + "severity": "major", + "thread_id": "THREAD", + "thread_resolution": "confirmed", + "resolved_sha": "a" * 40, + } + output = clean_output() + output["prior_findings"] = [ + { + "finding_id": "F-existing", + "disposition": "open", + "reason": "The issue looks unaddressed.", + } + ] + + payload = pipeline.compile_review(value, output) + + item = payload["manifest"]["findings"]["F-existing"] + self.assertEqual(item["status"], "resolved") + self.assertEqual(item["thread_resolution"], "confirmed") + self.assertEqual(item["resolved_sha"], "a" * 40) + def test_skip_with_open_finding_preserves_manifest(self): value = review_input(mode="skip") value["manifest"]["findings"]["F-existing"] = {