From 53d07fefb81736c48210fbf0806c6a9a14af2109 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 18 Feb 2026 21:36:00 -0800 Subject: [PATCH] feat: Fix new branch issues and address code review comments --- apps/server/src/providers/codex-provider.ts | 36 +- .../src/routes/git/routes/stage-files.ts | 66 +-- apps/server/src/routes/worktree/common.ts | 27 +- apps/server/src/routes/worktree/index.ts | 9 +- .../worktree/routes/branch-commit-log.ts | 26 +- .../routes/worktree/routes/check-changes.ts | 100 ++++ .../routes/worktree/routes/checkout-branch.ts | 79 +++- apps/server/src/services/auto-mode/facade.ts | 2 +- .../src/services/checkout-branch-service.ts | 383 +++++++++++++++ apps/server/src/services/merge-service.ts | 99 ++-- apps/server/src/services/pull-service.ts | 15 +- apps/server/src/services/rebase-service.ts | 12 +- .../src/services/stage-files-service.ts | 8 + .../src/services/worktree-branch-service.ts | 7 +- apps/ui/src/components/ui/git-diff-panel.tsx | 8 +- apps/ui/src/components/usage-popover.tsx | 44 +- .../dialogs/create-branch-dialog.tsx | 442 +++++++++++------- .../views/board-view/dialogs/index.ts | 5 + .../dialogs/stash-confirm-dialog.tsx | 185 ++++++++ .../hooks/use-worktree-actions.ts | 80 ++++ .../worktree-panel/worktree-panel.tsx | 30 ++ .../hooks/mutations/use-worktree-mutations.ts | 122 ++++- apps/ui/src/lib/electron.ts | 25 +- apps/ui/src/lib/http-api-client.ts | 18 +- apps/ui/src/types/electron.d.ts | 23 +- libs/git-utils/package.json | 1 + libs/git-utils/src/exec.ts | 70 +++ libs/git-utils/src/index.ts | 3 + libs/utils/src/git-validation.ts | 43 ++ libs/utils/src/index.ts | 3 + 30 files changed, 1604 insertions(+), 367 deletions(-) create mode 100644 apps/server/src/routes/worktree/routes/check-changes.ts create mode 100644 apps/server/src/services/checkout-branch-service.ts create mode 100644 apps/ui/src/components/views/board-view/dialogs/stash-confirm-dialog.tsx create mode 100644 libs/git-utils/src/exec.ts create mode 100644 libs/utils/src/git-validation.ts diff --git a/apps/server/src/providers/codex-provider.ts b/apps/server/src/providers/codex-provider.ts index 28fd7410..f23dd6c6 100644 --- a/apps/server/src/providers/codex-provider.ts +++ b/apps/server/src/providers/codex-provider.ts @@ -57,6 +57,7 @@ const CODEX_MODEL_FLAG = '--model'; const CODEX_VERSION_FLAG = '--version'; const CODEX_CONFIG_FLAG = '--config'; const CODEX_ADD_DIR_FLAG = '--add-dir'; +const CODEX_OUTPUT_SCHEMA_FLAG = '--output-schema'; const CODEX_SKIP_GIT_REPO_CHECK_FLAG = '--skip-git-repo-check'; const CODEX_REASONING_EFFORT_KEY = 'reasoning_effort'; const CODEX_YOLO_FLAG = '--dangerously-bypass-approvals-and-sandbox'; @@ -200,6 +201,12 @@ function isSdkEligible(options: ExecuteOptions): boolean { return isNoToolsRequested(options) && !hasMcpServersConfigured(options); } +function isSdkEligibleWithApiKey(options: ExecuteOptions): boolean { + // When using an API key (not CLI OAuth), prefer SDK over CLI to avoid OAuth issues. + // SDK mode is used when MCP servers are not configured (MCP requires CLI). + return !hasMcpServersConfigured(options); +} + async function resolveCodexExecutionPlan(options: ExecuteOptions): Promise { const cliPath = await findCodexCliPath(); const authIndicators = await getCodexAuthIndicators(); @@ -225,8 +232,10 @@ async function resolveCodexExecutionPlan(options: ExecuteOptions): Promise m.id).join(', ')}. ` + - `Some models require a ChatGPT Pro/Plus subscription—authenticate with 'codex login' instead of an API key. ` + - `For the current list of compatible models, visit https://platform.openai.com/docs/models.`; + `See https://platform.openai.com/docs/models for available models. ` + + `Some models require a ChatGPT Pro/Plus subscription—authenticate with 'codex login' instead of an API key.`; } else if ( errorLower.includes('stream disconnected') || errorLower.includes('stream ended') || diff --git a/apps/server/src/routes/git/routes/stage-files.ts b/apps/server/src/routes/git/routes/stage-files.ts index 9aba0be3..98ca44c1 100644 --- a/apps/server/src/routes/git/routes/stage-files.ts +++ b/apps/server/src/routes/git/routes/stage-files.ts @@ -2,11 +2,9 @@ * POST /stage-files endpoint - Stage or unstage files in the main project */ -import fs from 'fs'; -import path from 'path'; import type { Request, Response } from 'express'; import { getErrorMessage, logError } from '../common.js'; -import { execGitCommand } from '../../../lib/git.js'; +import { stageFiles, StageFilesValidationError } from '../../../services/stage-files-service.js'; export function createStageFilesHandler() { return async (req: Request, res: Response): Promise => { @@ -51,67 +49,17 @@ export function createStageFilesHandler() { return; } - // Resolve the canonical (symlink-dereferenced) project path so that - // startsWith(base) reliably prevents symlink traversal attacks. - // If projectPath does not exist or is unreadable, realpath rejects and - // we return a 400 instead of letting the error propagate as a 500. - let canonicalRoot: string; - try { - canonicalRoot = await fs.promises.realpath(projectPath); - } catch { - res.status(400).json({ - success: false, - error: `Invalid projectPath (non-existent or unreadable): ${projectPath}`, - }); - return; - } - - // Validate and sanitize each file path to prevent path traversal attacks - const base = path.resolve(canonicalRoot) + path.sep; - const sanitizedFiles: string[] = []; - for (const file of files) { - // Reject absolute paths - if (path.isAbsolute(file)) { - res.status(400).json({ - success: false, - error: `Invalid file path (absolute paths not allowed): ${file}`, - }); - return; - } - // Reject entries containing '..' - if (file.includes('..')) { - res.status(400).json({ - success: false, - error: `Invalid file path (path traversal not allowed): ${file}`, - }); - return; - } - // Ensure the resolved path stays within the project directory - const resolved = path.resolve(path.join(canonicalRoot, file)); - if (resolved !== path.resolve(canonicalRoot) && !resolved.startsWith(base)) { - res.status(400).json({ - success: false, - error: `Invalid file path (outside project directory): ${file}`, - }); - return; - } - sanitizedFiles.push(file); - } - - if (operation === 'stage') { - await execGitCommand(['add', '--', ...sanitizedFiles], canonicalRoot); - } else { - await execGitCommand(['reset', 'HEAD', '--', ...sanitizedFiles], canonicalRoot); - } + const result = await stageFiles(projectPath, files, operation); res.json({ success: true, - result: { - operation, - filesCount: sanitizedFiles.length, - }, + result, }); } catch (error) { + if (error instanceof StageFilesValidationError) { + res.status(400).json({ success: false, error: error.message }); + return; + } logError(error, `${(req.body as { operation?: string })?.operation ?? 'stage'} files failed`); res.status(500).json({ success: false, error: getErrorMessage(error) }); } diff --git a/apps/server/src/routes/worktree/common.ts b/apps/server/src/routes/worktree/common.ts index d5caba86..2164fe61 100644 --- a/apps/server/src/routes/worktree/common.ts +++ b/apps/server/src/routes/worktree/common.ts @@ -2,7 +2,7 @@ * Common utilities for worktree routes */ -import { createLogger } from '@automaker/utils'; +import { createLogger, isValidBranchName, MAX_BRANCH_NAME_LENGTH } from '@automaker/utils'; import { exec } from 'child_process'; import { promisify } from 'util'; import { getErrorMessage as getErrorMessageShared, createLogError } from '../common.js'; @@ -14,12 +14,9 @@ export { execGitCommand } from '../../lib/git.js'; const logger = createLogger('Worktree'); export const execAsync = promisify(exec); -// ============================================================================ -// Constants -// ============================================================================ - -/** Maximum allowed length for git branch names */ -export const MAX_BRANCH_NAME_LENGTH = 250; +// Re-export git validation utilities from the canonical shared module so +// existing consumers that import from this file continue to work. +export { isValidBranchName, MAX_BRANCH_NAME_LENGTH }; // ============================================================================ // Extended PATH configuration for Electron apps @@ -63,22 +60,6 @@ export const execEnv = { PATH: extendedPath, }; -// ============================================================================ -// Validation utilities -// ============================================================================ - -/** - * Validate branch name to prevent command injection. - * Git branch names cannot contain: space, ~, ^, :, ?, *, [, \, or control chars. - * We also reject shell metacharacters for safety. - * The first character must not be '-' to prevent git argument injection. - */ -export function isValidBranchName(name: string): boolean { - // First char must be alphanumeric, dot, underscore, or slash (not dash) - // to prevent git option injection via names like "-flag" or "--option". - return /^[a-zA-Z0-9._/][a-zA-Z0-9._\-/]*$/.test(name) && name.length < MAX_BRANCH_NAME_LENGTH; -} - /** * Validate git remote name to prevent command injection. * Allowed characters: alphanumerics, hyphen, underscore, dot, and slash. diff --git a/apps/server/src/routes/worktree/index.ts b/apps/server/src/routes/worktree/index.ts index 983d6bb8..a788bb48 100644 --- a/apps/server/src/routes/worktree/index.ts +++ b/apps/server/src/routes/worktree/index.ts @@ -66,6 +66,7 @@ import { createRebaseHandler } from './routes/rebase.js'; import { createAbortOperationHandler } from './routes/abort-operation.js'; import { createContinueOperationHandler } from './routes/continue-operation.js'; import { createStageFilesHandler } from './routes/stage-files.js'; +import { createCheckChangesHandler } from './routes/check-changes.js'; import type { SettingsService } from '../../services/settings-service.js'; export function createWorktreeRoutes( @@ -121,7 +122,13 @@ export function createWorktreeRoutes( '/checkout-branch', validatePathParams('worktreePath'), requireValidWorktree, - createCheckoutBranchHandler() + createCheckoutBranchHandler(events) + ); + router.post( + '/check-changes', + validatePathParams('worktreePath'), + requireGitRepoOnly, + createCheckChangesHandler() ); router.post( '/list-branches', diff --git a/apps/server/src/routes/worktree/routes/branch-commit-log.ts b/apps/server/src/routes/worktree/routes/branch-commit-log.ts index fdf84e84..60562d97 100644 --- a/apps/server/src/routes/worktree/routes/branch-commit-log.ts +++ b/apps/server/src/routes/worktree/routes/branch-commit-log.ts @@ -16,31 +16,7 @@ import type { Request, Response } from 'express'; import type { EventEmitter } from '../../../lib/events.js'; import { getErrorMessage, logError } from '../common.js'; import { getBranchCommitLog } from '../../../services/branch-commit-log-service.js'; - -/** - * Validates a branchName value before it is forwarded to execGitCommand. - * - * Rejects values that: - * - Start with '-' (would be interpreted as a git flag/option) - * - Contain NUL bytes (\0) - * - Contain path-traversal sequences (..) - * - * Only allows characters from a safe whitelist: - * alphanumerics, dot (.), slash (/), underscore (_), dash (-), plus (+), - * at-sign (@), tilde (~), caret (^), and colon (:). - * - * Returns `true` when the value is safe to pass to execGitCommand. - */ -function isValidBranchName(branchName: string): boolean { - // Must not start with '-' (git option injection) - if (branchName.startsWith('-')) return false; - // Must not contain NUL bytes - if (branchName.includes('\0')) return false; - // Must not contain path-traversal sequences - if (branchName.includes('..')) return false; - // Whitelist: alphanumerics and common ref characters - return /^[a-zA-Z0-9._/\-+@~^:]+$/.test(branchName); -} +import { isValidBranchName } from '@automaker/utils'; export function createBranchCommitLogHandler(events: EventEmitter) { return async (req: Request, res: Response): Promise => { diff --git a/apps/server/src/routes/worktree/routes/check-changes.ts b/apps/server/src/routes/worktree/routes/check-changes.ts new file mode 100644 index 00000000..5de1f260 --- /dev/null +++ b/apps/server/src/routes/worktree/routes/check-changes.ts @@ -0,0 +1,100 @@ +/** + * POST /check-changes endpoint - Check for uncommitted changes in a worktree + * + * Returns a summary of staged, unstaged, and untracked files to help + * the user decide whether to stash before a branch operation. + * + * Note: Git repository validation (isGitRepo) is handled by + * the requireGitRepoOnly middleware in index.ts + */ + +import type { Request, Response } from 'express'; +import { getErrorMessage, logError } from '../common.js'; +import { execGitCommand } from '../../../lib/git.js'; + +/** + * Parse `git status --porcelain` output into categorised file lists. + * + * Porcelain format gives two status characters per line: + * XY filename + * where X is the index (staged) status and Y is the worktree (unstaged) status. + * + * - '?' in both columns → untracked + * - Non-space/non-'?' in X → staged change + * - Non-space/non-'?' in Y (when not untracked) → unstaged change + * + * A file can appear in both staged and unstaged if it was partially staged. + */ +function parseStatusOutput(stdout: string): { + staged: string[]; + unstaged: string[]; + untracked: string[]; +} { + const staged: string[] = []; + const unstaged: string[] = []; + const untracked: string[] = []; + + const lines = stdout.trim().split('\n').filter(Boolean); + + for (const line of lines) { + if (line.length < 3) continue; + + const x = line[0]; // index status + const y = line[1]; // worktree status + // Handle renames which use " -> " separator + const rawPath = line.slice(3); + const filePath = rawPath.includes(' -> ') ? rawPath.split(' -> ')[1] : rawPath; + + if (x === '?' && y === '?') { + untracked.push(filePath); + } else { + if (x !== ' ' && x !== '?') { + staged.push(filePath); + } + if (y !== ' ' && y !== '?') { + unstaged.push(filePath); + } + } + } + + return { staged, unstaged, untracked }; +} + +export function createCheckChangesHandler() { + return async (req: Request, res: Response): Promise => { + try { + const { worktreePath } = req.body as { + worktreePath: string; + }; + + if (!worktreePath) { + res.status(400).json({ + success: false, + error: 'worktreePath required', + }); + return; + } + + // Get porcelain status (includes staged, unstaged, and untracked files) + const stdout = await execGitCommand(['status', '--porcelain'], worktreePath); + + const { staged, unstaged, untracked } = parseStatusOutput(stdout); + + const hasChanges = staged.length > 0 || unstaged.length > 0 || untracked.length > 0; + + res.json({ + success: true, + result: { + hasChanges, + staged, + unstaged, + untracked, + totalFiles: staged.length + unstaged.length + untracked.length, + }, + }); + } catch (error) { + logError(error, 'Check changes failed'); + res.status(500).json({ success: false, error: getErrorMessage(error) }); + } + }; +} diff --git a/apps/server/src/routes/worktree/routes/checkout-branch.ts b/apps/server/src/routes/worktree/routes/checkout-branch.ts index 4e720833..d7830a64 100644 --- a/apps/server/src/routes/worktree/routes/checkout-branch.ts +++ b/apps/server/src/routes/worktree/routes/checkout-branch.ts @@ -1,6 +1,14 @@ /** * POST /checkout-branch endpoint - Create and checkout a new branch * + * Supports automatic stash handling: when `stashChanges` is true, local changes + * 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. + * + * Git business logic is delegated to checkout-branch-service.ts when stash + * handling is requested. Otherwise, falls back to the original simple flow. + * * Note: Git repository validation (isGitRepo, hasCommits) is handled by * the requireValidWorktree middleware in index.ts. * Path validation (ALLOWED_ROOT_DIRECTORY) is handled by validatePathParams @@ -12,14 +20,20 @@ import path from 'path'; import { stat } from 'fs/promises'; import { getErrorMessage, logError, isValidBranchName } from '../common.js'; import { execGitCommand } from '../../../lib/git.js'; +import type { EventEmitter } from '../../../lib/events.js'; +import { performCheckoutBranch } from '../../../services/checkout-branch-service.js'; -export function createCheckoutBranchHandler() { +export function createCheckoutBranchHandler(events?: EventEmitter) { return async (req: Request, res: Response): Promise => { try { - const { worktreePath, branchName, baseBranch } = req.body as { + const { worktreePath, branchName, baseBranch, stashChanges, includeUntracked } = req.body as { worktreePath: string; branchName: string; - baseBranch?: string; // Optional base branch to create from (defaults to current HEAD) + baseBranch?: string; + /** When true, stash local changes before checkout and reapply after */ + stashChanges?: boolean; + /** When true, include untracked files in the stash (defaults to true) */ + includeUntracked?: boolean; }; if (!worktreePath) { @@ -59,8 +73,6 @@ export function createCheckoutBranchHandler() { } // Resolve and validate worktreePath to prevent traversal attacks. - // The validatePathParams middleware checks against ALLOWED_ROOT_DIRECTORY, - // but we also resolve the path and verify it exists as a directory. const resolvedPath = path.resolve(worktreePath); try { const stats = await stat(resolvedPath); @@ -79,7 +91,42 @@ export function createCheckoutBranchHandler() { return; } - // Get current branch for reference (using argument array to avoid shell injection) + // Use the service for stash-aware checkout + if (stashChanges) { + const result = await performCheckoutBranch( + resolvedPath, + branchName, + baseBranch, + { + stashChanges: true, + includeUntracked: includeUntracked ?? true, + }, + events + ); + + if (!result.success) { + const statusCode = isBranchError(result.error) ? 400 : 500; + res.status(statusCode).json({ + success: false, + error: result.error, + ...(result.stashPopConflicts !== undefined && { + stashPopConflicts: result.stashPopConflicts, + }), + ...(result.stashPopConflictMessage && { + stashPopConflictMessage: result.stashPopConflictMessage, + }), + }); + return; + } + + res.json({ + success: true, + result: result.result, + }); + return; + } + + // Original simple flow (no stash handling) const currentBranchOutput = await execGitCommand( ['rev-parse', '--abbrev-ref', 'HEAD'], resolvedPath @@ -89,7 +136,6 @@ export function createCheckoutBranchHandler() { // Check if branch already exists try { await execGitCommand(['rev-parse', '--verify', branchName], resolvedPath); - // Branch exists res.status(400).json({ success: false, error: `Branch '${branchName}' already exists`, @@ -112,8 +158,7 @@ export function createCheckoutBranchHandler() { } } - // Create and checkout the new branch (using argument array to avoid shell injection) - // If baseBranch is provided, create the branch from that starting point + // Create and checkout the new branch const checkoutArgs = ['checkout', '-b', branchName]; if (baseBranch) { checkoutArgs.push(baseBranch); @@ -129,8 +174,24 @@ export function createCheckoutBranchHandler() { }, }); } catch (error) { + events?.emit('switch:error', { + error: getErrorMessage(error), + }); + logError(error, 'Checkout branch failed'); res.status(500).json({ success: false, error: getErrorMessage(error) }); } }; } + +/** + * Determine whether an error message represents a client error (400) + */ +function isBranchError(error?: string): boolean { + if (!error) return false; + return ( + error.includes('already exists') || + error.includes('does not exist') || + error.includes('Failed to stash') + ); +} diff --git a/apps/server/src/services/auto-mode/facade.ts b/apps/server/src/services/auto-mode/facade.ts index 38a1c878..abecb46d 100644 --- a/apps/server/src/services/auto-mode/facade.ts +++ b/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'; diff --git a/apps/server/src/services/checkout-branch-service.ts b/apps/server/src/services/checkout-branch-service.ts new file mode 100644 index 00000000..eba5511c --- /dev/null +++ b/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 { + 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 { + 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 { + 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 { + 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; + } +} diff --git a/apps/server/src/services/merge-service.ts b/apps/server/src/services/merge-service.ts index 52bb34d6..72089a5e 100644 --- a/apps/server/src/services/merge-service.ts +++ b/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 }); diff --git a/apps/server/src/services/pull-service.ts b/apps/server/src/services/pull-service.ts index 3db5af85..b57bf3eb 100644 --- a/apps/server/src/services/pull-service.ts +++ b/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 { - 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 — resolves on success, throws on failure */ export async function stashChanges(worktreePath: string, branchName: string): Promise { const stashMessage = `automaker-pull-stash: Pre-pull stash on ${branchName}`; diff --git a/apps/server/src/services/rebase-service.ts b/apps/server/src/services/rebase-service.ts index 7a1a0316..6d21fcd1 100644 --- a/apps/server/src/services/rebase-service.ts +++ b/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. diff --git a/apps/server/src/services/stage-files-service.ts b/apps/server/src/services/stage-files-service.ts index 0e3299be..e155b3ee 100644 --- a/apps/server/src/services/stage-files-service.ts +++ b/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( diff --git a/apps/server/src/services/worktree-branch-service.ts b/apps/server/src/services/worktree-branch-service.ts index 5d3fc9bc..de2514b4 100644 --- a/apps/server/src/services/worktree-branch-service.ts +++ b/apps/server/src/services/worktree-branch-service.ts @@ -190,7 +190,12 @@ async function isRemoteBranch(cwd: string, branchName: string): Promise .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; } } diff --git a/apps/ui/src/components/ui/git-diff-panel.tsx b/apps/ui/src/components/ui/git-diff-panel.tsx index a3afe715..8529a5a8 100644 --- a/apps/ui/src/components/ui/git-diff-panel.tsx +++ b/apps/ui/src/components/ui/git-diff-panel.tsx @@ -534,8 +534,14 @@ export function GitDiffPanel({ onStart: () => void, onFinally: () => void ) => { - if (!worktreePath && !projectPath) return; onStart(); + if (!worktreePath && !projectPath) { + toast.error(failurePrefix, { + description: 'No project or worktree path configured', + }); + onFinally(); + return; + } try { const api = getElectronAPI(); let result: { success: boolean; error?: string } | undefined; diff --git a/apps/ui/src/components/usage-popover.tsx b/apps/ui/src/components/usage-popover.tsx index aad1b670..014a0f2c 100644 --- a/apps/ui/src/components/usage-popover.tsx +++ b/apps/ui/src/components/usage-popover.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useMemo } from 'react'; +import { useState, useEffect, useRef, useMemo } from 'react'; import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover'; import { Button } from '@/components/ui/button'; import { Tabs, TabsContent, TabsList, TabsTrigger } from '@/components/ui/tabs'; @@ -55,11 +55,6 @@ function formatResetTime(unixTimestamp: number, isMilliseconds = false): string return `Resets ${date.toLocaleDateString()} at ${date.toLocaleTimeString([], { hour: '2-digit', minute: '2-digit' })}`; } -// Legacy alias for Codex -function formatCodexResetTime(unixTimestamp: number): string { - return formatResetTime(unixTimestamp, false); -} - // Helper to format window duration for Codex function getCodexWindowLabel(durationMins: number): { title: string; subtitle: string } { if (durationMins < 60) { @@ -95,6 +90,8 @@ export function UsagePopover() { const [open, setOpen] = useState(false); const [activeTab, setActiveTab] = useState<'claude' | 'codex' | 'zai' | 'gemini'>('claude'); + // Track whether the user has manually selected a tab so we don't override their choice + const userHasSelected = useRef(false); // Check authentication status const isClaudeAuthenticated = !!claudeAuthStatus?.authenticated; @@ -190,8 +187,24 @@ export function UsagePopover() { return { code: ERROR_CODES.AUTH_ERROR, message }; }, [geminiQueryError]); - // Determine which tab to show by default + // Determine which tab to show by default. + // Only apply the default when the popover opens (open transitions to true) and the user has + // not yet made a manual selection during this session. This prevents auth-flag re-renders from + // overriding a tab the user explicitly clicked. useEffect(() => { + if (!open) { + // Reset the user-selection guard each time the popover closes so the next open always gets + // a fresh default. + userHasSelected.current = false; + return; + } + + // The user already picked a tab – respect their choice. + if (userHasSelected.current) { + return; + } + + // Pick the first available provider in priority order. if (isClaudeAuthenticated) { setActiveTab('claude'); } else if (isCodexAuthenticated) { @@ -201,7 +214,13 @@ export function UsagePopover() { } else if (isGeminiAuthenticated) { setActiveTab('gemini'); } - }, [isClaudeAuthenticated, isCodexAuthenticated, isZaiAuthenticated, isGeminiAuthenticated]); + }, [ + open, + isClaudeAuthenticated, + isCodexAuthenticated, + isZaiAuthenticated, + isGeminiAuthenticated, + ]); // Check if data is stale (older than 2 minutes) const isClaudeStale = useMemo(() => { @@ -463,7 +482,10 @@ export function UsagePopover() { > setActiveTab(v as 'claude' | 'codex' | 'zai' | 'gemini')} + onValueChange={(v) => { + userHasSelected.current = true; + setActiveTab(v as 'claude' | 'codex' | 'zai' | 'gemini'); + }} > {/* Tabs Header */} {tabCount > 1 && ( @@ -684,7 +706,7 @@ export function UsagePopover() { .subtitle } percentage={codexUsage.rateLimits.primary.usedPercent} - resetText={formatCodexResetTime(codexUsage.rateLimits.primary.resetsAt)} + resetText={formatResetTime(codexUsage.rateLimits.primary.resetsAt)} isPrimary={true} stale={isCodexStale} pacePercentage={getExpectedCodexPacePercentage( @@ -705,7 +727,7 @@ export function UsagePopover() { .subtitle } percentage={codexUsage.rateLimits.secondary.usedPercent} - resetText={formatCodexResetTime(codexUsage.rateLimits.secondary.resetsAt)} + resetText={formatResetTime(codexUsage.rateLimits.secondary.resetsAt)} stale={isCodexStale} pacePercentage={getExpectedCodexPacePercentage( codexUsage.rateLimits.secondary.resetsAt, diff --git a/apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx b/apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx index 5b6a5c4e..dca3cd4f 100644 --- a/apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx +++ b/apps/ui/src/components/views/board-view/dialogs/create-branch-dialog.tsx @@ -27,6 +27,11 @@ import { toast } from 'sonner'; import { Check, ChevronsUpDown, GitBranchPlus, Globe, RefreshCw } from 'lucide-react'; import { Spinner } from '@/components/ui/spinner'; import { cn } from '@/lib/utils'; +import { + StashConfirmDialog, + type UncommittedChangesInfo, + type StashConfirmAction, +} from './stash-confirm-dialog'; interface WorktreeInfo { path: string; @@ -67,6 +72,17 @@ export function CreateBranchDialog({ const baseBranchTriggerRef = useRef(null); const [baseBranchTriggerWidth, setBaseBranchTriggerWidth] = useState(0); + // Stash confirmation state + const [showStashConfirm, setShowStashConfirm] = useState(false); + const [uncommittedChanges, setUncommittedChanges] = useState(null); + + // Keep a ref in sync with baseBranch so fetchBranches can read the latest value + // without needing it in its dependency array (which would cause re-fetch loops) + const baseBranchRef = useRef(baseBranch); + useEffect(() => { + baseBranchRef.current = baseBranch; + }, [baseBranch]); + const fetchBranches = useCallback(async () => { if (!worktree) return; @@ -81,7 +97,8 @@ export function CreateBranchDialog({ // Only set the default base branch if no branch is currently selected, // or if the currently selected branch is no longer present in the fetched list const branchNames = result.result.branches.map((b: BranchInfo) => b.name); - if (!baseBranch || !branchNames.includes(baseBranch)) { + const currentBaseBranch = baseBranchRef.current; + if (!currentBaseBranch || !branchNames.includes(currentBaseBranch)) { if (result.result.currentBranch) { setBaseBranch(result.result.currentBranch); } @@ -92,7 +109,7 @@ export function CreateBranchDialog({ } finally { setIsLoadingBranches(false); } - }, [worktree, baseBranch]); + }, [worktree]); // Reset state and fetch branches when dialog opens useEffect(() => { @@ -102,6 +119,8 @@ export function CreateBranchDialog({ setError(null); setBranches([]); setBaseBranchPopoverOpen(false); + setShowStashConfirm(false); + setUncommittedChanges(null); fetchBranches(); } }, [open, fetchBranches]); @@ -118,6 +137,65 @@ export function CreateBranchDialog({ return () => observer.disconnect(); }, [baseBranchPopoverOpen]); + /** + * Execute the actual branch creation, optionally with stash handling + */ + const doCreate = useCallback( + async (stashChanges: boolean) => { + if (!worktree || !branchName.trim()) return; + + setIsCreating(true); + setError(null); + + try { + const api = getElectronAPI(); + if (!api?.worktree?.checkoutBranch) { + toast.error('Branch API not available'); + return; + } + + const selectedBase = baseBranch || undefined; + const result = await api.worktree.checkoutBranch( + worktree.path, + branchName.trim(), + selectedBase, + stashChanges, + true // includeUntracked + ); + + if (result.success && result.result) { + // Check if there were conflicts from stash reapply + if (result.result.hasConflicts) { + toast.warning('Branch created with conflicts', { + description: result.result.message, + duration: 8000, + }); + } else { + const desc = result.result.stashedChanges + ? 'Local changes were stashed and reapplied' + : undefined; + toast.success(result.result.message, { description: desc }); + } + onCreated(); + onOpenChange(false); + } else { + setError(result.error || 'Failed to create branch'); + } + } catch (err) { + logger.error('Create branch failed:', err); + setError('Failed to create branch'); + } finally { + setIsCreating(false); + setShowStashConfirm(false); + } + }, + [worktree, branchName, baseBranch, onCreated, onOpenChange] + ); + + /** + * Handle the initial "Create Branch" click. + * Checks for uncommitted changes first and shows confirmation if needed. + */ const handleCreate = async () => { if (!worktree || !branchName.trim()) return; @@ -128,39 +206,53 @@ export function CreateBranchDialog({ return; } - setIsCreating(true); setError(null); + // Check for uncommitted changes before proceeding try { - const api = getElectronAPI(); - if (!api?.worktree?.checkoutBranch) { - toast.error('Branch API not available'); + const api = getHttpApiClient(); + const changesResult = await api.worktree.checkChanges(worktree.path); + + if (changesResult.success && changesResult.result?.hasChanges) { + // Show the stash confirmation dialog + setUncommittedChanges({ + staged: changesResult.result.staged, + unstaged: changesResult.result.unstaged, + untracked: changesResult.result.untracked, + totalFiles: changesResult.result.totalFiles, + }); + setShowStashConfirm(true); return; } - - // Pass baseBranch if user selected one different from the current branch - const selectedBase = baseBranch || undefined; - const result = await api.worktree.checkoutBranch( - worktree.path, - branchName.trim(), - selectedBase - ); - - if (result.success && result.result) { - toast.success(result.result.message); - onCreated(); - onOpenChange(false); - } else { - setError(result.error || 'Failed to create branch'); - } } catch (err) { - logger.error('Create branch failed:', err); - setError('Failed to create branch'); - } finally { - setIsCreating(false); + // If we can't check for changes, proceed without stashing + logger.warn('Failed to check for uncommitted changes, proceeding without stash:', err); } + + // No changes detected, proceed directly + doCreate(false); }; + /** + * Handle the user's decision in the stash confirmation dialog + */ + const handleStashConfirmAction = useCallback( + (action: StashConfirmAction) => { + switch (action) { + case 'stash-and-proceed': + doCreate(true); + break; + case 'proceed-without-stash': + doCreate(false); + break; + case 'cancel': + setShowStashConfirm(false); + break; + } + }, + [doCreate] + ); + // Separate local and remote branches const localBranches = useMemo(() => branches.filter((b) => !b.isRemote), [branches]); const remoteBranches = useMemo(() => branches.filter((b) => b.isRemote), [branches]); @@ -174,124 +266,94 @@ export function CreateBranchDialog({ }, [baseBranch, branches]); return ( - - - - - - Create New Branch - - Create a new branch from a base branch - + <> + + + + + + Create New Branch + + Create a new branch from a base branch + -
-
- - { - setBranchName(e.target.value); - setError(null); - }} - onKeyDown={(e) => { - if (e.key === 'Enter' && branchName.trim() && !isCreating) { - handleCreate(); - } - }} - disabled={isCreating} - autoFocus - /> -
- -
-
- - +
+
+ + { + setBranchName(e.target.value); + setError(null); + }} + onKeyDown={(e) => { + if (e.key === 'Enter' && branchName.trim() && !isCreating) { + handleCreate(); + } + }} + disabled={isCreating} + autoFocus + />
- {isLoadingBranches && branches.length === 0 ? ( -
- - Loading branches... -
- ) : ( - - - - - e.stopPropagation()} - onTouchMove={(e) => e.stopPropagation()} + +
+
+ + +
+ {isLoadingBranches && branches.length === 0 ? ( +
+ + Loading branches... +
+ ) : ( + + + + + e.stopPropagation()} + onTouchMove={(e) => e.stopPropagation()} + > + + + + No matching branches + {localBranches.length > 0 && ( + + {localBranches.map((branch) => ( - - {branch.name} + + {branch.name} + + {branch.isCurrent && ( + + (current) + + )} ))} - - )} - - - - - )} + )} + {remoteBranches.length > 0 && ( + <> + {localBranches.length > 0 && } + + {remoteBranches.map((branch) => ( + { + setBaseBranch(value); + setBaseBranchPopoverOpen(false); + }} + > + + + {branch.name} + + ))} + + + )} + + + + + )} +
+ + {error &&

{error}

}
- {error &&

{error}

} -
+ + + + + +
- - - - - - + {/* Stash confirmation dialog - shown when uncommitted changes are detected */} + + ); } diff --git a/apps/ui/src/components/views/board-view/dialogs/index.ts b/apps/ui/src/components/views/board-view/dialogs/index.ts index f4c5c252..fd1ca22a 100644 --- a/apps/ui/src/components/views/board-view/dialogs/index.ts +++ b/apps/ui/src/components/views/board-view/dialogs/index.ts @@ -29,3 +29,8 @@ export { type BranchConflictData, type BranchConflictType, } from './branch-conflict-dialog'; +export { + StashConfirmDialog, + type UncommittedChangesInfo, + type StashConfirmAction, +} from './stash-confirm-dialog'; diff --git a/apps/ui/src/components/views/board-view/dialogs/stash-confirm-dialog.tsx b/apps/ui/src/components/views/board-view/dialogs/stash-confirm-dialog.tsx new file mode 100644 index 00000000..f88ed572 --- /dev/null +++ b/apps/ui/src/components/views/board-view/dialogs/stash-confirm-dialog.tsx @@ -0,0 +1,185 @@ +/** + * Dialog shown when uncommitted changes are detected before a branch operation. + * Presents the user with options to: + * 1. Stash and proceed - stash changes, perform the operation, then restore + * 2. Proceed without stashing - discard local changes and proceed + * 3. Cancel - abort the operation + * + * Displays a summary of affected files (staged, unstaged, untracked) so the + * user can make an informed decision. + */ + +import { useCallback } from 'react'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from '@/components/ui/dialog'; +import { Button } from '@/components/ui/button'; +import { AlertTriangle, Archive, XCircle, FileEdit, FilePlus, FileQuestion } from 'lucide-react'; + +export interface UncommittedChangesInfo { + staged: string[]; + unstaged: string[]; + untracked: string[]; + totalFiles: number; +} + +export type StashConfirmAction = 'stash-and-proceed' | 'proceed-without-stash' | 'cancel'; + +interface StashConfirmDialogProps { + open: boolean; + onOpenChange: (open: boolean) => void; + /** The branch operation being attempted (e.g., "switch to feature/xyz" or "create feature/xyz") */ + operationDescription: string; + /** Summary of uncommitted changes */ + changesInfo: UncommittedChangesInfo | null; + /** Called with the user's decision */ + onConfirm: (action: StashConfirmAction) => void; + /** Whether the operation is currently in progress */ + isLoading?: boolean; +} + +export function StashConfirmDialog({ + open, + onOpenChange, + operationDescription, + changesInfo, + onConfirm, + isLoading = false, +}: StashConfirmDialogProps) { + const handleStashAndProceed = useCallback(() => { + onConfirm('stash-and-proceed'); + }, [onConfirm]); + + const handleProceedWithoutStash = useCallback(() => { + onConfirm('proceed-without-stash'); + }, [onConfirm]); + + const handleCancel = useCallback(() => { + onConfirm('cancel'); + onOpenChange(false); + }, [onConfirm, onOpenChange]); + + if (!changesInfo) return null; + + const { staged, unstaged, untracked } = changesInfo; + + return ( + !isLoading && onOpenChange(isOpen)}> + + + + + Uncommitted Changes Detected + + +
+ + You have uncommitted changes that may be affected when you{' '} + {operationDescription}. + + + {/* File summary */} +
+ {staged.length > 0 && ( + } + label="Staged" + files={staged} + /> + )} + {unstaged.length > 0 && ( + } + label="Unstaged" + files={unstaged} + /> + )} + {untracked.length > 0 && ( + } + label="Untracked" + files={untracked} + /> + )} +
+ +
+

