feat: implement issue #1422 — [Phase 4] pr-review runs are ~49% cancelled-while-pending — cancel-in-progress:false protects in-flight only, and cancelled renders as a failing check

.github/workflows/lint.yml

@@ -63,6 +63,7 @@
             tests/reviewer_report.bats \
             tests/comment_noise.bats \
             tests/auto_rebase_health.bats \
+            tests/pr_review_outcomes.bats \
             tests/model_pricing.bats \
             tests/test_review_cycle.bats \
             tests/test_pr_automation_budget.bats \
@@ -202,7 +203,7 @@
     timeout-minutes: 5
     steps:
       - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1  # v7.0.1
         # NOTE: persist-credentials is intentionally left at the default (true) —
         # vci_resolve_reusable needs `git fetch origin` to authenticate against this
         # private repo when resolving same-repo channel tags. Setting
         # persist-credentials: false would silently push every such caller into the

.github/workflows/pr-review.yml

@@ -83,39 +83,63 @@ permissions:
   checks: read
 
 concurrency:
-  # Event-triggered and mention-triggered runs for a specific PR get a per-PR
-  # concurrency slot so they never queue behind unrelated PRs.
-  # check_suite events carry the associated PR in pull_requests[0]; including
-  # that URL here prevents cross-PR cancellation when multiple PRs' CI suites
-  # complete simultaneously (each PR competes only with itself, which is safe
-  # since same-PR runs are idempotent on the same head SHA).
-  # Note: check_suite.pull_requests entries expose the API `url` field, not
-  # `html_url` — using `url` here matches the payload shape.
-  # Manual batch runs (workflow_dispatch without pr_url) use a shared slot.
+  # Per-PR-AND-head-SHA concurrency slot so runs never queue behind unrelated
+  # PRs, AND a new push (new head SHA) does not share a slot with an in-flight
+  # review of a now-stale SHA (#1422). SHA sources per trigger:
+  #   pull_request / pull_request_review → github.event.pull_request.head.sha
+  #   check_suite                        → github.event.check_suite.head_sha
+  #   repository_dispatch (mention)      → github.event.client_payload.sha (if sent)
+  # Triggers whose payload carries no SHA (workflow_dispatch's pr_url, a mention
+  # without a sha) fall back to the bare per-PR group — identical to the prior
+  # behaviour — so those are still serialized per PR.
+  # PR identity is normalized to {repo}/pull/{number} for SHA-bearing event
+  # triggers (pull_request, pull_request_review, check_suite) so all of them
+  # land in the same concurrency slot for the same PR regardless of whether
+  # the payload carries an html_url or an API url. URL-keyed triggers
+  # (workflow_dispatch pr_url, repository_dispatch client_payload.pr_url)
+  # keep their raw URL as the identifier. Manual batch runs (workflow_dispatch
+  # without pr_url) use a shared slot.
   group: >-
     ${{
       (inputs.pr_url
        || github.event.client_payload.pr_url
-       || github.event.pull_request.html_url
-       || github.event.check_suite.pull_requests[0].url)
-      && format('pr-review-pr-{0}',
+       || (github.event.pull_request.number && format('{0}/pull/{1}', github.repository, github.event.pull_request.number))
+       || (github.event.check_suite.pull_requests[0].number && format('{0}/pull/{1}', github.repository, github.event.check_suite.pull_requests[0].number)))
+      && format('pr-review-pr-{0}{1}',
            inputs.pr_url
            || github.event.client_payload.pr_url
-           || github.event.pull_request.html_url
-           || github.event.check_suite.pull_requests[0].url)
+           || (github.event.pull_request.number && format('{0}/pull/{1}', github.repository, github.event.pull_request.number))
+           || (github.event.check_suite.pull_requests[0].number && format('{0}/pull/{1}', github.repository, github.event.check_suite.pull_requests[0].number)),
+           (github.event.pull_request.head.sha
+            || github.event.check_suite.head_sha
+            || github.event.client_payload.sha)
+           && format('-{0}',
+                github.event.pull_request.head.sha
+                || github.event.check_suite.head_sha
+                || github.event.client_payload.sha)
+           || '')
       || 'pr-review-batch'
     }}
