Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

### Changed
- **Deterministic fuzz test** — envelope fuzz round-trip test now uses a seeded xorshift32 PRNG instead of `Math.random()`, making failures reproducible across runs.
- **DRY chunk verification** — extracted `_readAndVerifyChunk()` in `CasService`; both the buffered and streaming restore paths now delegate to the same single-chunk verification method.
- **DRY KDF metadata** — extracted `buildKdfMetadata()` helper (`src/domain/helpers/buildKdfMetadata.js`); `VaultService` and `ContentAddressableStore` both call it instead of duplicating the KDF object construction.

## [5.2.2] — JSDoc total coverage (2026-02-28)

### Added
Expand Down
45 changes: 45 additions & 0 deletions ROADMAP.md
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,51 @@ All tasks completed (13.1–13.6). See [COMPLETED_TASKS.md](./COMPLETED_TASKS.md

---

## M15 — Prism (code hygiene)

Consistency and DRY fixes surfaced by architecture audit. No new features, no API changes.

### 14.1 — Consistent async `sha256()` across CryptoPort adapters

**Problem:** `NodeCryptoAdapter.sha256()` returns `string` (sync), while `BunCryptoAdapter` and `WebCryptoAdapter` return `Promise<string>`. Callers must defensively `await` every call. This is a Liskov Substitution violation — adapters are not interchangeable without the caller knowing which one it got.

**Fix:** Make `NodeCryptoAdapter.sha256()` return `Promise<string>` (wrap the sync result). All three adapters then have the same async signature. CasService already awaits every call via `_sha256()`, so no call-site changes are needed outside the adapter itself.

**Files:**
- `src/infrastructure/adapters/NodeCryptoAdapter.js` — wrap return in `Promise.resolve()`
- `src/ports/CryptoPort.js` — update JSDoc to document `Promise<string>` as the contract

**Risk:** None. All callers already `await`. Changing sync→async is backward compatible for awaiting code.

**Tests:** Existing crypto adapter tests already assert on the resolved value. Add an explicit test: `expect(adapter.sha256(buf)).toBeInstanceOf(Promise)`.

### 14.2 — Extract `KeyResolver` from CasService

**Problem:** `CasService` is a ~1087-line god object. Key resolution logic (`_resolveDecryptionKey`, `_resolvePassphraseForDecryption`, `_resolveKeyForRecipients`, `_unwrapDek`, `_wrapDek`, `_validateKeySourceExclusive`) is ~70 lines of self-contained logic that has nothing to do with chunking, storage, or manifests. It's a distinct responsibility: "given a manifest and caller-provided credentials, produce the decryption key."

**Fix:** Extract a `KeyResolver` class into `src/domain/services/KeyResolver.js`. It receives a `CryptoPort` via constructor injection. CasService delegates to it.

**API:**
```js
class KeyResolver {
constructor(crypto) { this.crypto = crypto; }
async resolveForDecryption(manifest, encryptionKey, passphrase) { ... }
async resolveForStore(encryptionKey, passphrase, kdfOptions) { ... }
async resolveRecipients(recipients) { ... }
async wrapDek(dek, kek) { ... }
async unwrapDek(recipientEntry, kek) { ... }
}
```

**Files:**
- New: `src/domain/services/KeyResolver.js`
- Modified: `src/domain/services/CasService.js` — delegate key resolution
- New: `test/unit/domain/services/KeyResolver.test.js`

**Risk:** Low — internal refactor, no public API change. CasService methods remain unchanged.

---

## Backlog (unscheduled)

Ideas for future milestones. Not committed, not prioritized — just captured.
Expand Down
11 changes: 2 additions & 9 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import EventEmitterObserver from './src/infrastructure/adapters/EventEmitterObse
import StatsCollector from './src/infrastructure/adapters/StatsCollector.js';
import FixedChunker from './src/infrastructure/chunkers/FixedChunker.js';
import CdcChunker from './src/infrastructure/chunkers/CdcChunker.js';
import buildKdfMetadata from './src/domain/helpers/buildKdfMetadata.js';

export {
CasService,
Expand Down Expand Up @@ -627,15 +628,7 @@ export default class ContentAddressableStore {
...metadata,
encryption: {
cipher: metadata.encryption.cipher,
kdf: {
algorithm: newParams.algorithm,
salt: newSalt.toString('base64'),
...('iterations' in newParams && { iterations: newParams.iterations }),
...('cost' in newParams && { cost: newParams.cost }),
...('blockSize' in newParams && { blockSize: newParams.blockSize }),
...('parallelization' in newParams && { parallelization: newParams.parallelization }),
keyLength: newParams.keyLength,
},
kdf: buildKdfMetadata(newSalt, newParams),
},
};
}
Expand Down
18 changes: 18 additions & 0 deletions src/domain/helpers/buildKdfMetadata.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Builds the KDF metadata object for vault/manifest encryption metadata.
*
* @param {Buffer} salt - KDF salt.
* @param {{ algorithm: string, keyLength: number, iterations?: number, cost?: number, blockSize?: number, parallelization?: number }} params - KDF parameters.
* @returns {{ algorithm: string, salt: string, keyLength: number, iterations?: number, cost?: number, blockSize?: number, parallelization?: number }}
*/
export default function buildKdfMetadata(salt, params) {
return {
algorithm: params.algorithm,
salt: salt.toString('base64'),
...('iterations' in params && { iterations: params.iterations }),
...('cost' in params && { cost: params.cost }),
...('blockSize' in params && { blockSize: params.blockSize }),
...('parallelization' in params && { parallelization: params.parallelization }),
keyLength: params.keyLength,
};
}
49 changes: 24 additions & 25 deletions src/domain/services/CasService.js
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,28 @@ export default class CasService {
return await this.persistence.writeTree(treeEntries);
}

/**
* Reads a single chunk blob from Git and verifies its SHA-256 digest.
* @private
* @param {{ index: number, size: number, digest: string, blob: string }} chunk - Chunk metadata.
* @returns {Promise<Buffer>} Verified chunk buffer.
* @throws {CasError} INTEGRITY_ERROR if the chunk digest does not match.
*/
async _readAndVerifyChunk(chunk) {
const blob = await this.persistence.readBlob(chunk.blob);
const digest = await this._sha256(blob);
if (digest !== chunk.digest) {
const err = new CasError(
`Chunk ${chunk.index} integrity check failed`,
'INTEGRITY_ERROR',
{ chunkIndex: chunk.index, expected: chunk.digest, actual: digest },
);
this.observability.metric('error', { code: err.code, message: err.message });
throw err;
}
return blob;
}

/**
* Reads chunk blobs from Git and verifies their SHA-256 digests.
* @private
Expand All @@ -529,17 +551,7 @@ export default class CasService {
async _readAndVerifyChunks(chunks) {
const buffers = [];
for (const chunk of chunks) {
const blob = await this.persistence.readBlob(chunk.blob);
const digest = await this._sha256(blob);
if (digest !== chunk.digest) {
const err = new CasError(
`Chunk ${chunk.index} integrity check failed`,
'INTEGRITY_ERROR',
{ chunkIndex: chunk.index, expected: chunk.digest, actual: digest },
);
this.observability.metric('error', { code: err.code, message: err.message });
throw err;
}
const blob = await this._readAndVerifyChunk(chunk);
buffers.push(blob);
this.observability.metric('chunk', { action: 'restored', index: chunk.index, size: blob.length, digest: chunk.digest });
}
Expand Down Expand Up @@ -659,20 +671,7 @@ export default class CasService {
const readAhead = this.concurrency;
let totalSize = 0;

const readAndVerify = async (chunk) => {
const blob = await this.persistence.readBlob(chunk.blob);
const digest = await this._sha256(blob);
if (digest !== chunk.digest) {
const err = new CasError(
`Chunk ${chunk.index} integrity check failed`,
'INTEGRITY_ERROR',
{ chunkIndex: chunk.index, expected: chunk.digest, actual: digest },
);
this.observability.metric('error', { code: err.code, message: err.message });
throw err;
}
return blob;
};
const readAndVerify = (chunk) => this._readAndVerifyChunk(chunk);

const ahead = [];
for (let i = 0; i < Math.min(readAhead, chunks.length); i++) {
Expand Down
11 changes: 2 additions & 9 deletions src/domain/services/VaultService.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* @fileoverview Domain service for vault (GC-safe ref-based asset index) operations.
*/
import CasError from '../errors/CasError.js';
import buildKdfMetadata from '../helpers/buildKdfMetadata.js';

const VAULT_REF = 'refs/cas/vault';
const MAX_CAS_RETRIES = 3;
Expand Down Expand Up @@ -327,15 +328,7 @@ export default class VaultService {
static #buildEncryptionMeta(salt, params) {
return {
cipher: 'aes-256-gcm',
kdf: {
algorithm: params.algorithm,
salt: salt.toString('base64'),
...('iterations' in params && { iterations: params.iterations }),
...('cost' in params && { cost: params.cost }),
...('blockSize' in params && { blockSize: params.blockSize }),
...('parallelization' in params && { parallelization: params.parallelization }),
keyLength: params.keyLength,
},
kdf: buildKdfMetadata(salt, params),
};
}

Expand Down
51 changes: 51 additions & 0 deletions test/unit/domain/helpers/buildKdfMetadata.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
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');
});
});
Comment on lines +1 to +51
Copy link

