feat: Fix new branch issues and address code review comments

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

@@ -22,7 +22,7 @@ import { getFeatureDir } from '@automaker/platform';
 import * as secureFs from '../../lib/secure-fs.js';
 import { validateWorkingDirectory } from '../../lib/sdk-options.js';
 import { getPromptCustomization, getProviderByModelId } from '../../lib/settings-helpers.js';
-import { execGitCommand } from '../../lib/git.js';
+import { execGitCommand } from '@automaker/git-utils';
 import { TypedEventBus } from '../typed-event-bus.js';
 import { ConcurrencyManager } from '../concurrency-manager.js';
 import { WorktreeResolver } from '../worktree-resolver.js';

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

@@ -0,0 +1,383 @@
+/**
+ * CheckoutBranchService - Create and checkout a new branch with stash handling
+ *
+ * Handles new branch creation with automatic stash/reapply of local changes.
+ * If there are uncommitted changes and the caller requests stashing, they are
+ * stashed before creating the branch and reapplied after. If the stash pop
+ * results in merge conflicts, returns a special response so the UI can create
+ * a conflict resolution task.
+ *
+ * Follows the same pattern as worktree-branch-service.ts (performSwitchBranch).
+ *
+ * The workflow:
+ * 1. Validate inputs (branch name, base branch)
+ * 2. Get current branch name
+ * 3. Check if target branch already exists
+ * 4. Optionally stash local changes
+ * 5. Create and checkout the new branch
+ * 6. Reapply stashed changes (detect conflicts)
+ * 7. Handle error recovery (restore stash if checkout fails)
+ */
+
+import { createLogger, getErrorMessage } from '@automaker/utils';
+import { execGitCommand, execGitCommandWithLockRetry } from '../lib/git.js';
+import type { EventEmitter } from '../lib/events.js';
+
+const logger = createLogger('CheckoutBranchService');
+
+// ============================================================================
+// Types
+// ============================================================================
+
+export interface CheckoutBranchOptions {
+  /** When true, stash local changes before checkout and reapply after */
+  stashChanges?: boolean;
+  /** When true, include untracked files in the stash */
+  includeUntracked?: boolean;
+}
+
+export interface CheckoutBranchResult {
+  success: boolean;
+  error?: string;
+  result?: {
+    previousBranch: string;
+    newBranch: string;
+    message: string;
+    hasConflicts?: boolean;
+    stashedChanges?: boolean;
+  };
+  /** Set when checkout fails and stash pop produced conflicts during recovery */
+  stashPopConflicts?: boolean;
+  /** Human-readable message when stash pop conflicts occur during error recovery */
+  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
+// ============================================================================
+
+/**
+ * Create and checkout a new branch, optionally stashing and restoring local changes.
+ *
+ * @param worktreePath - Path to the git worktree
+ * @param branchName - Name of the new branch to create
+ * @param baseBranch - Optional base branch to create from (defaults to current HEAD)
+ * @param options - Stash handling options
+ * @param events - Optional event emitter for lifecycle events
+ * @returns CheckoutBranchResult with detailed status information
+ */
+export async function performCheckoutBranch(
+  worktreePath: string,
+  branchName: string,
+  baseBranch?: string,
+  options?: CheckoutBranchOptions,
+  events?: EventEmitter
+): Promise<CheckoutBranchResult> {
+  const shouldStash = options?.stashChanges ?? false;
+  const includeUntracked = options?.includeUntracked ?? true;
+
+  // Emit start event
+  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();
+
+  // 2. Check if branch already exists
+  if (await localBranchExists(worktreePath, branchName)) {
+    events?.emit('switch:error', {
+      worktreePath,
+      branchName,
+      error: `Branch '${branchName}' already exists`,
+    });
+    return {
+      success: false,
+      error: `Branch '${branchName}' already exists`,
+    };
+  }
+
+  // 3. Validate base branch if provided
+  if (baseBranch) {
+    try {
+      await execGitCommand(['rev-parse', '--verify', baseBranch], worktreePath);
+    } catch {
+      events?.emit('switch:error', {
+        worktreePath,
+        branchName,
+        error: `Base branch '${baseBranch}' does not exist`,
+      });
+      return {
+        success: false,
+        error: `Base branch '${baseBranch}' does not exist`,
+      };
+    }
+  }
+
+  // 4. Stash local changes if requested and there are changes
+  let didStash = false;
+
+  if (shouldStash) {
+    const hadChanges = await hasAnyChanges(worktreePath);
+    if (hadChanges) {
+      events?.emit('switch:stash', {
+        worktreePath,
+        previousBranch,
+        targetBranch: branchName,
+        action: 'push',
+      });
+
+      const stashMessage = `Auto-stash before switching to ${branchName}`;
+      try {
+        didStash = await stashChanges(worktreePath, stashMessage, includeUntracked);
+      } catch (stashError) {
+        const stashErrorMsg = getErrorMessage(stashError);
+        events?.emit('switch:error', {
+          worktreePath,
+          branchName,
+          error: `Failed to stash local changes: ${stashErrorMsg}`,
+        });
+        return {
+          success: false,
+          error: `Failed to stash local changes before creating branch: ${stashErrorMsg}`,
+        };
+      }
+    }
+  }
+
+  try {
+    // 5. Create and checkout the new branch
+    events?.emit('switch:checkout', {
+      worktreePath,
+      targetBranch: branchName,
+      isRemote: false,
+      previousBranch,
+    });
+
+    const checkoutArgs = ['checkout', '-b', branchName];
+    if (baseBranch) {
+      checkoutArgs.push(baseBranch);
+    }
+    await execGitCommand(checkoutArgs, worktreePath);
+
+    // 6. Reapply stashed changes if we stashed earlier
+    let hasConflicts = false;
+    let conflictMessage = '';
+    let stashReapplied = false;
+
+    if (didStash) {
+      events?.emit('switch:pop', {
+        worktreePath,
+        targetBranch: branchName,
+        action: 'pop',
+      });
+
+      const popResult = await popStash(worktreePath);
+      hasConflicts = popResult.hasConflicts;
+      if (popResult.hasConflicts) {
+        conflictMessage = `Created branch '${branchName}' but merge conflicts occurred when reapplying your local changes. Please resolve the conflicts.`;
+      } else if (!popResult.success) {
+        conflictMessage = `Created branch '${branchName}' but failed to reapply stashed changes: ${popResult.error}. Your changes are still in the stash.`;
+      } else {
+        stashReapplied = true;
+      }
+    }
+
+    if (hasConflicts) {
+      events?.emit('switch:done', {
+        worktreePath,
+        previousBranch,
+        currentBranch: branchName,
+        hasConflicts: true,
+      });
+      return {
+        success: true,
+        result: {
+          previousBranch,
+          newBranch: branchName,
+          message: conflictMessage,
+          hasConflicts: true,
+          stashedChanges: true,
+        },
+      };
+    } else if (didStash && !stashReapplied) {
+      // Stash pop failed for a non-conflict reason — stash is still present
+      events?.emit('switch:done', {
+        worktreePath,
+        previousBranch,
+        currentBranch: branchName,
+        stashPopFailed: true,
+      });
+      return {
+        success: true,
+        result: {
+          previousBranch,
+          newBranch: branchName,
+          message: conflictMessage,
+          hasConflicts: false,
+          stashedChanges: true,
+        },
+      };
+    } else {
+      const stashNote = stashReapplied ? ' (local changes stashed and reapplied)' : '';
+      events?.emit('switch:done', {
+        worktreePath,
+        previousBranch,
+        currentBranch: branchName,
+        stashReapplied,
+      });
+      return {
+        success: true,
+        result: {
+          previousBranch,
+          newBranch: branchName,
+          message: `Created and checked out branch '${branchName}'${stashNote}`,
+          hasConflicts: false,
+          stashedChanges: stashReapplied,
+        },
+      };
+    }
+  } catch (checkoutError) {
+    // 7. If checkout failed and we stashed, try to restore the stash
+    if (didStash) {
+      const popResult = await popStash(worktreePath);
+      if (popResult.hasConflicts) {
+        const checkoutErrorMsg = getErrorMessage(checkoutError);
+        events?.emit('switch:error', {
+          worktreePath,
+          branchName,
+          error: checkoutErrorMsg,
+          stashPopConflicts: true,
+        });
+        return {
+          success: false,
+          error: checkoutErrorMsg,
+          stashPopConflicts: true,
+          stashPopConflictMessage:
+            'Stash pop resulted in conflicts: your stashed changes were partially reapplied ' +
+            'but produced merge conflicts. Please resolve the conflicts before retrying.',
+        };
+      } else if (!popResult.success) {
+        const checkoutErrorMsg = getErrorMessage(checkoutError);
+        const combinedMessage =
+          `${checkoutErrorMsg}. Additionally, restoring your stashed changes failed: ` +
+          `${popResult.error ?? 'unknown error'} — your changes are still saved in the stash.`;
+        events?.emit('switch:error', {
+          worktreePath,
+          branchName,
+          error: combinedMessage,
+        });
+        return {
+          success: false,
+          error: combinedMessage,
+          stashPopConflicts: false,
+        };
+      }
+      // popResult.success === true: stash was cleanly restored
+    }
+    const checkoutErrorMsg = getErrorMessage(checkoutError);
+    events?.emit('switch:error', {
+      worktreePath,
+      branchName,
+      error: checkoutErrorMsg,
+    });
+    throw checkoutError;
+  }
+}

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

@@ -4,7 +4,7 @@
  * Extracted from worktree merge route to allow internal service calls.
  */
 
-import { createLogger } from '@automaker/utils';
+import { createLogger, isValidBranchName } from '@automaker/utils';
 import { type EventEmitter } from '../lib/events.js';
 import { execGitCommand } from '../lib/git.js';
 const logger = createLogger('MergeService');
@@ -28,19 +28,6 @@ export interface MergeServiceResult {
   };
 }
 
