feat: multi-runtime support and hexagonal architecture hardening #1
Conversation
- Add GitSha value object with SHA-1 validation - Add GitRef value object with Git reference validation - Add GitObjectType value object with fast integer comparison - Add GitBlob entity for Git blob objects - Add custom error types for semantic error handling - Implement factory patterns for object creation - Use JSON schema validation with Ajv - Maintain backward compatibility with existing test structure
- Add Node.js, Bun, and Deno platform adapters with auto-detection - Introduce GitSignature, GitFileMode, and GitSignature value objects - Implement multi-runtime Docker testing infrastructure - Establish formal GitPlumbingError hierarchy - Harden security with CommandSanitizer and argument validation - Enforce strict SRP and "one class per file" structure - Replace magic numbers/strings with semantic constants - Provide Dev Container configurations for all runtimes
|
Warning Rate limit exceeded@flyingrobots has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 38 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 (10)
📝 WalkthroughWalkthroughAdds multi‑runtime support (Node, Bun, Deno) with runtime‑detecting shell runners, a streaming execution model (GitStream), extensive Zod schemas, many domain value objects/entities/services/errors, command sanitization/builders, execution orchestration/retry, package exports/engines, devcontainer/Docker CI, and comprehensive tests/scripts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant GitPlumbing
participant Sanitizer as CommandSanitizer / GitCommandBuilder
participant Factory as ShellRunnerFactory
participant Adapter as RuntimeAdapter (Node/Bun/Deno)
participant Runtime as OS
Client->>GitPlumbing: executeStream(args / commit request)
activate GitPlumbing
GitPlumbing->>Sanitizer: build & sanitize args
GitPlumbing->>Factory: ShellRunnerFactory.create()
Factory->>Factory: detect runtime (Bun → Deno → Node)
Factory-->>GitPlumbing: bound Adapter.run
GitPlumbing->>Adapter: run({command,args,cwd,input,timeout,env})
activate Adapter
Adapter->>Runtime: spawn process (streaming stdin/stdout/stderr)
Runtime-->>Adapter: stdout stream + stderr + exit code
Adapter-->>GitPlumbing: RunnerResult { stdoutStream, exitPromise }
deactivate Adapter
GitPlumbing->>GitPlumbing: orchestrate(retries/backoff/classify)
alt success
GitPlumbing-->>Client: GitStream / SHA / status
else failure
GitPlumbing-->>Client: throw GitPlumbingError (operation + details)
end
deactivate GitPlumbing
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 14
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 22-27: The CI job "test-multi-runtime" currently runs npm test
without setting up Node or installing dependencies; update the job to use
actions/setup-node (for example referencing the job name test-multi-runtime) to
install a specific Node.js version and add an install step (npm ci or npm
install) before the existing run: npm test step so npm is available and
dependencies are installed prior to running tests.
In @Dockerfile.deno:
- Line 1: The Dockerfile uses an unstable image tag ("denoland/deno:latest")
which can break reproducible builds; change the FROM instruction in
Dockerfile.deno to reference a specific Deno version tag (for example
"denoland/deno:2.6.3" or your chosen pinned version) so CI and local builds use
a consistent Deno runtime.
In @eslint.config.js:
- Around line 10-19: The globals object in eslint config currently exposes test
globals describe, it, and expect at top-level; remove those three entries from
the main globals block so they are only declared in the test-specific
configuration; update the globals object (remove "describe", "it", "expect") and
leave Bun/Deno/globalThis/TextEncoder-related entries intact to ensure test
globals are not available in non-test files.
In @package.json:
- Line 8: The current npm "test" script points to
"./scripts/run-multi-runtime-tests.sh", which will fail on Windows without
WSL/Git Bash; either document this requirement in the README and CONTRIBUTING
(mention WSL/Git Bash or Docker usage) or add a cross-platform alternative npm
script (e.g., a PowerShell-compatible "test:windows" or a Node/JS wrapper that
invokes the same Docker steps) and keep the original "test" for Unix; update
package.json script entries (the "test" key and add a new "test:windows" or
"test:docker" key) and add brief usage notes so Windows developers know how to
run tests.
In @scripts/run-multi-runtime-tests.sh:
- Line 7: The script uses the deprecated standalone command "docker-compose up
--build" which is not available in the CI; update the invocation in
scripts/run-multi-runtime-tests.sh to use the Docker Compose plugin by replacing
that command with "docker compose up --build" (i.e., remove the hyphen) so the
CI uses the built-in docker compose plugin instead of the legacy binary.
- Line 13: The status-check uses the deprecated "docker-compose" binary when
assigning STATUS; replace that invocation with the modern "docker compose" form
so the command that sets STATUS (STATUS=$(docker-compose ps -a --format
"{{.ExitCode}}" $service)) uses "docker compose ps -a --format ..." instead,
mirroring the fix applied earlier on line 7.
In @src/domain/entities/GitBlob.js:
- Around line 18-33: The constructor for GitBlob validates sha but not content,
so pass-through of undefined/null/invalid types can break ByteMeasurer.measure
in size(); update GitBlob.constructor to validate that content is either a
string or a Uint8Array (and not null/undefined) and throw an
InvalidArgumentError with context if invalid, update static fromContent to
preserve behavior (it can rely on the constructor validation), and ensure any
callers that construct GitBlob with content follow the same types.
In @src/domain/entities/GitCommit.js:
- Around line 23-42: The constructor of GitCommit lacks validation for the
parents parameter which should be an array of GitSha instances; add a runtime
check in the GitCommit constructor to ensure parents is an Array and every
element is an instance of GitSha (or allow null/empty array if intended), and
throw an InvalidArgumentError similar to other checks if validation fails;
update any related assumptions in methods like isRoot and isMerge to rely on
this validated parents array.
In @src/domain/entities/GitTree.js:
- Around line 18-24: Validate the entries parameter in the GitTree constructor:
ensure entries is an Array and every element is an instance of GitTreeEntry; if
not, throw an InvalidArgumentError (reusing the same error type and location
context like 'GitTree.constructor') and include the invalid entries in the error
metadata; after validation, assign this.entries = entries (keeping the sha
validation for GitSha as-is).
In @src/domain/entities/GitTreeEntry.js:
- Around line 18-28: The constructor of GitTreeEntry validates mode and sha but
not path, so add validation in GitTreeEntry.constructor to ensure path is a
non-empty string (e.g., typeof path === 'string' && path.trim().length > 0); if
invalid, throw InvalidArgumentError with a clear message and same context
pattern used for mode/sha (e.g., 'Path must be a non-empty string',
'GitTreeEntry.constructor', { path }) before assigning this.path.
In @src/domain/value-objects/GitObjectType.js:
- Around line 34-41: STRING_TO_INT currently maps OFS_DELTA and REF_DELTA to
their string constants, causing fromString('ofs-delta') to produce a
GitObjectType with a string value; replace the values for keys
GitObjectType.OFS_DELTA and GitObjectType.REF_DELTA with the numeric constants
GitObjectType.OFS_DELTA_INT and GitObjectType.REF_DELTA_INT respectively so
STRING_TO_INT maps all types to their numeric *_INT counterparts and preserves
isValid(), toNumber(), and equals() behavior.
In @src/domain/value-objects/GitRef.js:
- Around line 76-79: The current _isNotReserved(ref) rejects any ref containing
'@', which is too strict; update the method to only reject a lone '@' and the
'@{' sequence used by reflog syntax. Specifically, in the static
_isNotReserved(ref) function, return false if ref === '@' or if
ref.includes('@{'), otherwise return true (also ensure ref is treated as a
string). Use the _isNotReserved identifier to locate and replace the logic
accordingly.
- Around line 34-42: The isValid method in GitRef allows empty strings; update
GitRef.isValid to explicitly reject empty strings by adding a check that ref is
a non-empty string (e.g., ensure typeof ref === 'string' && ref.length > 0)
before calling this._hasValidStructure, this._hasNoProhibitedChars, and
this._isNotReserved so empty refs are treated as invalid.
In @src/infrastructure/adapters/deno/DenoShellRunner.js:
- Around line 26-30: The code writes to the stdin writable stream without
awaiting the write promise, causing a race between writer.write() and
writer.close(); update the input handling in the DenoShellRunner (the block that
checks if (input && child.stdin)) to await the promise returned by
writer.write(...) before calling await writer.close(), ensuring the write fully
completes to child.stdin.getWriter() prior to closing the writer.
🧹 Nitpick comments (32)
eslint.config.js (1)
24-24: Consider refactoring high-parameter functions to use config objects.While
GitCommitcurrently needs 6 parameters, allowing up to 7 parameters can lead to harder-to-maintain and harder-to-test code. Consider refactoring functions with many parameters to accept a configuration object or use a builder pattern instead.Example refactoring approach:
// Instead of: new GitCommit(sha, author, committer, message, tree, parents); // Consider: new GitCommit({ sha, author, committer, message, tree, parents });scripts/pre-commit (1)
1-7: Consider using shell idiom for cleaner exit status handling.The script works correctly. A minor improvement would be to use the shell's direct conditional syntax.
♻️ Optional refactor
#!/bin/sh echo "Running pre-commit linting..." -npm run lint -if [ $? -ne 0 ]; then +if ! npm run lint; then echo "Linting failed. Commit aborted." exit 1 fiCONTRIBUTING.md (1)
5-7: Consider adding a link to the Code of Conduct.The Code of Conduct is referenced but not linked. If one exists (e.g.,
CODE_OF_CONDUCT.md), consider linking it. If not, consider adding one or removing the reference.src/domain/services/GitCommandBuilder.js (2)
12-15: Remove unused_commandfield.The
_commandfield is stored in the constructor but never accessed elsewhere in the class. Since_argsalready contains the command at index 0, this field is redundant.♻️ Proposed refactor to remove unused field
constructor(command) { - this._command = command; this._args = [command]; }
47-52: Consider stricter type validation inarg()method.The current implementation converts any non-null/undefined value to a string using
String(arg). This could produce unexpected results for objects (e.g.,"[object Object]") or other complex types. Consider validating that the argument is already a string or a primitive type that safely coerces to a meaningful string representation.♻️ Proposed validation enhancement
arg(arg) { if (arg !== undefined && arg !== null) { + if (typeof arg !== 'string' && typeof arg !== 'number' && typeof arg !== 'boolean') { + throw new TypeError(`Argument must be a string, number, or boolean, got ${typeof arg}`); + } this._args.push(String(arg)); } return this; }README.md (1)
10-10: Fix grammatical issue in feature list."Harden Security" should be "Hardened Security" or "Security Hardening" for proper grammar.
📝 Proposed fix
-- **Harden Security**: Integrated `CommandSanitizer` to prevent argument injection attacks. +- **Hardened Security**: Integrated `CommandSanitizer` to prevent argument injection attacks.Dockerfile.node (1)
4-5: Optimize Docker layer caching for faster builds.The current
COPY . .followed bynpm installmeans any code change invalidates the dependency installation layer. Consider copyingpackage*.jsonfiles first, runningnpm install, then copying the rest of the application code. This leverages Docker's layer caching and significantly speeds up rebuilds when only application code changes.🐳 Proposed optimization
WORKDIR /app -COPY . . +COPY package*.json ./ RUN npm install --ignore-scripts +COPY . . CMD ["npm", "run", "test:local"]Additionally, consider adding a
.dockerignorefile to excludenode_modules,.git, and other unnecessary files from the build context.test/domain/services/ByteMeasurer.test.js (1)
4-13: Consider adding edge case tests for completeness.The current test coverage is solid for typical usage. Consider adding tests for edge cases such as empty strings, empty Uint8Array, and strings with various multi-byte characters (e.g., Chinese/Japanese characters) to ensure robustness across different UTF-8 scenarios.
📝 Example edge case tests
it('measures Uint8Array length', () => { const data = new Uint8Array([1, 2, 3, 4]); expect(ByteMeasurer.measure(data)).toBe(4); }); + + it('handles edge cases', () => { + expect(ByteMeasurer.measure('')).toBe(0); + expect(ByteMeasurer.measure(new Uint8Array([]))).toBe(0); + expect(ByteMeasurer.measure('你好')).toBe(6); // 2 Chinese chars = 6 bytes + }); });Dockerfile.bun (1)
1-1: Pin the Bun image version for reproducible builds.Using
oven/bun:latestcan lead to non-deterministic builds as the base image may change over time. Consider pinning to a specific Bun version to ensure consistent CI/CD behavior and easier debugging of environment-related issues.🔧 Suggested fix
-FROM oven/bun:latest +FROM oven/bun:1.1.34Note: Replace
1.1.34with your target Bun version..devcontainer/bun/devcontainer.json (1)
17-17: Consider using a non-root user for better security hygiene.While running as
rootis common in development containers, using a non-root user can help catch permission-related issues earlier and follows security best practices. This is especially valuable if the dev container mimics production environments.🔐 Alternative configuration
- "remoteUser": "root" + "remoteUser": "bun"Note: Ensure the
bunuser exists in the Dockerfile or create it if needed.docker-compose.yml (1)
8-9: Consider adding environment variables to all test services for consistency.The
node-testservice setsNODE_ENV=test, butbun-testanddeno-testdon't define any environment variables. If test mode configuration is needed for Node.js, consider whether Bun and Deno also benefit from explicit test environment indicators.♻️ Proposed enhancement for consistency
bun-test: build: context: . dockerfile: Dockerfile.bun + environment: + - NODE_ENV=test deno-test: build: context: . dockerfile: Dockerfile.deno + environment: + - DENO_ENV=testtest/ShellRunner.test.js (1)
25-34: Cross-platform consideration for stdin test.The
catcommand is Unix-specific. Given the PR context of Dockerized CI running Linux containers for Node/Bun/Deno, this should work in the test environments. However, if tests are ever run directly on Windows, this will fail..devcontainer/node/devcontainer.json (1)
1-18: LGTM!Well-configured dev container with appropriate VS Code extensions for the project's tooling stack.
Consider using a non-root user for development to better align with production security practices. This is optional for development environments but can help catch permission-related issues early.
SECURITY.md (1)
22-26: Minor style refinement.Per static analysis, "outside of" can be simplified to "outside" for conciseness.
📝 Suggested change
### Hardened Scope This project specifically focuses on preventing: - **Argument Injection**: Malicious flags passed to Git CLI. -- **Path Traversal**: Unauthorized access outside of the repository's `cwd`. +- **Path Traversal**: Unauthorized access outside the repository's `cwd`. - **ReDoS**: Regular expression denial of service in validation logic.src/domain/services/ByteMeasurer.js (1)
14-19: Consider defensive null/undefined check.The method assumes
contentis either a string or has a.lengthproperty. Ifcontentisnullorundefined, this will throw a TypeError. Since this is a domain service, callers should validate input, but a guard could improve robustness.🛡️ Optional defensive check
static measure(content) { + if (content == null) { + return 0; + } if (typeof content === 'string') { return new TextEncoder().encode(content).length; } return content.length; }test/domain/value-objects/GitObjectType.test.js (1)
1-65: LGTM!Comprehensive test coverage for GitObjectType value object, including constructor validation, factory methods, equality checks, and type identification helpers.
Consider adding tests for
fromNumber,toJSON, and the delta types (ofsDelta(),refDelta()) for complete API coverage. This is optional as the current tests cover the primary use cases.test/domain/entities/GitCommit.test.js (1)
15-41: LGTM!Good coverage of commit creation scenarios: root commits, single-parent commits, and merge commits. The invalid tree test correctly verifies the error type.
Consider adding tests for:
isWritten()behavior with null vs. valid SHA- Invalid author/committer arguments (non-GitSignature)
- Invalid SHA argument (non-GitSha, non-null)
These would strengthen validation coverage.
test/domain/value-objects/GitFileMode.test.js (1)
1-30: Consider expanding test coverage for completeness.The test suite covers basic scenarios but omits several modes and methods available in GitFileMode:
isExecutable()is tested with REGULAR mode but not with EXECUTABLE mode- SYMLINK and COMMIT modes are not tested
getObjectType()method is not testedWhile the current tests are correct, adding coverage for these would improve confidence in the value object's behavior.
📋 Suggested additional test cases
Add these test cases to the 'is methods' describe block:
+ it('correctly identifies executable file', () => { + const mode = new GitFileMode(GitFileMode.EXECUTABLE); + expect(mode.isExecutable()).toBe(true); + expect(mode.isRegular()).toBe(false); + }); + + it('correctly identifies symlink', () => { + const mode = new GitFileMode(GitFileMode.SYMLINK); + expect(mode.isExecutable()).toBe(false); + expect(mode.isRegular()).toBe(false); + expect(mode.isTree()).toBe(false); + }); + }); + + describe('getObjectType', () => { + it('returns tree type for TREE mode', () => { + const mode = new GitFileMode(GitFileMode.TREE); + expect(mode.getObjectType().isTree()).toBe(true); + }); + + it('returns blob type for file modes', () => { + const mode = new GitFileMode(GitFileMode.REGULAR); + expect(mode.getObjectType().isBlob()).toBe(true); + }); + + it('returns commit type for COMMIT mode', () => { + const mode = new GitFileMode(GitFileMode.COMMIT); + expect(mode.getObjectType().isCommit()).toBe(true); + });src/domain/value-objects/GitSignature.js (1)
21-23: Email validation is minimal.The current validation only checks for the presence of '@' in the email. While this is sufficient for basic Git signature requirements, consider adding more robust email validation if stricter conformance is needed.
📧 Optional: More robust email validation
- if (!email || typeof email !== 'string' || !email.includes('@')) { + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + if (!email || typeof email !== 'string' || !emailRegex.test(email)) { throw new ValidationError('Valid email is required', 'GitSignature.constructor', { email }); }test/GitSha.test.js (1)
11-11: Misleading constant name.The constant
INVALID_SHA_MIXED_CASEcontains a valid SHA-1 hash (the empty tree SHA in uppercase), not an invalid one. Consider renaming toMIXED_CASE_SHAorVALID_SHA_MIXED_CASEto accurately reflect that it's testing case normalization rather than invalidity.📝 Suggested rename
-const INVALID_SHA_MIXED_CASE = '4B825DC642CB6EB9A060E54BF8D69288FBEE4904'; +const MIXED_CASE_SHA = '4B825DC642CB6EB9A060E54BF8D69288FBEE4904';And update the usage on Line 35:
- const sha = new GitSha(INVALID_SHA_MIXED_CASE); + const sha = new GitSha(MIXED_CASE_SHA);src/domain/services/CommandSanitizer.js (1)
11-17: Consider adding additional prohibited flags for defense in depth.While the current list covers the most dangerous flags (
--upload-pack,--receive-pack,--ext-cmd,--config,-c), you might consider adding other flags that could pose security risks depending on your threat model:
--git-dir- could access arbitrary repositories--work-tree- could access arbitrary directories--exec- could execute arbitrary commands in some contextsThis depends on how the library is used and what trust boundaries exist.
src/domain/value-objects/GitFileMode.js (1)
45-90: Consider addingisSymlink()for completeness.The methods are well-implemented. For symmetry and completeness, you might consider adding an
isSymlink()method alongsideisRegular()andisExecutable():isExecutable() { return this._value === GitFileMode.EXECUTABLE; } + + /** + * Checks if this is a symbolic link + * @returns {boolean} + */ + isSymlink() { + return this._value === GitFileMode.SYMLINK; + }test/GitRef.test.js (1)
25-28: Optional: Consolidate redundant assertions.Lines 26 and 27 both assert the same behavior on the same call. The second assertion is redundant since if line 26 passes (throwing ValidationError), the error message is already verified by the error type. Consider removing line 27 or combining into a single assertion.
♻️ Proposed consolidation
- it('throws error for invalid reference string', () => { - expect(() => new GitRef(INVALID_REF_DOT_START)).toThrow(ValidationError); - expect(() => new GitRef(INVALID_REF_DOT_START)).toThrow('Invalid Git reference: .refs/heads/main'); - }); + it('throws ValidationError for invalid reference string', () => { + expect(() => new GitRef(INVALID_REF_DOT_START)).toThrow(ValidationError); + });Or if you want to verify the message:
- it('throws error for invalid reference string', () => { - expect(() => new GitRef(INVALID_REF_DOT_START)).toThrow(ValidationError); - expect(() => new GitRef(INVALID_REF_DOT_START)).toThrow('Invalid Git reference: .refs/heads/main'); - }); + it('throws ValidationError with descriptive message for invalid reference string', () => { + const error = () => new GitRef(INVALID_REF_DOT_START); + expect(error).toThrow(ValidationError); + expect(error).toThrow('Invalid Git reference: .refs/heads/main'); + });src/domain/entities/GitTree.js (1)
39-44: Consider defensive immutability for entries array.The
addEntrymethod correctly returns a newGitTreeinstance (Line 43), maintaining immutability. However, theentriesarray itself is mutable, allowing external code to modify it after construction.Consider freezing the entries array or making a defensive copy to prevent external mutation.
♻️ Optional defensive approach
addEntry(entry) { if (!(entry instanceof GitTreeEntry)) { throw new InvalidArgumentError('Entry must be a GitTreeEntry instance', 'GitTree.addEntry', { entry }); } - return new GitTree(this.sha, [...this.entries, entry]); + const newEntries = [...this.entries, entry]; + return new GitTree(this.sha, Object.freeze(newEntries)); }And in constructor:
constructor(sha, entries = []) { if (sha && !(sha instanceof GitSha)) { throw new InvalidArgumentError('SHA must be a GitSha instance or null', 'GitTree.constructor', { sha }); } this.sha = sha; - this.entries = entries; + this.entries = Object.freeze([...entries]); }ShellRunner.js (1)
21-24: Consider caching the runner instance if factory creation is expensive.The
runmethod creates a new runner instance on every call (Line 22). IfShellRunnerFactory.create()involves expensive initialization or ifrunis called frequently, consider caching the runner instance.However, if runners are stateless and factory creation is lightweight, the current approach is acceptable and avoids potential stale state issues.
♻️ Optional singleton pattern
export default class ShellRunner { + static #runner = null; + + /** + * Gets or creates the runner instance + * @private + */ + static #getRunner() { + if (!this.#runner) { + this.#runner = ShellRunnerFactory.create(); + } + return this.#runner; + } + /** * Executes a command. * @param {Object} options * @param {string} options.command * @param {string[]} options.args * @param {string} [options.cwd] * @param {string|Uint8Array} [options.input] * @returns {Promise<{stdout: string, stderr: string, code: number}>} */ static async run(options) { - const runner = ShellRunnerFactory.create(); + const runner = this.#getRunner(); return runner.run(options); } }src/infrastructure/adapters/node/NodeShellRunner.js (1)
24-29: Simplify the complex exit code logic for readability.Line 28 uses a nested ternary operator to determine the exit code, which is difficult to read and maintain. Consider extracting this logic into a helper method or using a more explicit if-else structure.
♻️ Proposed simplification
Option 1: Extract to helper method
export default class NodeShellRunner { static MAX_BUFFER = 100 * 1024 * 1024; // 100MB + /** + * Determines exit code from execFile error + * @private + */ + static _getExitCode(error) { + if (!error) return 0; + if (typeof error.code === 'number') return error.code; + return 1; + } + /** * Executes a command * @type {import('../../../../contract.js').CommandRunner} */ async run({ command, args, cwd, input }) { return new Promise((resolve) => { const child = execFile(command, args, { cwd, encoding: 'utf8', maxBuffer: NodeShellRunner.MAX_BUFFER }, (error, stdout, stderr) => { resolve(RunnerResultSchema.parse({ stdout: stdout || '', stderr: stderr || '', - code: error && typeof error.code === 'number' ? error.code : (error ? 1 : 0) + code: NodeShellRunner._getExitCode(error) })); }); if (input && child.stdin) { child.stdin.write(input); child.stdin.end(); } }); } }Option 2: Use explicit logic inline
}, (error, stdout, stderr) => { + let code = 0; + if (error) { + code = typeof error.code === 'number' ? error.code : 1; + } + resolve(RunnerResultSchema.parse({ stdout: stdout || '', stderr: stderr || '', - code: error && typeof error.code === 'number' ? error.code : (error ? 1 : 0) + code })); });src/domain/value-objects/GitRef.js (2)
95-102: Redundant try-catch infromStringOrNull.Since
isValid(ref)is called before constructing theGitRef, the constructor should never throw. The try-catch is defensive but unnecessary.♻️ Simplified version
static fromStringOrNull(ref) { - try { - if (!GitRef.isValid(ref)) {return null;} - return new GitRef(ref); - } catch { - return null; - } + if (!GitRef.isValid(ref)) {return null;} + return new GitRef(ref); }
154-169: JSDoc comment is incomplete.The JSDoc says "without refs/heads/ prefix" but the method also handles tags and remotes.
📝 Proposed fix
/** - * Gets the short name of the reference (without refs/heads/ prefix) + * Gets the short name of the reference (without refs/heads/, refs/tags/, or refs/remotes/ prefix) * @returns {string} */src/domain/value-objects/GitSha.js (3)
31-36: Consider compiling the regex once.The regex is recreated on every
isValid()call. For a hot path, this could be optimized by using a static compiled regex.♻️ Proposed optimization
export default class GitSha { static LENGTH = 40; static SHORT_LENGTH = 7; static EMPTY_TREE_VALUE = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'; + static #VALID_SHA_REGEX = /^[a-f0-9]{40}$/; // ... static isValid(sha) { if (typeof sha !== 'string') {return false;} if (sha.length !== GitSha.LENGTH) {return false;} - const regex = new RegExp(`^[a-f0-9]{${GitSha.LENGTH}}$`); - return regex.test(sha.toLowerCase()); + return GitSha.#VALID_SHA_REGEX.test(sha.toLowerCase()); }
52-59: Redundant try-catch infromStringOrNull.Same as
GitRef.fromStringOrNull— sinceisValid()is checked first, the constructor won't throw.♻️ Simplified version
static fromStringOrNull(sha) { - try { - if (!GitSha.isValid(sha)) {return null;} - return new GitSha(sha); - } catch { - return null; - } + if (!GitSha.isValid(sha)) {return null;} + return new GitSha(sha); }
107-109:EMPTY_TREEgetter creates a new instance on every access.Since
GitShais immutable, consider caching the empty tree instance to avoid repeated allocations.♻️ Proposed caching
export default class GitSha { static LENGTH = 40; static SHORT_LENGTH = 7; static EMPTY_TREE_VALUE = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'; + static #EMPTY_TREE_INSTANCE = null; // ... static get EMPTY_TREE() { - return new GitSha(GitSha.EMPTY_TREE_VALUE); + if (!GitSha.#EMPTY_TREE_INSTANCE) { + GitSha.#EMPTY_TREE_INSTANCE = new GitSha(GitSha.EMPTY_TREE_VALUE); + } + return GitSha.#EMPTY_TREE_INSTANCE; } }This is used in tests (
const sha = GitSha.EMPTY_TREE;) and potentially in production code for tree comparisons. Caching ensuressha1.equals(GitSha.EMPTY_TREE) && sha1 === GitSha.EMPTY_TREEworks as expected with reference equality.src/domain/value-objects/GitObjectType.js (1)
167-197: Consider addingisOfsDelta()andisRefDelta()inspection methods.For consistency with
isBlob(),isTree(),isCommit(), andisTag(), you may want to add inspection methods for delta types.♻️ Optional additions
/** * Checks if this is an ofs-delta type * @returns {boolean} */ isOfsDelta() { return this._value === GitObjectType.OFS_DELTA_INT; } /** * Checks if this is a ref-delta type * @returns {boolean} */ isRefDelta() { return this._value === GitObjectType.REF_DELTA_INT; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (56)
.devcontainer/bun/devcontainer.json.devcontainer/deno/devcontainer.json.devcontainer/node/devcontainer.json.dockerignore.github/workflows/ci.ymlCHANGELOG.mdCONTRIBUTING.mdDockerfile.bunDockerfile.denoDockerfile.nodeNOTICEREADME.mdSECURITY.mdShellRunner.jscontract.jsdeno.jsondocker-compose.ymleslint.config.jsindex.jspackage.jsonscripts/pre-commitscripts/pre-pushscripts/run-multi-runtime-tests.shsrc/domain/entities/GitBlob.jssrc/domain/entities/GitCommit.jssrc/domain/entities/GitTree.jssrc/domain/entities/GitTreeEntry.jssrc/domain/errors/GitPlumbingError.jssrc/domain/errors/InvalidArgumentError.jssrc/domain/errors/InvalidGitObjectTypeError.jssrc/domain/errors/ValidationError.jssrc/domain/services/ByteMeasurer.jssrc/domain/services/CommandSanitizer.jssrc/domain/services/GitCommandBuilder.jssrc/domain/value-objects/GitFileMode.jssrc/domain/value-objects/GitObjectType.jssrc/domain/value-objects/GitRef.jssrc/domain/value-objects/GitSha.jssrc/domain/value-objects/GitSignature.jssrc/infrastructure/adapters/bun/BunShellRunner.jssrc/infrastructure/adapters/deno/DenoShellRunner.jssrc/infrastructure/adapters/node/NodeShellRunner.jssrc/infrastructure/factories/ShellRunnerFactory.jstest/GitBlob.test.jstest/GitRef.test.jstest/GitSha.test.jstest/ShellRunner.test.jstest/deno_entry.jstest/deno_shim.jstest/domain/entities/GitCommit.test.jstest/domain/entities/GitTree.test.jstest/domain/entities/GitTreeEntry.test.jstest/domain/errors/Errors.test.jstest/domain/services/ByteMeasurer.test.jstest/domain/value-objects/GitFileMode.test.jstest/domain/value-objects/GitObjectType.test.js
🧰 Additional context used
🧬 Code graph analysis (25)
test/ShellRunner.test.js (1)
ShellRunner.js (1)
ShellRunner(11-25)
src/domain/entities/GitCommit.js (5)
src/domain/value-objects/GitSha.js (1)
GitSha(11-110)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/entities/GitTree.js (1)
GitTree(13-61)src/domain/value-objects/GitSignature.js (1)
GitSignature(10-40)src/domain/value-objects/GitObjectType.js (1)
GitObjectType(10-198)
src/domain/services/CommandSanitizer.js (1)
src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/domain/errors/InvalidGitObjectTypeError.js (1)
src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
src/domain/errors/ValidationError.js (2)
src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)test/domain/entities/GitCommit.test.js (1)
message(13-13)
test/domain/errors/Errors.test.js (4)
src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/errors/InvalidGitObjectTypeError.js (1)
InvalidGitObjectTypeError(10-15)
test/GitRef.test.js (2)
src/domain/value-objects/GitRef.js (1)
GitRef(11-198)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/domain/entities/GitTreeEntry.js (3)
src/domain/value-objects/GitFileMode.js (1)
GitFileMode(11-90)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/value-objects/GitSha.js (1)
GitSha(11-110)
src/infrastructure/factories/ShellRunnerFactory.js (3)
src/infrastructure/adapters/bun/BunShellRunner.js (1)
BunShellRunner(10-41)src/infrastructure/adapters/deno/DenoShellRunner.js (1)
DenoShellRunner(10-40)src/infrastructure/adapters/node/NodeShellRunner.js (1)
NodeShellRunner(11-38)
test/domain/entities/GitCommit.test.js (5)
src/domain/entities/GitTree.js (1)
GitTree(13-61)src/domain/value-objects/GitSignature.js (1)
GitSignature(10-40)src/domain/entities/GitCommit.js (1)
GitCommit(14-75)src/domain/value-objects/GitSha.js (1)
GitSha(11-110)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)
test/domain/services/ByteMeasurer.test.js (1)
src/domain/services/ByteMeasurer.js (1)
ByteMeasurer(8-20)
src/domain/entities/GitTree.js (4)
src/domain/value-objects/GitSha.js (1)
GitSha(11-110)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/entities/GitTreeEntry.js (1)
GitTreeEntry(12-53)src/domain/value-objects/GitObjectType.js (1)
GitObjectType(10-198)
test/domain/value-objects/GitFileMode.test.js (2)
src/domain/value-objects/GitFileMode.js (1)
GitFileMode(11-90)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/infrastructure/adapters/node/NodeShellRunner.js (1)
contract.js (2)
RunnerResultSchema(6-10)RunnerResultSchema(6-10)
src/domain/value-objects/GitFileMode.js (2)
src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/value-objects/GitObjectType.js (1)
GitObjectType(10-198)
src/domain/errors/InvalidArgumentError.js (1)
src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
src/domain/value-objects/GitRef.js (1)
src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/domain/value-objects/GitSha.js (3)
test/domain/entities/GitTree.test.js (1)
sha(9-9)test/domain/entities/GitTreeEntry.test.js (1)
sha(8-8)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
ShellRunner.js (1)
src/infrastructure/factories/ShellRunnerFactory.js (1)
ShellRunnerFactory(12-48)
src/domain/value-objects/GitSignature.js (1)
src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/infrastructure/adapters/deno/DenoShellRunner.js (1)
contract.js (2)
RunnerResultSchema(6-10)RunnerResultSchema(6-10)
test/domain/value-objects/GitObjectType.test.js (2)
src/domain/value-objects/GitObjectType.js (1)
GitObjectType(10-198)src/domain/errors/InvalidGitObjectTypeError.js (1)
InvalidGitObjectTypeError(10-15)
test/GitBlob.test.js (3)
src/domain/entities/GitBlob.js (1)
GitBlob(13-58)src/domain/value-objects/GitSha.js (1)
GitSha(11-110)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)
src/domain/value-objects/GitObjectType.js (1)
src/domain/errors/InvalidGitObjectTypeError.js (1)
InvalidGitObjectTypeError(10-15)
index.js (5)
src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)src/domain/services/CommandSanitizer.js (1)
CommandSanitizer(10-45)contract.js (4)
RunnerOptionsSchema(15-20)RunnerOptionsSchema(15-20)RunnerResultSchema(6-10)RunnerResultSchema(6-10)src/domain/services/GitCommandBuilder.js (1)
GitCommandBuilder(8-61)
🪛 GitHub Actions: CI
scripts/run-multi-runtime-tests.sh
[error] 7-7: docker-compose: not found
[error] 13-13: docker-compose: not found
🪛 LanguageTool
SECURITY.md
[style] ~25-~25: This phrase is redundant. Consider using “outside”.
Context: ...Path Traversal: Unauthorized access outside of the repository's cwd. - ReDoS: Re...
(OUTSIDE_OF)
🔇 Additional comments (45)
NOTICE (1)
1-16: LGTM!The NOTICE file is well-structured with proper copyright attribution and a complete Apache License 2.0 header. This is an appropriate way to document licensing compliance for the project.
eslint.config.js (2)
1-2: LGTM!The imports are correct and necessary for the multi-runtime configuration.
30-40: Test configuration is well-structured.The test-specific configuration properly scopes test globals to test files. Once the duplicate test globals are removed from the main configuration (lines 15-17), this block will correctly serve as the sole source of testing globals.
deno.json (1)
1-6: LGTM!The Deno configuration appropriately grants permissions needed for test execution (reading files, running git commands, environment access, writing test artifacts). The
nodeModulesDir: "auto"enables npm package compatibility.scripts/pre-push (1)
1-7: Pre-push runs Docker-based multi-runtime tests.Since
npm testnow executes./scripts/run-multi-runtime-tests.sh(Docker-based), this hook requires Docker to be running. This is a heavier check than typical pre-push hooks. Consider if this is intentional, or ifnpm run test:localwould be more appropriate for quick local validation.Same shell idiom improvement applies here:
♻️ Optional refactor
#!/bin/sh echo "Running pre-push tests..." -npm test -if [ $? -ne 0 ]; then +if ! npm test; then echo "Tests failed. Push aborted." exit 1 fiDockerfile.deno (1)
2-6: LGTM on the remaining Dockerfile structure.The apt cache cleanup, working directory setup, and command configuration follow Docker best practices.
.dockerignore (1)
1-6: LGTM!The exclusions appropriately reduce Docker context size. The
scriptsdirectory is excluded since the containerized tests use runtime-specific commands (deno task test,bun test, etc.) rather than the shell orchestration scripts.CONTRIBUTING.md (1)
1-37: LGTM on the overall contribution guidelines.The document thoroughly covers development workflow, architectural principles (Hexagonal Architecture, Value Objects, CommandSanitizer), and testing requirements. This aligns well with the PR's multi-runtime strategy and domain-driven design approach.
package.json (2)
10-10: Good DX: Automatic git hooks configuration.The
preparescript automatically setscore.hooksPathtoscriptsonnpm install, ensuring all contributors get the pre-commit and pre-push hooks without manual setup.
17-17: New dependency: ajv for JSON schema validation.The ajv library is a well-established JSON schema validator. Version
^8.17.1(released July 2024) is recent and contains no active security vulnerabilities. The only historical vulnerability (Prototype Pollution in versions < 6.12.3, patched February 2022) does not affect this version.CHANGELOG.md (1)
1-35: LGTM! Well-structured changelog following best practices.The changelog follows the "Keep a Changelog" format and comprehensively documents the architectural changes, security improvements, and bug fixes introduced in this PR. The categorization is clear and will help users understand the scope of changes.
test/deno_shim.js (1)
1-2: Version 0.224.0 is the final release of deno.land/std; no newer version exists on that distribution channel.The Deno standard library moved from deno.land/std to JSR (JavaScript Registry) as of January 2026. Version 0.224.0 is the last snapshot of the deno.land/std distribution. If you want to use newer packages, migrate to JSR-based packages (e.g.,
jsr:@std/fs,jsr:@std/path) instead of updating the version number.Likely an incorrect or invalid review comment.
contract.js (1)
27-29: LGTM!The CommandRunner typedef is well-structured and correctly references the existing RunnerOptions and RunnerResult types. This provides clear type documentation for higher-order command execution functions.
test/deno_entry.js (1)
1-14: LGTM!The Deno test bootstrap file correctly aggregates all test modules for execution in a single Deno process. The import structure is clean and appropriate for Deno's test runner.
Dockerfile.bun (1)
5-5: No action needed —--ignore-scriptsis appropriate here.All dependencies (ajv, zod, eslint, prettier, vitest) are pure JavaScript packages that don't require postinstall scripts. Using
--ignore-scriptsin a containerized CI environment is a security best practice, and the rationale is already documented in the README under "Dockerized CI.".devcontainer/deno/devcontainer.json (1)
1-20: LGTM!The Deno dev container configuration is well-structured with appropriate settings for VS Code integration, correct Deno extensions, and proper build context paths.
test/ShellRunner.test.js (2)
5-13: LGTM!Good coverage of basic command execution with exit code and stdout verification.
15-23: LGTM!Properly tests error handling by verifying non-zero exit code and stderr output for an invalid Git command.
src/domain/errors/ValidationError.js (1)
1-20: LGTM!Clean implementation following the established error hierarchy. Proper forwarding to the base class and appropriate name override for error identification.
SECURITY.md (1)
1-21: LGTM!Comprehensive security policy with clear vulnerability reporting instructions and appropriate acknowledgment timeline. The hardened scope aligns well with the CommandSanitizer and validation work in this PR.
test/domain/entities/GitCommit.test.js (2)
8-14: Good test setup with shared fixtures.Clean setup with reusable fixtures. Using the same signature for both author and committer is fine for tests focused on commit structure rather than signature differences.
43-48: LGTM!Correctly verifies the type() method returns a commit-type GitObjectType.
src/domain/errors/InvalidGitObjectTypeError.js (1)
1-15: LGTM!Clean implementation of a domain-specific error. The error follows the established pattern, properly extends GitPlumbingError, and provides clear messaging for invalid Git object type validation failures.
test/domain/errors/Errors.test.js (1)
1-34: LGTM!Comprehensive test coverage for the error hierarchy. The tests properly verify inheritance, properties, and message formatting for all custom error types.
src/domain/errors/GitPlumbingError.js (1)
1-15: LGTM!Solid foundation for the domain error hierarchy. The base error properly extends Error and provides structured context through the operation and details parameters, which enhances debuggability across the Git plumbing operations.
test/domain/entities/GitTree.test.js (1)
1-55: LGTM!Excellent test coverage for the GitTree entity. The tests properly verify constructor validation, the static factory pattern, immutability semantics (as indicated by the comment on line 40), and type checking behavior.
src/domain/errors/InvalidArgumentError.js (1)
10-20: LGTM!The error class correctly extends
GitPlumbingErrorand follows the established pattern for domain-specific errors in the hierarchy.src/domain/value-objects/GitSignature.js (1)
37-39: LGTM!The
toString()method correctly formats the signature in standard Git format.test/GitSha.test.js (1)
14-131: LGTM!The test suite provides comprehensive coverage of all GitSha functionality including validation, factory methods, equality, short form, empty tree detection, and JSON serialization.
test/GitBlob.test.js (1)
10-88: LGTM!The test suite provides excellent coverage of GitBlob functionality including constructor validation, factory methods, state checking (isWritten), size calculation for various content types, and type identification.
src/domain/entities/GitTreeEntry.js (1)
34-52: LGTM!The delegation pattern for type-related methods is clean and appropriate, leveraging the underlying
GitFileModeandGitObjectTypeimplementations.src/domain/entities/GitCommit.js (1)
44-75: LGTM!The methods
isWritten(),type(),isRoot(), andisMerge()are all correctly implemented and provide useful commit introspection capabilities.src/domain/entities/GitBlob.js (1)
35-58: LGTM!The methods
fromContent(),isWritten(),size(), andtype()are correctly implemented. The static factory method provides a clean way to create blobs from content.src/domain/services/CommandSanitizer.js (1)
24-44: LGTM!The sanitization logic is well-implemented with proper validation:
- Validates the structure (array of strings)
- Uses case-insensitive matching to catch variations
- Correctly handles both
--flagand--flag=valuepatterns- Returns the original (unmodified) args array
This provides solid protection against command injection.
src/infrastructure/adapters/bun/BunShellRunner.js (1)
15-40: LGTM!The Bun shell runner implementation is well-structured:
- Correctly handles both string and Uint8Array input with proper encoding
- Defensively checks for
process.stdinexistence before using it- Always closes stdin to prevent hangs
- Uses Bun's Response API appropriately for stream reading
- Properly awaits the exit code
- Validates the result structure with RunnerResultSchema
The implementation aligns well with the multi-runtime architecture pattern.
src/domain/value-objects/GitFileMode.js (1)
12-43: LGTM!The file mode constants and validation are correct:
- Covers the essential Git file modes (regular, executable, symlink, tree, submodule)
- Simple and effective validation using
isValid()with a whitelist approach- Proper error handling with descriptive ValidationError
src/infrastructure/factories/ShellRunnerFactory.js (3)
21-32: LGTM!The factory pattern is well implemented. The
create()method correctly maps detected environments to their corresponding runner classes, and the lookup is safe since_detectEnvironment()only returns predefined constants.
39-47: LGTM!The detection priority (Bun → Deno → Node fallback) is sensible. Checking for the global
BunandDenoobjects is the standard approach for runtime detection.
5-7: No issues found. The adapters safely reference their respective runtime globals only inside method implementations (run()), not at the top-level. BunShellRunner and DenoShellRunner can be imported without errors in Node.js since the runtime-specific code (Bun.spawn,Deno.Command) is only evaluated when methods are called, not at import time.Likely an incorrect or invalid review comment.
index.js (4)
23-31: LGTM!Good addition of CWD validation. Using synchronous
fs.existsSyncandfs.statSyncin the constructor is acceptable since this is one-time initialization, and async constructors are not idiomatic in JavaScript.
57-85: LGTM!The error handling is well-structured:
CommandSanitizer.sanitize()is called before execution to block prohibited flags- Non-zero exit codes are wrapped in
GitPlumbingErrorwith full context- Non-
GitPlumbingErrorexceptions are wrapped to maintain consistent error types
130-133: Breaking change:revParsenow throws instead of returning null.Per the AI summary,
revParsepreviously returnednullon failure. It now throwsGitPlumbingError. This is a breaking API change for callers that relied on null-checking.Consider documenting this in migration notes or providing a
revParseOrNullvariant for backward compatibility if needed.
142-149: LGTM!The refactor to use
GitCommandBuilderimproves readability and consistency. The fluent builder pattern cleanly constructs the update-ref arguments.src/domain/value-objects/GitObjectType.js (2)
58-61:isValidmay pass for string values due to theSTRING_TO_INTbug.Because of the bug above,
Object.values(STRING_TO_INT)includes the strings'ofs-delta'and'ref-delta'. The type check on line 59 prevents strings from passing, so this won't cause false positives, but fixingSTRING_TO_INTis still necessary.
119-165: LGTM!The factory methods (
blob(),tree(),commit(),tag(),ofsDelta(),refDelta()) and type inspection methods are well implemented and provide a clean API.
…rmance - Implement universal GitStream wrapper for unified Node/Web stream handling - Refactor CommandSanitizer to use strict allow-list for base plumbing commands - Add executeStream method to GitPlumbing for memory-efficient large object handling - Implement GitTreeBuilder to resolve O(N^2) complexity in tree construction - Enhance GitBlob with defensive copying for Uint8Array content - Cache TextEncoder/Decoder instances to reduce garbage collection pressure - Add configurable timeouts to all shell runners - Professionalize package.json with exports and engines enforcement
- Introduced exitPromise and GitStream.finished for tracking process status. - Hardened CommandSanitizer with strict git command list and resource limits. - Fixed async race conditions in test suite. - Improved multi-runtime streaming compatibility.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test.js (1)
11-18: Missingawaitfor asyncexecutecalls inbeforeEach.According to the AI summary,
executenow returns aPromise<string>. The setup calls on lines 15-17 are not awaited, which means the Git repository initialization may not complete before tests run, causing race conditions or flaky tests.🐛 Proposed fix
- beforeEach(() => { + beforeEach(async () => { tempDir = mkdtempSync(path.join(os.tmpdir(), 'git-plumbing-test-')); plumbing = new GitPlumbing({ cwd: tempDir }); // Initialize a repo for testing - plumbing.execute({ args: ['init'] }); - plumbing.execute({ args: ['config', 'user.name', 'Tester'] }); - plumbing.execute({ args: ['config', 'user.email', 'test@example.com'] }); + await plumbing.execute({ args: ['init'] }); + await plumbing.execute({ args: ['config', 'user.name', 'Tester'] }); + await plumbing.execute({ args: ['config', 'user.email', 'test@example.com'] }); });
🤖 Fix all issues with AI agents
In @index.js:
- Around line 21-34: The constructor currently performs Node-only filesystem
checks using fs.existsSync and fs.statSync (in GitPlumbing.constructor,
resolving cwd into resolvedCwd), which breaks Bun/Deno imports; remove direct fs
usage from the constructor and instead either (a) inject or import an abstract
filesystem validator (e.g., a FSAdapter with methods like exists and
isDirectory) and call that here, or (b) defer validation by storing the resolved
path but moving the fs checks into a separate validateCwdForNode() method that
is invoked only in Node-specific runtime adapters; ensure runner and cwd
handling remains unchanged and reference the resolvedCwd property in whichever
new validation path you choose.
In @package.json:
- Line 31: Remove the unused "ajv" dependency from package.json and update the
lockfile by running your package manager (npm/yarn/pnpm install) so the
dependency is actually removed; also search the repo for any "ajv"
imports/usages to confirm none remain and run the test suite/build to verify
nothing breaks.
- Around line 16-20: The engines block in package.json uses outdated runtime
constraints; update the "node" field from ">=20.0.0" to a current minimum
(recommend ">=24.0.0" or at least ">=22.0.0") and bump the "deno" field from
">=1.40.0" to ">=2.0.0", while leaving "bun" as ">=1.0.0" (or update to a more
recent minimum like ">=1.3.5" if you want to reflect current stable); modify the
"node", "deno", and optionally "bun" entries in the engines object accordingly
to reflect these new minimum versions.
In @src/domain/entities/GitBlob.js:
- Line 23: In the GitBlob constructor validate the incoming content type before
doing the defensive copy: check if content is either a string (typeof content
=== 'string') or a Uint8Array (content instanceof Uint8Array) and throw a
TypeError for any other type, then perform the defensive copy only for the
Uint8Array case (this._content = new Uint8Array(content)) and assign strings
directly; update the assignment at the line that currently sets this._content to
perform this validated-and-then-copied logic.
- Around line 18-24: The GitBlob constructor currently validates sha but allows
null/undefined or invalid content which breaks ByteMeasurer.measure() when
size() is called; update GitBlob.constructor to validate the content parameter
(reject null/undefined and types other than acceptable binary types), throwing
InvalidArgumentError with context ('GitBlob.constructor', { content }) if
invalid, and normalize valid inputs into this._content (e.g., copy Uint8Array or
accept Buffer/string as appropriate to the codebase) so ByteMeasurer.measure()
and size() always receive a proper binary buffer.
In @src/domain/services/ByteMeasurer.js:
- Around line 16-21: The measure method in ByteMeasurer assumes non-string
inputs have a .length and will throw or return wrong values for
null/undefined/unsupported types; update ByteMeasurer.measure to validate the
input: if content is a string use ENCODER.encode(content).length; else if
content is an ArrayBuffer use content.byteLength; else if content is a
TypedArray (e.g., Uint8Array) use content.length; else if content is an
object/array-like with a finite integer length use that length; otherwise throw
a TypeError with a clear message (e.g., "Unsupported content type for
ByteMeasurer.measure") to fail fast. Ensure checks guard against null/undefined
before accessing properties.
In @src/domain/services/CommandSanitizer.js:
- Around line 68-72: The code calls args[0].toLowerCase() before validating
argument types, causing a TypeError for non-string inputs; in
CommandSanitizer.sanitize, first check that args[0] is a string (e.g., typeof
args[0] === 'string') and throw the existing ValidationError with context (same
message and meta) if it is not, then call .toLowerCase() and proceed to check
this.ALLOWED_COMMANDS; this ensures type validation happens prior to invoking
string methods.
In @src/domain/value-objects/GitRef.js:
- Around line 35-50: The validate method in the GitRef value object misses an
explicit empty-string check so "" can slip through; update static validate(ref)
to first reject empty strings (e.g., if (ref === '' ) return { valid: false,
reason: 'Reference cannot be empty' }) before calling _hasValidStructure,
_hasNoProhibitedChars, and _isNotReserved so empty refs return a clear invalid
result.
- Around line 101-106: The current _isNotReserved(ref) incorrectly rejects any
ref containing '@'; change it to only reject a bare '@' and the '@{' sequence
per git-check-ref-format: in GitRef._isNotReserved check if ref === '@' and
return { valid: false, reason: "Reference cannot be '@'" }, then check if
ref.includes('@{') and return { valid: false, reason: "Reference cannot contain
'@{', the reflog syntax" }, otherwise return { valid: true }.
In @src/infrastructure/adapters/deno/DenoShellRunner.js:
- Around line 30-36: The write to the stdin writer is not awaited which can race
with writer.close; change the logic in the block that obtains
child.stdin.getWriter() so you await writer.write(...) before awaiting
writer.close(), i.e. ensure the writer returned by child.stdin.getWriter() is
used to await writer.write(...) (for both string and Uint8Array paths) and only
then call await writer.close() to prevent the stream closing before the write
completes; references: child.stdin, getWriter(), writer.write(), writer.close().
- Around line 87-93: The non-streaming write path for child.stdin has a race:
call and await the promise returned by writer.write(...) before calling await
writer.close(); specifically, in the block that creates const writer =
child.stdin.getWriter(), change writer.write(typeof input === 'string' ?
ENCODER.encode(input) : input) to await writer.write(... ) so the write
completes before awaiting writer.close(); keep the existing branch that closes
child.stdin when no input.
In @src/infrastructure/GitStream.js:
- Around line 23-74: The polyfill returned by getReader() doesn’t enforce the
Web Streams locking semantics and allows concurrent read() calls; modify
getReader() to introduce a readInProgress (or locked) flag checked at the start
of the read() method to throw or reject if another read is active, set the flag
when a read starts and clear it in all cleanup paths (onData, onEnd, onError,
and when chunk is synchronously consumed), and ensure releaseLock() clears the
flag and removes any listeners so subsequent calls can acquire the reader;
reference getReader(), the returned read() function, and releaseLock() when
implementing these changes.
In @test.js:
- Around line 44-50: The test "handles errors with telemetry" silently passes if
no error is thrown because it uses a try/catch without asserting failure on
success; update the test for the plumbing.execute invocation to assert rejection
instead (e.g., use await expect(plumbing.execute({ args: [...]
})).rejects.toThrow(/Git command failed/) or, if keeping the try/catch, add a
fail()/throw after the await to ensure the test fails when execute does not
throw), and ensure the assertion checks err.message contains "Git command
failed".
🧹 Nitpick comments (3)
index.js (1)
155-157: Consider using GitSha.EMPTY_TREE_VALUE for consistency.The
emptyTreegetter duplicates the constant value that's already defined inGitSha.EMPTY_TREE_VALUE. Consider refactoring to use the value object constant for better maintainability and consistency with the domain model.♻️ Proposed refactor
/** * Returns the SHA-1 of the empty tree. * @returns {string} */ get emptyTree() { - return '4b825dc642cb6eb9a060e54bf8d69288fbee4904'; + return GitSha.EMPTY_TREE_VALUE; }test/domain/entities/GitCommitBuilder.test.js (1)
8-38: LGTM! Clear and focused tests.The tests provide good coverage of the builder's happy path, verifying proper GitCommit construction and parent SHA handling with automatic GitSha conversion.
Consider adding edge case tests for error handling, such as:
- Building without required fields (tree, author, committer)
- Passing invalid types to builder methods
- Handling of multiple parents
These tests would complement the existing coverage and verify the builder's validation logic documented in
src/domain/entities/GitCommitBuilder.js.test/Streaming.test.js (1)
1-46: LGTM! Solid streaming functionality tests.The tests properly verify stream creation, chunk accumulation, and input piping. The handling of both string and
Uint8Arraychunks (lines 19, 39) ensures compatibility across different runtime implementations.Consider adding a test case for streaming error handling, such as:
- A command that fails mid-stream
- Verification that
stream.finishedproperly rejects or resolves with error detailsThis would complement the coverage in
test/StreamCompletion.test.jsand ensure error scenarios are well-tested.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
CHANGELOG.mddocker-compose.ymlindex.jspackage.jsonsrc/domain/entities/GitBlob.jssrc/domain/entities/GitCommit.jssrc/domain/entities/GitCommitBuilder.jssrc/domain/entities/GitTree.jssrc/domain/entities/GitTreeBuilder.jssrc/domain/services/ByteMeasurer.jssrc/domain/services/CommandSanitizer.jssrc/domain/value-objects/GitRef.jssrc/infrastructure/GitStream.jssrc/infrastructure/adapters/bun/BunShellRunner.jssrc/infrastructure/adapters/deno/DenoShellRunner.jssrc/infrastructure/adapters/node/NodeShellRunner.jssrc/infrastructure/factories/ShellRunnerFactory.jssrc/ports/CommandRunnerPort.jstest.jstest/GitBlob.test.jstest/ShellRunner.test.jstest/StreamCompletion.test.jstest/Streaming.test.jstest/domain/entities/GitCommit.test.jstest/domain/entities/GitCommitBuilder.test.jstest/domain/entities/GitTree.test.jstest/domain/entities/GitTreeBuilder.test.js
✅ Files skipped from review due to trivial changes (1)
- test/domain/entities/GitTreeBuilder.test.js
🚧 Files skipped from review as they are similar to previous changes (7)
- test/GitBlob.test.js
- CHANGELOG.md
- src/infrastructure/factories/ShellRunnerFactory.js
- test/domain/entities/GitTree.test.js
- docker-compose.yml
- src/domain/entities/GitCommit.js
- src/domain/entities/GitTree.js
🧰 Additional context used
🧬 Code graph analysis (14)
test/Streaming.test.js (2)
index.js (1)
GitPlumbing(15-199)ShellRunner.js (1)
ShellRunner(11-25)
test/ShellRunner.test.js (1)
ShellRunner.js (1)
ShellRunner(11-25)
src/domain/services/ByteMeasurer.js (1)
src/infrastructure/adapters/deno/DenoShellRunner.js (1)
ENCODER(7-7)
src/domain/services/CommandSanitizer.js (1)
src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/domain/value-objects/GitRef.js (1)
src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
test/StreamCompletion.test.js (1)
test/Streaming.test.js (1)
git(5-8)
test/domain/entities/GitCommitBuilder.test.js (6)
test/domain/entities/GitCommit.test.js (3)
tree(9-9)author(11-11)message(13-13)src/domain/entities/GitTree.js (1)
GitTree(13-69)src/domain/value-objects/GitSignature.js (1)
GitSignature(10-40)src/domain/entities/GitCommitBuilder.js (1)
GitCommitBuilder(14-135)src/domain/entities/GitCommit.js (1)
GitCommit(14-76)src/domain/value-objects/GitSha.js (1)
GitSha(11-110)
src/domain/entities/GitTreeBuilder.js (5)
src/domain/entities/GitTreeEntry.js (1)
GitTreeEntry(12-53)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/value-objects/GitSha.js (1)
GitSha(11-110)src/domain/value-objects/GitFileMode.js (1)
GitFileMode(11-90)src/domain/entities/GitTree.js (1)
GitTree(13-69)
src/domain/entities/GitBlob.js (4)
src/domain/value-objects/GitSha.js (1)
GitSha(11-110)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/services/ByteMeasurer.js (1)
ByteMeasurer(10-22)src/domain/value-objects/GitObjectType.js (1)
GitObjectType(10-198)
index.js (7)
src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)src/domain/services/CommandSanitizer.js (1)
CommandSanitizer(10-101)src/ports/CommandRunnerPort.js (4)
RunnerOptionsSchema(17-24)RunnerOptionsSchema(17-24)RunnerResultSchema(6-12)RunnerResultSchema(6-12)src/infrastructure/GitStream.js (1)
GitStream(9-100)src/domain/services/GitCommandBuilder.js (1)
GitCommandBuilder(8-61)src/domain/value-objects/GitSha.js (1)
GitSha(11-110)
src/infrastructure/adapters/bun/BunShellRunner.js (1)
src/ports/CommandRunnerPort.js (2)
RunnerResultSchema(6-12)RunnerResultSchema(6-12)
src/infrastructure/adapters/deno/DenoShellRunner.js (2)
src/domain/services/ByteMeasurer.js (1)
ENCODER(5-5)src/ports/CommandRunnerPort.js (2)
RunnerResultSchema(6-12)RunnerResultSchema(6-12)
src/domain/entities/GitCommitBuilder.js (5)
src/domain/value-objects/GitSha.js (1)
GitSha(11-110)src/domain/entities/GitTree.js (1)
GitTree(13-69)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/value-objects/GitSignature.js (1)
GitSignature(10-40)src/domain/entities/GitCommit.js (1)
GitCommit(14-76)
src/infrastructure/adapters/node/NodeShellRunner.js (1)
src/ports/CommandRunnerPort.js (2)
RunnerResultSchema(6-12)RunnerResultSchema(6-12)
🔇 Additional comments (32)
src/domain/services/CommandSanitizer.js (2)
15-32: LGTM: Comprehensive allow-list of Git commands.The ALLOWED_COMMANDS list includes essential plumbing commands while excluding potentially dangerous operations. Including
--versionandhelpis a nice touch for diagnostics.
34-48: Strong security posture with prohibited flags.The PROHIBITED_FLAGS list effectively blocks configuration overrides (
--config,-c), path manipulation (--git-dir,--work-tree), and remote execution vectors (--upload-pack,--receive-pack,--ext-cmd). This aligns well with the PR's goal of hardened security.src/domain/entities/GitTreeBuilder.js (1)
14-53: LGTM! Clean fluent builder implementation.The GitTreeBuilder provides an efficient O(N) builder pattern with proper validation, type safety through value objects, and a convenient API for both direct GitTreeEntry instances and primitive values. The defensive copy in
build()ensures immutability of the constructed GitTree.index.js (5)
36-49: LGTM! Proper Git binary verification.The
verifyInstallation()method provides a clean way to verify Git availability with appropriate error wrapping and context.
59-87: LGTM! Enhanced security and error handling.The addition of
CommandSanitizer.sanitize(args)provides critical protection against command injection, and the structured error handling withGitPlumbingErrorimproves debuggability by preserving operation context and details.
89-121: LGTM! Well-designed streaming support.The
executeStream()method provides memory-efficient streaming with proper error handling and a universalGitStreamabstraction that works across runtimes. TheexitPromisepattern enables reliable stream completion detection.
129-149: LGTM! Consistent command sanitization.The addition of
CommandSanitizer.sanitize(args)maintains security consistency across all execution paths, and the error wrapping aligns with the established error handling pattern.
166-198: LGTM! Clean refactoring with GitCommandBuilder.The use of
GitCommandBuilderimproves readability and maintainability. The conversion of SHA parameters toGitShaobjects provides automatic input validation, withValidationError(which extendsGitPlumbingError) thrown for invalid SHAs—the intended design pattern used throughout the codebase.test/StreamCompletion.test.js (1)
1-34: LGTM! Comprehensive stream completion tests.The tests properly verify the
finishedpromise behavior in both success and failure scenarios, ensuring that exit codes and stderr are correctly propagated. The pattern of consuming the stream before checking the completion promise (lines 15-16, 27-28) correctly mirrors real-world usage.test/ShellRunner.test.js (3)
4-12: LGTM!Clean integration test that validates basic command execution through the ShellRunner abstraction.
14-23: LGTM!Proper validation of stderr capture and non-zero exit codes for failed commands.
25-35: LGTM!Correctly validates stdin handling and SHA-1 computation for blob objects.
test/domain/entities/GitCommit.test.js (3)
8-13: LGTM!Clean test setup with appropriate fixtures for GitCommit validation.
15-41: LGTM!Comprehensive constructor validation covering root commits, single-parent commits, merge commits, and error cases.
43-48: LGTM!Validates GitCommit type identification correctly.
src/infrastructure/GitStream.js (2)
14-17: LGTM!Constructor correctly stores stream and exitPromise with sensible default.
79-99: LGTM!Async iterator correctly delegates to native implementation when available and provides proper fallback with cleanup.
src/ports/CommandRunnerPort.js (3)
6-12: LGTM!Schema appropriately models both streaming and non-streaming result modes with sensible defaults.
17-24: LGTM!Options schema provides comprehensive configuration with appropriate defaults. The increased timeout (120s) is well-documented for Docker CI compatibility.
26-33: LGTM!Type definitions correctly leverage Zod schema inference for consistency.
package.json (2)
7-15: LGTM!Well-structured exports map provides granular access to core functionality and domain primitives, enabling efficient tree-shaking.
23-24: LGTM!
test:localprovides convenient local testing, and thepreparescript safely configures git hooks with appropriate guards.test.js (3)
24-27: LGTM!The test correctly uses async/await with the updated promise-based
executeAPI.
33-42: LGTM!The test properly awaits all async operations:
execute,updateRef, andrevParse.
52-55: LGTM!The test correctly awaits
executeWithStatusand validates the non-zero exit status.src/infrastructure/adapters/bun/BunShellRunner.js (2)
23-59: LGTM!The streaming mode implementation correctly handles the race between process completion and timeout. The
timeoutPromiseonly activates when a timeout is explicitly set, and theclearTimeoutis properly guarded.
61-96: LGTM!The non-streaming mode correctly uses
try/finallyto ensure timeout cleanup, and efficiently parallelizes stdout/stderr reading withPromise.all.src/infrastructure/adapters/node/NodeShellRunner.js (2)
48-57: LGTM!
spawndefaults to'pipe'for stdio, sochild.stdinandchild.stdoutwill be available for streaming operations.
23-41: No changes needed. The review comment is based on an incorrect assumption about Node.jsexecFilebehavior. By default,execFilecreates pipes for stdin, stdout, and stderr (equivalent tostdio: ['pipe', 'pipe', 'pipe']). Thechild.stdinproperty is available and writable without explicit configuration. This is confirmed by the passing test intest/ShellRunner.test.jswhich successfully sends input togit hash-object --stdinand receives the correct SHA1 output.Likely an incorrect or invalid review comment.
src/domain/entities/GitCommitBuilder.js (3)
14-22: LGTM!The constructor properly initializes all builder state with sensible defaults.
29-109: LGTM!The setter methods implement proper validation with clear error messages and follow the fluent builder pattern correctly. Type coercion for SHA strings and validation via
instanceofchecks are consistent.
115-134: LGTM!The
build()method properly validates required fields and constructs theGitCommitwith all builder state. Allowing empty messages and empty parents (for root commits) aligns with Git semantics.
…ive zod validation
- Refactored shell runners to a single "Streaming Only" architecture across Node, Bun, and Deno.
- Centralized all domain validation into Zod schemas in `src/domain/schemas`.
- Enforced strict Hexagonal Architecture with improved Port/Adapter separation.
- Implemented OOM protection via `GitStream.collect({ maxBytes })`.
- Standardized object serialization with `toJSON()` across all entities.
- Eliminated magic numbers and strings by centralizing constants in the ports layer.
- Ensured 100% JSDoc coverage and "one class per file" adherence.
- Added `GitPlumbing.createDefault()` for streamlined runtime-aware instantiation.
- Verified all 114 tests pass across all environments.
- Removed explicit `vitest` imports from test files to prevent Deno from crashing during pre-push tests. - Reverted to using global `describe`, `it`, and `expect` as intended for the multi-runtime test shim.
- Updated README.md with v2.0.0 features and unified streaming usage. - Added ARCHITECTURE.md detailing Hexagonal Design and Zod validation strategy. - Refined JSDoc across domain services and infrastructure. - Ensured all tests and linting pass with latest documentation changes.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/domain/entities/GitCommit.js:
- Around line 24-32: The constructor in GitCommit currently calls
parents.map(...) which throws if parents is null/undefined; update the GitCommit
constructor to guard/coerce parents before mapping (e.g., treat non-array/null
as an empty array or use Array.isArray(parents) ? parents : []) so you only call
.map on an array, then continue converting items via GitSha logic; keep
conversions for sha/treeSha and author/committer as-is and let the subsequent
schema validation run to surface ValidationError.
In @src/domain/schemas/GitRefSchema.js:
- Line 16: The current refine on the Git ref schema incorrectly rejects any ref
containing '@'; change the predicate so it only rejects the literal
single-character ref '@' and any occurrence of the reflog sequence '@{' (i.e.,
use something like .refine(val => val !== '@' && !val.includes('@{'), "Cannot be
'@' or contain '@{') to allow valid refs like 'refs/heads/feature@work' while
still blocking invalid '@' and '@{...}' forms; update the error message
accordingly on the same .refine call in GitRefSchema.js.
In @src/infrastructure/adapters/node/NodeShellRunner.js:
- Around line 36-51: The timeout handler currently calls setTimeout
unconditionally which treats undefined as 0ms and kills the child immediately;
update the exitPromise logic in NodeShellRunner.js (the Promise, timeoutId,
child.kill() usage and the child.on('exit')/child.on('error') handlers) to only
set a timeout when timeout is a finite positive number (e.g., typeof timeout ===
'number' && timeout > 0), store timeoutId only in that case, and ensure you
clearTimeout(timeoutId) conditionally (if it was created) so the child is not
killed immediately when timeout is undefined.
🧹 Nitpick comments (7)
test/StreamCompletion.test.js (1)
22-33: Consider the error message assertion.The assertion
expect(result.stderr).toContain('unknown option')could be fragile if Git changes its error message format across versions. If stability is a concern, consider asserting only on the non-zero exit code, or use a regex pattern that's more resilient to minor wording changes.ARCHITECTURE.md (1)
34-34: Optional: Minor wording simplification."exactly the same way" could be shortened to "identically" for conciseness.
📝 Suggested change
-- **Consistency**: Every runner behaves exactly the same way. +- **Consistency**: Every runner behaves identically.test/domain/entities/GitCommit.test.js (1)
1-6: Minor: Leading blank line.Line 1 has an unnecessary leading blank line. This is a minor style nit.
♻️ Suggested fix
- import GitCommit from '../../../src/domain/entities/GitCommit.js';src/ports/RunnerResultSchema.js (1)
6-9: Consider cross-runtime Promise validation.
z.instanceof(Promise)may fail in edge cases across runtime boundaries (e.g., different realms or polyfills). For a multi-runtime library targeting Node/Bun/Deno, consider a duck-type check instead.♻️ More resilient Promise validation
export const RunnerResultSchema = z.object({ stdoutStream: z.any(), // ReadableStream (Web) or Readable (Node) - exitPromise: z.instanceof(Promise), // Resolves to {code, stderr} when process ends + exitPromise: z.custom((val) => + val && typeof val.then === 'function', + { message: 'Expected a Promise-like object' } + ), // Resolves to {code, stderr} when process ends });src/domain/schemas/GitSignatureSchema.js (1)
6-10: Consider: Strict email validation may reject existing Git signatures.Git itself stores emails in
<email>format but doesn't strictly validate RFC 5322 compliance. Signatures likeuser@localhostor evenuser(no domain) can exist in repositories. Zod's.email()enforces stricter validation that may reject valid historical commits when parsing.If this library needs to parse existing repositories, consider relaxing the email validation. If it's only for creating new signatures, the current strictness is acceptable.
♻️ Relaxed email validation option
export const GitSignatureSchema = z.object({ name: z.string().min(1), - email: z.string().email(), + email: z.string().min(1), // Git doesn't enforce RFC 5322 email format timestamp: z.number().int().nonnegative().default(() => Math.floor(Date.now() / 1000)) });src/ports/RunnerOptionsSchema.js (1)
21-27: Minor: Redundant.optional()before.default().On line 26,
.optional().default(...)is redundant. Zod's.default()already handles the undefined case and provides the default value. The.optional()is unnecessary.♻️ Cleaner schema definition
export const RunnerOptionsSchema = z.object({ command: z.string(), args: z.array(z.string()), cwd: z.string().optional(), input: z.union([z.string(), z.instanceof(Uint8Array)]).optional(), - timeout: z.number().optional().default(DEFAULT_COMMAND_TIMEOUT), + timeout: z.number().default(DEFAULT_COMMAND_TIMEOUT), });src/domain/entities/GitBlob.js (1)
67-69: Minor:size()creates unnecessary defensive copy.
this.contentreturns a defensive copy, butByteMeasurer.measure()only reads the data. Usingthis._contentdirectly would avoid the extra allocation.♻️ Suggested optimization
size() { - return ByteMeasurer.measure(this.content); + return ByteMeasurer.measure(this._content); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
ARCHITECTURE.mdCHANGELOG.mdREADME.mdShellRunner.jseslint.config.jsindex.jssrc/domain/entities/GitBlob.jssrc/domain/entities/GitCommit.jssrc/domain/entities/GitCommitBuilder.jssrc/domain/entities/GitTree.jssrc/domain/entities/GitTreeBuilder.jssrc/domain/entities/GitTreeEntry.jssrc/domain/schemas/GitBlobSchema.jssrc/domain/schemas/GitCommitSchema.jssrc/domain/schemas/GitFileModeSchema.jssrc/domain/schemas/GitObjectTypeSchema.jssrc/domain/schemas/GitRefSchema.jssrc/domain/schemas/GitShaSchema.jssrc/domain/schemas/GitSignatureSchema.jssrc/domain/schemas/GitTreeEntrySchema.jssrc/domain/schemas/GitTreeSchema.jssrc/domain/services/CommandSanitizer.jssrc/domain/services/GitCommandBuilder.jssrc/domain/value-objects/GitRef.jssrc/domain/value-objects/GitSha.jssrc/domain/value-objects/GitSignature.jssrc/infrastructure/GitStream.jssrc/infrastructure/adapters/bun/BunShellRunner.jssrc/infrastructure/adapters/deno/DenoShellRunner.jssrc/infrastructure/adapters/node/NodeShellRunner.jssrc/infrastructure/factories/ShellRunnerFactory.jssrc/ports/CommandRunnerPort.jssrc/ports/RunnerOptionsSchema.jssrc/ports/RunnerResultSchema.jstest.jstest/GitBlob.test.jstest/ShellRunner.test.jstest/StreamCompletion.test.jstest/domain/entities/GitCommit.test.jstest/domain/entities/GitCommitBuilder.test.jstest/domain/entities/GitTree.test.jstest/domain/entities/GitTreeEntry.test.js
🚧 Files skipped from review as they are similar to previous changes (11)
- test/domain/entities/GitTreeEntry.test.js
- test/GitBlob.test.js
- src/domain/value-objects/GitSignature.js
- src/infrastructure/factories/ShellRunnerFactory.js
- src/domain/services/GitCommandBuilder.js
- test/domain/entities/GitTree.test.js
- test.js
- src/domain/value-objects/GitSha.js
- src/domain/entities/GitCommitBuilder.js
- src/domain/entities/GitTree.js
- src/domain/services/CommandSanitizer.js
🧰 Additional context used
🧬 Code graph analysis (19)
src/domain/schemas/GitTreeEntrySchema.js (2)
src/domain/schemas/GitFileModeSchema.js (2)
GitFileModeSchema(6-12)GitFileModeSchema(6-12)src/domain/schemas/GitShaSchema.js (2)
GitShaSchema(6-9)GitShaSchema(6-9)
test/domain/entities/GitCommit.test.js (4)
src/domain/value-objects/GitSha.js (1)
GitSha(12-109)src/domain/value-objects/GitSignature.js (1)
GitSignature(11-52)src/domain/entities/GitCommit.js (1)
GitCommit(14-97)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
test/StreamCompletion.test.js (3)
test/Streaming.test.js (1)
git(5-8)index.js (1)
GitPlumbing(20-199)ShellRunner.js (1)
ShellRunner(12-29)
src/domain/schemas/GitTreeSchema.js (2)
src/domain/schemas/GitShaSchema.js (2)
GitShaSchema(6-9)GitShaSchema(6-9)src/domain/schemas/GitTreeEntrySchema.js (2)
GitTreeEntrySchema(8-12)GitTreeEntrySchema(8-12)
src/domain/schemas/GitBlobSchema.js (1)
src/domain/schemas/GitShaSchema.js (2)
GitShaSchema(6-9)GitShaSchema(6-9)
test/domain/entities/GitCommitBuilder.test.js (5)
src/domain/entities/GitTree.js (1)
GitTree(14-98)src/domain/value-objects/GitSignature.js (1)
GitSignature(11-52)src/domain/entities/GitCommitBuilder.js (1)
GitCommitBuilder(13-119)src/domain/entities/GitCommit.js (1)
GitCommit(14-97)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)
src/infrastructure/adapters/node/NodeShellRunner.js (2)
src/ports/RunnerOptionsSchema.js (2)
DEFAULT_MAX_STDERR_SIZE(16-16)DEFAULT_MAX_STDERR_SIZE(16-16)src/ports/RunnerResultSchema.js (2)
RunnerResultSchema(6-9)RunnerResultSchema(6-9)
src/domain/entities/GitCommit.js (5)
src/domain/value-objects/GitSha.js (1)
GitSha(12-109)src/domain/value-objects/GitSignature.js (1)
GitSignature(11-52)src/domain/schemas/GitCommitSchema.js (2)
GitCommitSchema(8-15)GitCommitSchema(8-15)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/value-objects/GitObjectType.js (1)
GitObjectType(10-198)
src/domain/entities/GitTreeBuilder.js (3)
src/domain/entities/GitTreeEntry.js (1)
GitTreeEntry(13-75)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/entities/GitTree.js (1)
GitTree(14-98)
ShellRunner.js (2)
src/infrastructure/factories/ShellRunnerFactory.js (1)
ShellRunnerFactory(12-49)src/ports/RunnerOptionsSchema.js (2)
DEFAULT_COMMAND_TIMEOUT(6-6)DEFAULT_COMMAND_TIMEOUT(6-6)
src/domain/entities/GitTreeEntry.js (6)
test/domain/entities/GitTree.test.js (1)
sha(9-9)test/domain/entities/GitTreeEntry.test.js (1)
sha(8-8)src/domain/value-objects/GitFileMode.js (1)
GitFileMode(11-90)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)src/domain/schemas/GitTreeEntrySchema.js (2)
GitTreeEntrySchema(8-12)GitTreeEntrySchema(8-12)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/domain/entities/GitBlob.js (8)
test/domain/entities/GitTree.test.js (1)
sha(9-9)test/domain/entities/GitTreeEntry.test.js (1)
sha(8-8)test/domain/entities/GitTreeBuilder.test.js (1)
sha(7-7)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)src/domain/schemas/GitBlobSchema.js (2)
GitBlobSchema(7-10)GitBlobSchema(7-10)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/services/ByteMeasurer.js (1)
ByteMeasurer(10-22)src/domain/value-objects/GitObjectType.js (1)
GitObjectType(10-198)
src/infrastructure/adapters/bun/BunShellRunner.js (1)
src/ports/RunnerResultSchema.js (2)
RunnerResultSchema(6-9)RunnerResultSchema(6-9)
src/infrastructure/adapters/deno/DenoShellRunner.js (2)
src/domain/services/ByteMeasurer.js (1)
ENCODER(5-5)src/ports/RunnerResultSchema.js (2)
RunnerResultSchema(6-9)RunnerResultSchema(6-9)
src/domain/schemas/GitCommitSchema.js (2)
src/domain/schemas/GitShaSchema.js (2)
GitShaSchema(6-9)GitShaSchema(6-9)src/domain/schemas/GitSignatureSchema.js (2)
GitSignatureSchema(6-10)GitSignatureSchema(6-10)
src/domain/value-objects/GitRef.js (2)
src/domain/schemas/GitRefSchema.js (2)
GitRefSchema(7-26)GitRefSchema(7-26)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
index.js (8)
src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/infrastructure/factories/ShellRunnerFactory.js (1)
ShellRunnerFactory(12-49)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)src/ports/RunnerOptionsSchema.js (4)
DEFAULT_MAX_BUFFER_SIZE(11-11)DEFAULT_MAX_BUFFER_SIZE(11-11)RunnerOptionsSchema(21-27)RunnerOptionsSchema(21-27)src/domain/services/CommandSanitizer.js (1)
CommandSanitizer(10-102)src/infrastructure/GitStream.js (1)
GitStream(11-113)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)src/domain/services/GitCommandBuilder.js (1)
GitCommandBuilder(8-61)
src/infrastructure/GitStream.js (1)
src/ports/RunnerOptionsSchema.js (2)
DEFAULT_MAX_BUFFER_SIZE(11-11)DEFAULT_MAX_BUFFER_SIZE(11-11)
test/ShellRunner.test.js (1)
ShellRunner.js (1)
ShellRunner(12-29)
🪛 LanguageTool
ARCHITECTURE.md
[style] ~34-~34: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...- Consistency: Every runner behaves exactly the same way. - Memory Safety: Large outputs...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
[style] ~47-~47: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...vironment (via Docker Compose) runs the exact same test suite in Bun and Deno simultaneous...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
🔇 Additional comments (46)
src/domain/schemas/GitObjectTypeSchema.js (1)
1-30: LGTM! Clean and correct Git object type schema implementation.The Git object type mappings are accurate according to Git's specification:
- Standard types (blob, tree, commit, tag) and delta types (ofs-delta, ref-delta) are correctly defined
- Integer mappings (1, 2, 3, 4, 6, 7) align with Git's internal representation
- Type 5 is correctly omitted (reserved/unused in Git)
The dual schema approach (string enum + integer union) provides flexibility for validation across different contexts, and the inline comments in
GitObjectTypeIntsaid maintainability.src/domain/schemas/GitTreeEntrySchema.js (1)
1-12: LGTM! Well-structured Zod schema.The schema composition is clean and leverages existing domain schemas effectively. The
pathvalidation with.min(1)ensures no empty paths can be created, which aligns with Git tree entry requirements.src/domain/schemas/GitFileModeSchema.js (1)
6-12: LGTM! Accurate Git file mode enum.The enum correctly captures all standard Git file modes. The inline comments enhance readability and serve as helpful documentation.
src/domain/entities/GitTreeEntry.js (2)
19-38: LGTM! Constructor properly validates all inputs via schema.The constructor design is solid:
- Accepts both primitives and value objects for flexibility
- Normalizes inputs for validation via
GitTreeEntrySchema- The schema validates
pathwithz.string().min(1), which addresses the past review concern about path validation- Reconstructs value objects from validated data for type safety
44-74: LGTM! Clean delegation pattern and proper serialization.The methods follow good object-oriented practices by delegating to value objects. The
toJSON()method provides clean serialization by converting value objects to strings.CHANGELOG.md (1)
1-42: LGTM! Comprehensive and well-structured changelog.The changelog follows Keep a Changelog format and documents the architectural evolution clearly. The major version bump (2.0.0) is appropriate given the breaking changes to hexagonal architecture and the elimination of magic numbers. The security hardening and performance improvements are well-documented.
src/domain/entities/GitTreeBuilder.js (1)
13-48: LGTM! Well-implemented builder pattern.The builder implementation is clean and follows best practices:
- Fluent API with method chaining via
return this- Type validation in
addEntryensures only valid entries are added- Convenience method
addreduces boilerplate without duplicating logicbuild()creates an immutable snapshot by spreading the entries arrayThe pattern effectively separates tree construction (mutable builder) from the immutable tree entity.
eslint.config.js (1)
30-40: LGTM!The test-specific configuration targeting
test/**/*.jswith Jest-compatible globals is well-structured for multi-runtime testing with Vitest.src/domain/schemas/GitBlobSchema.js (1)
1-10: LGTM!The schema is well-structured and follows the established pattern for domain validation. Using
z.unionfor content type flexibility (string or Uint8Array) aligns with Git's ability to handle both text and binary blob content.test/domain/entities/GitCommitBuilder.test.js (2)
8-26: LGTM!The test correctly validates the builder pattern for
GitCommit, including tree SHA linkage verification viaequals(). Good use of domain fixtures.
28-44: LGTM!Good test coverage for parent handling, validating both raw string SHA and
GitShainstance inputs are properly converted and stored.src/domain/schemas/GitShaSchema.js (2)
6-9: LGTM!The schema correctly validates SHA-1 format with length constraint, hex character validation, and normalization to lowercase for consistent comparisons. Good use of Zod's transform for canonicalization.
14-16: LGTM!Clean composition pattern for object validation reusing the base
GitShaSchema.test/StreamCompletion.test.js (1)
5-20: LGTM!Good test coverage for the streaming completion promise. The stream consumption pattern using
getReader()is correct for Web Streams API compatibility across runtimes.src/domain/schemas/GitTreeSchema.js (1)
1-11: LGTM!The schema follows the established pattern with proper composition of
GitShaSchemaandGitTreeEntrySchema. Consistent withGitBlobSchema's approach to nullable/optional SHA for unwritten objects.ARCHITECTURE.md (1)
1-48: LGTM!Excellent architectural documentation that clearly explains the hexagonal architecture, validation strategy, streaming model, and engineering principles. This will be valuable for onboarding and maintaining consistency.
test/ShellRunner.test.js (3)
1-19: LGTM! Tests correctly exercise the streaming API.The test properly validates the streaming-first runner design with
stdoutStreamandexitPromise. Good practice draining the stream to avoid resource leaks.Note: The JSDoc in
ShellRunner.js(lines 11-19) documents the return type as{stdout: string, stderr: string, code: number}but the actual implementation returns{stdoutStream, exitPromise}. Consider updating the JSDoc to match the streaming interface.
21-33: LGTM!Good error-case coverage. The comment explaining why stdout must be consumed even when empty is helpful for maintainability.
35-51: LGTM!Good stdin handling test with proper stream consumption and chunk type handling. The expected SHA-1 hash for "hello world" is correct.
test/domain/entities/GitCommit.test.js (1)
7-49: LGTM! Good coverage of core GitCommit functionality.Tests cover root commits, parent handling, merge detection, validation errors, and type checking. Consider adding tests for
toJSON()andisWritten()with a non-null SHA in a follow-up if comprehensive coverage is desired.src/domain/schemas/GitRefSchema.js (2)
17-26: LGTM!The control character detection correctly handles ASCII 0-31 and DEL (127). The explicit loop is readable and avoids regex complexity.
15-15: No changes needed—the regex is correct.The regex
/[ ~^:?*[\\]/is valid JavaScript and works as intended. Inside a character class,[is not special and doesn't require escaping. Testing confirms it correctly matches all prohibited characters (space,~,^,:,?,*,[, and\) while accepting valid ref names. No cross-runtime issues exist.Likely an incorrect or invalid review comment.
src/ports/RunnerResultSchema.js (1)
11-13: LGTM!Standard Zod pattern for exposing inferred types via JSDoc.
src/domain/schemas/GitCommitSchema.js (1)
8-15: LGTM! Well-structured commit schema.The schema correctly models Git commit structure with appropriate optionality for
sha(supporting pre-write commits) and required fields for tree, parents, signatures, and message.src/ports/RunnerOptionsSchema.js (1)
3-16: LGTM! Well-documented defaults.The timeout and buffer size constants are reasonable and well-documented. The 10MB stdout buffer aligns with OOM protection goals mentioned in the PR objectives.
src/infrastructure/adapters/bun/BunShellRunner.js (1)
15-57: Well-structured streaming shell runner implementation.The implementation correctly handles Bun's synchronous stdin API, timeout racing, and streaming stdout. The pattern aligns well with the other runtime adapters.
src/domain/entities/GitCommit.js (1)
51-96: Clean entity implementation with proper serialization.The helper methods (
isWritten,type,isRoot,isMerge) andtoJSONare well-implemented. The Zod schema validation pattern provides consistent domain object construction.src/ports/CommandRunnerPort.js (1)
1-18: Clean port definition with proper typedef documentation.This module correctly consolidates the command runner interface, re-exporting constants and defining typedefs for type-safe adapter implementations.
src/infrastructure/adapters/deno/DenoShellRunner.js (1)
47-74: Timeout handling and result construction look correct.The
exitPromiseimplementation properly races between completion and timeout, and the stderr collection pattern is appropriate for Deno's stream API.src/domain/value-objects/GitRef.js (2)
20-39: Clean validation using centralized schema.The constructor and
isValidproperly delegate toGitRefSchema. Note: The underlying schema (GitRefSchema.js) still rejects all refs containing@rather than just@{sequences and bare@per Git's rules—this may cause false rejections for valid refs likefeature/@work.Consider reviewing
src/domain/schemas/GitRefSchema.jsto relax the@validation pergit-check-ref-formatrules.
64-154: Well-designed value object with comprehensive API.The instance methods (
toString,toJSON,equals, classifiers,shortName) and static factory methods (branch,tag,remote) provide a clean, immutable API for working with Git references.src/domain/entities/GitBlob.js (2)
19-36: Proper validation and defensive copying implemented.The constructor correctly validates via
GitBlobSchemabefore processing, and defensively copiesUint8Arraycontent. This addresses the previously flagged validation concerns.
42-53: Clean entity design with proper encapsulation.The getter with defensive copying, factory method, type indicator, and JSON serialization are well-implemented.
Also applies to: 59-61, 75-77, 83-88
src/infrastructure/GitStream.js (3)
31-48: Async iterator approach is cleaner but still allows concurrent reads.The refactored implementation using
Symbol.asyncIteratoris simpler and avoids the event listener races from the previous design. However, callingread()concurrently could still interleave chunks since there's no explicit lock. If strict Web Streams compliance is required, consider adding areadInProgressflag.If the current usage is always sequential (e.g., via
for awaitor single consumer), this is fine. If concurrent readers are a concern, the lock pattern from the past review comment may still be warranted.
58-75: OOM protection viamaxBytesis well-implemented.The
collect()method properly enforces byte limits before exceeding memory. Minor note: creatingTextEncoderon each chunk (line 64) could be optimized by reusing a single instance, but the impact is negligible for typical use cases.
80-112: Cross-runtime async iteration and cleanup are properly handled.The async iterator correctly delegates to native iteration when available and the
destroy()method appropriately handles both Node.js and Web Streams cleanup APIs.ShellRunner.js (1)
22-27: LGTM! Clean facade implementation with proper default handling.The spread order correctly allows callers to override the default timeout. The delegation to
ShellRunnerFactory.create()aligns with the hexagonal architecture goal of environment-agnostic execution.README.md (2)
26-35: Documentation accurately reflects the new API surface.The
createDefault()example and streaming usage patterns are well-documented. The zero-config initialization approach is clearly explained.
41-55: Good domain object examples with clear validation semantics.The examples demonstrate the type-safe nature of
GitSha,GitRef, andGitSignaturevalue objects effectively.src/infrastructure/adapters/node/NodeShellRunner.js (2)
17-26: LGTM! Stdin handling is correct.The stdin handling properly closes the stream after writing input or immediately if no input is provided, preventing the child process from hanging.
28-34: Good defensive stderr buffering with size limit.Using
DEFAULT_MAX_STDERR_SIZEto cap stderr collection prevents memory issues from verbose error output.index.js (5)
43-54: Clean factory method for zero-config instantiation.
createDefault()properly delegates toShellRunnerFactory.create()for environment detection, enabling the zero-config pattern documented in README.
60-69: Good installation verification with structured error handling.The
verifyInstallation()method properly wraps failures inGitPlumbingErrorwith a descriptive code, enabling consumers to distinguish git availability issues.
80-100: Solid execute implementation with OOM protection.The streaming-first approach with
collect({ maxBytes })provides memory safety. Error handling properly re-throwsGitPlumbingErrorinstances while wrapping other errors.
110-127: Security-hardened stream execution path.
CommandSanitizer.sanitize(args)is called before execution, which validates against prohibited commands and flags. The error wrapping is consistent withexecute().
178-188: Type-safe SHA handling in updateRef.The method correctly normalizes both
GitShainstances and raw strings, ensuring consistent validation. TheGitCommandBuilderfluent API produces clean, readable command construction.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @index.js:
- Around line 5-6: The imports "node:path" and "node:fs" in index.js break
multi-runtime support; remove those Node-specific imports and instead accept a
filesystem/path adapter via constructor or options (e.g., an options param like
fsAdapter or runtimes.adapters with methods readFile/stat/join), or make CWD
validation optional by extracting it into a separate validateCwd() method that
callers invoke from runtime-specific bootstraps; implement sensible defaults
that use node:fs/node:path only when no adapter is provided and runtime is Node,
and update any internal uses that referenced the removed imports to call the
injected adapter methods (or the new validateCwd()).
In @src/domain/services/CommandSanitizer.js:
- Around line 81-85: The code calls args[0].toLowerCase() before ensuring
args[0] is a string, which can throw a TypeError instead of the intended
ValidationError; modify CommandSanitizer.sanitize so it validates the type of
args[0] (and ideally all args) as strings first (reuse the existing per-argument
string validation logic or add a typeof check) and only then call toLowerCase()
and check this.ALLOWED_COMMANDS; when invalid, throw the same ValidationError
with the existing message/metadata pattern.
In @src/infrastructure/adapters/bun/BunShellRunner.js:
- Around line 53-74: The timeoutPromise created inside the exitPromise IIFE can
remain pending when timeout is falsy; update the logic in exitPromise so that
you either (a) only include timeoutPromise in the Promise.race when timeout is
truthy, or (b) make timeoutPromise a resolved promise when timeout is falsy
(e.g., Promise.resolve({ timedOut: false }) ) to avoid a dangling unresolved
promise; adjust references around timeoutPromise, completionPromise, timeoutId
and the race call so Promise.race([completionPromise, timeoutPromise]) only
races resolvable promises and still clears timeoutId when completionPromise
wins.
In @src/infrastructure/adapters/node/NodeShellRunner.js:
- Around line 57-72: The timeout is set unconditionally inside the exitPromise
in NodeShellRunner.js, causing setTimeout to fire immediately when timeout is
undefined or 0; modify the exitPromise logic to only create the timeout (const
timeoutId = setTimeout(...)) when timeout is a positive finite number (e.g.,
Number.isFinite(timeout) && timeout > 0), and only call clearTimeout if
timeoutId was created; leave the child.on('exit') and child.on('error') handlers
to resolve as before but ensure they pass timedOut: false when no timeout was
used and include the error details in the resolved object as currently done.
In @test.js:
- Around line 53-59: The test "handles errors with telemetry" can silently
succeed if plumbing.execute does not throw; change the test to assert the
rejection explicitly by using the async expect pattern (await
expect(plumbing.execute({ args: [...] })).rejects.toThrow(...)) or, if keeping
try/catch, add a failing assertion immediately after the call (e.g., call fail()
or expect(true).toBe(false)) so the test fails when execute does not throw;
reference the test name and the plumbing.execute invocation to locate and update
the assertion accordingly.
🧹 Nitpick comments (1)
src/infrastructure/GitStream.js (1)
80-101: Consider reusing TextEncoder for performance.Line 87 creates a new
TextEncoderinstance for each string chunk to compute byte length. While functional, instantiating the encoder repeatedly adds minor GC pressure.♻️ Optional optimization
async collect({ maxBytes = DEFAULT_MAX_BUFFER_SIZE } = {}) { const decoder = new TextDecoder(); + const encoder = new TextEncoder(); let totalBytes = 0; let result = ''; try { for await (const chunk of this) { - const bytes = typeof chunk === 'string' ? new TextEncoder().encode(chunk) : chunk; + const bytes = typeof chunk === 'string' ? encoder.encode(chunk) : chunk; if (totalBytes + bytes.length > maxBytes) { throw new Error(`Buffer limit exceeded: ${maxBytes} bytes`); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
CHANGELOG.mdindex.jssrc/domain/entities/GitCommit.jssrc/domain/entities/GitCommitBuilder.jssrc/domain/entities/GitTree.jssrc/domain/entities/GitTreeBuilder.jssrc/domain/entities/GitTreeEntry.jssrc/domain/services/CommandSanitizer.jssrc/domain/services/GitRepositoryService.jssrc/infrastructure/GitStream.jssrc/infrastructure/adapters/bun/BunShellRunner.jssrc/infrastructure/adapters/deno/DenoShellRunner.jssrc/infrastructure/adapters/node/NodeShellRunner.jstest.jstest/ShellRunner.test.jstest/Streaming.test.jstest/domain/entities/GitCommit.test.jstest/domain/entities/GitTree.test.jstest/domain/entities/GitTreeEntry.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/domain/entities/GitCommitBuilder.js
- test/ShellRunner.test.js
- test/domain/entities/GitCommit.test.js
- test/Streaming.test.js
- test/domain/entities/GitTree.test.js
🧰 Additional context used
🧬 Code graph analysis (13)
test/domain/entities/GitTreeEntry.test.js (5)
test/domain/entities/GitTree.test.js (2)
sha(8-8)regularMode(9-9)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)src/domain/value-objects/GitFileMode.js (1)
GitFileMode(11-90)src/domain/entities/GitTreeEntry.js (1)
GitTreeEntry(17-80)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/domain/services/GitRepositoryService.js (2)
src/domain/services/GitCommandBuilder.js (1)
GitCommandBuilder(8-61)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)
src/infrastructure/adapters/deno/DenoShellRunner.js (2)
src/domain/services/ByteMeasurer.js (1)
ENCODER(5-5)src/ports/RunnerResultSchema.js (2)
RunnerResultSchema(6-9)RunnerResultSchema(6-9)
src/domain/entities/GitTreeEntry.js (4)
src/domain/value-objects/GitFileMode.js (1)
GitFileMode(11-90)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)src/domain/schemas/GitTreeEntrySchema.js (2)
GitTreeEntrySchema(8-12)GitTreeEntrySchema(8-12)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/domain/entities/GitTreeBuilder.js (3)
src/domain/entities/GitTreeEntry.js (1)
GitTreeEntry(17-80)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/entities/GitTree.js (1)
GitTree(18-113)
src/domain/entities/GitTree.js (4)
src/domain/value-objects/GitSha.js (1)
GitSha(12-109)src/domain/entities/GitTreeEntry.js (1)
GitTreeEntry(17-80)src/domain/schemas/GitTreeSchema.js (2)
GitTreeSchema(8-11)GitTreeSchema(8-11)src/domain/value-objects/GitObjectType.js (1)
GitObjectType(10-198)
test.js (2)
index.js (1)
GitPlumbing(19-201)src/domain/services/GitRepositoryService.js (1)
GitRepositoryService(12-60)
src/infrastructure/adapters/bun/BunShellRunner.js (1)
src/ports/RunnerResultSchema.js (2)
RunnerResultSchema(6-9)RunnerResultSchema(6-9)
src/domain/entities/GitCommit.js (5)
src/domain/value-objects/GitSha.js (1)
GitSha(12-109)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/value-objects/GitSignature.js (1)
GitSignature(11-52)src/domain/schemas/GitCommitSchema.js (2)
GitCommitSchema(8-15)GitCommitSchema(8-15)src/domain/value-objects/GitObjectType.js (1)
GitObjectType(10-198)
src/domain/services/CommandSanitizer.js (1)
src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/infrastructure/adapters/node/NodeShellRunner.js (2)
src/ports/RunnerOptionsSchema.js (2)
DEFAULT_MAX_STDERR_SIZE(16-16)DEFAULT_MAX_STDERR_SIZE(16-16)src/ports/RunnerResultSchema.js (2)
RunnerResultSchema(6-9)RunnerResultSchema(6-9)
src/infrastructure/GitStream.js (1)
src/ports/RunnerOptionsSchema.js (2)
DEFAULT_MAX_BUFFER_SIZE(11-11)DEFAULT_MAX_BUFFER_SIZE(11-11)
index.js (7)
src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/infrastructure/factories/ShellRunnerFactory.js (1)
ShellRunnerFactory(12-49)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)src/ports/RunnerOptionsSchema.js (4)
DEFAULT_MAX_BUFFER_SIZE(11-11)DEFAULT_MAX_BUFFER_SIZE(11-11)RunnerOptionsSchema(21-27)RunnerOptionsSchema(21-27)src/domain/services/CommandSanitizer.js (1)
CommandSanitizer(10-120)src/infrastructure/GitStream.js (1)
GitStream(26-154)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)
🔇 Additional comments (26)
src/infrastructure/adapters/bun/BunShellRunner.js (4)
15-22: LGTM: Security-focused environment allowlist.The restricted environment variable allowlist effectively prevents sensitive environment variable leakage to Git subprocesses.
29-36: LGTM: Defensive environment sanitization.The environment filtering correctly handles missing
process.envand only copies defined values from the allowlist.
46-51: LGTM: Proper stdin lifecycle management.The stdin handling correctly writes input when provided and closes the stream in both cases, preventing deadlocks.
76-79: LGTM: Schema validation ensures contract compliance.Using
RunnerResultSchema.parseat the return boundary ensures the runner adheres to the port interface contract.src/infrastructure/GitStream.js (5)
10-20: LGTM: Automatic resource cleanup with FinalizationRegistry.The registry-based cleanup ensures streams are destroyed if a GitStream instance is garbage collected before consumption, preventing resource leaks.
31-38: LGTM: Constructor properly initializes state and lifecycle tracking.The
_consumedflag and registry registration enable proper resource management and prevent double-consumption errors.
45-71: LGTM: Async iterator-based polyfill resolves previous locking concerns.The refactored implementation uses the stream's async iterator protocol instead of manual event listeners. This inherently prevents concurrent
read()calls becauseit.next()manages internal state sequentially. The previous review concern about Web Streams locking semantics has been addressed by this architectural change.
106-136: LGTM: Robust async iteration with consumption tracking.The implementation correctly prevents double-consumption, prefers native iteration for performance, and ensures cleanup via the
finallyblock.
142-153: LGTM: Safe stream destruction with error suppression.The
destroy()method safely handles both Node.js and Web Streams APIs and properly unregisters from the finalization registry.src/infrastructure/adapters/node/NodeShellRunner.js (5)
17-24: LGTM: Consistent security allowlist across runtimes.The environment variable allowlist matches the Bun implementation, ensuring consistent security posture.
31-38: LGTM: Defensive environment sanitization.The filtering logic correctly handles missing
process.envand only copies allowed, defined variables.
40-48: LGTM: Proper process spawning and stdin handling.The spawn configuration and stdin lifecycle management prevent deadlocks and follow the established pattern.
50-55: LGTM: Memory-safe stderr accumulation.The stderr buffer limit prevents memory exhaustion from verbose error output.
74-77: LGTM: Contract enforcement via schema validation.Schema validation at the return boundary ensures runtime consistency with the port interface.
src/domain/entities/GitCommit.js (4)
55-73: LGTM: Schema-driven factory with proper type coercion.The
fromData()method correctly validates input viaGitCommitSchemabefore constructing the entity. The schema ensuresparentsis an array of valid SHA strings, making the.map()call on line 68 safe.However, note that direct constructor usage bypasses this validation (see separate constructor comment).
79-89: LGTM: Simple accessor methods with clear semantics.The
isWritten()andtype()methods provide clear, self-documenting checks.
95-105: LGTM: Parent-based commit type checks.The
isRoot()andisMerge()methods correctly identify commit types based on parent count.
111-120: LGTM: Comprehensive JSON serialization.The
toJSON()method correctly serializes all fields, handling nullableshaand delegating to nestedtoJSON()methods.test/domain/entities/GitTreeEntry.test.js (1)
1-27: LGTM!The test suite effectively covers GitTreeEntry's core behaviors: valid entry creation, invalid SHA validation, and tree vs blob identification. The test structure is clean and assertions are appropriate.
src/domain/services/GitRepositoryService.js (1)
1-60: LGTM!GitRepositoryService provides a clean abstraction for repository operations. The GitSha normalization in
updateRef(lines 40-41) correctly handles both GitSha instances and strings, and the use of GitCommandBuilder ensures consistent command construction. The implementation is well-structured and follows domain-driven design principles.test.js (2)
18-27: LGTM!The async
beforeEachproperly initializes the test environment with the new GitRepositoryService and callsverifyInstallation()to ensure Git is ready. The sequential execution of initialization steps is correct.
42-51: LGTM!The test correctly uses the new GitRepositoryService API for
updateRefandrevParseoperations, demonstrating proper integration with the hexagonal architecture changes.index.js (1)
25-40: Node.js-specific filesystem APIs break multi-runtime support.Lines 31-34 use
fs.existsSync()andfs.statSync()fromnode:fs, which are Node.js-specific. Since the PR targets Bun and Deno support, this constructor will fail when instantiated in those environments.Likely an incorrect or invalid review comment.
src/domain/entities/GitTreeEntry.js (1)
1-80: LGTM! Well-designed domain entity with comprehensive validation.The implementation correctly:
- Validates all inputs via
GitTreeEntrySchema(lines 31-38), including the path validation addressed from previous reviews- Handles both value object and primitive inputs for
modeandsha- Uses proper value objects (
GitFileMode,GitSha) for domain modeling- Provides clear, delegating methods (
type(),isTree(),isBlob())- Implements standardized serialization via
toJSON()The schema-based validation approach ensures consistent validation rules across the domain layer.
src/domain/entities/GitTreeBuilder.js (1)
1-49: LGTM! Clean builder pattern with proper validation.The implementation correctly:
- Uses the fluent builder pattern for efficient tree construction (as noted in the fileoverview)
- Validates entries via
instanceofcheck (line 23)- Provides both high-level
add()and low-leveladdEntry()methods for flexibility- Creates a defensive copy in
build()(line 47) to prevent external mutation- Returns
thisfor method chainingThe O(N) construction mentioned in the fileoverview is achieved through simple array accumulation.
src/domain/entities/GitTree.js (1)
1-113: LGTM! Solid domain entity with immutability and validation.The implementation correctly:
- Validates entries in the constructor (lines 29-34), addressing previous review concerns
- Uses defensive copying via
map()to prevent external mutation- Implements immutable
addEntry()pattern (line 84) by returning a newGitTree- Provides a defensive copy in the
entriesgetter (line 64)- Offers both constructor and
fromData()factory with schema validation- Implements the empty tree pattern via
GitSha.EMPTY_TREEThe combination of validation, immutability, and defensive copying creates a robust, safe entity for the domain layer.
…rmance - Implement Uint8Array chunk accumulation in GitStream.collect() - Add 'asString' and 'encoding' options to GitStream.collect() - Optimize ByteMeasurer to use Buffer.byteLength() where available - Upgrade vitest to ^3.0.0 - Update index.js, README.md, and CHANGELOG.md
- Add GitRepositoryLockedError with remediation guidance - Add CommandRetryPolicy value object for configurable backoff - Refactor ShellRunnerFactory with registry and env overrides - Add GitPlumbing.createRepository() factory - Integrate retry policy and locked error in GitPlumbing.execute()
- Move retry loop and backoff logic to ExecutionOrchestrator - Move lock-detection logic and specialized error throwing to ExecutionOrchestrator - Clean up GitPlumbing.execute() to use the orchestrator - Update CHANGELOG and README
- Implement GIT_PLUMBING_ENV override in createDefault() - Add docs/RECIPES.md with 'Commit from Scratch' guide - Ensure execute() maintains backward compatibility with asString: true - Finalize GitPlumbing.createRepository() integration
- Update CHANGELOG.md with all recent additions - Add Documentation section to README.md - Link to new recipes and troubleshooting guides
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @CHANGELOG.md:
- Line 8: The CHANGELOG.md header shows version "2.2.0" while package.json's
"version" field is "1.0.0"; update one to match the other before release—either
bump package.json "version" to "2.2.0" (or the intended release version) or
correct CHANGELOG.md to the actual package.json version, and ensure the VERSION
in package.json (the "version" key) and the topmost CHANGELOG entry (## [x.y.z]
- date) are synchronized and consistent with your release plan.
In @README.md:
- Around line 59-72: The README import example uses named imports from the
package root for GitSha, GitRef, and GitSignature but package.json exposes them
only at subpath exports; either add root re-exports for those symbols (re-export
GitSha, GitRef, GitSignature from ./sha, ./ref, ./signature in the package
entry) or update the README import lines to import GitSha from
'@git-stunts/plumbing/sha', GitRef from '@git-stunts/plumbing/ref', and
GitSignature from '@git-stunts/plumbing/signature' so the example matches the
actual exports.
- Line 27: The README version references ("Version 2.0.0" and "Version 2.2.0")
are inconsistent with package.json's "version": "1.0.0"; update either README or
package.json so versions match: either change the README text to reflect
"Version 1.0.0" (or appropriate actual release notes) or bump the "version"
field in package.json to the intended release version (e.g., "2.2.0") and ensure
any changelog/notes in README align with that chosen version.
In @src/domain/errors/GitRepositoryLockedError.js:
- Around line 10-24: The documentation URL in the GitRepositoryLockedError
constructor points to a non-existent TROUBLESHOOTING.md; update the
documentation property in the constructor of GitRepositoryLockedError to a valid
existing resource (for example the current docs/RECIPES.md or another real URL)
or remove the documentation field entirely if no suitable doc exists, ensuring
the object passed to super(...) still includes code: 'GIT_REPOSITORY_LOCKED' and
remediation text.
In @src/domain/services/ExecutionOrchestrator.js:
- Around line 36-39: Off-by-one in the backoff calculation: in
ExecutionOrchestrator the code calls retryPolicy.getDelay(attempt + 1) when
scheduling the next wait, which computes the delay for the wrong attempt; change
the call to retryPolicy.getDelay(attempt) (or otherwise compute the delay for
the upcoming retry consistently) so the delay corresponds to the current retry
index, keeping the rest of the loop logic and the check against
retryPolicy.maxAttempts unchanged.
🧹 Nitpick comments (5)
package.json (1)
7-15: Exports map exposes internal directory structure.The exports map references deep paths like
./src/domain/value-objects/GitSha.js. This couples consumers to internal directory structure. If you refactor file locations, consumers' imports will break.Consider adding re-export barrel files or keeping these as implementation detail until the internal structure stabilizes.
💡 Example barrel file approach
// src/domain/value-objects/index.js (new barrel file) export { default as GitSha } from './GitSha.js'; export { default as GitRef } from './GitRef.js'; export { default as GitFileMode } from './GitFileMode.js'; export { default as GitSignature } from './GitSignature.js';Then update package.json:
- "./sha": "./src/domain/value-objects/GitSha.js", - "./ref": "./src/domain/value-objects/GitRef.js", - "./mode": "./src/domain/value-objects/GitFileMode.js", - "./signature": "./src/domain/value-objects/GitSignature.js", + "./value-objects": "./src/domain/value-objects/index.js",index.js (1)
160-179: Consider returning structured error info instead of throwing inexecuteWithStatus.The JSDoc states this method returns "stdout and exit status without throwing on non-zero exit", but Line 172-178 still throws
GitPlumbingErroron exceptions. This is correct for actual errors (e.g., stream failures), but the docstring could clarify that only command non-zero exits don't throw, while infrastructure errors still throw.src/domain/services/ExecutionOrchestrator.js (2)
21-74: Missing explicit return at end oforchestratemethod.If the while loop completes without returning or throwing (which shouldn't happen in practice), the function implicitly returns
undefined. Adding an explicit throw or unreachable assertion after the loop improves clarity and catches logic errors.♻️ Proposed fix
} } + // Should be unreachable - loop always returns or throws + throw new GitPlumbingError('Unexpected: retry loop exited without result', 'ExecutionOrchestrator.orchestrate', { args, traceId }); } }
33-34: Lock detection may produce false positives.The check
result.stderr.includes('index.lock') || result.stderr.includes('.lock')could match unrelated messages containing these substrings (e.g., file paths likeunlock.jsor error messages mentioning "block"). Consider a more specific pattern like checking for "Unable to create" combined with ".lock".src/domain/value-objects/CommandRetryPolicy.js (1)
17-25: Consider validatinginitialDelayMsandbackoffFactor.The constructor validates
maxAttempts >= 1but allows negative or zero values forinitialDelayMsandbackoffFactor, which would produce invalid delays.♻️ Proposed validation
constructor({ maxAttempts = 3, initialDelayMs = 100, backoffFactor = 2 } = {}) { if (maxAttempts < 1) { throw new InvalidArgumentError('maxAttempts must be at least 1', 'CommandRetryPolicy.constructor'); } + if (initialDelayMs < 0) { + throw new InvalidArgumentError('initialDelayMs must be non-negative', 'CommandRetryPolicy.constructor'); + } + if (backoffFactor < 1) { + throw new InvalidArgumentError('backoffFactor must be at least 1', 'CommandRetryPolicy.constructor'); + } this.maxAttempts = maxAttempts;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mddocs/RECIPES.mdindex.jspackage.jsonsrc/domain/errors/GitRepositoryLockedError.jssrc/domain/services/ByteMeasurer.jssrc/domain/services/ExecutionOrchestrator.jssrc/domain/value-objects/CommandRetryPolicy.jssrc/infrastructure/GitStream.jssrc/infrastructure/factories/ShellRunnerFactory.js
✅ Files skipped from review due to trivial changes (1)
- docs/RECIPES.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/domain/services/ByteMeasurer.js
- src/infrastructure/GitStream.js
- src/infrastructure/factories/ShellRunnerFactory.js
🧰 Additional context used
🧬 Code graph analysis (4)
src/domain/errors/GitRepositoryLockedError.js (1)
src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
src/domain/services/ExecutionOrchestrator.js (2)
src/domain/errors/GitRepositoryLockedError.js (1)
GitRepositoryLockedError(10-25)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
src/domain/value-objects/CommandRetryPolicy.js (1)
src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)
index.js (10)
src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/infrastructure/factories/ShellRunnerFactory.js (1)
ShellRunnerFactory(12-74)src/domain/services/GitRepositoryService.js (1)
GitRepositoryService(12-60)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)src/ports/RunnerOptionsSchema.js (4)
DEFAULT_MAX_BUFFER_SIZE(11-11)DEFAULT_MAX_BUFFER_SIZE(11-11)RunnerOptionsSchema(21-27)RunnerOptionsSchema(21-27)src/domain/value-objects/CommandRetryPolicy.js (1)
CommandRetryPolicy(10-66)src/domain/services/ExecutionOrchestrator.js (1)
ExecutionOrchestrator(11-75)src/domain/services/CommandSanitizer.js (1)
CommandSanitizer(10-120)src/infrastructure/GitStream.js (1)
GitStream(26-167)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)
🔇 Additional comments (6)
README.md (1)
100-108: Architecture section is well-structured.Good documentation of design principles (1 File = 1 Class, DI, centralized constants, serializability). The link to ARCHITECTURE.md provides a clear path for deeper exploration.
index.js (2)
104-122: Execute method design is solid.Good integration with
ExecutionOrchestratorfor retry logic, proper use ofCommandRetryPolicy, and inclusion of trace IDs for observability. The streaming-first approach with fallback to buffered collection is well-architected.
113-116: No action needed. The property name is correctly implemented.GitStream's constructor accepts
exitPromiseas a parameter (line 31) and immediately assigns it to thethis.finishedproperty (line 33). The code in index.js correctly accesses this property viastream.finishedon line 115. No mismatch exists.src/domain/value-objects/CommandRetryPolicy.js (2)
32-37: Backoff calculation is correct.The exponential backoff formula
Math.pow(backoffFactor, attempt - 1) * initialDelayMscorrectly produces increasing delays: 0ms → 200ms → 400ms → 800ms for the default configuration.
43-53: Good use of static factory methods.
default()andnone()provide clear, self-documenting ways to create common policy configurations.CHANGELOG.md (1)
8-78: Well-structured changelog with comprehensive entries.The changelog follows Keep a Changelog format with clear categorization (Added, Changed, Fixed). The entries provide good context for each change, including the rationale (e.g., "to improve SRP compliance", "preventing corruption of non-UTF8 binary data").
- Extracted environment variable whitelisting into EnvironmentPolicy domain service. - Expanded whitelist to include GIT_AUTHOR_*, GIT_COMMITTER_*, LANG, and LC_ALL. - Completed migration from ajv to zod for unified type-safety and reduced bundle size. - Updated all shell runners (Node, Bun, Deno) to use EnvironmentPolicy. - Enhanced multi-runtime test parity, adding missing tests to Deno suite. - Verified 100% test pass rate across Node.js, Bun, and Deno.
…dling - Refactored CommandSanitizer and ExecutionOrchestrator into injectable class instances. - Introduced GitErrorClassifier domain service for centralized error categorization. - Added ProhibitedFlagError for better security reporting and developer guidance. - Implemented memoization in CommandSanitizer to optimize repetitive command validation. - Added CommandSanitizer.allow() for dynamic plumbing command registration. - Updated multi-runtime test suite and Deno shim for lifecycle parity. - Verified 100% test pass rate across Node.js, Bun, and Deno.
- Added comprehensive static factory methods to GitCommandBuilder for all whitelisted Git commands. - Implemented fluent flag methods (.stdin(), .write(), .pretty(), etc.) in GitCommandBuilder for cleaner command construction. - Exported GitCommandBuilder from the main entry point. - Verified GitPlumbing constructor supports injectable sanitizer and orchestrator dependencies. - Updated README and ARCHITECTURE documentation. - Verified 100% test pass rate across Node.js, Bun, and Deno.
- Created GitPersistenceService to handle low-level object database interactions for Blobs, Trees, and Commits. - Implemented writeBlob, writeTree, and writeCommit in GitRepositoryService. - Added GitPlumbing.commit() orchestration method for end-to-end commit creation. - Enhanced shell runners and GitPlumbing.execute to support environment variable overrides. - Updated RunnerOptionsSchema to include optional env support. - Added comprehensive integration tests for persistence and commit flows. - Verified 100% test pass rate across Node.js, Bun, and Deno.
…hancements - Created docs/COMMIT_LIFECYCLE.md covering step-by-step Git object construction and persistence. - Added polymorphic GitRepositoryService.save() for unified entity persistence. - Updated README.md with hardened security details and prominent documentation links. - Verified 100% test pass rate across Node.js, Bun, and Deno.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @CHANGELOG.md:
- Line 8: Update package.json's "version" field to match the CHANGELOG top entry
(set version to 2.7.0) so package.json and CHANGELOG.md are consistent; then fix
the release dates in CHANGELOG.md for versions 2.7.0 through 1.1.0 so they
reflect the intended, distinct release dates instead of all using 2026-01-07 (or
revert them to their correct historical dates), and lastly change the phrase on
line 32 from "command building experience" to the hyphenated "command-building
experience."
In @index.js:
- Around line 5-6: Remove the Node-specific imports ("node:path" and "node:fs")
and stop performing file checks at module load; instead add an optional
fsAdapter option (e.g., fsAdapter with methods existsSync, statSync, resolve) to
the public initializer/constructor that currently triggers CWD validation,
replace direct calls to fs/stat/resolve in the CWD validation block with calls
to fsAdapter.existsSync/fsAdapter.statSync/fsAdapter.resolve, and if no
fsAdapter is provided make CWD validation a no-op (or expose a validateCwd()
helper that runtime-specific bootstraps can call with a provided adapter) so the
module stays runtime-agnostic.
In @src/domain/services/CommandSanitizer.js:
- Around line 110-114: The code calls args[0].toLowerCase() before
type-checking, so change CommandSanitizer.sanitize to validate that args and
args[0] are strings (or at least that typeof args[0] === 'string') before
calling .toLowerCase(); if the type check fails, throw the existing
ValidationError with the same message/context, then proceed to compute const
command = args[0].toLowerCase() and check
CommandSanitizer._ALLOWED_COMMANDS.has(command).
In @src/domain/services/ExecutionOrchestrator.js:
- Line 52: The backoff delay is computed for the wrong attempt because the code
calls retryPolicy.getDelay(attempt + 1) while the local variable attempt already
represents the 1-based retry count; change the call to
retryPolicy.getDelay(attempt) in the ExecutionOrchestrator retry loop so the
delay corresponds to the current attempt, ensuring you adjust only the argument
to getDelay and keep using the existing attempt and retryPolicy variables.
- Around line 61-70: In the catch block inside ExecutionOrchestrator (the
try/catch around execution invocation) remove the empty placeholder if
(err.name?.includes('Error')) { ... } and simply rethrow the caught error, or if
you need re-classification implement explicit checks for known conditions (e.g.,
err.name === 'SpawnError' or 'TimeoutError') and wrap those in your domain error
type before throwing; in short either delete the no-op conditional and keep
throw err; or replace the comments with explicit classification logic for
spawn/timeout errors and then throw the appropriate classified error.
In @src/domain/services/GitPersistenceService.js:
- Around line 74-76: The tree entry type detection is wrong: using
entry.sha.isEmptyTree() treats only empty trees as trees and marks regular
subdirectory tree SHAs as 'blob', corrupting the tree; update the mapping that
builds input (the const input = ... block) to use the GitTreeEntry type helpers
instead of inspecting the SHA—call entry.isTree() or entry.type() (whichever is
available on GitTreeEntry) to determine whether to emit 'tree' vs 'blob' so each
entry uses the correct type when constructing the tree object.
In @src/infrastructure/adapters/node/NodeShellRunner.js:
- Around line 40-55: The timeout setup in exitPromise is unguarded so setTimeout
is called with undefined (or 0) which fires immediately; change the logic in the
exitPromise to only create a timeoutId and call setTimeout when timeout is a
finite number > 0 (e.g., check typeof timeout === 'number' && timeout > 0),
store timeoutId in a let variable, and only call clearTimeout(timeoutId) when
timeoutId was created; ensure child.kill() remains inside the guarded timeout
callback and that the resolve paths behave the same when no timeout is
configured.
🧹 Nitpick comments (7)
CHANGELOG.md (1)
32-32: Minor: Consider hyphenating the compound modifier.Line 32 uses "command building experience"; when "command building" modifies "experience," it reads more clearly as "command-building experience" in technical writing.
Optional hyphenation improvement
- - **GitCommandBuilder Fluent API**: Added static factory methods for all whitelisted Git commands (e.g., `.hashObject()`, `.catFile()`, `.writeTree()`) and fluent flag methods (e.g., `.stdin()`, `.write()`, `.pretty()`) for a more expressive command building experience. + - **GitCommandBuilder Fluent API**: Added static factory methods for all whitelisted Git commands (e.g., `.hashObject()`, `.catFile()`, `.writeTree()`) and fluent flag methods (e.g., `.stdin()`, `.write()`, `.pretty()`) for a more expressive command-building experience.docs/COMMIT_LIFECYCLE.md (2)
48-48: Simplify redundant phrase.The phrase "a specific point in time" is redundant. Consider rewriting as "at a specific time" or "at a given moment."
✨ Proposed fix
-A commit object links a tree to a specific point in time, with an author, a committer, a message, and optional parent references. +A commit object links a tree to a specific time, with an author, a committer, a message, and optional parent references.
106-106: Add language identifier to fenced code block.The closing fence at line 106 lacks a language specifier. The previous code block (lines 93–104) is JavaScript and should be marked accordingly.
✨ Proposed fix
-``` +```javascriptARCHITECTURE.md (1)
1-50: Excellent architecture documentation.The documentation clearly articulates the hexagonal architecture, domain-driven design principles, and engineering mandates. The structure and content are comprehensive.
If you want to make the prose slightly more concise, consider:
- Line 36: "identically" instead of "exactly the same way"
- Line 49: "identical" instead of "exact same"
These are purely stylistic and don't affect clarity.
src/domain/services/EnvironmentPolicy.js (1)
14-35: Add documentation explaining the environment variable whitelist design.The whitelist in
_ALLOWED_KEYSomits several environment variables that might be expected (such asHOME,SSH_AUTH_SOCK,GIT_DIR,GIT_WORK_TREE, andTMPDIR). None of these variables appear to be referenced in the codebase, suggesting the whitelist is intentionally restrictive. However, without explicit documentation explaining why certain variables are excluded, it's unclear whether these omissions are by design or oversight. Consider adding a comment clarifying the whitelist philosophy (e.g., whether it prioritizes security over convenience, or whether variables are only included based on demonstrated need).test/domain/services/CommandSanitizer.test.js (1)
37-70: Consider testing cache behavior without mutating static state.The memoization and cache eviction tests work correctly but mutate the static
_ALLOWED_COMMANDSproperty (lines 44-45, 63-64) to verify caching. While the tests restore state, this approach couples tests to implementation details and could become brittle if the internal caching mechanism changes.Consider exposing cache statistics or providing test-only hooks to verify caching behavior without directly manipulating internal static state.
src/domain/services/GitErrorClassifier.js (1)
26-28: Consider refining the lock detection pattern to reduce false positives.The current pattern
.includes('.lock')is quite broad and may match unrelated strings. While false positives are acceptable for retry logic, a more precise check could improve accuracy.♻️ Optional refinement
- const isLocked = stderr.includes('index.lock') || - stderr.includes('.lock') || - (code === 128 && stderr.includes('lock')); + const isLocked = stderr.includes('index.lock') || + /\.git\/.*\.lock/.test(stderr) || + (code === 128 && /unable to create.*\.lock/.test(stderr));This tightens the detection to Git-specific lock patterns while maintaining the existing logic.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
ARCHITECTURE.mdCHANGELOG.mdREADME.mddocs/COMMIT_LIFECYCLE.mdindex.jspackage.jsonsrc/domain/errors/ProhibitedFlagError.jssrc/domain/services/CommandSanitizer.jssrc/domain/services/EnvironmentPolicy.jssrc/domain/services/ExecutionOrchestrator.jssrc/domain/services/GitCommandBuilder.jssrc/domain/services/GitErrorClassifier.jssrc/domain/services/GitPersistenceService.jssrc/domain/services/GitRepositoryService.jssrc/infrastructure/adapters/bun/BunShellRunner.jssrc/infrastructure/adapters/deno/DenoShellRunner.jssrc/infrastructure/adapters/node/NodeShellRunner.jssrc/ports/GitPersistencePort.jssrc/ports/RunnerOptionsSchema.jstest/GitCommitFlow.test.jstest/deno_entry.jstest/deno_shim.jstest/domain/services/CommandSanitizer.test.jstest/domain/services/EnvironmentPolicy.test.jstest/domain/services/GitCommandBuilder.test.jstest/domain/services/GitErrorClassifier.test.jstest/domain/services/GitPersistenceService.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- src/ports/RunnerOptionsSchema.js
- src/infrastructure/adapters/bun/BunShellRunner.js
- test/deno_shim.js
- src/infrastructure/adapters/deno/DenoShellRunner.js
🧰 Additional context used
🧬 Code graph analysis (12)
test/domain/services/EnvironmentPolicy.test.js (1)
src/domain/services/EnvironmentPolicy.js (1)
EnvironmentPolicy(9-51)
test/domain/services/CommandSanitizer.test.js (3)
src/domain/services/CommandSanitizer.js (1)
CommandSanitizer(11-156)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/errors/ProhibitedFlagError.js (1)
ProhibitedFlagError(10-20)
src/domain/errors/ProhibitedFlagError.js (1)
src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
src/domain/services/GitRepositoryService.js (3)
src/domain/services/GitPersistenceService.js (1)
GitPersistenceService(15-122)src/domain/services/GitCommandBuilder.js (1)
GitCommandBuilder(8-139)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)
test/domain/services/GitPersistenceService.test.js (7)
src/domain/services/GitPersistenceService.js (1)
GitPersistenceService(15-122)src/domain/entities/GitBlob.js (1)
GitBlob(14-89)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)src/domain/entities/GitTreeEntry.js (1)
GitTreeEntry(17-80)src/domain/entities/GitTree.js (1)
GitTree(18-113)src/domain/value-objects/GitSignature.js (1)
GitSignature(11-52)src/domain/entities/GitCommit.js (1)
GitCommit(18-121)
src/domain/services/GitErrorClassifier.js (2)
src/domain/errors/GitRepositoryLockedError.js (1)
GitRepositoryLockedError(10-25)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
src/domain/services/CommandSanitizer.js (2)
src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/errors/ProhibitedFlagError.js (1)
ProhibitedFlagError(10-20)
src/infrastructure/adapters/node/NodeShellRunner.js (3)
src/domain/services/EnvironmentPolicy.js (1)
EnvironmentPolicy(9-51)src/ports/RunnerOptionsSchema.js (2)
DEFAULT_MAX_STDERR_SIZE(16-16)DEFAULT_MAX_STDERR_SIZE(16-16)src/ports/RunnerResultSchema.js (2)
RunnerResultSchema(6-9)RunnerResultSchema(6-9)
test/domain/services/GitCommandBuilder.test.js (2)
src/domain/services/GitCommandBuilder.js (1)
GitCommandBuilder(8-139)test/domain/entities/GitCommit.test.js (1)
treeSha(7-7)
test/domain/services/GitErrorClassifier.test.js (3)
src/domain/services/GitErrorClassifier.js (1)
GitErrorClassifier(11-58)src/domain/errors/GitRepositoryLockedError.js (1)
GitRepositoryLockedError(10-25)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
src/domain/services/GitPersistenceService.js (6)
src/domain/entities/GitBlob.js (1)
GitBlob(14-89)src/domain/entities/GitTree.js (1)
GitTree(18-113)src/domain/entities/GitCommit.js (1)
GitCommit(18-121)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/services/GitCommandBuilder.js (1)
GitCommandBuilder(8-139)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)
index.js (13)
src/domain/services/ExecutionOrchestrator.js (1)
ExecutionOrchestrator(10-73)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/services/GitRepositoryService.js (1)
GitRepositoryService(13-99)src/domain/entities/GitBlob.js (1)
GitBlob(14-89)src/domain/entities/GitTreeEntry.js (1)
GitTreeEntry(17-80)src/domain/entities/GitTree.js (1)
GitTree(18-113)src/domain/entities/GitCommit.js (1)
GitCommit(18-121)src/infrastructure/factories/ShellRunnerFactory.js (1)
ShellRunnerFactory(12-74)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)src/ports/RunnerOptionsSchema.js (4)
DEFAULT_MAX_BUFFER_SIZE(11-11)DEFAULT_MAX_BUFFER_SIZE(11-11)RunnerOptionsSchema(21-28)RunnerOptionsSchema(21-28)src/domain/value-objects/CommandRetryPolicy.js (1)
CommandRetryPolicy(10-66)src/infrastructure/GitStream.js (1)
GitStream(26-167)src/domain/value-objects/GitSha.js (1)
GitSha(12-109)
🪛 LanguageTool
docs/COMMIT_LIFECYCLE.md
[style] ~48-~48: This phrase is redundant. Consider writing “point” or “time”.
Context: ...ommit object links a tree to a specific point in time, with an author, a committer, a message...
(MOMENT_IN_TIME)
CHANGELOG.md
[grammar] ~32-~32: Use a hyphen to join words.
Context: ...pretty()`) for a more expressive command building experience. ### Changed - **Gi...
(QB_NEW_EN_HYPHEN)
ARCHITECTURE.md
[style] ~36-~36: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...- Consistency: Every runner behaves exactly the same way. - Memory Safety: Large outputs...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
[style] ~49-~49: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...vironment (via Docker Compose) runs the exact same test suite in Bun and Deno simultaneous...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
🪛 markdownlint-cli2 (0.18.1)
docs/COMMIT_LIFECYCLE.md
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (33)
CHANGELOG.md (1)
8-127: Clarify the release date anomaly: multiple versions released on the same day.All versions from 2.7.0 down to 1.1.0 are dated 2026-01-07 (the PR creation date), which suggests these may be placeholder dates rather than actual release dates. If these represent a planned multi-version release, please clarify the intended release strategy. If they are staggered releases, update each version with its corresponding date.
docs/COMMIT_LIFECYCLE.md (1)
1-104: Documentation is clear and well-structured.The guide effectively walks through the Git commit lifecycle from low-level primitives (blobs, trees, commits) to high-level orchestration, with realistic code examples and progressive complexity. The progression from manual graph construction to the convenience
commit()method is pedagogically sound and aligns well with the hexagonal architecture and domain model introduced in this PR.test/deno_entry.js (1)
1-24: LGTM! Clean test aggregation for Deno.The bootstrap file correctly imports the shim and aggregates all test modules for consolidated Deno execution. The organization is clear and the approach aligns well with the multi-runtime testing strategy.
test/domain/services/EnvironmentPolicy.test.js (1)
1-45: Excellent test coverage for EnvironmentPolicy.The tests comprehensively verify the filtering behavior:
- Non-whitelisted variables are removed
- Whitelisted identity/localization variables are preserved
- Edge cases (empty/undefined) are handled
The test structure is clean and the assertions are appropriate.
src/domain/services/EnvironmentPolicy.js (1)
42-50: Clean and efficient filtering implementation.The
filter()method is straightforward and correct. The use of a default parameter forenvhandles undefined input gracefully, and the loop-based filtering is efficient for the whitelist size.README.md (2)
29-29: Version references still inconsistent with package.json.As previously noted, the README references "Version 2.0.0" and "Version 2.2.0", but package.json shows "1.0.0". Please align these version references.
Likely an incorrect or invalid review comment.
92-106: Import example still doesn't match package.json exports.As previously noted, the example imports
GitSha,GitRef, andGitSignaturefrom the root, but package.json exposes these at separate subpaths. Either update the exports or fix the example to use:import { GitSha } from '@git-stunts/plumbing/sha'; import { GitRef } from '@git-stunts/plumbing/ref'; import { GitSignature } from '@git-stunts/plumbing/signature';Likely an incorrect or invalid review comment.
test/domain/services/GitErrorClassifier.test.js (1)
1-56: LGTM! Comprehensive test coverage for error classification.The test suite thoroughly validates GitErrorClassifier behavior across all key scenarios: lock detection (index.lock and other .lock files), generic failures, and edge cases like code 128 without lock indicators. The shared
baseOptionspattern keeps tests DRY while maintaining clarity.test/domain/services/CommandSanitizer.test.js (1)
1-35: LGTM! Solid coverage of core sanitization behavior.The tests effectively validate whitelisting, prohibited flags (with detailed error message checks), and dynamic registration. The assertions properly distinguish between ValidationError and ProhibitedFlagError cases.
test/GitCommitFlow.test.js (1)
1-58: LGTM! Comprehensive end-to-end commit orchestration test.The test thoroughly validates the full commit flow: repository initialization, commit creation with both text and binary files, SHA verification, commit message persistence, and ref updates. The inclusion of binary content (
Uint8Array) is particularly valuable for ensuring content handling works across different data types.test/domain/services/GitPersistenceService.test.js (1)
1-79: LGTM! Excellent test coverage for persistence operations.The test suite comprehensively validates all three persistence operations (writeBlob, writeTree, writeCommit) with proper verification of both returned SHAs and persisted content. The progressive complexity (blob → tree → commit) demonstrates realistic usage patterns, and the commit test thoroughly validates signature formatting in the git object.
src/domain/errors/ProhibitedFlagError.js (1)
1-20: LGTM! Excellent error messaging with clear remediation guidance.The error class provides an exceptionally helpful message that explains not just what was detected, but why it's prohibited, what alternatives to use, and where to find more information. This level of detail significantly improves developer experience when encountering security-related restrictions.
test/domain/services/GitCommandBuilder.test.js (1)
1-59: LGTM! Comprehensive test coverage for GitCommandBuilder.The test suite effectively validates the fluent builder API, static factory methods, and command construction logic. All assertions match the expected behavior documented in the implementation.
src/ports/GitPersistencePort.js (1)
1-10: LGTM! Clean port definition.The GitPersistencePort typedef clearly defines the interface contract for Git object persistence operations with appropriate type annotations.
src/domain/services/GitErrorClassifier.js (1)
55-57: LGTM! Correct retry detection logic.The isRetryable method correctly identifies only GitRepositoryLockedError instances as retryable, which aligns with the intended retry strategy.
src/infrastructure/adapters/node/NodeShellRunner.js (1)
18-21: LGTM! Proper environment filtering and merging.The environment handling correctly applies EnvironmentPolicy filtering to both the base environment and any overrides before merging them.
src/domain/services/ExecutionOrchestrator.js (1)
29-60: LGTM! Core orchestration logic is sound.The retry loop correctly tracks attempts, measures latency, classifies errors, and applies backoff for retryable errors. The success path (code === 0) properly returns trimmed stdout.
src/domain/services/GitPersistenceService.js (3)
15-38: LGTM!The constructor and
persistmethod are well-structured. The instanceof-based dispatch pattern is appropriate for this domain service, and the error handling for unsupported entity types is clear.
45-61: LGTM!The
writeBlobimplementation correctly usesgit hash-object -w --stdinand properly validates the input before execution.
93-121: LGTM!The
writeCommitimplementation correctly constructs thegit commit-treecommand with all necessary parameters (tree SHA, parents, message) and properly sets the Git environment variables for author and committer metadata.src/domain/services/CommandSanitizer.js (3)
12-71: LGTM!The static configuration is well-designed with reasonable security limits, a comprehensive command whitelist, and a dynamic
allow()method for extensibility. The use of a Set for_ALLOWED_COMMANDSprovides efficient lookups.
77-82: LGTM!The constructor properly initializes the caching mechanism with a reasonable default size.
116-155: LGTM!The argument validation loop and cache management are well-implemented. The security checks for prohibited flags and configuration options are thorough, and the LRU-style cache eviction strategy is appropriate.
src/domain/services/GitCommandBuilder.js (3)
8-42: LGTM!The constructor and static factory methods provide a clean, fluent API for building git commands. The pattern is consistent and easy to use.
50-118: LGTM!The fluent flag methods are well-implemented with proper method chaining. Each method correctly appends its flag(s) to the args array and returns
thisfor fluent composition.
125-138: LGTM!The
arg()method correctly handles null/undefined while preserving other falsy values (0, false, ""), andbuild()properly returns a copy to prevent external mutation of the internal state.src/domain/services/GitRepositoryService.js (3)
13-58: LGTM!The constructor demonstrates good dependency injection with a sensible default for the persistence service. The persistence delegation methods provide a clean API surface while maintaining separation of concerns.
66-69: LGTM!The
revParsemethod correctly usesGitCommandBuilderto construct the command and delegates execution to the plumbing service.
78-98: LGTM!The
updateRefanddeleteRefmethods correctly normalize SHA parameters toGitShainstances and properly handle optional parameters. The command construction is accurate for both operations.index.js (4)
76-107: LGTM!The
commitmethod provides excellent end-to-end orchestration, properly coordinating blob/tree/commit persistence and reference updates. The code is well-structured and makes good use of domain entities and the repository service.
118-134: LGTM!The factory methods provide convenient initialization patterns.
createDefaultproperly delegates runtime detection toShellRunnerFactory, andcreateRepositoryprovides a clean high-level API entry point.
140-189: LGTM!The
verifyInstallationmethod provides thorough git binary and repository validation with proper error wrapping. Theexecutemethod correctly delegates to the orchestrator with retry support and telemetry.
200-256: LGTM!The streaming execution methods are well-designed.
executeStreamproperly sanitizes and validates before execution,executeWithStatusprovides useful status information without throwing, and theemptyTreegetter correctly returns the canonical empty tree SHA.
- Update GitStream.collect() for performance and manual cleanup - Replace FinalizationRegistry with try...finally in GitStream - Consolidate GitSha validation into GitSha.from() - Optimize ByteMeasurer for Node/Bun/Deno - Update package.json to v2.0.0 and vitest to v3.0.0 - Update CHANGELOG and README
- Implement EnvironmentPolicy with explicit block on GIT_CONFIG_PARAMETERS - Refactor CommandSanitizer to injectable instance with command caching - Add logic to CommandSanitizer to block global flags before subcommands - Add ShellRunnerFactory.register() for custom adapter injection - Update ProhibitedFlagError to support custom messages
- Implement totalTimeout in ExecutionOrchestrator to override retries - Enhance GitErrorClassifier with customRules and regex lock detection - Extract GitBinaryChecker from GitPlumbing to decouple binary verification - Remove GitRepositoryService circular dependency in GitPlumbing constructor - Update CommandRetryPolicy with totalTimeout property
- Implement createCommitFromFiles in GitRepositoryService with concurrency limit - Add toMktreeFormat to GitTree entity for encapsulated serialization - Hardened GitPersistenceService to use EnvironmentPolicy on all calls - Expand GitCommandBuilder with fluent factory methods and common flags - Decouple GitRepositoryService from GitPlumbing constructor
- Fix index.js exports to include all core entities and services - Add Prerequisites section to README.md (Git >= 2.30.0) - Update README.md Usage examples to new object-based commit syntax - Create CODE_OF_CONDUCT.md (Contributor Covenant) - Create docs/CUSTOM_RUNNERS.md (CommandRunner port contract) - Update docs/RECIPES.md with 'Commit from Scratch' plumbing guide
- Update run-multi-runtime-tests.sh to detect and use 'docker compose' (V2) or 'docker-compose' (V1) - Add 'docker compose down' to cleanup after test runs - Document fix in CHANGELOG
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @index.js:
- Around line 5-6: The file reintroduces Node-specific imports "node:path" and
"node:fs" and uses them for CWD validation, breaking multi-runtime
compatibility; remove those direct imports and instead accept a filesystem
adapter or make CWD validation optional: update the module to remove
node:path/node:fs usage, add a fsAdapter parameter (or accept an optional
validateCwd flag) to the constructor where CWD validation currently runs, move
the actual path/fs calls into a runtime-specific ShellRunnerFactory or a new
validateCwd() method that runtime factories call, and ensure any code that
relied on the removed imports calls into the injected adapter (e.g., use
fsAdapter.stat/exists and path utilities provided by the adapter) so
Bun/Deno-compatible factories can supply their own implementations.
In @src/domain/entities/GitCommit.js:
- Around line 28-48: The constructor for GitCommit currently skips validation
for parents and message; update GitCommit.constructor to (1) ensure parents is
provided and iterable (default to [] if null/undefined), (2) assert every
element of parents is a GitSha instance and throw a ValidationError('parents
must be an array of GitSha', 'GitCommit.constructor') if not, and (3) validate
message is a string (throw ValidationError('message must be a string',
'GitCommit.constructor') if invalid); make sure these checks occur before using
[...parents] and that isRoot() and isMerge() will safely rely on parents.length
afterwards.
In @src/domain/services/CommandSanitizer.js:
- Around line 122-143: The code calls string methods on args elements before
validating their type, so update the sanitize logic to validate each arg is a
string (and throw the intended ValidationError) before any use of .startsWith()
or .toLowerCase(); specifically, in the loop that sets subcommandIndex and in
the subsequent loop that checks GLOBAL_FLAGS, add an early type check for
args[i] and throw ValidationError for non-strings, then remove the later
duplicate type check around lines where ValidationError is currently raised;
reference symbols: args, subcommandIndex, CommandSanitizer.GLOBAL_FLAGS,
ProhibitedFlagError, and ValidationError when implementing the change.
In @src/domain/services/ExecutionOrchestrator.js:
- Around line 61-62: The backoff delay uses getDelay(attempt + 1) which shifts
delays one step later; in the retry branch inside ExecutionOrchestrator where
you check this.classifier.isRetryable(error) and compute backoff, call
retryPolicy.getDelay(attempt) instead so the delay corresponds to the current
retry attempt (use the existing attempt variable rather than attempt + 1).
- Around line 35-89: The orchestrate loop can fall through and return undefined
if all attempts are exhausted; add an explicit throw after the while loop to
fail deterministically: after the loop in ExecutionOrchestrator.orchestrate
throw a GitPlumbingError (or the existing error type used in the catch) with a
message like "Max retry attempts exhausted", include context { args, traceId }
and mark operation 'ExecutionOrchestrator.orchestrate' so callers get a clear
failure instead of undefined; ensure this uses the same GitPlumbingError
constructor pattern used in the catch block and references
retryPolicy.maxAttempts for the message if desired.
In @src/domain/services/GitPersistenceService.js:
- Around line 106-118: The environment values for GIT_AUTHOR_DATE and
GIT_COMMITTER_DATE are currently just the Unix timestamp
(commit.timestamp.toString()) and must include a timezone offset per Git's
documented format; update the code building the env object (where
EnvironmentPolicy.filter is called and GIT_AUTHOR_DATE / GIT_COMMITTER_DATE are
set) to append an appropriate timezone offset (e.g., " <unix-timestamp> +0000")
or compute the offset from the commit or runtime timezone, then pass that string
into plumbing.execute so the dates follow "<unix-timestamp> <timezone-offset>"
format.
In @test/domain/entities/GitTree.test.js:
- Around line 26-29: Rename the failing test's description to match the asserted
value: update the it(...) description in GitTree.test.js for the case
constructing new GitTree(null, []) so it references "returns single newline for
empty tree mktree format" (or equivalent) to reflect that toMktreeFormat()
returns '\n' rather than an empty string; keep the assertion and the use of
GitTree and its toMktreeFormat method unchanged.
🧹 Nitpick comments (8)
CODE_OF_CONDUCT.md (1)
37-37: Format bare URLs as markdown links for proper rendering.Lines 37, 43, and 47 contain bare URLs that should be wrapped in markdown link syntax for consistency and linting compliance.
🔗 Proposed markdown formatting fixes
Instances of abusive, harassing, or otherwise unacceptable behavior may be reported by contacting the project team at james@flyingrobots.dev. All complaints will be reviewed and investigated and will result in a response that is deemed necessary and appropriate to the circumstances. The project team is obligated to maintain confidentiality with regard to the reporter of an incident. Further details of specific enforcement policies may be posted separately.-This Code of Conduct is adapted from the [Contributor Covenant][homepage], version 1.4, available at https://www.contributor-covenant.org/version/1/4/code-of-conduct.html +This Code of Conduct is adapted from the [Contributor Covenant][homepage], version 1.4, available at [https://www.contributor-covenant.org/version/1/4/code-of-conduct.html](https://www.contributor-covenant.org/version/1/4/code-of-conduct.html)-For answers to common questions about this code of conduct, see https://www.contributor-covenant.org/faq +For answers to common questions about this code of conduct, see [https://www.contributor-covenant.org/faq](https://www.contributor-covenant.org/faq)Also applies to: 43-43, 47-47
src/domain/services/GitCommandBuilder.js (1)
69-72: Consider disambiguating overloaded-pflag.Both
pretty()andparent()add the-pflag, which serves different purposes in different Git commands (pretty-print vs. parent). While the inline comment acknowledges this, it could lead to user confusion or incorrect command construction.Consider either:
- Renaming one method to be more specific (e.g.,
prettyPrint())- Adding validation to prevent both being used together
- Documenting this clearly in the method JSDoc
Also applies to: 107-110
test/domain/services/ExecutionOrchestrator.test.js (1)
30-63: Clarify the misleading comments.
Line 31 says "Mock classifier to always return a retryable error" but you're using the real
GitErrorClassifier, which happens to classifycode: 128+stderr: 'index.lock'as retryable. This works but the comment is misleading.Line 62's comment states "500ms backoff" but with
backoffFactor=2andinitialDelayMs=500,getDelay(2)computes2^1 * 500 = 1000ms. The test still passes because 1000ms > 600ms, but the explanation is incorrect.📝 Suggested comment fixes
- // Mock classifier to always return a retryable error - const classifier = new GitErrorClassifier(); + // Use real classifier - code 128 + 'index.lock' triggers GitRepositoryLockedError (retryable) + const classifier = new GitErrorClassifier();- expect(attempts).toBe(1); // Should have stopped after 1st attempt because 500ms backoff would exceed 600ms total + expect(attempts).toBe(1); // Should have stopped after 1st attempt because 1000ms backoff (2^1 * 500) would exceed 600ms totalsrc/domain/entities/GitTreeEntry.js (1)
53-67: Minor inconsistency in delegation pattern.
isTree()delegates directly tothis.mode.isTree(), whileisBlob()goes throughthis.type().isBlob(). Both work correctly, but for consistency you could usethis.mode.getObjectType().isBlob()or add anisBlob()method toGitFileMode.This is purely stylistic and doesn't affect correctness.
src/domain/value-objects/CommandRetryPolicy.js (1)
18-27: Consider validating other constructor parameters.Only
maxAttemptsis validated. Negative or zero values forinitialDelayMs,backoffFactor, ortotalTimeoutcould cause unexpected behavior (e.g., negative delays, infinite loops if backoffFactor <= 0).🛡️ Optional defensive validation
constructor({ maxAttempts = 3, initialDelayMs = 100, backoffFactor = 2, totalTimeout = 30000 } = {}) { if (maxAttempts < 1) { throw new InvalidArgumentError('maxAttempts must be at least 1', 'CommandRetryPolicy.constructor'); } + if (initialDelayMs < 0) { + throw new InvalidArgumentError('initialDelayMs must be non-negative', 'CommandRetryPolicy.constructor'); + } + if (backoffFactor <= 0) { + throw new InvalidArgumentError('backoffFactor must be positive', 'CommandRetryPolicy.constructor'); + } + if (totalTimeout !== null && totalTimeout < 0) { + throw new InvalidArgumentError('totalTimeout must be non-negative or null', 'CommandRetryPolicy.constructor'); + } this.maxAttempts = maxAttempts;src/domain/value-objects/GitSha.js (1)
88-94: Consider caching the EMPTY_TREE instance.The getter creates a new
GitShainstance on every access. Since this is a constant value andGitShais immutable, caching would be more efficient ifEMPTY_TREEis accessed frequently.💡 Optional: Cache the singleton
+ static _emptyTreeInstance = null; + /** * Empty tree SHA constant * @returns {GitSha} */ static get EMPTY_TREE() { - return new GitSha(GitSha.EMPTY_TREE_VALUE); + if (!GitSha._emptyTreeInstance) { + GitSha._emptyTreeInstance = new GitSha(GitSha.EMPTY_TREE_VALUE); + } + return GitSha._emptyTreeInstance; }src/domain/services/GitRepositoryService.js (2)
54-70: Recursive approach works but risks stack overflow for very large file sets.The recursive
processBatchfunction processes files in batches ofconcurrencysize. With the defaultconcurrency=10and thousands of files, the recursion depth could be significant (e.g., 10,000 files = 1,000 recursive calls).Consider an iterative approach for better scalability.
♻️ Iterative alternative
- const processBatch = async () => { - const batch = remainingFiles.splice(0, concurrency); - if (batch.length === 0) {return;} - - const batchResults = await Promise.all(batch.map(async (file) => { - const blob = GitBlob.fromContent(file.content); - const sha = await this.writeBlob(blob); - return new GitTreeEntry({ - path: file.path, - sha, - mode: file.mode || '100644' - }); - })); - - entries.push(...batchResults); - await processBatch(); - }; - - await processBatch(); + while (remainingFiles.length > 0) { + const batch = remainingFiles.splice(0, concurrency); + const batchResults = await Promise.all(batch.map(async (file) => { + const blob = GitBlob.fromContent(file.content); + const sha = await this.writeBlob(blob); + return new GitTreeEntry({ + path: file.path, + sha, + mode: file.mode || '100644' + }); + })); + entries.push(...batchResults); + }
41-95: Consider validating required parameters increateCommitFromFiles.The method lacks input validation for critical parameters. Passing invalid data (e.g.,
filesas non-array,messageas non-string, missingauthor/committer) will cause cryptic errors deep in the call stack rather than clear validation errors upfront.♻️ Proposed validation
async createCommitFromFiles({ branch, message, author, committer, parents, files, concurrency = 10 }) { + if (!Array.isArray(files)) { + throw new ValidationError('files must be an array', 'GitRepositoryService.createCommitFromFiles'); + } + if (!Array.isArray(parents)) { + throw new ValidationError('parents must be an array', 'GitRepositoryService.createCommitFromFiles'); + } + if (typeof message !== 'string') { + throw new ValidationError('message must be a string', 'GitRepositoryService.createCommitFromFiles'); + } const entries = [];Note: You would need to import
ValidationErrorat the top of the file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
CHANGELOG.mdCODE_OF_CONDUCT.mdREADME.mddocs/CUSTOM_RUNNERS.mddocs/RECIPES.mdindex.jspackage.jsonscripts/run-multi-runtime-tests.shsrc/domain/entities/GitBlob.jssrc/domain/entities/GitCommit.jssrc/domain/entities/GitCommitBuilder.jssrc/domain/entities/GitTree.jssrc/domain/entities/GitTreeEntry.jssrc/domain/errors/ProhibitedFlagError.jssrc/domain/services/ByteMeasurer.jssrc/domain/services/CommandSanitizer.jssrc/domain/services/EnvironmentPolicy.jssrc/domain/services/ExecutionOrchestrator.jssrc/domain/services/GitBinaryChecker.jssrc/domain/services/GitCommandBuilder.jssrc/domain/services/GitErrorClassifier.jssrc/domain/services/GitPersistenceService.jssrc/domain/services/GitRepositoryService.jssrc/domain/value-objects/CommandRetryPolicy.jssrc/domain/value-objects/GitSha.jssrc/infrastructure/GitStream.jstest/GitSha.test.jstest/domain/entities/GitCommit.test.jstest/domain/entities/GitCommitBuilder.test.jstest/domain/entities/GitTree.test.jstest/domain/services/CommandSanitizer.test.jstest/domain/services/EnvironmentPolicy.test.jstest/domain/services/ExecutionOrchestrator.test.jstest/domain/services/GitErrorClassifier.test.js
🚧 Files skipped from review as they are similar to previous changes (11)
- test/GitSha.test.js
- src/domain/services/EnvironmentPolicy.js
- src/domain/entities/GitBlob.js
- src/domain/entities/GitCommitBuilder.js
- docs/RECIPES.md
- test/domain/entities/GitCommit.test.js
- src/domain/services/GitErrorClassifier.js
- test/domain/services/CommandSanitizer.test.js
- test/domain/services/GitErrorClassifier.test.js
- test/domain/services/EnvironmentPolicy.test.js
- src/domain/entities/GitTree.js
🧰 Additional context used
🧬 Code graph analysis (15)
src/domain/entities/GitCommit.js (5)
src/domain/value-objects/GitSha.js (1)
GitSha(12-95)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/value-objects/GitSignature.js (1)
GitSignature(11-52)src/domain/schemas/GitCommitSchema.js (2)
GitCommitSchema(8-15)GitCommitSchema(8-15)src/domain/value-objects/GitObjectType.js (1)
GitObjectType(10-198)
src/domain/errors/ProhibitedFlagError.js (1)
src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
src/domain/services/ExecutionOrchestrator.js (2)
src/domain/services/GitErrorClassifier.js (1)
GitErrorClassifier(11-74)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
src/domain/entities/GitTreeEntry.js (4)
src/domain/value-objects/GitFileMode.js (1)
GitFileMode(11-90)src/domain/value-objects/GitSha.js (1)
GitSha(12-95)src/domain/schemas/GitTreeEntrySchema.js (2)
GitTreeEntrySchema(8-12)GitTreeEntrySchema(8-12)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/domain/services/GitRepositoryService.js (7)
src/domain/services/GitPersistenceService.js (1)
GitPersistenceService(16-120)src/domain/entities/GitBlob.js (1)
GitBlob(14-89)src/domain/entities/GitTreeEntry.js (1)
GitTreeEntry(17-80)src/domain/entities/GitTree.js (1)
GitTree(18-127)src/domain/entities/GitCommit.js (1)
GitCommit(18-121)src/domain/services/GitCommandBuilder.js (1)
GitCommandBuilder(9-176)src/domain/value-objects/GitSha.js (1)
GitSha(12-95)
src/domain/value-objects/GitSha.js (3)
test/domain/entities/GitTreeEntry.test.js (1)
sha(7-7)src/domain/schemas/GitShaSchema.js (2)
GitShaSchema(6-9)GitShaSchema(6-9)src/domain/errors/ValidationError.js (1)
ValidationError(10-20)
src/infrastructure/GitStream.js (1)
src/ports/RunnerOptionsSchema.js (2)
DEFAULT_MAX_BUFFER_SIZE(11-11)DEFAULT_MAX_BUFFER_SIZE(11-11)
src/domain/services/GitCommandBuilder.js (1)
test/domain/entities/GitTreeEntry.test.js (1)
sha(7-7)
src/domain/services/GitBinaryChecker.js (1)
src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
src/domain/services/GitPersistenceService.js (7)
src/domain/entities/GitBlob.js (1)
GitBlob(14-89)src/domain/entities/GitTree.js (1)
GitTree(18-127)src/domain/entities/GitCommit.js (1)
GitCommit(18-121)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/services/GitCommandBuilder.js (1)
GitCommandBuilder(9-176)src/domain/value-objects/GitSha.js (1)
GitSha(12-95)src/domain/services/EnvironmentPolicy.js (1)
EnvironmentPolicy(12-69)
index.js (9)
src/domain/services/CommandSanitizer.js (1)
CommandSanitizer(13-191)src/domain/services/ExecutionOrchestrator.js (1)
ExecutionOrchestrator(12-107)src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)src/domain/services/GitRepositoryService.js (1)
GitRepositoryService(17-172)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)src/ports/RunnerOptionsSchema.js (4)
DEFAULT_MAX_BUFFER_SIZE(11-11)DEFAULT_MAX_BUFFER_SIZE(11-11)RunnerOptionsSchema(21-28)RunnerOptionsSchema(21-28)src/domain/value-objects/CommandRetryPolicy.js (1)
CommandRetryPolicy(10-69)src/infrastructure/GitStream.js (1)
GitStream(11-148)src/domain/value-objects/GitSha.js (1)
GitSha(12-95)
test/domain/services/ExecutionOrchestrator.test.js (5)
src/domain/services/ExecutionOrchestrator.js (1)
ExecutionOrchestrator(12-107)src/domain/value-objects/CommandRetryPolicy.js (1)
CommandRetryPolicy(10-69)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)src/domain/services/GitErrorClassifier.js (1)
GitErrorClassifier(11-74)src/domain/errors/GitRepositoryLockedError.js (1)
GitRepositoryLockedError(10-25)
src/domain/value-objects/CommandRetryPolicy.js (1)
src/domain/errors/InvalidArgumentError.js (1)
InvalidArgumentError(10-20)
src/domain/services/CommandSanitizer.js (2)
src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/errors/ProhibitedFlagError.js (1)
ProhibitedFlagError(10-23)
test/domain/entities/GitCommitBuilder.test.js (5)
src/domain/entities/GitTree.js (1)
GitTree(18-127)src/domain/value-objects/GitSignature.js (1)
GitSignature(11-52)src/domain/entities/GitCommitBuilder.js (1)
GitCommitBuilder(12-115)src/domain/entities/GitCommit.js (1)
GitCommit(18-121)src/domain/value-objects/GitSha.js (1)
GitSha(12-95)
🪛 LanguageTool
CODE_OF_CONDUCT.md
[style] ~20-~20: Try using a synonym here to strengthen your wording.
Context: ...vances * Trolling, insulting/derogatory comments, and personal or political attacks * Pu...
(COMMENT_REMARK)
[style] ~37-~37: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...s obligated to maintain confidentiality with regard to the reporter of an incident. Further de...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
🪛 markdownlint-cli2 (0.18.1)
CODE_OF_CONDUCT.md
37-37: Bare URL used
(MD034, no-bare-urls)
43-43: Bare URL used
(MD034, no-bare-urls)
47-47: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (41)
test/domain/entities/GitTree.test.js (2)
9-24: LGTM! Comprehensive mktree format test.The test correctly validates the serialization format with multiple entry types (file and directory), verifying mode, object type, SHA, and path formatting with proper delimiters.
31-41: LGTM! Factory method test is well-structured.The test properly validates the
fromDatafactory method and verifies both the SHA conversion and entry deserialization.docs/CUSTOM_RUNNERS.md (3)
1-41: LGTM! Clear and comprehensive contract documentation.The CommandRunner contract is well-documented with all required parameters, types, and return values. The structure aligns with the streaming-first architecture described in the PR objectives.
43-71: LGTM! Clear and practical example.The SSH runner example demonstrates the registration pattern and usage clearly. The placeholder comments appropriately indicate where implementation details would go without cluttering the documentation.
73-75: LGTM! Clear architectural rationale.The explanation effectively communicates why the streaming-first design was chosen and how it relates to memory safety.
CHANGELOG.md (1)
10-51: LGTM! Comprehensive and well-structured changelog.The changelog entries are detailed, well-organized, and follow the Keep a Changelog format. The categorization (Refactor, Changed, Fixed, Added) makes it easy to understand the scope and nature of changes.
src/domain/services/GitBinaryChecker.js (3)
10-18: LGTM! Clean dependency injection pattern.The constructor properly implements the DI pattern, storing the plumbing dependency for use in verification methods.
25-37: LGTM! Standard verification pattern.The method correctly verifies Git binary availability and provides detailed error context. The
Promise<boolean>return type is accurate (returnstrueon success, throws otherwise), though this "always true or throw" pattern is intentional and clear from the implementation.
44-55: LGTM! Correct work tree detection logic.The method properly handles the three cases: not in a repo (throws), in work tree (returns true), and in repo but not work tree (returns false). The string comparison with
'true'is appropriate for the Git command output.package.json (2)
3-15: LGTM! Well-structured exports map.The version now correctly matches CHANGELOG.md (2.0.0), resolving the previous review concern. The exports map provides a clean public API surface, exposing the main entry point, shell runner, value objects, and errors.
37-37: LGTM! Vitest v3 upgrade verified.The upgrade to Vitest v3 is a major version bump. The PR description confirms that 107 tests passed, indicating compatibility has been verified.
src/infrastructure/GitStream.js (2)
65-97: LGTM: Robust stream collection with OOM protection.The implementation properly enforces byte limits before accumulation, uses efficient chunk assembly, and ensures cleanup in the finally block.
102-131: LGTM: Safe async iterator with consumption guards.The implementation correctly prevents double consumption, prefers native async iteration for better performance, and guarantees cleanup via the finally block.
src/domain/services/GitCommandBuilder.js (1)
162-175: LGTM: Defensive argument handling and immutable build.The
arg()method correctly filters out null/undefined values, andbuild()returns a copy to prevent external mutation of the internal state.index.js (2)
155-174: LGTM: Well-orchestrated command execution with retry logic.The
execute()method properly delegates to the orchestrator, wraps streaming execution, and includes telemetry (trace ID, retry policy). The implementation correctly separates concerns between streaming and buffering.
185-205: LGTM: Clean streaming interface with proper sanitization.The
executeStream()method correctly sanitizes arguments before execution, validates options via Zod schema, and wraps errors consistently inGitPlumbingError.README.md (2)
36-36: Version reference inconsistent with package.json.The README references "Version 2.0.0", but the prior review noted that package.json shows
"version": "1.0.0". This inconsistency remains unresolved.Likely an incorrect or invalid review comment.
108-109: Import example doesn't match package.json exports.The example imports
GitSha,GitRef, andGitSignaturefrom the package root, but package.json exposes these at separate subpaths (./sha,./ref,./signature). This mismatch was flagged in a previous review and remains unresolved.Likely an incorrect or invalid review comment.
test/domain/entities/GitCommitBuilder.test.js (3)
1-12: LGTM! Well-structured test setup.The test fixtures are properly scoped at the
describelevel, and the imports cover all necessary domain entities. The use ofGitTree.empty()for test data is a good pattern.Minor: Line 1 is blank which is unconventional but not problematic.
14-26: Good coverage of the basic builder flow.The test properly validates that the builder produces a
GitCommitinstance and that the tree SHA linkage is correct via.equals().
28-44: Nice test for mixed parent types.Testing both raw string SHA and
GitShainstance as parents verifies the builder's type normalization. The assertions properly check length, string conversion, and equality.test/domain/services/ExecutionOrchestrator.test.js (1)
8-28: Solid test for totalTimeout enforcement.This correctly validates that
_checkTotalTimeoutis invoked afterexecute()completes, catching scenarios where the operation itself exceeds the timeout budget.src/domain/errors/ProhibitedFlagError.js (1)
10-23: LGTM! Clean error class with helpful default message.The implementation follows the established error hierarchy pattern. The default message provides clear remediation guidance (use
cwdoption orGitRepositoryService), and thedetails.messageoverride offers flexibility for custom error scenarios.src/domain/entities/GitTreeEntry.js (1)
24-43: LGTM! Proper validation and normalization.The constructor correctly normalizes inputs to strings for Zod schema validation, then reconstructs typed domain objects. Path validation is handled by
GitTreeEntrySchemawithz.string().min(1).src/domain/value-objects/CommandRetryPolicy.js (1)
34-39: LGTM! Standard exponential backoff implementation.The formula correctly implements exponential backoff with
backoffFactor^(attempt-1) * initialDelayMs, returning 0 for the first attempt.src/domain/value-objects/GitSha.js (1)
12-22: LGTM! Good separation between internal constructor and validated factory.The private constructor enables efficient internal instantiation (e.g., for
EMPTY_TREE), whilestatic from()provides the validated public entry point. This is a solid Value Object pattern.src/domain/services/GitPersistenceService.js (2)
30-39: LGTM! Clean dispatch pattern for entity persistence.The
persist()method properly routes to specialized write methods based on entity type, with a clear error for unsupported types.
69-83: Good refactor usingtoMktreeFormat().This correctly delegates tree entry type detection to
GitTree.toMktreeFormat(), which usesentry.isTree()to determine the type. This addresses the previous review concern about incorrect type detection usingisEmptyTree().src/domain/services/ExecutionOrchestrator.js (3)
12-20: LGTM!Constructor properly initializes with dependency injection for the classifier with a sensible default.
77-87: Catch block logic is now properly implemented.The previous placeholder comments have been replaced with proper logic: re-throwing
GitPlumbingErrorinstances and wrapping unexpected errors with metadata.
95-106: LGTM!The
_checkTotalTimeouthelper properly validates elapsed time and throws a descriptive error with relevant metadata.src/domain/entities/GitCommit.js (3)
55-73: LGTM!The
fromDatafactory properly validates input viaGitCommitSchemaand constructs properly typed domain objects. This is the safe entry point for untrusted data.
79-105: LGTM!Helper methods
isWritten(),type(),isRoot(), andisMerge()are correctly implemented given proper constructor validation is added.
111-120: LGTM!The
toJSON()method correctly serializes all fields using proper value object methods.src/domain/services/GitRepositoryService.js (3)
17-26: LGTM!Constructor properly sets up dependency injection with a default persistence service.
97-131: LGTM!The
save,writeBlob,writeTree, andwriteCommitmethods correctly delegate to the persistence service.
139-171: LGTM!The
revParse,updateRef, anddeleteRefmethods properly useGitCommandBuilderand handleGitShatype coercion correctly.src/domain/services/CommandSanitizer.js (4)
13-47: LGTM!The static configuration with whitelisted commands, prohibited flags, and global flags provides a solid defense-in-depth foundation.
79-92: LGTM!The
allow()static method and constructor with LRU cache configuration are well-implemented.
69-73: Case-sensitivity inconsistency between GLOBAL_FLAGS values.
GLOBAL_FLAGScontains-C(uppercase) and-c(lowercase), but the comparison at line 137 uses.toLowerCase(). This works, but-Cand-care distinct Git flags (-Cchanges directory,-csets config). The lowercase comparison at line 137 will block both-Cand-cwhen only-cis intended in the config context.If both should be blocked, this is fine. If
-Cshould be allowed elsewhere, consider separating the checks.
182-187: LGTM on cache management.The LRU-like eviction by deleting the first (oldest) Map entry is a reasonable simple approach for bounded cache size.
- NodeShellRunner: Guard timeout setup to prevent immediate process kill - Dockerfile.deno: Pin Deno version to 2.6.3 for reproducible builds - GitObjectType: Fix STRING_TO_INT mapping for delta types - CommandSanitizer: Add type validation before calling string methods - ExecutionOrchestrator: Fix off-by-one in backoff delay and clean up catch block - GitRefSchema: Refine '@' rules and add control character validation - package.json: Sync version to 2.7.0 and update engines (Node >=22, Deno >=2.0) - CHANGELOG: Sync version and fix release dates - Documentation: Fix imports in COMMIT_LIFECYCLE.md and broken links in errors - CI: Add Node.js setup and dependency install to test job - ESLint: Remove duplicate globals and allow underscore-prefixed unused vars
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/domain/services/ExecutionOrchestrator.js:
- Around line 35-88: The orchestrate method's retry loop can exit without
returning if all attempts fail; add an explicit throw after the while loop to
signal exhausted attempts. After the loop in ExecutionOrchestrator.orchestrate,
throw a GitPlumbingError (consistent with the catch block) that includes a clear
message like "All retry attempts exhausted", the operation name
'ExecutionOrchestrator.orchestrate', and context { args, traceId, attempt,
retryPolicy } (or similar fields) so callers never receive undefined.
- Around line 60-61: The backoff calculation has an off-by-one: since the retry
loop already increments the attempt counter before this check (attempt is
1,2,3...), calling retryPolicy.getDelay(attempt + 1) computes the next attempt's
delay; change the call to retryPolicy.getDelay(attempt) so the delay corresponds
to the current retry attempt (adjust the expression in the block guarded by
this.classifier.isRetryable(error) to use attempt instead of attempt + 1), and
run/adjust any related tests that assert expected backoff timings.
🧹 Nitpick comments (1)
eslint.config.js (1)
26-41: Consider removing redundant test global declarations.Most of the explicitly defined test globals (lines 31-37) are already included in
globals.jestfrom line 30. You could simplify by relying on the spread and only explicitly defining globals that aren't in the Jest preset (such asfailif it's not included).♻️ Potential simplification
If
failis the only non-standard Jest global needed, you could simplify to:{ files: ['test/**/*.js', 'test.js', '**/*.test.js'], languageOptions: { globals: { ...globals.jest, - describe: 'readonly', - it: 'readonly', - expect: 'readonly', - beforeEach: 'readonly', - afterEach: 'readonly', - beforeAll: 'readonly', - afterAll: 'readonly', fail: 'readonly' } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/ci.ymlCHANGELOG.mdDockerfile.denodocs/COMMIT_LIFECYCLE.mdeslint.config.jspackage.jsonsrc/domain/errors/GitRepositoryLockedError.jssrc/domain/schemas/GitRefSchema.jssrc/domain/services/CommandSanitizer.jssrc/domain/services/ExecutionOrchestrator.jssrc/domain/value-objects/GitObjectType.jssrc/infrastructure/adapters/bun/BunShellRunner.jssrc/infrastructure/adapters/node/NodeShellRunner.jstest.jstest/GitRef.test.js
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/COMMIT_LIFECYCLE.md
- src/infrastructure/adapters/bun/BunShellRunner.js
- .github/workflows/ci.yml
- Dockerfile.deno
- src/infrastructure/adapters/node/NodeShellRunner.js
- src/domain/value-objects/GitObjectType.js
- src/domain/errors/GitRepositoryLockedError.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/domain/services/ExecutionOrchestrator.js (2)
src/domain/services/GitErrorClassifier.js (1)
GitErrorClassifier(11-74)src/domain/errors/GitPlumbingError.js (1)
GitPlumbingError(8-15)
test.js (2)
index.js (1)
GitPlumbing(55-242)src/domain/services/GitRepositoryService.js (1)
GitRepositoryService(17-172)
src/domain/services/CommandSanitizer.js (3)
src/domain/errors/ValidationError.js (1)
ValidationError(10-20)src/domain/errors/ProhibitedFlagError.js (1)
ProhibitedFlagError(10-23)src/domain/schemas/GitRefSchema.js (1)
prohibited(17-17)
🔇 Additional comments (10)
eslint.config.js (1)
10-15: Good fix—test globals correctly scoped now.The duplicate test globals flagged in the previous review have been removed from the main configuration. Test globals are now only declared in the test-specific configuration block, preventing them from leaking into non-test files.
CHANGELOG.md (2)
1-127: Version synchronization successfully resolved.The version mismatch flagged in previous reviews has been fixed—both CHANGELOG.md and package.json now show version 2.7.0. Well done!
8-113: Clarify identical release dates for multiple versions.Eight versions (2.7.0 through 1.1.0) share the release date 2026-01-07, which is unusual. If this is a retroactive changelog documenting logical feature groupings from a single large refactor (as suggested by the PR scope), consider adding a note at the top explaining this approach. Otherwise, verify these dates reflect actual release history.
package.json (3)
16-20: Runtime version constraints successfully updated.The engine constraints now align with current stable releases (Node >=22.0.0, Deno >=2.0.0, Bun >=1.3.5), addressing the major issue flagged in previous reviews.
37-37: Verify vitest 3.0.0 compatibility.The major version upgrade from vitest 2.x to 3.0.0 may introduce breaking changes in the test suite. Confirm that all tests pass with this version, particularly the multi-runtime test scenarios.
Based on learnings, the user has already noted "107 tests passed in parallel across Node, Bun, and Deno via Docker" in the PR objectives, but it's worth confirming this holds with vitest 3.0.0 specifically.
7-15: Export paths are all valid and present.All seven exported module paths in package.json exist and are correctly located, with no missing files.
src/domain/services/CommandSanitizer.js (2)
122-131: Type validation properly implemented.The code correctly validates that each argument is a string (line 124) before calling
.startsWith()(line 127). This addresses previous concerns about type validation.
148-157: Command type validation correctly implemented.The code validates that
commandArgis a string (line 150) before calling.toLowerCase()(line 154), properly addressing the previous review feedback.src/domain/schemas/GitRefSchema.js (1)
27-27: Git ref@restriction correctly implemented.The validation now properly allows
@in refs likerefs/heads/feature@workwhile correctly blocking the standalone@and the reflog sequence@{. This addresses the previous review feedback about overly restrictive validation.test/GitRef.test.js (1)
1-233: Comprehensive test coverage for GitRef.The test suite thoroughly covers all aspects of the GitRef value object, including validation rules, static factories, instance methods, and edge cases. The test at lines 70-72 specifically validates that
@is allowed in valid contexts, confirming the fix to the schema validation.
- Update CHANGELOG with recent bug fixes and refinements - Sync version references in README.md to 2.7.0 - Update README.md and CONTRIBUTING.md with accurate runtime prerequisites - Final check of local test suite
…hestrator - Fix off-by-one error in ExecutionOrchestrator backoff calculation (use attempt instead of attempt + 1) - Add explicit throw after retry loop in orchestrate method to handle exhausted attempts - Adjust tests to match new backoff behavior and verify safety throw
- Remove Node-specific imports from index.js for cross-runtime compatibility - Implement optional fsAdapter pattern for CWD validation - Hardened GitCommit constructor validation for parents and message - Fix Git date format in GitPersistenceService by adding timezone offset - Simplify eslint.config.js and update test descriptions
…lity - ByteMeasurer: Add support for ArrayBuffer and TypedArrays with explicit type validation - Sync package.json and CHANGELOG versions - Finalize documentation refinements
…untime support - Normalize GitCommit constructor to handle plain objects for author/parents - Restore multi-runtime compatibility by removing direct Node imports from index.js - Sync versioning across CHANGELOG, README, and package.json - Finalize documentation and test descriptions
Description
This PR implements a robust Hexagonal Architecture for the Git Stunts Plumbing library, enabling first-class support for Node.js, Bun, and Deno runtimes.
Key Changes
GitSignature,GitFileMode, and improvedGitSha/GitRefvalue objects.CommandSanitizerto prevent argument injection attacks.Verification
Passed 107 tests in parallel across Node, Bun, and Deno via Docker.
npm testSummary by CodeRabbit
New Features
New APIs
Bug Fixes
Documentation
Infrastructure
Tests
✏️ Tip: You can customize this high-level summary in your review settings.