diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index c4e9d28ebd..cffb52cb53 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -2927,6 +2927,8 @@ def post_update_branch_followup( if wait_reason: return f"{head_note}; {wait_reason}" dispatch_result = dispatch_opencode_review(repo, workflow, updated_pr, dry_run=dry_run) + if dispatch_result == "merge_conflict": + return f"{head_note}; PR merge tree cannot be materialized while the head conflicts; review dispatch skipped" if dispatch_result == "admission_deferred": return f"{head_note}; bounded admission budget is exhausted" if dispatch_result == "already_running": @@ -3686,6 +3688,23 @@ def dispatch_opencode_review(repo: str, workflow: str, pr: dict[str, Any], *, dr return "already_running" if dry_run: return "dry_run" + if effective_merge_state(pr) in {"DIRTY", "CONFLICTING"}: + # Materializing the PR merge tree is a hard precondition of + # coverage-source-tree, so a conflicting head can only produce a failed + # dispatch. Returning before review_dispatch_admitted keeps the bounded + # admission budget for a PR a review could actually finish: measured on + # .github#1529, one conflicting head consumed 27 dispatches across 100.8 + # hours with zero successes (20 cancelled, 7 failed, and all 7 that + # reached coverage-source-tree died there; 2026-09-01T08:46Z..09-05T13:31Z). + # UNKNOWN is deliberately not blocked -- an + # uncomputed mergeability must not starve a reviewable PR. + print( + "OpenCode review dispatch skipped: GitHub reports the current head as " + f"{effective_merge_state(pr)}, so the PR merge tree cannot be materialized " + "and the review would fail. Repair the branch and push it, then the review " + "runs on the new head." + ) + return "merge_conflict" if not review_dispatch_admitted("opencode", repo, pr): return "admission_deferred" base_ref, base_sha, head_sha = validated_pr_dispatch_fields(pr) @@ -4134,6 +4153,8 @@ def dispatch_draft_review_only( f"draft PR review-only dispatch; current head has completed Strix evidence; {wait_reason}", ) dispatch_result = dispatch_opencode_review(repo, workflow, pr, dry_run=dry_run) + if dispatch_result == "merge_conflict": + return Decision(number, "wait", "draft PR review-only dispatch; PR merge tree cannot be materialized while the head conflicts; review dispatch skipped") if dispatch_result == "admission_deferred": return Decision(number, "wait", "draft PR review-only dispatch; bounded admission budget is exhausted") if dispatch_result == "already_running": @@ -4246,6 +4267,8 @@ def inspect_pr( if wait_reason: return Decision(number, "wait", f"stacked PR onto {base_ref}; {wait_reason}") dispatch_result = dispatch_opencode_review(repo, workflow, pr, dry_run=dry_run) + if dispatch_result == "merge_conflict": + return Decision(number, "wait", f"stacked PR onto {base_ref}; PR merge tree cannot be materialized while the head conflicts; review dispatch skipped") if dispatch_result == "admission_deferred": return Decision(number, "wait", f"stacked PR onto {base_ref}; bounded admission budget is exhausted") if dispatch_result == "already_running": @@ -4464,6 +4487,8 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio if wait_reason: return decide("wait", wait_reason) dispatch_result = dispatch_opencode_review(repo, workflow, pr, dry_run=dry_run) + if dispatch_result == "merge_conflict": + return decide("wait", "PR merge tree cannot be materialized while the head conflicts; review dispatch skipped") if dispatch_result == "admission_deferred": return decide("wait", "bounded admission budget is exhausted") if dispatch_result == "already_running": @@ -4879,6 +4904,8 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio f"OpenCode review exceeded {stale_opencode_minutes} minute retry threshold; review dispatch limit reached", ) dispatch_result = dispatch_opencode_review(repo, workflow, pr, dry_run=dry_run) + if dispatch_result == "merge_conflict": + return decide("wait", "PR merge tree cannot be materialized while the head conflicts; review dispatch skipped") if dispatch_result == "admission_deferred": return decide("wait", "bounded admission budget is exhausted") if dispatch_result == "already_running": @@ -4929,6 +4956,8 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio if wait_reason: return decide("wait", f"current head has completed Strix evidence; {wait_reason}") dispatch_result = dispatch_opencode_review(repo, workflow, pr, dry_run=dry_run) + if dispatch_result == "merge_conflict": + return decide("wait", "PR merge tree cannot be materialized while the head conflicts; review dispatch skipped") if dispatch_result == "admission_deferred": return decide("wait", "bounded admission budget is exhausted") if dispatch_result == "already_running": diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 2cbda7f85b..8b924291a5 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2330,6 +2330,75 @@ def test_dispatch_opencode_review_falls_back_to_bounded_discovery(monkeypatch): assert json.loads(dispatch_calls[0])["client_payload"]["required_run_id"] == 999 +def _dispatch_with_merge_state(monkeypatch, **overrides): + """Run the OpenCode dispatch funnel and report what it did.""" + monkeypatch.setenv("GITHUB_ACTIONS", "true") + monkeypatch.setenv("GH_TOKEN", "opencode-app-token") + monkeypatch.setattr( + sched, "active_opencode_run_refs", lambda repo, workflow, pr: ([], []) + ) + monkeypatch.setattr(sched, "discover_opencode_required_run_id", lambda repo, head_sha: None) + dispatched: list[str | None] = [] + monkeypatch.setattr( + sched, "run_github_dispatch", lambda args, stdin=None: dispatched.append(stdin) + ) + admitted: list[str] = [] + + def record_admission(component, repo, pr): + admitted.append(component) + return True + + monkeypatch.setattr(sched, "review_dispatch_admitted", record_admission) + pr = make_pr(headRefOid="a" * 40, baseRefOid="b" * 40, **overrides) + monkeypatch.setattr(sched, "fetch_pr", lambda *_args: [pr]) + result = sched.dispatch_opencode_review("owner/repo", "OpenCode Review", pr, dry_run=False) + return result, dispatched, admitted + + +def test_review_dispatch_skips_a_head_whose_merge_tree_cannot_materialize(monkeypatch): + """A conflicting head is skipped before it can spend the admission budget. + + coverage-source-tree must materialize the PR merge tree, which git cannot do + while the head conflicts, so the dispatch could only fail. Measured on + .github#1529: one conflicting head took 27 dispatches over 100.8 hours and + produced no review; the 7 that reached coverage-source-tree all died there, + and the other 20 were cancelled before they ever started it. + """ + for graph_state in ("DIRTY", "CONFLICTING"): + result, dispatched, admitted = _dispatch_with_merge_state( + monkeypatch, mergeStateStatus=graph_state + ) + assert result == "merge_conflict" + assert dispatched == [], f"{graph_state} must not reach the dispatch API" + assert admitted == [], f"{graph_state} must not consume the admission budget" + + +def test_review_dispatch_reads_the_rest_merge_state_not_only_graphql(monkeypatch): + """The skip honours REST mergeability, which outranks a stale GraphQL value.""" + result, dispatched, admitted = _dispatch_with_merge_state( + monkeypatch, mergeStateStatus="CLEAN", restMergeableState="DIRTY" + ) + assert result == "merge_conflict" + assert dispatched == [] + assert admitted == [] + + +def test_review_dispatch_still_runs_when_mergeability_is_not_yet_known(monkeypatch): + """UNKNOWN mergeability must not starve a reviewable PR. + + Negative control for the conflict skip: GitHub reports UNKNOWN while it is + still computing a merge commit, so blocking on it would defer every PR the + scheduler reached first. + """ + for graph_state in ("UNKNOWN", "BEHIND", "BLOCKED", "CLEAN"): + result, dispatched, admitted = _dispatch_with_merge_state( + monkeypatch, mergeStateStatus=graph_state + ) + assert result == "dispatched", f"{graph_state} must still dispatch" + assert len(dispatched) == 1 + assert admitted == ["opencode"] + + def test_central_progress_ignores_required_workflow_checkrun_placeholder( monkeypatch, ): @@ -10695,3 +10764,111 @@ def behind_with(nodes): assert "checks are still queued or running" not in resumed.reason assert sched.has_in_flight_check_runs(behind_with([])) is False + + +def _skip_opencode_dispatch(monkeypatch): + """Make the OpenCode dispatch funnel report an unmaterializable merge tree.""" + monkeypatch.setattr( + sched, "dispatch_opencode_review", lambda repo, workflow, pr, dry_run: "merge_conflict" + ) + monkeypatch.setattr( + sched, "dispatch_strix_evidence", lambda repo, workflow, pr, dry_run: "dispatched" + ) + + +SKIP_REASON = "PR merge tree cannot be materialized while the head conflicts; review dispatch skipped" + + +def test_every_review_dispatch_caller_reports_the_conflict_skip_truthfully(monkeypatch): + """No dispatch path may report a skipped conflicting head as a dispatch. + + Each caller's fall-through says the review was dispatched, so a new funnel + result that a caller does not handle would be reported as work that never + happened -- and that is the same telemetry used to find the treadmill this + skip removes. + """ + _skip_opencode_dispatch(monkeypatch) + + stacked = inspect(make_pr(baseRefName="develop")) + assert stacked.action == "wait" + assert stacked.reason == f"stacked PR onto develop; {SKIP_REASON}" + + draft = inspect( + make_pr(isDraft=True, statusCheckRollup={"contexts": {"nodes": [strix_check()]}}), + allow_draft_review_dispatch=True, + ) + assert draft.action == "wait" + assert draft.reason == f"draft PR review-only dispatch; {SKIP_REASON}" + + strix_done = inspect(make_pr(statusCheckRollup={"contexts": {"nodes": [strix_check()]}})) + assert strix_done.action == "wait" + assert strix_done.reason == SKIP_REASON + + stale = inspect( + make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + opencode_check(started_at="2026-06-25T07:00:00Z"), + strix_check(), + ] + } + } + ) + ) + assert stale.action == "wait" + assert stale.reason == SKIP_REASON + + coverage_retry = inspect( + make_pr( + reviews={ + "nodes": [ + { + **opencode_review("CHANGES_REQUESTED", "head"), + "body": ( + "OpenCode cannot approve yet because required coverage evidence " + "did not pass. The coverage-evidence gate reported that required " + "test/docstring evidence was not proven." + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + {**opencode_check(status="COMPLETED"), "conclusion": "FAILURE"}, + ] + } + }, + ) + ) + assert coverage_retry.action == "wait" + assert coverage_retry.reason == SKIP_REASON + + original = make_pr(headRefOid="old-head") + monkeypatch.setattr( + sched, + "wait_for_updated_branch_head", + lambda repo, pr: make_pr( + headRefOid="new-head", + statusCheckRollup={"contexts": {"nodes": [strix_check()]}}, + ), + ) + followup_reason = sched.post_update_branch_followup( + "owner/repo", + original, + dry_run=False, + trigger_reviews=True, + review_dispatch_allowed=True, + workflow="OpenCode Review", + security_workflow="Strix Security Scan", + stale_opencode_minutes=45, + ) + assert SKIP_REASON in followup_reason