fix: Improve error handling and validation across multiple services

apps/server/src/services/auto-mode/facade.ts

@@ -519,17 +519,15 @@ export class AutoModeServiceFacade {
     useWorktrees = false,
     _calledInternally = false
   ): Promise<void> {
-    try {
-      return await this.recoveryService.resumeFeature(
-        this.projectPath,
-        featureId,
-        useWorktrees,
-        _calledInternally
-      );
-    } catch (error) {
-      this.handleFacadeError(error, 'resumeFeature', featureId);
-      throw error;
-    }
+    // Do not call handleFacadeError here: ExecutionService.executeFeature already
+    // classifies and emits auto_mode_error, so calling handleFacadeError would
+    // produce duplicate error events. Simply let the error propagate to the caller.
+    return await this.recoveryService.resumeFeature(
+      this.projectPath,
+      featureId,
+      useWorktrees,
+      _calledInternally
+    );
   }
 
   /**

apps/server/src/services/branch-utils.ts

@@ -0,0 +1,155 @@
+/**
+ * branch-utils - Shared git branch helper utilities
+ *
+ * Provides common git operations used by both checkout-branch-service and
+ * worktree-branch-service. Extracted to avoid duplication and ensure
+ * consistent behaviour across branch-related services.
+ */
+
+import { createLogger, getErrorMessage } from '@automaker/utils';
+import { execGitCommand, execGitCommandWithLockRetry } from '../lib/git.js';
+
+const logger = createLogger('BranchUtils');
+
+// ============================================================================
+// Types
+// ============================================================================
+
+export interface HasAnyChangesOptions {
+  /**
+   * When true, lines that refer to worktree-internal paths (containing
+   * ".worktrees/" or ending with ".worktrees") are excluded from the count.
+   * Use this in contexts where worktree directory entries should not be
+   * considered as real working-tree changes (e.g. worktree-branch-service).
+   */
+  excludeWorktreePaths?: boolean;
+}
+
+// ============================================================================
+// Helpers
+// ============================================================================
+
+/**
+ * Returns true when a `git status --porcelain` output line refers to a
+ * worktree-internal path that should be ignored when deciding whether there
+ * are "real" local changes.
+ */
+function isExcludedWorktreeLine(line: string): boolean {
+  return line.includes('.worktrees/') || line.endsWith('.worktrees');
+}
+
+// ============================================================================
+// Exported Utilities
+// ============================================================================
+
+/**
+ * Check if there are any changes (including untracked) that should be stashed.
+ *
+ * @param cwd - Working directory of the git repository / worktree
+ * @param options - Optional flags controlling which lines are counted
+ * @param options.excludeWorktreePaths - When true, lines matching worktree
+ *   internal paths are excluded so they are not mistaken for real changes
+ */
+export async function hasAnyChanges(cwd: string, options?: HasAnyChangesOptions): Promise<boolean> {
+  try {
+    const stdout = await execGitCommand(['status', '--porcelain'], cwd);
+    const lines = stdout
+      .trim()
+      .split('\n')
+      .filter((line) => {
+        if (!line.trim()) return false;
+        if (options?.excludeWorktreePaths && isExcludedWorktreeLine(line)) return false;
+        return true;
+      });
+    return lines.length > 0;
+  } catch (err) {
+    logger.error('hasAnyChanges: execGitCommand failed — returning false', {
+      cwd,
+      error: getErrorMessage(err),
+    });
+    return false;
+  }
+}
+
+/**
+ * Stash all local changes (including untracked files if requested).
+ * Returns true if a stash was created, false if there was nothing to stash.
+ * Throws on unexpected errors so callers abort rather than proceeding silently.
+ *
+ * @param cwd - Working directory of the git repository / worktree
+ * @param message - Stash message
+ * @param includeUntracked - When true, passes `--include-untracked` to git stash
+ */
+export async function stashChanges(
+  cwd: string,
+  message: string,
+  includeUntracked: boolean = true
+): Promise<boolean> {
+  try {
+    const args = ['stash', 'push'];
+    if (includeUntracked) {
+      args.push('--include-untracked');
+    }
+    args.push('-m', message);
+
+    await execGitCommandWithLockRetry(args, cwd);
+    return true;
+  } catch (error) {
+    const errorMsg = getErrorMessage(error);
+
+    // "Nothing to stash" is benign – no work was lost, just return false
+    if (
+      errorMsg.toLowerCase().includes('no local changes to save') ||
+      errorMsg.toLowerCase().includes('nothing to stash')
+    ) {
+      logger.debug('stashChanges: nothing to stash', { cwd, message, error: errorMsg });
+      return false;
+    }
+
+    // Unexpected error – log full details and re-throw so the caller aborts
+    // rather than proceeding with an un-stashed working tree
+    logger.error('stashChanges: unexpected error during stash', {
+      cwd,
+      message,
+      error: errorMsg,
+    });
+    throw new Error(`Failed to stash changes in ${cwd}: ${errorMsg}`);
+  }
+}
+
+/**
+ * Pop the most recent stash entry.
+ * Returns an object indicating success and whether there were conflicts.
+ *
+ * @param cwd - Working directory of the git repository / worktree
+ */
+export async function popStash(
+  cwd: string
+): Promise<{ success: boolean; hasConflicts: boolean; error?: string }> {
+  try {
+    await execGitCommand(['stash', 'pop'], cwd);
+    // If execGitCommand succeeds (zero exit code), there are no conflicts
+    return { success: true, hasConflicts: false };
+  } catch (error) {
+    const errorMsg = getErrorMessage(error);
+    if (errorMsg.includes('CONFLICT') || errorMsg.includes('Merge conflict')) {
+      return { success: false, hasConflicts: true, error: errorMsg };
+    }
+    return { success: false, hasConflicts: false, error: errorMsg };
+  }
+}
+
+/**
+ * Check if a local branch already exists.
+ *
+ * @param cwd - Working directory of the git repository / worktree
+ * @param branchName - The branch name to look up (without refs/heads/ prefix)
+ */
+export async function localBranchExists(cwd: string, branchName: string): Promise<boolean> {
+  try {
+    await execGitCommand(['rev-parse', '--verify', `refs/heads/${branchName}`], cwd);
+    return true;
+  } catch {
+    return false;
+  }
+}

