diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index e86818640..f3ec34690 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -63,6 +63,7 @@ jobs: 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 \ diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index de1dd69da..eb74405ff 100644 --- a/.github/workflows/pr-review.yml +++ b/.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: diff --git a/docs/metrics-baseline.md b/docs/metrics-baseline.md index d85c67c1b..d783016b2 100644 --- a/docs/metrics-baseline.md +++ b/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 diff --git a/docs/pr-review-agent/concurrency-and-coverage.md b/docs/pr-review-agent/concurrency-and-coverage.md new file mode 100644 index 000000000..b575175ed --- /dev/null +++ b/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-`). 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 `-` 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-` 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 + (``) 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). diff --git a/docs/pr-review-agent/documentation-index.md b/docs/pr-review-agent/documentation-index.md index 528f85b23..35d3f4f7d 100644 --- a/docs/pr-review-agent/documentation-index.md +++ b/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 diff --git a/scripts/lib/pr-review-outcomes.sh b/scripts/lib/pr-review-outcomes.sh new file mode 100644 index 000000000..d02edee4e --- /dev/null +++ b/scripts/lib/pr-review-outcomes.sh @@ -0,0 +1,117 @@ +#!/usr/bin/env bash +# pr-review-outcomes.sh — pure helpers for the pr-review run-outcome-by-event +# breakdown surfaced on the daily health report (#1422 AC #1, epic #1402). +# +# The pr-review concurrency model admits one running + one pending run per group; +# a third same-group event evicts the older PENDING run, which concludes as +# `cancelled`. Over the 2026-08-02 observation window ~49% of runs were cancelled, +# and a cancelled run rendered as a failing check (#1421). Making the outcome mix a +# TRACKED number — split by the triggering event, since the fan-in is per-event — +# is what turns that cancellation rate from an anecdote into a monitored metric. +# +# These functions are PURE: they take run telemetry JSON on argv and write to +# stdout. No network, no side effects. Unit-tested in tests/pr_review_outcomes.bats. +# main-side network I/O lives in scripts/pr_review_health.sh, which sources this. + +# pr_review_outcomes_by_event +# Input : JSON array of run objects, each carrying {event, conclusion} as +# returned by the GitHub Actions runs API (.event / .conclusion). +# Output: TSV, one row per triggering event (sorted by event name) followed by a +# TOTAL row: +# eventsuccesscancelledskippedfailuretotal +# `total` is the count of ALL runs for the event (so in-flight/other +# conclusions are still counted in the denominator, not silently +# dropped). `failure` counts both `failure` and `timed_out` conclusions. +# A run with a null event folds into an "unknown" bucket. +# Absent/empty JSON → a single zeroed TOTAL row. +pr_review_outcomes_by_event() { + local json="${1:-}" + [ -n "$json" ] || json='[]' + jq -r ' + (if type == "array" then . else [] end) as $r + | ($r + | group_by(.event // "unknown") + | map({ + event: (.[0].event // "unknown"), + success: (map(select(.conclusion == "success")) | length), + cancelled: (map(select(.conclusion == "cancelled")) | length), + skipped: (map(select(.conclusion == "skipped")) | length), + failure: (map(select(.conclusion == "failure" or .conclusion == "timed_out")) | length), + total: length + }) + | sort_by(.event)) as $rows + | ($rows + [{ + event: "TOTAL", + success: ($rows | map(.success) | add // 0), + cancelled: ($rows | map(.cancelled) | add // 0), + skipped: ($rows | map(.skipped) | add // 0), + failure: ($rows | map(.failure) | add // 0), + total: ($rows | map(.total) | add // 0) + }]) + | .[] + | [ .event, + (.success | tostring), + (.cancelled | tostring), + (.skipped | tostring), + (.failure | tostring), + (.total | tostring) ] + | @tsv + ' <<< "$json" 2>/dev/null || printf 'TOTAL\t0\t0\t0\t0\t0\n' +} + +# _pr_review_cancel_rate +# Integer percentage "N%" of cancelled ÷ total, or "n/a" when total is 0 (a +# window with no runs must never divide by zero). Internal helper. +_pr_review_cancel_rate() { + local cancelled="${1:-0}" total="${2:-0}" + case "${total#-}" in + ''|*[!0-9]*) + printf 'n/a' + return 0 + ;; + esac + if [ "$total" -le 0 ]; then + printf 'n/a' + return 0 + fi + case "${cancelled#-}" in + ''|*[!0-9]*) + cancelled=0 + ;; + esac + printf '%d%%' "$(( cancelled * 100 / total ))" +} + +# pr_review_render_outcome_mix +# Renders the by-event outcome breakdown as a deterministic Markdown section +# (a table with a cancel-rate column). Pure: no network. Written by the health +# script into the report BEFORE any model-generated content so truncation can +# never discard it (same guarantee as the convergence-latency section). +pr_review_render_outcome_mix() { + local json="${1:-}" + [ -n "$json" ] || json='[]' + + printf '## Run-outcome mix by triggering event\n\n' + printf 'pr-review run conclusions over the window, split by the event that ' + printf 'triggered each run — computed deterministically by `%s`, not the model. ' "pr-review-outcomes.sh" + printf 'A high `cancelled` share means runs were evicted while PENDING ' + printf '(concurrency admits one running + one pending per group); a cancelled ' + printf 'run is non-blocking but historically rendered as a failing check (#1421).\n\n' + printf '| Event | success | cancelled | skipped | failure | total | cancel %% |\n' + printf '|---|---:|---:|---:|---:|---:|---:|\n' + + local event success cancelled skipped failure total rate label + while IFS=$'\t' read -r event success cancelled skipped failure total; do + [ -n "$event" ] || continue + rate="$(_pr_review_cancel_rate "$cancelled" "$total")" + if [ "$event" = "TOTAL" ]; then + label='**TOTAL**' + else + label="\`$event\`" + fi + printf '| %s | %s | %s | %s | %s | %s | %s |\n' \ + "$label" "$success" "$cancelled" "$skipped" "$failure" "$total" "$rate" + done < <(pr_review_outcomes_by_event "$json") + + printf '\n' +} diff --git a/scripts/pr_review_health.sh b/scripts/pr_review_health.sh index 5a8085f19..409dd671d 100644 --- a/scripts/pr_review_health.sh +++ b/scripts/pr_review_health.sh @@ -14,6 +14,11 @@ set -euo pipefail +# Pure by-event outcome-mix helpers (#1422). Sourced so the cancellation-rate +# breakdown is unit-tested (tests/pr_review_outcomes.bats) rather than inlined. +# shellcheck source=scripts/lib/pr-review-outcomes.sh +source "$(dirname "${BASH_SOURCE[0]}")/lib/pr-review-outcomes.sh" + LOOKBACK_DAYS="${LOOKBACK_DAYS:-1}" WORKFLOW_REPO="${AGENT_REPO:-petry-projects/.github-private}" WORKFLOW_FILE="pr-review-trigger.yml" @@ -50,16 +55,18 @@ CUTOFF=$(date -u -d "${LOOKBACK_DAYS} days ago" +%Y-%m-%dT%H:%M:%SZ 2>/dev/null echo "Fetching runs since: $CUTOFF" runs_json=$(gh api \ + --paginate \ "repos/${WORKFLOW_REPO}/actions/workflows/${WORKFLOW_FILE}/runs?per_page=100&created=>=${CUTOFF}" \ --jq '.workflow_runs | map({ id: .id, run_number: .run_number, status: .status, conclusion: .conclusion, + event: .event, created_at: .created_at, html_url: .html_url, duration_s: ((.updated_at | fromdate) - (.created_at | fromdate)) - })' 2>/dev/null || echo '[]') + })' 2>/dev/null | jq -s 'add // []' 2>/dev/null || echo '[]') # --------------------------------------------------------------------------- # 2. Compute aggregate stats @@ -176,9 +183,15 @@ workflow_source=$(gh api "repos/${WORKFLOW_REPO}/contents/.github/workflows/${WO printf '| `%s` | %s | %s | %s | %s | %s |\n' \ "$WORKFLOW_FILE" "$dur_n" \ "$(fmt_dur "$dur_min")" "$(fmt_dur "$dur_p50")" "$(fmt_dur "$dur_p95")" "$(fmt_dur "$dur_max")" + printf '\n' + # Run-outcome mix by triggering event (#1422). Deterministic — same truncation + # guarantee as the percentiles above: written before the model-generated body. + pr_review_render_outcome_mix "$runs_json" } > "$REPORT_FILE" echo "Duration percentiles — p50 $(fmt_dur "$dur_p50") / p95 $(fmt_dur "$dur_p95") across $dur_n run(s)" +echo "Outcome mix (by event):" +pr_review_outcomes_by_event "$runs_json" | sed 's/^/ /' echo "Invoking Claude for log analysis..." # Claude writes errors to stdout (not stderr), so they'd silently land in @@ -189,7 +202,7 @@ You are analyzing GitHub Actions workflow run logs for the PR Review Agent. ## Context - Workflow: \`${WORKFLOW_FILE}\` in repo \`${WORKFLOW_REPO}\` -- Analysis window: last ${LOOKBACK_DAYS} days, up to 100 most recent runs +- Analysis window: last ${LOOKBACK_DAYS} days (all runs fetched via pagination) - Report date: ${TODAY} - Total runs fetched: ${total_runs} | Successful: ${success_runs} | Failed: ${failed_runs} | Cancelled: ${cancelled_runs} - Run duration across ${dur_n} completed run(s): p50 $(fmt_dur "$dur_p50") | p95 $(fmt_dur "$dur_p95") (min $(fmt_dur "$dur_min") / max $(fmt_dur "$dur_max")) diff --git a/tests/pr_review_outcomes.bats b/tests/pr_review_outcomes.bats new file mode 100644 index 000000000..67eddb93d --- /dev/null +++ b/tests/pr_review_outcomes.bats @@ -0,0 +1,160 @@ +#!/usr/bin/env bats +# Unit tests for scripts/lib/pr-review-outcomes.sh — the pure run-outcome-by-event +# breakdown surfaced on the pr-review health report (#1422 AC #1). +# +# These functions take run telemetry JSON on argv and write to stdout; no network. +# Run locally: bats tests/pr_review_outcomes.bats + +setup() { + # shellcheck source=scripts/lib/pr-review-outcomes.sh + source "${BATS_TEST_DIRNAME}/../scripts/lib/pr-review-outcomes.sh" + + # Representative run telemetry mirroring the 2026-08-02 3-hour observation + # shape: cancellations dominated by pull_request_review and repository_dispatch, + # with a mix of success/skipped. Conclusions/events use the exact strings the + # GitHub Actions runs API returns. + RUNS_JSON='[ + {"event":"pull_request_review","conclusion":"cancelled"}, + {"event":"pull_request_review","conclusion":"cancelled"}, + {"event":"pull_request_review","conclusion":"success"}, + {"event":"repository_dispatch","conclusion":"cancelled"}, + {"event":"repository_dispatch","conclusion":"success"}, + {"event":"pull_request","conclusion":"success"}, + {"event":"pull_request","conclusion":"skipped"}, + {"event":"check_suite","conclusion":"cancelled"}, + {"event":"check_suite","conclusion":"skipped"}, + {"event":"workflow_dispatch","conclusion":"failure"} + ]' +} + +# --------------------------------------------------------------------------- +# pr_review_outcomes_by_event — TSV aggregation +# --------------------------------------------------------------------------- + +@test "outcomes_by_event: emits one sorted row per event plus a TOTAL row" { + run pr_review_outcomes_by_event "$RUNS_JSON" + [ "$status" -eq 0 ] + # 5 distinct events + TOTAL + [ "$(printf '%s\n' "$output" | grep -c .)" -eq 6 ] + # Events are sorted; first data row is check_suite, last data row workflow_dispatch + first_event=$(printf '%s\n' "$output" | head -n1 | cut -f1) + [ "$first_event" = "check_suite" ] + last_line=$(printf '%s\n' "$output" | tail -n1 | cut -f1) + [ "$last_line" = "TOTAL" ] +} + +@test "outcomes_by_event: per-event columns are success/cancelled/skipped/failure/total" { + run pr_review_outcomes_by_event "$RUNS_JSON" + # pull_request_review: 1 success, 2 cancelled, 0 skipped, 0 failure, 3 total + row=$(printf '%s\n' "$output" | awk -F'\t' '$1=="pull_request_review"') + [ "$row" = "$(printf 'pull_request_review\t1\t2\t0\t0\t3')" ] +} + +@test "outcomes_by_event: TOTAL row aggregates every event" { + run pr_review_outcomes_by_event "$RUNS_JSON" + # totals: success 3, cancelled 4, skipped 2, failure 1, total 10 + total=$(printf '%s\n' "$output" | awk -F'\t' '$1=="TOTAL"') + [ "$total" = "$(printf 'TOTAL\t3\t4\t2\t1\t10')" ] +} + +@test "outcomes_by_event: #1421 regression — a cancelled pull_request_review run counts as cancelled, never failure" { + # PR #1421: the review run conclusion was `cancelled` (evicted while pending), + # but gh pr checks rendered it `fail`. The breakdown must classify it as + # cancelled and must NOT inflate the failure column. + local one_run='[{"event":"pull_request_review","conclusion":"cancelled"}]' + run pr_review_outcomes_by_event "$one_run" + [ "$status" -eq 0 ] + row=$(printf '%s\n' "$output" | awk -F'\t' '$1=="pull_request_review"') + # success 0, cancelled 1, skipped 0, failure 0, total 1 + [ "$row" = "$(printf 'pull_request_review\t0\t1\t0\t0\t1')" ] +} + +@test "outcomes_by_event: timed_out runs are classified as failure, not cancelled" { + local one_run='[{"event":"pull_request","conclusion":"timed_out"}]' + run pr_review_outcomes_by_event "$one_run" + [ "$status" -eq 0 ] + row=$(printf '%s\n' "$output" | awk -F'\t' '$1=="pull_request"') + # success 0, cancelled 0, skipped 0, failure 1, total 1 + [ "$row" = "$(printf 'pull_request\t0\t0\t0\t1\t1')" ] +} + +@test "outcomes_by_event: empty array yields a zeroed TOTAL row only" { + run pr_review_outcomes_by_event '[]' + [ "$status" -eq 0 ] + [ "$output" = "$(printf 'TOTAL\t0\t0\t0\t0\t0')" ] +} + +@test "outcomes_by_event: absent/empty argument is treated as empty (no error)" { + run pr_review_outcomes_by_event "" + [ "$status" -eq 0 ] + [ "$output" = "$(printf 'TOTAL\t0\t0\t0\t0\t0')" ] +} + +@test "outcomes_by_event: runs with a null event fold into an 'unknown' bucket" { + run pr_review_outcomes_by_event '[{"conclusion":"success"}]' + [ "$status" -eq 0 ] + row=$(printf '%s\n' "$output" | awk -F'\t' '$1=="unknown"') + [ "$row" = "$(printf 'unknown\t1\t0\t0\t0\t1')" ] +} + +# --------------------------------------------------------------------------- +# pr_review_render_outcome_mix — Markdown section +# --------------------------------------------------------------------------- + +@test "render_outcome_mix: renders a table with a cancel-rate column and every event" { + run pr_review_render_outcome_mix "$RUNS_JSON" + [ "$status" -eq 0 ] + [[ "$output" == *"Run-outcome mix by triggering event"* ]] + [[ "$output" == *"cancelled"* ]] + [[ "$output" == *"cancel %"* ]] + [[ "$output" == *"pull_request_review"* ]] + [[ "$output" == *"repository_dispatch"* ]] + [[ "$output" == *"TOTAL"* ]] +} + +@test "render_outcome_mix: TOTAL cancel-rate reflects 4 of 10 cancelled" { + run pr_review_render_outcome_mix "$RUNS_JSON" + # 4/10 = 40% + [[ "$output" == *"40%"* ]] +} + +@test "render_outcome_mix: empty telemetry renders the section with an n/a cancel rate (no divide-by-zero)" { + run pr_review_render_outcome_mix '[]' + [ "$status" -eq 0 ] + [[ "$output" == *"Run-outcome mix by triggering event"* ]] + [[ "$output" == *"n/a"* ]] +} + +# --------------------------------------------------------------------------- +# _pr_review_cancel_rate — integer validation via case statement +# --------------------------------------------------------------------------- + +@test "_pr_review_cancel_rate: non-numeric total returns n/a without crashing" { + run _pr_review_cancel_rate 5 "abc" + [ "$status" -eq 0 ] + [ "$output" = "n/a" ] +} + +@test "_pr_review_cancel_rate: empty total returns n/a without crashing" { + run _pr_review_cancel_rate 5 "" + [ "$status" -eq 0 ] + [ "$output" = "n/a" ] +} + +@test "_pr_review_cancel_rate: non-numeric cancelled is treated as 0" { + run _pr_review_cancel_rate "abc" 10 + [ "$status" -eq 0 ] + [ "$output" = "0%" ] +} + +@test "_pr_review_cancel_rate: valid integers produce correct percentage" { + run _pr_review_cancel_rate 4 10 + [ "$status" -eq 0 ] + [ "$output" = "40%" ] +} + +@test "_pr_review_cancel_rate: zero total returns n/a" { + run _pr_review_cancel_rate 0 0 + [ "$status" -eq 0 ] + [ "$output" = "n/a" ] +} diff --git a/tests/test_ci_status.bats b/tests/test_ci_status.bats index 3015e65ec..45d5e546b 100644 --- a/tests/test_ci_status.bats +++ b/tests/test_ci_status.bats @@ -419,6 +419,25 @@ rollup() { [ "$output" = "passing" ] } +@test "own pr-review 'review / review' check CANCELLED → passing (issue #1421 regression)" { + # PR #1421: the pr-review run was evicted while pending, leaving a terminal + # CANCELLED "review / review" check that gh pr checks rendered as `fail`. The + # cascade must not treat its own cancelled check as a merge-blocking failure. + local r + r=$(rollup "$(check_run "review / review" "COMPLETED" "CANCELLED")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + +@test "own pr-review CANCELLED check alongside external SUCCESS → passing (issue #1421)" { + local r + r=$(rollup \ + "$(check_run "review / review" "COMPLETED" "CANCELLED")" \ + "$(check_run "Build" "COMPLETED" "SUCCESS")") + run compute_ci_status "$r" + [ "$output" = "passing" ] +} + @test "status context CANCELLED-equivalent: real TIMED_OUT conclusion still → failing" { # Guards that only CANCELLED is whitelisted — other non-success terminal # conclusions (e.g. TIMED_OUT) must still block.