LAB-783: StorageEnvelope compressed_data → msgpack bin encoding (protocol 1.1)#44
Conversation
… + vectors) Protocol 1.1: writers MUST emit compressed_data as msgpack bin (0xc4/0xc5/0xc6); readers MUST accept the legacy array-of-ints encoding permanently. Not a breaking change: dual-read is mutual in both directions under rmp-serde (toolchain-verified on all pinned vectors, including bin wire through the shipped ByteStorage::retrieve()); no version field or discriminator. checksum stays array-of-ints (crypto- adjacent, 1-7 B win); format untouched (feeds AAD v0x03, which contains no envelope bytes). Tiny envelopes grow <= +4 B (empty 25->26 B); incompressible payloads shrink ~35% with 6-11x codec throughput. - spec/wire-format.md: canonical + legacy byte layouts, dual-read compatibility statement, AAD independence, micro-regression note - decisions/envelope-bin-encoding.md: rationale, rejected options (versioned envelope would BE the breaking change), rollout order (readers-first as discipline), evidence - test-vectors/wire-format.json: 6 *_bin twins appended (append-only, legacy vectors byte-untouched); fixture 1.0.0 -> 1.1.0 - tools/wire-format-reference.py: stdlib generate/verify; proves codec fidelity by re-encoding legacy pins byte-identically; msgpack third-encoder conformance when importable; twins byte-verified against rmp-serde 1.3.1 + serde_bytes 0.11.19 output - verify.yml: wire-format verify added to both python steps (first protocol-side CI enforcement of this fixture)
Panel: bug-hunter-supreme, security-specialist, code-craftsman, catchphrase-agent. All three LAB-764 checkpoints CONFIRMED (AAD independence, checksum scope exclusion, format untouched). Applied: - Corrected micro-regression bound: +4 B was bad header arithmetic (bin32 never pairs with fixarray); true bound is +1 B, only for compressed_data <= 15 B with no byte >= 0x80. Fixed in spec, decision record, CHANGELOG, and the tool assert. - wire-format-reference.py fails closed on unclassifiable or duplicate-named vectors (was: silent skip in verify, silent drop in generate); ValueError now localized per-vector; impossible str8 codec branches deleted (format is always a fixstr registry token). All four corruption paths tested fail-closed. - Security Limits: decoders MUST validate declared bin/array length headers against remaining input before allocating (bin32 = single- header 4 GiB OOM for pre-allocating readers). - Implementation-status note: no shipped release emits bin yet. - bin16/bin32 width-boundary vector explicitly deferred to the implementation PR (needs real LZ4 output > 255 B; documented in the decision record as a MUST for that PR). - Prose dedupe: measured evidence consolidated into the decision record; dual-read table single-sourced in the spec; CHANGELOG trimmed to house style; fixture metadata de-normativized.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe protocol specification and decision record define canonical MessagePack ChangesEnvelope bin encoding
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CI
participant WireFormatReference
participant WireFormatFixture
CI->>WireFormatReference: run verify
WireFormatReference->>WireFormatFixture: load legacy and bin vectors
WireFormatReference->>WireFormatReference: decode and re-encode envelopes
WireFormatReference-->>CI: return verification status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 29 minutes. |
…ifier Expert-panel finding (LAB-783 crypto/protocol gate): _decode_bytes_field peeked r.buf[r.pos] directly — the one decode path bypassing the bounds-checked _Reader. A truncated/malformed vector raised a bare IndexError that verify()'s except did not catch, aborting the whole run with a raw traceback and no per-vector context. The decision record defers a bin16/bin32 boundary vector as a MUST for the implementation PR, so this decode path will receive new/adversarial input. - add bounds-checked _Reader.peek(); element[0] decode now raises the tool's own ValueError on truncation, consistent with every other field read. - widen verify()'s per-vector isolation to catch IndexError/KeyError so a malformed fixture entry fails only itself with a named FAIL line. Format-neutral by construction: no wire bytes change, all 12 vectors byte-identical, verify stays 6/6 green (stdlib + msgpack-python conformance). Only the exception type raised on already-malformed input changes.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tools/wire-format-reference.py`:
- Around line 259-346: Refactor verify() by extracting the per-vector validation
into a focused helper, such as _verify_vector(base, bins, msgpack), while
preserving all existing checks, failure isolation, output, and return behavior.
Keep fixture-level validation, failure aggregation, orphan handling, and final
reporting in verify(); have the helper perform codec fidelity, twin equivalence,
size-bound, and optional msgpack conformance checks.
🪄 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: 895ec16e-c886-439a-af29-dbd26ed83ad6
📒 Files selected for processing (6)
.github/workflows/verify.ymlCHANGELOG.mddecisions/envelope-bin-encoding.mdspec/wire-format.mdtest-vectors/wire-format.jsontools/wire-format-reference.py
CodeRabbit (Ruff PLR0915): verify() had 52 statements (>50). Extract the per-vector validation — codec fidelity, twin equivalence, size bound, optional msgpack-python conformance — into _verify_vector(base, bins, msgpack). The per-vector try/except failure isolation stays in verify(). Behavior-identical: verify still 6/6 green (stdlib + msgpack conformance), same output and same per-vector isolation; `ruff check --select PLR0915` now clean. CodeRabbit-Resolved: wire-format-reference.py:346:split verify into per-phase helpers
1 participant
LAB-783 — Protocol RFC: StorageEnvelope
compressed_data→ msgpackbinencodingImplements the RFC mandated by the LAB-764 GO decision (ray, 2026-07-25) for
cachekit-io/cachekit-core#54.
Spec change only — no implementation ships here; the writer flip is staged behind this PR on LAB-764.
What changes
spec/wire-format.md— protocol 1.1: writers MUST encodecompressed_data(element[0]) as msgpackbin(0xc4/0xc5/0xc6); readers MUST accept the legacy array-of-ints encoding permanently. Dual-read compatibility statement (mutual in both directions under rmp-serde, toolchain-verified incl.binwire through the shippedByteStorage::retrieve()), no version field or discriminator, AAD-independence statement, ≤ +1 B tiny-envelope micro-regression, implementation-status note, and a new normative rule in Security Limits (validate declared length headers before allocating). Scope exclusions explicit:checksumstays array-of-8-ints,formatuntouched (feeds AAD v0x03).decisions/envelope-bin-encoding.md— decision record: rationale, measured evidence (1.508× → 1.0039× wire, 6–11× codec throughput on 64 MiB incompressible), rejected options (a versioned envelope would itself be the breaking change), readers-first rollout order, deferred bin16/32 width-boundary vector (MUST land in the implementation PR).test-vectors/wire-format.json— 6*_bintwins appended (append-only; legacy vector objects byte-untouched, retained forever as legacy-read proof); fixture 1.0.0 → 1.1.0.tools/wire-format-reference.py— stdlib-only generator/verifier: proves codec fidelity by re-encoding the legacy pins byte-identically to the rmp_serde-generated originals; msgpack-python third-encoder conformance when importable; fails closed on unclassifiable/duplicate/tampered vectors (all four corruption paths tested)..github/workflows/verify.yml— addswire-format-reference.py verifyto both existing Python steps. Note for reviewers: this CI edit adds an enforcement gate (the wire-format fixture previously had no protocol-side verifier — LAB-423 finding); it does not touch existing checks.Verification performed
*_binvectors byte-verified against realrmp-serde 1.3.1 + serde_bytes 0.11.19output (local cross-check crate; dual-read matrix re-confirmed on the pinned bytes in both reader directions).compressed_data; (c)formatuntouched. Panel findings applied in the second commit: corrected +4 B → +1 B micro-regression bound (header arithmetic), fail-closed verifier guards, Security-Limits allocation rule, implementation-status honesty note, prose dedupe.Sequencing
Per LAB-783: authoring lands now; review/merge queues behind the LAB-244 merge wave.
sdk-feature-matrix.mddeliberately untouched (it never references element-level envelope encoding).Closes LAB-783 (Multica). Parent tracking: LAB-764.
Summary by CodeRabbit
Documentation
Tests
Changelog