@coderabbitai coderabbitai bot Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

❓ 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 -10

Repository: 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ✅.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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 Buffer in your app code, Deno added a flag --unstable-node-globals (introduced in Deno v2.1) that injects globals like Buffer. [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

18 changes: 16 additions & 2 deletions test/unit/domain/services/CasService.envelope.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,19 @@ import CasError from '../../../../src/domain/errors/CasError.js';

const testCrypto = await getTestCryptoAdapter();

// ---------------------------------------------------------------------------
// Deterministic PRNG (xorshift32) — keeps fuzz tests reproducible
// ---------------------------------------------------------------------------
function createSeededRng(seed = 42) {
let s = seed >>> 0 || 1;
return (max) => {
s ^= s << 13;
s ^= s >>> 17;
s ^= s << 5;
return (s >>> 0) % max;
};
}

// ---------------------------------------------------------------------------
// Helpers
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -310,8 +323,9 @@ describe('CasService – envelope encryption (fuzz round-trips)', () => {
beforeEach(() => { ({ service } = setup()); });

it('50 random plaintexts × 3 random KEKs all round-trip', async () => {
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)];
Comment on lines +326 to 330
Copy link

@coderabbitai coderabbitai bot Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


Expand All @@ -322,7 +336,7 @@ describe('CasService – envelope encryption (fuzz round-trips)', () => {
recipients: keys.map((key, j) => ({ label: `k${j}`, key })),
});

const idx = Math.floor(Math.random() * 3);
const idx = rng(3);
const { buffer } = await service.restore({ manifest, encryptionKey: keys[idx] });
expect(buffer.equals(original)).toBe(true);
}
Expand Down