fix(opencode-review): drop head-SHA concurrency scoping, structurally close #1568 instead

.github/workflows/opencode-review.yml

@@ -10,31 +10,53 @@ on:
   # isolated in opencode-review-dispatch.yml on repository_dispatch only.
   pull_request_target:
     # `converted_to_draft` is included so a PR going draft mid-poll fires a
-    # fresh run of this same workflow: the head-scoped concurrency group below
-    # (`cancel-in-progress: true`) cancels any in-flight non-draft
-    # "Fail closed without a current-head OpenCode verdict" poll for that
-    # exact same head. Every non-closed admission path revalidates the live
-    # PR/head/state before dispatching, exempting, or polling so out-of-order
-    # draft/ready/closed events cannot publish stale evidence or wait on an
-    # impossible verdict.
+    # fresh run of this same workflow. That new run does NOT cancel the old
+    # one (see the concurrency block below): the in-flight "Fail closed
+    # without a current-head OpenCode verdict" poll for the prior state
+    # instead notices the live draft flag itself on its own next iteration
+    # and self-exits within one poll_interval_seconds. Every non-closed
+    # admission path revalidates the live PR/head/state before dispatching,
+    # exempting, or polling so out-of-order draft/ready/closed events cannot
+    # publish stale evidence or wait on an impossible verdict.
     types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed]
 
 concurrency:
-  # Scoped by exact head SHA (not just PR number) so a delayed, out-of-order
-  # run for an older head cannot cancel the authoritative run already active
-  # for a newer head -- GitHub cancels whichever run is currently active in
-  # the group when a new one starts, with no notion of "older"/"newer", so
-  # sharing a group across different heads let a stale event retire the
-  # current head's still-valid run before its own live-head check could ever
-  # reject it (Devin Review on `#1568`). Same-head events (draft<->ready
-  # transitions, a synchronize retry) still share one group, so
-  # `converted_to_draft` still cancels an active same-head verdict poll.
+  # Scoped by repository + PR number ONLY (not head SHA) with
+  # cancel-in-progress: false -- by explicit user directive on 2026-09-03,
+  # refined after review from two peer sessions to fully close the race this
+  # is actually protecting against, not just trade one failure mode for
+  # another.
+  #
+  # History: head-SHA scoping was added for Devin Review's `#1568` finding --
+  # GitHub cancels whichever run is currently active in a concurrency group
+  # when a new one starts, with no notion of "older"/"newer", so a delayed,
+  # out-of-order run for an older head could cancel the authoritative run
+  # already active for a newer head. Scoping by head SHA gave each push its
+  # own group so this couldn't happen -- but it also meant rapid successive
+  # pushes to the SAME PR no longer shared a group at all, so they stopped
+  # cancelling each other's in-flight runs and instead queued up
+  # independently, directly worsening the self-inflicted queue-thrashing
+  # pattern this org measured directly (236/300 cancelled runs attributed to
+  # concurrent push volume; see internal memory
+  # project_queue_thrashing_self_inflicted_2026_09_03).
+  #
+  # The actual fix is not to re-key the group but to stop cancelling within
+  # it: with cancel-in-progress: false, a late-arriving run for an older head
+  # never preempts whichever run is already active, at any arrival order --
+  # the #1568 race is structurally impossible here, not just less likely.
+  # The now-queued older-head run still gets a turn once the active run
+  # finishes, but by then the poll step's own live-head/live-state
+  # revalidation (re-run every iteration, already required for correctness
+  # regardless of this setting) sees the head has moved and self-exits within
+  # one poll_interval_seconds instead of running to completion or publishing
+  # stale evidence. Plain repo+PR-number scoping also means rapid pushes
+  # naturally serialize through one queue instead of spawning N independent
+  # per-head groups, which is what actually bounds queue depth here.
   group: >-
     opencode-review-bootstrap-${{
     github.event.pull_request.base.repo.full_name || github.repository }}-${{
-    github.event.pull_request.number || github.run_id }}-${{
-    github.event.pull_request.head.sha || github.run_id }}
-  cancel-in-progress: true
+    github.event.pull_request.number || github.run_id }}
+  cancel-in-progress: false
 
 permissions:
   contents: read

