feat: M8 Spit Shine + M9 Cockpit — code review polish & CLI improvements#11
Merged
flyingrobots merged 6 commits intomainfrom Feb 28, 2026
Merged
feat: M8 Spit Shine + M9 Cockpit — code review polish & CLI improvements#11flyingrobots merged 6 commits intomainfrom
flyingrobots merged 6 commits intomainfrom
Conversation
…leanup - Refactor CryptoPort and all adapters (Node, Bun, Web) for key validation - Move completed task cards to COMPLETED_TASKS.md, superseded to GRAVEYARD.md - Add STATUS.md, ADR-001, and documentation links to README - Update ROADMAP.md to reflect current milestone state
… formatting - Add bin/actions.js with runAction() error wrapper and actionable HINTS - Replace all 10 identical catch blocks with runAction() - Add global --json flag with structured JSON output for 8 commands - Add verify command for integrity checking (exit 0/1) - Add vault list --filter with glob matching and TTY-aware table formatting - Convert validateRestoreFlags and vault history validation to throw
Replace process.exit(1) with exitCode, include encryption in vault info JSON, accept Uint8Array in NodeCryptoAdapter, guard matchGlob pattern length, harden writeError for non-Error throws, remove redundant _validateKey call, fix ADR-001 method names, update test count and CHANGELOG.
Member
Author
|
@coderabbitai review please 🥕 |
|
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 (9)
📝 WalkthroughWalkthroughThis PR introduces comprehensive fixes and refactoring across documentation, CLI infrastructure, and cryptographic services. Key changes include: centralizing CLI error handling with JSON output support, delegating key validation from CasService to CryptoPort, refactoring crypto adapter KDF flows via _doDeriveKey hooks, implementing vault list utilities with glob pattern matching, and updating documentation to reflect completed milestones and architectural patterns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
|
✅ 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: 5
🧹 Nitpick comments (4)
test/unit/cli/vault-list.test.js (1)
4-34: Consider adding test for pattern length limit.The CHANGELOG mentions that
matchGlobrejects patterns longer than 200 characters to prevent ReDoS. Consider adding a test case to verify this behavior:it('rejects patterns longer than 200 characters', () => { const longPattern = 'a'.repeat(201); expect(() => matchGlob(longPattern, 'test')).toThrow(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/cli/vault-list.test.js` around lines 4 - 34, Add a unit test that verifies matchGlob enforces the 200-character pattern limit by asserting that passing a pattern longer than 200 characters throws; create a test case (e.g., "rejects patterns longer than 200 characters") that builds a string of length 201 (e.g., 'a'.repeat(201)) and calls matchGlob(longPattern, 'test') inside an expect(...).toThrow() assertion so the behavior is covered in the matchGlob tests.STATUS.md (1)
26-39: Task checkboxes may need updating post-merge.The M8 and M9 task items are shown as unchecked (
[ ]), but this PR implements these features. Consider updating the checkboxes to[x]after merging, or clarify if STATUS.md is meant to reflect pre-merge state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@STATUS.md` around lines 26 - 39, The task checklist in STATUS.md still shows M8 and M9 items as unchecked although this PR implements them; update the three affected checkboxes to checked (change "- [ ] **8.2** Extract shared crypto helpers to CryptoPort base class", "- [ ] **8.3** README polish and architectural decision record (ADR-001)", "- [ ] **9.2** CLI `verify` command (`git cas verify --slug <slug>`)","- [ ] **9.3** CLI `--json` output mode (structured JSON for all commands)","- [ ] **9.4** CLI error handler DRY cleanup + actionable error messages","- [ ] **9.5** Vault list filtering (`--filter`) and table formatting" to "- [x] ..." as appropriate, or add a short note clarifying that STATUS.md represents pre-merge state if you prefer not to mark them; commit the change to STATUS.md in this PR so the merged README reflects the implemented work.src/infrastructure/adapters/NodeCryptoAdapter.js (2)
68-80: Orphaned comment on line 68.Line 68 has an incomplete JSDoc-style comment that appears orphaned—it describes delegation to the base class but doesn't attach to any method. The actual
_doDeriveKeymethod has its own@overridetag on line 70.🧹 Proposed fix to remove the orphaned comment
- /** `@override` – delegate to base class which accepts both Buffer and Uint8Array */ - /** `@override` */ async _doDeriveKey(passphrase, saltBuf, { algorithm, iterations, cost, blockSize, parallelization, keyLength }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/adapters/NodeCryptoAdapter.js` around lines 68 - 80, The file has an orphaned JSDoc comment above the _doDeriveKey method; remove the stray comment (or merge its text into the existing `@override` JSDoc for the async _doDeriveKey function) so the documentation properly attaches to the method—update NodeCryptoAdapter.js by deleting the incomplete comment preceding async _doDeriveKey or consolidating it into the method's JSDoc block.
70-80: Verify_doDeriveKeyshould be async despite synchronous promisify returns.The method is not marked
async, yet it returns promises frompromisify(pbkdf2)(...)andpromisify(scrypt)(...). This works correctly since the base class likely awaits the result, but addingasyncwould make the intent clearer and ensure consistent error wrapping in a rejected promise if synchronous code before the return throws.✨ Proposed improvement for clarity
/** `@override` */ - _doDeriveKey(passphrase, saltBuf, { algorithm, iterations, cost, blockSize, parallelization, keyLength }) { + async _doDeriveKey(passphrase, saltBuf, { algorithm, iterations, cost, blockSize, parallelization, keyLength }) { if (algorithm === 'pbkdf2') { return promisify(pbkdf2)(passphrase, saltBuf, iterations, keyLength, 'sha512'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/adapters/NodeCryptoAdapter.js` around lines 70 - 80, _mark _doDeriveKey_ should be declared async and should await the promisified operations to ensure synchronous thrown errors are wrapped as rejected promises; update the _doDeriveKey_ implementation to use the async keyword (if not already) and change the returns to await promisify(pbkdf2)(...) and await promisify(scrypt)(...) so any exceptions from promisify/pbkdf2/scrypt are consistently propagated as promise rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/actions.js`:
- Around line 47-50: The catch block in runAction currently calls
process.exit(1); change it to set process.exitCode = 1 instead to match the
verify command pattern — locate the catch in the runAction handler where
writeError(err, getJson()) is called and replace the explicit process.exit call
with process.exitCode = 1 so the handler signals failure without forcing an
immediate exit.
In `@bin/git-cas.js`:
- Around line 108-133: The CLI currently ignores opts.force unless opts.tree is
set, which can mislead users; update the command handler in the runAction async
callback to validate flags early: if opts.force is true and opts.tree is
false/undefined, throw an error or call process.exit(1) with a clear message
(e.g., "The --force flag requires --tree") before calling cas.storeFile;
reference the options used in the handler (opts.force, opts.tree) and perform
the check near the start of the action callback so users fail fast rather than
silently having --force ignored.
- Around line 374-390: The vault history command currently always prints
human-readable output; update the action handler around GitPlumbing.execute (in
the runAction callback that uses ShellRunnerFactory.create and
ContentAddressableStore.VAULT_REF) to respect the global/command --json flag:
detect JSON mode (check opts.json and/or program.opts().json) and when true emit
structured JSON to stdout instead of calling renderHistoryTimeline — either by
parsing the git log output into an array of commit objects (commit id, message,
date) and writing JSON.stringify(...) or by wrapping the raw output in a JSON
field like { history: [...] } if a parser isn’t available; otherwise preserve
the current pretty/plain branches for non-JSON mode.
- Around line 176-188: The inspect command currently chooses a pretty TTY view
whenever stdout.isTTY is true, which ignores a global JSON flag; update the
output selection in the async handler passed to runAction so that opts.json (or
the global JSON flag on opts) takes precedence: keep the heatmap branch
(renderHeatmap), then if opts.json produce the JSON string of manifest.toJSON(),
otherwise if process.stdout.isTTY use renderManifestView, else fall back to
JSON; adjust the branches around renderManifestView/JSON in the anonymous async
function (the block that calls createCas, cas.readManifest and writes to
process.stdout) so JSON mode is honored even in TTY.
In `@bin/ui/vault-list.js`:
- Around line 17-22: The current glob-to-regex replacement uses .replace(/\?/g,
'.') which allows '?' to match '/', letting a single-segment wildcard cross path
separators; change the replacement so '?' becomes a single non-separator token
(e.g., use '[^/]' instead of '.'). Update the code that constructs the RegExp
(the chain of .replace calls that includes .replace(/\?/g, '.')) to use
.replace(/\?/g, '[^/]'). Keep the other replacements (escaping, ** -> .*, * ->
[^/]* via the existing logic) unchanged so '**' still spans segments while '*'
and '?' do not cross '/'.
---
Nitpick comments:
In `@src/infrastructure/adapters/NodeCryptoAdapter.js`:
- Around line 68-80: The file has an orphaned JSDoc comment above the
_doDeriveKey method; remove the stray comment (or merge its text into the
existing `@override` JSDoc for the async _doDeriveKey function) so the
documentation properly attaches to the method—update NodeCryptoAdapter.js by
deleting the incomplete comment preceding async _doDeriveKey or consolidating it
into the method's JSDoc block.
- Around line 70-80: _mark _doDeriveKey_ should be declared async and should
await the promisified operations to ensure synchronous thrown errors are wrapped
as rejected promises; update the _doDeriveKey_ implementation to use the async
keyword (if not already) and change the returns to await promisify(pbkdf2)(...)
and await promisify(scrypt)(...) so any exceptions from promisify/pbkdf2/scrypt
are consistently propagated as promise rejections.
In `@STATUS.md`:
- Around line 26-39: The task checklist in STATUS.md still shows M8 and M9 items
as unchecked although this PR implements them; update the three affected
checkboxes to checked (change "- [ ] **8.2** Extract shared crypto helpers to
CryptoPort base class", "- [ ] **8.3** README polish and architectural decision
record (ADR-001)", "- [ ] **9.2** CLI `verify` command (`git cas verify --slug
<slug>`)","- [ ] **9.3** CLI `--json` output mode (structured JSON for all
commands)","- [ ] **9.4** CLI error handler DRY cleanup + actionable error
messages","- [ ] **9.5** Vault list filtering (`--filter`) and table formatting"
to "- [x] ..." as appropriate, or add a short note clarifying that STATUS.md
represents pre-merge state if you prefer not to mark them; commit the change to
STATUS.md in this PR so the merged README reflects the implemented work.
In `@test/unit/cli/vault-list.test.js`:
- Around line 4-34: Add a unit test that verifies matchGlob enforces the
200-character pattern limit by asserting that passing a pattern longer than 200
characters throws; create a test case (e.g., "rejects patterns longer than 200
characters") that builds a string of length 201 (e.g., 'a'.repeat(201)) and
calls matchGlob(longPattern, 'test') inside an expect(...).toThrow() assertion
so the behavior is covered in the matchGlob tests.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CHANGELOG.mdCOMPLETED_TASKS.mdGRAVEYARD.mdREADME.mdROADMAP.mdSTATUS.mdbin/actions.jsbin/git-cas.jsbin/ui/vault-list.jsdocs/ADR-001-vault-in-facade.mdsrc/domain/services/CasService.jssrc/infrastructure/adapters/BunCryptoAdapter.jssrc/infrastructure/adapters/NodeCryptoAdapter.jssrc/infrastructure/adapters/WebCryptoAdapter.jssrc/ports/CryptoPort.jstest/unit/cli/actions.test.jstest/unit/cli/vault-list.test.jstest/unit/ports/CryptoPort.test.js
📝 WalkthroughWalkthroughThis pull request introduces CLI enhancements with centralized error handling, adds structured documentation for project milestones and completions, refactors cryptographic key validation from CasService to CryptoPort, and implements new utility modules for vault listing, filtering, and formatting. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
- runAction: process.exitCode = 1 instead of process.exit(1) - store: --force without --tree now fails fast - inspect: --json honored in TTY mode - vault history: --json emits structured array - matchGlob: ? no longer matches / separator - NodeCryptoAdapter: remove orphaned comment, await in _doDeriveKey - Tests: pattern length guard, ? separator, exitCode assertions
Member
Author
|
@coderabbitai review please 🥕 |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
bin/ui/vault-list.js (1)
16-22:⚠️ Potential issue | 🟡 Minor
?wildcard should not match/in slug globs.Line 21 maps
?to., so single-character matches can cross path separators. Use a non-separator token instead.🔧 Proposed fix
- .replace(/\?/g, '.'); + .replace(/\?/g, '[^/]');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ui/vault-list.js` around lines 16 - 22, The current glob-to-regex transformation in vault-list.js builds `escaped` and maps `?` to `.` which allows matching a `/`; change the `?` mapping so it does not match path separators by replacing the `.replace(/\?/g, '.')` step with a mapping that forbids `/` (e.g. `.replace(/\?/g, '[^/]')`) so the RegExp built from `escaped` will treat `?` as “any single non-separator character” when testing `str`.bin/actions.js (1)
47-50:⚠️ Potential issue | 🟡 MinorUse
process.exitCodeinstead ofprocess.exit(1)inrunAction.Line 49 still performs an immediate exit. Please set
process.exitCode = 1so command cleanup/output flushing remains consistent with the rest of the CLI error path.🔧 Proposed fix
} catch (err) { writeError(err, getJson()); - process.exit(1); + process.exitCode = 1; }#!/bin/bash # Verify exit signaling style in the action wrapper and its tests. rg -n "process\.exitCode|process\.exit\(" bin/actions.js test/unit/cli/actions.test.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/actions.js` around lines 47 - 50, In the catch block of the action wrapper (the runAction error handler) replace the immediate termination call process.exit(1) with setting process.exitCode = 1 so that writeError(err, getJson()) can finish flushing and normal cleanup runs; locate the catch that calls writeError and swap process.exit(1) for process.exitCode = 1 (leave writeError and getJson() unchanged).
🧹 Nitpick comments (2)
test/unit/cli/vault-list.test.js (1)
4-34: Consider adding a test for the pattern length guard.The
matchGlobimplementation rejects patterns longer than 200 characters to prevent ReDoS (per CHANGELOG entry M2), but there's no test verifying this behavior.📝 Suggested test case
it('handles exact match', () => { expect(matchGlob('exact', 'exact')).toBe(true); expect(matchGlob('exact', 'other')).toBe(false); }); + + it('throws for patterns exceeding 200 characters', () => { + const longPattern = 'a'.repeat(201); + expect(() => matchGlob(longPattern, 'test')).toThrow(/too long/); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/cli/vault-list.test.js` around lines 4 - 34, Add a unit test in the vault-list.test.js suite that verifies matchGlob enforces the pattern-length guard: construct a pattern string longer than 200 characters (e.g., repeat 'a' 201 times or build a long glob) and assert matchGlob(longPattern, 'anything') returns false (and optionally add a complementary assertion that a 200-character pattern is accepted if desired); reference the matchGlob function in the new test so the rejection behavior is covered.src/infrastructure/adapters/NodeCryptoAdapter.js (1)
68-69: Remove orphan comment.Line 68 contains an orphan JSDoc comment that doesn't precede any method. It appears to be a leftover from removed code (likely the old
deriveKeymethod that now delegates to the base class).♻️ Proposed fix
- /** `@override` – delegate to base class which accepts both Buffer and Uint8Array */ - /** `@override` */ async _doDeriveKey(passphrase, saltBuf, { algorithm, iterations, cost, blockSize, parallelization, keyLength }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/adapters/NodeCryptoAdapter.js` around lines 68 - 69, Remove the orphan JSDoc comment that sits by itself (the "override – delegate to base class which accepts both Buffer and Uint8Array") in the NodeCryptoAdapter class; it no longer precedes any method (the old deriveKey was removed/delegated), so delete that standalone comment near the deriveKey-related area to avoid confusion and keep comments aligned with their methods (search for NodeCryptoAdapter and deriveKey to locate the orphan).
🤖 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/infrastructure/adapters/WebCryptoAdapter.js`:
- Around line 87-89: The encrypt async generator uses a shared variable finalTag
and the finalize() path can encode a null finalTag if the encryption stream
hasn't been fully consumed; update the encrypt function (and any finalize()
helper it uses) to track completion (e.g., an internal boolean like isConsumed
or finalTagSet) and refuse to run finalize() until the generator has emitted its
end (throw a consistent STREAM_NOT_CONSUMED error when finalize() is called
early), ensuring you only call the encoder on a non-null finalTag and set/clear
the guard when the stream finishes in the encrypt generator.
---
Duplicate comments:
In `@bin/actions.js`:
- Around line 47-50: In the catch block of the action wrapper (the runAction
error handler) replace the immediate termination call process.exit(1) with
setting process.exitCode = 1 so that writeError(err, getJson()) can finish
flushing and normal cleanup runs; locate the catch that calls writeError and
swap process.exit(1) for process.exitCode = 1 (leave writeError and getJson()
unchanged).
In `@bin/ui/vault-list.js`:
- Around line 16-22: The current glob-to-regex transformation in vault-list.js
builds `escaped` and maps `?` to `.` which allows matching a `/`; change the `?`
mapping so it does not match path separators by replacing the `.replace(/\?/g,
'.')` step with a mapping that forbids `/` (e.g. `.replace(/\?/g, '[^/]')`) so
the RegExp built from `escaped` will treat `?` as “any single non-separator
character” when testing `str`.
---
Nitpick comments:
In `@src/infrastructure/adapters/NodeCryptoAdapter.js`:
- Around line 68-69: Remove the orphan JSDoc comment that sits by itself (the
"override – delegate to base class which accepts both Buffer and Uint8Array") in
the NodeCryptoAdapter class; it no longer precedes any method (the old deriveKey
was removed/delegated), so delete that standalone comment near the
deriveKey-related area to avoid confusion and keep comments aligned with their
methods (search for NodeCryptoAdapter and deriveKey to locate the orphan).
In `@test/unit/cli/vault-list.test.js`:
- Around line 4-34: Add a unit test in the vault-list.test.js suite that
verifies matchGlob enforces the pattern-length guard: construct a pattern string
longer than 200 characters (e.g., repeat 'a' 201 times or build a long glob) and
assert matchGlob(longPattern, 'anything') returns false (and optionally add a
complementary assertion that a 200-character pattern is accepted if desired);
reference the matchGlob function in the new test so the rejection behavior is
covered.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CHANGELOG.mdCOMPLETED_TASKS.mdGRAVEYARD.mdREADME.mdROADMAP.mdSTATUS.mdbin/actions.jsbin/git-cas.jsbin/ui/vault-list.jsdocs/ADR-001-vault-in-facade.mdsrc/domain/services/CasService.jssrc/infrastructure/adapters/BunCryptoAdapter.jssrc/infrastructure/adapters/NodeCryptoAdapter.jssrc/infrastructure/adapters/WebCryptoAdapter.jssrc/ports/CryptoPort.jstest/unit/cli/actions.test.jstest/unit/cli/vault-list.test.jstest/unit/ports/CryptoPort.test.js
Throw STREAM_NOT_CONSUMED error if finalize() is called before the encrypt stream is fully consumed, preventing null finalTag encoding.
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
CryptoPortbase class), key validation hardening, roadmap cleanup (purge completed tasks, add STATUS.md, ADR-001)runActionerror handler,--jsonoutput mode for all commands,verifycommand, vault list--filter+ table formattingprocess.exitCodeoverprocess.exit, vault info JSON encryption metadata,Uint8Arrayacceptance inNodeCryptoAdapter,matchGlobReDoS guard, non-Error throw handling, redundant_validateKeyremoval, ADR method name correctionsTest plan
npm test— 616 tests passnpx eslint .— 0 errorsnode bin/git-cas.js --help— spot-check CLI outputSummary by CodeRabbit
Release Notes
New Features
--jsonflag across commands for structured outputverifycommand for asset integrity checksvault listwith--filterpattern supportvault infonow displays encryption metadata with--encryptionflagDocumentation
Tests