Feature: Add PR review comments and resolution, improve AI prompt handling (#790)

* feat: Add PR review comments and resolution endpoints, improve prompt handling

* Feature: File Editor (#789)

* feat: Add file management feature

* feat: Add auto-save functionality to file editor

* fix: Replace HardDriveDownload icon with Save icon for consistency

* fix: Prevent recursive copy/move and improve shell injection prevention

* refactor: Extract editor settings form into separate component

* ```
fix: Improve error handling and stabilize async operations

- Add error event handlers to GraphQL process spawns to prevent unhandled rejections
- Replace execAsync with execFile for safer command execution and better control
- Fix timeout cleanup in withTimeout generator to prevent memory leaks
- Improve outdated comment detection logic by removing redundant condition
- Use resolveModelString for consistent model string handling
- Replace || with ?? for proper falsy value handling in dialog initialization
- Add comments clarifying branch name resolution logic for local branches with slashes
- Add catch handler for project selection to handle async errors gracefully
```

* refactor: Extract PR review comments logic to dedicated service

* fix: Improve robustness and UX for PR review and file operations

* fix: Consolidate exec utilities and improve type safety

* refactor: Replace ScrollArea with div and improve file tree layout

apps/server/src/routes/fs/routes/write.ts

@@ -24,7 +24,9 @@ export function createWriteHandler() {
 
       // Ensure parent directory exists (symlink-safe)
       await mkdirSafe(path.dirname(path.resolve(filePath)));
-      await secureFs.writeFile(filePath, content, 'utf-8');
+      // Default content to empty string if undefined/null to prevent writing
+      // "undefined" as literal text (e.g. when content field is missing from request)
+      await secureFs.writeFile(filePath, content ?? '', 'utf-8');
 
       res.json({ success: true });
     } catch (error) {

apps/server/src/routes/github/index.ts

@@ -9,6 +9,8 @@ import { createCheckGitHubRemoteHandler } from './routes/check-github-remote.js'
 import { createListIssuesHandler } from './routes/list-issues.js';
 import { createListPRsHandler } from './routes/list-prs.js';
 import { createListCommentsHandler } from './routes/list-comments.js';
+import { createListPRReviewCommentsHandler } from './routes/list-pr-review-comments.js';
+import { createResolvePRCommentHandler } from './routes/resolve-pr-comment.js';
 import { createValidateIssueHandler } from './routes/validate-issue.js';
 import {
   createValidationStatusHandler,
@@ -29,6 +31,16 @@ export function createGitHubRoutes(
   router.post('/issues', validatePathParams('projectPath'), createListIssuesHandler());
   router.post('/prs', validatePathParams('projectPath'), createListPRsHandler());
   router.post('/issue-comments', validatePathParams('projectPath'), createListCommentsHandler());
+  router.post(
+    '/pr-review-comments',
+    validatePathParams('projectPath'),
+    createListPRReviewCommentsHandler()
+  );
+  router.post(
+    '/resolve-pr-comment',
+    validatePathParams('projectPath'),
+    createResolvePRCommentHandler()
+  );
   router.post(
     '/validate-issue',
     validatePathParams('projectPath'),

apps/server/src/routes/github/routes/common.ts

@@ -1,38 +1,14 @@
 /**
  * Common utilities for GitHub routes
+ *
+ * Re-exports shared utilities from lib/exec-utils so route consumers
+ * can continue importing from this module unchanged.
  */
 
 import { exec } from 'child_process';
 import { promisify } from 'util';
-import { createLogger } from '@automaker/utils';
-
-const logger = createLogger('GitHub');
 
 export const execAsync = promisify(exec);
 
-// Extended PATH to include common tool installation locations
-export const extendedPath = [
-  process.env.PATH,
-  '/opt/homebrew/bin',
-  '/usr/local/bin',
-  '/home/linuxbrew/.linuxbrew/bin',
-  `${process.env.HOME}/.local/bin`,
-]
-  .filter(Boolean)
-  .join(':');
-
-export const execEnv = {
-  ...process.env,
-  PATH: extendedPath,
-};
-
-export function getErrorMessage(error: unknown): string {
-  if (error instanceof Error) {
-    return error.message;
-  }
-  return String(error);
-}
-
-export function logError(error: unknown, context: string): void {
-  logger.error(`${context}:`, error);
-}
+// Re-export shared utilities from the canonical location
+export { extendedPath, execEnv, getErrorMessage, logError } from '../../../lib/exec-utils.js';

apps/server/src/routes/github/routes/list-pr-review-comments.ts

@@ -0,0 +1,72 @@
+/**
+ * POST /pr-review-comments endpoint - Fetch review comments for a GitHub PR
+ *
+ * Fetches both regular PR comments and inline code review comments
+ * for a specific pull request, providing file path and line context.
+ */
+
+import type { Request, Response } from 'express';
+import { getErrorMessage, logError } from './common.js';
+import { checkGitHubRemote } from './check-github-remote.js';
+import {
+  fetchPRReviewComments,
+  fetchReviewThreadResolvedStatus,
+  type PRReviewComment,
+  type ListPRReviewCommentsResult,
+} from '../../../services/pr-review-comments.service.js';
+
+// Re-export types so existing callers continue to work
+export type { PRReviewComment, ListPRReviewCommentsResult };
+// Re-export service functions so existing callers continue to work
+export { fetchPRReviewComments, fetchReviewThreadResolvedStatus };
+
+interface ListPRReviewCommentsRequest {
+  projectPath: string;
+  prNumber: number;
+}
+
+export function createListPRReviewCommentsHandler() {
+  return async (req: Request, res: Response): Promise<void> => {
+    try {
+      const { projectPath, prNumber } = req.body as ListPRReviewCommentsRequest;
+
+      if (!projectPath) {
+        res.status(400).json({ success: false, error: 'projectPath is required' });
+        return;
+      }
+
+      if (!prNumber || typeof prNumber !== 'number') {
+        res
+          .status(400)
+          .json({ success: false, error: 'prNumber is required and must be a number' });
+        return;
+      }
+
+      // Check if this is a GitHub repo and get owner/repo
+      const remoteStatus = await checkGitHubRemote(projectPath);
+      if (!remoteStatus.hasGitHubRemote || !remoteStatus.owner || !remoteStatus.repo) {
+        res.status(400).json({
+          success: false,
+          error: 'Project does not have a GitHub remote',
+        });
+        return;
+      }
+
+      const comments = await fetchPRReviewComments(
+        projectPath,
+        remoteStatus.owner,
+        remoteStatus.repo,
+        prNumber
+      );
+
+      res.json({
+        success: true,
+        comments,
+        totalCount: comments.length,
+      });
+    } catch (error) {
+      logError(error, 'Fetch PR review comments failed');
+      res.status(500).json({ success: false, error: getErrorMessage(error) });
+    }
+  };
+}

apps/server/src/routes/github/routes/resolve-pr-comment.ts

@@ -0,0 +1,66 @@
+/**
+ * POST /resolve-pr-comment endpoint - Resolve or unresolve a GitHub PR review thread
+ *
+ * Uses the GitHub GraphQL API to resolve or unresolve a review thread
+ * identified by its GraphQL node ID (threadId).
+ */
+
+import type { Request, Response } from 'express';
+import { getErrorMessage, logError } from './common.js';
+import { checkGitHubRemote } from './check-github-remote.js';
+import { executeReviewThreadMutation } from '../../../services/github-pr-comment.service.js';
+
+export interface ResolvePRCommentResult {
+  success: boolean;
+  isResolved?: boolean;
+  error?: string;
+}
+
+interface ResolvePRCommentRequest {
+  projectPath: string;
+  threadId: string;
+  resolve: boolean;
+}
+
+export function createResolvePRCommentHandler() {
+  return async (req: Request, res: Response): Promise<void> => {
+    try {
+      const { projectPath, threadId, resolve } = req.body as ResolvePRCommentRequest;
+
+      if (!projectPath) {
+        res.status(400).json({ success: false, error: 'projectPath is required' });
+        return;
+      }
+
+      if (!threadId) {
+        res.status(400).json({ success: false, error: 'threadId is required' });
+        return;
+      }
+
+      if (typeof resolve !== 'boolean') {
+        res.status(400).json({ success: false, error: 'resolve must be a boolean' });
+        return;
+      }
+
+      // Check if this is a GitHub repo
+      const remoteStatus = await checkGitHubRemote(projectPath);
+      if (!remoteStatus.hasGitHubRemote) {
+        res.status(400).json({
+          success: false,
+          error: 'Project does not have a GitHub remote',
+        });
+        return;
+      }
+
+      const result = await executeReviewThreadMutation(projectPath, threadId, resolve);
+
+      res.json({
+        success: true,
+        isResolved: result.isResolved,
+      });
+    } catch (error) {
+      logError(error, 'Resolve PR comment failed');
+      res.status(500).json({ success: false, error: getErrorMessage(error) });
+    }
+  };
+}

apps/server/src/routes/worktree/routes/generate-commit-message.ts

@@ -6,7 +6,7 @@
  */
 
 import type { Request, Response } from 'express';
-import { exec } from 'child_process';
+import { execFile } from 'child_process';
 import { promisify } from 'util';
 import { existsSync } from 'fs';
 import { join } from 'path';
@@ -20,7 +20,7 @@ import { getErrorMessage, logError } from '../common.js';
 import { getPhaseModelWithOverrides } from '../../../lib/settings-helpers.js';
 
 const logger = createLogger('GenerateCommitMessage');
-const execAsync = promisify(exec);
+const execFileAsync = promisify(execFile);
 
 /** Timeout for AI provider calls in milliseconds (30 seconds) */
 const AI_TIMEOUT_MS = 30_000;
@@ -33,20 +33,39 @@ async function* withTimeout<T>(
   generator: AsyncIterable<T>,
   timeoutMs: number
 ): AsyncGenerator<T, void, unknown> {
+  let timerId: ReturnType<typeof setTimeout> | undefined;
+
   const timeoutPromise = new Promise<never>((_, reject) => {
-    setTimeout(() => reject(new Error(`AI provider timed out after ${timeoutMs}ms`)), timeoutMs);
+    timerId = setTimeout(
+      () => reject(new Error(`AI provider timed out after ${timeoutMs}ms`)),
+      timeoutMs
+    );
   });
 
   const iterator = generator[Symbol.asyncIterator]();
   let done = false;
 
-  while (!done) {
-    const result = await Promise.race([iterator.next(), timeoutPromise]);
-    if (result.done) {
-      done = true;
-    } else {
-      yield result.value;
+  try {
+    while (!done) {
+      const result = await Promise.race([iterator.next(), timeoutPromise]).catch(async (err) => {
+        // Capture the original error, then attempt to close the iterator.
+        // If iterator.return() throws, log it but rethrow the original error
+        // so the timeout error (not the teardown error) is preserved.
+        try {
+          await iterator.return?.();
+        } catch (teardownErr) {
+          logger.warn('Error during iterator cleanup after timeout:', teardownErr);
+        }
+        throw err;
+      });
+      if (result.done) {
+        done = true;
+      } else {
+        yield result.value;
+      }
     }
+  } finally {
+    clearTimeout(timerId);
   }
 }
 
@@ -117,14 +136,14 @@ export function createGenerateCommitMessageHandler(
       let diff = '';
       try {
         // First try to get staged changes
-        const { stdout: stagedDiff } = await execAsync('git diff --cached', {
+        const { stdout: stagedDiff } = await execFileAsync('git', ['diff', '--cached'], {
           cwd: worktreePath,
           maxBuffer: 1024 * 1024 * 5, // 5MB buffer
         });
 
         // If no staged changes, get unstaged changes
         if (!stagedDiff.trim()) {
-          const { stdout: unstagedDiff } = await execAsync('git diff', {
+          const { stdout: unstagedDiff } = await execFileAsync('git', ['diff'], {
             cwd: worktreePath,
             maxBuffer: 1024 * 1024 * 5, // 5MB buffer
           });
@@ -213,14 +232,16 @@ export function createGenerateCommitMessageHandler(
             }
           }
         } else if (msg.type === 'result' && msg.subtype === 'success' && msg.result) {
-          // Use result if available (some providers return final text here)
-          responseText = msg.result;
+          // Use result text if longer than accumulated text (consistent with simpleQuery pattern)
+          if (msg.result.length > responseText.length) {
+            responseText = msg.result;
+          }
         }
       }
 
       const message = responseText.trim();
 
-      if (!message || message.trim().length === 0) {
+      if (!message) {
         logger.warn('Received empty response from model');
         const response: GenerateCommitMessageErrorResponse = {
           success: false,

apps/server/src/routes/worktree/routes/generate-pr-description.ts

@@ -30,6 +30,8 @@ const MAX_DIFF_SIZE = 15_000;
 
 const PR_DESCRIPTION_SYSTEM_PROMPT = `You are a pull request description generator. Your task is to create a clear, well-structured PR title and description based on the git diff and branch information provided.
 
+IMPORTANT: Do NOT include any conversational text, explanations, or preamble. Do NOT say things like "I'll analyze..." or "Here is...". Output ONLY the structured format below and nothing else.
+
 Output your response in EXACTLY this format (including the markers):
 ---TITLE---
 <a concise PR title, 50-72 chars, imperative mood>
@@ -41,6 +43,7 @@ Output your response in EXACTLY this format (including the markers):
 <Detailed list of what was changed and why>
 
 Rules:
+- Your ENTIRE response must start with ---TITLE--- and contain nothing before it
 - The title should be concise and descriptive (50-72 characters)
 - Use imperative mood for the title (e.g., "Add dark mode toggle" not "Added dark mode toggle")
 - The description should explain WHAT changed and WHY
@@ -397,7 +400,10 @@ export function createGeneratePRDescriptionHandler(
             }
           }
         } else if (msg.type === 'result' && msg.subtype === 'success' && msg.result) {
-          responseText = msg.result;
+          // Use result text if longer than accumulated text (consistent with simpleQuery pattern)
+          if (msg.result.length > responseText.length) {
+            responseText = msg.result;
+          }
         }
       }
 
@@ -413,7 +419,9 @@ export function createGeneratePRDescriptionHandler(
         return;
       }
 
-      // Parse the response to extract title and body
+      // Parse the response to extract title and body.
+      // The model may include conversational preamble before the structured markers,
+      // so we search for the markers anywhere in the response, not just at the start.
       let title = '';
       let body = '';
 
@@ -424,14 +432,46 @@ export function createGeneratePRDescriptionHandler(
         title = titleMatch[1].trim();
         body = bodyMatch[1].trim();
       } else {
-        // Fallback: treat first line as title, rest as body
-        const lines = fullResponse.split('\n');
-        title = lines[0].trim();
-        body = lines.slice(1).join('\n').trim();
+        // Fallback: try to extract meaningful content, skipping any conversational preamble.
+        // Common preamble patterns start with "I'll", "I will", "Here", "Let me", "Based on", etc.
+        const lines = fullResponse.split('\n').filter((line) => line.trim().length > 0);
+
+        // Skip lines that look like conversational preamble
+        let startIndex = 0;
+        for (let i = 0; i < lines.length; i++) {
+          const line = lines[i].trim();
+          // Check if this line looks like conversational AI preamble
+          if (
+            /^(I'll|I will|Here('s| is| are)|Let me|Based on|Looking at|Analyzing|Sure|OK|Okay|Of course)/i.test(
+              line
+            ) ||
+            /^(The following|Below is|This (is|will)|After (analyzing|reviewing|looking))/i.test(
+              line
+            )
+          ) {
+            startIndex = i + 1;
+            continue;
+          }
+          break;
+        }
+
+        // Use remaining lines after skipping preamble
+        const contentLines = lines.slice(startIndex);
+        if (contentLines.length > 0) {
+          title = contentLines[0].trim();
+          body = contentLines.slice(1).join('\n').trim();
+        } else {
+          // If all lines were filtered as preamble, use the original first non-empty line
+          title = lines[0]?.trim() || '';
+          body = lines.slice(1).join('\n').trim();
+        }
       }
 
-      // Clean up title - remove any markdown or quotes
-      title = title.replace(/^#+\s*/, '').replace(/^["']|["']$/g, '');
+      // Clean up title - remove any markdown headings, quotes, or marker artifacts
+      title = title
+        .replace(/^#+\s*/, '')
+        .replace(/^["']|["']$/g, '')
+        .replace(/^---\w+---\s*/, '');
 
       logger.info(`Generated PR title: ${title.substring(0, 100)}...`);