[PR #238] [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 #245

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/238
Author: @cloud-code-bot[bot]
Created: 8/27/2026
Status: 🔄 Open

Base: main ← Head: groom/github-workflows-whole-repo-github-cursor-review-33073755508-0

📝 Commits (4)

5e0f0f2 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
493a0e7 test(cursor-review): pin the badge contract on the table that gates rendering
9a2fb6b test(cursor-review): pin the ingress vocabulary and the default severity too
c3e8c62 Merge branch 'main' into groom/github-workflows-whole-repo-github-cursor-review-33073755508-0

📊 Changes

2 files changed (+120 additions, -0 deletions)

View changed files

📝 .github/cursor-review/build-ledger.py (+31 -0)
📝 .github/cursor-review/tests/test_build_ledger.py (+89 -0)

📄 Description

🤖 Auto-built by the groom sweep — this is a machine-proposed refactor. It runs full CI + cursor-review and requires human review; it is NOT auto-merged. Ranked from a CONFIRMED groom finding. · run

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 — new TestSeverityBadgeContract class 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 of post_review.SEVERITY_EMOJI — the table normalize_severity gates the renderable vocabulary on, not the sort-order list — renders a finding through post_review.normalize_comments (the actual renderer that builds the badge), and asserts build_ledger._strip_badge returns 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 older post-review.py still 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.py builds the badge from SEVERITY_EMOJI / SEVERITY_LABEL, and its own strip_severity_badge reverses 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_RE does 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 older post-review.py that 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) and SEVERITY_LABEL (line 49) are formatted into f"{SEVERITY_EMOJI[severity]} **{SEVERITY_LABEL[severity]}** — " at line 999, and strip_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 as Split post-review.py's severity badge off an inline comment body.

Why the usual objections do not apply

Packaging: none. The two files sit in .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 says Reused, 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's if build-ledger.py must stay import-free caveat.
The tolerant prefix is worth keeping. ^\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, a badge_for(severity) and the tolerant strip_badge(body) into .github/cursor-review/severity.py; import it from post-review.py directly and from build-ledger.py via the same importlib.util.spec_from_file_location idiom _load_gate_unresolved already uses; build _BADGE_RE's alternation from SEVERITY_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_RE matches badge_for(s) for EVERY key in SEVERITY_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 against main (373 tests), alongside the agents-md (46) and groom (365) suites, shellcheck -x and bash .github/bump-callers/tests/test_bump_callers.sh (437 passed). Every pin was checked by mutation, not argument: dropping nit from review-output-mcp.py's ingress tuple fails 1 test; renaming the medium key consistently across all three post-review.py tables fails 3 (it failed 1 before round 2, and none of them the dangling-DEFAULT_SEVERITY relationship); 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, pinned DEFAULT_SEVERITY's renderability (the one relationship whose breach raises KeyError rather than degrading), covered the spaceless badge shape, and corrected three over-claims in the _BADGE_RE comment: 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.

_{🔄 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/238 **Author:** [@cloud-code-bot[bot]](https://github.com/apps/cloud-code-bot) **Created:** 8/27/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `groom/github-workflows-whole-repo-github-cursor-review-33073755508-0` --- ### 📝 Commits (4) - [`5e0f0f2`](https://github.com/Comfy-Org/github-workflows/commit/5e0f0f235e5fa02cefc936db77686e3a606fe4b1) 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 - [`493a0e7`](https://github.com/Comfy-Org/github-workflows/commit/493a0e708ab3ceda6c4b24723f9ba2b61ca00bbd) test(cursor-review): pin the badge contract on the table that gates rendering - [`9a2fb6b`](https://github.com/Comfy-Org/github-workflows/commit/9a2fb6b1dc80d586ddf354b61c16e6d843f195f1) test(cursor-review): pin the ingress vocabulary and the default severity too - [`c3e8c62`](https://github.com/Comfy-Org/github-workflows/commit/c3e8c627543ae2b8b903352f74e67071aa5fa4cc) Merge branch 'main' into groom/github-workflows-whole-repo-github-cursor-review-33073755508-0 ### 📊 Changes **2 files changed** (+120 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/build-ledger.py` (+31 -0) 📝 `.github/cursor-review/tests/test_build_ledger.py` (+89 -0) </details> ### 📄 Description > 🤖 **Auto-built by the groom sweep** — this is a machine-proposed refactor. It runs full CI + cursor-review and **requires human review; it is NOT auto-merged.** Ranked from a CONFIRMED groom finding. · [run](https://github.com/Comfy-Org/github-workflows/actions/runs/33073755508) ## 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` — new `TestSeverityBadgeContract` class 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 of `post_review.SEVERITY_EMOJI` — the table `normalize_severity` gates the renderable vocabulary on, not the sort-order list — renders a finding through `post_review.normalize_comments` (the actual renderer that builds the badge), and asserts `build_ledger._strip_badge` returns 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 older `post-review.py` still 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.py` builds the badge from `SEVERITY_EMOJI` / `SEVERITY_LABEL`, and its own `strip_severity_badge` reverses 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_RE` does 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 older `post-review.py` that 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. <details> <summary><strong>Verifier rationale</strong></summary> ## Problem (verified end to end) post-review.py owns the badge: `SEVERITY_EMOJI` (line 42) and `SEVERITY_LABEL` (line 49) are formatted into `f"{SEVERITY_EMOJI[severity]} **{SEVERITY_LABEL[severity]}** — "` at line 999, and `strip_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 as `Split post-review.py's severity badge off an inline comment body`. ## Why the usual objections do not apply - **Packaging**: none. The two files sit in `.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 says `Reused, 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's `if build-ledger.py must stay import-free` caveat. - **The tolerant prefix is worth keeping.** `^\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`, a `badge_for(severity)` and the tolerant `strip_badge(body)` into `.github/cursor-review/severity.py`; import it from post-review.py directly and from build-ledger.py via the same `importlib.util.spec_from_file_location` idiom `_load_gate_unresolved` already uses; build `_BADGE_RE`'s alternation from `SEVERITY_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_RE` matches `badge_for(s)` for EVERY key in `SEVERITY_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. </details> ## Provenance **Verified:** `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` — 364 tests, OK. Green again on the MERGED result against `main` (373 tests), alongside the agents-md (46) and groom (365) suites, `shellcheck -x` and `bash .github/bump-callers/tests/test_bump_callers.sh` (437 passed). Every pin was checked by mutation, not argument: dropping `nit` from `review-output-mcp.py`'s ingress tuple fails 1 test; renaming the `medium` key consistently across all three post-review.py tables fails 3 (it failed 1 before round 2, and none of them the dangling-`DEFAULT_SEVERITY` relationship); 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, pinned `DEFAULT_SEVERITY`'s renderability (the one relationship whose breach raises `KeyError` rather than degrading), covered the spaceless badge shape, and corrected three over-claims in the `_BADGE_RE` comment: 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.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

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

bsakaguchi referenced this issue

2026-10-26 22:38:15 +00:00

[PR #304] [MERGED] refactor(pr-risk): extract the sourceable helpers into scripts/pr-risk/lib.sh #305