-  # cancel-in-progress is intentionally FALSE (#534). The per-PR group means
-  # the four trigger event types (check_suite, pull_request,
-  # pull_request_review, repository_dispatch) all land in the same slot for a
-  # given PR; with cancellation on, each new event killed the in-flight review,
-  # so reviews never completed and left a trail of `cancelled` check runs (the
-  # long-standing ~34% cancel rate). Same-head-SHA reviews are idempotent — the
-  # agent's marker makes a redundant run a fast no-op — so racing runs are safe
-  # to QUEUE rather than cancel: the first reviews, the rest no-op. No lost work,
-  # no cancelled-check noise. (Future refinement: key the group by head SHA so a
-  # new push doesn't queue behind a now-stale review; deferred because the SHA
-  # isn't in every trigger payload, e.g. workflow_dispatch's pr_url.)
+  # cancel-in-progress is intentionally FALSE (#534): re-enabling it kills the
+  # IN-FLIGHT review — strictly worse than dropping a pending duplicate.
+  # What FALSE does and does not protect (corrected per #1422 — the previous
+  # "no cancelled-check noise" claim was contradicted by measurement):
+  #   • IN-FLIGHT run — PROTECTED. A newer same-group event never cancels the
+  #     run that is already executing.
+  #   • PENDING run — NOT protected. GitHub's concurrency model admits one
+  #     running + one pending per group; a third same-group event evicts the
+  #     older PENDING run, which concludes as `cancelled`. Under high event
+  #     volume most runs are cancelled BEFORE they start (~49% over the
+  #     2026-08-02 window), and a cancelled run rendered as a failing check on
+  #     the PR (#1421). So FALSE does not eliminate cancelled-check noise; it
+  #     only moves it from in-flight to pending.
+  # Keying the group by head SHA (above) removes the dominant CROSS-SHA eviction
+  # path, so a run for the final head SHA is never evicted by churn on a stale
+  # SHA — the coverage guarantee argued in
+  # docs/pr-review-agent/concurrency-and-coverage.md. Same-head-SHA reviews stay
+  # idempotent (the agent's marker makes a redundant run a fast no-op), so the
+  # residual same-SHA races are safe to QUEUE rather than cancel. The outcome mix
+  # is now a tracked number via scripts/pr_review_health.sh (run-outcome by event).
   cancel-in-progress: false
 
 jobs:

docs/metrics-baseline.md

@@ -76,6 +76,36 @@ PRs it is labelled *(all PRs)*.
 | Redundancy — workflow runs/hr | — | **~52 runs/hr** (7-day window, all-PRs scope) | `pr_review_health.sh` run telemetry / `reviewer_report.sh` |
 | Redundancy — coverage overlap | — | PRs reviewed by ≥2 bots (see scorecard) | `reviewer_report.sh` "Coverage overlap" |
 | Noise — no-action comment share | — | **~12%** (all-comments historical estimate — phrase-scan over all bots, **not** a first-party marker count; see denominator reconciliation note and Correction 2026-08-02) | `reviewer_report.sh` "Agent comment noise" |
