[groom] cursor-review's severity badge is authored from two tables in post-review.py and re-parsed by a hardcoded vocabulary regex in build-ledger.py - #238
Open
cloud-code-bot[bot] wants to merge 3 commits into
Conversation
…post-review.py and re-parsed by a hardcoded vocabulary regex in build-ledger.py Auto-built groom refactor (github-workflows:whole-repo:github-cursor-review-build-ledger-py). Review required — do not auto-merge.
cloud-code-bot
Bot
added
cursor-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…endering Addresses the cursor-review panel's findings on this PR. - The contract loop iterated SEVERITY_ORDER, the sort-order table. The renderable vocabulary is gated on SEVERITY_EMOJI (normalize_severity) and the badge is built from SEVERITY_EMOJI/SEVERITY_LABEL, so a severity added to those two but not to SEVERITY_ORDER rendered a badge _BADGE_RE cannot strip while the test stayed green — the exact silent strip-stop the pin claims to make impossible. Iterate SEVERITY_EMOJI, and assert the three tables carry the same keys (which also stops the loop passing vacuously). - Correct the pin's advertised scope in both the comment and the docstring: `\s*` absorbs any spacing change, so the LABELS and the dash CHARACTER are pinned, not the whitespace. Verified by mutation: a spacing-only edit still strips and stays green; a dash or label change fails. - Record that `^\S*` swallows any non-whitespace run, not "any emoji or none" — a forged prefix strips too — and the caller-trust precondition that makes it safe (thread roots of Bot-authored consolidated reviews, bodies this workflow wrote), so nobody reuses it on imported PR text. - Record that a renamed label must be ADDED to the alternation with the old one KEPT, since rounds posted by a caller on an older ref still carry it. - Drop the assertNotEqual whose message stated the passing condition and which the following assertEqual subsumes with a better diff.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Round 2 — ledger: 5 prior finding(s) across 1 round(s) (5 never answered).
Found 6 finding(s).
| Severity | Count |
|---|---|
| 🟢 Low | 3 |
| ⚪ Nit | 3 |
Panel: 8/8 reviewers contributed findings.
…ity too Round 2 of the review panel. All six findings were valid; none needed a runtime change. Tests: - `review-output-mcp.py`'s `SEVERITIES` joins the key-set equality. It is the fourth copy of the vocabulary and the ingress schema gating what a model may submit at all, so a severity added to post-review.py's three tables and to `_BADGE_RE` was still rejected at submission with this contract green. - New `test_the_default_severity_is_one_post_review_can_render`. The equality above is satisfied by a key renamed CONSISTENTLY across all three tables, which leaves `DEFAULT_SEVERITY` dangling — and `normalize_comments` indexes `SEVERITY_EMOJI[severity]` unguarded after `normalize_severity` falls back to it, so the first model-supplied unknown severity would `KeyError` and kill consolidate after the panel and judge had already run. Degradation elsewhere in this class; a crash here. - A spaceless prefix (`🔴**Critical** — `) joins the tolerant-prefix loop. Every other case ended in whitespace, so nothing covered the greedy `^\S*` swallowing the label and backtracking — the behaviour build-ledger.py's comment leans on. Comment above `_BADGE_RE`, corrected where it claimed more than the code does: - "bodies this workflow itself wrote" overstated the gate. `_consolidated_reviews` requires author type Bot plus a marker this public repo puts within anyone's reach, so the guarantee is "some Bot wrote it"; the conclusion survives because clearing that gate already hands the forger the whole prose. - The keep-the-old rename rule now covers the em dash, which carries the identical old-ref exposure: change it to a class holding both characters, never a swap. - States that a new label's lowercase must equal its severity key, since `_strip_badge` derives the ledger's severity from `match.group(1).lower()`. Mutation-checked, not argued: dropping `nit` from the ingress tuple fails 1 test, renaming the `medium` key across all three tables fails 3 (was 1, and none of them the default-severity relationship), requiring whitespace after the emoji run fails the spaceless and empty prefixes. Suite: 364 tests OK; 373 on the merged result against main, alongside agents-md (46), groom (365), shellcheck and bump-callers (437). No runtime code touched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI-5
When the review bot leaves an inline comment, it starts the comment with a little severity badge like
🔴 **Critical** —. A second script, the one that builds the "what did earlier rounds already say" ledger, has to peel that badge back off before it quotes the finding to the next round's models. The badge is written in one file (post-review.py) and peeled off in another (build-ledger.py), and the peeler re-types the list of severity names and the badge's exact shape by hand. Nothing checks that the two spellings still agree, so if someone added a severity or reworded a label, the writer would keep working while the peeler quietly stopped peeling — no error, just slightly worse quoting. This PR adds a test that renders a real badge with the real writer and feeds it to the real peeler, for every severity, so the two can no longer drift apart unnoticed. No production behavior changes.What changed
.github/cursor-review/tests/test_build_ledger.py— newTestSeverityBadgeContractclass with three tests. The first asserts the three severity tables carry the same keys (SEVERITY_EMOJI/SEVERITY_LABEL/SEVERITY_ORDER) and are non-empty, so neither a partial table nor an emptied one slips through. The second loops over every key ofpost_review.SEVERITY_EMOJI— the tablenormalize_severitygates the renderable vocabulary on, not the sort-order list — renders a finding throughpost_review.normalize_comments(the actual renderer that builds the badge), and assertsbuild_ledger._strip_badgereturns that exact severity and the untouched prose. The badge string is never re-typed in the test, so a change to the label, the**…**shape or the dash CHARACTER fails here; spacing around the dash is deliberately NOT pinned, because_BADGE_RE's\s*runs absorb it. The third test holds the deliberately tolerant^\S*\s*prefix: a badge with no emoji, or with an emoji the writer does not use today, still parses..github/cursor-review/build-ledger.py— comment only, above_BADGE_RE: records that the vocabulary is re-typed on purpose (so the ledger keeps reading bodies written by an olderpost-review.pystill live on a pinned caller ref) and that the duplication is pinned by that test instead of shared via an import. It also states what the tolerance actually is —^\S*swallows any non-whitespace run, not just an emoji, so a forged badge strips too, which is safe only because the sole caller reads thread roots of Bot-authored consolidated reviews — and that renaming a severity label means ADDING the new spelling to the alternation while KEEPING the old one, since rounds already posted by a caller on an older ref still carry it.No runtime code was touched, so the ledger, the poster and their outputs are byte-for-byte what they were.
Why
post-review.pybuilds the badge fromSEVERITY_EMOJI/SEVERITY_LABEL, and its ownstrip_severity_badgereverses it by rebuilding the same string from those tables — its docstring says that is precisely so the two cannot drift.build-ledger.py's_BADGE_REdoes the opposite: it hardcodes(Critical|High|Medium|Low|Nit)and the badge's punctuation as a literal alternation, with nothing tying it back to the tables. If a severity were added, a label renamed, or the dash/spacing changed, the ledger could silently stop stripping and would carry badge-prefixed bodies into the prior-review context, which may degrade the repeat/dedup matching the ledger exists for; it would not raise. There is no evidence of this having happened.Sharing the formatter outright would cost something real, so this PR does not do it: the tolerant
^\S*\s*prefix accepts any emoji or none on purpose, which is what lets the ledger keep parsing comments written by an olderpost-review.pythat a consumer is still pinned to. A test is the smaller instrument that closes the drift gap without giving that up, and it matches the pattern already used in this suite for the body-only prose marker, which is likewise duplicated on purpose and pinned across the two files by a test.Verifier rationale
Problem (verified end to end)
post-review.py owns the badge:
SEVERITY_EMOJI(line 42) andSEVERITY_LABEL(line 49) are formatted intof"{SEVERITY_EMOJI[severity]} **{SEVERITY_LABEL[severity]}** — "at line 999, andstrip_severity_badge(line 805) reverses it by RECONSTRUCTING the same string. Its docstring states the rule explicitly:Reconstructed from the same two tables that built it rather than re-matched with a regex, so the two can never drift.build-ledger.py does the thing that docstring rejects.
_BADGE_RE(line 107) is^\S*\s*\*\*(Critical|High|Medium|Low|Nit)\*\*\s*—\s*— the severity vocabulary re-typed as a literal alternation — and_strip_badge(line 298) is documented only asSplit post-review.py's severity badge off an inline comment body.Why the usual objections do not apply
.github/cursor-review/and are always checked out at the same ref. build-ledger.py ALREADY loads a sibling module by path —_load_gate_unresolved()at line 87, whose docstring saysReused, not re-implemented: the ledger takes CONSOLIDATED_MARKER... from that one module. The precedent for sharing is in this very file, which also refutes the finder'sif build-ledger.py must stay import-freecaveat.^\S*\s*deliberately accepts any emoji or none, so the ledger keeps parsing bodies written by an OLDER post-review.py still live on a pinned caller ref. Sharing the strict FORMATTER would break that. The finder concedes this and it is correct.Risk (conditional)
Adding a severity, renaming a label, or changing the dash/spacing would leave post-review.py working while build-ledger.py silently stopped stripping — badge-prefixed bodies would then be carried into the prior-review context, degrading the repeat/dedup matching the ledger exists for. It would not error.
Fix (blind-implementable, minimal form first)
Preferred: move
SEVERITY_EMOJI,SEVERITY_LABEL, abadge_for(severity)and the tolerantstrip_badge(body)into.github/cursor-review/severity.py; import it from post-review.py directly and from build-ledger.py via the sameimportlib.util.spec_from_file_locationidiom_load_gate_unresolvedalready uses; build_BADGE_RE's alternation fromSEVERITY_LABEL.values()rather than a literal, keeping the^\S*\s*prefix as is.Minimum acceptable: keep both files as they are and add a test asserting that
_BADGE_REmatchesbadge_for(s)for EVERY key inSEVERITY_EMOJI. Both suites exist (tests/test_post_review.py, 1688 lines;tests/test_build_ledger.py, 1414 lines).Not security
The badge strip is a prefix removal on text the workflow itself wrote; the ledger's actual trust controls (the Bot-author requirement on prior reviews, the fence defanging, the body-only sentinel literals) are separate code and are untouched by this change.
Provenance
Verified:
python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'— 364 tests, OK. Green again on the MERGED result againstmain(373 tests), alongside the agents-md (46) and groom (365) suites,shellcheck -xandbash .github/bump-callers/tests/test_bump_callers.sh(437 passed). Every pin was checked by mutation, not argument: droppingnitfromreview-output-mcp.py's ingress tuple fails 1 test; renaming themediumkey consistently across all three post-review.py tables fails 3 (it failed 1 before round 2, and none of them the dangling-DEFAULT_SEVERITYrelationship); requiring whitespace after the emoji run (^\S*\s+) fails exactly the spaceless and empty prefixes; the em dash changed to a hyphen fails 5; and a spacing-only change stays green, which is why the "spacing" claim is absent from both the comment and the docstring.Deviations: The verifier's preferred fix (extract a shared
severity.py) was deliberately not taken; this is its "minimum acceptable" form, for the old-ref-tolerance reason the verifier itself records. All eleven review findings across two rounds (five Low, six Nit) were accepted and applied — round 2 added the ingress vocabulary to the key-set equality, pinnedDEFAULT_SEVERITY's renderability (the one relationship whose breach raisesKeyErrorrather than degrading), covered the spaceless badge shape, and corrected three over-claims in the_BADGE_REcomment: the caller's gate is Bot-author-plus-forgeable-marker rather than "bodies this workflow wrote", the keep-the-old rename rule now covers the em dash as well as labels, and a new label's lowercase must equal its severity key. No runtime code changed.