From c3b66b1f824501b8bd474db00eb0507190a3e0b6 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 03:08:36 +0900 Subject: [PATCH] Revert "fix(scheduler): skip review dispatch for a head whose merge tree cannot materialize (#1973)" This reverts commit ad0779bee66624c3997947d7691f4b0dbb973be1. I wrote that guard and its justification is false. It blocked every OpenCode dispatch on a DIRTY/CONFLICTING head on the grounds that "a conflicting head can only produce a failed dispatch". The run object does conclude failure, but the review is published anyway: the reviewer reads the pull request diff, not a merge tree -- "Coverage is a separate gate", in the review's own words -- and only `coverage-source-tree` needs the merge commit. Measured on .github#1529, the PR that motivated the guard: the last of its 27 dispatches published a 2404-character review at 2026-09-05T19:40:58Z. That review body carries its own run id, 33969161561, which concluded failure at the receipt gate nine seconds later. It is the only OpenCode review that head has (4 reviews total, 1 by opencode-agent[bot]) and it is why the PR now reads as reviewed. The guard would have discarded it. The pattern is not unique to #1529: .github#1555 is CONFLICTING right now and its current head carries a 2019-character CHANGES_REQUESTED review from the same reviewer. I reached "produced nothing" by reading run conclusions, which are roll-ups that cannot name what a run did. The cost the guard claimed to protect is also wrong by two orders of magnitude. Across all 27 dispatches of that head, 109 jobs: 27 were allocated a runner for 0.24 h in total, 82 never were, and the rest of the elapsed time was queue residency. Blocking the whole chain would have saved fourteen minutes of runner time. The repeat itself is a real problem, and it is fixed at its cause rather than here. `active_review_run_refs` matched a run's `name` exactly against the review workflow aliases, but the central review workflows define `run-name:`, so GitHub sends the rendered title in that field and every dispatch run was filtered out before the check could see it -- `already_running` never fired. With that repaired, a conflicting head receives one dispatch and the next is suppressed while it runs, which is the outcome this guard was reaching for without discarding the review. Developer experience: the scheduler no longer prints a skip reason that asserts an outcome contradicted by the runs it cites. User experience: a conflicting pull request receives a review and repair guidance, instead of repair guidance alone. Co-Authored-By: Claude Opus 5 --- scripts/ci/pr_review_merge_scheduler_core.py | 29 --- tests/test_pr_review_merge_scheduler.py | 177 ------------------- 2 files changed, 206 deletions(-) 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