diff --git a/CHANGELOG.md b/CHANGELOG.md index 5626956b6..c4328b082 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,16 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Narrow `inspect_and_review`'s remaining duplicate-verdict race window: it already re-fetches the + PR and re-validates `require_expected_head()` after the LLM call before publishing, but never + re-checked `existing_noema_review()` at that same point. `noema-review.yml`'s shared concurrency + group is the primary defense against two runs racing to review the same head, but GitHub's + `cancel-in-progress` is best-effort and does not preempt a run mid-step: a run already past its + own pre-model `existing_noema_review()` check when a newer trigger supersedes it could still + finish its diff/context/LLM call and post a duplicate review afterward, since an unchanged head + SHA does not prove no other process posted a review for that head in the meantime. Added the same + `existing_noema_review()` re-check immediately before `submit_review`, narrowing the window from + the full diff/context/LLM-call duration down to one GraphQL round trip. - **Fix a stale `test_strix_quick_gate.sh` assertion left broken by the `#1630` scheduler-cadence lengthening.** `pr-review-merge-scheduler.yml`'s repository-local heartbeat was changed from a quarter-hourly `cron: "*/30 * * * *"` to an hourly diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index ce90b8bc8..0da723bf3 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -1678,6 +1678,19 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: except RuntimeError: print("Pull request closed or its head changed during review; stale verdict was not published.") return 0 + # Re-check for a concurrent Noema submission immediately before posting. + # noema-review.yml's concurrency group (shared across pull_request_target, + # workflow_run, and repository_dispatch triggers) is the primary defense + # against two runs racing to review the same head, but GitHub's + # cancel-in-progress is best-effort and does not preempt a run mid-step: + # a run already past the pre-model existing_noema_review() check when a + # newer trigger supersedes it can still finish its own diff/context/LLM + # call and post afterward. require_expected_head() alone does not catch + # this -- it only proves the head SHA is unchanged, not that no other + # process posted a review for that same head in the meantime. + if existing_noema_review(current_pr, actor): + print("Current head already has a Noema review as of just before submission; not posting a duplicate.") + return 0 submit_review(repo, number, current_pr, actor, verdict) return 0 diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index ba65ba6b1..edec038ad 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -612,7 +612,6 @@ def make_pr(**overrides): "headRefOid": "head", "reviews": {"nodes": []}, "reviewThreads": {"nodes": []}, - "statusCheckRollup": {"contexts": {"nodes": []}}, } value.update(overrides) return value @@ -1633,7 +1632,6 @@ def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatc headRefOid=head, reviews={"nodes": [review("CHANGES_REQUESTED")]}, reviewThreads={"nodes": [{"isResolved": False, "isOutdated": False}]}, - statusCheckRollup={"contexts": {"nodes": [{"__typename": "StatusContext", "context": "ci", "state": "FAILURE"}]}}, ) calls = [] monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) @@ -1648,6 +1646,53 @@ def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatc assert calls +def test_inspect_and_review_rechecks_for_a_concurrent_submission_before_posting(monkeypatch): + """A second trigger (e.g. workflow_run) that starts while this run is still + building context/calling the LLM must not publish a duplicate verdict once + the first run has already submitted one for the same head. noema-review.yml's + concurrency-group serialization is the primary defense; this re-check + narrows the remaining race window (cancel-in-progress is best-effort and + does not preempt a run mid-step) down to one GraphQL round trip + immediately before the POST -- distinct from require_expected_head()'s own + post-model re-check, which only proves the head SHA is unchanged, not that + no other process posted a review for that head in the meantime.""" + head = "a" * 40 + first_call_pr = make_pr(headRefOid=head) + noema_marker = "\n".join( + [ + noema.NOEMA_REVIEW_FOOTER_MARKER, + "- Result: APPROVE", + f"- Head SHA: `{head}`", + "- Reviewer credential: `test`", + "- Actor: `noema`", + "", + f"", + ] + ) + already_reviewed_pr = make_pr( + headRefOid=head, + reviews={"nodes": [review(commit=head, login="noema", body=noema_marker)]}, + ) + fetch_pr_calls = [] + + def fake_fetch_pr(repo, number): + fetch_pr_calls.append(1) + return first_call_pr if len(fetch_pr_calls) == 1 else already_reviewed_pr + + calls = [] + monkeypatch.setattr(noema, "fetch_pr", fake_fetch_pr) + monkeypatch.setattr(noema, "current_actor", lambda: "noema") + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: []) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") + monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok", "findings": []}) + monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) + + assert noema.inspect_and_review("owner/repo", 7, head) == 0 + assert calls == [] + assert len(fetch_pr_calls) == 2 + + def test_stale_trigger_stops_before_identity_or_model_work(monkeypatch): monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr(headRefOid="b" * 40)) monkeypatch.setattr(