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 <stillknotknown@users.noreply.github.com> * 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 <stillknotknown@users.noreply.github.com> --------- Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com> Co-authored-by: StillKnotKnown <stillknotknown@users.noreply.github.com> Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com>
This commit is contained in:
StillKnotKnown
@@ -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);
|
||||
|
||||
@@ -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';
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<TaskState>((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<TaskState>((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<TaskState>((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,
|
||||
|
||||
@@ -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<ExecutionPhase, CompletablePhase[]> = {
|
||||
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<ExecutionPhase, ExecutionPhase | null> = {
|
||||
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];
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user