tests/test_opencode_required_verdict_regression.py

@@ -560,15 +560,18 @@ def test_fail_closed_step_exempts_a_pr_converted_to_draft_mid_poll(
 
     Devin Review on `#1568` found that `converted_to_draft` was missing from
     this workflow's `pull_request_target.types`, so converting a PR to draft
-    while an earlier event's "Fail closed" poll was still running never fired
-    a fresh run to cancel it via the PR-scoped `cancel-in-progress: true`
-    concurrency group -- the stale non-draft poll kept waiting for a verdict
-    the now-draft PR can never receive. Adding `converted_to_draft` to the
-    trigger set lets a fresh run's draft exemption below take over; this test
-    proves that exemption exits before ever reaching the Reviews API for the
-    exact `PR_ACTION=converted_to_draft` value GitHub sends for that event
-    (`PR_DRAFT` is always `"true"` on that event, mirroring GitHub's own
-    payload).
+    while an earlier event's "Fail closed" poll was still running left the
+    stale non-draft poll waiting for a verdict the now-draft PR can never
+    receive -- nothing re-triggered it to notice sooner. Adding
+    `converted_to_draft` to the trigger set doesn't cancel that in-flight
+    poll (the concurrency group is `cancel-in-progress: false`, see the
+    workflow's own comment); instead it's the in-flight poll's own live-state
+    recheck (already run every iteration) that notices the draft flag on its
+    next pass and exits within one `poll_interval_seconds`. This test proves
+    the step-level exemption logic that recheck relies on exits before ever
+    reaching the Reviews API for the exact `PR_ACTION=converted_to_draft`
+    value GitHub sends for that event (`PR_DRAFT` is always `"true"` on that
+    event, mirroring GitHub's own payload).
     """
     result = _run_fail_closed_step(
         tmp_path, pr_action="converted_to_draft", pr_draft="true"
@@ -595,32 +598,40 @@ def test_opencode_review_trigger_reacts_to_mid_poll_draft_conversion() -> None:
         "types: [opened, synchronize, reopened, ready_for_review, "
         "converted_to_draft, closed]"
     ) in trigger_block
-    assert "cancel-in-progress: true" in workflow
+    assert "cancel-in-progress: false" in workflow
 
 
-def test_opencode_review_concurrency_group_is_scoped_by_exact_head() -> None:
-    """The bootstrap concurrency group is keyed by head SHA, not just PR number.
+def test_opencode_review_concurrency_group_is_scoped_by_repo_and_pr_only() -> None:
+    """The bootstrap group is keyed by repo + PR number only, and never cancels.
 
-    Devin Review on `#1568` found that a delayed, out-of-order run for an
-    older head could cancel the authoritative run already active for a
-    newer head: GitHub cancels whichever run is currently active in a
+    Devin Review on `#1568` originally found that a delayed, out-of-order run
+    for an older head could cancel the authoritative run already active for
+    a newer head (GitHub cancels whichever run is currently active in a
     concurrency group when a new one starts, with no notion of "older" or
-    "newer", so a group shared across different heads let a stale event
-    retire the current head's still-valid run before its own live-head
-    check could ever reject it. Scoping the group by exact head SHA
-    isolates different heads from each other while events for the exact
-    same head (a `converted_to_draft`/`ready_for_review` transition, a
-    `synchronize` retry) still share one group and can still cancel each
-    other, which is what lets `converted_to_draft` retire an active
-    same-head verdict poll.
+    "newer"), and scoping the group by exact head SHA was the fix landed at
+    the time. Reverted 2026-09-03 by explicit user directive, refined after
+    peer review: head-SHA scoping meant every push to a PR got its own group,
+    so rapid successive pushes no longer cancelled each other's in-flight
+    runs -- they queued up independently instead, worsening the
+    self-inflicted queue-thrashing pattern this org measured directly
+    (236/300 cancelled runs attributed to concurrent push volume). Plain
+    repo+PR-number scoping combined with `cancel-in-progress: false`
+    structurally closes the #1568 race instead of just trading it for another
+    failure mode: nothing in this group is ever preempted regardless of
+    arrival order, so a late-arriving older-head run can never evict a
+    current one. The "Fail closed without a current-head OpenCode verdict"
+    step's own live-head/live-state revalidation (already run every poll
+    iteration for correctness) is what makes a now-queued older-head run
+    self-exit quickly once it finally gets its turn, instead of running to
+    completion or publishing stale evidence.
     """
     workflow = WORKFLOW.read_text(encoding="utf-8")
     concurrency_block = workflow.split("\n\nconcurrency:\n", 1)[1].split(
         "\n\npermissions:", 1
     )[0]
-    assert "github.event.pull_request.head.sha || github.run_id" in concurrency_block
+    assert "github.event.pull_request.head.sha || github.run_id" not in concurrency_block
     assert "github.event.pull_request.number || github.run_id" in concurrency_block
-    assert "cancel-in-progress: true" in concurrency_block
+    assert "cancel-in-progress: false" in concurrency_block
 
 
 def test_fail_closed_step_closed_still_takes_precedence_over_draft(tmp_path: Path) -> None:

tests/test_required_workflow_queue_contract.py

@@ -242,7 +242,7 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None:
         assert "github.event.pull_request.base.repo.full_name" in concurrency_contract
         assert "github.repository" in concurrency_contract
         assert "github.event.pull_request.number" in workflow
-        if filename != "noema-review.yml":
+        if filename not in {"noema-review.yml", "opencode-review.yml"}:
             assert "cancel-in-progress: true" in workflow
         if filename in {
             "close-empty-pr.yml",
@@ -254,16 +254,25 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None:
             )
         elif filename == "opencode-review.yml":
             assert "opencode-review-bootstrap-" in concurrency_contract
-            # Unlike the other required pull-request workflows below, this
-            # group is deliberately also scoped by exact head SHA: a
-            # delayed, out-of-order run for an older head must not be able
-            # to cancel the authoritative run already active for a newer
-            # head (Devin Review on `#1568`). Same-head events still share
-            # one group and can still cancel each other.
+            # Deliberately NOT scoped by head SHA and deliberately
+            # cancel-in-progress: false (reverted/refined 2026-09-03 by
+            # explicit user directive plus peer review): head-SHA scoping
+            # (originally added for Devin Review's `#1568` finding) meant
+            # every push to a PR got its own concurrency group, so rapid
+            # successive pushes no longer cancelled each other's in-flight
+            # runs -- they queued up independently instead, worsening the
+            # self-inflicted queue-thrashing pattern this org measured
+            # directly (236/300 cancelled runs from concurrent push volume).
+            # Plain repo+PR-number scoping with cancel-in-progress: false
+            # structurally closes the #1568 race instead of reopening it:
+            # nothing in the group is ever preempted, so a late-arriving
+            # older-head run can never evict a current one at any arrival
+            # order -- see the workflow's own comment for the full mechanism.
             assert (
                 "github.event.pull_request.head.sha || github.run_id"
-                in concurrency_contract
+                not in concurrency_contract
             )
+            assert "cancel-in-progress: false" in concurrency_contract
         elif filename == "noema-review.yml":
             assert "github.event.workflow_run" not in concurrency_contract
             assert "noema-review-${{" in concurrency_contract
@@ -279,7 +288,7 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None:
                 assert (
                     "github.event_name == 'pull_request_target'" in concurrency_contract
                 )
-        if filename not in {"noema-review.yml", "opencode-review.yml"}:
+        if filename != "noema-review.yml":
             assert "github.event.pull_request.head.sha" not in concurrency_contract
         assert "format('pr-{0}-{1}'" not in concurrency_contract