LAB-903: frame-crosscheck bin support + python-frame bin twins + matrix refresh (post-0.4.0 close-out) - #47
Conversation
… refresh (LAB-903) Post-0.4.0 close-out of the envelope bin-encoding rollout (LAB-764 stage 6): - tools/frame-crosscheck.mjs: accept BOTH envelope encodings (legacy array-of-ints and protocol 1.1 msgpack bin), like every real reader. The legacy pin lives in the legacy vectors' byte-identity, not in the reader rejecting bin. Vectors may declare envelope_encoding and the reader asserts the observed encoding matches (negative-tested). - test-vectors/python-frame.json: append-only bin twin of the default-path vector, generated from the real cachekit 0.17.0 PyPI wheel (first release emitting bin envelopes) via the new generate-bin-twin mode; per-vector generator provenance recorded; legacy vectors byte-untouched. - tools/python-frame-reference.py: new append-only generate-bin-twin mode (refuses non-bin wheels, refuses to overwrite); 'generate' now carries a clobber warning since a post-0.4.0 wheel cannot reproduce the legacy vectors. - sdk-feature-matrix.md: SDK Overview versions refreshed against the registries (py 0.17.0, rs 0.5.0, core 0.4.0, ts 0.1.4); core/rs architecture notes updated with rollout state (rs/ts bumps merged on main, unreleased). - spec/wire-format.md: descriptive-only fixture-inventory update in the 'Test vectors' paragraph (no normative text touched). The bin16/bin32 width-boundary vector for wire-format.json is LAB-868 (separate, already filed).
…903) Panel findings applied (bug-hunter + security + craftsman converged): - frame-crosscheck.mjs: checksum must be array-of-ints in BOTH encodings (protocol 1.1 excludes it from the bin flip) — a bin checksum now fails instead of passing silently. Negative-tested with a hand-built frame. - python-frame-reference.py: _build_default_path_vector asserts the checksum type; 'generate' gains a runtime refusal on bin-emitting wheels (the docstring warning alone left a one-keystroke clobber of the irreproducible legacy vectors that would now pass CI); --help/-h prints the module docstring instead of exiting 2. - python-frame.json: backfill envelope_encoding=int-array on the legacy default-path vector (JSON metadata only, frame_hex byte-untouched) so the legacy-read proof is tool-enforced again post-dual-decode. - sdk-feature-matrix.md: rollout state deduplicated to one site (the cachekit-core architecture note) per the pragmatism pass.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 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 (5)
WalkthroughThe PR documents protocol 1.1 envelope encoding changes, adds a binary-envelope Python frame twin, teaches cross-check tooling to decode both representations, and adds guarded reference-vector generation with a ChangesProtocol 1.1 envelope vectors
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as generate-bin-twin CLI
participant Cachekit as installed cachekit wheel
participant Fixture as python-frame.json
participant Crosscheck as frame-crosscheck.mjs
CLI->>Cachekit: Generate and round-trip the default frame
Cachekit-->>CLI: Return bin envelope metadata
CLI->>Fixture: Append the protocol 1.1 twin
Crosscheck->>Fixture: Read expected encoding and frame values
Crosscheck->>Cachekit: Validate the decoded frame round-trip
Cachekit-->>Crosscheck: Return compressed_data and checksum bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
…errors (Kody, LAB-903) Kody review round on PR #47: - critical: every generation-path 'assert' converted to _require() (explicit raise) — assert is stripped under python -O, which would silently disable round-trip validation and let a corrupt vector into the cross-SDK source of truth. Fixed as a class (9 sites), not just the flagged line. Verified: verify passes under -O and _require still raises under -O. - high: fixture read/parse extracted to _load_fixture() with explicit OSError/JSONDecodeError handling and a clear message (used by both verify and generate-bin-twin — same class, both call sites). - low (print vs logging): deliberately not changed — print IS the user-facing output contract of this zero-dependency CLI tool; rationale on the review thread.
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@kody start-review |
|
@kody start-review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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/python-frame-reference.py`:
- Around line 416-428: Update the unsupported-mode fallback in the __main__
dispatch to terminate with a non-zero exit status after printing the existing
error and usage messages. Preserve the current behavior for -h/--help, generate,
generate-bin-twin, and verify.
🪄 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: 3d7fca2e-1ec6-446b-9e3b-64a9e7a13667
📒 Files selected for processing (5)
sdk-feature-matrix.mdspec/wire-format.mdtest-vectors/python-frame.jsontools/frame-crosscheck.mjstools/python-frame-reference.py
Expert-panel review (crypto/protocol gate, critical stakes) mutation-tested the conformance gate this PR adds and found it self-certifying: every drift it was written to catch exited 0. Twelve mutations that previously passed now fail, in both verifiers independently. - generate() refused only the post-1.1 direction, and its message named the *_bin clobber in the one branch that cannot reach the write. A pre-1.1 wheel passed the sniff, rebuilt the fixture from 3 vectors and silently deleted the protocol-1.1 twin. Replaced with a name-level check: refuse if the rewrite would drop ANY committed vector. Closes both directions, on the names present rather than on a wheel version. - envelope_encoding was opt-in (`if (env.envelope_encoding && ...)`). Since compressed_data_hex / checksum_hex / inner_msgpack_hex / original_size / format are byte-identical between a legacy vector and its bin twin, that field is the ONLY assertion that can catch an encoding drift - a vector allowed to omit it pinned nothing. Now mandatory in both tools. - No coverage floor: deleting the twin, or flipping its declaration, left the gate green while proving one encoding or neither. "Readers MUST accept legacy forever" is only proven while a vector in each encoding exists - both tools now fail unless they observe int-array AND bin. - verify(), the stdlib leg SDK authors run standalone, never decoded payload_envelope at all: it printed ok for the bin twin having proved nothing about bin, leaving this PR's entire subject asserted by the Node tool alone. It now classifies the envelope's compressed_data tag from the msgpack byte and cross-checks the declaration. The two CI legs are independent again. - decodeBin's 0xc4 length prefix was used before validation: on a truncated buffer b[at+1] is undefined, so `at + NaN > b.length` is false, the overrun guard was bypassed and the reader returned an empty field plus a NaN offset. Failed closed in-repo only via decodeDocument's trailing-byte check; ported to a reader without it, a truncated envelope yields silent empty compressed_data. bin8 is the width every sub-256-byte rmp_serde envelope actually uses. Guard added at the read via u8len(), which also covers 0xd9 (str8, same class). Protocol 1.1 rule: validate declared length headers before using them. - generate-bin-twin asserted in prose that encoding was the sole delta from the legacy vector but never checked it; a wheel that also changed LZ4 level, msgpack key order or the frame header would append a vector that lies about what it isolates. Now _require()s value, header and all five envelope fields byte-equal before appending. - format joins checksum as a named exclusion from the bin flip in both tools; it feeds AAD construction, so a drift must not surface as a type pun. - Idempotent re-run of generate-bin-twin returned 1; an append-only no-op is success. Corrected to 0, keeping 1 for the wrong-encoding refusal. spec/wire-format.md said "no shipped release emits the canonical bin encoding yet - cachekit-core v0.3.0 (current)", contradicted by this PR's own bin vector from a shipping 0.17.0 wheel: an SDK author would conclude bin never appears on the wire. Replaced with shipped status (core 0.4.0 / py >= 0.17.0 emit it; rs 0.5.0 and ts 0.1.4 still write legacy, so readers see both), stamp bumped 0.3.0 -> 0.4.0. test-vectors/python-frame.json is deliberately UNCHANGED - downstream SDKs pin it by sha256.
d64ec93
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
The coverage floor proves the bin encoding is exercised, which today means bin8 only - no committed vector exceeds 255 B compressed. Say so at the two widths rather than letting them read as covered.
Expert-panel review (crypto/protocol gate) — complete, findings appliedThe workspace's mandatory crypto/protocol gate applies to this PR: the diff changes how the ByteStorage envelope is decoded, the cross-SDK test vectors, and the conformance checker four SDKs are written against. Panel run at critical stakes (4 agents: bug-hunter-supreme, security-specialist, code-craftsman, catchphrase-agent), with the ratified Three agents independently mutation-tested the gate this PR adds, and converged on the same structural problem: the gate was self-certifying. Every drift it was written to catch exited 0. Applied (commits
|
| # | Sev | Finding | Fix |
|---|---|---|---|
| 1 | CRIT | generate() refused only the post-1.1 direction — and its message named the *_bin clobber in the one branch that cannot reach the write. A pre-1.1 wheel passed the sniff, rebuilt the fixture from 3 vectors and silently deleted the protocol-1.1 twin; both verifiers stayed green. |
Name-level check before the write: refuse if the rewrite would drop any committed vector. Closes both directions, keyed on names present rather than a wheel version. |
| 2 | CRIT | envelope_encoding was opt-in (if (env.envelope_encoding && …)). compressed_data_hex, checksum_hex, inner_msgpack_hex, original_size and format are byte-identical between a legacy vector and its bin twin — so that field is the only assertion that can catch an encoding drift. A vector allowed to omit it pinned nothing. |
Mandatory in both tools. |
| 3 | CRIT | No coverage floor. Deleting the twin — or flipping its declaration — left the gate green while proving one encoding, or neither. "Readers MUST accept legacy forever" was unpinned. | Both tools now fail unless they observe int-array and bin. |
| 4 | MAJ | verify(), the stdlib leg SDK authors run standalone, never decoded payload_envelope at all — it printed ok for the bin twin having proved nothing about bin, leaving this PR's entire subject asserted by the Node tool alone. |
It now classifies compressed_data's msgpack tag (stdlib, no dep) and cross-checks the declaration. The two CI legs are independent again. |
| 5 | MAJ | decodeBin's 0xc4 length prefix was used before validation: on a truncated buffer b[at+1] is undefined, so at + NaN > b.length is false, the overrun guard was bypassed, and the reader returned an empty field plus a NaN offset. Failed closed in-repo only via decodeDocument's trailing-byte check — ported to a reader without it, a truncated envelope yields silent empty compressed_data. bin8 is the width every sub-256-byte rmp_serde envelope actually uses. |
Guard at the read via u8len(), which also covers 0xd9 (str8, same class). This is the ratified validate declared length headers before use rule. |
| 6 | MAJ | generate-bin-twin asserts in prose that encoding is the sole delta from the legacy vector, but never checked it. A wheel that also changed LZ4 level, msgpack key order or the frame header would append a vector that lies about what it isolates — into a fixture downstream SDKs pin by sha256. |
_require()s value, header and all five envelope fields byte-equal before appending. |
| 7 | MAJ | spec/wire-format.md NOTE read "no shipped release emits the canonical bin encoding yet — cachekit-core v0.3.0 (current)" — contradicted by this PR's own bin vector from a shipping 0.17.0 wheel. An SDK author would conclude bin never appears on the wire. |
Replaced with shipped status (core 0.4.0 / py ≥ 0.17.0 emit it; released rs 0.5.0 + ts 0.1.4 still write legacy, so readers see both). Stamp bumped 0.3.0 → 0.4.0. |
| 8 | MIN | format was unguarded while checksum was, though the decision excludes both — and format feeds AAD construction, so a drift surfaced as a json.dumps TypeError rather than a named invariant. |
Named exclusion in both tools. |
| 9 | MIN | Idempotent re-run of generate-bin-twin returned 1; an append-only no-op is success. |
Returns 0; 1 retained for the wrong-encoding refusal. |
| 10 | MIN | bin16/bin32 are implemented but unexercised (no vector exceeds 255 B). |
Noted at the two cases, pointing at LAB-868. |
Verification
Twelve mutations that previously exited 0 now fail, in both verifiers independently — baseline unmutated still passes both, and the fixture was restored byte-identical after each:
- bin twin deleted → coverage floor fires · legacy vector deleted → fires
envelope_encodingomitted → mandatory-declaration fires- twin declares
int-arraywhile carryingbin→ mismatch fires - legacy vector silently regenerated to
binbytes with the declaration dropped (the panel's demonstrated case) → fires - truncated
0xc4prefix → cleanbin8 length prefix overruns buffer, noNaNbypass
Full local CI suite green: interop-reference, encryption-verify --require-seal, wire-format-reference, python-frame-reference verify, file-backend-reference, frame-crosscheck.mjs.
test-vectors/python-frame.json is deliberately unchanged — downstream SDKs pin it by sha256, so this remediation touches only the tools and the spec.
Rejected, with reasons
- Share the envelope encoders with
tools/wire-format-reference.pyand deletegenerate-bin-twin(code-craftsman, MAJ). Correct that the two tools solve one problem twice — but rewriting the fixture-writing path for an artifact four SDKs pin by sha256, in order to remove a mode that is now guarded, is the wrong trade at this tier. The catchphrase agent independently vetoed it for this PR and I agree. Worth a follow-up ticket: onegeneratethat upserts by name dissolves the second entry point, the destructive warning and the refusal branch. - Assert
compressed_data_hex/checksum_hexoccur as substrings ofexpected_payload_hex(bug-hunter, MAJ). Wrong as stated — I implemented it and it failed on the committed vectors. The legacy encoding prefixes every byte ≥ 0x80 with0xcc, so the raw hex is not a substring of an int-array payload. Reverted; finding chore: gitignore AI agent configs #4's tag classification covers the actual gap. - Bound
lz4BlockDecompress's allocation from the declaredoriginal_size(security-specialist, MAJ). Real, and genuinely the same ratified rule as docs: update SDK feature matrix for cachekit-rs v0.2.0 #5 — but pre-existing, outside this diff, and safe in-repo because the driver comparesoriginalSizeto the vector first. Picking the cap is a judgment call on a shared artifact rather than a mechanical fix, so it belongs in its own ticket, not smuggled into this one. - Cut
payload_envelope.encoding's prose ternary (catchphrase + craftsman). No verifier reads the key — but cutting it means either rewriting committed fixture bytes (breaking downstream sha256 pins) or leaving generator output diverged from the committed fixture, which is worse than the redundancy. Aesthetic benefit, real cost. - Footnote-mark the
cachekit-corerow in the SDK Overview table (craftsman, MIN).²already carries two different meanings in the backends and compliance tables; adding a third in a table with no footnote block would collide. The overview lists SDK versions, not the core version each pins, so it isn't making the claim. The genuinely wrong status claim was chore: add org-canonical .coderabbit.yaml #7, and that is fixed. - Trim the "Last updated" changelog enumeration (catchphrase). Every prior LAB entry follows that convention in this file; changing it isn't this PR's job.
Panel-confirmed clean, for the record: AAD-neutrality holds provably — legacy and bin twin are byte-identical on every AAD input (header format/compressed/original_type, envelope format token, checksum, original_size); only field 0's msgpack tag differs, and AAD v0x03 carries no payload-derived bytes. Decision scope enforced both sides (a bin checksum and a bin format are each rejected). Single-document strictness intact — adding 0xc4/c5/c6 did not weaken decodeDocument; ck_frame_fed_to_interop_reader still rejects. No discriminator or version field introduced. No secrets in fixtures or tool output.
|
@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 17 minutes. |
|
@kody start-review |
|
@27Bslash6 — ready for your signoff / merge. CodeRabbit has now approved this head ( |
1 participant
LAB-903 — protocol 1.1 close-out: frame-crosscheck dual-encoding, python-frame bin twin, matrix refresh
Stage 6 of the envelope
bin-encoding rollout (LAB-764), unblocked by cachekit 0.17.0 shipping to PyPI today with cachekit-core 0.4.0 (cachekit-py#249).What changed
tools/frame-crosscheck.mjs— accepts BOTH envelope encodings (legacy array-of-ints and protocol 1.1 msgpackbin0xc4/c5/c6), like every real reader. The protocol#11-era deliberatebinrejection is gone; the legacy pin now lives where it belongs — in the legacy vectors' byte-identity plus a per-vectorenvelope_encodingassertion. Checksum remains array-of-ints-only in both encodings (protocol 1.1 excludes it from the flip) and the reader enforces that.test-vectors/python-frame.json— append-onlybintwin of the default-path vector (default_saas_write_msgpack_bytestorage_bin), generated from the real cachekit 0.17.0 PyPI wheel via the newgenerate-bin-twinmode; per-vector generator provenance recorded (same discipline as the 0.11.1 originals). Legacy vectors'frame_hexbytes untouched; the legacy default-path vector gains only the machine-readableenvelope_encoding: "int-array"pin.tools/python-frame-reference.py— new append-onlygenerate-bin-twinmode (refuses non-bin wheels, refuses double-append);generatenow REFUSES to run on bin-emitting wheels at runtime (it would clobber the irreproducible legacy vectors and the twin, and post-dual-decode that clobber would pass CI);--helpprints the docstring.sdk-feature-matrix.md— SDK Overview versions refreshed against the registries (py 0.17.0 PyPI, core 0.4.0 crates.io, rs 0.5.0 + macros 0.5.0 crates.io, ts 0.1.4 npm); core/rs architecture notes updated. Rollout state recorded once, in the cachekit-core note: py ≥ 0.17.0 ships the flip; released rs 0.5.0 / ts 0.1.4 still pin the core 0.3 line (their 0.4.0 bumps — cachekit-rs#53, cachekit-ts#91 — are merged on main, unreleased; registry-verified today).spec/wire-format.md— descriptive-only update to the "Test vectors" fixture-inventory paragraph. No normative text touched (no MUST/SHOULD/layout changes).Verification
verify.ymlsuite green: all reference verifiers + both JS crosschecks, stdlib and msgpack-enabled variants.envelope_encodingmismatch fails; bin-encoded checksum fails (hand-built frame);generaterefuses cachekit 0.17.0;generate-bin-twinrefuses double-append.94 c4 22 …= fixarray(4) → bin8(34) LZ4 block (identical compressed bytes to the legacy vector) → fixarray(8) int checksumdaf468618e624bfe→ size 32 → "msgpack".Gates
generateruntime clobber guard, legacy-vectorenvelope_encodingbackfill,--helpfix, matrix rollout-state dedup. Rejected:--force-clobberflag (YAGNI), deleting thegeneratemode (scope).wire-format.jsonis LAB-868 (separate, already filed) — referenced, not duplicated here.Summary by CodeRabbit
Documentation
binand legacy envelope encodings in the wire-format documentation.Tests
bin-encoded vectors.