feat: wire ByteStorage into cache pipeline for protocol-compliant wire format#27
Merged
Conversation
2 tasks
BREAKING: Removes Encryptor class and KeyRotationState class from @cachekit-io/cachekit-core-ts. Neither was used by the SDK. Critical fixes (from expert panel review): - C1: Remove KeyRotationState — leaked key material to JavaScript via encryption_key()/decryption_key() getters, violating the "keys never leave Rust memory" security model - C2: Remove Encryptor — had fake nonce counter (AtomicI64 not connected to actual nonce generation in ZeroKnowledgeEncryptor) Major fixes: - M2: derive_tenant_keys now requires exactly 32-byte master key (was >=16, inconsistent with derive_key) - M3: derive_key tenant_salt is now required String (was Option<String> that rejected None at runtime) - M4: Extract validate_encryption_input/validate_decryption_input helpers (DRY) - M5: Add size validation to decrypt_with_tenant_keys (was missing) Other: - Remove unused zeroize dependency (cachekit-core handles it) - Remove legacy extern crate syntax from build.rs - Regenerate index.d.ts and index.js (178 lines, down from 283)
…e format Integrates the existing Rust NAPI ByteStorage binding (LZ4 compression + xxHash3-64 integrity) into the cache set/get pipeline. This was the last protocol compliance gap — the TS SDK now produces wire-format-compliant envelopes that interoperate with Python and Rust SDKs. Pipeline: serialize → pack → encrypt → store (reverse on read). The AAD v0x03 `compressed` flag is threaded through encrypt/decrypt and derived from `!!this.byteStorage`, ensuring the AAD always reflects reality. Compression defaults to true (protocol compliance by default). Users can opt out with `compression: false` in CacheOptions. Includes Python-generated compressed=True test vector for cross-SDK interop validation, error propagation tests, and config mismatch tests.
27Bslash6
force-pushed
the
feat/bytestorage-wire-format
branch
from
1f0b773 to
5b740b4
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves native Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CacheImpl
participant ByteStorage
participant EncryptionManager
participant Backend
rect rgba(100,150,200,0.5)
Note over Client,Backend: Cache Set Flow (with Compression)
Client->>CacheImpl: set(key, value)
CacheImpl->>CacheImpl: serialize(value)
alt compression enabled
CacheImpl->>ByteStorage: pack(serialized)
ByteStorage-->>CacheImpl: compressed bytes
else compression disabled
CacheImpl-->>CacheImpl: serialized bytes
end
alt encryption enabled
CacheImpl->>EncryptionManager: encrypt(bytes, cacheKey, compressed?)
EncryptionManager->>EncryptionManager: buildAAD(cacheKey,'msgpack',compressed)
EncryptionManager-->>CacheImpl: encrypted bytes
end
CacheImpl->>Backend: persist(bytes)
end
rect rgba(150,200,100,0.5)
Note over Client,Backend: Cache Get Flow (with Compression)
Client->>CacheImpl: get(key)
CacheImpl->>Backend: retrieve(key)
Backend-->>CacheImpl: bytes
alt encryption enabled
CacheImpl->>EncryptionManager: decrypt(bytes, cacheKey, compressed?)
EncryptionManager->>EncryptionManager: buildAAD(cacheKey,'msgpack',compressed)
EncryptionManager-->>CacheImpl: decrypted bytes
end
alt compression enabled
CacheImpl->>ByteStorage: unpack(decrypted)
ByteStorage-->>CacheImpl: decompressed bytes
end
CacheImpl->>CacheImpl: deserialize(bytes)
CacheImpl-->>Client: value
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/cachekit/src/types/cache.ts (1)
84-85: Call out the migration cost of turning this on by default.Leaving
compressionunset now changes the stored wire format for every entry. Withpackages/cachekit/src/cache.tsalwayspack()/unpack()in that case, mixed old/new fleets and pre-existing entries will read as misses until they are rewritten. A namespace bump / cache flush / coordinated rollout note would help avoid surprise backend load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cachekit/src/types/cache.ts` around lines 84 - 85, Update the API docs and inline JSDoc for the compression?: boolean field to explicitly call out the migration cost of changing the default wire format (existing entries become unreadable until rewritten), and add a short migration note next to the pack()/unpack() logic in cache.ts describing the impact and recommended mitigations (namespace bump, cache flush, or coordinated rollout). Reference the compression property and the pack()/unpack() functions in the note so callers and maintainers see the behavior and operational steps to avoid backend load spikes.packages/cachekit/src/cache.ts (1)
21-21: Reconsider eager static import ofByteStorageif compression is intended to be optional.The static import at line 21 makes
@cachekit-io/cachekit-core-tsload eagerly when cache.ts is imported. Although instantiation at line 75 is conditional (options.compression ?? true ? new ByteStorage() : null), the module must still load successfully. On platforms where the native addon cannot load, even users withcompression: falsewill fail at import time. If compression support is optional, use dynamic import likeEncryptionManagerdoes (see line 94 in encryption/manager.ts) to defer loading until needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cachekit/src/cache.ts` at line 21, The static import of ByteStorage causes the native addon to load at module import time; change cache.ts to remove the top-level "ByteStorage" import and instead dynamically import '@cachekit-io/cachekit-core-ts' at the point where you create the instance (the conditional using options.compression ?? true ? new ByteStorage() : null), e.g., only await/import ByteStorage when options.compression is truthy; ensure you mirror the dynamic pattern used by EncryptionManager (see encryption/manager.ts) so the module is only required when compression is enabled and handle the imported symbol ByteStorage to construct the instance accordingly.packages/cachekit/src/cache.test.ts (1)
8-11: The "default compression" check doesn't actually prove the default.Lines 9-11 and 240-252 imply this suite will fail if
compressionever stops defaulting totrue, but a same-cache set/get round-trip still succeeds when both write and read silently stay uncompressed. Please compare the default path against acompression: falsecontrol, or assert on the stored wire bytes, so this catches a default regression.Suggested test shape
- it('should round-trip with compression enabled (default)', async () => { - const compressedCache = createCache({ - backend: new InMemoryBackend(), + it('should enable compression by default', async () => { + const defaultBackend = new InMemoryBackend(); + const uncompressedBackend = new InMemoryBackend(); + const defaultCache = createCache({ + backend: defaultBackend, l1: { enabled: false }, }); + const uncompressedCache = createCache({ + backend: uncompressedBackend, + compression: false, + l1: { enabled: false }, + }); const payload = { data: 'a'.repeat(1000) }; // Compressible - await compressedCache.set('test:compressed', payload); - const result = await compressedCache.get<typeof payload>('test:compressed'); + await defaultCache.set('test:compressed', payload); + await uncompressedCache.set('test:compressed', payload); + const result = await defaultCache.get<typeof payload>('test:compressed'); + const defaultBytes = await defaultBackend.get('test:compressed'); + const uncompressedBytes = await uncompressedBackend.get('test:compressed'); expect(result).toEqual(payload); - await compressedCache.close(); + expect(defaultBytes!.length).toBeLessThan(uncompressedBytes!.length); + await defaultCache.close(); + await uncompressedCache.close(); });Also applies to: 240-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cachekit/src/cache.test.ts` around lines 8 - 11, The test that assumes compression is enabled by default (referencing ByteStorage and CacheOptions.compression) is insufficient because a round-trip can pass when both write and read silently skip compression; update the test to explicitly compare the default path against a control using compression: false (e.g., create a cache with no options and another with { compression: false }) and assert differences (either by asserting stored wire bytes in the underlying ByteStorage or asserting that compressed marker/lengths differ) or add an explicit assertion that CacheOptions.compression defaults to true; modify the existing test(s) that reference ByteStorage and the default compression behavior to perform this control comparison or wire-bytes assertion so regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cachekit-core-ts/src/lib.rs`:
- Around line 269-274: The TypeScript guard currently allows masterKey lengths
>= MIN_MASTER_KEY_HEX_LENGTH which contradicts the Rust expectations (derive_key
and derive_tenant_keys require exactly 32 bytes); update the check in
packages/cachekit/src/encryption/manager.ts to require exact length by changing
the condition from masterKey.length < MIN_MASTER_KEY_HEX_LENGTH to
masterKey.length !== MIN_MASTER_KEY_HEX_LENGTH and revise the error string to
state the master key must be exactly 64 hex characters (not "at least") so
constructor-time validation matches the Rust-side 32-byte requirement.
In `@packages/cachekit/src/cache.test.ts`:
- Around line 374-388: The test currently treats arbitrary decoded garbage from
a compression/config mismatch as acceptable; update it to assert that such
mismatches fail closed to null (cache miss) instead of allowing silent corrupted
values. Modify the block that creates reader via createCache({ compression:
false, l1: { enabled: false }, backend: sharedBackend }) and replace the loose
assertions with a strict expect(result).toBeNull() (and remove the conditional
branch that tolerates non-null garbage); also ensure any helper behavior in
reader.get or the serializer used by createCache (serializer.decode) is
exercised such that deserialization errors surface as null/rejection per the
ReliabilityExecutor contract rather than producing a decoded object.
---
Nitpick comments:
In `@packages/cachekit/src/cache.test.ts`:
- Around line 8-11: The test that assumes compression is enabled by default
(referencing ByteStorage and CacheOptions.compression) is insufficient because a
round-trip can pass when both write and read silently skip compression; update
the test to explicitly compare the default path against a control using
compression: false (e.g., create a cache with no options and another with {
compression: false }) and assert differences (either by asserting stored wire
bytes in the underlying ByteStorage or asserting that compressed marker/lengths
differ) or add an explicit assertion that CacheOptions.compression defaults to
true; modify the existing test(s) that reference ByteStorage and the default
compression behavior to perform this control comparison or wire-bytes assertion
so regressions are caught.
In `@packages/cachekit/src/cache.ts`:
- Line 21: The static import of ByteStorage causes the native addon to load at
module import time; change cache.ts to remove the top-level "ByteStorage" import
and instead dynamically import '@cachekit-io/cachekit-core-ts' at the point
where you create the instance (the conditional using options.compression ?? true
? new ByteStorage() : null), e.g., only await/import ByteStorage when
options.compression is truthy; ensure you mirror the dynamic pattern used by
EncryptionManager (see encryption/manager.ts) so the module is only required
when compression is enabled and handle the imported symbol ByteStorage to
construct the instance accordingly.
In `@packages/cachekit/src/types/cache.ts`:
- Around line 84-85: Update the API docs and inline JSDoc for the compression?:
boolean field to explicitly call out the migration cost of changing the default
wire format (existing entries become unreadable until rewritten), and add a
short migration note next to the pack()/unpack() logic in cache.ts describing
the impact and recommended mitigations (namespace bump, cache flush, or
coordinated rollout). Reference the compression property and the pack()/unpack()
functions in the note so callers and maintainers see the behavior and
operational steps to avoid backend load spikes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7f6dd0c-bf2a-47d3-b4ad-0f8a5fd8e599
⛔ Files ignored due to path filters (1)
packages/cachekit-core-ts/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
packages/cachekit-core-ts/Cargo.tomlpackages/cachekit-core-ts/build.rspackages/cachekit-core-ts/index.d.tspackages/cachekit-core-ts/index.jspackages/cachekit-core-ts/src/lib.rspackages/cachekit/src/cache.test.tspackages/cachekit/src/cache.tspackages/cachekit/src/encryption/manager.tspackages/cachekit/src/index.tspackages/cachekit/src/types/cache.tspackages/cachekit/test/integration/encryption-real-crypto.integration.test.tspackages/cachekit/test/protocol/cross-sdk-interop.protocol.test.tspackages/cachekit/test/protocol/wire-format.protocol.test.ts
💤 Files with no reviewable changes (4)
- packages/cachekit-core-ts/build.rs
- packages/cachekit-core-ts/Cargo.toml
- packages/cachekit-core-ts/index.js
- packages/cachekit/test/integration/encryption-real-crypto.integration.test.ts
- Master key validation: >= 64 hex → exactly 64 hex (matches Rust's 32-byte requirement). Longer keys now rejected at constructor time instead of failing at lazy NAPI init. - Config mismatch test: asserts null (fail-closed via ReliabilityExecutor) instead of accepting garbage decode as valid behavior. - Updated existing tests that asserted longer keys were accepted.
Contributor
Author
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/cachekit-core-ts/index.d.ts (1)
118-135: Treat the newderiveKey(...)signature as a breaking API change.Requiring
tenantSalthere narrows the public surface of@cachekit-io/cachekit-core-ts: callers using the previous optional/null forms will now fail to compile, and untyped callers will fail at runtime. Please verify the release notes/versioning reflect that, or keep a temporary compatibility overload/shim if you need a softer migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cachekit-core-ts/index.d.ts` around lines 118 - 135, The new deriveKey(masterKey: Uint8Array, domain: string, tenantSalt: string) signature is a breaking change because tenantSalt used to be optional/null; restore compatibility by adding an overload or shim for deriveKey that accepts tenantSalt?: string | null (or the previous nullable form) and internally normalizes undefined/null to the previous behavior before delegating to the new implementation, and ensure callers of deriveKey are covered by the declaration; also update the package's release notes/versioning to mark this as a breaking change if you keep the stricter signature.packages/cachekit/test/protocol/wire-format.protocol.test.ts (1)
4-10: Pin one canonical packed vector in this protocol suite.Right now every assertion is pack/unpack symmetry against the same implementation. If the
ByteStorageenvelope changes butunpack()changes with it, this file still passes and cross-SDK wire compatibility can drift silently. One fixedpack()hex fixture would turn this into an actual protocol test.Also applies to: 13-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cachekit/test/protocol/wire-format.protocol.test.ts` around lines 4 - 10, The tests currently only assert pack/unpack symmetry against the same implementation which can mask protocol regressions; add a canonical hex fixture of a precomputed packed ByteStorage envelope and assert that unpack(ByteStorage) returns the expected original payload and metadata, and that pack(originalPayload) produces the exact same canonical hex; update the test suite (describe 'Protocol v1.0 Wire Format (ByteStorage)') to include a fixed packed vector constant and two assertions: one calling unpack(...) to verify decompressed data/checksum/format match expected values and one calling pack(...) to ensure byte-for-byte equality with the canonical fixture (use the existing pack() and unpack() helpers to locate where to wire the checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cachekit/src/cache.ts`:
- Line 21: ByteStorage's native NAPI construction must be guarded to avoid
crashing the whole cache; wrap the synchronous initialization(s) of ByteStorage
in try-catch (both at the top-level import/initialization and at the other
construction site around lines referenced 74-76) so failures are caught, log or
emit the error, and fall back to a safe no-op/null storage instead of letting
the exception propagate; locate the ByteStorage usage in this file (references
to ByteStorage and any constructor calls) and mirror the pattern used in
encryption/manager.ts (try { new ByteStorage(...) } catch (err) {
processLogger.error(...) / set storage = undefined }).
In `@packages/cachekit/src/types/cache.ts`:
- Around line 84-85: The current optional `compression?: boolean` makes
ByteStorage wire-format the default (opt-out), which causes mixed-version
deploys to split the cache; change the config to opt-in by default: set the
default for `compression` to false (or rename to an explicit flag like
`enableByteStorage` and default it to false), update any writer code that emits
ByteStorage envelopes (where `compressed=True` is set in AAD) to only emit
compressed envelopes when this flag is explicitly enabled, and update relevant
docs/tests to reflect the new opt-in behavior; look for usages of the
`compression` property and places that construct/read ByteStorage envelopes to
ensure readers remain compatible and writers do not emit compressed envelopes
unless the flag is explicitly true.
---
Nitpick comments:
In `@packages/cachekit-core-ts/index.d.ts`:
- Around line 118-135: The new deriveKey(masterKey: Uint8Array, domain: string,
tenantSalt: string) signature is a breaking change because tenantSalt used to be
optional/null; restore compatibility by adding an overload or shim for deriveKey
that accepts tenantSalt?: string | null (or the previous nullable form) and
internally normalizes undefined/null to the previous behavior before delegating
to the new implementation, and ensure callers of deriveKey are covered by the
declaration; also update the package's release notes/versioning to mark this as
a breaking change if you keep the stricter signature.
In `@packages/cachekit/test/protocol/wire-format.protocol.test.ts`:
- Around line 4-10: The tests currently only assert pack/unpack symmetry against
the same implementation which can mask protocol regressions; add a canonical hex
fixture of a precomputed packed ByteStorage envelope and assert that
unpack(ByteStorage) returns the expected original payload and metadata, and that
pack(originalPayload) produces the exact same canonical hex; update the test
suite (describe 'Protocol v1.0 Wire Format (ByteStorage)') to include a fixed
packed vector constant and two assertions: one calling unpack(...) to verify
decompressed data/checksum/format match expected values and one calling
pack(...) to ensure byte-for-byte equality with the canonical fixture (use the
existing pack() and unpack() helpers to locate where to wire the 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 712a7a2d-0892-4e86-b0df-984c00975552
⛔ Files ignored due to path filters (1)
packages/cachekit-core-ts/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
packages/cachekit-core-ts/Cargo.tomlpackages/cachekit-core-ts/build.rspackages/cachekit-core-ts/index.d.tspackages/cachekit-core-ts/index.jspackages/cachekit-core-ts/src/lib.rspackages/cachekit/src/cache.test.tspackages/cachekit/src/cache.tspackages/cachekit/src/encryption/manager.integration.test.tspackages/cachekit/src/encryption/manager.tspackages/cachekit/src/index.tspackages/cachekit/src/types/cache.tspackages/cachekit/test/integration/encryption-real-crypto.integration.test.tspackages/cachekit/test/protocol/cross-sdk-interop.protocol.test.tspackages/cachekit/test/protocol/wire-format.protocol.test.ts
💤 Files with no reviewable changes (4)
- packages/cachekit-core-ts/build.rs
- packages/cachekit-core-ts/Cargo.toml
- packages/cachekit-core-ts/index.js
- packages/cachekit/test/integration/encryption-real-crypto.integration.test.ts
Python-generated packed envelope verified byte-for-byte identical to TypeScript NAPI output. Catches envelope format regressions that round-trip symmetry tests alone would miss. Remaining CodeRabbit findings rejected with rationale: - ByteStorage try-catch: constructor cannot fail (stateless struct), import failure is a hard dep — silent fallback worse than crash - Default compression=false: v0.1.0 greenfield, zero existing deploys, matches Python SDK default (enable_integrity_checking=True) - deriveKey tenantSalt breaking change: pre-existing signature, not in diff
Merged
27Bslash6
pushed a commit
that referenced
this pull request
Apr 26, 2026
🤖 I have created a release *beep* *boop* --- <details><summary>cachekit: 0.1.1</summary> ## [0.1.1](cachekit-v0.1.0...cachekit-v0.1.1) (2026-04-26) ### Features * CachekitIO backend full parity — session, metrics, SSRF, errors, locking, TTL ([985cf09](985cf09)) * CachekitIO backend full parity (session, metrics, SSRF, locking, TTL) ([d408364](d408364)) * initial commit ([048585c](048585c)) * intent-based cache API (createCache.io, .minimal, .production, .secure) ([#42](#42)) ([c551bfb](c551bfb)) * wire ByteStorage into cache pipeline for protocol-compliant wire format ([#27](#27)) ([d246294](d246294)) </details> <details><summary>cachekit-core-ts: 0.1.1</summary> ## [0.1.1](cachekit-core-ts-v0.1.0...cachekit-core-ts-v0.1.1) (2026-04-26) ### Features * CachekitIO backend full parity (session, metrics, SSRF, locking, TTL) ([d408364](d408364)) * initial commit ([048585c](048585c)) * wire ByteStorage into cache pipeline for protocol-compliant wire format ([#27](#27)) ([d246294](d246294)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: cachekit-release-bot[bot] <247960786+cachekit-release-bot[bot]@users.noreply.github.com>
Closed
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.
Summary
compressedAAD flag through encrypt/decrypt (derived from!!this.byteStorage)true(protocol compliance by default, opt out withcompression: false)Pipeline
Changes
src/types/cache.tscompression?: booleanonCacheOptionssrc/cache.tssrc/encryption/manager.tscompressedparam on encrypt/decrypt, threaded to AADsrc/index.tsByteStoragefor advanced userssrc/cache.test.tstest/protocol/cross-sdk-interop.protocol.test.tscompressed=Truetest vector + 3 interop teststest/protocol/wire-format.protocol.test.tsTest plan
tsc --noEmit)eslint)compressed=Truetest vector decrypts correctlycompressedflag correctly rejected by AES-GCMSummary by CodeRabbit
New Features
Breaking Changes
Tests