Conversation
Adding unit tests to the WASM crate turned up a crash: blosc_decompress
panics on a corrupt header instead of returning the empty Vec the
TypeScript layer expects.
blosc_decompress(&[0xff; 64])
-> panicked at blusc-0.0.6/src/internal/mod.rs:597:
range end index 4294967311 out of range for slice of length 64
blusc reads nbytes/cbytes straight out of the header and, on the
BLOSC_MEMCPYED path, copies that many bytes out of the source without
first checking the source is that long. It allocates the declared nbytes
up front too, so the same header asks for a 4 GB buffer on the way past.
This matters more in WASM than the stack trace suggests. A panic traps
the instance and every later call into the module fails with it, so a
single corrupt chunk does not fail one read - it breaks every subsequent
read for the life of the page.
The other two decoders have the same shape without the panic:
- lz4_flex::block::decompress zero-fills the declared original size
before it looks at a byte of the block, so a header claiming u32::MAX
is a 4 GB calloc from an 8-byte input.
- zstd::bulk::decompress reserves the frame's declared content size,
which the frame header carries as a u64.
On wasm32 all three end the same way: the allocation fails, and an
allocation failure aborts exactly like a panic does.
Each decoder now validates before delegating. The Blosc guard checks the
version byte, that cbytes fits in the buffer we were handed, that nbytes
is within BLOSC2_MAX_BUFFERSIZE, and - for memcpy'd frames - the bound
blusc itself omits. LZ4 and Zstd bound the declared size against their
own format's maximum expansion.
Those expansion bounds are measured rather than guessed, because a bound
that is too tight silently rejects valid data:
lz4_flex 16 MB of one byte -> 255.0:1 (a match-length extension byte
adds 255; guard uses 256)
zstd 64 MB of one byte -> 32498:1 (a 4-byte RLE block regenerates
a 128 KB block, so 32768:1 is
the ceiling; guard uses 32768)
Tests compress a constant buffer - which sits on that ceiling - and
assert the guard accepts it, so tightening either bound past what real
data produces fails here rather than in the field.
Also replaces an `as usize` on the 64-bit frame content size field, which
on wasm32 would wrap a >4 GiB declaration into a small plausible-looking
one instead of rejecting it.
The crate had no tests at all before this, despite rust-ci.yml running
cargo test -- --test-threads=1: the suite was passing vacuously. 28 tests
now cover the decoders' error paths, the framing each codec emits, and
the branches of the zstd frame header parser.
Verified by mutation: the blosc case was red before the fix and green
after, and reverting either size guard fails its own test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The round-trip suites cannot show that this package is format-compatible
with numcodecs. Encode and decode could both drift off the format
together and every assertion would still pass, because nothing in CI read
a buffer this package did not also write.
test/fixtures now holds chunks encoded by Python numcodecs 0.16.5, and
interop.test.ts decodes each one and compares it to the source array.
Coverage is every codec this package exports plus four Blosc
compressor/shuffle combinations, including bitshuffle - the filter most
likely to break silently, since a wrong element width still yields output
of exactly the right length.
Each codec is built from the get_config() numcodecs itself recorded,
which is the path a Zarr reader takes from stored chunk metadata rather
than a hand-written config that could drift from it. A final test asserts
the fixture set covers every exported codec, so a new codec cannot land
without one.
Only the decode direction is pinned. Encoded output is deliberately not
compared byte for byte: a compressor may emit different bytes across
versions while staying readable, so that assertion would fail for reasons
that say nothing about compatibility.
fixtures/generate.py regenerates the set through `uv run --with
numcodecs`, so reproducing them needs no local install. fixtures/README.md
records why refreshing existing fixtures to clear a failure defeats the
purpose - it discards the evidence of the break.
Verified by mutation: flipping one byte in lz4.bin fails that fixture's
test and nothing else.
Also covers two contracts the codebase states but never asserted:
- decode(data, out) writing into the caller's buffer and returning that
same object, for the three WASM-backed codecs. GZip and Zlib return
fflate's own buffer instead; that matches numcodecs.js, so it is
pinned as deliberate rather than corrected.
- the package.json export map agreeing with both src/index.ts and the
vite entry list. A codec missing from vite.config.ts advertises a
subpath that resolves to a file that was never built, which otherwise
only surfaces after publishing.
Suite goes 24 -> 40 tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gives the repository a front door: what the package is, the five codecs
and the backend behind each, install and quick start, how the WASM gets
built, and a command table.
AGENTS.md is the map for anyone working in the repo - module layout, what
to read for a given task, and the conventions that are not visible from
the code alone: the drop-in contract with numcodecs.js, the byte formats
that must not change silently, the lazy WASM load that keeps the pure-JS
codecs free of it, the empty-Vec error convention across wasm_bindgen,
and the rule that sizes read out of a compressed header are untrusted.
CODE_OF_CONDUCT.md is Builder's Code v1.0, dedicated to the public domain
under CC0.
Claims were checked against the code rather than assumed, which turned up
four that would have shipped wrong:
- "the round-trip suites assert the formats" - they assert symmetry,
which survives both directions drifting off the format together. Now
points at the interop fixtures, which do assert interchange.
- "decode(data, out?) must honor the out buffer" - written as a blanket
rule, but GZip and Zlib ignore out and hand back fflate's own buffer.
That matches numcodecs.js exactly, so it is now documented as
deliberate parity rather than quietly changed.
- "a new codec touches four places" - it is five; the list omitted
vite.config.ts, without which the advertised subpath export resolves
to a file that was never built.
- the command table advertised Rust unit tests for the WASM crate that
did not exist. They do as of the fix two commits back, which is what
made the claim worth keeping.
The logo is a compressing-buffer motif in two variants, selected by
GitHub's <picture> colour-scheme switch; both were rendered and checked
rather than assumed to work. Note the paths are repo-relative, so the
logo resolves on GitHub but not necessarily on npmjs.com, where docs/ is
outside the published `files` allowlist.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe change adds pre-decompression validation for three WASM codecs, compatibility and interoperability tests, fixture tooling, package export checks, and repository documentation. ChangesCodec validation and compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
test/index.test.ts (1)
54-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare normalized paths instead of a suffix regex.
Line 55 checks only the
src/${codecId}.tssuffix. A wrong-root path can pass. Theresolvecalls invite.config.tsproduce backslash-separated paths on Windows, so the current regex rejects a valid entry there.Build the expected source path with
fileURLToPathandresolve, normalize both paths, and compare them exactly. If a regex remains, escapecodecId.Please confirm whether the test suite intentionally supports POSIX paths only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/index.test.ts` around lines 54 - 55, Update the entry-path assertion in the test around entries[codecId] to build the expected source path with fileURLToPath and resolve, normalize both the expected and actual paths, and compare them exactly rather than using a suffix regex. Preserve cross-platform behavior, including Windows separators; do not assume POSIX-only paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 113-125: Update the Key dependencies table to distinguish Python
numcodecs, documenting version 0.16.5 for fixture generation, from numcodecs.js,
documenting its actual development dependency version for the benchmark
baseline. Align the descriptions with the README terminology and avoid leaving
the ambiguous single numcodecs 0.3 entry.
In `@CODE_OF_CONDUCT.md`:
- Around line 35-37: Update the License section in CODE_OF_CONDUCT.md to clarify
that the CC0 dedication applies only to the Code of Conduct text, while
preserving the repository’s MIT license statement elsewhere.
In `@crates/rumcodecs-wasm/src/lib.rs`:
- Around line 158-168: Update lz4_decompress and the corresponding Zstd WASM
wrapper so decoder failures remain distinguishable from valid zero-length
frames, while preserving the no-panic empty-Vec failure contract. In the
TypeScript wrappers, validate the decoded result against the expected size
before returning it or copying into out, and throw an Error for corrupted
non-empty input instead of returning empty or stale data. Add compatibility
coverage for corrupted input with and without out for both codecs.
- Around line 177-201: Lower the default allocation limit used by
zstd_decompress and require an explicit opt-in before permitting larger declared
output sizes. Update zstd_declared_size_is_plausible and the zstd_decompress
call path so untrusted input cannot request the current large capacity, while
preserving decompression for inputs within the lower limit.
In `@README.md`:
- Around line 106-114: Add the `text` language identifier to the
repository-structure fenced code block in the README, changing only the fence
declaration while preserving its contents.
- Around line 22-29: Update the README interoperability claim to match the
currently verified behavior by narrowing it to Python-generated chunks decoding
in JavaScript, unless you add corresponding Python tests and fixtures that
validate JavaScript-generated output can be decoded by Python.
In `@test/compat.test.ts`:
- Around line 79-85: Update the out-buffer decode test to retain the supplied
Uint8Array in an out variable, pass it to codec.decode, and assert that the
returned decoded buffer is not the same object as out while preserving the
existing byte-content assertion.
In `@test/fixtures/generate.py`:
- Around line 3-8: Pin the fixture generator’s numcodecs dependency to version
0.16.5 in the command documented by the fixture generator instructions, and
apply the same pin in test/fixtures/README.md. Keep the existing fixture-refresh
guidance unchanged.
In `@test/index.test.ts`:
- Around line 41-57: Update the tests around the “every codec has a matching
subpath export” and “every subpath export is built by vite” cases to compare
sorted codec ID sets bidirectionally across CODECS, codec-specific package
exports, and Vite entries. Exclude intentional non-codec entries such as "." and
"index", then retain the existing per-codec target assertions after the set
comparisons.
In `@test/interop.test.ts`:
- Around line 21-27: Update the interoperability coverage around REGISTRY to
derive the expected codec set from the constructors exported by src/index.ts
rather than a separate handwritten manifest. Compare registry IDs against those
exports and require a corresponding Python fixture for every exported codec, so
newly exported codecs cannot be omitted from coverage.
---
Nitpick comments:
In `@test/index.test.ts`:
- Around line 54-55: Update the entry-path assertion in the test around
entries[codecId] to build the expected source path with fileURLToPath and
resolve, normalize both the expected and actual paths, and compare them exactly
rather than using a suffix regex. Preserve cross-platform behavior, including
Windows separators; do not assume POSIX-only paths.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3d341f2-ba53-4a98-8c67-839440c16ec1
⛔ Files ignored due to path filters (11)
docs/assets/rumcodecs-logo-dark.svgis excluded by!**/*.svgdocs/assets/rumcodecs-logo.svgis excluded by!**/*.svgtest/fixtures/blosc-blosclz-noshuffle.binis excluded by!**/*.bintest/fixtures/blosc-lz4-shuffle.binis excluded by!**/*.bintest/fixtures/blosc-zlib-shuffle.binis excluded by!**/*.bintest/fixtures/blosc-zstd-bitshuffle.binis excluded by!**/*.bintest/fixtures/gzip.binis excluded by!**/*.bintest/fixtures/lz4.binis excluded by!**/*.bintest/fixtures/source.u2.binis excluded by!**/*.bintest/fixtures/zlib.binis excluded by!**/*.bintest/fixtures/zstd.binis excluded by!**/*.bin
📒 Files selected for processing (10)
AGENTS.mdCODE_OF_CONDUCT.mdREADME.mdcrates/rumcodecs-wasm/src/lib.rstest/compat.test.tstest/fixtures/README.mdtest/fixtures/fixtures.jsontest/fixtures/generate.pytest/index.test.tstest/interop.test.ts
Every decoder here reports failure the same way — an empty Vec, which the TypeScript layer turned into a thrown Error. A frame that legitimately holds zero bytes decodes to exactly the same thing, so the two were never actually distinguishable, and the failure mode was silent: with `out` supplied, a failed decode wrote nothing and handed the caller back their own untouched buffer, which reads as data rather than as an error. Each wrapper now compares the result against the length the frame itself declares — `blosc_declared_size`, the 4-byte LE header in src/lz4.ts, and `zstd_declared_content_size` (the latter two added here). Only a mismatch is an error. This also fixes a live bug in Blosc: `encode` of an empty buffer emits a valid 32-byte frame declaring nbytes=0, and `decode` of that frame threw "decompression failed" on a correct result. Reading the raw `blosc_cbuffer_sizes` would not have been enough, since it reports zeroes for garbage too — the sanity check has to run first. The scope of the check is truncation and short decodes. A Blosc1 frame carries no checksum over its payload, so corruption that still parses can decode to the declared length and be returned as valid; that limit is pinned in a test rather than left as an assumption. Separately, the size guards added earlier bound what a header may claim but not what the allocator can supply, and an infallible `vec![0; n]` turns that shortfall into `handle_alloc_error` — which aborts the module exactly like the panic this all exists to prevent. LZ4 and Zstd now reserve through `try_reserve_exact`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both checks walked the source list and looked for a matching entry elsewhere. That catches a codec that was added and never exported, but not the reverse: a `./old` export and an `old` vite entry left behind after a codec is removed keep advertising a subpath nothing backs, and every one-way check still passes. Verified by adding exactly that pair — green before, red now. The interop registry had the same shape of gap for a different reason. It was written out by hand and compared against the fixture manifest, so a codec missing from both agreed with itself and passed. It is now derived from the package's own exports, which makes a new codec fail until it has a fixture. Both derivations are themselves pinned against the expected five ids, since a scan that silently found nothing would make every assertion built on it vacuously true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…roducible `uv run --with numcodecs` resolved whatever release was current, so adding a single fixture would rewrite every existing buffer against a newer numcodecs — the silent refresh the README warns against, performed as a side effect. The version is now pinned to 0.16.5, and the generator refuses to run under a different one so bumping it is a deliberate edit. Pinning alone was not enough to make regeneration verifiable: a gzip member stores the current time in its header, so `gzip.bin` differed on every run regardless of whether anything had changed. That buries a real format change in noise and means the buffer cannot be re-derived to check it. The field is advisory and decoders ignore it, so it is zeroed the way `gzip -n` does — the compressed payload is untouched Python output. Two consecutive runs now produce byte-identical fixtures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- README claimed chunks decode "and vice versa", but only the Python to JavaScript direction is tested; asserting the reverse would mean running Python in CI. Narrowed to what the fixtures actually pin. - CODE_OF_CONDUCT dedicated "this work" to CC0 while the repository is MIT. Scoped the dedication to the Code of Conduct text, so it stays reusable without appearing to relicense the project. - AGENTS.md listed one `numcodecs` at 0.3, conflating two different packages: numcodecs.js (npm, the benchmark baseline) and Python numcodecs (PyPI 0.16.5, which wrote the fixtures). Split into separate rows. - Recorded the empty-frame and fallible-allocation conventions, and tagged the repository-structure fence as `text` (MD040). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three commits: a crash fix in the WASM decoders, the tests that found it, and the docs this branch originally set out to add.
Why
The branch started as documentation only — README, contributor guide, code of conduct. Verifying each documented claim against the code turned up four that were wrong, and writing the Rust tests the README already advertised turned up a crash.
The fix —
fix(wasm): bound the sizes read out of compressed headersblosc_decompresspanicked on a corrupt header instead of returning the emptyVecthe TypeScript layer converts into a thrownError:bluscreadsnbytes/cbytesstraight out of the header and, on theBLOSC_MEMCPYEDpath, copies that many bytes out of the source without first checking the source is that long.This matters more in WASM than the stack trace suggests: a panic traps the instance, and every later call into the module fails with it. One corrupt chunk does not fail one read — it breaks every subsequent read for the life of the page.
The other two decoders have the same shape without the panic:
lz4_flex::block::decompressu32::MAXis a 4 GB calloc from an 8-byte inputzstd::bulk::decompressu64On wasm32 all three end the same way: the allocation fails, and an allocation failure aborts exactly like a panic does.
Each decoder now validates before delegating —
blosc_header_is_sane,lz4_declared_size_is_plausible,zstd_declared_size_is_plausible.The expansion bounds are measured, not guessed
A bound that is too tight silently rejects valid data, so both ceilings were measured against the real encoders:
Tests compress a constant buffer — which sits on that ceiling — and assert the guard accepts it, so tightening either bound past what real data produces fails here rather than in the field.
Also replaces an
as usizeon the 64-bit frame-content-size field, which on wasm32 would wrap a >4 GiB declaration into a small plausible-looking one instead of rejecting it.The tests —
test: pin numcodecs interop with fixtures written by PythonTwo gaps:
The Rust crate had no tests at all, despite
rust-ci.ymlrunningcargo test -- --test-threads=1— the suite was passing vacuously. Now 28, covering the decoders' error paths, the framing each codec emits, and the branches of the zstd frame-header parser.Nothing in CI read a buffer this package did not also write. Round-trip suites assert symmetry, which survives encode and decode drifting off the numcodecs format together.
test/fixtures/now holds chunks encoded by Python numcodecs 0.16.5, andtest/interop.test.tsdecodes each one — every exported codec plus four Blosc compressor/shuffle combinations, including bitshuffle, the filter most likely to break silently since a wrong element width still yields output of exactly the right length.Each codec is built from the
get_config()numcodecs itself recorded, which is the path a Zarr reader takes from stored chunk metadata rather than a hand-written config that could drift from it. Only the decode direction is pinned — encoded output is deliberately not compared byte for byte, since a compressor may emit different bytes across versions while staying readable.fixtures/generate.pyregenerates the set throughuv run --with numcodecs, so reproducing them needs no local install.Also covers two contracts the codebase stated but never asserted:
decode(data, out)writing into the caller's buffer and returning that same object, and thepackage.jsonexport map agreeing with bothsrc/index.tsand the vite entry list — a codec missing fromvite.config.tsadvertises a subpath that resolves to a file that was never built, which otherwise only surfaces after publishing.The docs —
docs: add README, contributor guide, and code of conductFour claims were checked against the code and corrected before they shipped:
"the round-trip suites assert the formats"— they assert symmetry. Now points at the interop fixtures, which do assert interchange."— GZip and Zlib ignoredecode(data, out?)must honor the out buffer"outand hand back fflate's own buffer. That matches numcodecs.js exactly, so it is documented as deliberate parity rather than quietly changed."a new codec touches four places"— it is five; the list omittedvite.config.ts.Verification
pnpm checkclean, wasm32 compileslz4.bin— fails that fixture and nothing elseNotes for review
docs/sits outside the publishedfilesallowlist.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests