From 462dbf1522d91bc2e7610d7b6e19b172bc50f87e Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 16 Feb 2026 11:53:09 -0800 Subject: [PATCH] fix: Address code review comments --- .../routes/auto-mode/routes/approve-plan.ts | 12 +++- .../server/src/routes/features/routes/list.ts | 23 ++++--- .../routes/github/routes/validate-issue.ts | 5 +- apps/server/src/services/agent-executor.ts | 2 + .../src/services/auto-loop-coordinator.ts | 20 ++++-- apps/server/src/services/auto-mode/facade.ts | 67 +++++++++++-------- apps/server/src/services/execution-service.ts | 9 ++- .../src/services/feature-state-manager.ts | 7 ++ apps/server/src/services/merge-service.ts | 44 +++++++----- .../src/services/pipeline-orchestrator.ts | 3 +- .../src/services/plan-approval-service.ts | 13 +++- .../src/components/views/graph-view-page.tsx | 16 +++-- 12 files changed, 147 insertions(+), 74 deletions(-) diff --git a/apps/server/src/routes/auto-mode/routes/approve-plan.ts b/apps/server/src/routes/auto-mode/routes/approve-plan.ts index 277b50e2..14673e31 100644 --- a/apps/server/src/routes/auto-mode/routes/approve-plan.ts +++ b/apps/server/src/routes/auto-mode/routes/approve-plan.ts @@ -17,7 +17,7 @@ export function createApprovePlanHandler(autoModeService: AutoModeServiceCompat) approved: boolean; editedPlan?: string; feedback?: string; - projectPath?: string; + projectPath: string; }; if (!featureId) { @@ -36,6 +36,14 @@ export function createApprovePlanHandler(autoModeService: AutoModeServiceCompat) return; } + if (!projectPath) { + res.status(400).json({ + success: false, + error: 'projectPath is required', + }); + return; + } + // Note: We no longer check hasPendingApproval here because resolvePlanApproval // can handle recovery when pending approval is not in Map but feature has planSpec.status='generated' // This supports cases where the server restarted while waiting for approval @@ -48,7 +56,7 @@ export function createApprovePlanHandler(autoModeService: AutoModeServiceCompat) // Resolve the pending approval (with recovery support) const result = await autoModeService.resolvePlanApproval( - projectPath || '', + projectPath, featureId, approved, editedPlan, diff --git a/apps/server/src/routes/features/routes/list.ts b/apps/server/src/routes/features/routes/list.ts index 766e625c..c0f22d33 100644 --- a/apps/server/src/routes/features/routes/list.ts +++ b/apps/server/src/routes/features/routes/list.ts @@ -33,18 +33,23 @@ export function createListHandler( // We don't await this to keep the list response fast // Note: detectOrphanedFeatures handles errors internally and always resolves if (autoModeService) { - autoModeService.detectOrphanedFeatures(projectPath).then((orphanedFeatures) => { - if (orphanedFeatures.length > 0) { - logger.info( - `[ProjectLoad] Detected ${orphanedFeatures.length} orphaned feature(s) in ${projectPath}` - ); - for (const { feature, missingBranch } of orphanedFeatures) { + autoModeService + .detectOrphanedFeatures(projectPath) + .then((orphanedFeatures) => { + if (orphanedFeatures.length > 0) { logger.info( - `[ProjectLoad] Orphaned: ${feature.title || feature.id} - branch "${missingBranch}" no longer exists` + `[ProjectLoad] Detected ${orphanedFeatures.length} orphaned feature(s) in ${projectPath}` ); + for (const { feature, missingBranch } of orphanedFeatures) { + logger.info( + `[ProjectLoad] Orphaned: ${feature.title || feature.id} - branch "${missingBranch}" no longer exists` + ); + } } - } - }); + }) + .catch((error) => { + logger.warn(`[ProjectLoad] Orphan detection failed for ${projectPath}:`, error); + }); } res.json({ success: true, features }); diff --git a/apps/server/src/routes/github/routes/validate-issue.ts b/apps/server/src/routes/github/routes/validate-issue.ts index 9f3af5cf..38220f6d 100644 --- a/apps/server/src/routes/github/routes/validate-issue.ts +++ b/apps/server/src/routes/github/routes/validate-issue.ts @@ -25,7 +25,7 @@ import { isOpencodeModel, supportsStructuredOutput, } from '@automaker/types'; -import { resolvePhaseModel } from '@automaker/model-resolver'; +import { resolvePhaseModel, resolveModelString } from '@automaker/model-resolver'; import { extractJson } from '../../../lib/json-extractor.js'; import { writeValidation } from '../../../lib/validation-storage.js'; import { streamingQuery } from '../../../providers/simple-query-service.js'; @@ -190,9 +190,10 @@ ${basePrompt}`; // CRITICAL: For custom providers (GLM, MiniMax), pass the provider's model ID (e.g. "GLM-4.7") // to the API, NOT the resolved Claude model - otherwise we get "model not found" + // For standard Claude models, resolve aliases (e.g., 'opus' -> 'claude-opus-4-20250514') const effectiveModel = claudeCompatibleProvider ? (model as string) - : providerResolvedModel || (model as string); + : providerResolvedModel || resolveModelString(model as string); logger.info(`Using model: ${effectiveModel}`); // Use streamingQuery with event callbacks diff --git a/apps/server/src/services/agent-executor.ts b/apps/server/src/services/agent-executor.ts index 1ccb7497..dd1c179c 100644 --- a/apps/server/src/services/agent-executor.ts +++ b/apps/server/src/services/agent-executor.ts @@ -560,6 +560,7 @@ export class AgentExecutor { revText += b.text || ''; this.eventBus.emitAutoModeEvent('auto_mode_progress', { featureId, + branchName, content: b.text, }); } @@ -638,6 +639,7 @@ export class AgentExecutor { cwd: o.workDir, allowedTools: o.sdkOptions?.allowedTools as string[] | undefined, abortController: o.abortController, + thinkingLevel: o.thinkingLevel, mcpServers: o.mcpServers && Object.keys(o.mcpServers).length > 0 ? (o.mcpServers as Record) diff --git a/apps/server/src/services/auto-loop-coordinator.ts b/apps/server/src/services/auto-loop-coordinator.ts index 0b03e5f8..3e63cff1 100644 --- a/apps/server/src/services/auto-loop-coordinator.ts +++ b/apps/server/src/services/auto-loop-coordinator.ts @@ -31,8 +31,16 @@ export interface ProjectAutoLoopState { branchName: string | null; } +/** + * Generate a unique key for a worktree auto-loop instance. + * + * When branchName is null, this represents the main worktree (uses '__main__' sentinel). + * Named branches always use their exact name — the caller is responsible for passing + * null for the primary branch (main/master/etc.) so key matching stays consistent + * with ConcurrencyManager's dynamic primary branch resolution. + */ export function getWorktreeAutoLoopKey(projectPath: string, branchName: string | null): string { - return `${projectPath}::${(branchName === 'main' ? null : branchName) ?? '__main__'}`; + return `${projectPath}::${branchName ?? '__main__'}`; } export type ExecuteFeatureFn = ( @@ -404,11 +412,15 @@ export class AutoLoopCoordinator { reject(new Error('Aborted')); return; } - const timeout = setTimeout(resolve, ms); - signal?.addEventListener('abort', () => { + const onAbort = () => { clearTimeout(timeout); reject(new Error('Aborted')); - }); + }; + const timeout = setTimeout(() => { + signal?.removeEventListener('abort', onAbort); + resolve(); + }, ms); + signal?.addEventListener('abort', onAbort); }); } } diff --git a/apps/server/src/services/auto-mode/facade.ts b/apps/server/src/services/auto-mode/facade.ts index 83acf678..af909d30 100644 --- a/apps/server/src/services/auto-mode/facade.ts +++ b/apps/server/src/services/auto-mode/facade.ts @@ -17,7 +17,7 @@ import { promisify } from 'util'; import type { Feature, PlanningMode, ThinkingLevel } from '@automaker/types'; import { DEFAULT_MAX_CONCURRENCY, stripProviderPrefix } from '@automaker/types'; import { createLogger, loadContextFiles, classifyError } from '@automaker/utils'; -import { getFeatureDir } from '@automaker/platform'; +import { getFeatureDir, spawnProcess } 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'; @@ -48,6 +48,24 @@ import type { const execAsync = promisify(exec); const logger = createLogger('AutoModeServiceFacade'); +/** + * Execute git command with array arguments to prevent command injection. + */ +async function execGitCommand(args: string[], cwd: string): Promise { + const result = await spawnProcess({ + command: 'git', + args, + cwd, + }); + + if (result.exitCode === 0) { + return result.stdout; + } else { + const errorMessage = result.stderr || `Git command failed with code ${result.exitCode}`; + throw new Error(errorMessage); + } +} + /** * AutoModeServiceFacade provides a clean interface for auto-mode functionality. * @@ -589,19 +607,8 @@ ${prompt} Address the follow-up instructions above. Review the previous work and make the requested changes or fixes.`; try { - this.eventBus.emitAutoModeEvent('auto_mode_feature_start', { - featureId, - projectPath: this.projectPath, - branchName: feature?.branchName ?? null, - feature: { - id: featureId, - title: feature?.title || 'Follow-up', - description: feature?.description || 'Following up on feature', - }, - }); - // NOTE: Facade does not have runAgent - this method requires AutoModeService - // For now, throw to indicate routes should use AutoModeService.followUpFeature + // Do NOT emit start events before throwing to prevent false start events throw new Error( 'followUpFeature not fully implemented in facade - use AutoModeService.followUpFeature instead' ); @@ -691,18 +698,22 @@ Address the follow-up instructions above. Review the previous work and make the // Use project path } } else { - const sanitizedFeatureId = featureId.replace(/[^a-zA-Z0-9_-]/g, '-'); - const legacyWorktreePath = path.join(this.projectPath, '.worktrees', sanitizedFeatureId); - try { - await secureFs.access(legacyWorktreePath); - workDir = legacyWorktreePath; - } catch { - // Use project path + // Use worktreeResolver instead of manual .worktrees lookup + const feature = await this.featureStateManager.loadFeature(this.projectPath, featureId); + const branchName = feature?.branchName; + if (branchName) { + const resolved = await this.worktreeResolver.findWorktreeForBranch( + this.projectPath, + branchName + ); + if (resolved) { + workDir = resolved; + } } } try { - const { stdout: status } = await execAsync('git status --porcelain', { cwd: workDir }); + const status = await execGitCommand(['status', '--porcelain'], workDir); if (!status.trim()) { return null; } @@ -712,9 +723,9 @@ Address the follow-up instructions above. Review the previous work and make the feature?.description?.split('\n')[0]?.substring(0, 60) || `Feature ${featureId}`; const commitMessage = `feat: ${title}\n\nImplemented by Automaker auto-mode`; - await execAsync('git add -A', { cwd: workDir }); - await execAsync(`git commit -m "${commitMessage.replace(/"/g, '\\"')}"`, { cwd: workDir }); - const { stdout: hash } = await execAsync('git rev-parse HEAD', { cwd: workDir }); + await execGitCommand(['add', '-A'], workDir); + await execGitCommand(['commit', '-m', commitMessage], workDir); + const hash = await execGitCommand(['rev-parse', 'HEAD'], workDir); this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { featureId, @@ -975,10 +986,10 @@ Address the follow-up instructions above. Review the previous work and make the return orphanedFeatures; } - // Get existing branches - const { stdout } = await execAsync( - 'git for-each-ref --format="%(refname:short)" refs/heads/', - { cwd: this.projectPath } + // Get existing branches (using safe array-based command) + const stdout = await execGitCommand( + ['for-each-ref', '--format=%(refname:short)', 'refs/heads/'], + this.projectPath ); const existingBranches = new Set( stdout diff --git a/apps/server/src/services/execution-service.ts b/apps/server/src/services/execution-service.ts index 54e8edd6..2af35fe7 100644 --- a/apps/server/src/services/execution-service.ts +++ b/apps/server/src/services/execution-service.ts @@ -190,9 +190,9 @@ ${feature.spec} } } - let worktreePath: string | null = null; + let worktreePath: string | null = providedWorktreePath ?? null; const branchName = feature.branchName; - if (useWorktrees && branchName) { + if (!worktreePath && useWorktrees && branchName) { worktreePath = await this.worktreeResolver.findWorktreeForBranch(projectPath, branchName); if (worktreePath) logger.info(`Using worktree for branch "${branchName}": ${worktreePath}`); } @@ -289,6 +289,11 @@ ${feature.spec} testAttempts: 0, maxTestAttempts: 5, }); + // Check if pipeline set a terminal status (e.g., merge_conflict) — don't overwrite it + const refreshed = await this.loadFeatureFn(projectPath, featureId); + if (refreshed?.status === 'merge_conflict') { + return; + } } const finalStatus = feature.skipTests ? 'waiting_approval' : 'verified'; diff --git a/apps/server/src/services/feature-state-manager.ts b/apps/server/src/services/feature-state-manager.ts index e7f37962..c74ad88b 100644 --- a/apps/server/src/services/feature-state-manager.ts +++ b/apps/server/src/services/feature-state-manager.ts @@ -115,6 +115,13 @@ export class FeatureStateManager { // PERSIST BEFORE EMIT (Pitfall 2) await atomicWriteJson(featurePath, feature, { backupCount: DEFAULT_BACKUP_COUNT }); + // Emit status change event so UI can react without polling + this.emitAutoModeEvent('feature_status_changed', { + featureId, + projectPath, + status, + }); + // Create notifications for important status changes const notificationService = getNotificationService(); if (status === 'waiting_approval') { diff --git a/apps/server/src/services/merge-service.ts b/apps/server/src/services/merge-service.ts index 837fbfbb..087aa801 100644 --- a/apps/server/src/services/merge-service.ts +++ b/apps/server/src/services/merge-service.ts @@ -4,12 +4,8 @@ * Extracted from worktree merge route to allow internal service calls. */ -import { exec } from 'child_process'; -import { promisify } from 'util'; import { createLogger } from '@automaker/utils'; import { spawnProcess } from '@automaker/platform'; - -const execAsync = promisify(exec); const logger = createLogger('MergeService'); export interface MergeOptions { @@ -80,9 +76,23 @@ export async function performMerge( const mergeTo = targetBranch || 'main'; - // Validate source branch exists + // Validate branch names early to reject invalid input before any git operations + if (!isValidBranchName(branchName)) { + return { + success: false, + error: `Invalid source branch name: "${branchName}"`, + }; + } + if (!isValidBranchName(mergeTo)) { + return { + success: false, + error: `Invalid target branch name: "${mergeTo}"`, + }; + } + + // Validate source branch exists (using safe array-based command) try { - await execAsync(`git rev-parse --verify ${branchName}`, { cwd: projectPath }); + await execGitCommand(['rev-parse', '--verify', branchName], projectPath); } catch { return { success: false, @@ -90,9 +100,9 @@ export async function performMerge( }; } - // Validate target branch exists + // Validate target branch exists (using safe array-based command) try { - await execAsync(`git rev-parse --verify ${mergeTo}`, { cwd: projectPath }); + await execGitCommand(['rev-parse', '--verify', mergeTo], projectPath); } catch { return { success: false, @@ -100,13 +110,14 @@ export async function performMerge( }; } - // Merge the feature branch into the target branch - const mergeCmd = options?.squash - ? `git merge --squash ${branchName}` - : `git merge ${branchName} -m "${options?.message || `Merge ${branchName} into ${mergeTo}`}"`; + // Merge the feature branch into the target branch (using safe array-based commands) + const mergeMessage = options?.message || `Merge ${branchName} into ${mergeTo}`; + const mergeArgs = options?.squash + ? ['merge', '--squash', branchName] + : ['merge', branchName, '-m', mergeMessage]; try { - await execAsync(mergeCmd, { cwd: projectPath }); + await execGitCommand(mergeArgs, projectPath); } catch (mergeError: unknown) { // Check if this is a merge conflict const err = mergeError as { stdout?: string; stderr?: string; message?: string }; @@ -125,11 +136,10 @@ export async function performMerge( throw mergeError; } - // If squash merge, need to commit + // If squash merge, need to commit (using safe array-based command) if (options?.squash) { - await execAsync(`git commit -m "${options?.message || `Merge ${branchName} (squash)`}"`, { - cwd: projectPath, - }); + const squashMessage = options?.message || `Merge ${branchName} (squash)`; + await execGitCommand(['commit', '-m', squashMessage], projectPath); } // Optionally delete the worktree and branch after merging diff --git a/apps/server/src/services/pipeline-orchestrator.ts b/apps/server/src/services/pipeline-orchestrator.ts index 08be4092..ea2bf69e 100644 --- a/apps/server/src/services/pipeline-orchestrator.ts +++ b/apps/server/src/services/pipeline-orchestrator.ts @@ -460,6 +460,7 @@ export class PipelineOrchestrator { const session = this.testRunnerService.getSession(sessionId); if (session && session.status !== 'running' && session.status !== 'pending') { clearInterval(checkInterval); + clearTimeout(timeoutId); resolve({ status: session.status, exitCode: session.exitCode, @@ -469,7 +470,7 @@ export class PipelineOrchestrator { }); } }, 1000); - setTimeout(() => { + const timeoutId = setTimeout(() => { clearInterval(checkInterval); resolve({ status: 'failed', exitCode: null, duration: 600000 }); }, 600000); diff --git a/apps/server/src/services/plan-approval-service.ts b/apps/server/src/services/plan-approval-service.ts index 836d999f..3a677d49 100644 --- a/apps/server/src/services/plan-approval-service.ts +++ b/apps/server/src/services/plan-approval-service.ts @@ -83,6 +83,13 @@ export class PlanApprovalService { ); return new Promise((resolve, reject) => { + // Prevent duplicate registrations for the same key — reject and clean up existing entry + const existing = this.pendingApprovals.get(key); + if (existing) { + existing.reject(new Error('Superseded by a new waitForApproval call')); + this.pendingApprovals.delete(key); + } + // Set up timeout to prevent indefinite waiting and memory leaks // timeoutId stored in closure, NOT in PendingApproval object const timeoutId = setTimeout(() => { @@ -226,11 +233,11 @@ export class PlanApprovalService { status: approved ? 'approved' : 'rejected', approvedAt: approved ? new Date().toISOString() : undefined, reviewedByUser: true, - content: editedPlan, // Update content if user provided an edited version + ...(editedPlan !== undefined && { content: editedPlan }), // Only update content if user provided an edited version }); - // If rejected with feedback, emit event so client knows the rejection reason - if (!approved && feedback) { + // If rejected, emit event so client knows the rejection reason (even without feedback) + if (!approved) { this.eventBus.emitAutoModeEvent('plan_rejected', { featureId, projectPath, diff --git a/apps/ui/src/components/views/graph-view-page.tsx b/apps/ui/src/components/views/graph-view-page.tsx index 306b8eaa..3167647f 100644 --- a/apps/ui/src/components/views/graph-view-page.tsx +++ b/apps/ui/src/components/views/graph-view-page.tsx @@ -313,14 +313,18 @@ export function GraphViewPage() { // Handle add and start feature const handleAddAndStartFeature = useCallback( async (featureData: Parameters[0]) => { - const featuresBeforeIds = new Set(useAppStore.getState().features.map((f) => f.id)); - await handleAddFeature(featureData); + try { + const featuresBeforeIds = new Set(useAppStore.getState().features.map((f) => f.id)); + await handleAddFeature(featureData); - const latestFeatures = useAppStore.getState().features; - const newFeature = latestFeatures.find((f) => !featuresBeforeIds.has(f.id)); + const latestFeatures = useAppStore.getState().features; + const newFeature = latestFeatures.find((f) => !featuresBeforeIds.has(f.id)); - if (newFeature) { - await handleStartImplementation(newFeature); + if (newFeature) { + await handleStartImplementation(newFeature); + } + } catch (error) { + logger.error('Failed to add and start feature:', error); } }, [handleAddFeature, handleStartImplementation]