diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 917672b5..a4cd67b6 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -133,6 +133,7 @@ jobs: env: CI: true VITE_SERVER_URL: http://localhost:3008 + SERVER_URL: http://localhost:3008 VITE_SKIP_SETUP: 'true' # Keep UI-side login/defaults consistent AUTOMAKER_API_KEY: test-api-key-for-e2e-tests diff --git a/OPENCODE_CONFIG_CONTENT b/OPENCODE_CONFIG_CONTENT new file mode 100644 index 00000000..9dabfe49 --- /dev/null +++ b/OPENCODE_CONFIG_CONTENT @@ -0,0 +1,2 @@ +{ + "$schema": "https://opencode.ai/config.json",} \ No newline at end of file diff --git a/apps/server/src/routes/features/index.ts b/apps/server/src/routes/features/index.ts index 3952e480..0c92a446 100644 --- a/apps/server/src/routes/features/index.ts +++ b/apps/server/src/routes/features/index.ts @@ -33,6 +33,11 @@ export function createFeaturesRoutes( validatePathParams('projectPath'), createListHandler(featureLoader, autoModeService) ); + router.get( + '/list', + validatePathParams('projectPath'), + createListHandler(featureLoader, autoModeService) + ); router.post('/get', validatePathParams('projectPath'), createGetHandler(featureLoader)); router.post( '/create', diff --git a/apps/server/src/routes/features/routes/list.ts b/apps/server/src/routes/features/routes/list.ts index c0f22d33..71d8a04a 100644 --- a/apps/server/src/routes/features/routes/list.ts +++ b/apps/server/src/routes/features/routes/list.ts @@ -1,5 +1,7 @@ /** - * POST /list endpoint - List all features for a project + * POST/GET /list endpoint - List all features for a project + * + * projectPath may come from req.body (POST) or req.query (GET fallback). * * Also performs orphan detection when a project is loaded to identify * features whose branches no longer exist. This runs on every project load/switch. @@ -19,7 +21,17 @@ export function createListHandler( ) { return async (req: Request, res: Response): Promise => { try { - const { projectPath } = req.body as { projectPath: string }; + const bodyProjectPath = + typeof req.body === 'object' && req.body !== null + ? (req.body as { projectPath?: unknown }).projectPath + : undefined; + const queryProjectPath = req.query.projectPath; + const projectPath = + typeof bodyProjectPath === 'string' + ? bodyProjectPath + : typeof queryProjectPath === 'string' + ? queryProjectPath + : undefined; if (!projectPath) { res.status(400).json({ success: false, error: 'projectPath is required' }); diff --git a/apps/server/src/routes/worktree/common.ts b/apps/server/src/routes/worktree/common.ts index 43c66bce..5ceb50bf 100644 --- a/apps/server/src/routes/worktree/common.ts +++ b/apps/server/src/routes/worktree/common.ts @@ -2,7 +2,12 @@ * Common utilities for worktree routes */ -import { createLogger, isValidBranchName, MAX_BRANCH_NAME_LENGTH } from '@automaker/utils'; +import { + createLogger, + isValidBranchName, + isValidRemoteName, + MAX_BRANCH_NAME_LENGTH, +} from '@automaker/utils'; import { exec } from 'child_process'; import { promisify } from 'util'; import { getErrorMessage as getErrorMessageShared, createLogError } from '../common.js'; @@ -16,7 +21,7 @@ export const execAsync = promisify(exec); // 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 }; +export { isValidBranchName, isValidRemoteName, MAX_BRANCH_NAME_LENGTH }; // ============================================================================ // Extended PATH configuration for Electron apps @@ -60,25 +65,6 @@ export const execEnv = { PATH: extendedPath, }; -/** - * Validate git remote name to prevent command injection. - * Matches the strict validation used in add-remote.ts: - * - Rejects empty strings and names that are too long - * - Disallows names that start with '-' or '.' - * - Forbids the substring '..' - * - Rejects '/' characters - * - Rejects NUL bytes - * - Must consist only of alphanumerics, hyphens, underscores, and dots - */ -export function isValidRemoteName(name: string): boolean { - if (!name || name.length === 0 || name.length >= MAX_BRANCH_NAME_LENGTH) return false; - if (name.startsWith('-') || name.startsWith('.')) return false; - if (name.includes('..')) return false; - if (name.includes('/')) return false; - if (name.includes('\0')) return false; - return /^[a-zA-Z0-9._-]+$/.test(name); -} - /** * Check if gh CLI is available on the system */ diff --git a/apps/server/src/routes/worktree/routes/create-pr.ts b/apps/server/src/routes/worktree/routes/create-pr.ts index af608cef..de63aea9 100644 --- a/apps/server/src/routes/worktree/routes/create-pr.ts +++ b/apps/server/src/routes/worktree/routes/create-pr.ts @@ -17,6 +17,7 @@ import { spawnProcess } from '@automaker/platform'; import { updateWorktreePRInfo } from '../../../lib/worktree-metadata.js'; import { createLogger } from '@automaker/utils'; import { validatePRState } from '@automaker/types'; +import { resolvePrTarget } from '../../../services/pr-service.js'; const logger = createLogger('CreatePR'); @@ -32,6 +33,7 @@ export function createCreatePRHandler() { baseBranch, draft, remote, + targetRemote, } = req.body as { worktreePath: string; projectPath?: string; @@ -41,6 +43,8 @@ export function createCreatePRHandler() { baseBranch?: string; draft?: boolean; remote?: string; + /** Remote to create the PR against (e.g. upstream). If not specified, inferred from repo setup. */ + targetRemote?: string; }; if (!worktreePath) { @@ -71,6 +75,52 @@ export function createCreatePRHandler() { return; } + // --- Input validation: run all validation before any git write operations --- + + // Validate remote names before use to prevent command injection + if (remote !== undefined && !isValidRemoteName(remote)) { + res.status(400).json({ + success: false, + error: 'Invalid remote name contains unsafe characters', + }); + return; + } + if (targetRemote !== undefined && !isValidRemoteName(targetRemote)) { + res.status(400).json({ + success: false, + error: 'Invalid target remote name contains unsafe characters', + }); + return; + } + + const pushRemote = remote || 'origin'; + + // Resolve repository URL, fork workflow, and target remote information. + // This is needed for both the existing PR check and PR creation. + // Resolve early so validation errors are caught before any writes. + let repoUrl: string | null = null; + let upstreamRepo: string | null = null; + let originOwner: string | null = null; + try { + const prTarget = await resolvePrTarget({ + worktreePath, + pushRemote, + targetRemote, + }); + repoUrl = prTarget.repoUrl; + upstreamRepo = prTarget.upstreamRepo; + originOwner = prTarget.originOwner; + } catch (resolveErr) { + // resolvePrTarget throws for validation errors (unknown targetRemote, missing pushRemote) + res.status(400).json({ + success: false, + error: getErrorMessage(resolveErr), + }); + return; + } + + // --- Validation complete — proceed with git operations --- + // Check for uncommitted changes logger.debug(`Checking for uncommitted changes in: ${worktreePath}`); const { stdout: status } = await execAsync('git status --porcelain', { @@ -119,30 +169,19 @@ export function createCreatePRHandler() { } } - // Validate remote name before use to prevent command injection - if (remote !== undefined && !isValidRemoteName(remote)) { - res.status(400).json({ - success: false, - error: 'Invalid remote name contains unsafe characters', - }); - return; - } - // Push the branch to remote (use selected remote or default to 'origin') - const pushRemote = remote || 'origin'; + // Uses array-based execGitCommand to avoid shell injection from pushRemote/branchName. let pushError: string | null = null; try { - await execAsync(`git push ${pushRemote} ${branchName}`, { - cwd: worktreePath, - env: execEnv, - }); + await execGitCommand(['push', pushRemote, branchName], worktreePath, execEnv); } catch { // If push fails, try with --set-upstream try { - await execAsync(`git push --set-upstream ${pushRemote} ${branchName}`, { - cwd: worktreePath, - env: execEnv, - }); + await execGitCommand( + ['push', '--set-upstream', pushRemote, branchName], + worktreePath, + execEnv + ); } catch (error2: unknown) { // Capture push error for reporting const err = error2 as { stderr?: string; message?: string }; @@ -164,82 +203,11 @@ export function createCreatePRHandler() { const base = baseBranch || 'main'; const title = prTitle || branchName; const body = prBody || `Changes from branch ${branchName}`; - const draftFlag = draft ? '--draft' : ''; - let prUrl: string | null = null; let prError: string | null = null; let browserUrl: string | null = null; let ghCliAvailable = false; - // Get repository URL and detect fork workflow FIRST - // This is needed for both the existing PR check and PR creation - let repoUrl: string | null = null; - let upstreamRepo: string | null = null; - let originOwner: string | null = null; - try { - const { stdout: remotes } = await execAsync('git remote -v', { - cwd: worktreePath, - env: execEnv, - }); - - // Parse remotes to detect fork workflow and get repo URL - const lines = remotes.split(/\r?\n/); // Handle both Unix and Windows line endings - for (const line of lines) { - // Try multiple patterns to match different remote URL formats - // Pattern 1: git@github.com:owner/repo.git (fetch) - // Pattern 2: https://github.com/owner/repo.git (fetch) - // Pattern 3: https://github.com/owner/repo (fetch) - let match = line.match(/^(\w+)\s+.*[:/]([^/]+)\/([^/\s]+?)(?:\.git)?\s+\(fetch\)/); - if (!match) { - // Try SSH format: git@github.com:owner/repo.git - match = line.match(/^(\w+)\s+git@[^:]+:([^/]+)\/([^\s]+?)(?:\.git)?\s+\(fetch\)/); - } - if (!match) { - // Try HTTPS format: https://github.com/owner/repo.git - match = line.match( - /^(\w+)\s+https?:\/\/[^/]+\/([^/]+)\/([^\s]+?)(?:\.git)?\s+\(fetch\)/ - ); - } - - if (match) { - const [, remoteName, owner, repo] = match; - if (remoteName === 'upstream') { - upstreamRepo = `${owner}/${repo}`; - repoUrl = `https://github.com/${owner}/${repo}`; - } else if (remoteName === 'origin') { - originOwner = owner; - if (!repoUrl) { - repoUrl = `https://github.com/${owner}/${repo}`; - } - } - } - } - } catch { - // Couldn't parse remotes - will try fallback - } - - // Fallback: Try to get repo URL from git config if remote parsing failed - if (!repoUrl) { - try { - const { stdout: originUrl } = await execAsync('git config --get remote.origin.url', { - cwd: worktreePath, - env: execEnv, - }); - const url = originUrl.trim(); - - // Parse URL to extract owner/repo - // Handle both SSH (git@github.com:owner/repo.git) and HTTPS (https://github.com/owner/repo.git) - let match = url.match(/[:/]([^/]+)\/([^/\s]+?)(?:\.git)?$/); - if (match) { - const [, owner, repo] = match; - originOwner = owner; - repoUrl = `https://github.com/${owner}/${repo}`; - } - } catch { - // Failed to get repo URL from config - } - } - // Check if gh CLI is available (cross-platform) ghCliAvailable = await isGhCliAvailable(); @@ -247,13 +215,16 @@ export function createCreatePRHandler() { if (repoUrl) { const encodedTitle = encodeURIComponent(title); const encodedBody = encodeURIComponent(body); + // Encode base branch and head branch to handle special chars like # or % + const encodedBase = encodeURIComponent(base); + const encodedBranch = encodeURIComponent(branchName); if (upstreamRepo && originOwner) { - // Fork workflow: PR to upstream from origin - browserUrl = `https://github.com/${upstreamRepo}/compare/${base}...${originOwner}:${branchName}?expand=1&title=${encodedTitle}&body=${encodedBody}`; + // Fork workflow (or cross-remote PR): PR to target from push remote + browserUrl = `https://github.com/${upstreamRepo}/compare/${encodedBase}...${originOwner}:${encodedBranch}?expand=1&title=${encodedTitle}&body=${encodedBody}`; } else { // Regular repo - browserUrl = `${repoUrl}/compare/${base}...${branchName}?expand=1&title=${encodedTitle}&body=${encodedBody}`; + browserUrl = `${repoUrl}/compare/${encodedBase}...${encodedBranch}?expand=1&title=${encodedTitle}&body=${encodedBody}`; } } @@ -263,18 +234,40 @@ export function createCreatePRHandler() { if (ghCliAvailable) { // First, check if a PR already exists for this branch using gh pr list // This is more reliable than gh pr view as it explicitly searches by branch name - // For forks, we need to use owner:branch format for the head parameter + // For forks/cross-remote, we need to use owner:branch format for the head parameter const headRef = upstreamRepo && originOwner ? `${originOwner}:${branchName}` : branchName; - const repoArg = upstreamRepo ? ` --repo "${upstreamRepo}"` : ''; logger.debug(`Checking for existing PR for branch: ${branchName} (headRef: ${headRef})`); try { - const listCmd = `gh pr list${repoArg} --head "${headRef}" --json number,title,url,state --limit 1`; - logger.debug(`Running: ${listCmd}`); - const { stdout: existingPrOutput } = await execAsync(listCmd, { + const listArgs = ['pr', 'list']; + if (upstreamRepo) { + listArgs.push('--repo', upstreamRepo); + } + listArgs.push( + '--head', + headRef, + '--json', + 'number,title,url,state,createdAt', + '--limit', + '1' + ); + logger.debug(`Running: gh ${listArgs.join(' ')}`); + const listResult = await spawnProcess({ + command: 'gh', + args: listArgs, cwd: worktreePath, env: execEnv, }); + if (listResult.exitCode !== 0) { + logger.error( + `gh pr list failed with exit code ${listResult.exitCode}: ` + + `stderr=${listResult.stderr}, stdout=${listResult.stdout}` + ); + throw new Error( + `gh pr list failed (exit code ${listResult.exitCode}): ${listResult.stderr || listResult.stdout}` + ); + } + const existingPrOutput = listResult.stdout; logger.debug(`gh pr list output: ${existingPrOutput}`); const existingPrs = JSON.parse(existingPrOutput); @@ -294,7 +287,7 @@ export function createCreatePRHandler() { url: existingPr.url, title: existingPr.title || title, state: validatePRState(existingPr.state), - createdAt: new Date().toISOString(), + createdAt: existingPr.createdAt || new Date().toISOString(), }); logger.debug( `Stored existing PR info for branch ${branchName}: PR #${existingPr.number}` @@ -372,11 +365,26 @@ export function createCreatePRHandler() { if (errorMessage.toLowerCase().includes('already exists')) { logger.debug(`PR already exists error - trying to fetch existing PR`); try { - const { stdout: viewOutput } = await execAsync( - `gh pr view --json number,title,url,state`, - { cwd: worktreePath, env: execEnv } - ); - const existingPr = JSON.parse(viewOutput); + // Build args as an array to avoid shell injection. + // When upstreamRepo is set (fork/cross-remote workflow) we must + // query the upstream repository so we find the correct PR. + const viewArgs = ['pr', 'view', '--json', 'number,title,url,state,createdAt']; + if (upstreamRepo) { + viewArgs.push('--repo', upstreamRepo); + } + logger.debug(`Running: gh ${viewArgs.join(' ')}`); + const viewResult = await spawnProcess({ + command: 'gh', + args: viewArgs, + cwd: worktreePath, + env: execEnv, + }); + if (viewResult.exitCode !== 0) { + throw new Error( + `gh pr view failed (exit code ${viewResult.exitCode}): ${viewResult.stderr || viewResult.stdout}` + ); + } + const existingPr = JSON.parse(viewResult.stdout); if (existingPr.url) { prUrl = existingPr.url; prNumber = existingPr.number; @@ -388,7 +396,7 @@ export function createCreatePRHandler() { url: existingPr.url, title: existingPr.title || title, state: validatePRState(existingPr.state), - createdAt: new Date().toISOString(), + createdAt: existingPr.createdAt || new Date().toISOString(), }); logger.debug(`Fetched and stored existing PR: #${existingPr.number}`); } diff --git a/apps/server/src/routes/worktree/routes/create.ts b/apps/server/src/routes/worktree/routes/create.ts index 81243d81..10c4e956 100644 --- a/apps/server/src/routes/worktree/routes/create.ts +++ b/apps/server/src/routes/worktree/routes/create.ts @@ -83,6 +83,41 @@ async function findExistingWorktreeForBranch( } } +/** + * Detect whether a base branch reference is a remote branch (e.g. "origin/main"). + * Returns the remote name if it matches a known remote, otherwise null. + */ +async function detectRemoteBranch( + projectPath: string, + baseBranch: string +): Promise<{ remote: string; branch: string } | null> { + const slashIndex = baseBranch.indexOf('/'); + if (slashIndex <= 0) return null; + + const possibleRemote = baseBranch.substring(0, slashIndex); + + try { + // Check if this is actually a remote name by listing remotes + const stdout = await execGitCommand(['remote'], projectPath); + const remotes = stdout + .trim() + .split('\n') + .map((r: string) => r.trim()) + .filter(Boolean); + + if (remotes.includes(possibleRemote)) { + return { + remote: possibleRemote, + branch: baseBranch.substring(slashIndex + 1), + }; + } + } catch { + // Not a git repo or no remotes — fall through + } + + return null; +} + export function createCreateHandler(events: EventEmitter, settingsService?: SettingsService) { const worktreeService = new WorktreeService(); @@ -91,7 +126,7 @@ export function createCreateHandler(events: EventEmitter, settingsService?: Sett const { projectPath, branchName, baseBranch } = req.body as { projectPath: string; branchName: string; - baseBranch?: string; // Optional base branch to create from (defaults to current HEAD) + baseBranch?: string; // Optional base branch to create from (defaults to current HEAD). Can be a remote branch like "origin/main". }; if (!projectPath || !branchName) { @@ -171,6 +206,28 @@ export function createCreateHandler(events: EventEmitter, settingsService?: Sett // Create worktrees directory if it doesn't exist await secureFs.mkdir(worktreesDir, { recursive: true }); + // If a base branch is specified and it's a remote branch, fetch from that remote first + // This ensures we have the latest refs before creating the worktree + if (baseBranch && baseBranch !== 'HEAD') { + const remoteBranchInfo = await detectRemoteBranch(projectPath, baseBranch); + if (remoteBranchInfo) { + logger.info( + `Fetching from remote "${remoteBranchInfo.remote}" before creating worktree (base: ${baseBranch})` + ); + try { + await execGitCommand( + ['fetch', remoteBranchInfo.remote, remoteBranchInfo.branch], + projectPath + ); + } catch (fetchErr) { + // Non-fatal: log but continue — the ref might already be cached locally + logger.warn( + `Failed to fetch from remote "${remoteBranchInfo.remote}": ${getErrorMessage(fetchErr)}` + ); + } + } + } + // Check if branch exists (using array arguments to prevent injection) let branchExists = false; try { diff --git a/apps/server/src/routes/worktree/routes/list-branches.ts b/apps/server/src/routes/worktree/routes/list-branches.ts index 68e0bca8..724a5761 100644 --- a/apps/server/src/routes/worktree/routes/list-branches.ts +++ b/apps/server/src/routes/worktree/routes/list-branches.ts @@ -130,6 +130,7 @@ export function createListBranchesHandler() { let aheadCount = 0; let behindCount = 0; let hasRemoteBranch = false; + let trackingRemote: string | undefined; try { // First check if there's a remote tracking branch const { stdout: upstreamOutput } = await execFileAsync( @@ -138,8 +139,14 @@ export function createListBranchesHandler() { { cwd: worktreePath } ); - if (upstreamOutput.trim()) { + const upstreamRef = upstreamOutput.trim(); + if (upstreamRef) { hasRemoteBranch = true; + // Extract the remote name from the upstream ref (e.g. "origin/main" -> "origin") + const slashIndex = upstreamRef.indexOf('/'); + if (slashIndex !== -1) { + trackingRemote = upstreamRef.slice(0, slashIndex); + } const { stdout: aheadBehindOutput } = await execFileAsync( 'git', ['rev-list', '--left-right', '--count', `${currentBranch}@{upstream}...HEAD`], @@ -174,6 +181,7 @@ export function createListBranchesHandler() { behindCount, hasRemoteBranch, hasAnyRemotes, + trackingRemote, }, }); } catch (error) { diff --git a/apps/server/src/routes/worktree/routes/merge.ts b/apps/server/src/routes/worktree/routes/merge.ts index 9f8b3bb4..bcd6fbc9 100644 --- a/apps/server/src/routes/worktree/routes/merge.ts +++ b/apps/server/src/routes/worktree/routes/merge.ts @@ -20,7 +20,12 @@ export function createMergeHandler(events: EventEmitter) { branchName: string; worktreePath: string; targetBranch?: string; // Branch to merge into (defaults to 'main') - options?: { squash?: boolean; message?: string; deleteWorktreeAndBranch?: boolean }; + options?: { + squash?: boolean; + message?: string; + deleteWorktreeAndBranch?: boolean; + remote?: string; + }; }; if (!projectPath || !branchName || !worktreePath) { diff --git a/apps/server/src/routes/worktree/routes/rebase.ts b/apps/server/src/routes/worktree/routes/rebase.ts index 7efb2c4b..05dc1e6a 100644 --- a/apps/server/src/routes/worktree/routes/rebase.ts +++ b/apps/server/src/routes/worktree/routes/rebase.ts @@ -14,17 +14,19 @@ import type { Request, Response } from 'express'; import path from 'path'; -import { getErrorMessage, logError, isValidBranchName } from '../common.js'; +import { getErrorMessage, logError, isValidBranchName, isValidRemoteName } from '../common.js'; import type { EventEmitter } from '../../../lib/events.js'; import { runRebase } from '../../../services/rebase-service.js'; export function createRebaseHandler(events: EventEmitter) { return async (req: Request, res: Response): Promise => { try { - const { worktreePath, ontoBranch } = req.body as { + const { worktreePath, ontoBranch, remote } = req.body as { worktreePath: string; /** The branch/ref to rebase onto (e.g., 'origin/main', 'main') */ ontoBranch: string; + /** Remote name to fetch from before rebasing (defaults to 'origin') */ + remote?: string; }; if (!worktreePath) { @@ -55,6 +57,15 @@ export function createRebaseHandler(events: EventEmitter) { return; } + // Validate optional remote name to reject unsafe characters at the route layer + if (remote !== undefined && !isValidRemoteName(remote)) { + res.status(400).json({ + success: false, + error: `Invalid remote name: "${remote}"`, + }); + return; + } + // Emit started event events.emit('rebase:started', { worktreePath: resolvedWorktreePath, @@ -62,7 +73,7 @@ export function createRebaseHandler(events: EventEmitter) { }); // Execute the rebase via the service - const result = await runRebase(resolvedWorktreePath, ontoBranch); + const result = await runRebase(resolvedWorktreePath, ontoBranch, { remote }); if (result.success) { // Emit success event diff --git a/apps/server/src/services/execution-service.ts b/apps/server/src/services/execution-service.ts index 3ebce443..8e3dc1bd 100644 --- a/apps/server/src/services/execution-service.ts +++ b/apps/server/src/services/execution-service.ts @@ -168,6 +168,20 @@ ${feature.spec} feature = await this.loadFeatureFn(projectPath, featureId); if (!feature) throw new Error(`Feature ${featureId} not found`); + // Update status to in_progress immediately after acquiring the feature. + // This prevents a race condition where the UI reloads features and sees the + // feature still in 'backlog' status while it's actually being executed. + // Only do this for the initial call (not internal/recursive calls which would + // redundantly update the status). + if ( + !options?._calledInternally && + (feature.status === 'backlog' || + feature.status === 'ready' || + feature.status === 'interrupted') + ) { + await this.updateFeatureStatusFn(projectPath, featureId, 'in_progress'); + } + if (!options?.continuationPrompt) { if (feature.planSpec?.status === 'approved') { const prompts = await getPromptCustomization(this.settingsService, '[ExecutionService]'); @@ -199,7 +213,18 @@ ${feature.spec} validateWorkingDirectory(workDir); tempRunningFeature.worktreePath = worktreePath; tempRunningFeature.branchName = branchName ?? null; - await this.updateFeatureStatusFn(projectPath, featureId, 'in_progress'); + // Ensure status is in_progress (may already be set from the early update above, + // but internal/recursive calls skip the early update and need it here). + // Mirror the external guard: only transition when the feature is still in + // backlog, ready, or interrupted to avoid overwriting a concurrent terminal status. + if ( + options?._calledInternally && + (feature.status === 'backlog' || + feature.status === 'ready' || + feature.status === 'interrupted') + ) { + await this.updateFeatureStatusFn(projectPath, featureId, 'in_progress'); + } this.eventBus.emitAutoModeEvent('auto_mode_feature_start', { featureId, projectPath, diff --git a/apps/server/src/services/feature-loader.ts b/apps/server/src/services/feature-loader.ts index 941194b7..5b21e44b 100644 --- a/apps/server/src/services/feature-loader.ts +++ b/apps/server/src/services/feature-loader.ts @@ -225,6 +225,14 @@ export class FeatureLoader { return null; } + // Clear transient runtime flag - titleGenerating is only meaningful during + // the current session's async title generation. If it was persisted (e.g., + // app closed before generation completed), it would cause the UI to show + // "Generating title..." indefinitely. + if (feature.titleGenerating) { + delete feature.titleGenerating; + } + return feature; }); @@ -323,7 +331,14 @@ export class FeatureLoader { logRecoveryWarning(result, `Feature ${featureId}`, logger); - return result.data; + const feature = result.data; + + // Clear transient runtime flag (same as in getAll) + if (feature?.titleGenerating) { + delete feature.titleGenerating; + } + + return feature; } /** @@ -367,8 +382,15 @@ export class FeatureLoader { descriptionHistory: initialHistory, }; + // Remove transient runtime fields before persisting to disk. + // titleGenerating is UI-only state that tracks in-flight async title generation. + // Persisting it can cause cards to show "Generating title..." indefinitely + // if the app restarts before generation completes. + const featureToWrite = { ...feature }; + delete featureToWrite.titleGenerating; + // Write feature.json atomically with backup support - await atomicWriteJson(featureJsonPath, feature, { backupCount: DEFAULT_BACKUP_COUNT }); + await atomicWriteJson(featureJsonPath, featureToWrite, { backupCount: DEFAULT_BACKUP_COUNT }); logger.info(`Created feature ${featureId}`); return feature; @@ -452,9 +474,13 @@ export class FeatureLoader { descriptionHistory: updatedHistory, }; + // Remove transient runtime fields before persisting (same as create) + const featureToWrite = { ...updatedFeature }; + delete featureToWrite.titleGenerating; + // Write back to file atomically with backup support const featureJsonPath = this.getFeatureJsonPath(projectPath, featureId); - await atomicWriteJson(featureJsonPath, updatedFeature, { backupCount: DEFAULT_BACKUP_COUNT }); + await atomicWriteJson(featureJsonPath, featureToWrite, { backupCount: DEFAULT_BACKUP_COUNT }); logger.info(`Updated feature ${featureId}`); return updatedFeature; diff --git a/apps/server/src/services/merge-service.ts b/apps/server/src/services/merge-service.ts index 1a814acc..0301d4a8 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, isValidBranchName } from '@automaker/utils'; +import { createLogger, isValidBranchName, isValidRemoteName } from '@automaker/utils'; import { type EventEmitter } from '../lib/events.js'; import { execGitCommand } from '@automaker/git-utils'; const logger = createLogger('MergeService'); @@ -13,6 +13,8 @@ export interface MergeOptions { squash?: boolean; message?: string; deleteWorktreeAndBranch?: boolean; + /** Remote name to fetch from before merging (defaults to 'origin') */ + remote?: string; } export interface MergeServiceResult { @@ -35,7 +37,11 @@ export interface MergeServiceResult { * @param branchName - Source branch to merge * @param worktreePath - Path to the worktree (used for deletion if requested) * @param targetBranch - Branch to merge into (defaults to 'main') - * @param options - Merge options (squash, message, deleteWorktreeAndBranch) + * @param options - Merge options + * @param options.squash - If true, perform a squash merge + * @param options.message - Custom merge commit message + * @param options.deleteWorktreeAndBranch - If true, delete worktree and branch after merge + * @param options.remote - Remote name to fetch from before merging (defaults to 'origin') */ export async function performMerge( projectPath: string, @@ -88,6 +94,33 @@ export async function performMerge( }; } + // Validate the remote name to prevent git option injection. + // Reject invalid remote names so the caller knows their input was wrong, + // consistent with how invalid branch names are handled above. + const remote = options?.remote || 'origin'; + if (!isValidRemoteName(remote)) { + logger.warn('Invalid remote name supplied to merge-service', { + remote, + projectPath, + }); + return { + success: false, + error: `Invalid remote name: "${remote}"`, + }; + } + + // Fetch latest from remote before merging to ensure we have up-to-date refs + try { + await execGitCommand(['fetch', remote], projectPath); + } catch (fetchError) { + logger.warn('Failed to fetch from remote before merge; proceeding with local refs', { + remote, + projectPath, + error: (fetchError as Error).message, + }); + // Non-fatal: proceed with local refs if fetch fails (e.g. offline) + } + // Emit merge:start after validating inputs emitter?.emit('merge:start', { branchName, targetBranch: mergeTo, worktreePath }); diff --git a/apps/server/src/services/pr-service.ts b/apps/server/src/services/pr-service.ts new file mode 100644 index 00000000..4b26f35b --- /dev/null +++ b/apps/server/src/services/pr-service.ts @@ -0,0 +1,225 @@ +/** + * Service for resolving PR target information from git remotes. + * + * Extracts remote-parsing and target-resolution logic that was previously + * inline in the create-pr route handler. + */ + +// TODO: Move execAsync/execEnv to a shared lib (lib/exec.ts or @automaker/utils) so that +// services no longer depend on route internals. Tracking issue: route-to-service dependency +// inversion. For now, a local thin wrapper is used within the service boundary. +import { exec } from 'child_process'; +import { promisify } from 'util'; +import { createLogger, isValidRemoteName } from '@automaker/utils'; + +// Thin local wrapper — duplicates the route-level execAsync/execEnv until a +// shared lib/exec.ts (or @automaker/utils export) is created. +const execAsync = promisify(exec); + +const pathSeparator = process.platform === 'win32' ? ';' : ':'; +const _additionalPaths: string[] = []; +if (process.platform === 'win32') { + if (process.env.LOCALAPPDATA) + _additionalPaths.push(`${process.env.LOCALAPPDATA}\\Programs\\Git\\cmd`); + if (process.env.PROGRAMFILES) _additionalPaths.push(`${process.env.PROGRAMFILES}\\Git\\cmd`); + if (process.env['ProgramFiles(x86)']) + _additionalPaths.push(`${process.env['ProgramFiles(x86)']}\\Git\\cmd`); +} else { + _additionalPaths.push( + '/opt/homebrew/bin', + '/usr/local/bin', + '/home/linuxbrew/.linuxbrew/bin', + `${process.env.HOME}/.local/bin` + ); +} +const execEnv = { + ...process.env, + PATH: [process.env.PATH, ..._additionalPaths.filter(Boolean)].filter(Boolean).join(pathSeparator), +}; + +const logger = createLogger('PRService'); + +export interface ParsedRemote { + owner: string; + repo: string; +} + +export interface PrTargetResult { + repoUrl: string | null; + targetRepo: string | null; + pushOwner: string | null; + upstreamRepo: string | null; + originOwner: string | null; + parsedRemotes: Map; +} + +/** + * Parse all git remotes for the given repo path and resolve the PR target. + * + * @param worktreePath - Working directory of the repository / worktree + * @param pushRemote - Remote used for pushing (e.g. "origin") + * @param targetRemote - Explicit remote to target the PR against (optional) + * + * @throws {Error} When targetRemote is specified but not found among repository remotes + * @throws {Error} When pushRemote is not found among parsed remotes (when targetRemote is specified) + */ +export async function resolvePrTarget({ + worktreePath, + pushRemote, + targetRemote, +}: { + worktreePath: string; + pushRemote: string; + targetRemote?: string; +}): Promise { + // Validate remote names — pushRemote is a required string so the undefined + // guard is unnecessary, but targetRemote is optional. + if (!isValidRemoteName(pushRemote)) { + throw new Error(`Invalid push remote name: "${pushRemote}"`); + } + if (targetRemote !== undefined && !isValidRemoteName(targetRemote)) { + throw new Error(`Invalid target remote name: "${targetRemote}"`); + } + + let repoUrl: string | null = null; + let upstreamRepo: string | null = null; + let originOwner: string | null = null; + const parsedRemotes: Map = new Map(); + + try { + const { stdout: remotes } = await execAsync('git remote -v', { + cwd: worktreePath, + env: execEnv, + }); + + // Parse remotes to detect fork workflow and get repo URL + const lines = remotes.split(/\r?\n/); // Handle both Unix and Windows line endings + for (const line of lines) { + // Try multiple patterns to match different remote URL formats + // Pattern 1: git@github.com:owner/repo.git (fetch) + // Pattern 2: https://github.com/owner/repo.git (fetch) + // Pattern 3: https://github.com/owner/repo (fetch) + let match = line.match( + /^([a-zA-Z0-9._-]+)\s+.*[:/]([^/]+)\/([^/\s]+?)(?:\.git)?\s+\(fetch\)/ + ); + if (!match) { + // Try SSH format: git@github.com:owner/repo.git + match = line.match( + /^([a-zA-Z0-9._-]+)\s+git@[^:]+:([^/]+)\/([^\s]+?)(?:\.git)?\s+\(fetch\)/ + ); + } + if (!match) { + // Try HTTPS format: https://github.com/owner/repo.git + match = line.match( + /^([a-zA-Z0-9._-]+)\s+https?:\/\/[^/]+\/([^/]+)\/([^\s]+?)(?:\.git)?\s+\(fetch\)/ + ); + } + + if (match) { + const [, remoteName, owner, repo] = match; + parsedRemotes.set(remoteName, { owner, repo }); + if (remoteName === 'upstream') { + upstreamRepo = `${owner}/${repo}`; + repoUrl = `https://github.com/${owner}/${repo}`; + } else if (remoteName === 'origin') { + originOwner = owner; + if (!repoUrl) { + repoUrl = `https://github.com/${owner}/${repo}`; + } + } + } + } + } catch (err) { + // Log the failure for debugging — control flow falls through to auto-detection + logger.debug('Failed to parse git remotes', { worktreePath, error: err }); + } + + // When targetRemote is explicitly provided but remote parsing failed entirely + // (parsedRemotes is empty), we cannot validate or resolve the requested remote. + // Silently proceeding to auto-detection would ignore the caller's explicit intent, + // so we fail fast with a clear error instead. + if (targetRemote && parsedRemotes.size === 0) { + throw new Error( + `targetRemote "${targetRemote}" was specified but no remotes could be parsed from the repository. ` + + `Ensure the repository has at least one configured remote (parsedRemotes is empty).` + ); + } + + // When a targetRemote is explicitly specified, validate that it is known + // before using it. Silently falling back to auto-detection when the caller + // explicitly requested a remote that doesn't exist is misleading, so we + // fail fast here instead. + if (targetRemote && parsedRemotes.size > 0 && !parsedRemotes.has(targetRemote)) { + throw new Error(`targetRemote "${targetRemote}" not found in repository remotes`); + } + + // When a targetRemote is explicitly specified, override fork detection + // to use the specified remote as the PR target + let targetRepo: string | null = null; + let pushOwner: string | null = null; + if (targetRemote && parsedRemotes.size > 0) { + const targetInfo = parsedRemotes.get(targetRemote); + const pushInfo = parsedRemotes.get(pushRemote); + + // If the push remote is not found in the parsed remotes, we cannot + // determine the push owner and would build incorrect URLs. Fail fast + // instead of silently proceeding with null values. + if (!pushInfo) { + logger.warn('Push remote not found in parsed remotes', { + pushRemote, + targetRemote, + availableRemotes: [...parsedRemotes.keys()], + }); + throw new Error(`Push remote "${pushRemote}" not found in repository remotes`); + } + + if (targetInfo) { + targetRepo = `${targetInfo.owner}/${targetInfo.repo}`; + repoUrl = `https://github.com/${targetInfo.owner}/${targetInfo.repo}`; + } + pushOwner = pushInfo.owner; + + // Override the auto-detected upstream/origin with explicit targetRemote + // Only treat as cross-remote if target differs from push remote + if (targetRemote !== pushRemote && targetInfo) { + upstreamRepo = targetRepo; + originOwner = pushOwner; + } else if (targetInfo) { + // Same remote for push and target - regular (non-fork) workflow + upstreamRepo = null; + originOwner = targetInfo.owner; + repoUrl = `https://github.com/${targetInfo.owner}/${targetInfo.repo}`; + } + } + + // Fallback: Try to get repo URL from git config if remote parsing failed + if (!repoUrl) { + try { + const { stdout: originUrl } = await execAsync('git config --get remote.origin.url', { + cwd: worktreePath, + env: execEnv, + }); + const url = originUrl.trim(); + + // Parse URL to extract owner/repo + // Handle both SSH (git@github.com:owner/repo.git) and HTTPS (https://github.com/owner/repo.git) + const match = url.match(/[:/]([^/]+)\/([^/\s]+?)(?:\.git)?$/); + if (match) { + const [, owner, repo] = match; + originOwner = owner; + repoUrl = `https://github.com/${owner}/${repo}`; + } + } catch { + // Failed to get repo URL from config + } + } + + return { + repoUrl, + targetRepo, + pushOwner, + upstreamRepo, + originOwner, + parsedRemotes, + }; +} diff --git a/apps/server/src/services/rebase-service.ts b/apps/server/src/services/rebase-service.ts index 14f806e9..758a667b 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, getErrorMessage } from '@automaker/utils'; +import { createLogger, getErrorMessage, isValidRemoteName } from '@automaker/utils'; import { execGitCommand, getCurrentBranch, getConflictFiles } from '@automaker/git-utils'; const logger = createLogger('RebaseService'); @@ -16,6 +16,11 @@ const logger = createLogger('RebaseService'); // Types // ============================================================================ +export interface RebaseOptions { + /** Remote name to fetch from before rebasing (defaults to 'origin') */ + remote?: string; +} + export interface RebaseResult { success: boolean; error?: string; @@ -36,9 +41,14 @@ export interface RebaseResult { * * @param worktreePath - Path to the git worktree * @param ontoBranch - The branch to rebase onto (e.g., 'origin/main') + * @param options - Optional rebase options (remote name for fetch) * @returns RebaseResult with success/failure information */ -export async function runRebase(worktreePath: string, ontoBranch: string): Promise { +export async function runRebase( + worktreePath: string, + ontoBranch: string, + options?: RebaseOptions +): Promise { // Reject empty, whitespace-only, or dash-prefixed branch names. const normalizedOntoBranch = ontoBranch?.trim() ?? ''; if (normalizedOntoBranch === '' || normalizedOntoBranch.startsWith('-')) { @@ -59,6 +69,33 @@ export async function runRebase(worktreePath: string, ontoBranch: string): Promi }; } + // Validate the remote name to prevent git option injection. + // Reject invalid remote names so the caller knows their input was wrong, + // consistent with how invalid branch names are handled above. + const remote = options?.remote || 'origin'; + if (!isValidRemoteName(remote)) { + logger.warn('Invalid remote name supplied to rebase-service', { + remote, + worktreePath, + }); + return { + success: false, + error: `Invalid remote name: "${remote}"`, + }; + } + + // Fetch latest from remote before rebasing to ensure we have up-to-date refs + try { + await execGitCommand(['fetch', remote], worktreePath); + } catch (fetchError) { + logger.warn('Failed to fetch from remote before rebase; proceeding with local refs', { + remote, + worktreePath, + error: getErrorMessage(fetchError), + }); + // Non-fatal: proceed with local refs if fetch fails (e.g. offline) + } + try { // Pass ontoBranch after '--' so git treats it as a ref, not an option. // Set LC_ALL=C so git always emits English output regardless of the system diff --git a/apps/ui/eslint.config.mjs b/apps/ui/eslint.config.mjs index 2b40bd8f..f7611145 100644 --- a/apps/ui/eslint.config.mjs +++ b/apps/ui/eslint.config.mjs @@ -53,6 +53,7 @@ const eslintConfig = defineConfig([ requestAnimationFrame: 'readonly', cancelAnimationFrame: 'readonly', requestIdleCallback: 'readonly', + cancelIdleCallback: 'readonly', alert: 'readonly', // DOM Element Types HTMLElement: 'readonly', diff --git a/apps/ui/index.html b/apps/ui/index.html index 3f12c9b0..afc0f07f 100644 --- a/apps/ui/index.html +++ b/apps/ui/index.html @@ -72,30 +72,169 @@ height: 100dvh; overflow: hidden; } + /* Inline app shell: shows logo + spinner while JS bundle downloads. + On slow mobile networks the bundle can take 2-5s; this eliminates + the blank screen and gives immediate visual feedback. + React's createRoot().render() replaces #app's innerHTML, removing this. */ + .app-shell { + display: flex; + flex-direction: column; + align-items: center; + justify-content: center; + height: 100%; + gap: 24px; + opacity: 1; + } + .app-shell-logo { + width: 56px; + height: 56px; + opacity: 0.9; + } + /* Default (dark): light spinner + logo strokes */ + .app-shell-spinner { + width: 20px; + height: 20px; + border: 2px solid rgba(255, 255, 255, 0.1); + border-top-color: rgba(255, 255, 255, 0.5); + border-radius: 50%; + animation: shell-spin 0.8s linear infinite; + } + .app-shell-logo-stroke { + stroke: rgba(255, 255, 255, 0.7); + } + .app-shell-logo-bg { + fill: rgba(255, 255, 255, 0.08); + } + /* Light themes: dark spinner + logo strokes. + The theme script below sets data-theme-type="light" on for any light + theme, so future theme additions only need to update the script — not CSS. */ + html[data-theme-type='light'] .app-shell-spinner { + border-color: rgba(0, 0, 0, 0.08); + border-top-color: rgba(0, 0, 0, 0.4); + } + html[data-theme-type='light'] .app-shell-logo-stroke { + stroke: rgba(0, 0, 0, 0.55); + } + html[data-theme-type='light'] .app-shell-logo-bg { + fill: rgba(0, 0, 0, 0.06); + } + /* System light preference when no theme class is applied yet */ + @media (prefers-color-scheme: light) { + html:not([class]) .app-shell-spinner { + border-color: rgba(0, 0, 0, 0.08); + border-top-color: rgba(0, 0, 0, 0.4); + } + html:not([class]) .app-shell-logo-stroke { + stroke: rgba(0, 0, 0, 0.55); + } + html:not([class]) .app-shell-logo-bg { + fill: rgba(0, 0, 0, 0.06); + } + } + @keyframes shell-spin { + to { + transform: rotate(360deg); + } + } -
+
+ +
+ + +
+
diff --git a/apps/ui/public/sw.js b/apps/ui/public/sw.js index ce97fc2a..1370cb7c 100644 --- a/apps/ui/public/sw.js +++ b/apps/ui/public/sw.js @@ -1,8 +1,8 @@ // Automaker Service Worker - Optimized for mobile PWA loading performance // NOTE: CACHE_NAME is injected with a build hash at build time by the swCacheBuster // Vite plugin (see vite.config.mts). In development it stays as-is; in production -// builds it becomes e.g. 'automaker-v3-a1b2c3d4' for automatic cache invalidation. -const CACHE_NAME = 'automaker-v3'; // replaced at build time → 'automaker-v3-' +// builds it becomes e.g. 'automaker-v5-a1b2c3d4' for automatic cache invalidation. +const CACHE_NAME = 'automaker-v5'; // replaced at build time → 'automaker-v5-' // Separate cache for immutable hashed assets (long-lived) const IMMUTABLE_CACHE = 'automaker-immutable-v2'; @@ -13,6 +13,7 @@ const API_CACHE = 'automaker-api-v1'; // Assets to cache on install (app shell for instant loading) const SHELL_ASSETS = [ '/', + '/index.html', '/manifest.json', '/logo.png', '/logo_larger.png', @@ -20,6 +21,12 @@ const SHELL_ASSETS = [ '/favicon.ico', ]; +// Critical JS/CSS assets extracted from index.html at build time by the swCacheBuster +// Vite plugin. Populated during production builds; empty in dev mode. +// These are precached on SW install so that PWA cold starts after memory eviction +// serve instantly from cache instead of requiring a full network download. +const CRITICAL_ASSETS = []; + // Whether mobile caching is enabled (set via message from main thread). // Persisted to Cache Storage so it survives aggressive SW termination on mobile. let mobileMode = false; @@ -60,7 +67,10 @@ async function restoreMobileMode() { } // Restore mobileMode immediately on SW startup -restoreMobileMode(); +// Keep a promise so fetch handlers can await restoration on cold SW starts. +// This prevents a race where early API requests run before mobileMode is loaded +// from Cache Storage, incorrectly falling back to network-first. +const mobileModeRestorePromise = restoreMobileMode(); // API endpoints that are safe to serve from stale cache on mobile. // These are GET-only, read-heavy endpoints where showing slightly stale data @@ -121,13 +131,68 @@ async function addCacheTimestamp(response) { } self.addEventListener('install', (event) => { + // Cache the app shell AND critical JS/CSS assets so the PWA loads instantly. + // SHELL_ASSETS go into CACHE_NAME (general cache), CRITICAL_ASSETS go into + // IMMUTABLE_CACHE (long-lived, content-hashed assets). This ensures that even + // the very first visit populates the immutable cache — previously, assets were + // only cached on fetch interception, but the SW isn't active during the first + // page load so nothing got cached until the second visit. + // + // self.skipWaiting() is NOT called here — activation is deferred until the main + // thread sends a SKIP_WAITING message to avoid disrupting a live page. event.waitUntil( - caches.open(CACHE_NAME).then((cache) => { - return cache.addAll(SHELL_ASSETS); - }) + Promise.all([ + // Cache app shell (HTML, icons, manifest) using individual fetch+put instead of + // cache.addAll(). This is critical because cache.addAll() respects the server's + // Cache-Control response headers — if the server sends 'Cache-Control: no-store' + // (which Vite dev server does for index.html), addAll() silently skips caching + // and the pre-React loading spinner is never served from cache. + // + // cache.put() bypasses Cache-Control headers entirely, ensuring the app shell + // is always cached on install regardless of what the server sends. This is the + // correct approach for SW-managed caches where the SW (not HTTP headers) controls + // freshness via the activate event's cache cleanup and the navigation strategy's + // background revalidation. + caches.open(CACHE_NAME).then((cache) => + Promise.all( + SHELL_ASSETS.map((url) => + fetch(url) + .then((response) => { + if (response.ok) return cache.put(url, response); + }) + .catch(() => { + // Individual asset fetch failure is non-fatal — the SW still activates + // and the next navigation will populate the cache via Strategy 3. + }) + ) + ) + ), + // Cache critical JS/CSS bundles (injected at build time by swCacheBuster). + // Uses individual fetch+put instead of cache.addAll() so a single asset + // failure doesn't prevent the rest from being cached. + // + // IMPORTANT: We fetch with { mode: 'cors' } because Vite's output HTML uses + //