From 43158bc17b0f7365ddcf5dbb50f5ca71c0ddd184 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 03:03:10 +0900 Subject: [PATCH] fix(scheduler): match the run name GitHub actually sends for central review runs `active_review_run_refs` matched a workflow run's `name` exactly against the review workflow aliases. But eight workflows in this repository define `run-name:`, and that set contains every workflow whose runs this matcher looks for -- `opencode-review.yml` ("Required OpenCode Review"), `opencode-review-dispatch.yml` ("OpenCode Review Dispatch") and `strix.yml` ("Strix Security Scan"). For such a workflow GitHub reports the *rendered* run name in `name` -- the same string as `display_title`, e.g. OpenCode Review Dispatch ContextualWisdomLab/.github#834@e748ee61... Sampled 2026-09-07: 100 of 100 opencode-review-dispatch runs carry that form and none carries the bare workflow name. So the exact match dropped every production dispatch run at this line, before the `event == "repository_dispatch"` branch immediately below that exists to read them. Two consequences: * `already_running` never suppressed a same-head repeat. .github#1529 took 27 dispatches on one unchanged head over 100.8 hours; each new run's creation preceded the previous run's cancellation by about three seconds, so the previous run was demonstrably still active when the check ran and did not see it. * `stale` never populated, so older-head central runs were never cancelled. A first count of the live queue said "20 duplicates of 45 active runs" and was wrong: it grouped by repository and PR without the workflow, so runs of different dispatch workflows on one PR were counted as duplicates of each other. Regrouped by (workflow, repository, PR): active repository_dispatch runs, queued + in_progress 33 codeql-scan-dispatch.yml 25 runs / 12 keys / 13 same-head duplicates opencode-review-dispatch.yml 5 runs / 5 keys / 0 duplicates pr-review-autofix.yml 3 runs / 3 keys / 0 duplicates So the workflows this matcher governs show no live duplication at this instant. The harm this fix addresses is the historical chain on .github#1529 and a suppression that has never once fired, not a backlog visible right now. The 13 duplicates all belong to CodeQL Scan Dispatch, which this matcher does not govern; that workflow also defines `run-name:`, which makes it a separate lead rather than evidence for this change. Reviving stale cancellation is separately safe: of 163 non-terminal central runs, 49 are review or dispatch kind and 4 become cancellable, all of them subjects that no longer exist (3 closed or merged PRs, 1 moved head). The fix is confined to the run comparison. `OPENCODE_WORKFLOW_NAMES` is unchanged, because its other consumer compares a *workflow* object's name, which is genuinely bare. `active_review_run_refs` has exactly two call sites, OpenCode's and Strix's, so both are fixed here; the Strix side is pinned by its own test so a later narrowing to the OpenCode aliases cannot silently reopen half of it. This is one instance of a class, and the file already contains the stable form. `run.name` is compared as an identifier at four places -- `:1250`, `:3198`, `:3254` (this one) and `:3783` -- while `:3060` keys on `run.get("workflow_id") or run.get("path") or run.get("name")`, which cannot be rewritten by a `run-name:`. `:1250` in particular feeds the REST fallback's workflow-level policy boundary and would see a rendered title where it expects a workflow name. Fixing the whole class means moving the callers from display names to paths, which also touches how `dispatch_title_prefixes` is built, so it is deliberately left out of this change; .github#1941 is the same root seen from the `display_title` side. Recorded here so the next reader does not rediscover it as a fifth instance. Note the new behaviour this enables: while a same-head central run is active, a repeat is now suppressed. A run that never terminates would therefore hold the PR, where before the check simply never fired. The existing fixture sets a bare `name` beside a rendered `display_title`, a payload GitHub never emits for a `run-name:` workflow, which is why 100 percent line coverage of that branch never revealed that production could not reach it. Developer experience: the scheduler's same-head suppression and stale-run cancellation work against real payloads instead of a shape only the tests produce. User experience: a pull request stops accumulating duplicate concurrent review runs that cancel each other, so a review that starts can finish. Co-Authored-By: Claude Opus 5 --- scripts/ci/pr_review_merge_scheduler_core.py | 19 +++- tests/test_pr_review_merge_scheduler.py | 95 ++++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index cffb52cb53..e535025a86 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -3254,7 +3254,24 @@ def active_review_run_refs( for run_repo in (dispatch_repo,): for run_data in active_workflow_runs(run_repo, statuses): run_name = str(run_data.get("name") or "") - if run_name != workflow and run_name not in workflow_aliases: + # GitHub reports the *rendered* ``run-name:`` in a run's ``name``, + # not the workflow name, and eight workflows here define one -- + # every workflow whose runs this matcher looks for + # (``opencode-review.yml`` = "Required OpenCode Review", + # ``opencode-review-dispatch.yml`` = "OpenCode Review Dispatch", + # ``strix.yml`` = "Strix Security Scan") is among them. Exact + # matching therefore dropped every production dispatch run here, + # before the ``repository_dispatch`` branch below that exists to + # handle it: ``already_running`` never suppressed a same-head + # repeat and ``stale`` never populated, so .github#1529 took 27 + # dispatches on one unchanged head and older-head central runs were + # never cancelled. Sampled 2026-09-07: 100 of 100 + # opencode-review-dispatch runs carry the rendered form, 0 bare. + # Accept it -- the workflow name, then a space, then the suffix. + if not any( + run_name == candidate or run_name.startswith(f"{candidate} ") + for candidate in (workflow, *workflow_aliases) + ): continue run_id = run_data.get("id") if not run_id: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 8b924291a5..b653efea18 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -6178,6 +6178,101 @@ def test_dispatch_strix_waits_for_active_target_repository_run(monkeypatch, caps assert "target repository already has active run(s) ContextualWisdomLab/.github@9350" in capsys.readouterr().out +def test_central_run_filter_accepts_the_run_name_github_actually_sends(monkeypatch): + """A ``run-name:`` workflow reports the rendered title in ``name``. + + ``opencode-review-dispatch.yml``, ``strix.yml`` and ``noema-review.yml`` all + define ``run-name:``, so GitHub sets each run's ``name`` to the rendered + string, identical to ``display_title`` -- sampled 2026-09-07, 100 of 100 + opencode-review-dispatch runs carry that form and none carries the bare + workflow name. Matching ``name`` exactly against the aliases dropped every + one of them before the ``repository_dispatch`` branch that exists to read + them, so ``already_running`` never suppressed a same-head repeat and + ``stale`` never populated: .github#1529 took 27 dispatches on one unchanged + head, and older-head central runs were never cancelled. + + The neighbouring fixture below sets a bare ``name`` alongside a rendered + ``display_title``, which is why 100% coverage of that branch never showed + that production could not reach it. + """ + head_sha = "a" * 40 + stale_sha = "b" * 40 + current_title = f"Required OpenCode Review owner/repo#1@{head_sha}" + stale_title = f"Required OpenCode Review owner/repo#1@{stale_sha}" + central_runs = [ + { + "id": 9500, + "name": current_title, + "display_title": current_title, + "event": "repository_dispatch", + }, + { + "id": 9501, + "name": stale_title, + "display_title": stale_title, + "event": "repository_dispatch", + }, + ] + + def fake_active_runs(repo, statuses=("queued", "in_progress")): + del statuses + return central_runs if repo == "ContextualWisdomLab/.github" else [] + + monkeypatch.setattr(sched, "active_workflow_runs", fake_active_runs) + monkeypatch.setenv( + "SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", + "ContextualWisdomLab/.github", + ) + + assert sched.active_opencode_run_refs( + "owner/repo", + "OpenCode Review", + make_pr(headRefOid=head_sha), + ) == ( + [("ContextualWisdomLab/.github", "9500")], + [("ContextualWisdomLab/.github", "9501")], + ) + + +def test_central_run_filter_reads_the_rendered_strix_run_name_too(monkeypatch): + """Strix shares the matcher, and ``strix.yml`` also defines ``run-name:``. + + ``active_review_run_refs`` has exactly two call sites -- OpenCode's and + ``dispatch_strix_evidence``'s -- so the exact-``name`` match blinded both. + Pinning the Strix side here keeps a later narrowing of the fix to the + OpenCode aliases from silently reopening the Strix half. + """ + head_sha = "c" * 40 + current_title = f"Strix Security Scan owner/repo#1@{head_sha}" + + def fake_active_runs(repo, statuses=("queued", "in_progress")): + del statuses + if repo != "ContextualWisdomLab/.github": + return [] + return [ + { + "id": 9600, + "name": current_title, + "display_title": current_title, + "event": "repository_dispatch", + } + ] + + monkeypatch.setattr(sched, "active_workflow_runs", fake_active_runs) + monkeypatch.setenv( + "SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", + "ContextualWisdomLab/.github", + ) + + assert sched.active_review_run_refs( + "owner/repo", + "Strix Security Scan", + make_pr(headRefOid=head_sha), + run_title="Strix Security Scan", + workflow_aliases=frozenset({"Strix Security Scan"}), + ) == ([("ContextualWisdomLab/.github", "9600")], []) + + def test_central_run_filter_ignores_malformed_and_non_dispatch_titles(monkeypatch): head_sha = "a" * 40 central_runs = [