fix(ACS-51, ACS-55, ACS-71): Fix Kanban state transitions and status flip-flop bug (#824)
* chore: update .gitignore to include auto-generated files and security logs - Added entries for .security-key and logs/security/ to ignore auto-generated files and security logs. * fix(ACS-51): prevent task workflow from halting after planning stage Root cause: Frontend accepted incomplete plan data (empty phases array) during spec creation, which overwrote subtask state and left tasks stuck. Changes: - Add validatePlanData() to reject incomplete plans in task-store - Add reloadPlanForIncompleteTask() hook for resume functionality - Enhance logging in project-store for plan loading diagnostics - Add comprehensive unit tests for plan validation edge cases - Add integration tests for task lifecycle IPC events - Add E2E test specs for full task workflow The fix ensures incomplete plans are rejected while the backend's validation/auto-fix pipeline completes, preserving UI state until valid data arrives. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(ACS-55, ACS-71): ensure Kanban state transitions render correctly ACS-55: Task card was showing "planning" even after moving to "coding" phase - Phase transitions now bypass the 16ms batching window and apply immediately - Added debug logging when sequence number checks drop out-of-order updates - This ensures intermediate phases (planning→coding→qa) are never coalesced ACS-71: Task immediately moved to Human Review with zero subtasks - Exit handler now checks if subtasks exist before moving to human_review - Added validateStatusTransition() function to prevent invalid state changes - Blocks human_review when no subtasks exist (task still in planning) - Blocks phase regression from coding back to planning Changes: - agent-events-handlers.ts: Added validation function, fixed exit handler - useIpc.ts: Phase changes bypass batching, apply immediately - task-store.ts: Added logging for dropped out-of-order updates 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: prevent status flip-flop between Human Review and AI Review When a task completed, `updateTaskFromPlan` would override the correct 'human_review' status with 'ai_review' when all subtasks were complete, causing tasks to flip between statuses on refresh. Root cause: The function only checked for "active" phases (planning, coding, qa_review, qa_fixing). When phase was 'complete' or 'idle', it would recalculate status from subtasks and set 'ai_review'. Fix: - Add 'complete' and 'failed' as terminal phases that skip recalculation - Respect explicit 'human_review' status from plan file - Never downgrade from 'human_review' to 'ai_review' This completes the Kanban state management fixes for ACS-51, ACS-55, ACS-71. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: add missing SubtaskStatus import to task-store The SubtaskStatus type was used but not imported, causing TypeScript compilation to fail in CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: use secure temp directories in tests to fix CodeQL alerts Replace hardcoded /tmp/ paths with mkdtempSync for secure temp directory creation. This prevents TOCTOU (time-of-check-time-of-use) attacks by using randomly generated directory names. Files fixed: - e2e/task-workflow.spec.ts - __tests__/integration/task-lifecycle.test.ts Resolves CodeQL "Insecure temporary file" high severity alerts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address PR review findings for Kanban state management - Fix reloadPlanForIncompleteTask to update Zustand store after reload - Extend flip-flop prevention to include pr_created and done statuses - Use wouldPhaseRegress() utility instead of hardcoded phase checks - Gate debug logging with debugLog utility for production - Fix unsafe type assertion for plan status - Remove redundant gitignore entry (logs/security/) - Add test coverage for terminal phase and status preservation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * chore: address follow-up PR review suggestions (5 LOW severity) - Add ExecutionPhase type cast after type guard check - Use crypto.randomUUID() for stronger subtask ID generation - Add optional chaining for defensive coding in useTaskDetail - Clarify comment about phase bypass batching behavior - Fix misleading test comment about human_review preservation - Update test regex to accept both UUID and fallback ID formats 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * chore: address final 3 LOW severity suggestions from CodeRabbit - Remove unused electronAPI variable in task-lifecycle test - Add comment explaining defensive fallback for description field - Rename test to clarify status recalculation skip behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Adam Slaker
@@ -165,3 +165,6 @@ _bmad-output/
|
||||
/docs
|
||||
OPUS_ANALYSIS_AND_IDEAS.md
|
||||
/.github/agents
|
||||
|
||||
# Auto Claude generated files
|
||||
.security-key
|
||||
|
||||
@@ -0,0 +1,318 @@
|
||||
# Root Cause Investigation: Task Workflow Halts After Planning Stage
|
||||
|
||||
## Investigation Summary
|
||||
|
||||
After adding comprehensive logging to the task loading and plan update pipeline, I've analyzed the data flow from backend to frontend to identify why subtasks fail to display after spec completion.
|
||||
|
||||
## Data Flow Analysis
|
||||
|
||||
### Current Architecture
|
||||
|
||||
```
|
||||
Backend (Python)
|
||||
↓
|
||||
Creates implementation_plan.json
|
||||
↓
|
||||
Emits IPC event: 'task:progress' with plan data
|
||||
↓
|
||||
Frontend (Electron Renderer)
|
||||
↓
|
||||
useIpc.ts: onTaskProgress handler (batched)
|
||||
↓
|
||||
task-store.ts: updateTaskFromPlan(taskId, plan)
|
||||
↓
|
||||
Creates subtasks from plan.phases.flatMap(phase => phase.subtasks)
|
||||
↓
|
||||
UI: TaskSubtasks.tsx renders subtasks
|
||||
```
|
||||
|
||||
### Critical Code Paths
|
||||
|
||||
**1. Plan Update Handler** (`apps/frontend/src/renderer/hooks/useIpc.ts:131-135`)
|
||||
```typescript
|
||||
window.electronAPI.onTaskProgress(
|
||||
(taskId: string, plan: ImplementationPlan) => {
|
||||
queueUpdate(taskId, { plan });
|
||||
}
|
||||
);
|
||||
```
|
||||
|
||||
**2. Subtask Creation** (`apps/frontend/src/renderer/stores/task-store.ts:124-133`)
|
||||
```typescript
|
||||
const subtasks: Subtask[] = plan.phases.flatMap((phase) =>
|
||||
phase.subtasks.map((subtask) => ({
|
||||
id: subtask.id,
|
||||
title: subtask.description,
|
||||
description: subtask.description,
|
||||
status: subtask.status,
|
||||
files: [],
|
||||
verification: subtask.verification as Subtask['verification']
|
||||
}))
|
||||
);
|
||||
```
|
||||
|
||||
**3. Initial Task Loading** (`apps/frontend/src/main/project-store.ts:461-470`)
|
||||
```typescript
|
||||
const subtasks = plan?.phases?.flatMap((phase) => {
|
||||
const items = phase.subtasks || (phase as { chunks?: PlanSubtask[] }).chunks || [];
|
||||
return items.map((subtask) => ({
|
||||
id: subtask.id,
|
||||
title: subtask.description,
|
||||
description: subtask.description,
|
||||
status: subtask.status,
|
||||
files: []
|
||||
}));
|
||||
}) || [];
|
||||
```
|
||||
|
||||
## Root Cause Identification
|
||||
|
||||
### Primary Root Cause: Early Plan Update Event with Empty Phases
|
||||
|
||||
**What's Happening:**
|
||||
|
||||
1. **Backend creates `implementation_plan.json` in stages:**
|
||||
- First writes the file with minimal structure: `{ "feature": "...", "phases": [] }`
|
||||
- Then adds phases and subtasks incrementally
|
||||
- Emits IPC event each time the plan is updated
|
||||
|
||||
2. **Frontend receives the FIRST plan update event:**
|
||||
- Plan has `feature` and basic metadata
|
||||
- **But `phases` array is EMPTY: `[]`**
|
||||
- `updateTaskFromPlan` is called with this incomplete plan
|
||||
- Subtasks are created as empty array: `plan.phases.flatMap(...)` → `[]`
|
||||
|
||||
3. **Later plan updates with full subtask data are ignored:**
|
||||
- When backend writes the complete plan with subtasks
|
||||
- Another IPC event is emitted
|
||||
- But due to race conditions or event handling issues, this update doesn't reach the frontend
|
||||
- Or it does reach but the task UI doesn't refresh
|
||||
|
||||
**Evidence from Code:**
|
||||
|
||||
Looking at `updateTaskFromPlan` (task-store.ts:106-190):
|
||||
- Line 108-114: Logs show `phases: plan.phases?.length || 0`
|
||||
- Line 112: If plan has 0 phases, `totalSubtasks` will be 0
|
||||
- Line 124-133: `plan.phases.flatMap(...)` on empty array creates `subtasks = []`
|
||||
- **No validation to check if plan is complete before updating state**
|
||||
|
||||
**Why "!" Indicators Appear:**
|
||||
|
||||
The "!" indicators likely come from the UI attempting to render subtasks when:
|
||||
- Subtask count shows as 18 (from later plan update metadata)
|
||||
- But `task.subtasks` array is actually empty `[]` (from early plan update)
|
||||
- This mismatch causes the UI to show warning indicators
|
||||
|
||||
### Secondary Contributing Factors
|
||||
|
||||
**A. No Plan Validation Before State Update**
|
||||
|
||||
Current code in `updateTaskFromPlan` immediately creates subtasks from whatever plan data it receives:
|
||||
```typescript
|
||||
const subtasks: Subtask[] = plan.phases.flatMap((phase) =>
|
||||
phase.subtasks.map((subtask) => ({ ... }))
|
||||
);
|
||||
```
|
||||
|
||||
**Problem:** No check if plan is "ready" or "complete" before updating state.
|
||||
|
||||
**B. Missing Reload Trigger After Spec Completion**
|
||||
|
||||
When spec creation completes and the full plan is written:
|
||||
- The IPC event might not fire again
|
||||
- Or the event fires but the batching mechanism drops it
|
||||
- Frontend state remains stuck with empty subtasks
|
||||
|
||||
**C. Race Condition in Batch Update Queue**
|
||||
|
||||
In `useIpc.ts:92-112`, the batching mechanism queues updates:
|
||||
```typescript
|
||||
function queueUpdate(taskId: string, update: BatchedUpdate): void {
|
||||
const existing = batchQueue.get(taskId) || {};
|
||||
batchQueue.set(taskId, { ...existing, ...update });
|
||||
}
|
||||
```
|
||||
|
||||
**Problem:** If two plan updates arrive within 16ms:
|
||||
- First update has empty phases: `{ plan: { phases: [] } }`
|
||||
- Second update has full phases: `{ plan: { phases: [...18 subtasks...] } }`
|
||||
- Second update **overwrites** first in the queue
|
||||
- But if order gets reversed, empty plan overwrites full plan
|
||||
|
||||
## Log Evidence to Look For
|
||||
|
||||
To confirm this root cause, check console logs for:
|
||||
|
||||
### 1. Plan Loading Sequence
|
||||
```
|
||||
[updateTaskFromPlan] called with plan:
|
||||
taskId: "xxx"
|
||||
feature: "..."
|
||||
phases: 0 ← SMOKING GUN: phases array is empty
|
||||
totalSubtasks: 0 ← No subtasks
|
||||
```
|
||||
|
||||
If you see `phases: 0` followed later by no update with `phases: 3` (or more), the early empty plan is stuck in state.
|
||||
|
||||
### 2. Multiple Plan Updates
|
||||
```
|
||||
[updateTaskFromPlan] called with plan:
|
||||
phases: 0
|
||||
totalSubtasks: 0
|
||||
|
||||
[updateTaskFromPlan] called with plan: ← This might never appear
|
||||
phases: 3
|
||||
totalSubtasks: 18
|
||||
```
|
||||
|
||||
If second log never appears, the plan update event isn't firing after spec completion.
|
||||
|
||||
### 3. Project Store Loading
|
||||
```
|
||||
[ProjectStore] Loading implementation_plan.json for spec: xxx
|
||||
[ProjectStore] Loaded plan for xxx:
|
||||
phaseCount: 0 ← Empty plan loaded from disk
|
||||
subtaskCount: 0
|
||||
```
|
||||
|
||||
If plan file on disk has empty phases, the issue is in backend plan writing.
|
||||
|
||||
### 4. Plan File Utils
|
||||
```
|
||||
[plan-file-utils] Reading implementation_plan.json to update status
|
||||
[plan-file-utils] Successfully persisted status ← Plan exists but might be incomplete
|
||||
```
|
||||
|
||||
Check if plan file reads/writes are happening during spec creation.
|
||||
|
||||
## Proposed Fix Approach
|
||||
|
||||
### Fix 1: Add Plan Completeness Validation (Immediate Fix)
|
||||
|
||||
**File:** `apps/frontend/src/renderer/stores/task-store.ts`
|
||||
|
||||
**Change:** Only update subtasks if plan has valid phases and subtasks:
|
||||
|
||||
```typescript
|
||||
updateTaskFromPlan: (taskId, plan) =>
|
||||
set((state) => {
|
||||
console.log('[updateTaskFromPlan] called with plan:', { ... });
|
||||
|
||||
const index = findTaskIndex(state.tasks, taskId);
|
||||
if (index === -1) {
|
||||
console.log('[updateTaskFromPlan] Task not found:', taskId);
|
||||
return state;
|
||||
}
|
||||
|
||||
// VALIDATION: Don't update if plan is incomplete
|
||||
if (!plan.phases || plan.phases.length === 0) {
|
||||
console.warn('[updateTaskFromPlan] Plan has no phases, skipping update:', taskId);
|
||||
return state; // Keep existing state, don't overwrite with empty data
|
||||
}
|
||||
|
||||
const totalSubtasks = plan.phases.reduce((acc, p) => acc + (p.subtasks?.length || 0), 0);
|
||||
if (totalSubtasks === 0) {
|
||||
console.warn('[updateTaskFromPlan] Plan has no subtasks, skipping update:', taskId);
|
||||
return state; // Keep existing state
|
||||
}
|
||||
|
||||
// ... rest of existing code to create subtasks ...
|
||||
})
|
||||
```
|
||||
|
||||
### Fix 2: Trigger Reload After Spec Completion (Comprehensive Fix)
|
||||
|
||||
**File:** `apps/frontend/src/renderer/hooks/useIpc.ts`
|
||||
|
||||
**Change:** Add explicit "spec completed" event handler that reloads the task:
|
||||
|
||||
```typescript
|
||||
// Add new IPC event listener
|
||||
const cleanupSpecComplete = window.electronAPI.onSpecComplete(
|
||||
async (taskId: string) => {
|
||||
console.log('[IPC] Spec completed for task:', taskId);
|
||||
// Force reload the task from disk to get the complete plan
|
||||
const task = useTaskStore.getState().tasks.find(t => t.id === taskId);
|
||||
if (task) {
|
||||
// Reload plan from file
|
||||
const result = await window.electronAPI.getTaskPlan(task.projectId, taskId);
|
||||
if (result.success && result.data) {
|
||||
updateTaskFromPlan(taskId, result.data);
|
||||
}
|
||||
}
|
||||
}
|
||||
);
|
||||
```
|
||||
|
||||
### Fix 3: Prevent Plan Overwrite in Batch Queue (Race Condition Fix)
|
||||
|
||||
**File:** `apps/frontend/src/renderer/hooks/useIpc.ts`
|
||||
|
||||
**Change:** Don't overwrite plan if incoming plan has fewer subtasks than existing:
|
||||
|
||||
```typescript
|
||||
function queueUpdate(taskId: string, update: BatchedUpdate): void {
|
||||
const existing = batchQueue.get(taskId) || {};
|
||||
|
||||
// For plan updates, only accept if it has MORE data than existing
|
||||
let mergedPlan = existing.plan;
|
||||
if (update.plan) {
|
||||
const existingSubtasks = existing.plan?.phases?.flatMap(p => p.subtasks || []).length || 0;
|
||||
const newSubtasks = update.plan.phases?.flatMap(p => p.subtasks || []).length || 0;
|
||||
|
||||
if (newSubtasks >= existingSubtasks) {
|
||||
mergedPlan = update.plan; // Accept new plan
|
||||
} else {
|
||||
console.warn('[IPC Batch] Rejecting plan update with fewer subtasks:',
|
||||
{ taskId, existing: existingSubtasks, new: newSubtasks });
|
||||
// Keep existing plan, don't overwrite with less complete data
|
||||
}
|
||||
}
|
||||
|
||||
// ... rest of existing code ...
|
||||
}
|
||||
```
|
||||
|
||||
## Testing the Fix
|
||||
|
||||
### Manual Verification Steps
|
||||
|
||||
1. **Create a new task** and move it to "In Progress"
|
||||
2. **Watch the console logs** for:
|
||||
```
|
||||
[updateTaskFromPlan] called with plan: { phases: 0, totalSubtasks: 0 }
|
||||
```
|
||||
3. **Wait for spec to complete** (planning phase finishes)
|
||||
4. **Check console logs** for:
|
||||
```
|
||||
[updateTaskFromPlan] called with plan: { phases: 3, totalSubtasks: 18 }
|
||||
```
|
||||
5. **Expand subtask list** in task card
|
||||
6. **Verify:** Subtasks display with full details, no "!" indicators
|
||||
|
||||
### Expected Outcome After Fix
|
||||
|
||||
- ✅ Empty/incomplete plan updates are ignored
|
||||
- ✅ Only complete plans with phases and subtasks update the UI
|
||||
- ✅ Subtasks display with id, description, and status
|
||||
- ✅ No "!" warning indicators
|
||||
- ✅ Subtask count shows "0/18 completed" (not "0/0")
|
||||
- ✅ Plan pulsing animation stops when spec completes
|
||||
- ✅ Resume functionality works without infinite loop
|
||||
|
||||
## Next Steps
|
||||
|
||||
1. ✅ **This Investigation** - Root cause identified (COMPLETE)
|
||||
2. 🔄 **Subtask 2-1** - Implement Fix 1 (validation in updateTaskFromPlan)
|
||||
3. 🔄 **Subtask 2-2** - Add data validation before subtask state updates
|
||||
4. 🔄 **Subtask 2-3** - Fix pulsing animation condition
|
||||
5. 🔄 **Subtask 2-4** - Fix resume logic to reload plan if subtasks missing
|
||||
6. 🔄 **Phase 3** - Add comprehensive tests to prevent regressions
|
||||
|
||||
## Conclusion
|
||||
|
||||
**Root Cause:** Frontend receives and accepts incomplete plan data (empty `phases` array) during the spec creation process, before subtasks are written. This overwrites any existing subtask data and leaves the UI in a stuck state with no subtasks to display.
|
||||
|
||||
**Fix Priority:** Implement Fix 1 (validation) immediately to prevent incomplete plans from updating state. This is a minimal, low-risk change that will resolve the core issue.
|
||||
|
||||
**Long-term Solution:** Add explicit event handling for spec completion (Fix 2) and improve batch queue logic (Fix 3) to make the system more robust against race conditions and out-of-order updates.
|
||||
@@ -0,0 +1,341 @@
|
||||
/**
|
||||
* End-to-End tests for full task workflow
|
||||
* Tests: create → spec → subtasks → resume
|
||||
*
|
||||
* NOTE: These tests require the Electron app to be built first.
|
||||
* Run `npm run build` before running E2E tests.
|
||||
*
|
||||
* To run: npx playwright test task-workflow --config=e2e/playwright.config.ts
|
||||
*/
|
||||
import { test, expect } from '@playwright/test';
|
||||
import { mkdirSync, mkdtempSync, rmSync, existsSync, writeFileSync, readFileSync } from 'fs';
|
||||
import { tmpdir } from 'os';
|
||||
import path from 'path';
|
||||
|
||||
// Test data directory - created securely with mkdtempSync to prevent TOCTOU attacks
|
||||
let TEST_DATA_DIR: string;
|
||||
let TEST_PROJECT_DIR: string;
|
||||
let SPECS_DIR: string;
|
||||
|
||||
// Setup test environment with secure temp directory
|
||||
function setupTestEnvironment(): void {
|
||||
// Create secure temp directory with random suffix
|
||||
TEST_DATA_DIR = mkdtempSync(path.join(tmpdir(), 'auto-claude-task-workflow-e2e-'));
|
||||
TEST_PROJECT_DIR = path.join(TEST_DATA_DIR, 'test-project');
|
||||
SPECS_DIR = path.join(TEST_PROJECT_DIR, '.auto-claude', 'specs');
|
||||
mkdirSync(TEST_PROJECT_DIR, { recursive: true });
|
||||
mkdirSync(SPECS_DIR, { recursive: true });
|
||||
}
|
||||
|
||||
// Cleanup test environment
|
||||
function cleanupTestEnvironment(): void {
|
||||
if (existsSync(TEST_DATA_DIR)) {
|
||||
rmSync(TEST_DATA_DIR, { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
|
||||
// Helper to create a task spec with subtasks
|
||||
function createTaskWithSubtasks(
|
||||
specId: string,
|
||||
subtaskStatuses: Array<'pending' | 'in_progress' | 'completed'>
|
||||
): void {
|
||||
const specDir = path.join(SPECS_DIR, specId);
|
||||
mkdirSync(specDir, { recursive: true });
|
||||
|
||||
// Create spec.md
|
||||
writeFileSync(
|
||||
path.join(specDir, 'spec.md'),
|
||||
`# ${specId}\n\n## Overview\n\nTest task for workflow validation.\n\n## Acceptance Criteria\n\n- [ ] All subtasks completed\n- [ ] Tests pass\n`
|
||||
);
|
||||
|
||||
// Create requirements.json
|
||||
writeFileSync(
|
||||
path.join(specDir, 'requirements.json'),
|
||||
JSON.stringify(
|
||||
{
|
||||
task_description: `Test task ${specId}`,
|
||||
user_requirements: ['Requirement 1', 'Requirement 2'],
|
||||
acceptance_criteria: ['All subtasks completed', 'Tests pass'],
|
||||
context: []
|
||||
},
|
||||
null,
|
||||
2
|
||||
)
|
||||
);
|
||||
|
||||
// Create implementation_plan.json with subtasks
|
||||
const subtasks = subtaskStatuses.map((status, index) => ({
|
||||
id: `subtask-${index + 1}`,
|
||||
phase: 'Implementation',
|
||||
service: 'backend',
|
||||
description: `Subtask ${index + 1}: Implement feature part ${index + 1}`,
|
||||
files_to_modify: [`src/file${index + 1}.py`],
|
||||
files_to_create: [],
|
||||
pattern_files: [],
|
||||
verification_command: 'pytest tests/',
|
||||
status: status,
|
||||
notes: status === 'completed' ? 'Completed successfully' : ''
|
||||
}));
|
||||
|
||||
writeFileSync(
|
||||
path.join(specDir, 'implementation_plan.json'),
|
||||
JSON.stringify(
|
||||
{
|
||||
feature: `Test Feature ${specId}`,
|
||||
workflow_type: 'feature',
|
||||
services_involved: ['backend'],
|
||||
subtasks: subtasks,
|
||||
final_acceptance: ['All subtasks completed', 'Tests pass'],
|
||||
created_at: new Date().toISOString(),
|
||||
updated_at: new Date().toISOString(),
|
||||
spec_file: 'spec.md'
|
||||
},
|
||||
null,
|
||||
2
|
||||
)
|
||||
);
|
||||
|
||||
// Create build-progress.txt
|
||||
writeFileSync(
|
||||
path.join(specDir, 'build-progress.txt'),
|
||||
`Task Progress: ${specId}\n\nSubtasks: ${subtasks.length}\nCompleted: ${subtasks.filter(s => s.status === 'completed').length}\n`
|
||||
);
|
||||
}
|
||||
|
||||
// Helper to simulate task resumption
|
||||
function simulateTaskResume(specId: string): void {
|
||||
const planPath = path.join(SPECS_DIR, specId, 'implementation_plan.json');
|
||||
const plan = JSON.parse(readFileSync(planPath, 'utf-8'));
|
||||
|
||||
// Find first pending subtask and mark as in_progress
|
||||
const pendingSubtask = plan.subtasks.find((st: { status: string }) => st.status === 'pending');
|
||||
if (pendingSubtask) {
|
||||
pendingSubtask.status = 'in_progress';
|
||||
pendingSubtask.notes = 'Resumed from checkpoint';
|
||||
}
|
||||
|
||||
plan.updated_at = new Date().toISOString();
|
||||
writeFileSync(planPath, JSON.stringify(plan, null, 2));
|
||||
}
|
||||
|
||||
test.describe('Task Workflow E2E Tests', () => {
|
||||
test.beforeAll(() => {
|
||||
setupTestEnvironment();
|
||||
});
|
||||
|
||||
test.afterAll(() => {
|
||||
cleanupTestEnvironment();
|
||||
});
|
||||
|
||||
test('should create task directory structure', () => {
|
||||
const specId = '001-test-task';
|
||||
const specDir = path.join(SPECS_DIR, specId);
|
||||
mkdirSync(specDir, { recursive: true });
|
||||
|
||||
// Verify directory created
|
||||
expect(existsSync(specDir)).toBe(true);
|
||||
});
|
||||
|
||||
test('should generate spec.md file', () => {
|
||||
const specId = '002-task-with-spec';
|
||||
const specDir = path.join(SPECS_DIR, specId);
|
||||
mkdirSync(specDir, { recursive: true });
|
||||
|
||||
// Write spec
|
||||
const specContent = '# Test Task\n\n## Overview\n\nThis is a test task.\n';
|
||||
writeFileSync(path.join(specDir, 'spec.md'), specContent);
|
||||
|
||||
// Verify spec file
|
||||
expect(existsSync(path.join(specDir, 'spec.md'))).toBe(true);
|
||||
const content = readFileSync(path.join(specDir, 'spec.md'), 'utf-8');
|
||||
expect(content).toContain('Test Task');
|
||||
});
|
||||
|
||||
test('should create implementation plan with subtasks', () => {
|
||||
const specId = '003-task-with-subtasks';
|
||||
createTaskWithSubtasks(specId, ['pending', 'pending', 'pending']);
|
||||
|
||||
const planPath = path.join(SPECS_DIR, specId, 'implementation_plan.json');
|
||||
expect(existsSync(planPath)).toBe(true);
|
||||
|
||||
const plan = JSON.parse(readFileSync(planPath, 'utf-8'));
|
||||
expect(plan.subtasks).toBeDefined();
|
||||
expect(plan.subtasks.length).toBe(3);
|
||||
expect(plan.subtasks[0].status).toBe('pending');
|
||||
});
|
||||
|
||||
test('should track subtask progress', () => {
|
||||
const specId = '004-task-in-progress';
|
||||
createTaskWithSubtasks(specId, ['completed', 'in_progress', 'pending']);
|
||||
|
||||
const planPath = path.join(SPECS_DIR, specId, 'implementation_plan.json');
|
||||
const plan = JSON.parse(readFileSync(planPath, 'utf-8'));
|
||||
|
||||
expect(plan.subtasks[0].status).toBe('completed');
|
||||
expect(plan.subtasks[1].status).toBe('in_progress');
|
||||
expect(plan.subtasks[2].status).toBe('pending');
|
||||
});
|
||||
|
||||
test('should resume task from checkpoint', () => {
|
||||
const specId = '005-task-resume';
|
||||
createTaskWithSubtasks(specId, ['completed', 'pending', 'pending']);
|
||||
|
||||
// Verify initial state
|
||||
let plan = JSON.parse(readFileSync(path.join(SPECS_DIR, specId, 'implementation_plan.json'), 'utf-8'));
|
||||
expect(plan.subtasks[1].status).toBe('pending');
|
||||
|
||||
// Simulate resume
|
||||
simulateTaskResume(specId);
|
||||
|
||||
// Verify resumed state
|
||||
plan = JSON.parse(readFileSync(path.join(SPECS_DIR, specId, 'implementation_plan.json'), 'utf-8'));
|
||||
expect(plan.subtasks[1].status).toBe('in_progress');
|
||||
expect(plan.subtasks[1].notes).toContain('Resumed from checkpoint');
|
||||
});
|
||||
|
||||
test('should complete all subtasks in sequence', () => {
|
||||
const specId = '006-task-completion';
|
||||
createTaskWithSubtasks(specId, ['completed', 'completed', 'completed']);
|
||||
|
||||
const plan = JSON.parse(readFileSync(path.join(SPECS_DIR, specId, 'implementation_plan.json'), 'utf-8'));
|
||||
const allCompleted = plan.subtasks.every((st: { status: string }) => st.status === 'completed');
|
||||
|
||||
expect(allCompleted).toBe(true);
|
||||
});
|
||||
|
||||
test('should maintain build progress log', () => {
|
||||
const specId = '007-task-with-progress';
|
||||
createTaskWithSubtasks(specId, ['completed', 'in_progress', 'pending']);
|
||||
|
||||
const progressPath = path.join(SPECS_DIR, specId, 'build-progress.txt');
|
||||
expect(existsSync(progressPath)).toBe(true);
|
||||
|
||||
const progressContent = readFileSync(progressPath, 'utf-8');
|
||||
expect(progressContent).toContain('Task Progress');
|
||||
expect(progressContent).toContain('Subtasks: 3');
|
||||
});
|
||||
});
|
||||
|
||||
test.describe('Full Task Workflow Integration', () => {
|
||||
test.beforeAll(() => {
|
||||
setupTestEnvironment();
|
||||
});
|
||||
|
||||
test.afterAll(() => {
|
||||
cleanupTestEnvironment();
|
||||
});
|
||||
|
||||
test('should complete full workflow: create → spec → subtasks → resume → complete', () => {
|
||||
const specId = '100-full-workflow';
|
||||
|
||||
// Step 1: Create task
|
||||
const specDir = path.join(SPECS_DIR, specId);
|
||||
mkdirSync(specDir, { recursive: true });
|
||||
expect(existsSync(specDir)).toBe(true);
|
||||
|
||||
// Step 2: Generate spec
|
||||
writeFileSync(
|
||||
path.join(specDir, 'spec.md'),
|
||||
'# Full Workflow Test\n\n## Overview\n\nComplete workflow test.\n'
|
||||
);
|
||||
expect(existsSync(path.join(specDir, 'spec.md'))).toBe(true);
|
||||
|
||||
// Step 3: Create subtasks
|
||||
createTaskWithSubtasks(specId, ['pending', 'pending', 'pending']);
|
||||
let plan = JSON.parse(readFileSync(path.join(specDir, 'implementation_plan.json'), 'utf-8'));
|
||||
expect(plan.subtasks.length).toBe(3);
|
||||
|
||||
// Step 4: Start first subtask
|
||||
plan.subtasks[0].status = 'in_progress';
|
||||
writeFileSync(path.join(specDir, 'implementation_plan.json'), JSON.stringify(plan, null, 2));
|
||||
|
||||
plan = JSON.parse(readFileSync(path.join(specDir, 'implementation_plan.json'), 'utf-8'));
|
||||
expect(plan.subtasks[0].status).toBe('in_progress');
|
||||
|
||||
// Step 5: Complete first subtask
|
||||
plan.subtasks[0].status = 'completed';
|
||||
plan.subtasks[0].notes = 'First subtask completed';
|
||||
writeFileSync(path.join(specDir, 'implementation_plan.json'), JSON.stringify(plan, null, 2));
|
||||
|
||||
// Step 6: Resume with second subtask
|
||||
simulateTaskResume(specId);
|
||||
plan = JSON.parse(readFileSync(path.join(specDir, 'implementation_plan.json'), 'utf-8'));
|
||||
expect(plan.subtasks[1].status).toBe('in_progress');
|
||||
|
||||
// Step 7: Complete remaining subtasks
|
||||
plan.subtasks[1].status = 'completed';
|
||||
plan.subtasks[2].status = 'completed';
|
||||
writeFileSync(path.join(specDir, 'implementation_plan.json'), JSON.stringify(plan, null, 2));
|
||||
|
||||
// Step 8: Verify all completed
|
||||
plan = JSON.parse(readFileSync(path.join(specDir, 'implementation_plan.json'), 'utf-8'));
|
||||
const allCompleted = plan.subtasks.every((st: { status: string }) => st.status === 'completed');
|
||||
expect(allCompleted).toBe(true);
|
||||
|
||||
// Step 9: Verify final state
|
||||
expect(plan.subtasks[0].notes).toContain('First subtask completed');
|
||||
expect(plan.subtasks[1].notes).toContain('Resumed from checkpoint');
|
||||
});
|
||||
|
||||
test('should handle workflow interruption and recovery', () => {
|
||||
const specId = '101-workflow-recovery';
|
||||
|
||||
// Create task with partial progress
|
||||
createTaskWithSubtasks(specId, ['completed', 'in_progress', 'pending']);
|
||||
|
||||
// Simulate interruption (task status is saved)
|
||||
const planPath = path.join(SPECS_DIR, specId, 'implementation_plan.json');
|
||||
let plan = JSON.parse(readFileSync(planPath, 'utf-8'));
|
||||
expect(plan.subtasks[1].status).toBe('in_progress');
|
||||
|
||||
// Simulate recovery: complete interrupted subtask
|
||||
plan.subtasks[1].status = 'completed';
|
||||
plan.subtasks[1].notes = 'Recovered and completed';
|
||||
writeFileSync(planPath, JSON.stringify(plan, null, 2));
|
||||
|
||||
// Resume with next subtask
|
||||
simulateTaskResume(specId);
|
||||
plan = JSON.parse(readFileSync(planPath, 'utf-8'));
|
||||
|
||||
// Verify recovery successful
|
||||
expect(plan.subtasks[1].status).toBe('completed');
|
||||
expect(plan.subtasks[2].status).toBe('in_progress');
|
||||
});
|
||||
|
||||
test('should validate workflow data integrity', () => {
|
||||
const specId = '102-data-integrity';
|
||||
createTaskWithSubtasks(specId, ['pending', 'pending', 'pending']);
|
||||
|
||||
const specDir = path.join(SPECS_DIR, specId);
|
||||
|
||||
// Verify all required files exist
|
||||
expect(existsSync(path.join(specDir, 'spec.md'))).toBe(true);
|
||||
expect(existsSync(path.join(specDir, 'requirements.json'))).toBe(true);
|
||||
expect(existsSync(path.join(specDir, 'implementation_plan.json'))).toBe(true);
|
||||
expect(existsSync(path.join(specDir, 'build-progress.txt'))).toBe(true);
|
||||
|
||||
// Verify data structure integrity
|
||||
const requirements = JSON.parse(readFileSync(path.join(specDir, 'requirements.json'), 'utf-8'));
|
||||
expect(requirements.task_description).toBeDefined();
|
||||
expect(requirements.acceptance_criteria).toBeDefined();
|
||||
|
||||
const plan = JSON.parse(readFileSync(path.join(specDir, 'implementation_plan.json'), 'utf-8'));
|
||||
expect(plan.feature).toBeDefined();
|
||||
expect(plan.subtasks).toBeDefined();
|
||||
expect(plan.created_at).toBeDefined();
|
||||
expect(plan.updated_at).toBeDefined();
|
||||
|
||||
// Verify subtask structure
|
||||
plan.subtasks.forEach((subtask: {
|
||||
id: string;
|
||||
description: string;
|
||||
status: string;
|
||||
verification_command: string;
|
||||
}) => {
|
||||
expect(subtask.id).toBeDefined();
|
||||
expect(subtask.description).toBeDefined();
|
||||
expect(subtask.status).toMatch(/^(pending|in_progress|completed)$/);
|
||||
expect(subtask.verification_command).toBeDefined();
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,382 @@
|
||||
/**
|
||||
* Integration tests for task lifecycle
|
||||
* Tests spec completion to subtask loading workflow (IPC communication)
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
|
||||
import { mkdirSync, mkdtempSync, writeFileSync, rmSync, existsSync } from 'fs';
|
||||
import { tmpdir } from 'os';
|
||||
import path from 'path';
|
||||
|
||||
// Test directories - created securely with mkdtempSync to prevent TOCTOU attacks
|
||||
let TEST_DIR: string;
|
||||
let TEST_PROJECT_PATH: string;
|
||||
let TEST_SPEC_DIR: string;
|
||||
|
||||
// Mock ipcRenderer for renderer-side tests
|
||||
const mockIpcRenderer = {
|
||||
invoke: vi.fn(),
|
||||
send: vi.fn(),
|
||||
on: vi.fn(),
|
||||
once: vi.fn(),
|
||||
removeListener: vi.fn(),
|
||||
removeAllListeners: vi.fn(),
|
||||
setMaxListeners: vi.fn()
|
||||
};
|
||||
|
||||
// Mock contextBridge
|
||||
const exposedApis: Record<string, unknown> = {};
|
||||
const mockContextBridge = {
|
||||
exposeInMainWorld: vi.fn((name: string, api: unknown) => {
|
||||
exposedApis[name] = api;
|
||||
})
|
||||
};
|
||||
|
||||
vi.mock('electron', () => ({
|
||||
ipcRenderer: mockIpcRenderer,
|
||||
contextBridge: mockContextBridge
|
||||
}));
|
||||
|
||||
// Sample implementation plan with subtasks
|
||||
function createTestPlan(overrides: Record<string, unknown> = {}): object {
|
||||
return {
|
||||
feature: 'Test Feature',
|
||||
workflow_type: 'feature',
|
||||
services_involved: ['frontend'],
|
||||
phases: [
|
||||
{
|
||||
id: 'phase-1',
|
||||
name: 'Implementation Phase',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{
|
||||
id: 'subtask-1-1',
|
||||
description: 'Implement feature A',
|
||||
status: 'pending',
|
||||
files_to_modify: ['file1.ts'],
|
||||
files_to_create: [],
|
||||
service: 'frontend'
|
||||
},
|
||||
{
|
||||
id: 'subtask-1-2',
|
||||
description: 'Add unit tests for feature A',
|
||||
status: 'pending',
|
||||
files_to_modify: [],
|
||||
files_to_create: ['file1.test.ts'],
|
||||
service: 'frontend'
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
status: 'in_progress',
|
||||
planStatus: 'in_progress',
|
||||
created_at: new Date().toISOString(),
|
||||
updated_at: new Date().toISOString(),
|
||||
...overrides
|
||||
};
|
||||
}
|
||||
|
||||
// Sample implementation plan with empty phases (incomplete state)
|
||||
function createIncompletePlan(): object {
|
||||
return {
|
||||
feature: 'Test Feature',
|
||||
workflow_type: 'feature',
|
||||
services_involved: ['frontend'],
|
||||
phases: [],
|
||||
status: 'planning',
|
||||
planStatus: 'planning',
|
||||
created_at: new Date().toISOString(),
|
||||
updated_at: new Date().toISOString()
|
||||
};
|
||||
}
|
||||
|
||||
// Setup test directories with secure temp directory
|
||||
function setupTestDirs(): void {
|
||||
// Create secure temp directory with random suffix
|
||||
TEST_DIR = mkdtempSync(path.join(tmpdir(), 'task-lifecycle-test-'));
|
||||
TEST_PROJECT_PATH = path.join(TEST_DIR, 'test-project');
|
||||
TEST_SPEC_DIR = path.join(TEST_PROJECT_PATH, '.auto-claude/specs/001-test-feature');
|
||||
mkdirSync(TEST_SPEC_DIR, { recursive: true });
|
||||
}
|
||||
|
||||
// Cleanup test directories
|
||||
function cleanupTestDirs(): void {
|
||||
if (TEST_DIR && existsSync(TEST_DIR)) {
|
||||
rmSync(TEST_DIR, { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
|
||||
describe('Task Lifecycle Integration', () => {
|
||||
beforeEach(async () => {
|
||||
cleanupTestDirs();
|
||||
setupTestDirs();
|
||||
vi.clearAllMocks();
|
||||
vi.resetModules();
|
||||
Object.keys(exposedApis).forEach((key) => delete exposedApis[key]);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanupTestDirs();
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
describe('Spec completion to subtask loading', () => {
|
||||
it('should load subtasks from implementation_plan.json after spec completion', async () => {
|
||||
// Create implementation_plan.json with full subtask data
|
||||
const planPath = path.join(TEST_SPEC_DIR, 'implementation_plan.json');
|
||||
const plan = createTestPlan();
|
||||
writeFileSync(planPath, JSON.stringify(plan, null, 2));
|
||||
|
||||
// Import preload script to get electronAPI
|
||||
await import('../../preload/index');
|
||||
const electronAPI = exposedApis['electronAPI'] as Record<string, unknown>;
|
||||
|
||||
// Mock IPC response for getTasks (loads implementation_plan.json)
|
||||
mockIpcRenderer.invoke.mockResolvedValueOnce({
|
||||
success: true,
|
||||
data: [
|
||||
{
|
||||
id: 'task-001',
|
||||
name: 'Test Feature',
|
||||
status: 'spec_complete',
|
||||
specDir: TEST_SPEC_DIR,
|
||||
plan: plan
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
// Call getTasks to load plan data
|
||||
const getTasks = electronAPI['getTasks'] as (projectId: string) => Promise<unknown>;
|
||||
const result = await getTasks('project-id');
|
||||
|
||||
// Verify IPC invocation
|
||||
expect(mockIpcRenderer.invoke).toHaveBeenCalledWith('task:list', 'project-id');
|
||||
|
||||
// Verify task data includes plan with subtasks
|
||||
expect(result).toMatchObject({
|
||||
success: true,
|
||||
data: expect.arrayContaining([
|
||||
expect.objectContaining({
|
||||
plan: expect.objectContaining({
|
||||
phases: expect.arrayContaining([
|
||||
expect.objectContaining({
|
||||
subtasks: expect.arrayContaining([
|
||||
expect.objectContaining({
|
||||
id: 'subtask-1-1',
|
||||
description: 'Implement feature A',
|
||||
status: 'pending'
|
||||
}),
|
||||
expect.objectContaining({
|
||||
id: 'subtask-1-2',
|
||||
description: 'Add unit tests for feature A',
|
||||
status: 'pending'
|
||||
})
|
||||
])
|
||||
})
|
||||
])
|
||||
})
|
||||
})
|
||||
])
|
||||
});
|
||||
});
|
||||
|
||||
it('should handle incomplete plan data with empty phases array', async () => {
|
||||
// Create implementation_plan.json with incomplete data (empty phases)
|
||||
const planPath = path.join(TEST_SPEC_DIR, 'implementation_plan.json');
|
||||
const incompletePlan = createIncompletePlan();
|
||||
writeFileSync(planPath, JSON.stringify(incompletePlan, null, 2));
|
||||
|
||||
await import('../../preload/index');
|
||||
const electronAPI = exposedApis['electronAPI'] as Record<string, unknown>;
|
||||
|
||||
// Mock IPC response for getTasks
|
||||
mockIpcRenderer.invoke.mockResolvedValueOnce({
|
||||
success: true,
|
||||
data: [
|
||||
{
|
||||
id: 'task-001',
|
||||
name: 'Test Feature',
|
||||
status: 'planning',
|
||||
specDir: TEST_SPEC_DIR,
|
||||
plan: incompletePlan
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
const getTasks = electronAPI['getTasks'] as (projectId: string) => Promise<unknown>;
|
||||
const result = await getTasks('project-id');
|
||||
|
||||
// Verify task data reflects incomplete state
|
||||
expect(result).toMatchObject({
|
||||
success: true,
|
||||
data: expect.arrayContaining([
|
||||
expect.objectContaining({
|
||||
plan: expect.objectContaining({
|
||||
phases: [],
|
||||
status: 'planning'
|
||||
})
|
||||
})
|
||||
])
|
||||
});
|
||||
});
|
||||
|
||||
it('should emit task:statusChange event when task transitions from planning to spec_complete', async () => {
|
||||
await import('../../preload/index');
|
||||
const electronAPI = exposedApis['electronAPI'] as Record<string, unknown>;
|
||||
|
||||
// Setup event listener
|
||||
const callback = vi.fn();
|
||||
const onTaskStatusChange = electronAPI['onTaskStatusChange'] as (cb: Function) => Function;
|
||||
onTaskStatusChange(callback);
|
||||
|
||||
// Verify listener was registered
|
||||
expect(mockIpcRenderer.on).toHaveBeenCalledWith(
|
||||
'task:statusChange',
|
||||
expect.any(Function)
|
||||
);
|
||||
|
||||
// Simulate status change event from main process
|
||||
// The event handler signature is: (_event, taskId, status)
|
||||
const eventHandler = mockIpcRenderer.on.mock.calls.find(
|
||||
(call) => call[0] === 'task:statusChange'
|
||||
)?.[1];
|
||||
|
||||
if (eventHandler) {
|
||||
eventHandler({}, 'task-001', 'spec_complete');
|
||||
}
|
||||
|
||||
// Verify callback was invoked with correct parameters (taskId, status, projectId)
|
||||
// Note: projectId is optional and undefined when not provided
|
||||
expect(callback).toHaveBeenCalledWith('task-001', 'spec_complete', undefined);
|
||||
});
|
||||
|
||||
it('should emit task:progress event with updated plan during spec creation', async () => {
|
||||
await import('../../preload/index');
|
||||
const electronAPI = exposedApis['electronAPI'] as Record<string, unknown>;
|
||||
|
||||
// Setup event listener
|
||||
const callback = vi.fn();
|
||||
const onTaskProgress = electronAPI['onTaskProgress'] as (cb: Function) => Function;
|
||||
onTaskProgress(callback);
|
||||
|
||||
// Verify listener was registered
|
||||
expect(mockIpcRenderer.on).toHaveBeenCalledWith(
|
||||
'task:progress',
|
||||
expect.any(Function)
|
||||
);
|
||||
|
||||
// Simulate progress event with plan update
|
||||
// The event handler signature is: (_event, taskId, plan)
|
||||
const eventHandler = mockIpcRenderer.on.mock.calls.find(
|
||||
(call) => call[0] === 'task:progress'
|
||||
)?.[1];
|
||||
|
||||
const plan = createTestPlan();
|
||||
if (eventHandler) {
|
||||
eventHandler({}, 'task-001', plan);
|
||||
}
|
||||
|
||||
// Verify callback was invoked with correct parameters (taskId, plan, projectId)
|
||||
// Note: projectId is optional and undefined when not provided
|
||||
expect(callback).toHaveBeenCalledWith(
|
||||
'task-001',
|
||||
expect.objectContaining({
|
||||
phases: expect.arrayContaining([
|
||||
expect.objectContaining({
|
||||
subtasks: expect.any(Array)
|
||||
})
|
||||
])
|
||||
}),
|
||||
undefined
|
||||
);
|
||||
});
|
||||
|
||||
it('should handle task resume by reloading implementation plan', async () => {
|
||||
// Create implementation_plan.json
|
||||
const planPath = path.join(TEST_SPEC_DIR, 'implementation_plan.json');
|
||||
const plan = createTestPlan();
|
||||
writeFileSync(planPath, JSON.stringify(plan, null, 2));
|
||||
|
||||
await import('../../preload/index');
|
||||
const electronAPI = exposedApis['electronAPI'] as Record<string, unknown>;
|
||||
|
||||
// Mock IPC response for task start (resume)
|
||||
mockIpcRenderer.invoke.mockResolvedValueOnce({
|
||||
success: true,
|
||||
message: 'Task resumed'
|
||||
});
|
||||
|
||||
// Call startTask (resume)
|
||||
const startTask = electronAPI['startTask'] as (id: string, options?: object) => void;
|
||||
startTask('task-001', { resume: true });
|
||||
|
||||
// Verify IPC send was called
|
||||
expect(mockIpcRenderer.send).toHaveBeenCalledWith(
|
||||
'task:start',
|
||||
'task-001',
|
||||
{ resume: true }
|
||||
);
|
||||
});
|
||||
|
||||
it('should handle task update status IPC call', async () => {
|
||||
await import('../../preload/index');
|
||||
// Note: electronAPI is exposed but we test the IPC channel directly below
|
||||
|
||||
// Check if updateTaskStatus method exists (might be part of updateTask)
|
||||
// Based on IPC_CHANNELS, we have TASK_UPDATE_STATUS
|
||||
mockIpcRenderer.invoke.mockResolvedValueOnce({
|
||||
success: true
|
||||
});
|
||||
|
||||
// Since updateTaskStatus might not be directly exposed, we test the IPC channel directly
|
||||
const result = await mockIpcRenderer.invoke('task:updateStatus', 'task-001', 'in_progress');
|
||||
|
||||
expect(mockIpcRenderer.invoke).toHaveBeenCalledWith(
|
||||
'task:updateStatus',
|
||||
'task-001',
|
||||
'in_progress'
|
||||
);
|
||||
expect(result).toMatchObject({ success: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe('Event listener cleanup', () => {
|
||||
it('should cleanup task:progress listener when cleanup function is called', async () => {
|
||||
await import('../../preload/index');
|
||||
const electronAPI = exposedApis['electronAPI'] as Record<string, unknown>;
|
||||
|
||||
const callback = vi.fn();
|
||||
const onTaskProgress = electronAPI['onTaskProgress'] as (cb: Function) => Function;
|
||||
const cleanup = onTaskProgress(callback);
|
||||
|
||||
expect(typeof cleanup).toBe('function');
|
||||
|
||||
// Call cleanup
|
||||
cleanup();
|
||||
|
||||
expect(mockIpcRenderer.removeListener).toHaveBeenCalledWith(
|
||||
'task:progress',
|
||||
expect.any(Function)
|
||||
);
|
||||
});
|
||||
|
||||
it('should cleanup task:statusChange listener when cleanup function is called', async () => {
|
||||
await import('../../preload/index');
|
||||
const electronAPI = exposedApis['electronAPI'] as Record<string, unknown>;
|
||||
|
||||
const callback = vi.fn();
|
||||
const onTaskStatusChange = electronAPI['onTaskStatusChange'] as (cb: Function) => Function;
|
||||
const cleanup = onTaskStatusChange(callback);
|
||||
|
||||
expect(typeof cleanup).toBe('function');
|
||||
|
||||
// Call cleanup
|
||||
cleanup();
|
||||
|
||||
expect(mockIpcRenderer.removeListener).toHaveBeenCalledWith(
|
||||
'task:statusChange',
|
||||
expect.any(Function)
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
});
|
||||
@@ -2,6 +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 type {
|
||||
SDKRateLimitInfo,
|
||||
Task,
|
||||
@@ -20,6 +21,54 @@ import { findTaskWorktree } from '../worktree-paths';
|
||||
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.
|
||||
*
|
||||
* @param task - The current task (may be undefined if not found)
|
||||
* @param newStatus - The proposed new status
|
||||
* @param phase - The execution phase that triggered this transition
|
||||
* @returns true if transition is valid, false if it should be blocked
|
||||
*/
|
||||
function validateStatusTransition(
|
||||
task: Task | undefined,
|
||||
newStatus: TaskStatus,
|
||||
phase: string
|
||||
): boolean {
|
||||
// Can't validate without task data - allow the transition
|
||||
if (!task) return true;
|
||||
|
||||
// Don't allow human_review without subtasks
|
||||
// This prevents tasks from jumping to review before planning is complete
|
||||
if (newStatus === 'human_review' && (!task.subtasks || task.subtasks.length === 0)) {
|
||||
console.warn(`[validateStatusTransition] Blocking human_review - task ${task.id} has no subtasks (phase: ${phase})`);
|
||||
return false;
|
||||
}
|
||||
|
||||
// FIX (PR Review): Use comprehensive phase regression check instead of hardcoded checks
|
||||
// This handles all phase regressions (qa_review→coding, complete→coding, etc.)
|
||||
// not just the specific coding→planning case
|
||||
const currentPhase = task.executionProgress?.phase;
|
||||
if (currentPhase && isValidExecutionPhase(currentPhase) && isValidExecutionPhase(phase)) {
|
||||
// Block transitions from terminal phases (complete/failed)
|
||||
if (isTerminalPhase(currentPhase)) {
|
||||
console.warn(`[validateStatusTransition] Blocking transition from terminal phase: ${currentPhase} for task ${task.id}`);
|
||||
return false;
|
||||
}
|
||||
|
||||
// Block any phase regression (going backwards in the workflow)
|
||||
// Note: Cast phase to ExecutionPhase since isValidExecutionPhase() type guard doesn't narrow through function calls
|
||||
if (wouldPhaseRegress(currentPhase, phase as ExecutionPhase)) {
|
||||
console.warn(`[validateStatusTransition] Blocking phase regression: ${currentPhase} -> ${phase} for task ${task.id}`);
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Register all agent-events-related IPC handlers
|
||||
*/
|
||||
@@ -150,13 +199,16 @@ export function registerAgenteventsHandlers(
|
||||
|
||||
// Fallback: Ensure status is updated even if COMPLETE phase event was missed
|
||||
// This prevents tasks from getting stuck in ai_review status
|
||||
// Uses inverted logic to also handle tasks with no subtasks (treats them as complete)
|
||||
// FIX (ACS-71): Only move to human_review if subtasks exist AND are all completed
|
||||
// If no subtasks exist, the task is still in planning and shouldn't move to human_review
|
||||
const isActiveStatus = task.status === 'in_progress' || task.status === 'ai_review';
|
||||
const hasIncompleteSubtasks = task.subtasks && task.subtasks.length > 0 &&
|
||||
const hasSubtasks = task.subtasks && task.subtasks.length > 0;
|
||||
const hasIncompleteSubtasks = hasSubtasks &&
|
||||
task.subtasks.some((s) => s.status !== 'completed');
|
||||
|
||||
if (isActiveStatus && !hasIncompleteSubtasks) {
|
||||
console.warn(`[Task ${taskId}] Fallback: Moving to human_review (process exited successfully)`);
|
||||
if (isActiveStatus && hasSubtasks && !hasIncompleteSubtasks) {
|
||||
// All subtasks completed - safe to move to human_review
|
||||
console.warn(`[Task ${taskId}] Fallback: Moving to human_review (process exited successfully, all ${task.subtasks.length} subtasks completed)`);
|
||||
persistStatus('human_review');
|
||||
// Include projectId for multi-project filtering (issue #723)
|
||||
mainWindow.webContents.send(
|
||||
@@ -165,6 +217,10 @@ export function registerAgenteventsHandlers(
|
||||
'human_review' as TaskStatus,
|
||||
projectId
|
||||
);
|
||||
} else if (isActiveStatus && !hasSubtasks) {
|
||||
// No subtasks yet - task is still in planning phase, don't change status
|
||||
// This prevents the bug where tasks jump to human_review before planning completes
|
||||
console.warn(`[Task ${taskId}] Process exited but no subtasks created yet - keeping current status (${task.status})`);
|
||||
}
|
||||
} else {
|
||||
notificationService.notifyTaskFailed(taskTitle, project.id, taskId);
|
||||
@@ -205,7 +261,8 @@ export function registerAgenteventsHandlers(
|
||||
};
|
||||
|
||||
const newStatus = phaseToStatus[progress.phase];
|
||||
if (newStatus) {
|
||||
// FIX (ACS-55, ACS-71): Validate status transition before sending/persisting
|
||||
if (newStatus && validateStatusTransition(task, newStatus, progress.phase)) {
|
||||
// Include projectId in status change event for multi-project filtering
|
||||
mainWindow.webContents.send(
|
||||
IPC_CHANNELS.TASK_STATUS_CHANGE,
|
||||
|
||||
@@ -102,6 +102,7 @@ export function mapStatusToPlanStatus(status: TaskStatus): string {
|
||||
export async function persistPlanStatus(planPath: string, status: TaskStatus, projectId?: string): Promise<boolean> {
|
||||
return withPlanLock(planPath, async () => {
|
||||
try {
|
||||
console.warn(`[plan-file-utils] Reading implementation_plan.json to update status to: ${status}`, { planPath });
|
||||
// Read file directly without existence check to avoid TOCTOU race condition
|
||||
const planContent = readFileSync(planPath, 'utf-8');
|
||||
const plan = JSON.parse(planContent);
|
||||
@@ -111,6 +112,7 @@ export async function persistPlanStatus(planPath: string, status: TaskStatus, pr
|
||||
plan.updated_at = new Date().toISOString();
|
||||
|
||||
writeFileSync(planPath, JSON.stringify(plan, null, 2));
|
||||
console.warn(`[plan-file-utils] Successfully persisted status: ${status} to implementation_plan.json`);
|
||||
|
||||
// Invalidate tasks cache since status changed
|
||||
if (projectId) {
|
||||
@@ -121,6 +123,7 @@ export async function persistPlanStatus(planPath: string, status: TaskStatus, pr
|
||||
} catch (err) {
|
||||
// File not found is expected - return false
|
||||
if (isFileNotFoundError(err)) {
|
||||
console.warn(`[plan-file-utils] implementation_plan.json not found at ${planPath} - status not persisted`);
|
||||
return false;
|
||||
}
|
||||
console.warn(`[plan-file-utils] Could not persist status to ${planPath}:`, err);
|
||||
@@ -195,6 +198,7 @@ export async function updatePlanFile<T extends Record<string, unknown>>(
|
||||
): Promise<T | null> {
|
||||
return withPlanLock(planPath, async () => {
|
||||
try {
|
||||
console.warn(`[plan-file-utils] Reading implementation_plan.json for update`, { planPath });
|
||||
// Read file directly without existence check to avoid TOCTOU race condition
|
||||
const planContent = readFileSync(planPath, 'utf-8');
|
||||
const plan = JSON.parse(planContent) as T;
|
||||
@@ -204,10 +208,12 @@ export async function updatePlanFile<T extends Record<string, unknown>>(
|
||||
(updatedPlan as Record<string, unknown>).updated_at = new Date().toISOString();
|
||||
|
||||
writeFileSync(planPath, JSON.stringify(updatedPlan, null, 2));
|
||||
console.warn(`[plan-file-utils] Successfully updated implementation_plan.json`);
|
||||
return updatedPlan;
|
||||
} catch (err) {
|
||||
// File not found is expected - return null
|
||||
if (isFileNotFoundError(err)) {
|
||||
console.warn(`[plan-file-utils] implementation_plan.json not found at ${planPath} - update skipped`);
|
||||
return null;
|
||||
}
|
||||
console.warn(`[plan-file-utils] Could not update plan at ${planPath}:`, err);
|
||||
|
||||
@@ -255,13 +255,17 @@ export class ProjectStore {
|
||||
return cached.tasks;
|
||||
}
|
||||
|
||||
console.warn('[ProjectStore] getTasks called with projectId:', projectId, cached ? '(cache expired)' : '(cache miss)');
|
||||
console.warn('[ProjectStore] getTasks called - will load from disk', {
|
||||
projectId,
|
||||
reason: cached ? 'cache expired' : 'cache miss',
|
||||
cacheAge: cached ? now - cached.timestamp : 'N/A'
|
||||
});
|
||||
const project = this.getProject(projectId);
|
||||
if (!project) {
|
||||
console.warn('[ProjectStore] Project not found for id:', projectId);
|
||||
return [];
|
||||
}
|
||||
console.warn('[ProjectStore] Found project:', project.name, 'autoBuildPath:', project.autoBuildPath);
|
||||
console.warn('[ProjectStore] Found project:', project.name, 'autoBuildPath:', project.autoBuildPath, 'path:', project.path);
|
||||
|
||||
const allTasks: Task[] = [];
|
||||
const specsBaseDir = getSpecsDir(project.autoBuildPath);
|
||||
@@ -319,7 +323,11 @@ export class ProjectStore {
|
||||
}
|
||||
|
||||
const tasks = Array.from(taskMap.values());
|
||||
console.warn('[ProjectStore] Returning', tasks.length, 'unique tasks (after deduplication)');
|
||||
console.warn('[ProjectStore] Scan complete - found', tasks.length, 'unique tasks', {
|
||||
mainTasks: allTasks.filter(t => t.location === 'main').length,
|
||||
worktreeTasks: allTasks.filter(t => t.location === 'worktree').length,
|
||||
deduplicated: allTasks.length - tasks.length
|
||||
});
|
||||
|
||||
// Update cache
|
||||
this.tasksCache.set(projectId, { tasks, timestamp: now });
|
||||
@@ -377,12 +385,22 @@ export class ProjectStore {
|
||||
// Try to read implementation plan
|
||||
let plan: ImplementationPlan | null = null;
|
||||
if (existsSync(planPath)) {
|
||||
console.warn(`[ProjectStore] Loading implementation_plan.json for spec: ${dir.name} from ${location}`);
|
||||
try {
|
||||
const content = readFileSync(planPath, 'utf-8');
|
||||
plan = JSON.parse(content);
|
||||
} catch {
|
||||
// Ignore parse errors
|
||||
console.warn(`[ProjectStore] Loaded plan for ${dir.name}:`, {
|
||||
hasDescription: !!plan?.description,
|
||||
hasFeature: !!plan?.feature,
|
||||
status: plan?.status,
|
||||
phaseCount: plan?.phases?.length || 0,
|
||||
subtaskCount: plan?.phases?.flatMap(p => p.subtasks || []).length || 0
|
||||
});
|
||||
} catch (err) {
|
||||
console.error(`[ProjectStore] Failed to parse implementation_plan.json for ${dir.name}:`, err);
|
||||
}
|
||||
} else {
|
||||
console.warn(`[ProjectStore] No implementation_plan.json found for spec: ${dir.name} at ${planPath}`);
|
||||
}
|
||||
|
||||
// PRIORITY 1: Read description from implementation_plan.json (user's original)
|
||||
|
||||
@@ -625,4 +625,726 @@ describe('Task Store', () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('updateTaskFromPlan - validation and subtask creation edge cases', () => {
|
||||
beforeEach(() => {
|
||||
// Spy on console methods to test validation logging and prevent crashes
|
||||
vi.spyOn(console, 'log').mockImplementation(() => {});
|
||||
vi.spyOn(console, 'warn').mockImplementation(() => {});
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
describe('plan validation', () => {
|
||||
it('should reject plan with missing phases array', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const invalidPlan = { feature: 'Test' } as any;
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', invalidPlan);
|
||||
|
||||
// Task should not be updated when plan is invalid
|
||||
expect(useTaskStore.getState().tasks[0].subtasks).toHaveLength(0);
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid plan: missing or invalid phases array')
|
||||
);
|
||||
});
|
||||
|
||||
it('should reject plan with null phases', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const invalidPlan = {
|
||||
feature: 'Test',
|
||||
phases: null
|
||||
} as any;
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', invalidPlan);
|
||||
|
||||
expect(useTaskStore.getState().tasks[0].subtasks).toHaveLength(0);
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid plan: missing or invalid phases array')
|
||||
);
|
||||
});
|
||||
|
||||
it('should reject plan with phase missing subtasks array', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const invalidPlan = {
|
||||
feature: 'Test',
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation'
|
||||
// Missing subtasks
|
||||
}
|
||||
]
|
||||
} as any;
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', invalidPlan);
|
||||
|
||||
expect(useTaskStore.getState().tasks[0].subtasks).toHaveLength(0);
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid phase 0: missing or invalid subtasks array')
|
||||
);
|
||||
});
|
||||
|
||||
it('should reject plan with phase having subtasks not as array', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const invalidPlan = {
|
||||
feature: 'Test',
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: 'not-an-array'
|
||||
}
|
||||
]
|
||||
} as any;
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', invalidPlan);
|
||||
|
||||
expect(useTaskStore.getState().tasks[0].subtasks).toHaveLength(0);
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid phase 0: missing or invalid subtasks array')
|
||||
);
|
||||
});
|
||||
|
||||
it('should reject plan with subtask not being an object', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const invalidPlan = {
|
||||
feature: 'Test',
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: ['not-an-object', 'also-not-an-object']
|
||||
}
|
||||
]
|
||||
} as any;
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', invalidPlan);
|
||||
|
||||
expect(useTaskStore.getState().tasks[0].subtasks).toHaveLength(0);
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid subtask at phase 0, index 0: not an object')
|
||||
);
|
||||
});
|
||||
|
||||
it('should reject plan with subtask missing description', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const invalidPlan = {
|
||||
feature: 'Test',
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'subtask-1', status: 'pending' } // Missing description
|
||||
]
|
||||
}
|
||||
]
|
||||
} as any;
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', invalidPlan);
|
||||
|
||||
expect(useTaskStore.getState().tasks[0].subtasks).toHaveLength(0);
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid subtask at phase 0, index 0: missing or empty description')
|
||||
);
|
||||
});
|
||||
|
||||
it('should reject plan with subtask having empty description', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const invalidPlan = {
|
||||
feature: 'Test',
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'subtask-1', description: '', status: 'pending' }
|
||||
]
|
||||
}
|
||||
]
|
||||
} as any;
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', invalidPlan);
|
||||
|
||||
expect(useTaskStore.getState().tasks[0].subtasks).toHaveLength(0);
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid subtask at phase 0, index 0: missing or empty description')
|
||||
);
|
||||
});
|
||||
|
||||
it('should reject plan with subtask having whitespace-only description', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const invalidPlan = {
|
||||
feature: 'Test',
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'subtask-1', description: ' ', status: 'pending' }
|
||||
]
|
||||
}
|
||||
]
|
||||
} as any;
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', invalidPlan);
|
||||
|
||||
expect(useTaskStore.getState().tasks[0].subtasks).toHaveLength(0);
|
||||
expect(console.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Invalid subtask at phase 0, index 0: missing or empty description')
|
||||
);
|
||||
});
|
||||
|
||||
it('should accept valid plan with all required fields', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const validPlan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'subtask-1', description: 'Valid subtask', status: 'pending' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', validPlan);
|
||||
|
||||
expect(useTaskStore.getState().tasks[0].subtasks).toHaveLength(1);
|
||||
expect(useTaskStore.getState().tasks[0].subtasks[0].description).toBe('Valid subtask');
|
||||
});
|
||||
});
|
||||
|
||||
describe('subtask creation edge cases', () => {
|
||||
it('should generate id for subtask missing id', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ description: 'Subtask without id', status: 'pending' } as any
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
const subtask = useTaskStore.getState().tasks[0].subtasks[0];
|
||||
expect(subtask.id).toBeDefined();
|
||||
// Accept either UUID format (crypto.randomUUID) or fallback format (subtask-timestamp-random)
|
||||
expect(subtask.id).toMatch(/^([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}|subtask-\d+-[a-z0-9]+)$/);
|
||||
});
|
||||
|
||||
it('should use description as title for subtasks', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'subtask-1', description: 'Test Description', status: 'pending' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
const subtask = useTaskStore.getState().tasks[0].subtasks[0];
|
||||
expect(subtask.title).toBe('Test Description');
|
||||
expect(subtask.description).toBe('Test Description');
|
||||
});
|
||||
|
||||
it('should accept all valid subtask statuses', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'subtask-1', description: 'Pending subtask', status: 'pending' },
|
||||
{ id: 'subtask-2', description: 'In progress subtask', status: 'in_progress' },
|
||||
{ id: 'subtask-3', description: 'Completed subtask', status: 'completed' },
|
||||
{ id: 'subtask-4', description: 'Failed subtask', status: 'failed' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
const subtasks = useTaskStore.getState().tasks[0].subtasks;
|
||||
expect(subtasks[0].status).toBe('pending');
|
||||
expect(subtasks[1].status).toBe('in_progress');
|
||||
expect(subtasks[2].status).toBe('completed');
|
||||
expect(subtasks[3].status).toBe('failed');
|
||||
});
|
||||
|
||||
it('should default status to pending when status is missing', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'subtask-1', description: 'Test subtask' } as any
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
const subtask = useTaskStore.getState().tasks[0].subtasks[0];
|
||||
expect(subtask.status).toBe('pending');
|
||||
});
|
||||
|
||||
it('should initialize subtask with empty files array', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'subtask-1', description: 'Test subtask', status: 'pending' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
const subtask = useTaskStore.getState().tasks[0].subtasks[0];
|
||||
expect(subtask.files).toEqual([]);
|
||||
});
|
||||
|
||||
it('should preserve verification field from plan subtask', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{
|
||||
id: 'subtask-1',
|
||||
description: 'Test subtask',
|
||||
status: 'pending',
|
||||
verification: { type: 'command', run: 'npm test' }
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
const subtask = useTaskStore.getState().tasks[0].subtasks[0];
|
||||
expect(subtask.verification).toEqual({ type: 'command', run: 'npm test' });
|
||||
});
|
||||
|
||||
it('should handle subtask with verification undefined', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{
|
||||
id: 'subtask-1',
|
||||
description: 'Test subtask',
|
||||
status: 'pending'
|
||||
// verification is undefined
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
const subtask = useTaskStore.getState().tasks[0].subtasks[0];
|
||||
expect(subtask.verification).toBeUndefined();
|
||||
});
|
||||
|
||||
it('should flatten subtasks from all phases in correct order', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'p1-s1', description: 'Phase 1 Subtask 1', status: 'pending' },
|
||||
{ id: 'p1-s2', description: 'Phase 1 Subtask 2', status: 'pending' }
|
||||
]
|
||||
},
|
||||
{
|
||||
phase: 2,
|
||||
name: 'Phase 2',
|
||||
type: 'testing',
|
||||
subtasks: [
|
||||
{ id: 'p2-s1', description: 'Phase 2 Subtask 1', status: 'pending' },
|
||||
{ id: 'p2-s2', description: 'Phase 2 Subtask 2', status: 'pending' }
|
||||
]
|
||||
},
|
||||
{
|
||||
phase: 3,
|
||||
name: 'Phase 3',
|
||||
type: 'cleanup',
|
||||
subtasks: [
|
||||
{ id: 'p3-s1', description: 'Phase 3 Subtask 1', status: 'pending' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
const subtasks = useTaskStore.getState().tasks[0].subtasks;
|
||||
expect(subtasks).toHaveLength(5);
|
||||
expect(subtasks[0].id).toBe('p1-s1');
|
||||
expect(subtasks[1].id).toBe('p1-s2');
|
||||
expect(subtasks[2].id).toBe('p2-s1');
|
||||
expect(subtasks[3].id).toBe('p2-s2');
|
||||
expect(subtasks[4].id).toBe('p3-s1');
|
||||
});
|
||||
|
||||
it('should handle phase with empty subtasks array', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({ id: 'task-1', subtasks: [] })]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'subtask-1', description: 'Valid subtask', status: 'pending' }
|
||||
]
|
||||
},
|
||||
{
|
||||
phase: 2,
|
||||
name: 'Phase 2',
|
||||
type: 'testing',
|
||||
subtasks: [] // Empty array
|
||||
},
|
||||
{
|
||||
phase: 3,
|
||||
name: 'Phase 3',
|
||||
type: 'cleanup',
|
||||
subtasks: [
|
||||
{ id: 'subtask-2', description: 'Another valid subtask', status: 'pending' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
const subtasks = useTaskStore.getState().tasks[0].subtasks;
|
||||
expect(subtasks).toHaveLength(2);
|
||||
expect(subtasks[0].id).toBe('subtask-1');
|
||||
expect(subtasks[1].id).toBe('subtask-2');
|
||||
});
|
||||
});
|
||||
|
||||
// FIX (PR Review): Test coverage for terminal phase status preservation
|
||||
describe('terminal phase status preservation', () => {
|
||||
it('should NOT update status when task is in terminal phase (complete)', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({
|
||||
id: 'task-1',
|
||||
status: 'human_review',
|
||||
executionProgress: { phase: 'complete', phaseProgress: 100, overallProgress: 100 }
|
||||
})]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'c1', description: 'Subtask 1', status: 'completed' },
|
||||
{ id: 'c2', description: 'Subtask 2', status: 'completed' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
// Status should remain human_review, not be recalculated to ai_review
|
||||
expect(useTaskStore.getState().tasks[0].status).toBe('human_review');
|
||||
});
|
||||
|
||||
it('should NOT update status when task is in terminal phase (failed)', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({
|
||||
id: 'task-1',
|
||||
status: 'human_review',
|
||||
executionProgress: { phase: 'failed', phaseProgress: 50, overallProgress: 30 }
|
||||
})]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'c1', description: 'Subtask 1', status: 'completed' },
|
||||
{ id: 'c2', description: 'Subtask 2', status: 'failed' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
// Status should remain human_review, not be recalculated
|
||||
expect(useTaskStore.getState().tasks[0].status).toBe('human_review');
|
||||
});
|
||||
});
|
||||
|
||||
// FIX (PR Review): Test coverage for explicit human_review from plan file
|
||||
describe('explicit human_review from plan file', () => {
|
||||
it('should skip status recalculation when plan explicitly sets human_review', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({
|
||||
id: 'task-1',
|
||||
status: 'backlog',
|
||||
executionProgress: undefined
|
||||
})]
|
||||
});
|
||||
|
||||
// Plan explicitly sets status to human_review
|
||||
const plan = {
|
||||
...createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'c1', description: 'Subtask 1', status: 'completed' },
|
||||
{ id: 'c2', description: 'Subtask 2', status: 'completed' }
|
||||
]
|
||||
}
|
||||
]
|
||||
}),
|
||||
status: 'human_review' as const
|
||||
};
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
// Status should remain unchanged (backlog) because when plan explicitly
|
||||
// sets human_review, status recalculation is skipped entirely
|
||||
expect(useTaskStore.getState().tasks[0].status).toBe('backlog');
|
||||
});
|
||||
|
||||
it('should NOT preserve status when plan does not explicitly set human_review', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({
|
||||
id: 'task-1',
|
||||
status: 'backlog',
|
||||
executionProgress: undefined
|
||||
})]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'c1', description: 'Subtask 1', status: 'completed' },
|
||||
{ id: 'c2', description: 'Subtask 2', status: 'completed' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
// Status should be recalculated to ai_review since no explicit human_review
|
||||
expect(useTaskStore.getState().tasks[0].status).toBe('ai_review');
|
||||
});
|
||||
});
|
||||
|
||||
// FIX (PR Review): Test coverage for terminal status downgrade prevention
|
||||
describe('terminal status downgrade prevention', () => {
|
||||
it('should NOT downgrade from pr_created to ai_review', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({
|
||||
id: 'task-1',
|
||||
status: 'pr_created',
|
||||
executionProgress: undefined
|
||||
})]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'c1', description: 'Subtask 1', status: 'completed' },
|
||||
{ id: 'c2', description: 'Subtask 2', status: 'completed' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
// Status should remain pr_created, not downgrade to ai_review
|
||||
expect(useTaskStore.getState().tasks[0].status).toBe('pr_created');
|
||||
});
|
||||
|
||||
it('should NOT downgrade from done to ai_review', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({
|
||||
id: 'task-1',
|
||||
status: 'done',
|
||||
executionProgress: undefined
|
||||
})]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'c1', description: 'Subtask 1', status: 'completed' },
|
||||
{ id: 'c2', description: 'Subtask 2', status: 'completed' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
// Status should remain done, not downgrade to ai_review
|
||||
expect(useTaskStore.getState().tasks[0].status).toBe('done');
|
||||
});
|
||||
|
||||
it('should NOT downgrade from human_review to ai_review', () => {
|
||||
useTaskStore.setState({
|
||||
tasks: [createTestTask({
|
||||
id: 'task-1',
|
||||
status: 'human_review',
|
||||
executionProgress: undefined
|
||||
})]
|
||||
});
|
||||
|
||||
const plan = createTestPlan({
|
||||
phases: [
|
||||
{
|
||||
phase: 1,
|
||||
name: 'Phase 1',
|
||||
type: 'implementation',
|
||||
subtasks: [
|
||||
{ id: 'c1', description: 'Subtask 1', status: 'completed' },
|
||||
{ id: 'c2', description: 'Subtask 2', status: 'completed' }
|
||||
]
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
useTaskStore.getState().updateTaskFromPlan('task-1', plan);
|
||||
|
||||
// Status should remain human_review, not downgrade to ai_review
|
||||
expect(useTaskStore.getState().tasks[0].status).toBe('human_review');
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -102,7 +102,8 @@ export const PhaseProgressIndicator = memo(function PhaseProgressIndicator({
|
||||
|
||||
// Determine if we should show indeterminate (activity) vs determinate (%) progress
|
||||
const isIndeterminatePhase = phase === 'planning' || phase === 'qa_review' || phase === 'qa_fixing';
|
||||
const showSubtaskProgress = phase === 'coding' || (totalSubtasks > 0 && !isIndeterminatePhase);
|
||||
// Show subtask progress whenever subtasks exist (stops pulsing animation when spec completes)
|
||||
const showSubtaskProgress = totalSubtasks > 0;
|
||||
|
||||
const colors = PHASE_COLORS[phase] || PHASE_COLORS.idle;
|
||||
const phaseLabel = t(PHASE_LABEL_KEYS[phase] || PHASE_LABEL_KEYS.idle);
|
||||
|
||||
@@ -86,10 +86,23 @@ function TaskDetailModalContent({ open, task, onOpenChange, onSwitchToTerminals,
|
||||
const totalSubtasks = task.subtasks.length;
|
||||
|
||||
// Event Handlers
|
||||
const handleStartStop = () => {
|
||||
const handleStartStop = async () => {
|
||||
if (state.isRunning && !state.isStuck) {
|
||||
stopTask(task.id);
|
||||
} else {
|
||||
// If task is incomplete, validate and reload plan before starting
|
||||
if (state.isIncomplete) {
|
||||
const isValid = await state.reloadPlanForIncompleteTask();
|
||||
if (!isValid) {
|
||||
toast({
|
||||
title: 'Cannot Resume Task',
|
||||
description: 'Failed to load implementation plan. Please try again or check the task files.',
|
||||
variant: 'destructive',
|
||||
duration: 5000,
|
||||
});
|
||||
return;
|
||||
}
|
||||
}
|
||||
startTask(task.id);
|
||||
}
|
||||
};
|
||||
@@ -242,9 +255,18 @@ function TaskDetailModalContent({ open, task, onOpenChange, onSwitchToTerminals,
|
||||
|
||||
if (state.isIncomplete) {
|
||||
return (
|
||||
<Button variant="default" onClick={handleStartStop}>
|
||||
<Play className="mr-2 h-4 w-4" />
|
||||
Resume Task
|
||||
<Button variant="default" onClick={handleStartStop} disabled={state.isLoadingPlan}>
|
||||
{state.isLoadingPlan ? (
|
||||
<>
|
||||
<Loader2 className="mr-2 h-4 w-4 animate-spin" />
|
||||
Loading Plan...
|
||||
</>
|
||||
) : (
|
||||
<>
|
||||
<Play className="mr-2 h-4 w-4" />
|
||||
Resume Task
|
||||
</>
|
||||
)}
|
||||
</Button>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1,8 +1,49 @@
|
||||
import { useState, useRef, useEffect, useCallback } from 'react';
|
||||
import { useProjectStore } from '../../../stores/project-store';
|
||||
import { checkTaskRunning, isIncompleteHumanReview, getTaskProgress } from '../../../stores/task-store';
|
||||
import { checkTaskRunning, isIncompleteHumanReview, getTaskProgress, useTaskStore } from '../../../stores/task-store';
|
||||
import type { Task, TaskLogs, TaskLogPhase, WorktreeStatus, WorktreeDiff, MergeConflict, MergeStats, GitConflictInfo } from '../../../../shared/types';
|
||||
|
||||
/**
|
||||
* Validates task subtasks structure to prevent infinite loops during resume.
|
||||
* Returns true if task has valid subtasks, false otherwise.
|
||||
*/
|
||||
function validateTaskSubtasks(task: Task): boolean {
|
||||
// Check if subtasks array exists
|
||||
if (!task.subtasks || !Array.isArray(task.subtasks)) {
|
||||
console.warn('[validateTaskSubtasks] Task has no subtasks array:', task.id);
|
||||
return false;
|
||||
}
|
||||
|
||||
// If subtasks array is empty and task is incomplete, it needs plan reload
|
||||
if (task.subtasks.length === 0) {
|
||||
console.warn('[validateTaskSubtasks] Task has empty subtasks array:', task.id);
|
||||
return false;
|
||||
}
|
||||
|
||||
// Validate each subtask has minimum required fields
|
||||
for (let i = 0; i < task.subtasks.length; i++) {
|
||||
const subtask = task.subtasks[i];
|
||||
if (!subtask || typeof subtask !== 'object') {
|
||||
console.warn(`[validateTaskSubtasks] Invalid subtask at index ${i}:`, subtask);
|
||||
return false;
|
||||
}
|
||||
|
||||
// Description is critical - we can't show a subtask without it
|
||||
if (!subtask.description || typeof subtask.description !== 'string' || subtask.description.trim() === '') {
|
||||
console.warn(`[validateTaskSubtasks] Subtask at index ${i} missing description:`, subtask);
|
||||
return false;
|
||||
}
|
||||
|
||||
// ID is required for tracking
|
||||
if (!subtask.id || typeof subtask.id !== 'string') {
|
||||
console.warn(`[validateTaskSubtasks] Subtask at index ${i} missing id:`, subtask);
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
export interface UseTaskDetailOptions {
|
||||
task: Task;
|
||||
}
|
||||
@@ -34,6 +75,7 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) {
|
||||
const [phaseLogs, setPhaseLogs] = useState<TaskLogs | null>(null);
|
||||
const [isLoadingLogs, setIsLoadingLogs] = useState(false);
|
||||
const [expandedPhases, setExpandedPhases] = useState<Set<TaskLogPhase>>(new Set());
|
||||
const [isLoadingPlan, setIsLoadingPlan] = useState(false);
|
||||
const logsEndRef = useRef<HTMLDivElement>(null);
|
||||
const logsContainerRef = useRef<HTMLDivElement>(null);
|
||||
|
||||
@@ -244,6 +286,81 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) {
|
||||
// User must click "Check for Conflicts" button to trigger the expensive preview operation.
|
||||
// This improves modal open performance significantly (avoids 1-30+ second Python subprocess).
|
||||
|
||||
/**
|
||||
* Reloads implementation plan for an incomplete task to ensure subtasks are properly loaded.
|
||||
* This prevents the "Task Incomplete" infinite loop when resuming stuck tasks.
|
||||
*/
|
||||
const reloadPlanForIncompleteTask = useCallback(async (): Promise<boolean> => {
|
||||
if (!selectedProject) {
|
||||
console.error('[reloadPlanForIncompleteTask] No selected project');
|
||||
return false;
|
||||
}
|
||||
|
||||
// Only reload if task is incomplete and subtasks are invalid
|
||||
if (!isIncomplete) {
|
||||
return true; // Not incomplete, no reload needed
|
||||
}
|
||||
|
||||
// Check if subtasks are valid
|
||||
if (validateTaskSubtasks(task)) {
|
||||
console.log('[reloadPlanForIncompleteTask] Subtasks are valid, no reload needed');
|
||||
return true; // Subtasks are valid, proceed
|
||||
}
|
||||
|
||||
console.warn('[reloadPlanForIncompleteTask] Task has invalid subtasks, reloading plan:', {
|
||||
taskId: task.id,
|
||||
specId: task.specId,
|
||||
subtaskCount: task.subtasks?.length || 0
|
||||
});
|
||||
|
||||
setIsLoadingPlan(true);
|
||||
try {
|
||||
// Reload tasks from the project to get fresh implementation plan
|
||||
const result = await window.electronAPI.getTasks(selectedProject.id);
|
||||
|
||||
if (!result.success || !result.data) {
|
||||
console.error('[reloadPlanForIncompleteTask] Failed to reload tasks:', result.error);
|
||||
return false;
|
||||
}
|
||||
|
||||
// Find the updated task in the result
|
||||
const updatedTask = result.data.find(t => t.id === task.id || t.specId === task.specId);
|
||||
if (!updatedTask) {
|
||||
console.error('[reloadPlanForIncompleteTask] Task not found in reloaded tasks');
|
||||
return false;
|
||||
}
|
||||
|
||||
// Validate the reloaded subtasks
|
||||
if (!validateTaskSubtasks(updatedTask)) {
|
||||
console.error('[reloadPlanForIncompleteTask] Reloaded task still has invalid subtasks');
|
||||
return false;
|
||||
}
|
||||
|
||||
console.log('[reloadPlanForIncompleteTask] Successfully reloaded plan with valid subtasks:', {
|
||||
taskId: task.id,
|
||||
subtaskCount: updatedTask.subtasks?.length ?? 0
|
||||
});
|
||||
|
||||
// FIX (PR Review): Update the Zustand store with the reloaded task data
|
||||
// Without this, the UI continues to display stale/invalid subtasks
|
||||
const store = useTaskStore.getState();
|
||||
store.updateTask(task.id, {
|
||||
subtasks: updatedTask.subtasks,
|
||||
title: updatedTask.title,
|
||||
description: updatedTask.description,
|
||||
metadata: updatedTask.metadata,
|
||||
updatedAt: new Date()
|
||||
});
|
||||
|
||||
return true;
|
||||
} catch (err) {
|
||||
console.error('[reloadPlanForIncompleteTask] Error reloading plan:', err);
|
||||
return false;
|
||||
} finally {
|
||||
setIsLoadingPlan(false);
|
||||
}
|
||||
}, [selectedProject, task, isIncomplete]);
|
||||
|
||||
return {
|
||||
// State
|
||||
feedback,
|
||||
@@ -286,6 +403,7 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) {
|
||||
showConflictDialog,
|
||||
showPRDialog,
|
||||
isCreatingPR,
|
||||
isLoadingPlan,
|
||||
|
||||
// Setters
|
||||
setFeedback,
|
||||
@@ -324,5 +442,6 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) {
|
||||
handleLogsScroll,
|
||||
togglePhase,
|
||||
loadMergePreview,
|
||||
reloadPlanForIncompleteTask,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -93,6 +93,31 @@ function flushBatch(): void {
|
||||
function queueUpdate(taskId: string, update: BatchedUpdate): void {
|
||||
const existing = batchQueue.get(taskId) || {};
|
||||
|
||||
// FIX (ACS-55): Phase changes bypass batching - apply immediately
|
||||
// This ensures phase transitions are applied in order and not batched together,
|
||||
// so the UI accurately reflects each phase state (e.g., planning → coding shows both)
|
||||
// rather than skipping directly to the latest phase if they arrive within 16ms.
|
||||
// Phase changes are rare (~3-4 per task) vs progress ticks (hundreds), so this is safe for perf
|
||||
if (update.progress?.phase && storeActionsRef) {
|
||||
const currentPhase = existing.progress?.phase ||
|
||||
useTaskStore.getState().tasks.find(t => t.id === taskId || t.specId === taskId)?.executionProgress?.phase;
|
||||
|
||||
if (update.progress.phase !== currentPhase) {
|
||||
// Flush any pending updates first to ensure correct ordering
|
||||
if (batchTimeout) {
|
||||
clearTimeout(batchTimeout);
|
||||
batchTimeout = null;
|
||||
flushBatch();
|
||||
}
|
||||
// Apply phase change immediately
|
||||
if (window.DEBUG) {
|
||||
console.warn(`[IPC Batch] Phase change detected: ${currentPhase} → ${update.progress.phase}, applying immediately`);
|
||||
}
|
||||
storeActionsRef.updateExecutionProgress(taskId, update.progress);
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
// For logs, accumulate rather than replace
|
||||
let mergedLogs = existing.logs;
|
||||
if (update.logs) {
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { create } from 'zustand';
|
||||
import type { Task, TaskStatus, ImplementationPlan, Subtask, TaskMetadata, ExecutionProgress, ExecutionPhase, ReviewReason, TaskDraft } from '../../shared/types';
|
||||
import type { Task, TaskStatus, SubtaskStatus, ImplementationPlan, Subtask, TaskMetadata, ExecutionProgress, ExecutionPhase, ReviewReason, TaskDraft } from '../../shared/types';
|
||||
import { debugLog } from '../../shared/utils/debug-logger';
|
||||
|
||||
interface TaskState {
|
||||
tasks: Task[];
|
||||
@@ -55,6 +56,44 @@ function updateTaskAtIndex(tasks: Task[], index: number, updater: (task: Task) =
|
||||
return newTasks;
|
||||
}
|
||||
|
||||
/**
|
||||
* Validates implementation plan data structure before processing.
|
||||
* Returns true if valid, false if invalid/incomplete.
|
||||
*/
|
||||
function validatePlanData(plan: ImplementationPlan): boolean {
|
||||
// Validate plan has phases array
|
||||
if (!plan.phases || !Array.isArray(plan.phases)) {
|
||||
console.warn('[validatePlanData] Invalid plan: missing or invalid phases array');
|
||||
return false;
|
||||
}
|
||||
|
||||
// Validate each phase has subtasks array
|
||||
for (let i = 0; i < plan.phases.length; i++) {
|
||||
const phase = plan.phases[i];
|
||||
if (!phase || !phase.subtasks || !Array.isArray(phase.subtasks)) {
|
||||
console.warn(`[validatePlanData] Invalid phase ${i}: missing or invalid subtasks array`);
|
||||
return false;
|
||||
}
|
||||
|
||||
// Validate each subtask has at minimum a description
|
||||
for (let j = 0; j < phase.subtasks.length; j++) {
|
||||
const subtask = phase.subtasks[j];
|
||||
if (!subtask || typeof subtask !== 'object') {
|
||||
console.warn(`[validatePlanData] Invalid subtask at phase ${i}, index ${j}: not an object`);
|
||||
return false;
|
||||
}
|
||||
|
||||
// Description is critical - we can't show a subtask without it
|
||||
if (!subtask.description || typeof subtask.description !== 'string' || subtask.description.trim() === '') {
|
||||
console.warn(`[validatePlanData] Invalid subtask at phase ${i}, index ${j}: missing or empty description`);
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
export const useTaskStore = create<TaskState>((set, get) => ({
|
||||
tasks: [],
|
||||
selectedTaskId: null,
|
||||
@@ -105,22 +144,65 @@ export const useTaskStore = create<TaskState>((set, get) => ({
|
||||
|
||||
updateTaskFromPlan: (taskId, plan) =>
|
||||
set((state) => {
|
||||
// FIX (PR Review): Gate debug logging to prevent production console clutter
|
||||
debugLog('[updateTaskFromPlan] called with plan:', {
|
||||
taskId,
|
||||
feature: plan.feature,
|
||||
phases: plan.phases?.length || 0,
|
||||
totalSubtasks: plan.phases?.reduce((acc, p) => acc + (p.subtasks?.length || 0), 0) || 0
|
||||
// Note: planData removed to avoid verbose output in logs
|
||||
});
|
||||
|
||||
const index = findTaskIndex(state.tasks, taskId);
|
||||
if (index === -1) return state;
|
||||
if (index === -1) {
|
||||
console.log('[updateTaskFromPlan] Task not found:', taskId);
|
||||
return state;
|
||||
}
|
||||
|
||||
// Validate plan data before processing
|
||||
if (!validatePlanData(plan)) {
|
||||
console.error('[updateTaskFromPlan] Invalid plan data, skipping update:', {
|
||||
taskId,
|
||||
plan
|
||||
});
|
||||
return state;
|
||||
}
|
||||
|
||||
return {
|
||||
tasks: updateTaskAtIndex(state.tasks, index, (t) => {
|
||||
const subtasks: Subtask[] = plan.phases.flatMap((phase) =>
|
||||
phase.subtasks.map((subtask) => ({
|
||||
id: subtask.id,
|
||||
title: subtask.description,
|
||||
description: subtask.description,
|
||||
status: subtask.status,
|
||||
files: [],
|
||||
verification: subtask.verification as Subtask['verification']
|
||||
}))
|
||||
phase.subtasks.map((subtask) => {
|
||||
// Ensure all required fields have valid values to prevent UI issues
|
||||
// Use crypto.randomUUID() for stronger randomness when available
|
||||
const id = subtask.id || (typeof crypto !== 'undefined' && crypto.randomUUID
|
||||
? crypto.randomUUID()
|
||||
: `subtask-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`);
|
||||
// Defensive fallback: validatePlanData() ensures description exists, but kept for safety
|
||||
const description = subtask.description || 'No description available';
|
||||
const title = description; // Title and description are the same for subtasks
|
||||
const status = (subtask.status as SubtaskStatus) || 'pending';
|
||||
|
||||
return {
|
||||
id,
|
||||
title,
|
||||
description,
|
||||
status,
|
||||
files: [],
|
||||
verification: subtask.verification as Subtask['verification']
|
||||
};
|
||||
})
|
||||
);
|
||||
|
||||
debugLog('[updateTaskFromPlan] Created subtasks:', {
|
||||
taskId,
|
||||
subtaskCount: subtasks.length,
|
||||
subtasks: subtasks.map(s => ({
|
||||
id: s.id,
|
||||
title: s.title,
|
||||
status: s.status
|
||||
}))
|
||||
});
|
||||
|
||||
const allCompleted = subtasks.every((s) => s.status === 'completed');
|
||||
const anyFailed = subtasks.some((s) => s.status === 'failed');
|
||||
const anyInProgress = subtasks.some((s) => s.status === 'in_progress');
|
||||
@@ -133,9 +215,30 @@ export const useTaskStore = create<TaskState>((set, get) => ({
|
||||
const activePhases: ExecutionPhase[] = ['planning', 'coding', 'qa_review', 'qa_fixing'];
|
||||
const isInActivePhase = t.executionProgress?.phase && activePhases.includes(t.executionProgress.phase);
|
||||
|
||||
if (!isInActivePhase) {
|
||||
// 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);
|
||||
|
||||
// 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
|
||||
// Note: ImplementationPlan type already defines status?: TaskStatus
|
||||
const planStatus = plan.status;
|
||||
const isExplicitHumanReview = planStatus === 'human_review';
|
||||
|
||||
// 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
|
||||
if (!isInActivePhase && !isInTerminalPhase && !isExplicitHumanReview) {
|
||||
if (allCompleted) {
|
||||
status = 'ai_review';
|
||||
// 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';
|
||||
}
|
||||
} else if (anyFailed) {
|
||||
status = 'human_review';
|
||||
reviewReason = 'errors';
|
||||
@@ -144,6 +247,21 @@ export const useTaskStore = create<TaskState>((set, get) => ({
|
||||
}
|
||||
}
|
||||
|
||||
debugLog('[updateTaskFromPlan] Status computation:', {
|
||||
taskId,
|
||||
currentStatus: t.status,
|
||||
newStatus: status,
|
||||
isInActivePhase,
|
||||
isInTerminalPhase,
|
||||
isExplicitHumanReview,
|
||||
planStatus,
|
||||
currentPhase: t.executionProgress?.phase,
|
||||
allCompleted,
|
||||
anyFailed,
|
||||
anyInProgress,
|
||||
anyCompleted
|
||||
});
|
||||
|
||||
return {
|
||||
...t,
|
||||
title: plan.feature || t.title,
|
||||
@@ -173,6 +291,15 @@ export const useTaskStore = create<TaskState>((set, get) => ({
|
||||
const incomingSeq = progress.sequenceNumber ?? 0;
|
||||
const currentSeq = existingProgress.sequenceNumber ?? 0;
|
||||
if (incomingSeq > 0 && currentSeq > 0 && incomingSeq < currentSeq) {
|
||||
// FIX (ACS-55): Log when updates are dropped due to sequence numbers
|
||||
// This helps debug phase transition issues
|
||||
console.warn('[updateExecutionProgress] Dropping out-of-order update:', {
|
||||
taskId,
|
||||
incomingSeq,
|
||||
currentSeq,
|
||||
incomingPhase: progress.phase,
|
||||
currentPhase: existingProgress.phase
|
||||
});
|
||||
return t; // Skip out-of-order update
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user