feat(pr-risk): opt-in Check Run + sticky comment with risk concentration and a dispute checkbox - #120
Conversation
mattmillerai
added
cursor-review
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 4 |
| 🟡 Medium | 4 |
| 🟢 Low | 2 |
Panel: 7/8 reviewers contributed findings.
Reviewers that did not contribute: gpt-5.6-sol-max:edge-case (empty)
…nt read (BE-6326) Addresses the review panel on #120. Every fix is a case where a transient API failure or a stale read produced a PERMANENT wrong result in the surfaces. * CI: the bounded-body fixture passed its 170KB files array to `jq --argjson`, which Linux refuses past 128KiB per argv entry (MAX_ARG_STRLEN) while macOS, having no per-argument cap, accepted it — green locally, red on every runner. It now goes in via `--slurpfile`, and the fixture is pinned before the size assertion so a record that failed to build can no longer pass it vacuously by rendering the short "unknown" body. * A failed comment LIST no longer falls through to the create branch (it posted a SECOND sticky, and another on every later failure), and a failed dispute RE-READ no longer reads as "unticked" (it rewrote the body with the box clear AND removed the dispute label, erasing a real disagreement on a 500). `comment_is_disputed` grew a third outcome — unknown — and both mean: write nothing, say why. * The sticky scan runs BACKWARDS from the last page. `sort`/`direction` are not parameters of GET /issues/{n}/comments and were silently ignored, so the listing was oldest-first and the page bound was unrecoverable on a chatty PR. * The Check Run attaches to the GRADED commit: the record now carries the oid whose rollup produced the tier, so a push during the rollup settle no longer gets the previous commit's grade stamped on it immutably. * `checks: write` is documented as REQUIRED OF EVERY CALLER. GitHub validates a nested job's declaration at startup, before any `if:`, so `check_run: false` did NOT leave enrolled callers untouched as the header claimed — a pin bump alone would have failed their next run. Same union rule groom.yml documents. * The aggregate publish payload is bounded (~100KB) instead of only per-target: 50 targets x the per-target cap is ~600KB in one env string and would E2BIG the publish step, reddening an advisory grade. The excess degrades and is named, per target and in a run-level tally. * Scratch init moved to startup (lazy init landed in a subshell, so every `gherr` in a warning printed nothing), a render failure is annotated with the renderer's stderr instead of /dev/null, the success log lines moved into the `else` of their write, and the truncation backstop closes the `<details>` it cut through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review panel resolved in 07d93a8 — all 10 threads addressed and closed, no deferrals. Details are on each thread; two things are worth surfacing here rather than leaving buried: 1. The red CI check was a real portability bug, not a flake. The bounded-body test fixture handed a ~170 KB files array to 2. Consequence for whoever merges: a caller moving its pin onto this commit must add Also fixed: a failed comment list no longer posts a duplicate sticky; a failed dispute re-read no longer clears the checkbox and strips the label; the sticky scan walks backwards from the last page (the All of it is pinned by tests — 20 new assertions across the three suites (67 / 105 / 63 passing, shellcheck clean). No test that existed was weakened or removed. |
…ion and a dispute checkbox The shadow risk check shipped the risk:* label only. The grade was therefore unexplained (the reasoning lived in an Actions run nobody opens), had no immutable per-commit audit artifact, and offered no low-friction way for a reviewer to register disagreement — which is the pilot's calibration input. Adds two publish surfaces, BOTH OFF BY DEFAULT so an enrolled caller behaves byte-identically until it opts in: * sticky_comment: ONE comment, created once and updated in place, carrying a per-file path-axis breakdown, the concentration sentence and a "this grade is wrong" checkbox that round-trips into a risk-grade-disputed label. Rides the pull-requests: write the label already needs. * check_run: the tier and reason as a neutral Check Run on the head commit, published from a SEPARATE job holding checks: write — a separate job because raising the grade job's checks: read would be rejected for every caller already enrolled, and because the escaping of PR-controlled text then happens in the job that never holds the elevated token. The concentration sentence is built on the SHIPPED grading model, not the superseded per-file-tier one: the grader now reports a per-file PATH FLOOR (same rules, matched per path, reporting only — worst over them equals the floor by construction, pinned by a test), and the sentence names the axis that supplied the headline whenever it exceeds that floor, so it cannot contradict the tier printed above it. Hardening, both covered by tests rather than by estimate: a filename may legally contain a pipe, a backtick or a newline, and rendered raw it breaks out of its table row and can inject a pre-ticked dispute checkbox that the next re-grade reads back as genuine reviewer disagreement — so newlines are flattened, pipes escaped, and a backtick or backslash forces fully-escaped plain text. And the comment body is bounded under GitHub's 65536-char limit by row-dropping plus an unconditional backstop that re-appends the footer, because the label write happens first and a 422 would leave the label fresh and the comment permanently stale. Neither surface can fail a check or block a merge: the Check Run conclusion is hardcoded neutral and every publish failure is an annotation with exit 0.
…nt read (BE-6326) Addresses the review panel on #120. Every fix is a case where a transient API failure or a stale read produced a PERMANENT wrong result in the surfaces. * CI: the bounded-body fixture passed its 170KB files array to `jq --argjson`, which Linux refuses past 128KiB per argv entry (MAX_ARG_STRLEN) while macOS, having no per-argument cap, accepted it — green locally, red on every runner. It now goes in via `--slurpfile`, and the fixture is pinned before the size assertion so a record that failed to build can no longer pass it vacuously by rendering the short "unknown" body. * A failed comment LIST no longer falls through to the create branch (it posted a SECOND sticky, and another on every later failure), and a failed dispute RE-READ no longer reads as "unticked" (it rewrote the body with the box clear AND removed the dispute label, erasing a real disagreement on a 500). `comment_is_disputed` grew a third outcome — unknown — and both mean: write nothing, say why. * The sticky scan runs BACKWARDS from the last page. `sort`/`direction` are not parameters of GET /issues/{n}/comments and were silently ignored, so the listing was oldest-first and the page bound was unrecoverable on a chatty PR. * The Check Run attaches to the GRADED commit: the record now carries the oid whose rollup produced the tier, so a push during the rollup settle no longer gets the previous commit's grade stamped on it immutably. * `checks: write` is documented as REQUIRED OF EVERY CALLER. GitHub validates a nested job's declaration at startup, before any `if:`, so `check_run: false` did NOT leave enrolled callers untouched as the header claimed — a pin bump alone would have failed their next run. Same union rule groom.yml documents. * The aggregate publish payload is bounded (~100KB) instead of only per-target: 50 targets x the per-target cap is ~600KB in one env string and would E2BIG the publish step, reddening an advisory grade. The excess degrades and is named, per target and in a run-level tally. * Scratch init moved to startup (lazy init landed in a subshell, so every `gherr` in a warning printed nothing), a render failure is annotated with the renderer's stderr instead of /dev/null, the success log lines moved into the `else` of their write, and the truncation backstop closes the `<details>` it cut through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sticky body led with an H2, the worst() formula, a concentration
paragraph and an always-open per-file table. That is a lot of vertical
space for an advisory grade nothing routes on, on PRs that already carry
CodeRabbit and an 8-cell review panel.
Visible body is now one line: the tier, the axis that DECIDED it, that
axis's reason, and — only when the path axis decided — what share of the
diff carries it. Naming the driver replaces `$R.reason` in the headline,
which was the machine trace ("worst of path_floor=R1, provenance=R2,
...") and restated the tier instead of explaining it. The concentration
fragment is suppressed when another axis drove the tier, since quoting a
path-floor percentage under a provenance-driven headline points at the
wrong number.
Everything else moves inside a collapsed <details>: the formula, the full
concentration sentence, a new per-axis table (tier + reason, previously
only in the Check Run), the map versions and the per-file breakdown. The
Check Run is unchanged and keeps the long form.
Invariants held: the marker is still line 1, the dispute checkbox is
still outside the <details> and still matches CHECKED_RE, the truncation
backstop still closes the section it opened, and assemble()'s row-dropping
loop is untouched. Adds an `endstop` helper so a reason without a trailing
stop cannot run into the following sentence.
Two tests updated for the new shape, not weakened: the per-file row counts
are now scoped to the per-file table (the per-axis table would otherwise
inflate them), and the unknown-headline assertion matches the new string.
280 pr-risk tests pass, shellcheck clean.
8c6ff0a to
5baabcd
Compare
|
Rebased onto |
2 participants
ELI-5
The risk grader stamps a
risk:R0..risk:R3sticker on every PR, and that sticker never says why. This adds two things you can turn on if you want them: a comment that explains the grade ("94% of this diff is R0/R1; the 6% that puts the path floor at R3 is these two files, 40 lines") with a tickbox for "this grade is wrong", and a Check Run that pins the grade to the exact commit so there's a permanent record. Both are off unless you switch them on, and neither can ever fail your build.What this adds
Two publish surfaces, both
falseby default:sticky_comment: true— ONE comment, created once and updated in place (N pushes leave one comment), carrying a per-file breakdown, the concentration sentence, and a dispute checkbox whose state round-trips into arisk-grade-disputedlabel. It rides thepull-requests: writethe label already needs, so switching it on requires no permission change.check_run: true— the tier and reason as a Check Run on the head commit, withconclusion: neutralhardcoded. Published from a separate job that holdschecks: write.Judgment calls
Per-file breakdown = the PATH axis, and it says so. The ticket asked me to pick between deriving per-path tiers from the same rules or reporting by matched class. I derived per-path tiers: the grader now emits
risk.axes.path_floor.files, matched with the same rules as the floor, reporting-only.worstover those per-file floors equals the floor by construction, and a test pins that equality so the breakdown cannot quietly become a second grading model. Because only the path axis is per-file at all, each row is labelled a contribution, and whenever the headline tier exceeds the path floor the concentration sentence names the axis that supplied it and quotes its reason — without that, a docs-only diff headlinesR2(no green checks) directly above "all 600 lines are R0" and the explanation contradicts the tier it explains.The Check Run is a separate job, and that is the two-job split the ticket asked about. Not primarily for elegance: raising the grade job's
checks: readtochecks: writewould be rejected before any step ran for every caller already enrolled with the reviewed permissions block inpr-risk.yml's header — a reusable workflow can only narrow the caller's token. Splitting it keeps the new grant opt-in and gives the posture BE-5508 wanted: all PR-controlled text is escaped in the grading job (narrow token), and the job holdingchecks: writereads nothing from a pull request — its whole input is a pre-rendered, pre-escaped string.Dispute state: the checkbox is the source of truth, the label mirrors it. PR #100 OR-ed the label back into the state, which made the label one-way — once set, no un-tick could ever clear it. Here ticking applies
risk-grade-disputedand un-ticking removes it. The cost is one narrow race (a tick landing between the read-back and the PATCH is overwritten), which is why the body is re-read by id immediately before the write rather than reused from the paginated scan.risk-grade-disputedstays distinct from the human-ownedrisk-dispute, which the grader still never touches.Hardening (ported, and tested rather than estimated)
|, backticks and newlines in a path; rendered raw into a table it breaks out of its code span and its row, letting an author inject a pre-ticked- [x] **This grade is wrong**that the next re-grade reads back as genuine reviewer disagreement — corrupting the exact calibration signal the checkbox exists to collect. Newlines are flattened (nothing PR-controlled may start a line), pipes are backslash-escaped, and a backtick or backslash forces fully-escaped plain text. Same treatment for anunknownreport's reason, which quotes PR-authored path names. Covered by a test using a filename containing all three plus a literal pre-ticked checkbox and an<img>tag.find_stickymatches — if they drifted, every push would POST a new comment instead of updating.Botand an allow-listed login and the marker at position 0. Marker-only matching would let a PR author pre-post a comment carrying the (public) marker and thereafter control the preserved checkbox state.Verification
All five suites green locally, plus shellcheck on every touched script:
test_publish_risk_surfaces.sh— 48 passed, 0 failed (new)test_grade_pr_risk.sh— 62 passed (was 57; +5 for the per-file-floor invariant)test_grade_targets.sh— 91 passed ·test_apply_risk_label.sh— 24 ·test_resolve_enabled.sh— 21shellcheck -xclean on all scripts and suites; both workflow files parse.test-pr-risk.ymlruns the new suite and shellchecks the new script.What a reviewer should check hardest
if: ... && inputs.check_run) declareschecks: writethe caller did not grant. I could not exercise that from here. If GitHub validates eagerly at graph construction instead of at job start, every existing caller breaks on the SHA bump — which would make the fix "gate the whole job behind a caller-sideif:instead", not a code change here. This is the one line I would want confirmed against a real run before the tag moves.surfacesas a job output. Job outputs share a ~1MB budget, so the rendered summary is capped at 12,000 chars per target before it travels (CHECK_SUMMARY_CAP). A 50-target batch therefore fits; without the cap it would silently truncate every target's payload, not just the offenders.Notes
unknownpath (which reportsunknown, neverR0) is unchanged and covered by tests including the empty and non-JSON record cases. So the negative-claim falsification rule does not apply.