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"] = {