Skip to content

fix: harden against confirmed review findings - #2

Merged
conorbronsdon merged 1 commit into
mainfrom
fix/hardening-review
Jul 7, 2026
Merged

fix: harden against confirmed review findings#2
conorbronsdon merged 1 commit into
mainfrom
fix/hardening-review

Conversation

@conorbronsdon

Copy link
Copy Markdown
Owner

From an automated multi-agent review (personal-context#62); implemented + verified by Claude Code.

All findings were reproduced on current source, fixed, and re-verified by building and running the suite against Mojo nightly 1.0.0b3.dev2026070506.

Fix 1 — conformance harness can never fail CI (test-integrity)

test/conformance.mojo printed a pass/total scoreboard but always exited 0. A future normalization regression would ship green under a "100%" badge. The CI workflow runs this harness as a step, so the step could never fail.

Fix: made it a real gate — exit non-zero when total_pass != total, and assert the corpus parses to exactly EXPECTED_ROWS = 20034 so a truncated/swapped NormalizationTest.txt cannot silently shrink coverage while still reporting 100%. The Unicode data is unchanged; today's run is a genuine 20034/20034 (100% still true).

Before/after evidence (injected a synthetic single-row regression into the harness logic):

scenario TOTAL old exit new exit
clean corpus (100%) 20034 / 20034 0 0
1 row fails (regression) 20033 / 20034 0 (ships green — bug) 1 (GATE FAIL: 1 row(s) failed)
truncated corpus 998 / 998 0 1 (GATE FAIL: parsed 998 ... expected 20034)

Regression test: added test_conformance_corpus_intact to test/test_unicodedata.mojo — independently re-counts the vendored corpus and asserts 20034 rows, so a coverage-shrinking data swap also fails the unit suite. No external services.

Bundled build prerequisite — pixi mojo pin

mojo = ">=1.0.0b3,<2" sorts above dev nightlies under PEP 440 (1.0.0b3.dev... < 1.0.0b3), so pixi install finds no candidates on the max-nightly channel (the channel only ships dev builds). Verified: old pin fails with No candidates were found for mojo >=1.0.0b3,<2; widened to >=1.0.0b3.dev0,<2 solves and installs cleanly. Same fix already applied in mojo-redis.

Verification

  • mojo run -I src test/test_unicodedata.mojo24 passed, 0 failed (was 23 + 1 new regression test).
  • mojo run -I src test/conformance.mojo20034 / 20034, exit 0.
  • pixi install → solves with fixed pin (failed with old pin).

Notes / parallel-agent overlap

  • Did not touch README.md or CHANGELOG.md (a separate agent is editing those in parallel PRs). The README/CHANGELOG both cite "20,034 / 20,034 rows (100%)" — that count remains accurate and is now the gate's asserted invariant.
  • No other findings were in scope for this task.

Draft — do not merge; awaiting Conor's review.

The conformance harness (test/conformance.mojo) printed a pass/total
scoreboard but always exited 0, so a future normalization regression would
ship green under a "100%" badge. Make it a real gate: exit non-zero when
total_pass != total, and assert the corpus parses to exactly 20034 rows so
a truncated or swapped NormalizationTest.txt cannot silently shrink
coverage while still reporting 100%. Data is unchanged; today's run is a
genuine 20034/20034.

Add test_conformance_corpus_intact to the unit suite as an independent
regression guard on the row count.

Also fix the pixi mojo pin: ">=1.0.0b3" sorts ABOVE dev nightlies under
PEP 440 (1.0.0b3.dev... < 1.0.0b3), so `pixi install` finds no candidates
on the max-nightly channel. Widen to ">=1.0.0b3.dev0,<2" so the build
solves against nightly (same fix already applied in mojo-redis).

Co-Authored-By: Claude <noreply@anthropic.com>
@conorbronsdon

Copy link
Copy Markdown
Owner Author

🤖 Independent Claude review: Ready to mark for review — no blocking correctness/security issue. The three fixes are each correct.

Verified

  • Corpus count: re-parsed test/data/normalization_test.txt with the harnesss exact row filter (non-empty, not @, split(";") >= 5 cols) → exactly 20034, so EXPECTED_ROWS and the unit-test assert are accurate. File is the comment-stripped 5-field variant; rows have no trailing ;, so the >= 5 filter is right.
  • Gate logic: test/conformance.mojo now exit(1) on total_pass != total and on total != EXPECTED_ROWS. .github/workflows/test.yml does run the harness as a step, so the "always-green" hole was real and is closed. New pin does not over-restrict (a future final 1.0.0b3 still satisfies >=1.0.0b3.dev0).
  • pixi pin: under PEP 440 1.0.0b3.devN < 1.0.0b3, so old >=1.0.0b3 rejected the nightly (No candidates); >=1.0.0b3.dev0 accepts it. Correct.
  • No src/ changes — public Normalizer API and Python-mirror surface untouched. No sibling always-exit-0 harness left unfixed.

Non-blocking notes

  1. The added test_conformance_corpus_intact guards the corpus-count invariant, not the headline fix. The exit-non-zero-on-regression behavior (Fix 1) is verified only by the manual before/after table in the PR body — no automated test would fail if someone reverts the exit(1) gate while the corpus is still healthy. Consider factoring the pass/total→exit decision into a testable helper.
  2. 20034 is now duplicated across conformance.mojo (EXPECTED_ROWS), the unit test, and README/CHANGELOG. A legitimate Unicode corpus bump will red-CI until all are updated in lockstep. A checksum or single-source constant would be sturdier.
  3. The unit test re-implements the harness parse loop rather than calling it, so it cannot catch a harness parser bug — it only fulfills its stated corpus-integrity purpose (fine, just noting it is not truly independent verification of the harness).
  4. The pixi pin has no effect on CI (CI installs via uv --prerelease allow, not pixi); it is a local-dev fix, correctly scoped.

@conorbronsdon
conorbronsdon marked this pull request as ready for review July 6, 2026 07:58
@conorbronsdon

Copy link
Copy Markdown
Owner Author

Code review (opus, static — verify CI): SHIP-WITH-NITS

Scope note for reviewers: this touches zero normalization logic — it's a test-gate fix + a pixi-pin fix, and the finding is legitimate. test/conformance.mojo printed a pass/total scoreboard but always exited 0, so a future normalization regression would ship green under a "100%" badge. The fix arms the gate (exit(1) when total_pass != total OR total != EXPECTED_ROWS) and adds a unit test that re-counts the corpus. Independently re-counted the vendored normalization_test.txt with the identical filter → exactly 20034, matching. Pixi pin >=1.0.0b3>=1.0.0b3.dev0 is the correct PEP-440 fix (dev nightlies sort below the release tag).

Nit: conformance.mojo:26 EXPECTED_ROWS = 20034 is version-pinned — a legitimate Unicode-data upgrade (more vectors) trips "corpus truncated/swapped" and must be bumped in lockstep. (low/maintainability) The deep normalization correctness (Hangul, canonical ordering, full-casefold) is unchanged — still covered only by the now-enforced corpus.

@conorbronsdon
conorbronsdon merged commit deed98b into main Jul 7, 2026
1 check passed
@conorbronsdon
conorbronsdon deleted the fix/hardening-review branch July 7, 2026 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant