chore: deterministic fuzz, DRY chunk verify, DRY KDF metadata#15
chore: deterministic fuzz, DRY chunk verify, DRY KDF metadata#15flyingrobots wants to merge 2 commits intomainfrom
Conversation
Three hygiene fixes from architecture audit: 1. Envelope fuzz test uses seeded xorshift32 PRNG instead of Math.random() 2. Extract _readAndVerifyChunk() — single chunk verification shared by buffered and streaming restore paths 3. Extract buildKdfMetadata() helper — eliminates duplicated KDF object construction in VaultService and ContentAddressableStore
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR implements code hygiene improvements by extracting a reusable KDF metadata builder helper to eliminate duplication between VaultService and ContentAddressableStore, centralizing chunk integrity verification logic in CasService, and enhancing fuzz test reproducibility with a seeded pseudorandom number generator. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Line 608: The milestone header "M14 — Prism (code hygiene)" conflicts with the
existing Conduit milestone using the same ID; update the milestone to an unused
ID (e.g., change "M14" to "M15") in the header "M14 — Prism (code hygiene)" and
any corresponding references (TOC entries or internal links) so the milestone ID
is unique across the roadmap; ensure all occurrences of the old "M14" label that
refer to this Prism section are updated consistently.
In `@test/unit/domain/helpers/buildKdfMetadata.test.js`:
- Around line 1-51: The tests currently use Node's Buffer which can fail on
Bun/Deno; update the test file so it is runtime-agnostic by replacing
Buffer.from(...) with a portable Uint8Array/TextEncoder approach and comparing
base64 using a small helper (e.g., const base64 = (u8) =>
btoa(String.fromCharCode(...new Uint8Array(u8))) or use globalThis.Buffer if
available) before calling buildKdfMetadata; ensure all expectations use salt
encoded via that helper and pass a Uint8Array to buildKdfMetadata so the tests
(test/unit/... and the buildKdfMetadata references) run on Node, Bun and Deno.
In `@test/unit/domain/services/CasService.envelope.test.js`:
- Around line 326-330: The fuzz test uses non-deterministic randomBytes for
payloads and keys which breaks reproducibility; replace calls to randomBytes in
the loop with deterministic byte buffers produced from the seeded PRNG returned
by createSeededRng(12345) (use rng to generate bytes for the payload of length
size and to produce three 32-byte key buffers), and do the same for the other
occurrences noted (e.g., the keys on the later loop), ensuring all test input
buffers are derived solely from rng so reruns are fully deterministic; reference
createSeededRng, rng, and the variables original and keys to locate the changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.mdROADMAP.mdindex.jssrc/domain/helpers/buildKdfMetadata.jssrc/domain/services/CasService.jssrc/domain/services/VaultService.jstest/unit/domain/helpers/buildKdfMetadata.test.jstest/unit/domain/services/CasService.envelope.test.js
| import { describe, it, expect } from 'vitest'; | ||
| import buildKdfMetadata from '../../../../src/domain/helpers/buildKdfMetadata.js'; | ||
|
|
||
| describe('buildKdfMetadata', () => { | ||
| it('builds PBKDF2 metadata with iterations', () => { | ||
| const salt = Buffer.from('test-salt'); | ||
| const params = { algorithm: 'pbkdf2', iterations: 100000, keyLength: 32 }; | ||
|
|
||
| const result = buildKdfMetadata(salt, params); | ||
|
|
||
| expect(result).toEqual({ | ||
| algorithm: 'pbkdf2', | ||
| salt: salt.toString('base64'), | ||
| iterations: 100000, | ||
| keyLength: 32, | ||
| }); | ||
| }); | ||
|
|
||
| it('builds scrypt metadata with cost/blockSize/parallelization', () => { | ||
| const salt = Buffer.from('scrypt-salt'); | ||
| const params = { algorithm: 'scrypt', cost: 16384, blockSize: 8, parallelization: 1, keyLength: 32 }; | ||
|
|
||
| const result = buildKdfMetadata(salt, params); | ||
|
|
||
| expect(result).toEqual({ | ||
| algorithm: 'scrypt', | ||
| salt: salt.toString('base64'), | ||
| cost: 16384, | ||
| blockSize: 8, | ||
| parallelization: 1, | ||
| keyLength: 32, | ||
| }); | ||
| }); | ||
|
|
||
| it('omits optional fields when absent', () => { | ||
| const salt = Buffer.from('minimal'); | ||
| const params = { algorithm: 'pbkdf2', keyLength: 32 }; | ||
|
|
||
| const result = buildKdfMetadata(salt, params); | ||
|
|
||
| expect(result).toEqual({ | ||
| algorithm: 'pbkdf2', | ||
| salt: salt.toString('base64'), | ||
| keyLength: 32, | ||
| }); | ||
| expect(result).not.toHaveProperty('iterations'); | ||
| expect(result).not.toHaveProperty('cost'); | ||
| expect(result).not.toHaveProperty('blockSize'); | ||
| expect(result).not.toHaveProperty('parallelization'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
❓ Verification inconclusive
🏁 Script executed:
#!/bin/bash
set -e
# Check repository structure
echo "=== Repository Structure ==="
ls -la
echo ""
echo "=== Checking for package.json and vitest config ==="
if [ -f package.json ]; then
cat package.json | head -30
fi
echo ""
echo "=== Checking test directory ==="
find test -type f -name "*.test.js" -o -name "*.test.ts" | head -10Repository: git-stunts/git-cas
Length of output: 3534
🏁 Script executed:
#!/bin/bash
set -e
# Try Node.js 22.x test run
echo "=== Running npm test (Node.js) ==="
npm test 2>&1 | tail -50 || echo "Node test exit code: $?"Repository: git-stunts/git-cas
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Try Bun test run
echo "=== Running bunx vitest run test/unit (Bun) ==="
bunx vitest run test/unit 2>&1 | tail -50 || echo "Bun test exit code: $?"Repository: git-stunts/git-cas
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Try Deno test run
echo "=== Running deno run -A npm:vitest run test/unit (Deno) ==="
deno run -A npm:vitest run test/unit 2>&1 | tail -50 || echo "Deno test exit code: $?"Repository: git-stunts/git-cas
Length of output: 164
Bun and Deno unit tests must pass before merge.
Per the test matrix requirements, test/unit/**/*.{js,ts} files must pass on all three supported runtimes:
npm test(Node.js 22.x)bunx vitest run test/unit(Bun)deno run -A npm:vitest run test/unit(Deno)
This change cannot merge until verified on all three runtimes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/helpers/buildKdfMetadata.test.js` around lines 1 - 51, The
tests currently use Node's Buffer which can fail on Bun/Deno; update the test
file so it is runtime-agnostic by replacing Buffer.from(...) with a portable
Uint8Array/TextEncoder approach and comparing base64 using a small helper (e.g.,
const base64 = (u8) => btoa(String.fromCharCode(...new Uint8Array(u8))) or use
globalThis.Buffer if available) before calling buildKdfMetadata; ensure all
expectations use salt encoded via that helper and pass a Uint8Array to
buildKdfMetadata so the tests (test/unit/... and the buildKdfMetadata
references) run on Node, Bun and Deno.
There was a problem hiding this comment.
False positive — CI already passed on all three runtimes (Node, Bun, Deno). Buffer is globally available on both Bun and Deno; no polyfill needed. See CI checks: test-docker (bun) ✅, test-docker (deno) ✅, test-docker (node) ✅.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is Buffer globally available in Bun runtime
💡 Result:
Yes. In the Bun runtime, Buffer is available as a global (Node.js-compatible) object, so you can use Buffer.from(...), etc., without importing it. [1][2]
🌐 Web query:
Is Buffer globally available in Deno runtime
💡 Result:
Buffer is not guaranteed to be globally available in ordinary Deno code.
- In Deno’s Node-compat layer, Node globals (including
Buffer) are available in the “npm package scope”, but for your own code Deno expects you to import it (e.g.,import { Buffer } from "node:buffer";). [1][2] - If you specifically want a Node-like global
Bufferin your app code, Deno added a flag--unstable-node-globals(introduced in Deno v2.1) that injects globals likeBuffer. [3]
Recommendation: use import { Buffer } from "node:buffer"; rather than relying on a global. [2]
Sources: [1] [2] [3]
@flyingrobots, you're absolutely right—this was a false positive on my part. I apologize for the noise.
The web search confirms that Buffer is globally available in Bun, and when Deno runs tests via npm:vitest (as in your CI setup with deno run -A npm:vitest run test/unit), Buffer is available in the npm package scope. Your passing CI checks across all three runtimes (test-docker (bun) ✅, test-docker (deno) ✅, test-docker (node) ✅) are concrete proof that Buffer.from() works correctly in your test environment.
Thank you for the correction—I've noted this for future reviews.
🧠 Learnings used
Learnt from: CR
Repo: git-stunts/git-cas PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-28T14:45:24.604Z
Learning: All code must pass on all three supported runtimes: Node.js 22.x (primary), Bun, and Deno.
Learnt from: CR
Repo: git-stunts/git-cas PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-28T14:45:24.604Z
Learning: Applies to test/unit/**/*.{js,ts} : Bun unit tests must pass via `bunx vitest run test/unit`
Learnt from: CR
Repo: git-stunts/git-cas PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-28T14:45:24.604Z
Learning: Applies to test/integration/**/*.{js,ts} : Bun integration tests must pass via `bunx vitest run test/integration`
Learnt from: CR
Repo: git-stunts/git-cas PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-28T14:45:24.604Z
Learning: Applies to test/unit/**/*.{js,ts} : Deno unit tests must pass via `deno run -A npm:vitest run test/unit`
Learnt from: CR
Repo: git-stunts/git-cas PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-28T14:45:24.604Z
Learning: Applies to test/integration/**/*.{js,ts} : Deno integration tests must pass via `deno run -A npm:vitest run test/integration`
Learnt from: CR
Repo: git-stunts/git-cas PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-28T14:45:24.604Z
Learning: Zero tolerance for failing tests. Pre-existing failures are not acceptable. If any test fails on any supported runtime (Node, Bun, Deno), it must be fixed before proceeding. Never skip, ignore, or defer a failing test.
Learnt from: CR
Repo: git-stunts/git-cas PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-28T14:45:24.604Z
Learning: Applies to test/unit/**/*.{js,ts} : Unit tests must pass via `npm test` on Node.js 22.x
| const rng = createSeededRng(12345); | ||
| for (let i = 0; i < 50; i++) { | ||
| const size = Math.floor(Math.random() * 4096); | ||
| const size = rng(4096); | ||
| const original = randomBytes(size); | ||
| const keys = [randomBytes(32), randomBytes(32), randomBytes(32)]; |
There was a problem hiding this comment.
Fuzz round-trip is still partially non-deterministic because test data/keys use randomBytes.
If a failure depends on byte content, it won’t be exactly reproducible across runs.
💡 Suggested fix to make fuzz inputs fully deterministic
function createSeededRng(seed = 42) {
let s = seed >>> 0 || 1;
return (max) => {
s ^= s << 13;
s ^= s >>> 17;
s ^= s << 5;
return (s >>> 0) % max;
};
}
+
+function seededBytes(rng, size) {
+ const buf = Buffer.alloc(size);
+ for (let i = 0; i < size; i++) {
+ buf[i] = rng(256);
+ }
+ return buf;
+}
@@
it('50 random plaintexts × 3 random KEKs all round-trip', async () => {
const rng = createSeededRng(12345);
for (let i = 0; i < 50; i++) {
const size = rng(4096);
- const original = randomBytes(size);
- const keys = [randomBytes(32), randomBytes(32), randomBytes(32)];
+ const original = seededBytes(rng, size);
+ const keys = [seededBytes(rng, 32), seededBytes(rng, 32), seededBytes(rng, 32)];
@@
const idx = rng(3);
const { buffer } = await service.restore({ manifest, encryptionKey: keys[idx] });
expect(buffer.equals(original)).toBe(true);
}
});Also applies to: 339-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/services/CasService.envelope.test.js` around lines 326 -
330, The fuzz test uses non-deterministic randomBytes for payloads and keys
which breaks reproducibility; replace calls to randomBytes in the loop with
deterministic byte buffers produced from the seeded PRNG returned by
createSeededRng(12345) (use rng to generate bytes for the payload of length size
and to produce three 32-byte key buffers), and do the same for the other
occurrences noted (e.g., the keys on the later loop), ensuring all test input
buffers are derived solely from rng so reruns are fully deterministic; reference
createSeededRng, rng, and the variables original and keys to locate the changes.
There was a problem hiding this comment.
Intentional design — randomBytes(32) is kept for crypto keys because keys must be fresh random material, even in tests. Making encryption keys deterministic would be a security anti-pattern and would mask real-world failure modes in the crypto path.
The seeded PRNG targets only the control flow variables (plaintext size selection and recipient index selection) — these are the values that determine which code path executes, and making them deterministic ensures failures in those paths are reproducible. The actual cryptographic operations remain properly random.
There was a problem hiding this comment.
@flyingrobots, you're absolutely right — thank you for the clarification! Keeping randomBytes() for the cryptographic keys preserves the security properties being tested, while the seeded PRNG correctly ensures control-flow reproducibility. This is a smart design that balances deterministic test paths with realistic crypto behavior. I appreciate the detailed explanation of the trade-offs.
🐰✨
(_(\
(='.'=) nods thoughtfully
(")_(")
✏️ Learnings added
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 15
File: test/unit/domain/services/CasService.envelope.test.js:326-330
Timestamp: 2026-02-28T19:21:13.982Z
Learning: In fuzz tests for cryptographic operations, use a seeded PRNG (e.g., xorshift32) for control-flow variables like plaintext size selection and recipient index selection to ensure reproducibility, but keep `randomBytes()` for cryptographic keys and nonces to maintain realistic randomness and avoid security anti-patterns.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@coderabbitai review please 🥕 |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Line 612: Update the milestone task labels under M15 so they match the
milestone ID: rename the heading "### 14.1 — Consistent async `sha256()` across
CryptoPort adapters" to "### 15.1 — Consistent async `sha256()` across
CryptoPort adapters" and rename the corresponding "14.2" task(s) to "15.2"; then
search the document for any other references to "14.1", "14.2" (including the
one noted around line 626) and update them to "15.1"/"15.2" to keep task IDs
consistent with milestone M15.
|
|
||
| Consistency and DRY fixes surfaced by architecture audit. No new features, no API changes. | ||
|
|
||
| ### 14.1 — Consistent async `sha256()` across CryptoPort adapters |
There was a problem hiding this comment.
Task IDs should match the milestone ID (M15).
Under M15, task labels 14.1 and 14.2 are inconsistent and can confuse tracking/searchability. Consider renaming them to 15.1 and 15.2 (and any related references).
Also applies to: 626-626
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ROADMAP.md` at line 612, Update the milestone task labels under M15 so they
match the milestone ID: rename the heading "### 14.1 — Consistent async
`sha256()` across CryptoPort adapters" to "### 15.1 — Consistent async
`sha256()` across CryptoPort adapters" and rename the corresponding "14.2"
task(s) to "15.2"; then search the document for any other references to "14.1",
"14.2" (including the one noted around line 626) and update them to
"15.1"/"15.2" to keep task IDs consistent with milestone M15.
1 participant
Summary
Three hygiene fixes from architecture audit — no public API changes, no new features:
Math.random(), making failures reproducible_readAndVerifyChunk()in CasService; buffered and streaming restore paths share one implementationbuildKdfMetadata()helper; VaultService and ContentAddressableStore both call it instead of duplicating the KDF object spreadTest plan
npx eslint .— 0 errorsnpm test— 788 tests pass (was 709 at last release; +3 newbuildKdfMetadatatests)Summary by CodeRabbit
Tests
Refactor
Chores