From 647a9dba76fd561d925d2e7750c17ab27e79f273 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 00:41:42 +0900 Subject: [PATCH 1/2] fix(scheduler): skip review dispatch when the merge tree cannot materialize coverage-source-tree must materialize the pull request merge tree before it can measure anything, and git cannot do that while the head conflicts. The scheduler had no way to know that, so it re-dispatched conflicting heads on every tick. Measured across the 400 most recent opencode-review-dispatch runs: 55 were repeats at an already-dispatched head, and four CONFLICTING pull requests produced 35 of them. The worst, .github#1529 at c352014a, took 20 dispatches across 80.5 hours and produced no review. Every attempt failed at the same step with the same log line, "Coverage merge tree could not be materialized". Nothing stopped it because admission idempotency is per-run only, its state file living in RUNNER_TEMP, and the one cross-run debounce engages only when the pull request already carries a current-head coverage change-request review. The guard sits in dispatch_opencode_review, ahead of review_dispatch_admitted, so a conflicting head no longer spends a dispatch budget that defaults to one per run. UNKNOWN mergeability is deliberately not blocked, so an uncomputed merge state cannot starve a reviewable pull request. All six callers learn the new result, because each one's fall-through reports a dispatch that did not happen. Refs #1972 Co-Authored-By: Claude Opus 5 --- scripts/ci/pr_review_merge_scheduler_core.py | 27 +++ tests/test_pr_review_merge_scheduler.py | 176 +++++++++++++++++++ 2 files changed, 203 insertions(+) diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index c4e9d28ebd..5a917fa88c 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,21 @@ 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 20 dispatches across 80.5 + # hours with zero successes. 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 +4151,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 +4265,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 +4485,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 +4902,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 +4954,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..e0df487f0b 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2330,6 +2330,74 @@ 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 20 dispatches over 80.5 hours and + produced no review. + """ + 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 +10763,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 From d88cf31726dd733bb934c60191558bc6bc18d842 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 01:24:53 +0900 Subject: [PATCH 2/2] fix(scheduler): correct the measured dispatch figure in the guard comment The original 20/80.5h came from a "most recent 400 runs" window that did not reach back to the pull request creation, so it truncated silently. Recounted over a window starting before .github#1529 existed: 27 dispatches on one head across 100.8 hours, zero successes, 20 cancelled and 7 failed. The 7 that reached coverage-source-tree are exactly the 7 that failed there; the 20 cancelled never started that job. The waste is larger than first claimed, so the guard is more justified, not less. Corrected because a measured value in production source gets quoted as fact, which is how the truncated number reached here. Co-Authored-By: Claude Opus 5 --- scripts/ci/pr_review_merge_scheduler_core.py | 6 ++++-- tests/test_pr_review_merge_scheduler.py | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 5a917fa88c..cffb52cb53 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -3693,8 +3693,10 @@ def dispatch_opencode_review(repo: str, workflow: str, pr: dict[str, Any], *, dr # 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 20 dispatches across 80.5 - # hours with zero successes. UNKNOWN is deliberately not blocked -- an + # .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 " diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index e0df487f0b..8b924291a5 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2360,8 +2360,9 @@ def test_review_dispatch_skips_a_head_whose_merge_tree_cannot_materialize(monkey 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 20 dispatches over 80.5 hours and - produced no review. + .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(