diff --git a/docs/reviewer-report.md b/docs/reviewer-report.md index e2c2c62d0..07fadd85e 100644 --- a/docs/reviewer-report.md +++ b/docs/reviewer-report.md @@ -18,7 +18,8 @@ matching the delivery pattern of the [Token Cost Observatory](./token-report.md) ## Reviewers tracked Identity comes from each bot's GraphQL App **login** — the single source of truth is the -advisory-review gate (`scripts/lib/advisory-review-gate.sh`), so this report can never +reviewer-source registry (`scripts/lib/reviewer-sources.tsv`, [#1425](https://github.com/petry-projects/.github-private/issues/1425)), +which the advisory-review gate and this report both project from, so the report can never drift from the approval gate's notion of who these bots are. | Reviewer | GraphQL login | @@ -44,18 +45,61 @@ counts), and issue `comments`. Pagination stops as soon as a page's PRs predate lookback window (PRs are ordered by `UPDATED_AT` descending), so we never walk a repo's entire history. This is far fewer API calls than per-PR REST fetches. +Pages are deliberately **light** — `PR_PAGE_SIZE` PRs per page (default 10) with bounded +nested connections — because a heavy page trips GitHub's GraphQL resource limit, which +returns an HTTP-200 body carrying an `errors` array and **null `data`**. The old code read +that as "no PRs" and silently dropped the whole repo (the ~8× Graphite undercount, [#1908](https://github.com/petry-projects/.github-private/issues/1908)). +A failed page is now **retried with a halved page size and backoff** (`PR_FETCH_MAX_ATTEMPTS`, +default 4); a repo that still fails is recorded as a `{kind:"collect_error"}` record and +surfaced prominently in the report — never a silent drop. + +### Check-run reviews (Graphite) + +Some reviewers report a review **only as a check run**, not as a PR review or comment. +Graphite's "Graphite / AI Reviews" check run is the canonical case: a clean pass posts the +check run with **zero** PR comments, so the old scorecard read every clean pass as *No +response* and undercounted its engagement. Which bots report this way — and the exact +check-run name — is **data-driven** in the reviewer-source registry +(`scripts/lib/reviewer-sources.tsv`, `check_run_name` column); nothing is hard-coded to +`graphite-app`. For each in-window PR we make a **separate, lightweight REST call** for the +head commit's check runs (`GET /repos/{o}/{r}/commits/{sha}/check-runs`) so the heavy PR +page query stays under the resource limit. Each matched check run is classified: + +- **Real response.** The run is completed, did **not** fail, and either has a `success` + conclusion or a summary that says the review ran. Latency is measured to its + `completed_at`. A run that failed does not count, even if its summary mentions the + review. Failed conclusions are `failure`, `cancelled`, `timed_out`, `action_required`, + `stale` and `startup_failure`. +- **Refusal.** The run is `completed/skipped` and its summary gives a decline reason, such + as "too large", "did not run" or a quota/rate-limit notice. +- **Contributes nothing.** Everything else: queued or in-progress runs, failed runs, and + runs skipped with an empty or unrelated summary. Without a PR review or comment, the PR + stays in **No response**. + Each PR node is normalized (pure `jq`) into two record kinds: - `{kind:"pr", …}` — one per PR (the eligible-PR **denominator**). - `{kind:"bot_pr", …}` — one per tracked bot that touched the PR, carrying its - `real_responses` / `refusals` split (a rate-limit-only touch is a refusal), latency to - first real response, verdict counts, inline-comment count, thread resolution, and reactions. + `real_responses` / `refusals` split (a rate-limit-only touch is a refusal, a check-run + "too large" skip is a refusal), latency to first real response, verdict counts, + inline-comment count, thread resolution, and reactions. A check-run clean pass adds **0** + to the review-event count but fills the `real_responses` / reviewed bucket. + +Three diagnostic record kinds keep collection gaps visible (never silent, [#1908](https://github.com/petry-projects/.github-private/issues/1908)): + +- `{kind:"collect_error", repo, reason}` — a repo that could not be collected after retries. +- `{kind:"truncation", repo, pr, connection}` — a nested connection (≥50), a review thread's + comments (≥20), or the per-repo PR pagination (`MAX_PR_PAGES`) that hit its fetch cap, so a + bot's reviews/comments on that PR — or a repo's later in-window PRs — may be undercounted. +- `{kind:"check_run_error", repo, pr}` — a per-PR check-run REST fetch that failed, so a + check-run reporter's clean pass on that PR may be missed and miscounted as *No response*. -Human authors and untracked bots are dropped at normalization time. +All are rendered in a **Collection health** section at the top of the report. Human authors +and untracked bots are dropped at normalization time. ## Metrics -Everything is deterministic. **Reviews** and **Rate-limited** count *every event*, not +Everything is deterministic. **Reviews** and **Refused** count *every event*, not distinct PRs — GitHub creates a new review submission each time a bot re-reviews after a new commit, so a PR reviewed across 5 commits contributes 5 reviews. This matters: on live data CodeRabbit produced ~2× more review events than the distinct-PR count would suggest (up to 11 @@ -66,11 +110,11 @@ reviews on a single PR). | **Total PRs** | Review-eligible (non-draft) PRs active in the window; the denominator each row is measured against. | | **Reviews** | Count of reviews the bot submitted, **each occurrence** (multiple per PR from multiple commits all count). A bot that posts no formal review but delivers its verdict as a top-level comment (e.g. SonarCloud's quality-gate comment) has that comment counted as its review; bots that do submit formal reviews are unaffected, so their extra summary comments are never double-counted. Rate-limit notices are never counted. | | **✅ / 🔄** | Of those reviews, how many carried state APPROVED / CHANGES_REQUESTED. | -| **Rate-limited** | Count of out-of-quota / rate-limit refusal events (detected by body text via the shared gate pattern), each occurrence. | -| **No response** | Eligible PRs the bot never engaged with at all — no review, comment, or refusal. | +| **Refused** | Count of refusal events — out-of-quota / rate-limit notices (detected by body text via the shared gate pattern) **and** a check-run reporter's own decline (a `too large` / `did not run` skip) — each occurrence. | +| **No response** | Eligible PRs the bot never engaged with at all: no review, no comment, no refusal, and **no check run classified as a real response or a refusal**. For a check-run reporter (Graphite), a clean-pass check run counts as engagement, so it no longer lands here. A queued, failed or unexplained-skip run still leaves the PR in **No response**. | | **Latency p50 / p95** | Seconds from PR creation to the bot's first **real** review (refusals excluded, so quota notices don't pollute the percentiles). Targets the "review arrived after auto-approval" failure mode (PR #453). | | **Coverage overlap** | PRs reviewed by ≥2 bots — a redundancy vs specialization signal. | -| **Trend** | Week-over-week Δ (▲/▼) on Reviews and Rate-limited (event counts) vs last week's snapshot. Arrows are directional only. | +| **Trend** | Week-over-week Δ (▲/▼) on Reviews and Refused (event counts) vs last week's snapshot. Arrows are directional only. | Per-PR bucket counts (`reviewed_prs` / `refused_prs` / `no_response_prs`, which partition the eligible PRs), plus thread-resolution and reaction counts, are still computed and stored in the @@ -157,7 +201,8 @@ ORG=petry-projects LOOKBACK_DAYS=7 GH_TOKEN="$(gh auth token)" \ | File | Role | |---|---| -| `scripts/lib/advisory-review-gate.sh` | Single source of truth for bot logins + rate-limit body patterns (reused, not forked). | +| `scripts/lib/reviewer-sources.tsv` + `scripts/lib/reviewer-sources.sh` | The reviewer-source registry ([#1425](https://github.com/petry-projects/.github-private/issues/1425)) + its sourced lookup helpers — the single source of truth for bot logins and the `check_run_name` map, projected by the gate and this report. | +| `scripts/lib/advisory-review-gate.sh` | Advisory approval gate; rate-limit body patterns (reused, not forked). Its bot list is a registry projection. | | `scripts/lib/comment-noise.sh` | Pure no-action agent-comment classifier + `cn_render_noise_section` (the noise metric). Unit-tested in `tests/comment_noise.bats`. | | `scripts/reviewer_report.sh` | Org-wide GraphQL collection (`main` / `collect_org_reviews`) + pure normalization, aggregation, and rendering (`aggregate_snapshot`, `render_reviewer_report`). | | `tests/reviewer_report.bats` | Unit tests for the pure normalize / aggregate / render path (no network). | @@ -173,7 +218,8 @@ ORG=petry-projects LOOKBACK_DAYS=7 GH_TOKEN="$(gh auth token)" \ | `REVIEWER_REPORT_OUT` | Optional path to also write the report to (used by the workflow to post the issue comment). | | `REVIEWER_SNAPSHOT_OUT` | Optional path to write this week's per-bot snapshot (uploaded as the WoW artifact). | | `REVIEWER_PREV_SNAPSHOT` | Optional path to last week's snapshot for deltas; `main()` fetches it automatically in CI. | -| `GH_OP_TIMEOUT` / `COLLECT_CONCURRENCY` / `MAX_PR_PAGES` | Per-call timeout (s), concurrent per-repo sweeps, and PR-page cap per repo. | +| `GH_OP_TIMEOUT` / `COLLECT_CONCURRENCY` / `MAX_PR_PAGES` | Per-call timeout (s), concurrent per-repo sweeps, and PR-page cap per repo (default 50). | +| `PR_PAGE_SIZE` / `PR_FETCH_MAX_ATTEMPTS` | PRs per GraphQL page (default 10 — kept light to stay under the resource limit) and maximum fetch attempts, including the initial request, before the repo is recorded as a `collect_error` (default 4). | ## Known limitations @@ -181,5 +227,18 @@ ORG=petry-projects LOOKBACK_DAYS=7 GH_TOKEN="$(gh auth token)" \ form). If a bot ever posts under a different identity, add it to the shared registry. - **Latency denominator**: PRs opened as drafts and later marked ready use creation time as the latency baseline in v1, which can overstate latency for long-draft PRs. -- **Per-repo pagination**: bounded by `MAX_PR_PAGES` (default 20 × 25 PRs). A repo with - more in-window PRs than that cap would be truncated; the cap is generous for this org. +- **Per-repo pagination**: bounded by `MAX_PR_PAGES` (default 50) × `PR_PAGE_SIZE` (default + 10 PRs). A repo with more in-window PRs than that cap is truncated; when the cap stops a + repo it emits a `{kind:"truncation", connection:"pullRequests"}` record surfaced in the + **Collection health** section, so the scorecard is marked as a floor rather than silently + dropping the overflow. The cap is generous for this org. +- **Nested-connection cap**: a PR's `reviews` / `comments` / `reviewThreads` are fetched up + to 50 each, and each review thread's `comments` up to 20. A PR that exceeds 50 on any of + those connections — or 20 comments on any single thread — emits a `{kind:"truncation"}` + record (the per-thread case as `connection:"reviewThreads.comments"`) rendered in the + **Collection health** section, so an undercount there is stated explicitly rather than + hidden. The scorecard counts every node it did fetch, including bots beyond the 50th review. +- **Check-run summary text**: a completed, non-failed check run is read as *review ran* when + its conclusion is `success` or its summary matches `review ran`. A reporter that changes its + summary wording would need the matcher updated. The reporter set and check-run name live + in `scripts/lib/reviewer-sources.tsv`. diff --git a/scripts/lib/reviewer-sources.sh b/scripts/lib/reviewer-sources.sh index ec831848b..5bb55bbed 100755 --- a/scripts/lib/reviewer-sources.sh +++ b/scripts/lib/reviewer-sources.sh @@ -75,6 +75,19 @@ reviewer_sources_advisory_gate_logins() { awk -F'\t' '!/^[[:space:]]*#/ && $1 != "" && $4 == "yes" { print $1 }' "$REVIEWER_SOURCES_MANIFEST" } +# reviewer_sources_check_run_reporters +# Sources that report a review through a check run (column 5 != "-"), emitted as +# "\t" one per line. A bot listed here posts each review as +# a check run whose completed run counts as a real response even with zero PR +# comments/reviews (the Graphite clean-pass case, #1908). Empty output means no +# registered source reports via check runs. Data-driven: the scorecard reads this +# instead of hard-coding graphite-app. +reviewer_sources_check_run_reporters() { + _reviewer_sources_manifest_or_die reviewer_sources_check_run_reporters || return 1 + awk -F'\t' '!/^[[:space:]]*#/ && $1 != "" && $5 != "" && $5 != "-" { print $1 "\t" $5 }' \ + "$REVIEWER_SOURCES_MANIFEST" +} + # reviewer_sources_trusted_bots_csv # dev-lead's webhook-facing trusted set: each trusted login with a "[bot]" # suffix, joined by commas. This is exactly the TRUSTED_BOTS default. diff --git a/scripts/lib/reviewer-sources.tsv b/scripts/lib/reviewer-sources.tsv index c1fceafe6..d8c220484 100644 --- a/scripts/lib/reviewer-sources.tsv +++ b/scripts/lib/reviewer-sources.tsv @@ -20,7 +20,7 @@ # (creates_threads=no) MAY be advisory-only-and-non-blocking (dev_lead_trusted=no) # with the reason recorded in the rationale column. # -# schema_version: 1 +# schema_version: 2 # # Columns (tab-separated): # login — the bot's GraphQL App login (no "[bot]" suffix). dev-lead's @@ -28,14 +28,21 @@ # creates_threads — yes|no: posts inline review comments (→ blocking review thread). # dev_lead_trusted — yes|no: in dev-lead's trusted set (dev-lead may act on it). # advisory_gate — yes|no: one of the bots the advisory approval gate waits on. +# check_run_name — the exact check-run name this bot reports its review through, +# or "-" when it reports only via PR reviews/comments. A bot with +# a check_run_name posts each review as a check run (e.g. Graphite's +# "Graphite / AI Reviews"); a completed run whose summary says the +# review ran is a real response EVEN WITH ZERO comments and no PR +# review, so the scorecard fetches it separately (#1908). Data, not +# code: the scorecard reads this column instead of hard-coding logins. # rationale — why this bot is classified as it is. # -# login creates_threads dev_lead_trusted advisory_gate rationale -copilot-pull-request-reviewer yes yes yes Inline review comments; dev-lead addresses the findings. -gemini-code-assist yes yes yes Inline review comments; dev-lead addresses the findings. -chatgpt-codex-connector yes yes yes Inline review comments (pull_request_review_comment); dev-lead addresses the findings. -coderabbitai yes yes no Inline review comments; tracked by the scorecard and rate-limit detector but not part of the advisory approval-wait set. -sonarqubecloud no yes yes Top-level quality-gate comment only — no inline review threads; trusted so dev-lead acts on the comment via the issue-comment path. -qodo-code-review yes yes yes Inline review comments (#1355 recognized it for gating/scorecard; trusted for acting per #1425). -codeant-ai yes yes yes Inline review comments (#1355 recognized it for gating/scorecard; trusted for acting per #1425). -graphite-app yes yes yes Inline review comments; root cause of the 2026-08-02 unclearable-thread deadlock — now trusted so dev-lead can act on its findings (#1425). +# login creates_threads dev_lead_trusted advisory_gate check_run_name rationale +copilot-pull-request-reviewer yes yes yes - Inline review comments; dev-lead addresses the findings. +gemini-code-assist yes yes yes - Inline review comments; dev-lead addresses the findings. +chatgpt-codex-connector yes yes yes - Inline review comments (pull_request_review_comment); dev-lead addresses the findings. +coderabbitai yes yes no - Inline review comments; tracked by the scorecard and rate-limit detector but not part of the advisory approval-wait set. +sonarqubecloud no yes yes - Top-level quality-gate comment only — no inline review threads; trusted so dev-lead acts on the comment via the issue-comment path. +qodo-code-review yes yes yes - Inline review comments (#1355 recognized it for gating/scorecard; trusted for acting per #1425). +codeant-ai yes yes yes - Inline review comments (#1355 recognized it for gating/scorecard; trusted for acting per #1425). +graphite-app yes yes yes Graphite / AI Reviews Inline review comments AND clean passes reported ONLY as the "Graphite / AI Reviews" check run (no PR review) — the scorecard must read that check run or every clean pass is miscounted as No response (#1908). Root cause of the 2026-08-02 unclearable-thread deadlock — trusted so dev-lead can act on its findings (#1425). diff --git a/scripts/reviewer_report.sh b/scripts/reviewer_report.sh index 8b49cf0ef..503478ef5 100755 --- a/scripts/reviewer_report.sh +++ b/scripts/reviewer_report.sh @@ -115,6 +115,20 @@ else RATE_LIMIT_RE='usage limit|rate[-_ ]?limit|too many requests|quota (exceeded|reached|exhausted)|out of (quota|credits|tokens|requests)|limit (reached|exceeded|exhausted)|(reached|exceeded|hit) (the |your )?(usage |rate |daily |monthly )?limit|used up its prepaid credits|Qodo.{0,40}(monthly|usage|PR|review) limit|CodeAnt.{0,40}(monthly|trial|usage) limit' fi +# Check-run reporters (#1908): logins that deliver a review as a check run rather +# than (or in addition to) a PR review — Graphite's clean pass posts ONLY a +# "Graphite / AI Reviews" check run and no PR review, so it must be fetched +# separately or every clean pass miscounts as "No response". Data-driven from +# reviewer-sources.tsv (check_run_name column), never a hard-coded login here. +# Map: login → exact check-run name. Empty when the registry is absent. +declare -gA REVIEWER_CHECK_RUN_NAMES=() +if declare -F reviewer_sources_check_run_reporters >/dev/null 2>&1; then + while IFS=$'\t' read -r _crr_login _crr_name || [ -n "$_crr_login" ]; do + [ -n "$_crr_login" ] && REVIEWER_CHECK_RUN_NAMES["$_crr_login"]="$_crr_name" + done < <(reviewer_sources_check_run_reporters 2>/dev/null || true) + unset _crr_login _crr_name +fi + # --------------------------------------------------------------------------- # Pure rendering / aggregation helpers (unit-tested, no network) # --------------------------------------------------------------------------- @@ -196,6 +210,38 @@ _NORMALIZE_JQ=' | { bot: (.author.login|ascii_downcase), at: .createdAt, kind: "issue", state: "COMMENTED", body: (.bodyText // ""), reactions_up: 0, reactions_down: 0, resolved: false, outdated: false } ] + # Check-run reviews (#1908): a bot that reports through a check run (Graphite) + # posts each review as a `` check run, not a PR review — a clean pass leaves + # NO PR review, only a completed check run. The collector attaches the matched + # check runs under ._checkRuns. Classify each: + # * completed AND (success OR summary says the review ran) → a REAL response, + # even with zero comments (refusal:false). It adds 0 to the review-event count + # (kind:"check_run", not "review"); latency is measured to completed_at. + # * completed/skipped WITH a decline summary ("too large" / "did not run") → + # a REFUSAL (refusal:true) — the bot itself declined. A run skipped for an + # unrelated workflow reason (empty/neutral summary, e.g. an `if:`-gated job) + # is NOT the bot declining, so it must NOT be counted as a rate-limited + # refusal — it contributes nothing, just like a queued run. + # * anything else (queued / in_progress / no run) contributes nothing → the bot + # gets no submission here, so absent any PR review it buckets as no-response. + + [ (.["_checkRuns"] // [])[] + | select(.bot as $a | is_bot($a)) + | (.status == "completed") as $done + | ((.conclusion // "") | ascii_downcase) as $concl + | (["failure","cancelled","timed_out","action_required","stale","startup_failure"] | index($concl)) as $failed + # The "review ran" summary substring only counts when the run did NOT fail — a + # failure/cancelled run whose summary happens to say "…review ran into an error…" + # is not a real review (#1913 review). A success conclusion always counts. + | ($done and ($failed == null) + and ((.conclusion // "") == "success" + or ((.summary // "") | test("review ran"; "i")))) as $ran + | ($done and ((.conclusion // "") == "skipped") + and ((.summary // "") | test("did not run|too large|declin|skip|rate.?limit|quota|usage limit"; "i"))) as $refused + | select($ran or $refused) + | { bot: (.bot|ascii_downcase), at: (.completed_at // .completedAt), + kind: "check_run", state: (if $ran then "COMMENTED" else "SKIPPED" end), + body: (.summary // ""), refusal: $refused, + reactions_up: 0, reactions_down: 0, resolved: false, outdated: false } ] ) as $subs | ( { kind: "pr", repo: $repo, pr: ($pr.url), created: $pr.createdAt, merged: ($pr.mergedAt // null), @@ -206,7 +252,11 @@ _NORMALIZE_JQ=' # even alongside a rate-limit notice; it counts as "refused" only when the # bot'"'"'s SOLE action on the PR was to decline. This is the key correctness # fix over the old "any rate-limit text present" flag. - | ($grp | map(. + {refusal: ((.body // "") | test($rl; "i"))})) as $mine + # A check-run submission carries an explicit `refusal` boolean (a "too large" + # skip); every other submission derives refusal from its body matching the + # rate-limit/out-of-quota pattern. Preserve a pre-set flag, else body-match. + | ($grp | map(. + {refusal: (if (.refusal != null) then .refusal + else ((.body // "") | test($rl; "i")) end)})) as $mine | ($mine | map(select(.refusal | not))) as $real | ($mine | map(select(.refusal))) as $refd | ($real | map(.at) | min) as $first_real @@ -296,6 +346,57 @@ _prev() { jq -r --arg b "$bot" --arg k "$field" '.bots?[$b]?[$k]? // empty' "$f" 2>/dev/null || printf '' } +# _render_collection_health — PURE. Prints a PROMINENT +# section for any repo the collector could not fetch (#1908) and for any per-PR +# connection that hit the GraphQL fetch cap. Prints NOTHING when collection was +# clean. This is the in-report surfacing the issue requires: a silent stderr WARN +# is not enough, because a dropped repo makes every count below a floor, not a +# total, and that must be visible to whoever reads the scorecard. +_render_collection_health() { + local dir="$1" repo_count="$2" + local files=("$dir"/*.jsonl) + [ -e "${files[0]}" ] || return 0 + + local errs + errs="$(jq -rs --argjson m "${repo_count:-0}" ' + ([ .[] | select(.kind=="collect_error") ] | unique_by(.repo)) as $e + | if ($e | length) == 0 then empty + else "## ⚠️ Collection health\n\n" + + "**\($e | length) of \($m) repos could not be collected** after retries — " + + "their PRs are absent or incomplete in every metric below, so all counts are a **floor, not a total**. " + + "This is the #1908 failure mode (silent page-1 GraphQL failure); investigate before trusting the numbers or the week-over-week deltas.\n\n" + + ([ $e[] | "- `\(.repo)` — \(.reason // "collection failed")" ] | join("\n")) + + "\n" + end + ' "${files[@]}" 2>/dev/null || true)" + [ -n "$errs" ] && printf '%s\n' "$errs" + + local trunc + trunc="$(jq -rs ' + ([ .[] | select(.kind=="truncation") ] | unique_by([.repo, .pr, .connection])) as $t + | if ($t | length) == 0 then empty + else "### Truncated connections\n\n" + + "These per-PR connections hit the GraphQL fetch cap, so the affected reviewer counts may **undercount** on these PRs (truncation is reported here, never silent — #1908):\n\n" + + ([ $t[] | "- `\(.repo)` PR `\(.pr)` — `\(.connection)` truncated" ] | join("\n")) + + "\n" + end + ' "${files[@]}" 2>/dev/null || true)" + [ -n "$trunc" ] && printf '%s\n' "$trunc" + + local crerr + crerr="$(jq -rs ' + ([ .[] | select(.kind=="check_run_error") ] | unique_by([.repo, .pr])) as $c + | if ($c | length) == 0 then empty + else "### Check-run fetch failures\n\n" + + "A check-run REST fetch failed on these PRs, so a check-run reporter clean pass may be missed and miscounted as No response (surfaced here, never silent — #1908):\n\n" + + ([ $c[] | "- `\(.repo)` PR `\(.pr)` — check-run fetch failed" ] | join("\n")) + + "\n" + end + ' "${files[@]}" 2>/dev/null || true)" + [ -n "$crerr" ] && printf '%s\n' "$crerr" + return 0 +} + # render_reviewer_report [generated_at] # Writes the full Markdown report to stdout. PURE: no network. Optional prior # snapshot for WoW deltas via REVIEWER_PREV_SNAPSHOT. @@ -317,6 +418,10 @@ render_reviewer_report() { "$ORG" "$repo_count" "$(_fmt_int "$total")" "$(_fmt_int "$eligible")" fi + # Surface collection failures / truncation FIRST and unconditionally, so a run + # that dropped repos never presents its partial counts as complete (#1908). + _render_collection_health "$dir" "$repo_count" + if [ "${total:-0}" -eq 0 ]; then printf 'No pull-request activity found in the last %s days.\n' "$lookback" if declare -F cn_render_noise_section >/dev/null 2>&1; then @@ -328,14 +433,14 @@ render_reviewer_report() { printf 'Deterministic report — every figure is computed with `jq`/`awk` from GitHub review data; ' printf 'no LLM is involved. Bots are identified by their GraphQL App login ' printf '(`scripts/lib/advisory-review-gate.sh`); rate-limit/out-of-quota notices are detected by body text. ' - printf '**Reviews** and **Rate-limited** count *every event*, not distinct PRs — a PR re-reviewed across N commits contributes N reviews. ' + printf '**Reviews** and **Refused** count *every event*, not distinct PRs — a PR re-reviewed across N commits contributes N reviews. ' printf '**No response** counts eligible PRs the reviewer never engaged with at all. ' printf '_Latency_ = time from PR creation to the bot'"'"'s first **real** review (refusals excluded). ' printf 'Deltas (▲/▼) are vs the prior week and directional only.\n\n' # ---- Scorecard table ----------------------------------------------------- printf '## Scorecard\n\n' - printf '| Reviewer | Total PRs | Reviews | ✅ / 🔄 | Rate-limited | No response | Latency p50 | Latency p95 |\n' + printf '| Reviewer | Total PRs | Reviews | ✅ / 🔄 | Refused | No response | Latency p50 | Latency p95 |\n' printf '|---|---:|---:|:--:|---:|---:|---:|---:|\n' local bot label reviews refusal_events no_resp approved changes_req p50 p95 @@ -371,7 +476,7 @@ render_reviewer_report() { printf -- '- **Total PRs** — review-eligible (non-draft) PRs in the window; the denominator each row is measured against.\n' printf -- '- **Reviews** — count of reviews the bot submitted, **each occurrence** (a PR re-reviewed on 5 commits = 5). A bot that posts no formal review but delivers its verdict as a comment (e.g. SonarCloud'"'"'s quality-gate comment) has that comment counted as its review. Rate-limit notices are never counted.\n' printf -- '- **✅ / 🔄** — of those reviews, how many were APPROVED / CHANGES_REQUESTED (comment-only responses carry no verdict).\n' - printf -- '- **Rate-limited** — count of out-of-quota / rate-limit refusals, each occurrence.\n' + printf -- '- **Refused** — count of refusal events: out-of-quota / rate-limit notices AND a check-run reporter'"'"'s own decline (a "too large" / "did not run" skip), each occurrence.\n' printf -- '- **No response** — eligible PRs the bot never engaged with at all (no review, comment, or refusal).\n\n' # ---- Coverage / overlap -------------------------------------------------- @@ -419,7 +524,17 @@ render_reviewer_report() { GH_OP_TIMEOUT="${GH_OP_TIMEOUT:-60}" COLLECT_CONCURRENCY="${COLLECT_CONCURRENCY:-8}" -MAX_PR_PAGES="${MAX_PR_PAGES:-20}" +# PR_PAGE_SIZE (#1908): PRs requested per GraphQL page. Kept deliberately SMALL +# (10, was 25) because the verified undercount root cause was busy repos exceeding +# GitHub's GraphQL resource limit on page 1 and being silently dropped entirely. +# A lighter page is the fix; do NOT raise this or the nested connections back up. +PR_PAGE_SIZE="${PR_PAGE_SIZE:-10}" +# On a resource-limit/timeout error the fetch retries with a HALVED page size and +# backoff up to this many attempts before the repo is recorded as un-collected. +PR_FETCH_MAX_ATTEMPTS="${PR_FETCH_MAX_ATTEMPTS:-4}" +# Safety cap on pages per repo. Raised (was 20) to keep total coverage steady now +# that each page holds fewer PRs; the UPDATED_AT early-stop ends most repos sooner. +MAX_PR_PAGES="${MAX_PR_PAGES:-50}" # _gh_timeout — run `gh` under a per-call timeout so one hung request # cannot stall the run. gh is invoked via `bash -c` so an exported shell-function @@ -433,13 +548,15 @@ _gh_timeout() { } # GraphQL query: one repo's PRs (newest first) with reviews, threads, comments. +# $prFirst is the per-page PR count (small, and halved on retry, see PR_PAGE_SIZE). +# headRefOid feeds the separate per-PR check-run fetch (#1908) for check-run reporters. _PR_QUERY=' -query($owner:String!, $name:String!, $cursor:String) { +query($owner:String!, $name:String!, $cursor:String, $prFirst:Int!) { repository(owner:$owner, name:$name) { - pullRequests(first: 25, after: $cursor, orderBy: {field: UPDATED_AT, direction: DESC}) { + pullRequests(first: $prFirst, after: $cursor, orderBy: {field: UPDATED_AT, direction: DESC}) { pageInfo { hasNextPage endCursor } nodes { - url number createdAt updatedAt mergedAt isDraft + url number createdAt updatedAt mergedAt isDraft headRefOid author { login } reviews(first: 50) { nodes { author { login } state submittedAt bodyText } } reviewThreads(first: 50) { @@ -459,10 +576,70 @@ query($owner:String!, $name:String!, $cursor:String) { } }' +# _collect_check_runs_for_page [out_jsonl] — echo a +# JSON object mapping each in-window PR url → [ {bot,status,conclusion,summary,completed_at} ] +# for the CHECK-RUN reporters (#1908). One lightweight REST call per in-window PR +# head commit (`/commits/{sha}/check-runs`), filtered to the configured names — a +# separate, cheap fetch that does NOT add weight to the (resource-limited) PR page +# query. Echoes `{}` when there are no reporters or no in-window PRs. Best-effort: +# a failed per-PR fetch is skipped, never fatal — but when is given a +# {kind:"check_run_error"} diagnostic is appended so the miss is surfaced in +# Collection health, never silent (#1908): a dropped fetch could turn a check-run +# reporter's clean pass into a false "No response". +_collect_check_runs_for_page() { + local owner="$1" name="$2" resp="$3" out="${4:-}" + local reporters_json="${REVIEWER_CHECK_RUN_JSON:-"{}"}" + [ "$reporters_json" = "{}" ] && { printf '{}'; return 0; } + + local prs + prs="$(jq -c --arg cutoff "$CUTOFF" ' + [ .data?.repository?.pullRequests?.nodes[]? + | select(.updatedAt >= $cutoff) + | {url, sha: (.headRefOid // "")} | select(.sha != "") ]' <<<"$resp" 2>/dev/null || printf '[]')" + { [ -z "$prs" ] || [ "$prs" = "[]" ]; } && { printf '{}'; return 0; } + + # Stream the (url, sha) pairs as TSV from a SINGLE jq, then loop — no per-row jq + # to read fields. @tsv also escapes embedded newlines, hardening against a + # crafted PR URL splitting a row. `|| [ -n "$url" ]` processes a final row with + # no trailing newline. + local acc='{}' url sha + while IFS=$'\t' read -r url sha || [ -n "$url" ]; do + [ -z "$url" ] && continue + local cr_resp matched + # `--paginate` follows Link headers so a reporter's run past the first 100 check + # runs is still seen — reading only page 1 would falsely report "No response" + # when the configured run sorts after the 100th (#1908). Assign first, then test, + # so a fetch failure under `set -e` is a clean skip, not a swallowed error. + if ! cr_resp="$(_gh_timeout api --paginate "repos/${owner}/${name}/commits/${sha}/check-runs?per_page=100" 2>/dev/null)" \ + || [ -z "$cr_resp" ]; then + # Fetch failed: without a record this PR's check-run reporters are silently absent, + # so a Graphite clean pass would miscount as "No response". Surface it (#1908). + [ -n "$out" ] && jq -cn --arg repo "${owner}/${name}" --arg pr "$url" \ + '{kind:"check_run_error", repo:$repo, pr:$pr, reason:"check-run REST fetch failed"}' \ + >> "$out" 2>/dev/null || true + continue + fi + # `-s` slurps the (possibly multi-page) `--paginate` object stream into an array; + # `.[].check_runs[]?` flattens runs across every page. + matched="$(jq -c -s --argjson names "$reporters_json" ' + ($names | to_entries | map({(.value): .key}) | add) as $byname + | [ .[].check_runs[]? + | select(($byname[.name] // null) != null) + | { bot: $byname[.name], status: .status, conclusion: .conclusion, + summary: (.output.summary // ""), completed_at: .completed_at } ]' \ + <<<"$cr_resp" 2>/dev/null || printf '[]')" + { [ -z "$matched" ] || [ "$matched" = "[]" ]; } && continue + acc="$(jq -c --arg url "$url" --argjson m "$matched" '. + {($url): $m}' <<<"$acc" 2>/dev/null || printf '%s' "$acc")" + done < <(jq -r '.[] | [.url, .sha] | @tsv' <<<"$prs" 2>/dev/null || true) + printf '%s' "$acc" +} + # _collect_one_repo — GraphQL-paginate one repo's recently-updated PRs, # stopping once a page's PRs predate CUTOFF (PRs are ordered by UPDATED_AT desc), # and append normalized JSONL records to a unique file in COLLECT_JSONL_DIR. -# Always exits 0: a failed repo degrades to a WARN + skip, never an aborted run. +# Always exits 0: a repo that fails even after retries degrades to a {kind: +# "collect_error"} record (surfaced prominently in the report, #1908), never an +# aborted run. _collect_one_repo() { local repo="$1" owner name cursor="" page=0 out owner="${repo%%/*}"; name="${repo##*/}" @@ -473,13 +650,45 @@ _collect_one_repo() { while :; do page=$((page + 1)) - [ "$page" -gt "${MAX_PR_PAGES}" ] && break + if [ "$page" -gt "${MAX_PR_PAGES}" ]; then + # The MAX_PR_PAGES cap stopped this repo before its in-window PRs were exhausted + # (the bottom-of-loop stop did not fire), so later PRs are dropped. Emit a + # truncation record so Collection health marks the scorecard a floor, never silent (#1908). + jq -cn --arg repo "$repo" --arg cap "$MAX_PR_PAGES" \ + '{kind:"truncation", repo:$repo, pr:"(pagination)", connection:"pullRequests", reason:"MAX_PR_PAGES (\($cap)) reached"}' \ + >> "$out" 2>/dev/null || true + break + fi - local resp gql_args - gql_args=(api graphql -f query="$_PR_QUERY" -F owner="$owner" -F name="$name") - [ -n "$cursor" ] && gql_args+=(-F cursor="$cursor") - if ! resp="$(_gh_timeout "${gql_args[@]}" 2>/dev/null)"; then - echo "WARN: GraphQL query for ${repo} (page ${page}) failed or timed out — partial data" >&2 + # Fetch this page, retrying with a HALVED page size + backoff on a resource-limit + # / timeout error before giving up (#1908). A GraphQL resource-limit error can + # arrive as a non-zero exit OR as an HTTP-200 body carrying an `errors` array — + # possibly ALONGSIDE partial `data`. Treat ANY of these as a failure to retry: a + # page is "fetched" only when it has non-null pullRequests AND no `errors`, so a + # partial page is never counted as complete. Assign the command substitution + # first, then test, so a non-zero exit under `set -e` is not swallowed. + local resp="" this_first="${PR_PAGE_SIZE}" attempt=0 fetched=0 + while :; do + attempt=$((attempt + 1)) + local gql_args + gql_args=(api graphql -f query="$_PR_QUERY" -F owner="$owner" -F name="$name" -F prFirst="$this_first") + [ -n "$cursor" ] && gql_args+=(-F cursor="$cursor") + resp="$(_gh_timeout "${gql_args[@]}" 2>/dev/null || true)" + if [ -n "$resp" ] \ + && jq -e '((.errors // []) | length) == 0 and (.data?.repository?.pullRequests != null)' <<<"$resp" >/dev/null 2>&1; then + fetched=1; break + fi + if [ "$attempt" -ge "${PR_FETCH_MAX_ATTEMPTS}" ] || [ "$this_first" -le 1 ]; then + break + fi + this_first=$((this_first / 2)); [ "$this_first" -lt 1 ] && this_first=1 + sleep "$(( attempt < 5 ? attempt : 5 ))" + done + if [ "$fetched" -ne 1 ]; then + echo "WARN: GraphQL query for ${repo} (page ${page}) failed after ${attempt} attempts — repo NOT collected (#1908)" >&2 + jq -cn --arg repo "$repo" \ + --arg reason "page-${page} GraphQL failure after ${attempt} attempts (resource-limit/timeout) — halved to first:${this_first}" \ + '{kind:"collect_error", repo:$repo, reason:$reason}' >> "$out" 2>/dev/null || true break fi @@ -490,10 +699,33 @@ _collect_one_repo() { [ .data?.repository?.pullRequests?.nodes[]? | select(.updatedAt >= $cutoff) ] | length ' <<<"$resp" 2>/dev/null || echo 0)" - jq -c --arg repo "$repo" --argjson bots "$bots_json" --arg rl "$RATE_LIMIT_RE" --arg cutoff "$CUTOFF" \ - ".data?.repository?.pullRequests?.nodes[]? | select(.updatedAt >= \$cutoff) | ${_NORMALIZE_JQ}" \ + # Check-run reviews (#1908): fetch each in-window PR's head-commit check runs for + # the configured reporters and inject them as ._checkRuns so normalization can + # count a clean pass (check run only, no PR review) as a real response. + local crmap='{}' + if [ "${REVIEWER_CHECK_RUN_JSON:-"{}"}" != "{}" ]; then + crmap="$(_collect_check_runs_for_page "$owner" "$name" "$resp" "$out")" || crmap='{}' + fi + + jq -c --arg repo "$repo" --argjson bots "$bots_json" --arg rl "$RATE_LIMIT_RE" --arg cutoff "$CUTOFF" --argjson crmap "$crmap" \ + ".data?.repository?.pullRequests?.nodes[]? | select(.updatedAt >= \$cutoff) | (. + {\"_checkRuns\": (\$crmap[.url] // [])}) | ${_NORMALIZE_JQ}" \ <<<"$resp" >> "$out" 2>/dev/null || true + # Truncation surfacing (#1908): a nested connection that hit its GraphQL fetch + # cap (≥50) may undercount a bot's reviews/comments on that PR. Emit one + # {kind:"truncation"} record per capped connection so the report states it + # explicitly — truncation must NEVER be silent (the old code only WARNed to + # stderr, and only when the noise pass was enabled). + jq -c --arg repo "$repo" --arg cutoff "$CUTOFF" ' + .data?.repository?.pullRequests?.nodes[]? | select(.updatedAt >= $cutoff) | . as $pr + | ( if ((.reviews?.nodes // []) | length) >= 50 then {kind:"truncation",repo:$repo,pr:($pr.url),connection:"reviews"} else empty end), + ( if ((.comments?.nodes // []) | length) >= 50 then {kind:"truncation",repo:$repo,pr:($pr.url),connection:"comments"} else empty end), + ( if ((.reviewThreads?.nodes // []) | length) >= 50 then {kind:"truncation",repo:$repo,pr:($pr.url),connection:"reviewThreads"} else empty end), + # Each reviewThread'"'"'s comments are fetched only up to 20 (comments(first: 20)); a + # thread that exceeds that undercounts inline comments / thread metrics, so surface it too. + ( if any((.reviewThreads?.nodes // [])[]?; ((.comments?.nodes // []) | length) >= 20) then {kind:"truncation",repo:$repo,pr:($pr.url),connection:"reviewThreads.comments"} else empty end) + ' <<<"$resp" >> "$out" 2>/dev/null || true + # Additive no-action-noise pass (#1411): emit one agent_comment record per # marker-bearing comment/review on each in-window PR, regardless of author # (our agents commit as human logins). A distinct record kind, so it never @@ -558,9 +790,19 @@ collect_org_reviews() { CUTOFF="$(date -u -d "${LOOKBACK_DAYS} days ago" +%Y-%m-%dT%H:%M:%SZ 2>/dev/null \ || date -u -v-"${LOOKBACK_DAYS}"d +%Y-%m-%dT%H:%M:%SZ)" - export CUTOFF COLLECT_JSONL_DIR="$jsonl_dir" GH_OP_TIMEOUT MAX_PR_PAGES + export CUTOFF COLLECT_JSONL_DIR="$jsonl_dir" GH_OP_TIMEOUT MAX_PR_PAGES PR_PAGE_SIZE PR_FETCH_MAX_ATTEMPTS # shellcheck disable=SC2090 # jq/GraphQL program strings: literal content is intended export _PR_QUERY _NORMALIZE_JQ RATE_LIMIT_RE + # Check-run reporters (#1908) — serialize the login→check-run-name map to JSON so + # each worker subshell (associative arrays cannot cross the process boundary) can + # fetch check runs for the configured reporters. `{}` when none are registered. + local _crk + REVIEWER_CHECK_RUN_JSON='{}' + for _crk in "${!REVIEWER_CHECK_RUN_NAMES[@]}"; do + REVIEWER_CHECK_RUN_JSON="$(jq -c --arg k "$_crk" --arg v "${REVIEWER_CHECK_RUN_NAMES[$_crk]}" \ + '. + {($k): $v}' <<<"$REVIEWER_CHECK_RUN_JSON")" + done + export REVIEWER_CHECK_RUN_JSON # No-action noise classifier (#1411) — exported so each worker subshell can run # the second (agent_comment) collection pass. Absent in a stripped lib env. export CN_AGENT_COMMENT_JQ="${CN_AGENT_COMMENT_JQ:-}" CN_MARKER_RE="${CN_MARKER_RE:-}" CN_NOACTION_RE="${CN_NOACTION_RE:-}" @@ -572,7 +814,7 @@ collect_org_reviews() { # REVIEWER_BOTS is a plain array; export as newline string the workers re-split. REVIEWER_BOTS_STR="$(printf '%s\n' "${REVIEWER_BOTS[@]}")" export REVIEWER_BOTS_STR - export -f _collect_one_repo _gh_timeout + export -f _collect_one_repo _collect_check_runs_for_page _gh_timeout printf '%s\n' "$repos_raw" \ | xargs -P "$COLLECT_CONCURRENCY" -I {} \ diff --git a/tests/reviewer_report.bats b/tests/reviewer_report.bats index 8549a8d38..25f6ff940 100644 --- a/tests/reviewer_report.bats +++ b/tests/reviewer_report.bats @@ -158,6 +158,166 @@ JSON echo "$output" | jq -e '.[] | select(.bot=="coderabbitai") | .reviews==1' } +# --------------------------------------------------------------------------- +# Check-run reviews (issue #1908) — Graphite reports clean passes as a check run +# --------------------------------------------------------------------------- + +@test "check-run: a clean pass (check run only, no PR review) counts as reviewed" { + local gbots='["graphite-app","copilot-pull-request-reviewer"]' + tmp="$(mktemp "$BATS_TEST_TMPDIR/tmp.XXXXXX")" + cat > "$tmp" <<'JSON' +{"url":"u","createdAt":"2026-09-23T10:00:00Z","updatedAt":"2026-09-23T10:00:00Z","mergedAt":null,"isDraft":false,"author":{"login":"h"}, + "reviews":{"nodes":[]}, + "reviewThreads":{"nodes":[]}, + "comments":{"nodes":[]}, + "_checkRuns":[{"bot":"graphite-app","status":"completed","conclusion":"success","summary":"AI review ran and left 0 comments","completed_at":"2026-09-23T10:05:00Z"}]} +JSON + run jq -c --arg repo "r" --argjson bots "$gbots" --arg rl "$RATE_LIMIT_RE" "[ $_NORMALIZE_JQ ]" "$tmp" + # Reviewed: real_responses>=1, zero refusals, and (per AC1) 0 review events. + echo "$output" | jq -e '.[] | select(.bot=="graphite-app") | .real_responses>=1 and .refusals==0 and .reviews==0' + # Latency measured to the check run's completed_at (10:05:00 − 10:00:00 = 300s). + echo "$output" | jq -e '.[] | select(.bot=="graphite-app") | .latency_s==300' +} + +@test "check-run: a 'too large' skip counts as a refusal, not a review" { + local gbots='["graphite-app"]' + tmp="$(mktemp "$BATS_TEST_TMPDIR/tmp.XXXXXX")" + cat > "$tmp" <<'JSON' +{"url":"u","createdAt":"2026-09-23T10:00:00Z","updatedAt":"2026-09-23T10:00:00Z","mergedAt":null,"isDraft":false,"author":{"login":"h"}, + "reviews":{"nodes":[]}, + "reviewThreads":{"nodes":[]}, + "comments":{"nodes":[]}, + "_checkRuns":[{"bot":"graphite-app","status":"completed","conclusion":"skipped","summary":"AI code review did not run because this PR is too large.","completed_at":"2026-09-23T10:04:00Z"}]} +JSON + run jq -c --arg repo "r" --argjson bots "$gbots" --arg rl "$RATE_LIMIT_RE" "[ $_NORMALIZE_JQ ]" "$tmp" + echo "$output" | jq -e '.[] | select(.bot=="graphite-app") | .real_responses==0 and .refusals>=1 and .reviews==0' +} + +@test "check-run: a FAILED run whose summary says 'review ran' is NOT a real review (#1913)" { + # Regression: the "review ran" summary substring must be gated on a non-failure + # conclusion, else "AI review ran into an error and did not complete" on a `failure` + # run would be misread as a real response (real_responses:1). + local gbots='["graphite-app"]' + tmp="$(mktemp "$BATS_TEST_TMPDIR/tmp.XXXXXX")" + cat > "$tmp" <<'JSON' +{"url":"u","createdAt":"2026-09-23T10:00:00Z","updatedAt":"2026-09-23T10:00:00Z","mergedAt":null,"isDraft":false,"author":{"login":"h"}, + "reviews":{"nodes":[]}, + "reviewThreads":{"nodes":[]}, + "comments":{"nodes":[]}, + "_checkRuns":[{"bot":"graphite-app","status":"completed","conclusion":"failure","summary":"AI review ran into an error and did not complete.","completed_at":"2026-09-23T10:04:00Z"}]} +JSON + run jq -c --arg repo "r" --argjson bots "$gbots" --arg rl "$RATE_LIMIT_RE" "[ $_NORMALIZE_JQ ]" "$tmp" + # A failed run is neither a real response nor a refusal → no bot_pr record at all. + echo "$output" | jq -e 'map(select(.kind=="bot_pr" and .bot=="graphite-app")) | length == 0' +} + +@test "check-run: a skip with an unrelated/empty summary is NOT a refusal" { + # A run skipped for an unrelated workflow reason (no decline summary) must not be + # counted as a rate-limited refusal — it contributes nothing, like a queued run. + local gbots='["graphite-app"]' + tmp="$(mktemp "$BATS_TEST_TMPDIR/tmp.XXXXXX")" + cat > "$tmp" <<'JSON' +{"url":"u","createdAt":"2026-09-23T10:00:00Z","updatedAt":"2026-09-23T10:00:00Z","mergedAt":null,"isDraft":false,"author":{"login":"h"}, + "reviews":{"nodes":[]}, + "reviewThreads":{"nodes":[]}, + "comments":{"nodes":[]}, + "_checkRuns":[{"bot":"graphite-app","status":"completed","conclusion":"skipped","summary":"","completed_at":"2026-09-23T10:04:00Z"}]} +JSON + run jq -c --arg repo "r" --argjson bots "$gbots" --arg rl "$RATE_LIMIT_RE" "[ $_NORMALIZE_JQ ]" "$tmp" + # No bot_pr record → aggregator buckets it as no-response, not a refusal. + echo "$output" | jq -e 'map(select(.kind=="bot_pr" and .bot=="graphite-app")) | length == 0' +} + +@test "check-run: a queued suite (no completed check run, no review) is no response" { + local gbots='["graphite-app"]' + tmp="$(mktemp "$BATS_TEST_TMPDIR/tmp.XXXXXX")" + cat > "$tmp" <<'JSON' +{"url":"u","createdAt":"2026-09-23T10:00:00Z","updatedAt":"2026-09-23T10:00:00Z","mergedAt":null,"isDraft":false,"author":{"login":"h"}, + "reviews":{"nodes":[]}, + "reviewThreads":{"nodes":[]}, + "comments":{"nodes":[]}, + "_checkRuns":[{"bot":"graphite-app","status":"queued","conclusion":null,"summary":null,"completed_at":null}]} +JSON + run jq -c --arg repo "r" --argjson bots "$gbots" --arg rl "$RATE_LIMIT_RE" "[ $_NORMALIZE_JQ ]" "$tmp" + # No bot_pr record at all → the aggregator buckets it as no-response. + echo "$output" | jq -e 'map(select(.kind=="bot_pr" and .bot=="graphite-app")) | length == 0' +} + +@test "check-run: an inline review AND a clean check run — review events unaffected" { + local gbots='["graphite-app"]' + tmp="$(mktemp "$BATS_TEST_TMPDIR/tmp.XXXXXX")" + cat > "$tmp" <<'JSON' +{"url":"u","createdAt":"2026-09-23T10:00:00Z","updatedAt":"2026-09-23T10:00:00Z","mergedAt":null,"isDraft":false,"author":{"login":"h"}, + "reviews":{"nodes":[{"author":{"login":"graphite-app"},"state":"COMMENTED","submittedAt":"2026-09-23T10:06:00Z","bodyText":"one finding"}]}, + "reviewThreads":{"nodes":[]}, + "comments":{"nodes":[]}, + "_checkRuns":[{"bot":"graphite-app","status":"completed","conclusion":"success","summary":"AI review ran and left 1 comment","completed_at":"2026-09-23T10:05:00Z"}]} +JSON + run jq -c --arg repo "r" --argjson bots "$gbots" --arg rl "$RATE_LIMIT_RE" "[ $_NORMALIZE_JQ ]" "$tmp" + # The real PR review is the 1 review event; the check run adds 0 more. + echo "$output" | jq -e '.[] | select(.bot=="graphite-app") | .reviews==1' +} + +@test "check-run: collector returns the matched runs when REVIEWER_CHECK_RUN_JSON is set (#1913)" { + # Regression: "${REVIEWER_CHECK_RUN_JSON:-{}}" ends the expansion at the FIRST + # brace and appends a literal "}", so a SET map became invalid JSON, the jq + # --argjson failed, and every PR's check runs were silently dropped. The + # normalizer tests above inject _checkRuns directly and never exercised this. + export CUTOFF="2026-09-16T00:00:00Z" + export REVIEWER_CHECK_RUN_JSON='{"graphite-app":"Graphite / AI Reviews"}' + _gh_timeout() { + printf '%s' '{"total_count":2,"check_runs":[{"name":"CI","status":"completed","conclusion":"success","output":{"summary":"ok"},"completed_at":"2026-09-23T10:01:00Z"},{"name":"Graphite / AI Reviews","status":"completed","conclusion":"success","output":{"summary":"AI review ran and left 0 comments"},"completed_at":"2026-09-23T10:05:00Z"}]}' + } + local resp='{"data":{"repository":{"pullRequests":{"nodes":[{"url":"https://github.com/o/r/pull/1","updatedAt":"2026-09-23T10:00:00Z","headRefOid":"abc123"}]}}}}' + run _collect_check_runs_for_page o r "$resp" + [ "$status" -eq 0 ] + echo "$output" | jq -e '.["https://github.com/o/r/pull/1"] | length == 1' + echo "$output" | jq -e '.["https://github.com/o/r/pull/1"][0] | .bot == "graphite-app" and .conclusion == "success" and .completed_at == "2026-09-23T10:05:00Z"' +} + +@test "check-run: collector short-circuits to {} when REVIEWER_CHECK_RUN_JSON is unset (#1913)" { + unset REVIEWER_CHECK_RUN_JSON + _gh_timeout() { : > "$BATS_TEST_TMPDIR/gh_called"; return 1; } + local resp='{"data":{"repository":{"pullRequests":{"nodes":[{"url":"u","updatedAt":"2099-01-01T00:00:00Z","headRefOid":"abc"}]}}}}' + run _collect_check_runs_for_page o r "$resp" + [ "$status" -eq 0 ] + [ "$output" = "{}" ] + [ ! -e "$BATS_TEST_TMPDIR/gh_called" ] +} + +@test "check-run: collector records check_run_error when the REST fetch errors non-zero (#1913)" { + # An HTTP error (403 rate limit, 5xx) makes gh write an error body to stdout AND + # exit non-zero. Relying on empty output alone would miss it (cr_resp holds the + # non-empty error object), silently dropping the PR — the #1908 silent failure. + export CUTOFF="2026-09-16T00:00:00Z" + export REVIEWER_CHECK_RUN_JSON='{"graphite-app":"Graphite / AI Reviews"}' + _gh_timeout() { printf '%s' '{"message":"API rate limit exceeded"}'; return 1; } + local resp='{"data":{"repository":{"pullRequests":{"nodes":[{"url":"https://github.com/o/r/pull/1","updatedAt":"2026-09-23T10:00:00Z","headRefOid":"abc123"}]}}}}' + local out; out="$(mktemp "$BATS_TEST_TMPDIR/out.XXXXXX")" + run _collect_check_runs_for_page o r "$resp" "$out" + [ "$status" -eq 0 ] + [ "$output" = "{}" ] + jq -e 'select(.kind=="check_run_error" and .pr=="https://github.com/o/r/pull/1")' "$out" +} + +@test "reviews: a PR with more than 50 reviews still counts a bot whose review is past the 50th" { + # The GraphQL 50-cap is a COLLECTION concern; normalization must count whatever + # nodes it is handed. Feed 51 reviews with the tracked bot last (issue #1908 AC5). + local latebots='["graphite-app"]' + tmp="$(mktemp "$BATS_TEST_TMPDIR/tmp.XXXXXX")" + { + printf '{"url":"u","createdAt":"2026-09-23T10:00:00Z","updatedAt":"2026-09-23T10:00:00Z","mergedAt":null,"isDraft":false,"author":{"login":"h"},' + printf '"reviews":{"nodes":[' + for i in $(seq 1 50); do + printf '{"author":{"login":"human%d"},"state":"COMMENTED","submittedAt":"2026-09-23T10:0%d:00Z","bodyText":"x"},' "$i" $((i % 9)) + done + printf '{"author":{"login":"graphite-app"},"state":"CHANGES_REQUESTED","submittedAt":"2026-09-23T10:59:00Z","bodyText":"51st review"}' + printf ']},"reviewThreads":{"nodes":[]},"comments":{"nodes":[]}}' + } > "$tmp" + run jq -c --arg repo "r" --argjson bots "$latebots" --arg rl "$RATE_LIMIT_RE" "[ $_NORMALIZE_JQ ]" "$tmp" + echo "$output" | jq -e '.[] | select(.bot=="graphite-app") | .reviews==1 and .changes_req==1' +} + # --------------------------------------------------------------------------- # aggregate_snapshot # --------------------------------------------------------------------------- @@ -212,6 +372,16 @@ JSON echo "$output" | jq -e '.bots["gemini-code-assist"].latency_p50 == 50' } +@test "aggregate: a check-run clean pass (0 review events) still fills the reviewed bucket" { + tmp="$(mktemp -d "$BATS_TEST_TMPDIR/cr.XXXXXX")" + cat > "$tmp/r.jsonl" <<'JSON' +{"kind":"pr","repo":"o/r","pr":"o/r/1","created":"2026-09-23T10:00:00Z","merged":null,"draft":false,"author":"h"} +{"kind":"bot_pr","repo":"o/r","pr":"o/r/1","bot":"graphite-app","created":"2026-09-23T10:00:00Z","real_responses":1,"refusals":0,"latency_s":300,"reviews":0,"approved":0,"changes_req":0,"inline_comments":0,"threads_total":0,"threads_resolved":0,"thumbs_up":0,"thumbs_down":0} +JSON + run aggregate_snapshot "$tmp" + echo "$output" | jq -e '.bots["graphite-app"] | .reviewed_prs==1 and .reviews==0 and .no_response_prs==0' +} + @test "aggregate: empty dir yields a zeroed snapshot" { empty="$(mktemp -d "$BATS_TEST_TMPDIR/empty.XXXXXX")" run aggregate_snapshot "$empty" @@ -250,7 +420,7 @@ JSON @test "render: scorecard exposes the event-count columns" { run render_reviewer_report "$FIXTURES" 7 12 2026-07-13 - echo "$output" | grep -q "| Reviewer | Total PRs | Reviews | ✅ / 🔄 | Rate-limited | No response |" + echo "$output" | grep -q "| Reviewer | Total PRs | Reviews | ✅ / 🔄 | Refused | No response |" } @test "render: empty dir yields a no-data message" { @@ -277,6 +447,38 @@ JSON [ "$status" -eq 0 ] } +@test "render: a repo that could not be collected is surfaced prominently (#1908)" { + dir="$(mktemp -d "$BATS_TEST_TMPDIR/cerr.XXXXXX")" + cat > "$dir/r.jsonl" <<'JSON' +{"kind":"pr","repo":"o/r","pr":"o/r/1","created":"2026-09-23T10:00:00Z","merged":null,"draft":false,"author":"h"} +{"kind":"collect_error","repo":"petry-projects/markets","reason":"page-1 resource-limit after retries"} +JSON + run render_reviewer_report "$dir" 7 3 2026-09-23 + [ "$status" -eq 0 ] + # Prominent, in-report (not a stderr WARN): names the count and the repo. + echo "$output" | grep -q "could not be collected" + echo "$output" | grep -q "petry-projects/markets" +} + +@test "render: a truncated connection is reported explicitly, never silently (#1908)" { + dir="$(mktemp -d "$BATS_TEST_TMPDIR/trunc.XXXXXX")" + cat > "$dir/r.jsonl" <<'JSON' +{"kind":"pr","repo":"o/r","pr":"o/r/1","created":"2026-09-23T10:00:00Z","merged":null,"draft":false,"author":"h"} +{"kind":"truncation","repo":"o/r","pr":"o/r/1","connection":"reviews"} +JSON + run render_reviewer_report "$dir" 7 1 2026-09-23 + [ "$status" -eq 0 ] + echo "$output" | grep -qi "truncat" + echo "$output" | grep -q "o/r/1" +} + +@test "render: no collection-failure section when every repo was collected" { + run render_reviewer_report "$FIXTURES" 7 12 2026-07-13 + [ "$status" -eq 0 ] + echo "$output" | grep -qv "could not be collected" || true + ! echo "$output" | grep -q "could not be collected" +} + @test "render: agent-comment noise section is wired into the report (#1411)" { dir="$(mktemp -d "$BATS_TEST_TMPDIR/noise.XXXXXX")" cat > "$dir/r.jsonl" <<'JSON' diff --git a/tests/test_reviewer_sources.bats b/tests/test_reviewer_sources.bats index 7e5b3fe44..8afc3161c 100644 --- a/tests/test_reviewer_sources.bats +++ b/tests/test_reviewer_sources.bats @@ -105,6 +105,41 @@ TSV [[ "$csv" == *"chatgpt-codex-connector[bot]"* ]] } +# ── Check-run reporters (issue #1908) ──────────────────────────────────────── + +@test "check-run: graphite-app reports through the 'Graphite / AI Reviews' check run" { + local reporters + reporters="$(reviewer_sources_check_run_reporters)" + [[ "$reporters" == *"graphite-app Graphite / AI Reviews"* ]] +} + +@test "check-run: a review-only bot is NOT listed as a check-run reporter" { + local reporters + reporters="$(reviewer_sources_check_run_reporters)" + # These seven post PR reviews / comments, not check runs — column 5 is "-", so none + # of them may appear as a check-run reporter (adding a check_run_name to any, or + # regressing the awk filter, must fail this test rather than pass silently). + [[ "$reporters" != *"copilot-pull-request-reviewer"* ]] + [[ "$reporters" != *"gemini-code-assist"* ]] + [[ "$reporters" != *"coderabbitai"* ]] + [[ "$reporters" != *"chatgpt-codex-connector"* ]] + [[ "$reporters" != *"sonarqubecloud"* ]] + [[ "$reporters" != *"qodo-code-review"* ]] + [[ "$reporters" != *"codeant-ai"* ]] +} + +@test "check-run: reporters helper propagates a missing-manifest failure" { + REVIEWER_SOURCES_MANIFEST="/nonexistent/reviewer-sources.tsv" \ + run reviewer_sources_check_run_reporters + # Assert the specific expected exit code (1, from _reviewer_sources_manifest_or_die) + # so an unexpected failure (syntax / command-not-found) can't pass this test. + [ "$status" -eq 1 ] +} + +@test "check-run: manifest schema_version is 2 (check_run_name column added)" { + [ "$(reviewer_sources_version)" = "2" ] +} + # ── Consistency: the three consumer lists are registry projections ─────────── @test "consistency: advisory gate ADVISORY_BOTS == registry advisory-gate projection" {