[PR #169] [MERGED] feat(cursor-review): announce the over-cap skip instead of a silent green run (BE-7646) #176

New issue

Closed

opened 2026-10-26 22:37:03 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:03 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/169
Author: @mattmillerai
Created: 8/14/2026
Status: ✅ Merged
Merged: 8/14/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-7646-over-cap-comment

📝 Commits (3)

de92954 feat(cursor-review): announce the over-cap skip instead of a silent green run (BE-7646)
bf779f5 fix(cursor-review): address panel findings on the over-cap notice (BE-7646)
1b886c5 fix(cursor-review): fail closed when the live-head lookup fails (BE-7646)

📊 Changes

3 files changed (+358 additions, -3 deletions)

View changed files

📝 .github/cursor-review/README.md (+25 -1)
📝 .github/workflows/cursor-review.yml (+330 -1)
📝 docs/callers/cursor-review.md (+3 -1)

📄 Description

ELI-5

If your PR is too big, the Cursor review panel quietly doesn't run — and the check still turns green, so it looks exactly like "eight models reviewed this and found nothing." This PR makes that skip say so out loud: a warning annotation, a step-summary block, and a sticky PR comment that names how many lines you counted and what the cap is. Shrink the PR and the same comment flips to a ✅ on the next run, when the panel actually runs. Nothing becomes blocking — the run is still green either way.

What changed

.github/workflows/cursor-review.yml:

diff-size exposes counted as a job output alongside within_cap. The shared check-pr-size tool already writes counted=<n> to GITHUB_OUTPUT (scripts/check-pr-size/main.go:733); the degraded raw-count fallback now appends counted=${RAW} too, with the same last-value-wins mechanism it already uses for over_cap, so the number quoted to the author always agrees with whichever count actually decided the gate.
Announce over-cap skip — a new step in diff-size on the over_cap == 'true' path: a ::warning:: annotation plus a step-summary block. This half needs no credentials, so it reaches fork and Dependabot PRs and consumers that never configured a bot app.
A new over-cap-comment job, modelled line-for-line on pr-size.yml's comment job (same action SHA pin, same continue-on-error mint + upsert, same author-gated startswith sticky-comment lookup). It upserts one comment behind the marker , POSTing only on the over-cap path and PATCHing an existing one either way — so a stale "skipped" comment flips to ✅ once the PR is trimmed, and a PR that was always under the cap never gets a comment at all.

Docs: the diff_size_cap input description, docs/callers/cursor-review.md (a new gotcha + the inputs-table note), and .github/cursor-review/README.md (a new "Over the diff-size cap" section + a pointer under the flow diagram).

Deliberately not touched: the notify-* jobs (Slack-on-skip would change notification traffic fleet-wide and wants its own opt-in input), and no job was changed to fail on over-cap.

Why non-blocking

cursor-review is advisory — it posts a review, it does not gate. Verified live rather than assumed: repos/Comfy-Org/cloud/rules/branches/main lists eleven required status checks and no cursor-review job is among them, so failing the job would block nothing while breaking the reusable's contract for every caller. The fix is visibility, not enforcement — hence continue-on-error on both the mint and the upsert, and the diff-size annotation as the credential-free fallback signal.

Judgment calls

Gated on needs.diff-size.result == 'success', not on within_cap != 'true'. A git-level diff-size crash leaves the job's outputs empty, and an empty within_cap satisfies != 'true' — which would post a confident "over cap" comment about a job that never counted anything. Gating on the result also lets the job run on within-cap runs, which is the only way a stale over-cap comment ever gets flipped.
Marker collision checked, not assumed.  is distinct from gate-unresolved.py's CONSOLIDATED_MARKER, and the two live in different spaces entirely: build-ledger.py reads pulls/{n}/reviews and pulls/{n}/comments, and nothing in cursor-review reads issues/{n}/comments at all.
The counted number is rendered as unknown / an unknown number of when the output is empty, rather than printing an empty string as if it were a fact.
The new job runs on within-cap runs too — one short runner job per review run, which is the cost of being able to flip a stale comment. Its log states which of "creds absent / mint failed / post failed" applied, and says explicitly when nothing was owed.

Verification

actionlint .github/workflows/cursor-review.yml — one finding, and it is the pre-existing github.job_workflow_sha schema complaint that is identical on origin/main (confirmed by linting the base version). Zero new findings, including shellcheck over the new run: blocks.
python3 -m unittest discover -s .github/cursor-review/tests (187 tests) — OK. .github/workflow-pins check + tests, and .github/groom tests — OK.
Behavioural simulation of the upsert logic. I extracted the run: script out of the YAML and exercised it against a stubbed gh across eight scenarios: over-cap with no existing comment (POST), over-cap with an existing one (PATCH), within-cap with an existing one (PATCH to ✅), within-cap with none (no write at all), empty counted (falls back to the "unknown" wording), two duplicate stickies (both PATCHed), every write 403 (step exits 1, so continue-on-error keeps the job green and the degraded note fires), and a POST 403. The author-gating held: a comment carrying the marker but posted by a User, and a bot comment merely quoting the marker mid-body, were both correctly ignored. The Announce over-cap skip step was run the same way for both the populated and empty counted cases.
The "no review panel ran on this PR" wording is a claim about the run, so I falsified it against the file rather than trusting the ticket: preflight, review, consolidate, notify-start and notify-complete are each gated on within_cap == 'true', and the only other conditions referencing over_cap are != 'true' guards. There is no cap-bypass path on main — the cursor-review-xl label lives in an unmerged draft, and is deliberately not referenced in the comment text.

Unexercised artifacts

No live end-to-end run. Proving this in production needs a cursor-review-labeled PR with a >5000-line counted diff on a consumer that has the bot app configured, which is only possible after this merges and bump-cursor-review-callers.yml fans the SHA bump out. Everything above is static analysis plus a stubbed-API simulation of the shell, not a real Actions run.
The two open PRs on the same file. #167 (extra_generated_globs default) and #168 (diff fencing) both touch .github/workflows/cursor-review.yml and .github/cursor-review/README.md. My hunks are in different regions, but whichever lands second should rebase.

Refs BE-7646 — Refs, not Closes: nothing has yet observed the annotation, the summary or the sticky comment on a real over-cap run, so the ticket stays open to carry that post-bump behavioural check.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/169 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/14/2026 **Status:** ✅ Merged **Merged:** 8/14/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-7646-over-cap-comment` --- ### 📝 Commits (3) - [`de92954`](https://github.com/Comfy-Org/github-workflows/commit/de929546f6541353f85d859a10486d30d5b6511c) feat(cursor-review): announce the over-cap skip instead of a silent green run (BE-7646) - [`bf779f5`](https://github.com/Comfy-Org/github-workflows/commit/bf779f5cc5541def48982b805a2f4417f81ee007) fix(cursor-review): address panel findings on the over-cap notice (BE-7646) - [`1b886c5`](https://github.com/Comfy-Org/github-workflows/commit/1b886c5b11719f70fbccae62ca8629ba2f4ea698) fix(cursor-review): fail closed when the live-head lookup fails (BE-7646) ### 📊 Changes **3 files changed** (+358 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+25 -1) 📝 `.github/workflows/cursor-review.yml` (+330 -1) 📝 `docs/callers/cursor-review.md` (+3 -1) </details> ### 📄 Description ## ELI-5 If your PR is too big, the Cursor review panel quietly doesn't run — and the check still turns green, so it looks exactly like "eight models reviewed this and found nothing." This PR makes that skip say so out loud: a warning annotation, a step-summary block, and a sticky PR comment that names how many lines you counted and what the cap is. Shrink the PR and the same comment flips to a ✅ on the next run, when the panel actually runs. Nothing becomes blocking — the run is still green either way. ## What changed `.github/workflows/cursor-review.yml`: 1. **`diff-size` exposes `counted`** as a job output alongside `within_cap`. The shared `check-pr-size` tool already writes `counted=<n>` to `GITHUB_OUTPUT` (`scripts/check-pr-size/main.go:733`); the degraded raw-count fallback now appends `counted=${RAW}` too, with the same last-value-wins mechanism it already uses for `over_cap`, so the number quoted to the author always agrees with whichever count actually decided the gate. 2. **`Announce over-cap skip`** — a new step in `diff-size` on the `over_cap == 'true'` path: a `::warning::` annotation plus a step-summary block. This half needs no credentials, so it reaches fork and Dependabot PRs and consumers that never configured a bot app. 3. **A new `over-cap-comment` job**, modelled line-for-line on `pr-size.yml`'s `comment` job (same action SHA pin, same `continue-on-error` mint + upsert, same author-gated `startswith` sticky-comment lookup). It upserts one comment behind the marker ``, POSTing only on the over-cap path and PATCHing an existing one either way — so a stale "skipped" comment flips to ✅ once the PR is trimmed, and a PR that was always under the cap never gets a comment at all. Docs: the `diff_size_cap` input description, `docs/callers/cursor-review.md` (a new gotcha + the inputs-table note), and `.github/cursor-review/README.md` (a new "Over the diff-size cap" section + a pointer under the flow diagram). Deliberately **not** touched: the `notify-*` jobs (Slack-on-skip would change notification traffic fleet-wide and wants its own opt-in input), and no job was changed to fail on over-cap. ## Why non-blocking `cursor-review` is advisory — it posts a review, it does not gate. Verified live rather than assumed: `repos/Comfy-Org/cloud/rules/branches/main` lists eleven required status checks and no cursor-review job is among them, so failing the job would block nothing while breaking the reusable's contract for every caller. The fix is visibility, not enforcement — hence `continue-on-error` on both the mint and the upsert, and the `diff-size` annotation as the credential-free fallback signal. ## Judgment calls - **Gated on `needs.diff-size.result == 'success'`, not on `within_cap != 'true'`.** A git-level `diff-size` crash leaves the job's outputs empty, and an empty `within_cap` satisfies `!= 'true'` — which would post a confident "over cap" comment about a job that never counted anything. Gating on the result also lets the job run on within-cap runs, which is the only way a stale over-cap comment ever gets flipped. - **Marker collision checked, not assumed.** `` is distinct from `gate-unresolved.py`'s `CONSOLIDATED_MARKER`, and the two live in different spaces entirely: `build-ledger.py` reads `pulls/{n}/reviews` and `pulls/{n}/comments`, and nothing in `cursor-review` reads `issues/{n}/comments` at all. - **The counted number is rendered as `unknown` / `an unknown number of` when the output is empty**, rather than printing an empty string as if it were a fact. - **The new job runs on within-cap runs too** — one short runner job per review run, which is the cost of being able to flip a stale comment. Its log states which of "creds absent / mint failed / post failed" applied, and says explicitly when nothing was owed. ## Verification - `actionlint .github/workflows/cursor-review.yml` — one finding, and it is the pre-existing `github.job_workflow_sha` schema complaint that is identical on `origin/main` (confirmed by linting the base version). Zero new findings, including shellcheck over the new `run:` blocks. - `python3 -m unittest discover -s .github/cursor-review/tests` (187 tests) — OK. `.github/workflow-pins` check + tests, and `.github/groom` tests — OK. - **Behavioural simulation of the upsert logic.** I extracted the `run:` script out of the YAML and exercised it against a stubbed `gh` across eight scenarios: over-cap with no existing comment (POST), over-cap with an existing one (PATCH), within-cap with an existing one (PATCH to ✅), within-cap with none (no write at all), empty `counted` (falls back to the "unknown" wording), two duplicate stickies (both PATCHed), every write 403 (step exits 1, so `continue-on-error` keeps the job green and the degraded note fires), and a POST 403. The author-gating held: a comment carrying the marker but posted by a *User*, and a bot comment merely *quoting* the marker mid-body, were both correctly ignored. The `Announce over-cap skip` step was run the same way for both the populated and empty `counted` cases. - The "no review panel ran on this PR" wording is a claim about the run, so I falsified it against the file rather than trusting the ticket: `preflight`, `review`, `consolidate`, `notify-start` and `notify-complete` are each gated on `within_cap == 'true'`, and the only other conditions referencing `over_cap` are `!= 'true'` guards. There is no cap-bypass path on `main` — the `cursor-review-xl` label lives in an unmerged draft, and is deliberately not referenced in the comment text. ## Unexercised artifacts - **No live end-to-end run.** Proving this in production needs a `cursor-review`-labeled PR with a >5000-line counted diff on a consumer that has the bot app configured, which is only possible after this merges and `bump-cursor-review-callers.yml` fans the SHA bump out. Everything above is static analysis plus a stubbed-API simulation of the shell, not a real Actions run. - **The two open PRs on the same file.** #167 (`extra_generated_globs` default) and #168 (diff fencing) both touch `.github/workflows/cursor-review.yml` and `.github/cursor-review/README.md`. My hunks are in different regions, but whichever lands second should rebase. Refs BE-7646 — `Refs`, not `Closes`: nothing has yet observed the annotation, the summary or the sticky comment on a real over-cap run, so the ticket stays open to carry that post-bump behavioural check. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:03 +00:00