refactor: extract helpers from facade + cleanup#16
Merged
flyingrobots merged 16 commits intomainfrom Mar 3, 2026
Merged
Conversation
Move runtime crypto detection (Bun/Deno/Node sniffing) from the top-level getDefaultCryptoAdapter() in index.js to its own module at src/infrastructure/adapters/createCryptoAdapter.js. - test/helpers/crypto-adapter.js now delegates to the new module instead of duplicating the detection logic - New unit test: createCryptoAdapter.test.js
Move the #resolveChunker() private method from index.js to a pure
function at src/infrastructure/chunkers/resolveChunker.js.
- Facade calls resolveChunker({ chunker, chunking }) in #initService()
- New unit test: resolveChunker.test.js (9 tests)
Move storeFile() and restoreFile() stream wiring from index.js to src/infrastructure/adapters/FileIOHelper.js. - All node:fs, node:path, node:stream, node:stream/promises imports removed from index.js - Facade methods become 3-line delegations - New unit test: FileIOHelper.test.js (3 tests)
Move rotateVaultPassphrase() and its 3 private helpers (deriveKekFromKdf, rotateEntries, buildRotatedMetadata) from index.js to src/domain/services/VaultPassphraseRotator.js. - CasError and buildKdfMetadata imports removed from facade - Facade method becomes 4-line delegation - Rotation tests migrated to VaultPassphraseRotator.test.js (6 tests) - Facade rotation test slimmed to single wiring assertion
The facade constructor now stores all options in a single private #config field instead of 10 individual this.fooConfig public properties. These were only consumed by #initService() and the chunkSize getter, so the public surface was unnecessary.
10 modules that were imported solely for re-export now use the
`export { default as X } from '...'` form, eliminating unnecessary
local bindings. Imports section is split into three groups:
- Internal imports (used in class body)
- Re-exports of locally-used modules
- Barrel-only re-exports (no local binding)
rotateVaultPassphrase() now accepts optional maxRetries (default 3) and retryBaseMs (default 50) options for tuning the optimistic- concurrency backoff on VAULT_CONFLICT. - Extract isRetryableConflict() helper to reduce cyclomatic complexity - Update facade JSDoc and index.d.ts with new options - New tests: retry success, maxRetries exhausted, default retry count
Add lastchat.txt to .gitignore and jsr.json exclude list, and remove it from Git tracking.
Aligns task numbering with the milestone ID after the M14 → M15 rename in 090f7a9.
|
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 (8)
📝 WalkthroughWalkthroughThis PR refactors the facade and internal architecture by extracting key management (KeyResolver), file I/O operations (FileIOHelper), crypto adapter selection (createCryptoAdapter), chunker resolution, and vault passphrase rotation into dedicated modules. Additionally, sha256() is normalized to consistently return a Promise across all adapters. The main entry point is restructured to use barrel exports and delegate to extracted modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Facade as ContentAddressableStore
participant FileIOHelper
participant CasService
participant VaultService
Client->>Facade: storeFile(options)
Facade->>FileIOHelper: storeFile(service, options)
FileIOHelper->>FileIOHelper: createReadStream(filePath)
FileIOHelper->>CasService: store(stream, metadata)
CasService->>VaultService: commit(entry)
VaultService-->>CasService: manifest
CasService-->>FileIOHelper: manifest
FileIOHelper-->>Facade: manifest
Facade-->>Client: manifest
sequenceDiagram
participant Client
participant Facade as ContentAddressableStore
participant KeyResolver
participant CryptoAdapter
participant VaultService
Client->>Facade: rotateVaultPassphrase(options)
Facade->>VaultService: read vault state
VaultService-->>Facade: vault entries + metadata
Facade->>KeyResolver: resolveForStore(oldPassphrase, kdfOptions)
KeyResolver->>CryptoAdapter: async sha256(buffer)
CryptoAdapter-->>KeyResolver: Promise<digest>
KeyResolver-->>Facade: old KEK
Facade->>KeyResolver: resolveForStore(newPassphrase, newKdfOptions)
KeyResolver-->>Facade: new KEK + metadata
Facade->>Facade: rotateEntries(oldKek, newKek)
Facade->>VaultService: writeCommit(rotatedEntries, maxRetries)
alt Conflict within retries
VaultService-->>Facade: VAULT_CONFLICT
Facade->>Facade: backoff(retryBaseMs)
Facade->>VaultService: retry writeCommit
end
VaultService-->>Facade: commitOid
Facade-->>Client: { commitOid, rotatedSlugs }
sequenceDiagram
participant Client
participant createCryptoAdapter
participant BunCryptoAdapter
participant WebCryptoAdapter
participant NodeCryptoAdapter
Client->>createCryptoAdapter: await createCryptoAdapter()
alt globalThis.Bun exists
createCryptoAdapter->>BunCryptoAdapter: dynamic import
BunCryptoAdapter-->>createCryptoAdapter: adapter instance
else globalThis.Deno exists
createCryptoAdapter->>WebCryptoAdapter: dynamic import
WebCryptoAdapter-->>createCryptoAdapter: adapter instance
else Node.js
createCryptoAdapter->>NodeCryptoAdapter: static import
NodeCryptoAdapter-->>createCryptoAdapter: adapter instance
end
createCryptoAdapter-->>Client: CryptoPort implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
🧹 Nitpick comments (2)
test/unit/infrastructure/adapters/FileIOHelper.test.js (1)
11-12: Optional: guard teardown to avoid secondary failures.Consider checking
tmpDirbeforermSync(...)so a setup failure doesn’t produce a misleading cleanup error.Also applies to: 58-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/infrastructure/adapters/FileIOHelper.test.js` around lines 11 - 12, Guard the teardown calls that delete the temporary directory so a failed setup won't cause rmSync to throw; modify the afterEach blocks using rmSync(tmpDir, { recursive: true, force: true }) to first check that tmpDir is defined and non-empty (and optionally exists) before calling rmSync. Locate the afterEach that contains rmSync and any similar teardown near lines referencing tmpDir (e.g., the afterEach closures using rmSync and the tmpDir variable) and wrap the rmSync invocation with a simple tmpDir truthy/existence check so cleanup is skipped when setup failed.CHANGELOG.md (1)
11-14: Optional: vary repeated “Extract …” lead-ins for readability.Three consecutive bullets start similarly; small rewording will scan better.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 11 - 14, The three consecutive changelog bullets all start with "Extract …" which is repetitive; edit the entries for createCryptoAdapter, resolveChunker, storeFile/restoreFile, and VaultPassphraseRotator (and the note about CasError and buildKdfMetadata) to vary the lead-in phrases for better readability—for example, use alternatives like "Move", "Relocate", "Introduce", or "Refactor" for some bullets and keep the function/class names (createCryptoAdapter, resolveChunker, storeFile, restoreFile, VaultPassphraseRotator, CasError, buildKdfMetadata) intact so the changes are still easy to find.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/services/VaultPassphraseRotator.js`:
- Around line 16-24: The deriveKekFromKdf call is omitting the stored key
length, causing rotations to fail for vaults using non-default key sizes; update
the service.deriveKey invocation inside deriveKekFromKdf to pass keyLength:
kdf.keyLength (or equivalent property) along with passphrase, salt, algorithm,
iterations, cost, blockSize and parallelization so the derived KEK uses the
original stored key length.
In `@src/infrastructure/chunkers/resolveChunker.js`:
- Around line 24-35: Validate chunking.chunkSize before constructing
FixedChunker: in resolveChunker (where you currently check chunking.strategy ===
'fixed'), ensure chunking.chunkSize is a finite positive integer (e.g., typeof
=== 'number' && Number.isFinite(chunking.chunkSize) &&
Number.isInteger(chunking.chunkSize) && chunking.chunkSize > 0) and only then
return new FixedChunker({ chunkSize: chunking.chunkSize }); if the value is
missing/invalid, fall through to the default behavior (do not construct
FixedChunker) or handle via a clear error/log message so invalid values cannot
propagate into FixedChunker.
In `@test/unit/infrastructure/adapters/createCryptoAdapter.test.js`:
- Around line 10-14: The afterEach hook mutates globalThis.Bun/globalThis.Deno
which fails on Bun/Deno due to immutable globals and the Node-specific test must
be skipped on those runtimes; wrap restoration in a safe try/catch (or check
property descriptors) so assignment or deletion is attempted but non-fatal
(e.g., try { if (origBun === undefined) { delete globalThis.Bun; } else {
globalThis.Bun = origBun; } } catch (_) {} and same for origDeno), and update
the Node-specific test to only run when neither Bun nor Deno are present (guard
with a runtime check like if (typeof Bun === 'undefined' && typeof Deno ===
'undefined') to skip the test on Bun/Deno).
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 11-14: The three consecutive changelog bullets all start with
"Extract …" which is repetitive; edit the entries for createCryptoAdapter,
resolveChunker, storeFile/restoreFile, and VaultPassphraseRotator (and the note
about CasError and buildKdfMetadata) to vary the lead-in phrases for better
readability—for example, use alternatives like "Move", "Relocate", "Introduce",
or "Refactor" for some bullets and keep the function/class names
(createCryptoAdapter, resolveChunker, storeFile, restoreFile,
VaultPassphraseRotator, CasError, buildKdfMetadata) intact so the changes are
still easy to find.
In `@test/unit/infrastructure/adapters/FileIOHelper.test.js`:
- Around line 11-12: Guard the teardown calls that delete the temporary
directory so a failed setup won't cause rmSync to throw; modify the afterEach
blocks using rmSync(tmpDir, { recursive: true, force: true }) to first check
that tmpDir is defined and non-empty (and optionally exists) before calling
rmSync. Locate the afterEach that contains rmSync and any similar teardown near
lines referencing tmpDir (e.g., the afterEach closures using rmSync and the
tmpDir variable) and wrap the rmSync invocation with a simple tmpDir
truthy/existence check so cleanup is skipped when setup failed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.gitignoreCHANGELOG.mdROADMAP.mdindex.d.tsindex.jsjsr.jsonsrc/domain/services/VaultPassphraseRotator.jssrc/infrastructure/adapters/FileIOHelper.jssrc/infrastructure/adapters/createCryptoAdapter.jssrc/infrastructure/chunkers/resolveChunker.jstest/helpers/crypto-adapter.jstest/unit/domain/services/VaultPassphraseRotator.test.jstest/unit/facade/ContentAddressableStore.rotation.test.jstest/unit/infrastructure/adapters/FileIOHelper.test.jstest/unit/infrastructure/adapters/createCryptoAdapter.test.jstest/unit/infrastructure/chunkers/resolveChunker.test.js
NodeCryptoAdapter.sha256() was the only sync adapter — Bun and Web adapters already returned Promise<string>. This Liskov Substitution violation forced callers to defensively await. Adding `async` to the Node adapter unifies the contract across all three runtimes. All 21+ call sites already `await`, so this is backward compatible. - src/infrastructure/adapters/NodeCryptoAdapter.js: add async keyword - src/ports/CryptoPort.js: update JSDoc @returns to Promise<string> - src/domain/services/CasService.d.ts: update CryptoPort.sha256 type - test: add explicit Promise instance check for sha256 return value
CasService was ~1085 lines with ~170 lines of self-contained key resolution logic (validation, DEK wrap/unwrap, passphrase derivation, envelope recipient resolution). This commit extracts that cohesive responsibility into a dedicated KeyResolver class. KeyResolver receives a CryptoPort via constructor injection and exposes: - validateKeySourceExclusive() — static, mutual exclusion guard - wrapDek() / unwrapDek() — DEK envelope operations - resolveForDecryption() — manifest + credentials → decryption key - resolveForStore() — key/passphrase → store-ready key + metadata - resolveRecipients() — multi-recipient DEK generation - resolveKeyForRecipients() — envelope unwrap with fallback iteration CasService now instantiates a KeyResolver and delegates all 8 call sites. Nine extracted methods removed from CasService (1085 → 909 lines). No public API changes. 24 new unit tests for KeyResolver.
- CHANGELOG: add async sha256 and KeyResolver extraction entries under [Unreleased] - ROADMAP: mark M15 Prism, Task 15.1, and Task 15.2 as complete with implementation details
- package.json + jsr.json: 5.2.2 → 5.2.3 - CHANGELOG: move [Unreleased] to [5.2.3] — Prism refactor (2026-03-03) - README: add "What's new in v5.2.3" section
- Fix CryptoPortBase.sha256() type: `string | Promise<string>` → `Promise<string>`
- Make CasService.keyResolver private (#keyResolver)
- Rename VaultPassphraseRotator.js → rotateVaultPassphrase.js (camelCase for functions)
- Fix README: "no public API changes" → "no breaking API changes"
- Fix README barrel re-export syntax: `export { default as X } from '...'`
- Remove vestigial lastchat.txt from jsr.json exclude
- Break 165-char rotateVaultPassphrase() function signature across lines
- Add @fileoverview JSDoc to FileIOHelper, createCryptoAdapter, resolveChunker
- Add salt assertion to KeyResolver.resolveForStore passphrase test
- Document KeyResolver's direct CryptoPort.deriveKey() call in class JSDoc
- Fix Deno CI: guard immutable globalThis.Deno restoration with try/catch; skip Node-only test on Bun/Deno runtimes - Forward kdf.keyLength in KeyResolver.#resolveKeyFromPassphrase and deriveKekFromKdf (latent bug for non-default key lengths) - Validate chunkSize as finite positive number in resolveChunker before constructing FixedChunker - Guard rmSync teardown in FileIOHelper.test.js against setup failure - Vary "Extract..." lead-ins in CHANGELOG for readability - Add keyLength forwarding + resolveChunker edge-case tests (837 total)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/domain/services/rotateVaultPassphrase.js (1)
16-24:⚠️ Potential issue | 🟠 MajorPreserve
keyLengthfor both old/new KEK derivations.The rotator currently drops stored
kdf.keyLengthwhen deriving the old KEK and does not preserve it by default for the new KEK derivation. This can break rotation compatibility for non-default key lengths.💡 Proposed fix
async function deriveKekFromKdf(service, passphrase, kdf) { const { key } = await service.deriveKey({ passphrase, salt: Buffer.from(kdf.salt, 'base64'), algorithm: kdf.algorithm, iterations: kdf.iterations, cost: kdf.cost, blockSize: kdf.blockSize, parallelization: kdf.parallelization, + keyLength: kdf.keyLength, }); return key; } @@ - const { key: newKek, salt: newSalt, params: newParams } = await service.deriveKey({ - passphrase: newPassphrase, ...kdfOptions, algorithm: kdfOptions?.algorithm || kdf.algorithm, - }); + const { key: newKek, salt: newSalt, params: newParams } = await service.deriveKey({ + passphrase: newPassphrase, + ...kdfOptions, + algorithm: kdfOptions?.algorithm || kdf.algorithm, + keyLength: kdfOptions?.keyLength ?? kdf.keyLength, + });Also applies to: 117-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/rotateVaultPassphrase.js` around lines 16 - 24, The deriveKey calls that compute the old and new KEKs currently omit kdf.keyLength, which can break rotations when a non-default key length was used; update both service.deriveKey invocations (the old KEK and the new KEK derivations) to pass keyLength: kdf.keyLength (or fallback to the stored kdf value) alongside passphrase, salt, algorithm, iterations, cost, blockSize, and parallelization so the derived KEKs preserve the original key length.
🧹 Nitpick comments (3)
test/unit/domain/services/rotateVaultPassphrase.test.js (2)
47-52: MakestoreEnvelopederive helper pass full stored KDF params.This keeps the test helper aligned with vault metadata shape and avoids implicit dependence on defaults.
💡 Proposed fix
const { key } = await service.deriveKey({ passphrase, salt: Buffer.from(metadata.encryption.kdf.salt, 'base64'), algorithm: metadata.encryption.kdf.algorithm, iterations: metadata.encryption.kdf.iterations, + cost: metadata.encryption.kdf.cost, + blockSize: metadata.encryption.kdf.blockSize, + parallelization: metadata.encryption.kdf.parallelization, + keyLength: metadata.encryption.kdf.keyLength, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/rotateVaultPassphrase.test.js` around lines 47 - 52, The test helper storeEnvelope should derive the key using the full stored KDF params from metadata rather than relying on defaults: update the helper to pass salt (as Buffer.from(metadata.encryption.kdf.salt, 'base64')), algorithm (metadata.encryption.kdf.algorithm) and iterations (metadata.encryption.kdf.iterations) into the derive call so it matches the vault metadata shape used in service.deriveKey; ensure the helper constructs the same parameter object shape the production code expects (pass passphrase plus the full metadata.encryption.kdf fields) when calling deriveKey.
170-178: Assert the expected error code for wrong old passphrase.A generic
.toThrow()can pass for unrelated failures; assert the rotation-specific error contract here.💡 Proposed fix
await expect( rotateVaultPassphrase({ service, vault }, { oldPassphrase: 'wrong', newPassphrase: 'new' }), - ).rejects.toThrow(); + ).rejects.toMatchObject({ code: 'NO_MATCHING_RECIPIENT' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/rotateVaultPassphrase.test.js` around lines 170 - 178, Update the test "wrong old passphrase → error" to assert the rotation-specific error instead of a generic throw: call rotateVaultPassphrase({ service, vault }, { oldPassphrase: 'wrong', newPassphrase: 'new' }) and expect the promise to reject with the specific error contract returned by the rotation code (e.g., check error.code or error.name/messaage) — replace .rejects.toThrow() with an assertion like .rejects.toMatchObject({ code: 'INVALID_OLD_PASSPHRASE' }) or .rejects.toThrowError(/incorrect old passphrase/) that matches the actual error produced by rotateVaultPassphrase.test/unit/facade/ContentAddressableStore.rotation.test.js (1)
53-58: Use full vault KDF params in the derive helper.The helper currently forwards only part of the stored KDF fields. Passing through all KDF metadata makes this wiring test resilient to non-PBKDF2/default parameter combinations.
💡 Proposed fix
const { key } = await cas.deriveKey({ passphrase: oldPass, salt: Buffer.from(metadata.encryption.kdf.salt, 'base64'), algorithm: metadata.encryption.kdf.algorithm, iterations: metadata.encryption.kdf.iterations, + cost: metadata.encryption.kdf.cost, + blockSize: metadata.encryption.kdf.blockSize, + parallelization: metadata.encryption.kdf.parallelization, + keyLength: metadata.encryption.kdf.keyLength, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/facade/ContentAddressableStore.rotation.test.js` around lines 53 - 58, The deriveKey call only forwards a subset of the stored KDF fields (algorithm, iterations, salt) which makes the test brittle for non-default KDFs; update the cas.deriveKey invocation to pass the full stored KDF params from metadata.encryption.kdf (e.g. spread or explicitly include all fields present in that object such as algorithm, iterations, salt, keylen/dkLen, digest, and any other kdf-specific options) so the helper uses the exact vault KDF configuration when deriving the key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 29-32: The four changelog bullet lines all begin with "Extract …";
update the entries for `createCryptoAdapter`, `resolveChunker`,
`storeFile()/restoreFile()` (or `FileIOHelper`), and `rotateVaultPassphrase` to
vary the lead verb(s) for better readability—e.g., keep one as "Extract", change
another to "Move", another to "Relocate" or "Move to", and another to "Refactor"
or "Split out"—while preserving the existing identifiers (`createCryptoAdapter`,
`resolveChunker`, `FileIOHelper`/`storeFile`/`restoreFile`,
`rotateVaultPassphrase`) and the brief descriptions of what changed.
In `@src/domain/services/KeyResolver.js`:
- Around line 207-216: The passphrase-based resolver method
`#resolveKeyFromPassphrase` is not passing kdf.keyLength into
this.#crypto.deriveKey, which can produce a different key when non-default
lengths were used; update `#resolveKeyFromPassphrase` to forward kdf.keyLength
(e.g., include keyLength: kdf.keyLength) into the deriveKey call alongside salt,
algorithm, iterations, cost, blockSize and parallelization so the derived key
matches the originally stored KDF parameters.
In `@src/domain/services/rotateVaultPassphrase.js`:
- Around line 107-109: In rotateVaultPassphrase, validate and normalize the
incoming maxRetries and retryBaseMs before entering the retry loop: ensure
maxRetries is an integer >= 1 (or else replace with DEFAULT_MAX_RETRIES or throw
a RangeError) and ensure retryBaseMs is a number >= 0 (or else replace with
DEFAULT_RETRY_BASE_MS or throw), then use the normalized values in the for-loop;
update references to maxRetries and retryBaseMs in the function so the loop and
backoff math always operate on safe, validated values (identify the validation
location at the top of the rotateVaultPassphrase parameter handling, and
reference DEFAULT_MAX_RETRIES and DEFAULT_RETRY_BASE_MS).
---
Duplicate comments:
In `@src/domain/services/rotateVaultPassphrase.js`:
- Around line 16-24: The deriveKey calls that compute the old and new KEKs
currently omit kdf.keyLength, which can break rotations when a non-default key
length was used; update both service.deriveKey invocations (the old KEK and the
new KEK derivations) to pass keyLength: kdf.keyLength (or fallback to the stored
kdf value) alongside passphrase, salt, algorithm, iterations, cost, blockSize,
and parallelization so the derived KEKs preserve the original key length.
---
Nitpick comments:
In `@test/unit/domain/services/rotateVaultPassphrase.test.js`:
- Around line 47-52: The test helper storeEnvelope should derive the key using
the full stored KDF params from metadata rather than relying on defaults: update
the helper to pass salt (as Buffer.from(metadata.encryption.kdf.salt,
'base64')), algorithm (metadata.encryption.kdf.algorithm) and iterations
(metadata.encryption.kdf.iterations) into the derive call so it matches the
vault metadata shape used in service.deriveKey; ensure the helper constructs the
same parameter object shape the production code expects (pass passphrase plus
the full metadata.encryption.kdf fields) when calling deriveKey.
- Around line 170-178: Update the test "wrong old passphrase → error" to assert
the rotation-specific error instead of a generic throw: call
rotateVaultPassphrase({ service, vault }, { oldPassphrase: 'wrong',
newPassphrase: 'new' }) and expect the promise to reject with the specific error
contract returned by the rotation code (e.g., check error.code or
error.name/messaage) — replace .rejects.toThrow() with an assertion like
.rejects.toMatchObject({ code: 'INVALID_OLD_PASSPHRASE' }) or
.rejects.toThrowError(/incorrect old passphrase/) that matches the actual error
produced by rotateVaultPassphrase.
In `@test/unit/facade/ContentAddressableStore.rotation.test.js`:
- Around line 53-58: The deriveKey call only forwards a subset of the stored KDF
fields (algorithm, iterations, salt) which makes the test brittle for
non-default KDFs; update the cas.deriveKey invocation to pass the full stored
KDF params from metadata.encryption.kdf (e.g. spread or explicitly include all
fields present in that object such as algorithm, iterations, salt, keylen/dkLen,
digest, and any other kdf-specific options) so the helper uses the exact vault
KDF configuration when deriving the key.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.mdREADME.mdindex.d.tsindex.jsjsr.jsonsrc/domain/services/CasService.jssrc/domain/services/KeyResolver.jssrc/domain/services/rotateVaultPassphrase.jssrc/infrastructure/adapters/FileIOHelper.jssrc/infrastructure/adapters/createCryptoAdapter.jssrc/infrastructure/chunkers/resolveChunker.jstest/unit/domain/services/KeyResolver.test.jstest/unit/domain/services/rotateVaultPassphrase.test.jstest/unit/facade/ContentAddressableStore.rotation.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/infrastructure/chunkers/resolveChunker.js
- src/infrastructure/adapters/FileIOHelper.js
- jsr.json
- README.md
- src/infrastructure/adapters/createCryptoAdapter.js
Comment on lines
+107
to
+109
| { oldPassphrase, newPassphrase, kdfOptions, maxRetries = DEFAULT_MAX_RETRIES, retryBaseMs = DEFAULT_RETRY_BASE_MS }, | ||
| ) { | ||
| for (let attempt = 0; attempt < maxRetries; attempt++) { |
There was a problem hiding this comment.
Validate maxRetries and retryBaseMs before starting retries.
Invalid values (for example maxRetries <= 0 or negative retryBaseMs) currently lead to confusing behavior and error surfaces.
💡 Proposed fix
export default async function rotateVaultPassphrase(
{ service, vault },
{ oldPassphrase, newPassphrase, kdfOptions, maxRetries = DEFAULT_MAX_RETRIES, retryBaseMs = DEFAULT_RETRY_BASE_MS },
) {
+ if (!Number.isInteger(maxRetries) || maxRetries < 1) {
+ throw new CasError('maxRetries must be a positive integer', 'INVALID_OPTIONS');
+ }
+ if (!Number.isFinite(retryBaseMs) || retryBaseMs < 0) {
+ throw new CasError('retryBaseMs must be a non-negative number', 'INVALID_OPTIONS');
+ }
+
for (let attempt = 0; attempt < maxRetries; attempt++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/rotateVaultPassphrase.js` around lines 107 - 109, In
rotateVaultPassphrase, validate and normalize the incoming maxRetries and
retryBaseMs before entering the retry loop: ensure maxRetries is an integer >= 1
(or else replace with DEFAULT_MAX_RETRIES or throw a RangeError) and ensure
retryBaseMs is a number >= 0 (or else replace with DEFAULT_RETRY_BASE_MS or
throw), then use the normalized values in the for-loop; update references to
maxRetries and retryBaseMs in the function so the loop and backoff math always
operate on safe, validated values (identify the validation location at the top
of the rotateVaultPassphrase parameter handling, and reference
DEFAULT_MAX_RETRIES and DEFAULT_RETRY_BASE_MS).
Member
Author
There was a problem hiding this comment.
Acknowledged — this is an internal function with sensible defaults (maxRetries=3, retryBaseMs=50). The facade passes these through directly from user options. With maxRetries: 0 the loop simply does not execute and throws VAULT_CONFLICT, which is the correct semantic for "zero attempts." Adding explicit guards would duplicate the defaults without catching a real user scenario. Leaving as-is for now.
There was a problem hiding this comment.
@flyingrobots, that's a fair point — maxRetries: 0 producing an immediate VAULT_CONFLICT is a coherent and self-documenting contract ("attempt nothing, conflict"), and retryBaseMs: 0 simply removes the backoff delay. Understood, leaving as-is.
Member
Author
|
@coderabbitai review please 🥕 |
|
✅ Actions performedReview triggered.
|
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
Follow-up to #15 — these refactoring commits were completed in the same session but never pushed before the PR was merged.
createCryptoAdapterfrom facade intosrc/infrastructure/adapters/createCryptoAdapter.jsresolveChunkerfrom facade intosrc/infrastructure/chunkers/resolveChunker.jsreadInput,writeOutput) from facade intosrc/infrastructure/adapters/FileIOHelper.jsVaultPassphraseRotatorfrom facade intosrc/domain/services/VaultPassphraseRotator.js#configfield — replace public config properties with a private#configfield onContentAddressableStoreindex.jsmaxRetriesconfigurable inVaultPassphraseRotatorlastchat.txt, rename task IDs14.x→15.xin ROADMAPTest plan
npx eslint .— 0 errorsnpm test— 808 tests pass (verified by pre-push hook)Summary by CodeRabbit
New Features
Bug Fixes
Changed