Compare Vetted Reviewer Markers Without Regard to Letter Case - #1135
Conversation
A review body carrying `### Reviewed Changes` reported as a shape this script had never seen, while `### Reviewed changes` sat in `VETTED_HEADINGS`. Case carries no meaning in any of these markers, so a one-letter drift blocked a review loop on a round every reader had parsed correctly, `coverage=full` among them, since `SUPPRESSED`, `CR_OUTSIDE_DIFF` and `REFUSAL` are all case-insensitive already and only the membership test was not. A vetting list whose entries stop matching for a reason that means nothing is the silent narrowing the verification rules name. It arrived loudly here, which is better and still wrong, and the cost of that shape is an override asked of the maintainer that nothing warranted. `unvetted()` folds at the membership test for all three lists, with a case per list so a one-arm revert cannot stay green. `normal()` is unchanged, because the report strings carry its value and a reported shape's remedy names the shape beside the body it quotes, so a folded name would not match the body printed next to it. Its docstring, the comment above the three lists, and the `scripts/README.md` paragraph each stated the comparison as a closed set that no longer held. That paragraph also carried a heading count correct when it was written and stale from the eighth entry onward, which is the count a maintainer reads when deciding whether a reported marker is a drift or a missing entry. Closes #1132
PR Summary by QodoCompare Vetted Reviewer Markers Without Regard to Case
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe review script preserves marker casing for reports while comparing vetted headings, summaries, and metadata labels case-insensitively. Tests cover case variations and unknown markers. The README updates the vetted heading count from seven to nine. ChangesReview marker vetting
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change makes vetted review markers insensitive to letter case while preserving reporting and unknown-marker detection; no actionable merge-blocking risk remains, so it is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes implement case-insensitive matching for vetted headings, summaries, and labels while preserving reported marker casing. Regression tests verify both recognized case variations and genuinely unknown markers, satisfying issue Full details: Out of Scope Changes checkExplanation The changes remain within the linked issue scope. Documentation, comments, behavior-preserving summary normalization, and regression tests support the marker-comparison fix. The body-count discrepancy is explicitly deferred and is not included as an unrelated change. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, consistent across all vetted inventories, and has targeted regression tests protecting both the fix and the “unknown marker” floor.
Pull request overview
Updates scripts/pr_review.py’s “unrecognized shape” detection to treat vetted structural markers as case-insensitive, preventing review-loop blocks caused solely by heading/label case drift in reviewer output.
Changes:
- Add
unvetted()helper to compare normalized markers against vetted inventories usingcasefold()(leavingnormal()’s output unchanged for reporting clarity). - Route heading/summary/label membership checks in
unrecognized_in()throughunvetted(). - Add regression tests for case-drift acceptance and “unknown marker still reports”, and update
scripts/README.mdto document the case-folding behavior.
File summaries
| File | Description |
|---|---|
| scripts/pr_review.py | Adds case-insensitive vetted-marker membership via unvetted() and uses it for all three vetted inventories. |
| scripts/tests/test_pr_review.py | Adds regression coverage for case drift not blocking and unknown markers still being reported. |
| scripts/README.md | Documents that letter case is folded at comparison time for the vetted inventory checks. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The docstring narrated the incident, cited the silent-narrowing rule, and explained where the fold sits, which is a historic narrative and a rule citation in a place the fleet's comment rules keep for the behavior contract. It is now one line, and the two-line why that survives sits inside the function beside the comparison it explains, matching where `unrecognized_in` keeps its own. The `normal` addition loses the same restatement. That why names no count. "The three report strings" would have been the one stale-able claim left in it, in the same commit that fixes a heading count stale from the eighth entry onward. The reviewer also read three prose lines in the new test comment, which is one past what a constraint earns.
|
Recording the disposition of Qodo's four findings, since it resolved its own threads after the fix push and the exchange would otherwise be legible only from resolved threads. All four judged real against the repository's own rules rather than accepted on assertion, and all four fixed in
Those two have one fix. The docstring is now a single line carrying the contract, and the two-line why that survives sits inside the function beside the comparison it explains, matching where
Worth noting for the record that the fourth would have been a decline on mechanism rather than on precedent, in case the same finding recurs on a docstring that genuinely needs more than one line. |
There was a problem hiding this comment.
🔵 Needs a closer look
The README’s measured-corpus count and a small inefficiency in summary normalization should be corrected to keep documentation and implementation consistent and avoid avoidable work.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
scripts/pr_review.py:1129
- Avoid calling normal(s) twice in the VETTED_SUMMARIES check. In the current comprehension, normal(s) is computed once for unvetted() and again for the rendered string, which is unnecessary work and makes the logic slightly harder to follow.
found += [
f"summary: {normal(s)}"
for s in dict.fromkeys(SUMMARY.findall(plain))
if unvetted(normal(s), VETTED_SUMMARIES)
]
scripts/README.md:186
- Keep the measured-corpus count consistent across scripts/README.md and scripts/pr_review.py/tests. This paragraph still refers to 332 bodies, while pr_review.py and the unit-test docstring both describe the same measurement as 333, which makes it unclear which number is authoritative.
The vetted inventory is measured rather than imagined, and it is small because the output is regular. Across those 332 bodies, with fenced blocks dropped and text reduced to ASCII, the whole corpus is nine headings, six `<summary>` texts and three metadata labels. Counts are normalized to `(N)` and the verdict headings' colored circle is dropped before comparing, since both change on every review without the section having changed, and dropping the emoji is also what keeps this repository's charset rule satisfied. Letter case is folded at the comparison rather than in the reduction, so a marker keeps the case the reviewer wrote while a drift of case alone stops blocking a loop, which it did once, on a heading already in the inventory. A body carrying **no** heading at all is itself unrecognized, which is what catches a rewrite that changes every marker at once, and a **refusal is exempt** because it is a bare paragraph by design and `REFUSAL` is its vetted spelling. That exemption is the pattern rather than a carve-out, so a refusal reworded stops being exempt and blocks, which is the refusal check's own failure mode caught one rewording later. The last reading is the quietest: a reviewer **login** that reads as this reviewer without being the spelling every query filters on, since a rename leaves every filter matching nothing and the digest then reports a review that landed as no review at all. A case runs the whole inventory over the measured corpus, where it raises nothing.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
The summaries arm computed `normal(s)` for the membership test and again for the string it renders. Hoisting it into a generator leaves the deduplication on the raw text, where it was, so a marker pair that differs only in what `normal` strips still reads as two rather than collapsing to one.
|
Answering the two suppressed findings from the round on
Real, fixed in The deduplication deliberately stays on the raw text, where it already was, so this is a refactor rather than a behavior change. Verified by running the old and new comprehensions side by side over ten inputs, including three pairs that differ only by a count, only by a stripped emoji, and only by collapsed whitespace, each pair normalizing to one value. Identical output in content and order, zero mismatches.
Real, and deferred to #1136 rather than fixed here. It was declared out of scope in this pull request's description before the review ran, for the reason the issue now records: settling it is a re-measure rather than an edit, since picking whichever number appears more often would assert a corpus size nobody counted. The issue enumerates all twelve occurrences by file, three saying 332 and nine saying 333, because a partial fix is exactly how the vetted-heading count in that same README paragraph survived two releases stale, which is what this pull request fixes. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, consistent with existing case-insensitive parsing, and is covered by targeted regression tests.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
What happened
scripts/pr_review.py statusandwaitreportedshapes=UNRECOGNIZEDon PR #1125, on a round that was clean by every other measure:review_on_head=yes,unresolved=0,merge=CLEAN,checks=8/8.The shape it could not place:
### Reviewed changesis the fifth entry inVETTED_HEADINGS. The only difference is theC.The defect
unrecognized_in()compares each marker against the three vetted lists.normal()reduces a marker to ASCII, collapses whitespace and rewrites a count as(N), but does not fold letter case, so a heading drifting by one letter's case reads as a shape never seen even though that exact section is already vetted.Every reader in the file was already case-insensitive:
SUPPRESSED,CR_OUTSIDE_DIFFandREFUSALall carryre.IGNORECASE, and the coverage reader parsed the file table correctly on that round and reportedcoverage=full. Only the membership test was not.An unrecognized shape blocks a review loop by rule, because every other field is then a reading of output the script does not fully understand. That rule is right and is unchanged here. What it cost was an override asked of the maintainer that nothing warranted, which is the shape
GOVERNANCE.md"Verification Discipline" names: a vetting list whose entries stop matching for a reason that carries no meaning. It arrived loudly rather than quietly, which is better and still wrong.The change
unvetted(marker, vetted)compares case-folded, and all three membership tests route through it.normal()is unchanged, deliberately: the report strings carry its value, and a reported shape's remedy names the shape beside the body it quotes, so a folded name would not match the body printed next to it.Two tests, one per behavior.
test_a_vetted_marker_survives_a_change_of_letter_casecarries a case per vetted list, andtest_a_genuinely_unknown_marker_is_still_reportedis the floor under it, since folding could have turned the check off. Both sit inTestUnrecognizedShapes, the class whose docstring owns the inventory contract.Three surfaces stated the comparison as a closed set that no longer held, and each now names where the fold happens:
normal()'s docstring, the comment above the three lists, and thescripts/README.mdvetted-inventory paragraph. That paragraph also carried a heading count correct when written and stale from the eighth entry onward, which is the count a maintainer reads when deciding whether a reported marker is a drift or a missing entry.Verification
Reverting each of the three arms to
x not in VETTED_*independently fails the suite, and in each case the single named failure istest_a_vetted_marker_survives_a_change_of_letter_caserather than another test tripping. Stubbingunvettedto returnFalsefails nine tests including the floor, so the fold cannot be turned off silently either.Five local review rounds before this was pushed, all five on the five-line docstring rather than the code: the first four wordings of it were each false in a different way, three of them by asserting something about the report that
normal()contradicts two functions up.Full gate set green: ruff, mypy, 963 unittest cases,
build_dist.py --check,repo_gate.py,prose_lint.py,spec/validate.py, all seven Docker linters.Not in scope
scripts/README.mdsays "332 bodies" where the code says 333, consistently across both files rather than as a typo, so settling it is a re-measure rather than an edit and belongs in its own change.Closes #1132
Summary by CodeRabbit
Bug Fixes
Documentation
Tests