refactor comments into single GH review

sayansisodiya · sayansisodiya · commit aeffbf651b48 · 2025-08-26T23:28:18.000-07:00
diff --git a/FILE_BASED_COMMENTS.md b/FILE_BASED_COMMENTS.md
@@ -0,0 +1,67 @@
+# File-Based Comment Collection System
+
+## Overview
+This implementation uses file-based inter-process communication to collect comments from MCP tools and create a single GitHub review.
+
+## Architecture
+
+### 1. Comment Collection (MCP Server)
+- **File**: `src/mcp/comment-collector.ts`
+- **Purpose**: Handles JSONL append-only writes to avoid concurrency issues
+- **Format**: Each line is a JSON object with `type`, `message`, and optional `path`/`line` for inline comments
+
+### 2. Filename Generation (Main Server)  
+- **Location**: `src/review/reviewer.ts`
+- **Format**: `comments-${installationId}-${prNumber}-${uuid}.jsonl`
+- **Passed via**: `COMMENTS_FILE` environment variable to MCP server
+
+### 3. MCP Tools (Collection Mode)
+- **leave_general_comment**: Appends `{"type":"general","message":"..."}`
+- **leave_inline_comment**: Appends `{"type":"inline","message":"...","path":"...","line":123}`
+- **Fallback**: If no collector available, logs warning but continues
+
+### 4. Review Creation (Main Server)
+- **Location**: `src/github/process-review.ts`  
+- **Process**: 
+  1. Reads JSONL file after Amp CLI completion
+  2. Parses comment types and aggregates
+  3. Creates single GitHub review with all comments
+  4. Cleans up temporary file
+
+## Data Flow
+
+```
+1. PR Event → Queue Job
+2. Generate filename: comments-${installationId}-${prNumber}-${uuid}.jsonl
+3. Pass filename via COMMENTS_FILE env var to MCP server
+4. Amp CLI executes → MCP tools append to file
+5. Main server reads file → Creates GitHub review
+6. Clean up temp file
+```
+
+## File Format (JSONL)
+
+```jsonl
+{"type":"inline","message":"Fix this bug","path":"src/auth.js","line":25}
+{"type":"inline","message":"Add error handling","path":"src/api.js","line":10}  
+{"type":"general","message":"Overall looks good, just minor issues above"}
+```
+
+## Concurrency Safety
+- **appendFileSync()**: Atomic at OS level for small writes
+- **JSONL format**: No read-modify-write cycles
+- **Unique filenames**: Prevent collisions across jobs
+
+## Benefits
+- ✅ Simple file-based IPC 
+- ✅ No complex env var passing
+- ✅ Atomic review creation
+- ✅ Robust error handling with cleanup
+- ✅ Maintains tool abstractions
+- ✅ Enables "Request re-review" button
+
+## Error Handling
+- **MCP server crash**: File remains, main server handles gracefully
+- **File corruption**: JSON parsing errors logged, review skipped
+- **Missing file**: Logged and skipped (no comments case)
+- **Cleanup failures**: Logged but don't fail the job
diff --git a/src/github/process-review.ts b/src/github/process-review.ts
@@ -2,7 +2,6 @@ import { getConfig } from "../config.js";
 import { reviewDiff } from "../review/reviewer.js";
 import { GitHubClient } from "./client.js";
 import { CHECK_STATUS, CHECK_CONCLUSION, PRDetails, GitHubPullRequestEvent } from "./types.js";