apps/server/src/services/checkout-branch-service.ts

@@ -19,11 +19,10 @@
  * 7. Handle error recovery (restore stash if checkout fails)
  */
 
-import { createLogger, getErrorMessage } from '@automaker/utils';
-import { execGitCommand, execGitCommandWithLockRetry } from '../lib/git.js';
+import { getErrorMessage } from '@automaker/utils';
+import { execGitCommand } from '../lib/git.js';
 import type { EventEmitter } from '../lib/events.js';
-
-const logger = createLogger('CheckoutBranchService');
+import { hasAnyChanges, stashChanges, popStash, localBranchExists } from './branch-utils.js';
 
 // ============================================================================
 // Types
@@ -52,101 +51,6 @@ export interface CheckoutBranchResult {
   stashPopConflictMessage?: string;
 }
 
-// ============================================================================
-// Helper Functions
-// ============================================================================
-
-/**
- * Check if there are any changes (including untracked) that should be stashed
- */
-async function hasAnyChanges(cwd: string): Promise<boolean> {
-  try {
-    const stdout = await execGitCommand(['status', '--porcelain'], cwd);
-    const lines = stdout
-      .trim()
-      .split('\n')
-      .filter((line) => line.trim());
-    return lines.length > 0;
-  } catch (err) {
-    logger.error('hasAnyChanges: execGitCommand failed — returning false', {
-      cwd,
-      error: getErrorMessage(err),
-    });
-    return false;
-  }
-}
-
-/**
- * Stash all local changes (including untracked files if requested)
- * Returns true if a stash was created, false if there was nothing to stash.
- * Throws on unexpected errors so callers abort rather than proceeding silently.
- */
-async function stashChanges(
-  cwd: string,
-  message: string,
-  includeUntracked: boolean
-): Promise<boolean> {
-  try {
-    const args = ['stash', 'push'];
-    if (includeUntracked) {
-      args.push('--include-untracked');
-    }
-    args.push('-m', message);
-
-    await execGitCommandWithLockRetry(args, cwd);
-    return true;
-  } catch (error) {
-    const errorMsg = getErrorMessage(error);
-
-    // "Nothing to stash" is benign
-    if (
-      errorMsg.toLowerCase().includes('no local changes to save') ||
-      errorMsg.toLowerCase().includes('nothing to stash')
-    ) {
-      logger.debug('stashChanges: nothing to stash', { cwd, message, error: errorMsg });
-      return false;
-    }
-
-    logger.error('stashChanges: unexpected error during stash', {
-      cwd,
-      message,
-      error: errorMsg,
-    });
-    throw new Error(`Failed to stash changes in ${cwd}: ${errorMsg}`);
-  }
-}
-
-/**
- * Pop the most recent stash entry
- * Returns an object indicating success and whether there were conflicts
- */
-async function popStash(
-  cwd: string
-): Promise<{ success: boolean; hasConflicts: boolean; error?: string }> {
-  try {
-    await execGitCommand(['stash', 'pop'], cwd);
-    return { success: true, hasConflicts: false };
-  } catch (error) {
-    const errorMsg = getErrorMessage(error);
-    if (errorMsg.includes('CONFLICT') || errorMsg.includes('Merge conflict')) {
-      return { success: false, hasConflicts: true, error: errorMsg };
-    }
-    return { success: false, hasConflicts: false, error: errorMsg };
-  }
-}
-
-/**
- * Check if a local branch already exists
- */
-async function localBranchExists(cwd: string, branchName: string): Promise<boolean> {
-  try {
-    await execGitCommand(['rev-parse', '--verify', `refs/heads/${branchName}`], cwd);
-    return true;
-  } catch {
-    return false;
-  }
-}
-
 // ============================================================================
 // Main Service Function
 // ============================================================================
