diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 68e6caf..34fe875 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -50,7 +50,9 @@ PR gets the `cursor-review` label ``` Slack start/complete DMs to the triggerer are sent alongside (optional — -skipped if no Slack token is configured). +skipped if no Slack token is configured). A skip for the diff-size cap is +announced on the PR rather than passing for a clean review — see [Over the +diff-size cap](#over-the-diff-size-cap). ### The panel @@ -196,6 +198,28 @@ All optional except `workflows_ref` (required, no default) — pass them under There is **no `blocking` input** — see [the regression note above](#the-blocking-gate-is-currently-not-available-regressed). +### Over the diff-size cap + +A PR whose counted diff exceeds `diff_size_cap` gets **no review panel**, and +that skip is not a failure — the run is green either way. So it announces itself +rather than passing for a clean review: the *Diff size check* job emits a +`::warning::` annotation and a step-summary block naming the counted total and +the cap (both credential-free, so they reach Dependabot PRs, whose runs can't +read Actions secrets), and a separate `over-cap-comment` job upserts one sticky +PR comment saying no panel ran. Get the PR under the cap and re-trigger, and +that same comment flips to ✅ instead of stacking a second one; it never posts on +a PR that was under the cap all along. Neither half reaches a **fork** PR — the +gate skips a cross-repo head before the size check runs at all, so a fork PR is +skipped for being a fork, not for its size. + +The comment posts as the bot app when one is configured (`bot_app_id` + +`BOT_APP_PRIVATE_KEY`) and as `github-actions[bot]` otherwise, so it works in the +default configuration; the sticky finder matches both logins, so switching +identities updates the existing comment rather than posting a second one. If the +API write fails the job degrades to the annotation + summary and logs why. Mint +and upsert are both `continue-on-error`: the size verdict lives in the +`diff-size` job, so the comment path can never redden a run. + ### Escape hatches - **Skip a PR**: add the `skip-cursor-review` label. It wins even if the trigger diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 7dfc932..bca0111 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -63,7 +63,12 @@ on: Max counted changed lines. PRs over the cap are skipped — too large for a useful single-pass review. "Counted" is added + removed lines after the shared classifier's generated-file exclusion and (when - ignore_comments is true) blank/comment-line discounting. + ignore_comments is true) blank/comment-line discounting. The skip is + NOT silent: it emits a ::warning:: annotation and a step-summary block, + and upserts one sticky PR comment saying no panel ran — as the bot app + when bot_app_id + BOT_APP_PRIVATE_KEY are configured, else as + github-actions[bot] — flipped to ✅ once the PR is back under the cap. + The run stays green either way; nothing gates. type: number required: false default: 5000 @@ -452,6 +457,17 @@ jobs: # small PR but can never feed an unbounded diff to the panel. outputs: within_cap: ${{ steps.check.outputs.over_cap != 'true' }} + # The counted total behind that verdict, so the over-cap comment can name + # the number the author has to get under. Written by the tool on the + # normal path and by the raw-count fallback below on the degraded one, so + # it always agrees with whichever count actually decided `over_cap`. + counted: ${{ steps.check.outputs.counted }} + # Whether that count came from the raw-numstat fallback (classifier tool + # failed) rather than the classifier. The over-cap comment needs it to + # describe the number honestly: on the degraded path NOTHING was excluded, + # so calling it "after generated-file exclusion" would send an author + # hunting for a lockfile that was, in fact, counted. + degraded: ${{ steps.check.outputs.degraded }} steps: - name: Checkout PR head uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -586,11 +602,42 @@ jobs: git diff --numstat "${BASE_SHA}...${HEAD_SHA}" > "${RUNNER_TEMP}/raw-numstat" RAW=$(awk '{ if ($1 != "-") total += $1; if ($2 != "-") total += $2 } END { print total + 0 }' "${RUNNER_TEMP}/raw-numstat") echo "Raw changed lines (no exclusions): ${RAW} (cap: ${DIFF_SIZE_CAP})" + # Same last-value-wins append as over_cap below: whichever count + # decided the gate is the one the skip notice must quote. Without + # this the tool's pre-crash `counted` (or nothing at all) would be + # reported next to a verdict the RAW count actually made. + echo "counted=${RAW}" >> "$GITHUB_OUTPUT" if [ "${RAW}" -gt "${DIFF_SIZE_CAP}" ]; then echo "over_cap=true" >> "$GITHUB_OUTPUT" fi fi + - name: Announce over-cap skip + # The over-cap path used to run NOTHING: every downstream job is gated + # on within_cap, the tool runs in --mode warn (never non-zero), so the + # whole run went green with no review and no signal — indistinguishable + # from "the panel reviewed this and found nothing" (BE-7646). This + # annotation + summary is the credential-free half of the signal, so it + # reaches Dependabot PRs (whose runs read the Dependabot secret store, + # not Actions secrets) and any consumer whose comment write fails. NOT + # fork PRs: `gate` sets should_run=false for a cross-repo head before + # this job runs at all, so an over-cap fork PR reaches neither half of + # the signal — it is skipped for being a fork first. + if: steps.check.outputs.over_cap == 'true' + env: + COUNTED: ${{ steps.check.outputs.counted }} + run: | + # `counted` is written on both paths above, but a tool that died + # before writing outputs AND left the raw count within the cap can + # still leave it empty while over_cap came from elsewhere — say + # "unknown" rather than render an empty number as fact. + COUNTED_TEXT="${COUNTED:-unknown}" + echo "::warning::Cursor review SKIPPED — counted ${COUNTED_TEXT} changed lines exceeds diff_size_cap=${DIFF_SIZE_CAP}. This PR gets no review panel. Split the PR or reduce the counted diff." + { + echo "## ⚠️ Cursor review skipped (over cap)" + echo "Counted ${COUNTED_TEXT} changed lines > cap ${DIFF_SIZE_CAP} — no panel ran." + } >> "$GITHUB_STEP_SUMMARY" + - name: Build reviewed diff (degraded fallback) # Only when the classifier tool failed but the raw count is within the # cap: no generated-file exclusions are available, so build the diff @@ -646,6 +693,288 @@ jobs: if-no-files-found: error retention-days: 7 + # Sticky PR comment for the over-cap skip. Modelled on pr-size.yml's `comment` + # job, same best-effort contract: the verdict lives in `diff-size` (its + # ::warning:: annotation and step summary), and this path may never redden the + # run — hence continue-on-error on both the mint and the upsert. + # + # Non-blocking on purpose. cursor-review is advisory — it posts a review, it + # does not gate (no cursor-review job is a required status check on the + # largest consumer's default branch, checked live via the rulesets API), so + # failing the run over an over-cap PR would gate nothing while breaking the + # reusable's contract for every caller. The fix here is VISIBILITY, not + # enforcement. + # + # Isolation: this job checks out NO code at all — it only reads job outputs + # and calls the comments API — so its write-scoped token is never present in + # a job that held a PR checkout. + over-cap-comment: + name: Comment when over the diff-size cap + needs: [gate, diff-size] + # Gating on diff-size's RESULT, not on within_cap, is load-bearing twice + # over. A git-level diff-size failure leaves every output empty, and an + # empty within_cap satisfies `!= 'true'` — which would post a confident + # "over cap" comment about a job that never counted anything. And the job + # must ALSO run on within-cap runs, because that is the only way a stale + # over-cap comment ever gets flipped to ✅ once the PR is trimmed. + # + # `!cancelled()` rather than `always()`: the documented caller sets + # cancel-in-progress, so a superseded run reaching here would PATCH its + # obsolete verdict over the newer run's — and this comment's whole content + # is "did a review run", the one thing that must not go stale. The upsert + # additionally re-checks the live PR head, which covers the other half + # (someone manually re-running an old, uncancelled run). + if: ${{ !cancelled() && needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.result == 'success' }} + runs-on: ubuntu-latest + # Bounded like every other job here. The work is two API calls, but the list + # paginates over an attacker-growable comment list, so the 6-hour default + # would let a rate-limited or hung call hold a runner all day. + timeout-minutes: 5 + permissions: + contents: read + # For the GITHUB_TOKEN fallback below. Not a new demand on callers: + # `consolidate` already requires this exact scope to post the review, so + # every caller that can run this workflow at all already grants it. + pull-requests: write + env: + # The `secrets` context is unavailable in `if` conditions, so the + # credentials-present test is evaluated here (job env allows it) and the + # mint below gates on the result. Absent creds are NOT the end of the + # road any more — the upsert falls back to the run token. + BOT_CONFIGURED: ${{ inputs.bot_app_id != '' && secrets.BOT_APP_PRIVATE_KEY != '' }} + steps: + - name: Mint bot token + id: bot + if: env.BOT_CONFIGURED == 'true' + # Best-effort: a consumer whose bot app lacks the requested scope (or + # has creds misconfigured) falls back to the run token below, and never + # gets a red check from the comment path. + continue-on-error: true + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ inputs.bot_app_id }} + private-key: ${{ secrets.BOT_APP_PRIVATE_KEY }} + # Exactly what the upsert uses. The sticky comment rides the + # issues-comments API on a PULL REQUEST, and for GitHub Apps that + # endpoint checks the app's Pull requests permission (not Issues) — so + # pull-requests:write is the one scope required (read is implied for + # the list GET). Requesting an ungranted extra (e.g. issues:write) + # would 422 the mint on installations that never granted it. + permission-pull-requests: write + + - name: Upsert sticky over-cap comment + id: upsert + # Same best-effort contract as the mint: a runtime permission failure + # (e.g. a 403 because the bot app lacks Pull requests on this + # installation) must not fail the job — "Note degraded mode" reports it. + continue-on-error: true + env: + # Dedicated bot identity when configured, else github-actions[bot] — + # the same fallback `consolidate` uses to post the review itself. + # Without it the sticky half of this fix would never fire for the + # (likely majority) of callers that configure no bot app, leaving them + # with exactly the silent green run BE-7646 is about. + GH_TOKEN: ${{ steps.bot.outputs.token || secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + # The minted token's OWN bot login. app-slug comes from the mint, so + # it identifies this installation whether bot_app_id was given as a + # numeric App ID or a Client ID (`Iv1.…`). Empty when no app is + # configured or the mint failed; RUN_LOGIN then carries the identity. + BOT_LOGIN: ${{ steps.bot.outputs.app-slug && format('{0}[bot]', steps.bot.outputs.app-slug) || '' }} + # The fallback identity. BOTH logins are matched when finding the + # sticky comment, because the identity can change between runs (a bot + # app added later, or a mint that fails once) and a finder that knew + # only the current one would post a second comment beside the old one + # and then maintain two contradictory verdicts. Still safe as an + # author filter: a PR author cannot post as either login. + RUN_LOGIN: github-actions[bot] + WITHIN_CAP: ${{ needs.diff-size.outputs.within_cap }} + COUNTED: ${{ needs.diff-size.outputs.counted }} + DEGRADED: ${{ needs.diff-size.outputs.degraded }} + run: | + set -euo pipefail + # Don't let an obsolete run overwrite a newer verdict. `!cancelled()` + # covers the cancel-in-progress path; this covers a manual re-run of + # an older run, whose event payload still carries the head SHA that + # was current when it was queued. + # + # FAILS CLOSED. A lookup that errors leaves us unable to tell a + # current run from an obsolete one, and writing anyway would let a + # stale "no panel ran" (or a premature ✅) land on top of the right + # answer — the one failure this guard exists to prevent. Skipping + # costs little: the list and write below hit the same API with the + # same token, so a real outage would take them out a line later + # regardless. `exit 1` under continue-on-error routes it to "Note + # degraded mode", which reports it against the annotation + summary + # that remain the primary signal. + if ! LIVE_HEAD="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha')" || [ -z "$LIVE_HEAD" ]; then + echo "Could not read the PR's current head (see gh error above) — refusing to write a verdict that may already be stale." >&2 + exit 1 + fi + if [ "$LIVE_HEAD" != "$EVENT_HEAD_SHA" ]; then + echo "This run's head (${EVENT_HEAD_SHA}) is no longer the PR head (${LIVE_HEAD}) — a newer run owns the verdict. Not writing." + exit 0 + fi + # Distinct from the panel's CONSOLIDATED_MARKER (gate-unresolved.py): + # that one matches PR *reviews*, this one an issue comment, and + # nothing in cursor-review reads issues/*/comments at all — so the two + # cannot collide. + MARKER='' + COUNTED_TEXT="${COUNTED:-an unknown number of}" + # On the degraded path `counted` is the RAW numstat total — the + # classifier never ran, so no generated file was excluded from it. + # Describing it as "after generated-file exclusion" there would tell + # an author whose lockfile tipped the PR over the cap to go shrink a + # number the stated rule says already excludes their lockfile. + if [ "${DEGRADED:-}" = "true" ]; then + BASIS='raw changed lines; the generated-file classifier failed on this run, so nothing was excluded from the count' + else + BASIS='changed lines, after generated-file exclusion' + fi + if [ "$WITHIN_CAP" = "true" ]; then + { + printf '%s\n\n' "$MARKER" + # Deliberately claims only what THIS job knows. It needs just + # `gate` + `diff-size`, so it runs alongside the panel and cannot + # see whether the panel finished, failed preflight, or was + # cancelled — asserting "the panel ran" here would re-create the + # false green this whole change exists to kill. + # shellcheck disable=SC2016 + printf '✅ **Now under the cap** — this PR no longer exceeds `diff_size_cap`, so the review is no longer skipped for size. Check this run for the panel'"'"'s result.\n' + } > body.md + else + { + printf '%s\n\n' "$MARKER" + # The backticks are a markdown code span in the comment body, not + # a command substitution — single quotes are what keeps them so. + # shellcheck disable=SC2016 + printf '⚠️ **Cursor review skipped** — the counted diff (%s %s) exceeds `diff_size_cap` (%s). **No review panel ran on this PR.**\n\n' \ + "$COUNTED_TEXT" "$BASIS" "$DIFF_SIZE_CAP" + # NOT "re-runs automatically on the next push": the documented + # caller subscribes to `types: [labeled, unlabeled]` only, so a + # push fires nothing and the gate only sets should_run on a + # `labeled` event (or in run_without_label mode). Telling the + # author to push would leave them waiting on a run that never + # starts, under a comment that never flips. + # shellcheck disable=SC2016 + printf 'Reduce the counted diff or split the PR, then re-apply the `%s` label to trigger a fresh review. (If this repo runs the review on every push, a push is enough.)\n' \ + "$REVIEW_LABEL" + } > body.md + fi + # Find our existing sticky comment(s) by the hidden marker, so we + # update across pushes instead of stacking new ones. Both filters are + # load-bearing, and this mirrors pr-size.yml's `comment` job (and + # `find_sticky` in scripts/pr-risk/publish-risk-surfaces.sh) + # deliberately — same hazards, so the same shape rather than a third + # dialect of it. + # + # .user.login is one of OUR two possible identities (the bot app, if + # configured, and the run token's github-actions[bot]) — the + # marker is published in this PUBLIC workflow file, so without an + # author filter a PR author could pre-seed a comment carrying it + # and we would PATCH THEIRS instead of posting ours, after which + # they rewrite the "no review ran" notice freely. Matching the + # LOGIN (not merely `.user.type == "Bot"`) also keeps us off any + # OTHER app's comment that happens to echo author-controlled text + # as its first line. Neither login is postable by a human. + # + # startswith, not contains — the body renders the marker as its + # FIRST line, while another bot QUOTING this comment carries the + # marker nested in its own prose. + # + # Both values are passed as jq DATA (--arg), never spliced into the + # program text, so nothing can turn the filter into a syntax error — + # which under `set -euo pipefail` plus continue-on-error would become + # a silent "no comment posted". + existing="$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ + | jq -r --arg m "$MARKER" --arg login "$BOT_LOGIN" --arg run_login "$RUN_LOGIN" ' + # $login is "" when no app token was minted; the != "" guard + # keeps that empty string from matching a login-less comment. + .[] | select(($login != "" and (.user.login // "") == $login) + or (.user.login // "") == $run_login) + | select((.user.type // "") == "Bot") + | select(((.body // "") | startswith($m))) + | .id')" + if [ -n "$existing" ]; then + # Update EVERY match, not just the first: the find-then-post is + # check-then-act with no lock, so two runs that both list before + # either posts leave two comments — reachable for a caller that adds + # `synchronize` to its `types:` (or runs in run_without_label mode), + # where a push can race a label toggle into a different concurrency + # group. Updating every copy keeps the duplicates in AGREEMENT + # instead of split-brained. Taking `head -n1` would pin + # one forever while the other displayed a stale verdict — and here a + # stale verdict says a review did or did not run, which is the whole + # point of the comment. Unquoted on purpose: the ids are numeric and + # newline-separated, so word splitting is the right parse. + # + # Per-id failure is TOLERATED rather than fatal — under + # `set -euo pipefail` a bare call would abort the loop on the first + # error and leave the remaining copies stale, exactly the split + # brain this loop exists to prevent. A 404 is expected: a maintainer + # (or another run) deleting one between the list and the write. + patched=0 + failed=0 + for id in $existing; do + if gh api -X PATCH "repos/${REPO}/issues/comments/${id}" -F body=@body.md >/dev/null; then + echo "Updated sticky over-cap comment ${id}." + patched=$((patched + 1)) + else + failed=$((failed + 1)) + # gh's stderr is deliberately NOT discarded: a 403 from + # permission drift, a 422 on an over-long body and a concurrent + # delete all land here. + echo "Failed to update sticky over-cap comment ${id} (see gh error above); continuing." >&2 + fi + done + # Fail the step only when NOTHING was patched — a partial failure is + # the case this loop exists for, and reporting it as a step failure + # would make the degraded-mode note below claim no comment was + # posted, which is affirmatively false. + if [ "$patched" -eq 0 ] && [ "$failed" -gt 0 ]; then + echo "No sticky comment could be updated (${failed} failed); the PR may show a stale verdict." >&2 + exit 1 + elif [ "$failed" -gt 0 ]; then + echo "Updated ${patched} sticky comment(s); ${failed} could not be updated (see errors above)." + fi + elif [ "$WITHIN_CAP" != "true" ]; then + gh api -X POST "repos/${REPO}/issues/${PR_NUMBER}/comments" -F body=@body.md >/dev/null + echo "Posted over-cap skip comment." + else + # Never POST on the happy path — a PR that was always under the cap + # gets a review, not a comment about not getting one. + echo "Within the cap and no existing comment — nothing to post." + fi + + - name: Note degraded mode + # The upsert no longer depends on the bot app — it falls back to the run + # token — so the only way no comment is written is the upsert itself + # failing. Reported here so the job never goes green silently; the + # ::warning:: annotation and step summary from the diff-size job remain + # the signal in that case. A failed MINT alone is not degraded mode (the + # run token covered it), but it is worth naming, since it changes which + # identity authored the comment. + if: steps.upsert.outcome == 'failure' || steps.bot.outcome == 'failure' + env: + BOT_OUTCOME: ${{ steps.bot.outcome }} + UPSERT_OUTCOME: ${{ steps.upsert.outcome }} + WITHIN_CAP: ${{ needs.diff-size.outputs.within_cap }} + run: | + if [ "$BOT_OUTCOME" = "failure" ]; then + echo "Bot app token could not be minted for this repo — the run token (github-actions[bot]) was used instead." + fi + if [ "$UPSERT_OUTCOME" != "failure" ]; then + exit 0 + fi + echo "Sticky comment NOT written: the upsert failed at runtime — see the 'Upsert sticky over-cap comment' step log above for the actual error (a network/5xx failure, a GitHub rate limit, or a 403 from a token without Pull requests: write on this repo)." + if [ "$WITHIN_CAP" != "true" ]; then + echo "This PR is OVER diff_size_cap and got no review panel — the ::warning:: annotation and step summary on the 'Diff size check' job are the signal." + else + echo "This PR is within diff_size_cap, so there is no skip to announce; nothing was owed on this run." + fi + preflight: # Validate the panel's pinned model ids against the LIVE Cursor catalog # once, before the 8-cell fan-out. When Cursor delists a pinned id the diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index aa897bd..9842cd3 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -96,7 +96,7 @@ pull-requests: write # posting the consolidated review | Input | Default | Notes | |---|---|---| | `judge_model` | `claude-opus-4-8-thinking-max` | Consolidates the panel into one review. | -| `diff_size_cap` | `5000` | Skip review above this diff size. | +| `diff_size_cap` | `5000` | Skip review above this diff size. An over-cap PR is not silent — see the gotcha below. | | `review_label` | `cursor-review` | The label that triggers a run. | | `diff_excludes` | lockfiles, `node_modules`, `.claude`, `dist`, `vendor`, `*.generated.*`, `*.min.js` | Paths kept out of **both** the size-budget count and the reviewed diff. Passing your own value **replaces** the default list, so re-state the entries you still want. | | `workflows_ref` | `main` | **Set to your `uses:` SHA** — prompts load from this ref at run time. | @@ -126,6 +126,8 @@ above it does **not** by itself start a run — `types: [labeled, unlabeled]` om the label. Add `synchronize` to `types:` if you want every push re-reviewed (and see the spend warning below). +**An over-cap PR gets no review, and now says so.** When the counted diff exceeds `diff_size_cap` the panel is skipped and the run still goes green — nothing about it is a failure. So the skip announces itself in three places instead: a `::warning::` annotation and a step-summary block on the *Diff size check* job (both credential-free, so they still show on Dependabot PRs, whose runs can't read Actions secrets), plus a sticky PR comment naming the counted total and the cap. Get the PR under the cap and **re-apply the label** — with the label-gated caller above a push alone starts no run — and that comment flips to ✅. The comment posts as your bot app when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are set and as `github-actions[bot]` otherwise, so it works out of the box; if the write fails it degrades to the annotation and the summary and the job log says why. The comment path is best-effort throughout — it never reddens the run. Note that **fork PRs get neither half**: the gate skips a cross-repo head before the size check runs, so a fork PR is skipped for being a fork, whatever its size. + **Dependabot PRs are not covered by the fork skip.** Dependabot's branches live in the base repo, so the gate's cross-repo check treats them as ordinary PRs — but Dependabot-triggered runs read the *Dependabot* secret store, not Actions secrets.