From de929546f6541353f85d859a10486d30d5b6511c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 14 Aug 2026 14:59:57 -0700 Subject: [PATCH 1/3] feat(cursor-review): announce the over-cap skip instead of a silent green run (BE-7646) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A labeled PR whose counted diff exceeds diff_size_cap got no review and no PR-visible signal: every downstream job is gated on within_cap, the shared classifier runs in --mode warn (never non-zero), and nothing at all ran on the over-cap path — so the run went green and the author reasonably concluded the panel had reviewed the PR and found nothing. Three additions, all non-blocking (cursor-review is advisory; failing the job would gate nothing and would break the reusable's contract for its consumers): - diff-size exposes the counted total as a job output, and the degraded raw-count fallback appends counted=RAW so the number always agrees with whichever count actually decided over_cap. - A credential-free ::warning:: annotation + step-summary block on the over-cap path, so the skip is visible on fork and Dependabot PRs and on consumers with no bot app. - A new over-cap-comment job, modelled on pr-size.yml's comment job, that upserts one sticky PR comment naming the counted total and the cap — and flips it to a green note once the PR is back under the cap. Mint and upsert are both continue-on-error, so the comment path can never redden a run. --- .github/cursor-review/README.md | 22 ++- .github/workflows/cursor-review.yml | 236 +++++++++++++++++++++++++++- docs/callers/cursor-review.md | 4 +- 3 files changed, 259 insertions(+), 3 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 68e6caf7..c1403719 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,24 @@ 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 fork and Dependabot PRs too), and a +separate `over-cap-comment` job upserts one sticky PR comment saying no panel +ran. Get the PR under the cap and the next run flips that same comment to ✅ +instead of stacking a second one; it never posts on a PR that was under the cap +all along. + +The comment needs the bot app (`bot_app_id` + `BOT_APP_PRIVATE_KEY`). Without it +— or if the token mint or the API write fails — the job degrades silently to the +annotation + summary and logs which case applied. 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 7dfc9321..30d3e5c2 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -63,7 +63,11 @@ 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 (when bot_app_id + BOT_APP_PRIVATE_KEY are configured) upserts one + sticky PR comment saying no panel ran — 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 +456,11 @@ 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 }} steps: - name: Checkout PR head uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -586,11 +595,39 @@ 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 fork and Dependabot PRs and consumers with no bot app, where + # the sticky comment below cannot post. + 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 +683,203 @@ 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 two job + # outputs and calls the comments API — so the bot's 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. + if: always() && needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.result == 'success' + runs-on: ubuntu-latest + permissions: + contents: read + env: + # The `secrets` context is unavailable in `if` conditions, so the + # credentials-present test is evaluated here (job env allows it) and the + # steps below gate on the result. With no bot credentials they no-op and + # the annotation + step summary from `diff-size` stay the only signal. + 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) must degrade to the annotation + summary, + # never get 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 + if: steps.bot.outcome == 'success' + # 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: + GH_TOKEN: ${{ steps.bot.outputs.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + # 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.…`). + BOT_LOGIN: ${{ steps.bot.outputs.app-slug }}[bot] + WITHIN_CAP: ${{ needs.diff-size.outputs.within_cap }} + COUNTED: ${{ needs.diff-size.outputs.counted }} + run: | + set -euo pipefail + # 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}" + if [ "$WITHIN_CAP" = "true" ]; then + { + printf '%s\n\n' "$MARKER" + printf '✅ **Now under the cap** — the review panel ran on the latest push.\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 changed lines, after generated-file exclusion) exceeds `diff_size_cap` (%s). **No review panel ran on this PR.**\n\n' \ + "$COUNTED_TEXT" "$DIFF_SIZE_CAP" + printf 'Reduce the counted diff or split the PR; the review re-runs automatically on the next push.\n' + } > 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 == our own 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. + # + # 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" ' + .[] | select((.user.login // "") == $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, and the documented caller subscribes + # to labeled/unlabeled alongside synchronize, so a push racing a + # label toggle can leave two comments. 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 + # Covers every path where no comment is posted, so the job never goes + # green silently: "creds absent" (mint skipped), "mint failed" (mint + # errored), "post failed" (upsert ran but failed at runtime). The + # ::warning:: annotation and step summary from the diff-size job remain + # the signal in every one of those cases. + if: steps.bot.outcome != 'success' || steps.upsert.outcome == 'failure' + env: + BOT_OUTCOME: ${{ steps.bot.outcome }} + UPSERT_OUTCOME: ${{ steps.upsert.outcome }} + WITHIN_CAP: ${{ needs.diff-size.outputs.within_cap }} + run: | + if [ "$UPSERT_OUTCOME" = "failure" ]; then + 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 permission 403 on an installation that has not granted Pull requests: write)." + elif [ "$BOT_OUTCOME" = "failure" ]; then + echo "Sticky comment NOT written: the bot app token could not be minted for this repo." + else + echo "Sticky comment NOT written: no bot credentials configured (bot_app_id and/or BOT_APP_PRIVATE_KEY are unset)." + fi + 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 aa897bd0..0f3a9695 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 show on fork and Dependabot PRs too), plus a sticky PR comment naming the counted total and the cap. Push the PR under the cap and that comment flips to ✅ on the next run, when the panel actually runs. The comment needs `bot_app_id` + `BOT_APP_PRIVATE_KEY`; without them it degrades silently to the annotation and the summary, and the job log says which of "creds absent / mint failed / post failed" applied. The comment path is best-effort throughout — it never reddens the run. + **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. From bf779f5cc5541def48982b805a2f4417f81ee007 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 14 Aug 2026 15:42:26 -0700 Subject: [PATCH 2/3] fix(cursor-review): address panel findings on the over-cap notice (BE-7646) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-panel follow-ups on the over-cap skip announcement: - The ✅ flip no longer claims "the review panel ran". The job needs only `gate` + `diff-size`, so it runs alongside the panel and cannot know whether the panel finished, failed preflight or was cancelled — asserting it did would re-create the false green this change exists to kill. It now states only what it knows: the PR is under the cap. - The skip notice no longer describes the count as "after generated-file exclusion" on the degraded path, where the classifier never ran and nothing was excluded. `diff-size` exposes a new `degraded` output and the body words the number from it. - "re-runs automatically on the next push" was false for the documented caller (`types: [labeled, unlabeled]` — a push fires no event). It now tells the author to re-apply the trigger label. - `always()` → `!cancelled()`, plus a live-head check in the upsert, so a superseded or manually re-run older run cannot PATCH its obsolete verdict over a newer one. - The sticky comment now falls back to `secrets.GITHUB_TOKEN` when no bot app is configured — the same fallback `consolidate` already uses to post the review — so the notice reaches the default configuration instead of only bot-app callers. The finder matches both logins, so a change of identity updates the existing comment rather than stacking a second one. - Added `timeout-minutes: 5`; the job paginated an attacker-growable comment list under the 6-hour default. - Corrected the fork claim in all three places: `gate` skips a cross-repo head before `diff-size` runs, so neither half of the signal reaches a fork PR. Only the Dependabot half of the claim was true. --- .github/cursor-review/README.md | 26 +++-- .github/workflows/cursor-review.yml | 172 +++++++++++++++++++++------- docs/callers/cursor-review.md | 2 +- 3 files changed, 144 insertions(+), 56 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index c1403719..34fe8758 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -204,17 +204,21 @@ 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 fork and Dependabot PRs too), and a -separate `over-cap-comment` job upserts one sticky PR comment saying no panel -ran. Get the PR under the cap and the next run flips that same comment to ✅ -instead of stacking a second one; it never posts on a PR that was under the cap -all along. - -The comment needs the bot app (`bot_app_id` + `BOT_APP_PRIVATE_KEY`). Without it -— or if the token mint or the API write fails — the job degrades silently to the -annotation + summary and logs which case applied. 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. +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 diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 30d3e5c2..5f9e4271 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -65,9 +65,10 @@ on: after the shared classifier's generated-file exclusion and (when ignore_comments is true) blank/comment-line discounting. The skip is NOT silent: it emits a ::warning:: annotation and a step-summary block, - and (when bot_app_id + BOT_APP_PRIVATE_KEY are configured) upserts one - sticky PR comment saying no panel ran — flipped to ✅ once the PR is - back under the cap. The run stays green either way; nothing gates. + 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 @@ -461,6 +462,12 @@ jobs: # 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 @@ -611,8 +618,11 @@ jobs: # 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 fork and Dependabot PRs and consumers with no bot app, where - # the sticky comment below cannot post. + # 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 }} @@ -695,9 +705,9 @@ jobs: # 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 two job - # outputs and calls the comments API — so the bot's write-scoped token is - # never present in a job that held a PR checkout. + # 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] @@ -707,23 +717,38 @@ jobs: # "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. - if: always() && needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.result == 'success' + # + # `!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 - # steps below gate on the result. With no bot credentials they no-op and - # the annotation + step summary from `diff-size` stay the only signal. + # 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) must degrade to the annotation + summary, - # never get a red check from the comment path. + # 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: @@ -739,33 +764,74 @@ jobs: - name: Upsert sticky over-cap comment id: upsert - if: steps.bot.outcome == 'success' # 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: - GH_TOKEN: ${{ steps.bot.outputs.token }} + # 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.…`). - BOT_LOGIN: ${{ steps.bot.outputs.app-slug }}[bot] + # 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. Tolerant of a failed lookup (|| true + # → empty → proceed): a stale write is worse than a missing one, but a + # transient API blip must not silence the notice entirely. + LIVE_HEAD="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true)" + if [ -n "$LIVE_HEAD" ] && [ "$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" - printf '✅ **Now under the cap** — the review panel ran on the latest push.\n' + # 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 { @@ -773,9 +839,17 @@ jobs: # 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 changed lines, after generated-file exclusion) exceeds `diff_size_cap` (%s). **No review panel ran on this PR.**\n\n' \ - "$COUNTED_TEXT" "$DIFF_SIZE_CAP" - printf 'Reduce the counted diff or split the PR; the review re-runs automatically on the next push.\n' + 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 @@ -785,13 +859,15 @@ jobs: # deliberately — same hazards, so the same shape rather than a third # dialect of it. # - # .user.login == our own 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. + # .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 @@ -802,16 +878,22 @@ jobs: # 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" ' - .[] | select((.user.login // "") == $login) + | 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, and the documented caller subscribes - # to labeled/unlabeled alongside synchronize, so a push racing a - # label toggle can leave two comments. Taking `head -n1` would pin + # 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 @@ -856,24 +938,26 @@ jobs: fi - name: Note degraded mode - # Covers every path where no comment is posted, so the job never goes - # green silently: "creds absent" (mint skipped), "mint failed" (mint - # errored), "post failed" (upsert ran but failed at runtime). The + # 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 every one of those cases. - if: steps.bot.outcome != 'success' || steps.upsert.outcome == 'failure' + # 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 [ "$UPSERT_OUTCOME" = "failure" ]; then - 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 permission 403 on an installation that has not granted Pull requests: write)." - elif [ "$BOT_OUTCOME" = "failure" ]; then - echo "Sticky comment NOT written: the bot app token could not be minted for this repo." - else - echo "Sticky comment NOT written: no bot credentials configured (bot_app_id and/or BOT_APP_PRIVATE_KEY are unset)." + 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 diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 0f3a9695..9842cd3b 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -126,7 +126,7 @@ 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 show on fork and Dependabot PRs too), plus a sticky PR comment naming the counted total and the cap. Push the PR under the cap and that comment flips to ✅ on the next run, when the panel actually runs. The comment needs `bot_app_id` + `BOT_APP_PRIVATE_KEY`; without them it degrades silently to the annotation and the summary, and the job log says which of "creds absent / mint failed / post failed" applied. The comment path is best-effort throughout — it never reddens the run. +**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 From 1b886c5b11719f70fbccae62ca8629ba2f4ea698 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 14 Aug 2026 15:47:55 -0700 Subject: [PATCH 3/3] fix(cursor-review): fail closed when the live-head lookup fails (BE-7646) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit, on the guard added in bf779f5: a failed `gh api` call left LIVE_HEAD empty and the run proceeded to write, so a transient API failure could let an obsolete run PATCH its stale verdict over the current one — the exact case the guard exists to prevent. Fails closed now: an errored or empty lookup exits 1, which under continue-on-error routes to "Note degraded mode". Costs almost no availability, since the list and write below hit the same API with the same token a line later. --- .github/workflows/cursor-review.yml | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 5f9e4271..bca01110 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -798,11 +798,22 @@ jobs: # 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. Tolerant of a failed lookup (|| true - # → empty → proceed): a stale write is worse than a missing one, but a - # transient API blip must not silence the notice entirely. - LIVE_HEAD="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true)" - if [ -n "$LIVE_HEAD" ] && [ "$LIVE_HEAD" != "$EVENT_HEAD_SHA" ]; then + # 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