@@ -175,11 +79,25 @@ export async function performCheckoutBranch(
   events?.emit('switch:start', { worktreePath, branchName, operation: 'checkout' });
 
   // 1. Get current branch
-  const currentBranchOutput = await execGitCommand(
-    ['rev-parse', '--abbrev-ref', 'HEAD'],
-    worktreePath
-  );
-  const previousBranch = currentBranchOutput.trim();
+  let previousBranch: string;
+  try {
+    const currentBranchOutput = await execGitCommand(
+      ['rev-parse', '--abbrev-ref', 'HEAD'],
+      worktreePath
+    );
+    previousBranch = currentBranchOutput.trim();
+  } catch (branchError) {
+    const branchErrorMsg = getErrorMessage(branchError);
+    events?.emit('switch:error', {
+      worktreePath,
+      branchName,
+      error: branchErrorMsg,
+    });
+    return {
+      success: false,
+      error: `Failed to determine current branch: ${branchErrorMsg}`,
+    };
+  }
 
   // 2. Check if branch already exists
   if (await localBranchExists(worktreePath, branchName)) {

apps/server/src/services/merge-service.ts

@@ -6,7 +6,7 @@
 
 import { createLogger, isValidBranchName } from '@automaker/utils';
 import { type EventEmitter } from '../lib/events.js';
-import { execGitCommand } from '../lib/git.js';
+import { execGitCommand } from '@automaker/git-utils';
 const logger = createLogger('MergeService');
 
 export interface MergeOptions {
@@ -157,9 +157,22 @@ export async function performMerge(
       });
       // Unmerged status codes occupy the first two characters of each line.
       // Standard unmerged codes: UU, AA, DD, AU, UA, DU, UD.
-      hasUnmergedPaths = statusOutput
+      const unmergedLines = statusOutput
         .split('\n')
-        .some((line) => /^(UU|AA|DD|AU|UA|DU|UD)/.test(line));
+        .filter((line) => /^(UU|AA|DD|AU|UA|DU|UD)/.test(line));
+      hasUnmergedPaths = unmergedLines.length > 0;
+
+      // If Layer 2 did not populate conflictFiles (e.g. diff failed or returned
+      // nothing) but Layer 3 does detect unmerged paths, parse the status lines
+      // to extract filenames and assign them to conflictFiles so callers always
+      // receive an accurate file list when conflicts are present.
+      if (hasUnmergedPaths && conflictFiles === undefined) {
+        const parsedFiles = unmergedLines
+          .map((line) => line.slice(2).trim())
+          .filter((f) => f.length > 0);
+        // Deduplicate (e.g. rename entries can appear twice)
+        conflictFiles = [...new Set(parsedFiles)];
+      }
     } catch {
       // git status failing is itself a sign something is wrong; leave
       // hasUnmergedPaths as false and rely on the other layers.

apps/server/src/services/pipeline-orchestrator.ts

@@ -519,7 +519,8 @@ export class PipelineOrchestrator {
         targetBranch || 'main',
         {
           deleteWorktreeAndBranch: false,
-        }
+        },
+        this.eventBus.getUnderlyingEmitter()
       );
 
       if (!result.success) {

apps/server/src/services/pull-service.ts

@@ -80,7 +80,11 @@ export async function getLocalChanges(
       .trim()
       .split('\n')
       .filter((line) => line.trim().length > 0)
-      .map((line) => line.substring(3).trim());
+      .map((line) => {
+        const entry = line.substring(3).trim();
+        const arrowIndex = entry.indexOf(' -> ');
+        return arrowIndex !== -1 ? entry.substring(arrowIndex + 4).trim() : entry;
+      });
   }
 
   return { hasLocalChanges, localChangedFiles };
@@ -321,7 +325,11 @@ export async function performPull(
 
     if (isConflictError(errorOutput)) {
       pullConflict = true;
-      pullConflictFiles = await getConflictFiles(worktreePath);
+      try {
+        pullConflictFiles = await getConflictFiles(worktreePath);
+      } catch {
+        pullConflictFiles = [];
+      }
     } else {
       // Non-conflict pull error
       let stashRecoveryFailed = false;
@@ -407,7 +415,12 @@ async function reapplyStash(worktreePath: string, branchName: string): Promise<P
     // Check if stash pop failed due to conflicts
     // The stash remains in the stash list when conflicts occur, so stashRestored is false
     if (isStashConflict(errorOutput)) {
-      const stashConflictFiles = await getConflictFiles(worktreePath);
+      let stashConflictFiles: string[] = [];
+      try {
+        stashConflictFiles = await getConflictFiles(worktreePath);
+      } catch {
+        stashConflictFiles = [];
+      }
 
       return {
         success: true,

apps/server/src/services/rebase-service.ts

@@ -40,12 +40,12 @@ export interface RebaseResult {
  * @returns RebaseResult with success/failure information
  */
 export async function runRebase(worktreePath: string, ontoBranch: string): Promise<RebaseResult> {
-  // Reject branch names that start with a dash to prevent them from being
-  // misinterpreted as git options.
-  if (ontoBranch.startsWith('-')) {
+  // Reject empty, whitespace-only, or dash-prefixed branch names.
+  const normalizedOntoBranch = ontoBranch?.trim() ?? '';
+  if (normalizedOntoBranch === '' || normalizedOntoBranch.startsWith('-')) {
     return {
       success: false,
-      error: `Invalid branch name: "${ontoBranch}" must not start with a dash.`,
+      error: `Invalid branch name: "${ontoBranch}" must not be empty or start with a dash.`,
     };
   }
 

apps/server/src/services/worktree-branch-service.ts

@@ -17,8 +17,9 @@
  */
 
 import { createLogger, getErrorMessage } from '@automaker/utils';
-import { execGitCommand, execGitCommandWithLockRetry } from '../lib/git.js';
+import { execGitCommand } from '../lib/git.js';
 import type { EventEmitter } from '../lib/events.js';
+import { hasAnyChanges, stashChanges, popStash, localBranchExists } from './branch-utils.js';
 
 const logger = createLogger('WorktreeBranchService');
 
@@ -43,92 +44,9 @@ export interface SwitchBranchResult {
 }
 
 // ============================================================================
-// Helper Functions
+// Local Helpers
 // ============================================================================
 
-function isExcludedWorktreeLine(line: string): boolean {
-  return line.includes('.worktrees/') || line.endsWith('.worktrees');
-}
-
-/**
- * Check if there are any changes at all (including untracked) that should be stashed
- */
-async function hasAnyChanges(cwd: string): Promise<boolean> {
-  try {
-    const stdout = await execGitCommand(['status', '--porcelain'], cwd);
-    const lines = stdout
-      .trim()
-      .split('\n')
-      .filter((line) => {
-        if (!line.trim()) return false;
-        if (isExcludedWorktreeLine(line)) return false;
-        return true;
-      });
-    return lines.length > 0;
-  } catch (err) {
-    logger.error('hasAnyChanges: execGitCommand failed — returning false', {
-      cwd,
-      error: getErrorMessage(err),
-    });
-    return false;
-  }
-}
-
-/**
- * Stash all local changes (including untracked files)
- * Returns true if a stash was created, false if there was nothing to stash.
- * Throws on unexpected errors so callers abort rather than proceeding silently.
- */
-async function stashChanges(cwd: string, message: string): Promise<boolean> {
-  try {
-    // Stash including untracked files — a successful execGitCommand is proof
-    // the stash was created. No need for a post-push listing which can throw
-    // and incorrectly report a failed stash.
-    await execGitCommandWithLockRetry(['stash', 'push', '--include-untracked', '-m', message], cwd);
-    return true;
-  } catch (error) {
-    const errorMsg = getErrorMessage(error);
-
-    // "Nothing to stash" is benign – no work was lost, just return false
-    if (
-      errorMsg.toLowerCase().includes('no local changes to save') ||
-      errorMsg.toLowerCase().includes('nothing to stash')
-    ) {
-      logger.debug('stashChanges: nothing to stash', { cwd, message, error: errorMsg });
-      return false;
-    }
-
-    // Unexpected error – log full details and re-throw so the caller aborts
-    // rather than proceeding with an un-stashed working tree
-    logger.error('stashChanges: unexpected error during stash', {
-      cwd,
-      message,
-      error: errorMsg,
-    });
-    throw new Error(`Failed to stash changes in ${cwd}: ${errorMsg}`);
-  }
-}
-
-/**
- * Pop the most recent stash entry
- * Returns an object indicating success and whether there were conflicts
- */
-async function popStash(
-  cwd: string
-): Promise<{ success: boolean; hasConflicts: boolean; error?: string }> {
-  try {
-    await execGitCommand(['stash', 'pop'], cwd);
-    // If execGitCommand succeeds (zero exit code), there are no conflicts
-    return { success: true, hasConflicts: false };
-  } catch (error) {
-    const errorMsg = getErrorMessage(error);
-    if (errorMsg.includes('CONFLICT') || errorMsg.includes('Merge conflict')) {
-      return { success: false, hasConflicts: true, error: errorMsg };
-    }
-    return { success: false, hasConflicts: false, error: errorMsg };
-  }
-}
-
 /** Timeout for git fetch operations (30 seconds) */
 const FETCH_TIMEOUT_MS = 30_000;
 
@@ -200,18 +118,6 @@ async function isRemoteBranch(cwd: string, branchName: string): Promise<boolean>
   }
 }
 
-/**
- * Check if a local branch already exists
- */
-async function localBranchExists(cwd: string, branchName: string): Promise<boolean> {
-  try {
-    await execGitCommand(['rev-parse', '--verify', `refs/heads/${branchName}`], cwd);
-    return true;
-  } catch {
-    return false;
-  }
-}
-
 // ============================================================================
 // Main Service Function
 // ============================================================================
@@ -308,7 +214,7 @@ export async function performSwitchBranch(
   }
 
   // 5. Stash local changes if any exist
-  const hadChanges = await hasAnyChanges(worktreePath);
+  const hadChanges = await hasAnyChanges(worktreePath, { excludeWorktreePaths: true });
   let didStash = false;
 
   if (hadChanges) {
@@ -320,7 +226,7 @@ export async function performSwitchBranch(
     });
     const stashMessage = `automaker-branch-switch: ${previousBranch} → ${targetBranch}`;
     try {
-      didStash = await stashChanges(worktreePath, stashMessage);
+      didStash = await stashChanges(worktreePath, stashMessage, true);
     } catch (stashError) {
       const stashErrorMsg = getErrorMessage(stashError);
       events?.emit('switch:error', {