basen/cbase32: decode max-length vectors - #1710

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 95dcaac4c0

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa103b3e9a

Fix is correct — I could not break it. The boundary reproduces on the merge-base: payloads of 1048575 and 1048576 bits (209716 chars) return a silent null, and cBase32ToVec rejects vecToCBase32's own output at those two lengths. Head decodes both, and max+1 (1048577) still returns null, so the bound was replaced, not removed. 371 round-trip cases and a 110,080-case base-vs-head differential over lengths ≤3 show 0 divergences below the boundary.

Two things to fix:

Two mutants survive the new proofs — 3381 pass, 0 fail under both.

1. > maxLength → > maxLength + 2n survives. The decoder then accepts 1048577- and 1048578-bit payloads, returning Vecs that break the module's own maxLength invariant, and nothing fails. decodeOverflow only asserts 1048579; max+1 and max+2 are unpinned. Add assertEq(cBase32ToVec('0'.repeat(209_715) + '4'), null) (= max+1).

2. if (tail === null) return null → continue survives. That is the rejection path the backward scan actually introduced, and neither new assert reaches it: cBase32ToVec('u') and cBase32ToVec('u8') both put the invalid character at or before the sentinel. With the check skipped, '8u' decodes to a 1-bit vector instead of null. Add assertEq(cBase32ToVec('8u'), null).

Changelog: is misplaced. It sits at the end of ### Description with ### Testing after it. Per CONTRIBUTING.md it must be the last section before the trailer block — pure reorder, move it below ### Testing. Content itself is right; changelog/unreleased/1710.md exists and conforms.

Two body nits: ### Description names a test maxLengthBoundary that does not exist (the additions are decodeAtMaxLengthSucceeds and trailingZeroSymbols), and ### Testing cites 3,380 / 7 cbase32 proofs while npm test gives 3381 / 8.

Deleting base64/todo/decode-rejects-max-size-input.md is fine, not premature — base64's own encodeAtMaxLengthSucceeds/decodeAtMaxLengthSucceeds are already at the merge-base; the file was the spun-off cbase32 residual, and its task 3 explicitly permits closing. No dangling links.

One note, not blocking, not yours: vecToCBase32 always appends a full padding block via msb.concat, which has no maxLength guard (only tryListToVec does). At exactly the two lengths this PR makes decodable, the encode-side intermediate Vec is 1048580 bits — over maxLength. So decode now returns values encode cannot round-trip within contract. Pre-existing and encode is untouched here, but no todo records it any more.

Gates: npm test 3381 pass / 0 fail; npx tsc --noEmit exit 0.

Both mutants are dead. > maxLength + 1n and > maxLength + 2n are killed by decodeOverflow — one assert catching both, since '0'×209715 + '4' is max+1 and exceeds neither relaxed bound — and if (tail === null) return null → continue is killed by unterminated. Every previously-dead neighbour is still dead and nothing was displaced: >= maxLength and the merge-base revert still fail decodeAtMaxLengthSucceeds, +3n and dropping the length check still fail decodeOverflow. The head === null guard is still a tsc-kill only, unchanged. Verdicts pinned are the right ones — I recomputed max−1 / max / max+1 / max+2 / max+3 and the '8u'/'8-'/'u'/'u8'/'8' family directly and every one matches. npm test 3381/0, tsc exit 0, assert sites 21 → 23, zero deletions.

The code is done. What's left is all in the PR body, and none of it moved:

Changelog: is still misplaced — line 12, inside ### Description, with ### Testing after it and the trailer below that. It has to be the last section before the trailer block.

And its contents don't match the file. changelog/unreleased/1710.md says "decode vectors at the maxLength boundary without building an oversized sentinel-padded intermediate" as a list item; the body says "ensure decoding at the maxLength boundary succeeds and prevent intermediate Vec overflow when stripping sentinel padding" as inline prose. CONTRIBUTING wants exactly the list items, same Markdown subset — both the wording and the form differ here. I hadn't caught the wording divergence last time; only the placement.

Two nits also unchanged: line 9 still names a test maxLengthBoundary that doesn't exist (decodeAtMaxLengthSucceeds, trailingZeroSymbols, decodeOverflow), and lines 17-18 still cite 7/7 cbase32 proofs and 3,380 where npm test gives 8 and 3381.

The encode-side note from last time is left as-is, which is fine — I'd just mention todo/178.md is about moving padding into bit_vec, not about vecToCBase32's unguarded intermediate, so it doesn't record it.

Approving. The delta is merge-only — the PR's own changeset is byte-identical at both heads (same 4 files, same 44/63, identical blob SHAs), so nothing new to review there. Main moved a lot in between (#1711 restructured edag, #1712 changed rtti tuple reading, #1713 touched effects/node), so I re-ran the three mutants at the merged head: > maxLength + 1n and + 2n both still die naming decodeOverflow, and return null → continue still dies naming unterminated. Nothing displaced. npm test 3396/0, tsc exit 0.

Body findings are fixed: Changelog: is now the last section before the trailer, and its contents are character-identical to changelog/unreleased/1710.md — same two-line list item, same continuation indent, same md5. The nonexistent maxLengthBoundary is gone and the three real test names are there; the counts now read 8/8 and 3,381/3,381, matching the run you actually did.

Two things, neither blocking:

There's a blank line between Changelog: and the list item. CONTRIBUTING's example has none. Cosmetic, mentioning it only because you've been tidying these.

The encode-side residual is still unrecorded anywhere. vecToCBase32 is unchanged and still concatenates a full padding block with no maxLength guard, so at exactly the two lengths this PR makes decodable the intermediate Vec is over the limit. todo/178.md doesn't cover it — zero matches for maxLength or overflow. Pre-existing and out of scope for this PR; it just means nothing in the tree remembers it once this merges.