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 } 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/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); }); }); 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, 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'); 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'); });