[codex] harden v6 vault service decomposition#36
Merged
flyingrobots merged 49 commits intomainfrom May 9, 2026
Merged
Conversation
📝 WalkthroughWalkthroughLarge refactor: vault internals decomposed into collaborators with retry/backoff; unified ErrorCodes and CasError (options/toJSON); added CLI/agent/UI “doctor” with encryption-key threading; enforced restore baseDirectory; added per-operation merkleThreshold; adapters gained blob limits; extensive docs and tests updated. Vault, Errors, CLI/UI, and Docs
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant AgentDoctor
participant VaultService
participant GitAdapter
User->>CLI: git cas doctor [--key-file|--vault-passphrase]
CLI->>AgentDoctor: invoke(args, stdin, session)
AgentDoctor->>VaultService: inspectVaultHealth({ encryptionKey? })
VaultService->>GitAdapter: readState / readTree / readBlob (maxBytes)
GitAdapter-->>VaultService: tree/meta/blob bytes
VaultService-->>AgentDoctor: report { status, stats, issues }
AgentDoctor-->>CLI: exitCode, { report }
Estimated code review effort: Possibly related PRs:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Member
Author
|
Feedback/CI resolution summary:
Verification completed before push:
No unresolved inline review threads were present when processed; the visible PR feedback was the draft-skip CodeRabbit notice plus failing CI. CI is rerunning on the pushed branch. |
Member
Author
Feedback Resolution SummaryAll unresolved inline review threads have been replied to and marked resolved. Final pushed head is
Verification on final head
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/unit/vault/VaultService.privacy.test.js (1)
474-499: ⚡ Quick winPrefer
ErrorCodesconstants in new assertions instead of raw strings.New tests assert
'VAULT_PRIVACY_INDEX_INVALID'as a literal even thoughErrorCodesis already imported. Using constants here reduces drift/typo risk as error surfaces evolve.Suggested patch
- await expect(vault.readState({ encryptionKey: TEST_KEY })).rejects.toMatchObject({ - code: 'VAULT_PRIVACY_INDEX_INVALID', + await expect(vault.readState({ encryptionKey: TEST_KEY })).rejects.toMatchObject({ + code: ErrorCodes.VAULT_PRIVACY_INDEX_INVALID, meta: { field: 'privacy.indexMeta' }, }); @@ await expect(vault.resolveVaultEntry({ slug: 'alpha', encryptionKey: TEST_KEY })) .rejects.toMatchObject({ - code: 'VAULT_PRIVACY_INDEX_INVALID', + code: ErrorCodes.VAULT_PRIVACY_INDEX_INVALID, meta: { field: 'privacy.indexMeta' }, }); @@ - await expect(vault.readState({ encryptionKey: TEST_KEY })).rejects.toMatchObject({ - code: 'VAULT_PRIVACY_INDEX_INVALID', + await expect(vault.readState({ encryptionKey: TEST_KEY })).rejects.toMatchObject({ + code: ErrorCodes.VAULT_PRIVACY_INDEX_INVALID, meta: { unmatchedCount: 1, treeEntryCount: 2, resolvedCount: 1, }, }); @@ - await expect(vault.listVault({ encryptionKey: TEST_KEY })).rejects.toMatchObject({ - code: 'VAULT_PRIVACY_INDEX_INVALID', + await expect(vault.listVault({ encryptionKey: TEST_KEY })).rejects.toMatchObject({ + code: ErrorCodes.VAULT_PRIVACY_INDEX_INVALID, meta: { unmatchedCount: 1, treeEntryCount: 2, resolvedCount: 1, }, });Also applies to: 515-535
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/vault/VaultService.privacy.test.js` around lines 474 - 499, Replace the hard-coded error code string 'VAULT_PRIVACY_INDEX_INVALID' in the test assertions with the ErrorCodes constant to avoid drift; update the expect(...).rejects.toMatchObject calls that reference the literal (e.g., the assertions against vault.readState and vault.resolveVaultEntry) to use ErrorCodes.VAULT_PRIVACY_INDEX_INVALID for the code field, and similarly change the same literal occurrences noted around lines 515-535 to use the ErrorCodes constant.test/unit/types/declaration-accuracy.test.js (1)
52-60: ⚡ Quick winConsider using regex patterns for more robust JSDoc matching.
The exact string matching with
toContainwill fail on benign formatting changes (e.g., double quotes in import paths, extra whitespace). The existing tests in this file use regex patterns withtoMatchfor flexibility (see lines 27–30 and 42). Adopting the same approach here would make the test more maintainable and consistent.🔧 Suggested refactor using regex patterns
it('keeps ManifestDiff parameter typedefs resolvable', () => { const source = read('src/domain/services/ManifestDiff.js'); - expect(source).toContain( - '@typedef {import(\'../value-objects/Manifest.js\').default} Manifest', - ); - expect(source).toContain('@param {Manifest} oldManifest'); - expect(source).toContain('@param {Manifest} newManifest'); + expect(source).toMatch( + /@typedef\s+\{import\(['"]\.\.\/value-objects\/Manifest\.js['"]\)\.default\}\s+Manifest/ + ); + expect(source).toMatch(/@param\s+\{Manifest\}\s+oldManifest/); + expect(source).toMatch(/@param\s+\{Manifest\}\s+newManifest/); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/types/declaration-accuracy.test.js` around lines 52 - 60, The test that verifies ManifestDiff JSDoc currently uses exact string checks with toContain; change these assertions to use regex-based matching with toMatch so they tolerate benign formatting differences (e.g., different quotes or whitespace). Update the three assertions that inspect the content read('src/domain/services/ManifestDiff.js') to use appropriate RegExp patterns that match the typedef import (Manifest) and the `@param` {Manifest} oldManifest and `@param` {Manifest} newManifest patterns, following the style used earlier in this file (replace toContain with toMatch and supply flexible regexes).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit/vault/VaultService.verifier.test.js`:
- Around line 190-209: The test claims to verify the mutation occurs after
readState has already validated the key but never calls readState; update the
test to call the vault.readState method (e.g., await vault.readState({ slug:
'asset', treeOid: 'asset-tree', encryptionKey: TEST_KEY })) before invoking
vault.addToVault so the verifier cache is populated, then assert
crypto.decryptBuffer was called only once; keep existing mocks (persistence,
ref, crypto) and call sequence (readState then addToVault) to exercise the
cross-operation cache scenario for the verifier.
---
Nitpick comments:
In `@test/unit/types/declaration-accuracy.test.js`:
- Around line 52-60: The test that verifies ManifestDiff JSDoc currently uses
exact string checks with toContain; change these assertions to use regex-based
matching with toMatch so they tolerate benign formatting differences (e.g.,
different quotes or whitespace). Update the three assertions that inspect the
content read('src/domain/services/ManifestDiff.js') to use appropriate RegExp
patterns that match the typedef import (Manifest) and the `@param` {Manifest}
oldManifest and `@param` {Manifest} newManifest patterns, following the style used
earlier in this file (replace toContain with toMatch and supply flexible
regexes).
In `@test/unit/vault/VaultService.privacy.test.js`:
- Around line 474-499: Replace the hard-coded error code string
'VAULT_PRIVACY_INDEX_INVALID' in the test assertions with the ErrorCodes
constant to avoid drift; update the expect(...).rejects.toMatchObject calls that
reference the literal (e.g., the assertions against vault.readState and
vault.resolveVaultEntry) to use ErrorCodes.VAULT_PRIVACY_INDEX_INVALID for the
code field, and similarly change the same literal occurrences noted around lines
515-535 to use the ErrorCodes constant.
🪄 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: ba5f4ab8-bf1f-4a08-a63c-4fe6c7801af8
📒 Files selected for processing (37)
CHANGELOG.mdGUIDE.mdSECURITY.mdbin/credentials.jsbin/restore-output-target.jsbin/ui/vault-report.jsdocs/API.mddocs/EXTENDING.mddocs/VAULT_INTERNALS.mddocs/design/manifest-diffing.mdsrc/domain/errors/Codes.jssrc/domain/services/ManifestDiff.jssrc/domain/services/VaultMetadataCodec.jssrc/domain/services/VaultPersistence.jssrc/domain/services/VaultService.jssrc/domain/services/VaultStateCache.jssrc/infrastructure/adapters/FileIOHelper.jssrc/infrastructure/adapters/GitPersistenceAdapter.jssrc/infrastructure/adapters/GitRefAdapter.jstest/helpers/MemoryRefAdapter.jstest/unit/cli/credential-resolution.test.jstest/unit/cli/restore-output-target.test.jstest/unit/cli/vault-report.test.jstest/unit/docs/release-truth.test.jstest/unit/docs/test-style.test.jstest/unit/domain/errors/domain-errors.test.jstest/unit/domain/services/VaultMetadataCodec.test.jstest/unit/domain/services/VaultPersistence.test.jstest/unit/domain/services/VaultStateCache.test.jstest/unit/helpers/MemoryRefAdapter.test.jstest/unit/infrastructure/adapters/FileIOHelper.test.jstest/unit/infrastructure/adapters/GitPersistenceAdapter.readBlob.test.jstest/unit/infrastructure/adapters/GitRefAdapter.test.jstest/unit/types/declaration-accuracy.test.jstest/unit/vault/VaultService.privacy.test.jstest/unit/vault/VaultService.test.jstest/unit/vault/VaultService.verifier.test.js
✅ Files skipped from review due to trivial changes (7)
- docs/design/manifest-diffing.md
- src/domain/services/ManifestDiff.js
- test/unit/infrastructure/adapters/GitPersistenceAdapter.readBlob.test.js
- SECURITY.md
- test/unit/helpers/MemoryRefAdapter.test.js
- GUIDE.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (22)
- bin/restore-output-target.js
- docs/EXTENDING.md
- test/unit/cli/credential-resolution.test.js
- src/domain/errors/Codes.js
- test/unit/infrastructure/adapters/GitRefAdapter.test.js
- test/unit/docs/release-truth.test.js
- test/unit/cli/restore-output-target.test.js
- test/unit/domain/errors/domain-errors.test.js
- bin/credentials.js
- test/unit/docs/test-style.test.js
- test/unit/vault/VaultService.test.js
- src/infrastructure/adapters/FileIOHelper.js
- test/helpers/MemoryRefAdapter.js
- test/unit/domain/services/VaultPersistence.test.js
- bin/ui/vault-report.js
- src/infrastructure/adapters/GitPersistenceAdapter.js
- test/unit/domain/services/VaultMetadataCodec.test.js
- src/domain/services/VaultMetadataCodec.js
- test/unit/cli/vault-report.test.js
- src/domain/services/VaultStateCache.js
- src/domain/services/VaultPersistence.js
- src/domain/services/VaultService.js
Member
Author
|
Final feedback round summary:
Current verification:
|
Member
Author
|
Additional non-inline CodeRabbit nitpicks resolved:
Verification after this commit:
|
Member
Author
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
Member
Author
|
Additional release-gate fix resolved:
Current review-thread scan: 0 unresolved non-outdated threads, 0 unresolved outdated threads. |
Member
Author
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/infrastructure/adapters/GitRefAdapter.js (1)
15-16: ⚡ Quick winAdapt null-OID length to match the repository's active object format.
Git's
update-refCAS validation requires the null-OID length to match the repository's OID format (40 chars for SHA-1, 64 chars for SHA-256). Hardcoding 40 assumes SHA-1 and will fail on repositories configured with a different object format.Replace the hardcoded
GIT_NULL_OIDwith a dynamic calculation usingnewOid.lengthat call sites:♻️ Proposed change
-const GIT_NULL_OID = '0'.repeat(40); @@ async updateRef({ ref, newOid, expectedOldOid }) { const args = ['update-ref', ref, newOid]; if (expectedOldOid !== undefined) { - args.push(expectedOldOid ?? GIT_NULL_OID); + args.push(expectedOldOid ?? '0'.repeat(newOid.length)); } await this.policy.execute(() => this.plumbing.execute({ args }), ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/infrastructure/adapters/GitRefAdapter.js` around lines 15 - 16, GIT_NULL_OID is hardcoded to 40 bytes which breaks repos using non-SHA1 OIDs; remove or stop using the constant GIT_NULL_OID and instead generate the null OID dynamically at each call site by using '0'.repeat(newOid.length) (or equivalent) where newOid is the computed OID passed into functions like updateRef/updateRefWithCAS in GitRefAdapter.js so the null-OID length matches the repository's active object format.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/infrastructure/adapters/GitRefAdapter.js`:
- Around line 15-16: GIT_NULL_OID is hardcoded to 40 bytes which breaks repos
using non-SHA1 OIDs; remove or stop using the constant GIT_NULL_OID and instead
generate the null OID dynamically at each call site by using
'0'.repeat(newOid.length) (or equivalent) where newOid is the computed OID
passed into functions like updateRef/updateRefWithCAS in GitRefAdapter.js so the
null-OID length matches the repository's active object format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c0cbb26-687c-41f9-86df-80469e677f15
📒 Files selected for processing (11)
CHANGELOG.mddocs/VAULT_INTERNALS.mdsrc/domain/helpers/gitRefErrors.jssrc/domain/services/VaultPersistence.jssrc/infrastructure/adapters/GitRefAdapter.jstest/unit/docs/test-style.test.jstest/unit/domain/services/VaultPersistence.test.jstest/unit/infrastructure/adapters/GitRefAdapter.test.jstest/unit/types/declaration-accuracy.test.jstest/unit/vault/VaultService.privacy.test.jstest/unit/vault/VaultService.verifier.test.js
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (7)
- test/unit/types/declaration-accuracy.test.js
- test/unit/infrastructure/adapters/GitRefAdapter.test.js
- test/unit/vault/VaultService.verifier.test.js
- test/unit/docs/test-style.test.js
- test/unit/domain/services/VaultPersistence.test.js
- test/unit/vault/VaultService.privacy.test.js
- src/domain/services/VaultPersistence.js
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
This PR prepares the v6.0.0 vault/service hardening branch for review. It decomposes the vault implementation into cohesive collaborators, centralizes domain error codes, tightens diagnostic behavior, and updates the public and maintainer documentation for the v6 security posture.
Key changes include:
VaultServiceinto an orchestrator backed byVaultPersistence,VaultStateCache,VaultMetadataCodec,VaultTreeCodec,VaultPrivacyIndex,VaultKeyVerifier, andVaultMutationRetryPolicy.Slugvalue object and codecs.VAULT_HEAD_INVALIDinstead of being treated as missing vaults..privacy-indexdoes not cover every HMAC tree entry.Why
The v6 release branch had accumulated security and architecture blockers around vault responsibilities, error consistency, and diagnostics. The root cause was that
VaultServiceowned too many concerns at once: Git persistence details, state caching, metadata decoding, privacy index resolution, key verification, and retry timing. That made correctness-sensitive behavior harder to isolate and test.This PR moves those concerns into explicit collaborators and adds behavior-level regression coverage for the release blockers.
Impact
CasErrorfrom the package root for typed error handling.Validation
npx eslint .npm test— 183 test files, 1598 passed, 2 skippedcycle/vault-service-decomposition.Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
Documentation