comments handled from upstream

src/gitGraph.ts

@@ -11,6 +11,7 @@ export class GitGraphAnalyzer {
             this.gitCliAvailable = true;
         } catch (e) {
             this.gitCliAvailable = false;
+            core.debug('Git CLI not available during initialization');
         }
     }
 
@@ -21,6 +22,19 @@ export class GitGraphAnalyzer {
         return this.gitCliAvailable;
     }
 
+    /**
+     * Validate that a ref matches expected git reference patterns.
+     * Accepts SHA hashes, branch names, and tag names.
+     */
+    private isValidRef(ref: string): boolean {
+        if (!ref || ref.length === 0) {
+            return false;
+        }
+        // Allow: hex SHA (short or full), branch/tag names (alphanumeric, dots, underscores, hyphens, slashes)
+        const validRefPattern = /^[a-zA-Z0-9._\-/]+$/;
+        return validRefPattern.test(ref);
+    }
+
     /**
      * Get git ancestry using topological order
      */
@@ -30,6 +44,11 @@ export class GitGraphAnalyzer {
             return [];
         }
 
+        if (!this.isValidRef(ref)) {
+            core.warning(`Invalid git ref format: ${ref}`);
+            return [];
+        }
+
         try {
             const result = spawnSync('git', ['log', '--oneline', '--topo-order', ref], {
                 encoding: 'utf8',

test/addBenchmarkEntryGitGraph.test.ts

@@ -16,6 +16,7 @@ jest.mock('../src/gitGraph', () => ({
 
 import { addBenchmarkEntry } from '../src/addBenchmarkEntry';
 import { Benchmark } from '../src/extract';
+import { BenchmarkSuites } from '../src/write';
 
 describe('addBenchmarkEntry with Git Graph', () => {
     const createMockBenchmark = (id: string, timestamp?: string): Benchmark => ({
@@ -82,7 +83,7 @@ describe('addBenchmarkEntry with Git Graph', () => {
     it('should create new benchmark suite when none exists', () => {
         const benchName = 'test-suite';
         const benchEntry = createMockBenchmark('abc123');
-        const entries: any = {};
+        const entries: BenchmarkSuites = {};
 
         mockFindPreviousBenchmark.mockReturnValue(null);
 

test/gitGraphAnalyzer.test.ts

@@ -1,20 +1,34 @@
 // Mock modules before imports
 const mockDebug = jest.fn();
 const mockWarning = jest.fn();
-const mockExecSync = jest.fn();
+const mockSpawnSync = jest.fn();
 
 jest.mock('@actions/core', () => ({
     debug: mockDebug,
     warning: mockWarning,
 }));
 
 jest.mock('child_process', () => ({
-    execSync: mockExecSync,
+    spawnSync: mockSpawnSync,
 }));
 
 import { GitGraphAnalyzer } from '../src/gitGraph';
 import { Benchmark } from '../src/extract';
 
+const createMockBenchmark = (id: string, timestamp: string): Benchmark => ({
+    commit: {
+        id,
+        timestamp,
+        message: `Commit ${id}`,
+        url: `https://github.com/test/repo/commit/${id}`,
+        author: { username: 'testuser' },
+        committer: { username: 'testuser' },
+    },
+    date: Date.now(),
+    tool: 'cargo',
+    benches: [],
+});
+
 describe('GitGraphAnalyzer', () => {
     let analyzer: GitGraphAnalyzer;
     const originalEnv = process.env;
@@ -31,17 +45,17 @@ describe('GitGraphAnalyzer', () => {
 
     describe('constructor', () => {
         it('should detect git CLI availability', () => {
-            mockExecSync.mockReturnValue('git version 2.40.0');
+            mockSpawnSync.mockReturnValue({ stdout: 'git version 2.40.0', error: null });
             analyzer = new GitGraphAnalyzer();
             expect(analyzer.isGitAvailable()).toBe(true);
         });
 
         it('should detect git CLI unavailability', () => {
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') {
+            mockSpawnSync.mockImplementation((cmd: string) => {
+                if (cmd === 'git') {
                     throw new Error('Command not found');
                 }
-                return '';
+                return { stdout: '', error: null };
             });
 
             analyzer = new GitGraphAnalyzer();
@@ -51,22 +65,26 @@ describe('GitGraphAnalyzer', () => {
 
     describe('getAncestry', () => {
         beforeEach(() => {
-            mockExecSync.mockReturnValue('git version 2.40.0');
+            mockSpawnSync.mockReturnValue({ stdout: 'git version 2.40.0', error: null });
             process.env.GITHUB_WORKSPACE = '/github/workspace';
             analyzer = new GitGraphAnalyzer();
         });
 
         it('should parse git log output correctly', () => {
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') return 'git version 2.40.0';
-                return 'abc123 Commit message 1\ndef456 Commit message 2\nghi789 Commit message 3';
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return {
+                    stdout: 'abc123 Commit message 1\ndef456 Commit message 2\nghi789 Commit message 3',
+                    error: null,
+                };
             });
             analyzer = new GitGraphAnalyzer();
 
             const ancestry = analyzer.getAncestry('main');
 
-            expect(mockExecSync).toHaveBeenCalledWith(
-                'git log --oneline --topo-order main',
+            expect(mockSpawnSync).toHaveBeenCalledWith(
+                'git',
+                ['log', '--oneline', '--topo-order', 'main'],
                 expect.objectContaining({
                     encoding: 'utf8',
                     cwd: '/github/workspace',
@@ -76,9 +94,9 @@ describe('GitGraphAnalyzer', () => {
         });
 
         it('should handle empty git log output', () => {
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') return 'git version 2.40.0';
-                return '';
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: '', error: null };
             });
             analyzer = new GitGraphAnalyzer();
 
@@ -87,9 +105,9 @@ describe('GitGraphAnalyzer', () => {
         });
 
         it('should handle git command failure', () => {
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') return 'git version 2.40.0';
-                throw new Error('Git command failed');
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: '', error: new Error('Git command failed') };
             });
             analyzer = new GitGraphAnalyzer();
 
@@ -99,35 +117,75 @@ describe('GitGraphAnalyzer', () => {
         });
 
         it('should return empty array when git CLI not available', () => {
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') {
+            mockSpawnSync.mockImplementation((cmd: string) => {
+                if (cmd === 'git') {
                     throw new Error('Command not found');
                 }
-                return '';
+                return { stdout: '', error: null };
             });
             analyzer = new GitGraphAnalyzer();
 
             const ancestry = analyzer.getAncestry('main');
             expect(ancestry).toEqual([]);
             expect(mockWarning).toHaveBeenCalledWith('Git CLI not available, cannot determine ancestry');
         });
-    });
 
-    describe('findPreviousBenchmark', () => {
-        const createMockBenchmark = (id: string, timestamp: string): Benchmark => ({
-            commit: {
-                id,
-                timestamp,
-                message: `Commit ${id}`,
-                url: `https://github.com/test/repo/commit/${id}`,
-                author: { username: 'testuser' },
-                committer: { username: 'testuser' },
-            },
-            date: Date.now(),
-            tool: 'cargo',
-            benches: [],
+        it('should reject empty ref', () => {
+            const ancestry = analyzer.getAncestry('');
+            expect(ancestry).toEqual([]);
+            expect(mockWarning).toHaveBeenCalledWith('Invalid git ref format: ');
+        });
+
+        it('should reject ref with shell metacharacters', () => {
+            const ancestry = analyzer.getAncestry('main; rm -rf /');
+            expect(ancestry).toEqual([]);
+            expect(mockWarning).toHaveBeenCalledWith('Invalid git ref format: main; rm -rf /');
+        });
+
+        it('should reject ref with backticks', () => {
+            const ancestry = analyzer.getAncestry('`whoami`');
+            expect(ancestry).toEqual([]);
+            expect(mockWarning).toHaveBeenCalledWith('Invalid git ref format: `whoami`');
+        });
+
+        it('should accept valid SHA hash', () => {
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: 'abc123def456 Commit message', error: null };
+            });
+            analyzer = new GitGraphAnalyzer();
+
+            const ancestry = analyzer.getAncestry('abc123def456');
+            expect(ancestry).toEqual(['abc123def456']);
+            expect(mockWarning).not.toHaveBeenCalled();
+        });
+
+        it('should accept valid branch name with slashes', () => {
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: 'abc123 Commit message', error: null };
+            });
+            analyzer = new GitGraphAnalyzer();
+
+            const ancestry = analyzer.getAncestry('feature/my-branch');
+            expect(ancestry).toEqual(['abc123']);
+            expect(mockWarning).not.toHaveBeenCalled();
         });
 
+        it('should accept valid tag name with dots', () => {
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: 'abc123 Commit message', error: null };
+            });
+            analyzer = new GitGraphAnalyzer();
+
+            const ancestry = analyzer.getAncestry('v1.2.3');
+            expect(ancestry).toEqual(['abc123']);
+            expect(mockWarning).not.toHaveBeenCalled();
+        });
+    });
+
+    describe('findPreviousBenchmark', () => {
         beforeEach(() => {
             process.env.GITHUB_WORKSPACE = '/github/workspace';
         });
@@ -139,9 +197,9 @@ describe('GitGraphAnalyzer', () => {
                 createMockBenchmark('ghi789', '2025-01-03T00:00:00Z'),
             ];
 
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') return 'git version 2.40.0';
-                return 'ghi789 Commit 3\ndef456 Commit 2\nabc123 Commit 1';
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: 'ghi789 Commit 3\ndef456 Commit 2\nabc123 Commit 1', error: null };
             });
             analyzer = new GitGraphAnalyzer();
 
@@ -155,9 +213,9 @@ describe('GitGraphAnalyzer', () => {
         it('should return null when no previous benchmark found', () => {
             const suites = [createMockBenchmark('abc123', '2025-01-01T00:00:00Z')];
 
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') return 'git version 2.40.0';
-                return 'abc123 Commit 1';
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: 'abc123 Commit 1', error: null };
             });
             analyzer = new GitGraphAnalyzer();
 
@@ -173,9 +231,9 @@ describe('GitGraphAnalyzer', () => {
                 createMockBenchmark('def456', '2025-01-02T00:00:00Z'),
             ];
 
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') return 'git version 2.40.0';
-                throw new Error('Git failed');
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: '', error: new Error('Git failed') };
             });
             analyzer = new GitGraphAnalyzer();
 
@@ -192,9 +250,9 @@ describe('GitGraphAnalyzer', () => {
                 createMockBenchmark('def456', '2025-01-02T00:00:00Z'),
             ];
 
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') return 'git version 2.40.0';
-                return 'xyz999 Other commit';
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: 'xyz999 Other commit', error: null };
             });
             analyzer = new GitGraphAnalyzer();
 
@@ -207,20 +265,6 @@ describe('GitGraphAnalyzer', () => {
     });
 
     describe('findInsertionIndex', () => {
-        const createMockBenchmark = (id: string, timestamp: string): Benchmark => ({
-            commit: {
-                id,
-                timestamp,
-                message: `Commit ${id}`,
-                url: `https://github.com/test/repo/commit/${id}`,
-                author: { username: 'testuser' },
-                committer: { username: 'testuser' },
-            },
-            date: Date.now(),
-            tool: 'cargo',
-            benches: [],
-        });
-
         beforeEach(() => {
             process.env.GITHUB_WORKSPACE = '/github/workspace';
         });
@@ -232,9 +276,9 @@ describe('GitGraphAnalyzer', () => {
             ];
 
             // New commit ghi789 has def456 as ancestor
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') return 'git version 2.40.0';
-                return 'ghi789 Commit 3\ndef456 Commit 2\nabc123 Commit 1';
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: 'ghi789 Commit 3\ndef456 Commit 2\nabc123 Commit 1', error: null };
             });
             analyzer = new GitGraphAnalyzer();
 
@@ -252,9 +296,9 @@ describe('GitGraphAnalyzer', () => {
             ];
 
             // New commit has no relation to existing commits
-            mockExecSync.mockImplementation((cmd: string) => {
-                if (cmd === 'git --version') return 'git version 2.40.0';
-                return 'xyz999 Unrelated commit';
+            mockSpawnSync.mockImplementation((_cmd: string, args: string[]) => {
+                if (args && args[0] === '--version') return { stdout: 'git version 2.40.0', error: null };
+                return { stdout: 'xyz999 Unrelated commit', error: null };
             });
             analyzer = new GitGraphAnalyzer();
 
@@ -265,7 +309,7 @@ describe('GitGraphAnalyzer', () => {
         });
 
         it('should append to end for empty suites', () => {
-            mockExecSync.mockReturnValue('git version 2.40.0');
+            mockSpawnSync.mockReturnValue({ stdout: 'git version 2.40.0', error: null });
             analyzer = new GitGraphAnalyzer();
 
             const index = analyzer.findInsertionIndex([], 'abc123');

test/write.spec.ts

@@ -116,17 +116,18 @@ jest.mock('../src/git', () => ({
     },
 }));
 
+// Simplified mock for general write tests - git-graph-specific logic is tested in dedicated test files
 jest.mock('../src/gitGraph', () => ({
     GitGraphAnalyzer: jest.fn().mockImplementation(() => ({
         isGitAvailable: () => true,
-        getAncestry: (ref: string) => [],
-        findPreviousBenchmark: (suites: any[], currentSha: string) => {
+        getAncestry: (_ref: string) => [],
+        findPreviousBenchmark: (suites: any[], _currentSha: string) => {
             if (suites.length > 0) {
                 return suites[suites.length - 1];
             }
             return null;
         },
-        findInsertionIndex: (suites: any[], newCommitSha: string) => suites.length,
+        findInsertionIndex: (suites: any[], _newCommitSha: string) => suites.length,
     })),
 }));