From 96fc61298db8f9246dce340dd3920d84bc672c4e Mon Sep 17 00:00:00 2001 From: StillKnotKnown <192589389+StillKnotKnown@users.noreply.github.com> Date: Sun, 11 Jan 2026 08:26:54 +0200 Subject: [PATCH] fix(ACS-203): Fix Kanban status flip-flop and phase state inconsistency (#898) * fix(ACS-203): Fix Kanban status flip-flop and phase state inconsistency Fixes two related bugs affecting task state management: Issue 1 - Premature "done" status with incomplete subtasks: - Added subtask validation before allowing terminal status transitions - Tasks now only move to terminal statuses when all subtasks are completed - Prevents flip-flop when plan file is written with partial data Issue 2 - Phase state inconsistency (multiple phases active): - Added completedPhases tracking to ExecutionProgress type - Implemented phase prerequisite validation (e.g., planning must complete before coding) - Phase transitions now validate that previous phase completed - Prevents coding phase from starting while planning still shows as active Changes: - apps/frontend/src/renderer/stores/task-store.ts: Add defensive checks for terminal status transitions - apps/frontend/src/shared/types/task.ts: Add completedPhases to ExecutionProgress - apps/frontend/src/shared/constants/phase-protocol.ts: Add isValidPhaseTransition() and getExpectedPreviousPhase() - apps/frontend/src/main/agent/types.ts: Add completedPhases to ExecutionProgressData - apps/frontend/src/main/agent/agent-process.ts: Track and emit completedPhases on phase transitions - apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts: Validate phase transitions based on completed phases Signed-off-by: StillKnotKnown * refactor(ACS-203): Address PR review feedback - type safety and code improvements Improvements based on code review: HIGH: - Add explicit blocking for 'done' and 'pr_created' when subtasks are incomplete in shouldBlockTerminalTransition function MEDIUM: - Create shared CompletablePhase type for type consistency - Replace 'as any' cast with proper type guard function LOW: - Capture attemptedStatus before reassignment for accurate debug logging - Use centralized isTerminalPhase function instead of local array - Remove unused getExpectedPreviousPhase import Files changed: - apps/frontend/src/renderer/stores/task-store.ts - apps/frontend/src/shared/constants/phase-protocol.ts - apps/frontend/src/shared/types/task.ts - apps/frontend/src/main/agent/agent-process.ts - apps/frontend/src/main/agent/types.ts - apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts Signed-off-by: StillKnotKnown --------- Signed-off-by: StillKnotKnown Co-authored-by: StillKnotKnown Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com> --- apps/frontend/src/main/agent/agent-process.ts | 33 +++++- apps/frontend/src/main/agent/types.ts | 3 + .../ipc-handlers/agent-events-handlers.ts | 19 +++- .../src/renderer/stores/task-store.ts | 56 ++++++++- .../src/shared/constants/phase-protocol.ts | 106 ++++++++++++++++++ apps/frontend/src/shared/types/task.ts | 6 +- 6 files changed, 212 insertions(+), 11 deletions(-) diff --git a/apps/frontend/src/main/agent/agent-process.ts b/apps/frontend/src/main/agent/agent-process.ts index 03010bf9..fe256b96 100644 --- a/apps/frontend/src/main/agent/agent-process.ts +++ b/apps/frontend/src/main/agent/agent-process.ts @@ -6,6 +6,7 @@ import { EventEmitter } from 'events'; import { AgentState } from './agent-state'; import { AgentEvents } from './agent-events'; import { ProcessType, ExecutionProgressData } from './types'; +import type { CompletablePhase } from '../../shared/constants/phase-protocol'; import { detectRateLimit, createSDKRateLimitInfo, getProfileEnv, detectAuthFailure } from '../rate-limit-detector'; import { getAPIProfileEnv } from '../services/profile'; import { projectStore } from '../project-store'; @@ -454,13 +455,17 @@ export class AgentProcessManager { let stdoutBuffer = ''; let stderrBuffer = ''; let sequenceNumber = 0; + // FIX (ACS-203): Track completed phases to prevent phase overlaps + // When a phase completes, it's added to this array before transitioning to the next phase + let completedPhases: CompletablePhase[] = []; this.emitter.emit('execution-progress', taskId, { phase: currentPhase, phaseProgress: 0, overallProgress: this.events.calculateOverallProgress(currentPhase, 0), message: isSpecRunner ? 'Starting spec creation...' : 'Starting build process...', - sequenceNumber: ++sequenceNumber + sequenceNumber: ++sequenceNumber, + completedPhases: [...completedPhases] }); const isDebug = ['true', '1', 'yes', 'on'].includes(process.env.DEBUG?.toLowerCase() ?? ''); @@ -486,6 +491,21 @@ export class AgentProcessManager { console.log(`[PhaseDebug:${taskId}] Phase update: ${currentPhase} -> ${phaseUpdate.phase} (changed: ${phaseChanged})`); } + // FIX (ACS-203): Manage completedPhases when phases transition + // When leaving a non-terminal phase (not complete/failed), add it to completedPhases + if (phaseChanged && currentPhase !== 'idle' && currentPhase !== phaseUpdate.phase) { + // Type guard to narrow currentPhase to CompletablePhase + const isCompletablePhase = (phase: ExecutionProgressData['phase']): phase is CompletablePhase => { + return ['planning', 'coding', 'qa_review', 'qa_fixing'].includes(phase); + }; + if (isCompletablePhase(currentPhase) && !completedPhases.includes(currentPhase)) { + completedPhases.push(currentPhase); + if (isDebug) { + console.log(`[PhaseDebug:${taskId}] Marked phase as completed:`, { phase: currentPhase, completedPhases }); + } + } + } + currentPhase = phaseUpdate.phase; if (phaseUpdate.currentSubtask) { @@ -504,7 +524,7 @@ export class AgentProcessManager { const overallProgress = this.events.calculateOverallProgress(currentPhase, phaseProgress); if (isDebug) { - console.log(`[PhaseDebug:${taskId}] Emitting execution-progress:`, { phase: currentPhase, phaseProgress, overallProgress }); + console.log(`[PhaseDebug:${taskId}] Emitting execution-progress:`, { phase: currentPhase, phaseProgress, overallProgress, completedPhases }); } this.emitter.emit('execution-progress', taskId, { @@ -513,7 +533,8 @@ export class AgentProcessManager { overallProgress, currentSubtask, message: lastMessage, - sequenceNumber: ++sequenceNumber + sequenceNumber: ++sequenceNumber, + completedPhases: [...completedPhases] }); } }; @@ -585,7 +606,8 @@ export class AgentProcessManager { phaseProgress: 0, overallProgress: this.events.calculateOverallProgress(currentPhase, phaseProgress), message: `Process exited with code ${code}`, - sequenceNumber: ++sequenceNumber + sequenceNumber: ++sequenceNumber, + completedPhases: [...completedPhases] }); } @@ -602,7 +624,8 @@ export class AgentProcessManager { phaseProgress: 0, overallProgress: 0, message: `Error: ${err.message}`, - sequenceNumber: ++sequenceNumber + sequenceNumber: ++sequenceNumber, + completedPhases: [...completedPhases] }); this.emitter.emit('error', taskId, err.message); diff --git a/apps/frontend/src/main/agent/types.ts b/apps/frontend/src/main/agent/types.ts index 8d9eb9fc..f409db34 100644 --- a/apps/frontend/src/main/agent/types.ts +++ b/apps/frontend/src/main/agent/types.ts @@ -1,5 +1,6 @@ import { ChildProcess } from 'child_process'; import type { IdeationConfig } from '../../shared/types'; +import type { CompletablePhase } from '../../shared/constants/phase-protocol'; /** * Agent-specific types for process and state management @@ -22,6 +23,8 @@ export interface ExecutionProgressData { overallProgress: number; currentSubtask?: string; message?: string; + // FIX (ACS-203): Track completed phases to prevent phase overlaps + completedPhases?: CompletablePhase[]; } export type ProcessType = 'spec-creation' | 'task-execution' | 'qa-process'; 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 8a878724..eb208ff0 100644 --- a/apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts @@ -2,7 +2,7 @@ import type { BrowserWindow } from 'electron'; import path from 'path'; import { existsSync } from 'fs'; import { IPC_CHANNELS, AUTO_BUILD_PATHS, getSpecsDir } from '../../shared/constants'; -import { wouldPhaseRegress, isTerminalPhase, isValidExecutionPhase, type ExecutionPhase } from '../../shared/constants/phase-protocol'; +import { wouldPhaseRegress, isTerminalPhase, isValidExecutionPhase, isValidPhaseTransition, type ExecutionPhase } from '../../shared/constants/phase-protocol'; import type { SDKRateLimitInfo, Task, @@ -25,6 +25,7 @@ import { findTaskAndProject } from './task/shared'; * Validates status transitions to prevent invalid state changes. * FIX (ACS-55, ACS-71): Adds guardrails against bad status transitions. * FIX (PR Review): Uses comprehensive wouldPhaseRegress() utility instead of hardcoded checks. + * FIX (ACS-203): Adds phase completion validation to prevent phase overlaps. * * @param task - The current task (may be undefined if not found) * @param newStatus - The proposed new status @@ -50,6 +51,8 @@ function validateStatusTransition( // This handles all phase regressions (qa_review→coding, complete→coding, etc.) // not just the specific coding→planning case const currentPhase = task.executionProgress?.phase; + const completedPhases = task.executionProgress?.completedPhases || []; + if (currentPhase && isValidExecutionPhase(currentPhase) && isValidExecutionPhase(phase)) { // Block transitions from terminal phases (complete/failed) if (isTerminalPhase(currentPhase)) { @@ -63,6 +66,20 @@ function validateStatusTransition( console.warn(`[validateStatusTransition] Blocking phase regression: ${currentPhase} -> ${phase} for task ${task.id}`); return false; } + + // FIX (ACS-203): Validate phase transitions based on completed phases + // This prevents multiple phases from being active simultaneously + // e.g., coding starting while planning is still marked as active + const newPhase = phase as ExecutionPhase; + if (!isValidPhaseTransition(currentPhase, newPhase, completedPhases)) { + console.warn(`[validateStatusTransition] Blocking invalid phase transition: ${currentPhase} -> ${newPhase} for task ${task.id}`, { + currentPhase, + newPhase, + completedPhases, + reason: 'Prerequisite phases not completed' + }); + return false; + } } return true; diff --git a/apps/frontend/src/renderer/stores/task-store.ts b/apps/frontend/src/renderer/stores/task-store.ts index f3086d98..2b6837d8 100644 --- a/apps/frontend/src/renderer/stores/task-store.ts +++ b/apps/frontend/src/renderer/stores/task-store.ts @@ -1,6 +1,7 @@ import { create } from 'zustand'; import type { Task, TaskStatus, SubtaskStatus, ImplementationPlan, Subtask, TaskMetadata, ExecutionProgress, ExecutionPhase, ReviewReason, TaskDraft } from '../../shared/types'; import { debugLog } from '../../shared/utils/debug-logger'; +import { isTerminalPhase } from '../../shared/constants/phase-protocol'; interface TaskState { tasks: Task[]; @@ -217,8 +218,7 @@ export const useTaskStore = create((set, get) => ({ // FIX (Flip-Flop Bug): Terminal phases should NOT trigger status recalculation // When phase is 'complete' or 'failed', the task has finished and status should be stable - const terminalPhases: ExecutionPhase[] = ['complete', 'failed']; - const isInTerminalPhase = t.executionProgress?.phase && terminalPhases.includes(t.executionProgress.phase); + const isInTerminalPhase = t.executionProgress?.phase && isTerminalPhase(t.executionProgress.phase); // FIX (Flip-Flop Bug): Respect explicit human_review status from plan file // When the plan explicitly says 'human_review', don't override it with calculated status @@ -226,16 +226,45 @@ export const useTaskStore = create((set, get) => ({ const planStatus = plan.status; const isExplicitHumanReview = planStatus === 'human_review'; + // FIX (ACS-203): Add defensive check for terminal status transitions + // Before allowing transition to 'done', 'human_review', or 'ai_review', verify: + // 1. Subtasks array is properly populated (not empty) + // 2. All subtasks are actually completed (for 'done' and 'ai_review' statuses) + const hasSubtasks = subtasks.length > 0; + const terminalStatuses: TaskStatus[] = ['human_review', 'pr_created', 'done']; + + // If task is currently in a terminal status, validate subtasks before allowing downgrade + // This prevents flip-flop when plan file is written with incomplete data + const shouldBlockTerminalTransition = (newStatus: TaskStatus): boolean => { + // Block if: moving to terminal status but subtasks indicate incomplete work + if (terminalStatuses.includes(newStatus) || newStatus === 'ai_review') { + // For ai_review, all subtasks must be completed + if (newStatus === 'ai_review' && (!allCompleted || !hasSubtasks)) { + return true; + } + // For done and pr_created, all subtasks must be completed + if ((newStatus === 'done' || newStatus === 'pr_created') && (!allCompleted || !hasSubtasks)) { + return true; + } + // For human_review with 'completed' reason, all subtasks must be done + // But allow 'errors' or 'qa_rejected' reasons even with incomplete subtasks + if (newStatus === 'human_review' && anyFailed) { + return false; // Allow human_review for failed subtasks + } + } + return false; + }; + // Only recalculate status if: // 1. NOT in an active execution phase (planning, coding, qa_review, qa_fixing) // 2. NOT in a terminal phase (complete, failed) - status should be stable // 3. Plan doesn't explicitly say human_review + // 4. Would not create an invalid terminal transition (ACS-203) if (!isInActivePhase && !isInTerminalPhase && !isExplicitHumanReview) { - if (allCompleted) { + if (allCompleted && hasSubtasks) { // FIX (Flip-Flop Bug): Don't downgrade from terminal statuses to ai_review // Once a task reaches human_review, pr_created, or done, it should stay there // unless explicitly changed (these are finalized workflow states) - const terminalStatuses: TaskStatus[] = ['human_review', 'pr_created', 'done']; if (!terminalStatuses.includes(t.status)) { status = 'ai_review'; } @@ -247,6 +276,25 @@ export const useTaskStore = create((set, get) => ({ } } + // FIX (ACS-203): Final validation - prevent invalid terminal status transitions + // This catches cases where the logic above would set a terminal status + // but the subtask state doesn't support it (e.g., empty subtasks array) + if (shouldBlockTerminalTransition(status)) { + // Capture attempted status before reassignment for accurate logging + const attemptedStatus = status; + // Keep current status instead of transitioning to invalid terminal state + status = t.status; + debugLog('[updateTaskFromPlan] Blocked invalid terminal transition:', { + taskId, + attemptedStatus, + currentStatus: t.status, + hasSubtasks, + allCompleted, + anyFailed, + subtaskCount: subtasks.length + }); + } + debugLog('[updateTaskFromPlan] Status computation:', { taskId, currentStatus: t.status, diff --git a/apps/frontend/src/shared/constants/phase-protocol.ts b/apps/frontend/src/shared/constants/phase-protocol.ts index 0748bb37..2d539eac 100644 --- a/apps/frontend/src/shared/constants/phase-protocol.ts +++ b/apps/frontend/src/shared/constants/phase-protocol.ts @@ -48,6 +48,12 @@ export const BACKEND_PHASES = [ export type ExecutionPhase = (typeof EXECUTION_PHASES)[number]; export type BackendPhase = (typeof BACKEND_PHASES)[number]; +/** + * Phases that can be completed and tracked in completedPhases array. + * Excludes 'idle', 'complete', and 'failed' which are not completable workflow phases. + */ +export type CompletablePhase = 'planning' | 'coding' | 'qa_review' | 'qa_fixing'; + /** * Phase ordering index for regression detection. * Higher index = later in the pipeline. @@ -112,3 +118,103 @@ export function isValidBackendPhase(value: string): value is BackendPhase { export function isValidExecutionPhase(value: string): value is ExecutionPhase { return (EXECUTION_PHASES as readonly string[]).includes(value); } + +/** + * FIX (ACS-203): Validate that a phase transition is valid based on completed phases. + * This prevents multiple phases from being active simultaneously. + * + * Phase transition rules: + * - 'idle' can transition to any phase + * - 'planning' can transition to 'coding' (once planning is in completedPhases) + * - 'coding' can transition to 'qa_review' (once coding is in completedPhases) + * - 'qa_review' can transition to 'qa_fixing' or 'complete' + * - 'qa_fixing' can transition to 'qa_review' or 'complete' + * - 'complete' and 'failed' are terminal (no transitions out) + * + * @param currentPhase - The current phase + * @param newPhase - The proposed new phase + * @param completedPhases - Array of phases that have completed + * @returns true if the transition is valid, false otherwise + */ +export function isValidPhaseTransition( + currentPhase: ExecutionPhase, + newPhase: ExecutionPhase, + completedPhases: CompletablePhase[] = [] +): boolean { + // Terminal phases can't transition to anything else + if (isTerminalPhase(currentPhase)) { + return false; + } + + // idle can transition to any active phase + if (currentPhase === 'idle') { + return BACKEND_PHASES.includes(newPhase as BackendPhase); + } + + // Same phase is always valid (progress update within phase) + if (currentPhase === newPhase) { + return true; + } + + // Define expected previous phases for each transition + const phasePrerequisites: Record = { + idle: [], + planning: [], + coding: ['planning'], + qa_review: ['coding'], + qa_fixing: ['qa_review'], + complete: ['qa_review', 'qa_fixing'], + failed: [] // Can enter failed from any phase + }; + + // Check if the prerequisite phase has been completed + const prerequisites = phasePrerequisites[newPhase]; + + // Special cases that don't require prerequisites: + // - Can go to failed from any phase (error handling) + // - Can go from qa_fixing back to qa_review (re-running QA after fixes) + if (newPhase === 'failed') { + return true; + } + if (currentPhase === 'qa_fixing' && newPhase === 'qa_review') { + return true; // Re-running QA after fixes + } + + // For all other transitions, verify prerequisites are met + if (prerequisites.length === 0) { + return true; // No prerequisites needed + } + + // Check if at least one prerequisite phase has been completed + const hasCompletedPrerequisite = prerequisites.some(p => completedPhases.includes(p)); + + if (!hasCompletedPrerequisite) { + console.warn(`[isValidPhaseTransition] Blocked transition ${currentPhase} -> ${newPhase}: prerequisite phases not completed`, { + required: prerequisites, + completed: completedPhases + }); + return false; + } + + return true; +} + +/** + * Get the expected previous phase for a given phase. + * Used to validate that phase transitions follow the expected workflow. + * + * @param phase - The phase to get the prerequisite for + * @returns The expected previous phase, or null if no prerequisite + */ +export function getExpectedPreviousPhase(phase: ExecutionPhase): ExecutionPhase | null { + const previousPhases: Record = { + idle: null, + planning: 'idle', + coding: 'planning', + qa_review: 'coding', + qa_fixing: 'qa_review', + complete: 'qa_review', + failed: null // Can fail from any phase + }; + return previousPhases[phase]; +} diff --git a/apps/frontend/src/shared/types/task.ts b/apps/frontend/src/shared/types/task.ts index c272402a..e3f89802 100644 --- a/apps/frontend/src/shared/types/task.ts +++ b/apps/frontend/src/shared/types/task.ts @@ -3,7 +3,7 @@ */ import type { ThinkingLevel, PhaseModelConfig, PhaseThinkingConfig } from './settings'; -import type { ExecutionPhase as ExecutionPhaseType } from '../constants/phase-protocol'; +import type { ExecutionPhase as ExecutionPhaseType, CompletablePhase } from '../constants/phase-protocol'; export type TaskStatus = 'backlog' | 'in_progress' | 'ai_review' | 'human_review' | 'pr_created' | 'done'; @@ -27,6 +27,10 @@ export interface ExecutionProgress { message?: string; // Current status message startedAt?: Date; sequenceNumber?: number; // Monotonically increasing counter to detect stale updates + // FIX (ACS-203): Track completed phases to prevent phase overlaps + // When a phase completes, it's added to this array before transitioning to the next phase + // This ensures that planning is marked complete before coding starts, etc. + completedPhases?: CompletablePhase[]; // Phases that have successfully completed } export interface Subtask {