You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Added comprehensive unit tests for all exported functions in src/git/diff.ts using Node's built-in node:test and node:assert/strict.
Why
Git operations are central to the tool but had no test coverage. These tests validate the core git interaction layer in isolated temporary repositories.
Test coverage
checkGitRepo() — returns successfully inside a git repo, throws outside
getStagedDiff() — returns diff when staged, empty when nothing staged
getUnstagedDiff() — detects unstaged changes on tracked files
commit() — returns hash and summary, works with body, throws on empty commit
getRepoRoot() — returns the absolute path of the repository root
Testing
npm run build && node --test tests/git-diff.test.mjs — all 9 tests pass
Summary by CodeRabbit
Bug Fixes
Improved commit success messages by displaying concise commit summaries.
Improved handling of large staged and unstaged changes.
Added more reliable repository, branch, and commit detection, including detached HEAD scenarios.
Tests
Added comprehensive coverage for Git repository operations, commit creation, diff handling, and edge cases.
Covers all exported functions: checkGitRepo, getStagedDiff,
getUnstagedDiff, commit, and getRepoRoot. Each test creates
isolated temporary git repos via mkdtempSync + git init.
The reason will be displayed to describe this comment to others. Learn more.
Review: PR #76 — test: add unit tests for src/git/diff.ts
Author
lb1192176991-lab
Base → Head
main → feat/git-diff-tests
Files changed
2 (+165 / -2)
State
Open
Overview ✅
The PR title and description clearly explain what (add unit tests for src/git/diff.ts) and why (git operations are central but had no coverage). Scope is focused — tests plus a minor refactor to the commit() return type. Size is manageable at 165 additions.
Correctness & Logic — ⚠️ Blocking
1. commit() return type change breaks all callers
The function signature changed from return string to return CommitResult (an object), but the two call sites in src/commands/suggest.ts were not updated:
Line 149 (auto path): result.trim() — .trim() on an object will throw at runtime.
Line 210 (interactive path): result.trim() — same issue.
Both need to become result.raw.trim() (or result.raw).
2. Existing test will break
tests/commit-echo.test.mjs does console.log(out) where out is the return value of commit(). With the new object return type, this prints [object Object] instead of the raw git output string, causing assertions like res.stdout.includes(title) to fail. The test runner needs console.log(out.raw).
Code Quality & Maintainability — Comment
3. Regex parsing of git output is fragile
The regex /[\[\S+(?:\s+\([^)]+\))?\s+([a-f0-9]+)\]\s+(.+)/ targets the standard [branch hash] summary format, but git output varies by detached HEAD state, rebase/merge states, non-English locale, and custom format.pretty overrides. A mismatch silently produces hash: undefined, summary: undefined. Consider adding a fallback when the regex doesn't match.
Testing ✅
The test suite itself is well-structured:
Isolated environments: Each test creates a temp dir, initializes a real git repo, and cleans up via try/finally — clean and reliable.
Coverage: All 5 exported functions are tested (9 tests) covering happy paths and error paths.
Error cases: checkGitRepo() throws outside a repo, commit() throws when nothing is staged, getStagedDiff() returns empty when nothing is staged.
No concerns. No new dependencies, no security-sensitive changes.
Verdict: Request changes
Must fix before merge:
Update src/commands/suggest.ts lines 149 and 210 to use result.raw instead of bare result (since .trim() is called on the value).
Update tests/commit-echo.test.mjs to handle the new CommitResult return type (change console.log(out) to console.log(out.raw)).
Recommended (non-blocking):
3. Add a fallback in commit() when the regex doesn't match the git output format, so hash and summary are explicitly undefined rather than silently failing.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The PR introduces a comprehensive test suite and refactors git utility functions, but several issues prevent it from meeting quality standards. Codacy analysis indicates the PR is not up to standards due to new quality and security issues. Key concerns include a breaking change to the commit function API that is not explicitly documented as such, and the use of fragile regex for parsing human-readable git porcelain output which is susceptible to ReDoS and fails in detached HEAD states. Additionally, the test suite introduces global state side effects and resource leaks that could affect CI stability. Addressing these architectural and security concerns is required before merging.
About this PR
Multiple functions rely on parsing human-readable git output (porcelain). This is fragile as output varies between Git versions and system locales. It is recommended to use plumbing commands (e.g., git rev-parse) which are designed for programmatic consumption.
The refactoring of the commit function introduces a breaking change to the public API by changing the return type from a string to an object. This should be explicitly documented in the PR description to alert consumers.
2 comments outside of the diffsrc/git/diff.ts
line 50 🔴 HIGH RISK
The file path used in writeFileSync is constructed from a variable, which triggers a security audit for potential path traversal. Ensure the resulting path is strictly validated or contained within the intended temporary directory.
line 17 🟡 MEDIUM RISK
Suggestion: Replace the logical OR (||) with the nullish coalescing operator (??) for safer default value assignment when handling errors. Note that if stderr is an empty string, the error message will now be empty.
throw new Error(stderr ?? 'Not a git repository');
Test suggestions
checkGitRepo returns successfully when inside a git repository
checkGitRepo throws an error when executed outside a git repository
getStagedDiff returns a DiffResult with content and hasChanges=true when changes are staged
getStagedDiff returns empty string and hasChanges=false when no changes are staged
getUnstagedDiff detects modifications to tracked files
commit successfully creates a commit and returns the hash and summary line
commit correctly incorporates an optional body into the full commit message
commit throws an error when there are no changes to commit
getRepoRoot returns the correct absolute path to the initialized repository
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
The regular expression used to parse git commit output is both fragile and insecure. It will fail to match branch descriptions in a 'detached HEAD' state (where branch names contain spaces) and is susceptible to ReDoS (Regular Expression Denial of Service) due to its structure. It is safer to use a non-greedy approach or, preferably, use plumbing commands like git rev-parse HEAD for reliable results.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: Using process.chdir() modifies the global state of the Node.js process, which prevents parallel test execution and can cause race conditions. Additionally, temporary directories created with mkdtempSync are not cleaned up, leading to resource leaks. Consider refactoring functions to accept a cwd parameter and using a finally block with fs.rmSync for cleanup.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
Nitpick: Asserting that the hash length is exactly 7 is fragile as short hashes can vary in length. Validate that it is a valid hex string of at least 7 characters instead.
Suggested change
assert.equal(result.hash.length,7);
assert.ok(/^[a-f0-9]{7,}$/.test(result.hash),'commit should return a valid hex hash');
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
Git utilities now return structured commit results, expose repository metadata helpers, normalize repository roots, and support larger diffs. The suggest command displays commit summaries, with comprehensive integration tests covering these behaviors.
Changes
Git commit results and repository helpers
Layer / File(s)
Summary
Git utility contracts and implementation src/git/diff.ts
I’m a rabbit with commits in my paws,
Structured summaries, neat Git claws.
Big diffs hop without a fright,
Tests guard branches day and night. result.summary shines bright!
Resolve merge conflicts in src/git/diff.ts and tests/git-diff.test.mjs
- Keep main's refactored parseCommitOutput() with CommitResult.output
- Keep main's additional functions (hasCommits, getBranchName, getLastCommitMessage)
- Add missing 'commit throws when nothing is staged' test case
- Preserve main's large-diff tests and detached HEAD parsing test
Creating predictable filenames in the shared OS temporary directory (tmpdir()) is vulnerable to symlink attacks (CWE-377). An attacker can pre-create a symlink with the predicted name (since PIDs and timestamps can be brute-forced or spammed) to overwrite arbitrary files when writeFileSync is executed.
Use fs.mkdtempSync to create a securely randomized directory first, and write the temporary file inside it. Note: you will need to import mkdtempSync and rmdirSync from node:fs at the top of the file.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/git/diff.ts` around lines 79 - 96, Replace the predictable tmpdir-based
filename in the commit flow with a securely randomized directory created via
mkdtempSync, then write the temporary commit message file inside that directory.
Import mkdtempSync and rmdirSync, and update the finally cleanup around the
existing writeFileSync/unlinkSync logic to remove both the file and its
temporary directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/git/diff.ts`:
- Around line 79-96: Replace the predictable tmpdir-based filename in the commit
flow with a securely randomized directory created via mkdtempSync, then write
the temporary commit message file inside that directory. Import mkdtempSync and
rmdirSync, and update the finally cleanup around the existing
writeFileSync/unlinkSync logic to remove both the file and its temporary
directory.
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60e09f2f-52d0-46dc-a6bc-e706e7f8ff81
📥 Commits
Reviewing files that changed from the base of the PR and between 50eebed and f61d796.
The project is ESM-only: use .js extensions in local imports and keep verbatimModuleSyntax-compatible TypeScript imports (for example, import type for type-only imports).
Files:
src/commands/suggest.ts
src/git/diff.ts
tests/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.mjs: Use Node.js built-in node:test for test files in tests/ and tests/e2e/; do not use Jest or Mocha.
Write test assertions with node:assert/strict in test files under tests/ and tests/e2e/.
Files:
tests/git-diff.test.mjs
src/git/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Git operations should use execSync/spawnSync rather than simple-git; diff buffering must stay within the 100MB limit, and commits should use temp files via git commit -F.
Files:
src/git/diff.ts
🪛 ast-grep (0.44.1)src/git/diff.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync, spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync, spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync, spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync, spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
withCwd still mutates the process-wide cwd; a prior review already raised this as a medium-risk pattern that prevents parallel test execution and can race if concurrency is ever enabled for this suite. The cleanup-leak half of that same comment (temp dirs not removed) has since been fixed via the try/finally + rmSync blocks throughout this file, but the chdir mutation itself is unchanged.
1-51: LGTM!
64-352: LGTM!
src/git/diff.ts (5)
4-19: LGTM!
29-46: LGTM!
55-64: LGTM!
66-77: LGTM!
99-116: LGTM!
src/commands/suggest.ts (2)
150-150: LGTM!
211-211: LGTM!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
404-Page-Found
left a comment
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
What
Added comprehensive unit tests for all exported functions in
src/git/diff.tsusing Node's built-innode:testandnode:assert/strict.Why
Git operations are central to the tool but had no test coverage. These tests validate the core git interaction layer in isolated temporary repositories.
Test coverage
checkGitRepo()— returns successfully inside a git repo, throws outsidegetStagedDiff()— returns diff when staged, empty when nothing stagedgetUnstagedDiff()— detects unstaged changes on tracked filescommit()— returns hash and summary, works with body, throws on empty commitgetRepoRoot()— returns the absolute path of the repository rootTesting
npm run build && node --test tests/git-diff.test.mjs— all 9 tests passSummary by CodeRabbit
Bug Fixes
Tests