From 472b81f79d89dece117f2b6d9f715b60e87a08d3 Mon Sep 17 00:00:00 2001 From: Nilok Bose Date: Sun, 1 Mar 2026 20:37:25 +0100 Subject: [PATCH 1/5] fix(actions): remove eval from CI action command execution Harden composite actions by building CLI arguments as arrays and invoking `node` directly, eliminating eval-based command construction. --- .../actions/generate-embeddings/action.yml | 21 ++++++++-------- .github/actions/pr-review/action.yml | 25 +++++++++---------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/.github/actions/generate-embeddings/action.yml b/.github/actions/generate-embeddings/action.yml index 8a85303..02a2ef6 100644 --- a/.github/actions/generate-embeddings/action.yml +++ b/.github/actions/generate-embeddings/action.yml @@ -91,45 +91,44 @@ runs: run: | echo "🧠 Generating embeddings for codebase..." - # Build command arguments based on actual CLI options - ARGS="embeddings:generate" - ARGS="${ARGS} --directory \"$GITHUB_WORKSPACE\"" + # Build command arguments safely - avoid eval/string interpolation + ARGS=("embeddings:generate" "--directory" "$GITHUB_WORKSPACE") # Add concurrency only if provided if [ -n "${{ inputs.concurrency }}" ]; then - ARGS="${ARGS} --concurrency ${{ inputs.concurrency }}" + ARGS+=("--concurrency" "${{ inputs.concurrency }}") fi # Add specific files if provided if [ -n "${{ inputs.files }}" ]; then for file_pattern in ${{ inputs.files }}; do - ARGS="${ARGS} --files '${file_pattern}'" + ARGS+=("--files" "${file_pattern}") done fi # Always exclude action-related directories and files - ARGS="${ARGS} --exclude 'codecritique-tool/**'" - ARGS="${ARGS} --exclude '.codecritique-artifacts/**'" + ARGS+=("--exclude" "codecritique-tool/**") + ARGS+=("--exclude" ".codecritique-artifacts/**") # Add user-specified exclude patterns if [ -n "${{ inputs.exclude }}" ]; then for pattern in ${{ inputs.exclude }}; do - ARGS="${ARGS} --exclude '${pattern}'" + ARGS+=("--exclude" "${pattern}") done fi # Add exclude file if specified if [ -n "${{ inputs.exclude-file }}" ] && [ -f "$GITHUB_WORKSPACE/${{ inputs.exclude-file }}" ]; then - ARGS="${ARGS} --exclude-file \"$GITHUB_WORKSPACE/${{ inputs.exclude-file }}\"" + ARGS+=("--exclude-file" "$GITHUB_WORKSPACE/${{ inputs.exclude-file }}") fi # Add verbose flag if [ "${{ inputs.verbose }}" = "true" ]; then - ARGS="${ARGS} --verbose" + ARGS+=("--verbose") fi # Execute with error handling - if eval "node src/index.js ${ARGS}"; then + if node src/index.js "${ARGS[@]}"; then echo "✅ Embedding generation completed successfully" echo "success=true" >> $GITHUB_OUTPUT else diff --git a/.github/actions/pr-review/action.yml b/.github/actions/pr-review/action.yml index d495e75..8a8fe10 100644 --- a/.github/actions/pr-review/action.yml +++ b/.github/actions/pr-review/action.yml @@ -156,33 +156,33 @@ runs: DEBUG: ${{ inputs.verbose }} VERBOSE: ${{ inputs.verbose }} run: | - # Build CLI command arguments - analyze workspace directly - CLI_ARGS="analyze -b ${{ github.head_ref }} --directory \"$GITHUB_WORKSPACE\"" + # Build CLI command arguments safely - avoid eval/string interpolation. + CLI_ARGS=("analyze" "-b" "${{ github.head_ref }}" "--directory" "$GITHUB_WORKSPACE") # Add output format and output file (always JSON) - CLI_ARGS="$CLI_ARGS --output json --output-file review_output.json" + CLI_ARGS+=("--output" "json" "--output-file" "review_output.json") # Add verbose flag if enabled if [ "${{ inputs.verbose }}" = "true" ]; then - CLI_ARGS="$CLI_ARGS --verbose" + CLI_ARGS+=("--verbose") fi # Add model configuration if [ -n "${{ inputs.model }}" ]; then - CLI_ARGS="$CLI_ARGS --model ${{ inputs.model }}" + CLI_ARGS+=("--model" "${{ inputs.model }}") fi # Only add optional parameters if they have values if [ -n "${{ inputs.max-tokens }}" ]; then - CLI_ARGS="$CLI_ARGS --max-tokens ${{ inputs.max-tokens }}" + CLI_ARGS+=("--max-tokens" "${{ inputs.max-tokens }}") fi if [ -n "${{ inputs.cache-ttl }}" ]; then - CLI_ARGS="$CLI_ARGS --cache-ttl ${{ inputs.cache-ttl }}" + CLI_ARGS+=("--cache-ttl" "${{ inputs.cache-ttl }}") fi if [ -n "${{ inputs.concurrency }}" ]; then - CLI_ARGS="$CLI_ARGS --concurrency ${{ inputs.concurrency }}" + CLI_ARGS+=("--concurrency" "${{ inputs.concurrency }}") fi # Add custom documents if specified @@ -190,22 +190,21 @@ runs: IFS=',' read -ra DOCS <<< "${{ inputs.custom-docs }}" for doc in "${DOCS[@]}"; do if [ -n "$doc" ]; then - CLI_ARGS="$CLI_ARGS --doc \"$doc\"" + CLI_ARGS+=("--doc" "$doc") fi done fi # Add feedback parameters - CLI_ARGS="$CLI_ARGS --track-feedback" - CLI_ARGS="$CLI_ARGS --feedback-path \"$GITHUB_WORKSPACE/.ai-feedback\"" + CLI_ARGS+=("--track-feedback" "--feedback-path" "$GITHUB_WORKSPACE/.ai-feedback") # Run the AI code review echo "🤖 Running AI Code Review..." - echo "Command: node src/index.js $CLI_ARGS" + echo "Command: node src/index.js ${CLI_ARGS[*]}" # Execute the command and capture output # Run the CLI - JSON output goes to file, logs to console - eval "node src/index.js $CLI_ARGS" || { + node src/index.js "${CLI_ARGS[@]}" || { echo "❌ AI Code Review failed" exit 1 } From 5cc7a53176becc10be328b4264071f322d939d8c Mon Sep 17 00:00:00 2001 From: Nilok Bose Date: Sun, 1 Mar 2026 20:37:40 +0100 Subject: [PATCH 2/5] fix(git): harden diff execution and avoid branch checkout side effects Use safely escaped git diff invocation and create tracking branches without switching the working tree during branch preparation. --- src/utils/git.js | 10 ++++------ src/utils/git.test.js | 19 ++++++++++--------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/utils/git.js b/src/utils/git.js index 3479df6..df2c26f 100644 --- a/src/utils/git.js +++ b/src/utils/git.js @@ -65,8 +65,8 @@ export function ensureBranchExists(branchName, workingDir = process.cwd()) { // Check if branch exists on remote try { execGitSafe('git show-ref', ['--verify', '--quiet', `refs/remotes/origin/${branchName}`], { cwd: workingDir }); - // Create local tracking branch - execGitSafe('git checkout', ['-b', branchName, `origin/${branchName}`], { stdio: 'pipe', cwd: workingDir }); + // Create local tracking branch without switching working tree. + execGitSafe('git branch', ['--track', branchName, `origin/${branchName}`], { stdio: 'pipe', cwd: workingDir }); console.log(chalk.green(`Successfully created local branch '${branchName}' tracking origin/${branchName}`)); } catch { throw new Error(`Branch '${branchName}' not found locally or on remote origin`); @@ -125,10 +125,8 @@ export function findBaseBranch(workingDir = process.cwd()) { */ function getFileDiff(filePath, baseBranch, targetBranch, workingDir = process.cwd()) { try { - // Use git diff to get changes for the specific file - // Format: git diff base...target -- filepath - const gitCommand = `git diff ${baseBranch}...${targetBranch} -- "${filePath}"`; - const diffOutput = execSync(gitCommand, { cwd: workingDir, encoding: 'utf8' }); + // Use safely escaped args to avoid command injection. + const diffOutput = execGitSafe('git diff', [`${baseBranch}...${targetBranch}`, '--', filePath], { cwd: workingDir, encoding: 'utf8' }); return diffOutput; } catch (error) { diff --git a/src/utils/git.test.js b/src/utils/git.test.js index 8e76b73..89b140f 100644 --- a/src/utils/git.test.js +++ b/src/utils/git.test.js @@ -138,7 +138,7 @@ describe('ensureBranchExists', () => { if (cmd === 'git show-ref' && args?.includes('refs/remotes/origin/feature-branch')) { return ''; // Remote branch exists } - if (cmd === 'git checkout') { + if (cmd === 'git branch') { return ''; } throw new Error('Unexpected call'); @@ -163,11 +163,12 @@ describe('ensureBranchExists', () => { describe('getChangedLinesInfo', () => { beforeEach(() => { + vi.clearAllMocks(); mockConsoleSelective('error'); }); it('should parse added lines from diff', () => { - execSync.mockReturnValue(`@@ -10,3 +10,5 @@ function test() { + command.execGitSafe.mockReturnValue(`@@ -10,3 +10,5 @@ function test() { context line +added line 1 +added line 2 @@ -185,7 +186,7 @@ describe('getChangedLinesInfo', () => { }); it('should parse removed lines from diff', () => { - execSync.mockReturnValue(`@@ -10,4 +10,2 @@ function test() { + command.execGitSafe.mockReturnValue(`@@ -10,4 +10,2 @@ function test() { context line -removed line 1 -removed line 2 @@ -201,7 +202,7 @@ describe('getChangedLinesInfo', () => { }); it('should parse context lines from diff', () => { - execSync.mockReturnValue(`@@ -10,3 +10,4 @@ function test() { + command.execGitSafe.mockReturnValue(`@@ -10,3 +10,4 @@ function test() { context before +added line context after @@ -215,7 +216,7 @@ describe('getChangedLinesInfo', () => { }); it('should return hasChanges false for empty diff', () => { - execSync.mockReturnValue(''); + command.execGitSafe.mockReturnValue(''); const result = getChangedLinesInfo('src/file.js', 'main', 'feature'); @@ -225,7 +226,7 @@ describe('getChangedLinesInfo', () => { }); it('should handle multiple hunks', () => { - execSync.mockReturnValue(`@@ -5,3 +5,4 @@ first section + command.execGitSafe.mockReturnValue(`@@ -5,3 +5,4 @@ first section context +first add more context @@ -242,7 +243,7 @@ describe('getChangedLinesInfo', () => { }); it('should handle hunk headers without line counts', () => { - execSync.mockReturnValue(`@@ -10 +10 @@ function + command.execGitSafe.mockReturnValue(`@@ -10 +10 @@ function +single line change `); @@ -254,7 +255,7 @@ describe('getChangedLinesInfo', () => { it('should include full diff in result', () => { const diffContent = '@@ -1,1 +1,2 @@\n context\n+added'; - execSync.mockReturnValue(diffContent); + command.execGitSafe.mockReturnValue(diffContent); const result = getChangedLinesInfo('src/file.js', 'main', 'feature'); @@ -262,7 +263,7 @@ describe('getChangedLinesInfo', () => { }); it('should handle errors gracefully', () => { - execSync.mockImplementation(() => { + command.execGitSafe.mockImplementation(() => { throw new Error('Git error'); }); From 2ff1f163d2251e6bca612a85e14a371c0b8d59a0 Mon Sep 17 00:00:00 2001 From: Nilok Bose Date: Sun, 1 Mar 2026 20:38:03 +0100 Subject: [PATCH 3/5] fix(rag): prevent parse fallback crash on non-string LLM responses Serialize fallback raw responses safely so malformed analysis payloads no longer throw during reconstruction. --- src/rag-analyzer.js | 2 +- src/rag-analyzer.test.js | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/rag-analyzer.js b/src/rag-analyzer.js index b19143f..13d0f79 100644 --- a/src/rag-analyzer.js +++ b/src/rag-analyzer.js @@ -774,7 +774,7 @@ async function callLLMForAnalysis(context, options = {}) { return { summary: analysisResponse.summary || 'Analysis completed with parsing issues', issues: Array.isArray(analysisResponse.issues) ? analysisResponse.issues : [], - rawResponse: analysisResponse.rawResponse || llmResponse.substring(0, 500), + rawResponse: analysisResponse.rawResponse || JSON.stringify(llmResponse).substring(0, 500), parseWarning: 'Response structure was reconstructed due to parsing issues', }; } diff --git a/src/rag-analyzer.test.js b/src/rag-analyzer.test.js index 69dfd44..17ab00a 100644 --- a/src/rag-analyzer.test.js +++ b/src/rag-analyzer.test.js @@ -385,6 +385,17 @@ describe('rag-analyzer', () => { expect(result.results).toBeDefined(); }); + it('should reconstruct malformed responses without crashing', async () => { + llm.sendPromptToClaude.mockResolvedValue({ + content: 'non-json fallback text', + json: { foo: 'bar' }, + }); + const result = await runAnalysis('/test/file.js'); + expect(result.success).toBe(true); + expect(result.results.summary).toBeDefined(); + expect(Array.isArray(result.results.issues)).toBe(true); + }); + it('should handle LLM response with null json', async () => { llm.sendPromptToClaude.mockResolvedValue({ json: null }); const result = await runAnalysis('/test/file.js'); From 993eb2237c238c430448b72c415294a1157ff17d Mon Sep 17 00:00:00 2001 From: Nilok Bose Date: Sun, 1 Mar 2026 20:38:24 +0100 Subject: [PATCH 4/5] fix(runtime): harden cleanup paths and status query fallbacks Ensure temporary DB connections and progress timers are always cleaned up, guard missing review output paths in the PR comment action, and use table query APIs for last-analysis lookups. --- .github/actions/pr-review/post-comments.js | 5 ++ src/embeddings/database.js | 16 ++++++ src/index.js | 58 +++++++++++----------- src/pr-history/database.js | 4 +- src/pr-history/database.test.js | 8 +-- 5 files changed, 57 insertions(+), 34 deletions(-) diff --git a/.github/actions/pr-review/post-comments.js b/.github/actions/pr-review/post-comments.js index 7b2871a..3ffc9be 100644 --- a/.github/actions/pr-review/post-comments.js +++ b/.github/actions/pr-review/post-comments.js @@ -257,6 +257,11 @@ export default async ({ github, context, core }) => { const currentFeedback = {}; // Check if review output exists + if (!reviewOutputPath) { + console.log('❌ REVIEW_OUTPUT_PATH is not set'); + return; + } + if (!fs.existsSync(reviewOutputPath)) { console.log('❌ Review output file not found'); return; diff --git a/src/embeddings/database.js b/src/embeddings/database.js index 915295d..f0f6c8b 100644 --- a/src/embeddings/database.js +++ b/src/embeddings/database.js @@ -445,6 +445,14 @@ export class DatabaseManager { this.dbConnection = null; this.tablesInitialized = false; throw error; + } finally { + if (db) { + try { + await db.close(); + } catch (closeError) { + console.warn(chalk.yellow(`Warning: Failed to close temporary database connection: ${closeError.message}`)); + } + } } } @@ -524,6 +532,14 @@ export class DatabaseManager { } catch (error) { console.error(chalk.red(`Error clearing project embeddings: ${error.message}`), error); throw error; + } finally { + if (db) { + try { + await db.close(); + } catch (closeError) { + console.warn(chalk.yellow(`Warning: Failed to close temporary database connection: ${closeError.message}`)); + } + } } } diff --git a/src/index.js b/src/index.js index 8b74ea0..db3cd54 100755 --- a/src/index.js +++ b/src/index.js @@ -576,35 +576,37 @@ async function generateEmbeddings(options) { // Start the progress update interval const progressInterval = setInterval(updateSpinner, 100); + let results; + try { + results = await embeddingsSystem.processBatchEmbeddings(filesToProcess, { + concurrency, + verbose: options.verbose, + excludePatterns, + respectGitignore: options.gitignore !== false, + baseDir: baseDir, + batchSize: 100, // Set a reasonable batch size + maxLines: parseInt(options.maxLines || '1000', 10), + onProgress: (status) => { + // Update counters based on status + if (status === 'processed') { + processedCount++; + } else if (status === 'skipped') { + skippedCount++; + } else if (status === 'failed') { + failedCount++; + } else if (status === 'excluded') { + excludedCount++; + } - const results = await embeddingsSystem.processBatchEmbeddings(filesToProcess, { - concurrency, - verbose: options.verbose, - excludePatterns, - respectGitignore: options.gitignore !== false, - baseDir: baseDir, - batchSize: 100, // Set a reasonable batch size - maxLines: parseInt(options.maxLines || '1000', 10), - onProgress: (status) => { - // Update counters based on status - if (status === 'processed') { - processedCount++; - } else if (status === 'skipped') { - skippedCount++; - } else if (status === 'failed') { - failedCount++; - } else if (status === 'excluded') { - excludedCount++; - } - - // Update the spinner with new progress information - updateSpinner(); - }, - }); - - // Clean up the progress display - clearInterval(progressInterval); - spinner.stop(true); + // Update the spinner with new progress information + updateSpinner(); + }, + }); + } finally { + // Clean up the progress display even if embedding generation fails. + clearInterval(progressInterval); + spinner.stop(true); + } console.log(chalk.green(`\nEmbedding generation complete!`)); console.log(chalk.cyan(`Processed: ${results.processed} files`)); diff --git a/src/pr-history/database.js b/src/pr-history/database.js index dd789fd..747280c 100644 --- a/src/pr-history/database.js +++ b/src/pr-history/database.js @@ -421,11 +421,11 @@ export async function getLastAnalysisTimestamp(repository, projectPath) { const filters = [`repository = '${repository.replace(/'/g, "''")}'`, `project_path = '${resolvedProjectPath.replace(/'/g, "''")}'`]; const results = await table - .search() + .query() .where(filters.join(' AND ')) - .limit(1) .select(['created_at']) .orderBy([{ column: 'created_at', order: 'desc' }]) + .limit(1) .toArray(); if (results.length > 0) { diff --git a/src/pr-history/database.test.js b/src/pr-history/database.test.js index 089c9d6..1a06a38 100644 --- a/src/pr-history/database.test.js +++ b/src/pr-history/database.test.js @@ -392,11 +392,11 @@ describe('PR History Database', () => { describe('getLastAnalysisTimestamp', () => { const setupMockLastAnalysis = (results) => { - mockTable.search.mockReturnValue({ + mockTable.query.mockReturnValue({ where: vi.fn().mockReturnThis(), - limit: vi.fn().mockReturnThis(), select: vi.fn().mockReturnThis(), orderBy: vi.fn().mockReturnThis(), + limit: vi.fn().mockReturnThis(), toArray: vi.fn().mockResolvedValue(results), }); }; @@ -407,11 +407,11 @@ describe('PR History Database', () => { [ 'null on errors', () => - mockTable.search.mockReturnValue({ + mockTable.query.mockReturnValue({ where: vi.fn().mockReturnThis(), - limit: vi.fn().mockReturnThis(), select: vi.fn().mockReturnThis(), orderBy: vi.fn().mockReturnThis(), + limit: vi.fn().mockReturnThis(), toArray: vi.fn().mockRejectedValue(new Error('Search error')), }), null, From 7958578604a90ce1f5d823762ef31fd21ecd3647 Mon Sep 17 00:00:00 2001 From: Nilok Bose Date: Sun, 1 Mar 2026 20:38:44 +0100 Subject: [PATCH 5/5] fix(custom-docs): use canonical cache-manager custom chunk APIs Remove now-unused alias methods and update custom document processor/tests to rely on the existing custom chunk cache interfaces. --- src/custom-documents.js | 4 ++-- src/custom-documents.test.js | 20 +++++++++++--------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/custom-documents.js b/src/custom-documents.js index aa4bf61..50e0ed5 100644 --- a/src/custom-documents.js +++ b/src/custom-documents.js @@ -350,7 +350,7 @@ export class CustomDocumentProcessor { } // Try cache manager - const cachedChunks = await this.cacheManager.getCustomDocuments(resolvedProjectPath); + const cachedChunks = this.cacheManager.getCustomDocumentChunks(resolvedProjectPath); if (cachedChunks && cachedChunks.length > 0) { // Restore to memory this.customDocumentChunks.set(resolvedProjectPath, cachedChunks); @@ -520,7 +520,7 @@ export class CustomDocumentProcessor { try { const resolvedProjectPath = path.resolve(projectPath); this.customDocumentChunks.delete(resolvedProjectPath); - await this.cacheManager.clearCustomDocuments(resolvedProjectPath); + this.cacheManager.customDocumentChunks.delete(resolvedProjectPath); console.log(chalk.green(`Cleared custom document chunks for project: ${resolvedProjectPath}`)); } catch (error) { console.error(chalk.red(`Error clearing project chunks: ${error.message}`)); diff --git a/src/custom-documents.test.js b/src/custom-documents.test.js index 7067fed..fa81334 100644 --- a/src/custom-documents.test.js +++ b/src/custom-documents.test.js @@ -11,8 +11,7 @@ vi.mock('./embeddings/model-manager.js', () => ({ vi.mock('./embeddings/cache-manager.js', () => ({ CacheManager: class { storeCustomDocuments = vi.fn().mockResolvedValue(undefined); - getCustomDocuments = vi.fn().mockResolvedValue([]); - clearCustomDocuments = vi.fn().mockResolvedValue(undefined); + getCustomDocumentChunks = vi.fn().mockReturnValue([]); }, })); @@ -32,8 +31,7 @@ describe('CustomDocumentProcessor', () => { mockCacheManager = { storeCustomDocuments: vi.fn().mockResolvedValue(undefined), - getCustomDocuments: vi.fn().mockResolvedValue([]), - clearCustomDocuments: vi.fn().mockResolvedValue(undefined), + getCustomDocumentChunks: vi.fn().mockReturnValue([]), }; processor = new CustomDocumentProcessor({ @@ -319,7 +317,7 @@ describe('CustomDocumentProcessor', () => { it('should return chunks from cache if not in memory', async () => { const cachedChunks = [{ id: 'cached', content: 'from cache' }]; - mockCacheManager.getCustomDocuments.mockResolvedValue(cachedChunks); + mockCacheManager.getCustomDocumentChunks.mockReturnValue(cachedChunks); const result = await processor.getExistingChunks('/project'); @@ -328,7 +326,7 @@ describe('CustomDocumentProcessor', () => { it('should restore cached chunks to memory', async () => { const cachedChunks = [{ id: 'cached', content: 'from cache' }]; - mockCacheManager.getCustomDocuments.mockResolvedValue(cachedChunks); + mockCacheManager.getCustomDocumentChunks.mockReturnValue(cachedChunks); await processor.getExistingChunks('/project'); @@ -336,7 +334,7 @@ describe('CustomDocumentProcessor', () => { }); it('should return empty array when no chunks exist', async () => { - mockCacheManager.getCustomDocuments.mockResolvedValue([]); + mockCacheManager.getCustomDocumentChunks.mockReturnValue([]); const result = await processor.getExistingChunks('/project'); @@ -353,10 +351,14 @@ describe('CustomDocumentProcessor', () => { expect(processor.customDocumentChunks.has('/project')).toBe(false); }); - it('should clear chunks from cache', async () => { + it('should clear chunks for only the selected project', async () => { + processor.customDocumentChunks.set('/project', [{ id: 'chunk-a' }]); + processor.customDocumentChunks.set('/other', [{ id: 'chunk-b' }]); + await processor.clearProjectChunks('/project'); - expect(mockCacheManager.clearCustomDocuments).toHaveBeenCalled(); + expect(processor.customDocumentChunks.has('/project')).toBe(false); + expect(processor.customDocumentChunks.has('/other')).toBe(true); }); });