From 946e84bf3c97ed47f994bf99ed7b1c7c7d1f0619 Mon Sep 17 00:00:00 2001 From: Nilok Bose Date: Sun, 8 Mar 2026 11:48:14 +0100 Subject: [PATCH 1/5] fix: enhance chunk processing with error handling - Updated `reviewLargePRInChunks` to use `Promise.allSettled` for better error handling during chunk processing. - Implemented logic to preserve successful chunk results even when some chunks fail, ensuring comprehensive results are returned. --- src/rag-review.js | 21 ++++++++++++++- src/rag-review.test.js | 60 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/src/rag-review.js b/src/rag-review.js index 26597b2..11c4b55 100644 --- a/src/rag-review.js +++ b/src/rag-review.js @@ -520,10 +520,29 @@ async function reviewLargePRInChunks(prFiles, options) { // Step 3: Process chunks in parallel console.log(chalk.blue('🔄 Processing chunks in parallel...')); - const chunkResults = await Promise.all( + const settledChunkResults = await Promise.allSettled( chunks.map((chunk, index) => reviewPRChunk(chunk, sharedContext, options, index + 1, chunks.length)) ); + const chunkResults = settledChunkResults.map((result, index) => { + if (result.status === 'fulfilled') { + return { + chunkId: index + 1, + ...result.value, + }; + } + + const errorMessage = result.reason instanceof Error ? result.reason.message : String(result.reason); + console.warn(chalk.yellow(`Chunk ${index + 1}/${chunks.length} failed: ${errorMessage}`)); + + return { + chunkId: index + 1, + success: false, + error: errorMessage, + results: [], + }; + }); + // Step 4: Combine results console.log(chalk.blue('🔗 Combining chunk results...')); return combineChunkResults(chunkResults, prFiles.length); diff --git a/src/rag-review.test.js b/src/rag-review.test.js index a2d2e77..6f4fda8 100644 --- a/src/rag-review.test.js +++ b/src/rag-review.test.js @@ -219,6 +219,66 @@ describe('rag-review', () => { expect(result.success).toBe(true); }); + it('should preserve successful chunk results when one chunk fails', async () => { + shouldProcessFile.mockReturnValue(true); + getChangedLinesInfo.mockReturnValue({ + hasChanges: true, + addedLines: [1, 2, 3], + removedLines: [], + fullDiff: '+ new code', + }); + + shouldChunkPR.mockReturnValue({ shouldChunk: true, estimatedTokens: 100000, recommendedChunks: 2 }); + chunkPRFiles.mockReturnValue([ + { files: [{ filePath: '/file1.js' }], totalTokens: 30000 }, + { files: [{ filePath: '/file2.js' }], totalTokens: 30000 }, + ]); + + const allSettledSpy = vi.spyOn(Promise, 'allSettled').mockResolvedValue([ + { + status: 'fulfilled', + value: { + success: true, + results: [{ filePath: '/file1.js', success: true }], + }, + }, + { + status: 'rejected', + reason: new Error('Chunk LLM failed'), + }, + ]); + + combineChunkResults.mockImplementation((chunkResults) => ({ + success: true, + results: chunkResults.filter((chunk) => chunk.success).flatMap((chunk) => chunk.results || []), + chunkResults, + })); + + runAnalysis.mockResolvedValueOnce({ + success: true, + results: { fileSpecificIssues: {}, crossFileIssues: [], summary: 'OK' }, + }); + + const result = await reviewPullRequest(['/file1.js', '/file2.js'], { verbose: true }); + + expect(combineChunkResults).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ chunkId: 1, success: true }), + expect.objectContaining({ + chunkId: 2, + success: false, + error: 'Chunk LLM failed', + results: [], + }), + ]), + 2 + ); + expect(allSettledSpy).toHaveBeenCalled(); + expect(result.success).toBe(true); + expect(result.results).toHaveLength(1); + expect(result.chunkResults).toHaveLength(2); + }); + it('should gather unified context for PR files', async () => { // Setup analysis to return valid holistic result runAnalysis.mockResolvedValue({ From 4da469390c2a98abd18be1704e9635bf1abb054c Mon Sep 17 00:00:00 2001 From: Nilok Bose Date: Sun, 8 Mar 2026 11:56:04 +0100 Subject: [PATCH 2/5] fix: normalize reviewFiles result shape - Flatten file-level results when aggregating multi-file reviews so output formatters receive consistent entries. - Also wrap skipped single-file results with file metadata. --- src/rag-review.js | 26 ++++++++++++++++++- src/rag-review.test.js | 57 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/rag-review.js b/src/rag-review.js index 11c4b55..592432a 100644 --- a/src/rag-review.js +++ b/src/rag-review.js @@ -30,6 +30,22 @@ async function reviewFile(filePath, options = {}) { // If analysis successful, return the result if (analyzeResult.success) { + if (analyzeResult.skipped) { + return { + success: true, + results: [ + { + filePath: analyzeResult.filePath || filePath, + language: analyzeResult.language, + success: true, + skipped: true, + message: analyzeResult.message, + results: analyzeResult.results, + }, + ], + }; + } + // Convert object results to array format expected by the output functions if (analyzeResult.results && !Array.isArray(analyzeResult.results)) { console.log(chalk.blue('Converting results object to array format')); @@ -101,7 +117,15 @@ async function reviewFiles(filePaths, options = {}) { const batchPromises = batch.map((filePath) => reviewFile(filePath, options)); const batchResults = await Promise.all(batchPromises); - results.push(...batchResults); + const flattenedBatchResults = batchResults.flatMap((batchResult) => { + if (batchResult?.success && Array.isArray(batchResult.results)) { + return batchResult.results; + } + + return batchResult ? [batchResult] : []; + }); + + results.push(...flattenedBatchResults); } // Filter out potential null results if any step could return null/undefined (though analyzeFile should always return an object) diff --git a/src/rag-review.test.js b/src/rag-review.test.js index 6f4fda8..3c64873 100644 --- a/src/rag-review.test.js +++ b/src/rag-review.test.js @@ -94,16 +94,62 @@ describe('rag-review', () => { expect(runAnalysis).toHaveBeenCalledWith('/test/file.js', { verbose: true, maxExamples: 10 }); }); + + it('should wrap skipped files with file metadata', async () => { + runAnalysis.mockResolvedValue({ + success: true, + skipped: true, + message: 'File skipped based on exclusion patterns', + }); + + const result = await reviewFile('/test/skipped.js'); + + expect(result.success).toBe(true); + expect(result.results).toEqual([ + expect.objectContaining({ + filePath: '/test/skipped.js', + success: true, + skipped: true, + message: 'File skipped based on exclusion patterns', + }), + ]); + }); }); describe('reviewFiles', () => { it('should review multiple files', async () => { - runAnalysis.mockResolvedValue({ success: true, results: [] }); + runAnalysis + .mockResolvedValueOnce({ + success: true, + filePath: '/test/file1.js', + language: 'javascript', + results: { issues: [] }, + }) + .mockResolvedValueOnce({ + success: true, + filePath: '/test/file2.js', + language: 'javascript', + results: { issues: [] }, + }); const result = await reviewFiles(['/test/file1.js', '/test/file2.js']); expect(result.success).toBe(true); expect(result.results.length).toBe(2); + expect(result.results[0]).toEqual( + expect.objectContaining({ + filePath: '/test/file1.js', + success: true, + results: { issues: [] }, + }) + ); + expect(result.results[1]).toEqual( + expect.objectContaining({ + filePath: '/test/file2.js', + success: true, + results: { issues: [] }, + }) + ); }); it('should process files in batches based on concurrency', async () => { @@ -116,8 +162,13 @@ describe('rag-review', () => { it('should count successes, skips, and errors', async () => { runAnalysis - .mockResolvedValueOnce({ success: true, results: [] }) - .mockResolvedValueOnce({ success: true, skipped: true, results: [] }) + .mockResolvedValueOnce({ + success: true, + filePath: '/file1.js', + language: 'javascript', + results: { issues: [] }, + }) + .mockResolvedValueOnce({ success: true, skipped: true, message: 'Skipped' }) .mockResolvedValueOnce({ success: false, error: 'Error' }); const result = await reviewFiles(['/file1.js', '/file2.js', '/file3.js']); From 722820680640d0e6c621ab3d8ccccbe84cd4e532 Mon Sep 17 00:00:00 2001 From: Nilok Bose Date: Sun, 8 Mar 2026 12:02:28 +0100 Subject: [PATCH 3/5] chore: cleanup code and documentation references to unused `positives` field in review results --- AGENTS.md | 1 - docs/OUTPUT_FORMATS.md | 14 +------------- src/index.js | 21 ++++----------------- 3 files changed, 5 insertions(+), 31 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 27c6e83..22c3244 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -461,7 +461,6 @@ export function myUtility(input) { newCode: string } }], - positives: string[] }, skipped: boolean, error: string | undefined diff --git a/docs/OUTPUT_FORMATS.md b/docs/OUTPUT_FORMATS.md index 31055ce..4d5a108 100644 --- a/docs/OUTPUT_FORMATS.md +++ b/docs/OUTPUT_FORMATS.md @@ -44,9 +44,6 @@ Issues: interface ButtonProps { onClick?: () => void; disabled?: boolean; } const Button = (props: ButtonProps) => { -Positives: - - Good use of semantic HTML elements - - Proper accessibility attributes ``` ## JSON Format @@ -91,8 +88,7 @@ The JSON format provides structured output perfect for programmatic processing, "newCode": "interface ButtonProps { onClick?: () => void; disabled?: boolean; }\nconst Button = (props: ButtonProps) => {" } } - ], - "positives": ["Good use of semantic HTML elements", "Proper accessibility attributes"] + ] } } ] @@ -149,14 +145,6 @@ The markdown format is documentation-friendly and ideal for generating reports, ``` ```` -**Positives Found (2):** - -- Good use of semantic HTML elements - -- Proper accessibility attributes - -```` - ## Usage Specify the output format using the `--output` (or `-o`) option: diff --git a/src/index.js b/src/index.js index db3cd54..7ac31f8 100755 --- a/src/index.js +++ b/src/index.js @@ -1015,7 +1015,7 @@ function outputJson(reviewResults, options) { filePath: r.filePath, success: true, language: r.language, - review: r.results, // Contains summary, issues (with optional codeSuggestion), positives from LLM + review: r.results, // Contains summary and actionable issues (with optional codeSuggestion) // Optionally include similar examples if needed // similarExamplesUsed: r.similarExamples }; @@ -1069,8 +1069,8 @@ function outputMarkdown(reviewResults) { console.log(`*Skipped (based on exclusion patterns or file type).*\n`); return; } - if (!fileResult.results || (!fileResult.results.issues?.length && !fileResult.results.positives?.length)) { - console.log(`*No significant findings or issues reported.*\n`); + if (!fileResult.results || !fileResult.results.issues?.length) { + console.log(`*No actionable issues reported.*\n`); if (fileResult.results?.summary) { console.log(`**Summary:** ${fileResult.results.summary}\n`); } @@ -1111,12 +1111,6 @@ function outputMarkdown(reviewResults) { }); } - if (review.positives && review.positives.length > 0) { - console.log(`**Positives Found (${review.positives.length}):**\n`); - review.positives.forEach((positive) => { - console.log(` - ${positive}\n`); - }); - } }); } @@ -1154,7 +1148,7 @@ function outputText(reviewResults, cliOptions) { } return; } - if (!fileResult.results || (!fileResult.results.issues?.length && !fileResult.results.positives?.length)) { + if (!fileResult.results || !fileResult.results.issues?.length) { if (cliOptions.verbose) { console.log(chalk.green(`\nNo findings for: ${fileResult.filePath}`)); if (fileResult.results?.summary) { @@ -1194,13 +1188,6 @@ function outputText(reviewResults, cliOptions) { }); } - if (review.positives && review.positives.length > 0) { - console.log(chalk.bold.green('\nPositives:')); - review.positives.forEach((positive) => { - console.log(` - ${positive}`); - }); - console.log(''); - } console.log(chalk.gray(`========================================${'='.repeat(fileResult.filePath.length)}`)); }); } From 42f830d0883db40fd602d81926f3861a81f4766a Mon Sep 17 00:00:00 2001 From: Nilok Bose Date: Sun, 8 Mar 2026 12:07:00 +0100 Subject: [PATCH 4/5] fix: ensure markdown output is shown when `--output-file` option is set to markdown --- docs/OUTPUT_FORMATS.md | 5 ++-- src/index.js | 68 +++++++++++++++++++++++------------------- 2 files changed, 40 insertions(+), 33 deletions(-) diff --git a/docs/OUTPUT_FORMATS.md b/docs/OUTPUT_FORMATS.md index 4d5a108..28cf436 100644 --- a/docs/OUTPUT_FORMATS.md +++ b/docs/OUTPUT_FORMATS.md @@ -158,7 +158,7 @@ codecritique analyze --file src/app.ts --output json # Markdown format codecritique analyze --file src/app.ts --output markdown -```` +``` ### Saving Output to a File @@ -168,10 +168,11 @@ You can redirect output to a file using shell redirection: codecritique analyze --files "src/**/*.ts" --output json > review.json ``` -Or use the `--output-file` option (for JSON format): +Or use the `--output-file` option for `json` or `markdown` output: ```bash codecritique analyze --files "src/**/*.ts" --output json --output-file review.json +codecritique analyze --files "src/**/*.ts" --output markdown --output-file review.md ``` --- diff --git a/src/index.js b/src/index.js index 7ac31f8..a3a0d12 100755 --- a/src/index.js +++ b/src/index.js @@ -1038,80 +1038,86 @@ function outputJson(reviewResults, options) { * Output results in Markdown format * * @param {Array} reviewResults - Array of individual file review results - * @param {Object} cliOptions - Command line options + * @param {Object} options - Command line options */ -function outputMarkdown(reviewResults) { - console.log('# AI Code Review Results (RAG Approach)\n'); - +function outputMarkdown(reviewResults, options) { const totalFiles = reviewResults.length; const filesWithIssues = reviewResults.filter((r) => r.success && !r.skipped && r.results?.issues?.length > 0).length; const totalIssues = reviewResults.reduce((sum, r) => sum + (r.results?.issues?.length || 0), 0); const skippedFiles = reviewResults.filter((r) => r.skipped).length; const errorFiles = reviewResults.filter((r) => !r.success).length; - console.log('## Summary\n'); - console.log(`- **Files Analyzed:** ${totalFiles}`); - console.log(`- **Files with Issues:** ${filesWithIssues}`); - console.log(`- **Total Issues Found:** ${totalIssues}`); - if (skippedFiles > 0) console.log(`- **Files Skipped:** ${skippedFiles}`); - if (errorFiles > 0) console.log(`- **Errors:** ${errorFiles}`); - console.log('\n'); + const lines = [ + '# AI Code Review Results (RAG Approach)', + '', + '## Summary', + '', + `- **Files Analyzed:** ${totalFiles}`, + `- **Files with Issues:** ${filesWithIssues}`, + `- **Total Issues Found:** ${totalIssues}`, + ]; - console.log('## Detailed Review per File\n'); + if (skippedFiles > 0) lines.push(`- **Files Skipped:** ${skippedFiles}`); + if (errorFiles > 0) lines.push(`- **Errors:** ${errorFiles}`); + + lines.push('', '## Detailed Review per File', ''); reviewResults.forEach((fileResult) => { - console.log(`### ${fileResult.filePath}\n`); + lines.push(`### ${fileResult.filePath}`, ''); if (!fileResult.success) { - console.log(`**Error:** ${fileResult.error}\n`); + lines.push(`**Error:** ${fileResult.error}`, ''); return; } if (fileResult.skipped) { - console.log(`*Skipped (based on exclusion patterns or file type).*\n`); + lines.push('*Skipped (based on exclusion patterns or file type).*', ''); return; } if (!fileResult.results || !fileResult.results.issues?.length) { - console.log(`*No actionable issues reported.*\n`); + lines.push('*No actionable issues reported.*', ''); if (fileResult.results?.summary) { - console.log(`**Summary:** ${fileResult.results.summary}\n`); + lines.push(`**Summary:** ${fileResult.results.summary}`, ''); } return; } const review = fileResult.results; if (review.summary) { - console.log(`**Summary:** ${review.summary}\n`); + lines.push(`**Summary:** ${review.summary}`, ''); } if (review.issues && review.issues.length > 0) { - console.log(`**Issues Found (${review.issues.length}):**\n`); + lines.push(`**Issues Found (${review.issues.length}):**`, ''); review.issues.forEach((issue) => { const severityEmoji = getSeverityEmoji(issue.severity); - console.log( + lines.push( `- **[${issue.severity.toUpperCase()}] ${severityEmoji} (Lines: ${issue.lineNumbers?.join(', ') || 'N/A'})**: ${ issue.description }` ); if (issue.suggestion) { - console.log(`\n *Suggestion:* ${issue.suggestion}\n`); + lines.push('', ` *Suggestion:* ${issue.suggestion}`, ''); } // Include code suggestion if available if (issue.codeSuggestion) { const { startLine, endLine, newCode } = issue.codeSuggestion; const lineRange = endLine ? `${startLine}-${endLine}` : `${startLine}`; - console.log(`\n **Suggested change (lines ${lineRange}):**\n`); - console.log(' ```suggestion'); - console.log( - newCode - .split('\n') - .map((line) => ` ${line}`) - .join('\n') - ); - console.log(' ```\n'); + lines.push('', ` **Suggested change (lines ${lineRange}):**`, '', ' ```suggestion'); + lines.push(...newCode.split('\n').map((line) => ` ${line}`)); + lines.push(' ```', ''); } }); } - }); + + const markdownOutput = `${lines.join('\n')}\n`; + + if (options?.outputFile) { + fs.writeFileSync(options.outputFile, markdownOutput, 'utf8'); + console.log(chalk.green(`Markdown output saved to: ${options.outputFile}`)); + return; + } + + process.stdout.write(markdownOutput); } /** From dbd1c378df122fe3c58172ad61e40e1898fc9542 Mon Sep 17 00:00:00 2001 From: Nilok Bose Date: Sun, 8 Mar 2026 12:11:01 +0100 Subject: [PATCH 5/5] fix: preserve partial context retrieval results - Handle context retrieval failures per source so one rejected lookup does not discard successful PR comments, guidelines, code examples, or custom document results. --- src/rag-analyzer.js | 84 ++++++++++++++++++++++++---------------- src/rag-analyzer.test.js | 23 +++++++++++ 2 files changed, 74 insertions(+), 33 deletions(-) diff --git a/src/rag-analyzer.js b/src/rag-analyzer.js index 3c35db1..7b758d8 100644 --- a/src/rag-analyzer.js +++ b/src/rag-analyzer.js @@ -1975,40 +1975,58 @@ async function getContextForFile(filePath, content, options = {}) { } }; + const withContextFallback = async (promise, fallbackValue, label) => { + try { + return await promise; + } catch (error) { + console.warn(chalk.yellow(`${label} failed: ${error.message}`)); + return fallbackValue; + } + }; + const [prContextResult, guidelineCandidates, codeExampleCandidates, relevantCustomDocChunks] = await Promise.all([ - getPRCommentContext(filePath, { - ...options, - projectPath, - precomputedQueryEmbedding: fileContentQueryEmbedding, - maxComments: MAX_PR_COMMENTS_FOR_CONTEXT, - similarityThreshold: options.prSimilarityThreshold || 0.3, - timeout: options.prTimeout || 300000, - repository: options.repository || null, - }), - embeddingsSystem.findRelevantDocs(guidelineQuery, { - ...options, - projectPath, - precomputedQueryEmbedding: guidelineQueryEmbedding, - limit: GUIDELINE_CANDIDATE_LIMIT, - similarityThreshold: 0.05, - useReranking: true, - queryContextForReranking: reviewedSnippetContext, - }), - embeddingsSystem.findSimilarCode(isTestFile ? `${content}\\n// Looking for similar test files and testing patterns` : content, { - ...options, - projectPath, - isTestFile, - precomputedQueryEmbedding: fileContentQueryEmbedding, - limit: CODE_EXAMPLE_LIMIT, - similarityThreshold: 0.3, - queryFilePath: filePath, - includeProjectStructure: false, - }), - processCustomDocuments(), // Add custom document processing as 4th parallel operation - ]).catch((error) => { - console.warn(chalk.yellow(`Parallel context retrieval failed: ${error.message}`)); - return [[], [], [], []]; - }); + withContextFallback( + getPRCommentContext(filePath, { + ...options, + projectPath, + precomputedQueryEmbedding: fileContentQueryEmbedding, + maxComments: MAX_PR_COMMENTS_FOR_CONTEXT, + similarityThreshold: options.prSimilarityThreshold || 0.3, + timeout: options.prTimeout || 300000, + repository: options.repository || null, + }), + { comments: [] }, + 'PR comment context retrieval' + ), + withContextFallback( + embeddingsSystem.findRelevantDocs(guidelineQuery, { + ...options, + projectPath, + precomputedQueryEmbedding: guidelineQueryEmbedding, + limit: GUIDELINE_CANDIDATE_LIMIT, + similarityThreshold: 0.05, + useReranking: true, + queryContextForReranking: reviewedSnippetContext, + }), + [], + 'Guideline retrieval' + ), + withContextFallback( + embeddingsSystem.findSimilarCode(isTestFile ? `${content}\\n// Looking for similar test files and testing patterns` : content, { + ...options, + projectPath, + isTestFile, + precomputedQueryEmbedding: fileContentQueryEmbedding, + limit: CODE_EXAMPLE_LIMIT, + similarityThreshold: 0.3, + queryFilePath: filePath, + includeProjectStructure: false, + }), + [], + 'Code example retrieval' + ), + withContextFallback(processCustomDocuments(), [], 'Custom document retrieval'), + ]); const prCommentContext = prContextResult?.comments || []; const prContextAvailable = prCommentContext.length > 0; diff --git a/src/rag-analyzer.test.js b/src/rag-analyzer.test.js index 17ab00a..bc1beda 100644 --- a/src/rag-analyzer.test.js +++ b/src/rag-analyzer.test.js @@ -326,6 +326,29 @@ describe('rag-analyzer', () => { const result = await runAnalysis('/test/file.js'); expect(result.success).toBe(true); }); + + it('should preserve successful context sources when one parallel retrieval fails', async () => { + mockEmbeddingsSystem.findRelevantDocs.mockResolvedValue([ + { + path: '/docs/api.md', + content: 'API docs', + similarity: 0.8, + type: 'documentation-chunk', + document_title: 'API Docs', + heading_text: 'Usage', + }, + ]); + mockEmbeddingsSystem.findSimilarCode.mockResolvedValue([{ path: '/similar.js', content: 'similar code', similarity: 0.9 }]); + findRelevantPRComments.mockRejectedValue(new Error('PR comments failed')); + llm.sendPromptToClaude.mockResolvedValue({ json: { summary: 'Review', issues: [] } }); + + const result = await runAnalysis('/test/file.js'); + + expect(result.success).toBe(true); + expect(result.context.codeExamples).toBeGreaterThan(0); + expect(result.context.guidelines).toBeGreaterThanOrEqual(0); + expect(result.context.prComments).toBe(0); + }); }); // ==========================================================================