From ab5d6a0e54636433b8e673aaa4c921cdc46bb524 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 16 Feb 2026 13:14:55 -0800 Subject: [PATCH] feat: Improve callback safety and remove unnecessary formatting in auto-mode facade --- apps/server/src/services/agent-executor.ts | 6 +- apps/server/src/services/auto-mode/facade.ts | 101 ++++++------------ .../src/services/feature-state-manager.ts | 46 ++++---- .../src/services/pipeline-orchestrator.ts | 23 ++-- apps/ui/eslint.config.mjs | 1 + apps/ui/src/store/app-store.ts | 17 +-- apps/ui/src/store/types/state-types.ts | 4 +- 7 files changed, 80 insertions(+), 118 deletions(-) diff --git a/apps/server/src/services/agent-executor.ts b/apps/server/src/services/agent-executor.ts index 9a8772ef..0d9c2399 100644 --- a/apps/server/src/services/agent-executor.ts +++ b/apps/server/src/services/agent-executor.ts @@ -126,9 +126,7 @@ export class AgentExecutor { const appendRawEvent = (event: unknown): void => { if (!enableRawOutput) return; try { - rawOutputLines.push( - JSON.stringify({ timestamp: new Date().toISOString(), event }, null, 4) - ); + rawOutputLines.push(JSON.stringify({ timestamp: new Date().toISOString(), event })); if (rawWriteTimeout) clearTimeout(rawWriteTimeout); rawWriteTimeout = setTimeout(async () => { try { @@ -552,7 +550,7 @@ export class AgentExecutor { }); let revText = ''; for await (const msg of provider.executeQuery( - this.buildExecOpts(options, revPrompt, sdkOptions?.maxTurns || 100) + this.buildExecOpts(options, revPrompt, sdkOptions?.maxTurns ?? 100) )) { if (msg.type === 'assistant' && msg.message?.content) for (const b of msg.message.content) diff --git a/apps/server/src/services/auto-mode/facade.ts b/apps/server/src/services/auto-mode/facade.ts index 01985081..5d8bde58 100644 --- a/apps/server/src/services/auto-mode/facade.ts +++ b/apps/server/src/services/auto-mode/facade.ts @@ -143,9 +143,20 @@ export class AutoModeServiceFacade { return prompt; }; - // Create placeholder callbacks - will be bound to facade methods after creation - // These use closures to capture the facade instance once created + // Create placeholder callbacks - will be bound to facade methods after creation. + // These use closures to capture the facade instance once created. + // INVARIANT: All callbacks passed to PipelineOrchestrator, AutoLoopCoordinator, + // and ExecutionService are invoked asynchronously (never during construction), + // so facadeInstance is guaranteed to be assigned before any callback runs. let facadeInstance: AutoModeServiceFacade | null = null; + const getFacade = (): AutoModeServiceFacade => { + if (!facadeInstance) { + throw new Error( + 'AutoModeServiceFacade not yet initialized — callback invoked during construction' + ); + } + return facadeInstance; + }; // PipelineOrchestrator - runAgentFn is a stub; routes use AutoModeService directly const pipelineOrchestrator = new PipelineOrchestrator( @@ -162,7 +173,7 @@ export class AutoModeServiceFacade { loadContextFiles, buildFeaturePrompt, (pPath, featureId, useWorktrees, _isAutoMode, _model, opts) => - facadeInstance!.executeFeature(featureId, useWorktrees, false, undefined, opts), + getFacade().executeFeature(featureId, useWorktrees, false, undefined, opts), // runAgentFn - delegates to AgentExecutor async ( workDir: string, @@ -227,7 +238,7 @@ export class AutoModeServiceFacade { .replace(/\{\{taskName\}\}/g, task.description) .replace(/\{\{taskIndex\}\}/g, String(taskIndex + 1)) .replace(/\{\{totalTasks\}\}/g, String(allTasks.length)) - .replace(/\{\{taskDescription\}\}/g, task.description || task.name); + .replace(/\{\{taskDescription\}\}/g, task.description || `Task ${task.id}`); if (feedback) { taskPrompt = taskPrompt.replace(/\{\{userFeedback\}\}/g, feedback); } @@ -248,7 +259,7 @@ export class AutoModeServiceFacade { settingsService, // Callbacks (pPath, featureId, useWorktrees, isAutoMode) => - facadeInstance!.executeFeature(featureId, useWorktrees, isAutoMode), + getFacade().executeFeature(featureId, useWorktrees, isAutoMode), async (pPath, branchName) => { const features = await featureLoader.getAll(pPath); // For main worktree (branchName === null), resolve the actual primary branch name @@ -266,8 +277,8 @@ export class AutoModeServiceFacade { ); }, (pPath, branchName, maxConcurrency) => - facadeInstance!.saveExecutionStateForProject(branchName, maxConcurrency), - (pPath, branchName) => facadeInstance!.clearExecutionState(branchName), + getFacade().saveExecutionStateForProject(branchName, maxConcurrency), + (pPath, branchName) => getFacade().clearExecutionState(branchName), (pPath) => featureStateManager.resetStuckFeatures(pPath), (feature) => feature.status === 'completed' || @@ -375,16 +386,16 @@ export class AutoModeServiceFacade { async () => { /* recordLearnings - stub */ }, - (pPath, featureId) => facadeInstance!.contextExists(featureId), + (pPath, featureId) => getFacade().contextExists(featureId), (pPath, featureId, useWorktrees, _calledInternally) => - facadeInstance!.resumeFeature(featureId, useWorktrees, _calledInternally), + getFacade().resumeFeature(featureId, useWorktrees, _calledInternally), (errorInfo) => autoLoopCoordinator.trackFailureAndCheckPauseForProject(projectPath, null, errorInfo), (errorInfo) => autoLoopCoordinator.signalShouldPauseForProject(projectPath, null, errorInfo), () => { /* recordSuccess - no-op */ }, - (_pPath) => facadeInstance!.saveExecutionState(), + (_pPath) => getFacade().saveExecutionState(), loadContextFiles ); @@ -395,13 +406,7 @@ export class AutoModeServiceFacade { settingsService, // Callbacks (pPath, featureId, useWorktrees, isAutoMode, providedWorktreePath, opts) => - facadeInstance!.executeFeature( - featureId, - useWorktrees, - isAutoMode, - providedWorktreePath, - opts - ), + getFacade().executeFeature(featureId, useWorktrees, isAutoMode, providedWorktreePath, opts), (pPath, featureId) => featureStateManager.loadFeature(pPath, featureId), (pPath, featureId, status) => pipelineOrchestrator.detectPipelineStatus(pPath, featureId, status), @@ -547,7 +552,9 @@ export class AutoModeServiceFacade { imagePaths?: string[], useWorktrees = true ): Promise { - // This method contains substantial logic - delegates most work to AgentExecutor + // Stub: acquire concurrency slot then immediately throw. + // Heavy I/O (loadFeature, worktree resolution, context reading, prompt building) + // is deferred to the real AutoModeService.followUpFeature implementation. validateWorkingDirectory(this.projectPath); const runningEntry = this.concurrencyManager.acquire({ @@ -555,56 +562,6 @@ export class AutoModeServiceFacade { projectPath: this.projectPath, isAutoMode: false, }); - const abortController = runningEntry.abortController; - - const feature = await this.featureStateManager.loadFeature(this.projectPath, featureId); - let workDir = path.resolve(this.projectPath); - let worktreePath: string | null = null; - const branchName = feature?.branchName || `feature/${featureId}`; - - if (useWorktrees && branchName) { - worktreePath = await this.worktreeResolver.findWorktreeForBranch( - this.projectPath, - branchName - ); - if (worktreePath) { - workDir = worktreePath; - } - } - - // Load previous context - const featureDir = getFeatureDir(this.projectPath, featureId); - const contextPath = path.join(featureDir, 'agent-output.md'); - let previousContext = ''; - try { - previousContext = (await secureFs.readFile(contextPath, 'utf-8')) as string; - } catch { - // No previous context - } - - const prompts = await getPromptCustomization(this.settingsService, '[Facade]'); - - // Build follow-up prompt inline (no template in TaskExecutionPrompts) - let fullPrompt = `## Follow-up on Feature Implementation - -${feature ? `**Feature ID:** ${feature.id}\n**Title:** ${feature.title || 'Untitled'}\n**Description:** ${feature.description}` : `**Feature ID:** ${featureId}`} -`; - - if (previousContext) { - fullPrompt += ` -## Previous Agent Work -The following is the output from the previous implementation attempt: - -${previousContext} -`; - } - - fullPrompt += ` -## Follow-up Instructions -${prompt} - -## Task -Address the follow-up instructions above. Review the previous work and make the requested changes or fixes.`; try { // NOTE: Facade does not have runAgent - this method requires AutoModeService @@ -617,8 +574,8 @@ Address the follow-up instructions above. Review the previous work and make the if (!errorInfo.isAbort) { this.eventBus.emitAutoModeEvent('auto_mode_error', { featureId, - featureName: feature?.title, - branchName: feature?.branchName ?? null, + featureName: undefined, + branchName: null, error: errorInfo.message, errorType: errorInfo.type, projectPath: this.projectPath, @@ -854,7 +811,9 @@ Address the follow-up instructions above. Review the previous work and make the async checkWorktreeCapacity(featureId: string): Promise { const feature = await this.featureStateManager.loadFeature(this.projectPath, featureId); const rawBranchName = feature?.branchName ?? null; - const branchName = rawBranchName === 'main' ? null : rawBranchName; + // Normalize primary branch to null (works for main, master, or any default branch) + const primaryBranch = await this.worktreeResolver.getCurrentBranch(this.projectPath); + const branchName = rawBranchName === primaryBranch ? null : rawBranchName; const maxAgents = await this.autoLoopCoordinator.resolveMaxConcurrency( this.projectPath, diff --git a/apps/server/src/services/feature-state-manager.ts b/apps/server/src/services/feature-state-manager.ts index 3fcf69fc..cd35859e 100644 --- a/apps/server/src/services/feature-state-manager.ts +++ b/apps/server/src/services/feature-state-manager.ts @@ -123,23 +123,28 @@ export class FeatureStateManager { }); // Create notifications for important status changes - const notificationService = getNotificationService(); - if (status === 'waiting_approval') { - await notificationService.createNotification({ - type: 'feature_waiting_approval', - title: 'Feature Ready for Review', - message: `"${feature.name || featureId}" is ready for your review and approval.`, - featureId, - projectPath, - }); - } else if (status === 'verified') { - await notificationService.createNotification({ - type: 'feature_verified', - title: 'Feature Verified', - message: `"${feature.name || featureId}" has been verified and is complete.`, - featureId, - projectPath, - }); + // Wrapped in try-catch so failures don't block syncFeatureToAppSpec below + try { + const notificationService = getNotificationService(); + if (status === 'waiting_approval') { + await notificationService.createNotification({ + type: 'feature_waiting_approval', + title: 'Feature Ready for Review', + message: `"${feature.name || featureId}" is ready for your review and approval.`, + featureId, + projectPath, + }); + } else if (status === 'verified') { + await notificationService.createNotification({ + type: 'feature_verified', + title: 'Feature Verified', + message: `"${feature.name || featureId}" has been verified and is complete.`, + featureId, + projectPath, + }); + } + } catch (notificationError) { + logger.warn(`Failed to create notification for feature ${featureId}:`, notificationError); } // Sync completed/verified features to app_spec.txt @@ -334,7 +339,7 @@ export class FeatureStateManager { Object.assign(feature.planSpec, updates); // If content is being updated and it's different from old content, increment version - if (updates.content && updates.content !== oldContent) { + if (updates.content !== undefined && updates.content !== oldContent) { feature.planSpec.version = (feature.planSpec.version || 0) + 1; } @@ -446,6 +451,11 @@ export class FeatureStateManager { status, tasks: feature.planSpec.tasks, }); + } else { + const availableIds = feature.planSpec.tasks.map((t) => t.id).join(', '); + logger.warn( + `[updateTaskStatus] Task ${taskId} not found in feature ${featureId} (${projectPath}). Available task IDs: [${availableIds}]` + ); } } catch (error) { logger.error(`Failed to update task ${taskId} status for ${featureId}:`, error); diff --git a/apps/server/src/services/pipeline-orchestrator.ts b/apps/server/src/services/pipeline-orchestrator.ts index 3eb427c8..4308825b 100644 --- a/apps/server/src/services/pipeline-orchestrator.ts +++ b/apps/server/src/services/pipeline-orchestrator.ts @@ -362,7 +362,7 @@ export class PipelineOrchestrator { await this.executePipeline(context); // Re-fetch feature to check if executePipeline set a terminal status (e.g., merge_conflict) - const reloadedFeature = await this.featureLoader.getById(projectPath, featureId); + const reloadedFeature = await this.featureStateManager.loadFeature(projectPath, featureId); const finalStatus = feature.skipTests ? 'waiting_approval' : 'verified'; // Only update status if not already in a terminal state @@ -516,7 +516,7 @@ export class PipelineOrchestrator { projectPath, branchName, worktreePath || projectPath, - targetBranch, + targetBranch || 'main', { deleteWorktreeAndBranch: false, } @@ -562,22 +562,33 @@ export class PipelineOrchestrator { let passCount = 0; let failCount = 0; + let inFailureContext = false; for (const line of lines) { const trimmed = line.trim(); if (trimmed.includes('FAIL') || trimmed.includes('FAILED')) { const match = trimmed.match(/(?:FAIL|FAILED)\s+(.+)/); if (match) failedTests.push(match[1].trim()); failCount++; + inFailureContext = true; } else if (trimmed.includes('PASS') || trimmed.includes('PASSED')) { passCount++; + inFailureContext = false; } if (trimmed.match(/^>\s+.*\.(test|spec)\./)) { failedTests.push(trimmed.replace(/^>\s+/, '')); } - if ( - trimmed.includes('AssertionError') || - trimmed.includes('toBe') || - trimmed.includes('toEqual') + // Only capture assertion details when they appear in failure context + // or match explicit assertion error / expect patterns + if (trimmed.includes('AssertionError') || trimmed.includes('AssertionError')) { + failedTests.push(trimmed); + } else if ( + inFailureContext && + /expect\(.+\)\.(toBe|toEqual|toMatch|toThrow|toContain)\s*\(/.test(trimmed) + ) { + failedTests.push(trimmed); + } else if ( + inFailureContext && + (trimmed.startsWith('Expected') || trimmed.startsWith('Received')) ) { failedTests.push(trimmed); } diff --git a/apps/ui/eslint.config.mjs b/apps/ui/eslint.config.mjs index 6cf025de..3ad4d79d 100644 --- a/apps/ui/eslint.config.mjs +++ b/apps/ui/eslint.config.mjs @@ -96,6 +96,7 @@ const eslintConfig = defineConfig([ setInterval: 'readonly', clearTimeout: 'readonly', clearInterval: 'readonly', + queueMicrotask: 'readonly', // Node.js (for scripts and Electron) process: 'readonly', require: 'readonly', diff --git a/apps/ui/src/store/app-store.ts b/apps/ui/src/store/app-store.ts index c0735355..7cc77907 100644 --- a/apps/ui/src/store/app-store.ts +++ b/apps/ui/src/store/app-store.ts @@ -7,7 +7,6 @@ import { createLogger } from '@automaker/utils/logger'; // Note: setItem/getItem moved to ./utils/theme-utils.ts import { UI_SANS_FONT_OPTIONS, UI_MONO_FONT_OPTIONS } from '@/config/ui-font-options'; import type { - Feature as BaseFeature, FeatureImagePath, FeatureTextFilePath, ModelAlias, @@ -15,25 +14,11 @@ import type { ThinkingLevel, ReasoningEffort, ModelProvider, - CursorModelId, - CodexModelId, - OpencodeModelId, - GeminiModelId, - CopilotModelId, - PhaseModelConfig, PhaseModelKey, PhaseModelEntry, - MCPServerConfig, - FeatureStatusWithPipeline, - PipelineConfig, PipelineStep, - PromptCustomization, ModelDefinition, ServerLogLevel, - EventHook, - ClaudeApiProfile, - ClaudeCompatibleProvider, - SidebarStyle, ParsedTask, PlanSpec, } from '@automaker/types'; @@ -2131,7 +2116,7 @@ export const useAppStore = create()((set, get) => ({ const updateSizes = (layout: TerminalPanelContent): TerminalPanelContent => { if (layout.type === 'split') { // Find matching panels and update sizes - const updatedPanels = layout.panels.map((panel, index) => { + const updatedPanels = layout.panels.map((panel, _index) => { // Generate key for this panel const panelKey = panel.type === 'split' diff --git a/apps/ui/src/store/types/state-types.ts b/apps/ui/src/store/types/state-types.ts index 4febb1ca..e06bb618 100644 --- a/apps/ui/src/store/types/state-types.ts +++ b/apps/ui/src/store/types/state-types.ts @@ -2,8 +2,6 @@ import type { Project, TrashedProject } from '@/lib/electron'; import type { ModelAlias, PlanningMode, - ThinkingLevel, - ReasoningEffort, ModelProvider, CursorModelId, CodexModelId, @@ -33,7 +31,7 @@ import type { BackgroundSettings, } from './ui-types'; import type { ApiKeys } from './settings-types'; -import type { ChatMessage, ChatSession, FeatureImage } from './chat-types'; +import type { ChatMessage, ChatSession } from './chat-types'; import type { TerminalState, TerminalPanelContent, PersistedTerminalState } from './terminal-types'; import type { Feature, ProjectAnalysis } from './project-types'; import type { ClaudeUsage, CodexUsage } from './usage-types';