+ Choose how to proceed: +

+
    +
  • + Stash & Proceed — Saves your changes, performs the + operation, then restores them +
    • +
  • + Proceed Without Stashing — Carries your uncommitted + changes into the new branch as-is +
    • +
+
+
+ + + + + + + + + + + ); +} + +/** Renders a collapsible section of files with a category label */ +function FileSection({ + icon, + label, + files, +}: { + icon: React.ReactNode; + label: string; + files: string[]; +}) { + const maxDisplay = 5; + const displayFiles = files.slice(0, maxDisplay); + const remaining = files.length - maxDisplay; + + return ( +
+ + {icon} + {label} ({files.length}) + +
+ {displayFiles.map((file) => ( +
+ {file} +
+ ))} + {remaining > 0 && ( +
+ ...and {remaining} more {remaining === 1 ? 'file' : 'files'} +
+ )} +
+
+ ); +} diff --git a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts index 9d83281a..fe8e8ff9 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts +++ b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts @@ -2,6 +2,7 @@ import { useState, useCallback } from 'react'; import { useNavigate } from '@tanstack/react-router'; import { createLogger } from '@automaker/utils/logger'; import { getElectronAPI } from '@/lib/electron'; +import { getHttpApiClient } from '@/lib/http-api-client'; import { toast } from 'sonner'; import { useSwitchBranch, @@ -10,9 +11,17 @@ import { useOpenInEditor, } from '@/hooks/mutations'; import type { WorktreeInfo } from '../types'; +import type { UncommittedChangesInfo } from '../../dialogs/stash-confirm-dialog'; const logger = createLogger('WorktreeActions'); +/** Pending branch switch details, stored while awaiting user confirmation */ +export interface PendingSwitchInfo { + worktree: WorktreeInfo; + branchName: string; + changesInfo: UncommittedChangesInfo; +} + interface UseWorktreeActionsOptions { /** Callback when merge conflicts occur after branch switch stash reapply */ onBranchSwitchConflict?: (info: { @@ -32,6 +41,9 @@ export function useWorktreeActions(options?: UseWorktreeActionsOptions) { const navigate = useNavigate(); const [isActivating, setIsActivating] = useState(false); + // Pending branch switch state (waiting for user stash decision) + const [pendingSwitch, setPendingSwitch] = useState(null); + // Use React Query mutations const switchBranchMutation = useSwitchBranch({ onConflict: options?.onBranchSwitchConflict, @@ -41,9 +53,40 @@ export function useWorktreeActions(options?: UseWorktreeActionsOptions) { const pushMutation = usePushWorktree(); const openInEditorMutation = useOpenInEditor(); + /** + * Initiate a branch switch. + * First checks for uncommitted changes and, if found, stores the pending + * switch so the caller can show a confirmation dialog. + */ const handleSwitchBranch = useCallback( async (worktree: WorktreeInfo, branchName: string) => { if (switchBranchMutation.isPending || branchName === worktree.branch) return; + + // Check for uncommitted changes before switching + try { + const api = getHttpApiClient(); + const changesResult = await api.worktree.checkChanges(worktree.path); + + if (changesResult.success && changesResult.result?.hasChanges) { + // Store the pending switch and let the UI show the confirmation dialog + setPendingSwitch({ + worktree, + branchName, + changesInfo: { + staged: changesResult.result.staged, + unstaged: changesResult.result.unstaged, + untracked: changesResult.result.untracked, + totalFiles: changesResult.result.totalFiles, + }, + }); + return; + } + } catch (err) { + // If we can't check for changes, proceed with the switch (server will auto-stash) + logger.warn('Failed to check for uncommitted changes, proceeding with switch:', err); + } + + // No changes detected, proceed directly (server still handles stash as safety net) switchBranchMutation.mutate({ worktreePath: worktree.path, branchName, @@ -52,6 +95,39 @@ export function useWorktreeActions(options?: UseWorktreeActionsOptions) { [switchBranchMutation] ); + /** + * Confirm the pending branch switch after the user chooses an action. + * The server-side performSwitchBranch always auto-stashes when there are changes, + * so when the user chooses "proceed without stashing" we still switch (the server + * detects and stashes automatically). When "cancel", we just clear the pending state. + */ + const confirmPendingSwitch = useCallback( + (action: 'stash-and-proceed' | 'proceed-without-stash' | 'cancel') => { + if (!pendingSwitch) return; + + if (action === 'cancel') { + setPendingSwitch(null); + return; + } + + // Both 'stash-and-proceed' and 'proceed-without-stash' trigger the switch. + // The server-side performSwitchBranch handles the stash/pop cycle automatically. + // 'proceed-without-stash' means the user is OK with the server's auto-stash behavior. + switchBranchMutation.mutate({ + worktreePath: pendingSwitch.worktree.path, + branchName: pendingSwitch.branchName, + }); + + setPendingSwitch(null); + }, + [pendingSwitch, switchBranchMutation] + ); + + /** Clear the pending switch without performing any action */ + const cancelPendingSwitch = useCallback(() => { + setPendingSwitch(null); + }, []); + const handlePull = useCallback( async (worktree: WorktreeInfo, remote?: string) => { if (pullMutation.isPending) return; @@ -130,5 +206,9 @@ export function useWorktreeActions(options?: UseWorktreeActionsOptions) { handleOpenInIntegratedTerminal, handleOpenInEditor, handleOpenInExternalTerminal, + // Stash confirmation state for branch switching + pendingSwitch, + confirmPendingSwitch, + cancelPendingSwitch, }; } diff --git a/apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx b/apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx index 833e5a19..50f62e68 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx +++ b/apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx @@ -43,6 +43,7 @@ import { CherryPickDialog, GitPullDialog, } from '../dialogs'; +import { StashConfirmDialog } from '../dialogs/stash-confirm-dialog'; import type { SelectRemoteOperation } from '../dialogs'; import { TestLogsPanel } from '@/components/ui/test-logs-panel'; import { getElectronAPI } from '@/lib/electron'; @@ -114,6 +115,9 @@ export function WorktreePanel({ handleOpenInIntegratedTerminal, handleOpenInEditor, handleOpenInExternalTerminal, + pendingSwitch, + confirmPendingSwitch, + cancelPendingSwitch, } = useWorktreeActions({ onBranchSwitchConflict: onBranchSwitchConflict, onStashPopConflict: onStashPopConflict, @@ -880,6 +884,20 @@ export function WorktreePanel({ onStashed={handleStashCompleted} /> + {/* Stash Confirm Dialog for Branch Switching */} + { + if (!isOpen) cancelPendingSwitch(); + }} + operationDescription={ + pendingSwitch ? `switch to branch '${pendingSwitch.branchName}'` : '' + } + changesInfo={pendingSwitch?.changesInfo ?? null} + onConfirm={confirmPendingSwitch} + isLoading={isSwitching} + /> + {/* View Stashes Dialog */} + {/* Stash Confirm Dialog for Branch Switching */} + { + if (!isOpen) cancelPendingSwitch(); + }} + operationDescription={pendingSwitch ? `switch to branch '${pendingSwitch.branchName}'` : ''} + changesInfo={pendingSwitch?.changesInfo ?? null} + onConfirm={confirmPendingSwitch} + isLoading={isSwitching} + /> + {/* View Stashes Dialog */} void; + onStashPopConflict?: (info: { + worktreePath: string; + branchName: string; + stashPopConflictMessage: string; + }) => void; +}) { const queryClient = useQueryClient(); return useMutation({ mutationFn: async ({ worktreePath, branchName, + baseBranch, + stashChanges, + includeUntracked, }: { worktreePath: string; branchName: string; + baseBranch?: string; + stashChanges?: boolean; + includeUntracked?: boolean; }) => { const api = getElectronAPI(); if (!api.worktree) throw new Error('Worktree API not available'); - const result = await api.worktree.checkoutBranch(worktreePath, branchName); + const result = await api.worktree.checkoutBranch( + worktreePath, + branchName, + baseBranch, + stashChanges, + includeUntracked + ); if (!result.success) { + // When the checkout failed and restoring the stash produced conflicts + if (result.stashPopConflicts) { + const conflictError = new Error(result.error || 'Failed to checkout branch'); + ( + conflictError as Error & { + stashPopConflicts: boolean; + stashPopConflictMessage: string; + worktreePath: string; + branchName: string; + } + ).stashPopConflicts = true; + ( + conflictError as Error & { + stashPopConflicts: boolean; + stashPopConflictMessage: string; + worktreePath: string; + branchName: string; + } + ).stashPopConflictMessage = + result.stashPopConflictMessage ?? + 'Stash pop resulted in conflicts: please resolve conflicts before retrying.'; + ( + conflictError as Error & { + stashPopConflicts: boolean; + stashPopConflictMessage: string; + worktreePath: string; + branchName: string; + } + ).worktreePath = worktreePath; + ( + conflictError as Error & { + stashPopConflicts: boolean; + stashPopConflictMessage: string; + worktreePath: string; + branchName: string; + } + ).branchName = branchName; + throw conflictError; + } throw new Error(result.error || 'Failed to checkout branch'); } return result.result; }, - onSuccess: () => { + onSuccess: (data, variables) => { queryClient.invalidateQueries({ queryKey: ['worktrees'] }); - toast.success('New branch created and checked out'); + + if (data?.hasConflicts) { + toast.warning('Branch created with conflicts', { + description: data.message, + duration: 8000, + }); + options?.onConflict?.({ + worktreePath: variables.worktreePath, + branchName: data.newBranch ?? variables.branchName, + previousBranch: data.previousBranch ?? '', + }); + } else { + const desc = data?.stashedChanges ? 'Local changes were stashed and reapplied' : undefined; + toast.success('New branch created and checked out', { description: desc }); + } }, onError: (error: Error) => { - toast.error('Failed to checkout branch', { - description: error.message, - }); + const enrichedError = error as Error & { + stashPopConflicts?: boolean; + stashPopConflictMessage?: string; + worktreePath?: string; + branchName?: string; + }; + + if ( + enrichedError.stashPopConflicts && + enrichedError.worktreePath && + enrichedError.branchName + ) { + toast.error('Branch creation failed with stash conflicts', { + description: + enrichedError.stashPopConflictMessage ?? + 'Stash pop resulted in conflicts. Please resolve the conflicts in your working tree.', + duration: 10000, + }); + options?.onStashPopConflict?.({ + worktreePath: enrichedError.worktreePath, + branchName: enrichedError.branchName, + stashPopConflictMessage: + enrichedError.stashPopConflictMessage ?? + 'Stash pop resulted in conflicts. Please resolve the conflicts in your working tree.', + }); + } else { + toast.error('Failed to checkout branch', { + description: error.message, + }); + } }, }); } diff --git a/apps/ui/src/lib/electron.ts b/apps/ui/src/lib/electron.ts index c70c1062..9938e05a 100644 --- a/apps/ui/src/lib/electron.ts +++ b/apps/ui/src/lib/electron.ts @@ -2291,11 +2291,18 @@ function createMockWorktreeAPI(): WorktreeAPI { }; }, - checkoutBranch: async (worktreePath: string, branchName: string, baseBranch?: string) => { + checkoutBranch: async ( + worktreePath: string, + branchName: string, + baseBranch?: string, + stashChanges?: boolean, + _includeUntracked?: boolean + ) => { console.log('[Mock] Creating and checking out branch:', { worktreePath, branchName, baseBranch, + stashChanges, }); return { success: true, @@ -2303,6 +2310,22 @@ function createMockWorktreeAPI(): WorktreeAPI { previousBranch: 'main', newBranch: branchName, message: `Created and checked out branch '${branchName}'`, + hasConflicts: false, + stashedChanges: stashChanges ?? false, + }, + }; + }, + + checkChanges: async (worktreePath: string) => { + console.log('[Mock] Checking for uncommitted changes:', worktreePath); + return { + success: true, + result: { + hasChanges: false, + staged: [], + unstaged: [], + untracked: [], + totalFiles: 0, }, }; }, diff --git a/apps/ui/src/lib/http-api-client.ts b/apps/ui/src/lib/http-api-client.ts index 1ce9bed3..bba799d3 100644 --- a/apps/ui/src/lib/http-api-client.ts +++ b/apps/ui/src/lib/http-api-client.ts @@ -2139,8 +2139,22 @@ export class HttpApiClient implements ElectronAPI { this.post('/api/worktree/stage-files', { worktreePath, files, operation }), pull: (worktreePath: string, remote?: string, stashIfNeeded?: boolean) => this.post('/api/worktree/pull', { worktreePath, remote, stashIfNeeded }), - checkoutBranch: (worktreePath: string, branchName: string, baseBranch?: string) => - this.post('/api/worktree/checkout-branch', { worktreePath, branchName, baseBranch }), + checkoutBranch: ( + worktreePath: string, + branchName: string, + baseBranch?: string, + stashChanges?: boolean, + includeUntracked?: boolean + ) => + this.post('/api/worktree/checkout-branch', { + worktreePath, + branchName, + baseBranch, + stashChanges, + includeUntracked, + }), + checkChanges: (worktreePath: string) => + this.post('/api/worktree/check-changes', { worktreePath }), listBranches: (worktreePath: string, includeRemote?: boolean) => this.post('/api/worktree/list-branches', { worktreePath, includeRemote }), switchBranch: (worktreePath: string, branchName: string) => diff --git a/apps/ui/src/types/electron.d.ts b/apps/ui/src/types/electron.d.ts index c2d6c946..2483d0f8 100644 --- a/apps/ui/src/types/electron.d.ts +++ b/apps/ui/src/types/electron.d.ts @@ -1026,20 +1026,39 @@ export interface WorktreeAPI { code?: 'NOT_GIT_REPO' | 'NO_COMMITS'; }>; - // Create and checkout a new branch + // Check for uncommitted changes in a worktree + checkChanges: (worktreePath: string) => Promise<{ + success: boolean; + result?: { + hasChanges: boolean; + staged: string[]; + unstaged: string[]; + untracked: string[]; + totalFiles: number; + }; + error?: string; + }>; + + // Create and checkout a new branch (with optional stash handling) checkoutBranch: ( worktreePath: string, branchName: string, - baseBranch?: string + baseBranch?: string, + stashChanges?: boolean, + includeUntracked?: boolean ) => Promise<{ success: boolean; result?: { previousBranch: string; newBranch: string; message: string; + hasConflicts?: boolean; + stashedChanges?: boolean; }; error?: string; code?: 'NOT_GIT_REPO' | 'NO_COMMITS'; + stashPopConflicts?: boolean; + stashPopConflictMessage?: string; }>; // List branches (local and optionally remote) diff --git a/libs/git-utils/package.json b/libs/git-utils/package.json index ee8fbb79..1987fa6d 100644 --- a/libs/git-utils/package.json +++ b/libs/git-utils/package.json @@ -22,6 +22,7 @@ "node": ">=22.0.0 <23.0.0" }, "dependencies": { + "@automaker/platform": "1.0.0", "@automaker/types": "1.0.0", "@automaker/utils": "1.0.0" }, diff --git a/libs/git-utils/src/exec.ts b/libs/git-utils/src/exec.ts new file mode 100644 index 00000000..53d22eef --- /dev/null +++ b/libs/git-utils/src/exec.ts @@ -0,0 +1,70 @@ +/** + * Git command execution utilities + */ + +import { spawnProcess } from '@automaker/platform'; + +/** + * Execute git command with array arguments to prevent command injection. + * Uses spawnProcess from @automaker/platform for secure, cross-platform execution. + * + * @param args - Array of git command arguments (e.g., ['worktree', 'add', path]) + * @param cwd - Working directory to execute the command in + * @param env - Optional additional environment variables to pass to the git process. + * These are merged on top of the current process environment. Pass + * `{ LC_ALL: 'C' }` to force git to emit English output regardless of the + * system locale so that text-based output parsing remains reliable. + * @param abortController - Optional AbortController to cancel the git process. + * When the controller is aborted the underlying process is sent SIGTERM and + * the returned promise rejects with an Error whose message is 'Process aborted'. + * @returns Promise resolving to stdout output + * @throws Error with stderr/stdout message if command fails. The thrown error + * also has `stdout` and `stderr` string properties for structured access. + * + * @example + * ```typescript + * // Safe: no injection possible + * await execGitCommand(['branch', '-D', branchName], projectPath); + * + * // Force English output for reliable text parsing: + * await execGitCommand(['rebase', '--', 'main'], worktreePath, { LC_ALL: 'C' }); + * + * // With a process-level timeout: + * const controller = new AbortController(); + * const timerId = setTimeout(() => controller.abort(), 30_000); + * try { + * await execGitCommand(['fetch', '--all', '--quiet'], cwd, undefined, controller); + * } finally { + * clearTimeout(timerId); + * } + * + * // Instead of unsafe: + * // await execAsync(`git branch -D ${branchName}`, { cwd }); + * ``` + */ +export async function execGitCommand( + args: string[], + cwd: string, + env?: Record, + abortController?: AbortController +): Promise { + const result = await spawnProcess({ + command: 'git', + args, + cwd, + ...(env !== undefined ? { env } : {}), + ...(abortController !== undefined ? { abortController } : {}), + }); + + // spawnProcess returns { stdout, stderr, exitCode } + if (result.exitCode === 0) { + return result.stdout; + } else { + const errorMessage = + result.stderr || result.stdout || `Git command failed with code ${result.exitCode}`; + throw Object.assign(new Error(errorMessage), { + stdout: result.stdout, + stderr: result.stderr, + }); + } +} diff --git a/libs/git-utils/src/index.ts b/libs/git-utils/src/index.ts index d624a3df..cdcadffc 100644 --- a/libs/git-utils/src/index.ts +++ b/libs/git-utils/src/index.ts @@ -3,6 +3,9 @@ * Git operations utilities for AutoMaker */ +// Export command execution utilities +export { execGitCommand } from './exec.js'; + // Export types and constants export { BINARY_EXTENSIONS, GIT_STATUS_MAP, type FileStatus } from './types.js'; diff --git a/libs/utils/src/git-validation.ts b/libs/utils/src/git-validation.ts new file mode 100644 index 00000000..71ed5e4c --- /dev/null +++ b/libs/utils/src/git-validation.ts @@ -0,0 +1,43 @@ +/** + * Git validation utilities + * + * Canonical validators for git-related inputs (branch names, etc.) + * used across the server codebase. + */ + +/** Maximum allowed length for git branch names */ +export const MAX_BRANCH_NAME_LENGTH = 250; + +/** + * Validate a git branch name to prevent command injection and ensure + * it conforms to safe git ref naming rules. + * + * Enforces: + * - Allowed characters: alphanumeric, dot (.), underscore (_), slash (/), dash (-) + * - First character must NOT be a dash (prevents git argument injection via + * names like "-flag" or "--option") + * - Rejects path-traversal sequences (..) + * - Rejects NUL bytes (\0) + * - Enforces a maximum length of {@link MAX_BRANCH_NAME_LENGTH} characters + * + * @param name - The branch name to validate + * @returns `true` when the name is safe to pass to git commands + * + * @example + * ```typescript + * isValidBranchName('feature/my-branch'); // true + * isValidBranchName('-flag'); // false (starts with dash) + * isValidBranchName('a..b'); // false (contains ..) + * isValidBranchName('a\0b'); // false (contains NUL) + * ``` + */ +export function isValidBranchName(name: string): boolean { + // Must not contain NUL bytes + if (name.includes('\0')) return false; + // Must not contain path-traversal sequences + if (name.includes('..')) return false; + // First char must be alphanumeric, dot, underscore, or slash (not dash). + // Remaining chars may also include dash. + // Must be within the length limit. + return /^[a-zA-Z0-9._/][a-zA-Z0-9._\-/]*$/.test(name) && name.length < MAX_BRANCH_NAME_LENGTH; +} diff --git a/libs/utils/src/index.ts b/libs/utils/src/index.ts index cdfc52a2..5f80a806 100644 --- a/libs/utils/src/index.ts +++ b/libs/utils/src/index.ts @@ -117,3 +117,6 @@ export { type ThrottleOptions, type DebouncedFunction, } from './debounce.js'; + +// Git validation utilities +export { isValidBranchName, MAX_BRANCH_NAME_LENGTH } from './git-validation.js';