From 9734b70b0520b62be0ff10af2d19cd134451e696 Mon Sep 17 00:00:00 2001 From: souky-byte <130178626+souky-byte@users.noreply.github.com> Date: Sat, 27 Dec 2025 15:21:35 +0100 Subject: [PATCH] chore: Refactor/kanban realtime status sync (#249) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(execution): add structured phase event emission and improve phase transition handling - Add emit_phase() calls in coder.py for PLANNING, CODING, COMPLETE, and FAILED phases - Add emit_phase() calls in planner.py for follow-up planning phase - Add emit_phase() calls in qa/loop.py for QA_REVIEW, QA_FIXING, COMPLETE, and FAILED phases - Add parsePhaseEvent() to agent-events.ts to prioritize structured events over log parsing - Default to 'planning' phase in PhaseProgressIndicator when running but phase * fix(execution): prevent premature 'complete' phase during QA workflow - Remove COMPLETE phase emission from coder.py when subtasks finish (QA hasn't run yet) - Add phase regression prevention in agent-events.ts to block fallback text matching from moving backwards (e.g., QA β†’ coding) - Remove 'complete' phase detection from "BUILD COMPLETE" banner text (only structured emit_phase(COMPLETE) from QA approval should set complete) - Add line buffering in agent-process.ts to prevent split __EXEC_PHASE * fix(execution): prevent phase regression and improve JSON parsing robustness - Add phase regression check to 'planning' phase detection in agent-events.ts - Prevent 'failed' phase from overwriting 'complete' or 'failed' from structured events in agent-process.ts - Add extractJsonObject() to handle JSON with trailing garbage in phase-event-parser.ts - Implements brace-matching parser that handles escaped quotes and nested objects - Prevents parse failures when __EXEC_PHASE__ JSON is followed by log * refactor: improve variable naming clarity in phase event parsing - Rename 'escape' to 'isEscaped' in extractJsonObject() for better readability - Rename list comprehension variable 'l' to 'line' in test_phase_event.py * feat(agent-events): add Zod validation and refactor phase event parsing - Add zod dependency (^4.2.1) for runtime type validation - Refactor phase event parsing into specialized parser classes: - ExecutionPhaseParser for task execution phases - IdeationPhaseParser for ideation workflow phases - RoadmapPhaseParser for roadmap generation phases - Add strict Zod schemas for phase event validation: - Reject invalid message types (must be string) - Reject invalid progress values (must be 0-100 * refactor(agent-events): remove parser delegation and inline phase detection logic - Remove ExecutionPhaseParser, IdeationPhaseParser, and RoadmapPhaseParser delegation - Inline all phase detection logic directly into AgentEvents methods - Add wouldPhaseRegress() check to prevent fallback text matching from moving backwards - Add parsePhaseEvent() call to prioritize structured __EXEC_PHASE__ events - Add checkRegression() helper to validate phase transitions before applying fallback matches - Filter * test(subprocess): update test expectations to include newlines in buffered output - Update subprocess-spawn.test.ts to append '\n' to test data and expectations - Reflects line buffering behavior where output is processed line-by-line - Skip ipc-handlers.test.ts exit event test (status change logic removed) - Remove exit code 0 test case that no longer applies after status change removal * refactor(ideation-phase-parser): add terminal state guard and extract progress calculation - Add terminal state check to prevent phase changes after completion - Extract calculateGeneratingProgress() helper with division-by-zero protection - Return 90% progress fallback when totalTypes is 0 or negative - Apply helper to both progress calculation paths (no phase change and phase detected) * fix(phase-parser): prevent premature QA phase detection during planning Add canEnterQAPhase guard to fallback text matching in agent-events.ts and execution-phase-parser.ts. QA phases can now only be triggered via text matching if currentPhase is already 'coding', 'qa_review', or 'qa_fixing'. This prevents tasks from jumping to QA Review column when planning phase output contains QA-related text. Structured events from backend (__EXEC_PHASE__:...) bypass this check. * fix(task-store): prevent stale plan data from overriding status during active execution When a task is restarted, the file watcher immediately reads the existing implementation_plan.json and calls updateTaskFromPlan. If the old plan has all subtasks completed, it would set status to 'ai_review' before the agent process emits the 'planning' phase event. This fix checks if the task is in an active execution phase (planning, coding, qa_review, qa_fixing) and if so, does NOT let the plan data override the status. The execution phase takes precedence. Added 4 tests to verify the behavior. * Reorder imports in coder.py for clarity Moved the import of ExecutionPhase and emit_phase from phase_event to follow the project's import organization conventions and improve code readability. * fix: address PR review comments from CodeRabbit - Clamp progress values to 0-100 range in phase_event.py - Remove unused PhaseEvent import in test file - Simplify terminal phase check in ideation-phase-parser.ts - Add regression prevention in roadmap-phase-parser.ts - Use z.infer for PhaseEvent type derivation * fix: add type assertions for Zod-validated phase values TypeScript couldn't infer the literal type from Zod enum validation. Added explicit type assertions since the phase is already validated. * fix: correct misleading test name for QA loop transition The test was named 'should not regress' but actually verified that qa_fixing β†’ qa_review IS allowed (valid re-review after fix). Renamed to clarify the expected behavior. * fix: define phase variable from rawPhase in PhaseProgressIndicator The prop was renamed during destructuring but the derived variable was never defined, causing 'phase is not defined' runtime error. * fix(security): add Python path validation to prevent command injection Add validatePythonPath() function that validates user-configurable Python paths before use in spawn(). This prevents potential command injection attacks via malicious paths. Security checks implemented: - Block shell metacharacters (;|&<> etc.) - Validate against allowlist of known Python locations - Verify file exists and is executable - Confirm it's actually Python via --version Applied validation to all affected locations: - AgentProcessManager.configure() - InsightsConfig.configure() - ChangelogService.configure() - TitleGenerator.configure() Addresses: PR #249 review - CRITICAL security finding * fix: add sequence tracking to prevent race conditions in state updates Add sequenceNumber field to ExecutionProgress to track update order and prevent stale updates from overwriting newer state. Changes: - Add sequenceNumber to ExecutionProgress interface - updateExecutionProgress now rejects updates with lower sequence numbers - All execution-progress emissions now include monotonically increasing sequence numbers This prevents race conditions where out-of-order updates could cause incorrect task state display. Addresses: PR #249 review - HIGH severity race condition finding * refactor: extract helper methods from spawnProcess() to reduce complexity Break down the 294-line spawnProcess() into smaller focused methods: - setupProcessEnvironment(): Creates the process environment object - handleProcessFailure(): Orchestrates rate limit and auth failure handling - handleRateLimitWithAutoSwap(): Handles auto-swap logic for rate limits - handleAuthFailure(): Detects and handles authentication failures The main spawnProcess() is now significantly cleaner with single-responsibility helper methods that are easier to test and maintain. Addresses: PR #249 review - HIGH severity complexity finding * fix: improve phase handling with type guards and better error reporting - Add type guard validation in checkRegression() before calling wouldPhaseRegress() to prevent undefined lookups in PHASE_ORDER_INDEX - Add warning log when calculateOverallProgress() receives unknown phase instead of silently returning 0% - Change 'failed' phase index from 5 to 99 to clearly indicate it's outside normal progression (like 'idle' uses -1) These changes improve defensive programming and debugging capabilities for phase state management. Addresses: PR #249 review - MEDIUM severity findings * refactor(security): consolidate Python path validation logic into reusable helper Extract repeated validation pattern into getValidatedPythonPath() helper to reduce code duplication across services. Changes: - Add getValidatedPythonPath() helper that encapsulates validation logic - Replace duplicated validation blocks in ChangelogService, InsightsConfig, and TitleGenerator with helper call - Improve isSafePythonCommand() to normalize whitespace before checking - Add newline/carriage return to DANGEROUS_SHELL_CHARS regex * fix(tests): enable exit event forwarding test - Remove it.skip from 'should forward exit events with status change on failure' - Add proper test setup: create project and task before emitting exit event - Add mock for notificationService to prevent errors during test * fix(security): use mkdtempSync for secure temp directory in tests Addresses CodeQL 'Insecure temporary file' warning by using mkdtempSync with a random suffix instead of a predictable path. --------- Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com> Co-authored-by: Alex <63423455+AlexMadera@users.noreply.github.com> --- apps/backend/agents/coder.py | 11 +- apps/backend/agents/planner.py | 2 + apps/backend/core/phase_event.py | 55 ++ apps/backend/phase_event.py | 16 + apps/backend/qa/loop.py | 11 + apps/frontend/package-lock.json | 8 +- apps/frontend/package.json | 1 + .../integration/subprocess-spawn.test.ts | 21 +- .../src/main/__tests__/agent-events.test.ts | 532 ++++++++++++++++++ .../src/main/__tests__/ipc-handlers.test.ts | 29 +- .../src/main/__tests__/parsers.test.ts | 351 ++++++++++++ .../main/__tests__/phase-event-parser.test.ts | 421 ++++++++++++++ .../main/__tests__/phase-event-schema.test.ts | 143 +++++ apps/frontend/src/main/agent/agent-events.ts | 109 ++-- apps/frontend/src/main/agent/agent-process.ts | 365 +++++++----- .../main/agent/parsers/base-phase-parser.ts | 78 +++ .../agent/parsers/execution-phase-parser.ts | 206 +++++++ .../agent/parsers/ideation-phase-parser.ts | 130 +++++ apps/frontend/src/main/agent/parsers/index.ts | 37 ++ .../agent/parsers/roadmap-phase-parser.ts | 81 +++ .../src/main/agent/phase-event-parser.ts | 120 ++++ .../src/main/agent/phase-event-schema.ts | 37 ++ .../src/main/changelog/changelog-service.ts | 6 +- apps/frontend/src/main/insights/config.ts | 6 +- .../ipc-handlers/agent-events-handlers.ts | 95 +--- apps/frontend/src/main/python-detector.ts | 231 +++++++- apps/frontend/src/main/title-generator.ts | 7 +- .../src/renderer/__tests__/task-store.test.ts | 113 ++++ .../components/PhaseProgressIndicator.tsx | 3 +- .../src/renderer/stores/task-store.ts | 61 +- apps/frontend/src/shared/constants/index.ts | 3 + .../src/shared/constants/phase-protocol.ts | 114 ++++ apps/frontend/src/shared/types/task.ts | 6 +- tests/test_phase_event.py | 487 ++++++++++++++++ 34 files changed, 3565 insertions(+), 331 deletions(-) create mode 100644 apps/backend/core/phase_event.py create mode 100644 apps/backend/phase_event.py create mode 100644 apps/frontend/src/main/__tests__/agent-events.test.ts create mode 100644 apps/frontend/src/main/__tests__/parsers.test.ts create mode 100644 apps/frontend/src/main/__tests__/phase-event-parser.test.ts create mode 100644 apps/frontend/src/main/__tests__/phase-event-schema.test.ts create mode 100644 apps/frontend/src/main/agent/parsers/base-phase-parser.ts create mode 100644 apps/frontend/src/main/agent/parsers/execution-phase-parser.ts create mode 100644 apps/frontend/src/main/agent/parsers/ideation-phase-parser.ts create mode 100644 apps/frontend/src/main/agent/parsers/index.ts create mode 100644 apps/frontend/src/main/agent/parsers/roadmap-phase-parser.ts create mode 100644 apps/frontend/src/main/agent/phase-event-parser.ts create mode 100644 apps/frontend/src/main/agent/phase-event-schema.ts create mode 100644 apps/frontend/src/shared/constants/phase-protocol.ts create mode 100644 tests/test_phase_event.py diff --git a/apps/backend/agents/coder.py b/apps/backend/agents/coder.py index 79727512..3e286303 100644 --- a/apps/backend/agents/coder.py +++ b/apps/backend/agents/coder.py @@ -18,6 +18,7 @@ from linear_updater import ( linear_task_stuck, ) from phase_config import get_phase_model, get_phase_thinking_budget +from phase_event import ExecutionPhase, emit_phase from progress import ( count_subtasks, count_subtasks_detailed, @@ -146,6 +147,7 @@ async def run_autonomous_agent( # Update status for planning phase status_manager.update(state=BuildState.PLANNING) + emit_phase(ExecutionPhase.PLANNING, "Creating implementation plan") is_planning_phase = True current_log_phase = LogPhase.PLANNING @@ -173,6 +175,9 @@ async def run_autonomous_agent( if task_logger: task_logger.start_phase(LogPhase.CODING, "Continuing implementation...") + # Emit phase event when continuing build + emit_phase(ExecutionPhase.CODING, "Continuing implementation") + # Show human intervention hint content = [ bold("INTERACTIVE CONTROLS"), @@ -273,6 +278,7 @@ async def run_autonomous_agent( if is_planning_phase: is_planning_phase = False current_log_phase = LogPhase.CODING + emit_phase(ExecutionPhase.CODING, "Starting implementation") if task_logger: task_logger.end_phase( LogPhase.PLANNING, @@ -386,10 +392,11 @@ async def run_autonomous_agent( # Handle session status if status == "complete": + # Don't emit COMPLETE here - subtasks are done but QA hasn't run yet + # QA loop will emit COMPLETE after actual approval print_build_complete_banner(spec_dir) status_manager.update(state=BuildState.COMPLETE) - # End coding phase in task logger if task_logger: task_logger.end_phase( LogPhase.CODING, @@ -397,7 +404,6 @@ async def run_autonomous_agent( message="All subtasks completed successfully", ) - # Notify Linear that build is complete (moving to QA) if linear_task and linear_task.task_id: await linear_build_complete(spec_dir) print_status("Linear notified: build complete, ready for QA", "success") @@ -432,6 +438,7 @@ async def run_autonomous_agent( await asyncio.sleep(AUTO_CONTINUE_DELAY_SECONDS) elif status == "error": + emit_phase(ExecutionPhase.FAILED, "Session encountered an error") print_status("Session encountered an error", "error") print(muted("Will retry with a fresh session...")) status_manager.update(state=BuildState.ERROR) diff --git a/apps/backend/agents/planner.py b/apps/backend/agents/planner.py index db84aa1e..34acf84a 100644 --- a/apps/backend/agents/planner.py +++ b/apps/backend/agents/planner.py @@ -10,6 +10,7 @@ from pathlib import Path from core.client import create_client from phase_config import get_phase_thinking_budget +from phase_event import ExecutionPhase, emit_phase from task_logger import ( LogPhase, get_task_logger, @@ -67,6 +68,7 @@ async def run_followup_planner( # Initialize status manager for ccstatusline status_manager = StatusManager(project_dir) status_manager.set_active(spec_dir.name, BuildState.PLANNING) + emit_phase(ExecutionPhase.PLANNING, "Follow-up planning") # Initialize task logger for persistent logging task_logger = get_task_logger(spec_dir) diff --git a/apps/backend/core/phase_event.py b/apps/backend/core/phase_event.py new file mode 100644 index 00000000..a86321cf --- /dev/null +++ b/apps/backend/core/phase_event.py @@ -0,0 +1,55 @@ +""" +Execution phase event protocol for frontend synchronization. + +Protocol: __EXEC_PHASE__:{"phase":"coding","message":"Starting"} +""" + +import json +import os +import sys +from enum import Enum +from typing import Any + +PHASE_MARKER_PREFIX = "__EXEC_PHASE__:" +_DEBUG = os.environ.get("DEBUG", "").lower() in ("1", "true", "yes") + + +class ExecutionPhase(str, Enum): + """Maps to frontend's ExecutionPhase type for task card badges.""" + + PLANNING = "planning" + CODING = "coding" + QA_REVIEW = "qa_review" + QA_FIXING = "qa_fixing" + COMPLETE = "complete" + FAILED = "failed" + + +def emit_phase( + phase: ExecutionPhase | str, + message: str = "", + *, + progress: int | None = None, + subtask: str | None = None, +) -> None: + """Emit structured phase event to stdout for frontend parsing.""" + phase_value = phase.value if isinstance(phase, ExecutionPhase) else phase + + payload: dict[str, Any] = { + "phase": phase_value, + "message": message, + } + + if progress is not None: + if not (0 <= progress <= 100): + progress = max(0, min(100, progress)) + payload["progress"] = progress + + if subtask is not None: + payload["subtask"] = subtask + + try: + print(f"{PHASE_MARKER_PREFIX}{json.dumps(payload, default=str)}", flush=True) + except (OSError, UnicodeEncodeError) as e: + if _DEBUG: + print(f"[phase_event] emit failed: {e}", file=sys.stderr, flush=True) diff --git a/apps/backend/phase_event.py b/apps/backend/phase_event.py new file mode 100644 index 00000000..8fe05d59 --- /dev/null +++ b/apps/backend/phase_event.py @@ -0,0 +1,16 @@ +""" +Phase event facade for frontend synchronization. +Re-exports from core.phase_event for clean imports. +""" + +from core.phase_event import ( + PHASE_MARKER_PREFIX, + ExecutionPhase, + emit_phase, +) + +__all__ = [ + "PHASE_MARKER_PREFIX", + "ExecutionPhase", + "emit_phase", +] diff --git a/apps/backend/qa/loop.py b/apps/backend/qa/loop.py index db864928..ff830869 100644 --- a/apps/backend/qa/loop.py +++ b/apps/backend/qa/loop.py @@ -20,6 +20,7 @@ from linear_updater import ( linear_qa_started, ) from phase_config import get_phase_model, get_phase_thinking_budget +from phase_event import ExecutionPhase, emit_phase from progress import count_subtasks, is_build_complete from task_logger import ( LogPhase, @@ -109,6 +110,9 @@ async def run_qa_validation_loop( print(f" Progress: {completed}/{total} subtasks completed") return False + # Emit phase event at start of QA validation (before any early returns) + emit_phase(ExecutionPhase.QA_REVIEW, "Starting QA validation") + # Check if there's pending human feedback that needs to be processed fix_request_file = spec_dir / "QA_FIX_REQUEST.md" has_human_feedback = fix_request_file.exists() @@ -126,6 +130,7 @@ async def run_qa_validation_loop( "Human feedback detected - will run fixer first", fix_request_file=str(fix_request_file), ) + emit_phase(ExecutionPhase.QA_FIXING, "Processing human feedback") print("\nπŸ“ Human feedback detected. Running QA Fixer first...") # Get model and thinking budget for fixer (uses QA phase config) @@ -202,6 +207,9 @@ async def run_qa_validation_loop( ) print(f"\n--- QA Iteration {qa_iteration}/{MAX_QA_ITERATIONS} ---") + emit_phase( + ExecutionPhase.QA_REVIEW, f"Running QA review iteration {qa_iteration}" + ) # Run QA reviewer with phase-specific model and thinking budget qa_model = get_phase_model(spec_dir, "qa", model) @@ -242,6 +250,7 @@ async def run_qa_validation_loop( ) if status == "approved": + emit_phase(ExecutionPhase.COMPLETE, "QA validation passed") # Reset error tracking on success consecutive_errors = 0 last_error_context = None @@ -365,6 +374,7 @@ async def run_qa_validation_loop( model=qa_model, thinking_budget=fixer_thinking_budget, ) + emit_phase(ExecutionPhase.QA_FIXING, "Fixing QA issues") print("\nRunning QA Fixer Agent...") fix_client = create_client( @@ -455,6 +465,7 @@ async def run_qa_validation_loop( print("Retrying with error feedback...") # Max iterations reached without approval + emit_phase(ExecutionPhase.FAILED, "QA validation incomplete") debug_error( "qa_loop", "QA VALIDATION INCOMPLETE - max iterations reached", diff --git a/apps/frontend/package-lock.json b/apps/frontend/package-lock.json index 9378a744..ce812372 100644 --- a/apps/frontend/package-lock.json +++ b/apps/frontend/package-lock.json @@ -51,6 +51,7 @@ "remark-gfm": "^4.0.1", "tailwind-merge": "^3.4.0", "uuid": "^13.0.0", + "zod": "^4.2.1", "zustand": "^5.0.9" }, "devDependencies": { @@ -15871,10 +15872,9 @@ } }, "node_modules/zod": { - "version": "4.2.0", - "resolved": "https://registry.npmjs.org/zod/-/zod-4.2.0.tgz", - "integrity": "sha512-Bd5fw9wlIhtqCCxotZgdTOMwGm1a0u75wARVEY9HMs1X17trvA/lMi4+MGK5EUfYkXVTbX8UDiDKW4OgzHVUZw==", - "dev": true, + "version": "4.2.1", + "resolved": "https://registry.npmjs.org/zod/-/zod-4.2.1.tgz", + "integrity": "sha512-0wZ1IRqGGhMP76gLqz8EyfBXKk0J2qo2+H3fi4mcUP/KtTocoX08nmIAHl1Z2kJIZbZee8KOpBCSNPRgauucjw==", "license": "MIT", "peer": true, "funding": { diff --git a/apps/frontend/package.json b/apps/frontend/package.json index c40be042..008d27ec 100644 --- a/apps/frontend/package.json +++ b/apps/frontend/package.json @@ -88,6 +88,7 @@ "remark-gfm": "^4.0.1", "tailwind-merge": "^3.4.0", "uuid": "^13.0.0", + "zod": "^4.2.1", "zustand": "^5.0.9" }, "devDependencies": { diff --git a/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts b/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts index 7ad289e2..1ef0da9d 100644 --- a/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts +++ b/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts @@ -42,6 +42,15 @@ vi.mock('../../main/claude-profile-manager', () => ({ }) })); +// Mock validatePythonPath to allow test paths (security validation is tested separately) +vi.mock('../../main/python-detector', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + validatePythonPath: (path: string) => ({ valid: true, sanitizedPath: path }) + }; +}); + // Auto-claude source path (for getAutoBuildSourcePath to find) const AUTO_CLAUDE_SOURCE = path.join(TEST_DIR, 'auto-claude-source'); @@ -197,10 +206,10 @@ describe('Subprocess Spawn Integration', () => { manager.startSpecCreation('task-1', TEST_PROJECT_PATH, 'Test'); - // Simulate stdout data - mockStdout.emit('data', Buffer.from('Test log output')); + // Simulate stdout data (must include newline for buffered output processing) + mockStdout.emit('data', Buffer.from('Test log output\n')); - expect(logHandler).toHaveBeenCalledWith('task-1', 'Test log output'); + expect(logHandler).toHaveBeenCalledWith('task-1', 'Test log output\n'); }); it('should emit log events from stderr', async () => { @@ -213,10 +222,10 @@ describe('Subprocess Spawn Integration', () => { manager.startSpecCreation('task-1', TEST_PROJECT_PATH, 'Test'); - // Simulate stderr data (Python progress output often goes here) - mockStderr.emit('data', Buffer.from('Progress: 50%')); + // Simulate stderr data (must include newline for buffered output processing) + mockStderr.emit('data', Buffer.from('Progress: 50%\n')); - expect(logHandler).toHaveBeenCalledWith('task-1', 'Progress: 50%'); + expect(logHandler).toHaveBeenCalledWith('task-1', 'Progress: 50%\n'); }); it('should emit exit event when process exits', async () => { diff --git a/apps/frontend/src/main/__tests__/agent-events.test.ts b/apps/frontend/src/main/__tests__/agent-events.test.ts new file mode 100644 index 00000000..fb54903c --- /dev/null +++ b/apps/frontend/src/main/__tests__/agent-events.test.ts @@ -0,0 +1,532 @@ +/** + * Agent Events Tests + * =================== + * Tests phase transition logic, regression prevention, and fallback text matching. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AgentEvents } from '../agent/agent-events'; +import type { ExecutionProgressData } from '../agent/types'; + +describe('AgentEvents', () => { + let agentEvents: AgentEvents; + + beforeEach(() => { + agentEvents = new AgentEvents(); + }); + + describe('parseExecutionPhase', () => { + describe('Structured Event Priority', () => { + it('should prioritize structured events over text matching', () => { + // Line contains both structured event and text that would match fallback + const line = '__EXEC_PHASE__:{"phase":"complete","message":"Done"} also contains qa reviewer text'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).toBe('complete'); + expect(result?.message).toBe('Done'); + }); + + it('should use structured event phase value', () => { + const line = '__EXEC_PHASE__:{"phase":"qa_fixing","message":"Fixing issues"}'; + const result = agentEvents.parseExecutionPhase(line, 'qa_review', false); + + expect(result?.phase).toBe('qa_fixing'); + }); + + it('should pass through message from structured event', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Custom message here"}'; + const result = agentEvents.parseExecutionPhase(line, 'planning', false); + + expect(result?.message).toBe('Custom message here'); + }); + + it('should pass through subtask from structured event', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","subtask":"task-123"}'; + const result = agentEvents.parseExecutionPhase(line, 'planning', false); + + expect(result?.currentSubtask).toBe('task-123'); + }); + }); + + describe('Phase Regression Prevention', () => { + it('should not regress from qa_review to coding via fallback', () => { + const line = 'coder agent starting'; // Would normally trigger coding phase + const result = agentEvents.parseExecutionPhase(line, 'qa_review', false); + + // Should not change phase backwards + expect(result).toBeNull(); + }); + + it('should not regress from qa_fixing to coding via fallback', () => { + const line = 'starting coder'; + const result = agentEvents.parseExecutionPhase(line, 'qa_fixing', false); + + expect(result).toBeNull(); + }); + + it('should not regress from qa_review to planning via fallback', () => { + const line = 'planner agent running'; + const result = agentEvents.parseExecutionPhase(line, 'qa_review', false); + + expect(result).toBeNull(); + }); + + it('should not change complete phase via fallback', () => { + const line = 'coder agent starting new work'; + const result = agentEvents.parseExecutionPhase(line, 'complete', false); + + expect(result).toBeNull(); + }); + + it('should not change failed phase via fallback', () => { + const line = 'starting qa reviewer'; + const result = agentEvents.parseExecutionPhase(line, 'failed', false); + + expect(result).toBeNull(); + }); + + it('should allow forward progression via fallback', () => { + const line = 'starting qa reviewer'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).toBe('qa_review'); + }); + + it('should allow structured events to set any phase (override regression)', () => { + // Structured events are authoritative and can set any phase + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Back to coding"}'; + const result = agentEvents.parseExecutionPhase(line, 'qa_review', false); + + // Structured events bypass regression check + expect(result?.phase).toBe('coding'); + }); + }); + + describe('Fallback Text Matching - Planning Phase', () => { + it('should detect planning phase from planner agent text', () => { + const line = 'Starting planner agent...'; + const result = agentEvents.parseExecutionPhase(line, 'idle', false); + + expect(result?.phase).toBe('planning'); + }); + + it('should detect planning phase from creating implementation plan', () => { + const line = 'Creating implementation plan for feature'; + const result = agentEvents.parseExecutionPhase(line, 'idle', false); + + expect(result?.phase).toBe('planning'); + }); + }); + + describe('Fallback Text Matching - Coding Phase', () => { + it('should detect coding phase from coder agent text', () => { + const line = 'Coder agent processing subtask'; + const result = agentEvents.parseExecutionPhase(line, 'planning', false); + + expect(result?.phase).toBe('coding'); + }); + + it('should detect coding phase from starting coder text', () => { + const line = 'Starting coder for implementation'; + const result = agentEvents.parseExecutionPhase(line, 'planning', false); + + expect(result?.phase).toBe('coding'); + }); + + it('should detect subtask progress', () => { + const line = 'Working on subtask: 2/5'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).toBe('coding'); + expect(result?.currentSubtask).toBe('2/5'); + }); + + it('should detect subtask completion', () => { + const line = 'Subtask completed successfully'; + const result = agentEvents.parseExecutionPhase(line, 'planning', false); + + expect(result?.phase).toBe('coding'); + }); + }); + + describe('Fallback Text Matching - QA Phases', () => { + it('should detect qa_review phase from qa reviewer text', () => { + const line = 'Starting QA reviewer agent'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).toBe('qa_review'); + }); + + it('should detect qa_review phase from qa_reviewer text', () => { + const line = 'qa_reviewer checking acceptance criteria'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).toBe('qa_review'); + }); + + it('should detect qa_review phase from starting qa text', () => { + const line = 'Starting QA validation'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).toBe('qa_review'); + }); + + it('should detect qa_fixing phase from qa fixer text', () => { + const line = 'QA fixer processing issues'; + const result = agentEvents.parseExecutionPhase(line, 'qa_review', false); + + expect(result?.phase).toBe('qa_fixing'); + }); + + it('should detect qa_fixing phase from fixing issues text', () => { + const line = 'Fixing issues found by QA'; + const result = agentEvents.parseExecutionPhase(line, 'qa_review', false); + + expect(result?.phase).toBe('qa_fixing'); + }); + }); + + describe('Fallback Text Matching - Complete Phase (IMPORTANT)', () => { + it('should NOT set complete from BUILD COMPLETE banner', () => { + // This is critical - the BUILD COMPLETE banner appears after subtasks + // finish but BEFORE QA runs. We must NOT set complete phase from this. + const line = '=== BUILD COMPLETE ==='; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + // Should NOT return complete phase + expect(result?.phase).not.toBe('complete'); + }); + + it('should NOT set complete from qa passed text via fallback', () => { + // Complete phase should only come from structured events + const line = 'qa passed successfully'; + const result = agentEvents.parseExecutionPhase(line, 'qa_review', false); + + // Fallback should not set complete + expect(result?.phase).not.toBe('complete'); + }); + + it('should NOT set complete from all subtasks completed text', () => { + const line = 'All subtasks completed'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).not.toBe('complete'); + }); + }); + + describe('Fallback Text Matching - Failed Phase', () => { + it('should detect failed phase from build failed text', () => { + const line = 'Build failed: compilation error'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).toBe('failed'); + }); + + it('should detect failed phase from fatal error text', () => { + const line = 'Fatal error: unable to continue'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).toBe('failed'); + }); + + it('should detect failed phase from agent failed text', () => { + const line = 'Agent failed to complete task'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).toBe('failed'); + }); + + it('should NOT detect failed from tool errors', () => { + // Tool errors are recoverable and shouldn't trigger failed phase + const line = 'Tool error: file not found'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).not.toBe('failed'); + }); + + it('should NOT detect failed from tool_use_error', () => { + const line = 'tool_use_error: invalid arguments'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).not.toBe('failed'); + }); + }); + + describe('Task Logger Filtering', () => { + it('should ignore __TASK_LOG_ events', () => { + const line = '__TASK_LOG_:{"type":"subtask_start","id":"1"}'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result).toBeNull(); + }); + + it('should ignore lines containing __TASK_LOG_', () => { + const line = 'Processing __TASK_LOG_:{"event":"progress"}'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result).toBeNull(); + }); + }); + + describe('Spec Runner Mode', () => { + it('should detect discovering phase in spec runner mode', () => { + const line = 'Discovering project structure...'; + const result = agentEvents.parseExecutionPhase(line, 'idle', true); + + expect(result?.phase).toBe('planning'); + expect(result?.message).toContain('Discovering'); + }); + + it('should detect requirements gathering in spec runner mode', () => { + const line = 'Gathering requirements from user'; + const result = agentEvents.parseExecutionPhase(line, 'idle', true); + + expect(result?.phase).toBe('planning'); + expect(result?.message).toContain('requirements'); + }); + + it('should detect spec writing in spec runner mode', () => { + const line = 'Writing spec document...'; + const result = agentEvents.parseExecutionPhase(line, 'idle', true); + + expect(result?.phase).toBe('planning'); + }); + + it('should detect validation in spec runner mode', () => { + const line = 'Validating specification...'; + const result = agentEvents.parseExecutionPhase(line, 'idle', true); + + expect(result?.phase).toBe('planning'); + }); + + it('should detect spec complete in spec runner mode', () => { + const line = 'Spec complete, ready for implementation'; + const result = agentEvents.parseExecutionPhase(line, 'idle', true); + + expect(result?.phase).toBe('planning'); + }); + }); + + describe('Case Insensitivity', () => { + it('should match regardless of case', () => { + const line = 'CODER AGENT Starting'; + const result = agentEvents.parseExecutionPhase(line, 'planning', false); + + expect(result?.phase).toBe('coding'); + }); + + it('should match mixed case', () => { + const line = 'QA Reviewer starting validation'; + const result = agentEvents.parseExecutionPhase(line, 'coding', false); + + expect(result?.phase).toBe('qa_review'); + }); + }); + + describe('Edge Cases', () => { + it('should return null for empty string', () => { + const result = agentEvents.parseExecutionPhase('', 'coding', false); + expect(result).toBeNull(); + }); + + it('should return null for whitespace only', () => { + const result = agentEvents.parseExecutionPhase(' \n\t ', 'coding', false); + expect(result).toBeNull(); + }); + + it('should handle very long log lines', () => { + const longMessage = 'x'.repeat(10000); + const line = `Starting coder ${longMessage}`; + const result = agentEvents.parseExecutionPhase(line, 'planning', false); + + expect(result?.phase).toBe('coding'); + }); + }); + }); + + describe('calculateOverallProgress', () => { + it('should return 0 for idle phase', () => { + const progress = agentEvents.calculateOverallProgress('idle', 50); + expect(progress).toBe(0); + }); + + it('should calculate planning phase progress (0-20%)', () => { + expect(agentEvents.calculateOverallProgress('planning', 0)).toBe(0); + expect(agentEvents.calculateOverallProgress('planning', 50)).toBe(10); + expect(agentEvents.calculateOverallProgress('planning', 100)).toBe(20); + }); + + it('should calculate coding phase progress (20-80%)', () => { + expect(agentEvents.calculateOverallProgress('coding', 0)).toBe(20); + expect(agentEvents.calculateOverallProgress('coding', 50)).toBe(50); + expect(agentEvents.calculateOverallProgress('coding', 100)).toBe(80); + }); + + it('should calculate qa_review phase progress (80-95%)', () => { + expect(agentEvents.calculateOverallProgress('qa_review', 0)).toBe(80); + expect(agentEvents.calculateOverallProgress('qa_review', 100)).toBe(95); + }); + + it('should calculate qa_fixing phase progress (80-95%)', () => { + expect(agentEvents.calculateOverallProgress('qa_fixing', 0)).toBe(80); + expect(agentEvents.calculateOverallProgress('qa_fixing', 100)).toBe(95); + }); + + it('should return 100 for complete phase', () => { + expect(agentEvents.calculateOverallProgress('complete', 0)).toBe(100); + expect(agentEvents.calculateOverallProgress('complete', 100)).toBe(100); + }); + + it('should return 0 for failed phase', () => { + expect(agentEvents.calculateOverallProgress('failed', 50)).toBe(0); + }); + + it('should handle unknown phase gracefully', () => { + const progress = agentEvents.calculateOverallProgress('unknown' as ExecutionProgressData['phase'], 50); + expect(progress).toBe(0); + }); + }); + + describe('parseIdeationProgress', () => { + it('should detect analyzing phase', () => { + const completedTypes = new Set(); + const result = agentEvents.parseIdeationProgress( + 'PROJECT ANALYSIS starting', + 'idle', + 0, + completedTypes, + 5 + ); + + expect(result.phase).toBe('analyzing'); + expect(result.progress).toBe(10); + }); + + it('should detect discovering phase', () => { + const completedTypes = new Set(); + const result = agentEvents.parseIdeationProgress( + 'CONTEXT GATHERING in progress', + 'analyzing', + 10, + completedTypes, + 5 + ); + + expect(result.phase).toBe('discovering'); + expect(result.progress).toBe(20); + }); + + it('should detect generating phase', () => { + const completedTypes = new Set(); + const result = agentEvents.parseIdeationProgress( + 'GENERATING IDEAS (PARALLEL)', + 'discovering', + 20, + completedTypes, + 5 + ); + + expect(result.phase).toBe('generating'); + expect(result.progress).toBe(30); + }); + + it('should update progress based on completed types', () => { + const completedTypes = new Set(['security', 'performance']); + const result = agentEvents.parseIdeationProgress( + 'Still generating...', + 'generating', + 30, + completedTypes, + 5 + ); + + // 30% + (2/5 * 60%) = 30% + 24% = 54% + expect(result.progress).toBe(54); + }); + + it('should detect finalizing phase', () => { + const completedTypes = new Set(); + const result = agentEvents.parseIdeationProgress( + 'MERGE AND FINALIZE', + 'generating', + 60, + completedTypes, + 5 + ); + + expect(result.phase).toBe('finalizing'); + expect(result.progress).toBe(90); + }); + + it('should detect complete phase', () => { + const completedTypes = new Set(); + const result = agentEvents.parseIdeationProgress( + 'IDEATION COMPLETE', + 'finalizing', + 90, + completedTypes, + 5 + ); + + expect(result.phase).toBe('complete'); + expect(result.progress).toBe(100); + }); + }); + + describe('parseRoadmapProgress', () => { + it('should detect analyzing phase', () => { + const result = agentEvents.parseRoadmapProgress( + 'PROJECT ANALYSIS starting', + 'idle', + 0 + ); + + expect(result.phase).toBe('analyzing'); + expect(result.progress).toBe(20); + }); + + it('should detect discovering phase', () => { + const result = agentEvents.parseRoadmapProgress( + 'PROJECT DISCOVERY in progress', + 'analyzing', + 20 + ); + + expect(result.phase).toBe('discovering'); + expect(result.progress).toBe(40); + }); + + it('should detect generating phase', () => { + const result = agentEvents.parseRoadmapProgress( + 'FEATURE GENERATION starting', + 'discovering', + 40 + ); + + expect(result.phase).toBe('generating'); + expect(result.progress).toBe(70); + }); + + it('should detect complete phase', () => { + const result = agentEvents.parseRoadmapProgress( + 'ROADMAP GENERATED successfully', + 'generating', + 70 + ); + + expect(result.phase).toBe('complete'); + expect(result.progress).toBe(100); + }); + + it('should maintain current state for unrecognized log', () => { + const result = agentEvents.parseRoadmapProgress( + 'Some random log message', + 'analyzing', + 25 + ); + + expect(result.phase).toBe('analyzing'); + expect(result.progress).toBe(25); + }); + }); +}); diff --git a/apps/frontend/src/main/__tests__/ipc-handlers.test.ts b/apps/frontend/src/main/__tests__/ipc-handlers.test.ts index 1fca1808..3367b3e6 100644 --- a/apps/frontend/src/main/__tests__/ipc-handlers.test.ts +++ b/apps/frontend/src/main/__tests__/ipc-handlers.test.ts @@ -4,11 +4,12 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { EventEmitter } from 'events'; -import { mkdirSync, writeFileSync, rmSync, existsSync } from 'fs'; +import { mkdirSync, mkdtempSync, writeFileSync, rmSync, existsSync } from 'fs'; +import { tmpdir } from 'os'; import path from 'path'; // Test data directory -const TEST_DIR = '/tmp/ipc-handlers-test'; +const TEST_DIR = mkdtempSync(path.join(tmpdir(), 'ipc-handlers-test-')); const TEST_PROJECT_PATH = path.join(TEST_DIR, 'test-project'); // Mock electron-updater before importing @@ -47,6 +48,14 @@ vi.mock('../updater/version-manager', () => ({ compareVersions: vi.fn(() => 0) })); +vi.mock('../notification-service', () => ({ + notificationService: { + initialize: vi.fn(), + notifyReviewNeeded: vi.fn(), + notifyTaskFailed: vi.fn() + } +})); + // Mock modules before importing vi.mock('electron', () => { const mockIpcMain = new (class extends EventEmitter { @@ -503,12 +512,22 @@ describe('IPC Handlers', () => { ); }); - it('should forward exit events with status change', async () => { + it('should forward exit events with status change on failure', async () => { const { setupIpcHandlers } = await import('../ipc-handlers'); setupIpcHandlers(mockAgentManager as never, mockTerminalManager as never, () => mockMainWindow as never, mockPythonEnvManager as never); - // Exit event with task-execution processType should result in human_review status - mockAgentManager.emit('exit', 'task-1', 0, 'task-execution'); + // Add project first + await ipcMain.invokeHandler('project:add', {}, TEST_PROJECT_PATH); + + // Create a spec/task directory with implementation_plan.json + const specDir = path.join(TEST_PROJECT_PATH, '.auto-claude', 'specs', 'task-1'); + mkdirSync(specDir, { recursive: true }); + writeFileSync( + path.join(specDir, 'implementation_plan.json'), + JSON.stringify({ feature: 'Test Task', status: 'in_progress' }) + ); + + mockAgentManager.emit('exit', 'task-1', 1, 'task-execution'); expect(mockMainWindow.webContents.send).toHaveBeenCalledWith( 'task:statusChange', diff --git a/apps/frontend/src/main/__tests__/parsers.test.ts b/apps/frontend/src/main/__tests__/parsers.test.ts new file mode 100644 index 00000000..3e2babde --- /dev/null +++ b/apps/frontend/src/main/__tests__/parsers.test.ts @@ -0,0 +1,351 @@ +/** + * Phase Parsers Tests + * ==================== + * Unit tests for the specialized phase parsers. + */ + +import { describe, it, expect } from 'vitest'; +import { + ExecutionPhaseParser, + IdeationPhaseParser, + RoadmapPhaseParser, + type ExecutionParserContext, + type IdeationParserContext +} from '../agent/parsers'; + +describe('ExecutionPhaseParser', () => { + const parser = new ExecutionPhaseParser(); + + const makeContext = ( + currentPhase: ExecutionParserContext['currentPhase'], + isSpecRunner = false + ): ExecutionParserContext => ({ + currentPhase, + isTerminal: currentPhase === 'complete' || currentPhase === 'failed', + isSpecRunner + }); + + describe('structured event parsing', () => { + it('should parse structured phase events', () => { + const log = '__EXEC_PHASE__:{"phase":"coding","message":"Starting implementation"}'; + const result = parser.parse(log, makeContext('planning')); + + expect(result).toEqual({ + phase: 'coding', + message: 'Starting implementation', + currentSubtask: undefined + }); + }); + + it('should parse structured events with subtask', () => { + const log = '__EXEC_PHASE__:{"phase":"coding","message":"Working","subtask":"auth-1"}'; + const result = parser.parse(log, makeContext('coding')); + + expect(result).toEqual({ + phase: 'coding', + message: 'Working', + currentSubtask: 'auth-1' + }); + }); + }); + + describe('terminal state handling', () => { + it('should not change phase when current phase is complete', () => { + const log = 'Starting coder agent...'; + const result = parser.parse(log, makeContext('complete')); + + expect(result).toBeNull(); + }); + + it('should not change phase when current phase is failed', () => { + const log = 'QA Reviewer starting...'; + const result = parser.parse(log, makeContext('failed')); + + expect(result).toBeNull(); + }); + + it('should still parse structured events in terminal state', () => { + // Structured events are authoritative and can transition away from terminal states + const log = '__EXEC_PHASE__:{"phase":"coding","message":"Retry"}'; + const result = parser.parse(log, makeContext('complete')); + + // The parser returns the structured event; it's up to the caller to decide + expect(result).toEqual({ + phase: 'coding', + message: 'Retry', + currentSubtask: undefined + }); + }); + }); + + describe('spec runner mode', () => { + it('should detect discovery phase', () => { + const log = 'Discovering project structure...'; + const result = parser.parse(log, makeContext('idle', true)); + + expect(result).toEqual({ + phase: 'planning', + message: 'Discovering project context...' + }); + }); + + it('should detect requirements phase', () => { + const log = 'Gathering requirements from user...'; + const result = parser.parse(log, makeContext('planning', true)); + + expect(result).toEqual({ + phase: 'planning', + message: 'Gathering requirements...' + }); + }); + + it('should detect spec writing phase', () => { + const log = 'Writing spec document...'; + const result = parser.parse(log, makeContext('planning', true)); + + expect(result).toEqual({ + phase: 'planning', + message: 'Writing specification...' + }); + }); + }); + + describe('run.py mode', () => { + it('should detect planner agent', () => { + const log = 'Starting planner agent...'; + const result = parser.parse(log, makeContext('idle')); + + expect(result).toEqual({ + phase: 'planning', + message: 'Creating implementation plan...' + }); + }); + + it('should detect coder agent', () => { + const log = 'Starting coder agent for subtask 1'; + const result = parser.parse(log, makeContext('planning')); + + expect(result).toEqual({ + phase: 'coding', + message: 'Implementing code changes...' + }); + }); + + it('should detect QA reviewer', () => { + const log = 'Starting QA Reviewer...'; + const result = parser.parse(log, makeContext('coding')); + + expect(result).toEqual({ + phase: 'qa_review', + message: 'Running QA review...' + }); + }); + + it('should detect QA fixer', () => { + const log = 'Starting QA Fixer to address issues...'; + const result = parser.parse(log, makeContext('qa_review')); + + expect(result).toEqual({ + phase: 'qa_fixing', + message: 'Fixing QA issues...' + }); + }); + + it('should detect build failure', () => { + const log = 'Build failed: compilation error'; + const result = parser.parse(log, makeContext('coding')); + + expect(result?.phase).toBe('failed'); + expect(result?.message).toContain('Build failed'); + }); + }); + + describe('regression prevention', () => { + it('should not regress from qa_review to coding', () => { + const log = 'Starting coder agent...'; + const result = parser.parse(log, makeContext('qa_review')); + + expect(result).toBeNull(); + }); + + it('should allow qa_fixing to qa_review transition (re-review after fix)', () => { + const log = 'Starting QA Reviewer...'; + const result = parser.parse(log, makeContext('qa_fixing')); + + // QA reviewer in qa_fixing is normal - it's checking the fix + expect(result?.phase).toBe('qa_review'); + }); + }); + + describe('subtask detection', () => { + it('should detect subtask progress in coding phase', () => { + const log = 'Working on subtask: 2/5'; + const result = parser.parse(log, makeContext('coding')); + + expect(result).toEqual({ + phase: 'coding', + currentSubtask: '2/5', + message: 'Working on subtask 2/5...' + }); + }); + + it('should not detect subtask in non-coding phase', () => { + const log = 'Subtask: 1/3'; + const result = parser.parse(log, makeContext('planning')); + + expect(result).toBeNull(); + }); + }); + + describe('internal event filtering', () => { + it('should ignore task logger events', () => { + const log = '__TASK_LOG__:{"event":"progress","data":{}}'; + const result = parser.parse(log, makeContext('coding')); + + expect(result).toBeNull(); + }); + }); +}); + +describe('IdeationPhaseParser', () => { + const parser = new IdeationPhaseParser(); + + const makeContext = ( + currentPhase: IdeationParserContext['currentPhase'], + completedTypes = new Set(), + totalTypes = 5 + ): IdeationParserContext => ({ + currentPhase, + isTerminal: currentPhase === 'complete', + completedTypes, + totalTypes + }); + + describe('phase detection', () => { + it('should detect analyzing phase', () => { + const log = 'Starting PROJECT ANALYSIS...'; + const result = parser.parse(log, makeContext('idle')); + + expect(result).toEqual({ + phase: 'analyzing', + progress: 10 + }); + }); + + it('should detect discovering phase', () => { + const log = 'CONTEXT GATHERING in progress...'; + const result = parser.parse(log, makeContext('analyzing')); + + expect(result).toEqual({ + phase: 'discovering', + progress: 20 + }); + }); + + it('should detect generating phase', () => { + const log = 'GENERATING IDEAS (PARALLEL)...'; + const result = parser.parse(log, makeContext('discovering')); + + expect(result).toEqual({ + phase: 'generating', + progress: 30 + }); + }); + + it('should detect finalizing phase', () => { + const log = 'MERGE results from all agents...'; + const result = parser.parse(log, makeContext('generating')); + + expect(result).toEqual({ + phase: 'finalizing', + progress: 90 + }); + }); + + it('should detect complete phase', () => { + const log = 'IDEATION COMPLETE'; + const result = parser.parse(log, makeContext('finalizing')); + + expect(result).toEqual({ + phase: 'complete', + progress: 100 + }); + }); + }); + + describe('progress calculation', () => { + it('should calculate progress based on completed types', () => { + const completedTypes = new Set(['perf', 'security']); + const result = parser.parse('Some log', makeContext('generating', completedTypes, 5)); + + // 30 + (2/5 * 60) = 30 + 24 = 54 + expect(result?.progress).toBe(54); + }); + + it('should return null when no phase change and no completed types', () => { + const result = parser.parse('Some random log', makeContext('generating')); + + expect(result).toBeNull(); + }); + }); +}); + +describe('RoadmapPhaseParser', () => { + const parser = new RoadmapPhaseParser(); + + const makeContext = (currentPhase: 'idle' | 'analyzing' | 'discovering' | 'generating' | 'complete') => ({ + currentPhase, + isTerminal: currentPhase === 'complete' + }); + + describe('phase detection', () => { + it('should detect analyzing phase', () => { + const log = 'Starting PROJECT ANALYSIS...'; + const result = parser.parse(log, makeContext('idle')); + + expect(result).toEqual({ + phase: 'analyzing', + progress: 20 + }); + }); + + it('should detect discovering phase', () => { + const log = 'PROJECT DISCOVERY in progress...'; + const result = parser.parse(log, makeContext('analyzing')); + + expect(result).toEqual({ + phase: 'discovering', + progress: 40 + }); + }); + + it('should detect generating phase', () => { + const log = 'FEATURE GENERATION starting...'; + const result = parser.parse(log, makeContext('discovering')); + + expect(result).toEqual({ + phase: 'generating', + progress: 70 + }); + }); + + it('should detect complete phase', () => { + const log = 'ROADMAP GENERATED successfully'; + const result = parser.parse(log, makeContext('generating')); + + expect(result).toEqual({ + phase: 'complete', + progress: 100 + }); + }); + }); + + describe('terminal state handling', () => { + it('should not change phase when complete', () => { + const log = 'PROJECT ANALYSIS...'; + const result = parser.parse(log, makeContext('complete')); + + expect(result).toBeNull(); + }); + }); +}); diff --git a/apps/frontend/src/main/__tests__/phase-event-parser.test.ts b/apps/frontend/src/main/__tests__/phase-event-parser.test.ts new file mode 100644 index 00000000..20d20cab --- /dev/null +++ b/apps/frontend/src/main/__tests__/phase-event-parser.test.ts @@ -0,0 +1,421 @@ +/** + * Phase Event Parser Tests + * ========================= + * Tests the parser for __EXEC_PHASE__ protocol between Python backend and TypeScript frontend. + */ + +import { describe, it, expect } from 'vitest'; +import { + parsePhaseEvent, + hasPhaseMarker, + PHASE_MARKER_PREFIX +} from '../agent/phase-event-parser'; + +describe('Phase Event Parser', () => { + describe('PHASE_MARKER_PREFIX', () => { + it('should have correct value', () => { + expect(PHASE_MARKER_PREFIX).toBe('__EXEC_PHASE__:'); + }); + + it('should end with colon', () => { + expect(PHASE_MARKER_PREFIX.endsWith(':')).toBe(true); + }); + }); + + describe('parsePhaseEvent', () => { + describe('Basic Parsing', () => { + it('should parse valid phase event', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Starting implementation"}'; + const result = parsePhaseEvent(line); + + expect(result).not.toBeNull(); + expect(result?.phase).toBe('coding'); + expect(result?.message).toBe('Starting implementation'); + }); + + it('should return null for line without marker', () => { + const line = 'Just a regular log line'; + const result = parsePhaseEvent(line); + + expect(result).toBeNull(); + }); + + it('should handle marker at start of line', () => { + const line = '__EXEC_PHASE__:{"phase":"planning","message":"Creating plan"}'; + const result = parsePhaseEvent(line); + + expect(result).not.toBeNull(); + expect(result?.phase).toBe('planning'); + }); + + it('should handle marker with prefix text', () => { + const line = '[2024-01-01 12:00:00] INFO: __EXEC_PHASE__:{"phase":"coding","message":"Working"}'; + const result = parsePhaseEvent(line); + + expect(result).not.toBeNull(); + expect(result?.phase).toBe('coding'); + }); + + it('should handle ANSI color codes around JSON by extracting valid JSON', () => { + const line = '\x1b[32m__EXEC_PHASE__:{"phase":"coding","message":"Test"}\x1b[0m'; + const result = parsePhaseEvent(line); + + expect(result).not.toBeNull(); + expect(result?.phase).toBe('coding'); + expect(result?.message).toBe('Test'); + }); + }); + + describe('Phase Validation', () => { + it('should accept planning phase', () => { + const line = '__EXEC_PHASE__:{"phase":"planning","message":""}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('planning'); + }); + + it('should accept coding phase', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":""}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('coding'); + }); + + it('should accept qa_review phase', () => { + const line = '__EXEC_PHASE__:{"phase":"qa_review","message":""}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('qa_review'); + }); + + it('should accept qa_fixing phase', () => { + const line = '__EXEC_PHASE__:{"phase":"qa_fixing","message":""}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('qa_fixing'); + }); + + it('should accept complete phase', () => { + const line = '__EXEC_PHASE__:{"phase":"complete","message":""}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('complete'); + }); + + it('should accept failed phase', () => { + const line = '__EXEC_PHASE__:{"phase":"failed","message":""}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('failed'); + }); + + it('should reject unknown phase value', () => { + const line = '__EXEC_PHASE__:{"phase":"unknown_phase","message":""}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should reject uppercase phase value', () => { + const line = '__EXEC_PHASE__:{"phase":"CODING","message":""}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should reject numeric phase value', () => { + const line = '__EXEC_PHASE__:{"phase":123,"message":""}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should reject null phase value', () => { + const line = '__EXEC_PHASE__:{"phase":null,"message":""}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + }); + + describe('Message Handling', () => { + it('should extract message field', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Building feature X"}'; + const result = parsePhaseEvent(line); + expect(result?.message).toBe('Building feature X'); + }); + + it('should handle empty message', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":""}'; + const result = parsePhaseEvent(line); + expect(result?.message).toBe(''); + }); + + it('should default to empty string for missing message', () => { + const line = '__EXEC_PHASE__:{"phase":"coding"}'; + const result = parsePhaseEvent(line); + expect(result?.message).toBe(''); + }); + + it('should handle unicode in message', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Building πŸš€ feature with Γ©mojis"}'; + const result = parsePhaseEvent(line); + expect(result?.message).toContain('πŸš€'); + expect(result?.message).toContain('Γ©mojis'); + }); + + it('should handle escaped quotes in message', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Message with \\"quotes\\""}'; + const result = parsePhaseEvent(line); + expect(result?.message).toContain('"quotes"'); + }); + + it('should handle escaped newlines in message (JSON format)', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Line1\\nLine2"}'; + const result = parsePhaseEvent(line); + expect(result?.message).toBe('Line1\nLine2'); + }); + + it('should handle escaped backslashes', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"path\\\\to\\\\file"}'; + const result = parsePhaseEvent(line); + expect(result?.message).toBe('path\\to\\file'); + }); + + it('should reject non-string message', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":123}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + }); + + describe('Optional Fields', () => { + it('should extract progress when present', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","progress":50}'; + const result = parsePhaseEvent(line); + expect(result?.progress).toBe(50); + }); + + it('should not include progress when not present', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working"}'; + const result = parsePhaseEvent(line); + expect(result?.progress).toBeUndefined(); + }); + + it('should handle progress of 0', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Starting","progress":0}'; + const result = parsePhaseEvent(line); + expect(result?.progress).toBe(0); + }); + + it('should handle progress of 100', () => { + const line = '__EXEC_PHASE__:{"phase":"complete","message":"Done","progress":100}'; + const result = parsePhaseEvent(line); + expect(result?.progress).toBe(100); + }); + + it('should reject non-numeric progress', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","progress":"50%"}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should reject progress below 0', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","progress":-1}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should reject progress above 100', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","progress":101}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should reject non-integer progress', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","progress":50.5}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should extract subtask when present', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","subtask":"task-123"}'; + const result = parsePhaseEvent(line); + expect(result?.subtask).toBe('task-123'); + }); + + it('should not include subtask when not present', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working"}'; + const result = parsePhaseEvent(line); + expect(result?.subtask).toBeUndefined(); + }); + + it('should handle subtask with special characters', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","subtask":"feat/add-login#123"}'; + const result = parsePhaseEvent(line); + expect(result?.subtask).toBe('feat/add-login#123'); + }); + + it('should reject non-string subtask', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","subtask":123}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should handle all optional fields together', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","progress":75,"subtask":"feat-1"}'; + const result = parsePhaseEvent(line); + expect(result?.progress).toBe(75); + expect(result?.subtask).toBe('feat-1'); + }); + + it('should ignore unknown fields', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Working","unknown":"field","extra":123}'; + const result = parsePhaseEvent(line); + expect(result).not.toBeNull(); + expect(result?.phase).toBe('coding'); + expect(result).not.toHaveProperty('unknown'); + }); + }); + + describe('Error Handling', () => { + it('should return null for invalid JSON', () => { + const line = '__EXEC_PHASE__:{invalid json}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should return null for empty JSON string', () => { + const line = '__EXEC_PHASE__:'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should return null for non-object JSON', () => { + const line = '__EXEC_PHASE__:"just a string"'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should return null for JSON array', () => { + const line = '__EXEC_PHASE__:["phase","coding"]'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should return null for truncated JSON', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Trun'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should return null for JSON without phase field', () => { + const line = '__EXEC_PHASE__:{"message":"No phase field"}'; + const result = parsePhaseEvent(line); + expect(result).toBeNull(); + }); + + it('should handle whitespace after marker', () => { + const line = '__EXEC_PHASE__: {"phase":"coding","message":"With spaces"}'; + const result = parsePhaseEvent(line); + expect(result).not.toBeNull(); + expect(result?.phase).toBe('coding'); + }); + + it('should handle trailing whitespace in JSON', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Test"} '; + const result = parsePhaseEvent(line); + expect(result).not.toBeNull(); + }); + }); + + describe('Real-world Scenarios', () => { + it('should parse typical planning event', () => { + const line = '__EXEC_PHASE__:{"phase":"planning","message":"Creating implementation plan"}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('planning'); + expect(result?.message).toBe('Creating implementation plan'); + }); + + it('should parse typical coding event with subtask', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Implementing feature","subtask":"1/3","progress":33}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('coding'); + expect(result?.subtask).toBe('1/3'); + expect(result?.progress).toBe(33); + }); + + it('should parse QA review event', () => { + const line = '__EXEC_PHASE__:{"phase":"qa_review","message":"Running QA validation iteration 1"}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('qa_review'); + }); + + it('should parse QA fixing event', () => { + const line = '__EXEC_PHASE__:{"phase":"qa_fixing","message":"Fixing QA issues"}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('qa_fixing'); + }); + + it('should parse complete event', () => { + const line = '__EXEC_PHASE__:{"phase":"complete","message":"QA validation passed","progress":100}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('complete'); + expect(result?.progress).toBe(100); + }); + + it('should parse failed event with error message', () => { + const line = '__EXEC_PHASE__:{"phase":"failed","message":"Build failed: TypeError: Cannot read property of undefined"}'; + const result = parsePhaseEvent(line); + expect(result?.phase).toBe('failed'); + expect(result?.message).toContain('TypeError'); + }); + }); + }); + + describe('hasPhaseMarker', () => { + it('should return true when marker present at start', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":""}'; + expect(hasPhaseMarker(line)).toBe(true); + }); + + it('should return true when marker present in middle', () => { + const line = 'Some prefix __EXEC_PHASE__:{"phase":"coding","message":""}'; + expect(hasPhaseMarker(line)).toBe(true); + }); + + it('should return false when marker absent', () => { + const line = 'Just a regular log line without marker'; + expect(hasPhaseMarker(line)).toBe(false); + }); + + it('should return false for partial marker', () => { + const line = '__EXEC_PHASE'; + expect(hasPhaseMarker(line)).toBe(false); + }); + + it('should return false for similar but different marker', () => { + const line = '__EXEC_PHASE_:{"phase":"coding"}'; + expect(hasPhaseMarker(line)).toBe(false); + }); + + it('should return false for empty string', () => { + expect(hasPhaseMarker('')).toBe(false); + }); + + it('should be case-sensitive', () => { + const line = '__exec_phase__:{"phase":"coding","message":""}'; + expect(hasPhaseMarker(line)).toBe(false); + }); + }); + + describe('Type Safety', () => { + it('should return correct PhaseEvent type', () => { + const line = '__EXEC_PHASE__:{"phase":"coding","message":"Test","progress":50,"subtask":"t1"}'; + const result = parsePhaseEvent(line); + + // TypeScript compile-time check + if (result) { + const phase: string = result.phase; + const message: string = result.message; + const progress: number | undefined = result.progress; + const subtask: string | undefined = result.subtask; + + expect(phase).toBe('coding'); + expect(message).toBe('Test'); + expect(progress).toBe(50); + expect(subtask).toBe('t1'); + } + }); + }); +}); diff --git a/apps/frontend/src/main/__tests__/phase-event-schema.test.ts b/apps/frontend/src/main/__tests__/phase-event-schema.test.ts new file mode 100644 index 00000000..56f0a7a0 --- /dev/null +++ b/apps/frontend/src/main/__tests__/phase-event-schema.test.ts @@ -0,0 +1,143 @@ +import { describe, it, expect } from 'vitest'; +import { + PhaseEventSchema, + validatePhaseEvent, + isValidPhasePayload, + type PhaseEventPayload +} from '../agent/phase-event-schema'; +import { BACKEND_PHASES } from '../../shared/constants/phase-protocol'; + +describe('Phase Event Schema', () => { + describe('PhaseEventSchema', () => { + it('should parse valid complete payload', () => { + const input = { + phase: 'coding', + message: 'Working on feature', + progress: 50, + subtask: 'task-1' + }; + const result = PhaseEventSchema.safeParse(input); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.phase).toBe('coding'); + expect(result.data.message).toBe('Working on feature'); + expect(result.data.progress).toBe(50); + expect(result.data.subtask).toBe('task-1'); + } + }); + + it('should parse minimal payload with defaults', () => { + const input = { phase: 'planning' }; + const result = PhaseEventSchema.safeParse(input); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.phase).toBe('planning'); + expect(result.data.message).toBe(''); + expect(result.data.progress).toBeUndefined(); + expect(result.data.subtask).toBeUndefined(); + } + }); + + it('should reject invalid phase', () => { + const input = { phase: 'invalid_phase', message: '' }; + const result = PhaseEventSchema.safeParse(input); + expect(result.success).toBe(false); + }); + + it('should reject missing phase', () => { + const input = { message: 'No phase' }; + const result = PhaseEventSchema.safeParse(input); + expect(result.success).toBe(false); + }); + + describe('Progress Validation', () => { + it('should accept progress at 0', () => { + const input = { phase: 'coding', progress: 0 }; + const result = PhaseEventSchema.safeParse(input); + expect(result.success).toBe(true); + }); + + it('should accept progress at 100', () => { + const input = { phase: 'coding', progress: 100 }; + const result = PhaseEventSchema.safeParse(input); + expect(result.success).toBe(true); + }); + + it('should reject progress below 0', () => { + const input = { phase: 'coding', progress: -1 }; + const result = PhaseEventSchema.safeParse(input); + expect(result.success).toBe(false); + }); + + it('should reject progress above 100', () => { + const input = { phase: 'coding', progress: 101 }; + const result = PhaseEventSchema.safeParse(input); + expect(result.success).toBe(false); + }); + + it('should reject non-integer progress', () => { + const input = { phase: 'coding', progress: 50.5 }; + const result = PhaseEventSchema.safeParse(input); + expect(result.success).toBe(false); + }); + }); + }); + + describe('validatePhaseEvent', () => { + it('should return success result for valid payload', () => { + const input = { phase: 'coding', message: 'Test' }; + const result = validatePhaseEvent(input); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.phase).toBe('coding'); + } + }); + + it('should return error result for invalid payload', () => { + const input = { phase: 'invalid', message: 'Test' }; + const result = validatePhaseEvent(input); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toBeDefined(); + } + }); + }); + + describe('isValidPhasePayload', () => { + it('should return true for valid payload', () => { + const input = { phase: 'coding', message: 'Test' }; + expect(isValidPhasePayload(input)).toBe(true); + }); + + it('should return false for invalid payload', () => { + const input = { phase: 'invalid', message: 'Test' }; + expect(isValidPhasePayload(input)).toBe(false); + }); + + it('should return false for null', () => { + expect(isValidPhasePayload(null)).toBe(false); + }); + + it('should return false for undefined', () => { + expect(isValidPhasePayload(undefined)).toBe(false); + }); + + it('should act as type guard', () => { + const input: unknown = { phase: 'coding', message: 'Test' }; + if (isValidPhasePayload(input)) { + const typed: PhaseEventPayload = input; + expect(typed.phase).toBe('coding'); + } + }); + }); + + describe('All Valid Phases', () => { + BACKEND_PHASES.forEach((phase) => { + it(`should accept phase: ${phase}`, () => { + const input = { phase, message: '' }; + const result = PhaseEventSchema.safeParse(input); + expect(result.success).toBe(true); + }); + }); + }); +}); diff --git a/apps/frontend/src/main/agent/agent-events.ts b/apps/frontend/src/main/agent/agent-events.ts index 42b40520..99dd9d6b 100644 --- a/apps/frontend/src/main/agent/agent-events.ts +++ b/apps/frontend/src/main/agent/agent-events.ts @@ -1,17 +1,45 @@ import { ExecutionProgressData } from './types'; +import { parsePhaseEvent } from './phase-event-parser'; +import { + wouldPhaseRegress, + isTerminalPhase, + isValidExecutionPhase, + type ExecutionPhase +} from '../../shared/constants/phase-protocol'; +import { EXECUTION_PHASE_WEIGHTS } from '../../shared/constants/task'; -/** - * Event handling and progress parsing logic - */ export class AgentEvents { - /** - * Parse log output to detect execution phase transitions - */ parseExecutionPhase( log: string, currentPhase: ExecutionProgressData['phase'], isSpecRunner: boolean ): { phase: ExecutionProgressData['phase']; message?: string; currentSubtask?: string } | null { + const structuredEvent = parsePhaseEvent(log); + if (structuredEvent) { + return { + phase: structuredEvent.phase as ExecutionProgressData['phase'], + message: structuredEvent.message, + currentSubtask: structuredEvent.subtask + }; + } + + // Terminal states can't be changed by fallback matching + if (isTerminalPhase(currentPhase as ExecutionPhase)) { + return null; + } + + // Ignore internal task logger events - they're not phase transitions + if (log.includes('__TASK_LOG_')) { + return null; + } + + const checkRegression = (newPhase: string): boolean => { + if (!isValidExecutionPhase(currentPhase) || !isValidExecutionPhase(newPhase)) { + return true; + } + return wouldPhaseRegress(currentPhase, newPhase); + }; + const lowerLog = log.toLowerCase(); // Spec runner phase detection (all part of "planning") @@ -34,24 +62,23 @@ export class AgentEvents { } // Run.py phase detection - // Planner agent running - if (lowerLog.includes('planner agent') || lowerLog.includes('creating implementation plan')) { + if (!checkRegression('planning') && (lowerLog.includes('planner agent') || lowerLog.includes('creating implementation plan'))) { return { phase: 'planning', message: 'Creating implementation plan...' }; } - // Coder agent running - if (lowerLog.includes('coder agent') || lowerLog.includes('starting coder')) { + // Coder agent running - don't regress from QA phases + if (!checkRegression('coding') && (lowerLog.includes('coder agent') || lowerLog.includes('starting coder'))) { return { phase: 'coding', message: 'Implementing code changes...' }; } - // Subtask progress detection + // Subtask progress detection - only when in coding phase const subtaskMatch = log.match(/subtask[:\s]+(\d+(?:\/\d+)?|\w+[-_]\w+)/i); if (subtaskMatch && currentPhase === 'coding') { return { phase: 'coding', currentSubtask: subtaskMatch[1], message: `Working on subtask ${subtaskMatch[1]}...` }; } - // Subtask completion detection - if (lowerLog.includes('subtask completed') || lowerLog.includes('subtask done')) { + // Subtask completion detection - don't regress from QA phases + if (!checkRegression('coding') && (lowerLog.includes('subtask completed') || lowerLog.includes('subtask done'))) { const completedSubtask = log.match(/subtask[:\s]+"?([^"]+)"?\s+completed/i); return { phase: 'coding', @@ -60,61 +87,47 @@ export class AgentEvents { }; } + // QA phases require at least coding phase first (prevents false positives from early logs) + const canEnterQAPhase = currentPhase === 'coding' || currentPhase === 'qa_review' || currentPhase === 'qa_fixing'; + // QA Review phase - if (lowerLog.includes('qa reviewer') || lowerLog.includes('qa_reviewer') || lowerLog.includes('starting qa')) { + if (canEnterQAPhase && (lowerLog.includes('qa reviewer') || lowerLog.includes('qa_reviewer') || lowerLog.includes('starting qa'))) { return { phase: 'qa_review', message: 'Running QA review...' }; } // QA Fixer phase - if (lowerLog.includes('qa fixer') || lowerLog.includes('qa_fixer') || lowerLog.includes('fixing issues')) { + if (canEnterQAPhase && (lowerLog.includes('qa fixer') || lowerLog.includes('qa_fixer') || lowerLog.includes('fixing issues'))) { return { phase: 'qa_fixing', message: 'Fixing QA issues...' }; } - // Completion detection - be conservative, require explicit success markers - // The AI agent prints "=== BUILD COMPLETE ===" when truly done (from coder.md) - // Only trust this pattern, not generic "all subtasks completed" which could be false positive - if (lowerLog.includes('=== build complete ===') || lowerLog.includes('qa passed')) { - return { phase: 'complete', message: 'Build completed successfully' }; - } + // IMPORTANT: Don't set 'complete' phase via fallback text matching! + // The "=== BUILD COMPLETE ===" banner is printed when SUBTASKS finish, + // but QA hasn't run yet. Only the structured emit_phase(COMPLETE) from + // QA approval (in qa/loop.py) should set the complete phase. + // Removing this prevents the brief "Completed" flash before QA review. - // "All subtasks completed" is informational - don't change phase based on this alone - // The coordinator may print this even when subtasks are blocked, so we stay in coding phase - // and let the actual implementation_plan.json status drive the UI - if (lowerLog.includes('all subtasks completed')) { - return { phase: 'coding', message: 'Subtasks marked complete' }; - } - - // Incomplete build detection - when coordinator exits with pending subtasks - if (lowerLog.includes('build incomplete') || lowerLog.includes('subtasks still pending')) { + // Incomplete build detection - don't regress from QA phases + if (!checkRegression('coding') && (lowerLog.includes('build incomplete') || lowerLog.includes('subtasks still pending'))) { return { phase: 'coding', message: 'Build paused - subtasks still pending' }; } - // Error/failure detection - if (lowerLog.includes('build failed') || lowerLog.includes('error:') || lowerLog.includes('fatal')) { + // Error/failure detection - be specific to avoid false positives from tool errors + const isToolError = lowerLog.includes('tool error') || lowerLog.includes('tool_use_error'); + if (!isToolError && (lowerLog.includes('build failed') || lowerLog.includes('fatal error') || lowerLog.includes('agent failed'))) { return { phase: 'failed', message: log.trim().substring(0, 200) }; } return null; } - /** - * Calculate overall progress based on phase and phase progress - */ calculateOverallProgress(phase: ExecutionProgressData['phase'], phaseProgress: number): number { - // Phase weight ranges (same as in constants.ts) - const weights: Record = { - idle: { start: 0, end: 0 }, - planning: { start: 0, end: 20 }, - coding: { start: 20, end: 80 }, - qa_review: { start: 80, end: 95 }, - qa_fixing: { start: 80, end: 95 }, - complete: { start: 100, end: 100 }, - failed: { start: 0, end: 0 } - }; - - const phaseWeight = weights[phase] || { start: 0, end: 0 }; + const phaseWeight = EXECUTION_PHASE_WEIGHTS[phase]; + if (!phaseWeight) { + console.warn(`[AgentEvents] Unknown phase "${phase}" in calculateOverallProgress - defaulting to 0%`); + return 0; + } const phaseRange = phaseWeight.end - phaseWeight.start; - return Math.round(phaseWeight.start + (phaseRange * phaseProgress / 100)); + return Math.round(phaseWeight.start + ((phaseRange * phaseProgress) / 100)); } /** diff --git a/apps/frontend/src/main/agent/agent-process.ts b/apps/frontend/src/main/agent/agent-process.ts index c3442180..fbcb27fa 100644 --- a/apps/frontend/src/main/agent/agent-process.ts +++ b/apps/frontend/src/main/agent/agent-process.ts @@ -9,7 +9,7 @@ import { ProcessType, ExecutionProgressData } from './types'; import { detectRateLimit, createSDKRateLimitInfo, getProfileEnv, detectAuthFailure } from '../rate-limit-detector'; import { projectStore } from '../project-store'; import { getClaudeProfileManager } from '../claude-profile-manager'; -import { parsePythonCommand } from '../python-detector'; +import { parsePythonCommand, validatePythonPath } from '../python-detector'; import { getConfiguredPythonPath } from '../python-env-manager'; /** @@ -30,18 +30,138 @@ export class AgentProcessManager { this.emitter = emitter; } - /** - * Configure paths for Python and auto-claude source - */ configure(pythonPath?: string, autoBuildSourcePath?: string): void { if (pythonPath) { - this._pythonPath = pythonPath; + const validation = validatePythonPath(pythonPath); + if (validation.valid) { + this._pythonPath = validation.sanitizedPath || pythonPath; + } else { + console.error(`[AgentProcess] Invalid Python path rejected: ${validation.reason}`); + console.error(`[AgentProcess] Falling back to getConfiguredPythonPath()`); + // Don't set _pythonPath - let getPythonPath() use getConfiguredPythonPath() fallback + } } if (autoBuildSourcePath) { this.autoBuildSourcePath = autoBuildSourcePath; } } + private setupProcessEnvironment( + extraEnv: Record + ): NodeJS.ProcessEnv { + const profileEnv = getProfileEnv(); + return { + ...process.env, + ...extraEnv, + ...profileEnv, + PYTHONUNBUFFERED: '1', + PYTHONIOENCODING: 'utf-8', + PYTHONUTF8: '1' + } as NodeJS.ProcessEnv; + } + + private handleProcessFailure( + taskId: string, + allOutput: string, + processType: ProcessType + ): boolean { + console.log('[AgentProcess] Checking for rate limit in output (last 500 chars):', allOutput.slice(-500)); + + const rateLimitDetection = detectRateLimit(allOutput); + console.log('[AgentProcess] Rate limit detection result:', { + isRateLimited: rateLimitDetection.isRateLimited, + resetTime: rateLimitDetection.resetTime, + limitType: rateLimitDetection.limitType, + profileId: rateLimitDetection.profileId, + suggestedProfile: rateLimitDetection.suggestedProfile + }); + + if (rateLimitDetection.isRateLimited) { + const wasHandled = this.handleRateLimitWithAutoSwap( + taskId, + rateLimitDetection, + processType + ); + if (wasHandled) return true; + + const source = processType === 'spec-creation' ? 'roadmap' : 'task'; + const rateLimitInfo = createSDKRateLimitInfo(source, rateLimitDetection, { taskId }); + console.log('[AgentProcess] Emitting sdk-rate-limit event (manual):', rateLimitInfo); + this.emitter.emit('sdk-rate-limit', rateLimitInfo); + return true; + } + + return this.handleAuthFailure(taskId, allOutput); + } + + private handleRateLimitWithAutoSwap( + taskId: string, + rateLimitDetection: ReturnType, + processType: ProcessType + ): boolean { + const profileManager = getClaudeProfileManager(); + const autoSwitchSettings = profileManager.getAutoSwitchSettings(); + + console.log('[AgentProcess] Auto-switch settings:', { + enabled: autoSwitchSettings.enabled, + autoSwitchOnRateLimit: autoSwitchSettings.autoSwitchOnRateLimit, + proactiveSwapEnabled: autoSwitchSettings.proactiveSwapEnabled + }); + + if (!autoSwitchSettings.enabled || !autoSwitchSettings.autoSwitchOnRateLimit) { + console.log('[AgentProcess] Auto-switch disabled - showing manual modal'); + return false; + } + + const currentProfileId = rateLimitDetection.profileId; + const bestProfile = profileManager.getBestAvailableProfile(currentProfileId); + + console.log('[AgentProcess] Best available profile:', bestProfile ? { + id: bestProfile.id, + name: bestProfile.name + } : 'NONE'); + + if (!bestProfile) { + console.log('[AgentProcess] No alternative profile available - falling back to manual modal'); + return false; + } + + console.log('[AgentProcess] AUTO-SWAP: Switching from', currentProfileId, 'to', bestProfile.id); + profileManager.setActiveProfile(bestProfile.id); + + const source = processType === 'spec-creation' ? 'roadmap' : 'task'; + const rateLimitInfo = createSDKRateLimitInfo(source, rateLimitDetection, { taskId }); + rateLimitInfo.wasAutoSwapped = true; + rateLimitInfo.swappedToProfile = { id: bestProfile.id, name: bestProfile.name }; + rateLimitInfo.swapReason = 'reactive'; + + console.log('[AgentProcess] Emitting sdk-rate-limit event (auto-swapped):', rateLimitInfo); + this.emitter.emit('sdk-rate-limit', rateLimitInfo); + + console.log('[AgentProcess] Emitting auto-swap-restart-task event for task:', taskId); + this.emitter.emit('auto-swap-restart-task', taskId, bestProfile.id); + return true; + } + + private handleAuthFailure(taskId: string, allOutput: string): boolean { + console.log('[AgentProcess] No rate limit detected - checking for auth failure'); + const authFailureDetection = detectAuthFailure(allOutput); + + if (authFailureDetection.isAuthFailure) { + console.log('[AgentProcess] Auth failure detected:', authFailureDetection); + this.emitter.emit('auth-failure', taskId, { + profileId: authFailureDetection.profileId, + failureType: authFailureDetection.failureType, + message: authFailureDetection.message, + originalError: authFailureDetection.originalError + }); + return true; + } + + console.log('[AgentProcess] Process failed but no rate limit or auth failure detected'); + return false; + } + /** * Get the configured Python path. * Returns explicitly configured path, or falls back to getConfiguredPythonPath() @@ -156,9 +276,6 @@ export class AgentProcessManager { } } - /** - * Spawn a Python process for task execution - */ spawnProcess( taskId: string, cwd: string, @@ -167,28 +284,14 @@ export class AgentProcessManager { processType: ProcessType = 'task-execution' ): void { const isSpecRunner = processType === 'spec-creation'; - // Kill existing process for this task if any this.killProcess(taskId); - // Generate unique spawn ID for this process instance const spawnId = this.state.generateSpawnId(); - - // Get active Claude profile environment (CLAUDE_CONFIG_DIR if not default) - const profileEnv = getProfileEnv(); + const env = this.setupProcessEnvironment(extraEnv); // Parse Python command to handle space-separated commands like "py -3" const [pythonCommand, pythonBaseArgs] = parsePythonCommand(this.getPythonPath()); - const childProcess = spawn(pythonCommand, [...pythonBaseArgs, ...args], { - cwd, - env: { - ...process.env, - ...extraEnv, - ...profileEnv, // Include active Claude profile config - PYTHONUNBUFFERED: '1', // Ensure real-time output - PYTHONIOENCODING: 'utf-8', // Ensure UTF-8 encoding on Windows - PYTHONUTF8: '1' // Force Python UTF-8 mode on Windows (Python 3.7+) - } - }); + const childProcess = spawn(pythonCommand, [...pythonBaseArgs, ...args], { cwd, env }); this.state.addProcess(taskId, { taskId, @@ -197,30 +300,46 @@ export class AgentProcessManager { spawnId }); - // Track execution progress let currentPhase: ExecutionProgressData['phase'] = isSpecRunner ? 'planning' : 'planning'; let phaseProgress = 0; let currentSubtask: string | undefined; let lastMessage: string | undefined; - // Collect all output for rate limit detection let allOutput = ''; + let stdoutBuffer = ''; + let stderrBuffer = ''; + let sequenceNumber = 0; - // Emit initial progress this.emitter.emit('execution-progress', taskId, { phase: currentPhase, phaseProgress: 0, overallProgress: this.events.calculateOverallProgress(currentPhase, 0), - message: isSpecRunner ? 'Starting spec creation...' : 'Starting build process...' + message: isSpecRunner ? 'Starting spec creation...' : 'Starting build process...', + sequenceNumber: ++sequenceNumber }); - const processLog = (log: string) => { - // Collect output for rate limit detection (keep last 10KB) - allOutput = (allOutput + log).slice(-10000); - // Parse for phase transitions - const phaseUpdate = this.events.parseExecutionPhase(log, currentPhase, isSpecRunner); + const isDebug = ['true', '1', 'yes', 'on'].includes(process.env.DEBUG?.toLowerCase() ?? ''); + + const processLog = (line: string) => { + allOutput = (allOutput + line).slice(-10000); + + const hasMarker = line.includes('__EXEC_PHASE__'); + if (isDebug && hasMarker) { + console.log(`[PhaseDebug:${taskId}] Found marker in line: "${line.substring(0, 200)}"`); + } + + const phaseUpdate = this.events.parseExecutionPhase(line, currentPhase, isSpecRunner); + + if (isDebug && hasMarker) { + console.log(`[PhaseDebug:${taskId}] Parse result:`, phaseUpdate); + } if (phaseUpdate) { const phaseChanged = phaseUpdate.phase !== currentPhase; + + if (isDebug) { + console.log(`[PhaseDebug:${taskId}] Phase update: ${currentPhase} -> ${phaseUpdate.phase} (changed: ${phaseChanged})`); + } + currentPhase = phaseUpdate.phase; if (phaseUpdate.currentSubtask) { @@ -230,158 +349,99 @@ export class AgentProcessManager { lastMessage = phaseUpdate.message; } - // Reset phase progress on phase change, otherwise increment if (phaseChanged) { - phaseProgress = 10; // Start new phase at 10% + phaseProgress = 10; } else { - phaseProgress = Math.min(90, phaseProgress + 5); // Increment within phase + phaseProgress = Math.min(90, phaseProgress + 5); } const overallProgress = this.events.calculateOverallProgress(currentPhase, phaseProgress); + if (isDebug) { + console.log(`[PhaseDebug:${taskId}] Emitting execution-progress:`, { phase: currentPhase, phaseProgress, overallProgress }); + } + this.emitter.emit('execution-progress', taskId, { phase: currentPhase, phaseProgress, overallProgress, currentSubtask, - message: lastMessage + message: lastMessage, + sequenceNumber: ++sequenceNumber }); } }; - // Handle stdout - explicitly decode as UTF-8 for cross-platform Unicode support + const processBufferedOutput = (buffer: string, newData: string): string => { + if (isDebug && newData.includes('__EXEC_PHASE__')) { + console.log(`[PhaseDebug:${taskId}] Raw chunk with marker (${newData.length} bytes): "${newData.substring(0, 300)}"`); + console.log(`[PhaseDebug:${taskId}] Current buffer before append (${buffer.length} bytes): "${buffer.substring(0, 100)}"`); + } + + buffer += newData; + const lines = buffer.split('\n'); + const remaining = lines.pop() || ''; + + if (isDebug && newData.includes('__EXEC_PHASE__')) { + console.log(`[PhaseDebug:${taskId}] Split into ${lines.length} complete lines, remaining buffer: "${remaining.substring(0, 100)}"`); + } + + for (const line of lines) { + if (line.trim()) { + this.emitter.emit('log', taskId, line + '\n'); + processLog(line); + if (isDebug) { + console.log(`[Agent:${taskId}] ${line}`); + } + } + } + + return remaining; + }; + childProcess.stdout?.on('data', (data: Buffer) => { - const log = data.toString('utf8'); - this.emitter.emit('log', taskId, log); - processLog(log); - // Print to console when DEBUG is enabled (visible in pnpm dev terminal) - if (['true', '1', 'yes', 'on'].includes(process.env.DEBUG?.toLowerCase() ?? '')) { - console.log(`[Agent:${taskId}] ${log.trim()}`); - } + stdoutBuffer = processBufferedOutput(stdoutBuffer, data.toString('utf8')); }); - // Handle stderr - explicitly decode as UTF-8 for cross-platform Unicode support childProcess.stderr?.on('data', (data: Buffer) => { - const log = data.toString('utf8'); - // Some Python output goes to stderr (like progress bars) - // so we treat it as log, not error - this.emitter.emit('log', taskId, log); - processLog(log); - // Print to console when DEBUG is enabled (visible in pnpm dev terminal) - if (['true', '1', 'yes', 'on'].includes(process.env.DEBUG?.toLowerCase() ?? '')) { - console.log(`[Agent:${taskId}] ${log.trim()}`); - } + stderrBuffer = processBufferedOutput(stderrBuffer, data.toString('utf8')); }); - // Handle process exit childProcess.on('exit', (code: number | null) => { + if (stdoutBuffer.trim()) { + this.emitter.emit('log', taskId, stdoutBuffer + '\n'); + processLog(stdoutBuffer); + } + if (stderrBuffer.trim()) { + this.emitter.emit('log', taskId, stderrBuffer + '\n'); + processLog(stderrBuffer); + } + this.state.deleteProcess(taskId); - // Check if this specific spawn was killed (vs exited naturally) - // If killed, don't emit exit event to prevent race condition with new process if (this.state.wasSpawnKilled(spawnId)) { this.state.clearKilledSpawn(spawnId); return; } - // Check for rate limit if process failed if (code !== 0) { console.log('[AgentProcess] Process failed with code:', code, 'for task:', taskId); - console.log('[AgentProcess] Checking for rate limit in output (last 500 chars):', allOutput.slice(-500)); - - const rateLimitDetection = detectRateLimit(allOutput); - console.log('[AgentProcess] Rate limit detection result:', { - isRateLimited: rateLimitDetection.isRateLimited, - resetTime: rateLimitDetection.resetTime, - limitType: rateLimitDetection.limitType, - profileId: rateLimitDetection.profileId, - suggestedProfile: rateLimitDetection.suggestedProfile - }); - - if (rateLimitDetection.isRateLimited) { - // Check if auto-swap is enabled - const profileManager = getClaudeProfileManager(); - const autoSwitchSettings = profileManager.getAutoSwitchSettings(); - - console.log('[AgentProcess] Auto-switch settings:', { - enabled: autoSwitchSettings.enabled, - autoSwitchOnRateLimit: autoSwitchSettings.autoSwitchOnRateLimit, - proactiveSwapEnabled: autoSwitchSettings.proactiveSwapEnabled - }); - - if (autoSwitchSettings.enabled && autoSwitchSettings.autoSwitchOnRateLimit) { - const currentProfileId = rateLimitDetection.profileId; - const bestProfile = profileManager.getBestAvailableProfile(currentProfileId); - - console.log('[AgentProcess] Best available profile:', bestProfile ? { - id: bestProfile.id, - name: bestProfile.name - } : 'NONE'); - - if (bestProfile) { - // Switch active profile - console.log('[AgentProcess] AUTO-SWAP: Switching from', currentProfileId, 'to', bestProfile.id); - profileManager.setActiveProfile(bestProfile.id); - - // Emit swap info (for modal) - const source = processType === 'spec-creation' ? 'roadmap' : 'task'; - const rateLimitInfo = createSDKRateLimitInfo(source, rateLimitDetection, { - taskId - }); - rateLimitInfo.wasAutoSwapped = true; - rateLimitInfo.swappedToProfile = { - id: bestProfile.id, - name: bestProfile.name - }; - rateLimitInfo.swapReason = 'reactive'; - - console.log('[AgentProcess] Emitting sdk-rate-limit event (auto-swapped):', rateLimitInfo); - this.emitter.emit('sdk-rate-limit', rateLimitInfo); - - // Restart task - console.log('[AgentProcess] Emitting auto-swap-restart-task event for task:', taskId); - this.emitter.emit('auto-swap-restart-task', taskId, bestProfile.id); - return; - } else { - console.log('[AgentProcess] No alternative profile available - falling back to manual modal'); - } - } else { - console.log('[AgentProcess] Auto-switch disabled - showing manual modal'); - } - - // Fall back to manual modal (no auto-swap or no alternative profile) - const source = processType === 'spec-creation' ? 'roadmap' : 'task'; - const rateLimitInfo = createSDKRateLimitInfo(source, rateLimitDetection, { - taskId - }); - console.log('[AgentProcess] Emitting sdk-rate-limit event (manual):', rateLimitInfo); - this.emitter.emit('sdk-rate-limit', rateLimitInfo); - } else { - console.log('[AgentProcess] No rate limit detected - checking for auth failure'); - // Not rate limited - check for authentication failure - const authFailureDetection = detectAuthFailure(allOutput); - if (authFailureDetection.isAuthFailure) { - console.log('[AgentProcess] Auth failure detected:', authFailureDetection); - this.emitter.emit('auth-failure', taskId, { - profileId: authFailureDetection.profileId, - failureType: authFailureDetection.failureType, - message: authFailureDetection.message, - originalError: authFailureDetection.originalError - }); - } else { - console.log('[AgentProcess] Process failed but no rate limit or auth failure detected'); - } + const wasHandled = this.handleProcessFailure(taskId, allOutput, processType); + if (wasHandled) { + this.emitter.emit('exit', taskId, code, processType); + return; } } - // Emit final progress - const finalPhase = code === 0 ? 'complete' : 'failed'; - this.emitter.emit('execution-progress', taskId, { - phase: finalPhase, - phaseProgress: 100, - overallProgress: code === 0 ? 100 : this.events.calculateOverallProgress(currentPhase, phaseProgress), - message: code === 0 ? 'Process completed successfully' : `Process exited with code ${code}` - }); + if (code !== 0 && currentPhase !== 'complete' && currentPhase !== 'failed') { + this.emitter.emit('execution-progress', taskId, { + phase: 'failed', + phaseProgress: 0, + overallProgress: this.events.calculateOverallProgress(currentPhase, phaseProgress), + message: `Process exited with code ${code}`, + sequenceNumber: ++sequenceNumber + }); + } this.emitter.emit('exit', taskId, code, processType); }); @@ -395,7 +455,8 @@ export class AgentProcessManager { phase: 'failed', phaseProgress: 0, overallProgress: 0, - message: `Error: ${err.message}` + message: `Error: ${err.message}`, + sequenceNumber: ++sequenceNumber }); this.emitter.emit('error', taskId, err.message); diff --git a/apps/frontend/src/main/agent/parsers/base-phase-parser.ts b/apps/frontend/src/main/agent/parsers/base-phase-parser.ts new file mode 100644 index 00000000..fd02090d --- /dev/null +++ b/apps/frontend/src/main/agent/parsers/base-phase-parser.ts @@ -0,0 +1,78 @@ +/** + * Base Phase Parser + * ================== + * Abstract base class for phase parsing with regression prevention. + * Provides common functionality for all phase parsers. + */ + +/** + * Result of parsing a phase event. + * Generic over the phase type for type safety. + */ +export interface PhaseParseResult { + phase: TPhase; + message?: string; + currentSubtask?: string; + progress?: number; +} + +/** + * Context for phase parsing decisions. + * Provides current state information to the parser. + */ +export interface PhaseParserContext { + currentPhase: TPhase; + isTerminal: boolean; +} + +/** + * Abstract base class for phase parsers. + * Implements regression prevention and terminal state checking. + * + * @template TPhase - The union type of valid phases + */ +export abstract class BasePhaseParser { + /** + * Ordered array of phases for regression detection. + * Index determines progression order. + */ + protected abstract readonly phaseOrder: readonly TPhase[]; + + /** + * Set of terminal phases that cannot be changed by fallback matching. + */ + protected abstract readonly terminalPhases: ReadonlySet; + + /** + * Check if transitioning to a new phase would be a regression. + * + * @param currentPhase - The current phase + * @param newPhase - The proposed new phase + * @returns true if the transition would go backwards + */ + protected wouldRegress(currentPhase: TPhase, newPhase: TPhase): boolean { + const currentIdx = this.phaseOrder.indexOf(currentPhase); + const newIdx = this.phaseOrder.indexOf(newPhase); + // Only regress if both phases are in the order array and new is before current + return currentIdx >= 0 && newIdx >= 0 && newIdx < currentIdx; + } + + /** + * Check if a phase is a terminal state. + * + * @param phase - The phase to check + * @returns true if the phase is terminal + */ + protected isTerminal(phase: TPhase): boolean { + return this.terminalPhases.has(phase); + } + + /** + * Parse a log line and extract phase information. + * + * @param log - The log line to parse + * @param context - Current parsing context + * @returns Parsed phase result, or null if no phase detected + */ + abstract parse(log: string, context: PhaseParserContext): PhaseParseResult | null; +} diff --git a/apps/frontend/src/main/agent/parsers/execution-phase-parser.ts b/apps/frontend/src/main/agent/parsers/execution-phase-parser.ts new file mode 100644 index 00000000..4537cc9a --- /dev/null +++ b/apps/frontend/src/main/agent/parsers/execution-phase-parser.ts @@ -0,0 +1,206 @@ +/** + * Execution Phase Parser + * ======================= + * Parses task execution phases from log output. + * Handles both structured events and fallback text matching. + */ + +import { BasePhaseParser, type PhaseParseResult, type PhaseParserContext } from './base-phase-parser'; +import { + EXECUTION_PHASES, + TERMINAL_PHASES, + type ExecutionPhase +} from '../../../shared/constants/phase-protocol'; +import { parsePhaseEvent } from '../phase-event-parser'; + +/** + * Context for execution phase parsing. + * Extends base context with spec runner flag. + */ +export interface ExecutionParserContext extends PhaseParserContext { + isSpecRunner: boolean; +} + +/** + * Parser for task execution phases. + * Handles the planning β†’ coding β†’ qa_review β†’ qa_fixing β†’ complete flow. + */ +export class ExecutionPhaseParser extends BasePhaseParser { + protected readonly phaseOrder = EXECUTION_PHASES; + protected readonly terminalPhases = TERMINAL_PHASES; + + /** + * Parse execution phase from log line. + * + * @param log - The log line to parse + * @param context - Execution parser context + * @returns Phase result or null + */ + parse(log: string, context: ExecutionParserContext): PhaseParseResult | null { + // 1. Try structured event first (authoritative source) + const structuredEvent = parsePhaseEvent(log); + if (structuredEvent) { + return { + phase: structuredEvent.phase as ExecutionPhase, + message: structuredEvent.message, + currentSubtask: structuredEvent.subtask + }; + } + + // 2. Terminal states can't be changed by fallback matching + if (this.isTerminal(context.currentPhase)) { + return null; + } + + // 3. Fall back to text pattern matching + return this.parseFallbackPatterns(log, context); + } + + /** + * Parse phase from text patterns when no structured event is found. + * Implements regression prevention for all phase transitions. + */ + private parseFallbackPatterns( + log: string, + context: ExecutionParserContext + ): PhaseParseResult | null { + // Ignore internal task logger events + if (log.includes('__TASK_LOG_')) { + return null; + } + + const lowerLog = log.toLowerCase(); + const { currentPhase, isSpecRunner } = context; + + // Spec runner phase detection (all part of "planning") + if (isSpecRunner) { + return this.parseSpecRunnerPhase(lowerLog); + } + + // Run.py phase detection + return this.parseRunPhase(lowerLog, log, currentPhase); + } + + /** + * Parse phases for spec_runner.py execution. + * All spec runner phases map to 'planning'. + */ + private parseSpecRunnerPhase(lowerLog: string): PhaseParseResult | null { + if (lowerLog.includes('discovering') || lowerLog.includes('discovery')) { + return { phase: 'planning', message: 'Discovering project context...' }; + } + if (lowerLog.includes('requirements') || lowerLog.includes('gathering')) { + return { phase: 'planning', message: 'Gathering requirements...' }; + } + if (lowerLog.includes('writing spec') || lowerLog.includes('spec writer')) { + return { phase: 'planning', message: 'Writing specification...' }; + } + if (lowerLog.includes('validating') || lowerLog.includes('validation')) { + return { phase: 'planning', message: 'Validating specification...' }; + } + if (lowerLog.includes('spec complete') || lowerLog.includes('specification complete')) { + return { phase: 'planning', message: 'Specification complete' }; + } + + return null; + } + + /** + * Parse phases for run.py execution. + * Handles the full build pipeline phases. + */ + private parseRunPhase( + lowerLog: string, + originalLog: string, + currentPhase: ExecutionPhase + ): PhaseParseResult | null { + // Planning phase + if ( + !this.wouldRegress(currentPhase, 'planning') && + (lowerLog.includes('planner agent') || lowerLog.includes('creating implementation plan')) + ) { + return { phase: 'planning', message: 'Creating implementation plan...' }; + } + + // Coding phase - don't regress from QA phases + if ( + !this.wouldRegress(currentPhase, 'coding') && + (lowerLog.includes('coder agent') || lowerLog.includes('starting coder')) + ) { + return { phase: 'coding', message: 'Implementing code changes...' }; + } + + // Subtask progress detection - only when in coding phase + const subtaskMatch = originalLog.match(/subtask[:\s]+(\d+(?:\/\d+)?|\w+[-_]\w+)/i); + if (subtaskMatch && currentPhase === 'coding') { + return { + phase: 'coding', + currentSubtask: subtaskMatch[1], + message: `Working on subtask ${subtaskMatch[1]}...` + }; + } + + // Subtask completion detection + if ( + !this.wouldRegress(currentPhase, 'coding') && + (lowerLog.includes('subtask completed') || lowerLog.includes('subtask done')) + ) { + const completedSubtask = originalLog.match(/subtask[:\s]+"?([^"]+)"?\s+completed/i); + return { + phase: 'coding', + currentSubtask: completedSubtask?.[1], + message: `Subtask ${completedSubtask?.[1] || ''} completed` + }; + } + + // QA phases require at least coding phase to be completed first + // This prevents false positives from early log messages mentioning QA + const canEnterQAPhase = currentPhase === 'coding' || currentPhase === 'qa_review' || currentPhase === 'qa_fixing'; + + // QA Fixer phase (check before QA reviewer - more specific pattern) + if ( + canEnterQAPhase && + (lowerLog.includes('qa fixer') || + lowerLog.includes('qa_fixer') || + lowerLog.includes('fixing issues')) + ) { + return { phase: 'qa_fixing', message: 'Fixing QA issues...' }; + } + + // QA Review phase + if ( + canEnterQAPhase && + (lowerLog.includes('qa reviewer') || + lowerLog.includes('qa_reviewer') || + lowerLog.includes('starting qa')) + ) { + return { phase: 'qa_review', message: 'Running QA review...' }; + } + + // IMPORTANT: Don't set 'complete' phase via fallback text matching! + // The "=== BUILD COMPLETE ===" banner is printed when SUBTASKS finish, + // but QA hasn't run yet. Only the structured emit_phase(COMPLETE) from + // QA approval (in qa/loop.py) should set the complete phase. + + // Incomplete build detection + if ( + !this.wouldRegress(currentPhase, 'coding') && + (lowerLog.includes('build incomplete') || lowerLog.includes('subtasks still pending')) + ) { + return { phase: 'coding', message: 'Build paused - subtasks still pending' }; + } + + // Error/failure detection - be specific to avoid false positives + const isToolError = lowerLog.includes('tool error') || lowerLog.includes('tool_use_error'); + if ( + !isToolError && + (lowerLog.includes('build failed') || + lowerLog.includes('fatal error') || + lowerLog.includes('agent failed')) + ) { + return { phase: 'failed', message: originalLog.trim().substring(0, 200) }; + } + + return null; + } +} diff --git a/apps/frontend/src/main/agent/parsers/ideation-phase-parser.ts b/apps/frontend/src/main/agent/parsers/ideation-phase-parser.ts new file mode 100644 index 00000000..81c79ba4 --- /dev/null +++ b/apps/frontend/src/main/agent/parsers/ideation-phase-parser.ts @@ -0,0 +1,130 @@ +/** + * Ideation Phase Parser + * ====================== + * Parses ideation flow phases from log output. + * Handles analyzing β†’ discovering β†’ generating β†’ finalizing β†’ complete flow. + */ + +import { BasePhaseParser, type PhaseParseResult, type PhaseParserContext } from './base-phase-parser'; + +/** + * Ideation phase values. + */ +export const IDEATION_PHASES = [ + 'idle', + 'analyzing', + 'discovering', + 'generating', + 'finalizing', + 'complete' +] as const; + +export type IdeationPhase = (typeof IDEATION_PHASES)[number]; + +/** + * Terminal phases for ideation flow. + */ +export const IDEATION_TERMINAL_PHASES: ReadonlySet = new Set(['complete']); + +/** + * Context for ideation phase parsing. + */ +export interface IdeationParserContext extends PhaseParserContext { + completedTypes: Set; + totalTypes: number; +} + +/** + * Result type for ideation parsing, includes progress. + */ +export interface IdeationParseResult extends PhaseParseResult { + progress: number; +} + +/** + * Parser for ideation flow phases. + */ +export class IdeationPhaseParser extends BasePhaseParser { + protected readonly phaseOrder = IDEATION_PHASES; + protected readonly terminalPhases = IDEATION_TERMINAL_PHASES; + + /** + * Parse ideation phase from log line. + * + * @param log - The log line to parse + * @param context - Ideation parser context + * @returns Phase result with progress, or null if no phase detected + */ + parse(log: string, context: IdeationParserContext): IdeationParseResult | null { + // Terminal states cannot be changed + if (context.isTerminal) { + return null; + } + + const result = this.parsePhaseFromLog(log); + + if (!result) { + // No phase change, but calculate progress if in generating phase + if (context.currentPhase === 'generating' && context.completedTypes.size > 0) { + const progress = this.calculateGeneratingProgress(context.completedTypes.size, context.totalTypes); + return { + phase: 'generating', + progress + }; + } + return null; + } + + // Calculate progress for the detected phase + let progress = result.progress; + if (result.phase === 'generating' && context.completedTypes.size > 0) { + progress = this.calculateGeneratingProgress(context.completedTypes.size, context.totalTypes); + } + + return { + ...result, + progress + }; + } + + /** + * Calculate progress during generating phase with division-by-zero protection. + * Progress ranges from 30% to 90% based on completed types. + */ + private calculateGeneratingProgress(completedCount: number, totalTypes: number): number { + if (totalTypes <= 0) { + return 90; // Max generating progress fallback + } + return 30 + Math.floor((completedCount / totalTypes) * 60); + } + + /** + * Parse phase transitions from log text. + */ + private parsePhaseFromLog(log: string): IdeationParseResult | null { + if (log.includes('PROJECT INDEX') || log.includes('PROJECT ANALYSIS')) { + return { phase: 'analyzing', progress: 10 }; + } + + if (log.includes('CONTEXT GATHERING')) { + return { phase: 'discovering', progress: 20 }; + } + + if ( + log.includes('GENERATING IDEAS (PARALLEL)') || + (log.includes('Starting') && log.includes('ideation agents in parallel')) + ) { + return { phase: 'generating', progress: 30 }; + } + + if (log.includes('MERGE') || log.includes('FINALIZE')) { + return { phase: 'finalizing', progress: 90 }; + } + + if (log.includes('IDEATION COMPLETE')) { + return { phase: 'complete', progress: 100 }; + } + + return null; + } +} diff --git a/apps/frontend/src/main/agent/parsers/index.ts b/apps/frontend/src/main/agent/parsers/index.ts new file mode 100644 index 00000000..5ef94c60 --- /dev/null +++ b/apps/frontend/src/main/agent/parsers/index.ts @@ -0,0 +1,37 @@ +/** + * Phase Parsers + * ============== + * Barrel export for all phase parsers. + */ + +// Base types and class +export { + BasePhaseParser, + type PhaseParseResult, + type PhaseParserContext +} from './base-phase-parser'; + +// Execution phase parser +export { + ExecutionPhaseParser, + type ExecutionParserContext +} from './execution-phase-parser'; + +// Ideation phase parser +export { + IdeationPhaseParser, + IDEATION_PHASES, + IDEATION_TERMINAL_PHASES, + type IdeationPhase, + type IdeationParserContext, + type IdeationParseResult +} from './ideation-phase-parser'; + +// Roadmap phase parser +export { + RoadmapPhaseParser, + ROADMAP_PHASES, + ROADMAP_TERMINAL_PHASES, + type RoadmapPhase, + type RoadmapParseResult +} from './roadmap-phase-parser'; diff --git a/apps/frontend/src/main/agent/parsers/roadmap-phase-parser.ts b/apps/frontend/src/main/agent/parsers/roadmap-phase-parser.ts new file mode 100644 index 00000000..e29a1481 --- /dev/null +++ b/apps/frontend/src/main/agent/parsers/roadmap-phase-parser.ts @@ -0,0 +1,81 @@ +/** + * Roadmap Phase Parser + * ===================== + * Parses roadmap generation phases from log output. + * Handles analyzing β†’ discovering β†’ generating β†’ complete flow. + */ + +import { BasePhaseParser, type PhaseParseResult, type PhaseParserContext } from './base-phase-parser'; + +/** + * Roadmap phase values. + */ +export const ROADMAP_PHASES = ['idle', 'analyzing', 'discovering', 'generating', 'complete'] as const; + +export type RoadmapPhase = (typeof ROADMAP_PHASES)[number]; + +/** + * Terminal phases for roadmap flow. + */ +export const ROADMAP_TERMINAL_PHASES: ReadonlySet = new Set(['complete']); + +/** + * Result type for roadmap parsing, includes progress. + */ +export interface RoadmapParseResult extends PhaseParseResult { + progress: number; +} + +/** + * Parser for roadmap generation phases. + */ +export class RoadmapPhaseParser extends BasePhaseParser { + protected readonly phaseOrder = ROADMAP_PHASES; + protected readonly terminalPhases = ROADMAP_TERMINAL_PHASES; + + /** + * Parse roadmap phase from log line. + * + * @param log - The log line to parse + * @param context - Roadmap parser context + * @returns Phase result with progress, or null if no phase detected + */ + parse(log: string, context: PhaseParserContext): RoadmapParseResult | null { + // Terminal states can't be changed + if (this.isTerminal(context.currentPhase)) { + return null; + } + + const result = this.parsePhaseFromLog(log); + + // Prevent backwards transitions + if (result && this.wouldRegress(context.currentPhase, result.phase)) { + return null; + } + + return result; + } + + /** + * Parse phase transitions from log text. + */ + private parsePhaseFromLog(log: string): RoadmapParseResult | null { + if (log.includes('PROJECT ANALYSIS')) { + return { phase: 'analyzing', progress: 20 }; + } + + if (log.includes('PROJECT DISCOVERY')) { + return { phase: 'discovering', progress: 40 }; + } + + if (log.includes('FEATURE GENERATION')) { + return { phase: 'generating', progress: 70 }; + } + + if (log.includes('ROADMAP GENERATED')) { + return { phase: 'complete', progress: 100 }; + } + + return null; + } +} diff --git a/apps/frontend/src/main/agent/phase-event-parser.ts b/apps/frontend/src/main/agent/phase-event-parser.ts new file mode 100644 index 00000000..c29a9f0c --- /dev/null +++ b/apps/frontend/src/main/agent/phase-event-parser.ts @@ -0,0 +1,120 @@ +/** + * Structured phase event parser for Python ↔ TypeScript protocol. + * Protocol: __EXEC_PHASE__:{"phase":"coding","message":"Starting"} + */ + +import { PHASE_MARKER_PREFIX } from '../../shared/constants/phase-protocol'; +import { validatePhaseEvent, type PhaseEventPayload } from './phase-event-schema'; + +export { PHASE_MARKER_PREFIX }; +export type { PhaseEventPayload as PhaseEvent }; + +const DEBUG = process.env.DEBUG?.toLowerCase() === 'true' || process.env.DEBUG === '1'; + +export function parsePhaseEvent(line: string): PhaseEventPayload | null { + const markerIndex = line.indexOf(PHASE_MARKER_PREFIX); + if (markerIndex === -1) { + return null; + } + + if (DEBUG) { + console.log('[phase-event-parser] Found marker at index', markerIndex, 'in line:', line.substring(0, 200)); + } + + const rawJsonStr = line.slice(markerIndex + PHASE_MARKER_PREFIX.length).trim(); + if (!rawJsonStr) { + if (DEBUG) { + console.log('[phase-event-parser] Empty JSON string after marker'); + } + return null; + } + + const jsonStr = extractJsonObject(rawJsonStr); + if (!jsonStr) { + if (DEBUG) { + console.log('[phase-event-parser] Could not extract JSON object from:', rawJsonStr.substring(0, 200)); + } + return null; + } + + if (DEBUG) { + console.log('[phase-event-parser] Attempting to parse JSON:', jsonStr.substring(0, 200)); + } + + try { + const rawPayload = JSON.parse(jsonStr) as unknown; + const result = validatePhaseEvent(rawPayload); + + if (!result.success) { + if (DEBUG) { + console.log('[phase-event-parser] Validation failed:', result.error.format()); + } + return null; + } + + if (DEBUG) { + console.log('[phase-event-parser] Successfully parsed event:', result.data); + } + + return result.data; + } catch (e) { + if (DEBUG) { + console.log('[phase-event-parser] JSON parse FAILED for:', jsonStr); + console.log('[phase-event-parser] Error:', e); + } + return null; + } +} + +export function hasPhaseMarker(line: string): boolean { + return line.includes(PHASE_MARKER_PREFIX); +} + +/** + * Extract a JSON object from a string that may have trailing garbage. + * Finds the matching closing brace for the first opening brace. + */ +function extractJsonObject(str: string): string | null { + const firstBrace = str.indexOf('{'); + if (firstBrace === -1) { + return null; + } + + let depth = 0; + let inString = false; + let isEscaped = false; + + for (let i = firstBrace; i < str.length; i++) { + const char = str[i]; + + if (isEscaped) { + isEscaped = false; + continue; + } + + if (char === '\\' && inString) { + isEscaped = true; + continue; + } + + if (char === '"') { + inString = !inString; + continue; + } + + if (inString) { + continue; + } + + if (char === '{') { + depth++; + } else if (char === '}') { + depth--; + if (depth === 0) { + return str.slice(firstBrace, i + 1); + } + } + } + + return null; +} diff --git a/apps/frontend/src/main/agent/phase-event-schema.ts b/apps/frontend/src/main/agent/phase-event-schema.ts new file mode 100644 index 00000000..508a55d9 --- /dev/null +++ b/apps/frontend/src/main/agent/phase-event-schema.ts @@ -0,0 +1,37 @@ +import { z } from 'zod'; +import { BACKEND_PHASES } from '../../shared/constants/phase-protocol'; + +const BackendPhaseSchema = z.enum(BACKEND_PHASES as unknown as [string, ...string[]]); + +export const PhaseEventSchema = z.object({ + phase: BackendPhaseSchema, + message: z.string().default(''), + progress: z.number().int().min(0).max(100).optional(), + subtask: z.string().optional() +}); + +export type PhaseEventPayload = z.infer; + +export interface ValidationResult { + success: true; + data: PhaseEventPayload; +} + +export interface ValidationError { + success: false; + error: z.ZodError; +} + +export type ParseResult = ValidationResult | ValidationError; + +export function validatePhaseEvent(data: unknown): ParseResult { + const result = PhaseEventSchema.safeParse(data); + if (result.success) { + return { success: true, data: result.data as PhaseEventPayload }; + } + return { success: false, error: result.error }; +} + +export function isValidPhasePayload(data: unknown): data is PhaseEventPayload { + return PhaseEventSchema.safeParse(data).success; +} diff --git a/apps/frontend/src/main/changelog/changelog-service.ts b/apps/frontend/src/main/changelog/changelog-service.ts index 049cfc3a..fce7ced8 100644 --- a/apps/frontend/src/main/changelog/changelog-service.ts +++ b/apps/frontend/src/main/changelog/changelog-service.ts @@ -27,6 +27,7 @@ import { getCommits, getBranchDiffCommits } from './git-integration'; +import { getValidatedPythonPath } from '../python-detector'; import { getConfiguredPythonPath } from '../python-env-manager'; /** @@ -125,12 +126,9 @@ export class ChangelogService extends EventEmitter { } } - /** - * Configure paths for Python and auto-claude source - */ configure(pythonPath?: string, autoBuildSourcePath?: string): void { if (pythonPath) { - this._pythonPath = pythonPath; + this._pythonPath = getValidatedPythonPath(pythonPath, 'ChangelogService'); } if (autoBuildSourcePath) { this.autoBuildSourcePath = autoBuildSourcePath; diff --git a/apps/frontend/src/main/insights/config.ts b/apps/frontend/src/main/insights/config.ts index aa75c487..0ca1609c 100644 --- a/apps/frontend/src/main/insights/config.ts +++ b/apps/frontend/src/main/insights/config.ts @@ -2,6 +2,7 @@ import path from 'path'; import { existsSync, readFileSync } from 'fs'; import { app } from 'electron'; import { getProfileEnv } from '../rate-limit-detector'; +import { getValidatedPythonPath } from '../python-detector'; import { getConfiguredPythonPath } from '../python-env-manager'; /** @@ -14,12 +15,9 @@ export class InsightsConfig { private _pythonPath: string | null = null; private autoBuildSourcePath: string = ''; - /** - * Configure paths for Python and auto-claude source - */ configure(pythonPath?: string, autoBuildSourcePath?: string): void { if (pythonPath) { - this._pythonPath = pythonPath; + this._pythonPath = getValidatedPythonPath(pythonPath, 'InsightsConfig'); } if (autoBuildSourcePath) { this.autoBuildSourcePath = autoBuildSourcePath; diff --git a/apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts b/apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts index 0d88fbae..21986aa2 100644 --- a/apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts @@ -61,32 +61,13 @@ export function registerAgenteventsHandlers( agentManager.on('exit', (taskId: string, code: number | null, processType: ProcessType) => { const mainWindow = getMainWindow(); if (mainWindow) { - // Stop file watcher fileWatcher.unwatch(taskId); - // Determine new status based on process type and exit code - // Flow: Planning β†’ In Progress β†’ AI Review (QA agent) β†’ Human Review (QA passed) - let newStatus: TaskStatus; - - if (processType === 'task-execution') { - // Task execution completed (includes spec_runner β†’ run.py chain) - // Success (code 0) = QA agent signed off β†’ Human Review - // Failure = needs human attention β†’ Human Review - newStatus = 'human_review'; - } else if (processType === 'qa-process') { - // QA retry process completed - newStatus = 'human_review'; - } else if (processType === 'spec-creation') { - // Pure spec creation (shouldn't happen with current flow, but handle it) - // Stay in backlog/planning + if (processType === 'spec-creation') { console.warn(`[Task ${taskId}] Spec creation completed with code ${code}`); return; - } else { - // Unknown process type - newStatus = 'human_review'; } - // Find task and project for status persistence and notifications let task: Task | undefined; let project: Project | undefined; @@ -102,57 +83,23 @@ export function registerAgenteventsHandlers( } } - // Persist status to disk so it survives hot reload - // This is a backup in case the Python backend didn't sync properly if (task && project) { - const specsBaseDir = getSpecsDir(project.autoBuildPath); - const specDir = path.join(project.path, specsBaseDir, task.specId); - const planPath = path.join(specDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN); + const taskTitle = task.title || task.specId; - if (existsSync(planPath)) { - const planContent = readFileSync(planPath, 'utf-8'); - const plan = JSON.parse(planContent); - - // Only update if not already set to a "further along" status - // (e.g., don't override 'done' with 'human_review') - const currentStatus = plan.status; - const shouldUpdate = !currentStatus || - currentStatus === 'in_progress' || - currentStatus === 'ai_review' || - currentStatus === 'backlog' || - currentStatus === 'pending'; - - if (shouldUpdate) { - plan.status = newStatus; - plan.planStatus = 'review'; - plan.updated_at = new Date().toISOString(); - writeFileSync(planPath, JSON.stringify(plan, null, 2)); - console.warn(`[Task ${taskId}] Persisted status '${newStatus}' to implementation_plan.json`); - } + if (code === 0) { + notificationService.notifyReviewNeeded(taskTitle, project.id, taskId); + } else { + notificationService.notifyTaskFailed(taskTitle, project.id, taskId); + mainWindow.webContents.send( + IPC_CHANNELS.TASK_STATUS_CHANGE, + taskId, + 'human_review' as TaskStatus + ); } } - } catch (persistError) { - console.error(`[Task ${taskId}] Failed to persist status:`, persistError); + } catch (error) { + console.error(`[Task ${taskId}] Exit handler error:`, error); } - - // Send notifications based on task completion status - if (task && project) { - const taskTitle = task.title || task.specId; - - if (code === 0) { - // Task completed successfully - ready for review - notificationService.notifyReviewNeeded(taskTitle, project.id, taskId); - } else { - // Task failed - notificationService.notifyTaskFailed(taskTitle, project.id, taskId); - } - } - - mainWindow.webContents.send( - IPC_CHANNELS.TASK_STATUS_CHANGE, - taskId, - newStatus - ); } }); @@ -161,12 +108,22 @@ export function registerAgenteventsHandlers( if (mainWindow) { mainWindow.webContents.send(IPC_CHANNELS.TASK_EXECUTION_PROGRESS, taskId, progress); - // Auto-move task to AI Review when entering qa_review phase - if (progress.phase === 'qa_review') { + const phaseToStatus: Record = { + 'idle': null, + 'planning': 'in_progress', + 'coding': 'in_progress', + 'qa_review': 'ai_review', + 'qa_fixing': 'ai_review', + 'complete': 'human_review', + 'failed': 'human_review' + }; + + const newStatus = phaseToStatus[progress.phase]; + if (newStatus) { mainWindow.webContents.send( IPC_CHANNELS.TASK_STATUS_CHANGE, taskId, - 'ai_review' + newStatus ); } } diff --git a/apps/frontend/src/main/python-detector.ts b/apps/frontend/src/main/python-detector.ts index 20d8317d..899739ba 100644 --- a/apps/frontend/src/main/python-detector.ts +++ b/apps/frontend/src/main/python-detector.ts @@ -1,5 +1,5 @@ -import { execSync } from 'child_process'; -import { existsSync } from 'fs'; +import { execSync, execFileSync } from 'child_process'; +import { existsSync, accessSync, constants } from 'fs'; import path from 'path'; import { app } from 'electron'; @@ -256,3 +256,230 @@ export function parsePythonCommand(pythonPath: string): [string, string[]] { const baseArgs = parts.slice(1); return [command, baseArgs]; } + +/** + * Result of Python path validation. + */ +export interface PythonPathValidation { + valid: boolean; + reason?: string; + sanitizedPath?: string; +} + +/** + * Shell metacharacters that could be used for command injection. + * These are dangerous in spawn() context and must be rejected. + */ +const DANGEROUS_SHELL_CHARS = /[;|`$()&<>{}[\]!#*?~\n\r]/; + +/** + * Allowlist patterns for valid Python paths. + * Matches common system Python locations and virtual environments. + */ +const ALLOWED_PATH_PATTERNS: RegExp[] = [ + // System Python (Unix) + /^\/usr\/bin\/python\d*(\.\d+)?$/, + /^\/usr\/local\/bin\/python\d*(\.\d+)?$/, + // Homebrew Python (macOS) + /^\/opt\/homebrew\/bin\/python\d*(\.\d+)?$/, + /^\/opt\/homebrew\/opt\/python@[\d.]+\/bin\/python\d*(\.\d+)?$/, + // pyenv + /^.*\/\.pyenv\/versions\/[\d.]+\/bin\/python\d*(\.\d+)?$/, + // Virtual environments (various naming conventions) + /^.*\/\.?venv\/bin\/python\d*(\.\d+)?$/, + /^.*\/\.?virtualenv\/bin\/python\d*(\.\d+)?$/, + /^.*\/env\/bin\/python\d*(\.\d+)?$/, + // Windows virtual environments + /^.*\\\.?venv\\Scripts\\python\.exe$/i, + /^.*\\\.?virtualenv\\Scripts\\python\.exe$/i, + /^.*\\env\\Scripts\\python\.exe$/i, + // Windows system Python + /^[A-Za-z]:\\Python\d+\\python\.exe$/i, + /^[A-Za-z]:\\Program Files\\Python\d+\\python\.exe$/i, + /^[A-Za-z]:\\Program Files \(x86\)\\Python\d+\\python\.exe$/i, + /^[A-Za-z]:\\Users\\[^\\]+\\AppData\\Local\\Programs\\Python\\Python\d+\\python\.exe$/i, + // Conda environments + /^.*\/anaconda\d*\/bin\/python\d*(\.\d+)?$/, + /^.*\/miniconda\d*\/bin\/python\d*(\.\d+)?$/, + /^.*\/anaconda\d*\/envs\/[^/]+\/bin\/python\d*(\.\d+)?$/, + /^.*\/miniconda\d*\/envs\/[^/]+\/bin\/python\d*(\.\d+)?$/, +]; + +/** + * Known safe Python commands (not full paths). + * These are resolved by the shell/OS and are safe. + */ +const SAFE_PYTHON_COMMANDS = new Set([ + 'python', + 'python3', + 'python3.10', + 'python3.11', + 'python3.12', + 'python3.13', + 'py', + 'py -3', +]); + +function isSafePythonCommand(cmd: string): boolean { + const normalized = cmd.replace(/\s+/g, ' ').trim().toLowerCase(); + return SAFE_PYTHON_COMMANDS.has(normalized); +} + +/** + * Check if a path matches any allowed pattern. + */ +function matchesAllowedPattern(pythonPath: string): boolean { + // Normalize path separators for consistent matching + const normalizedPath = pythonPath.replace(/\\/g, '/'); + return ALLOWED_PATH_PATTERNS.some(pattern => pattern.test(pythonPath) || pattern.test(normalizedPath)); +} + +/** + * Check if a file is executable. + */ +function isExecutable(filePath: string): boolean { + try { + accessSync(filePath, constants.X_OK); + return true; + } catch { + return false; + } +} + +/** + * Verify that a command/path actually runs Python by checking --version output. + * Uses execFileSync to avoid shell injection risks with paths containing spaces. + */ +function verifyIsPython(pythonCmd: string): boolean { + try { + const [cmd, args] = parsePythonCommand(pythonCmd); + const output = execFileSync(cmd, [...args, '--version'], { + stdio: 'pipe', + timeout: 5000, + windowsHide: true, + shell: false + }).toString().trim(); + + // Must output "Python X.Y.Z" + return /^Python \d+\.\d+/.test(output); + } catch { + return false; + } +} + +/** + * Validate a Python path for security before use in spawn(). + * + * Security checks: + * 1. No shell metacharacters that could enable command injection + * 2. Path must match allowlist of known Python locations OR be a safe command + * 3. If a file path, must exist and be executable + * 4. Must actually be Python (verified via --version) + * + * @param pythonPath - The Python path or command to validate + * @returns Validation result with success status and reason + */ +export function validatePythonPath(pythonPath: string): PythonPathValidation { + if (!pythonPath || typeof pythonPath !== 'string') { + return { valid: false, reason: 'Python path is empty or invalid' }; + } + + const trimmedPath = pythonPath.trim(); + + // Strip surrounding quotes for validation + let cleanPath = trimmedPath; + if ((cleanPath.startsWith('"') && cleanPath.endsWith('"')) || + (cleanPath.startsWith("'") && cleanPath.endsWith("'"))) { + cleanPath = cleanPath.slice(1, -1); + } + + // Security check 1: No shell metacharacters + if (DANGEROUS_SHELL_CHARS.test(cleanPath)) { + return { + valid: false, + reason: 'Path contains dangerous shell metacharacters' + }; + } + + // Check if it's a known safe command (not a path) + if (isSafePythonCommand(cleanPath)) { + // Verify it actually runs Python + if (verifyIsPython(cleanPath)) { + return { valid: true, sanitizedPath: cleanPath }; + } + return { + valid: false, + reason: `Command '${cleanPath}' does not appear to be Python` + }; + } + + // It's a file path - apply stricter validation + const isFilePath = cleanPath.includes('/') || cleanPath.includes('\\'); + + if (isFilePath) { + // Normalize the path to prevent directory traversal tricks + const normalizedPath = path.normalize(cleanPath); + + // Check for path traversal attempts + if (normalizedPath.includes('..')) { + return { + valid: false, + reason: 'Path contains directory traversal sequences' + }; + } + + // Security check 2: Must match allowlist + if (!matchesAllowedPattern(normalizedPath)) { + return { + valid: false, + reason: 'Path does not match allowed Python locations. Expected: system Python, Homebrew, pyenv, or virtual environment paths' + }; + } + + // Security check 3: File must exist + if (!existsSync(normalizedPath)) { + return { + valid: false, + reason: 'Python executable does not exist at specified path' + }; + } + + // Security check 4: Must be executable (Unix) or .exe (Windows) + if (process.platform !== 'win32' && !isExecutable(normalizedPath)) { + return { + valid: false, + reason: 'File exists but is not executable' + }; + } + + // Security check 5: Verify it's actually Python + if (!verifyIsPython(normalizedPath)) { + return { + valid: false, + reason: 'File exists but does not appear to be a Python interpreter' + }; + } + + return { valid: true, sanitizedPath: normalizedPath }; + } + + // Unknown format - reject + return { + valid: false, + reason: 'Unrecognized Python path format' + }; +} + +export function getValidatedPythonPath(providedPath: string | undefined, serviceName: string): string { + if (!providedPath) { + return findPythonCommand() || 'python'; + } + + const validation = validatePythonPath(providedPath); + if (validation.valid) { + return validation.sanitizedPath || providedPath; + } + + console.error(`[${serviceName}] Invalid Python path rejected: ${validation.reason}`); + return findPythonCommand() || 'python'; +} diff --git a/apps/frontend/src/main/title-generator.ts b/apps/frontend/src/main/title-generator.ts index 2963b5f4..2844449d 100644 --- a/apps/frontend/src/main/title-generator.ts +++ b/apps/frontend/src/main/title-generator.ts @@ -4,7 +4,7 @@ import { spawn } from 'child_process'; import { app } from 'electron'; import { EventEmitter } from 'events'; import { detectRateLimit, createSDKRateLimitInfo, getProfileEnv } from './rate-limit-detector'; -import { parsePythonCommand } from './python-detector'; +import { parsePythonCommand, getValidatedPythonPath } from './python-detector'; import { getConfiguredPythonPath } from './python-env-manager'; /** @@ -31,12 +31,9 @@ export class TitleGenerator extends EventEmitter { debug('TitleGenerator initialized'); } - /** - * Configure paths for Python and auto-claude source - */ configure(pythonPath?: string, autoBuildSourcePath?: string): void { if (pythonPath) { - this._pythonPath = pythonPath; + this._pythonPath = getValidatedPythonPath(pythonPath, 'TitleGenerator'); } if (autoBuildSourcePath) { this.autoBuildSourcePath = autoBuildSourcePath; diff --git a/apps/frontend/src/renderer/__tests__/task-store.test.ts b/apps/frontend/src/renderer/__tests__/task-store.test.ts index d349ae85..830bd1d3 100644 --- a/apps/frontend/src/renderer/__tests__/task-store.test.ts +++ b/apps/frontend/src/renderer/__tests__/task-store.test.ts @@ -331,6 +331,119 @@ describe('Task Store', () => { expect(useTaskStore.getState().tasks[0].title).toBe('New Feature Name'); }); + + it('should NOT update status when task is in active execution phase (planning)', () => { + useTaskStore.setState({ + tasks: [createTestTask({ + id: 'task-1', + status: 'in_progress', + executionProgress: { phase: 'planning', phaseProgress: 10, overallProgress: 5 } + })] + }); + + 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); + + expect(useTaskStore.getState().tasks[0].status).toBe('in_progress'); + expect(useTaskStore.getState().tasks[0].subtasks).toHaveLength(2); + }); + + it('should NOT update status when task is in active execution phase (coding)', () => { + useTaskStore.setState({ + tasks: [createTestTask({ + id: 'task-1', + status: 'in_progress', + executionProgress: { phase: 'coding', phaseProgress: 50, overallProgress: 40 } + })] + }); + + 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); + + expect(useTaskStore.getState().tasks[0].status).toBe('in_progress'); + }); + + it('should update status when task is in idle phase', () => { + useTaskStore.setState({ + tasks: [createTestTask({ + id: 'task-1', + status: 'in_progress', + executionProgress: { phase: 'idle', phaseProgress: 0, overallProgress: 0 } + })] + }); + + 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); + + expect(useTaskStore.getState().tasks[0].status).toBe('ai_review'); + }); + + it('should update status when task has no execution progress', () => { + 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); + + expect(useTaskStore.getState().tasks[0].status).toBe('ai_review'); + }); }); describe('appendLog', () => { diff --git a/apps/frontend/src/renderer/components/PhaseProgressIndicator.tsx b/apps/frontend/src/renderer/components/PhaseProgressIndicator.tsx index c73cd755..82965985 100644 --- a/apps/frontend/src/renderer/components/PhaseProgressIndicator.tsx +++ b/apps/frontend/src/renderer/components/PhaseProgressIndicator.tsx @@ -41,7 +41,7 @@ const PHASE_LABEL_KEYS: Record = { * - Stuck: Shows warning state with interrupted animation */ export function PhaseProgressIndicator({ - phase = 'idle', + phase: rawPhase, subtasks, phaseLogs, isStuck = false, @@ -49,6 +49,7 @@ export function PhaseProgressIndicator({ className, }: PhaseProgressIndicatorProps) { const { t } = useTranslation('tasks'); + const phase = rawPhase || 'idle'; // Calculate subtask-based progress (for coding phase) const completedSubtasks = subtasks.filter((c) => c.status === 'completed').length; diff --git a/apps/frontend/src/renderer/stores/task-store.ts b/apps/frontend/src/renderer/stores/task-store.ts index 5500a537..f88ec08e 100644 --- a/apps/frontend/src/renderer/stores/task-store.ts +++ b/apps/frontend/src/renderer/stores/task-store.ts @@ -50,11 +50,18 @@ export const useTaskStore = create((set, get) => ({ tasks: state.tasks.map((t) => { if (t.id !== taskId && t.specId !== taskId) return t; - // When status goes to backlog, reset execution progress to idle - // This ensures the planning/coding animation stops when task is stopped - const executionProgress = status === 'backlog' - ? { phase: 'idle' as ExecutionPhase, phaseProgress: 0, overallProgress: 0 } - : t.executionProgress; + // Determine execution progress based on status transition + let executionProgress = t.executionProgress; + + if (status === 'backlog') { + // When status goes to backlog, reset execution progress to idle + // This ensures the planning/coding animation stops when task is stopped + executionProgress = { phase: 'idle' as ExecutionPhase, phaseProgress: 0, overallProgress: 0 }; + } else if (status === 'in_progress' && !t.executionProgress?.phase) { + // When starting a task and no phase is set yet, default to planning + // This prevents the "no active phase" UI state during startup race condition + executionProgress = { phase: 'planning' as ExecutionPhase, phaseProgress: 0, overallProgress: 0 }; + } return { ...t, status, executionProgress, updatedAt: new Date() }; }) @@ -65,7 +72,6 @@ export const useTaskStore = create((set, get) => ({ tasks: state.tasks.map((t) => { if (t.id !== taskId && t.specId !== taskId) return t; - // Extract subtasks from plan const subtasks: Subtask[] = plan.phases.flatMap((phase) => phase.subtasks.map((subtask) => ({ id: subtask.id, @@ -77,32 +83,27 @@ export const useTaskStore = create((set, get) => ({ })) ); - // Determine status and reviewReason based on subtasks - // This logic must match the backend (project-store.ts) exactly - const allCompleted = subtasks.length > 0 && subtasks.every((s) => s.status === 'completed'); - const anyInProgress = subtasks.some((s) => s.status === 'in_progress'); + 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'); const anyCompleted = subtasks.some((s) => s.status === 'completed'); let status: TaskStatus = t.status; let reviewReason: ReviewReason | undefined = t.reviewReason; - if (allCompleted) { - // Manual tasks skip AI review and go directly to human review - status = t.metadata?.sourceType === 'manual' ? 'human_review' : 'ai_review'; - if (t.metadata?.sourceType === 'manual') { - reviewReason = 'completed'; - } else { - reviewReason = undefined; + // RACE CONDITION FIX: Don't let stale plan data override status during active execution + const activePhases: ExecutionPhase[] = ['planning', 'coding', 'qa_review', 'qa_fixing']; + const isInActivePhase = t.executionProgress?.phase && activePhases.includes(t.executionProgress.phase); + + if (!isInActivePhase) { + if (allCompleted) { + status = 'ai_review'; + } else if (anyFailed) { + status = 'human_review'; + reviewReason = 'errors'; + } else if (anyInProgress || anyCompleted) { + status = 'in_progress'; } - } else if (anyFailed) { - // Some subtasks failed - needs human attention - status = 'human_review'; - reviewReason = 'errors'; - } else if (anyInProgress || anyCompleted) { - // Work in progress - status = 'in_progress'; - reviewReason = undefined; } return { @@ -121,13 +122,19 @@ export const useTaskStore = create((set, get) => ({ tasks: state.tasks.map((t) => { if (t.id !== taskId && t.specId !== taskId) return t; - // Merge with existing progress const existingProgress = t.executionProgress || { phase: 'idle' as ExecutionPhase, phaseProgress: 0, - overallProgress: 0 + overallProgress: 0, + sequenceNumber: 0 }; + const incomingSeq = progress.sequenceNumber ?? 0; + const currentSeq = existingProgress.sequenceNumber ?? 0; + if (incomingSeq > 0 && currentSeq > 0 && incomingSeq < currentSeq) { + return t; + } + return { ...t, executionProgress: { diff --git a/apps/frontend/src/shared/constants/index.ts b/apps/frontend/src/shared/constants/index.ts index 8d33d033..ea90dce6 100644 --- a/apps/frontend/src/shared/constants/index.ts +++ b/apps/frontend/src/shared/constants/index.ts @@ -3,6 +3,9 @@ * Re-exports from domain-specific constant modules */ +// Phase event protocol constants (Python ↔ TypeScript) +export * from './phase-protocol'; + // IPC Channel constants export * from './ipc'; diff --git a/apps/frontend/src/shared/constants/phase-protocol.ts b/apps/frontend/src/shared/constants/phase-protocol.ts new file mode 100644 index 00000000..0748bb37 --- /dev/null +++ b/apps/frontend/src/shared/constants/phase-protocol.ts @@ -0,0 +1,114 @@ +/** + * Phase Event Protocol Constants + * =============================== + * Single source of truth for execution phase communication between + * Python backend and TypeScript frontend. + * + * SYNC REQUIREMENT: Phase values must match apps/backend/core/phase_event.py + * + * Protocol: __EXEC_PHASE__:{"phase":"coding","message":"Starting"} + */ + +// Protocol marker prefix - must match Python's PHASE_MARKER_PREFIX +export const PHASE_MARKER_PREFIX = '__EXEC_PHASE__:' as const; + +// Protocol version for future compatibility checks +export const PHASE_PROTOCOL_VERSION = '1.0.0' as const; + +/** + * All execution phases in order of progression. + * Order matters for regression detection. + * + * 'idle' is frontend-only (initial state before any backend events) + */ +export const EXECUTION_PHASES = [ + 'idle', + 'planning', + 'coding', + 'qa_review', + 'qa_fixing', + 'complete', + 'failed' +] as const; + +/** + * Phases that can be emitted by the Python backend. + * Subset of EXECUTION_PHASES (excludes 'idle') + */ +export const BACKEND_PHASES = [ + 'planning', + 'coding', + 'qa_review', + 'qa_fixing', + 'complete', + 'failed' +] as const; + +// Types derived from constants (single source of truth) +export type ExecutionPhase = (typeof EXECUTION_PHASES)[number]; +export type BackendPhase = (typeof BACKEND_PHASES)[number]; + +/** + * Phase ordering index for regression detection. + * Higher index = later in the pipeline. + * Used to prevent fallback text matching from regressing phases. + */ +export const PHASE_ORDER_INDEX: Readonly> = { + idle: -1, + planning: 0, + coding: 1, + qa_review: 2, + qa_fixing: 3, + complete: 4, + failed: 99 +} as const; + +/** + * Terminal phases that cannot be changed by fallback text matching. + * Only structured events can transition away from these. + */ +export const TERMINAL_PHASES: ReadonlySet = new Set(['complete', 'failed']); + +/** + * Check if a phase transition would be a regression. + * Used to prevent fallback text matching from going backwards. + * + * @param currentPhase - The current phase + * @param newPhase - The proposed new phase + * @returns true if transitioning to newPhase would be a regression + */ +export function wouldPhaseRegress(currentPhase: ExecutionPhase, newPhase: ExecutionPhase): boolean { + const currentIndex = PHASE_ORDER_INDEX[currentPhase]; + const newIndex = PHASE_ORDER_INDEX[newPhase]; + return newIndex < currentIndex; +} + +/** + * Check if a phase is a terminal state. + * + * @param phase - The phase to check + * @returns true if the phase is terminal (complete or failed) + */ +export function isTerminalPhase(phase: ExecutionPhase): boolean { + return TERMINAL_PHASES.has(phase); +} + +/** + * Validate that a string is a valid backend phase. + * + * @param value - The string to validate + * @returns true if the value is a valid BackendPhase + */ +export function isValidBackendPhase(value: string): value is BackendPhase { + return (BACKEND_PHASES as readonly string[]).includes(value); +} + +/** + * Validate that a string is a valid execution phase. + * + * @param value - The string to validate + * @returns true if the value is a valid ExecutionPhase + */ +export function isValidExecutionPhase(value: string): value is ExecutionPhase { + return (EXECUTION_PHASES as readonly string[]).includes(value); +} diff --git a/apps/frontend/src/shared/types/task.ts b/apps/frontend/src/shared/types/task.ts index 23fff03e..42c9e61a 100644 --- a/apps/frontend/src/shared/types/task.ts +++ b/apps/frontend/src/shared/types/task.ts @@ -3,6 +3,7 @@ */ import type { ThinkingLevel, PhaseModelConfig, PhaseThinkingConfig } from './settings'; +import type { ExecutionPhase as ExecutionPhaseType } from '../constants/phase-protocol'; export type TaskStatus = 'backlog' | 'in_progress' | 'ai_review' | 'human_review' | 'done'; @@ -15,8 +16,8 @@ export type ReviewReason = 'completed' | 'errors' | 'qa_rejected' | 'plan_review export type SubtaskStatus = 'pending' | 'in_progress' | 'completed' | 'failed'; -// Execution phases for visual progress tracking -export type ExecutionPhase = 'idle' | 'planning' | 'coding' | 'qa_review' | 'qa_fixing' | 'complete' | 'failed'; +// Re-exported from constants - single source of truth +export type ExecutionPhase = ExecutionPhaseType; export interface ExecutionProgress { phase: ExecutionPhase; @@ -25,6 +26,7 @@ export interface ExecutionProgress { currentSubtask?: string; // Current subtask being processed message?: string; // Current status message startedAt?: Date; + sequenceNumber?: number; // Monotonically increasing counter to detect stale updates } export interface Subtask { diff --git a/tests/test_phase_event.py b/tests/test_phase_event.py new file mode 100644 index 00000000..18e05abd --- /dev/null +++ b/tests/test_phase_event.py @@ -0,0 +1,487 @@ +#!/usr/bin/env python3 +""" +Tests for Phase Event Emission Protocol +======================================== + +Tests the phase_event.py module including: +- ExecutionPhase enum +- emit_phase function +- Edge case handling (newlines, unicode, long messages) +- Error handling +""" + +import json +import sys +from io import StringIO +from pathlib import Path +from unittest.mock import patch + +import pytest + +# Add backend to path +sys.path.insert(0, str(Path(__file__).parent.parent / "apps" / "backend")) + +from core.phase_event import ( + PHASE_MARKER_PREFIX, + ExecutionPhase, + emit_phase, +) + + +class TestExecutionPhaseEnum: + """Tests for ExecutionPhase enum values.""" + + def test_all_phases_have_string_values(self): + """All phases have valid string values.""" + for phase in ExecutionPhase: + assert isinstance(phase.value, str) + assert len(phase.value) > 0 + + def test_phase_values_are_lowercase(self): + """Phase values are lowercase for consistency.""" + for phase in ExecutionPhase: + assert phase.value == phase.value.lower() + + def test_phase_count(self): + """Expected number of phases exists.""" + # planning, coding, qa_review, qa_fixing, complete, failed + assert len(ExecutionPhase) == 6 + + def test_planning_phase_exists(self): + """PLANNING phase has correct value.""" + assert ExecutionPhase.PLANNING.value == "planning" + + def test_coding_phase_exists(self): + """CODING phase has correct value.""" + assert ExecutionPhase.CODING.value == "coding" + + def test_qa_review_phase_exists(self): + """QA_REVIEW phase has correct value.""" + assert ExecutionPhase.QA_REVIEW.value == "qa_review" + + def test_qa_fixing_phase_exists(self): + """QA_FIXING phase has correct value.""" + assert ExecutionPhase.QA_FIXING.value == "qa_fixing" + + def test_complete_phase_exists(self): + """COMPLETE phase has correct value.""" + assert ExecutionPhase.COMPLETE.value == "complete" + + def test_failed_phase_exists(self): + """FAILED phase has correct value.""" + assert ExecutionPhase.FAILED.value == "failed" + + def test_phase_is_string_subclass(self): + """ExecutionPhase inherits from str for easy serialization.""" + assert issubclass(ExecutionPhase, str) + + +class TestMarkerFormat: + """Tests for marker format consistency.""" + + def test_marker_prefix_constant(self): + """PHASE_MARKER_PREFIX is correct.""" + assert PHASE_MARKER_PREFIX == "__EXEC_PHASE__:" + + def test_marker_prefix_ends_with_colon(self): + """Marker ends with colon for easy JSON parsing.""" + assert PHASE_MARKER_PREFIX.endswith(":") + + +class TestEmitPhase: + """Tests for emit_phase function.""" + + def test_emits_valid_json(self, capsys): + """Emits valid JSON with marker prefix.""" + emit_phase(ExecutionPhase.CODING, "Test message") + captured = capsys.readouterr() + + assert PHASE_MARKER_PREFIX in captured.out + # Extract JSON part + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert isinstance(payload, dict) + + def test_includes_phase_field(self, capsys): + """Output includes phase field.""" + emit_phase(ExecutionPhase.PLANNING, "Starting") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert "phase" in payload + assert payload["phase"] == "planning" + + def test_includes_message_field(self, capsys): + """Output includes message field.""" + emit_phase(ExecutionPhase.CODING, "Building feature") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert "message" in payload + assert payload["message"] == "Building feature" + + def test_optional_progress_field(self, capsys): + """Progress field is included when provided.""" + emit_phase(ExecutionPhase.CODING, "Working", progress=50) + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert "progress" in payload + assert payload["progress"] == 50 + + def test_progress_not_included_when_none(self, capsys): + """Progress field is not included when None.""" + emit_phase(ExecutionPhase.CODING, "Working") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert "progress" not in payload + + def test_optional_subtask_field(self, capsys): + """Subtask field is included when provided.""" + emit_phase(ExecutionPhase.CODING, "Working", subtask="subtask-1") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert "subtask" in payload + assert payload["subtask"] == "subtask-1" + + def test_subtask_not_included_when_none(self, capsys): + """Subtask field is not included when None.""" + emit_phase(ExecutionPhase.CODING, "Working") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert "subtask" not in payload + + def test_enum_value_extracted(self, capsys): + """ExecutionPhase enum is converted to string value.""" + emit_phase(ExecutionPhase.QA_REVIEW, "Reviewing") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert payload["phase"] == "qa_review" + + def test_string_phase_accepted(self, capsys): + """String phase value is accepted.""" + emit_phase("custom_phase", "Custom") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert payload["phase"] == "custom_phase" + + def test_output_ends_with_newline(self, capsys): + """Output ends with newline for line-based parsing.""" + emit_phase(ExecutionPhase.CODING, "Test") + captured = capsys.readouterr() + assert captured.out.endswith("\n") + + def test_all_fields_together(self, capsys): + """All fields work together correctly.""" + emit_phase( + ExecutionPhase.CODING, + "Working on feature", + progress=75, + subtask="feat-123", + ) + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + + assert payload["phase"] == "coding" + assert payload["message"] == "Working on feature" + assert payload["progress"] == 75 + assert payload["subtask"] == "feat-123" + + +class TestEdgeCases: + """Tests for edge case handling.""" + + def test_empty_message_allowed(self, capsys): + """Empty message is valid.""" + emit_phase(ExecutionPhase.CODING, "") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert payload["message"] == "" + + def test_unicode_in_message(self, capsys): + """Unicode characters are handled correctly.""" + emit_phase(ExecutionPhase.CODING, "Building πŸš€ feature with Γ©mojis") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert "πŸš€" in payload["message"] + assert "Γ©mojis" in payload["message"] + + def test_special_json_chars_escaped(self, capsys): + """Special JSON characters (quotes, backslash) are escaped.""" + emit_phase(ExecutionPhase.CODING, 'Message with "quotes" and \\backslash') + captured = capsys.readouterr() + + # Should be valid JSON + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert '"quotes"' in payload["message"] + assert "\\backslash" in payload["message"] + + def test_newline_in_message(self, capsys): + """Newlines in message are properly serialized as JSON.""" + emit_phase(ExecutionPhase.CODING, "Line1\nLine2") + captured = capsys.readouterr() + + # Output should be single line (JSON escaped newline) + lines = captured.out.strip().split("\n") + assert len(lines) == 1, "Output should be single line" + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + # JSON.loads unescapes the newline + assert payload["message"] == "Line1\nLine2" + + def test_carriage_return_in_message(self, capsys): + """Carriage returns are handled.""" + emit_phase(ExecutionPhase.CODING, "Line1\r\nLine2") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert "Line1" in payload["message"] + assert "Line2" in payload["message"] + + def test_tab_in_message(self, capsys): + """Tab characters are handled.""" + emit_phase(ExecutionPhase.CODING, "Col1\tCol2") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert "\t" in payload["message"] + + def test_very_long_message(self, capsys): + """Very long messages are handled.""" + long_message = "x" * 10000 + emit_phase(ExecutionPhase.CODING, long_message) + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + # Either full message or truncated is acceptable + assert len(payload["message"]) > 0 + + def test_progress_zero(self, capsys): + """Progress of 0 is included (not treated as falsy).""" + emit_phase(ExecutionPhase.CODING, "Starting", progress=0) + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert "progress" in payload + assert payload["progress"] == 0 + + def test_progress_100(self, capsys): + """Progress of 100 works correctly.""" + emit_phase(ExecutionPhase.COMPLETE, "Done", progress=100) + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert payload["progress"] == 100 + + def test_subtask_with_special_chars(self, capsys): + """Subtask with special characters works.""" + emit_phase(ExecutionPhase.CODING, "Working", subtask="feat/add-login#123") + captured = capsys.readouterr() + + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + assert payload["subtask"] == "feat/add-login#123" + + +class TestErrorHandling: + """Tests for error handling.""" + + def test_oserror_handled_silently(self, monkeypatch): + """OSError during print is handled silently.""" + + def raise_oserror(*args, **kwargs): + raise OSError("Broken pipe") + + monkeypatch.setattr("builtins.print", raise_oserror) + + # Should not raise + emit_phase(ExecutionPhase.CODING, "Test") + + def test_unicode_encode_error_handled(self, monkeypatch): + """UnicodeEncodeError is handled silently.""" + + def raise_unicode_error(*args, **kwargs): + raise UnicodeEncodeError("utf-8", "", 0, 1, "test") + + monkeypatch.setattr("builtins.print", raise_unicode_error) + + # Should not raise + emit_phase(ExecutionPhase.CODING, "Test") + + def test_debug_mode_logs_errors(self, monkeypatch, capsys): + """In debug mode, errors are logged to stderr.""" + monkeypatch.setenv("DEBUG", "true") + + import importlib + from core import phase_event + + importlib.reload(phase_event) + + call_count = [0] + original_print = print + + def raise_oserror_once(*args, **kwargs): + call_count[0] += 1 + if call_count[0] == 1: + raise OSError("Test error") + return original_print(*args, **kwargs) + + monkeypatch.setattr("builtins.print", raise_oserror_once) + + from core.phase_event import emit_phase as emit_phase_reloaded + + emit_phase_reloaded(ExecutionPhase.CODING, "Test") + + captured = capsys.readouterr() + assert "emit failed" in captured.err + + +class TestPhaseTransitions: + """Tests for typical phase transition scenarios.""" + + def test_planning_to_coding(self, capsys): + """Typical planning β†’ coding transition.""" + emit_phase(ExecutionPhase.PLANNING, "Creating plan") + emit_phase(ExecutionPhase.CODING, "Starting implementation") + + captured = capsys.readouterr() + lines = captured.out.strip().split("\n") + assert len(lines) == 2 + + # First line is planning + payload1 = json.loads(lines[0].replace(PHASE_MARKER_PREFIX, "")) + assert payload1["phase"] == "planning" + + # Second line is coding + payload2 = json.loads(lines[1].replace(PHASE_MARKER_PREFIX, "")) + assert payload2["phase"] == "coding" + + def test_coding_to_qa_review(self, capsys): + """Typical coding β†’ qa_review transition.""" + emit_phase(ExecutionPhase.CODING, "Done coding") + emit_phase(ExecutionPhase.QA_REVIEW, "Starting QA") + + captured = capsys.readouterr() + lines = captured.out.strip().split("\n") + + payload2 = json.loads(lines[1].replace(PHASE_MARKER_PREFIX, "")) + assert payload2["phase"] == "qa_review" + + def test_qa_review_to_complete(self, capsys): + """Typical qa_review β†’ complete transition.""" + emit_phase(ExecutionPhase.QA_REVIEW, "Reviewing") + emit_phase(ExecutionPhase.COMPLETE, "QA passed") + + captured = capsys.readouterr() + lines = captured.out.strip().split("\n") + + payload2 = json.loads(lines[1].replace(PHASE_MARKER_PREFIX, "")) + assert payload2["phase"] == "complete" + + def test_qa_review_to_qa_fixing(self, capsys): + """Typical qa_review β†’ qa_fixing transition.""" + emit_phase(ExecutionPhase.QA_REVIEW, "Found issues") + emit_phase(ExecutionPhase.QA_FIXING, "Fixing issues") + + captured = capsys.readouterr() + lines = captured.out.strip().split("\n") + + payload2 = json.loads(lines[1].replace(PHASE_MARKER_PREFIX, "")) + assert payload2["phase"] == "qa_fixing" + + def test_failed_phase(self, capsys): + """Failed phase emission.""" + emit_phase(ExecutionPhase.FAILED, "Build failed: test error") + + captured = capsys.readouterr() + json_str = captured.out.strip().replace(PHASE_MARKER_PREFIX, "") + payload = json.loads(json_str) + + assert payload["phase"] == "failed" + assert "Build failed" in payload["message"] + + +class TestIntegration: + """Integration tests simulating real usage patterns.""" + + def test_full_successful_workflow(self, capsys): + """Simulate complete successful build workflow.""" + emit_phase(ExecutionPhase.PLANNING, "Creating implementation plan") + emit_phase(ExecutionPhase.CODING, "Starting implementation", subtask="1/3") + emit_phase( + ExecutionPhase.CODING, "Implementing feature", subtask="2/3", progress=33 + ) + emit_phase(ExecutionPhase.CODING, "Finalizing", subtask="3/3", progress=66) + emit_phase(ExecutionPhase.QA_REVIEW, "Running QA validation") + emit_phase(ExecutionPhase.COMPLETE, "QA validation passed", progress=100) + + captured = capsys.readouterr() + lines = captured.out.strip().split("\n") + + assert len(lines) == 6 + + # Verify final phase + final = json.loads(lines[-1].replace(PHASE_MARKER_PREFIX, "")) + assert final["phase"] == "complete" + assert final["progress"] == 100 + + def test_workflow_with_qa_fixes(self, capsys): + """Simulate workflow with QA rejection and fixes.""" + emit_phase(ExecutionPhase.PLANNING, "Planning") + emit_phase(ExecutionPhase.CODING, "Coding") + emit_phase(ExecutionPhase.QA_REVIEW, "First review") + emit_phase(ExecutionPhase.QA_FIXING, "Fixing issues") + emit_phase(ExecutionPhase.QA_REVIEW, "Second review") + emit_phase(ExecutionPhase.COMPLETE, "Passed on second try") + + captured = capsys.readouterr() + lines = captured.out.strip().split("\n") + + assert len(lines) == 6 + + # Verify we had two QA reviews + phases = [ + json.loads(line.replace(PHASE_MARKER_PREFIX, ""))["phase"] for line in lines + ] + assert phases.count("qa_review") == 2 + assert phases.count("qa_fixing") == 1 + + def test_failed_workflow(self, capsys): + """Simulate failed build workflow.""" + emit_phase(ExecutionPhase.PLANNING, "Planning") + emit_phase(ExecutionPhase.CODING, "Coding") + emit_phase(ExecutionPhase.FAILED, "Unrecoverable error occurred") + + captured = capsys.readouterr() + lines = captured.out.strip().split("\n") + + assert len(lines) == 3 + + final = json.loads(lines[-1].replace(PHASE_MARKER_PREFIX, "")) + assert final["phase"] == "failed"