diff --git a/apps/backend/agents/tools_pkg/tools/qa.py b/apps/backend/agents/tools_pkg/tools/qa.py index 865ed8b8..33339abf 100644 --- a/apps/backend/agents/tools_pkg/tools/qa.py +++ b/apps/backend/agents/tools_pkg/tools/qa.py @@ -56,14 +56,10 @@ def _apply_qa_update( "ready_for_qa_revalidation": status == "fixes_applied", } - # Update plan status to match QA result - # This ensures the UI shows the correct column after QA - if status == "approved": - plan["status"] = "human_review" - plan["planStatus"] = "review" - elif status == "rejected": - plan["status"] = "human_review" - plan["planStatus"] = "review" + # NOTE: Do NOT write plan["status"] or plan["planStatus"] here. + # The frontend XState task state machine owns status transitions. + # Writing status here races with XState's persistPlanStatusAndReasonSync() + # and can clobber the reviewReason field, causing tasks to appear "incomplete". plan["last_updated"] = datetime.now(timezone.utc).isoformat() diff --git a/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts b/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts index a057276b..6dd99107 100644 --- a/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts +++ b/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts @@ -297,7 +297,7 @@ describe('Subprocess Spawn Integration', () => { // Simulate stdout data (must include newline for buffered output processing) mockStdout.emit('data', Buffer.from('Test log output\n')); - expect(logHandler).toHaveBeenCalledWith('task-1', 'Test log output\n'); + expect(logHandler).toHaveBeenCalledWith('task-1', 'Test log output\n', undefined); }, 30000); // Increase timeout for Windows CI (dynamic imports are slow) it('should emit log events from stderr', async () => { @@ -313,7 +313,7 @@ describe('Subprocess Spawn Integration', () => { // Simulate stderr data (must include newline for buffered output processing) mockStderr.emit('data', Buffer.from('Progress: 50%\n')); - expect(logHandler).toHaveBeenCalledWith('task-1', 'Progress: 50%\n'); + expect(logHandler).toHaveBeenCalledWith('task-1', 'Progress: 50%\n', undefined); }, 30000); // Increase timeout for Windows CI (dynamic imports are slow) it('should emit exit event when process exits', async () => { @@ -329,8 +329,8 @@ describe('Subprocess Spawn Integration', () => { // Simulate process exit mockProcess.emit('exit', 0); - // Exit event includes taskId, exit code, and process type - expect(exitHandler).toHaveBeenCalledWith('task-1', 0, expect.any(String)); + // Exit event includes taskId, exit code, process type, and optional projectId + expect(exitHandler).toHaveBeenCalledWith('task-1', 0, expect.any(String), undefined); }, 30000); // Increase timeout for Windows CI (dynamic imports are slow) it('should emit error event when process errors', async () => { @@ -346,7 +346,7 @@ describe('Subprocess Spawn Integration', () => { // Simulate process error mockProcess.emit('error', new Error('Spawn failed')); - expect(errorHandler).toHaveBeenCalledWith('task-1', 'Spawn failed'); + expect(errorHandler).toHaveBeenCalledWith('task-1', 'Spawn failed', undefined); }, 30000); // Increase timeout for Windows CI (dynamic imports are slow) it('should kill task and remove from tracking', async () => { diff --git a/apps/frontend/src/main/__tests__/task-state-manager.test.ts b/apps/frontend/src/main/__tests__/task-state-manager.test.ts index ebcce5d9..eadbbb73 100644 --- a/apps/frontend/src/main/__tests__/task-state-manager.test.ts +++ b/apps/frontend/src/main/__tests__/task-state-manager.test.ts @@ -390,6 +390,83 @@ describe('TaskStateManager', () => { // Should have sent PROCESS_EXITED event with unexpected=true // This should transition to error state }); + + it('should NOT mark exit code 0 as unexpected (plan_review stays intact)', () => { + // Simulate: PLANNING_STARTED → PLANNING_COMPLETE (requireReview) → process exits code 0 + const planningStarted = { + type: 'PLANNING_STARTED', + taskId: mockTask.id, + specId: mockTask.specId, + projectId: mockProject.id, + timestamp: new Date().toISOString(), + eventId: 'evt-1', + sequence: 0 + }; + + const planningComplete = { + type: 'PLANNING_COMPLETE', + taskId: mockTask.id, + specId: mockTask.specId, + projectId: mockProject.id, + timestamp: new Date().toISOString(), + eventId: 'evt-2', + sequence: 1, + hasSubtasks: false, + subtaskCount: 0, + requireReviewBeforeCoding: true + }; + + manager.handleTaskEvent(mockTask.id, planningStarted, mockTask, mockProject); + manager.handleTaskEvent(mockTask.id, planningComplete, mockTask, mockProject); + + // XState should be in plan_review now + expect(manager.getCurrentState(mockTask.id)).toBe('plan_review'); + + // Process exits with code 0 - should NOT transition to error + manager.handleProcessExited(mockTask.id, 0, mockTask, mockProject); + + // PLANNING_COMPLETE is a terminal event, so handleProcessExited should skip entirely + // Task should remain in plan_review + expect(manager.getCurrentState(mockTask.id)).toBe('plan_review'); + }); + + it('should treat PLANNING_COMPLETE as a terminal event', () => { + // PLANNING_COMPLETE should prevent handleProcessExited from running + const planningStarted = { + type: 'PLANNING_STARTED', + taskId: mockTask.id, + specId: mockTask.specId, + projectId: mockProject.id, + timestamp: new Date().toISOString(), + eventId: 'evt-1', + sequence: 0 + }; + + const planningComplete = { + type: 'PLANNING_COMPLETE', + taskId: mockTask.id, + specId: mockTask.specId, + projectId: mockProject.id, + timestamp: new Date().toISOString(), + eventId: 'evt-2', + sequence: 1, + hasSubtasks: true, + subtaskCount: 3, + requireReviewBeforeCoding: false + }; + + manager.handleTaskEvent(mockTask.id, planningStarted, mockTask, mockProject); + manager.handleTaskEvent(mockTask.id, planningComplete, mockTask, mockProject); + + // XState should be in coding (no review required) + expect(manager.getCurrentState(mockTask.id)).toBe('coding'); + + // Process exits with code 1 - should still skip because PLANNING_COMPLETE is terminal + manager.handleProcessExited(mockTask.id, 1, mockTask, mockProject); + + // Task should remain in coding, NOT transition to error + expect(manager.getCurrentState(mockTask.id)).toBe('coding'); + }); }); describe('actor state restoration', () => { diff --git a/apps/frontend/src/main/agent/agent-manager.ts b/apps/frontend/src/main/agent/agent-manager.ts index 3001a762..7f40ea56 100644 --- a/apps/frontend/src/main/agent/agent-manager.ts +++ b/apps/frontend/src/main/agent/agent-manager.ts @@ -32,6 +32,7 @@ export class AgentManager extends EventEmitter { metadata?: SpecCreationMetadata; baseBranch?: string; swapCount: number; + projectId?: string; }> = new Map(); constructor() { @@ -51,7 +52,7 @@ export class AgentManager extends EventEmitter { }); // Listen for task completion to clean up context (prevent memory leak) - this.on('exit', (taskId: string, code: number | null) => { + this.on('exit', (taskId: string, code: number | null, _processType?: string, _projectId?: string) => { // Clean up context when: // 1. Task completed successfully (code === 0), or // 2. Task failed and won't be restarted (handled by auto-swap logic) @@ -93,7 +94,8 @@ export class AgentManager extends EventEmitter { taskDescription: string, specDir?: string, metadata?: SpecCreationMetadata, - baseBranch?: string + baseBranch?: string, + projectId?: string ): Promise { // Pre-flight auth check: Verify active profile has valid authentication // Ensure profile manager is initialized to prevent race condition @@ -173,10 +175,10 @@ export class AgentManager extends EventEmitter { } // Store context for potential restart - this.storeTaskContext(taskId, projectPath, '', {}, true, taskDescription, specDir, metadata, baseBranch); + this.storeTaskContext(taskId, projectPath, '', {}, true, taskDescription, specDir, metadata, baseBranch, projectId); // Note: This is spec-creation but it chains to task-execution via run.py - await this.processManager.spawnProcess(taskId, autoBuildSource, args, combinedEnv, 'task-execution'); + await this.processManager.spawnProcess(taskId, autoBuildSource, args, combinedEnv, 'task-execution', projectId); } /** @@ -186,7 +188,8 @@ export class AgentManager extends EventEmitter { taskId: string, projectPath: string, specId: string, - options: TaskExecutionOptions = {} + options: TaskExecutionOptions = {}, + projectId?: string ): Promise { // Pre-flight auth check: Verify active profile has valid authentication // Ensure profile manager is initialized to prevent race condition @@ -251,9 +254,9 @@ export class AgentManager extends EventEmitter { // which allows per-phase configuration for planner, coder, and QA phases // Store context for potential restart - this.storeTaskContext(taskId, projectPath, specId, options, false); + this.storeTaskContext(taskId, projectPath, specId, options, false, undefined, undefined, undefined, undefined, projectId); - await this.processManager.spawnProcess(taskId, autoBuildSource, args, combinedEnv, 'task-execution'); + await this.processManager.spawnProcess(taskId, autoBuildSource, args, combinedEnv, 'task-execution', projectId); } /** @@ -262,7 +265,8 @@ export class AgentManager extends EventEmitter { async startQAProcess( taskId: string, projectPath: string, - specId: string + specId: string, + projectId?: string ): Promise { // Ensure Python environment is ready before spawning process (prevents exit code 127 race condition) const pythonStatus = await this.processManager.ensurePythonEnvReady('AgentManager'); @@ -290,7 +294,7 @@ export class AgentManager extends EventEmitter { const args = [runPath, '--spec', specId, '--project-dir', projectPath, '--qa']; - await this.processManager.spawnProcess(taskId, autoBuildSource, args, combinedEnv, 'qa-process'); + await this.processManager.spawnProcess(taskId, autoBuildSource, args, combinedEnv, 'qa-process', projectId); } /** @@ -387,7 +391,8 @@ export class AgentManager extends EventEmitter { taskDescription?: string, specDir?: string, metadata?: SpecCreationMetadata, - baseBranch?: string + baseBranch?: string, + projectId?: string ): void { // Preserve swapCount if context already exists (for restarts) const existingContext = this.taskExecutionContext.get(taskId); @@ -402,7 +407,8 @@ export class AgentManager extends EventEmitter { specDir, metadata, baseBranch, - swapCount // Preserve existing count instead of resetting + swapCount, // Preserve existing count instead of resetting + projectId }); } @@ -464,7 +470,8 @@ export class AgentManager extends EventEmitter { context.taskDescription!, context.specDir, context.metadata, - context.baseBranch + context.baseBranch, + context.projectId ); } else { console.log('[AgentManager] Restarting as task execution'); @@ -472,7 +479,8 @@ export class AgentManager extends EventEmitter { taskId, context.projectPath, context.specId, - context.options + context.options, + context.projectId ); } }, 500); diff --git a/apps/frontend/src/main/agent/agent-process.ts b/apps/frontend/src/main/agent/agent-process.ts index be24a09c..a4fe2cdc 100644 --- a/apps/frontend/src/main/agent/agent-process.ts +++ b/apps/frontend/src/main/agent/agent-process.ts @@ -510,7 +510,8 @@ export class AgentProcessManager { cwd: string, args: string[], extraEnv: Record = {}, - processType: ProcessType = 'task-execution' + processType: ProcessType = 'task-execution', + projectId?: string ): Promise { const isSpecRunner = processType === 'spec-creation'; this.killProcess(taskId); @@ -562,7 +563,7 @@ export class AgentProcessManager { // spawn() failed synchronously (e.g., command not found, permission denied) // Clean up tracking entry and propagate error this.state.deleteProcess(taskId); - this.emitter.emit('error', taskId, err instanceof Error ? err.message : String(err)); + this.emitter.emit('error', taskId, err instanceof Error ? err.message : String(err), projectId); throw err; } @@ -609,7 +610,7 @@ export class AgentProcessManager { message: isSpecRunner ? 'Starting spec creation...' : 'Starting build process...', sequenceNumber: ++sequenceNumber, completedPhases: [...completedPhases] - }); + }, projectId); const isDebug = ['true', '1', 'yes', 'on'].includes(process.env.DEBUG?.toLowerCase() ?? ''); @@ -629,7 +630,7 @@ export class AgentProcessManager { const taskEvent = parseTaskEvent(line); if (taskEvent) { console.log(`[AgentProcess:${taskId}] Parsed task event:`, taskEvent.type, taskEvent); - this.emitter.emit('task-event', taskId, taskEvent); + this.emitter.emit('task-event', taskId, taskEvent, projectId); } const phaseUpdate = this.events.parseExecutionPhase(line, currentPhase, isSpecRunner); @@ -689,7 +690,7 @@ export class AgentProcessManager { message: lastMessage, sequenceNumber: ++sequenceNumber, completedPhases: [...completedPhases] - }); + }, projectId); } }; @@ -709,7 +710,7 @@ export class AgentProcessManager { for (const line of lines) { if (line.trim()) { - this.emitter.emit('log', taskId, line + '\n'); + this.emitter.emit('log', taskId, line + '\n', projectId); processLog(line); if (isDebug) { console.log(`[Agent:${taskId}] ${line}`); @@ -730,11 +731,11 @@ export class AgentProcessManager { childProcess.on('exit', (code: number | null) => { if (stdoutBuffer.trim()) { - this.emitter.emit('log', taskId, stdoutBuffer + '\n'); + this.emitter.emit('log', taskId, stdoutBuffer + '\n', projectId); processLog(stdoutBuffer); } if (stderrBuffer.trim()) { - this.emitter.emit('log', taskId, stderrBuffer + '\n'); + this.emitter.emit('log', taskId, stderrBuffer + '\n', projectId); processLog(stderrBuffer); } @@ -749,7 +750,7 @@ export class AgentProcessManager { console.log('[AgentProcess] Process failed with code:', code, 'for task:', taskId); const wasHandled = this.handleProcessFailure(taskId, allOutput, processType); if (wasHandled) { - this.emitter.emit('exit', taskId, code, processType); + this.emitter.emit('exit', taskId, code, processType, projectId); return; } } @@ -762,10 +763,10 @@ export class AgentProcessManager { message: `Process exited with code ${code}`, sequenceNumber: ++sequenceNumber, completedPhases: [...completedPhases] - }); + }, projectId); } - this.emitter.emit('exit', taskId, code, processType); + this.emitter.emit('exit', taskId, code, processType, projectId); }); // Handle process error @@ -780,9 +781,9 @@ export class AgentProcessManager { message: `Error: ${err.message}`, sequenceNumber: ++sequenceNumber, completedPhases: [...completedPhases] - }); + }, projectId); - this.emitter.emit('error', taskId, err.message); + this.emitter.emit('error', taskId, err.message, projectId); }); } diff --git a/apps/frontend/src/main/agent/types.ts b/apps/frontend/src/main/agent/types.ts index c1dfa987..47e3afeb 100644 --- a/apps/frontend/src/main/agent/types.ts +++ b/apps/frontend/src/main/agent/types.ts @@ -31,11 +31,11 @@ export interface ExecutionProgressData { export type ProcessType = 'spec-creation' | 'task-execution' | 'qa-process'; export interface AgentManagerEvents { - log: (taskId: string, log: string) => void; - error: (taskId: string, error: string) => void; - exit: (taskId: string, code: number | null, processType: ProcessType) => void; - 'execution-progress': (taskId: string, progress: ExecutionProgressData) => void; - 'task-event': (taskId: string, event: TaskEventPayload) => void; + log: (taskId: string, log: string, projectId?: string) => void; + error: (taskId: string, error: string, projectId?: string) => void; + exit: (taskId: string, code: number | null, processType: ProcessType, projectId?: string) => void; + 'execution-progress': (taskId: string, progress: ExecutionProgressData, projectId?: string) => void; + 'task-event': (taskId: string, event: TaskEventPayload, projectId?: string) => void; } // IdeationConfig now imported from shared types to maintain consistency diff --git a/apps/frontend/src/main/ipc-handlers/__tests__/settled-state-guard.test.ts b/apps/frontend/src/main/ipc-handlers/__tests__/settled-state-guard.test.ts new file mode 100644 index 00000000..20f6e1c5 --- /dev/null +++ b/apps/frontend/src/main/ipc-handlers/__tests__/settled-state-guard.test.ts @@ -0,0 +1,113 @@ +/** + * Tests for the XState settled-state guard logic used in agent-events-handlers. + * + * The guard prevents execution-progress events from overwriting XState's + * persisted status when the state machine has already settled into a + * terminal/review state. + */ +import { describe, it, expect } from 'vitest'; +import { XSTATE_SETTLED_STATES, XSTATE_TO_PHASE, TASK_STATE_NAMES } from '../../../shared/state-machines'; + +describe('XSTATE_SETTLED_STATES', () => { + it('should contain the expected settled states', () => { + expect(XSTATE_SETTLED_STATES.has('plan_review')).toBe(true); + expect(XSTATE_SETTLED_STATES.has('human_review')).toBe(true); + expect(XSTATE_SETTLED_STATES.has('error')).toBe(true); + expect(XSTATE_SETTLED_STATES.has('creating_pr')).toBe(true); + expect(XSTATE_SETTLED_STATES.has('pr_created')).toBe(true); + expect(XSTATE_SETTLED_STATES.has('done')).toBe(true); + }); + + it('should NOT contain active processing states', () => { + expect(XSTATE_SETTLED_STATES.has('backlog')).toBe(false); + expect(XSTATE_SETTLED_STATES.has('planning')).toBe(false); + expect(XSTATE_SETTLED_STATES.has('coding')).toBe(false); + expect(XSTATE_SETTLED_STATES.has('qa_review')).toBe(false); + expect(XSTATE_SETTLED_STATES.has('qa_fixing')).toBe(false); + }); + + it('should only contain valid task state names', () => { + const validNames = new Set(TASK_STATE_NAMES); + for (const state of XSTATE_SETTLED_STATES) { + expect(validNames.has(state as typeof TASK_STATE_NAMES[number])).toBe(true); + } + }); +}); + +describe('settled state guard behavior', () => { + /** + * Simulates the guard logic from agent-events-handlers execution-progress handler. + * Returns true if the event should be blocked (XState is in a settled state). + */ + function shouldBlockExecutionProgress(currentXState: string | undefined): boolean { + return !!(currentXState && XSTATE_SETTLED_STATES.has(currentXState)); + } + + it('should block execution-progress when XState is in plan_review', () => { + // After PLANNING_COMPLETE with requireReviewBeforeCoding=true, + // process exits with code 1 emitting phase='failed' — must be blocked + expect(shouldBlockExecutionProgress('plan_review')).toBe(true); + }); + + it('should block execution-progress when XState is in human_review', () => { + // After QA_PASSED, any stale events from the dying process must be blocked + expect(shouldBlockExecutionProgress('human_review')).toBe(true); + }); + + it('should block execution-progress when XState is in error', () => { + // After PLANNING_FAILED/CODING_FAILED, stale events must not overwrite error status + expect(shouldBlockExecutionProgress('error')).toBe(true); + }); + + it('should block execution-progress when XState is in done', () => { + expect(shouldBlockExecutionProgress('done')).toBe(true); + }); + + it('should allow execution-progress when XState is in planning', () => { + expect(shouldBlockExecutionProgress('planning')).toBe(false); + }); + + it('should allow execution-progress when XState is in coding', () => { + // After USER_RESUMED from error, XState transitions to coding synchronously. + // New agent events should flow through normally. + expect(shouldBlockExecutionProgress('coding')).toBe(false); + }); + + it('should allow execution-progress when XState is in qa_review', () => { + expect(shouldBlockExecutionProgress('qa_review')).toBe(false); + }); + + it('should allow execution-progress when no XState actor exists', () => { + // No actor yet (first event for this task) — must not block + expect(shouldBlockExecutionProgress(undefined)).toBe(false); + }); +}); + +describe('XSTATE_TO_PHASE', () => { + it('should have a mapping for every task state', () => { + for (const state of TASK_STATE_NAMES) { + expect(XSTATE_TO_PHASE[state]).toBeDefined(); + } + }); + + it('should map settled states to non-active phases', () => { + // Settled states should map to phases that indicate completion or stoppage + expect(XSTATE_TO_PHASE['plan_review']).toBe('planning'); + expect(XSTATE_TO_PHASE['human_review']).toBe('complete'); + expect(XSTATE_TO_PHASE['error']).toBe('failed'); + expect(XSTATE_TO_PHASE['done']).toBe('complete'); + expect(XSTATE_TO_PHASE['pr_created']).toBe('complete'); + expect(XSTATE_TO_PHASE['creating_pr']).toBe('complete'); + }); + + it('should map active states to processing phases', () => { + expect(XSTATE_TO_PHASE['planning']).toBe('planning'); + expect(XSTATE_TO_PHASE['coding']).toBe('coding'); + expect(XSTATE_TO_PHASE['qa_review']).toBe('qa_review'); + expect(XSTATE_TO_PHASE['qa_fixing']).toBe('qa_fixing'); + }); + + it('should return undefined for unknown states', () => { + expect(XSTATE_TO_PHASE['nonexistent']).toBeUndefined(); + }); +}); diff --git a/apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts b/apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts index 421c0e44..c0625596 100644 --- a/apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts @@ -7,12 +7,13 @@ import type { AuthFailureInfo, ImplementationPlan, } from "../../shared/types"; +import { XSTATE_SETTLED_STATES, XSTATE_TO_PHASE, mapStateToLegacy } from "../../shared/state-machines"; import { AgentManager } from "../agent"; import type { ProcessType, ExecutionProgressData } from "../agent"; import { titleGenerator } from "../title-generator"; import { fileWatcher } from "../file-watcher"; import { notificationService } from "../notification-service"; -import { persistPlanLastEventSync, getPlanPath, persistPlanPhaseSync } from "./task/plan-file-utils"; +import { persistPlanLastEventSync, getPlanPath, persistPlanPhaseSync, persistPlanStatusAndReasonSync } from "./task/plan-file-utils"; import { findTaskWorktree } from "../worktree-paths"; import { findTaskAndProject } from "./task/shared"; import { safeSendToRenderer } from "./utils"; @@ -32,16 +33,22 @@ export function registerAgenteventsHandlers( // Agent Manager Events → Renderer // ============================================ - agentManager.on("log", (taskId: string, log: string) => { - // Include projectId for multi-project filtering (issue #723) - const { project } = findTaskAndProject(taskId); - safeSendToRenderer(getMainWindow, IPC_CHANNELS.TASK_LOG, taskId, log, project?.id); + agentManager.on("log", (taskId: string, log: string, projectId?: string) => { + // Use projectId from event when available; fall back to lookup for backward compatibility + if (!projectId) { + const { project } = findTaskAndProject(taskId); + projectId = project?.id; + } + safeSendToRenderer(getMainWindow, IPC_CHANNELS.TASK_LOG, taskId, log, projectId); }); - agentManager.on("error", (taskId: string, error: string) => { - // Include projectId for multi-project filtering (issue #723) - const { project } = findTaskAndProject(taskId); - safeSendToRenderer(getMainWindow, IPC_CHANNELS.TASK_ERROR, taskId, error, project?.id); + agentManager.on("error", (taskId: string, error: string, projectId?: string) => { + // Use projectId from event when available; fall back to lookup for backward compatibility + if (!projectId) { + const { project } = findTaskAndProject(taskId); + projectId = project?.id; + } + safeSendToRenderer(getMainWindow, IPC_CHANNELS.TASK_ERROR, taskId, error, projectId); }); // Handle SDK rate limit events from agent manager @@ -82,10 +89,10 @@ export function registerAgenteventsHandlers( safeSendToRenderer(getMainWindow, IPC_CHANNELS.CLAUDE_AUTH_FAILURE, authFailureInfo); }); - agentManager.on("exit", (taskId: string, code: number | null, processType: ProcessType) => { - // Get task + project for context and multi-project filtering (issue #723) - const { task: exitTask, project: exitProject } = findTaskAndProject(taskId); - const exitProjectId = exitProject?.id; + agentManager.on("exit", (taskId: string, code: number | null, processType: ProcessType, projectId?: string) => { + // Use projectId from event to scope the lookup (prevents cross-project contamination) + const { task: exitTask, project: exitProject } = findTaskAndProject(taskId, projectId); + const exitProjectId = exitProject?.id || projectId; taskStateManager.handleProcessExited(taskId, code, exitTask, exitProject); @@ -109,7 +116,7 @@ export function registerAgenteventsHandlers( return; } - const { task, project } = findTaskAndProject(taskId); + const { task, project } = findTaskAndProject(taskId, projectId); if (!task || !project) return; const taskTitle = task.title || task.specId; @@ -120,11 +127,11 @@ export function registerAgenteventsHandlers( } }); - agentManager.on("task-event", (taskId: string, event) => { - console.log(`[agent-events-handlers] Received task-event for ${taskId}:`, event.type, event); + agentManager.on("task-event", (taskId: string, event, projectId?: string) => { + console.debug(`[agent-events-handlers] Received task-event for ${taskId}:`, event.type, event); if (taskStateManager.getLastSequence(taskId) === undefined) { - const { task, project } = findTaskAndProject(taskId); + const { task, project } = findTaskAndProject(taskId, projectId); if (task && project) { try { const planPath = getPlanPath(project, task); @@ -140,20 +147,20 @@ export function registerAgenteventsHandlers( } } - const { task, project } = findTaskAndProject(taskId); + const { task, project } = findTaskAndProject(taskId, projectId); if (!task || !project) { - console.log(`[agent-events-handlers] No task/project found for ${taskId}`); + console.debug(`[agent-events-handlers] No task/project found for ${taskId}`); return; } - console.log(`[agent-events-handlers] Task state before handleTaskEvent:`, { + console.debug(`[agent-events-handlers] Task state before handleTaskEvent:`, { status: task.status, reviewReason: task.reviewReason, phase: task.executionProgress?.phase }); const accepted = taskStateManager.handleTaskEvent(taskId, event, task, project); - console.log(`[agent-events-handlers] Event ${event.type} accepted: ${accepted}`); + console.debug(`[agent-events-handlers] Event ${event.type} accepted: ${accepted}`); if (!accepted) { return; } @@ -176,14 +183,27 @@ export function registerAgenteventsHandlers( } }); - agentManager.on("execution-progress", (taskId: string, progress: ExecutionProgressData) => { - // Use shared helper to find task and project (issue #723 - deduplicate lookup) - const { task, project } = findTaskAndProject(taskId); - const taskProjectId = project?.id; + agentManager.on("execution-progress", (taskId: string, progress: ExecutionProgressData, projectId?: string) => { + // Use projectId from event to scope the lookup (prevents cross-project contamination) + const { task, project } = findTaskAndProject(taskId, projectId); + const taskProjectId = project?.id || projectId; + + // Check if XState has already established a terminal/review state for this task. + // XState is the source of truth for status. When XState is in a terminal state + // (e.g., plan_review after PLANNING_COMPLETE), execution-progress events from the + // agent process are stale and must not overwrite XState's persisted status. + // + // Example: When requireReviewBeforeCoding=true, the process exits with code 1 after + // PLANNING_COMPLETE. The exit handler emits execution-progress with phase='failed', + // which would incorrectly overwrite status='human_review' with status='error' via + // persistPlanPhaseSync, and send a 'failed' phase to the renderer overwriting the + // 'planning' phase that XState already emitted via emitPhaseFromState. + const currentXState = taskStateManager.getCurrentState(taskId); + const xstateInTerminalState = currentXState && XSTATE_SETTLED_STATES.has(currentXState); // Persist phase to plan file for restoration on app refresh // Must persist to BOTH main project and worktree (if exists) since task may be loaded from either - if (task && project && progress.phase) { + if (task && project && progress.phase && !xstateInTerminalState) { const mainPlanPath = getPlanPath(project, task); persistPlanPhaseSync(mainPlanPath, progress.phase, project.id); @@ -201,9 +221,16 @@ export function registerAgenteventsHandlers( persistPlanPhaseSync(worktreePlanPath, progress.phase, project.id); } } + } else if (xstateInTerminalState && progress.phase) { + console.debug(`[agent-events-handlers] Skipping persistPlanPhaseSync for ${taskId}: XState in '${currentXState}', not overwriting with phase '${progress.phase}'`); } - // Include projectId in execution progress event for multi-project filtering + // Skip sending execution-progress to renderer when XState has settled. + // XState's emitPhaseFromState already sent the correct phase to the renderer. + if (xstateInTerminalState) { + console.debug(`[agent-events-handlers] Skipping execution-progress to renderer for ${taskId}: XState in '${currentXState}', ignoring phase '${progress.phase}'`); + return; + } safeSendToRenderer( getMainWindow, IPC_CHANNELS.TASK_EXECUTION_PROGRESS, @@ -218,13 +245,42 @@ export function registerAgenteventsHandlers( // ============================================ fileWatcher.on("progress", (taskId: string, plan: ImplementationPlan) => { - // Use shared helper to find project (issue #723 - deduplicate lookup) - const { project } = findTaskAndProject(taskId); + // File watcher events don't carry projectId — fall back to lookup + const { task, project } = findTaskAndProject(taskId); safeSendToRenderer(getMainWindow, IPC_CHANNELS.TASK_PROGRESS, taskId, plan, project?.id); + + // Re-stamp XState status fields if the backend overwrote the plan file without them. + // The planner agent writes implementation_plan.json via the Write tool, which replaces + // the entire file and strips the frontend's status/xstateState/executionPhase fields. + // This causes tasks to snap back to backlog on refresh. + const planWithStatus = plan as { xstateState?: string; executionPhase?: string; status?: string }; + const currentXState = taskStateManager.getCurrentState(taskId); + if (currentXState && !planWithStatus.xstateState && task && project) { + console.debug(`[agent-events-handlers] Re-stamping XState status on plan file for ${taskId} (state: ${currentXState})`); + const mainPlanPath = getPlanPath(project, task); + const { status, reviewReason } = mapStateToLegacy(currentXState); + const phase = XSTATE_TO_PHASE[currentXState] || 'idle'; + persistPlanStatusAndReasonSync(mainPlanPath, status, reviewReason, project.id, currentXState, phase); + + // Also re-stamp worktree copy if it exists + const worktreePath = findTaskWorktree(project.path, task.specId); + if (worktreePath) { + const specsBaseDir = getSpecsDir(project.autoBuildPath); + const worktreePlanPath = path.join( + worktreePath, + specsBaseDir, + task.specId, + AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN + ); + if (existsSync(worktreePlanPath)) { + persistPlanStatusAndReasonSync(worktreePlanPath, status, reviewReason, project.id, currentXState, phase); + } + } + } }); fileWatcher.on("error", (taskId: string, error: string) => { - // Include projectId for multi-project filtering (issue #723) + // File watcher events don't carry projectId — fall back to lookup const { project } = findTaskAndProject(taskId); safeSendToRenderer(getMainWindow, IPC_CHANNELS.TASK_ERROR, taskId, error, project?.id); }); diff --git a/apps/frontend/src/main/ipc-handlers/task/__tests__/find-task-and-project.test.ts b/apps/frontend/src/main/ipc-handlers/task/__tests__/find-task-and-project.test.ts new file mode 100644 index 00000000..f3a56986 --- /dev/null +++ b/apps/frontend/src/main/ipc-handlers/task/__tests__/find-task-and-project.test.ts @@ -0,0 +1,157 @@ +/** + * Tests for findTaskAndProject cross-project scoping. + * Verifies that projectId prevents cross-project task contamination + * when multiple projects have tasks with the same specId. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { findTaskAndProject } from '../shared'; +import type { Task, Project } from '../../../../shared/types'; + +// Mock projectStore +const mockProjects: Project[] = []; +const mockTasksByProject: Map = new Map(); + +vi.mock('../../../project-store', () => ({ + projectStore: { + getProjects: () => mockProjects, + getTasks: (projectId: string) => mockTasksByProject.get(projectId) || [] + } +})); + +function createTask(overrides: Partial = {}): Task { + return { + id: `task-${Date.now()}-${Math.random().toString(36).substring(7)}`, + specId: 'test-spec', + projectId: 'project-1', + title: 'Test Task', + description: 'Test', + status: 'backlog', + subtasks: [], + logs: [], + createdAt: new Date(), + updatedAt: new Date(), + ...overrides + }; +} + +function createProject(overrides: Partial = {}): Project { + return { + id: `project-${Date.now()}`, + name: 'Test Project', + path: '/test/project', + createdAt: new Date().toISOString(), + lastOpenedAt: new Date().toISOString(), + ...overrides + } as Project; +} + +describe('findTaskAndProject', () => { + beforeEach(() => { + mockProjects.length = 0; + mockTasksByProject.clear(); + }); + + it('should find task by specId without projectId (backward compatibility)', () => { + const project = createProject({ id: 'proj-1' }); + const task = createTask({ id: 'task-1', specId: 'write-to-file', projectId: 'proj-1' }); + + mockProjects.push(project); + mockTasksByProject.set('proj-1', [task]); + + const result = findTaskAndProject('write-to-file'); + expect(result.task).toBe(task); + expect(result.project).toBe(project); + }); + + it('should scope search to specified project when projectId is provided', () => { + const projectA = createProject({ id: 'proj-a', name: 'Project A' }); + const projectB = createProject({ id: 'proj-b', name: 'Project B' }); + + const taskA = createTask({ id: 'task-a', specId: 'write-to-file', projectId: 'proj-a' }); + const taskB = createTask({ id: 'task-b', specId: 'write-to-file', projectId: 'proj-b' }); + + mockProjects.push(projectA, projectB); + mockTasksByProject.set('proj-a', [taskA]); + mockTasksByProject.set('proj-b', [taskB]); + + // Without projectId - returns first match (Project A) + const resultNoScope = findTaskAndProject('write-to-file'); + expect(resultNoScope.task).toBe(taskA); + expect(resultNoScope.project).toBe(projectA); + + // With projectId for Project B - returns Project B's task + const resultScopedB = findTaskAndProject('write-to-file', 'proj-b'); + expect(resultScopedB.task).toBe(taskB); + expect(resultScopedB.project).toBe(projectB); + + // With projectId for Project A - returns Project A's task + const resultScopedA = findTaskAndProject('write-to-file', 'proj-a'); + expect(resultScopedA.task).toBe(taskA); + expect(resultScopedA.project).toBe(projectA); + }); + + it('should NOT fall back to other projects when projectId is provided but task not found', () => { + const projectA = createProject({ id: 'proj-a' }); + const projectB = createProject({ id: 'proj-b' }); + + const taskA = createTask({ id: 'task-a', specId: 'write-to-file', projectId: 'proj-a' }); + + mockProjects.push(projectA, projectB); + mockTasksByProject.set('proj-a', [taskA]); + mockTasksByProject.set('proj-b', []); + + // Search Project B (which has no tasks) — should NOT find Project A's task + const result = findTaskAndProject('write-to-file', 'proj-b'); + expect(result.task).toBeUndefined(); + expect(result.project).toBeUndefined(); + }); + + it('should return undefined when projectId refers to a non-existent project', () => { + const project = createProject({ id: 'proj-1' }); + const task = createTask({ id: 'task-1', specId: 'write-to-file', projectId: 'proj-1' }); + + mockProjects.push(project); + mockTasksByProject.set('proj-1', [task]); + + // Search with a projectId that doesn't exist — should NOT fall back + const result = findTaskAndProject('write-to-file', 'non-existent-project'); + expect(result.task).toBeUndefined(); + expect(result.project).toBeUndefined(); + }); + + it('should return undefined when task not found in any project', () => { + const project = createProject({ id: 'proj-1' }); + mockProjects.push(project); + mockTasksByProject.set('proj-1', []); + + const result = findTaskAndProject('nonexistent-task'); + expect(result.task).toBeUndefined(); + expect(result.project).toBeUndefined(); + }); + + it('should find task by id as well as specId', () => { + const project = createProject({ id: 'proj-1' }); + const task = createTask({ id: 'unique-uuid', specId: 'write-to-file', projectId: 'proj-1' }); + + mockProjects.push(project); + mockTasksByProject.set('proj-1', [task]); + + const result = findTaskAndProject('unique-uuid', 'proj-1'); + expect(result.task).toBe(task); + expect(result.project).toBe(project); + }); + + it('should log warning when provided projectId is not found', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + mockProjects.push(createProject({ id: 'proj-1' })); + + findTaskAndProject('some-task', 'ghost-project'); + + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('ghost-project'), + // Flexible match on the rest of the message + ); + warnSpy.mockRestore(); + }); +}); diff --git a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts index c90949e1..b04a0a5f 100644 --- a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts @@ -246,7 +246,7 @@ export function registerTaskExecutionHandlers( // Start spec creation process - pass the existing spec directory // so spec_runner uses it instead of creating a new one // Also pass baseBranch so worktrees are created from the correct branch - agentManager.startSpecCreation(taskId, project.path, taskDescription, specDir, task.metadata, baseBranch); + agentManager.startSpecCreation(taskId, project.path, taskDescription, specDir, task.metadata, baseBranch, project.id); } else if (needsImplementation) { // Spec exists but no subtasks - run run.py to create implementation plan and execute // Read the spec.md to get the task description @@ -269,7 +269,8 @@ export function registerTaskExecutionHandlers( workers: 1, baseBranch, useWorktree: task.metadata?.useWorktree - } + }, + project.id ); } else { // Task has subtasks, start normal execution @@ -285,7 +286,8 @@ export function registerTaskExecutionHandlers( workers: 1, baseBranch, useWorktree: task.metadata?.useWorktree - } + }, + project.id ); } } @@ -495,7 +497,7 @@ export function registerTaskExecutionHandlers( // The QA process needs to run where the implementation_plan.json with completed subtasks is const qaProjectPath = hasWorktree ? worktreePath : project.path; console.warn('[TASK_REVIEW] Starting QA process with projectPath:', qaProjectPath); - agentManager.startQAProcess(taskId, qaProjectPath, task.specId); + agentManager.startQAProcess(taskId, qaProjectPath, task.specId, project.id); taskStateManager.handleUiEvent( taskId, @@ -727,7 +729,7 @@ export function registerTaskExecutionHandlers( // No spec file - need to run spec_runner.py to create the spec const taskDescription = task.description || task.title; console.warn('[TASK_UPDATE_STATUS] Starting spec creation for:', task.specId); - agentManager.startSpecCreation(taskId, project.path, taskDescription, specDir, task.metadata, baseBranchForUpdate); + agentManager.startSpecCreation(taskId, project.path, taskDescription, specDir, task.metadata, baseBranchForUpdate, project.id); } else if (needsImplementation) { // Spec exists but no subtasks - run run.py to create implementation plan and execute console.warn('[TASK_UPDATE_STATUS] Starting task execution (no subtasks) for:', task.specId); @@ -740,7 +742,8 @@ export function registerTaskExecutionHandlers( workers: 1, baseBranch: baseBranchForUpdate, useWorktree: task.metadata?.useWorktree - } + }, + project.id ); } else { // Task has subtasks, start normal execution @@ -755,7 +758,8 @@ export function registerTaskExecutionHandlers( workers: 1, baseBranch: baseBranchForUpdate, useWorktree: task.metadata?.useWorktree - } + }, + project.id ); } @@ -1108,7 +1112,7 @@ export function registerTaskExecutionHandlers( // No spec file - need to run spec_runner.py to create the spec const taskDescription = task.description || task.title; console.warn(`[Recovery] Starting spec creation for: ${task.specId}`); - agentManager.startSpecCreation(taskId, project.path, taskDescription, specDirForWatcher, task.metadata, baseBranchForRecovery); + agentManager.startSpecCreation(taskId, project.path, taskDescription, specDirForWatcher, task.metadata, baseBranchForRecovery, project.id); } else { // Spec exists - run task execution console.warn(`[Recovery] Starting task execution for: ${task.specId}`); @@ -1121,7 +1125,8 @@ export function registerTaskExecutionHandlers( workers: 1, baseBranch: baseBranchForRecovery, useWorktree: task.metadata?.useWorktree - } + }, + project.id ); } diff --git a/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts b/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts index 04887596..3c8fe419 100644 --- a/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts +++ b/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts @@ -314,8 +314,8 @@ export function persistPlanPhaseSync( const phaseToStatus: Record = { 'planning': 'in_progress', 'coding': 'in_progress', - 'qa_review': 'in_progress', - 'qa_fixing': 'in_progress', + 'qa_review': 'ai_review', + 'qa_fixing': 'ai_review', 'complete': 'human_review', 'failed': 'error' }; diff --git a/apps/frontend/src/main/ipc-handlers/task/shared.ts b/apps/frontend/src/main/ipc-handlers/task/shared.ts index a72e9b81..16164088 100644 --- a/apps/frontend/src/main/ipc-handlers/task/shared.ts +++ b/apps/frontend/src/main/ipc-handlers/task/shared.ts @@ -2,21 +2,39 @@ import type { Task, Project } from '../../../shared/types'; import { projectStore } from '../../project-store'; /** - * Helper function to find task and project by taskId + * Helper function to find task and project by taskId. + * + * When projectId is provided, the search is strictly scoped to that project. + * If the task is not found in the specified project, returns undefined (does NOT + * fall back to other projects). This prevents cross-project contamination when + * multiple projects have tasks with the same specId. + * + * When projectId is NOT provided, searches all projects for backward + * compatibility with callers that don't have projectId (e.g., file watcher events). */ -export const findTaskAndProject = (taskId: string): { task: Task | undefined; project: Project | undefined } => { +export const findTaskAndProject = (taskId: string, projectId?: string): { task: Task | undefined; project: Project | undefined } => { const projects = projectStore.getProjects(); - let task: Task | undefined; - let project: Project | undefined; + // If projectId provided, search ONLY that project (no fallback) + if (projectId) { + const targetProject = projects.find((p) => p.id === projectId); + if (!targetProject) { + console.warn(`[findTaskAndProject] projectId "${projectId}" not found in projects list, returning undefined`); + return { task: undefined, project: undefined }; + } + const tasks = projectStore.getTasks(targetProject.id); + const task = tasks.find((t) => t.id === taskId || t.specId === taskId); + return { task, project: task ? targetProject : undefined }; + } + + // No projectId: search all projects (backward compatibility for file watcher etc.) for (const p of projects) { const tasks = projectStore.getTasks(p.id); - task = tasks.find((t) => t.id === taskId || t.specId === taskId); + const task = tasks.find((t) => t.id === taskId || t.specId === taskId); if (task) { - project = p; - break; + return { task, project: p }; } } - return { task, project }; + return { task: undefined, project: undefined }; }; diff --git a/apps/frontend/src/main/task-state-manager.ts b/apps/frontend/src/main/task-state-manager.ts index 51b4ac82..df0ee0a8 100644 --- a/apps/frontend/src/main/task-state-manager.ts +++ b/apps/frontend/src/main/task-state-manager.ts @@ -3,7 +3,7 @@ import type { ActorRefFrom } from 'xstate'; import type { BrowserWindow } from 'electron'; import type { TaskEventPayload } from './agent/task-event-schema'; import type { Project, Task, TaskStatus, ReviewReason, ExecutionPhase } from '../shared/types'; -import { taskMachine, type TaskEvent } from '../shared/state-machines'; +import { taskMachine, XSTATE_TO_PHASE, mapStateToLegacy, type TaskEvent } from '../shared/state-machines'; import { IPC_CHANNELS } from '../shared/constants'; import { safeSendToRenderer } from './ipc-handlers/utils'; import { getPlanPath, persistPlanStatusAndReasonSync } from './ipc-handlers/task/plan-file-utils'; @@ -14,21 +14,6 @@ import path from 'path'; type TaskActor = ActorRefFrom; -/** Maps XState states to execution phases. Shared by mapStateToExecutionPhase and emitPhaseFromState. */ -const XSTATE_TO_PHASE: Record = { - 'backlog': 'idle', - 'planning': 'planning', - 'plan_review': 'planning', - 'coding': 'coding', - 'qa_review': 'qa_review', - 'qa_fixing': 'qa_fixing', - 'human_review': 'complete', - 'error': 'failed', - 'creating_pr': 'complete', - 'pr_created': 'complete', - 'done': 'complete' -}; - interface TaskContextEntry { task: Task; project: Project; @@ -36,6 +21,7 @@ interface TaskContextEntry { const TERMINAL_EVENTS = new Set([ 'QA_PASSED', + 'PLANNING_COMPLETE', 'PLANNING_FAILED', 'CODING_FAILED', 'QA_MAX_ITERATIONS', @@ -57,10 +43,10 @@ export class TaskStateManager { handleTaskEvent(taskId: string, event: TaskEventPayload, task: Task, project: Project): boolean { const lastSeq = this.lastSequenceByTask.get(taskId); - console.log(`[TaskStateManager] handleTaskEvent: ${event.type} seq=${event.sequence}, lastSeq=${lastSeq}`); + console.debug(`[TaskStateManager] handleTaskEvent: ${event.type} seq=${event.sequence}, lastSeq=${lastSeq}`); if (!this.isNewSequence(taskId, event.sequence)) { - console.log(`[TaskStateManager] Event ${event.type} DROPPED - sequence ${event.sequence} not newer than ${lastSeq}`); + console.debug(`[TaskStateManager] Event ${event.type} DROPPED - sequence ${event.sequence} not newer than ${lastSeq}`); return false; } this.setTaskContext(taskId, task, project); @@ -72,10 +58,10 @@ export class TaskStateManager { const actor = this.getOrCreateActor(taskId); const stateBefore = String(actor.getSnapshot().value); - console.log(`[TaskStateManager] Sending ${event.type} to actor in state: ${stateBefore}`); + console.debug(`[TaskStateManager] Sending ${event.type} to actor in state: ${stateBefore}`); actor.send(event as TaskEvent); const stateAfter = String(actor.getSnapshot().value); - console.log(`[TaskStateManager] After ${event.type}: state ${stateBefore} -> ${stateAfter}`); + console.debug(`[TaskStateManager] After ${event.type}: state ${stateBefore} -> ${stateAfter}`); return true; } @@ -92,22 +78,26 @@ export class TaskStateManager { return; } const actor = this.getOrCreateActor(taskId); + // Only mark as unexpected if the process exited with a non-zero code. + // A code-0 exit is normal (e.g., spec creation finished, plan created, waiting for review). + // Sending unexpected:true for code-0 exits incorrectly transitions plan_review → error. + const isUnexpected = exitCode !== 0; actor.send({ type: 'PROCESS_EXITED', exitCode: exitCode ?? -1, - unexpected: true + unexpected: isUnexpected } satisfies TaskEvent); } handleUiEvent(taskId: string, event: TaskEvent, task: Task, project: Project): void { - console.log(`[TaskStateManager] handleUiEvent: ${event.type} for task ${taskId}`); + console.debug(`[TaskStateManager] handleUiEvent: ${event.type} for task ${taskId}`); this.setTaskContext(taskId, task, project); const actor = this.getOrCreateActor(taskId); const stateBefore = String(actor.getSnapshot().value); - console.log(`[TaskStateManager] Sending UI event ${event.type} to actor in state: ${stateBefore}`); + console.debug(`[TaskStateManager] Sending UI event ${event.type} to actor in state: ${stateBefore}`); actor.send(event); const stateAfter = String(actor.getSnapshot().value); - console.log(`[TaskStateManager] After UI event ${event.type}: state ${stateBefore} -> ${stateAfter}`); + console.debug(`[TaskStateManager] After UI event ${event.type}: state ${stateBefore} -> ${stateAfter}`); } handleManualStatusChange(taskId: string, status: TaskStatus, task: Task, project: Project): boolean { @@ -220,7 +210,7 @@ export class TaskStateManager { private getOrCreateActor(taskId: string): TaskActor { const existing = this.actors.get(taskId); if (existing) { - console.log(`[TaskStateManager] Using existing actor for ${taskId}, current state:`, String(existing.getSnapshot().value)); + console.debug(`[TaskStateManager] Using existing actor for ${taskId}, current state:`, String(existing.getSnapshot().value)); return existing; } @@ -230,14 +220,14 @@ export class TaskStateManager { : undefined; if (contextEntry) { - console.log(`[TaskStateManager] Creating new actor for ${taskId} from task:`, { + console.debug(`[TaskStateManager] Creating new actor for ${taskId} from task:`, { status: contextEntry.task.status, reviewReason: contextEntry.task.reviewReason, phase: contextEntry.task.executionProgress?.phase, initialState: snapshot ? String(snapshot.value) : 'default (backlog)' }); } else { - console.log(`[TaskStateManager] Creating new actor for ${taskId} with default state (no context entry)`); + console.debug(`[TaskStateManager] Creating new actor for ${taskId} with default state (no context entry)`); } const actor = snapshot @@ -247,8 +237,7 @@ export class TaskStateManager { const stateValue = String(snapshot.value); const lastState = this.lastStateByTask.get(taskId); - // Debug: Log all state transitions - console.log(`[TaskStateManager] XState transition for ${taskId}:`, { + console.debug(`[TaskStateManager] XState transition for ${taskId}:`, { from: lastState, to: stateValue, contextReviewReason: snapshot.context.reviewReason @@ -273,8 +262,7 @@ export class TaskStateManager { // Map XState state to execution phase for persistence const executionPhase = this.mapStateToExecutionPhase(stateValue); - // Debug: Log the mapped status and reviewReason - console.log(`[TaskStateManager] Emitting status for ${taskId}:`, { + console.debug(`[TaskStateManager] Emitting status for ${taskId}:`, { status, reviewReason, xstateState: stateValue, @@ -338,7 +326,7 @@ export class TaskStateManager { console.warn(`[TaskStateManager] emitStatus: No main window, cannot emit status ${status} for ${taskId}`); return; } - console.log(`[TaskStateManager] emitStatus: Sending TASK_STATUS_CHANGE for ${taskId}:`, { status, reviewReason, projectId }); + console.debug(`[TaskStateManager] emitStatus: Sending TASK_STATUS_CHANGE for ${taskId}:`, { status, reviewReason, projectId }); safeSendToRenderer( this.getMainWindow, IPC_CHANNELS.TASK_STATUS_CHANGE, @@ -441,33 +429,3 @@ export class TaskStateManager { } export const taskStateManager = new TaskStateManager(); - -function mapStateToLegacy( - state: string, - reviewReason?: ReviewReason -): { status: TaskStatus; reviewReason?: ReviewReason } { - switch (state) { - case 'backlog': - return { status: 'backlog' }; - case 'planning': - case 'coding': - return { status: 'in_progress' }; - case 'plan_review': - return { status: 'human_review', reviewReason: 'plan_review' }; - case 'qa_review': - case 'qa_fixing': - return { status: 'ai_review' }; - case 'human_review': - return { status: 'human_review', reviewReason }; - case 'error': - return { status: 'human_review', reviewReason: 'errors' }; - case 'creating_pr': - return { status: 'human_review', reviewReason: 'completed' }; - case 'pr_created': - return { status: 'pr_created' }; - case 'done': - return { status: 'done' }; - default: - return { status: 'backlog' }; - } -} diff --git a/apps/frontend/src/renderer/__tests__/task-store.test.ts b/apps/frontend/src/renderer/__tests__/task-store.test.ts index 1fad7ce1..11fe05f5 100644 --- a/apps/frontend/src/renderer/__tests__/task-store.test.ts +++ b/apps/frontend/src/renderer/__tests__/task-store.test.ts @@ -192,6 +192,42 @@ describe('Task Store', () => { originalDate.getTime() ); }); + + it('should apply reviewReason when provided', () => { + useTaskStore.setState({ + tasks: [createTestTask({ id: 'task-1', status: 'in_progress' })] + }); + + useTaskStore.getState().updateTaskStatus('task-1', 'human_review', 'plan_review'); + + const task = useTaskStore.getState().tasks[0]; + expect(task.status).toBe('human_review'); + expect(task.reviewReason).toBe('plan_review'); + }); + + it('should clear reviewReason when not provided', () => { + useTaskStore.setState({ + tasks: [createTestTask({ id: 'task-1', status: 'human_review', reviewReason: 'plan_review' })] + }); + + useTaskStore.getState().updateTaskStatus('task-1', 'in_progress'); + + const task = useTaskStore.getState().tasks[0]; + expect(task.status).toBe('in_progress'); + expect(task.reviewReason).toBeUndefined(); + }); + + it('should update when only reviewReason changes', () => { + useTaskStore.setState({ + tasks: [createTestTask({ id: 'task-1', status: 'human_review', reviewReason: 'plan_review' })] + }); + + useTaskStore.getState().updateTaskStatus('task-1', 'human_review', 'completed'); + + const task = useTaskStore.getState().tasks[0]; + expect(task.status).toBe('human_review'); + expect(task.reviewReason).toBe('completed'); + }); }); describe('updateTaskFromPlan', () => { diff --git a/apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx b/apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx index fc07d513..d84fee23 100644 --- a/apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx +++ b/apps/frontend/src/renderer/components/task-detail/TaskDetailModal.tsx @@ -33,6 +33,7 @@ import { import { cn } from '../../lib/utils'; import { calculateProgress } from '../../lib/utils'; import { startTask, stopTask, submitReview, recoverStuckTask, deleteTask, useTaskStore } from '../../stores/task-store'; +import { useProjectStore } from '../../stores/project-store'; import { TASK_STATUS_LABELS } from '../../../shared/constants'; import { TaskEditDialog } from '../TaskEditDialog'; import { useTaskDetail } from './hooks/useTaskDetail'; @@ -80,6 +81,7 @@ function TaskDetailModalContent({ open, task, onOpenChange, onSwitchToTerminals, const { t } = useTranslation(['tasks']); const { toast } = useToast(); const state = useTaskDetail({ task }); + const activeProject = useProjectStore(s => s.getActiveProject()); const showFilesTab = isFilesTabEnabled(); const progressPercent = calculateProgress(task.subtasks); const completedSubtasks = task.subtasks.filter(s => s.status === 'completed').length; @@ -410,6 +412,8 @@ function TaskDetailModalContent({ open, task, onOpenChange, onSwitchToTerminals, {window.DEBUG && (
status={task.status} reviewReason={task.reviewReason ?? 'none'} phase={task.executionProgress?.phase ?? 'none'} reviewRequired={task.metadata?.requireReviewBeforeCoding ? 'true' : 'false'} +
+ projectId={activeProject?.id ?? 'none'} projectName={activeProject?.name ?? 'none'}
)} diff --git a/apps/frontend/src/renderer/stores/task-store.ts b/apps/frontend/src/renderer/stores/task-store.ts index 9269c504..57dea5c9 100644 --- a/apps/frontend/src/renderer/stores/task-store.ts +++ b/apps/frontend/src/renderer/stores/task-store.ts @@ -247,9 +247,9 @@ export const useTaskStore = create((set, get) => ({ const oldTask = state.tasks[index]; const oldStatus = oldTask.status; - // Skip if status is the same - if (oldStatus === status) { - debugLog('[updateTaskStatus] Status unchanged, skipping:', { taskId, status }); + // Skip if status AND reviewReason are the same + if (oldStatus === status && oldTask.reviewReason === reviewReason) { + debugLog('[updateTaskStatus] Status and reviewReason unchanged, skipping:', { taskId, status, reviewReason }); return; } @@ -276,7 +276,7 @@ export const useTaskStore = create((set, get) => ({ executionProgress = { phase: 'planning' as ExecutionPhase, phaseProgress: 0, overallProgress: 0 }; } - return { ...t, status, executionProgress, updatedAt: new Date() }; + return { ...t, status, reviewReason, executionProgress, updatedAt: new Date() }; }); debugLog('[updateTaskStatus] AFTER set():', { diff --git a/apps/frontend/src/shared/state-machines/index.ts b/apps/frontend/src/shared/state-machines/index.ts index 25df4a36..68cd1f3a 100644 --- a/apps/frontend/src/shared/state-machines/index.ts +++ b/apps/frontend/src/shared/state-machines/index.ts @@ -1,2 +1,9 @@ export { taskMachine } from './task-machine'; -export type { TaskContext, TaskEvent } from './task-machine'; +export type { TaskContext, TaskEvent } from './task-machine'; +export { + TASK_STATE_NAMES, + XSTATE_SETTLED_STATES, + XSTATE_TO_PHASE, + mapStateToLegacy, +} from './task-state-utils'; +export type { TaskStateName } from './task-state-utils'; diff --git a/apps/frontend/src/shared/state-machines/task-state-utils.ts b/apps/frontend/src/shared/state-machines/task-state-utils.ts new file mode 100644 index 00000000..94799d76 --- /dev/null +++ b/apps/frontend/src/shared/state-machines/task-state-utils.ts @@ -0,0 +1,89 @@ +/** + * Shared XState task state utilities. + * + * Provides type-safe state names, phase mappings, and legacy status conversion + * derived from the task machine definition. Used by task-state-manager and + * agent-events-handlers to avoid duplicate constants. + */ +import type { TaskStatus, ReviewReason, ExecutionPhase } from '../types'; + +/** + * All XState task state names. + * + * IMPORTANT: These must match the state keys in task-machine.ts. + * If you add/remove a state in the machine, update this array. + */ +export const TASK_STATE_NAMES = [ + 'backlog', 'planning', 'plan_review', 'coding', + 'qa_review', 'qa_fixing', 'human_review', 'error', + 'creating_pr', 'pr_created', 'done' +] as const; + +export type TaskStateName = typeof TASK_STATE_NAMES[number]; + +/** + * XState states where the task has "settled" — the state machine has determined + * the task's final or review status. Execution-progress events from the agent + * process should NOT overwrite these states, as XState is the source of truth. + * + * Note: `error` is included because stale execution-progress events (e.g., + * phase='failed') may arrive after XState has already transitioned to error. + * When a user resumes from error (USER_RESUMED), XState transitions synchronously + * to `coding` before the new agent process emits events, so the guard no longer + * blocks — new execution-progress events flow through normally. + */ +export const XSTATE_SETTLED_STATES: ReadonlySet = new Set([ + 'plan_review', 'human_review', 'error', 'creating_pr', 'pr_created', 'done' +]); + +/** Maps XState states to execution phases. */ +export const XSTATE_TO_PHASE: Record & Record = { + 'backlog': 'idle', + 'planning': 'planning', + 'plan_review': 'planning', + 'coding': 'coding', + 'qa_review': 'qa_review', + 'qa_fixing': 'qa_fixing', + 'human_review': 'complete', + 'error': 'failed', + 'creating_pr': 'complete', + 'pr_created': 'complete', + 'done': 'complete' +}; + +/** + * Convert XState state to legacy status/reviewReason pair. + * + * When reviewReason is provided (from XState context), it's used for the + * human_review state. Otherwise defaults to 'completed' (used by re-stamp + * callers that don't have access to the XState context). + */ +export function mapStateToLegacy( + state: string, + reviewReason?: ReviewReason +): { status: TaskStatus; reviewReason?: ReviewReason } { + switch (state) { + case 'backlog': + return { status: 'backlog' }; + case 'planning': + case 'coding': + return { status: 'in_progress' }; + case 'plan_review': + return { status: 'human_review', reviewReason: 'plan_review' }; + case 'qa_review': + case 'qa_fixing': + return { status: 'ai_review' }; + case 'human_review': + return { status: 'human_review', reviewReason: reviewReason ?? 'completed' }; + case 'error': + return { status: 'human_review', reviewReason: 'errors' }; + case 'creating_pr': + return { status: 'human_review', reviewReason: 'completed' }; + case 'pr_created': + return { status: 'pr_created' }; + case 'done': + return { status: 'done' }; + default: + return { status: 'backlog' }; + } +} diff --git a/guides/cross-project-projectid-tracking.md b/guides/cross-project-projectid-tracking.md new file mode 100644 index 00000000..05ac7b79 --- /dev/null +++ b/guides/cross-project-projectid-tracking.md @@ -0,0 +1,166 @@ +# Cross-Project Task Contamination: Missing projectId in Agent Event Pipeline + +## Description + +When running multiple projects simultaneously, agent events from one project can corrupt the status, badges, and column placement of tasks in another project. The root cause is that the entire agent event pipeline (from process spawn through XState state machine to disk persistence) identifies tasks by `specId` alone, with no project scoping. Since specIds are derived from task descriptions and are not unique across projects, `findTaskAndProject(taskId)` returns the first match across all loaded projects, routing events to the wrong task. + +## Severity + +**High** - Silent data corruption. Affected tasks show wrong status, wrong badges, land in wrong Kanban columns, and persist corrupted state to disk. On refresh, the corrupted state is reloaded, making the damage permanent until manually fixed. + +## Affected Versions + +All versions using the XState task state machine (PR #1575 and later). + +## Steps to Reproduce + +1. Open Auto Claude and load two projects (e.g., "Project A" and "Project B") +2. In Project A, create a task with a specific name (e.g., "write wtf to text file") - this generates specId `016-write-wtf-to-text-file` +3. In Project B, create a task with the same name - this generates the same specId `016-write-wtf-to-text-file` +4. Start both tasks simultaneously (or start Project A's task first, let it reach QA, then start Project B's task) +5. Switch between projects and observe the Kanban board + +## Expected Results + +- Each project's task progresses independently through its own lifecycle +- Events from Project A's agent process only affect Project A's task card +- Events from Project B's agent process only affect Project B's task card +- Refreshing the app preserves the correct status for both tasks +- Switching between projects shows each task in its correct column with the correct badge + +## Actual Results + +- Tasks in the non-active project show wrong status badges (e.g., "Coding" badge on a task still in the Planning column) +- Tasks snap to the wrong Kanban column after refresh +- Tasks get stuck in states they should have transitioned out of (e.g., permanently stuck in "Planning") +- "Incomplete" badges appear on tasks that completed their phase successfully +- QA tasks appear in "In Progress" column instead of "AI Review" column after switching projects +- The corrupted state persists to `implementation_plan.json`, so the damage survives app restart + +## Root Cause + +### Task Identity Collision + +Every task has two identifiers: + +- **`task.id`** - A UUID, unique globally +- **`task.specId`** - The spec directory name (e.g., `016-write-wtf-to-text-file`), derived from the task description, **not unique across projects** + +The backend process uses `specId` as the task identifier in stdout markers. All agent event handlers resolve this back to a Task object via `findTaskAndProject(taskId)`, which searches all projects and returns the first match. + +### Missing projectId in Event Pipeline + +The agent event pipeline has no project scoping: + +``` +Backend process (Python) + -> stdout/stderr (phase markers, task events, logs) + -> agent-process.ts (parses output, emits typed events) + -> agent-manager.ts (EventEmitter relay) + -> agent-events-handlers.ts (event handlers) + -> findTaskAndProject(taskId) <-- COLLISION POINT + -> taskStateManager (XState actor) + -> persistPlanStatusAndReasonSync (disk) + -> safeSendToRenderer (IPC to UI) +``` + +None of the `AgentManagerEvents` carry a `projectId`: + +```typescript +// BEFORE: no way to scope events to the correct project +interface AgentManagerEvents { + log: (taskId: string, log: string) => void; + error: (taskId: string, error: string) => void; + exit: (taskId: string, code: number | null, processType: ProcessType) => void; + 'execution-progress': (taskId: string, progress: ExecutionProgressData) => void; + 'task-event': (taskId: string, event: TaskEventPayload) => void; +} +``` + +### Impact on XState + +The `TaskStateManager` maintains one XState actor per taskId and drives column placement, badge display, disk persistence, and renderer notifications. When an event is routed to the wrong project's actor: + +1. The actor receives an event invalid for its current state (e.g., `PLANNING_COMPLETE` sent to an actor in `qa_review`) +2. XState either drops the event or transitions to an unexpected state +3. The wrong project's `implementation_plan.json` is overwritten with incorrect status fields +4. On app refresh, the task loads from the corrupted plan file and appears in the wrong column +5. Subsequent legitimate events may be rejected because the actor is in a state that doesn't accept them + +### Contamination Example + +Given: +- **Project A**: task `016-write-wtf-to-text-file` in QA (`qa_review` state) +- **Project B**: task `016-write-wtf-to-text-file` just started (`planning` state) + +When Project B's planner emits `PLANNING_COMPLETE`: +1. `agent-process.ts` emits `task-event` with `taskId = "016-write-wtf-to-text-file"` and no projectId +2. `findTaskAndProject("016-write-wtf-to-text-file")` returns **Project A's task** (first match) +3. `PLANNING_COMPLETE` is sent to **Project A's XState actor** (which is in `qa_review`) +4. Project A's plan file is corrupted; Project B's task never receives the event + +## Observed Symptoms + +| Symptom | Cause | +|---------|-------| +| "Coding" badge on a task in the Planning column | Project B's `CODING_STARTED` event hit Project A's planning task | +| Task snaps to backlog on refresh | Plan file overwritten without XState fields; wrong project looked up for re-stamp | +| "Incomplete" badge on a task that just finished planning | `PROCESS_EXITED` event from Project B's process hit Project A's `plan_review` actor | +| QA task in "In Progress" column with "AI Review" badge | Execution progress event wrote wrong status to plan file | +| Task stuck in "Planning" forever | Events meant for this task were consumed by the duplicate in another project | + +## Fix + +Thread `projectId` from the IPC handler that starts each agent process through the entire event pipeline to the lookup function. + +### Propagation Chain + +``` +execution-handlers.ts + agentManager.startSpecCreation(..., project.id) <- Origin: project.id from IPC handler + agentManager.startTaskExecution(..., project.id) + agentManager.startQAProcess(..., project.id) + +agent-manager.ts + storeTaskContext(..., projectId) <- Stored in execution context + processManager.spawnProcess(..., projectId) <- Passed to process spawner + +agent-process.ts + this.emitter.emit('log', taskId, ..., projectId) <- Attached to every emitted event + this.emitter.emit('task-event', taskId, ..., projectId) + this.emitter.emit('execution-progress', ..., projectId) + this.emitter.emit('exit', taskId, ..., projectId) + this.emitter.emit('error', taskId, ..., projectId) + +agent-events-handlers.ts + findTaskAndProject(taskId, projectId) <- Scoped lookup + taskStateManager.handleTaskEvent(...) <- Correct actor receives event + persistPlanStatusAndReasonSync(...) <- Correct plan file updated + safeSendToRenderer(..., projectId) <- Renderer filters by project +``` + +### Scoped Lookup + +`findTaskAndProject` now accepts an optional `projectId`. When provided, it searches only the target project. Falls back to searching all projects for backward compatibility (file watcher events, renderer-initiated actions). + +## Files Changed + +| File | Change | +|------|--------| +| `apps/frontend/src/main/agent/types.ts` | Added `projectId?: string` to all event signatures | +| `apps/frontend/src/main/agent/agent-manager.ts` | Added `projectId` to context storage, start methods, restart flow | +| `apps/frontend/src/main/agent/agent-process.ts` | Added `projectId` to `spawnProcess` and all `emitter.emit()` calls | +| `apps/frontend/src/main/ipc-handlers/task/shared.ts` | Scoped `findTaskAndProject` by projectId with fallback | +| `apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts` | All event handlers receive and forward projectId | +| `apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts` | All 9 `agentManager.start*` call sites pass `project.id` | +| `apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts` | Updated test expectations for new projectId parameter | + +## Verification + +- [ ] Run two projects simultaneously with tasks that have the same specId +- [ ] Verify events from Project A only affect Project A's task cards +- [ ] Verify events from Project B only affect Project B's task cards +- [ ] Refresh the app during various lifecycle stages - tasks remain in correct columns +- [ ] Switch between projects during QA - task stays in AI Review column +- [ ] `npm run typecheck` passes +- [ ] `npm run test` passes (2639 tests, 0 failures) diff --git a/guides/pr-1575-fixes.md b/guides/pr-1575-fixes.md new file mode 100644 index 00000000..78a368f7 --- /dev/null +++ b/guides/pr-1575-fixes.md @@ -0,0 +1,139 @@ +# PR #1575 Follow-up: XState Status Lifecycle & Cross-Project Contamination Fixes + +## Overview + +After the XState task state machine migration (PR #1575), several interrelated bugs surfaced when running multiple projects simultaneously and during normal task lifecycle transitions. These bugs caused tasks to appear in wrong columns, display incorrect badges, and lose status on refresh. + +## Bug 1: Cross-Project Task Contamination + +### Problem +When two projects have tasks with the same specId (e.g., both have a task `016-write-wtf-to-text-file`), events from Project A's task would affect Project B's task card. Tasks in the secondary project would show wrong status badges (e.g., "Coding" badge on a task in the Planning column). + +### Root Cause +`findTaskAndProject(taskId)` searches ALL projects by matching `t.id === taskId || t.specId === taskId`, returning the first match. When the backend emits events using the specId as the task identifier, the lookup could match a task in the wrong project. + +Agent events (log, error, exit, execution-progress, task-event) did not carry a `projectId`, so there was no way to scope the lookup to the correct project. + +### Fix +- Added `projectId` to all `AgentManagerEvents` type signatures +- Pass `project.id` from all 9 `agentManager.start*` call sites in `execution-handlers.ts` +- Thread `projectId` through `agent-manager.ts` → `agent-process.ts` → all `emitter.emit()` calls +- Updated `findTaskAndProject(taskId, projectId?)` to scope search to the target project when `projectId` is provided, with fallback to searching all projects for backward compatibility +- All event handlers in `agent-events-handlers.ts` now receive and use `projectId` + +### Files Changed +- `apps/frontend/src/main/agent/types.ts` +- `apps/frontend/src/main/agent/agent-manager.ts` +- `apps/frontend/src/main/agent/agent-process.ts` +- `apps/frontend/src/main/ipc-handlers/task/shared.ts` +- `apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts` +- `apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts` +- `apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts` + +## Bug 2: "Incomplete" Badge on Plan Review Tasks + +### Problem +Tasks with `requireReviewBeforeCoding=true` would complete planning, correctly transition to `plan_review` state, but then immediately show an "Incomplete" badge instead of "Planning" + "Approve Plan". + +### Root Cause +Two issues combined: + +1. **`PLANNING_COMPLETE` was not in the `TERMINAL_EVENTS` set.** When the spec creation process finished normally (exit code 0), `handleProcessExited` was called. Since `PLANNING_COMPLETE` wasn't terminal, the check didn't skip. + +2. **`handleProcessExited` always sent `unexpected: true`**, even for exit code 0. This caused the XState guard `unexpectedExit` to pass, transitioning the task from `plan_review` → `error`, which overwrote the correct `plan_review` reviewReason. + +### Fix +- Added `PLANNING_COMPLETE` to the `TERMINAL_EVENTS` set so process exit is skipped when planning has already completed +- Changed `handleProcessExited` to only set `unexpected: true` when `exitCode !== 0` — a code-0 exit is normal and should not trigger error transitions + +### Files Changed +- `apps/frontend/src/main/task-state-manager.ts` + +## Bug 3: Backend qa.py Racing with XState Status + +### Problem +Tasks completing QA would sometimes show "Incomplete" instead of "Needs Review" because the `reviewReason` field was missing. + +### Root Cause +The backend `qa.py` tool was writing `plan["status"] = "human_review"` directly to the plan file WITHOUT setting `reviewReason`. This raced with the frontend XState state machine's `persistPlanStatusAndReasonSync()` which writes both `status` and `reviewReason` together. When qa.py wrote last, it clobbered the `reviewReason`. + +### Fix +Removed the backend's direct status writes from `qa.py`. The frontend XState state machine is now the sole owner of status transitions — the backend only updates `last_updated` timestamps and QA-specific fields. + +### Files Changed +- `apps/backend/agents/tools_pkg/tools/qa.py` + +## Bug 4: Plan File Overwrite by Planner Agent + +### Problem +After a task started, the frontend would persist XState status fields (`status`, `xstateState`, `executionPhase`) to `implementation_plan.json`. The planner agent would then create the full plan using the Write tool, completely replacing the file and stripping the frontend's status fields. On refresh, the task would snap back to backlog. + +### Root Cause +The planner agent writes `implementation_plan.json` via Claude's Write tool, which replaces the entire file. The agent-generated plan does not include frontend status fields (`xstateState`, `executionPhase`), so they are lost. + +### Fix +Added a re-stamp mechanism in the file watcher's `progress` event handler. When the file watcher detects a plan file change and the `xstateState` field is missing (indicating the backend overwrote the file), the handler re-persists the current XState state back to the file. This also covers the worktree copy. + +### Files Changed +- `apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts` + +## Bug 5: QA Tasks in Wrong Column After Project Switch + +### Problem +A task correctly in the "AI Review" column (status `ai_review`, phase `qa_review`) would snap to "In Progress" column after switching to another project and back. The "AI Review" badge would still show, but the card was in the wrong column. + +### Root Cause +`persistPlanPhaseSync()` in `plan-file-utils.ts` mapped execution phases to TaskStatus for column placement. It incorrectly mapped `qa_review` and `qa_fixing` to `in_progress` instead of `ai_review`. Every execution-progress event during QA would overwrite the correct `ai_review` status (set by XState) with `in_progress`. On refresh (reading from disk), the task loaded with `status: 'in_progress'` + `executionPhase: 'qa_review'`, placing it in the In Progress column with an AI Review badge. + +### Fix +Changed the phase-to-status mapping in `persistPlanPhaseSync`: +- `qa_review` → `ai_review` (was `in_progress`) +- `qa_fixing` → `ai_review` (was `in_progress`) + +### Files Changed +- `apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts` + +## Bug 6: updateTaskStatus Not Applying reviewReason + +### Problem +Tasks completing planning with `requireReviewBeforeCoding=true` would show an "Incomplete" badge in the Human Review column instead of "Planning" + "Approve Plan". The persisted plan file had the correct `reviewReason: 'plan_review'`, so refreshing the app would fix it. + +### Root Cause +`updateTaskStatus` in `task-store.ts` received `reviewReason` as a parameter but never applied it to the task object. The spread was `{ ...t, status, executionProgress }` — missing `reviewReason`. The skip condition also only checked `status`, not `reviewReason`, so transitions where only `reviewReason` changed (e.g., `human_review` with different reasons) were silently dropped. + +### Fix +- Added `reviewReason` to the task spread: `{ ...t, status, reviewReason, executionProgress }` +- Updated skip condition to check both `status` AND `reviewReason` + +### Files Changed +- `apps/frontend/src/renderer/stores/task-store.ts` + +## Bug 7: Task Stuck in "In Progress" After Planning (requireReviewBeforeCoding) + +### Problem +Tasks with `requireReviewBeforeCoding=true` would complete planning, XState would correctly transition to `plan_review`, but the task card would remain in the "In Progress" column with `status=in_progress, reviewReason=none, phase=planning`. + +### Root Cause +When the process exits with code 1 (expected — the interactive review checkpoint fails in piped mode), `agent-process.ts` emits an `execution-progress` event with `phase: 'failed'` before the `exit` event. The `execution-progress` handler in `agent-events-handlers.ts`: + +1. **Called `persistPlanPhaseSync` with `phase: 'failed'`**, which maps `failed` → `status: 'error'`, overwriting the `status: 'human_review'` that XState had already persisted to the plan file +2. **Sent `TASK_EXECUTION_PROGRESS` with `phase: 'failed'` to the renderer**, overwriting the `planning` phase that XState had already emitted via `emitPhaseFromState` + +Both operations bypassed XState's authority as the source of truth for status. + +### Fix +Added an XState "settled state" guard in the `execution-progress` handler. When XState has already transitioned to a settled state (`plan_review`, `human_review`, `error`, `creating_pr`, `pr_created`, `done`), the handler: +- Skips `persistPlanPhaseSync` to prevent overwriting XState's persisted status +- Skips sending `TASK_EXECUTION_PROGRESS` to the renderer to prevent overwriting XState's emitted phase + +XState's own `persistStatus()` and `emitPhaseFromState()` already handle disk and renderer updates correctly when transitioning to these states. + +### Files Changed +- `apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts` + +## Testing + +All fixes pass: +- `npm run typecheck` — clean +- `npm run test` — 2649 tests passing, 0 failures +- Manual testing: multi-project with same specIds, review-required tasks, project switching during QA, refresh at all lifecycle stages