test: prove dual-decode of legacy + bin envelopes, re-pin protocol 1.1 fixture (LAB-791) - #57
Merged
Merged
Conversation
…1 fixture (LAB-791) Readers-first release for the StorageEnvelope compressed_data bin encoding (protocol 1.1, decisions/envelope-bin-encoding.md; rollout step 1 of #54): - Re-vendor tests/vectors/wire-format.json from protocol@6863495d (fixture 1.1.0: 6 legacy vectors + 6 *_bin twins) and re-pin FIXTURE_SHA256 with updated provenance. - Decode-identity through ByteStorage::retrieve() now runs against BOTH vector sets, forever; re-encode byte-identity stays pinned to the legacy set until the writer flip (which switches it to *_bin). - tests/dual_decode.rs: full reader matrix (plain Vec<u8> reader and serde_bytes reader x array/bin wire) over every pinned vector, seeded from the LAB-764 toolchain experiment, plus bin8/bin16 boundary sweep and bin32 round-trips with real LZ4 output. No src/ change; the writer still emits legacy array-of-ints. Test-only patch release so the stage-3 SDK rebuilds have a version to pin. Release-As: 0.3.1
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughProtocol 1.1 wire-format fixtures now include legacy and MessagePack ChangesProtocol 1.1 wire-format compatibility
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/dual_decode.rs`:
- Around line 106-138: Replace the bare unwrap calls in the vector test flow,
including the conversions around assert_all_readers_decode, the additional lines
in the referenced range, assert_width_tier, and the sweep loop, with contextual
expect messages that include the vector name and relevant wire/input operation.
Follow wire_format_vectors.rs conventions so every failure identifies the
triggering vector while preserving the existing test behavior.
- Around line 72-90: The dual-decode tests currently use plain Vec<u8> for
StorageEnvelope::compressed_data, so they do not validate MessagePack binary
decoding. Update StorageEnvelope and all affected test fixtures, including
tests/dual_decode.rs:72-90 and tests/wire_format_vectors.rs:215-285, to use
serde_bytes or an equivalent byte-buffer type, then retain the assertions
proving both plain and _bin envelopes decode through the old reader, new reader,
and retrieve().
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52204c60-3a5a-4eff-908f-cc2deb4a2b29
📒 Files selected for processing (4)
README.mdtests/dual_decode.rstests/vectors/wire-format.jsontests/wire_format_vectors.rs
CodeRabbit review: bare .unwrap() calls lost which-vector context on failure; align with wire_format_vectors.rs's panic-with-name convention.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Apply the FIX-FIRST expert-panel findings on cachekit-core#57 (all test-only,
no src/ change, wire bytes unchanged):
- must-fix: tighten the bin micro-regression bound from +4 to +1 B. The panel
re-derived and measured (lz4_flex 0.12.2) worst-case delta(bin-legacy) as
exactly +1 ("empty", 25->26 B); at +4 a +2/+3 bloat regression stays green --
the exact false-green this readers-first drift gate exists to catch, and the
ceiling the writer-flip step (LAB-764) will read as the micro-regression
contract.
- should-fix: pin the fixed-size bin marker asserts to == 0xc4 (bin8) in
dual_decode.rs and wire_format_vectors.rs. The 0xc4..=0xc6 range accepted any
width, so a width-selection regression emitting bin16/bin32 on a tiny payload
passed while breaking cross-SDK byte-identity. Range kept only in the
width_boundary_* tests that legitimately span widths.
- cut: drop the redundant (256..=65535)/>65535 range asserts in
width_boundary_bin16_bin32 (dominated by the 0xc5/0xc6 marker asserts -- rmp
selects width by length, so marker <=> range) and assert_width_tier's
now-unused -> usize return.
Optional cosmetic finding (per-vector [name] context on fixture hex-decode
expects) deferred: the sha256-pinned fixture already makes any decode failure a
whole-suite RED, so per-site context adds little for 7 sites of churn.
fmt + clippy -D warnings + affected integration tests all green.
Co-authored-by: multica-agent <github@multica.ai>
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
This was referenced Jul 26, 2026
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
1 participant
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.
Readers-first release for the StorageEnvelope
compressed_databin encoding — rollout step 1 of #54, per the merged protocol 1.1 RFC (protocol#44,decisions/envelope-bin-encoding.md). Test-only: nosrc/change, the writer still emits legacy array-of-ints. Ticket: LAB-791 (parent LAB-764).What
tests/vectors/wire-format.jsonre-vendored from protocol@6863495d— fixture 1.1.0, 12 vectors (6 legacy + 6*_bintwins).FIXTURE_SHA256re-pinned and the provenance comment's commit hash updated together (deliberate update per the LAB-423 precedent; the.gitattributes -textrule keeps windows runners from breaking the pin).tests/wire_format_vectors.rs: decode-identity throughByteStorage::retrieve()now runs against both vector sets, forever. Re-encode byte-identity (store()and codec-level) is filtered to the legacy set — the encoding the writer currently emits; the writer-flip diff switches that filter to*_bin. New guard test: every*_bintwin actually carries a bin marker and is field-identical to its legacy parent.tests/dual_decode.rs(seeded from the LAB-764 toolchain experiment): full reader matrix — plainVec<u8>reader (shipped today) andserde_bytesreader (post-flip) × array/bin wire — over every pinned vector, including bin wire through today's shippedretrieve()with checksum + ratio guards intact. Also asserts the writer-flip forecast:serde_bytesre-encode of each pinned*_bintwin is byte-identical to the protocol pin, so the step-3 assertion switch is guaranteed to land green. The experiment's throughput half is deliberately absent — benchmark evidence belongs to the writer-flip stage (LAB-510 harness).width_boundary_bin8_bin16sweeps real LZ4 output across the 255/256 B boundary (asserting both header widths are hit) andwidth_boundary_bin16_bin32round-trips a >65,535 B compressed envelope (bin32). Unit-level in core rather than fixture vectors: the decision record assigns fixture-level width vectors to the writer-flip PR, where the real writer can generate them natively.Release
Release-As: 0.3.1footer in the commit — test-only patch release so the stage-3 SDK rebuilds (py wheel / ts NAPI+wasm / rs bump) have a core version to pin.Gates
src/change can alter wire bytes; LAB-423/LAB-347 precedent, recorded on LAB-764). The stage-4 writer flip takes the panel.cargo fmt --check,cargo clippy --all-features -- -D warnings,cargo test --all-features,cargo test --features ffiall green.Summary by CodeRabbit
Documentation
*_bincompressed_dataencodings for protocol 1.1, including identical payload expectations and re-encoding verification.Tests
*_bintwins, and that payloads decode and re-encode correctly across MessagePack bin8/bin16/bin32 boundaries.