+| Noise — pr-review cancelled-run share | — | **~49%** cancelled (2026-08-02 3-hour observation; see below) | `pr_review_health.sh` "Run-outcome mix by triggering event" |
+
+### pr-review run-outcome mix — 2026-08-02 starting observation (#1422)
+
+The starting observation recorded by issue #1422, over a **3-hour window on 2026-08-02** (the
+epic #1402 PR traffic on `pr-review-trigger.yml`). This is the dated "before" the SHA-keyed
+concurrency change (#1422) and the trigger-narrowing coupled to #1408 are measured against. The
+after-run source is the `scripts/pr_review_health.sh` **"Run-outcome mix by triggering event"**
+table (pure aggregation in `scripts/lib/pr-review-outcomes.sh`), which makes this a tracked number
+on the existing daily report — no new cron.
+
+| Conclusion | Count |
+|---|---:|
+| cancelled | **49** |
+| success | 34 |
+| skipped | 17 |
+
+Cancellation rate ≈ **49%** (49 / 100), roughly the magnitude of the ~34% in-flight cancel rate
+that issue #534 addressed. Cancellations by triggering event in the same window:
+
+| Event | Cancelled |
+|---|---:|
+| `pull_request_review` | 21 |
+| `repository_dispatch` | 17 |
+| `pull_request` | 5 |
+| `check_suite` | 5 |
+| `workflow_dispatch` | 1 |
+
+Mechanism and the coverage guarantee behind the fix:
+[`docs/pr-review-agent/concurrency-and-coverage.md`](./pr-review-agent/concurrency-and-coverage.md).
 
 ### Noise metric — raw measurement details
 

docs/pr-review-agent/concurrency-and-coverage.md

@@ -0,0 +1,116 @@
+# PR-review concurrency, coverage & the cancelled-check surface
+
+This note is the documented argument behind the `concurrency` block in
+[`.github/workflows/pr-review.yml`](../../.github/workflows/pr-review.yml). It closes the
+falsifiability gap that issue #1422 raised: the previous design *assumed* "at least one review
+runs to completion at the final head SHA" but never demonstrated it, and its inline comment
+asserted "no cancelled-check noise" — which the 2026-08-02 measurement (~49% of runs cancelled)
+contradicted.
+
+## The concurrency model (what actually happens)
+
+`pr-review.yml` uses a per-PR **and per-head-SHA** concurrency group with
+`cancel-in-progress: false`. GitHub's concurrency model admits **one running + one pending** run
+per group. Given that model, for a single group:
+
+- A newer same-group event **never cancels the run already executing** (`cancel-in-progress: false`).
+  → the **in-flight** run is protected.
+- When a run is already running and one is already pending, a third same-group event **evicts the
+  older pending run**, which concludes as `cancelled`.
+  → the **pending** run is *not* protected.
+
+So `cancel-in-progress: false` moves cancellation from in-flight to pending; it does not remove it.
+This is the correction to the old comment (AC #5).
+
+## Why the group is keyed by head SHA (AC #3)
+
+Before #1422 the group was keyed by PR only (`pr-review-pr-<PR>`). Every event for a PR — across
+**every** head SHA — shared one slot, so rapid pushes plus review/CI events for *different* SHAs
+contended for the same running+pending pair. The dominant cancellation path was therefore
+**cross-SHA**: a run enqueued for a new push could be evicted by churn relating to an older,
+now-stale SHA (and vice-versa).
+
+Keying the group by `<PR>-<head-sha>` splits that contention per SHA:
+
+| Trigger | Head-SHA source in the group key |
+|---|---|
+| `pull_request` (`synchronize`/`reopened`/`ready_for_review`) | `github.event.pull_request.head.sha` |
+| `pull_request_review` | `github.event.pull_request.head.sha` |
+| `check_suite` (`completed`) | `github.event.check_suite.head_sha` |
+| `repository_dispatch` (mention) | `github.event.client_payload.sha` if the payload carries one |
+| `workflow_dispatch` (`pr_url`) | none — **falls back** to the bare `pr-review-pr-<PR>` group |
+
+Triggers whose payload carries **no** SHA (the deferred-refinement caveat in the original comment:
+`workflow_dispatch` with only a `pr_url`, or a mention without a sha) fall back to the exact prior
+per-PR group, so their behaviour is unchanged. This makes the change strictly additive: SHA-bearing
+triggers gain per-SHA isolation; SHA-less triggers behave as before.
+
+## Coverage argument — at least one completed review at the final head SHA (AC #4)
+
+Claim: **for any push, at least one review runs to completion at that push's head SHA.**
+
+Let `S` be the head SHA produced by a push, `N` the PR number, and
+`G(S) = pr-review-pr-{repo}/pull/{N}-{S}` its concurrency group. The PR identity is derived from
+the PR **number** field (not the raw event URL) so that `pull_request` (which carries an `html_url`)
+and `check_suite` (which carries an API `url`) produce the **same group key** for the same PR.
+
+1. A push emits `pull_request: synchronize`, whose group key resolves to `G(S)` (the SHA comes from
+   `github.event.pull_request.head.sha`). CI completing for `S` later emits `check_suite: completed`
+   with `github.event.check_suite.head_sha == S`, also resolving to `G(S)`. Every event *for `S`*
+   maps to `G(S)` and **no event for a different SHA maps to `G(S)`** — that is exactly what SHA
+   keying buys.
+2. Within `G(S)`, `cancel-in-progress: false` guarantees the running member is never cancelled by a
+   newer member. At most the *pending* member is evicted. Eviction requires a running member to
+   already exist — i.e. a run for `S` is already executing. So an eviction inside `G(S)` never
+   reduces the count of *executing* runs for `S` below one.
+3. Therefore some run for `S` reaches execution and is not cancelled. The agent's idempotency marker
+   (`<!-- pr-review-agent v1 sha=<HEAD> -->`) makes that run either post the review or confirm an
+   existing review at `S` — a completion. A duplicate racing run for `S` no-ops against the same
+   marker; it does not undo the completion.
+
+The property that could previously fail — a run for the final SHA evicted by churn on an **older**
+SHA sharing the PR-level slot — cannot occur once the older SHA lives in a different group `G(S')`.
+That is the concrete gap the SHA key closes.
+
+**Residual (stated honestly).** Three or more events for the *same* SHA in quick succession can still
+evict a pending same-SHA run, producing a `cancelled` conclusion. That cancelled run is a redundant
+duplicate of a run that is already executing for the same SHA (point 2), so it is not lost coverage —
+but it is still a `cancelled` check on the PR. `gh pr checks` buckets `cancelled` as `fail`; the
+automation surface our own tooling keys on does **not** (`scripts/lib/ci-status.sh`
+`compute_ci_status` treats `CANCELLED` as non-blocking, issue #608 / #1421 regression fixture in
+`tests/test_ci_status.bats`). Fully eliminating the same-SHA cancelled conclusion requires cutting
+the trigger fan-in itself — see the #1408 interaction below.
+
+## Interaction with #1408 (narrowing `pr-review-sweep`) (AC #6)
+
+`pr-review-sweep.yml` (#573/#898) re-dispatches a review for a PR that went green after a
+`ci-pending` / `ci-failing` skip, via two paths: a `workflow_run: completed` fast path and a cron
+backstop. #1408 proposes **narrowing** that sweep.
+
+The two mechanisms cover **different** failure modes, and this must stay true after #1408:
+
+- **This change (SHA keying)** guarantees the *final head SHA* is reviewed once it has been seen by a
+  trigger and CI has completed for it — it removes the *cross-SHA eviction* drop.
+- **The sweep** covers the case where a review was **skipped on purpose** because CI was pending or
+  failing at review time, and CI *later* goes green with **no new pr-review trigger** firing for that
+  transition. SHA keying does nothing for that case — no run was evicted; none was ever started for
+  the green state.
+
+Therefore #1408 must **not** narrow away the sweep's `ci-pending/ci-failing → green` re-trigger,
+because this change does not backstop it. What #1408 *can* safely narrow is any sweep behaviour that
+merely re-fires a review the SHA-keyed concurrency now already guarantees (a redundant belt-and-braces
+re-dispatch for a SHA that was already reviewed). The dependency is one-directional: land and verify
+the SHA-keyed coverage guarantee here **before** #1408 removes any re-trigger path, and keep the
+CI-transition re-trigger regardless. The pre-existing stall detector (`scripts/pr_stall_scan.sh`, run
+from `daily-pr-review-health.yml`) remains the detect-only net for a PR that ends up green +
+`REVIEW_REQUIRED` with no pending event — the instrument built *before* the timers narrow.
+
+## Measuring it (AC #1)
+
+The cancellation rate is now a **tracked number**, not an anecdote. `scripts/pr_review_health.sh`
+(daily, no new cron) renders a deterministic "Run-outcome mix by triggering event" table — success /
+cancelled / skipped / failure and a cancel-rate column, split by triggering event — alongside the
+existing p50/p95 convergence-latency table. The pure aggregation lives in
+`scripts/lib/pr-review-outcomes.sh` (unit-tested in `tests/pr_review_outcomes.bats`). The dated
+starting observation (49 cancelled / 34 success / 17 skipped over three hours on 2026-08-02) is
+recorded in [`docs/metrics-baseline.md`](../metrics-baseline.md).

docs/pr-review-agent/documentation-index.md

@@ -36,6 +36,13 @@ This repository contains several documentation files describing the PR Review Ag
   - How pr-review withholds approval on an unaddressed maintainer issue comment
   - Its fail-closed semantics, what clears it, and the `FORCE_REVIEW` bypass
 
+- **[concurrency-and-coverage.md](concurrency-and-coverage.md)** — Concurrency, coverage & the cancelled-check surface (#1422)
+  - What `cancel-in-progress: false` does and does not protect (in-flight yes, pending no)
+  - Why the concurrency group is keyed by head SHA, and the SHA-source per trigger
+  - How the PR identity is normalized to `{repo}/pull/{N}` so `pull_request` and `check_suite` share one slot
+  - The argument that at least one review completes at the final head SHA per push
+  - The #1408 sweep-narrowing interaction and how the outcome mix is now tracked
+
 ## Setting Up Authentication
 - **[machine-user-setup.md](machine-user-setup.md)** — Machine user and PAT setup
   - Create machine user account and org team