test: guard the runtime sets both sides depend on - #52
Merged
Conversation
PR Summary by QodoGuard cross-runtime Unicode parity for whitespace and lowercasing
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
=======================================
Coverage 87.40% 87.40%
=======================================
Files 3 3
Lines 691 691
=======================================
Hits 604 604
Misses 87 87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
🟡 Changes recommended
The new Rust drift-detector test’s predicate can miss lowercase expansions that begin with the original code point, and one new Python test comment misstates what the Rust implementation includes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR strengthens cross-implementation parity by adding explicit “drift detector” tests that pin two runtime-dependent Unicode sets (Python whitespace stripping and full-Unicode lowercasing behavior relevant to HTML-block detection), and adds corpus cases covering the only two non-ASCII characters that can fold into ASCII closing-tag needles.
Changes:
- Add Python unit tests that assert the interpreter’s
str.strip()whitespace set andstr.lower()“folds into ASCII” set remain exactly as expected. - Add a Rust unit test that asserts the same “folds into ASCII” set against Rust’s Unicode tables, and update
is_python_spacedocs with a re-measured interpreter range/date. - Add two new corpus cases covering Kelvin sign and dotted capital I behavior in HTML closing tags.
File summaries
| File | Description |
|---|---|
| tests/test_unwrap.py | Adds Python-side guards pinning str.strip() whitespace code points and str.lower() ASCII-reachable folds. |
| src/scan.rs | Updates whitespace doc comment and adds a Rust-side guard test for lowercase-to-ASCII reachability. |
| corpus/cases/a-kelvin-sign-folds-into-an-html-closing-tag/input.md | New corpus input covering Kelvin sign in a closing tag. |
| corpus/cases/a-kelvin-sign-folds-into-an-html-closing-tag/expected.md | New corpus expected output demonstrating the block closes and prose unwraps. |
| corpus/cases/a-kelvin-sign-folds-into-an-html-closing-tag/case.txt | New corpus metadata pinning the rationale and expected unwrap counts. |
| corpus/cases/a-dotted-capital-i-does-not-close-an-html-block/input.md | New corpus input covering dotted capital I in a closing tag. |
| corpus/cases/a-dotted-capital-i-does-not-close-an-html-block/expected.md | New corpus expected output demonstrating the block stays open and nothing unwraps. |
| corpus/cases/a-dotted-capital-i-does-not-close-an-html-block/case.txt | New corpus metadata pinning the rationale and expected counts. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Parity between the implementations is only as stable as the sets they take from their runtimes, and one of those moved under this tool already: argparse's negative-number matcher changed in 3.14 and nothing went red. Sweeping for the rest of that class found the transform depends on four such sets, three of which are already stated by the tool rather than by a runtime -- the digit class, the line boundaries, and now the negative-number rule. The fourth is the whitespace set, and it was half guarded. `scan.rs` writes the 29 code points out and has a detector that fails if Rust's tables move. Nothing on the Python side asserted the same list, so an interpreter that changed `str.strip()` would have diverged from a Rust that could not follow. Both sides now pin the same list against their own runtime, which is what makes them agree by construction rather than by coincidence. Measured identical on 3.10 through 3.14. The fifth is lowercasing, and it was not guarded at all. The HTML block matcher folds a whole line and looks for an ASCII needle, so a character whose lowercase contains ASCII can complete one. That set is 26 letters plus exactly two others, and it is why the fold cannot be narrowed to ASCII. The case tables do move -- 1393 code points gained a mapping by 3.11 and 1460 by 3.14 -- and none of those additions landed in this set only by luck. Both implementations now pin it. Two corpus cases cover the two characters, which is the whole of what that dependency can reach: the Kelvin sign, which folds into an ASCII needle and closes a block, and the dotted capital I, which folds to two code points and does not. The second is the one that says the fold feeds a containment test rather than an offset. Every guard was checked by mutating its expected set and watching the test name the character that moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
michen00
force-pushed
the
test/version-parity-guards
branch
from
September 5, 2026 05:09
5ca1458 to
d335ebf
Compare
The drift detector asked whether the first code point of the fold differed from the char. Python asks whether the fold differs from the char. Those part ways for a mapping that expands while keeping the original first -- `X` becoming `Xy` -- which the Rust would drop from the set and the Python would keep, so the two sides would disagree about a character neither had been told about. No such character exists today, which is exactly why the predicate has to be right before one does. Folded once and reused rather than walked twice.
`is_python_space` is pinned as the Rust equivalent of three Python things at once -- `str.strip()`, `str.isspace()`, and `\s` on a `str` pattern -- and the guard only asked the first. The matcher constants use `\s` directly, so an interpreter that moved that one without moving `strip()` would leave the guard green while the transform had already diverged from the set the Rust holds. All three are enumerated against the same expected tuple now, and the failure names which one moved. Verified by breaking each arm in turn: each reports itself and not the others. Measured identical across 3.10, 3.13 and 3.14, all three APIs, one 29-point set.
The comment called the C0 separators "the four Rust does not carry", and `is_python_space` carries them explicitly -- they are half of why it writes the set out by hand. What omits them is `char::is_whitespace`, which is the thing the sentence meant and the thing a reader of a parity guard needs named.
Owner
Author
|
/agentic_review |
Owner
Author
|
[Review-Convergence] Round 1: active
|
|
Code review by qodo was updated up to the latest commit 9d8bd88 |
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete correctness/maintainability issues in the newly added guard tests/comments (including a misnamed referenced function and avoidable full-Unicode rescans) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
The drift detector's comment credited the lowercase fold to `match_html_block_open`. No such symbol exists; the function is `match_opening_html_block`. A guard explaining why it pins a set should be traceable to the code path that depends on it, and a name that resolves to nothing breaks that trail at the first hop.
The guard has to cover the whole of Unicode to catch an addition anywhere in it, so the sweep is the cost of the test -- and it was being paid three times over for the same answer, with a fresh `chr` per code point per API and the pattern recompiled on every call. One pass now, one `chr` per code point, the pattern compiled once, the three hit sets collected side by side. Same assertions and the same failure messages, which still name the API that moved: each arm was broken in turn and reported itself. 0.45s to 0.14s.
Owner
Author
|
/agentic_review |
Owner
Author
|
[Review-Convergence] Round 2: active
|
|
Code review by qodo was updated up to the latest commit a476a98 |
There was a problem hiding this comment.
🔵 Needs a closer look
The new full-Unicode Rust/Python drift-detector sweeps introduce avoidable per-codepoint overhead (extra allocations / repeated chr()), which should be addressed to keep the test suite cost bounded.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/scan.rs:616
- This drift-detector allocates a fresh
Stringforc.to_string()on every iteration to compare againstlowered. Over a full Unicode sweep this is a lot of extra allocation and can be avoided by comparing theloweredcharacter sequence directly toc(while still correctly treating multi-code-point folds as different).
tests/test_unwrap.py:575 - In the Unicode sweep for the lowercasing drift detector,
chr(cp)is computed twice per code point (chr(cp).lower()and then!= chr(cp)). Since this runs over the full Unicode range, the extrachr()call is measurable overhead and can be avoided by storing the original character once.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Owner
Author
|
[Review-Convergence] Round 3: converged-merge-blocked
Five findings were raised and fixed across three rounds, one commit each. Two clean signals from two families now hold on the same head. |
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
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.
What changes, and why
Parity between the two implementations is only as stable as the sets they take from their runtimes — and one of those already moved: argparse's negative-number matcher changed in 3.14 and nothing went red (#30). This sweeps the rest of that class.
The transform depends on five runtime-defined sets. Three are already stated by the tool rather than by a runtime — the
\ddigit class (narrowed to[0-9]), the line boundaries (narrowed to three), and the negative-number matcher (#30). The other two are what this closes.Whitespace — was half guarded
scan.rswrites the 29 code points out and carries a detector that fails if Rust's tables move; its own comment calls it "the drift detector for that choice." Nothing on the Python side asserted the same list, so an interpreter that changedstr.strip()would have diverged from a Rust that could not follow. Both sides now pin the same list against their own runtime — they agree by construction rather than because two runtimes happen to match.The Python side checks all three APIs that share the set —
str.strip(),str.isspace(), and\son astrpattern — becauseis_python_spaceis pinned as the equivalent of all three and the matcher constants (_MATCH_LIST,_MATCH_ALPHA_LIST,_MATCH_SETEXT,_MATCH_THEMATIC) use\sdirectly. A runtime that moved\salone would otherwise leave astrip()-only guard green while the transform had already diverged. One sweep of the range covers all three, since the sweep is the cost of the guard and cannot be sampled without losing the additions it exists to catch.Measured identical on 3.10, 3.11, 3.12, 3.13 and 3.14 (sha
690d49e0bc6f, 29 code points).Lowercasing — was not guarded at all
The HTML block matcher folds a whole line and looks for an ASCII needle like
</blockquote>, so a character whose lowercase contains ASCII can complete one. That set is 26 letters plus exactly two others:This is why the fold cannot be narrowed to
to_ascii_lowercase. And the case tables do move — 1393 code points gained a lowercase mapping by 3.11, 1460 by 3.14 — so it is only luck that none of those additions landed in this set. Both implementations now pin it.Rust's tables were spot-checked locally before pinning the values, across whatever toolchains this machine had (1.86 through 1.98) — identical digests for both sets, which is why the guard could be written against the current numbers without failing on arrival. That sample was opportunistic and is not the coverage.
The coverage is CI, and it is the two versions the MSRV policy already names:
rust-testat the pinned 1.86.0 floor on three platforms, andrust-test-stableat whatever stable is on the day. Both runcargo test, so both run these guards — on this PR's last runrust-test-stableinstalled rustc 1.98.1 and executedpython_whitespace_matches_rusts_view_plus_fourandthe_characters_that_fold_into_ascii_have_not_moved. That is also why the Rust guard belongs incargo testrather than the CLI tier: the CLI tier is never run against a floating toolchain.Two corpus cases
They cover the two characters, which is the whole of what that dependency can reach:
a-kelvin-sign-folds-into-an-html-closing-tag— the closing tag folds to</blockquote>and closes the block, so the prose after it joins. An ASCII-only fold leaves the block open to end of file and joins nothing, which is what the counts separate. Verified discriminating: an ordinaryXin the same position leaves it open.a-dotted-capital-i-does-not-close-an-html-block— folds to two code points and does not match. This is the case that says the fold feeds a containment test rather than an offset; a port that folded in place and then indexed by the result would read past its own line.Every guard was checked by breaking it
is_python_space's doc comment also gains a re-measured date — it said "verified across all three on 3.10 and 3.13", and the range is now 3.10 through 3.14.Merge position
Rebased onto main after #30 landed; the append conflict in
tests/test_unwrap.pyis resolved and both tests are present. It touches nothing that #37 or #49 touch.Review rounds
Five findings were raised by Copilot and Qodo across three convergence rounds and fixed one commit each — the whitespace guard widened from one API to three (a2b31ab), the fold predicate corrected to compare the whole mapping rather than its first code point (bf164ae), the guard's range sweep reduced from three passes to one (a476a98, 0.45s to 0.14s), and two comments corrected to name what they meant (9d8bd88, 643ae81). Both reviewers are clean on the current head.
Corpus
The corpus is the specification, and both implementations answer to it. Tick what applies.
corpus/pins the new behavior. The case was written first and failed first.No behaviour changes — both implementations already did all of this. The cases and guards say so, which is the point: the parity was real and unasserted.
Checks
make checkpasses, ormake testdoes and this touches no Rust.make check— tidy, the Python suite, the 3.10 floor,rust-lint,rust-testandparity.Closes #56 (review-convergence bulletin)