From bf24747a10ae691a7589972bf44fe8a8a80b5290 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 6 Sep 2026 19:27:37 +0900 Subject: [PATCH] ci(review): coalesce superseded OpenCode review dispatches before admission opencode-review-dispatch.yml carried its concurrency group only on the long opencode-review-target job. A job-level group is never evaluated while the whole run waits behind the organization job ceiling, so two dispatches for one pull request each queued for hours and each was allocated a runner before the older one could be discarded. Measured on 2026-09-06: of the five dispatch runs that passed validate-pr-metadata, four were then rejected by the privileged metadata check because the head had moved while they queued (34002473295, 34010256951, 34015973300, 34016922761), every one of them after coverage-source-tree and coverage-evidence had already run. The privileged check behaved correctly; the cost is that a runner slot is spent discovering that the review's subject no longer exists. Add the workflow-level group keyed by the dispatched pull request, matching codeql-scan-dispatch.yml's workflow-level group and the rationale recorded in strix.yml, noema-review.yml and opencode-review.yml. The job-level group stays. No behaviour changes between two runs that are both executing -- the job-level group already cancels there; what changes is that a superseded run is now cancelled while queued. Co-Authored-By: Claude Opus 5 --- .../workflows/opencode-review-dispatch.yml | 22 ++++++++++++ CHANGELOG.md | 4 +++ tests/test_opencode_agent_contract.py | 8 +++-- ...t_pr_review_autofix_nvidia_nim_contract.py | 2 +- .../test_required_workflow_queue_contract.py | 36 +++++++++++++++++++ 5 files changed, 69 insertions(+), 3 deletions(-) diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 26e8555967..fd0e5ff75d 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -11,6 +11,28 @@ on: repository_dispatch: types: [opencode-review] +concurrency: + # Workflow-level admission, for the same reason strix.yml, noema-review.yml and + # opencode-review.yml carry theirs at this level: a job-level group is never + # evaluated while the whole run waits behind the organization job ceiling, so + # superseded dispatches for one pull request coalesce only after each of them + # has already been allocated a runner. Measured on 2026-09-06: of the five + # dispatch runs that passed `validate-pr-metadata`, four were rejected hours + # later by `opencode-review`'s privileged metadata check because the head had + # moved while they queued (runs 34002473295, 34010256951, 34015973300, + # 34016922761) -- each after `coverage-source-tree` and `coverage-evidence` + # had run. Cancelling the superseded run at creation returns that slot instead + # of spending it to discover the review's subject no longer exists. + # + # The key is the target pull request, matching the job-level group below and + # codeql-scan-dispatch.yml's workflow-level group; `github.run_id` keeps runs + # without a payload in their own groups rather than colliding. + group: >- + opencode-review-dispatch-${{ + github.event.client_payload.target_repository || github.repository }}-${{ + github.event.client_payload.pr_number || github.run_id }} + cancel-in-progress: true + permissions: contents: read diff --git a/CHANGELOG.md b/CHANGELOG.md index 75a4109c9d..1f535e06c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +### Superseded OpenCode review dispatches coalesce before they take a runner + +- `opencode-review-dispatch.yml` now carries a workflow-level `concurrency` group keyed by the dispatched pull request (`opencode-review-dispatch--`, `cancel-in-progress: true`), matching `codeql-scan-dispatch.yml`'s workflow-level group and the rationale already recorded in `strix.yml`, `noema-review.yml` and `opencode-review.yml`: a job-level group is never evaluated while the whole run waits behind the organization job ceiling. The workflow kept its group only on the long `opencode-review-target` job, so two dispatches for one pull request each queued for hours and each was allocated a runner before the older one could be discarded. Measured on 2026-09-06: four of the five dispatch runs that passed `validate-pr-metadata` were rejected hours later by the privileged metadata check because the head had moved while they queued (runs `34002473295`, `34010256951`, `34015973300`, `34016922761`), each after `coverage-source-tree` and `coverage-evidence` had run. The privileged check itself is unchanged -- it rejected exactly what it should; what changes is that the superseded run is now cancelled at creation instead of spending a slot to discover its subject moved. + ### Strix gate names the sandbox bootstrap failure and retries it once - `scripts/ci/strix_quick_gate.sh` gives the Caido sandbox bootstrap race (`loginAsGuest failed after 10 attempts` on `127.0.0.1:`, upstream usestrix/strix#1036/#1037/#1056) its own bounded same-model retry budget, `STRIX_SANDBOX_BOOTSTRAP_RETRIES` (default 1), drawn on top of `STRIX_TRANSIENT_RETRY_PER_MODEL`. That budget is 0 in production because the gateway owns model failover, so the documented sandbox retry never ran: `argos` Strix run 34013128112 (2026-09-06) shows one attempt, `Docker image ready`, the proxy never reachable, Strix exiting after 240 s -- while the sidecar reported four ready and four deferred routes that were never called. The budget is charged in the same branch that grants the attempt, so a log matching the sandbox class together with a gateway class cannot extend the loop without charging it (caught by adversarial review of the first draft). The primary-scan verdict for that class now reads `STRIX_PROVIDER_UNAVAILABLE: STRIX_SANDBOX_UNAVAILABLE: the last Strix attempt ended in the sandbox bootstrap (...) after N sandbox-specific same-model retries (budget B); this verdict names Strix's sandbox, not the LLM gateway.` instead of `orchestrator/free exhausted`, stating only what the gate observed; the leading token is unchanged so the workflow's finding-free classification and its tests are untouched, and the second token lets the review census split sandbox outages from gateway ones (two of six recent Strix artifacts were this class). Refs #1948. diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 37ec068db9..321d25bd57 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1815,8 +1815,12 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): assert "run_opencode_review_model_pool.sh" in workflow assert "rekick_model_pool_on_exhaustion" not in workflow assert "publish stage performs no duplicate model-catalog pass" in workflow - concurrency_contract = workflow.split("concurrency:", 1)[1].split( - "permissions:", 1 + # The review job's own group, addressed by its indentation: the workflow + # also carries a workflow-level admission group (pinned in + # tests/test_required_workflow_queue_contract.py), so splitting on the + # first "concurrency:" would read that one instead of this one. + concurrency_contract = workflow.split("\n concurrency:", 1)[1].split( + "\n runs-on:", 1 )[0] assert "needs.validate-pr-metadata.outputs.target_repository" in concurrency_contract assert "needs.validate-pr-metadata.outputs.pr_number || github.run_id" in concurrency_contract diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 2d2304aaf1..af49190142 100644 --- a/tests/test_pr_review_autofix_nvidia_nim_contract.py +++ b/tests/test_pr_review_autofix_nvidia_nim_contract.py @@ -17,7 +17,7 @@ DOCTORING_RECORD = Path("docs/doctoring/hourly-nvidia-nim-autofix.md") CHANGELOG = Path("CHANGELOG.md") REVIEW_DISPATCH_WORKFLOW = Path(".github/workflows/opencode-review-dispatch.yml") -REVIEW_DISPATCH_BLOB_SHA = "26e8555967171a5f3974602ac05700c27bddebf1" +REVIEW_DISPATCH_BLOB_SHA = "fd0e5ff75d9ae6ec68cc6945174dff5c9d487662" def _workflow_text(path: Path) -> str: diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 803d43ab59..48035c2c40 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -214,6 +214,42 @@ def test_privileged_review_retries_use_default_branch_repository_dispatch() -> N assert '"gh",\n "workflow",\n "run"' not in autofix_scheduler +def test_privileged_review_dispatch_coalesces_superseded_runs_before_admission() -> None: + """A superseded dispatch must be cancelled while queued, not after it takes a runner. + + ``opencode-review-dispatch.yml`` carried its concurrency group only on the + long ``opencode-review-target`` job. A job-level group is not evaluated + while the whole run waits behind the organization job ceiling, so two + dispatches for one pull request each waited hours and each was allocated a + runner before the older one could be discarded. Measured on 2026-09-06: + four of the five dispatch runs that passed ``validate-pr-metadata`` were + then rejected by the privileged metadata check because the head had moved + while they queued, every one of them after ``coverage-source-tree`` and + ``coverage-evidence`` had already run. + + The workflow-level group is keyed by the dispatched pull request, matching + ``codeql-scan-dispatch.yml``'s workflow-level group and the job-level group + this workflow keeps for the review job itself. + """ + workflow = workflow_text("opencode-review-dispatch.yml") + header = workflow.split("permissions:", 1)[0] + concurrency_contract = header.split("concurrency:", 1)[1] + + assert re.search(r"(?m)^concurrency:", header) + assert "opencode-review-dispatch-" in concurrency_contract + assert ( + "github.event.client_payload.target_repository || github.repository" + in concurrency_contract + ) + assert ( + "github.event.client_payload.pr_number || github.run_id" + in concurrency_contract + ) + assert "cancel-in-progress: true" in concurrency_contract + assert "github.event.client_payload.pr_head_sha" not in concurrency_contract + assert re.search(r"(?m)^ concurrency:", workflow) + + def test_required_opencode_dispatch_does_not_wait_on_merge_scheduler() -> None: """Dispatch review execution directly so polling cannot starve its producer.""" workflow = workflow_text("opencode-review.yml")