-/**
- * Validate branch name to prevent command injection.
- * The first character must not be '-' to prevent git argument injection
- * via names like "-flag" or "--option".
- */
-function isValidBranchName(name: string): boolean {
-  // First char must be alphanumeric, dot, underscore, or slash (not dash)
-  // Reject names containing '..' to prevent git ref traversal
-  return (
-    /^[a-zA-Z0-9._/][a-zA-Z0-9._\-/]*$/.test(name) && name.length < 250 && !name.includes('..')
-  );
-}
-
 /**
  * Perform a git merge operation directly without HTTP.
  *
@@ -111,30 +98,76 @@ export async function performMerge(
     : ['merge', branchName, '-m', mergeMessage];
 
   try {
-    await execGitCommand(mergeArgs, projectPath);
+    // Set LC_ALL=C so git always emits English output regardless of the system
+    // locale, making text-based conflict detection reliable.
+    await execGitCommand(mergeArgs, projectPath, { LC_ALL: 'C' });
   } catch (mergeError: unknown) {
-    // Check if this is a merge conflict
+    // Check if this is a merge conflict.  We use a multi-layer strategy so
+    // that detection is reliable even when locale settings vary or git's text
+    // output changes across versions:
+    //
+    //  1. Primary (text-based): scan the error output for well-known English
+    //     conflict markers.  Because we pass LC_ALL=C above these strings are
+    //     always in English, but we keep the check as one layer among several.
+    //
+    //  2. Unmerged-path check: run `git diff --name-only --diff-filter=U`
+    //     (locale-stable) and treat any non-empty output as a conflict
+    //     indicator, capturing the file list at the same time.
+    //
+    //  3. Fallback status check: run `git status --porcelain` and look for
+    //     lines whose first two characters indicate an unmerged state
+    //     (UU, AA, DD, AU, UA, DU, UD).
+    //
+    // hasConflicts is true when ANY of the three layers returns positive.
     const err = mergeError as { stdout?: string; stderr?: string; message?: string };
     const output = `${err.stdout || ''} ${err.stderr || ''} ${err.message || ''}`;
-    const hasConflicts = output.includes('CONFLICT') || output.includes('Automatic merge failed');
+
+    // Layer 1 – text matching (locale-safe because we set LC_ALL=C above).
+    const textIndicatesConflict =
+      output.includes('CONFLICT') || output.includes('Automatic merge failed');
+
+    // Layers 2 & 3 – repository state inspection (locale-independent).
+    // Layer 2: get conflicted files via diff (also locale-stable output).
+    let conflictFiles: string[] | undefined;
+    let diffIndicatesConflict = false;
+    try {
+      const diffOutput = await execGitCommand(
+        ['diff', '--name-only', '--diff-filter=U'],
+        projectPath,
+        { LC_ALL: 'C' }
+      );
+      const files = diffOutput
+        .trim()
+        .split('\n')
+        .filter((f) => f.trim().length > 0);
+      if (files.length > 0) {
+        diffIndicatesConflict = true;
+        conflictFiles = files;
+      }
+    } catch {
+      // If we can't get the file list, leave conflictFiles undefined so callers
+      // can distinguish "no conflicts" (empty array) from "unknown due to diff failure" (undefined)
+    }
+
+    // Layer 3: check for unmerged paths via machine-readable git status.
+    let hasUnmergedPaths = false;
+    try {
+      const statusOutput = await execGitCommand(['status', '--porcelain'], projectPath, {
+        LC_ALL: 'C',
+      });
+      // Unmerged status codes occupy the first two characters of each line.
+      // Standard unmerged codes: UU, AA, DD, AU, UA, DU, UD.
+      hasUnmergedPaths = statusOutput
+        .split('\n')
+        .some((line) => /^(UU|AA|DD|AU|UA|DU|UD)/.test(line));
+    } catch {
+      // git status failing is itself a sign something is wrong; leave
+      // hasUnmergedPaths as false and rely on the other layers.
+    }
+
+    const hasConflicts = textIndicatesConflict || diffIndicatesConflict || hasUnmergedPaths;
 
     if (hasConflicts) {
-      // Get list of conflicted files
-      let conflictFiles: string[] | undefined;
-      try {
-        const diffOutput = await execGitCommand(
-          ['diff', '--name-only', '--diff-filter=U'],
-          projectPath
-        );
-        conflictFiles = diffOutput
-          .trim()
-          .split('\n')
-          .filter((f) => f.trim().length > 0);
-      } catch {
-        // If we can't get the file list, leave conflictFiles undefined so callers
-        // can distinguish "no conflicts" (empty array) from "unknown due to diff failure" (undefined)
-      }
-
       // Emit merge:conflict event with conflict details
       emitter?.emit('merge:conflict', { branchName, targetBranch: mergeTo, conflictFiles });
 

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

@@ -17,7 +17,7 @@
 
 import { createLogger, getErrorMessage } from '@automaker/utils';
 import { getConflictFiles } from '@automaker/git-utils';
-import { execGitCommand, execGitCommandWithLockRetry } from '../lib/git.js';
+import { execGitCommand, execGitCommandWithLockRetry, getCurrentBranch } from '../lib/git.js';
 
 const logger = createLogger('PullService');
 
@@ -52,17 +52,6 @@ export interface PullResult {
 // Helper Functions
 // ============================================================================
 
-/**
- * Get the current branch name for the worktree.
- *
- * @param worktreePath - Path to the git worktree
- * @returns The current branch name (returns 'HEAD' for detached HEAD state)
- */
-export async function getCurrentBranch(worktreePath: string): Promise<string> {
-  const branchOutput = await execGitCommand(['rev-parse', '--abbrev-ref', 'HEAD'], worktreePath);
-  return branchOutput.trim();
-}
-
 /**
  * Fetch the latest refs from a remote.
  *
@@ -102,7 +91,7 @@ export async function getLocalChanges(
  *
  * @param worktreePath - Path to the git worktree
  * @param branchName - Current branch name (used in stash message)
- * @returns true if stash was created
+ * @returns Promise<void> — resolves on success, throws on failure
  */
 export async function stashChanges(worktreePath: string, branchName: string): Promise<void> {
   const stashMessage = `automaker-pull-stash: Pre-pull stash on ${branchName}`;

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

@@ -7,7 +7,7 @@
 
 import fs from 'fs/promises';
 import path from 'path';
-import { createLogger } from '@automaker/utils';
+import { createLogger, getErrorMessage } from '@automaker/utils';
 import { getConflictFiles } from '@automaker/git-utils';
 import { execGitCommand, getCurrentBranch } from '../lib/git.js';
 
@@ -50,7 +50,15 @@ export async function runRebase(worktreePath: string, ontoBranch: string): Promi
   }
 
   // Get current branch name before rebase
