diff --git a/.geminiignore b/.geminiignore new file mode 100644 index 00000000..703ef020 --- /dev/null +++ b/.geminiignore @@ -0,0 +1,14 @@ +# Auto-generated by Automaker to speed up Gemini CLI startup +# Prevents Gemini CLI from scanning large directories during context discovery +.git +node_modules +dist +build +.next +.nuxt +coverage +.automaker +.worktrees +.vscode +.idea +*.lock diff --git a/apps/server/src/index.ts b/apps/server/src/index.ts index dcd45da8..48850472 100644 --- a/apps/server/src/index.ts +++ b/apps/server/src/index.ts @@ -434,21 +434,18 @@ eventHookService.initialize(events, settingsService, eventHistoryService, featur logger.info('[STARTUP] Feature state reconciliation complete - no stale states found'); } - // Resume interrupted features in the background after reconciliation. - // This uses the saved execution state to identify features that were running - // before the restart (their statuses have been reset to ready/backlog by - // reconciliation above). Running in background so it doesn't block startup. - if (totalReconciled > 0) { - for (const project of globalSettings.projects) { - autoModeService.resumeInterruptedFeatures(project.path).catch((err) => { - logger.warn( - `[STARTUP] Failed to resume interrupted features for ${project.path}:`, - err - ); - }); - } - logger.info('[STARTUP] Initiated background resume of interrupted features'); + // Resume interrupted features in the background for all projects. + // This handles features stuck in transient states (in_progress, pipeline_*) + // or explicitly marked as interrupted. Running in background so it doesn't block startup. + for (const project of globalSettings.projects) { + autoModeService.resumeInterruptedFeatures(project.path).catch((err) => { + logger.warn( + `[STARTUP] Failed to resume interrupted features for ${project.path}:`, + err + ); + }); } + logger.info('[STARTUP] Initiated background resume of interrupted features'); } } catch (err) { logger.warn('[STARTUP] Failed to reconcile feature states:', err); diff --git a/apps/server/src/services/agent-executor-types.ts b/apps/server/src/services/agent-executor-types.ts index e84964a1..ef0dcb68 100644 --- a/apps/server/src/services/agent-executor-types.ts +++ b/apps/server/src/services/agent-executor-types.ts @@ -44,6 +44,8 @@ export interface AgentExecutionOptions { specAlreadyDetected?: boolean; existingApprovedPlanContent?: string; persistedTasks?: ParsedTask[]; + /** Feature status - used to check if pipeline summary extraction is required */ + status?: string; } export interface AgentExecutionResult { diff --git a/apps/server/src/services/agent-executor.ts b/apps/server/src/services/agent-executor.ts index f7930766..15731b92 100644 --- a/apps/server/src/services/agent-executor.ts +++ b/apps/server/src/services/agent-executor.ts @@ -4,6 +4,7 @@ import path from 'path'; import type { ExecuteOptions, ParsedTask } from '@automaker/types'; +import { isPipelineStatus } from '@automaker/types'; import { buildPromptWithImages, createLogger, isAuthenticationError } from '@automaker/utils'; import { getFeatureDir } from '@automaker/platform'; import * as secureFs from '../lib/secure-fs.js'; @@ -91,6 +92,7 @@ export class AgentExecutor { existingApprovedPlanContent, persistedTasks, credentials, + status, // Feature status for pipeline summary check claudeCompatibleProvider, mcpServers, sdkSessionId, @@ -207,6 +209,17 @@ export class AgentExecutor { if (writeTimeout) clearTimeout(writeTimeout); if (rawWriteTimeout) clearTimeout(rawWriteTimeout); await writeToFile(); + + // Extract and save summary from the new content generated in this session + await this.extractAndSaveSessionSummary( + projectPath, + featureId, + result.responseText, + previousContent, + callbacks, + status + ); + return { responseText: result.responseText, specDetected: true, @@ -340,9 +353,78 @@ export class AgentExecutor { } } } + + // Capture summary if it hasn't been captured by handleSpecGenerated or executeTasksLoop + // or if we're in a simple execution mode (planningMode='skip') + await this.extractAndSaveSessionSummary( + projectPath, + featureId, + responseText, + previousContent, + callbacks, + status + ); + return { responseText, specDetected, tasksCompleted, aborted }; } + /** + * Strip the follow-up session scaffold marker from content. + * The scaffold is added when resuming a session with previous content: + * "\n\n---\n\n## Follow-up Session\n\n" + * This ensures fallback summaries don't include the scaffold header. + * + * The regex pattern handles variations in whitespace while matching the + * scaffold structure: dashes followed by "## Follow-up Session" at the + * start of the content. + */ + private static stripFollowUpScaffold(content: string): string { + // Pattern matches: ^\s*---\s*##\s*Follow-up Session\s* + // - ^ = start of content (scaffold is always at the beginning of sessionContent) + // - \s* = any whitespace (handles \n\n before ---, spaces/tabs between markers) + // - --- = literal dashes + // - \s* = whitespace between dashes and heading + // - ## = heading marker + // - \s* = whitespace before "Follow-up" + // - Follow-up Session = literal heading text + // - \s* = trailing whitespace/newlines after heading + const scaffoldPattern = /^\s*---\s*##\s*Follow-up Session\s*/; + return content.replace(scaffoldPattern, ''); + } + + /** + * Extract summary ONLY from the new content generated in this session + * and save it via the provided callback. + */ + private async extractAndSaveSessionSummary( + projectPath: string, + featureId: string, + responseText: string, + previousContent: string | undefined, + callbacks: AgentExecutorCallbacks, + status?: string + ): Promise { + const sessionContent = responseText.substring(previousContent ? previousContent.length : 0); + const summary = extractSummary(sessionContent); + if (summary) { + await callbacks.saveFeatureSummary(projectPath, featureId, summary); + return; + } + + // If we're in a pipeline step, a summary is expected. Use a fallback if extraction fails. + if (isPipelineStatus(status)) { + // Strip any follow-up session scaffold before using as fallback + const cleanSessionContent = AgentExecutor.stripFollowUpScaffold(sessionContent); + const fallback = cleanSessionContent.trim(); + if (fallback) { + await callbacks.saveFeatureSummary(projectPath, featureId, fallback); + } + logger.warn( + `[AgentExecutor] Mandatory summary extraction failed for pipeline feature ${featureId} (status="${status}")` + ); + } + } + private async executeTasksLoop( options: AgentExecutionOptions, tasks: ParsedTask[], @@ -439,14 +521,15 @@ export class AgentExecutor { } } if (!taskCompleteDetected) { - const cid = detectTaskCompleteMarker(taskOutput); - if (cid) { + const completeMarker = detectTaskCompleteMarker(taskOutput); + if (completeMarker) { taskCompleteDetected = true; await this.featureStateManager.updateTaskStatus( projectPath, featureId, - cid, - 'completed' + completeMarker.id, + 'completed', + completeMarker.summary ); } } @@ -524,8 +607,6 @@ export class AgentExecutor { } } } - const summary = extractSummary(responseText); - if (summary) await callbacks.saveFeatureSummary(projectPath, featureId, summary); return { responseText, tasksCompleted, aborted: false }; } @@ -722,8 +803,6 @@ export class AgentExecutor { ); responseText = r.responseText; } - const summary = extractSummary(responseText); - if (summary) await callbacks.saveFeatureSummary(projectPath, featureId, summary); return { responseText, tasksCompleted }; } diff --git a/apps/server/src/services/auto-mode/facade.ts b/apps/server/src/services/auto-mode/facade.ts index af660ea5..6999beb0 100644 --- a/apps/server/src/services/auto-mode/facade.ts +++ b/apps/server/src/services/auto-mode/facade.ts @@ -15,7 +15,12 @@ import path from 'path'; import { exec } from 'child_process'; import { promisify } from 'util'; import type { Feature, PlanningMode, ThinkingLevel, ReasoningEffort } from '@automaker/types'; -import { DEFAULT_MAX_CONCURRENCY, DEFAULT_MODELS, stripProviderPrefix } from '@automaker/types'; +import { + DEFAULT_MAX_CONCURRENCY, + DEFAULT_MODELS, + stripProviderPrefix, + isPipelineStatus, +} from '@automaker/types'; import { resolveModelString } from '@automaker/model-resolver'; import { createLogger, loadContextFiles, classifyError } from '@automaker/utils'; import { getFeatureDir } from '@automaker/platform'; @@ -79,6 +84,37 @@ export class AutoModeServiceFacade { private readonly settingsService: SettingsService | null ) {} + /** + * Determine if a feature is eligible to be picked up by the auto-mode loop. + * + * @param feature - The feature to check + * @param branchName - The current worktree branch name (null for main) + * @param primaryBranch - The resolved primary branch name for the project + * @returns True if the feature is eligible for auto-dispatch + */ + public static isFeatureEligibleForAutoMode( + feature: Feature, + branchName: string | null, + primaryBranch: string | null + ): boolean { + const isEligibleStatus = + feature.status === 'backlog' || + feature.status === 'ready' || + feature.status === 'interrupted' || + isPipelineStatus(feature.status); + + if (!isEligibleStatus) return false; + + // Filter by branch/worktree alignment + if (branchName === null) { + // For main worktree, include features with no branch or matching primary branch + return !feature.branchName || (primaryBranch != null && feature.branchName === primaryBranch); + } else { + // For named worktrees, only include features matching that branch + return feature.branchName === branchName; + } + } + /** * Classify and log an error at the facade boundary. * Emits an error event to the UI so failures are surfaced to the user. @@ -217,6 +253,7 @@ export class AutoModeServiceFacade { thinkingLevel?: ThinkingLevel; reasoningEffort?: ReasoningEffort; branchName?: string | null; + status?: string; // Feature status for pipeline summary check [key: string]: unknown; } ): Promise => { @@ -300,6 +337,7 @@ export class AutoModeServiceFacade { thinkingLevel: opts?.thinkingLevel as ThinkingLevel | undefined, reasoningEffort: opts?.reasoningEffort as ReasoningEffort | undefined, branchName: opts?.branchName as string | null | undefined, + status: opts?.status as string | undefined, provider, effectiveBareModel, credentials, @@ -373,12 +411,8 @@ export class AutoModeServiceFacade { if (branchName === null) { primaryBranch = await worktreeResolver.getCurrentBranch(pPath); } - return features.filter( - (f) => - (f.status === 'backlog' || f.status === 'ready') && - (branchName === null - ? !f.branchName || (primaryBranch && f.branchName === primaryBranch) - : f.branchName === branchName) + return features.filter((f) => + AutoModeServiceFacade.isFeatureEligibleForAutoMode(f, branchName, primaryBranch) ); }, (pPath, branchName, maxConcurrency) => diff --git a/apps/server/src/services/execution-service.ts b/apps/server/src/services/execution-service.ts index 633ac809..3139a1cf 100644 --- a/apps/server/src/services/execution-service.ts +++ b/apps/server/src/services/execution-service.ts @@ -461,7 +461,10 @@ Please continue from where you left off and complete all remaining tasks. Use th const hasIncompleteTasks = totalTasks > 0 && completedTasks < totalTasks; try { - if (agentOutput) { + // Only save summary if feature doesn't already have one (e.g., accumulated from pipeline steps) + // This prevents overwriting accumulated summaries with just the last step's output + // The agent-executor already extracts and saves summaries during execution + if (agentOutput && !completedFeature?.summary) { const summary = extractSummary(agentOutput); if (summary) await this.saveFeatureSummaryFn(projectPath, featureId, summary); } diff --git a/apps/server/src/services/feature-state-manager.ts b/apps/server/src/services/feature-state-manager.ts index 1f8a4952..aa53d08a 100644 --- a/apps/server/src/services/feature-state-manager.ts +++ b/apps/server/src/services/feature-state-manager.ts @@ -14,7 +14,8 @@ */ import path from 'path'; -import type { Feature, ParsedTask, PlanSpec } from '@automaker/types'; +import type { Feature, FeatureStatusWithPipeline, ParsedTask, PlanSpec } from '@automaker/types'; +import { isPipelineStatus } from '@automaker/types'; import { atomicWriteJson, readJsonWithRecovery, @@ -28,6 +29,7 @@ import type { EventEmitter } from '../lib/events.js'; import type { AutoModeEventType } from './typed-event-bus.js'; import { getNotificationService } from './notification-service.js'; import { FeatureLoader } from './feature-loader.js'; +import { pipelineService } from './pipeline-service.js'; const logger = createLogger('FeatureStateManager'); @@ -252,7 +254,7 @@ export class FeatureStateManager { const currentStatus = feature?.status; // Preserve pipeline_* statuses so resumePipelineFeature can resume from the correct step - if (currentStatus && currentStatus.startsWith('pipeline_')) { + if (isPipelineStatus(currentStatus)) { logger.info( `Feature ${featureId} was in ${currentStatus}; preserving pipeline status for resume` ); @@ -270,7 +272,8 @@ export class FeatureStateManager { /** * Shared helper that scans features in a project directory and resets any stuck - * in transient states (in_progress, interrupted, pipeline_*) back to resting states. + * in transient states (in_progress, interrupted) back to resting states. + * Pipeline_* statuses are preserved so they can be resumed. * * Also resets: * - generating planSpec status back to pending @@ -324,10 +327,7 @@ export class FeatureStateManager { // Reset features in active execution states back to a resting state // After a server restart, no processes are actually running - const isActiveState = - originalStatus === 'in_progress' || - originalStatus === 'interrupted' || - (originalStatus != null && originalStatus.startsWith('pipeline_')); + const isActiveState = originalStatus === 'in_progress' || originalStatus === 'interrupted'; if (isActiveState) { const hasApprovedPlan = feature.planSpec?.status === 'approved'; @@ -338,6 +338,17 @@ export class FeatureStateManager { ); } + // Handle pipeline_* statuses separately: preserve them so they can be resumed + // but still count them as needing attention if they were stuck. + if (isPipelineStatus(originalStatus)) { + // We don't change the status, but we still want to reset planSpec/task states + // if they were stuck in transient generation/execution modes. + // No feature.status change here. + logger.debug( + `[${callerLabel}] Preserving pipeline status for feature ${feature.id}: ${originalStatus}` + ); + } + // Reset generating planSpec status back to pending (spec generation was interrupted) if (feature.planSpec?.status === 'generating') { feature.planSpec.status = 'pending'; @@ -396,10 +407,12 @@ export class FeatureStateManager { * Resets: * - in_progress features back to ready (if has plan) or backlog (if no plan) * - interrupted features back to ready (if has plan) or backlog (if no plan) - * - pipeline_* features back to ready (if has plan) or backlog (if no plan) * - generating planSpec status back to pending * - in_progress tasks back to pending * + * Preserves: + * - pipeline_* statuses (so resumePipelineFeature can resume from correct step) + * * @param projectPath - The project path to reset features for */ async resetStuckFeatures(projectPath: string): Promise { @@ -530,6 +543,10 @@ export class FeatureStateManager { * This is called after agent execution completes to save a summary * extracted from the agent's output using tags. * + * For pipeline features (status starts with pipeline_), summaries are accumulated + * across steps with a header identifying each step. For non-pipeline features, + * the summary is replaced entirely. + * * @param projectPath - The project path * @param featureId - The feature ID * @param summary - The summary text to save @@ -537,6 +554,7 @@ export class FeatureStateManager { async saveFeatureSummary(projectPath: string, featureId: string, summary: string): Promise { const featureDir = getFeatureDir(projectPath, featureId); const featurePath = path.join(featureDir, 'feature.json'); + const normalizedSummary = summary.trim(); try { const result = await readJsonWithRecovery(featurePath, null, { @@ -552,7 +570,63 @@ export class FeatureStateManager { return; } - feature.summary = summary; + if (!normalizedSummary) { + logger.debug( + `[saveFeatureSummary] Skipping empty summary for feature ${featureId} (status="${feature.status}")` + ); + return; + } + + // For pipeline features, accumulate summaries across steps + if (isPipelineStatus(feature.status)) { + // If we already have a non-phase summary (typically the initial implementation + // summary from in_progress), normalize it into a named phase before appending + // pipeline step summaries. This keeps the format consistent for UI phase parsing. + const implementationHeader = '### Implementation'; + if (feature.summary && !feature.summary.trimStart().startsWith('### ')) { + feature.summary = `${implementationHeader}\n\n${feature.summary.trim()}`; + } + + const stepName = await this.getPipelineStepName(projectPath, feature.status); + const stepHeader = `### ${stepName}`; + const stepSection = `${stepHeader}\n\n${normalizedSummary}`; + + if (feature.summary) { + // Check if this step already exists in the summary (e.g., if retried) + // Use section splitting to only match real section boundaries, not text in body content + const separator = '\n\n---\n\n'; + const sections = feature.summary.split(separator); + let replaced = false; + const updatedSections = sections.map((section) => { + if (section.startsWith(`${stepHeader}\n\n`)) { + replaced = true; + return stepSection; + } + return section; + }); + + if (replaced) { + feature.summary = updatedSections.join(separator); + logger.info( + `[saveFeatureSummary] Updated existing pipeline step summary for feature ${featureId}: step="${stepName}"` + ); + } else { + // Append as a new section + feature.summary = `${feature.summary}${separator}${stepSection}`; + logger.info( + `[saveFeatureSummary] Appended new pipeline step summary for feature ${featureId}: step="${stepName}"` + ); + } + } else { + feature.summary = stepSection; + logger.info( + `[saveFeatureSummary] Initialized pipeline summary for feature ${featureId}: step="${stepName}"` + ); + } + } else { + feature.summary = normalizedSummary; + } + feature.updatedAt = new Date().toISOString(); // PERSIST BEFORE EMIT @@ -562,13 +636,42 @@ export class FeatureStateManager { this.emitAutoModeEvent('auto_mode_summary', { featureId, projectPath, - summary, + summary: feature.summary, }); } catch (error) { logger.error(`Failed to save summary for ${featureId}:`, error); } } + /** + * Look up the pipeline step name from the current pipeline status. + * + * @param projectPath - The project path + * @param status - The current pipeline status (e.g., 'pipeline_abc123') + * @returns The step name, or a fallback based on the step ID + */ + private async getPipelineStepName(projectPath: string, status: string): Promise { + try { + const stepId = pipelineService.getStepIdFromStatus(status as FeatureStatusWithPipeline); + if (stepId) { + const step = await pipelineService.getStep(projectPath, stepId); + if (step) return step.name; + } + } catch (error) { + logger.debug( + `[getPipelineStepName] Failed to look up step name for status "${status}", using fallback:`, + error + ); + } + // Fallback: derive a human-readable name from the status suffix + // e.g., 'pipeline_code_review' → 'Code Review' + const suffix = status.replace('pipeline_', ''); + return suffix + .split('_') + .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) + .join(' '); + } + /** * Update the status of a specific task within planSpec.tasks * @@ -581,7 +684,8 @@ export class FeatureStateManager { projectPath: string, featureId: string, taskId: string, - status: ParsedTask['status'] + status: ParsedTask['status'], + summary?: string ): Promise { const featureDir = getFeatureDir(projectPath, featureId); const featurePath = path.join(featureDir, 'feature.json'); @@ -604,6 +708,9 @@ export class FeatureStateManager { const task = feature.planSpec.tasks.find((t) => t.id === taskId); if (task) { task.status = status; + if (summary) { + task.summary = summary; + } feature.updatedAt = new Date().toISOString(); // PERSIST BEFORE EMIT @@ -615,6 +722,7 @@ export class FeatureStateManager { projectPath, taskId, status, + summary, tasks: feature.planSpec.tasks, }); } else { diff --git a/apps/server/src/services/pipeline-orchestrator.ts b/apps/server/src/services/pipeline-orchestrator.ts index c8564b18..6548592f 100644 --- a/apps/server/src/services/pipeline-orchestrator.ts +++ b/apps/server/src/services/pipeline-orchestrator.ts @@ -115,6 +115,7 @@ export class PipelineOrchestrator { projectPath, }); const model = resolveModelString(feature.model, DEFAULT_MODELS.claude); + const currentStatus = `pipeline_${step.id}`; await this.runAgentFn( workDir, featureId, @@ -133,6 +134,7 @@ export class PipelineOrchestrator { useClaudeCodeSystemPrompt, thinkingLevel: feature.thinkingLevel, reasoningEffort: feature.reasoningEffort, + status: currentStatus, } ); try { @@ -165,7 +167,18 @@ export class PipelineOrchestrator { if (previousContext) prompt += `### Previous Work\n${previousContext}\n\n`; return ( prompt + - `### Pipeline Step Instructions\n${step.instructions}\n\n### Task\nComplete the pipeline step instructions above.` + `### Pipeline Step Instructions\n${step.instructions}\n\n### Task\nComplete the pipeline step instructions above.\n\n` + + `**CRITICAL: After completing the instructions, you MUST output a summary using this EXACT format:**\n\n` + + `\n` + + `## Summary: ${step.name}\n\n` + + `### Changes Implemented\n` + + `- [List all changes made in this step]\n\n` + + `### Files Modified\n` + + `- [List all files modified in this step]\n\n` + + `### Outcome\n` + + `- [Describe the result of this step]\n` + + `\n\n` + + `The and tags MUST be on their own lines. This is REQUIRED.` ); } @@ -491,6 +504,7 @@ export class PipelineOrchestrator { useClaudeCodeSystemPrompt: context.useClaudeCodeSystemPrompt, autoLoadClaudeMd: context.autoLoadClaudeMd, reasoningEffort: context.feature.reasoningEffort, + status: context.feature.status, } ); } diff --git a/apps/server/src/services/spec-parser.ts b/apps/server/src/services/spec-parser.ts index cd1c8050..1c9f527e 100644 --- a/apps/server/src/services/spec-parser.ts +++ b/apps/server/src/services/spec-parser.ts @@ -101,12 +101,32 @@ export function detectTaskStartMarker(text: string): string | null { } /** - * Detect [TASK_COMPLETE] marker in text and extract task ID + * Detect [TASK_COMPLETE] marker in text and extract task ID and summary * Format: [TASK_COMPLETE] T###: Brief summary */ -export function detectTaskCompleteMarker(text: string): string | null { - const match = text.match(/\[TASK_COMPLETE\]\s*(T\d{3})/); - return match ? match[1] : null; +export function detectTaskCompleteMarker(text: string): { id: string; summary?: string } | null { + // Use a regex that captures the summary until newline or next task marker + // Allow brackets in summary content (e.g., "supports array[index] access") + // Pattern breakdown: + // - \[TASK_COMPLETE\]\s* - Match the marker + // - (T\d{3}) - Capture task ID + // - (?::\s*([^\n\[]+))? - Optionally capture summary (stops at newline or bracket) + // - But we want to allow brackets in summary, so we use a different approach: + // - Match summary until newline, then trim any trailing markers in post-processing + const match = text.match(/\[TASK_COMPLETE\]\s*(T\d{3})(?::\s*(.+?))?(?=\n|$)/i); + if (!match) return null; + + // Post-process: remove trailing task markers from summary if present + let summary = match[2]?.trim(); + if (summary) { + // Remove trailing content that looks like another marker + summary = summary.replace(/\s*\[TASK_[A-Z_]+\].*$/i, '').trim(); + } + + return { + id: match[1], + summary: summary || undefined, + }; } /** diff --git a/apps/server/tests/unit/services/agent-executor-summary.test.ts b/apps/server/tests/unit/services/agent-executor-summary.test.ts new file mode 100644 index 00000000..3bb3cc06 --- /dev/null +++ b/apps/server/tests/unit/services/agent-executor-summary.test.ts @@ -0,0 +1,446 @@ +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import { AgentExecutor } from '../../../src/services/agent-executor.js'; +import type { TypedEventBus } from '../../../src/services/typed-event-bus.js'; +import type { FeatureStateManager } from '../../../src/services/feature-state-manager.js'; +import type { PlanApprovalService } from '../../../src/services/plan-approval-service.js'; +import type { BaseProvider } from '../../../src/providers/base-provider.js'; +import * as secureFs from '../../../src/lib/secure-fs.js'; +import { getFeatureDir } from '@automaker/platform'; +import { buildPromptWithImages } from '@automaker/utils'; + +vi.mock('../../../src/lib/secure-fs.js', () => ({ + mkdir: vi.fn().mockResolvedValue(undefined), + writeFile: vi.fn().mockResolvedValue(undefined), + appendFile: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockResolvedValue(''), +})); + +vi.mock('@automaker/platform', () => ({ + getFeatureDir: vi.fn(), +})); + +vi.mock('@automaker/utils', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + buildPromptWithImages: vi.fn(), + createLogger: vi.fn().mockReturnValue({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }), + }; +}); + +describe('AgentExecutor Summary Extraction', () => { + let mockEventBus: TypedEventBus; + let mockFeatureStateManager: FeatureStateManager; + let mockPlanApprovalService: PlanApprovalService; + + beforeEach(() => { + vi.clearAllMocks(); + + mockEventBus = { + emitAutoModeEvent: vi.fn(), + } as unknown as TypedEventBus; + + mockFeatureStateManager = { + updateTaskStatus: vi.fn().mockResolvedValue(undefined), + updateFeaturePlanSpec: vi.fn().mockResolvedValue(undefined), + saveFeatureSummary: vi.fn().mockResolvedValue(undefined), + } as unknown as FeatureStateManager; + + mockPlanApprovalService = { + waitForApproval: vi.fn(), + } as unknown as PlanApprovalService; + + (getFeatureDir as Mock).mockReturnValue('/mock/feature/dir'); + (buildPromptWithImages as Mock).mockResolvedValue({ content: 'mocked prompt' }); + }); + + it('should extract summary from new session content only', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + null + ); + + const previousContent = `Some previous work. +Old summary`; + const newWork = `New implementation work. +New summary`; + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: newWork }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const options = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet', + planningMode: 'skip' as const, + previousContent, + }; + + const callbacks = { + waitForApproval: vi.fn(), + saveFeatureSummary: vi.fn(), + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn(), + }; + + await executor.execute(options, callbacks); + + // Verify it called saveFeatureSummary with the NEW summary + expect(callbacks.saveFeatureSummary).toHaveBeenCalledWith( + '/project', + 'test-feature', + 'New summary' + ); + + // Ensure it didn't call it with Old summary + expect(callbacks.saveFeatureSummary).not.toHaveBeenCalledWith( + '/project', + 'test-feature', + 'Old summary' + ); + }); + + it('should not save summary if no summary in NEW session content', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + null + ); + + const previousContent = `Some previous work. +Old summary`; + const newWork = `New implementation work without a summary tag.`; + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: newWork }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const options = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet', + planningMode: 'skip' as const, + previousContent, + }; + + const callbacks = { + waitForApproval: vi.fn(), + saveFeatureSummary: vi.fn(), + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn(), + }; + + await executor.execute(options, callbacks); + + // Verify it NEVER called saveFeatureSummary because there was no NEW summary + expect(callbacks.saveFeatureSummary).not.toHaveBeenCalled(); + }); + + it('should extract task summary and update task status during streaming', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + null + ); + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: 'Working... ' }], + }, + }; + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: '[TASK_COMPLETE] T001: Task finished successfully' }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + // We trigger executeTasksLoop by providing persistedTasks + const options = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet', + planningMode: 'skip' as const, + existingApprovedPlanContent: 'Some plan', + persistedTasks: [{ id: 'T001', description: 'Task 1', status: 'pending' as const }], + }; + + const callbacks = { + waitForApproval: vi.fn(), + saveFeatureSummary: vi.fn(), + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + // Verify it updated task status with summary + expect(mockFeatureStateManager.updateTaskStatus).toHaveBeenCalledWith( + '/project', + 'test-feature', + 'T001', + 'completed', + 'Task finished successfully' + ); + }); + + describe('Pipeline step summary fallback', () => { + it('should save fallback summary when extraction fails for pipeline step', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + null + ); + + // Content without a summary tag (extraction will fail) + const newWork = 'Implementation completed without summary tag.'; + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: newWork }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const options = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet', + planningMode: 'skip' as const, + status: 'pipeline_step1' as const, // Pipeline status triggers fallback + }; + + const callbacks = { + waitForApproval: vi.fn(), + saveFeatureSummary: vi.fn(), + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn(), + }; + + await executor.execute(options, callbacks); + + // Verify fallback summary was saved with trimmed content + expect(callbacks.saveFeatureSummary).toHaveBeenCalledWith( + '/project', + 'test-feature', + 'Implementation completed without summary tag.' + ); + }); + + it('should not save fallback for non-pipeline status when extraction fails', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + null + ); + + // Content without a summary tag + const newWork = 'Implementation completed without summary tag.'; + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: newWork }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const options = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet', + planningMode: 'skip' as const, + status: 'in_progress' as const, // Non-pipeline status + }; + + const callbacks = { + waitForApproval: vi.fn(), + saveFeatureSummary: vi.fn(), + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn(), + }; + + await executor.execute(options, callbacks); + + // Verify no fallback was saved for non-pipeline status + expect(callbacks.saveFeatureSummary).not.toHaveBeenCalled(); + }); + + it('should not save empty fallback for pipeline step', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + null + ); + + // Empty/whitespace-only content + const newWork = ' \n\t '; + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: newWork }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const options = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet', + planningMode: 'skip' as const, + status: 'pipeline_step1' as const, + }; + + const callbacks = { + waitForApproval: vi.fn(), + saveFeatureSummary: vi.fn(), + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn(), + }; + + await executor.execute(options, callbacks); + + // Verify no fallback was saved since content was empty/whitespace + expect(callbacks.saveFeatureSummary).not.toHaveBeenCalled(); + }); + + it('should prefer extracted summary over fallback for pipeline step', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + null + ); + + // Content WITH a summary tag + const newWork = `Implementation details here. +Proper summary from extraction`; + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: newWork }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const options = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet', + planningMode: 'skip' as const, + status: 'pipeline_step1' as const, + }; + + const callbacks = { + waitForApproval: vi.fn(), + saveFeatureSummary: vi.fn(), + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn(), + }; + + await executor.execute(options, callbacks); + + // Verify extracted summary was saved, not the full content + expect(callbacks.saveFeatureSummary).toHaveBeenCalledWith( + '/project', + 'test-feature', + 'Proper summary from extraction' + ); + // Ensure it didn't save the full content as fallback + expect(callbacks.saveFeatureSummary).not.toHaveBeenCalledWith( + '/project', + 'test-feature', + expect.stringContaining('Implementation details here') + ); + }); + }); +}); diff --git a/apps/server/tests/unit/services/agent-executor.test.ts b/apps/server/tests/unit/services/agent-executor.test.ts index e47b1a01..f905de48 100644 --- a/apps/server/tests/unit/services/agent-executor.test.ts +++ b/apps/server/tests/unit/services/agent-executor.test.ts @@ -1235,4 +1235,471 @@ describe('AgentExecutor', () => { expect(typeof result.aborted).toBe('boolean'); }); }); + + describe('pipeline summary fallback with scaffold stripping', () => { + it('should strip follow-up scaffold from fallback summary when extraction fails', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + mockSettingsService + ); + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: 'Some agent output without summary markers' }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const saveFeatureSummary = vi.fn().mockResolvedValue(undefined); + + const options: AgentExecutionOptions = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet-4-6', + planningMode: 'skip', + previousContent: 'Previous session content', + status: 'pipeline_step1', // Pipeline status to trigger fallback + }; + + const callbacks = { + waitForApproval: vi.fn().mockResolvedValue({ approved: true }), + saveFeatureSummary, + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + // The fallback summary should be called without the scaffold header + expect(saveFeatureSummary).toHaveBeenCalled(); + const savedSummary = saveFeatureSummary.mock.calls[0][2]; + // Should not contain the scaffold header + expect(savedSummary).not.toContain('---'); + expect(savedSummary).not.toContain('Follow-up Session'); + // Should contain the actual content + expect(savedSummary).toContain('Some agent output without summary markers'); + }); + + it('should not save fallback when scaffold is the only content after stripping', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + mockSettingsService + ); + + // Provider yields no content - only scaffold will be present + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + // Empty stream - no actual content + }), + } as unknown as BaseProvider; + + const saveFeatureSummary = vi.fn().mockResolvedValue(undefined); + + const options: AgentExecutionOptions = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet-4-6', + planningMode: 'skip', + previousContent: 'Previous session content', + status: 'pipeline_step1', // Pipeline status + }; + + const callbacks = { + waitForApproval: vi.fn().mockResolvedValue({ approved: true }), + saveFeatureSummary, + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + // Should not save an empty fallback (after scaffold is stripped) + expect(saveFeatureSummary).not.toHaveBeenCalled(); + }); + + it('should save extracted summary when available, not fallback', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + mockSettingsService + ); + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [ + { + type: 'text', + text: 'Some content\n\nExtracted summary here\n\nMore content', + }, + ], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const saveFeatureSummary = vi.fn().mockResolvedValue(undefined); + + const options: AgentExecutionOptions = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet-4-6', + planningMode: 'skip', + previousContent: 'Previous session content', + status: 'pipeline_step1', // Pipeline status + }; + + const callbacks = { + waitForApproval: vi.fn().mockResolvedValue({ approved: true }), + saveFeatureSummary, + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + // Should save the extracted summary, not the full content + expect(saveFeatureSummary).toHaveBeenCalledTimes(1); + expect(saveFeatureSummary).toHaveBeenCalledWith( + '/project', + 'test-feature', + 'Extracted summary here' + ); + }); + + it('should handle scaffold with various whitespace patterns', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + mockSettingsService + ); + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: 'Agent response here' }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const saveFeatureSummary = vi.fn().mockResolvedValue(undefined); + + const options: AgentExecutionOptions = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet-4-6', + planningMode: 'skip', + previousContent: 'Previous session content', + status: 'pipeline_step1', + }; + + const callbacks = { + waitForApproval: vi.fn().mockResolvedValue({ approved: true }), + saveFeatureSummary, + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + // Should strip scaffold and save actual content + expect(saveFeatureSummary).toHaveBeenCalled(); + const savedSummary = saveFeatureSummary.mock.calls[0][2]; + expect(savedSummary.trim()).toBe('Agent response here'); + }); + + it('should handle scaffold with extra newlines between markers', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + mockSettingsService + ); + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: 'Actual content after scaffold' }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const saveFeatureSummary = vi.fn().mockResolvedValue(undefined); + + // Set up with previous content to trigger scaffold insertion + const options: AgentExecutionOptions = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet-4-6', + planningMode: 'skip', + previousContent: 'Previous session content', + status: 'pipeline_step1', + }; + + const callbacks = { + waitForApproval: vi.fn().mockResolvedValue({ approved: true }), + saveFeatureSummary, + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + expect(saveFeatureSummary).toHaveBeenCalled(); + const savedSummary = saveFeatureSummary.mock.calls[0][2]; + // Verify the scaffold is stripped + expect(savedSummary).not.toMatch(/---\s*##\s*Follow-up Session/); + }); + + it('should handle content without any scaffold (first session)', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + mockSettingsService + ); + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: 'First session output without summary' }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const saveFeatureSummary = vi.fn().mockResolvedValue(undefined); + + // No previousContent means no scaffold + const options: AgentExecutionOptions = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet-4-6', + planningMode: 'skip', + previousContent: undefined, // No previous content + status: 'pipeline_step1', + }; + + const callbacks = { + waitForApproval: vi.fn().mockResolvedValue({ approved: true }), + saveFeatureSummary, + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + expect(saveFeatureSummary).toHaveBeenCalled(); + const savedSummary = saveFeatureSummary.mock.calls[0][2]; + expect(savedSummary).toBe('First session output without summary'); + }); + + it('should handle non-pipeline status without saving fallback', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + mockSettingsService + ); + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: 'Output without summary' }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const saveFeatureSummary = vi.fn().mockResolvedValue(undefined); + + const options: AgentExecutionOptions = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet-4-6', + planningMode: 'skip', + previousContent: 'Previous content', + status: 'implementing', // Non-pipeline status + }; + + const callbacks = { + waitForApproval: vi.fn().mockResolvedValue({ approved: true }), + saveFeatureSummary, + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + // Should NOT save fallback for non-pipeline status + expect(saveFeatureSummary).not.toHaveBeenCalled(); + }); + + it('should correctly handle content that starts with dashes but is not scaffold', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + mockSettingsService + ); + + // Content that looks like it might have dashes but is actual content + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: '---This is a code comment or separator---' }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const saveFeatureSummary = vi.fn().mockResolvedValue(undefined); + + const options: AgentExecutionOptions = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet-4-6', + planningMode: 'skip', + previousContent: undefined, + status: 'pipeline_step1', + }; + + const callbacks = { + waitForApproval: vi.fn().mockResolvedValue({ approved: true }), + saveFeatureSummary, + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + expect(saveFeatureSummary).toHaveBeenCalled(); + const savedSummary = saveFeatureSummary.mock.calls[0][2]; + // Content should be preserved since it's not the scaffold pattern + expect(savedSummary).toContain('---This is a code comment or separator---'); + }); + + it('should handle scaffold at different positions in content', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + mockSettingsService + ); + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { + type: 'assistant', + message: { + content: [{ type: 'text', text: 'Content after scaffold marker' }], + }, + }; + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const saveFeatureSummary = vi.fn().mockResolvedValue(undefined); + + // With previousContent, scaffold will be at the start of sessionContent + const options: AgentExecutionOptions = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet-4-6', + planningMode: 'skip', + previousContent: 'Previous content', + status: 'pipeline_step1', + }; + + const callbacks = { + waitForApproval: vi.fn().mockResolvedValue({ approved: true }), + saveFeatureSummary, + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + expect(saveFeatureSummary).toHaveBeenCalled(); + const savedSummary = saveFeatureSummary.mock.calls[0][2]; + // Scaffold should be stripped, only actual content remains + expect(savedSummary).toBe('Content after scaffold marker'); + }); + }); }); diff --git a/apps/server/tests/unit/services/auto-mode-facade.test.ts b/apps/server/tests/unit/services/auto-mode-facade.test.ts new file mode 100644 index 00000000..8a2ab4cf --- /dev/null +++ b/apps/server/tests/unit/services/auto-mode-facade.test.ts @@ -0,0 +1,127 @@ +import { describe, it, expect } from 'vitest'; +import { AutoModeServiceFacade } from '@/services/auto-mode/facade.js'; +import type { Feature } from '@automaker/types'; + +describe('AutoModeServiceFacade', () => { + describe('isFeatureEligibleForAutoMode', () => { + it('should include features with pipeline_* status', () => { + const features: Partial[] = [ + { id: '1', status: 'ready', branchName: 'main' }, + { id: '2', status: 'pipeline_testing', branchName: 'main' }, + { id: '3', status: 'in_progress', branchName: 'main' }, + { id: '4', status: 'interrupted', branchName: 'main' }, + { id: '5', status: 'backlog', branchName: 'main' }, + ]; + + const branchName = 'main'; + const primaryBranch = 'main'; + + const filtered = features.filter((f) => + AutoModeServiceFacade.isFeatureEligibleForAutoMode(f as Feature, branchName, primaryBranch) + ); + + expect(filtered.map((f) => f.id)).toContain('1'); // ready + expect(filtered.map((f) => f.id)).toContain('2'); // pipeline_testing + expect(filtered.map((f) => f.id)).toContain('4'); // interrupted + expect(filtered.map((f) => f.id)).toContain('5'); // backlog + expect(filtered.map((f) => f.id)).not.toContain('3'); // in_progress + }); + + it('should correctly handle main worktree alignment', () => { + const features: Partial[] = [ + { id: '1', status: 'ready', branchName: undefined }, + { id: '2', status: 'ready', branchName: 'main' }, + { id: '3', status: 'ready', branchName: 'other' }, + ]; + + const branchName = null; // main worktree + const primaryBranch = 'main'; + + const filtered = features.filter((f) => + AutoModeServiceFacade.isFeatureEligibleForAutoMode(f as Feature, branchName, primaryBranch) + ); + + expect(filtered.map((f) => f.id)).toContain('1'); // no branch + expect(filtered.map((f) => f.id)).toContain('2'); // matching primary branch + expect(filtered.map((f) => f.id)).not.toContain('3'); // mismatching branch + }); + + it('should exclude completed, verified, and waiting_approval statuses', () => { + const features: Partial[] = [ + { id: '1', status: 'completed', branchName: 'main' }, + { id: '2', status: 'verified', branchName: 'main' }, + { id: '3', status: 'waiting_approval', branchName: 'main' }, + ]; + + const filtered = features.filter((f) => + AutoModeServiceFacade.isFeatureEligibleForAutoMode(f as Feature, 'main', 'main') + ); + + expect(filtered).toHaveLength(0); + }); + + it('should include pipeline_complete as eligible (still a pipeline status)', () => { + const feature: Partial = { + id: '1', + status: 'pipeline_complete', + branchName: 'main', + }; + + const result = AutoModeServiceFacade.isFeatureEligibleForAutoMode( + feature as Feature, + 'main', + 'main' + ); + + expect(result).toBe(true); + }); + + it('should filter pipeline features by branch in named worktrees', () => { + const features: Partial[] = [ + { id: '1', status: 'pipeline_testing', branchName: 'feature-branch' }, + { id: '2', status: 'pipeline_review', branchName: 'other-branch' }, + { id: '3', status: 'pipeline_deploy', branchName: undefined }, + ]; + + const filtered = features.filter((f) => + AutoModeServiceFacade.isFeatureEligibleForAutoMode(f as Feature, 'feature-branch', null) + ); + + expect(filtered.map((f) => f.id)).toEqual(['1']); + }); + + it('should handle null primaryBranch for main worktree', () => { + const features: Partial[] = [ + { id: '1', status: 'ready', branchName: undefined }, + { id: '2', status: 'ready', branchName: 'main' }, + ]; + + const filtered = features.filter((f) => + AutoModeServiceFacade.isFeatureEligibleForAutoMode(f as Feature, null, null) + ); + + // When primaryBranch is null, only features with no branchName are included + expect(filtered.map((f) => f.id)).toEqual(['1']); + }); + + it('should include various pipeline_* step IDs as eligible', () => { + const statuses = [ + 'pipeline_step_abc_123', + 'pipeline_code_review', + 'pipeline_step1', + 'pipeline_testing', + 'pipeline_deploy', + ]; + + for (const status of statuses) { + const feature: Partial = { id: '1', status, branchName: 'main' }; + const result = AutoModeServiceFacade.isFeatureEligibleForAutoMode( + feature as Feature, + 'main', + 'main' + ); + expect(result).toBe(true); + } + }); + }); +}); diff --git a/apps/server/tests/unit/services/execution-service.test.ts b/apps/server/tests/unit/services/execution-service.test.ts index 7c2f3e0f..8faf02cc 100644 --- a/apps/server/tests/unit/services/execution-service.test.ts +++ b/apps/server/tests/unit/services/execution-service.test.ts @@ -1439,6 +1439,114 @@ describe('execution-service.ts', () => { expect.objectContaining({ passes: true }) ); }); + + // Helper to create ExecutionService with a custom loadFeatureFn that returns + // different features on first load (initial) vs subsequent loads (after completion) + const createServiceWithCustomLoad = (completedFeature: Feature): ExecutionService => { + let loadCallCount = 0; + mockLoadFeatureFn = vi.fn().mockImplementation(() => { + loadCallCount++; + return loadCallCount === 1 ? testFeature : completedFeature; + }); + + return new ExecutionService( + mockEventBus, + mockConcurrencyManager, + mockWorktreeResolver, + mockSettingsService, + mockRunAgentFn, + mockExecutePipelineFn, + mockUpdateFeatureStatusFn, + mockLoadFeatureFn, + mockGetPlanningPromptPrefixFn, + mockSaveFeatureSummaryFn, + mockRecordLearningsFn, + mockContextExistsFn, + mockResumeFeatureFn, + mockTrackFailureFn, + mockSignalPauseFn, + mockRecordSuccessFn, + mockSaveExecutionStateFn, + mockLoadContextFilesFn + ); + }; + + it('does not overwrite accumulated summary when feature already has one', async () => { + const featureWithAccumulatedSummary: Feature = { + ...testFeature, + summary: + '### Implementation\n\nFirst step output\n\n---\n\n### Code Review\n\nReview findings', + }; + + const svc = createServiceWithCustomLoad(featureWithAccumulatedSummary); + await svc.executeFeature('/test/project', 'feature-1'); + + // saveFeatureSummaryFn should NOT be called because feature already has a summary + // This prevents overwriting accumulated pipeline summaries with just the last step's output + expect(mockSaveFeatureSummaryFn).not.toHaveBeenCalled(); + }); + + it('saves summary when feature has no existing summary', async () => { + const featureWithoutSummary: Feature = { + ...testFeature, + summary: undefined, + }; + + vi.mocked(secureFs.readFile).mockResolvedValue( + '🔧 Tool: Edit\nInput: {"file_path": "/src/index.ts"}\n\nNew summary' + ); + + const svc = createServiceWithCustomLoad(featureWithoutSummary); + await svc.executeFeature('/test/project', 'feature-1'); + + // Should save the extracted summary since feature has none + expect(mockSaveFeatureSummaryFn).toHaveBeenCalledWith( + '/test/project', + 'feature-1', + 'Test summary' + ); + }); + + it('does not overwrite summary when feature has empty string summary (treats as no summary)', async () => { + // Empty string is falsy, so it should be treated as "no summary" and a new one should be saved + const featureWithEmptySummary: Feature = { + ...testFeature, + summary: '', + }; + + vi.mocked(secureFs.readFile).mockResolvedValue( + '🔧 Tool: Edit\nInput: {"file_path": "/src/index.ts"}\n\nNew summary' + ); + + const svc = createServiceWithCustomLoad(featureWithEmptySummary); + await svc.executeFeature('/test/project', 'feature-1'); + + // Empty string is falsy, so it should save a new summary + expect(mockSaveFeatureSummaryFn).toHaveBeenCalledWith( + '/test/project', + 'feature-1', + 'Test summary' + ); + }); + + it('preserves accumulated summary when feature is transitioned from pipeline to verified', async () => { + // This is the key scenario: feature went through pipeline steps, accumulated a summary, + // then status changed to 'verified' - we should NOT overwrite the accumulated summary + const featureWithAccumulatedSummary: Feature = { + ...testFeature, + status: 'verified', + summary: + '### Implementation\n\nCreated auth module\n\n---\n\n### Code Review\n\nApproved\n\n---\n\n### Testing\n\nAll tests pass', + }; + + vi.mocked(secureFs.readFile).mockResolvedValue('Agent output with summary'); + + const svc = createServiceWithCustomLoad(featureWithAccumulatedSummary); + await svc.executeFeature('/test/project', 'feature-1'); + + // The accumulated summary should be preserved + expect(mockSaveFeatureSummaryFn).not.toHaveBeenCalled(); + }); }); describe('executeFeature - agent output validation', () => { diff --git a/apps/server/tests/unit/services/feature-state-manager.test.ts b/apps/server/tests/unit/services/feature-state-manager.test.ts index 6abd4764..cc00e1e3 100644 --- a/apps/server/tests/unit/services/feature-state-manager.test.ts +++ b/apps/server/tests/unit/services/feature-state-manager.test.ts @@ -2,12 +2,17 @@ import { describe, it, expect, beforeEach, vi, type Mock } from 'vitest'; import path from 'path'; import { FeatureStateManager } from '@/services/feature-state-manager.js'; import type { Feature } from '@automaker/types'; +import { isPipelineStatus } from '@automaker/types'; + +const PIPELINE_SUMMARY_SEPARATOR = '\n\n---\n\n'; +const PIPELINE_SUMMARY_HEADER_PREFIX = '### '; import type { EventEmitter } from '@/lib/events.js'; import type { FeatureLoader } from '@/services/feature-loader.js'; import * as secureFs from '@/lib/secure-fs.js'; import { atomicWriteJson, readJsonWithRecovery } from '@automaker/utils'; import { getFeatureDir, getFeaturesDir } from '@automaker/platform'; import { getNotificationService } from '@/services/notification-service.js'; +import { pipelineService } from '@/services/pipeline-service.js'; /** * Helper to normalize paths for cross-platform test compatibility. @@ -42,6 +47,16 @@ vi.mock('@/services/notification-service.js', () => ({ })), })); +vi.mock('@/services/pipeline-service.js', () => ({ + pipelineService: { + getStepIdFromStatus: vi.fn((status: string) => { + if (status.startsWith('pipeline_')) return status.replace('pipeline_', ''); + return null; + }), + getStep: vi.fn(), + }, +})); + describe('FeatureStateManager', () => { let manager: FeatureStateManager; let mockEvents: EventEmitter; @@ -341,9 +356,6 @@ describe('FeatureStateManager', () => { describe('markFeatureInterrupted', () => { it('should mark feature as interrupted', async () => { - (secureFs.readFile as Mock).mockResolvedValue( - JSON.stringify({ ...mockFeature, status: 'in_progress' }) - ); (readJsonWithRecovery as Mock).mockResolvedValue({ data: { ...mockFeature, status: 'in_progress' }, recovered: false, @@ -358,20 +370,25 @@ describe('FeatureStateManager', () => { }); it('should preserve pipeline_* statuses', async () => { - (secureFs.readFile as Mock).mockResolvedValue( - JSON.stringify({ ...mockFeature, status: 'pipeline_step_1' }) - ); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step_1' }, + recovered: false, + source: 'main', + }); await manager.markFeatureInterrupted('/project', 'feature-123', 'server shutdown'); // Should NOT call atomicWriteJson because pipeline status is preserved expect(atomicWriteJson).not.toHaveBeenCalled(); + expect(isPipelineStatus('pipeline_step_1')).toBe(true); }); it('should preserve pipeline_complete status', async () => { - (secureFs.readFile as Mock).mockResolvedValue( - JSON.stringify({ ...mockFeature, status: 'pipeline_complete' }) - ); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_complete' }, + recovered: false, + source: 'main', + }); await manager.markFeatureInterrupted('/project', 'feature-123'); @@ -379,7 +396,6 @@ describe('FeatureStateManager', () => { }); it('should handle feature not found', async () => { - (secureFs.readFile as Mock).mockRejectedValue(new Error('ENOENT')); (readJsonWithRecovery as Mock).mockResolvedValue({ data: null, recovered: true, @@ -439,6 +455,29 @@ describe('FeatureStateManager', () => { expect(savedFeature.status).toBe('backlog'); }); + it('should preserve pipeline_* statuses during reset', async () => { + const pipelineFeature: Feature = { + ...mockFeature, + status: 'pipeline_testing', + planSpec: { status: 'approved', version: 1, reviewedByUser: true }, + }; + + (secureFs.readdir as Mock).mockResolvedValue([ + { name: 'feature-123', isDirectory: () => true }, + ]); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: pipelineFeature, + recovered: false, + source: 'main', + }); + + await manager.resetStuckFeatures('/project'); + + // Status should NOT be changed, but needsUpdate might be true if other things reset + // In this case, nothing else should be reset, so atomicWriteJson shouldn't be called + expect(atomicWriteJson).not.toHaveBeenCalled(); + }); + it('should reset generating planSpec status to pending', async () => { const stuckFeature: Feature = { ...mockFeature, @@ -628,6 +667,379 @@ describe('FeatureStateManager', () => { expect(atomicWriteJson).not.toHaveBeenCalled(); expect(mockEvents.emit).not.toHaveBeenCalled(); }); + + it('should accumulate summary with step header for pipeline features', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Code Review', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'First step output'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Code Review\n\nFirst step output` + ); + }); + + it('should append subsequent pipeline step summaries with separator', async () => { + const existingSummary = `${PIPELINE_SUMMARY_HEADER_PREFIX}Code Review\n\nFirst step output`; + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step2', summary: existingSummary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Second step output'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Code Review\n\nFirst step output${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Testing\n\nSecond step output` + ); + }); + + it('should normalize existing non-phase summary before appending pipeline step summary', async () => { + const existingSummary = 'Implemented authentication and settings management.'; + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Code Review', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step1', summary: existingSummary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Reviewed and approved changes'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Implementation\n\nImplemented authentication and settings management.${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Code Review\n\nReviewed and approved changes` + ); + }); + + it('should use fallback step name when pipeline step not found', async () => { + (pipelineService.getStep as Mock).mockResolvedValue(null); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_unknown_step', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Step output'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Unknown Step\n\nStep output` + ); + }); + + it('should overwrite summary for non-pipeline features', async () => { + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'in_progress', summary: 'Old summary' }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'New summary'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe('New summary'); + }); + + it('should emit full accumulated summary for pipeline features', async () => { + const existingSummary = '### Code Review\n\nFirst step output'; + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Refinement', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step2', summary: existingSummary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Refinement output'); + + const expectedSummary = + '### Code Review\n\nFirst step output\n\n---\n\n### Refinement\n\nRefinement output'; + expect(mockEvents.emit).toHaveBeenCalledWith('auto-mode:event', { + type: 'auto_mode_summary', + featureId: 'feature-123', + projectPath: '/project', + summary: expectedSummary, + }); + }); + + it('should skip accumulation for pipeline features when summary is empty', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step1', summary: '' }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Test output'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + // Empty string is falsy, so should start fresh + expect(savedFeature.summary).toBe('### Testing\n\nTest output'); + }); + + it('should skip persistence when incoming summary is only whitespace', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step1', summary: '### Existing\n\nValue' }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', ' \n\t '); + + expect(atomicWriteJson).not.toHaveBeenCalled(); + expect(mockEvents.emit).not.toHaveBeenCalled(); + }); + + it('should accumulate three pipeline steps in chronological order', async () => { + // Step 1: Code Review + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Code Review', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Review findings'); + const afterStep1 = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(afterStep1.summary).toBe('### Code Review\n\nReview findings'); + + // Step 2: Testing (summary from step 1 exists) + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/feature-123'); + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step2', summary: afterStep1.summary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'All tests pass'); + const afterStep2 = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + + // Step 3: Refinement (summaries from steps 1+2 exist) + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/feature-123'); + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Refinement', id: 'step3' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step3', summary: afterStep2.summary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Code polished'); + const afterStep3 = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + + // Verify the full accumulated summary has all three steps in order + expect(afterStep3.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Code Review\n\nReview findings${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Testing\n\nAll tests pass${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Refinement\n\nCode polished` + ); + }); + + it('should replace existing step summary if called again for the same step', async () => { + const existingSummary = `${PIPELINE_SUMMARY_HEADER_PREFIX}Implementation\n\nInitial code${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Code Review\n\nFirst review attempt`; + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Code Review', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step1', summary: existingSummary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary( + '/project', + 'feature-123', + 'Second review attempt (success)' + ); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + // Should REPLACE "First review attempt" with "Second review attempt (success)" + // and NOT append it as a new section + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Implementation\n\nInitial code${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Code Review\n\nSecond review attempt (success)` + ); + // Ensure it didn't duplicate the separator or header + expect( + savedFeature.summary.match(new RegExp(PIPELINE_SUMMARY_HEADER_PREFIX + 'Code Review', 'g')) + ?.length + ).toBe(1); + expect( + savedFeature.summary.match(new RegExp(PIPELINE_SUMMARY_SEPARATOR.trim(), 'g'))?.length + ).toBe(1); + }); + + it('should replace last step summary without trailing separator', async () => { + // Test case: replacing the last step which has no separator after it + const existingSummary = `${PIPELINE_SUMMARY_HEADER_PREFIX}Implementation\n\nInitial code${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Testing\n\nFirst test run`; + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step2', summary: existingSummary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'All tests pass'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Implementation\n\nInitial code${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Testing\n\nAll tests pass` + ); + }); + + it('should replace first step summary with separator after it', async () => { + // Test case: replacing the first step which has a separator after it + const existingSummary = `${PIPELINE_SUMMARY_HEADER_PREFIX}Implementation\n\nFirst attempt${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Testing\n\nAll tests pass`; + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Implementation', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step1', summary: existingSummary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Second attempt'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Implementation\n\nSecond attempt${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Testing\n\nAll tests pass` + ); + }); + + it('should not match step header appearing in body text, only at section boundaries', async () => { + // Test case: body text contains "### Testing" which should NOT be matched + // Only headers at actual section boundaries should be replaced + const existingSummary = `${PIPELINE_SUMMARY_HEADER_PREFIX}Implementation\n\nThis step covers the Testing module.\n\n### Testing\n\nThe above is just markdown in body, not a section header.${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Testing\n\nReal test section`; + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step2', summary: existingSummary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Updated test results'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + // The section replacement should only replace the actual Testing section at the boundary + // NOT the "### Testing" that appears in the body text + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Implementation\n\nThis step covers the Testing module.\n\n### Testing\n\nThe above is just markdown in body, not a section header.${PIPELINE_SUMMARY_SEPARATOR}${PIPELINE_SUMMARY_HEADER_PREFIX}Testing\n\nUpdated test results` + ); + }); + + it('should handle step name with special regex characters safely', async () => { + // Test case: step name contains characters that would break regex + const existingSummary = `${PIPELINE_SUMMARY_HEADER_PREFIX}Code (Review)\n\nFirst attempt`; + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Code (Review)', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step1', summary: existingSummary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Second attempt'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Code (Review)\n\nSecond attempt` + ); + }); + + it('should handle step name with brackets safely', async () => { + // Test case: step name contains array-like syntax [0] + const existingSummary = `${PIPELINE_SUMMARY_HEADER_PREFIX}Step [0]\n\nFirst attempt`; + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Step [0]', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step1', summary: existingSummary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Second attempt'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Step [0]\n\nSecond attempt` + ); + }); + + it('should handle pipelineService.getStepIdFromStatus throwing an error gracefully', async () => { + (pipelineService.getStepIdFromStatus as Mock).mockImplementation(() => { + throw new Error('Config not found'); + }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_my_step', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Step output'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + // Should use fallback: capitalize each word in the status suffix + expect(savedFeature.summary).toBe(`${PIPELINE_SUMMARY_HEADER_PREFIX}My Step\n\nStep output`); + }); + + it('should handle pipelineService.getStep throwing an error gracefully', async () => { + (pipelineService.getStep as Mock).mockRejectedValue(new Error('Disk read error')); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_code_review', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Step output'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + // Should use fallback: capitalize each word in the status suffix + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Code Review\n\nStep output` + ); + }); + + it('should handle summary content with markdown formatting', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Code Review', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + const markdownSummary = + '## Changes Made\n- Fixed **bug** in `parser.ts`\n- Added `validateInput()` function\n\n```typescript\nconst x = 1;\n```'; + + await manager.saveFeatureSummary('/project', 'feature-123', markdownSummary); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe( + `${PIPELINE_SUMMARY_HEADER_PREFIX}Code Review\n\n${markdownSummary}` + ); + }); + + it('should persist before emitting event for pipeline summary accumulation', async () => { + const callOrder: string[] = []; + const existingSummary = '### Code Review\n\nFirst step output'; + + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...mockFeature, status: 'pipeline_step2', summary: existingSummary }, + recovered: false, + source: 'main', + }); + (atomicWriteJson as Mock).mockImplementation(async () => { + callOrder.push('persist'); + }); + (mockEvents.emit as Mock).mockImplementation(() => { + callOrder.push('emit'); + }); + + await manager.saveFeatureSummary('/project', 'feature-123', 'Test results'); + + expect(callOrder).toEqual(['persist', 'emit']); + }); }); describe('updateTaskStatus', () => { @@ -668,6 +1080,48 @@ describe('FeatureStateManager', () => { }); }); + it('should update task status and summary and emit event', async () => { + const featureWithTasks: Feature = { + ...mockFeature, + planSpec: { + status: 'approved', + version: 1, + reviewedByUser: true, + tasks: [{ id: 'task-1', title: 'Task 1', status: 'pending', description: '' }], + }, + }; + + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: featureWithTasks, + recovered: false, + source: 'main', + }); + + await manager.updateTaskStatus( + '/project', + 'feature-123', + 'task-1', + 'completed', + 'Task finished successfully' + ); + + // Verify persisted + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.planSpec?.tasks?.[0].status).toBe('completed'); + expect(savedFeature.planSpec?.tasks?.[0].summary).toBe('Task finished successfully'); + + // Verify event emitted + expect(mockEvents.emit).toHaveBeenCalledWith('auto-mode:event', { + type: 'auto_mode_task_status', + featureId: 'feature-123', + projectPath: '/project', + taskId: 'task-1', + status: 'completed', + summary: 'Task finished successfully', + tasks: expect.any(Array), + }); + }); + it('should handle task not found', async () => { const featureWithTasks: Feature = { ...mockFeature, diff --git a/apps/server/tests/unit/services/pipeline-orchestrator-prompts.test.ts b/apps/server/tests/unit/services/pipeline-orchestrator-prompts.test.ts new file mode 100644 index 00000000..ba847bd0 --- /dev/null +++ b/apps/server/tests/unit/services/pipeline-orchestrator-prompts.test.ts @@ -0,0 +1,57 @@ +import { describe, it, expect, vi } from 'vitest'; +import { PipelineOrchestrator } from '../../../src/services/pipeline-orchestrator.js'; +import type { Feature } from '@automaker/types'; + +describe('PipelineOrchestrator Prompts', () => { + const mockFeature: Feature = { + id: 'feature-123', + title: 'Test Feature', + description: 'A test feature', + status: 'in_progress', + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + tasks: [], + }; + + const mockBuildFeaturePrompt = (feature: Feature) => `Feature: ${feature.title}`; + + it('should include mandatory summary requirement in pipeline step prompt', () => { + const orchestrator = new PipelineOrchestrator( + null as any, // eventBus + null as any, // featureStateManager + null as any, // agentExecutor + null as any, // testRunnerService + null as any, // worktreeResolver + null as any, // concurrencyManager + null as any, // settingsService + null as any, // updateFeatureStatusFn + null as any, // loadContextFilesFn + mockBuildFeaturePrompt, + null as any, // executeFeatureFn + null as any // runAgentFn + ); + + const step = { + id: 'step1', + name: 'Code Review', + instructions: 'Review the code for quality.', + }; + + const prompt = orchestrator.buildPipelineStepPrompt( + step as any, + mockFeature, + 'Previous work context', + { implementationInstructions: '', playwrightVerificationInstructions: '' } + ); + + expect(prompt).toContain('## Pipeline Step: Code Review'); + expect(prompt).toContain('Review the code for quality.'); + expect(prompt).toContain( + '**CRITICAL: After completing the instructions, you MUST output a summary using this EXACT format:**' + ); + expect(prompt).toContain(''); + expect(prompt).toContain('## Summary: Code Review'); + expect(prompt).toContain(''); + expect(prompt).toContain('The and tags MUST be on their own lines.'); + }); +}); diff --git a/apps/server/tests/unit/services/pipeline-summary-accumulation.test.ts b/apps/server/tests/unit/services/pipeline-summary-accumulation.test.ts new file mode 100644 index 00000000..23668a8b --- /dev/null +++ b/apps/server/tests/unit/services/pipeline-summary-accumulation.test.ts @@ -0,0 +1,598 @@ +/** + * Integration tests for pipeline summary accumulation across multiple steps. + * + * These tests verify the end-to-end behavior where: + * 1. Each pipeline step produces a summary via agent-executor → callbacks.saveFeatureSummary() + * 2. FeatureStateManager.saveFeatureSummary() accumulates summaries with step headers + * 3. The emitted auto_mode_summary event contains the full accumulated summary + * 4. The UI can use feature.summary (accumulated) instead of extractSummary() (last-only) + */ + +import { describe, it, expect, beforeEach, vi, type Mock } from 'vitest'; +import { FeatureStateManager } from '@/services/feature-state-manager.js'; +import type { Feature } from '@automaker/types'; +import type { EventEmitter } from '@/lib/events.js'; +import type { FeatureLoader } from '@/services/feature-loader.js'; +import { atomicWriteJson, readJsonWithRecovery } from '@automaker/utils'; +import { getFeatureDir } from '@automaker/platform'; +import { pipelineService } from '@/services/pipeline-service.js'; + +// Mock dependencies +vi.mock('@/lib/secure-fs.js', () => ({ + readFile: vi.fn(), + readdir: vi.fn(), +})); + +vi.mock('@automaker/utils', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + atomicWriteJson: vi.fn(), + readJsonWithRecovery: vi.fn(), + logRecoveryWarning: vi.fn(), + }; +}); + +vi.mock('@automaker/platform', () => ({ + getFeatureDir: vi.fn(), + getFeaturesDir: vi.fn(), +})); + +vi.mock('@/services/notification-service.js', () => ({ + getNotificationService: vi.fn(() => ({ + createNotification: vi.fn(), + })), +})); + +vi.mock('@/services/pipeline-service.js', () => ({ + pipelineService: { + getStepIdFromStatus: vi.fn((status: string) => { + if (status.startsWith('pipeline_')) return status.replace('pipeline_', ''); + return null; + }), + getStep: vi.fn(), + }, +})); + +describe('Pipeline Summary Accumulation (Integration)', () => { + let manager: FeatureStateManager; + let mockEvents: EventEmitter; + + const baseFeature: Feature = { + id: 'pipeline-feature-1', + name: 'Pipeline Feature', + title: 'Pipeline Feature Title', + description: 'A feature going through pipeline steps', + status: 'pipeline_step1', + createdAt: '2024-01-01T00:00:00Z', + updatedAt: '2024-01-01T00:00:00Z', + }; + + beforeEach(() => { + vi.clearAllMocks(); + + mockEvents = { + emit: vi.fn(), + subscribe: vi.fn(() => vi.fn()), + }; + + const mockFeatureLoader = { + syncFeatureToAppSpec: vi.fn(), + } as unknown as FeatureLoader; + + manager = new FeatureStateManager(mockEvents, mockFeatureLoader); + + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/pipeline-feature-1'); + }); + + describe('multi-step pipeline summary accumulation', () => { + it('should accumulate summaries across three pipeline steps in chronological order', async () => { + // --- Step 1: Implementation --- + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Implementation', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary( + '/project', + 'pipeline-feature-1', + '## Changes\n- Added auth module\n- Created user service' + ); + + const step1Feature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(step1Feature.summary).toBe( + '### Implementation\n\n## Changes\n- Added auth module\n- Created user service' + ); + + // --- Step 2: Code Review --- + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/pipeline-feature-1'); + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Code Review', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step2', summary: step1Feature.summary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary( + '/project', + 'pipeline-feature-1', + '## Review Findings\n- Style issues fixed\n- Added error handling' + ); + + const step2Feature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + + // --- Step 3: Testing --- + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/pipeline-feature-1'); + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step3' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step3', summary: step2Feature.summary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary( + '/project', + 'pipeline-feature-1', + '## Test Results\n- 42 tests pass\n- 98% coverage' + ); + + const finalFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + + // Verify the full accumulated summary has all three steps separated by --- + const expectedSummary = [ + '### Implementation', + '', + '## Changes', + '- Added auth module', + '- Created user service', + '', + '---', + '', + '### Code Review', + '', + '## Review Findings', + '- Style issues fixed', + '- Added error handling', + '', + '---', + '', + '### Testing', + '', + '## Test Results', + '- 42 tests pass', + '- 98% coverage', + ].join('\n'); + + expect(finalFeature.summary).toBe(expectedSummary); + }); + + it('should emit the full accumulated summary in auto_mode_summary event', async () => { + // Step 1 + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Implementation', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'Step 1 output'); + + // Verify the event was emitted with correct data + expect(mockEvents.emit).toHaveBeenCalledWith('auto-mode:event', { + type: 'auto_mode_summary', + featureId: 'pipeline-feature-1', + projectPath: '/project', + summary: '### Implementation\n\nStep 1 output', + }); + + // Step 2 (with accumulated summary from step 1) + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/pipeline-feature-1'); + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { + ...baseFeature, + status: 'pipeline_step2', + summary: '### Implementation\n\nStep 1 output', + }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'Step 2 output'); + + // The event should contain the FULL accumulated summary, not just step 2 + expect(mockEvents.emit).toHaveBeenCalledWith('auto-mode:event', { + type: 'auto_mode_summary', + featureId: 'pipeline-feature-1', + projectPath: '/project', + summary: '### Implementation\n\nStep 1 output\n\n---\n\n### Testing\n\nStep 2 output', + }); + }); + }); + + describe('edge cases in pipeline accumulation', () => { + it('should normalize a legacy implementation summary before appending pipeline output', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Code Review', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { + ...baseFeature, + status: 'pipeline_step2', + summary: 'Implemented authentication and settings updates.', + }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'Reviewed and approved'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe( + '### Implementation\n\nImplemented authentication and settings updates.\n\n---\n\n### Code Review\n\nReviewed and approved' + ); + }); + + it('should skip persistence when a pipeline step summary is empty', async () => { + const existingSummary = '### Step 1\n\nFirst step output'; + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Step 2', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step2', summary: existingSummary }, + recovered: false, + source: 'main', + }); + + // Empty summary should be ignored to avoid persisting blank sections. + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', ''); + + expect(atomicWriteJson).not.toHaveBeenCalled(); + expect(mockEvents.emit).not.toHaveBeenCalled(); + }); + + it('should handle pipeline step name lookup failure with fallback', async () => { + (pipelineService.getStepIdFromStatus as Mock).mockImplementation(() => { + throw new Error('Pipeline config not loaded'); + }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_code_review', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'Review output'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + // Fallback: capitalize words from status suffix + expect(savedFeature.summary).toBe('### Code Review\n\nReview output'); + }); + + it('should handle summary with special markdown characters in pipeline mode', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Implementation', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + const markdownSummary = [ + '## Changes Made', + '- Fixed **critical bug** in `parser.ts`', + '- Added `validateInput()` function', + '', + '```typescript', + 'const x = 1;', + '```', + '', + '| Column | Value |', + '|--------|-------|', + '| Tests | Pass |', + ].join('\n'); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', markdownSummary); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe(`### Implementation\n\n${markdownSummary}`); + // Verify markdown is preserved + expect(savedFeature.summary).toContain('```typescript'); + expect(savedFeature.summary).toContain('| Column | Value |'); + }); + + it('should correctly handle rapid sequential pipeline steps without data loss', async () => { + // Simulate 5 rapid pipeline steps + const stepConfigs = [ + { name: 'Planning', status: 'pipeline_step1', content: 'Plan created' }, + { name: 'Implementation', status: 'pipeline_step2', content: 'Code written' }, + { name: 'Code Review', status: 'pipeline_step3', content: 'Review complete' }, + { name: 'Testing', status: 'pipeline_step4', content: 'All tests pass' }, + { name: 'Refinement', status: 'pipeline_step5', content: 'Code polished' }, + ]; + + let currentSummary: string | undefined = undefined; + + for (const step of stepConfigs) { + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/pipeline-feature-1'); + (pipelineService.getStep as Mock).mockResolvedValue({ + name: step.name, + id: step.status.replace('pipeline_', ''), + }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: step.status, summary: currentSummary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', step.content); + + currentSummary = ((atomicWriteJson as Mock).mock.calls[0][1] as Feature).summary; + } + + // Final summary should contain all 5 steps + expect(currentSummary).toContain('### Planning'); + expect(currentSummary).toContain('Plan created'); + expect(currentSummary).toContain('### Implementation'); + expect(currentSummary).toContain('Code written'); + expect(currentSummary).toContain('### Code Review'); + expect(currentSummary).toContain('Review complete'); + expect(currentSummary).toContain('### Testing'); + expect(currentSummary).toContain('All tests pass'); + expect(currentSummary).toContain('### Refinement'); + expect(currentSummary).toContain('Code polished'); + + // Verify there are exactly 4 separators (between 5 steps) + const separatorCount = (currentSummary!.match(/\n\n---\n\n/g) || []).length; + expect(separatorCount).toBe(4); + }); + }); + + describe('UI summary display logic', () => { + it('should emit accumulated summary that UI can display directly (no extractSummary needed)', async () => { + // This test verifies the UI can use feature.summary directly + // without needing to call extractSummary() which only returns the last entry + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Implementation', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'First step'); + + const step1Summary = ((atomicWriteJson as Mock).mock.calls[0][1] as Feature).summary; + + // Step 2 + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/pipeline-feature-1'); + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step2', summary: step1Summary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'Second step'); + + const emittedEvent = (mockEvents.emit as Mock).mock.calls[0][1]; + const accumulatedSummary = emittedEvent.summary; + + // The accumulated summary should contain BOTH steps + expect(accumulatedSummary).toContain('### Implementation'); + expect(accumulatedSummary).toContain('First step'); + expect(accumulatedSummary).toContain('### Testing'); + expect(accumulatedSummary).toContain('Second step'); + }); + + it('should handle single-step pipeline (no accumulation needed)', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Implementation', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'Single step output'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe('### Implementation\n\nSingle step output'); + + // No separator should be present for single step + expect(savedFeature.summary).not.toContain('---'); + }); + + it('should preserve chronological order of summaries', async () => { + // Step 1 + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Alpha', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'First'); + + const step1Summary = ((atomicWriteJson as Mock).mock.calls[0][1] as Feature).summary; + + // Step 2 + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/pipeline-feature-1'); + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Beta', id: 'step2' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step2', summary: step1Summary }, + recovered: false, + source: 'main', + }); + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'Second'); + + const finalSummary = ((atomicWriteJson as Mock).mock.calls[0][1] as Feature).summary; + + // Verify order: Alpha should come before Beta + const alphaIndex = finalSummary!.indexOf('### Alpha'); + const betaIndex = finalSummary!.indexOf('### Beta'); + expect(alphaIndex).toBeLessThan(betaIndex); + }); + }); + + describe('non-pipeline features', () => { + it('should overwrite summary for non-pipeline features', async () => { + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { + ...baseFeature, + status: 'in_progress', // Non-pipeline status + summary: 'Old summary', + }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'New summary'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe('New summary'); + }); + + it('should not add step headers for non-pipeline features', async () => { + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { + ...baseFeature, + status: 'in_progress', // Non-pipeline status + summary: undefined, + }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'Simple summary'); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toBe('Simple summary'); + expect(savedFeature.summary).not.toContain('###'); + }); + }); + + describe('summary content edge cases', () => { + it('should handle summary with unicode characters', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + const unicodeSummary = 'Test results: ✅ 42 passed, ❌ 0 failed, 🎉 100% coverage'; + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', unicodeSummary); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toContain('✅'); + expect(savedFeature.summary).toContain('❌'); + expect(savedFeature.summary).toContain('🎉'); + }); + + it('should handle very long summary content', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Implementation', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + // Generate a very long summary (10KB+) + const longContent = 'This is a line of content.\n'.repeat(500); + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', longContent); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary!.length).toBeGreaterThan(10000); + }); + + it('should handle summary with markdown tables', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + const tableSummary = ` +## Test Results + +| Test Suite | Passed | Failed | Skipped | +|------------|--------|--------|---------| +| Unit | 42 | 0 | 2 | +| Integration| 15 | 0 | 0 | +| E2E | 8 | 1 | 0 | +`; + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', tableSummary); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toContain('| Test Suite |'); + expect(savedFeature.summary).toContain('| Unit | 42 |'); + }); + + it('should handle summary with nested markdown headers', async () => { + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Implementation', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + + const nestedSummary = ` +## Main Changes +### Backend +- Added API endpoints +### Frontend +- Created components +#### Deep nesting +- Minor fix +`; + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', nestedSummary); + + const savedFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + expect(savedFeature.summary).toContain('### Backend'); + expect(savedFeature.summary).toContain('### Frontend'); + expect(savedFeature.summary).toContain('#### Deep nesting'); + }); + }); + + describe('persistence and event ordering', () => { + it('should persist summary BEFORE emitting event', async () => { + const callOrder: string[] = []; + + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + (atomicWriteJson as Mock).mockImplementation(async () => { + callOrder.push('persist'); + }); + (mockEvents.emit as Mock).mockImplementation(() => { + callOrder.push('emit'); + }); + + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'Summary'); + + expect(callOrder).toEqual(['persist', 'emit']); + }); + + it('should not emit event if persistence fails (error is caught silently)', async () => { + // Note: saveFeatureSummary catches errors internally and logs them + // It does NOT re-throw, so the method completes successfully even on error + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'step1' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_step1', summary: undefined }, + recovered: false, + source: 'main', + }); + (atomicWriteJson as Mock).mockRejectedValue(new Error('Disk full')); + + // Method completes without throwing (error is logged internally) + await manager.saveFeatureSummary('/project', 'pipeline-feature-1', 'Summary'); + + // Event should NOT be emitted since persistence failed + expect(mockEvents.emit).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/apps/server/tests/unit/services/spec-parser.test.ts b/apps/server/tests/unit/services/spec-parser.test.ts index e917622c..411c9290 100644 --- a/apps/server/tests/unit/services/spec-parser.test.ts +++ b/apps/server/tests/unit/services/spec-parser.test.ts @@ -207,12 +207,21 @@ Let me begin by... describe('detectTaskCompleteMarker', () => { it('should detect task complete marker and return task ID', () => { - expect(detectTaskCompleteMarker('[TASK_COMPLETE] T001')).toBe('T001'); - expect(detectTaskCompleteMarker('[TASK_COMPLETE] T042')).toBe('T042'); + expect(detectTaskCompleteMarker('[TASK_COMPLETE] T001')).toEqual({ + id: 'T001', + summary: undefined, + }); + expect(detectTaskCompleteMarker('[TASK_COMPLETE] T042')).toEqual({ + id: 'T042', + summary: undefined, + }); }); it('should handle marker with summary', () => { - expect(detectTaskCompleteMarker('[TASK_COMPLETE] T001: User model created')).toBe('T001'); + expect(detectTaskCompleteMarker('[TASK_COMPLETE] T001: User model created')).toEqual({ + id: 'T001', + summary: 'User model created', + }); }); it('should return null when no marker present', () => { @@ -229,7 +238,28 @@ Done with the implementation: Moving on to... `; - expect(detectTaskCompleteMarker(accumulated)).toBe('T003'); + expect(detectTaskCompleteMarker(accumulated)).toEqual({ + id: 'T003', + summary: 'Database setup complete', + }); + }); + + it('should find marker in the middle of a stream with trailing text', () => { + const streamText = + 'The implementation is complete! [TASK_COMPLETE] T001: Added user model and tests. Now let me check the next task...'; + expect(detectTaskCompleteMarker(streamText)).toEqual({ + id: 'T001', + summary: 'Added user model and tests. Now let me check the next task...', + }); + }); + + it('should find marker in the middle of a stream with multiple tasks and return the FIRST match', () => { + const streamText = + '[TASK_COMPLETE] T001: Task one done. Continuing... [TASK_COMPLETE] T002: Task two done. Moving on...'; + expect(detectTaskCompleteMarker(streamText)).toEqual({ + id: 'T001', + summary: 'Task one done. Continuing...', + }); }); it('should not confuse with TASK_START marker', () => { @@ -240,6 +270,44 @@ Moving on to... expect(detectTaskCompleteMarker('[TASK_COMPLETE] TASK1')).toBeNull(); expect(detectTaskCompleteMarker('[TASK_COMPLETE] T1')).toBeNull(); }); + + it('should allow brackets in summary text', () => { + // Regression test: summaries containing array[index] syntax should not be truncated + expect( + detectTaskCompleteMarker('[TASK_COMPLETE] T001: Supports array[index] access syntax') + ).toEqual({ + id: 'T001', + summary: 'Supports array[index] access syntax', + }); + }); + + it('should handle summary with multiple brackets', () => { + expect( + detectTaskCompleteMarker('[TASK_COMPLETE] T042: Fixed bug in data[0].items[key] mapping') + ).toEqual({ + id: 'T042', + summary: 'Fixed bug in data[0].items[key] mapping', + }); + }); + + it('should stop at newline in summary', () => { + const result = detectTaskCompleteMarker( + '[TASK_COMPLETE] T001: First line\nSecond line without marker' + ); + expect(result).toEqual({ + id: 'T001', + summary: 'First line', + }); + }); + + it('should stop at next TASK_START marker', () => { + expect( + detectTaskCompleteMarker('[TASK_COMPLETE] T001: Summary text[TASK_START] T002') + ).toEqual({ + id: 'T001', + summary: 'Summary text', + }); + }); }); describe('detectPhaseCompleteMarker', () => { @@ -637,5 +705,85 @@ Second paragraph of summary. expect(extractSummary(text)).toBe('First paragraph of summary.'); }); }); + + describe('pipeline accumulated output (multiple tags)', () => { + it('should return only the LAST summary tag from accumulated pipeline output', () => { + // Documents WHY the UI needs server-side feature.summary: + // When pipeline steps accumulate raw output in agent-output.md, each step + // writes its own tag. extractSummary takes only the LAST match, + // losing all previous steps' summaries. + const accumulatedOutput = ` +## Step 1: Code Review + +Some review output... + + +## Code Review Summary +- Found 3 issues +- Suggested 2 improvements + + +--- + +## Follow-up Session + +## Step 2: Testing + +Running tests... + + +## Testing Summary +- All 15 tests pass +- Coverage at 92% + +`; + const result = extractSummary(accumulatedOutput); + // Only the LAST summary tag is returned - the Code Review summary is lost + expect(result).toBe('## Testing Summary\n- All 15 tests pass\n- Coverage at 92%'); + expect(result).not.toContain('Code Review'); + }); + + it('should return only the LAST summary from three pipeline steps', () => { + const accumulatedOutput = ` +Step 1: Implementation complete + +--- + +## Follow-up Session + +Step 2: Code review findings + +--- + +## Follow-up Session + +Step 3: All tests passing +`; + const result = extractSummary(accumulatedOutput); + expect(result).toBe('Step 3: All tests passing'); + expect(result).not.toContain('Step 1'); + expect(result).not.toContain('Step 2'); + }); + + it('should handle accumulated output where only one step has a summary tag', () => { + const accumulatedOutput = ` +## Step 1: Implementation +Some raw output without summary tags... + +--- + +## Follow-up Session + +## Step 2: Testing + + +## Test Results +- All tests pass + +`; + const result = extractSummary(accumulatedOutput); + expect(result).toBe('## Test Results\n- All tests pass'); + }); + }); }); }); diff --git a/apps/server/tests/unit/types/pipeline-types.test.ts b/apps/server/tests/unit/types/pipeline-types.test.ts new file mode 100644 index 00000000..d978cc4f --- /dev/null +++ b/apps/server/tests/unit/types/pipeline-types.test.ts @@ -0,0 +1,48 @@ +import { describe, it, expect } from 'vitest'; +import { isPipelineStatus } from '@automaker/types'; + +describe('isPipelineStatus', () => { + it('should return true for valid pipeline statuses', () => { + expect(isPipelineStatus('pipeline_step1')).toBe(true); + expect(isPipelineStatus('pipeline_testing')).toBe(true); + expect(isPipelineStatus('pipeline_code_review')).toBe(true); + expect(isPipelineStatus('pipeline_complete')).toBe(true); + }); + + it('should return true for pipeline_ prefix with any non-empty suffix', () => { + expect(isPipelineStatus('pipeline_')).toBe(false); // Empty suffix is invalid + expect(isPipelineStatus('pipeline_123')).toBe(true); + expect(isPipelineStatus('pipeline_step_abc_123')).toBe(true); + }); + + it('should return false for non-pipeline statuses', () => { + expect(isPipelineStatus('in_progress')).toBe(false); + expect(isPipelineStatus('backlog')).toBe(false); + expect(isPipelineStatus('ready')).toBe(false); + expect(isPipelineStatus('interrupted')).toBe(false); + expect(isPipelineStatus('waiting_approval')).toBe(false); + expect(isPipelineStatus('verified')).toBe(false); + expect(isPipelineStatus('completed')).toBe(false); + }); + + it('should return false for null and undefined', () => { + expect(isPipelineStatus(null)).toBe(false); + expect(isPipelineStatus(undefined)).toBe(false); + }); + + it('should return false for empty string', () => { + expect(isPipelineStatus('')).toBe(false); + }); + + it('should return false for partial matches', () => { + expect(isPipelineStatus('pipeline')).toBe(false); + expect(isPipelineStatus('pipelin_step1')).toBe(false); + expect(isPipelineStatus('Pipeline_step1')).toBe(false); + expect(isPipelineStatus('PIPELINE_step1')).toBe(false); + }); + + it('should return false for pipeline prefix embedded in longer string', () => { + expect(isPipelineStatus('not_pipeline_step1')).toBe(false); + expect(isPipelineStatus('my_pipeline_step')).toBe(false); + }); +}); diff --git a/apps/server/tests/unit/ui/agent-output-summary-e2e.test.ts b/apps/server/tests/unit/ui/agent-output-summary-e2e.test.ts new file mode 100644 index 00000000..67187c16 --- /dev/null +++ b/apps/server/tests/unit/ui/agent-output-summary-e2e.test.ts @@ -0,0 +1,563 @@ +/** + * End-to-end integration tests for agent output summary display flow. + * + * These tests validate the complete flow from: + * 1. Server-side summary accumulation (FeatureStateManager.saveFeatureSummary) + * 2. Event emission with accumulated summary (auto_mode_summary event) + * 3. UI-side summary retrieval (feature.summary via API) + * 4. UI-side summary parsing and display (parsePhaseSummaries, extractSummary) + * + * The tests simulate what happens when: + * - A feature goes through multiple pipeline steps + * - Each step produces a summary + * - The server accumulates all summaries + * - The UI displays the accumulated summary + */ + +import { describe, it, expect, beforeEach, vi, type Mock } from 'vitest'; +import { FeatureStateManager } from '@/services/feature-state-manager.js'; +import type { Feature } from '@automaker/types'; +import type { EventEmitter } from '@/lib/events.js'; +import type { FeatureLoader } from '@/services/feature-loader.js'; +import { atomicWriteJson, readJsonWithRecovery } from '@automaker/utils'; +import { getFeatureDir } from '@automaker/platform'; +import { pipelineService } from '@/services/pipeline-service.js'; + +// Mock dependencies +vi.mock('@/lib/secure-fs.js', () => ({ + readFile: vi.fn(), + readdir: vi.fn(), +})); + +vi.mock('@automaker/utils', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + atomicWriteJson: vi.fn(), + readJsonWithRecovery: vi.fn(), + logRecoveryWarning: vi.fn(), + }; +}); + +vi.mock('@automaker/platform', () => ({ + getFeatureDir: vi.fn(), + getFeaturesDir: vi.fn(), +})); + +vi.mock('@/services/notification-service.js', () => ({ + getNotificationService: vi.fn(() => ({ + createNotification: vi.fn(), + })), +})); + +vi.mock('@/services/pipeline-service.js', () => ({ + pipelineService: { + getStepIdFromStatus: vi.fn((status: string) => { + if (status.startsWith('pipeline_')) return status.replace('pipeline_', ''); + return null; + }), + getStep: vi.fn(), + }, +})); + +// ============================================================================ +// UI-side parsing functions (mirrored from apps/ui/src/lib/log-parser.ts) +// ============================================================================ + +function parsePhaseSummaries(summary: string | undefined): Map { + const phaseSummaries = new Map(); + if (!summary || !summary.trim()) return phaseSummaries; + + const sections = summary.split(/\n\n---\n\n/); + for (const section of sections) { + const headerMatch = section.match(/^###\s+(.+?)(?:\n|$)/); + if (headerMatch) { + const phaseName = headerMatch[1].trim().toLowerCase(); + const content = section.substring(headerMatch[0].length).trim(); + phaseSummaries.set(phaseName, content); + } + } + return phaseSummaries; +} + +function extractSummary(rawOutput: string): string | null { + if (!rawOutput || !rawOutput.trim()) return null; + + const regexesToTry: Array<{ + regex: RegExp; + processor: (m: RegExpMatchArray) => string; + }> = [ + { regex: /([\s\S]*?)<\/summary>/gi, processor: (m) => m[1] }, + { regex: /^##\s+Summary[^\n]*\n([\s\S]*?)(?=\n##\s+[^#]|\n🔧|$)/gm, processor: (m) => m[1] }, + ]; + + for (const { regex, processor } of regexesToTry) { + const matches = [...rawOutput.matchAll(regex)]; + if (matches.length > 0) { + const lastMatch = matches[matches.length - 1]; + return processor(lastMatch).trim(); + } + } + return null; +} + +function isAccumulatedSummary(summary: string | undefined): boolean { + if (!summary || !summary.trim()) return false; + return summary.includes('\n\n---\n\n') && (summary.match(/###\s+.+/g)?.length ?? 0) > 0; +} + +/** + * Returns the first summary candidate that contains non-whitespace content. + * Mirrors getFirstNonEmptySummary from apps/ui/src/lib/summary-selection.ts + */ +function getFirstNonEmptySummary(...candidates: (string | null | undefined)[]): string | null { + for (const candidate of candidates) { + if (typeof candidate === 'string' && candidate.trim().length > 0) { + return candidate; + } + } + return null; +} + +// ============================================================================ +// Unit tests for helper functions +// ============================================================================ + +describe('getFirstNonEmptySummary', () => { + it('should return the first non-empty string', () => { + expect(getFirstNonEmptySummary(null, undefined, 'first', 'second')).toBe('first'); + }); + + it('should skip null and undefined candidates', () => { + expect(getFirstNonEmptySummary(null, undefined, 'valid')).toBe('valid'); + }); + + it('should skip whitespace-only strings', () => { + expect(getFirstNonEmptySummary(' ', '\n\t', 'actual content')).toBe('actual content'); + }); + + it('should return null when all candidates are empty', () => { + expect(getFirstNonEmptySummary(null, undefined, '', ' ')).toBeNull(); + }); + + it('should return null when no candidates provided', () => { + expect(getFirstNonEmptySummary()).toBeNull(); + }); + + it('should handle empty string as invalid', () => { + expect(getFirstNonEmptySummary('', 'valid')).toBe('valid'); + }); + + it('should prefer first valid candidate', () => { + expect(getFirstNonEmptySummary('first', 'second', 'third')).toBe('first'); + }); + + it('should handle strings with only spaces as invalid', () => { + expect(getFirstNonEmptySummary(' ', ' \n ', 'valid')).toBe('valid'); + }); + + it('should accept strings with content surrounded by whitespace', () => { + expect(getFirstNonEmptySummary(' content with spaces ')).toBe(' content with spaces '); + }); +}); + +describe('Agent Output Summary E2E Flow', () => { + let manager: FeatureStateManager; + let mockEvents: EventEmitter; + + const baseFeature: Feature = { + id: 'e2e-feature-1', + name: 'E2E Feature', + title: 'E2E Feature Title', + description: 'A feature going through complete pipeline', + status: 'pipeline_implementation', + createdAt: '2024-01-01T00:00:00Z', + updatedAt: '2024-01-01T00:00:00Z', + }; + + beforeEach(() => { + vi.clearAllMocks(); + + mockEvents = { + emit: vi.fn(), + subscribe: vi.fn(() => vi.fn()), + }; + + const mockFeatureLoader = { + syncFeatureToAppSpec: vi.fn(), + } as unknown as FeatureLoader; + + manager = new FeatureStateManager(mockEvents, mockFeatureLoader); + + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/e2e-feature-1'); + }); + + describe('complete pipeline flow: server accumulation → UI display', () => { + it('should maintain complete summary across all pipeline steps', async () => { + // ===== STEP 1: Implementation ===== + (pipelineService.getStep as Mock).mockResolvedValue({ + name: 'Implementation', + id: 'implementation', + }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_implementation', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary( + '/project', + 'e2e-feature-1', + '## Changes\n- Created auth module\n- Added user service' + ); + + const step1Feature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + const step1Summary = step1Feature.summary; + + // Verify server-side accumulation format + expect(step1Summary).toBe( + '### Implementation\n\n## Changes\n- Created auth module\n- Added user service' + ); + + // Verify UI can parse this summary + const phases1 = parsePhaseSummaries(step1Summary); + expect(phases1.size).toBe(1); + expect(phases1.get('implementation')).toContain('Created auth module'); + + // ===== STEP 2: Code Review ===== + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/e2e-feature-1'); + (pipelineService.getStep as Mock).mockResolvedValue({ + name: 'Code Review', + id: 'code_review', + }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_code_review', summary: step1Summary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary( + '/project', + 'e2e-feature-1', + '## Review Results\n- Approved with minor suggestions' + ); + + const step2Feature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + const step2Summary = step2Feature.summary; + + // Verify accumulation now has both steps + expect(step2Summary).toContain('### Implementation'); + expect(step2Summary).toContain('Created auth module'); + expect(step2Summary).toContain('### Code Review'); + expect(step2Summary).toContain('Approved with minor suggestions'); + expect(step2Summary).toContain('\n\n---\n\n'); // Separator + + // Verify UI can parse accumulated summary + expect(isAccumulatedSummary(step2Summary)).toBe(true); + const phases2 = parsePhaseSummaries(step2Summary); + expect(phases2.size).toBe(2); + expect(phases2.get('implementation')).toContain('Created auth module'); + expect(phases2.get('code review')).toContain('Approved with minor suggestions'); + + // ===== STEP 3: Testing ===== + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/e2e-feature-1'); + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'testing' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_testing', summary: step2Summary }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary( + '/project', + 'e2e-feature-1', + '## Test Results\n- 42 tests pass\n- 98% coverage' + ); + + const finalFeature = (atomicWriteJson as Mock).mock.calls[0][1] as Feature; + const finalSummary = finalFeature.summary; + + // Verify final accumulation has all three steps + expect(finalSummary).toContain('### Implementation'); + expect(finalSummary).toContain('Created auth module'); + expect(finalSummary).toContain('### Code Review'); + expect(finalSummary).toContain('Approved with minor suggestions'); + expect(finalSummary).toContain('### Testing'); + expect(finalSummary).toContain('42 tests pass'); + + // Verify UI-side parsing of complete pipeline + expect(isAccumulatedSummary(finalSummary)).toBe(true); + const finalPhases = parsePhaseSummaries(finalSummary); + expect(finalPhases.size).toBe(3); + + // Verify chronological order (implementation before testing) + const summaryLines = finalSummary!.split('\n'); + const implIndex = summaryLines.findIndex((l) => l.includes('### Implementation')); + const reviewIndex = summaryLines.findIndex((l) => l.includes('### Code Review')); + const testIndex = summaryLines.findIndex((l) => l.includes('### Testing')); + expect(implIndex).toBeLessThan(reviewIndex); + expect(reviewIndex).toBeLessThan(testIndex); + }); + + it('should emit events with accumulated summaries for real-time UI updates', async () => { + // Step 1 + (pipelineService.getStep as Mock).mockResolvedValue({ + name: 'Implementation', + id: 'implementation', + }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_implementation', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'e2e-feature-1', 'Step 1 output'); + + // Verify event emission + expect(mockEvents.emit).toHaveBeenCalledWith('auto-mode:event', { + type: 'auto_mode_summary', + featureId: 'e2e-feature-1', + projectPath: '/project', + summary: '### Implementation\n\nStep 1 output', + }); + + // Step 2 + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/e2e-feature-1'); + (pipelineService.getStep as Mock).mockResolvedValue({ name: 'Testing', id: 'testing' }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { + ...baseFeature, + status: 'pipeline_testing', + summary: '### Implementation\n\nStep 1 output', + }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'e2e-feature-1', 'Step 2 output'); + + // Event should contain FULL accumulated summary + expect(mockEvents.emit).toHaveBeenCalledWith('auto-mode:event', { + type: 'auto_mode_summary', + featureId: 'e2e-feature-1', + projectPath: '/project', + summary: '### Implementation\n\nStep 1 output\n\n---\n\n### Testing\n\nStep 2 output', + }); + }); + }); + + describe('UI display logic: feature.summary vs extractSummary()', () => { + it('should prefer feature.summary (server-accumulated) over extractSummary() (last only)', () => { + // Simulate what the server has accumulated + const featureSummary = [ + '### Implementation', + '', + '## Changes', + '- Created feature', + '', + '---', + '', + '### Testing', + '', + '## Results', + '- All tests pass', + ].join('\n'); + + // Simulate raw agent output (only contains last summary) + const rawOutput = ` +Working on tests... + + +## Results +- All tests pass + +`; + + // UI logic: getFirstNonEmptySummary(feature?.summary, extractSummary(output)) + const displaySummary = getFirstNonEmptySummary(featureSummary, extractSummary(rawOutput)); + + // Should use server-accumulated summary + expect(displaySummary).toBe(featureSummary); + expect(displaySummary).toContain('### Implementation'); + expect(displaySummary).toContain('### Testing'); + + // If server summary was missing, only last summary would be shown + const fallbackSummary = extractSummary(rawOutput); + expect(fallbackSummary).not.toContain('Implementation'); + expect(fallbackSummary).toContain('All tests pass'); + }); + + it('should handle legacy features without server accumulation', () => { + // Legacy features have no feature.summary + const featureSummary = undefined; + + // Raw output contains the summary + const rawOutput = ` + +## Implementation Complete +- Created the feature +- All tests pass + +`; + + // UI logic: getFirstNonEmptySummary(feature?.summary, extractSummary(output)) + const displaySummary = getFirstNonEmptySummary(featureSummary, extractSummary(rawOutput)); + + // Should fall back to client-side extraction + expect(displaySummary).toContain('Implementation Complete'); + expect(displaySummary).toContain('All tests pass'); + }); + }); + + describe('error recovery and edge cases', () => { + it('should gracefully handle pipeline interruption', async () => { + // Step 1 completes + (pipelineService.getStep as Mock).mockResolvedValue({ + name: 'Implementation', + id: 'implementation', + }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_implementation', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'e2e-feature-1', 'Implementation done'); + + const step1Summary = ((atomicWriteJson as Mock).mock.calls[0][1] as Feature).summary; + + // Pipeline gets interrupted (status changes but summary is preserved) + // When user views the feature later, the summary should still be available + expect(step1Summary).toBe('### Implementation\n\nImplementation done'); + + // UI can still parse the partial pipeline + const phases = parsePhaseSummaries(step1Summary); + expect(phases.size).toBe(1); + expect(phases.get('implementation')).toBe('Implementation done'); + }); + + it('should handle very large accumulated summaries', async () => { + // Generate large content for each step + const generateLargeContent = (stepNum: number) => { + const lines = [`## Step ${stepNum} Changes`]; + for (let i = 0; i < 100; i++) { + lines.push( + `- Change ${i}: This is a detailed description of the change made during step ${stepNum}` + ); + } + return lines.join('\n'); + }; + + // Simulate 5 pipeline steps with large content + let currentSummary: string | undefined = undefined; + const stepNames = ['Planning', 'Implementation', 'Code Review', 'Testing', 'Refinement']; + + for (let i = 0; i < 5; i++) { + vi.clearAllMocks(); + (getFeatureDir as Mock).mockReturnValue('/project/.automaker/features/e2e-feature-1'); + (pipelineService.getStep as Mock).mockResolvedValue({ + name: stepNames[i], + id: stepNames[i].toLowerCase().replace(' ', '_'), + }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { + ...baseFeature, + status: `pipeline_${stepNames[i].toLowerCase().replace(' ', '_')}`, + summary: currentSummary, + }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'e2e-feature-1', generateLargeContent(i + 1)); + + currentSummary = ((atomicWriteJson as Mock).mock.calls[0][1] as Feature).summary; + } + + // Final summary should be large but still parseable + expect(currentSummary!.length).toBeGreaterThan(5000); + expect(isAccumulatedSummary(currentSummary)).toBe(true); + + const phases = parsePhaseSummaries(currentSummary); + expect(phases.size).toBe(5); + + // Verify all steps are present + for (const stepName of stepNames) { + expect(phases.has(stepName.toLowerCase())).toBe(true); + } + }); + }); + + describe('query invalidation simulation', () => { + it('should trigger UI refetch on auto_mode_summary event', async () => { + // This test documents the expected behavior: + // When saveFeatureSummary is called, it emits auto_mode_summary event + // The UI's use-query-invalidation.ts invalidates the feature query + // This causes a refetch of the feature, getting the updated summary + + (pipelineService.getStep as Mock).mockResolvedValue({ + name: 'Implementation', + id: 'implementation', + }); + (readJsonWithRecovery as Mock).mockResolvedValue({ + data: { ...baseFeature, status: 'pipeline_implementation', summary: undefined }, + recovered: false, + source: 'main', + }); + + await manager.saveFeatureSummary('/project', 'e2e-feature-1', 'Summary content'); + + // Verify event was emitted (triggers React Query invalidation) + expect(mockEvents.emit).toHaveBeenCalledWith( + 'auto-mode:event', + expect.objectContaining({ + type: 'auto_mode_summary', + featureId: 'e2e-feature-1', + summary: expect.any(String), + }) + ); + + // The UI would then: + // 1. Receive the event via WebSocket + // 2. Invalidate the feature query + // 3. Refetch the feature (GET /api/features/:id) + // 4. Display the updated feature.summary + }); + }); +}); + +/** + * KEY E2E FLOW SUMMARY: + * + * 1. PIPELINE EXECUTION: + * - Feature starts with status='pipeline_implementation' + * - Agent runs and produces summary + * - FeatureStateManager.saveFeatureSummary() accumulates with step header + * - Status advances to 'pipeline_testing' + * - Process repeats for each step + * + * 2. SERVER-SIDE ACCUMULATION: + * - First step: `### Implementation\n\n` + * - Second step: `### Implementation\n\n\n\n---\n\n### Testing\n\n` + * - Pattern continues with each step + * + * 3. EVENT EMISSION: + * - auto_mode_summary event contains FULL accumulated summary + * - UI receives event via WebSocket + * - React Query invalidates feature query + * - Feature is refetched with updated summary + * + * 4. UI DISPLAY: + * - AgentOutputModal uses: getFirstNonEmptySummary(feature?.summary, extractSummary(output)) + * - feature.summary is preferred (contains all steps) + * - extractSummary() is fallback (last summary only) + * - parsePhaseSummaries() can split into individual phases for UI + * + * 5. FALLBACK FOR LEGACY: + * - Old features may not have feature.summary + * - UI falls back to extracting from raw output + * - Only last summary is available in this case + */ diff --git a/apps/server/tests/unit/ui/agent-output-summary-priority.test.ts b/apps/server/tests/unit/ui/agent-output-summary-priority.test.ts new file mode 100644 index 00000000..c31f950e --- /dev/null +++ b/apps/server/tests/unit/ui/agent-output-summary-priority.test.ts @@ -0,0 +1,403 @@ +/** + * Unit tests for the agent output summary priority logic. + * + * These tests verify the summary display logic used in AgentOutputModal + * where the UI must choose between server-accumulated summaries and + * client-side extracted summaries. + * + * Priority order (from agent-output-modal.tsx): + * 1. feature.summary (server-accumulated, contains all pipeline steps) + * 2. extractSummary(output) (client-side fallback, last summary only) + * + * This priority is crucial for pipeline features where the server-side + * accumulation provides the complete history of all step summaries. + */ + +import { describe, it, expect } from 'vitest'; +// Import the actual extractSummary function to ensure test behavior matches production +import { extractSummary } from '../../../../ui/src/lib/log-parser.ts'; +import { getFirstNonEmptySummary } from '../../../../ui/src/lib/summary-selection.ts'; + +/** + * Simulates the summary priority logic from AgentOutputModal. + * + * Priority: + * 1. feature?.summary (server-accumulated) + * 2. extractSummary(output) (client-side fallback) + */ +function getDisplaySummary( + featureSummary: string | undefined | null, + rawOutput: string +): string | null { + return getFirstNonEmptySummary(featureSummary, extractSummary(rawOutput)); +} + +describe('Agent Output Summary Priority Logic', () => { + describe('priority order: feature.summary over extractSummary', () => { + it('should use feature.summary when available (server-accumulated wins)', () => { + const featureSummary = '### Step 1\n\nFirst step\n\n---\n\n### Step 2\n\nSecond step'; + const rawOutput = ` + +Only the last summary is extracted client-side + +`; + + const result = getDisplaySummary(featureSummary, rawOutput); + + // Server-accumulated summary should be used, not client-side extraction + expect(result).toBe(featureSummary); + expect(result).toContain('### Step 1'); + expect(result).toContain('### Step 2'); + expect(result).not.toContain('Only the last summary'); + }); + + it('should use client-side extractSummary when feature.summary is undefined', () => { + const rawOutput = ` + +This is the only summary + +`; + + const result = getDisplaySummary(undefined, rawOutput); + + expect(result).toBe('This is the only summary'); + }); + + it('should use client-side extractSummary when feature.summary is null', () => { + const rawOutput = ` + +Client-side extracted summary + +`; + + const result = getDisplaySummary(null, rawOutput); + + expect(result).toBe('Client-side extracted summary'); + }); + + it('should use client-side extractSummary when feature.summary is empty string', () => { + const rawOutput = ` + +Fallback content + +`; + + const result = getDisplaySummary('', rawOutput); + + // Empty string is falsy, so fallback is used + expect(result).toBe('Fallback content'); + }); + + it('should use client-side extractSummary when feature.summary is whitespace only', () => { + const rawOutput = ` + +Fallback for whitespace summary + +`; + + const result = getDisplaySummary(' \n ', rawOutput); + + expect(result).toBe('Fallback for whitespace summary'); + }); + + it('should preserve original server summary formatting when non-empty after trim', () => { + const featureSummary = '\n### Implementation\n\n- Added API route\n'; + + const result = getDisplaySummary(featureSummary, ''); + + expect(result).toBe(featureSummary); + expect(result).toContain('### Implementation'); + }); + }); + + describe('pipeline step accumulation scenarios', () => { + it('should display all pipeline steps when using server-accumulated summary', () => { + // This simulates a feature that went through 3 pipeline steps + const featureSummary = [ + '### Implementation', + '', + '## Changes', + '- Created new module', + '- Added tests', + '', + '---', + '', + '### Code Review', + '', + '## Review Results', + '- Approved with minor suggestions', + '', + '---', + '', + '### Testing', + '', + '## Test Results', + '- All 42 tests pass', + '- Coverage: 98%', + ].join('\n'); + + const rawOutput = ` + +Only testing step visible in raw output + +`; + + const result = getDisplaySummary(featureSummary, rawOutput); + + // All pipeline steps should be visible + expect(result).toContain('### Implementation'); + expect(result).toContain('### Code Review'); + expect(result).toContain('### Testing'); + expect(result).toContain('All 42 tests pass'); + }); + + it('should display only last summary when server-side accumulation not available', () => { + // When feature.summary is not available, only the last summary is shown + const rawOutput = ` + +Step 1: Implementation complete + + +--- + + +Step 2: Code review complete + + +--- + + +Step 3: Testing complete + +`; + + const result = getDisplaySummary(undefined, rawOutput); + + // Only the LAST summary should be shown (client-side fallback behavior) + expect(result).toBe('Step 3: Testing complete'); + expect(result).not.toContain('Step 1'); + expect(result).not.toContain('Step 2'); + }); + + it('should handle single-step pipeline (no accumulation needed)', () => { + const featureSummary = '### Implementation\n\nCreated the feature'; + const rawOutput = ''; + + const result = getDisplaySummary(featureSummary, rawOutput); + + expect(result).toBe(featureSummary); + expect(result).not.toContain('---'); // No separator for single step + }); + }); + + describe('edge cases', () => { + it('should return null when both feature.summary and extractSummary are unavailable', () => { + const rawOutput = 'No summary tags here, just regular output.'; + + const result = getDisplaySummary(undefined, rawOutput); + + expect(result).toBeNull(); + }); + + it('should return null when rawOutput is empty and no feature summary', () => { + const result = getDisplaySummary(undefined, ''); + + expect(result).toBeNull(); + }); + + it('should return null when rawOutput is whitespace only', () => { + const result = getDisplaySummary(undefined, ' \n\n '); + + expect(result).toBeNull(); + }); + + it('should use client-side fallback when feature.summary is empty string (falsy)', () => { + // Empty string is falsy in JavaScript, so fallback is correctly used. + // This is the expected behavior - an empty summary has no value to display. + const rawOutput = ` + +Fallback content when server summary is empty + +`; + + // Empty string is falsy, so fallback is used + const result = getDisplaySummary('', rawOutput); + expect(result).toBe('Fallback content when server summary is empty'); + }); + + it('should behave identically when feature is null vs feature.summary is undefined', () => { + // This test verifies that the behavior is consistent whether: + // - The feature object itself is null/undefined + // - The feature object exists but summary property is undefined + const rawOutput = ` + +Client-side extracted summary + +`; + + // Both scenarios should use client-side fallback + const resultWithUndefined = getDisplaySummary(undefined, rawOutput); + const resultWithNull = getDisplaySummary(null, rawOutput); + + expect(resultWithUndefined).toBe('Client-side extracted summary'); + expect(resultWithNull).toBe('Client-side extracted summary'); + expect(resultWithUndefined).toBe(resultWithNull); + }); + }); + + describe('markdown content preservation', () => { + it('should preserve markdown formatting in server-accumulated summary', () => { + const featureSummary = `### Code Review + +## Changes Made +- Fixed **critical bug** in \`parser.ts\` +- Added \`validateInput()\` function + +\`\`\`typescript +const x = 1; +\`\`\` + +| Test | Result | +|------|--------| +| Unit | Pass |`; + + const result = getDisplaySummary(featureSummary, ''); + + expect(result).toContain('**critical bug**'); + expect(result).toContain('`parser.ts`'); + expect(result).toContain('```typescript'); + expect(result).toContain('| Test | Result |'); + }); + + it('should preserve unicode in server-accumulated summary', () => { + const featureSummary = '### Testing\n\n✅ 42 passed\n❌ 0 failed\n🎉 100% coverage'; + + const result = getDisplaySummary(featureSummary, ''); + + expect(result).toContain('✅'); + expect(result).toContain('❌'); + expect(result).toContain('🎉'); + }); + }); + + describe('real-world scenarios', () => { + it('should handle typical pipeline feature with server accumulation', () => { + // Simulates a real pipeline feature that went through Implementation → Testing + const featureSummary = `### Implementation + +## Changes Made +- Created UserProfile component +- Added authentication middleware +- Updated API endpoints + +--- + +### Testing + +## Test Results +- Unit tests: 15 passed +- Integration tests: 8 passed +- E2E tests: 3 passed`; + + const rawOutput = ` +Working on the feature... + + +## Test Results +- Unit tests: 15 passed +- Integration tests: 8 passed +- E2E tests: 3 passed + +`; + + const result = getDisplaySummary(featureSummary, rawOutput); + + // Both steps should be visible + expect(result).toContain('### Implementation'); + expect(result).toContain('### Testing'); + expect(result).toContain('UserProfile component'); + expect(result).toContain('15 passed'); + }); + + it('should handle non-pipeline feature (single summary)', () => { + // Non-pipeline features have a single summary, no accumulation + const featureSummary = '## Implementation Complete\n- Created the feature\n- All tests pass'; + const rawOutput = ''; + + const result = getDisplaySummary(featureSummary, rawOutput); + + expect(result).toBe(featureSummary); + expect(result).not.toContain('###'); // No step headers for non-pipeline + }); + + it('should handle legacy feature without server summary (fallback)', () => { + // Legacy features may not have feature.summary set + const rawOutput = ` + +Legacy implementation from before server-side accumulation + +`; + + const result = getDisplaySummary(undefined, rawOutput); + + expect(result).toBe('Legacy implementation from before server-side accumulation'); + }); + }); + + describe('view mode determination logic', () => { + /** + * Simulates the effectiveViewMode logic from agent-output-modal.tsx line 86 + * Default to 'summary' if summary is available, otherwise 'parsed' + */ + function getEffectiveViewMode( + viewMode: string | null, + summary: string | null + ): 'summary' | 'parsed' { + return (viewMode ?? (summary ? 'summary' : 'parsed')) as 'summary' | 'parsed'; + } + + it('should default to summary view when server summary is available', () => { + const summary = '### Implementation\n\nContent'; + const result = getEffectiveViewMode(null, summary); + expect(result).toBe('summary'); + }); + + it('should default to summary view when client-side extraction succeeds', () => { + const summary = 'Extracted from raw output'; + const result = getEffectiveViewMode(null, summary); + expect(result).toBe('summary'); + }); + + it('should default to parsed view when no summary is available', () => { + const result = getEffectiveViewMode(null, null); + expect(result).toBe('parsed'); + }); + + it('should respect explicit view mode selection over default', () => { + const summary = 'Summary is available'; + expect(getEffectiveViewMode('raw', summary)).toBe('raw'); + expect(getEffectiveViewMode('parsed', summary)).toBe('parsed'); + expect(getEffectiveViewMode('changes', summary)).toBe('changes'); + }); + }); +}); + +/** + * KEY ARCHITECTURE INSIGHT: + * + * The priority order (feature.summary > extractSummary(output)) is essential for + * pipeline features because: + * + * 1. Server-side accumulation (FeatureStateManager.saveFeatureSummary) collects + * ALL step summaries with headers and separators in chronological order. + * + * 2. Client-side extractSummary() only returns the LAST summary tag from raw output, + * losing all previous step summaries. + * + * 3. The UI must prefer feature.summary to display the complete history of all + * pipeline steps to the user. + * + * For non-pipeline features (single execution), both sources contain the same + * summary, so the priority doesn't matter. But for pipeline features, using the + * wrong source would result in incomplete information display. + */ diff --git a/apps/server/tests/unit/ui/log-parser-mixed-format.test.ts b/apps/server/tests/unit/ui/log-parser-mixed-format.test.ts new file mode 100644 index 00000000..37a03e9d --- /dev/null +++ b/apps/server/tests/unit/ui/log-parser-mixed-format.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from 'vitest'; +import { + parseAllPhaseSummaries, + parsePhaseSummaries, + extractPhaseSummary, + extractImplementationSummary, + isAccumulatedSummary, +} from '../../../../ui/src/lib/log-parser.ts'; + +describe('log-parser mixed summary format compatibility', () => { + const mixedSummary = [ + 'Implemented core auth flow and API wiring.', + '', + '---', + '', + '### Code Review', + '', + 'Addressed lint warnings and improved error handling.', + '', + '---', + '', + '### Testing', + '', + 'All tests passing.', + ].join('\n'); + + it('treats leading headerless section as Implementation phase', () => { + const phases = parsePhaseSummaries(mixedSummary); + + expect(phases.get('implementation')).toBe('Implemented core auth flow and API wiring.'); + expect(phases.get('code review')).toBe('Addressed lint warnings and improved error handling.'); + expect(phases.get('testing')).toBe('All tests passing.'); + }); + + it('returns implementation summary from mixed format', () => { + expect(extractImplementationSummary(mixedSummary)).toBe( + 'Implemented core auth flow and API wiring.' + ); + }); + + it('includes Implementation as the first parsed phase entry', () => { + const entries = parseAllPhaseSummaries(mixedSummary); + + expect(entries[0]).toMatchObject({ + phaseName: 'Implementation', + content: 'Implemented core auth flow and API wiring.', + }); + expect(entries.map((entry) => entry.phaseName)).toEqual([ + 'Implementation', + 'Code Review', + 'Testing', + ]); + }); + + it('extracts specific phase summaries from mixed format', () => { + expect(extractPhaseSummary(mixedSummary, 'Implementation')).toBe( + 'Implemented core auth flow and API wiring.' + ); + expect(extractPhaseSummary(mixedSummary, 'Code Review')).toBe( + 'Addressed lint warnings and improved error handling.' + ); + expect(extractPhaseSummary(mixedSummary, 'Testing')).toBe('All tests passing.'); + }); + + it('treats mixed format as accumulated summary', () => { + expect(isAccumulatedSummary(mixedSummary)).toBe(true); + }); +}); diff --git a/apps/server/tests/unit/ui/log-parser-phase-summary.test.ts b/apps/server/tests/unit/ui/log-parser-phase-summary.test.ts new file mode 100644 index 00000000..c919b5a5 --- /dev/null +++ b/apps/server/tests/unit/ui/log-parser-phase-summary.test.ts @@ -0,0 +1,973 @@ +/** + * Unit tests for log-parser phase summary parsing functions. + * + * These functions are used to parse accumulated summaries that contain multiple + * pipeline step summaries separated by `---` and identified by `### StepName` headers. + * + * Functions tested: + * - parsePhaseSummaries: Parses the entire accumulated summary into a Map + * - extractPhaseSummary: Extracts a specific phase's content + * - extractImplementationSummary: Extracts implementation phase content (convenience) + * - isAccumulatedSummary: Checks if a summary is in accumulated format + */ + +import { describe, it, expect } from 'vitest'; + +// Mirror the functions from apps/ui/src/lib/log-parser.ts +// (We can't import directly because it's a UI file) + +/** + * Parses an accumulated summary string into individual phase summaries. + */ +function parsePhaseSummaries(summary: string | undefined): Map { + const phaseSummaries = new Map(); + + if (!summary || !summary.trim()) { + return phaseSummaries; + } + + // Split by the horizontal rule separator + const sections = summary.split(/\n\n---\n\n/); + + for (const section of sections) { + // Match the phase header pattern: ### Phase Name + const headerMatch = section.match(/^###\s+(.+?)(?:\n|$)/); + if (headerMatch) { + const phaseName = headerMatch[1].trim().toLowerCase(); + // Extract content after the header (skip the header line and leading newlines) + const content = section.substring(headerMatch[0].length).trim(); + phaseSummaries.set(phaseName, content); + } + } + + return phaseSummaries; +} + +/** + * Extracts a specific phase summary from an accumulated summary string. + */ +function extractPhaseSummary(summary: string | undefined, phaseName: string): string | null { + const phaseSummaries = parsePhaseSummaries(summary); + const normalizedPhaseName = phaseName.toLowerCase(); + return phaseSummaries.get(normalizedPhaseName) || null; +} + +/** + * Extracts the implementation phase summary from an accumulated summary string. + */ +function extractImplementationSummary(summary: string | undefined): string | null { + if (!summary || !summary.trim()) { + return null; + } + + const phaseSummaries = parsePhaseSummaries(summary); + + // Try exact match first + const implementationContent = phaseSummaries.get('implementation'); + if (implementationContent) { + return implementationContent; + } + + // Fallback: find any phase containing "implement" + for (const [phaseName, content] of phaseSummaries) { + if (phaseName.includes('implement')) { + return content; + } + } + + // If no phase summaries found, the summary might not be in accumulated format + // (legacy or non-pipeline feature). In this case, return the whole summary + // if it looks like a single summary (no phase headers). + if (!summary.includes('### ') && !summary.includes('\n---\n')) { + return summary; + } + + return null; +} + +/** + * Checks if a summary string is in the accumulated multi-phase format. + */ +function isAccumulatedSummary(summary: string | undefined): boolean { + if (!summary || !summary.trim()) { + return false; + } + + // Check for the presence of phase headers with separator + const hasMultiplePhases = + summary.includes('\n\n---\n\n') && summary.match(/###\s+.+/g)?.length > 0; + + return hasMultiplePhases; +} + +/** + * Represents a single phase entry in an accumulated summary. + */ +interface PhaseSummaryEntry { + /** The phase name (e.g., "Implementation", "Testing", "Code Review") */ + phaseName: string; + /** The content of this phase's summary */ + content: string; + /** The original header line (e.g., "### Implementation") */ + header: string; +} + +/** Default phase name used for non-accumulated summaries */ +const DEFAULT_PHASE_NAME = 'Summary'; + +/** + * Parses an accumulated summary into individual phase entries. + * Returns phases in the order they appear in the summary. + */ +function parseAllPhaseSummaries(summary: string | undefined): PhaseSummaryEntry[] { + const entries: PhaseSummaryEntry[] = []; + + if (!summary || !summary.trim()) { + return entries; + } + + // Check if this is an accumulated summary (has phase headers) + if (!summary.includes('### ')) { + // Not an accumulated summary - return as single entry with generic name + return [ + { phaseName: DEFAULT_PHASE_NAME, content: summary, header: `### ${DEFAULT_PHASE_NAME}` }, + ]; + } + + // Split by the horizontal rule separator + const sections = summary.split(/\n\n---\n\n/); + + for (const section of sections) { + // Match the phase header pattern: ### Phase Name + const headerMatch = section.match(/^(###\s+)(.+?)(?:\n|$)/); + if (headerMatch) { + const header = headerMatch[0].trim(); + const phaseName = headerMatch[2].trim(); + // Extract content after the header (skip the header line and leading newlines) + const content = section.substring(headerMatch[0].length).trim(); + entries.push({ phaseName, content, header }); + } + } + + return entries; +} + +describe('parsePhaseSummaries', () => { + describe('basic parsing', () => { + it('should parse single phase summary', () => { + const summary = `### Implementation + +## Changes Made +- Created new module +- Added unit tests`; + + const result = parsePhaseSummaries(summary); + + expect(result.size).toBe(1); + expect(result.get('implementation')).toBe( + '## Changes Made\n- Created new module\n- Added unit tests' + ); + }); + + it('should parse multiple phase summaries', () => { + const summary = `### Implementation + +## Changes Made +- Created new module + +--- + +### Testing + +## Test Results +- All tests pass`; + + const result = parsePhaseSummaries(summary); + + expect(result.size).toBe(2); + expect(result.get('implementation')).toBe('## Changes Made\n- Created new module'); + expect(result.get('testing')).toBe('## Test Results\n- All tests pass'); + }); + + it('should handle three or more phases', () => { + const summary = `### Planning + +Plan created + +--- + +### Implementation + +Code written + +--- + +### Testing + +Tests pass + +--- + +### Refinement + +Code polished`; + + const result = parsePhaseSummaries(summary); + + expect(result.size).toBe(4); + expect(result.get('planning')).toBe('Plan created'); + expect(result.get('implementation')).toBe('Code written'); + expect(result.get('testing')).toBe('Tests pass'); + expect(result.get('refinement')).toBe('Code polished'); + }); + }); + + describe('edge cases', () => { + it('should return empty map for undefined summary', () => { + const result = parsePhaseSummaries(undefined); + expect(result.size).toBe(0); + }); + + it('should return empty map for null summary', () => { + const result = parsePhaseSummaries(null as unknown as string); + expect(result.size).toBe(0); + }); + + it('should return empty map for empty string', () => { + const result = parsePhaseSummaries(''); + expect(result.size).toBe(0); + }); + + it('should return empty map for whitespace-only string', () => { + const result = parsePhaseSummaries(' \n\n '); + expect(result.size).toBe(0); + }); + + it('should handle summary without phase headers', () => { + const summary = 'Just some regular content without headers'; + const result = parsePhaseSummaries(summary); + expect(result.size).toBe(0); + }); + + it('should handle section without header after separator', () => { + const summary = `### Implementation + +Content here + +--- + +This section has no header`; + + const result = parsePhaseSummaries(summary); + + expect(result.size).toBe(1); + expect(result.get('implementation')).toBe('Content here'); + }); + }); + + describe('phase name normalization', () => { + it('should normalize phase names to lowercase', () => { + const summary = `### IMPLEMENTATION + +Content`; + + const result = parsePhaseSummaries(summary); + expect(result.has('implementation')).toBe(true); + expect(result.has('IMPLEMENTATION')).toBe(false); + }); + + it('should handle mixed case phase names', () => { + const summary = `### Code Review + +Content`; + + const result = parsePhaseSummaries(summary); + expect(result.has('code review')).toBe(true); + }); + + it('should preserve spaces in multi-word phase names', () => { + const summary = `### Code Review + +Content`; + + const result = parsePhaseSummaries(summary); + expect(result.get('code review')).toBe('Content'); + }); + }); + + describe('content preservation', () => { + it('should preserve markdown formatting in content', () => { + const summary = `### Implementation + +## Heading +- **Bold text** +- \`code\` +\`\`\`typescript +const x = 1; +\`\`\``; + + const result = parsePhaseSummaries(summary); + const content = result.get('implementation'); + + expect(content).toContain('**Bold text**'); + expect(content).toContain('`code`'); + expect(content).toContain('```typescript'); + }); + + it('should preserve unicode in content', () => { + const summary = `### Testing + +Results: ✅ 42 passed, ❌ 0 failed`; + + const result = parsePhaseSummaries(summary); + expect(result.get('testing')).toContain('✅'); + expect(result.get('testing')).toContain('❌'); + }); + + it('should preserve tables in content', () => { + const summary = `### Testing + +| Test | Result | +|------|--------| +| Unit | Pass |`; + + const result = parsePhaseSummaries(summary); + expect(result.get('testing')).toContain('| Test | Result |'); + }); + + it('should handle empty phase content', () => { + const summary = `### Implementation + +--- + +### Testing + +Content`; + + const result = parsePhaseSummaries(summary); + expect(result.get('implementation')).toBe(''); + expect(result.get('testing')).toBe('Content'); + }); + }); +}); + +describe('extractPhaseSummary', () => { + describe('extraction by phase name', () => { + it('should extract specified phase content', () => { + const summary = `### Implementation + +Implementation content + +--- + +### Testing + +Testing content`; + + expect(extractPhaseSummary(summary, 'Implementation')).toBe('Implementation content'); + expect(extractPhaseSummary(summary, 'Testing')).toBe('Testing content'); + }); + + it('should be case-insensitive for phase name', () => { + const summary = `### Implementation + +Content`; + + expect(extractPhaseSummary(summary, 'implementation')).toBe('Content'); + expect(extractPhaseSummary(summary, 'IMPLEMENTATION')).toBe('Content'); + expect(extractPhaseSummary(summary, 'ImPlEmEnTaTiOn')).toBe('Content'); + }); + + it('should return null for non-existent phase', () => { + const summary = `### Implementation + +Content`; + + expect(extractPhaseSummary(summary, 'NonExistent')).toBeNull(); + }); + }); + + describe('edge cases', () => { + it('should return null for undefined summary', () => { + expect(extractPhaseSummary(undefined, 'Implementation')).toBeNull(); + }); + + it('should return null for empty summary', () => { + expect(extractPhaseSummary('', 'Implementation')).toBeNull(); + }); + + it('should handle whitespace in phase name', () => { + const summary = `### Code Review + +Content`; + + expect(extractPhaseSummary(summary, 'Code Review')).toBe('Content'); + expect(extractPhaseSummary(summary, 'code review')).toBe('Content'); + }); + }); +}); + +describe('extractImplementationSummary', () => { + describe('exact match', () => { + it('should extract implementation phase by exact name', () => { + const summary = `### Implementation + +## Changes Made +- Created feature +- Added tests + +--- + +### Testing + +Tests pass`; + + const result = extractImplementationSummary(summary); + expect(result).toBe('## Changes Made\n- Created feature\n- Added tests'); + }); + + it('should be case-insensitive', () => { + const summary = `### IMPLEMENTATION + +Content`; + + expect(extractImplementationSummary(summary)).toBe('Content'); + }); + }); + + describe('partial match fallback', () => { + it('should find phase containing "implement"', () => { + const summary = `### Feature Implementation + +Content here`; + + const result = extractImplementationSummary(summary); + expect(result).toBe('Content here'); + }); + + it('should find phase containing "implementation"', () => { + const summary = `### Implementation Phase + +Content here`; + + const result = extractImplementationSummary(summary); + expect(result).toBe('Content here'); + }); + }); + + describe('legacy/non-accumulated summary handling', () => { + it('should return full summary if no phase headers present', () => { + const summary = `## Changes Made +- Created feature +- Added tests`; + + const result = extractImplementationSummary(summary); + expect(result).toBe(summary); + }); + + it('should return null if summary has phase headers but no implementation', () => { + const summary = `### Testing + +Tests pass + +--- + +### Review + +Review complete`; + + const result = extractImplementationSummary(summary); + expect(result).toBeNull(); + }); + + it('should not return full summary if it contains phase headers', () => { + const summary = `### Testing + +Tests pass`; + + const result = extractImplementationSummary(summary); + expect(result).toBeNull(); + }); + }); + + describe('edge cases', () => { + it('should return null for undefined summary', () => { + expect(extractImplementationSummary(undefined)).toBeNull(); + }); + + it('should return null for empty string', () => { + expect(extractImplementationSummary('')).toBeNull(); + }); + + it('should return null for whitespace-only string', () => { + expect(extractImplementationSummary(' \n\n ')).toBeNull(); + }); + }); +}); + +describe('isAccumulatedSummary', () => { + describe('accumulated format detection', () => { + it('should return true for accumulated summary with separator and headers', () => { + const summary = `### Implementation + +Content + +--- + +### Testing + +Content`; + + expect(isAccumulatedSummary(summary)).toBe(true); + }); + + it('should return true for accumulated summary with multiple phases', () => { + const summary = `### Phase 1 + +Content 1 + +--- + +### Phase 2 + +Content 2 + +--- + +### Phase 3 + +Content 3`; + + expect(isAccumulatedSummary(summary)).toBe(true); + }); + + it('should return true for accumulated summary with just one phase and separator', () => { + // Even a single phase with a separator suggests it's in accumulated format + const summary = `### Implementation + +Content + +--- + +### Testing + +More content`; + + expect(isAccumulatedSummary(summary)).toBe(true); + }); + }); + + describe('non-accumulated format detection', () => { + it('should return false for summary without separator', () => { + const summary = `### Implementation + +Just content`; + + expect(isAccumulatedSummary(summary)).toBe(false); + }); + + it('should return false for summary with separator but no headers', () => { + const summary = `Content + +--- + +More content`; + + expect(isAccumulatedSummary(summary)).toBe(false); + }); + + it('should return false for simple text summary', () => { + const summary = 'Just a simple summary without any special formatting'; + expect(isAccumulatedSummary(summary)).toBe(false); + }); + + it('should return false for markdown summary without phase headers', () => { + const summary = `## Changes Made +- Created feature +- Added tests`; + expect(isAccumulatedSummary(summary)).toBe(false); + }); + }); + + describe('edge cases', () => { + it('should return false for undefined summary', () => { + expect(isAccumulatedSummary(undefined)).toBe(false); + }); + + it('should return false for null summary', () => { + expect(isAccumulatedSummary(null as unknown as string)).toBe(false); + }); + + it('should return false for empty string', () => { + expect(isAccumulatedSummary('')).toBe(false); + }); + + it('should return false for whitespace-only string', () => { + expect(isAccumulatedSummary(' \n\n ')).toBe(false); + }); + }); +}); + +describe('Integration: Full parsing workflow', () => { + it('should correctly parse typical server-accumulated pipeline summary', () => { + // This simulates what FeatureStateManager.saveFeatureSummary() produces + const summary = [ + '### Implementation', + '', + '## Changes', + '- Added auth module', + '- Created user service', + '', + '---', + '', + '### Code Review', + '', + '## Review Results', + '- Style issues fixed', + '- Added error handling', + '', + '---', + '', + '### Testing', + '', + '## Test Results', + '- 42 tests pass', + '- 98% coverage', + ].join('\n'); + + // Verify isAccumulatedSummary + expect(isAccumulatedSummary(summary)).toBe(true); + + // Verify parsePhaseSummaries + const phases = parsePhaseSummaries(summary); + expect(phases.size).toBe(3); + expect(phases.get('implementation')).toContain('Added auth module'); + expect(phases.get('code review')).toContain('Style issues fixed'); + expect(phases.get('testing')).toContain('42 tests pass'); + + // Verify extractPhaseSummary + expect(extractPhaseSummary(summary, 'Implementation')).toContain('Added auth module'); + expect(extractPhaseSummary(summary, 'Code Review')).toContain('Style issues fixed'); + expect(extractPhaseSummary(summary, 'Testing')).toContain('42 tests pass'); + + // Verify extractImplementationSummary + expect(extractImplementationSummary(summary)).toContain('Added auth module'); + }); + + it('should handle legacy non-pipeline summary correctly', () => { + // Legacy features have simple summaries without accumulation + const summary = `## Implementation Complete +- Created the feature +- All tests pass`; + + // Should NOT be detected as accumulated + expect(isAccumulatedSummary(summary)).toBe(false); + + // parsePhaseSummaries should return empty + const phases = parsePhaseSummaries(summary); + expect(phases.size).toBe(0); + + // extractPhaseSummary should return null + expect(extractPhaseSummary(summary, 'Implementation')).toBeNull(); + + // extractImplementationSummary should return the full summary (legacy handling) + expect(extractImplementationSummary(summary)).toBe(summary); + }); + + it('should handle single-step pipeline summary', () => { + // A single pipeline step still gets the header but no separator + const summary = `### Implementation + +## Changes +- Created the feature`; + + // Should NOT be detected as accumulated (no separator) + expect(isAccumulatedSummary(summary)).toBe(false); + + // parsePhaseSummaries should still extract the single phase + const phases = parsePhaseSummaries(summary); + expect(phases.size).toBe(1); + expect(phases.get('implementation')).toContain('Created the feature'); + }); +}); + +/** + * KEY ARCHITECTURE NOTES: + * + * 1. The accumulated summary format uses: + * - `### PhaseName` for step headers + * - `\n\n---\n\n` as separator between steps + * + * 2. Phase names are normalized to lowercase in the Map for case-insensitive lookup. + * + * 3. Legacy summaries (non-pipeline features) don't have phase headers and should + * be returned as-is by extractImplementationSummary. + * + * 4. isAccumulatedSummary() checks for BOTH separator AND phase headers to be + * confident that the summary is in the accumulated format. + * + * 5. The server-side FeatureStateManager.saveFeatureSummary() is responsible for + * creating summaries in this accumulated format. + */ + +describe('parseAllPhaseSummaries', () => { + describe('basic parsing', () => { + it('should parse single phase summary into array with one entry', () => { + const summary = `### Implementation + +## Changes Made +- Created new module +- Added unit tests`; + + const result = parseAllPhaseSummaries(summary); + + expect(result.length).toBe(1); + expect(result[0].phaseName).toBe('Implementation'); + expect(result[0].content).toBe('## Changes Made\n- Created new module\n- Added unit tests'); + expect(result[0].header).toBe('### Implementation'); + }); + + it('should parse multiple phase summaries in order', () => { + const summary = `### Implementation + +## Changes Made +- Created new module + +--- + +### Testing + +## Test Results +- All tests pass`; + + const result = parseAllPhaseSummaries(summary); + + expect(result.length).toBe(2); + // Verify order is preserved + expect(result[0].phaseName).toBe('Implementation'); + expect(result[0].content).toBe('## Changes Made\n- Created new module'); + expect(result[1].phaseName).toBe('Testing'); + expect(result[1].content).toBe('## Test Results\n- All tests pass'); + }); + + it('should parse three or more phases in correct order', () => { + const summary = `### Planning + +Plan created + +--- + +### Implementation + +Code written + +--- + +### Testing + +Tests pass + +--- + +### Refinement + +Code polished`; + + const result = parseAllPhaseSummaries(summary); + + expect(result.length).toBe(4); + expect(result[0].phaseName).toBe('Planning'); + expect(result[1].phaseName).toBe('Implementation'); + expect(result[2].phaseName).toBe('Testing'); + expect(result[3].phaseName).toBe('Refinement'); + }); + }); + + describe('non-accumulated summary handling', () => { + it('should return single entry for summary without phase headers', () => { + const summary = `## Changes Made +- Created feature +- Added tests`; + + const result = parseAllPhaseSummaries(summary); + + expect(result.length).toBe(1); + expect(result[0].phaseName).toBe('Summary'); + expect(result[0].content).toBe(summary); + }); + + it('should return single entry for simple text summary', () => { + const summary = 'Just a simple summary without any special formatting'; + + const result = parseAllPhaseSummaries(summary); + + expect(result.length).toBe(1); + expect(result[0].phaseName).toBe('Summary'); + expect(result[0].content).toBe(summary); + }); + }); + + describe('edge cases', () => { + it('should return empty array for undefined summary', () => { + const result = parseAllPhaseSummaries(undefined); + expect(result.length).toBe(0); + }); + + it('should return empty array for empty string', () => { + const result = parseAllPhaseSummaries(''); + expect(result.length).toBe(0); + }); + + it('should return empty array for whitespace-only string', () => { + const result = parseAllPhaseSummaries(' \n\n '); + expect(result.length).toBe(0); + }); + + it('should handle section without header after separator', () => { + const summary = `### Implementation + +Content here + +--- + +This section has no header`; + + const result = parseAllPhaseSummaries(summary); + + expect(result.length).toBe(1); + expect(result[0].phaseName).toBe('Implementation'); + }); + }); + + describe('content preservation', () => { + it('should preserve markdown formatting in content', () => { + const summary = `### Implementation + +## Heading +- **Bold text** +- \`code\` +\`\`\`typescript +const x = 1; +\`\`\``; + + const result = parseAllPhaseSummaries(summary); + const content = result[0].content; + + expect(content).toContain('**Bold text**'); + expect(content).toContain('`code`'); + expect(content).toContain('```typescript'); + }); + + it('should preserve unicode in content', () => { + const summary = `### Testing + +Results: ✅ 42 passed, ❌ 0 failed`; + + const result = parseAllPhaseSummaries(summary); + expect(result[0].content).toContain('✅'); + expect(result[0].content).toContain('❌'); + }); + + it('should preserve tables in content', () => { + const summary = `### Testing + +| Test | Result | +|------|--------| +| Unit | Pass |`; + + const result = parseAllPhaseSummaries(summary); + expect(result[0].content).toContain('| Test | Result |'); + }); + + it('should handle empty phase content', () => { + const summary = `### Implementation + +--- + +### Testing + +Content`; + + const result = parseAllPhaseSummaries(summary); + expect(result.length).toBe(2); + expect(result[0].content).toBe(''); + expect(result[1].content).toBe('Content'); + }); + }); + + describe('header preservation', () => { + it('should preserve original header text', () => { + const summary = `### Code Review + +Content`; + + const result = parseAllPhaseSummaries(summary); + expect(result[0].header).toBe('### Code Review'); + }); + + it('should preserve phase name with original casing', () => { + const summary = `### CODE REVIEW + +Content`; + + const result = parseAllPhaseSummaries(summary); + expect(result[0].phaseName).toBe('CODE REVIEW'); + }); + }); + + describe('chronological order preservation', () => { + it('should maintain order: Alpha before Beta before Gamma', () => { + const summary = `### Alpha + +First + +--- + +### Beta + +Second + +--- + +### Gamma + +Third`; + + const result = parseAllPhaseSummaries(summary); + + expect(result.length).toBe(3); + const names = result.map((e) => e.phaseName); + expect(names).toEqual(['Alpha', 'Beta', 'Gamma']); + }); + + it('should preserve typical pipeline order', () => { + const summary = [ + '### Implementation', + '', + '## Changes', + '- Added auth module', + '', + '---', + '', + '### Code Review', + '', + '## Review Results', + '- Style issues fixed', + '', + '---', + '', + '### Testing', + '', + '## Test Results', + '- 42 tests pass', + ].join('\n'); + + const result = parseAllPhaseSummaries(summary); + + expect(result.length).toBe(3); + expect(result[0].phaseName).toBe('Implementation'); + expect(result[1].phaseName).toBe('Code Review'); + expect(result[2].phaseName).toBe('Testing'); + }); + }); +}); diff --git a/apps/server/tests/unit/ui/log-parser-summary.test.ts b/apps/server/tests/unit/ui/log-parser-summary.test.ts new file mode 100644 index 00000000..e5bc0b70 --- /dev/null +++ b/apps/server/tests/unit/ui/log-parser-summary.test.ts @@ -0,0 +1,453 @@ +/** + * Unit tests for the UI's log-parser extractSummary() function. + * + * These tests document the behavior of extractSummary() which is used as a + * CLIENT-SIDE FALLBACK when feature.summary (server-accumulated) is not available. + * + * IMPORTANT: extractSummary() returns only the LAST tag from raw output. + * For pipeline features with multiple steps, the server-side FeatureStateManager + * accumulates all step summaries into feature.summary, which the UI prefers. + * + * The tests below verify that extractSummary() correctly: + * - Returns the LAST summary when multiple exist (mimicking pipeline accumulation) + * - Handles various summary formats ( tags, markdown headers) + * - Returns null when no summary is found + * - Handles edge cases like empty input and malformed tags + */ + +import { describe, it, expect } from 'vitest'; + +// Recreate the extractSummary logic from apps/ui/src/lib/log-parser.ts +// We can't import directly because it's a UI file, so we mirror the logic here + +/** + * Cleans up fragmented streaming text by removing spurious newlines + */ +function cleanFragmentedText(content: string): string { + let cleaned = content.replace(/([a-zA-Z])\n+([a-zA-Z])/g, '$1$2'); + cleaned = cleaned.replace(/<([a-zA-Z]+)\n*([a-zA-Z]*)\n*>/g, '<$1$2>'); + cleaned = cleaned.replace(/<\/([a-zA-Z]+)\n*([a-zA-Z]*)\n*>/g, ''); + return cleaned; +} + +/** + * Extracts summary content from raw log output + * Returns the LAST summary text if found, or null if no summary exists + */ +function extractSummary(rawOutput: string): string | null { + if (!rawOutput || !rawOutput.trim()) { + return null; + } + + const cleanedOutput = cleanFragmentedText(rawOutput); + + const regexesToTry: Array<{ + regex: RegExp; + processor: (m: RegExpMatchArray) => string; + }> = [ + { regex: /([\s\S]*?)<\/summary>/gi, processor: (m) => m[1] }, + { regex: /^##\s+Summary[^\n]*\n([\s\S]*?)(?=\n##\s+[^#]|\n🔧|$)/gm, processor: (m) => m[1] }, + { + regex: /^##\s+(Feature|Changes|Implementation)[^\n]*\n([\s\S]*?)(?=\n##\s+[^#]|\n🔧|$)/gm, + processor: (m) => `## ${m[1]}\n${m[2]}`, + }, + { + regex: /(^|\n)(All tasks completed[\s\S]*?)(?=\n🔧|\n📋|\n⚡|\n❌|$)/g, + processor: (m) => m[2], + }, + { + regex: + /(^|\n)((I've|I have) (successfully |now )?(completed|finished|implemented)[\s\S]*?)(?=\n🔧|\n📋|\n⚡|\n❌|$)/g, + processor: (m) => m[2], + }, + ]; + + for (const { regex, processor } of regexesToTry) { + const matches = [...cleanedOutput.matchAll(regex)]; + if (matches.length > 0) { + const lastMatch = matches[matches.length - 1]; + return cleanFragmentedText(processor(lastMatch)).trim(); + } + } + + return null; +} + +describe('log-parser extractSummary (UI fallback)', () => { + describe('basic summary extraction', () => { + it('should extract summary from tags', () => { + const output = ` +Some agent output... + + +## Changes Made +- Fixed the bug in parser.ts +- Added error handling + + +More output... +`; + const result = extractSummary(output); + expect(result).toBe('## Changes Made\n- Fixed the bug in parser.ts\n- Added error handling'); + }); + + it('should prefer tags over markdown headers', () => { + const output = ` +## Summary + +Markdown summary here. + + +XML summary here. + +`; + const result = extractSummary(output); + expect(result).toBe('XML summary here.'); + }); + }); + + describe('multiple summaries (pipeline accumulation scenario)', () => { + it('should return ONLY the LAST summary tag when multiple exist', () => { + // This is the key behavior for pipeline features: + // extractSummary returns only the LAST, which is why server-side + // accumulation is needed for multi-step pipelines + const output = ` +## Step 1: Code Review + + +- Found 3 issues +- Approved with changes + + +--- + +## Step 2: Testing + + +- All tests pass +- Coverage 95% + +`; + const result = extractSummary(output); + expect(result).toBe('- All tests pass\n- Coverage 95%'); + expect(result).not.toContain('Code Review'); + expect(result).not.toContain('Found 3 issues'); + }); + + it('should return ONLY the LAST summary from three pipeline steps', () => { + const output = ` +Step 1 complete + +--- + +Step 2 complete + +--- + +Step 3 complete - all done! +`; + const result = extractSummary(output); + expect(result).toBe('Step 3 complete - all done!'); + expect(result).not.toContain('Step 1'); + expect(result).not.toContain('Step 2'); + }); + + it('should handle mixed summary formats across pipeline steps', () => { + const output = ` +## Step 1 + + +Implementation done + + +--- + +## Step 2 + +## Summary +Review complete + +--- + +## Step 3 + + +All tests passing + +`; + const result = extractSummary(output); + // The tag format takes priority, and returns the LAST match + expect(result).toBe('All tests passing'); + }); + }); + + describe('priority order of summary patterns', () => { + it('should try patterns in priority order: first, then markdown headers', () => { + // When both tags and markdown headers exist, + // tags should take priority + const output = ` +## Summary + +This markdown summary should be ignored. + + +This XML summary should be used. + +`; + const result = extractSummary(output); + expect(result).toBe('This XML summary should be used.'); + expect(result).not.toContain('ignored'); + }); + + it('should fall back to Feature/Changes/Implementation headers when no tag', () => { + // Note: The regex for these headers requires content before the header + // (^ at start or preceded by newline). Adding some content before. + const output = ` +Agent output here... + +## Feature + +New authentication system with OAuth support. + +## Next +`; + const result = extractSummary(output); + // Should find the Feature header and include it in result + // Note: Due to regex behavior, it captures content until next ## + expect(result).toContain('## Feature'); + }); + + it('should fall back to completion phrases when no structured summary found', () => { + const output = ` +Working on the feature... +Making progress... + +All tasks completed successfully. The feature is ready. + +🔧 Tool: Bash +`; + const result = extractSummary(output); + expect(result).toContain('All tasks completed'); + }); + }); + + describe('edge cases', () => { + it('should return null for empty string', () => { + expect(extractSummary('')).toBeNull(); + }); + + it('should return null for whitespace-only string', () => { + expect(extractSummary(' \n\n ')).toBeNull(); + }); + + it('should return null when no summary pattern found', () => { + expect(extractSummary('Random agent output without any summary patterns')).toBeNull(); + }); + + it('should handle malformed tags gracefully', () => { + const output = ` + +This summary is never closed... +`; + // Without closing tag, the regex won't match + expect(extractSummary(output)).toBeNull(); + }); + + it('should handle empty tags', () => { + const output = ` + +`; + const result = extractSummary(output); + expect(result).toBe(''); // Empty string is valid + }); + + it('should handle tags with only whitespace', () => { + const output = ` + + + +`; + const result = extractSummary(output); + expect(result).toBe(''); // Trimmed to empty string + }); + + it('should handle summary with markdown code blocks', () => { + const output = ` + +## Changes + +\`\`\`typescript +const x = 1; +\`\`\` + +Done! + +`; + const result = extractSummary(output); + expect(result).toContain('```typescript'); + expect(result).toContain('const x = 1;'); + }); + + it('should handle summary with special characters', () => { + const output = ` + +Fixed bug in parser.ts: "quotes" and 'apostrophes' +Special chars: <>&$@#%^* + +`; + const result = extractSummary(output); + expect(result).toContain('"quotes"'); + expect(result).toContain('<>&$@#%^*'); + }); + }); + + describe('fragmented streaming text handling', () => { + it('should handle fragmented tags from streaming', () => { + // Sometimes streaming providers split text like "" + const output = ` + +Fixed the issue + +`; + const result = extractSummary(output); + // The cleanFragmentedText function should normalize this + expect(result).toBe('Fixed the issue'); + }); + + it('should handle fragmented text within summary content', () => { + const output = ` + +Fixed the bug in par +ser.ts + +`; + const result = extractSummary(output); + // cleanFragmentedText should join "par\n\nser" into "parser" + expect(result).toBe('Fixed the bug in parser.ts'); + }); + }); + + describe('completion phrase detection', () => { + it('should extract "All tasks completed" summaries', () => { + const output = ` +Some output... + +All tasks completed successfully. The feature is ready for review. + +🔧 Tool: Bash +`; + const result = extractSummary(output); + expect(result).toContain('All tasks completed'); + }); + + it("should extract I've completed summaries", () => { + const output = ` +Working on feature... + +I've successfully implemented the feature with all requirements met. + +🔧 Tool: Read +`; + const result = extractSummary(output); + expect(result).toContain("I've successfully implemented"); + }); + + it('should extract "I have finished" summaries', () => { + const output = ` +Implementation phase... + +I have finished the implementation. + +📋 Planning +`; + const result = extractSummary(output); + expect(result).toContain('I have finished'); + }); + }); + + describe('real-world pipeline scenarios', () => { + it('should handle typical multi-step pipeline output (returns last only)', () => { + // This test documents WHY server-side accumulation is essential: + // extractSummary only returns the last step's summary + const output = ` +📋 Planning Mode: Full + +🔧 Tool: Read +Input: {"file_path": "src/parser.ts"} + + +## Code Review +- Analyzed parser.ts +- Found potential improvements + + +--- + +## Follow-up Session + +🔧 Tool: Edit +Input: {"file_path": "src/parser.ts"} + + +## Implementation +- Applied suggested improvements +- Updated tests + + +--- + +## Follow-up Session + +🔧 Tool: Bash +Input: {"command": "npm test"} + + +## Testing +- All 42 tests pass +- No regressions detected + +`; + const result = extractSummary(output); + // Only the LAST summary is returned + expect(result).toBe('## Testing\n- All 42 tests pass\n- No regressions detected'); + // Earlier summaries are lost + expect(result).not.toContain('Code Review'); + expect(result).not.toContain('Implementation'); + }); + + it('should handle single-step non-pipeline output', () => { + // For non-pipeline features, extractSummary works correctly + const output = ` +Working on feature... + + +## Implementation Complete +- Created new component +- Added unit tests +- Updated documentation + +`; + const result = extractSummary(output); + expect(result).toContain('Implementation Complete'); + expect(result).toContain('Created new component'); + }); + }); +}); + +/** + * These tests verify the UI fallback behavior for summary extraction. + * + * KEY INSIGHT: The extractSummary() function returns only the LAST summary, + * which is why the server-side FeatureStateManager.saveFeatureSummary() method + * accumulates all step summaries into feature.summary. + * + * The UI's AgentOutputModal component uses this priority: + * 1. feature.summary (server-accumulated, contains all steps) + * 2. extractSummary(output) (client-side fallback, last summary only) + * + * For pipeline features, this ensures all step summaries are displayed. + */ diff --git a/apps/server/tests/unit/ui/phase-summary-parser.test.ts b/apps/server/tests/unit/ui/phase-summary-parser.test.ts new file mode 100644 index 00000000..aa7f0533 --- /dev/null +++ b/apps/server/tests/unit/ui/phase-summary-parser.test.ts @@ -0,0 +1,533 @@ +/** + * Unit tests for the UI's log-parser phase summary parsing functions. + * + * These tests verify the behavior of: + * - parsePhaseSummaries(): Parses accumulated summary into individual phases + * - extractPhaseSummary(): Extracts a specific phase's summary + * - extractImplementationSummary(): Extracts only the implementation phase + * - isAccumulatedSummary(): Checks if summary is in accumulated format + * + * The accumulated summary format uses markdown headers with `###` for phase names + * and `---` as separators between phases. + * + * TODO: These test helper functions are mirrored from apps/ui/src/lib/log-parser.ts + * because server-side tests cannot import from the UI module. If the production + * implementation changes, these tests may pass while production fails. + * Consider adding an integration test that validates the actual UI parsing behavior. + */ + +import { describe, it, expect } from 'vitest'; + +// ============================================================================ +// MIRRORED FUNCTIONS from apps/ui/src/lib/log-parser.ts +// ============================================================================ +// NOTE: These functions are mirrored from the UI implementation because +// server-side tests cannot import from apps/ui/. Keep these in sync with the +// production implementation. The UI implementation includes additional +// handling for getPhaseSections/leadingImplementationSection for backward +// compatibility with mixed formats. + +/** + * Parses an accumulated summary string into individual phase summaries. + */ +function parsePhaseSummaries(summary: string | undefined): Map { + const phaseSummaries = new Map(); + + if (!summary || !summary.trim()) { + return phaseSummaries; + } + + // Split by the horizontal rule separator + const sections = summary.split(/\n\n---\n\n/); + + for (const section of sections) { + // Match the phase header pattern: ### Phase Name + const headerMatch = section.match(/^###\s+(.+?)(?:\n|$)/); + if (headerMatch) { + const phaseName = headerMatch[1].trim().toLowerCase(); + // Extract content after the header (skip the header line and leading newlines) + const content = section.substring(headerMatch[0].length).trim(); + phaseSummaries.set(phaseName, content); + } + } + + return phaseSummaries; +} + +/** + * Extracts a specific phase summary from an accumulated summary string. + */ +function extractPhaseSummary(summary: string | undefined, phaseName: string): string | null { + const phaseSummaries = parsePhaseSummaries(summary); + const normalizedPhaseName = phaseName.toLowerCase(); + return phaseSummaries.get(normalizedPhaseName) || null; +} + +/** + * Gets the implementation phase summary from an accumulated summary string. + */ +function extractImplementationSummary(summary: string | undefined): string | null { + if (!summary || !summary.trim()) { + return null; + } + + const phaseSummaries = parsePhaseSummaries(summary); + + // Try exact match first + const implementationContent = phaseSummaries.get('implementation'); + if (implementationContent) { + return implementationContent; + } + + // Fallback: find any phase containing "implement" + for (const [phaseName, content] of phaseSummaries) { + if (phaseName.includes('implement')) { + return content; + } + } + + // If no phase summaries found, the summary might not be in accumulated format + // (legacy or non-pipeline feature). In this case, return the whole summary + // if it looks like a single summary (no phase headers). + if (!summary.includes('### ') && !summary.includes('\n---\n')) { + return summary; + } + + return null; +} + +/** + * Checks if a summary string is in the accumulated multi-phase format. + */ +function isAccumulatedSummary(summary: string | undefined): boolean { + if (!summary || !summary.trim()) { + return false; + } + + // Check for the presence of phase headers with separator + const hasMultiplePhases = + summary.includes('\n\n---\n\n') && summary.match(/###\s+.+/g)?.length > 0; + + return hasMultiplePhases; +} + +describe('phase summary parser', () => { + describe('parsePhaseSummaries', () => { + it('should parse single phase summary', () => { + const summary = `### Implementation + +Created auth module with login functionality.`; + + const result = parsePhaseSummaries(summary); + + expect(result.size).toBe(1); + expect(result.get('implementation')).toBe('Created auth module with login functionality.'); + }); + + it('should parse multiple phase summaries', () => { + const summary = `### Implementation + +Created auth module. + +--- + +### Testing + +All tests pass. + +--- + +### Code Review + +Approved with minor suggestions.`; + + const result = parsePhaseSummaries(summary); + + expect(result.size).toBe(3); + expect(result.get('implementation')).toBe('Created auth module.'); + expect(result.get('testing')).toBe('All tests pass.'); + expect(result.get('code review')).toBe('Approved with minor suggestions.'); + }); + + it('should handle empty input', () => { + expect(parsePhaseSummaries('').size).toBe(0); + expect(parsePhaseSummaries(undefined).size).toBe(0); + expect(parsePhaseSummaries(' \n\n ').size).toBe(0); + }); + + it('should handle phase names with spaces', () => { + const summary = `### Code Review + +Review findings here.`; + + const result = parsePhaseSummaries(summary); + expect(result.get('code review')).toBe('Review findings here.'); + }); + + it('should normalize phase names to lowercase', () => { + const summary = `### IMPLEMENTATION + +Content here.`; + + const result = parsePhaseSummaries(summary); + expect(result.get('implementation')).toBe('Content here.'); + expect(result.get('IMPLEMENTATION')).toBeUndefined(); + }); + + it('should handle content with markdown', () => { + const summary = `### Implementation + +## Changes Made +- Fixed bug in parser.ts +- Added error handling + +\`\`\`typescript +const x = 1; +\`\`\``; + + const result = parsePhaseSummaries(summary); + expect(result.get('implementation')).toContain('## Changes Made'); + expect(result.get('implementation')).toContain('```typescript'); + }); + + it('should return empty map for non-accumulated format', () => { + // Legacy format without phase headers + const summary = `## Summary + +This is a simple summary without phase headers.`; + + const result = parsePhaseSummaries(summary); + expect(result.size).toBe(0); + }); + }); + + describe('extractPhaseSummary', () => { + it('should extract specific phase by name (case-insensitive)', () => { + const summary = `### Implementation + +Implementation content. + +--- + +### Testing + +Testing content.`; + + expect(extractPhaseSummary(summary, 'implementation')).toBe('Implementation content.'); + expect(extractPhaseSummary(summary, 'IMPLEMENTATION')).toBe('Implementation content.'); + expect(extractPhaseSummary(summary, 'Implementation')).toBe('Implementation content.'); + expect(extractPhaseSummary(summary, 'testing')).toBe('Testing content.'); + }); + + it('should return null for non-existent phase', () => { + const summary = `### Implementation + +Content here.`; + + expect(extractPhaseSummary(summary, 'code review')).toBeNull(); + }); + + it('should return null for empty input', () => { + expect(extractPhaseSummary('', 'implementation')).toBeNull(); + expect(extractPhaseSummary(undefined, 'implementation')).toBeNull(); + }); + }); + + describe('extractImplementationSummary', () => { + it('should extract implementation phase from accumulated summary', () => { + const summary = `### Implementation + +Created auth module. + +--- + +### Testing + +All tests pass. + +--- + +### Code Review + +Approved.`; + + const result = extractImplementationSummary(summary); + expect(result).toBe('Created auth module.'); + expect(result).not.toContain('Testing'); + expect(result).not.toContain('Code Review'); + }); + + it('should return implementation phase even when not first', () => { + const summary = `### Planning + +Plan created. + +--- + +### Implementation + +Implemented the feature. + +--- + +### Review + +Reviewed.`; + + const result = extractImplementationSummary(summary); + expect(result).toBe('Implemented the feature.'); + }); + + it('should handle phase with "implementation" in name', () => { + const summary = `### Feature Implementation + +Built the feature.`; + + const result = extractImplementationSummary(summary); + expect(result).toBe('Built the feature.'); + }); + + it('should return full summary for non-accumulated format (legacy)', () => { + // Non-pipeline features store summary without phase headers + const summary = `## Changes +- Fixed bug +- Added tests`; + + const result = extractImplementationSummary(summary); + expect(result).toBe(summary); + }); + + it('should return null for empty input', () => { + expect(extractImplementationSummary('')).toBeNull(); + expect(extractImplementationSummary(undefined)).toBeNull(); + expect(extractImplementationSummary(' \n\n ')).toBeNull(); + }); + + it('should return null when no implementation phase in accumulated summary', () => { + const summary = `### Testing + +Tests written. + +--- + +### Code Review + +Approved.`; + + const result = extractImplementationSummary(summary); + expect(result).toBeNull(); + }); + }); + + describe('isAccumulatedSummary', () => { + it('should return true for accumulated multi-phase summary', () => { + const summary = `### Implementation + +Content. + +--- + +### Testing + +Content.`; + + expect(isAccumulatedSummary(summary)).toBe(true); + }); + + it('should return false for single phase summary (no separator)', () => { + const summary = `### Implementation + +Content.`; + + expect(isAccumulatedSummary(summary)).toBe(false); + }); + + it('should return false for legacy non-accumulated format', () => { + const summary = `## Summary + +This is a simple summary.`; + + expect(isAccumulatedSummary(summary)).toBe(false); + }); + + it('should return false for empty input', () => { + expect(isAccumulatedSummary('')).toBe(false); + expect(isAccumulatedSummary(undefined)).toBe(false); + expect(isAccumulatedSummary(' \n\n ')).toBe(false); + }); + + it('should return true even for two phases', () => { + const summary = `### Implementation + +Content A. + +--- + +### Code Review + +Content B.`; + + expect(isAccumulatedSummary(summary)).toBe(true); + }); + }); + + describe('acceptance criteria scenarios', () => { + it('AC1: Implementation summary preserved when Testing completes', () => { + // Given a task card completes the Implementation phase, + // when the Testing phase subsequently completes, + // then the Implementation phase summary must remain stored independently + const summary = `### Implementation + +- Created auth module +- Added user service + +--- + +### Testing + +- 42 tests pass +- 98% coverage`; + + const impl = extractImplementationSummary(summary); + const testing = extractPhaseSummary(summary, 'testing'); + + expect(impl).toBe('- Created auth module\n- Added user service'); + expect(testing).toBe('- 42 tests pass\n- 98% coverage'); + expect(impl).not.toContain('Testing'); + expect(testing).not.toContain('auth module'); + }); + + it('AC4: Implementation Summary tab shows only implementation phase', () => { + // Given a task card has completed the Implementation phase + // (regardless of how many subsequent phases have run), + // when the user opens the "Implementation Summary" tab, + // then it must display only the summary produced by the Implementation phase + const summary = `### Implementation + +Implementation phase output here. + +--- + +### Testing + +Testing phase output here. + +--- + +### Code Review + +Code review output here.`; + + const impl = extractImplementationSummary(summary); + + expect(impl).toBe('Implementation phase output here.'); + expect(impl).not.toContain('Testing'); + expect(impl).not.toContain('Code Review'); + }); + + it('AC5: Empty state when implementation not started', () => { + // Given a task card has not yet started the Implementation phase + const summary = `### Planning + +Planning phase complete.`; + + const impl = extractImplementationSummary(summary); + + // Should return null (UI shows "No implementation summary available") + expect(impl).toBeNull(); + }); + + it('AC6: Single phase summary displayed correctly', () => { + // Given a task card where Implementation was the only completed phase + const summary = `### Implementation + +Only implementation was done.`; + + const impl = extractImplementationSummary(summary); + + expect(impl).toBe('Only implementation was done.'); + }); + + it('AC9: Mid-progress shows only completed phases', () => { + // Given a task card is mid-progress + // (e.g., Implementation and Testing complete, Code Review pending) + const summary = `### Implementation + +Implementation done. + +--- + +### Testing + +Testing done.`; + + const phases = parsePhaseSummaries(summary); + + expect(phases.size).toBe(2); + expect(phases.has('implementation')).toBe(true); + expect(phases.has('testing')).toBe(true); + expect(phases.has('code review')).toBe(false); + }); + + it('AC10: All phases in chronological order', () => { + // Given all phases of a task card are complete + const summary = `### Implementation + +First phase content. + +--- + +### Testing + +Second phase content. + +--- + +### Code Review + +Third phase content.`; + + // ParsePhaseSummaries should preserve order + const phases = parsePhaseSummaries(summary); + const phaseNames = [...phases.keys()]; + + expect(phaseNames).toEqual(['implementation', 'testing', 'code review']); + }); + + it('AC17: Retried phase shows only latest', () => { + // Given a phase was retried, when viewing the Summary tab, + // only one entry for the retried phase must appear (the latest retry's summary) + // + // Note: The server-side FeatureStateManager overwrites the phase summary + // when the same phase runs again, so we only have one entry per phase name. + // This test verifies that the parser correctly handles this. + const summary = `### Implementation + +First attempt content. + +--- + +### Testing + +First test run. + +--- + +### Implementation + +Retry content - fixed issues. + +--- + +### Testing + +Retry - all tests now pass.`; + + const phases = parsePhaseSummaries(summary); + + // The parser will have both entries, but Map keeps last value for same key + expect(phases.get('implementation')).toBe('Retry content - fixed issues.'); + expect(phases.get('testing')).toBe('Retry - all tests now pass.'); + }); + }); +}); diff --git a/apps/server/tests/unit/ui/summary-auto-scroll.test.ts b/apps/server/tests/unit/ui/summary-auto-scroll.test.ts new file mode 100644 index 00000000..544e88a0 --- /dev/null +++ b/apps/server/tests/unit/ui/summary-auto-scroll.test.ts @@ -0,0 +1,238 @@ +/** + * Unit tests for the summary auto-scroll detection logic. + * + * These tests verify the behavior of the scroll detection function used in + * AgentOutputModal to determine if auto-scroll should be enabled. + * + * The logic mirrors the handleSummaryScroll function in: + * apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx + * + * Auto-scroll behavior: + * - When user is at or near the bottom (< 50px from bottom), auto-scroll is enabled + * - When user scrolls up to view older content, auto-scroll is disabled + * - Scrolling back to bottom re-enables auto-scroll + */ + +import { describe, it, expect } from 'vitest'; + +/** + * Determines if the scroll position is at the bottom of the container. + * This is the core logic from handleSummaryScroll in AgentOutputModal. + * + * @param scrollTop - Current scroll position from top + * @param scrollHeight - Total scrollable height + * @param clientHeight - Visible height of the container + * @param threshold - Distance from bottom to consider "at bottom" (default: 50px) + * @returns true if at bottom, false otherwise + */ +function isScrollAtBottom( + scrollTop: number, + scrollHeight: number, + clientHeight: number, + threshold = 50 +): boolean { + const distanceFromBottom = scrollHeight - scrollTop - clientHeight; + return distanceFromBottom < threshold; +} + +describe('Summary Auto-Scroll Detection Logic', () => { + describe('basic scroll position detection', () => { + it('should return true when scrolled to exact bottom', () => { + // Container: 500px tall, content: 1000px tall + // ScrollTop: 500 (scrolled to bottom) + const result = isScrollAtBottom(500, 1000, 500); + expect(result).toBe(true); + }); + + it('should return true when near bottom (within threshold)', () => { + // 49px from bottom - within 50px threshold + const result = isScrollAtBottom(451, 1000, 500); + expect(result).toBe(true); + }); + + it('should return true when exactly at threshold boundary (49px)', () => { + // 49px from bottom + const result = isScrollAtBottom(451, 1000, 500); + expect(result).toBe(true); + }); + + it('should return false when just outside threshold (51px)', () => { + // 51px from bottom - outside 50px threshold + const result = isScrollAtBottom(449, 1000, 500); + expect(result).toBe(false); + }); + + it('should return false when scrolled to top', () => { + const result = isScrollAtBottom(0, 1000, 500); + expect(result).toBe(false); + }); + + it('should return false when scrolled to middle', () => { + const result = isScrollAtBottom(250, 1000, 500); + expect(result).toBe(false); + }); + }); + + describe('edge cases with small content', () => { + it('should return true when content fits in viewport (no scroll needed)', () => { + // Content is smaller than container - no scrolling possible + const result = isScrollAtBottom(0, 300, 500); + expect(result).toBe(true); + }); + + it('should return true when content exactly fits viewport', () => { + const result = isScrollAtBottom(0, 500, 500); + expect(result).toBe(true); + }); + + it('should return true when content slightly exceeds viewport (within threshold)', () => { + // Content: 540px, Viewport: 500px, can scroll 40px + // At scroll 0, we're 40px from bottom - within threshold + const result = isScrollAtBottom(0, 540, 500); + expect(result).toBe(true); + }); + }); + + describe('large content scenarios', () => { + it('should correctly detect bottom in very long content', () => { + // Simulate accumulated summary from many pipeline steps + // Content: 10000px, Viewport: 500px + const result = isScrollAtBottom(9500, 10000, 500); + expect(result).toBe(true); + }); + + it('should correctly detect non-bottom in very long content', () => { + // User scrolled up to read earlier summaries + const result = isScrollAtBottom(5000, 10000, 500); + expect(result).toBe(false); + }); + + it('should detect when user scrolls up from bottom', () => { + // Started at bottom (scroll: 9500), then scrolled up 100px + const result = isScrollAtBottom(9400, 10000, 500); + expect(result).toBe(false); + }); + }); + + describe('custom threshold values', () => { + it('should work with larger threshold (100px)', () => { + // 75px from bottom - within 100px threshold + const result = isScrollAtBottom(425, 1000, 500, 100); + expect(result).toBe(true); + }); + + it('should work with smaller threshold (10px)', () => { + // 15px from bottom - outside 10px threshold + const result = isScrollAtBottom(485, 1000, 500, 10); + expect(result).toBe(false); + }); + + it('should work with zero threshold (exact match only)', () => { + // At exact bottom - distanceFromBottom = 0, which is NOT < 0 with strict comparison + // This is an edge case: the implementation uses < not <= + const result = isScrollAtBottom(500, 1000, 500, 0); + expect(result).toBe(false); // 0 < 0 is false + + // 1px from bottom - also fails + const result2 = isScrollAtBottom(499, 1000, 500, 0); + expect(result2).toBe(false); + + // For exact match with 0 threshold, we need negative distanceFromBottom + // which happens when scrollTop > scrollHeight - clientHeight (overscroll) + const result3 = isScrollAtBottom(501, 1000, 500, 0); + expect(result3).toBe(true); // -1 < 0 is true + }); + }); + + describe('pipeline summary scrolling scenarios', () => { + it('should enable auto-scroll when new content arrives while at bottom', () => { + // User is at bottom viewing step 2 summary + // Step 3 summary is added, increasing scrollHeight from 1000 to 1500 + // ScrollTop stays at 950 (was at bottom), but now user needs to scroll + + // Before new content: isScrollAtBottom(950, 1000, 500) = true + // After new content: auto-scroll should kick in to scroll to new bottom + + // Simulating the auto-scroll effect setting scrollTop to new bottom + const newScrollTop = 1500 - 500; // scrollHeight - clientHeight + const result = isScrollAtBottom(newScrollTop, 1500, 500); + expect(result).toBe(true); + }); + + it('should not auto-scroll when user is reading earlier summaries', () => { + // User scrolled up to read step 1 summary while step 3 is added + // scrollHeight increases, but scrollTop stays same + // User is now further from bottom + + // User was at scroll position 200 (reading early content) + // New content increases scrollHeight from 1000 to 1500 + // Distance from bottom goes from 300 to 800 + const result = isScrollAtBottom(200, 1500, 500); + expect(result).toBe(false); + }); + + it('should re-enable auto-scroll when user scrolls back to bottom', () => { + // User was reading step 1 (scrollTop: 200) + // User scrolls back to bottom to see latest content + const result = isScrollAtBottom(1450, 1500, 500); + expect(result).toBe(true); + }); + }); + + describe('decimal scroll values', () => { + it('should handle fractional scroll positions', () => { + // Browsers can report fractional scroll values + const result = isScrollAtBottom(499.5, 1000, 500); + expect(result).toBe(true); + }); + + it('should handle fractional scroll heights', () => { + const result = isScrollAtBottom(450.7, 1000.3, 500); + expect(result).toBe(true); + }); + }); + + describe('negative and invalid inputs', () => { + it('should handle negative scrollTop (bounce scroll)', () => { + // iOS can report negative scrollTop during bounce + const result = isScrollAtBottom(-10, 1000, 500); + expect(result).toBe(false); + }); + + it('should handle zero scrollHeight', () => { + // Empty content + const result = isScrollAtBottom(0, 0, 500); + expect(result).toBe(true); + }); + + it('should handle zero clientHeight', () => { + // Hidden container - distanceFromBottom = 1000 - 0 - 0 = 1000 + // This is not < threshold, so returns false + // This edge case represents a broken/invisible container + const result = isScrollAtBottom(0, 1000, 0); + expect(result).toBe(false); + }); + }); + + describe('real-world accumulated summary dimensions', () => { + it('should handle typical 3-step pipeline summary dimensions', () => { + // Approximate: 3 steps x ~800px each = ~2400px + // Viewport: 400px (modal height) + const result = isScrollAtBottom(2000, 2400, 400); + expect(result).toBe(true); + }); + + it('should handle large 10-step pipeline summary dimensions', () => { + // Approximate: 10 steps x ~800px each = ~8000px + // Viewport: 400px + const result = isScrollAtBottom(7600, 8000, 400); + expect(result).toBe(true); + }); + + it('should detect scroll to top of large summary', () => { + // User at top of 10-step summary + const result = isScrollAtBottom(0, 8000, 400); + expect(result).toBe(false); + }); + }); +}); diff --git a/apps/server/tests/unit/ui/summary-normalization.test.ts b/apps/server/tests/unit/ui/summary-normalization.test.ts new file mode 100644 index 00000000..999a4f35 --- /dev/null +++ b/apps/server/tests/unit/ui/summary-normalization.test.ts @@ -0,0 +1,128 @@ +/** + * Unit tests for summary normalization between UI components and parser functions. + * + * These tests verify that: + * - getFirstNonEmptySummary returns string | null + * - parseAllPhaseSummaries and isAccumulatedSummary expect string | undefined + * - The normalization (summary ?? undefined) correctly converts null to undefined + * + * This ensures the UI components properly bridge the type gap between: + * - getFirstNonEmptySummary (returns string | null) + * - parseAllPhaseSummaries (expects string | undefined) + * - isAccumulatedSummary (expects string | undefined) + */ + +import { describe, it, expect } from 'vitest'; +import { parseAllPhaseSummaries, isAccumulatedSummary } from '../../../../ui/src/lib/log-parser.ts'; +import { getFirstNonEmptySummary } from '../../../../ui/src/lib/summary-selection.ts'; + +describe('Summary Normalization', () => { + describe('getFirstNonEmptySummary', () => { + it('should return the first non-empty string', () => { + const result = getFirstNonEmptySummary(null, undefined, 'valid summary', 'another'); + expect(result).toBe('valid summary'); + }); + + it('should return null when all candidates are empty', () => { + const result = getFirstNonEmptySummary(null, undefined, '', ' '); + expect(result).toBeNull(); + }); + + it('should return null when no candidates provided', () => { + const result = getFirstNonEmptySummary(); + expect(result).toBeNull(); + }); + + it('should return null for all null/undefined candidates', () => { + const result = getFirstNonEmptySummary(null, undefined, null); + expect(result).toBeNull(); + }); + + it('should preserve original string formatting (not trim)', () => { + const result = getFirstNonEmptySummary(' summary with spaces '); + expect(result).toBe(' summary with spaces '); + }); + }); + + describe('parseAllPhaseSummaries with normalized input', () => { + it('should handle null converted to undefined via ?? operator', () => { + const summary = getFirstNonEmptySummary(null, undefined); + // This is the normalization: summary ?? undefined + const normalizedSummary = summary ?? undefined; + + // TypeScript should accept this without error + const result = parseAllPhaseSummaries(normalizedSummary); + expect(result).toEqual([]); + }); + + it('should parse accumulated summary when non-null is normalized', () => { + const rawSummary = + '### Implementation\n\nDid some work\n\n---\n\n### Testing\n\nAll tests pass'; + const summary = getFirstNonEmptySummary(null, rawSummary); + const normalizedSummary = summary ?? undefined; + + const result = parseAllPhaseSummaries(normalizedSummary); + expect(result).toHaveLength(2); + expect(result[0].phaseName).toBe('Implementation'); + expect(result[1].phaseName).toBe('Testing'); + }); + }); + + describe('isAccumulatedSummary with normalized input', () => { + it('should return false for null converted to undefined', () => { + const summary = getFirstNonEmptySummary(null, undefined); + const normalizedSummary = summary ?? undefined; + + const result = isAccumulatedSummary(normalizedSummary); + expect(result).toBe(false); + }); + + it('should return true for valid accumulated summary after normalization', () => { + const rawSummary = + '### Implementation\n\nDid some work\n\n---\n\n### Testing\n\nAll tests pass'; + const summary = getFirstNonEmptySummary(rawSummary); + const normalizedSummary = summary ?? undefined; + + const result = isAccumulatedSummary(normalizedSummary); + expect(result).toBe(true); + }); + + it('should return false for single-phase summary after normalization', () => { + const rawSummary = '### Implementation\n\nDid some work'; + const summary = getFirstNonEmptySummary(rawSummary); + const normalizedSummary = summary ?? undefined; + + const result = isAccumulatedSummary(normalizedSummary); + expect(result).toBe(false); + }); + }); + + describe('Type safety verification', () => { + it('should demonstrate that null must be normalized to undefined', () => { + // This test documents the type mismatch that requires normalization + const summary: string | null = getFirstNonEmptySummary(null); + const normalizedSummary: string | undefined = summary ?? undefined; + + // parseAllPhaseSummaries expects string | undefined, not string | null + // The normalization converts null -> undefined, which is compatible + const result = parseAllPhaseSummaries(normalizedSummary); + expect(result).toEqual([]); + }); + + it('should work with the actual usage pattern from components', () => { + // Simulates the actual pattern used in summary-dialog.tsx and agent-output-modal.tsx + const featureSummary: string | null | undefined = null; + const extractedSummary: string | null | undefined = undefined; + + const rawSummary = getFirstNonEmptySummary(featureSummary, extractedSummary); + const normalizedSummary = rawSummary ?? undefined; + + // Both parser functions should work with the normalized value + const phases = parseAllPhaseSummaries(normalizedSummary); + const hasMultiple = isAccumulatedSummary(normalizedSummary); + + expect(phases).toEqual([]); + expect(hasMultiple).toBe(false); + }); + }); +}); diff --git a/apps/server/tests/unit/ui/summary-source-flow.integration.test.ts b/apps/server/tests/unit/ui/summary-source-flow.integration.test.ts new file mode 100644 index 00000000..6f268d4a --- /dev/null +++ b/apps/server/tests/unit/ui/summary-source-flow.integration.test.ts @@ -0,0 +1,108 @@ +import { describe, it, expect } from 'vitest'; +import { parseAllPhaseSummaries, isAccumulatedSummary } from '../../../../ui/src/lib/log-parser.ts'; +import { getFirstNonEmptySummary } from '../../../../ui/src/lib/summary-selection.ts'; + +/** + * Mirrors summary source priority in agent-info-panel.tsx: + * freshFeature.summary > feature.summary > summaryProp > agentInfo.summary + */ +function getCardEffectiveSummary(params: { + freshFeatureSummary?: string | null; + featureSummary?: string | null; + summaryProp?: string | null; + agentInfoSummary?: string | null; +}): string | undefined | null { + return getFirstNonEmptySummary( + params.freshFeatureSummary, + params.featureSummary, + params.summaryProp, + params.agentInfoSummary + ); +} + +/** + * Mirrors SummaryDialog raw summary selection in summary-dialog.tsx: + * summaryProp > feature.summary > agentInfo.summary + */ +function getDialogRawSummary(params: { + summaryProp?: string | null; + featureSummary?: string | null; + agentInfoSummary?: string | null; +}): string | undefined | null { + return getFirstNonEmptySummary( + params.summaryProp, + params.featureSummary, + params.agentInfoSummary + ); +} + +describe('Summary Source Flow Integration', () => { + it('uses fresh per-feature summary in card and preserves it through summary dialog', () => { + const staleListSummary = '## Old summary from stale list cache'; + const freshAccumulatedSummary = `### Implementation + +Implemented auth + profile flow. + +--- + +### Testing + +- Unit tests: 18 passed +- Integration tests: 6 passed`; + const parsedAgentInfoSummary = 'Fallback summary from parsed agent output'; + + const cardEffectiveSummary = getCardEffectiveSummary({ + freshFeatureSummary: freshAccumulatedSummary, + featureSummary: staleListSummary, + summaryProp: undefined, + agentInfoSummary: parsedAgentInfoSummary, + }); + + expect(cardEffectiveSummary).toBe(freshAccumulatedSummary); + + const dialogRawSummary = getDialogRawSummary({ + summaryProp: cardEffectiveSummary, + featureSummary: staleListSummary, + agentInfoSummary: parsedAgentInfoSummary, + }); + + expect(dialogRawSummary).toBe(freshAccumulatedSummary); + expect(isAccumulatedSummary(dialogRawSummary ?? undefined)).toBe(true); + + const phases = parseAllPhaseSummaries(dialogRawSummary ?? undefined); + expect(phases).toHaveLength(2); + expect(phases[0]?.phaseName).toBe('Implementation'); + expect(phases[1]?.phaseName).toBe('Testing'); + }); + + it('falls back in order when fresher sources are absent', () => { + const cardEffectiveSummary = getCardEffectiveSummary({ + freshFeatureSummary: undefined, + featureSummary: '', + summaryProp: undefined, + agentInfoSummary: 'Agent parsed fallback', + }); + + expect(cardEffectiveSummary).toBe('Agent parsed fallback'); + + const dialogRawSummary = getDialogRawSummary({ + summaryProp: undefined, + featureSummary: undefined, + agentInfoSummary: cardEffectiveSummary, + }); + + expect(dialogRawSummary).toBe('Agent parsed fallback'); + expect(isAccumulatedSummary(dialogRawSummary ?? undefined)).toBe(false); + }); + + it('treats whitespace-only summaries as empty during fallback selection', () => { + const cardEffectiveSummary = getCardEffectiveSummary({ + freshFeatureSummary: ' \n', + featureSummary: '\t', + summaryProp: ' ', + agentInfoSummary: 'Agent parsed fallback', + }); + + expect(cardEffectiveSummary).toBe('Agent parsed fallback'); + }); +}); diff --git a/apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx b/apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx index 86cb6fa4..f3da1525 100644 --- a/apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx +++ b/apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx @@ -12,6 +12,7 @@ import { SummaryDialog } from './summary-dialog'; import { getProviderIconForModel } from '@/components/ui/provider-icon'; import { useFeature, useAgentOutput } from '@/hooks/queries'; import { queryKeys } from '@/lib/query-keys'; +import { getFirstNonEmptySummary } from '@/lib/summary-selection'; /** * Formats thinking level for compact display @@ -67,6 +68,8 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({ const [taskStatusMap, setTaskStatusMap] = useState< Map >(new Map()); + // Track real-time task summary updates from WebSocket events + const [taskSummaryMap, setTaskSummaryMap] = useState>(new Map()); // Track last WebSocket event timestamp to know if we're receiving real-time updates const [lastWsEventTimestamp, setLastWsEventTimestamp] = useState(null); @@ -163,6 +166,11 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({ return null; }, [contextContent, agentOutputContent]); + // Prefer freshly fetched feature summary over potentially stale list data. + const effectiveSummary = + getFirstNonEmptySummary(freshFeature?.summary, feature.summary, summary, agentInfo?.summary) ?? + undefined; + // Fresh planSpec data from API (more accurate than store data for task progress) const freshPlanSpec = useMemo(() => { if (!freshFeature?.planSpec) return null; @@ -197,11 +205,13 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({ return { content: task.description, status: (finalStatus || 'completed') as 'pending' | 'in_progress' | 'completed', + summary: task.summary, }; } // Use real-time status from WebSocket events if available const realtimeStatus = taskStatusMap.get(task.id); + const realtimeSummary = taskSummaryMap.get(task.id); // Calculate status: WebSocket status > index-based status > task.status let effectiveStatus: 'pending' | 'in_progress' | 'completed'; @@ -224,6 +234,7 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({ return { content: task.description, status: effectiveStatus, + summary: realtimeSummary ?? task.summary, }; }); } @@ -236,6 +247,7 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({ feature.planSpec?.currentTaskId, agentInfo?.todos, taskStatusMap, + taskSummaryMap, isFeatureFinished, ]); @@ -280,6 +292,19 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({ newMap.set(taskEvent.taskId, 'completed'); return newMap; }); + + if ('summary' in event) { + setTaskSummaryMap((prev) => { + const newMap = new Map(prev); + // Allow empty string (reset) or non-empty string to be set + const summary = + typeof event.summary === 'string' && event.summary.trim().length > 0 + ? event.summary + : null; + newMap.set(taskEvent.taskId, summary); + return newMap; + }); + } } break; } @@ -331,7 +356,13 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({ // OR if the feature is actively running (ensures panel stays visible during execution) // Note: hasPlanSpecTasks is already defined above and includes freshPlanSpec // (The backlog case was already handled above and returned early) - if (agentInfo || hasPlanSpecTasks || effectiveTodos.length > 0 || isActivelyRunning) { + if ( + agentInfo || + hasPlanSpecTasks || + effectiveTodos.length > 0 || + isActivelyRunning || + effectiveSummary + ) { return ( <>
@@ -379,24 +410,31 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({ > {(isTodosExpanded ? effectiveTodos : effectiveTodos.slice(0, 3)).map( (todo, idx) => ( -
- {todo.status === 'completed' ? ( - - ) : todo.status === 'in_progress' ? ( - - ) : ( - - )} - +
+ {todo.status === 'completed' ? ( + + ) : todo.status === 'in_progress' ? ( + + ) : ( + )} - > - {todo.content} - + + {todo.content} + +
+ {todo.summary && isTodosExpanded && ( +
+ {todo.summary} +
+ )}
) )} @@ -417,10 +455,12 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({
)} - {/* Summary for waiting_approval and verified */} - {(feature.status === 'waiting_approval' || feature.status === 'verified') && ( - <> - {(feature.summary || summary || agentInfo?.summary) && ( + {/* Summary for waiting_approval, verified, and pipeline steps */} + {(feature.status === 'waiting_approval' || + feature.status === 'verified' || + (typeof feature.status === 'string' && feature.status.startsWith('pipeline_'))) && ( +
+ {effectiveSummary && (
@@ -446,37 +486,35 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({ onPointerDown={(e) => e.stopPropagation()} onMouseDown={(e) => e.stopPropagation()} > - {feature.summary || summary || agentInfo?.summary} + {effectiveSummary}

)} - {!feature.summary && - !summary && - !agentInfo?.summary && - (agentInfo?.toolCallCount ?? 0) > 0 && ( -
+ {!effectiveSummary && (agentInfo?.toolCallCount ?? 0) > 0 && ( +
+ + + {agentInfo?.toolCallCount ?? 0} tool calls + + {effectiveTodos.length > 0 && ( - - {agentInfo?.toolCallCount ?? 0} tool calls + + {effectiveTodos.filter((t) => t.status === 'completed').length} tasks done - {effectiveTodos.length > 0 && ( - - - {effectiveTodos.filter((t) => t.status === 'completed').length} tasks done - - )} -
- )} - + )} +
+ )} +
)}
{/* SummaryDialog must be rendered alongside the expand button */} ); @@ -488,9 +526,10 @@ export const AgentInfoPanel = memo(function AgentInfoPanel({ ); }); diff --git a/apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx b/apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx index 1fed1310..7b6629cf 100644 --- a/apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx +++ b/apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx @@ -1,6 +1,13 @@ -// @ts-nocheck - dialog state typing with feature summary extraction -import { Feature } from '@/store/app-store'; -import { AgentTaskInfo } from '@/lib/agent-context-parser'; +import { useMemo, useState, useRef, useEffect } from 'react'; +import type { Feature } from '@/store/app-store'; +import type { AgentTaskInfo } from '@/lib/agent-context-parser'; +import { + parseAllPhaseSummaries, + isAccumulatedSummary, + type PhaseSummaryEntry, +} from '@/lib/log-parser'; +import { getFirstNonEmptySummary } from '@/lib/summary-selection'; +import { useAgentOutput } from '@/hooks/queries'; import { Dialog, DialogContent, @@ -11,7 +18,10 @@ import { } from '@/components/ui/dialog'; import { Button } from '@/components/ui/button'; import { Markdown } from '@/components/ui/markdown'; -import { Sparkles } from 'lucide-react'; +import { LogViewer } from '@/components/ui/log-viewer'; +import { Sparkles, Layers, FileText, ChevronLeft, ChevronRight } from 'lucide-react'; +import { Spinner } from '@/components/ui/spinner'; +import { cn } from '@/lib/utils'; interface SummaryDialogProps { feature: Feature; @@ -19,6 +29,118 @@ interface SummaryDialogProps { summary?: string; isOpen: boolean; onOpenChange: (open: boolean) => void; + projectPath?: string; +} + +type ViewMode = 'summary' | 'output'; + +/** + * Renders a single phase entry card with header and content. + * Extracted for better separation of concerns and readability. + */ +function PhaseEntryCard({ + entry, + index, + totalPhases, + hasMultiplePhases, + isActive, + onClick, +}: { + entry: PhaseSummaryEntry; + index: number; + totalPhases: number; + hasMultiplePhases: boolean; + isActive?: boolean; + onClick?: () => void; +}) { + const handleKeyDown = (event: React.KeyboardEvent) => { + if (onClick && (event.key === 'Enter' || event.key === ' ')) { + event.preventDefault(); + onClick(); + } + }; + + return ( +
+ {/* Phase header - styled to stand out */} +
+ {entry.phaseName} + {hasMultiplePhases && ( + + Step {index + 1} of {totalPhases} + + )} +
+ {/* Phase content */} + {entry.content || 'No summary available'} +
+ ); +} + +/** + * Step navigator component for multi-phase summaries + */ +function StepNavigator({ + phaseEntries, + activeIndex, + onIndexChange, +}: { + phaseEntries: PhaseSummaryEntry[]; + activeIndex: number; + onIndexChange: (index: number) => void; +}) { + if (phaseEntries.length <= 1) return null; + + return ( +
+ + +
+ {phaseEntries.map((entry, index) => ( + + ))} +
+ + +
+ ); } export function SummaryDialog({ @@ -27,7 +149,63 @@ export function SummaryDialog({ summary, isOpen, onOpenChange, + projectPath, }: SummaryDialogProps) { + const [viewMode, setViewMode] = useState('summary'); + const [activePhaseIndex, setActivePhaseIndex] = useState(0); + const contentRef = useRef(null); + + // Prefer explicitly provided summary (can come from fresh per-feature query), + // then fall back to feature/agent-info summaries. + const rawSummary = getFirstNonEmptySummary(summary, feature.summary, agentInfo?.summary); + + // Normalize null to undefined for parser helpers that expect string | undefined + const normalizedSummary = rawSummary ?? undefined; + + // Memoize the parsed phases to avoid re-parsing on every render + const phaseEntries = useMemo( + () => parseAllPhaseSummaries(normalizedSummary), + [normalizedSummary] + ); + + // Memoize the multi-phase check + const hasMultiplePhases = useMemo( + () => isAccumulatedSummary(normalizedSummary), + [normalizedSummary] + ); + + // Fetch agent output + const { data: agentOutput = '', isLoading: isLoadingOutput } = useAgentOutput( + projectPath || '', + feature.id, + { + enabled: isOpen && !!projectPath && viewMode === 'output', + } + ); + + // Reset active phase index when summary changes + useEffect(() => { + setActivePhaseIndex(0); + }, [normalizedSummary]); + + // Scroll to active phase when it changes or when normalizedSummary changes + useEffect(() => { + if (contentRef.current && hasMultiplePhases) { + const phaseCards = contentRef.current.querySelectorAll('[data-phase-index]'); + // Ensure index is within bounds + const safeIndex = Math.min(activePhaseIndex, phaseCards.length - 1); + const targetCard = phaseCards[safeIndex]; + if (targetCard) { + targetCard.scrollIntoView({ behavior: 'smooth', block: 'start' }); + } + } + }, [activePhaseIndex, hasMultiplePhases, normalizedSummary]); + + // Determine the dialog title based on number of phases + const dialogTitle = hasMultiplePhases + ? `Pipeline Summary (${phaseEntries.length} steps)` + : 'Implementation Summary'; + return ( e.stopPropagation()} > - - - Implementation Summary - +
+ + {hasMultiplePhases ? ( + + ) : ( + + )} + {dialogTitle} + + + {/* View mode tabs */} +
+ + +
+
-
- - {feature.summary || summary || agentInfo?.summary || 'No summary available'} - -
+ + {/* Step navigator for multi-phase summaries */} + {viewMode === 'summary' && hasMultiplePhases && ( + + )} + + {/* Content area */} + {viewMode === 'summary' ? ( +
+ {phaseEntries.length > 0 ? ( + phaseEntries.map((entry, index) => ( +
+ setActivePhaseIndex(index) : undefined} + /> +
+ )) + ) : ( +
+ No summary available +
+ )} +
+ ) : ( +
+ {isLoadingOutput ? ( +
+ + Loading output... +
+ ) : !agentOutput ? ( +
+ No agent output available. +
+ ) : ( + + )} +
+ )} + + +
+ {phaseEntries.map((entry, index) => ( + + ))} +
+ + +
+ ); +} + export function AgentOutputModal({ open, onClose, @@ -56,10 +170,19 @@ export function AgentOutputModal({ const [viewMode, setViewMode] = useState(null); // Use React Query for initial output loading - const { data: initialOutput = '', isLoading } = useAgentOutput(resolvedProjectPath, featureId, { + const { + data: initialOutput = '', + isLoading, + refetch: refetchAgentOutput, + } = useAgentOutput(resolvedProjectPath, featureId, { enabled: open && !!resolvedProjectPath, }); + // Fetch feature data to access the server-side accumulated summary + const { data: feature, refetch: refetchFeature } = useFeature(resolvedProjectPath, featureId, { + enabled: open && !!resolvedProjectPath && !isBacklogPlan, + }); + // Reset streamed content when modal opens or featureId changes useEffect(() => { if (open) { @@ -70,8 +193,31 @@ export function AgentOutputModal({ // Combine initial output from query with streamed content from WebSocket const output = initialOutput + streamedContent; - // Extract summary from output - const summary = useMemo(() => extractSummary(output), [output]); + // Extract summary from output (client-side fallback) + const extractedSummary = useMemo(() => extractSummary(output), [output]); + + // Prefer server-side accumulated summary (handles pipeline step accumulation), + // fall back to client-side extraction from raw output. + const summary = getFirstNonEmptySummary(feature?.summary, extractedSummary); + + // Normalize null to undefined for parser helpers that expect string | undefined + const normalizedSummary = summary ?? undefined; + + // Parse summary into phases for multi-step navigation + const phaseEntries = useMemo( + () => parseAllPhaseSummaries(normalizedSummary), + [normalizedSummary] + ); + const hasMultiplePhases = useMemo( + () => isAccumulatedSummary(normalizedSummary), + [normalizedSummary] + ); + const [activePhaseIndex, setActivePhaseIndex] = useState(0); + + // Reset active phase index when summary changes + useEffect(() => { + setActivePhaseIndex(0); + }, [normalizedSummary]); // Determine the effective view mode - default to summary if available, otherwise parsed const effectiveViewMode = viewMode ?? (summary ? 'summary' : 'parsed'); @@ -79,6 +225,15 @@ export function AgentOutputModal({ const autoScrollRef = useRef(true); const useWorktrees = useAppStore((state) => state.useWorktrees); + // Force a fresh fetch when opening to avoid showing stale cached summaries. + useEffect(() => { + if (!open || !resolvedProjectPath || !featureId) return; + if (!isBacklogPlan) { + void refetchFeature(); + } + void refetchAgentOutput(); + }, [open, resolvedProjectPath, featureId, isBacklogPlan, refetchFeature, refetchAgentOutput]); + // Auto-scroll to bottom when output changes useEffect(() => { if (autoScrollRef.current && scrollRef.current) { @@ -86,6 +241,39 @@ export function AgentOutputModal({ } }, [output]); + // Auto-scroll to bottom when summary changes (for pipeline step accumulation) + const summaryScrollRef = useRef(null); + const [summaryAutoScroll, setSummaryAutoScroll] = useState(true); + + // Auto-scroll summary panel to bottom when summary is updated + useEffect(() => { + if (summaryAutoScroll && summaryScrollRef.current && normalizedSummary) { + summaryScrollRef.current.scrollTop = summaryScrollRef.current.scrollHeight; + } + }, [normalizedSummary, summaryAutoScroll]); + + // Handle scroll to detect if user scrolled up in summary panel + const handleSummaryScroll = () => { + if (!summaryScrollRef.current) return; + + const { scrollTop, scrollHeight, clientHeight } = summaryScrollRef.current; + const isAtBottom = scrollHeight - scrollTop - clientHeight < 50; + setSummaryAutoScroll(isAtBottom); + }; + + // Scroll to active phase when it changes or when summary changes + useEffect(() => { + if (summaryScrollRef.current && hasMultiplePhases) { + const phaseCards = summaryScrollRef.current.querySelectorAll('[data-phase-index]'); + // Ensure index is within bounds + const safeIndex = Math.min(activePhaseIndex, phaseCards.length - 1); + const targetCard = phaseCards[safeIndex]; + if (targetCard) { + targetCard.scrollIntoView({ behavior: 'smooth', block: 'start' }); + } + } + }, [activePhaseIndex, hasMultiplePhases, normalizedSummary]); + // Listen to auto mode events and update output useEffect(() => { if (!open) return; @@ -420,9 +608,49 @@ export function AgentOutputModal({ )} ) : effectiveViewMode === 'summary' && summary ? ( -
- {summary} -
+ <> + {/* Step navigator for multi-phase summaries */} + {hasMultiplePhases && ( + + )} + +
+ {hasMultiplePhases ? ( + // Multi-phase: render individual phase cards + phaseEntries.map((entry, index) => ( +
+ setActivePhaseIndex(index)} + /> +
+ )) + ) : ( + // Single phase: render as markdown +
+ {summary} +
+ )} +
+ +
+ {summaryAutoScroll + ? 'Auto-scrolling enabled' + : 'Scroll to bottom to enable auto-scroll'} +
+ ) : ( <>
) : (
- {completedFeatures.map((feature) => ( - - - - {feature.description || feature.summary || feature.id} - - - {feature.category || 'Uncategorized'} - - -
- - -
- - ))} + {completedFeatures.map((feature) => { + const implementationSummary = extractImplementationSummary(feature.summary); + const displayText = getFirstNonEmptySummary( + implementationSummary, + feature.summary, + feature.description, + feature.id + ); + + return ( + + + + {displayText ?? feature.id} + + + {feature.category || 'Uncategorized'} + + +
+ + +
+ + ); + })}
)}
diff --git a/apps/ui/src/lib/log-parser.ts b/apps/ui/src/lib/log-parser.ts index 5228a8fc..11456725 100644 --- a/apps/ui/src/lib/log-parser.ts +++ b/apps/ui/src/lib/log-parser.ts @@ -1246,8 +1246,236 @@ export function extractSummary(rawOutput: string): string | null { } /** - * Gets the color classes for a log entry type + * Parses an accumulated summary string into individual phase summaries. + * + * The accumulated summary format uses markdown headers with `###` for phase names + * and `---` as separators between phases: + * + * ``` + * ### Implementation + * + * [content] + * + * --- + * + * ### Testing + * + * [content] + * ``` + * + * @param summary - The accumulated summary string to parse + * @returns A map of phase names (lowercase) to their content, or empty map if not parseable */ +const PHASE_SEPARATOR = '\n\n---\n\n'; +const PHASE_SEPARATOR_REGEX = /\n\n---\n\n/; +const PHASE_HEADER_REGEX = /^###\s+(.+?)(?:\n|$)/; +const PHASE_HEADER_WITH_PREFIX_REGEX = /^(###\s+)(.+?)(?:\n|$)/; + +function getPhaseSections(summary: string): { + sections: string[]; + leadingImplementationSection: string | null; +} { + const sections = summary.split(PHASE_SEPARATOR_REGEX); + const hasSeparator = summary.includes(PHASE_SEPARATOR); + const hasAnyHeader = sections.some((section) => PHASE_HEADER_REGEX.test(section.trim())); + const firstSection = sections[0]?.trim() ?? ''; + const leadingImplementationSection = + hasSeparator && hasAnyHeader && firstSection && !PHASE_HEADER_REGEX.test(firstSection) + ? firstSection + : null; + + return { sections, leadingImplementationSection }; +} + +export function parsePhaseSummaries(summary: string | undefined): Map { + const phaseSummaries = new Map(); + + if (!summary || !summary.trim()) { + return phaseSummaries; + } + + const { sections, leadingImplementationSection } = getPhaseSections(summary); + + // Backward compatibility for mixed format: + // [implementation summary without header] + --- + [### Pipeline Step ...] + // Treat the leading headerless section as "Implementation". + if (leadingImplementationSection) { + phaseSummaries.set('implementation', leadingImplementationSection); + } + + for (const section of sections) { + // Match the phase header pattern: ### Phase Name + const headerMatch = section.match(PHASE_HEADER_REGEX); + if (headerMatch) { + const phaseName = headerMatch[1].trim().toLowerCase(); + // Extract content after the header (skip the header line and leading newlines) + const content = section.substring(headerMatch[0].length).trim(); + phaseSummaries.set(phaseName, content); + } + } + + return phaseSummaries; +} + +/** + * Extracts a specific phase summary from an accumulated summary string. + * + * @param summary - The accumulated summary string + * @param phaseName - The phase name to extract (case-insensitive, e.g., "Implementation", "implementation") + * @returns The content for the specified phase, or null if not found + */ +export function extractPhaseSummary(summary: string | undefined, phaseName: string): string | null { + const phaseSummaries = parsePhaseSummaries(summary); + const normalizedPhaseName = phaseName.toLowerCase(); + return phaseSummaries.get(normalizedPhaseName) || null; +} + +/** + * Gets the implementation phase summary from an accumulated summary string. + * + * This is a convenience function that handles various naming conventions: + * - "implementation" + * - "Implementation" + * - Any phase that contains "implement" in its name + * + * @param summary - The accumulated summary string + * @returns The implementation phase content, or null if not found + */ +export function extractImplementationSummary(summary: string | undefined): string | null { + if (!summary || !summary.trim()) { + return null; + } + + const phaseSummaries = parsePhaseSummaries(summary); + + // Try exact match first + const implementationContent = phaseSummaries.get('implementation'); + if (implementationContent) { + return implementationContent; + } + + // Fallback: find any phase containing "implement" + for (const [phaseName, content] of phaseSummaries) { + if (phaseName.includes('implement')) { + return content; + } + } + + // If no phase summaries found, the summary might not be in accumulated format + // (legacy or non-pipeline feature). In this case, return the whole summary + // if it looks like a single summary (no phase headers). + if (!summary.includes('### ') && !summary.includes(PHASE_SEPARATOR)) { + return summary; + } + + return null; +} + +/** + * Checks if a summary string is in the accumulated multi-phase format. + * + * @param summary - The summary string to check + * @returns True if the summary has multiple phases, false otherwise + */ +export function isAccumulatedSummary(summary: string | undefined): boolean { + if (!summary || !summary.trim()) { + return false; + } + + // Check for the presence of phase headers with separator + const hasMultiplePhases = + summary.includes(PHASE_SEPARATOR) && summary.match(/###\s+.+/g)?.length > 0; + + return hasMultiplePhases; +} + +/** + * Represents a single phase entry in an accumulated summary. + */ +export interface PhaseSummaryEntry { + /** The phase name (e.g., "Implementation", "Testing", "Code Review") */ + phaseName: string; + /** The content of this phase's summary */ + content: string; + /** The original header line (e.g., "### Implementation") */ + header: string; +} + +/** Default phase name used for non-accumulated summaries */ +const DEFAULT_PHASE_NAME = 'Summary'; + +/** + * Parses an accumulated summary into individual phase entries. + * Returns phases in the order they appear in the summary. + * + * The accumulated summary format: + * ``` + * ### Implementation + * + * [content] + * + * --- + * + * ### Testing + * + * [content] + * ``` + * + * @param summary - The accumulated summary string to parse + * @returns Array of PhaseSummaryEntry objects, or empty array if not parseable + */ +export function parseAllPhaseSummaries(summary: string | undefined): PhaseSummaryEntry[] { + const entries: PhaseSummaryEntry[] = []; + + if (!summary || !summary.trim()) { + return entries; + } + + // Check if this is an accumulated summary (has phase headers at line starts) + // Use a more precise check: ### must be at the start of a line (not just anywhere in content) + const hasPhaseHeaders = /^###\s+/m.test(summary); + if (!hasPhaseHeaders) { + // Not an accumulated summary - return as single entry with generic name + return [ + { phaseName: DEFAULT_PHASE_NAME, content: summary, header: `### ${DEFAULT_PHASE_NAME}` }, + ]; + } + + const { sections, leadingImplementationSection } = getPhaseSections(summary); + + // Backward compatibility for mixed format: + // [implementation summary without header] + --- + [### Pipeline Step ...] + if (leadingImplementationSection) { + entries.push({ + phaseName: 'Implementation', + content: leadingImplementationSection, + header: '### Implementation', + }); + } + + for (const section of sections) { + // Match the phase header pattern: ### Phase Name + const headerMatch = section.match(PHASE_HEADER_WITH_PREFIX_REGEX); + if (headerMatch) { + const header = headerMatch[0].trim(); + const phaseName = headerMatch[2].trim(); + // Extract content after the header (skip the header line and leading newlines) + const content = section.substring(headerMatch[0].length).trim(); + entries.push({ phaseName, content, header }); + } + } + + // Fallback: if we detected phase headers but couldn't parse any entries, + // treat the entire summary as a single entry to avoid showing "No summary available" + if (entries.length === 0) { + return [ + { phaseName: DEFAULT_PHASE_NAME, content: summary, header: `### ${DEFAULT_PHASE_NAME}` }, + ]; + } + + return entries; +} + export function getLogTypeColors(type: LogEntryType): { bg: string; border: string; diff --git a/apps/ui/src/lib/summary-selection.ts b/apps/ui/src/lib/summary-selection.ts new file mode 100644 index 00000000..3553d3f4 --- /dev/null +++ b/apps/ui/src/lib/summary-selection.ts @@ -0,0 +1,14 @@ +export type SummaryValue = string | null | undefined; + +/** + * Returns the first summary candidate that contains non-whitespace content. + * The original string is returned (without trimming) to preserve formatting. + */ +export function getFirstNonEmptySummary(...candidates: SummaryValue[]): string | null { + for (const candidate of candidates) { + if (typeof candidate === 'string' && candidate.trim().length > 0) { + return candidate; + } + } + return null; +} diff --git a/libs/prompts/src/defaults.ts b/libs/prompts/src/defaults.ts index 330afe0c..da716722 100644 --- a/libs/prompts/src/defaults.ts +++ b/libs/prompts/src/defaults.ts @@ -291,6 +291,23 @@ export const DEFAULT_AUTO_MODE_PIPELINE_STEP_PROMPT_TEMPLATE = `## Pipeline Step ### Pipeline Step Instructions {{stepInstructions}} + +**CRITICAL: After completing the instructions, you MUST output a summary using this EXACT format:** + + +## Summary: {{stepName}} + +### Changes Implemented +- [List all changes made in this step] + +### Files Modified +- [List all files modified in this step] + +### Outcome +- [Describe the result of this step] + + +The and tags MUST be on their own lines. This is REQUIRED. `; /** diff --git a/libs/types/src/feature.ts b/libs/types/src/feature.ts index 2b983cfe..4cb9f146 100644 --- a/libs/types/src/feature.ts +++ b/libs/types/src/feature.ts @@ -47,6 +47,8 @@ export interface ParsedTask { phase?: string; /** Task execution status */ status: 'pending' | 'in_progress' | 'completed' | 'failed'; + /** Optional task summary, e.g., "Created User model with email and password fields" */ + summary?: string; } /** diff --git a/libs/types/src/index.ts b/libs/types/src/index.ts index b52df413..f02ff3c0 100644 --- a/libs/types/src/index.ts +++ b/libs/types/src/index.ts @@ -306,6 +306,7 @@ export type { PipelineStatus, FeatureStatusWithPipeline, } from './pipeline.js'; +export { isPipelineStatus } from './pipeline.js'; // Port configuration export { STATIC_PORT, SERVER_PORT, RESERVED_PORTS } from './ports.js'; diff --git a/libs/types/src/pipeline.ts b/libs/types/src/pipeline.ts index 7190abbd..f3eb0f10 100644 --- a/libs/types/src/pipeline.ts +++ b/libs/types/src/pipeline.ts @@ -19,6 +19,17 @@ export interface PipelineConfig { export type PipelineStatus = `pipeline_${string}`; +/** + * Type guard to check if a status string represents a valid pipeline stage. + * Requires the 'pipeline_' prefix followed by at least one character. + */ +export function isPipelineStatus(status: string | null | undefined): status is PipelineStatus { + if (typeof status !== 'string') return false; + // Require 'pipeline_' prefix with at least one character after it + const prefix = 'pipeline_'; + return status.startsWith(prefix) && status.length > prefix.length; +} + export type FeatureStatusWithPipeline = | 'backlog' | 'ready' @@ -28,3 +39,6 @@ export type FeatureStatusWithPipeline = | 'verified' | 'completed' | PipelineStatus; + +export const PIPELINE_SUMMARY_SEPARATOR = '\n\n---\n\n'; +export const PIPELINE_SUMMARY_HEADER_PREFIX = '### ';