From 5a5c56a4cfff9d7d1a84e728d0a8ea072365c45b Mon Sep 17 00:00:00 2001 From: Shirone Date: Sat, 31 Jan 2026 13:59:24 +0100 Subject: [PATCH] fix: address PR review issues for auto-mode refactor - agent-executor: move executeQuery into try block for proper heartbeat cleanup, re-parse tasks when edited plan is approved - auto-loop-coordinator: handle feature execution failures with proper logging and failure tracking, support backward-compatible method signatures - facade: delegate getActiveAutoLoopProjects/Worktrees to coordinator, always create own AutoLoopCoordinator (not shared), pass projectPath to approval methods and branchName to failure tracking - global-service: document shared autoLoopCoordinator is for monitoring only - execution-types: fix ExecuteFeatureFn type to match implementation - feature-state-manager: use readJsonWithRecovery for loadFeature - pipeline-orchestrator: add defensive null check and try/catch for merge response parsing - plan-approval-service: use project-scoped keys to prevent cross-project collisions, maintain backward compatibility for featureId-only lookups Co-Authored-By: Claude Opus 4.5 --- apps/server/src/services/agent-executor.ts | 11 ++- .../src/services/auto-loop-coordinator.ts | 78 ++++++++++++++---- apps/server/src/services/auto-mode/facade.ts | 82 +++++++++---------- .../src/services/auto-mode/global-service.ts | 12 ++- apps/server/src/services/execution-types.ts | 6 +- .../src/services/feature-state-manager.ts | 8 +- .../src/services/pipeline-orchestrator.ts | 22 +++-- .../src/services/plan-approval-service.ts | 74 ++++++++++++++--- .../services/feature-state-manager.test.ts | 12 +-- 9 files changed, 214 insertions(+), 91 deletions(-) diff --git a/apps/server/src/services/agent-executor.ts b/apps/server/src/services/agent-executor.ts index 49f75fa1..148c339c 100644 --- a/apps/server/src/services/agent-executor.ts +++ b/apps/server/src/services/agent-executor.ts @@ -180,9 +180,9 @@ export class AgentExecutor { } logger.info(`Starting stream for feature ${featureId}...`); - const stream = provider.executeQuery(executeOptions); try { + const stream = provider.executeQuery(executeOptions); streamLoop: for await (const msg of stream) { receivedAnyStreamMessage = true; appendRawEvent(msg); @@ -502,10 +502,17 @@ export class AgentExecutor { planApproved = true; userFeedback = approvalResult.feedback; approvedPlanContent = approvalResult.editedPlan || currentPlanContent; - if (approvalResult.editedPlan) + if (approvalResult.editedPlan) { + // Re-parse tasks from edited plan to ensure we execute the updated tasks + const editedTasks = parseTasksFromSpec(approvalResult.editedPlan); + parsedTasks = editedTasks; await this.featureStateManager.updateFeaturePlanSpec(projectPath, featureId, { content: approvalResult.editedPlan, + tasks: editedTasks, + tasksTotal: editedTasks.length, + tasksCompleted: 0, }); + } this.eventBus.emitAutoModeEvent('plan_approved', { featureId, projectPath, diff --git a/apps/server/src/services/auto-loop-coordinator.ts b/apps/server/src/services/auto-loop-coordinator.ts index b1a25f51..0b03e5f8 100644 --- a/apps/server/src/services/auto-loop-coordinator.ts +++ b/apps/server/src/services/auto-loop-coordinator.ts @@ -183,7 +183,13 @@ export class AutoLoopCoordinator { nextFeature.id, projectState.config.useWorktrees, true - ).catch(() => {}); + ).catch((error) => { + const errorInfo = classifyError(error); + logger.error(`Auto-loop feature ${nextFeature.id} failed:`, errorInfo.message); + if (this.trackFailureAndCheckPauseForProject(projectPath, branchName, errorInfo)) { + this.signalShouldPauseForProject(projectPath, branchName, errorInfo); + } + }); } await this.sleep(2000, projectState.abortController.signal); } catch { @@ -268,27 +274,64 @@ export class AutoLoopCoordinator { trackFailureAndCheckPauseForProject( projectPath: string, - errorInfo: { type: string; message: string } + branchNameOrError: string | null | { type: string; message: string }, + errorInfo?: { type: string; message: string } ): boolean { - const projectState = this.autoLoopsByProject.get(getWorktreeAutoLoopKey(projectPath, null)); + // Support both old (projectPath, errorInfo) and new (projectPath, branchName, errorInfo) signatures + let branchName: string | null; + let actualErrorInfo: { type: string; message: string }; + if ( + typeof branchNameOrError === 'object' && + branchNameOrError !== null && + 'type' in branchNameOrError + ) { + // Old signature: (projectPath, errorInfo) + branchName = null; + actualErrorInfo = branchNameOrError; + } else { + // New signature: (projectPath, branchName, errorInfo) + branchName = branchNameOrError; + actualErrorInfo = errorInfo!; + } + const projectState = this.autoLoopsByProject.get( + getWorktreeAutoLoopKey(projectPath, branchName) + ); if (!projectState) return false; const now = Date.now(); - projectState.consecutiveFailures.push({ timestamp: now, error: errorInfo.message }); + projectState.consecutiveFailures.push({ timestamp: now, error: actualErrorInfo.message }); projectState.consecutiveFailures = projectState.consecutiveFailures.filter( (f) => now - f.timestamp < FAILURE_WINDOW_MS ); return ( projectState.consecutiveFailures.length >= CONSECUTIVE_FAILURE_THRESHOLD || - errorInfo.type === 'quota_exhausted' || - errorInfo.type === 'rate_limit' + actualErrorInfo.type === 'quota_exhausted' || + actualErrorInfo.type === 'rate_limit' ); } signalShouldPauseForProject( projectPath: string, - errorInfo: { type: string; message: string } + branchNameOrError: string | null | { type: string; message: string }, + errorInfo?: { type: string; message: string } ): void { - const projectState = this.autoLoopsByProject.get(getWorktreeAutoLoopKey(projectPath, null)); + // Support both old (projectPath, errorInfo) and new (projectPath, branchName, errorInfo) signatures + let branchName: string | null; + let actualErrorInfo: { type: string; message: string }; + if ( + typeof branchNameOrError === 'object' && + branchNameOrError !== null && + 'type' in branchNameOrError + ) { + branchName = null; + actualErrorInfo = branchNameOrError; + } else { + branchName = branchNameOrError; + actualErrorInfo = errorInfo!; + } + + const projectState = this.autoLoopsByProject.get( + getWorktreeAutoLoopKey(projectPath, branchName) + ); if (!projectState || projectState.pausedDueToFailures) return; projectState.pausedDueToFailures = true; const failureCount = projectState.consecutiveFailures.length; @@ -297,24 +340,29 @@ export class AutoLoopCoordinator { failureCount >= CONSECUTIVE_FAILURE_THRESHOLD ? `Auto Mode paused: ${failureCount} consecutive failures detected.` : 'Auto Mode paused: Usage limit or API error detected.', - errorType: errorInfo.type, - originalError: errorInfo.message, + errorType: actualErrorInfo.type, + originalError: actualErrorInfo.message, failureCount, projectPath, + branchName, }); - this.stopAutoLoopForProject(projectPath); + this.stopAutoLoopForProject(projectPath, branchName); } - resetFailureTrackingForProject(projectPath: string): void { - const projectState = this.autoLoopsByProject.get(getWorktreeAutoLoopKey(projectPath, null)); + resetFailureTrackingForProject(projectPath: string, branchName: string | null = null): void { + const projectState = this.autoLoopsByProject.get( + getWorktreeAutoLoopKey(projectPath, branchName) + ); if (projectState) { projectState.consecutiveFailures = []; projectState.pausedDueToFailures = false; } } - recordSuccessForProject(projectPath: string): void { - const projectState = this.autoLoopsByProject.get(getWorktreeAutoLoopKey(projectPath, null)); + recordSuccessForProject(projectPath: string, branchName: string | null = null): void { + const projectState = this.autoLoopsByProject.get( + getWorktreeAutoLoopKey(projectPath, branchName) + ); if (projectState) projectState.consecutiveFailures = []; } diff --git a/apps/server/src/services/auto-mode/facade.ts b/apps/server/src/services/auto-mode/facade.ts index ab7a60a3..d1faa33f 100644 --- a/apps/server/src/services/auto-mode/facade.ts +++ b/apps/server/src/services/auto-mode/facade.ts @@ -210,39 +210,39 @@ export class AutoModeServiceFacade { } ); - // AutoLoopCoordinator - use shared if provided, otherwise create new - // Note: When using shared autoLoopCoordinator, callbacks are already set up by the global service - const autoLoopCoordinator = - sharedServices?.autoLoopCoordinator ?? - new AutoLoopCoordinator( - eventBus, - concurrencyManager, - settingsService, - // Callbacks - (pPath, featureId, useWorktrees, isAutoMode) => - facadeInstance!.executeFeature(featureId, useWorktrees, isAutoMode), - (pPath, branchName) => - featureLoader - .getAll(pPath) - .then((features) => - features.filter( - (f) => - (f.status === 'backlog' || f.status === 'ready') && - (branchName === null - ? !f.branchName || f.branchName === 'main' - : f.branchName === branchName) - ) - ), - (pPath, branchName, maxConcurrency) => - facadeInstance!.saveExecutionStateForProject(branchName, maxConcurrency), - (pPath, branchName) => facadeInstance!.clearExecutionState(branchName), - (pPath) => featureStateManager.resetStuckFeatures(pPath), - (feature) => - feature.status === 'completed' || - feature.status === 'verified' || - feature.status === 'waiting_approval', - (featureId) => concurrencyManager.isRunning(featureId) - ); + // AutoLoopCoordinator - ALWAYS create new with proper execution callbacks + // NOTE: We don't use sharedServices.autoLoopCoordinator because it doesn't have + // execution callbacks. Each facade needs its own coordinator to execute features. + // The shared coordinator in GlobalAutoModeService is for monitoring only. + const autoLoopCoordinator = new AutoLoopCoordinator( + eventBus, + concurrencyManager, + settingsService, + // Callbacks + (pPath, featureId, useWorktrees, isAutoMode) => + facadeInstance!.executeFeature(featureId, useWorktrees, isAutoMode), + (pPath, branchName) => + featureLoader + .getAll(pPath) + .then((features) => + features.filter( + (f) => + (f.status === 'backlog' || f.status === 'ready') && + (branchName === null + ? !f.branchName || f.branchName === 'main' + : f.branchName === branchName) + ) + ), + (pPath, branchName, maxConcurrency) => + facadeInstance!.saveExecutionStateForProject(branchName, maxConcurrency), + (pPath, branchName) => facadeInstance!.clearExecutionState(branchName), + (pPath) => featureStateManager.resetStuckFeatures(pPath), + (feature) => + feature.status === 'completed' || + feature.status === 'verified' || + feature.status === 'waiting_approval', + (featureId) => concurrencyManager.isRunning(featureId) + ); // ExecutionService - runAgentFn calls AgentExecutor.execute const executionService = new ExecutionService( @@ -328,8 +328,8 @@ export class AutoModeServiceFacade { (pPath, featureId, useWorktrees, _calledInternally) => facadeInstance!.resumeFeature(featureId, useWorktrees, _calledInternally), (errorInfo) => - autoLoopCoordinator.trackFailureAndCheckPauseForProject(projectPath, errorInfo), - (errorInfo) => autoLoopCoordinator.signalShouldPauseForProject(projectPath, errorInfo), + autoLoopCoordinator.trackFailureAndCheckPauseForProject(projectPath, null, errorInfo), + (errorInfo) => autoLoopCoordinator.signalShouldPauseForProject(projectPath, null, errorInfo), () => { /* recordSuccess - no-op */ }, @@ -746,18 +746,14 @@ Address the follow-up instructions above. Review the previous work and make the * Get all active auto loop projects (unique project paths) */ getActiveAutoLoopProjects(): string[] { - // This needs access to internal state - for now return empty - // Routes should migrate to getActiveAutoLoopWorktrees - return []; + return this.autoLoopCoordinator.getActiveProjects(); } /** * Get all active auto loop worktrees */ getActiveAutoLoopWorktrees(): Array<{ projectPath: string; branchName: string | null }> { - // This needs access to internal state - for now return empty - // Will be properly implemented when routes migrate - return []; + return this.autoLoopCoordinator.getActiveWorktrees(); } /** @@ -891,7 +887,7 @@ Address the follow-up instructions above. Review the previous work and make the * @param featureId - The feature ID */ hasPendingApproval(featureId: string): boolean { - return this.planApprovalService.hasPendingApproval(featureId); + return this.planApprovalService.hasPendingApproval(featureId, this.projectPath); } /** @@ -899,7 +895,7 @@ Address the follow-up instructions above. Review the previous work and make the * @param featureId - The feature ID */ cancelPlanApproval(featureId: string): void { - this.planApprovalService.cancelApproval(featureId); + this.planApprovalService.cancelApproval(featureId, this.projectPath); } // =========================================================================== diff --git a/apps/server/src/services/auto-mode/global-service.ts b/apps/server/src/services/auto-mode/global-service.ts index a371272f..ca787440 100644 --- a/apps/server/src/services/auto-mode/global-service.ts +++ b/apps/server/src/services/auto-mode/global-service.ts @@ -51,15 +51,19 @@ export class GlobalAutoModeService { this.featureStateManager = new FeatureStateManager(events, featureLoader); // Create AutoLoopCoordinator with callbacks - // These callbacks use placeholders since GlobalAutoModeService doesn't execute features - // Feature execution is done via facades + // IMPORTANT: This coordinator is for MONITORING ONLY (getActiveProjects, getActiveWorktrees). + // Facades MUST create their own AutoLoopCoordinator for actual execution. + // The executeFeatureFn here is a safety guard - it should never be called. this.autoLoopCoordinator = new AutoLoopCoordinator( this.eventBus, this.concurrencyManager, settingsService, - // executeFeatureFn - not used by global service, routes handle execution + // executeFeatureFn - throws because facades must use their own coordinator for execution async () => { - throw new Error('executeFeatureFn not available in GlobalAutoModeService'); + throw new Error( + 'executeFeatureFn not available in GlobalAutoModeService. ' + + 'Facades must create their own AutoLoopCoordinator for execution.' + ); }, // getBacklogFeaturesFn (pPath, branchName) => diff --git a/apps/server/src/services/execution-types.ts b/apps/server/src/services/execution-types.ts index 553d1fb7..6cb9cb5f 100644 --- a/apps/server/src/services/execution-types.ts +++ b/apps/server/src/services/execution-types.ts @@ -138,9 +138,9 @@ export type ExecuteFeatureFn = ( projectPath: string, featureId: string, useWorktrees: boolean, - useScreenshots: boolean, - model?: string, - options?: { _calledInternally?: boolean } + isAutoMode: boolean, + providedWorktreePath?: string, + options?: { continuationPrompt?: string; _calledInternally?: boolean } ) => Promise; /** diff --git a/apps/server/src/services/feature-state-manager.ts b/apps/server/src/services/feature-state-manager.ts index 05ba4987..e7f37962 100644 --- a/apps/server/src/services/feature-state-manager.ts +++ b/apps/server/src/services/feature-state-manager.ts @@ -60,8 +60,12 @@ export class FeatureStateManager { const featurePath = path.join(featureDir, 'feature.json'); try { - const data = (await secureFs.readFile(featurePath, 'utf-8')) as string; - return JSON.parse(data); + const result = await readJsonWithRecovery(featurePath, null, { + maxBackups: DEFAULT_BACKUP_COUNT, + autoRestore: true, + }); + logRecoveryWarning(result, `Feature ${featureId}`, logger); + return result.data; } catch { return null; } diff --git a/apps/server/src/services/pipeline-orchestrator.ts b/apps/server/src/services/pipeline-orchestrator.ts index af35ef71..92fbe2dd 100644 --- a/apps/server/src/services/pipeline-orchestrator.ts +++ b/apps/server/src/services/pipeline-orchestrator.ts @@ -495,11 +495,23 @@ export class PipelineOrchestrator { }), }); - const data = (await response.json()) as { - success: boolean; - hasConflicts?: boolean; - error?: string; - }; + if (!response) { + return { success: false, error: 'No response from merge endpoint' }; + } + + // Defensively parse JSON response + let data: { success: boolean; hasConflicts?: boolean; error?: string }; + try { + data = (await response.json()) as { + success: boolean; + hasConflicts?: boolean; + error?: string; + }; + } catch (parseError) { + logger.error(`Failed to parse merge response:`, parseError); + return { success: false, error: 'Invalid response from merge endpoint' }; + } + if (!response.ok) { if (data.hasConflicts) { await this.updateFeatureStatusFn(projectPath, featureId, 'merge_conflict'); diff --git a/apps/server/src/services/plan-approval-service.ts b/apps/server/src/services/plan-approval-service.ts index 06c47284..836d999f 100644 --- a/apps/server/src/services/plan-approval-service.ts +++ b/apps/server/src/services/plan-approval-service.ts @@ -66,12 +66,18 @@ export class PlanApprovalService { this.settingsService = settingsService; } + /** Generate project-scoped key to prevent collisions across projects */ + private approvalKey(projectPath: string, featureId: string): string { + return `${projectPath}::${featureId}`; + } + /** Wait for plan approval with timeout (default 30 min). Rejects on timeout/cancellation. */ async waitForApproval(featureId: string, projectPath: string): Promise { const timeoutMs = await this.getTimeoutMs(projectPath); const timeoutMinutes = Math.round(timeoutMs / 60000); + const key = this.approvalKey(projectPath, featureId); - logger.info(`Registering pending approval for feature ${featureId}`); + logger.info(`Registering pending approval for feature ${featureId} in project ${projectPath}`); logger.info( `Current pending approvals: ${Array.from(this.pendingApprovals.keys()).join(', ') || 'none'}` ); @@ -80,12 +86,12 @@ export class PlanApprovalService { // Set up timeout to prevent indefinite waiting and memory leaks // timeoutId stored in closure, NOT in PendingApproval object const timeoutId = setTimeout(() => { - const pending = this.pendingApprovals.get(featureId); + const pending = this.pendingApprovals.get(key); if (pending) { logger.warn( `Plan approval for feature ${featureId} timed out after ${timeoutMinutes} minutes` ); - this.pendingApprovals.delete(featureId); + this.pendingApprovals.delete(key); reject( new Error( `Plan approval timed out after ${timeoutMinutes} minutes - feature execution cancelled` @@ -106,7 +112,7 @@ export class PlanApprovalService { reject(error); }; - this.pendingApprovals.set(featureId, { + this.pendingApprovals.set(key, { resolve: wrappedResolve, reject: wrappedReject, featureId, @@ -132,7 +138,23 @@ export class PlanApprovalService { `Current pending approvals: ${Array.from(this.pendingApprovals.keys()).join(', ') || 'none'}` ); - const pending = this.pendingApprovals.get(featureId); + // Try to find pending approval using project-scoped key if projectPath is available + let foundKey: string | undefined; + let pending: PendingApproval | undefined; + + if (projectPathFromClient) { + foundKey = this.approvalKey(projectPathFromClient, featureId); + pending = this.pendingApprovals.get(foundKey); + } else { + // Fallback: search by featureId (backward compatibility) + for (const [key, approval] of this.pendingApprovals) { + if (approval.featureId === featureId) { + foundKey = key; + pending = approval; + break; + } + } + } if (!pending) { logger.info(`No pending approval in Map for feature ${featureId}`); @@ -219,32 +241,60 @@ export class PlanApprovalService { // Resolve the promise with all data including feedback // This triggers the wrapped resolve which clears the timeout pending.resolve({ approved, editedPlan, feedback }); - this.pendingApprovals.delete(featureId); + if (foundKey) { + this.pendingApprovals.delete(foundKey); + } return { success: true }; } /** Cancel approval (e.g., when feature stopped). Timeout cleared via wrapped reject. */ - cancelApproval(featureId: string): void { + cancelApproval(featureId: string, projectPath?: string): void { logger.info(`cancelApproval called for feature ${featureId}`); logger.info( `Current pending approvals: ${Array.from(this.pendingApprovals.keys()).join(', ') || 'none'}` ); - const pending = this.pendingApprovals.get(featureId); - if (pending) { + // If projectPath provided, use project-scoped key; otherwise search by featureId + let foundKey: string | undefined; + let pending: PendingApproval | undefined; + + if (projectPath) { + foundKey = this.approvalKey(projectPath, featureId); + pending = this.pendingApprovals.get(foundKey); + } else { + // Fallback: search for any approval with this featureId (backward compatibility) + for (const [key, approval] of this.pendingApprovals) { + if (approval.featureId === featureId) { + foundKey = key; + pending = approval; + break; + } + } + } + + if (pending && foundKey) { logger.info(`Found and cancelling pending approval for feature ${featureId}`); // Wrapped reject clears timeout automatically pending.reject(new Error('Plan approval cancelled - feature was stopped')); - this.pendingApprovals.delete(featureId); + this.pendingApprovals.delete(foundKey); } else { logger.info(`No pending approval to cancel for feature ${featureId}`); } } /** Check if a feature has a pending plan approval. */ - hasPendingApproval(featureId: string): boolean { - return this.pendingApprovals.has(featureId); + hasPendingApproval(featureId: string, projectPath?: string): boolean { + if (projectPath) { + return this.pendingApprovals.has(this.approvalKey(projectPath, featureId)); + } + // Fallback: search by featureId (backward compatibility) + for (const approval of this.pendingApprovals.values()) { + if (approval.featureId === featureId) { + return true; + } + } + return false; } /** Get timeout from project settings or default (30 min). */ diff --git a/apps/server/tests/unit/services/feature-state-manager.test.ts b/apps/server/tests/unit/services/feature-state-manager.test.ts index 71cce08d..c8328a54 100644 --- a/apps/server/tests/unit/services/feature-state-manager.test.ts +++ b/apps/server/tests/unit/services/feature-state-manager.test.ts @@ -71,20 +71,21 @@ describe('FeatureStateManager', () => { describe('loadFeature', () => { it('should load feature from disk', async () => { - (secureFs.readFile as Mock).mockResolvedValue(JSON.stringify(mockFeature)); + (readJsonWithRecovery as Mock).mockResolvedValue({ data: mockFeature, recovered: false }); const feature = await manager.loadFeature('/project', 'feature-123'); expect(feature).toEqual(mockFeature); expect(getFeatureDir).toHaveBeenCalledWith('/project', 'feature-123'); - expect(secureFs.readFile).toHaveBeenCalledWith( + expect(readJsonWithRecovery).toHaveBeenCalledWith( '/project/.automaker/features/feature-123/feature.json', - 'utf-8' + null, + expect.objectContaining({ autoRestore: true }) ); }); it('should return null if feature does not exist', async () => { - (secureFs.readFile as Mock).mockRejectedValue(new Error('ENOENT')); + (readJsonWithRecovery as Mock).mockRejectedValue(new Error('ENOENT')); const feature = await manager.loadFeature('/project', 'non-existent'); @@ -92,7 +93,8 @@ describe('FeatureStateManager', () => { }); it('should return null if feature JSON is invalid', async () => { - (secureFs.readFile as Mock).mockResolvedValue('not valid json'); + // readJsonWithRecovery returns null as the default value when JSON is invalid + (readJsonWithRecovery as Mock).mockResolvedValue({ data: null, recovered: false }); const feature = await manager.loadFeature('/project', 'feature-123');