Package#1
Conversation
- Implements Hexagonal Architecture - Adds VaultService and KeychainAdapter - Comprehensive unit tests - Documentation - Proper .gitignore
…ling - Introduce smart `npm test` and `benchmark` wrappers that auto-detect execution context; commands now auto-boot Docker containers if run from the host, ensuring reproducible results. - Standardize code style/IDE config via .editorconfig and .prettierrc. - Update Docker build context to support monorepo sibling dependencies (mounting `plumbing` alongside `vault`). - Remove local file dependencies from package.json in favor of container-time assembly.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new Vault package: domain services, errors, a KeychainAdapter for OS-native secret storage, an index facade, tests, documentation, and project tooling/config (linting, formatting, Docker, CI-oriented scripts). Changes
Sequence DiagramssequenceDiagram
participant User
participant Vault as index.js (Vault)
participant Service as VaultService
participant Adapter as KeychainAdapter
participant OS as OS Keychain
User->>Vault: getSecret({ target })
Vault->>Service: getSecret(target)
Service->>Adapter: get(target)
Adapter->>OS: platform-specific query (security/secret-tool/PowerShell)
OS-->>Adapter: credential value
Adapter-->>Service: return value
Service-->>Vault: return value
Vault-->>User: secret
sequenceDiagram
participant User
participant Vault as index.js (Vault)
participant Service as VaultService
participant Env as process.env
participant Adapter as KeychainAdapter
User->>Vault: resolveSecret({ envKey, vaultTarget })
Vault->>Service: resolveSecret(envKey, vaultTarget)
Service->>Env: check envKey
alt env present
Env-->>Service: env value
Service-->>Vault: env value
Vault-->>User: env value
else env missing
Service->>Adapter: get(vaultTarget)
alt success
Adapter-->>Service: vault value
Service-->>Vault: vault value
Vault-->>User: vault value
else failure
Service-->>Vault: null
Vault-->>User: null
end
end
sequenceDiagram
participant User
participant Vault as index.js (Vault)
participant Service as VaultService
participant TTY as readline (TTY)
participant Adapter as KeychainAdapter
participant OS as OS Keychain
User->>Vault: ensureSecret({ target, promptMessage })
Vault->>Service: ensureSecret(target, promptMessage)
Service->>Adapter: get(target)
alt secret exists
Adapter-->>Service: value
Service-->>Vault: value
Vault-->>User: value
else secret missing
Service->>TTY: prompt (masked)
TTY->>User: prompt
User-->>TTY: input (masked)
TTY-->>Service: input value
Service->>Adapter: set(target, value)
Adapter->>OS: store credential
OS-->>Adapter: success
Adapter-->>Service: confirmed
Service-->>Vault: value
Vault-->>User: provisioned secret
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~85 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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: 2
🤖 Fix all issues with AI agents
In @src/domain/services/VaultService.js:
- Around line 96-118: The promise executor currently only has (resolve) and
throws inside the rl.question callback (in rl.question and this.setSecret) which
will crash instead of rejecting; change the Promise constructor to accept
(resolve, reject), replace the thrown Error('Secret cannot be empty') with
reject(new Error(...)) (or call reject with the trimmed-empty error), and wrap
the this.setSecret(target, trimmed) call in a try/catch that calls reject(err)
on failure (ensuring rl.close() and process.stderr.write('\n') still run before
resolving/rejecting).
In @src/infrastructure/adapters/KeychainAdapter.js:
- Around line 73-85: The PowerShell script in KeychainAdapter.js interpolates
this.account without escaping which can break the command or allow injection;
update the psLiteral helper (used in the block where this.platform === 'win32')
or call it for the account value and use that escaped value in the script string
(the same way psLiteral(value) and psLiteral(target) are used) so the
New-StoredCredential -UserName argument receives a properly quoted/escaped
string; ensure you replace the direct '${this.account}' interpolation with the
escaped token before calling spawnSync.
🧹 Nitpick comments (15)
.gitignore (1)
1-3: Consider adding common Node.js ignore patterns.The current patterns are correct, but typical Node.js projects benefit from additional entries such as coverage reports (
coverage/), build outputs (dist/,lib/,*.tsbuildinfo), log files (*.log), and environment files (.env,.env.local).📝 Suggested additions
node_modules/ .DS_Store .vite/ + +# Test coverage +coverage/ +*.lcov + +# Build outputs +dist/ +lib/ +*.tsbuildinfo + +# Logs +*.log +npm-debug.log* +yarn-debug.log* +yarn-error.log* + +# Environment +.env +.env.local +.env.*.local + +# IDE +.idea/ +.vscode/ +*.swp +*.swo +*~ + +# OS +Thumbs.dbCONTRIBUTING.md (1)
16-19: Consider adding conventional commit examples.While the guidance to use conventional commits is clear, developers unfamiliar with the format might benefit from a brief example (e.g.,
feat: add new feature,fix: resolve bug) or a link to the specification at https://www.conventionalcommits.org/.📝 Suggested addition
## Style Guide - Use `ESLint` and `Prettier`. -- Commit messages should follow conventional commits. +- Commit messages should follow [conventional commits](https://www.conventionalcommits.org/): + - `feat: add new feature` + - `fix: resolve bug` + - `docs: update documentation` + - `chore: update dependencies`docker-compose.yml (1)
1-7: LGTM! Consider adding a .dockerignore for build optimization.The Docker Compose configuration is clean and functional for a test service. The parent build context aligns with the monorepo structure described in the commit messages.
To optimize build performance and avoid sending unnecessary files to the Docker daemon (especially with the parent directory as build context), consider adding a
.dockerignorefile at the repository root.🐳 Example .dockerignore at repository root
Create a
.dockerignorefile in the parent directory:node_modules/ .git/ .vite/ coverage/ *.log .DS_Store Thumbs.db .env .env.local.dockerignore (1)
1-9: Consider excluding.gitdirectory to reduce build context size.The current ignore patterns are sensible. You might want to add
.gitto the ignore list, as the git history can significantly increase the Docker build context size and isn't needed for the build.💡 Suggested addition
node_modules npm-debug.log .gitignore *.md !README.md .DS_Store coverage .vscode .idea +.gitpackage.json (2)
1-31: Consider adding anenginesfield for Node.js version requirement.The Dockerfile uses
node:22-slim, but there's noenginesfield to document the minimum Node.js version. This helps catch version mismatches early for developers running locally.💡 Suggested addition
"license": "Apache-2.0", + "engines": { + "node": ">=22" + }, "dependencies": {
22-24: zod@3.24.1 is valid and secure, but consider upgrading to the current version.The version is a valid published release (2024-12-11) with no known vulnerabilities. However, zod v4.3.5 is the current version. The caret constraint
^3.24.1locks the package to v3.x and prevents automatic upgrades to v4, which may include improvements and bug fixes. Consider updating the dependency to a recent v4 version for better long-term maintainability.Dockerfile (1)
6-6: Consider usingnpm cifor reproducible builds.
npm installcan produce different dependency trees across builds. Since apackage-lock.jsonis present, usingnpm ciensures deterministic, reproducible installs and is faster in Docker environments.♻️ Proposed fix
-RUN npm install +RUN npm cieslint.config.js (1)
17-24: Consider adding overrides for test files.The configuration is well-structured with good security rules. However,
no-console: "error"may be overly restrictive if the CLI needs user output, andmax-lines-per-function: 50could be limiting for test files which often have longer setup/assertion blocks.💡 Optional: Add test file overrides
export default [ js.configs.recommended, { languageOptions: { ecmaVersion: 2022, sourceType: "module", globals: { process: "readonly", Buffer: "readonly", console: "readonly", setTimeout: "readonly", clearTimeout: "readonly" } }, rules: { // ... existing rules } - } + }, + { + files: ["test/**/*.js"], + rules: { + "max-lines-per-function": "off", + "max-nested-callbacks": "off" + } + } ];ARCHITECTURE.md (2)
20-27: Add language specifier to fenced code block.The directory structure visualization is helpful. Per markdownlint (MD040), specify a language for the fenced code block (e.g.,
textorplaintext).📝 Proposed fix
-``` +```text src/ ├── domain/ │ ├── errors/ # VaultError, etc.
11-11: Consider listing all domain errors for completeness.
PlatformNotSupportedErroris also part of the domain errors (as shown in the codebase) but isn't mentioned here. This is minor since "etc." covers it, but explicit listing could help developers discover available error types.README.md (1)
28-41: Clarify sync vs async API behavior.The examples show
getSecretandresolveSecretas synchronous whileensureSecretusesawait. This is reasonable (interactive prompt needs async), but consider adding a brief note in the documentation about which methods are sync vs async, especially given the audit's recommendation to make all methods async.test/unit/domain/services/VaultService.test.js (1)
7-52: Test suite covers core happy paths but lacks coverage fordeleteSecretandensureSecret.The existing tests are well-structured and correctly validate the primary flows. Consider adding tests for:
deleteSecretmethod validation and adapter interactionensureSecretasync behavior (may require mockingreadlineandprocess.stdin.isTTY)- Edge case:
resolveSecretwhengetSecretthrows (verifying the catch block returnsnull)src/infrastructure/adapters/KeychainAdapter.js (2)
55-62:set()on macOS bypasses the injectable runner, breaking testability.The
get()method usesthis._run()which allows injecting a test runner, butset()callsspawnSyncdirectly. This inconsistency makes theset()path harder to unit test.♻️ Suggested refactor to use runner consistently
set(target, value) { if (this.platform === 'darwin') { - spawnSync('security', ['delete-generic-password', '-a', this.account, '-s', target], { stdio: 'ignore' }); - const res = spawnSync('security', ['add-generic-password', '-a', this.account, '-s', target, '-w', value, '-U'], { - stdio: 'ignore', - }); - return res.status === 0; + // Delete any existing entry (ignore failures) + this._run('security', ['delete-generic-password', '-a', this.account, '-s', target], { stdio: 'ignore' }); + const result = this._run('security', ['add-generic-password', '-a', this.account, '-s', target, '-w', value, '-U'], { + stdio: 'ignore', + }); + return result !== undefined;Note: This requires the runner to return something truthy on success or adjusting the return convention.
40-49: Consider extractingpsLiteralto reduce duplication.The
psLiteralhelper is defined identically inget(),set(), anddelete(). Extract it as a private method or module-level function.♻️ Suggested refactor
+ _psLiteral(val) { + return `'${val.replace(/'/g, "''")}'`; + } + get(target) { // ... if (this.platform === 'win32') { - const psLiteral = (val) => `'${val.replace(/'/g, "''")}'`; const script = `try { if (Get-Module -ListAvailable -Name CredentialManager) { Import-Module CredentialManager -ErrorAction Stop - $c = Get-StoredCredential -Target ${psLiteral(target)} + $c = Get-StoredCredential -Target ${this._psLiteral(target)} if ($c -and $c.Password) { Write-Output $c.Password } } } catch { }`;src/domain/services/VaultService.js (1)
102-105: Reliance on internal readline API for input masking.Overriding
rl._writeToOutputworks but is undocumented internal behavior that could change in future Node.js versions. This is acceptable for now, but consider using a library likereadorpromptsif robustness across Node versions becomes important.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.dockerignore.editorconfig.gitignore.prettierrcARCHITECTURE.mdAUDITS.mdCONTRIBUTING.mdDockerfileREADME.mddocker-compose.ymleslint.config.jsindex.jspackage.jsonsrc/domain/errors/PlatformNotSupportedError.jssrc/domain/errors/SecretNotFoundError.jssrc/domain/errors/VaultError.jssrc/domain/services/VaultService.jssrc/infrastructure/adapters/KeychainAdapter.jstest/unit/domain/services/VaultService.test.js
🧰 Additional context used
🧬 Code graph analysis (6)
src/domain/errors/SecretNotFoundError.js (1)
src/domain/errors/VaultError.js (1)
VaultError(4-12)
src/domain/errors/PlatformNotSupportedError.js (1)
src/domain/errors/VaultError.js (1)
VaultError(4-12)
src/domain/services/VaultService.js (1)
src/infrastructure/adapters/KeychainAdapter.js (1)
KeychainAdapter(7-115)
test/unit/domain/services/VaultService.test.js (2)
src/infrastructure/adapters/KeychainAdapter.js (1)
KeychainAdapter(7-115)src/domain/services/VaultService.js (1)
VaultService(7-120)
index.js (1)
src/domain/services/VaultService.js (1)
VaultService(7-120)
src/infrastructure/adapters/KeychainAdapter.js (1)
src/domain/errors/PlatformNotSupportedError.js (1)
PlatformNotSupportedError(3-7)
🪛 markdownlint-cli2 (0.18.1)
ARCHITECTURE.md
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
AUDITS.md
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
34-34: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
78-78: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
88-88: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
91-91: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
92-92: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (15)
CONTRIBUTING.md (2)
3-8: LGTM!The Development Philosophy section clearly outlines the Hexagonal Architecture approach, distinguishing between Domain and Infrastructure layers. This aligns well with the PR's architectural goals.
10-14: LGTM!The Testing section provides clear guidance on the testing framework (Vitest) and approach (mocking KeychainAdapter), which aligns with the project's architecture and testing needs.
.prettierrc (1)
1-7: LGTM!Standard Prettier configuration with sensible defaults. The
tabWidth: 2aligns with the.editorconfigsettings for consistency.Dockerfile (1)
1-8: Overall Dockerfile structure looks good.The multi-step setup with
node:22-slim, git installation with apt cache cleanup, and the Docker flag for conditional script behavior is well-organized for the test workflow..editorconfig (1)
1-12: LGTM!Well-configured
.editorconfigwith appropriate defaults. The Markdown override fortrim_trailing_whitespace = falsecorrectly preserves intentional trailing spaces used for line breaks in Markdown.package.json (2)
1-11: Well-structured package configuration with clean ESM exports.The subpath exports for
./serviceand./errorsprovide a clean public API surface. The ESM configuration with"type": "module"is appropriate for modern Node.js.
12-19: Clever Docker auto-detection in scripts.The shell-based auto-detection for
GIT_STUNTS_DOCKERprovides a seamless developer experience between host and container execution. The argument passthrough with"$@"and trailing--is correctly implemented.AUDITS.md (1)
1-137: Valuable security audit with actionable findings.This audit document thoroughly identifies critical security vulnerabilities in the vault implementation—particularly the process table exposure (Risk 1) and PowerShell injection (Vuln 2). The "DO NOT SHIP" recommendation is appropriate until these are remediated.
The markdown indentation warnings from static analysis (MD007) are minor stylistic issues—nested list items use 4-space indentation instead of 2-space. This is optional to fix but would improve linter compliance.
eslint.config.js (1)
1-16: LGTM!The ESLint flat config is properly structured with appropriate ECMAScript 2022 support and Node.js globals. The rule set enforces good coding practices and security measures.
README.md (1)
1-42: LGTM!The README provides clear motivation, cross-platform requirements, and practical usage examples. The Zero-Secret Architecture concept is well-explained.
src/domain/errors/PlatformNotSupportedError.js (1)
1-7: LGTM!Clean error class implementation that follows the established
VaultErrorpattern. The structured error with code'PLATFORM_NOT_SUPPORTED'and metadata provides good debugging context and programmatic error handling.src/domain/errors/SecretNotFoundError.js (1)
1-7: LGTM!Clean error class implementation that follows the established pattern from
PlatformNotSupportedError. The structure correctly extendsVaultErrorwith appropriate message, code, and metadata.Note: This error class is exported as part of the public API but doesn't appear to be thrown by
VaultServiceorKeychainAdapterin the current implementation. Consider using it ingetSecretwhen the adapter returnsundefined.src/domain/errors/VaultError.js (1)
4-11: LGTM with minor portability note.Well-structured base error class with appropriate message, code, and metadata support. The
this.name = this.constructor.namepattern ensures derived classes report their correct names.
Error.captureStackTraceis V8-specific. This is fine for a Node.js-only package, but if browser/non-V8 runtime support is ever needed, wrap it in a guard:if (Error.captureStackTrace) { Error.captureStackTrace(this, this.constructor); }src/domain/services/VaultService.js (1)
7-16: LGTM - Clean dependency injection pattern.Constructor properly defaults the adapter while allowing injection for testing. The architecture follows hexagonal design principles well.
index.js (1)
21-64: LGTM - Well-designed facade with consistent API.The facade provides a cleaner API surface with named parameters (e.g.,
getSecret({ target })) compared to the service's positional parameters. Platform detection getters are convenient for consumers.The delegation pattern is correctly implemented, and
ensureSecretproperly preserves the async nature.
- Sanitize `account` parameter in Windows KeychainAdapter to prevent PowerShell command injection via unescaped quotes. - Fix unhandled exceptions in VaultService interactive prompt by properly wiring `reject()` into the Readline callback loop. - Implement comprehensive unit tests for TTY interaction and shell argument escaping constraints.
1 participant
Setting up the vault
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.