Skip to content

basen: decouple normalization proof from cbase32 - #1717

Merged
sergey-shandar merged 5 commits into
mainfrom
codex/fix-an-important-open-issue-from-todo-files
Aug 26, 2026
Merged

basen: decouple normalization proof from cbase32#1717
sergey-shandar merged 5 commits into
mainfrom
codex/fix-an-important-open-issue-from-todo-files

Conversation

@sergey-shandar

Copy link
Copy Markdown
Contributor

Motivation

  • Avoid duplicating cbase32's Crockford alphabet/normalizer in the basen proof so the generic baseN tests remain self-contained and do not silently validate a copied codec implementation.

Description

  • Replace the duplicated Crockford test codec with a small synthetic normalizer in fjs/basen/proof.f.mjs and remove the completed fjs/basen/todo/proof-crockford-copy.md todo file.

Testing

  • Ran npx tsc and the FunctionalScript test runner (node ./fjs/module.mjs test / fjs test), and all tests passed (3396 passed, 0 failed); cargo clippy and cargo fmt -- --check also passed. Changelog: none

Codex Task

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
functionalscript 6dc219c Commit Preview URL

Branch Preview URL
Aug 26 2026, 06:25 PM

@o2alexanderfedin o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deletion costs nothing, and I checked that the hard way rather than by reading the diff. Same four mutations run on head and base, one at a time, full npm test each: normalization made a no-op, folding reversed, one alias broken, case folding dropped. Failure sets are byte-identical between head and base — 3387/9, 3298/98, 3394/2, 3388/8. Zero coverage regression. Proof keys are the same 9 leaves at both revs and totals are 3396/0 either way.

The interesting part is what that shows about the old assertions. At base, breaking cbase32's real normalizer did not fail fjs/basen/proof.f.mjs at all — the removed I→1/l→1/o→0 asserts were exercising the proof file's own local copy of the fold table, with zero production coverage. Which is exactly what the todo said. All three have character-for-character survivors in cbase32/proof.f.mjs:65-71 caseInsensitive, and mutants C and D confirm those have real kill power.

Decoupling is real: head imports only ../asserts, ../types/bit_vec and ./module.f.mjs, no path to cbase32 direct or transitive. The new synthetic hex codec earns its place — mutant A kills through basen/proof.normalizeHit, so the replacement pins a live mechanism rather than a copy. All four Crockford properties stay pinned. The deleted todo implements its option 2 verbatim, both tasks done, no dangling links.

One thing to fix:

Changelog: none is glued to the end of a prose bullet rather than being a section:

### Testing

- Ran `npx tsc` and the FunctionalScript test runner (…), and all tests
  passed (3396 passed, 0 failed); `cargo clippy` and `cargo fmt -- --check`
  also passed. Changelog: none

------
[Codex Task](…)

Positionally it is last before the trailer, but CONTRIBUTING.md:186-202 wants a section. Squashed, the commit body reads "…also passed. Changelog: none". Breaking it onto its own line fixes it. The content none is right and correctly no changelog/unreleased/1717.md was added — this is proof-only.

Minor, not blocking: normalizeMiss ('z' → null) is now a near-duplicate of decodeInvalid ('z' → null), differing only in exercising the normalized toIndex branch. At base the 'u' case carried Crockford meaning; now it carries little.

Gates: npm test 3396/0 at head and base, tsc --noEmit exit 0, purge matched nothing.

@o2alexanderfedin o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The strengthening is real, and I checked it the only way that settles it.

The new row is genuinely distinct from decodeInvalid. That one feeds 'z' through the non-normalizing hex codec — a character absent from the alphabet before any transform. normalizeMiss now feeds 'y', which the synthetic normalizer rewrites to 'z', and 'z' is absent from the alphabet. So it covers the case where normalization succeeds but its output misses the lookup. The old form was the same pre-normalization miss, since 'z'.toLowerCase() is 'z' and the normalizer was a no-op on it.

The mutation that separates them is the bug the row exists to refute — "if the normalizer rewrote the character, trust that the result is in the alphabet":

const n = normalize(c)
return n === c ? alphabet.indexOf(c) : Math.max(alphabet.indexOf(n), 0)

At head that gives 3399/1 with normalizeMiss as the sole failure. With the row reverted to its pre-01330c291 form and the same mutation applied: 3400/0, clean pass — invisible to the entire suite, cbase32 and base64 included. So the assertion buys coverage nothing else had. Flipping its expectation also fails naming itself, so it isn't dead weight.

One thing still open, unchanged from last time: Changelog: none is glued to the tail of a prose sentence inside ### Testing, not its own section. Verbatim:

  • Ran npx tsc and the FunctionalScript test runner …, and all tests passed (3396 passed, 0 failed); cargo clippy and cargo fmt -- --check also passed. Changelog: none

Positionally it is last before the ------/Codex-Task trailer, so a scraper keying on position may still find it — but the squash commit will read "…cargo fmt -- --check also passed. Changelog: none". One line break fixes it.

Minor drift, not a defect: the body says 3396 tests, head runs 3400.

Gates: npm test 3400/0, tsc --noEmit exit 0, purge matched nothing. Measured at 535c5cbdb; the head has since moved to 6dc219c83, which is a merge of main only (nanvm-lib Rust, #1699) with nothing under fjs/, so everything above still applies.

@sergey-shandar
sergey-shandar added this pull request to the merge queue Aug 26, 2026
Merged via the queue into main with commit 5b1e1ee Aug 26, 2026
19 checks passed
@sergey-shandar
sergey-shandar deleted the codex/fix-an-important-open-issue-from-todo-files branch August 26, 2026 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants