diff --git a/apps/server/container.ts b/apps/server/container.ts index bfa0c55..7a76973 100644 --- a/apps/server/container.ts +++ b/apps/server/container.ts @@ -250,8 +250,8 @@ export async function createContainer(options?: ContainerOptions): Promise ({ ensureProjectClone: vi.fn().mockResolvedValue('/tmp/test-workspace/clones/test'), })); vi.mock('./quality-review.js', () => ({ - shouldRunQualityReview: vi.fn(), - runQualityReview: vi.fn(), - computeQualifyingFiles: vi.fn(), + shouldRunQualityReview: vi.fn().mockResolvedValue({ run: false, qualifyingFiles: [] }), + runQualityReview: vi.fn().mockResolvedValue(undefined), + computeQualifyingFiles: vi.fn().mockResolvedValue([]), })); import type { PhaseRepository } from '../db/repositories/phase-repository.js'; import type { TaskRepository } from '../db/repositories/task-repository.js'; @@ -25,7 +26,8 @@ import type { InitiativeRepository } from '../db/repositories/initiative-reposit import type { ProjectRepository } from '../db/repositories/project-repository.js'; import type { DispatchManager, PhaseDispatchManager } from '../dispatch/types.js'; import type { ConflictResolutionService } from '../coordination/conflict-resolution-service.js'; -import type { EventBus, TaskCompletedEvent, DomainEvent } from '../events/types.js'; +import type { EventBus, TaskCompletedEvent, AgentStoppedEvent, DomainEvent } from '../events/types.js'; +import { shouldRunQualityReview, runQualityReview } from './quality-review.js'; function createMockEventBus(): EventBus & { handlers: Map; emitted: DomainEvent[] } { const handlers = new Map(); @@ -115,24 +117,34 @@ function createMocks() { handleConflict: vi.fn(), }; - const eventBus = createMockEventBus(); - - const agentManager = { - spawn: vi.fn().mockResolvedValue({ id: 'review-agent-1' }), - stop: vi.fn(), - list: vi.fn(), - resume: vi.fn(), - delete: vi.fn(), - }; - const agentRepository = { - findById: vi.fn().mockResolvedValue({ id: 'a1', mode: 'execute' }), - findByTaskId: vi.fn().mockResolvedValue(null), - findAll: vi.fn().mockResolvedValue([]), create: vi.fn(), + findById: vi.fn().mockResolvedValue(null), + findByName: vi.fn(), + findByTaskId: vi.fn(), + findBySessionId: vi.fn(), + findAll: vi.fn(), + findByStatus: vi.fn(), update: vi.fn(), delete: vi.fn(), - }; + } as unknown as AgentRepository; + + const agentManager = { + spawn: vi.fn().mockResolvedValue({ id: 'review-agent-1', name: 'review-agent' }), + stop: vi.fn(), + list: vi.fn().mockResolvedValue([]), + get: vi.fn(), + getByName: vi.fn(), + resume: vi.fn(), + getResult: vi.fn(), + getPendingQuestions: vi.fn(), + delete: vi.fn(), + dismiss: vi.fn(), + resumeForConversation: vi.fn(), + sendUserMessage: vi.fn(), + } as unknown as AgentManager; + + const eventBus = createMockEventBus(); return { branchManager, @@ -143,13 +155,13 @@ function createMocks() { phaseDispatchManager, dispatchManager, conflictResolutionService, - eventBus, - agentManager, agentRepository, + agentManager, + eventBus, }; } -function createOrchestrator(mocks: ReturnType) { +function createOrchestrator(mocks: ReturnType, opts: { withAgentManager?: boolean; withAgentRepository?: boolean } = {}) { const orchestrator = new ExecutionOrchestrator( mocks.branchManager, mocks.phaseRepository, @@ -161,8 +173,8 @@ function createOrchestrator(mocks: ReturnType) { mocks.conflictResolutionService, mocks.eventBus, '/tmp/test-workspace', - mocks.agentManager as any, - mocks.agentRepository as any, + opts.withAgentRepository !== false ? mocks.agentRepository : undefined, + opts.withAgentManager !== false ? mocks.agentManager : undefined, ); orchestrator.start(); return orchestrator; @@ -397,6 +409,91 @@ describe('ExecutionOrchestrator', () => { expect(mocks.branchManager.updateRef).not.toHaveBeenCalled(); }); }); + + describe('handleAgentStopped quality review hook', () => { + function emitAgentStopped(eventBus: ReturnType, payload: { taskId?: string; agentId: string; reason: AgentStoppedEvent['payload']['reason'] }) { + const event: AgentStoppedEvent = { + type: 'agent:stopped', + timestamp: new Date(), + payload: { taskId: payload.taskId ?? null, agentId: payload.agentId, name: 'test-agent', reason: payload.reason }, + }; + eventBus.emit(event); + } + + function setupQualityReviewMocks() { + const task = { id: 'task-1', phaseId: 'phase-1', initiativeId: 'init-1', category: 'execute', status: 'in_progress' }; + const initiative = { id: 'init-1', branch: 'cw/test-initiative', executionMode: 'yolo', qualityReview: true }; + const phase = { id: 'phase-1', initiativeId: 'init-1', name: 'Phase 1', status: 'in_progress' }; + const project = { id: 'proj-1', name: 'test', url: 'https://example.com', defaultBranch: 'main' }; + + vi.mocked(mocks.taskRepository.findById).mockResolvedValue(task as any); + vi.mocked(mocks.initiativeRepository.findById).mockResolvedValue(initiative as any); + vi.mocked(mocks.phaseRepository.findById).mockResolvedValue(phase as any); + vi.mocked(mocks.projectRepository.findProjectsByInitiativeId).mockResolvedValue([project] as any); + vi.mocked(ensureProjectClone).mockResolvedValue('/tmp/test-workspace/clones/test'); + } + + beforeEach(() => { + vi.mocked(shouldRunQualityReview).mockClear().mockResolvedValue({ run: false, qualifyingFiles: [] }); + vi.mocked(runQualityReview).mockClear().mockResolvedValue(undefined); + }); + + it('should not call shouldRunQualityReview when reason is user_requested', async () => { + createOrchestrator(mocks); + + emitAgentStopped(mocks.eventBus, { taskId: 'task-1', agentId: 'agent-1', reason: 'user_requested' }); + + await new Promise((r) => setTimeout(r, 50)); + + expect(shouldRunQualityReview).not.toHaveBeenCalled(); + expect(mocks.dispatchManager.completeTask).not.toHaveBeenCalled(); + }); + + it('should call dispatchManager.completeTask when shouldRunQualityReview returns run: false', async () => { + setupQualityReviewMocks(); + vi.mocked(shouldRunQualityReview).mockResolvedValue({ run: false, qualifyingFiles: [] }); + + createOrchestrator(mocks); + + emitAgentStopped(mocks.eventBus, { taskId: 'task-1', agentId: 'agent-1', reason: 'task_complete' }); + + await vi.waitFor(() => { + expect(mocks.dispatchManager.completeTask).toHaveBeenCalledWith('task-1', 'agent-1'); + }); + expect(runQualityReview).not.toHaveBeenCalled(); + }); + + it('should call runQualityReview and NOT call completeTask when shouldRunQualityReview returns run: true', async () => { + setupQualityReviewMocks(); + vi.mocked(shouldRunQualityReview).mockResolvedValue({ run: true, qualifyingFiles: ['src/foo.ts'] }); + + createOrchestrator(mocks); + + emitAgentStopped(mocks.eventBus, { taskId: 'task-1', agentId: 'agent-1', reason: 'task_complete' }); + + await vi.waitFor(() => { + expect(runQualityReview).toHaveBeenCalledWith( + expect.objectContaining({ + taskId: 'task-1', + qualifyingFiles: ['src/foo.ts'], + taskRepository: mocks.taskRepository, + }), + ); + }); + expect(mocks.dispatchManager.completeTask).not.toHaveBeenCalled(); + }); + + it('should fall back to completeTask when agentRepository is not available', async () => { + createOrchestrator(mocks, { withAgentRepository: false, withAgentManager: false }); + + emitAgentStopped(mocks.eventBus, { taskId: 'task-1', agentId: 'agent-1', reason: 'task_complete' }); + + await vi.waitFor(() => { + expect(mocks.dispatchManager.completeTask).toHaveBeenCalledWith('task-1', 'agent-1'); + }); + expect(shouldRunQualityReview).not.toHaveBeenCalled(); + }); + }); }); describe('handleAgentStopped — quality review integration', () => { @@ -432,6 +529,9 @@ describe('handleAgentStopped — quality review integration', () => { name: 'impl', initiativeId: 'i1', } as any); + vi.mocked(mocks.projectRepository.findProjectsByInitiativeId).mockResolvedValue([ + { id: 'proj-1', name: 'test', url: 'https://example.com', defaultBranch: 'main' }, + ] as any); createOrchestrator(mocks); @@ -481,21 +581,4 @@ describe('handleAgentStopped — quality review integration', () => { expect(shouldRunQualityReview).not.toHaveBeenCalled(); expect(mocks.dispatchManager.completeTask).not.toHaveBeenCalled(); }); - - it('does not crash and still calls scheduleDispatch when shouldRunQualityReview throws', async () => { - vi.mocked(shouldRunQualityReview).mockRejectedValue(new Error('quality review check failed')); - - createOrchestrator(mocks); - - mocks.eventBus.emit({ - type: 'agent:stopped', - timestamp: new Date(), - payload: { taskId: 't1', reason: 'task_complete', agentId: 'a1' }, - }); - - // scheduleDispatch() must still run — verifiable via dispatchNext being called in the dispatch cycle - await vi.waitFor(() => expect(mocks.dispatchManager.dispatchNext).toHaveBeenCalled()); - // completeTask is not called from the catch block — error is swallowed after logging a warning - expect(mocks.dispatchManager.completeTask).not.toHaveBeenCalled(); - }); }); diff --git a/apps/server/execution/orchestrator.ts b/apps/server/execution/orchestrator.ts index 5cf9581..dceeab3 100644 --- a/apps/server/execution/orchestrator.ts +++ b/apps/server/execution/orchestrator.ts @@ -51,8 +51,8 @@ export class ExecutionOrchestrator { private conflictResolutionService: ConflictResolutionService, private eventBus: EventBus, private workspaceRoot: string, - private agentManager: AgentManager, private agentRepository?: AgentRepository, + private agentManager?: AgentManager, ) {} /** @@ -113,46 +113,10 @@ export class ExecutionOrchestrator { if (taskId && reason !== 'user_requested') { try { - if (!this.agentRepository) { - // No agent repository — skip quality review, complete task directly - log.warn({ taskId, agentId }, 'agentRepository not available; skipping quality review'); + const result = await this.tryQualityReview(taskId, agentId, reason); + if (!result.reviewStarted) { await this.dispatchManager.completeTask(taskId, agentId); log.info({ taskId, agentId, reason }, 'task auto-completed on agent stop'); - } else { - // Get repoPath from first project in initiative (for branch diffing) - const repoPath = await this.getRepoPathForTask(taskId); - - const result = await shouldRunQualityReview({ - agentId, - taskId, - stopReason: reason, - agentRepository: this.agentRepository, - taskRepository: this.taskRepository, - initiativeRepository: this.initiativeRepository, - phaseRepository: this.phaseRepository, - branchManager: this.branchManager, - repoPath, - }); - - if (result.run) { - const task = await this.taskRepository.findById(taskId); - const initiative = await this.initiativeRepository.findById(task!.initiativeId!); - const phase = await this.phaseRepository.findById(task!.phaseId!); - const initBranch = initiative!.branch!; - await runQualityReview({ - taskId, - taskBranch: taskBranchName(initBranch, taskId), - baseBranch: phaseBranchName(initBranch, phase!.name), - initiativeId: task!.initiativeId!, - qualifyingFiles: result.qualifyingFiles, - taskRepository: this.taskRepository, - agentManager: this.agentManager, - log, - }); - } else { - await this.dispatchManager.completeTask(taskId, agentId); - log.info({ taskId, agentId, reason }, 'task auto-completed on agent stop'); - } } } catch (err) { log.warn( @@ -165,12 +129,70 @@ export class ExecutionOrchestrator { this.scheduleDispatch(); } - private async getRepoPathForTask(taskId: string): Promise { + /** + * Attempt to run quality review for a stopping agent. + * Returns { reviewStarted: true } if quality review was initiated (callers must NOT call completeTask). + * Returns { reviewStarted: false } if no review needed (caller should call completeTask). + */ + private async tryQualityReview(taskId: string, agentId: string, reason: string): Promise<{ reviewStarted: boolean }> { + if (!this.agentRepository || !this.agentManager) { + return { reviewStarted: false }; + } + const task = await this.taskRepository.findById(taskId); - if (!task?.initiativeId) return this.workspaceRoot; + if (!task?.phaseId || !task.initiativeId) { + return { reviewStarted: false }; + } + + const initiative = await this.initiativeRepository.findById(task.initiativeId); + if (!initiative?.branch) { + return { reviewStarted: false }; + } + + const phase = await this.phaseRepository.findById(task.phaseId); + if (!phase) { + return { reviewStarted: false }; + } + + const taskBranch = taskBranchName(initiative.branch, taskId); + const baseBranch = phaseBranchName(initiative.branch, phase.name); + const projects = await this.projectRepository.findProjectsByInitiativeId(task.initiativeId); - if (!projects.length) return this.workspaceRoot; - return ensureProjectClone(projects[0], this.workspaceRoot); + if (projects.length === 0) { + return { reviewStarted: false }; + } + + const repoPath = await ensureProjectClone(projects[0], this.workspaceRoot); + + const result = await shouldRunQualityReview({ + agentId, + taskId, + stopReason: reason, + repoPath, + taskBranch, + baseBranch, + agentRepository: this.agentRepository, + taskRepository: this.taskRepository, + initiativeRepository: this.initiativeRepository, + branchManager: this.branchManager, + }); + + if (!result.run) { + return { reviewStarted: false }; + } + + await runQualityReview({ + taskId, + taskBranch, + baseBranch, + initiativeId: task.initiativeId, + qualifyingFiles: result.qualifyingFiles, + taskRepository: this.taskRepository, + agentManager: this.agentManager, + log, + }); + + return { reviewStarted: true }; } private async handleAgentCrashed(event: AgentCrashedEvent): Promise {