From 18a04550dd0325b10ecfafa41b06d3f9c16ff3fa Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 07:44:45 +0900 Subject: [PATCH 1/5] fix(actions): give both stale-run cleanup jobs an identity they can match `strix.yml` and `opencode-review.yml` each run a cleanup job whose only purpose is retiring superseded runs of their own workflow, and both selected those runs with `select(.name == "")`. Both workflows declare `run-name:`, and GitHub reports the *rendered* run-name in a run's `name` field -- so neither equality ever matched and both jobs were silent no-ops. Measured 2026-09-07 against the live API: of the 100 most recent runs of each workflow, 0 carried the bare declared name and 100 carried the rendered `" #@"` form. The workflow-level `concurrency` group hides this on `synchronize`, where it cancels the previous head's run itself. What was actually lost is every case no successor run supersedes: a `closed` or `converted_to_draft` pull request left its in-flight scans running to completion, holding admission slots under the shared 60-job ceiling for a pull request nobody is waiting on. Identity now comes from `.path`, the signal `noema-review.yml` already adopted for this same defect. The same measurement confirmed it stable in both contexts: `.github/workflows/strix.yml` on native runs in `.github` and on the nine ruleset-injected runs in `bandscope`, which carries no local copy. Deleting the filter instead would have been wrong -- the surviving `$metadata_matches` branch accepts any run whose `pull_requests[]` names the pull request, so an unfiltered job would cancel other workflows' runs. A negative control per selector pins that. The same equality appears twice in the scheduler and is repaired with one shared predicate, `run_name_identifies_workflow`: - `dispatch_strix_evidence`'s `busy_refs` was always empty, so the `repository_busy` guard that serializes Strix dispatch per target repository never fired. Since 2026-09-04 that let 20 dispatch runs reach 5 simultaneous for one target repository, against the 1 the guard specifies. - `stale_pr_run_ids`'s `workflow=` filter had the same defect. Its only production caller passes no workflow, so it never ran; it stayed a trap that silently returned nothing for any caller that did supply one. Two fixtures asserted the dead behaviour and are corrected rather than deleted. `test_dispatch_strix_waits_for_active_target_repository_run` paired a bare `name` with a rendered `display_title` inside one run record -- a combination GitHub cannot produce -- which is how a passing test coexisted with a predicate matching 0 of 100 live runs. `run_name_identifies_workflow` accepts a candidate followed by a space, so a future workflow named as an extension of an existing one would start answering for it. Nothing else in the repository would notice, so the premise that makes the prefix accept safe -- no central workflow name prefixes another, 0 of 35 -- is pinned as its own test. Co-Authored-By: Claude Opus 5 --- .github/workflows/opencode-review.yml | 13 +- .github/workflows/strix.yml | 23 +- scripts/ci/pr_review_merge_scheduler_core.py | 46 ++- ...st_opencode_required_verdict_regression.py | 24 +- tests/test_pr_review_merge_scheduler.py | 7 +- .../test_required_workflow_queue_contract.py | 18 +- ...est_stale_run_cleanup_workflow_identity.py | 338 ++++++++++++++++++ 7 files changed, 450 insertions(+), 19 deletions(-) create mode 100644 tests/test_stale_run_cleanup_workflow_identity.py diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 19ea58003f..b8b7833712 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -565,11 +565,22 @@ jobs: return 0 fi local run_ids + # Workflow identity comes from `.path`, not `.name` -- the same + # defect strix.yml's cleanup job carried. This workflow declares + # `run-name:`, so a run's `name` is the rendered title + # "Required OpenCode Review #@", never the bare + # workflow name, and the former `.name ==` equality selected + # nothing: sampled 2026-09-07, 0 of 100 runs carried the bare name + # and 100 of 100 carried the rendered one. `.path` is the signal + # noema-review.yml already adopted here, stable for both native and + # ruleset-injected runs; the `startswith` check keeps the second, + # independent signal the equality was reaching for. if ! run_ids="$(jq -r --arg pr "$TARGET_PR_NUMBER" --arg head_sha "$TARGET_PR_HEAD_SHA" \ --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" ' .workflow_runs[] | select((.id | tostring) != $current) - | select(.name == "Required OpenCode Review") + | select(.path == ".github/workflows/opencode-review.yml") + | select((.name // "") | startswith("Required OpenCode Review")) | select(.event == "pull_request_target") | ((.display_title // "") | startswith("Required OpenCode Review " + $repo + "#" + $pr + "@")) as $title_matches | ((.pull_requests // []) | any((.number | tostring) == $pr)) as $metadata_matches diff --git a/.github/workflows/strix.yml b/.github/workflows/strix.yml index 58ed3dab8d..b3f4cd5776 100644 --- a/.github/workflows/strix.yml +++ b/.github/workflows/strix.yml @@ -292,11 +292,32 @@ jobs: return 0 fi local run_ids + # Workflow identity comes from `.path`, not `.name`. This workflow + # declares `run-name:`, and GitHub reports the *rendered* run-name + # in a run's `name` -- "Strix Security Scan #@", + # never the bare workflow name. The former `.name ==` equality + # therefore matched nothing and this whole cleanup job was a silent + # no-op: sampled 2026-09-07, 0 of 100 strix runs carried the bare + # name while 100 of 100 carried the rendered one. `.path` is the + # signal noema-review.yml already adopted for this same defect, and + # it holds in both contexts -- verified the same day as + # ".github/workflows/strix.yml" on native `.github` runs and on the + # 9 ruleset-injected runs in bandscope, which carries no local + # strix.yml. The `startswith` name check is kept as the second, + # independent signal the original equality was reaching for. + # + # The workflow-level `concurrency` group above hides this on the + # `synchronize` path (it cancels the previous head's run itself), + # so what was actually lost is every case no successor run + # supersedes: `closed` and `converted_to_draft` left their in-flight + # scans running to completion, holding admission slots under the + # shared 60-job ceiling for a PR nobody is waiting on. if ! run_ids="$(jq -r --arg pr "$TARGET_PR_NUMBER" --arg head_sha "$TARGET_PR_HEAD_SHA" \ --arg action "$PR_ACTION" --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" ' .workflow_runs[] | select((.id | tostring) != $current) - | select(.name == "Strix Security Scan") + | select(.path == ".github/workflows/strix.yml") + | select((.name // "") | startswith("Strix Security Scan")) | select(.event == "pull_request_target") | ((.display_title // "") | startswith("Strix Security Scan " + $repo + "#" + $pr + "@")) as $title_matches | ((.pull_requests // []) | any((.number | tostring) == $pr)) as $metadata_matches diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 4df4dac3de..2c60c44636 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -3175,6 +3175,41 @@ def workflow_run_mentions_pr(run_data: dict[str, Any], pr_number: int) -> bool: return any(pr.get("number") == pr_number for pr in run_data.get("pull_requests") or []) +def run_name_identifies_workflow(run_name: str, *workflow_names: str) -> bool: + """Return whether a run's reported ``name`` identifies one of these workflows. + + GitHub reports the *rendered* ``run-name:`` in a run's ``name`` field, not + the workflow's declared ``name:``. Every central review workflow here + declares one, so ``name`` arrives as ``" #@"`` + and an equality test against the declared name matches nothing in + production at all: sampled 2026-09-07, 0 of 100 ``strix.yml`` runs and 0 of + 100 ``opencode-review.yml`` runs carried the bare form while 100 of 100 + carried the rendered one. + + Both forms are accepted because both occur. The bare name is what a + workflow declaring no ``run-name:`` sends, and what GitHub can fall back to + for an organization-required-workflow run materialized in a sibling + repository (recorded on ``noema-review.yml``'s cleanup job). + + The space this requires after a candidate is a word boundary, and that is + the whole of what it buys: a workflow named "Strix Security Scanner" does + not answer for the candidate "Strix Security Scan". It deliberately does + *not* separate a longer workflow name that begins with the candidate and a + space -- a hypothetical "Strix Security Scan Extended" would be accepted. + Two things make that safe rather than latent. No central workflow name + prefixes another (verified 0 of 35 on 2026-09-07, and pinned by + :mod:`tests.test_stale_run_cleanup_workflow_identity`), and every call site + pins identity a second time -- by ``display_title`` prefix or by the run's + ``path`` -- so this predicate narrows a candidate set rather than deciding + identity alone. Accepting a prefix is also required, not merely tolerated: + callers pass short aliases ("Strix") for the same workflow on purpose. + """ + return any( + run_name == candidate or run_name.startswith(f"{candidate} ") + for candidate in workflow_names + ) + + def stale_pr_run_ids( repo: str, pr: dict[str, Any], @@ -3195,7 +3230,9 @@ def stale_pr_run_ids( number = int(pr["number"]) stale: list[str] = [] for run_data in active_workflow_runs(repo, statuses): - if workflow is not None and run_data.get("name") != workflow: + if workflow is not None and not run_name_identifies_workflow( + str(run_data.get("name") or ""), workflow + ): continue if str(run_data.get("head_sha") or "").lower() == head: continue @@ -3266,10 +3303,7 @@ def active_review_run_refs( # 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) - ): + if not run_name_identifies_workflow(run_name, workflow, *workflow_aliases): continue run_id = run_data.get("id") if not run_id: @@ -3797,7 +3831,7 @@ def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry for run_data in active_workflow_runs(dispatch_repo) if run_data.get("id") and str(run_data["id"]) not in cancelled_ids - and run_data.get("name") == workflow + and run_name_identifies_workflow(str(run_data.get("name") or ""), workflow) and run_data.get("event") == "repository_dispatch" and str(run_data.get("display_title") or "").startswith( f"Strix Security Scan {target_repo}#" diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 5c5325d1aa..7be9ec1481 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -215,12 +215,22 @@ def _cleanup_run( *, run_id: int, head_sha: str = HEAD, - name: str = "Required OpenCode Review", + name: str | None = None, + path: str = ".github/workflows/opencode-review.yml", event: str = "pull_request_target", display_title: str | None = None, pr_number: int = 1437, ) -> dict[str, object]: - """Build one synthetic workflow-run record for the cleanup filter.""" + """Build one synthetic workflow-run record for the cleanup filter. + + ``name`` defaults to the same string as ``display_title`` because that is + what GitHub sends: this workflow declares ``run-name:``, and a run's + ``name`` is the rendered result, never the declared workflow name. The + earlier default paired a bare ``name`` with a rendered ``display_title``, a + combination the API cannot produce, which is how the selector's former + ``.name ==`` equality passed here while matching 0 of 100 live runs. + ``path`` carries the workflow identity the selector now reads. + """ title = ( display_title if display_title is not None @@ -228,7 +238,8 @@ def _cleanup_run( ) return { "id": run_id, - "name": name, + "name": title if name is None else name, + "path": path, "event": event, "display_title": title, "pull_requests": [{"number": pr_number, "head": {"sha": head_sha}}], @@ -261,7 +272,12 @@ def test_cleanup_excludes_a_different_pull_request() -> None: def test_cleanup_excludes_a_differently_named_or_triggered_run() -> None: """A same-PR run for another workflow or trigger is left untouched.""" - other_workflow = _cleanup_run(run_id=1, head_sha="b" * 40, name="Strix Security Scan") + other_workflow = _cleanup_run( + run_id=1, + head_sha="b" * 40, + name="Strix Security Scan", + path=".github/workflows/strix.yml", + ) other_event = _cleanup_run(run_id=2, head_sha="b" * 40, event="workflow_dispatch") assert ( cleanup_candidate_run_ids([other_workflow, other_event], current_run_id="999") diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index ba47b89c8d..e3a69677c3 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -6079,9 +6079,14 @@ def fake_run(args, stdin=None): def test_dispatch_strix_waits_for_active_target_repository_run(monkeypatch, capsys): calls = [] + # The run name is the *rendered* ``run-name:``, identical to + # ``display_title`` -- the only shape GitHub actually sends for a workflow + # that declares one. The previous fixture paired a bare ``name`` with a + # rendered ``display_title``, a combination GitHub cannot produce, so this + # test passed while the production predicate matched 0 of 100 live runs. active_run = { "id": 9350, - "name": "Strix Security Scan", + "name": f"Strix Security Scan owner/repo#2@{'c' * 40}", "event": "repository_dispatch", "display_title": f"Strix Security Scan owner/repo#2@{'c' * 40}", "pull_requests": [], diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 4ab09b1d0c..9b6518fef6 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -781,13 +781,19 @@ def test_strix_cleanup_uses_pr_metadata_when_custom_title_is_absent() -> None: marker = '--arg action "$PR_ACTION" --arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" \'\n' start = workflow.index(marker) + len(marker) end = workflow.index('\n \' <<<"$runs_json"', start) + # Every row carries ``path``: the selector reads workflow identity there + # now, because a run's ``name`` is the rendered ``run-name:`` and the old + # ``.name ==`` equality matched 0 of 100 live runs. The bare ``name`` here + # is deliberate and still accepted -- it is the required-workflow-ruleset + # shape this test exists for, where GitHub renders no run-name at all. + strix_path = ".github/workflows/strix.yml" runs = { "workflow_runs": [ - {"id": 1, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 7, "head": {"sha": "old"}}]}, - {"id": 2, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 7, "head": {"sha": "current"}}]}, - {"id": 3, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 7}]}, - {"id": 4, "name": "Strix Security Scan", "event": "pull_request_target", "display_title": "Strix Security Scan owner/repo#7@old", "pull_requests": [{"number": 7, "head": {"sha": "current"}}]}, - {"id": 5, "name": "Strix Security Scan", "event": "pull_request_target", "pull_requests": [{"number": 8, "head": {"sha": "old"}}]}, + {"id": 1, "name": "Strix Security Scan", "path": strix_path, "event": "pull_request_target", "pull_requests": [{"number": 7, "head": {"sha": "old"}}]}, + {"id": 2, "name": "Strix Security Scan", "path": strix_path, "event": "pull_request_target", "pull_requests": [{"number": 7, "head": {"sha": "current"}}]}, + {"id": 3, "name": "Strix Security Scan", "path": strix_path, "event": "pull_request_target", "pull_requests": [{"number": 7}]}, + {"id": 4, "name": "Strix Security Scan", "path": strix_path, "event": "pull_request_target", "display_title": "Strix Security Scan owner/repo#7@old", "pull_requests": [{"number": 7, "head": {"sha": "current"}}]}, + {"id": 5, "name": "Strix Security Scan", "path": strix_path, "event": "pull_request_target", "pull_requests": [{"number": 8, "head": {"sha": "old"}}]}, ] } result = subprocess.run( @@ -836,7 +842,7 @@ def _run_strix_cleanup( exit 0 fi if [[ "$*" == *"actions/runs?status=queued"* ]]; then - printf '%s\n' '{"workflow_runs":[{"id":100,"name":"Strix Security Scan","event":"pull_request_target","pull_requests":[{"number":7,"head":{"sha":"old"}}]}]}' + printf '%s\n' '{"workflow_runs":[{"id":100,"name":"Strix Security Scan","path":".github/workflows/strix.yml","event":"pull_request_target","pull_requests":[{"number":7,"head":{"sha":"old"}}]}]}' exit 0 fi if [[ "$*" == *"actions/runs?status="* ]]; then diff --git a/tests/test_stale_run_cleanup_workflow_identity.py b/tests/test_stale_run_cleanup_workflow_identity.py new file mode 100644 index 0000000000..c7f810fff4 --- /dev/null +++ b/tests/test_stale_run_cleanup_workflow_identity.py @@ -0,0 +1,338 @@ +"""Pin the run-identity signal both stale-run cleanup jobs select on. + +``strix.yml`` and ``opencode-review.yml`` each carry a cleanup job whose whole +purpose is retiring superseded runs of their own workflow. Both selected those +runs with ``select(.name == "")``. Both workflows +declare ``run-name:``, and GitHub reports the *rendered* run-name in a run's +``name`` field -- so the equality matched nothing and each job was a silent +no-op. Measured 2026-09-07 against the live API: of the 100 most recent runs of +each workflow, 0 carried the bare declared name and 100 carried the rendered +``" #@"`` form. + +The repair adopts the signal ``noema-review.yml``'s cleanup job already uses +for the identical defect: ``.path``, which the same measurement confirmed +stable in both contexts -- ``.github/workflows/strix.yml`` on native runs in +``.github`` and on the nine ruleset-injected runs in ``bandscope``, which +carries no local copy of that workflow. + +These tests execute the production jq against fixtures in the rendered shape, +so a revert to equality fails here rather than shipping green. Each selector +also gets a negative control: a foreign workflow's run attached to the same +pull request must not be selected, because the surviving ``$metadata_matches`` +branch would otherwise let this job cancel other workflows' runs. +""" + +from __future__ import annotations + +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +import scripts.ci.pr_review_merge_scheduler_core as sched + +STRIX_WORKFLOW = Path(".github/workflows/strix.yml") +OPENCODE_WORKFLOW = Path(".github/workflows/opencode-review.yml") + + +def _selector(workflow: Path, start_marker: str) -> str: + """Return the jq program the named cleanup job runs, read from the workflow.""" + text = workflow.read_text(encoding="utf-8") + start = text.index(start_marker) + len(start_marker) + end = text.index('\n \' <<<"$runs_json"', start) + return text[start:end] + + +def _run_jq(selector: str, args: list[str], runs: dict[str, object]) -> list[str]: + """Execute a selector against a runs payload, returning the selected ids.""" + jq = shutil.which("jq") + if jq is None: # pragma: no cover - environment without jq + pytest.skip("jq is required to execute the production cleanup selector") + result = subprocess.run( + [jq, "-r", *args, selector], + input=json.dumps(runs), + text=True, + capture_output=True, + check=True, + ) + return result.stdout.split() + + +def _strix_selector() -> str: + """Return the Strix cleanup job's jq selector.""" + return _selector( + STRIX_WORKFLOW, + '--arg action "$PR_ACTION" --arg repo "$TARGET_REPOSITORY"' + ' --arg current "$CURRENT_RUN_ID" \'\n', + ) + + +def _opencode_selector() -> str: + """Return the OpenCode review cleanup job's jq selector.""" + return _selector( + OPENCODE_WORKFLOW, + '--arg repo "$TARGET_REPOSITORY" --arg current "$CURRENT_RUN_ID" \'\n', + ) + + +def test_strix_cleanup_selects_a_superseded_run_whose_name_is_rendered() -> None: + """The Strix cleanup selector matches the only run shape GitHub sends. + + ``name`` and ``display_title`` are identical here because that is what the + API returns for a workflow declaring ``run-name:``; a fixture pairing a + bare ``name`` with a rendered ``display_title`` describes no real run and + is what let the equality survive. + """ + old = f"Strix Security Scan owner/repo#7@{'a' * 40}" + current = f"Strix Security Scan owner/repo#7@{'b' * 40}" + runs = { + "workflow_runs": [ + { + "id": 4001, + "path": ".github/workflows/strix.yml", + "name": old, + "display_title": old, + "event": "pull_request_target", + "pull_requests": [{"number": 7, "head": {"sha": "a" * 40}}], + }, + { + "id": 4002, + "path": ".github/workflows/strix.yml", + "name": current, + "display_title": current, + "event": "pull_request_target", + "pull_requests": [{"number": 7, "head": {"sha": "b" * 40}}], + }, + ] + } + selected = _run_jq( + _strix_selector(), + [ + "--arg", "pr", "7", + "--arg", "head_sha", "b" * 40, + "--arg", "action", "synchronize", + "--arg", "repo", "owner/repo", + "--arg", "current", "4002", + ], + runs, + ) + assert selected == ["4001"] + + +def test_strix_cleanup_retires_a_run_of_a_closed_pull_request() -> None: + """A closed pull request's in-flight scan is retired with no successor run. + + This is the case the workflow-level ``concurrency`` group cannot reach: on + ``synchronize`` a newer run supersedes the old one and GitHub cancels it, + but ``closed`` and ``converted_to_draft`` start no successor, so the dead + selector left those scans running to completion against the shared job + ceiling for a pull request nobody is waiting on. + """ + title = f"Strix Security Scan owner/repo#7@{'a' * 40}" + runs = { + "workflow_runs": [ + { + "id": 4010, + "path": ".github/workflows/strix.yml", + "name": title, + "display_title": title, + "event": "pull_request_target", + "pull_requests": [{"number": 7, "head": {"sha": "a" * 40}}], + } + ] + } + selected = _run_jq( + _strix_selector(), + [ + "--arg", "pr", "7", + "--arg", "head_sha", "a" * 40, + "--arg", "action", "closed", + "--arg", "repo", "owner/repo", + "--arg", "current", "4099", + ], + runs, + ) + assert selected == ["4010"] + + +def test_strix_cleanup_leaves_another_workflows_run_alone() -> None: + """A foreign workflow's run on the same pull request is never selected. + + ``$metadata_matches`` accepts any run whose ``pull_requests[]`` names this + pull request, so without a workflow-identity filter this job would cancel + every other central workflow's runs. ``.path`` is that filter. + """ + foreign = f"Required OpenCode Review owner/repo#7@{'a' * 40}" + runs = { + "workflow_runs": [ + { + "id": 4020, + "path": ".github/workflows/opencode-review.yml", + "name": foreign, + "display_title": foreign, + "event": "pull_request_target", + "pull_requests": [{"number": 7, "head": {"sha": "a" * 40}}], + } + ] + } + selected = _run_jq( + _strix_selector(), + [ + "--arg", "pr", "7", + "--arg", "head_sha", "b" * 40, + "--arg", "action", "closed", + "--arg", "repo", "owner/repo", + "--arg", "current", "4099", + ], + runs, + ) + assert selected == [] + + +def test_opencode_cleanup_selects_a_superseded_run_whose_name_is_rendered() -> None: + """The OpenCode review cleanup selector matches the rendered run shape.""" + old = f"Required OpenCode Review owner/repo#9@{'c' * 40}" + current = f"Required OpenCode Review owner/repo#9@{'d' * 40}" + runs = { + "workflow_runs": [ + { + "id": 5001, + "path": ".github/workflows/opencode-review.yml", + "name": old, + "display_title": old, + "event": "pull_request_target", + "pull_requests": [{"number": 9, "head": {"sha": "c" * 40}}], + }, + { + "id": 5002, + "path": ".github/workflows/opencode-review.yml", + "name": current, + "display_title": current, + "event": "pull_request_target", + "pull_requests": [{"number": 9, "head": {"sha": "d" * 40}}], + }, + ] + } + selected = _run_jq( + _opencode_selector(), + [ + "--arg", "pr", "9", + "--arg", "head_sha", "d" * 40, + "--arg", "repo", "owner/repo", + "--arg", "current", "5002", + ], + runs, + ) + assert selected == ["5001"] + + +def test_opencode_cleanup_leaves_another_workflows_run_alone() -> None: + """A Strix run on the same pull request is not cancelled by OpenCode cleanup.""" + foreign = f"Strix Security Scan owner/repo#9@{'c' * 40}" + runs = { + "workflow_runs": [ + { + "id": 5010, + "path": ".github/workflows/strix.yml", + "name": foreign, + "display_title": foreign, + "event": "pull_request_target", + "pull_requests": [{"number": 9, "head": {"sha": "c" * 40}}], + } + ] + } + selected = _run_jq( + _opencode_selector(), + [ + "--arg", "pr", "9", + "--arg", "head_sha", "d" * 40, + "--arg", "repo", "owner/repo", + "--arg", "current", "5099", + ], + runs, + ) + assert selected == [] + + +def test_run_name_identifies_workflow_accepts_both_forms_github_sends() -> None: + """Both the rendered and the bare run name identify their workflow.""" + assert sched.run_name_identifies_workflow( + f"Strix Security Scan owner/repo#7@{'a' * 40}", "Strix Security Scan" + ) + assert sched.run_name_identifies_workflow("Strix Security Scan", "Strix Security Scan") + + +def test_run_name_identifies_workflow_requires_a_word_boundary() -> None: + """A name that extends the candidate without a separator is rejected. + + This is the whole of what the required space buys, and the assertion is + written to say so rather than to imply the predicate resolves identity by + itself: "Strix Security Scan Extended" *is* accepted, and is safe only + because no central workflow name prefixes another and every call site pins + identity a second time by ``.path`` or ``display_title``. + """ + assert not sched.run_name_identifies_workflow( + "Strix Security Scanner owner/repo#7@abc", "Strix Security Scan" + ) + assert not sched.run_name_identifies_workflow("", "Strix Security Scan") + assert sched.run_name_identifies_workflow( + "Strix Security Scan Extended", "Strix Security Scan" + ) + + +def test_no_central_workflow_name_prefixes_another() -> None: + """Pin the premise that makes a prefix accept safe for these workflows. + + :func:`scripts.ci.pr_review_merge_scheduler_core.run_name_identifies_workflow` + accepts a candidate followed by a space, so a new workflow named as an + extension of an existing one ("Strix Security Scan Extended") would start + answering for it. Nothing else in the repository would notice; this test is + the notice. + """ + names = sorted( + { + line.split(":", 1)[1].strip().strip("\"'") + for workflow in sorted(Path(".github/workflows").glob("*.yml")) + for line in workflow.read_text(encoding="utf-8").splitlines() + if line.startswith("name:") + } + ) + assert len(names) >= 30 + collisions = [ + (shorter, longer) + for shorter in names + for longer in names + if shorter != longer and longer.startswith(f"{shorter} ") + ] + assert collisions == [] + + +def test_stale_pr_run_ids_matches_a_rendered_workflow_run_name(monkeypatch) -> None: + """``stale_pr_run_ids`` narrows by workflow using the name GitHub sends. + + Its only production caller passes no ``workflow``, so the equality this + replaces never ran; the parameter stayed a trap that silently returned no + runs for any caller that did supply one. + """ + monkeypatch.setattr(sched, "validate_git_sha", lambda value: str(value)) + rendered = f"Required OpenCode Review owner/repo#1@{'a' * 40}" + runs = [ + {"name": rendered, "id": 61, "head_sha": "old", "pull_requests": [{"number": 1}]}, + {"name": rendered, "id": 62, "head_sha": "head", "pull_requests": [{"number": 1}]}, + { + "name": f"Strix Security Scan owner/repo#1@{'a' * 40}", + "id": 63, + "head_sha": "old", + "pull_requests": [{"number": 1}], + }, + ] + monkeypatch.setattr( + sched, "active_workflow_runs", lambda repo, statuses=("queued", "in_progress"): runs + ) + pr = {"number": 1, "headRefOid": "head"} + + assert sched.stale_pr_run_ids( + "owner/repo", pr, workflow="Required OpenCode Review" + ) == ["61"] From 5c3e6b272838dca0a1496123976d5f6fcc26349d Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 08:37:44 +0900 Subject: [PATCH 2/5] test(actions): close a file-extension hole in the workflow-name premise `run_name_identifies_workflow` accepts a candidate followed by a space, which is safe only while no central workflow name prefixes another. The test pinning that premise globbed `*.yml` alone, so a future `.yaml` workflow would drop out of the check silently -- and the `>= 30` vacuity guard would not catch it either, since 35 files minus one still clears the floor. A test whose only purpose is announcing a change nobody would otherwise see must not be bypassable by a file extension. Reported by a peer session's read review of #1993. Both extensions are globbed now, and a second assertion requires every workflow file to contribute a name, so a file dropping out by losing its top-level `name:` fails as loudly as one dropping out by extension. Neither change alters today's result: 35 `.yml`, 0 `.yaml`, 35 distinct names. This closes a future hole rather than fixing a present miss, and is stated that way because nothing in the tree can demonstrate it yet. Co-Authored-By: Claude Opus 5 --- ...est_stale_run_cleanup_workflow_identity.py | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/test_stale_run_cleanup_workflow_identity.py b/tests/test_stale_run_cleanup_workflow_identity.py index c7f810fff4..6842fc27f7 100644 --- a/tests/test_stale_run_cleanup_workflow_identity.py +++ b/tests/test_stale_run_cleanup_workflow_identity.py @@ -291,15 +291,33 @@ def test_no_central_workflow_name_prefixes_another() -> None: answering for it. Nothing else in the repository would notice; this test is the notice. """ + # Both extensions, though every workflow here is currently ``.yml``. A + # single ``*.yml`` glob would drop a future ``.yaml`` workflow out of the + # premise silently, and the count guard below would not notice either -- + # 35 files minus one is still comfortably over the floor. A test whose + # whole purpose is announcing a change nobody would otherwise see must not + # be bypassable by a file extension. + workflows = sorted( + { + *Path(".github/workflows").glob("*.yml"), + *Path(".github/workflows").glob("*.yaml"), + } + ) names = sorted( { line.split(":", 1)[1].strip().strip("\"'") - for workflow in sorted(Path(".github/workflows").glob("*.yml")) + for workflow in workflows for line in workflow.read_text(encoding="utf-8").splitlines() if line.startswith("name:") } ) + # Refuse a vacuous pass: an empty or mis-rooted glob makes the collision + # check below trivially true, which is the exact shape this repository has + # shipped before (an audit reporting "PASS: all 0 repositories"). assert len(names) >= 30 + # Every workflow file must contribute a name, or a file could drop out of + # the premise by losing its top-level ``name:`` rather than its extension. + assert len(names) == len(workflows) collisions = [ (shorter, longer) for shorter in names From b36d3d8dfba64768f99bde4b58da9574c4872e69 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 09:04:03 +0900 Subject: [PATCH 3/5] fix(strix): split repository-wide serialization policy --- scripts/ci/pr_review_merge_scheduler_core.py | 22 ++---------- tests/test_pr_review_merge_scheduler.py | 35 ------------------- ...est_stale_run_cleanup_workflow_identity.py | 2 +- 3 files changed, 3 insertions(+), 56 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 2c60c44636..42ddb5b152 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -3199,8 +3199,8 @@ def run_name_identifies_workflow(run_name: str, *workflow_names: str) -> bool: Two things make that safe rather than latent. No central workflow name prefixes another (verified 0 of 35 on 2026-09-07, and pinned by :mod:`tests.test_stale_run_cleanup_workflow_identity`), and every call site - pins identity a second time -- by ``display_title`` prefix or by the run's - ``path`` -- so this predicate narrows a candidate set rather than deciding + pins identity a second time -- by ``display_title`` prefix, the run's + ``path``, or pull-request metadata -- so this predicate narrows a candidate identity alone. Accepting a prefix is also required, not merely tolerated: callers pass short aliases ("Strix") for the same workflow on purpose. """ @@ -3825,24 +3825,6 @@ def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry return "already_running" target_repo = validate_github_repository(repo) dispatch_repo = repository_dispatch_target(target_repo) - cancelled_ids = {run_id for _, run_id in cancelled_refs} - busy_refs = [ - (dispatch_repo, str(run_data["id"])) - for run_data in active_workflow_runs(dispatch_repo) - if run_data.get("id") - and str(run_data["id"]) not in cancelled_ids - and run_name_identifies_workflow(str(run_data.get("name") or ""), workflow) - and run_data.get("event") == "repository_dispatch" - and str(run_data.get("display_title") or "").startswith( - f"Strix Security Scan {target_repo}#" - ) - ] - if busy_refs: - print( - "Strix evidence dispatch skipped: target repository already has active run(s) " - + ", ".join(f"{run_repo}@{run_id}" for run_repo, run_id in busy_refs) - ) - return "repository_busy" if not review_dispatch_admitted("strix", repo, pr): return "admission_deferred" base_ref, base_sha, head_sha = validated_pr_dispatch_fields(pr) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index e3a69677c3..4319850616 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -6077,41 +6077,6 @@ def fake_run(args, stdin=None): ) -def test_dispatch_strix_waits_for_active_target_repository_run(monkeypatch, capsys): - calls = [] - # The run name is the *rendered* ``run-name:``, identical to - # ``display_title`` -- the only shape GitHub actually sends for a workflow - # that declares one. The previous fixture paired a bare ``name`` with a - # rendered ``display_title``, a combination GitHub cannot produce, so this - # test passed while the production predicate matched 0 of 100 live runs. - active_run = { - "id": 9350, - "name": f"Strix Security Scan owner/repo#2@{'c' * 40}", - "event": "repository_dispatch", - "display_title": f"Strix Security Scan owner/repo#2@{'c' * 40}", - "pull_requests": [], - } - - monkeypatch.setattr( - sched, - "active_workflow_runs", - lambda repo, statuses=("queued", "in_progress"): [active_run], - ) - monkeypatch.setattr(sched, "run_github_dispatch", lambda args, stdin=None: calls.append(args)) - monkeypatch.setenv("GITHUB_ACTIONS", "true") - monkeypatch.setenv("GH_TOKEN", "workflow-token") - monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") - - result = sched.dispatch_strix_evidence( - "owner/repo", - "Strix Security Scan", - make_pr(headRefOid="a" * 40), - dry_run=False, - ) - - assert result == "repository_busy" - assert calls == [] - 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): diff --git a/tests/test_stale_run_cleanup_workflow_identity.py b/tests/test_stale_run_cleanup_workflow_identity.py index 6842fc27f7..0f34e76ebc 100644 --- a/tests/test_stale_run_cleanup_workflow_identity.py +++ b/tests/test_stale_run_cleanup_workflow_identity.py @@ -271,7 +271,7 @@ def test_run_name_identifies_workflow_requires_a_word_boundary() -> None: written to say so rather than to imply the predicate resolves identity by itself: "Strix Security Scan Extended" *is* accepted, and is safe only because no central workflow name prefixes another and every call site pins - identity a second time by ``.path`` or ``display_title``. + identity again by ``.path``, ``display_title``, or pull-request metadata. """ assert not sched.run_name_identifies_workflow( "Strix Security Scanner owner/repo#7@abc", "Strix Security Scan" From da7636a7a5c01a24de4008195409ed00387bef9c Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 09:04:43 +0900 Subject: [PATCH 4/5] style(tests): normalize scheduler test spacing --- tests/test_pr_review_merge_scheduler.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 4319850616..06cf737780 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -6077,8 +6077,6 @@ def fake_run(args, stdin=None): ) - - def test_central_run_filter_accepts_the_run_name_github_actually_sends(monkeypatch): """A ``run-name:`` workflow reports the rendered title in ``name``. From c35acda41249b2492fa31d51d4398b15ecdb6617 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 10:27:29 +0900 Subject: [PATCH 5/5] fix(scheduler): restore a docstring sentence that a line-boundary edit inverted Splitting the `repository_busy` policy out of this branch also rewrapped the sentence explaining why a prefix accept is safe, and four words fell out at the line boundary. It read: ... so this predicate narrows a candidate identity alone. instead of: ... so this predicate narrows a candidate set rather than deciding identity alone. The shortened form asserts exactly what the sentence exists to deny, and it is the sentence a future reader relies on to decide whether the prefix accept can stand. No test reads prose, so the full suite stayed green over it; the split's own addition -- pull-request metadata as a third identity pin -- is correct and is kept. `cancelled_refs` also became assigned-but-never-read once `busy_refs` was removed, since that comprehension was its only consumer. Renamed to `_cancelled_refs`, matching the sibling call thirty lines above which already discards the same value as `_cancelled_run_refs`. Co-Authored-By: Claude Opus 5 --- scripts/ci/pr_review_merge_scheduler_core.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 42ddb5b152..5b30dab6b0 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -3201,8 +3201,9 @@ def run_name_identifies_workflow(run_name: str, *workflow_names: str) -> bool: :mod:`tests.test_stale_run_cleanup_workflow_identity`), and every call site pins identity a second time -- by ``display_title`` prefix, the run's ``path``, or pull-request metadata -- so this predicate narrows a candidate - identity alone. Accepting a prefix is also required, not merely tolerated: - callers pass short aliases ("Strix") for the same workflow on purpose. + set rather than deciding identity alone. Accepting a prefix is also + required, not merely tolerated: callers pass short aliases ("Strix") for + the same workflow on purpose. """ return any( run_name == candidate or run_name.startswith(f"{candidate} ") @@ -3811,7 +3812,7 @@ def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry run_title="Strix Security Scan", workflow_aliases=frozenset({"Strix Security Scan"}), ) - preserved_run_refs, cancelled_refs = _cancel_revalidated_review_run_refs( + preserved_run_refs, _cancelled_refs = _cancel_revalidated_review_run_refs( repo, workflow, pr, stale_run_refs ) current_run_refs = [*current_run_refs, *preserved_run_refs]