diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index cffb52cb53..c4e9d28ebd 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -2927,8 +2927,6 @@ 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": @@ -3688,23 +3686,6 @@ 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) @@ -4153,8 +4134,6 @@ 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": @@ -4267,8 +4246,6 @@ 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": @@ -4487,8 +4464,6 @@ 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": @@ -4904,8 +4879,6 @@ 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": @@ -4956,8 +4929,6 @@ 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 8b924291a5..2cbda7f85b 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2330,75 +2330,6 @@ 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, ): @@ -10764,111 +10695,3 @@ 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