Deno and bun#3
Conversation
- Add dedicated `Dockerfile.bun` and `Dockerfile.deno` for hermetic runtime verification. - Implement platform-specific command runners to bypass Node.js shims when running in alternative JavaScript runtimes. - Refactor `KeychainAdapter` to delegate OS interactions to injected command runners. - Update entry point to auto-detect global runtime namespace.
- Set `publishConfig.access` to public for scoped package release. - Define `engines` field to explicitly support Node >=20, Bun >=1.3.5, and Deno >=2. - Populate discovery metadata (repository, homepage, bugs, keywords). - Enable bundler tree-shaking via `sideEffects: false`. - Update package description to reflect "Zero-Secret Architecture" positioning.
- Install `nodejs` and `npm` in Deno Docker image to support Vitest execution (hybrid runtime environment). - Correct `CMD` arguments in Bun Dockerfile to successfully invoke Vitest. - Refactor `Bun` and `Deno` command runners to accept injected globals, eliminating `globalThis` pollution in unit tests. - Upgrade `zod` to v4.3.5 and enforce strict engine versions in lockfile.
|
Warning Rate limit exceeded@flyingrobots has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 18 minutes and 23 seconds before requesting another review. ⌛ 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 (2)
📝 WalkthroughWalkthroughAdds a CommandRunnerPort and runtime-specific command runners/adapters (Node, Bun, Deno); refactors KeychainAdapter and VaultService to accept injected command runners/platform getters; adds runtime utilities, new error types, Dockerfiles, compose/services and multi-runtime test orchestration, plus tests and package exports. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Vault
participant AdapterFactory
participant KeychainAdapter
participant CommandRunner
participant HostRuntime
Client->>Vault: new Vault({ account, adapterFactory? })
alt adapterFactory provided
Vault->>AdapterFactory: adapterFactory({ account })
AdapterFactory-->>Vault: KeychainAdapter (with CommandRunner)
else auto-detect
Vault->>AdapterFactory: detectAdapter({ account })
AdapterFactory->>CommandRunner: instantiate runtime-specific runner
AdapterFactory-->>Vault: KeychainAdapter (with CommandRunner)
end
Client->>Vault: getSecret({ target })
Vault->>KeychainAdapter: get(target)
KeychainAdapter->>CommandRunner: run(command, args, options)
CommandRunner->>HostRuntime: execute (spawnSync / Bun.spawnSync / Deno.Command)
HostRuntime-->>CommandRunner: { status, stdout, stderr }
CommandRunner-->>KeychainAdapter: result
KeychainAdapter-->>Vault: secret
Vault-->>Client: secret
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
27-33:npm testusessh -c→ breaks on Windows shells.Line 27:
sh -c 'if ...'will fail for users/CI on Windows without a POSIX shell. If you want this to be cross-platform, consider moving the branching logic into a small Node script (or use a cross-platform runner) and call that fromnpm test.src/infrastructure/adapters/KeychainAdapter.js (1)
66-103: AvoidInstall-Module CredentialManagerduringset()(security + reliability risk).Auto-installing a PowerShell module at runtime (Line 91-93) can introduce network dependency, policy failures, and supply-chain risk. Prefer: document the prerequisite, or gate installation behind an explicit opt-in flag.
Safer default (no auto-install)
const script = `try { - if (!(Get-Module -ListAvailable -Name CredentialManager)) { - Install-Module -Name CredentialManager -Scope CurrentUser -Force -ErrorAction Stop - } + if (!(Get-Module -ListAvailable -Name CredentialManager)) { exit 1 } Import-Module CredentialManager -ErrorAction Stop $pwd = ${psLiteral(value)} New-StoredCredential -Target ${psLiteral(target)} -UserName ${psLiteral(this.account)} -Password $pwd -Persist CurrentUser | Out-Null } catch { exit 1 }`;
🤖 Fix all issues with AI agents
In @Dockerfile.deno:
- Around line 1-14: The Dockerfile currently installs the distro default
nodejs/npm which can be <20 and violate package.json engines; change the RUN
that installs Node to explicitly install Node 20+ via NodeSource (e.g., run the
NodeSource setup script for setup_20.x before apt-get install) so apt-get
installs nodejs@20, or otherwise add explicit package source/versioning and pin
NODE_VERSION (use setup_20.x), keeping npm and preserving the rest of the
Dockerfile (WORKDIR, COPY, ENV, CMD) intact.
In @index.js:
- Around line 41-45: The constructor JSDoc incorrectly types
options.adapterFactory as Object while the code invokes it as a function;
add/update the JSDoc to indicate adapterFactory is a Function (or callable) and
add a runtime guard in the constructor that checks if adapterFactory is provided
and typeof adapterFactory === 'function', otherwise throw a clear TypeError;
then call adapterFactory({ account }) when valid, falling back to
detectAdapterFactory({ account }) and pass the resulting adapter into new
VaultService({ account, adapter }).
- Around line 9-21: The current eager static imports (createNodeKeychainAdapter,
createBunKeychainAdapter, createDenoKeychainAdapter) cause Node-only modules to
be evaluated in non-Node runtimes; change detectAdapterFactory to load adapters
dynamically at runtime (e.g., use dynamic import() inside detectAdapterFactory)
so only the matching adapter module is imported for the detected environment, or
alternatively replace the direct import of createNodeKeychainAdapter with a lazy
factory that does a runtime import of the Node adapter (ensuring
node:child_process stays inside that dynamically imported module); update
references to
createNodeKeychainAdapter/createBunKeychainAdapter/createDenoKeychainAdapter
accordingly so no Node-specific top-level imports execute in Bun/Deno.
In @README.md:
- Around line 57-62: The README incorrectly references plumbing/Dockerfile.node
which doesn't exist; update the Docker images section to reference the actual
root-level files (Dockerfile, Dockerfile.bun, Dockerfile.deno) and describe that
Dockerfile (Node) mirrors the Node workflow at the repository root and runs npm
test; alternatively, if you prefer keeping the plumbing path, create the missing
plumbing/Dockerfile.node file by copying the root Dockerfile contents into
plumbing/Dockerfile.node and ensure README points to the chosen location
consistently; update any mention of plumbing/ to use the correct root-level file
names (Dockerfile, Dockerfile.bun, Dockerfile.deno).
🧹 Nitpick comments (11)
test/unit/infrastructure/adapters/bun/CommandRunner.test.js (1)
18-34: Nice DI-based test; consider asserting I/O piping +inputpassthrough too.Lines 25-33: You validate
cwd/envand output decoding—good. Ifstdin/stdout/stderr: 'pipe'andoptions.inputare part of the contract, add expectations so regressions get caught.src/infrastructure/adapters/node/index.js (1)
4-9: Consider extractingdefaultPlatformGetterto a shared utility.This function is duplicated in
src/infrastructure/adapters/bun/index.js. Extracting it to a shared location (e.g.,src/infrastructure/adapters/shared/platformGetter.js) would reduce duplication and ensure consistent behavior across adapters.Dockerfile.bun (1)
1-1: Pin the Bun version for reproducible builds.Using
oven/bun:latestcan lead to unexpected build failures when new versions are released. Pinning to a specific version improves reproducibility and makes debugging easier when issues arise.Suggested change
-FROM oven/bun:latest +FROM oven/bun:1.3.4src/infrastructure/adapters/bun/index.js (1)
4-9: Consider extractingdefaultPlatformGetterto a shared utility.This exact function is duplicated in
src/infrastructure/adapters/node/index.jsandsrc/infrastructure/adapters/KeychainAdapter.js. Extracting it to a shared module (e.g.,src/utils/platform.js) would reduce maintenance burden and ensure consistent behavior.♻️ Suggested refactor
Create a shared utility:
// src/utils/platform.js export function getPlatform() { if (typeof process !== 'undefined' && process && typeof process.platform === 'string') { return process.platform; } return 'unknown'; }Then import in each adapter:
+import { getPlatform } from '../../utils/platform.js'; import KeychainAdapter from '../KeychainAdapter.js'; import BunCommandRunner from './CommandRunner.js'; -const defaultPlatformGetter = () => { - if (typeof process !== 'undefined' && process && typeof process.platform === 'string') { - return process.platform; - } - return 'unknown'; -}; +const defaultPlatformGetter = getPlatform;scripts/run-multi-runtime-tests.sh (1)
19-32: Consider handlingdocker compose upfailure before checking individual services.If
docker compose up --buildfails (e.g., build error, network issue), the script continues to query exit codes for services that may not have run. Capturing the exit code ofupwould provide earlier, clearer feedback.♻️ Suggested improvement
$DOCKER_COMPOSE up --build --remove-orphans +UP_EXIT=$? +if [ "$UP_EXIT" -ne 0 ]; then + printf "❌ docker compose up failed (exit %s)\n" "$UP_EXIT" + $DOCKER_COMPOSE down + exit 1 +fi EXIT_CODE=0 for service in $SERVICES; dosrc/infrastructure/adapters/bun/CommandRunner.js (1)
3-14: Consider extracting shareddecodeutility.The
decodefunction is duplicated nearly identically insrc/infrastructure/adapters/deno/CommandRunner.js. Consider extracting this to a shared utility module (e.g.,src/infrastructure/adapters/shared/decode.js) to reduce duplication and ensure consistent behavior across runtimes.ARCHITECTURE.md (1)
34-37: Minor directory structure redundancy.The structure shows
adapters/as both a direct child and then repeated with runtime subdirectories. Consider consolidating for clarity:📝 Suggested fix
└── infrastructure/ - ├── adapters/ # KeychainAdapter - ├── adapters/node/ # NodeCommandRunner, factories - ├── adapters/bun/ # BunCommandRunner, factories - └── adapters/deno/ # DenoCommandRunner, factories + └── adapters/ + ├── KeychainAdapter.js + ├── node/ # NodeCommandRunner, factories + ├── bun/ # BunCommandRunner, factories + └── deno/ # DenoCommandRunner, factoriessrc/infrastructure/adapters/deno/CommandRunner.js (1)
31-34: Inconsistent runtime resolution pattern.
BunCommandRunnerresolves the runtime in the constructor (this.bun = bun ?? globalThis?.Bun), whileDenoCommandRunnerstores only the injected value and resolves the global inrun(). Consider aligning the patterns for consistency:♻️ Align with BunCommandRunner pattern
export default class DenoCommandRunner { constructor({ deno } = {}) { - this.deno = deno; + this.deno = deno ?? getGlobalDeno(); } run(command, args = [], options = {}) { - const deno = this.deno ?? getGlobalDeno(); + const deno = this.deno; if (typeof deno === 'undefined' || typeof deno.Command !== 'function') {test/unit/infrastructure/adapters/KeychainAdapter.test.js (1)
61-80: Consider adding linux platform tests.The darwin platform tests look good. Consider adding similar tests for the linux platform to ensure complete coverage of
secret-toolcommand invocations.index.js (1)
13-21:detectAdapterFactorynaming mismatch (it returns an adapter, not a factory).
detectAdapterFactory({ account })returnscreate*KeychainAdapter({ account })(an adapter instance) (Line 13-21). Either rename it (e.g.,detectAdapter) or change it to return a factory function for consistency with theadapterFactoryconcept.src/infrastructure/adapters/KeychainAdapter.js (1)
38-64: Windowsget()swallows all PowerShell errors (always exits 0).The script’s
catch { }(Line 59) suppresses failures, so_run()can’t distinguish “missing credential” vs “PowerShell/module failure”. Considercatch { exit 1 }(likeset/delete) so callers can surface real errors when desired.
📜 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 (22)
ARCHITECTURE.mdDockerfile.bunDockerfile.denoREADME.mddeno.jsondocker-compose.ymlindex.jspackage.jsonscripts/run-multi-runtime-tests.shsrc/domain/services/VaultService.jssrc/infrastructure/adapters/KeychainAdapter.jssrc/infrastructure/adapters/bun/CommandRunner.jssrc/infrastructure/adapters/bun/index.jssrc/infrastructure/adapters/deno/CommandRunner.jssrc/infrastructure/adapters/deno/index.jssrc/infrastructure/adapters/node/CommandRunner.jssrc/infrastructure/adapters/node/index.jssrc/ports/CommandRunnerPort.jstest/unit/domain/services/VaultService.test.jstest/unit/infrastructure/adapters/KeychainAdapter.test.jstest/unit/infrastructure/adapters/bun/CommandRunner.test.jstest/unit/infrastructure/adapters/deno/CommandRunner.test.js
🧰 Additional context used
🧬 Code graph analysis (11)
src/infrastructure/adapters/node/index.js (3)
src/infrastructure/adapters/KeychainAdapter.js (2)
defaultPlatformGetter(3-8)KeychainAdapter(10-134)src/infrastructure/adapters/bun/index.js (1)
defaultPlatformGetter(4-9)src/infrastructure/adapters/node/CommandRunner.js (1)
NodeCommandRunner(6-15)
src/domain/services/VaultService.js (1)
test/unit/infrastructure/adapters/KeychainAdapter.test.js (1)
adapter(5-5)
test/unit/infrastructure/adapters/KeychainAdapter.test.js (1)
src/infrastructure/adapters/KeychainAdapter.js (1)
KeychainAdapter(10-134)
src/infrastructure/adapters/bun/CommandRunner.js (1)
src/infrastructure/adapters/deno/CommandRunner.js (2)
DECODER(2-2)decode(11-22)
src/infrastructure/adapters/deno/index.js (4)
src/infrastructure/adapters/KeychainAdapter.js (2)
defaultPlatformGetter(3-8)KeychainAdapter(10-134)src/infrastructure/adapters/bun/index.js (1)
defaultPlatformGetter(4-9)src/infrastructure/adapters/node/index.js (1)
defaultPlatformGetter(4-9)src/infrastructure/adapters/deno/CommandRunner.js (1)
DenoCommandRunner(31-59)
src/infrastructure/adapters/bun/index.js (3)
src/infrastructure/adapters/KeychainAdapter.js (2)
defaultPlatformGetter(3-8)KeychainAdapter(10-134)src/infrastructure/adapters/node/index.js (1)
defaultPlatformGetter(4-9)src/infrastructure/adapters/bun/CommandRunner.js (1)
BunCommandRunner(16-48)
test/unit/infrastructure/adapters/deno/CommandRunner.test.js (1)
src/infrastructure/adapters/deno/CommandRunner.js (1)
DenoCommandRunner(31-59)
index.js (4)
src/infrastructure/adapters/bun/index.js (1)
createBunKeychainAdapter(11-17)src/infrastructure/adapters/deno/index.js (1)
createDenoKeychainAdapter(17-23)src/infrastructure/adapters/node/index.js (1)
createNodeKeychainAdapter(11-17)src/domain/services/VaultService.js (1)
VaultService(6-131)
test/unit/infrastructure/adapters/bun/CommandRunner.test.js (1)
src/infrastructure/adapters/bun/CommandRunner.js (1)
BunCommandRunner(16-48)
src/infrastructure/adapters/deno/CommandRunner.js (1)
src/infrastructure/adapters/bun/CommandRunner.js (2)
DECODER(1-1)decode(3-14)
src/infrastructure/adapters/KeychainAdapter.js (4)
src/infrastructure/adapters/bun/index.js (1)
defaultPlatformGetter(4-9)src/infrastructure/adapters/deno/index.js (1)
defaultPlatformGetter(10-15)src/infrastructure/adapters/node/index.js (1)
defaultPlatformGetter(4-9)src/domain/errors/PlatformNotSupportedError.js (1)
PlatformNotSupportedError(3-7)
🔇 Additional comments (26)
package.json (3)
7-16: Exports expansion: please treat as public API + verify packaging coverage.Lines 7-16 and 17-22: Since
./adapters/*and./ports/command-runnerare now public entrypoints, make sure they’re documented and covered by semver expectations, and that the published tarball actually contains all referenced files (yourfileslist includessrc, so it should).Also applies to: 17-25
36-38: Zod v3 → v4 upgrade: verify behavioral compatibility across the library.Lines 36-38: Zod 4 introduces behavior changes (notably around defaults and number/int safety). Please confirm all schema usage in
src/**still behaves as intended and that consumers aren’t relying on v3 quirks.
62-68:sideEffects: falseis correctly set.The runtime auto-detection and adapter factories only read from global objects (
typeof Bun,typeof Deno,process.platform) without mutating them. No module-level code executes global registrations, prototype modifications, or monkeypatching on import. Side effects (running OS commands) only occur when adapter methods are explicitly called, not during module initialization.test/unit/infrastructure/adapters/bun/CommandRunner.test.js (1)
8-16: Good coverage for the “Bun.spawnSync required” guard.test/unit/domain/services/VaultService.test.js (1)
10-18: Good test decoupling: using a plain adapter mock matches the new DI shape.Lines 12-16: This is cleaner than mocking the KeychainAdapter module and makes the unit test focus on VaultService behavior.
deno.json (1)
1-5:deno task testdepends on npm/Node, which is by intentional design.The task runs
npm run test:local, a Vitest command (Node.js test runner). This is the project's unified testing approach across all runtimes (Node, Bun, Deno). The Dockerfile.deno explicitly installs Node.js for this reason. Deno-specific adapter code (test/unit/infrastructure/adapters/deno/CommandRunner.test.js) is exercised through this setup. No changes needed.Likely an incorrect or invalid review comment.
Dockerfile.deno (1)
10-12:npm install --ignore-scriptsis safe for this project—no dependencies use postinstall scripts.None of the project's dependencies (zod, vitest, eslint, prettier, @eslint/js) rely on postinstall scripts, so
--ignore-scriptshas no impact on functionality. Vitest and all other toolchain dependencies work correctly as-is.test/unit/infrastructure/adapters/deno/CommandRunner.test.js (1)
1-46: LGTM! Well-structured tests covering error and success paths.The test correctly validates:
- Error throwing when
Deno.Commandis unavailable- Proper invocation of
Deno.Commandconstructor with expected options- Correct decoding of stdout/stderr from
Uint8Arrayto stringsOne optional enhancement: consider adding a test case where
options.inputis provided to verify theencodeInputpath produces aUint8Arrayinstead ofundefined.Dockerfile.bun (1)
2-8: LGTM! Good Dockerfile practices.The Dockerfile follows good practices:
- Cleans up apt cache after install to reduce image size
- Uses
--ignore-scriptsfor safer dependency installation- Sets the environment variable for test detection
src/domain/services/VaultService.js (1)
14-19: LGTM! Clean dependency injection with proper validation.The strict runtime validation ensures the adapter contract is fulfilled before proceeding. This aligns well with the port/adapter architecture, making the service runtime-agnostic and properly testable.
src/infrastructure/adapters/node/index.js (1)
11-19: LGTM! Clean factory pattern with sensible defaults.The factory properly:
- Provides defaults via nullish coalescing (
??)- Allows full dependency injection for testing
- Re-exports
NodeCommandRunnerfor direct access when neededdocker-compose.yml (1)
1-21: LGTM! Well-structured multi-runtime test configuration.The docker-compose configuration cleanly separates runtime-specific test services while maintaining consistent structure:
- Each service uses the appropriate Dockerfile for its runtime
- Common environment variable
GIT_STUNTS_DOCKER=1is set across all services- Build context is shared appropriately
This enables running
docker-compose run node-test,docker-compose run bun-test, ordocker-compose run deno-testindependently.README.md (1)
21-33: LGTM! Clear documentation of the new multi-runtime architecture.The documentation accurately describes the adapter auto-detection behavior and testing infrastructure. The references to
plumbing/,docker-compose.yml, and the runtime-specific Dockerfiles align well with the changes introduced in this PR.src/ports/CommandRunnerPort.js (1)
1-15: LGTM! Clean port interface for command execution abstraction.The port pattern enables dependency injection and testability across Node, Bun, and Deno runtimes. The JSDoc clearly documents the expected contract, and the throw-on-call pattern is an appropriate way to enforce implementation in JavaScript.
src/infrastructure/adapters/node/CommandRunner.js (1)
7-14: LGTM! Solid implementation with consistent return shape.The type guards ensure a predictable return structure regardless of
spawnSyncedge cases. One note: the current spread order{ encoding: 'utf8', ...options }allows callers to override the encoding — if this is intentional for flexibility, consider documenting it; otherwise, swap the order to enforce UTF-8.src/infrastructure/adapters/bun/index.js (1)
11-17: LGTM! Clean factory pattern with sensible defaults.The factory enables dependency injection for testing while providing reasonable defaults for production use.
src/infrastructure/adapters/bun/CommandRunner.js (1)
16-47: LGTM!The
BunCommandRunnerimplementation is well-structured:
- Proper dependency injection via constructor with fallback to
globalThis.Bun- Defensive validation of
Bun.spawnSyncavailability- Correct handling of both
exitCodeandstatusproperties for compatibility- Clean return shape matching the expected interface
ARCHITECTURE.md (1)
13-24: LGTM!Good documentation of the new port-driven architecture. The descriptions clearly explain the separation of concerns between the
CommandRunnerPortinterface and the runtime-specific implementations.src/infrastructure/adapters/deno/CommandRunner.js (1)
36-58: LGTM!The
run()implementation correctly:
- Validates
Deno.Commandavailability before use- Configures piped stdio for input/output capture
- Encodes input when provided
- Returns a consistent
{ status, stdout, stderr }shapesrc/infrastructure/adapters/deno/index.js (2)
4-15: LGTM!The
PLATFORM_MAPcorrectly translates Deno's OS identifiers (darwin,linux,windows) to the platform strings expected byKeychainAdapter(darwin,linux,win32). ThedefaultPlatformGetterhas appropriate defensive checks for the Deno runtime.
17-25: LGTM!The factory function follows the same pattern as the Node and Bun adapters, providing sensible defaults while allowing full dependency injection for testing.
test/unit/infrastructure/adapters/KeychainAdapter.test.js (2)
6-11: LGTM!Good test setup with a mock
runnerobject. The{ run: vi.fn() }pattern correctly implements the minimal interface required byKeychainAdapter.
13-59: LGTM!Comprehensive tests for win32 platform covering PowerShell escaping in
set(),delete(), andget()operations. The single-quote escaping validation ('my''target') is important for preventing injection vulnerabilities.src/infrastructure/adapters/KeychainAdapter.js (3)
105-133: Delete path looks consistent with new runner abstraction.Branching on
platformonce (Line 106-107) and returningres.status === 0is clean and consistent withset().
25-36:_run()should handle non-string stdout (Buffer/Uint8Array) to avoid always returningundefined.Right now
_run()only returns output whenresult.stdoutis a string (Line 34-36). Many spawn APIs returnBuffer/Uint8Arrayby default, which would makeget()returnundefinedeven on success.Proposed patch
_run(command, args = [], options = {}) { - const optionKeys = options ? Object.keys(options) : []; - const result = - optionKeys.length > 0 - ? this.commandRunner.run(command, args, options) - : this.commandRunner.run(command, args); + const result = this.commandRunner.run(command, args, options); if (!result || result.status !== 0) { return undefined; } const stdout = result.stdout; - return typeof stdout === 'string' ? stdout.trim() : undefined; + if (typeof stdout === 'string') return stdout.trim(); + if (stdout && (stdout instanceof Uint8Array)) { + return new TextDecoder().decode(stdout).trim(); + } + return undefined; }
3-19: No action required — allKeychainAdapterinstantiations in the codebase use factory methods that always providecommandRunner, preventing any breaking change.The constructor now validates
commandRunneris required (lines 11–14), but all three platform adapters (createNodeKeychainAdapter,createBunKeychainAdapter,createDenoKeychainAdapter) guaranteecommandRunneris passed via nullish coalescing (commandRunner ?? new XCommandRunner()). Test files also providecommandRunner. No external direct instantiation paths exist in the codebase, so the constructor change does not break any existing call sites.The
_run()method properly handles stdout type checking (line 35:typeof stdout === 'string') before callingtrim(). All three command runners (Node, Deno, Bun) return stdout as either a decoded string orundefined, which is correctly handled.
- Replace `sh` based test runner with `scripts/run-tests.js` for better cross-platform portability (Windows support). - Pin Node.js v20.11.0 in `Dockerfile.deno` via manual binary installation, avoiding stale apt repository issues. - Refactor `index.js` to use dynamic imports for Node adapters, preventing crashes in strict ESM runtimes (Bun/Deno) where `require` is absent. - Remove automatic `Install-Module` logic from Windows adapter; library now fails fast if dependencies are missing rather than altering system state.
- Extract duplicate codec logic (TextEncoder/Decoder) into `shared/codec.js`. - Unify platform detection strategy into `utils/platform.js` to ensure consistent behavior across adapters. - Pin Bun Docker image to v1.3.4 for deterministic CI builds. - Harden shell orchestration: `run-multi-runtime-tests.sh` now fails fast if Docker Compose startup fails.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ARCHITECTURE.md (1)
28-39: Directory structure is incomplete and has formatting issues.The
ports/directory mentioned in lines 13-16 is missing from the tree, and the indentation forinfrastructure/appears inconsistent.Suggested fix
src/ ├── domain/ │ ├── errors/ # VaultError, etc. │ └── services/ # VaultService - └── infrastructure/ - └── adapters/ - ├── KeychainAdapter.js - ├── node/ # NodeCommandRunner, factories - ├── bun/ # BunCommandRunner, factories - └── deno/ # DenoCommandRunner, factories +├── ports/ # CommandRunnerPort +└── infrastructure/ + └── adapters/ + ├── KeychainAdapter.js + ├── node/ # NodeCommandRunner, factories + ├── bun/ # BunCommandRunner, factories + └── deno/ # DenoCommandRunner, factoriesindex.js (1)
77-87: Platform getters are incompatible with Deno runtime.Since this PR adds Deno support, these getters will throw
ReferenceError: process is not definedwhen accessed in Deno (whereprocessglobal doesn't exist without explicit polyfills).Consider using the adapter's platform detection or adding runtime-aware platform checks:
Suggested approach
get isMac() { - return process.platform === 'darwin'; + if (typeof Deno !== 'undefined') { + return Deno.build.os === 'darwin'; + } + return process.platform === 'darwin'; } get isLinux() { - return process.platform === 'linux'; + if (typeof Deno !== 'undefined') { + return Deno.build.os === 'linux'; + } + return process.platform === 'linux'; } get isWindows() { - return process.platform === 'win32'; + if (typeof Deno !== 'undefined') { + return Deno.build.os === 'windows'; + } + return process.platform === 'win32'; }Alternatively, delegate to the adapter's
platformGetterfor consistency with the rest of the architecture.
🤖 Fix all issues with AI agents
In @Dockerfile.deno:
- Around line 19-20: The Dockerfile.deno references a non-existent plumbing
directory (COPY plumbing /plumbing) and lives at repo root while
docker-compose.yml expects the Dockerfile in a vault/ subdirectory with a parent
build context; fix by either removing or replacing the COPY plumbing line (or
add the missing plumbing directory/files) and make the Dockerfile location and
build context consistent with docker-compose.yml—either move Dockerfile.deno
into the vault/ subdirectory and adjust COPY paths to COPY ./vault /app (or
similar), or update docker-compose.yml to point to the current Dockerfile.deno
path and root build context so COPY vault /app resolves correctly.
In @index.js:
- Around line 44-52: The export block duplicates the previously declared export
function createNodeKeychainAdapter, causing a SyntaxError; remove
createNodeKeychainAdapter from the export list in the final export block so only
one export declaration exists, leaving the other symbols (VaultService,
VaultError, PlatformNotSupportedError, SecretNotFoundError,
createBunKeychainAdapter, createDenoKeychainAdapter) in that block unchanged.
In @src/infrastructure/adapters/KeychainAdapter.js:
- Line 96: The throw in the KeychainAdapter.set method references a misspelled
identifier (plATFORM) causing a ReferenceError; update the throw to pass the
correct parameter name (platform) to PlatformNotSupportedError so it uses the
method's platform argument, and verify the set(...) parameter name matches the
variable you pass to new PlatformNotSupportedError.
In @src/infrastructure/adapters/shared/decode.js:
- Around line 1-14: The decodeValue implementation is duplicated; consolidate by
moving/exporting decodeValue from the module that already has it alongside
encodeString (the codec.js export) and remove the duplicate in decode.js, then
update the Bun adapter to import decodeValue from that shared codec export (or
create a small shared utility module and import both adapters from it). Ensure
you export decodeValue (named export) from the existing codec module and replace
any local declarations of decodeValue (e.g., in decode.js) with an import of
that exported function so both adapters use the same implementation.
🧹 Nitpick comments (5)
scripts/run-tests.js (2)
46-47: Consider handlingdocker compose upfailures.If
docker compose upfails (returns non-zero), the script continues to check container exit codes, which may produce misleading results or errors. Consider checking the return code and failing early.Proposed fix
const upArgs = [...docker.argsPrefix, 'up', '--build', '--remove-orphans']; - await runCommand(docker.command, upArgs); + const upCode = await runCommand(docker.command, upArgs); + if (upCode !== 0) { + console.error('❌ docker compose up failed'); + process.exitCode = upCode; + return; + }
73-78: Verifyvitestis available in PATH for local tests.When running locally (not in Docker),
vitestis invoked directly. This assumes it's globally installed or available vianpx. Consider usingnpx vitestornpm exec vitestfor reliability.Proposed fix
const runLocalTests = async () => { const extraArgs = process.argv.slice(2); - const args = ['run', 'test/unit', ...extraArgs]; - const code = await runCommand('vitest', args); + const args = ['exec', 'vitest', 'run', 'test/unit', ...extraArgs]; + const code = await runCommand('npm', args); process.exitCode = code; };src/infrastructure/adapters/shared/codec.js (1)
4-15: DuplicatedecodeValueimplementation exists indecode.js.The
decodeValuefunction here is identical to the one insrc/infrastructure/adapters/shared/decode.js. Consider consolidating to a single source of truth to avoid maintenance drift.Options:
- Remove
decodeValuefrom this file and re-export fromdecode.js- Move
encodeStringtodecode.js(or rename it tocodec.js) and remove the duplicateOption 1: Re-export from decode.js
+export { decodeValue } from './decode.js'; + const ENCODER = new TextEncoder(); -const DECODER = new TextDecoder(); - -export const decodeValue = (value) => { - if (typeof value === 'string') { - return value; - } - if (value instanceof Uint8Array) { - return DECODER.decode(value); - } - if (value instanceof ArrayBuffer) { - return DECODER.decode(new Uint8Array(value)); - } - return undefined; -}; export const encodeString = (input) => { return ENCODER.encode(input); };src/infrastructure/adapters/bun/CommandRunner.js (1)
29-34: Inconsistent indentation in return statement.The
stdoutandstderrproperties are not aligned withstatus, which reduces readability.Fix indentation
return { status, - stdout: decodeValue(result.stdout), - stderr: decodeValue(result.stderr) - }; -} + stdout: decodeValue(result.stdout), + stderr: decodeValue(result.stderr) + }; + } }src/infrastructure/adapters/KeychainAdapter.js (1)
21-32: Simplify_runby always passing options.The check for
optionKeys.lengthadds unnecessary complexity. The command runners handle empty options objects gracefully.Simplified implementation
_run(command, args = [], options = {}) { - const optionKeys = options ? Object.keys(options) : []; - const result = - optionKeys.length > 0 - ? this.commandRunner.run(command, args, options) - : this.commandRunner.run(command, args); + const result = this.commandRunner.run(command, args, options); if (!result || result.status !== 0) { return undefined; } const stdout = result.stdout; return typeof stdout === 'string' ? stdout.trim() : undefined; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
ARCHITECTURE.mdDockerfile.bunDockerfile.denoREADME.mdindex.jspackage.jsonscripts/run-multi-runtime-tests.shscripts/run-tests.jssrc/infrastructure/adapters/KeychainAdapter.jssrc/infrastructure/adapters/bun/CommandRunner.jssrc/infrastructure/adapters/bun/index.jssrc/infrastructure/adapters/deno/CommandRunner.jssrc/infrastructure/adapters/node/index.jssrc/infrastructure/adapters/shared/codec.jssrc/infrastructure/adapters/shared/decode.jssrc/utils/platform.jstest/unit/infrastructure/adapters/KeychainAdapter.test.jstest/unit/infrastructure/adapters/bun/CommandRunner.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/infrastructure/adapters/bun/index.js
- test/unit/infrastructure/adapters/bun/CommandRunner.test.js
- README.md
- src/infrastructure/adapters/deno/CommandRunner.js
- Dockerfile.bun
🧰 Additional context used
🧬 Code graph analysis (8)
src/infrastructure/adapters/shared/codec.js (1)
src/infrastructure/adapters/shared/decode.js (3)
DECODER(1-1)decodeValue(3-14)decodeValue(3-14)
src/infrastructure/adapters/shared/decode.js (1)
src/infrastructure/adapters/shared/codec.js (3)
DECODER(1-1)decodeValue(4-15)decodeValue(4-15)
src/infrastructure/adapters/bun/CommandRunner.js (1)
src/infrastructure/adapters/shared/decode.js (2)
decodeValue(3-14)decodeValue(3-14)
src/infrastructure/adapters/KeychainAdapter.js (4)
src/infrastructure/adapters/bun/index.js (1)
defaultPlatformGetter(5-5)src/infrastructure/adapters/node/index.js (1)
defaultPlatformGetter(5-5)src/utils/platform.js (1)
getPlatform(1-6)src/domain/errors/PlatformNotSupportedError.js (1)
PlatformNotSupportedError(3-7)
test/unit/infrastructure/adapters/KeychainAdapter.test.js (1)
src/infrastructure/adapters/KeychainAdapter.js (1)
KeychainAdapter(6-128)
index.js (4)
src/infrastructure/adapters/node/index.js (1)
createNodeKeychainAdapter(7-13)src/infrastructure/adapters/bun/index.js (1)
createBunKeychainAdapter(7-13)src/infrastructure/adapters/deno/index.js (1)
createDenoKeychainAdapter(17-23)src/domain/services/VaultService.js (1)
VaultService(6-131)
src/infrastructure/adapters/node/index.js (6)
src/infrastructure/adapters/KeychainAdapter.js (2)
defaultPlatformGetter(4-4)KeychainAdapter(6-128)src/infrastructure/adapters/bun/index.js (1)
defaultPlatformGetter(5-5)src/infrastructure/adapters/deno/index.js (1)
defaultPlatformGetter(10-15)src/utils/platform.js (1)
getPlatform(1-6)index.js (1)
createNodeKeychainAdapter(30-32)src/infrastructure/adapters/node/CommandRunner.js (1)
NodeCommandRunner(6-15)
scripts/run-multi-runtime-tests.sh (1)
scripts/run-tests.js (1)
docker(39-39)
🪛 Hadolint (2.14.0)
Dockerfile.deno
[error] 15-15: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
🔇 Additional comments (11)
scripts/run-tests.js (2)
6-11: LGTM on the promise-based command runner.Clean abstraction using
spawnwith promise wrapping. Thecode ?? 0fallback handles edge cases where exit code might be null.
13-35: LGTM on docker compose detection.Properly handles both modern
docker compose(plugin) and legacydocker-compose(standalone) commands with appropriate fallback.package.json (3)
10-16: LGTM on expanded exports.The new export paths for adapters (keychain, node, bun, deno) and ports (command-runner) align well with the multi-runtime architecture introduced in this PR.
45-66: LGTM on package metadata.Good additions for npm publishing: repository, homepage, bugs, keywords, and engine requirements. The
sideEffects: falsedeclaration enables better tree-shaking.
36-38: Remove unused Zod dependency from package.json.Zod is listed as a dependency but is not imported or used anywhere in the codebase. Remove it to reduce bundle size and avoid maintaining an unused package.
Likely an incorrect or invalid review comment.
src/utils/platform.js (1)
1-6: LGTM with a note on Deno compatibility.The defensive checks handle edge cases well. Note that Deno natively uses
Deno.build.osfor platform detection, but since Deno's Node compatibility layer providesprocess.platform, this should work in the adapter context.Dockerfile.deno (1)
15-15: Static analysis hint is a false positive.The Hadolint DL1000 error on line 15 is a false positive. The heredoc syntax (
<<'EOF') combined with the subsequentRUNcommand is valid Docker BuildKit syntax. The linter doesn't fully support heredoc parsing.scripts/run-multi-runtime-tests.sh (1)
1-56: Well-structured orchestration script.The script correctly handles both Docker Compose v1 and v2, aggregates exit codes from all test services, and ensures cleanup on both success and failure paths.
src/infrastructure/adapters/node/index.js (1)
1-15: Clean factory implementation with sensible defaults.The adapter factory follows a consistent pattern with the Bun and Deno adapters, properly applies defaults using nullish coalescing, and re-exports the command runner for flexibility.
test/unit/infrastructure/adapters/KeychainAdapter.test.js (1)
4-115: Good coverage of the runner-based API across platforms.The tests properly validate the injected
commandRunnerandplatformGetterpatterns, with coverage for win32 escaping, darwin security commands, and linux secret-tool operations.Consider adding tests for:
- Constructor throwing when
commandRunneris missing or lacks arunmethodPlatformNotSupportedErrorthrown for unsupported platformsindex.js (1)
64-71: LGTM!Constructor properly validates
adapterFactoryand falls back todetectAdapterwhen not provided. The loose inequality!= nullcorrectly handles bothnullandundefinedcases.
- Replace generic `Error` throws with typed domain exceptions (`VaultServiceError`, `AdapterError`, `CommandRunnerError`). - Implement static factory methods on error classes to enforce consistent error codes and metadata (e.g., `adapter.missing-command-runner`). - Add comprehensive JSDoc annotations to public exports (`index.js`) and factory functions to enable IntelliSense/TS-in-JS support.
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/domain/services/VaultService.js (1)
73-82: Deno compatibility requires abstracting Node-specific APIs (process.env,process.stdin,process.platform).The code directly uses Node.js-specific APIs without abstraction:
VaultService.resolveSecret()accessesprocess.env(lines 74–75)VaultService.ensureSecret()accessesprocess.stdin.isTTYandprocess.stdin(lines 97, 103)Vaultfacade accessesprocess.platform(lines 90, 94, 98)readlineis imported from'node:readline'(VaultService.js:1)The adapters currently abstract only command execution (via
Deno.Command), not environment or stdin access. While the Dockerfile.deno installs Node.js as a workaround, true Deno support requires either:
- Abstracting these APIs into injected dependencies (environment resolver, stdin provider, platform detector)
- Using Deno equivalents where applicable (
Deno.env, stdin handling via Deno's I/O APIs)- Properly configuring Node compatibility mode with necessary permissions (
--allow-env)This affects both
VaultServiceand theVaultfacade.
🤖 Fix all issues with AI agents
In @src/domain/errors/CommandRunnerError.js:
- Around line 15-21: The factory method missingSpawnSync on CommandRunnerError
currently hardcodes "BunCommandRunner" in its message; update the message to
interpolate the runtime parameter so it reads like `${runtime}.spawnSync is
required for ${runtime}CommandRunner` or otherwise include runtime dynamically,
leaving the error code 'command-runner.missing-spawn-sync' and the metadata {
runtime } unchanged; modify the static missingSpawnSync(runtime) method in the
CommandRunnerError class accordingly.
🧹 Nitpick comments (2)
src/infrastructure/adapters/deno/index.js (1)
4-15: Consider additional Deno platform mappings.The current PLATFORM_MAP covers the major platforms, but Deno also supports additional OS values like
freebsd,netbsd,aix,solaris,illumos, andandroid. These would currently map to'unknown'. This is acceptable if those platforms aren't targeted, but worth noting for future expansion.src/infrastructure/adapters/KeychainAdapter.js (1)
22-33: Consider simplifying the_runmethod.The conditional logic checking
optionKeys.lengthto decide whether to passoptionsappears unnecessary. ThecommandRunner.runimplementations should handle empty options objects gracefully, and this adds complexity without clear benefit.Suggested simplification
_run(command, args = [], options = {}) { - const optionKeys = options ? Object.keys(options) : []; - const result = - optionKeys.length > 0 - ? this.commandRunner.run(command, args, options) - : this.commandRunner.run(command, args); + const result = this.commandRunner.run(command, args, options); if (!result || result.status !== 0) { return undefined; } const stdout = result.stdout; return typeof stdout === 'string' ? stdout.trim() : undefined; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
index.jssrc/domain/errors/AdapterError.jssrc/domain/errors/CommandRunnerError.jssrc/domain/errors/CommandRunnerPortError.jssrc/domain/errors/VaultServiceError.jssrc/domain/services/VaultService.jssrc/infrastructure/adapters/KeychainAdapter.jssrc/infrastructure/adapters/bun/CommandRunner.jssrc/infrastructure/adapters/bun/index.jssrc/infrastructure/adapters/deno/CommandRunner.jssrc/infrastructure/adapters/deno/index.jssrc/ports/CommandRunnerPort.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/infrastructure/adapters/bun/CommandRunner.js
- src/infrastructure/adapters/bun/index.js
- src/ports/CommandRunnerPort.js
- index.js
🧰 Additional context used
🧬 Code graph analysis (7)
src/domain/errors/VaultServiceError.js (1)
src/domain/errors/VaultError.js (1)
VaultError(4-12)
src/infrastructure/adapters/KeychainAdapter.js (5)
src/infrastructure/adapters/bun/index.js (1)
defaultPlatformGetter(5-5)src/infrastructure/adapters/node/index.js (1)
defaultPlatformGetter(5-5)src/utils/platform.js (1)
getPlatform(1-6)src/domain/errors/AdapterError.js (1)
AdapterError(3-15)src/domain/errors/PlatformNotSupportedError.js (1)
PlatformNotSupportedError(3-7)
src/infrastructure/adapters/deno/index.js (4)
src/infrastructure/adapters/KeychainAdapter.js (2)
defaultPlatformGetter(5-5)KeychainAdapter(7-129)src/infrastructure/adapters/bun/index.js (1)
defaultPlatformGetter(5-5)src/infrastructure/adapters/node/index.js (1)
defaultPlatformGetter(5-5)src/infrastructure/adapters/deno/CommandRunner.js (1)
DenoCommandRunner(18-46)
src/domain/errors/CommandRunnerError.js (1)
src/domain/errors/VaultError.js (1)
VaultError(4-12)
src/domain/errors/AdapterError.js (1)
src/domain/errors/VaultError.js (1)
VaultError(4-12)
src/domain/services/VaultService.js (1)
src/domain/errors/VaultServiceError.js (1)
VaultServiceError(3-35)
src/domain/errors/CommandRunnerPortError.js (1)
src/domain/errors/VaultError.js (1)
VaultError(4-12)
🔇 Additional comments (22)
src/domain/errors/CommandRunnerPortError.js (2)
1-6: LGTM!The class correctly extends
VaultErrorwith matching constructor signature. The pass-through constructor is acceptable for establishing a distinct error type in the domain hierarchy.
8-13: Clean factory method implementation.The static factory method follows a good pattern for creating well-defined, standardized errors. The kebab-case error code
'command-runner-port.missing-implementation'is consistent with domain naming conventions.src/domain/errors/CommandRunnerError.js (2)
3-6: LGTM!The class properly extends VaultError and the constructor correctly delegates to the superclass. The pass-through constructor maintains flexibility for future extensions.
8-13: LGTM!The
missingCommand()factory method is clean and provides a clear error message specific to the Deno runtime.src/domain/errors/AdapterError.js (1)
3-14: LGTM!The
AdapterErrorclass is well-structured with clear factory methods for common error scenarios. The error codes follow a consistent naming convention.src/infrastructure/adapters/deno/CommandRunner.js (4)
4-9: LGTM!Good defensive coding handling both string and Uint8Array inputs.
11-16: LGTM!Safe check for the Deno global with proper undefined handling.
18-21: LGTM!Constructor with optional dependency injection allows for testing while defaulting to the global Deno runtime.
23-45: Deno.Command API usage is correct.The implementation properly uses
result.codeto access the exit code fromoutputSync(). The Deno API confirms thatDeno.CommandOutputhas acodeproperty (number) for the child process exit code, along withstdoutandstderrasUint8Arrayproperties, all of which are correctly accessed in this implementation.src/infrastructure/adapters/deno/index.js (2)
17-31: LGTM!The factory function is well-documented and follows the same pattern as the Node and Bun adapters. The JSDoc comments provide clear guidance on the optional parameters.
33-33: LGTM!Re-exporting
DenoCommandRunnerenables direct usage when needed.src/infrastructure/adapters/KeychainAdapter.js (4)
8-15: LGTM!Good defensive validation requiring a
commandRunnerwith arunmethod. The dependency injection pattern withplatformGetterenables proper cross-runtime support.
35-61: LGTM!The
getmethod correctly uses the injected platform getter and handles all three supported platforms appropriately.
63-98: LGTM!The
setmethod correctly usescommandRunner.rundirectly for operations that don't need stdout processing, and properly returns boolean success status.
100-128: LGTM!The
deletemethod follows the same consistent pattern asset, using the command runner directly and returning boolean success status.src/domain/errors/VaultServiceError.js (2)
1-6: LGTM! Clean error class design.The class properly extends
VaultErrorand the constructor correctly delegates to the parent. This establishes a clear inheritance hierarchy for domain-specific errors.
8-34: Well-designed factory methods for standardized error creation.The static factory methods provide a clean API for creating domain-specific errors with consistent codes and messages. The
targetmetadata inclusion instoreFailedandsecretMissingis useful for debugging.src/domain/services/VaultService.js (5)
2-2: LGTM!Import correctly added for the new error class.
15-20: Clean adapter validation with proper guard ordering.The validation correctly short-circuits on
!adapterbefore attempting property access, preventing potential TypeErrors. The duck-typing approach (checking for get/set/delete functions) aligns well with the port-based architecture.
28-32: LGTM!Error handling correctly migrated to
VaultServiceError.missingTarget().
40-51: LGTM!Consistent error handling with appropriate factory methods for each failure mode.
59-63: LGTM!Consistent with other method error patterns.
- Implement dynamic architecture detection (x64 vs arm64) in Deno Dockerfile when fetching the Node.js binary; fixes build failures on AMD64 hosts. - Remove redundant `decode.js` module in favor of unified `codec.js` utility. - Fix duplicate export definition for `createNodeKeychainAdapter` in index.js.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile.deno (1)
20-25: Improve reproducibility/caching ofnpm installand avoid copyingnode_modulesat all.
If you have a lockfile, prefernpm ci --ignore-scripts; also consider copying onlypackage*.jsonfirst to maximize layer cache. Removing/app/node_modulesafterCOPYis usually a hint that.dockerignoreshould exclude it instead.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile.denoindex.jssrc/infrastructure/adapters/bun/CommandRunner.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/infrastructure/adapters/bun/CommandRunner.js
- index.js
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile.deno
[error] 16-16: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
🔇 Additional comments (2)
Dockerfile.deno (2)
26-27: Runtime entrypoint looks fine for Deno-based test execution.
GIT_STUNTS_DOCKER=1+CMD ["deno","task","test"]is a clean default.
5-17: The proposed fix introduces a critical security regression and is based on a misunderstanding of Deno permission semantics.The current code correctly uses
deno runwith explicit permission flags (--allow-net,--allow-write,--allow-env), which creates a sandboxed execution environment where only specified permissions are granted. The proposed fix switches todeno eval, which according to Deno's official documentation has implicit full permissions and ignores permission flags—including the--allow-net=nodejs.org,--allow-write=/tmp/node.tar.gz, and other scoped permissions shown in the diff. This would replace controlled sandboxing with unrestricted access, making the implementation less secure than the current approach, not more.Additionally, there is no evidence of an actual hadolint DL1000 parser error on this Dockerfile. The heredoc syntax (
RUN ... <<'EOF') is valid Docker syntax supported since Docker 18.09, and the multi-line structure itself does not inherently cause parsing fragility unless there are underlying BuildKit configuration issues specific to the build environment.The legitimate concern here is the lack of integrity verification (checksums or signatures) for the downloaded Node.js tarball, which the current or any revised approach should address. However, that requires adding hash validation logic, not switching to
deno eval.Likely an incorrect or invalid review comment.
…ration - Update `Vault` getter properties (`isMac`, `isLinux`, `isWindows`) to check `Deno.build.os` preventing crashes in Deno environments where `process` is undefined. - Enhance `scripts/run-tests.js` to fail fast if Docker Compose startup exits with an error code. - Switch local test execution to `npm exec vitest` for better binary resolution. - Simplify option passing in `KeychainAdapter` and update Architecture diagrams.
- Introduce `src/runtime` abstraction layer to wrap environment access, TTY detection, and interactive prompting. - Decouple `VaultService` from Node.js `process` globals; delegates I/O operations to the injected runtime strategy. - Implement native Deno support for `getEnv` and `promptSecret`, replacing Node-specific polyfills. - Pin Deno Docker base image SHA and upgrade Node sidecar to v20.19.6.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/infrastructure/adapters/KeychainAdapter.js:
- Around line 44-54: The get() PowerShell branch in KeychainAdapter currently
returns undefined when the CredentialManager module is absent, while set() exits
with code 1; make behavior consistent by updating the PowerShell script in get()
(the script string passed to this._run in the get() method) to explicitly exit
with a non-zero code when CredentialManager is not found (e.g., add an else {
exit 1 } after the Get-Module check or throw in the try block), so that
this._run reliably signals failure like set(); keep existing psLiteral usage and
ensure error handling remains in the catch block.
In @src/runtime/index.js:
- Around line 54-66: The Deno branch in defaultIsTTY uses the removed resource
ID API (Deno.stdin.rid and Deno.isatty) — update it to call
Deno.stdin.isTerminal() instead: in the defaultIsTTY function, when
isDenoRuntime is true, check for Deno.stdin?.isTerminal being a function (or
property) and call/inspect it inside a try/catch to return its boolean result;
remove the Deno.stdin.rid and Deno.isatty usage and keep the existing
process.stdin.isTTY fallback intact.
- Around line 32-42: The denoPromptSecret function currently uses the global
prompt() which does not mask input; update denoPromptSecret to use a
masked-secret input API (e.g., import and call promptSecret from "@std/cli") or
implement raw-mode reading from Deno.stdin to hide characters while typing;
ensure you keep the existing getPromptLabel(promptMessage) usage for the label,
throw the same cancellation error when the user aborts, and return the secret
string so the function signature remains unchanged.
🧹 Nitpick comments (2)
src/runtime/index.js (1)
7-30: Consider adding error handling for readline failures.The
nodePromptSecretfunction wraps readline in a Promise that only resolves, never rejects. Ifprocess.stdinemits an error or closes unexpectedly, the Promise will hang indefinitely.♻️ Suggested improvement
const nodePromptSecret = async (promptMessage) => { if (typeof process === 'undefined' || !process.stdin || !process.stderr) { throw new Error('Node stdin is required for prompting secrets'); } const { createInterface } = await import('node:readline'); - return new Promise((resolve) => { + return new Promise((resolve, reject) => { const label = getPromptLabel(promptMessage); const rl = createInterface({ input: process.stdin, output: process.stderr, terminal: true, }); rl.stdoutMuted = true; rl._writeToOutput = function (stringToWrite) { this.output.write(this.stdoutMuted ? '*' : stringToWrite); }; + rl.on('close', () => { + reject(new Error('Secret prompt was cancelled')); + }); rl.question(`${label}: `, (answer) => { rl.stdoutMuted = false; + rl.removeAllListeners('close'); rl.close(); process.stderr.write('\n'); resolve(answer); }); }); };src/infrastructure/adapters/KeychainAdapter.js (1)
22-29: Consider handling non-string stdout values.The
_runhelper assumesresult.stdoutis a string, but depending on the runner implementation, it could be aBufferorUint8Array. Adding explicit conversion would improve robustness.♻️ Suggested improvement
_run(command, args = [], options = {}) { const result = this.commandRunner.run(command, args, options); if (!result || result.status !== 0) { return undefined; } - const stdout = result.stdout; - return typeof stdout === 'string' ? stdout.trim() : undefined; + let stdout = result.stdout; + if (stdout instanceof Uint8Array || Buffer.isBuffer?.(stdout)) { + stdout = new TextDecoder().decode(stdout); + } + return typeof stdout === 'string' ? stdout.trim() : undefined; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ARCHITECTURE.mdDockerfile.denoindex.jsscripts/run-tests.jssrc/domain/errors/CommandRunnerError.jssrc/domain/services/VaultService.jssrc/infrastructure/adapters/KeychainAdapter.jssrc/runtime/index.jssrc/utils/platform.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/domain/errors/CommandRunnerError.js
- scripts/run-tests.js
- src/utils/platform.js
- ARCHITECTURE.md
🧰 Additional context used
🧬 Code graph analysis (3)
src/domain/services/VaultService.js (2)
src/domain/errors/VaultServiceError.js (1)
VaultServiceError(3-35)src/runtime/index.js (3)
defaultRuntime(70-75)defaultRuntime(70-75)answer(37-37)
index.js (5)
src/infrastructure/adapters/node/index.js (1)
createNodeKeychainAdapter(7-13)src/infrastructure/adapters/bun/index.js (1)
createBunKeychainAdapter(15-21)src/infrastructure/adapters/deno/index.js (1)
createDenoKeychainAdapter(25-31)src/domain/services/VaultService.js (1)
VaultService(7-111)src/runtime/index.js (2)
defaultRuntime(70-75)defaultRuntime(70-75)
src/infrastructure/adapters/KeychainAdapter.js (5)
src/infrastructure/adapters/bun/index.js (1)
defaultPlatformGetter(5-5)src/infrastructure/adapters/node/index.js (1)
defaultPlatformGetter(5-5)src/utils/platform.js (1)
getPlatform(1-18)src/domain/errors/AdapterError.js (1)
AdapterError(3-15)src/domain/errors/PlatformNotSupportedError.js (1)
PlatformNotSupportedError(3-7)
🪛 Hadolint (2.14.0)
Dockerfile.deno
[error] 16-16: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
🔇 Additional comments (10)
Dockerfile.deno (1)
18-26: LGTM!The Dockerfile structure follows best practices: layer caching is optimized by copying
package*.jsonbefore the full source,--ignore-scriptsmitigates supply-chain risks during install, and the pinned base image with SHA digest ensures reproducibility.src/runtime/index.js (1)
44-52: LGTM!The
defaultEnvGettercorrectly abstracts environment variable access across Deno and Node runtimes with appropriate fallbacks. ThedefaultRuntimeexport provides a clean interface for runtime-agnostic operations.Also applies to: 68-75
src/domain/services/VaultService.js (3)
73-83: Empty string environment values are treated as missing.The truthy check on
envValue(line 75) means an explicitly set empty environment variable (export MY_VAR="") will be ignored and fallback to the vault. This may be intentional, but it's worth noting as it differs from checkingenvValue !== undefined.Is treating empty env vars as "not set" the intended behavior? If an explicit empty value should be respected, consider:
- if (envValue) { + if (envValue !== undefined) {
13-21: LGTM!The constructor properly validates the adapter interface, ensuring
get,set, anddeletemethods are present before assignment. UsingVaultServiceError.missingAdapter()provides a clear, domain-specific error.
92-110: LGTM!The
ensureSecretmethod has good defensive checks: TTY validation before prompting, input trimming, empty value rejection, and proper error propagation via domain-specific errors.src/infrastructure/adapters/KeychainAdapter.js (1)
8-16: LGTM!The dependency injection pattern for
commandRunnerandplatformGetteris well-implemented. Constructor validation ensures the adapter won't fail at runtime due to missing methods. The platform-specific branching is clear and covers macOS, Linux, and Windows appropriately.Also applies to: 31-57
index.js (4)
47-55: LGTM!The
detectAdapterfunction provides sensible runtime detection with clear precedence: Bun → Deno → Node (fallback). The checks use runtime-specific APIs (Bun.spawnSync,Deno.Command) which are reliable indicators of the environment.
76-83: LGTM!The Vault constructor properly validates
adapterFactorytype when provided and cleanly delegates to either the custom factory ordetectAdapter. This maintains backward compatibility while enabling custom adapter injection.
89-99: LGTM!The platform detection getters consistently use
defaultRuntime.getPlatform(), ensuring uniform behavior across all runtimes instead of relying onprocess.platformdirectly.
18-22: No changes needed. The package is correctly configured as an ES module ("type": "module"in package.json), making the top-levelawaitvalid and appropriate for this context.
- Switch Deno secret prompting to `@std/cli/prompt` to ensure password input is masked (previously echoed to stdout). - Update Deno TTY detection to use modern `isTerminal()` API, replacing deprecated `isatty(rid)`. - Harden `KeychainAdapter` to explicitly decode `Buffer`/`Uint8Array` outputs, preventing regressions in environments returning raw bytes. - Fix Node.js `readline` flow to properly reject Promises on stream close (e.g., Ctrl+C interruptions).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/runtime/index.js:
- Around line 36-44: The denoPromptSecret function is calling promptSecret with
an incorrect object; update denoPromptSecret to call promptSecret with the
prompt string as the first argument (e.g., promptSecret(`${label}: `)) and, if
using options, pass a valid PromptSecretOptions as the second arg (only mask and
clear are allowed, e.g., { mask: '*' }) while keeping the existing
null/undefined check and error throw; locate and modify the denoPromptSecret
function and its call to promptSecret accordingly.
🧹 Nitpick comments (2)
src/runtime/index.js (1)
19-22: Relies on internal readline API for input masking.The
_writeToOutputoverride and customstdoutMutedproperty are implementation details of Node's readline that could change in future versions. This is a well-known workaround, but consider documenting this fragility or exploring alternatives like thereadpackage for more stable secret prompting.src/infrastructure/adapters/KeychainAdapter.js (1)
67-75: Consider removing redundant delete before add on macOS.The
security add-generic-passwordcommand with-Uflag already handles updating existing entries. The precedingdelete-generic-passwordcall (lines 68-70) may be redundant and adds an extra system call.♻️ Proposed simplification
if (platform === 'darwin') { - this.commandRunner.run('security', ['delete-generic-password', '-a', this.account, '-s', target], { - stdio: 'ignore', - }); const res = this.commandRunner.run('security', ['add-generic-password', '-a', this.account, '-s', target, '-w', value, '-U'], { stdio: 'ignore', }); return res && res.status === 0; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignoresrc/infrastructure/adapters/KeychainAdapter.jssrc/runtime/index.js
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
src/infrastructure/adapters/KeychainAdapter.js (5)
src/infrastructure/adapters/bun/index.js (1)
defaultPlatformGetter(5-5)src/infrastructure/adapters/node/index.js (1)
defaultPlatformGetter(5-5)src/utils/platform.js (1)
getPlatform(1-18)src/domain/errors/AdapterError.js (1)
AdapterError(3-15)src/domain/errors/PlatformNotSupportedError.js (1)
PlatformNotSupportedError(3-7)
🔇 Additional comments (9)
src/runtime/index.js (3)
1-5: LGTM!The runtime detection pattern using
typeof Deno !== 'undefined' && typeof Deno.version?.deno === 'string'is a robust way to identify the Deno runtime, and thegetPromptLabelhelper provides a safe default.
46-68: LGTM!Both
defaultEnvGetteranddefaultIsTTYproperly handle runtime detection with appropriate fallbacks. The try-catch indefaultIsTTYfor Deno is good defensive coding to handle potential permission errors.
70-77: LGTM!The runtime-based selection at module load time is appropriate, and the exported
defaultRuntimeobject provides a clean interface for cross-environment operations.src/infrastructure/adapters/KeychainAdapter.js (6)
1-5: LGTM!Imports are clean and the
defaultPlatformGetterpattern is consistent with other adapter implementations (bun, node).
7-16: LGTM!Good dependency injection pattern with proper validation. The constructor correctly validates that
commandRunnerexists and has arunmethod, using the domain-specific error class.
18-20: LGTM!The computed
platformgetter properly delegates to the injectedplatformGetter.
22-34: LGTM!The
_runmethod properly handles both Deno (Uint8Array) and Node (Buffer) stdout formats with appropriate decoding. The null-check pattern handles edge cases correctly.
36-62: LGTM!The
getmethod properly handles platform-specific credential retrieval with correct command arguments. The PowerShell escaping viapsLiteralcorrectly handles single quotes, and explicitexit 1codes ensure proper error signaling.
101-129: LGTM!The
deletemethod properly handles platform-specific credential deletion. The Windows implementation correctly usesErrorAction SilentlyContinueto handle non-existent credentials gracefully.
- Update `promptSecret` call site to pass the message string as the first argument and options as the second, matching the library interface. - Explicitly set mask character to `*` for visual feedback during entry.
1 participant
Summary by CodeRabbit
New Features
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.