-  const currentBranch = await getCurrentBranch(worktreePath);
+  let currentBranch: string;
+  try {
+    currentBranch = await getCurrentBranch(worktreePath);
+  } catch (branchErr) {
+    return {
+      success: false,
+      error: `Failed to resolve current branch for worktree "${worktreePath}": ${getErrorMessage(branchErr)}`,
+    };
+  }
 
   try {
     // Pass ontoBranch after '--' so git treats it as a ref, not an option.

apps/server/src/services/stage-files-service.ts

@@ -68,6 +68,14 @@ export async function stageFiles(
   const base = canonicalRoot + path.sep;
   const sanitizedFiles: string[] = [];
   for (const file of files) {
+    // Reject empty or whitespace-only paths — path.resolve(canonicalRoot, '')
+    // returns canonicalRoot itself, so without this guard an empty string would
+    // pass all subsequent checks and be forwarded to git unchanged.
+    if (file.trim() === '') {
+      throw new StageFilesValidationError(
+        'Invalid file path (empty or whitespace-only paths not allowed)'
+      );
+    }
     // Reject absolute paths
     if (path.isAbsolute(file)) {
       throw new StageFilesValidationError(

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

@@ -190,7 +190,12 @@ async function isRemoteBranch(cwd: string, branchName: string): Promise<boolean>
       .map((b) => b.trim().replace(/^['"]|['"]$/g, ''))
       .filter((b) => b);
     return remoteBranches.includes(branchName);
-  } catch {
+  } catch (err) {
+    logger.error('isRemoteBranch: failed to list remote branches — returning false', {
+      branchName,
+      cwd,
+      error: getErrorMessage(err),
+    });
     return false;
   }
 }