Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/git/diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ export function getUnstagedDiff(): DiffResult {
}


export function commit(message: string, body?: string): string {
export interface CommitResult {
raw: string;
hash?: string;
summary?: string;
}

export function commit(message: string, body?: string): CommitResult {
const fullMessage = body ? `${message}\n\n${body}` : message;
const tmpFile = join(tmpdir(), `commit-echo-msg-${process.pid}-${Date.now()}.txt`);
try {
Expand All @@ -51,7 +57,14 @@ export function commit(message: string, body?: string): string {
const detail = [result.stderr, result.stdout].filter(Boolean).join('\n').trim();
throw new Error(detail || `git commit exited with code ${result.status}`);
}
return result.stdout;
const raw = result.stdout;
// Parse "[branch hash] summary" or "[branch (extra) hash] summary" pattern
const match = raw.match(/\[\S+(?:\s+\([^)]+\))?\s+([a-f0-9]+)\]\s+(.+)/);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.

Suggested change
const match = raw.match(/\[\S+(?:\s+\([^)]+\))?\s+([a-f0-9]+)\]\s+(.+)/);
const match = raw.match(/\[[^\]]+?\s+([a-f0-9]+)\]\s+(.+)/);

See Issue in Codacy

return {
raw,
hash: match?.[1],
summary: match?.[2],
};
} finally {
try { unlinkSync(tmpFile); } catch {}
}
Expand Down
150 changes: 150 additions & 0 deletions tests/git-diff.test.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
import assert from 'node:assert/strict';
import test from 'node:test';
import { mkdtempSync, writeFileSync } from 'node:fs';
import { join } from 'node:path';
import { tmpdir } from 'node:os';
import { execSync, spawnSync } from 'node:child_process';

import { checkGitRepo, getStagedDiff, getUnstagedDiff, commit, getRepoRoot } from '../dist/git/diff.js';

function createGitRepo() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.

const dir = mkdtempSync(join(tmpdir(), 'commit-echo-test-'));
execSync('git init', { cwd: dir, stdio: 'pipe' });
execSync('git config user.email test@test.com', { cwd: dir, stdio: 'pipe' });
execSync('git config user.name Test', { cwd: dir, stdio: 'pipe' });
return dir;
}

function createFile(dir, name, content) {
const filePath = join(dir, name);
writeFileSync(filePath, content, 'utf-8');
return filePath;
}

function gitAdd(dir) {
execSync('git add -A', { cwd: dir, stdio: 'pipe' });
}

function gitCommit(dir, msg) {
spawnSync('git', ['commit', '-m', msg], { cwd: dir, stdio: 'pipe' });
}

const originalDir = process.cwd();

test('checkGitRepo() returns successfully inside a git repo', () => {
const dir = createGitRepo();
try {
process.chdir(dir);
checkGitRepo(); // should not throw
assert.ok(true);
} finally {
process.chdir(originalDir);
}
});

test('checkGitRepo() throws outside a git repo', () => {
const dir = mkdtempSync(join(tmpdir(), 'commit-echo-test-'));
try {
process.chdir(dir);
assert.throws(() => checkGitRepo(), /not a git repository/);
} finally {
process.chdir(originalDir);
}
});

test('getStagedDiff() returns diff when changes are staged', () => {
const dir = createGitRepo();
createFile(dir, 'test.txt', 'hello');
gitAdd(dir);
try {
process.chdir(dir);
const result = getStagedDiff();
assert.equal(result.staged, true);
assert.equal(result.hasChanges, true);
assert.ok(result.diff.includes('test.txt'));
} finally {
process.chdir(originalDir);
}
});

test('getStagedDiff() returns empty when nothing is staged', () => {
const dir = createGitRepo();
createFile(dir, 'test.txt', 'hello');
try {
process.chdir(dir);
const result = getStagedDiff();
assert.equal(result.hasChanges, false);
assert.equal(result.diff, '');
} finally {
process.chdir(originalDir);
}
});

test('getUnstagedDiff() returns diff for unstaged changes', () => {
const dir = createGitRepo();
createFile(dir, 'tracked.txt', 'original');
gitAdd(dir);
gitCommit(dir, 'initial');
writeFileSync(join(dir, 'tracked.txt'), 'modified', 'utf-8');
try {
process.chdir(dir);
const result = getUnstagedDiff();
assert.equal(result.hasChanges, true);
assert.equal(result.staged, false);
assert.ok(result.diff.includes('tracked.txt'));
} finally {
process.chdir(originalDir);
}
});

test('commit() commits staged changes and returns hash', () => {
const dir = createGitRepo();
createFile(dir, 'test.txt', 'hello');
gitAdd(dir);
try {
process.chdir(dir);
const result = commit('feat: add test file');
assert.ok(result.hash, 'commit should return a hash');
assert.equal(result.hash.length, 7);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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');

assert.ok(result.summary);
assert.ok(result.summary.includes('feat: add test file'));
assert.ok(result.raw.length > 0);
} finally {
process.chdir(originalDir);
}
});

test('commit() with body returns hash and summary', () => {
const dir = createGitRepo();
createFile(dir, 'hello.txt', 'world');
gitAdd(dir);
try {
process.chdir(dir);
const result = commit('feat: add hello', 'This is the body');
assert.ok(result.hash);
assert.ok(result.summary.includes('feat: add hello'));
} finally {
process.chdir(originalDir);
}
});

test('getRepoRoot() returns the absolute path', () => {
const dir = createGitRepo();
try {
process.chdir(dir);
const root = getRepoRoot();
assert.equal(root, dir);
} finally {
process.chdir(originalDir);
}
});

test('commit() throws with error message when nothing is staged', () => {
const dir = createGitRepo();
try {
process.chdir(dir);
assert.throws(() => commit('feat: no changes'), /nothing to commit/);
} finally {
process.chdir(originalDir);
}
});