-import { startReviewSession, endReviewSession } from "../review/comment-collector.js";
 
 export async function processReview(
     jobId: string, 
@@ -62,36 +61,61 @@ export async function processReview(
 
       const prDetailsContent = `Repository: ${payload.repository.full_name}, PR Number: ${prDetails.pr_number}, Commit SHA: ${prDetails.commit_sha}, PR URL: ${prDetails.pr_url}`;
 
-      console.log(`Starting review session for job ${jobId}`);
-      startReviewSession();
-
       console.log(`Calling reviewDiff() for job ${jobId}`);
-      await reviewDiff(diffContent, prDetailsContent, installationId);
+      const reviewResult = await reviewDiff(diffContent, prDetailsContent, installationId);
       console.log(`Review completed for job ${jobId}`);
 
-      // Collect all comments from the review session
-      const collector = endReviewSession();
-      console.log(`Collected ${collector.getInlineComments().length} inline comments and ${collector.getGeneralComments().length} general comments`);
-
-      // Post aggregated review if there are any comments
-      if (collector.hasComments()) {
-        console.log('📋 Posting aggregated PR review...');
-        const reviewSummary = collector.getReviewSummary();
-        const inlineComments = collector.getInlineComments();
-        
-        await githubClient.createPRReview(
-          owner,
-          repo,
-          prNumber,
-          reviewSummary,
-          'COMMENT',
-          inlineComments.map(comment => ({
-            path: comment.path,
-            line: comment.line,
-            body: comment.body
-          }))
-        );
-        console.log('✅ PR review posted successfully');
+      // Read collected comments from file
+      const fs = await import('fs');
+      const commentsFilePath = reviewResult.commentsFilePath;
+      
+      if (fs.existsSync(commentsFilePath)) {
+        try {
+          console.log(`📖 Reading collected comments from ${commentsFilePath}`);
+          const fileContent = fs.readFileSync(commentsFilePath, 'utf8').trim();
+          
+          if (fileContent) {
+            const commentLines = fileContent.split('\n');
+            const comments = commentLines.map(line => JSON.parse(line));
+            
+            const inlineComments = comments.filter(c => c.type === 'inline');
+            const generalComments = comments.filter(c => c.type === 'general');
+            
+            console.log(`📝 Collected ${inlineComments.length} inline comments and ${generalComments.length} general comments`);
+            
+            // Create review summary from general comments
+            const reviewSummary = generalComments.length > 0 
+              ? generalComments.map(c => c.message).join('\n\n')
+              : 'Code review completed.';
+            
+            // Post aggregated review
+            console.log('📋 Posting aggregated PR review...');
+            await githubClient.createPRReview(
+              owner,
+              repo,
+              prNumber,
+              reviewSummary,
+              'COMMENT',
+              inlineComments.map(comment => ({
+                path: comment.path,
+                line: comment.line,
+                body: comment.message
+              }))
+            );
+            console.log('✅ PR review posted successfully');
+          } else {
+            console.log('📝 No comments collected, skipping review creation');
+          }
+          
+          // Clean up the comments file
+          fs.unlinkSync(commentsFilePath);
+          console.log('🗑️ Cleaned up comments file');
+          
+        } catch (error) {
+          console.error('❌ Failed to read or process comments file:', error);
+        }
+      } else {
+        console.log('📝 No comments file found, skipping review creation');
       }
 
       // Update check run with success (simplified since review details are now in PR review)
@@ -108,13 +132,6 @@ export async function processReview(
     } catch (error) {
       console.error(`Review job ${jobId} failed with exception:`, error);
       
-      // End review session if it was started
-      try {
-        endReviewSession();
-      } catch {
-        console.log('No active review session to end');
-      }
-      
       // Post FAILED check run for exceptions
       await postFailureCheckRun(githubClient, payload, error instanceof Error ? error.message : String(error));
   }
diff --git a/src/mcp/comment-collector.ts b/src/mcp/comment-collector.ts
@@ -0,0 +1,68 @@
+import { writeFileSync, appendFileSync } from 'fs';
+
+export interface CommentData {
+  type: 'inline' | 'general';
+  message: string;
+  path?: string;    // Only for inline comments
+  line?: number;    // Only for inline comments
+}
+
+export class FileBasedCommentCollector {
+  private filePath: string;
+
+  constructor(filePath: string) {
+    this.filePath = filePath;
+    // Initialize empty file
+    writeFileSync(this.filePath, '', 'utf8');
+  }
+
+  addInlineComment(path: string, line: number, message: string): void {
+    const comment: CommentData = {
+      type: 'inline',
+      message,
+      path,
+      line
+    };
+
+    this.appendComment(comment);
+  }
+
+  addGeneralComment(message: string): void {
+    const comment: CommentData = {
+      type: 'general',
+      message
+    };
+
+    this.appendComment(comment);
+  }
+
+  private appendComment(comment: CommentData): void {
+    const jsonLine = JSON.stringify(comment) + '\n';
+    appendFileSync(this.filePath, jsonLine, 'utf8');
+  }
+}
+
+// Global collector instance for the MCP server
+let globalCollector: FileBasedCommentCollector | null = null;
+
+export function initializeCollector(): FileBasedCommentCollector | null {
+  const commentsFile = process.env.COMMENTS_FILE;
+
+  if (!commentsFile) {
+    console.log('📝 No COMMENTS_FILE environment variable, collector not initialized');
+    return null;
+  }
+
+  try {
+    globalCollector = new FileBasedCommentCollector(commentsFile);
+    console.log('📝 Comment collector initialized with file:', commentsFile);
+    return globalCollector;
+  } catch (error) {
+    console.error('❌ Failed to initialize comment collector:', error);
+    return null;
+  }
+}
+
+export function getCollector(): FileBasedCommentCollector | null {
+  return globalCollector;
+}
diff --git a/src/mcp/server.ts b/src/mcp/server.ts
@@ -19,6 +19,7 @@ import {
   validateLeaveInlineCommentArgs,
   validateGetPRCommentsArgs
 } from './validation.js';
+import { initializeCollector } from './comment-collector.js';
 
 class GitHubMCPServer {
   private server: Server;
@@ -39,6 +40,7 @@ class GitHubMCPServer {
     );
 
     this.config = getConfig();
+    initializeCollector(); // Initialize comment collector
     this.setupToolHandlers();
   }
 
diff --git a/src/mcp/tools/leave_comment.ts b/src/mcp/tools/leave_comment.ts
@@ -1,4 +1,4 @@
-import { getCurrentCollector } from '../../review/comment-collector.js';
+import { getCollector } from '../comment-collector.js';
 
 export interface LeaveGeneralCommentArgs {
   message: string;
@@ -15,8 +15,12 @@ export async function leaveGeneralComment(
 
     console.log('📝 Collecting general comment for later review');
     
-    const collector = getCurrentCollector();
-    collector.addGeneralComment(message);
+    const collector = getCollector();
+    if (collector) {
+      collector.addGeneralComment(message);
+    } else {
+      console.log('⚠️ No collector available, comment will be skipped');
+    }
     
     return {
       success: true
diff --git a/src/mcp/tools/leave_inline_comment.ts b/src/mcp/tools/leave_inline_comment.ts
@@ -1,4 +1,4 @@
-import { getCurrentCollector } from '../../review/comment-collector.js';
+import { getCollector } from '../comment-collector.js';
 
 export interface LeaveInlineCommentArgs {
   message: string;
@@ -18,8 +18,12 @@ export async function leaveInlineComment(
 
     console.log('📝 Collecting inline comment for later review:', { path, line });
     
-    const collector = getCurrentCollector();
-    collector.addInlineComment(path, line, message);
+    const collector = getCollector();
+    if (collector) {
+      collector.addInlineComment(path, line, message);
+    } else {
+      console.log('⚠️ No collector available, comment will be skipped');
+    }
     
     return {
       success: true
diff --git a/src/review/comment-collector.ts b/src/review/comment-collector.ts
diff --git a/src/review/reviewer.ts b/src/review/reviewer.ts
