v3.0.0 — Vault #7
Conversation
Add vault subsystem backed by refs/cas/vault with init, add, list, remove, resolve, and metadata APIs. Purge completed milestones M1–M7 from ROADMAP (3,153 → 1,675 lines).
Extract all vault logic from facade into VaultService with proper hexagonal architecture. Add GitRefPort/GitRefAdapter for ref/commit operations. Add vault info and vault history CLI commands. Add vault integration tests.
|
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. 📝 WalkthroughWalkthroughThis PR introduces a Vault subsystem: a ref-based, GC-safe vault index with optional encryption and CLI management; it adds VaultService, GitRefPort/GitRefAdapter, facade/vault APIs, CLI vault commands, docs/tests, and removes AUDITS.md. Changes
Sequence DiagramsequenceDiagram
participant Client as CLI/Client
participant CAS as ContentAddressableStore
participant Vault as VaultService
participant GitRef as GitRefAdapter
participant GitPlumb as Git Plumbing
participant Crypto as Crypto Port
Client->>CAS: store(file, { --vault-passphrase? })
CAS->>Vault: addToVault({ slug, treeOid })
Vault->>Crypto: deriveKey(metadata?, passphrase?)
Crypto-->>Vault: encryptionKey
Vault->>GitRef: resolveRef(refs/cas/vault)
GitRef->>GitPlumb: execute(rev-parse ...)
GitPlumb-->>GitRef: commitOid
Vault->>Vault: readState() / validateSlug / prepare tree entries
Vault->>GitRef: createCommit({ treeOid, parentOid, message })
GitRef->>GitPlumb: execute(commit-tree ...)
GitPlumb-->>GitRef: newCommitOid
Vault->>GitRef: updateRef({ ref, newOid, expectedOldOid })
GitRef->>GitPlumb: execute(update-ref ...)
GitPlumb-->>GitRef: success
Vault-->>CAS: { commitOid }
CAS-->>Client: { treeOid, commitOid }
Client->>CAS: restore({ --slug | --oid }, { --vault-passphrase? })
CAS->>Vault: resolveVaultEntry({ slug })
Vault->>GitRef: resolveRef/resolveTree as needed
Vault-->>CAS: treeOid
CAS->>Crypto: deriveKey(metadata?, passphrase?)
Crypto-->>CAS: decryptionKey
CAS-->>Client: decrypted content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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: 6
🤖 Fix all issues with AI agents
In `@bin/git-cas.js`:
- Around line 47-62: resolveEncryptionKey currently returns undefined silently
when opts.vaultPassphrase is provided but metadata.encryption is falsy; update
resolveEncryptionKey to emit a clear warning to stderr (e.g., via console.warn
or process.stderr.write) in the branch after const metadata = await
cas.getVaultMetadata() when metadata?.encryption is not set, stating that the
provided --vault-passphrase will be ignored because the vault is not encrypted;
keep the existing return undefined behavior but ensure the message references
opts.vaultPassphrase, cas.getVaultMetadata, and metadata.encryption so users
understand why their passphrase had no effect.
In `@docs/API.md`:
- Line 1384: Update the API docs row for the VAULT_METADATA_INVALID error to
reference the actual exported method name: change the "Thrown By" value from
`_readVaultState()` to `readState()` so it matches the VaultService
implementation (readState) and avoids confusion for consumers; ensure any other
doc references using `_readVaultState()` are replaced with `readState()` for
consistency.
- Line 734: The docs import uses an invalid subpath "@git-stunts/cas/vault";
either change the example to a valid exported path (e.g., import VaultService
from '@git-stunts/cas/service' or import from the package root '@git-stunts/cas'
if the root re-exports VaultService) or add the "./vault" subpath to
package.json exports; update the line referencing "VaultService" accordingly so
it matches one of the existing exports (".", "./service", or "./schema").
- Around line 492-497: The fenced code block that displays "refs/cas/vault →
commit → tree" and the subsequent tree entries is missing a language identifier;
update the opening fence from ``` to ```text (i.e., add the language identifier
"text") so markdownlint MD040 is satisfied and the block is treated as plain
text.
In `@GUIDE.md`:
- Around line 975-983: The fenced code block under the "Vault Tree Structure"
section (the block containing "refs/cas/vault → commit → tree" and the file/tree
lines) is missing a language specifier; update the opening fence from ``` to
```text so the block is tagged as plain text (i.e., change the fenced block that
wraps the refs/cas/vault diagram to use ```text).
In `@src/domain/services/VaultService.js`:
- Around line 104-123: The metadata validator (static `#validateMetadata`)
currently requires cipher, kdf.algorithm and kdf.salt but omits kdf.keyLength;
update `#validateMetadata` to also require kdf.keyLength (matching VaultMetadata
and `#buildEncryptionMeta`) by checking it exists and is a positive integer (or
valid number) and include it in the thrown CasError payload when missing/invalid
so manually edited .vault.json files fail validation early.
🧹 Nitpick comments (8)
test/integration/vault.test.js (2)
184-220: Encrypted vault tests only cover init/metadata — no encrypted store→restore round-trip.The encrypted vault block verifies KDF metadata storage and the re-init guard, but doesn't exercise the full encrypted workflow: store with
--vault-passphrase→ add to vault → resolve → restore with passphrase → assert buffer equality. This would validate thatderiveVaultKeyand the end-to-end encrypted flow work through the real stack.
47-52:tempFiledirs leak if a test assertion fails before thermSynccall.Minor concern since these tests run in Docker, but you could move cleanup to
afterAll/afterEachor collect dirs for bulk cleanup. Not blocking.bin/git-cas.js (3)
196-197: Vault passphrase is visible in the process argument list.
--vault-passphrase <pass>passes the secret as a CLI argument, which is visible viaps,/proc/*/cmdline, shell history, etc. Consider supporting--vault-passphrase-stdinor an environment variable (GIT_CAS_VAULT_PASSPHRASE) as a more secure alternative. Not blocking for this PR, but worth tracking.Also applies to: 159-159
275-294:vault historyhardcodesrefs/cas/vaultinstead of reusingVaultService.VAULT_REF.Line 284 uses the string literal
'refs/cas/vault'rather than importing the constant. If the ref path ever changes, this will silently diverge.♻️ Use the constant
+import VaultService from '../src/domain/services/VaultService.js'; // ... - const args = ['log', '--oneline', 'refs/cas/vault']; + const args = ['log', '--oneline', VaultService.VAULT_REF];
64-76:readManifestFromTreeduplicates functionality fromcas.readManifest({ treeOid }), but direct substitution would introduce subtle behavior changes.The helper uses
startsWith('manifest.')for entry matching (permissive), while the facade uses exact match against the codec-specific extension. Additionally, the facade handles v2 manifests with submanifest resolution—a feature the helper lacks. If the vault context never uses these extended features and entry matching is intended to be lenient, the refactoring is valid; otherwise, verify the behavioral equivalence before simplifying.test/unit/vault/VaultService.test.js (1)
492-536: CAS retry tests use realsetTimeoutdelays — consider using fake timers.The retry logic in VaultService uses real
setTimeoutwith exponential backoff (50ms, 100ms). In the exhausted test (3 retries), this adds ~150ms of real wall-clock time. While tolerable now, as the test suite grows this can accumulate.Consider using
vi.useFakeTimers()and advancing timers to make these tests deterministic and faster.src/domain/services/VaultService.js (2)
223-237:#casUpdateReftreats all errors asVAULT_CONFLICT— intentional but worth a doc note.Any failure from
updateRef(including transient I/O errors, permission issues, etc.) is wrapped asVAULT_CONFLICT. This is safe from a correctness standpoint since the caller will retry, but it means non-conflict errors get masked. A caller debugging a permission issue would seeVAULT_CONFLICTinstead of the root cause.Consider preserving the original error in the meta for diagnostics:
Proposed fix
- } catch { + } catch (err) { throw new CasError( 'Concurrent vault update detected', 'VAULT_CONFLICT', - { expectedParent: expectedOldOid, newCommit: newOid }, + { expectedParent: expectedOldOid, newCommit: newOid, originalError: err }, ); }
300-322:initVaultdoes not use#retryMutation, so it's not CAS-safe against concurrent calls.Unlike
addToVaultandremoveFromVault,initVaultcallswriteCommitdirectly. If two concurrentinitVaultcalls race, the second will get aVAULT_CONFLICTwithout automatic retry. While this is documented in the error codes table, it's an inconsistency in the retry posture.This may be acceptable given
initVaultis typically called once, but flagging for awareness.
…tests - Percent-encode slashes in vault slugs so git tree entries are valid - Remove redundant alfred retry policy from adapters (plumbing already retries) - Add Dockerized vault CLI integration test exercising all subcommands - Configure git identity env vars in docker-compose for commit-tree support
flyingrobots
changed the title
#validateMetadata now rejects encryption metadata missing kdf.keyLength, preventing downstream KDF failures from manually edited .vault.json files.
#casUpdateRef now includes the original error in the VAULT_CONFLICT meta for better diagnostics when non-conflict errors (e.g., permissions) occur.
resolveEncryptionKey now emits a stderr warning when a passphrase is provided but the vault has no encryption config, preventing silent unencrypted store/restore surprises.
Replace hardcoded 'refs/cas/vault' string with ContentAddressableStore.VAULT_REF to prevent divergence.
- Add `text` language identifier to fenced code blocks (MD040) - Fix invalid import path @git-stunts/cas/vault → @git-stunts/cas - Fix _readVaultState() → readState() in error codes table
- Extract #validateEncryption helper to reduce #validateMetadata complexity - Split kdf.keyLength test into own describe block to stay under max-lines
|
@coderabbitai Please review 🥕 |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/infrastructure/adapters/GitPersistenceAdapter.js (1)
25-25:⚠️ Potential issue | 🟡 MinorStale JSDoc: still mentions "2 retries".
The
@paramdescription says "30 s timeout + 2 retries" but the retry layer was removed. Line 19 was updated; this one was missed.📝 Proposed fix
- * `@param` {import('@git-stunts/alfred').Policy} [options.policy] - Resilience policy (defaults to 30 s timeout + 2 retries). + * `@param` {import('@git-stunts/alfred').Policy} [options.policy] - Resilience policy (defaults to 30 s timeout, no retry).
🤖 Fix all issues with AI agents
In `@bin/git-cas.js`:
- Line 12: Update the CLI version string from '2.0.0' to '3.0.0' by changing the
argument passed to the .version(...) call (the .version method on the
commander/CLI setup in bin/git-cas.js) so the CLI reports the correct v3.0.0
release.
🧹 Nitpick comments (6)
docker-compose.yml (1)
8-11: LGTM — sensible git identity for containerised tests.Setting explicit author/committer env vars avoids
"Please tell me who you are"errors inside containers that lack a global git config. Values are clearly synthetic, no real PII.Minor DRY nit: the same four vars are copy-pasted into all three services. A YAML anchor keeps them in sync:
♻️ Optional: deduplicate with a YAML anchor
+x-git-env: &git-env + - GIT_AUTHOR_NAME=git-cas-test + - GIT_AUTHOR_EMAIL=test@git-cas.local + - GIT_COMMITTER_NAME=git-cas-test + - GIT_COMMITTER_EMAIL=test@git-cas.local + services: test-node: build: context: . target: node environment: - GIT_STUNTS_DOCKER=1 - - GIT_AUTHOR_NAME=git-cas-test - - GIT_AUTHOR_EMAIL=test@git-cas.local - - GIT_COMMITTER_NAME=git-cas-test - - GIT_COMMITTER_EMAIL=test@git-cas.local + *git-env(Repeat for
test-bunandtest-deno.)src/domain/services/VaultService.js (2)
271-290: CAS retry delay usessetTimeout— consider timer cleanup for testability.The retry loop's
setTimeout(line 285) works but means tests exercising retries incur real wall-clock delays (50ms, 100ms). This is acceptable at current scale (MAX_CAS_RETRIES = 3, max total ~350ms) but worth noting if retry counts increase.
326-348:initVaultdoesn't use#retryMutation— concurrent inits will throwVAULT_CONFLICT.Unlike
addToVault/removeFromVault,initVaultcallswriteCommitdirectly without retry. A concurrentinitVaultwould fail withVAULT_CONFLICTand no automatic retry. This seems intentional for a one-time setup operation, but it's worth confirming that callers (CLI) are prepared for this.bin/git-cas.js (1)
276-295:vault historybuilds git args from user input — injection risk is mitigated by array-based execution.
opts.maxCountis interpolated into a flag string (-${opts.maxCount}), but sinceplumbing.execute({ args })uses an args array rather than shell string interpolation, this is safe from command injection. However, non-numeric input will produce a confusing git error.Consider parsing
maxCountas an integer for friendlier validation:Proposed improvement
.option('-n, --max-count <n>', 'Limit number of commits') .action(async (opts) => { try { const runner = ShellRunnerFactory.create(); const plumbing = new GitPlumbing({ runner, cwd: opts.cwd || '.' }); const args = ['log', '--oneline', ContentAddressableStore.VAULT_REF]; if (opts.maxCount) { + const n = parseInt(opts.maxCount, 10); + if (Number.isNaN(n) || n <= 0) { + process.stderr.write('error: --max-count must be a positive integer\n'); + process.exit(1); + } - args.push(`-${opts.maxCount}`); + args.push(`-${n}`); }test/integration/vault-cli.test.js (1)
52-57: Shared mutable state (storeOid,repoDir) across describe blocks creates implicit ordering dependency.
storeOidis set in the first describe block and implicitly relied upon by assertions invault list(line 86). The restore/remove describe block also depends on the vault state from the first block. This works because vitest runs describe blocks sequentially within a file, but it makes the tests brittle if anyone reorders them.This is a common trade-off in integration tests; just flagging for awareness.
Also applies to: 72-129
test/unit/vault/VaultService.test.js (1)
142-159: Unit tests don't exercisedecodeSlugwith percent-encoded tree entry names.Mock tree entries use raw names like
'demo/hello'and'a/b/c'rather than the percent-encoded forms ('demo%2Fhello','a%2Fb%2Fc') thatreadTreewould return in production. TheencodeSlugpath is well-tested (line 244 checks for'demo%2Fhello'), but thedecodeSlugpath is only exercised by integration tests.Consider adding a test with encoded mock names to verify the full round-trip at the unit level:
Example test for decode path
it('decodes percent-encoded tree entry names', async () => { const ref = mockRef(); const persistence = mockPersistence(); setupExistingVault({ ref, persistence, metaJson: JSON.stringify({ version: 1 }), entries: [ { mode: '040000', type: 'tree', oid: 'tree-a', name: 'demo%2Fhello' }, ] }); const vault = createVault({ ref, persistence }); const state = await vault.readState(); expect(state.entries.get('demo/hello')).toBe('tree-a'); });Also applies to: 313-339
- CLI version string 2.0.0 → 3.0.0 - Stale JSDoc in GitPersistenceAdapter (remove mention of retries) - Validate --max-count as positive integer in vault history - Add decodeSlug round-trip unit test with percent-encoded entry names - Update CHANGELOG with final fixes and test count
1 participant
Summary
refs/cas/vault. Assets indexed by slug;git gccan no longer silently discard stored data.VaultServicedomain service withGitRefPort/GitRefAdapterfor ref operations.vault init,vault list,vault info,vault remove,vault history.store --treeauto-vaults.restoreuses--oid/--slugflags (breaking: no more positional arg).package.json,jsr.json), CHANGELOG stamped, README install section + "What's new in v3.0.0" blurb.Test plan
npm test)npx eslint .)v3.0.0, push tag, CI publishes to npm + JSRSummary by CodeRabbit
New Features
Breaking Changes
--oidor--slugflags; positional tree-oid argument removed.Documentation