chore: resolve merge conflicts — wire quality review into orchestrator handleAgentStopped
Resolved conflicts between cw/continuous-code-quality-phase-quality-review-dispatch-hook and cw/continuous-code-quality-task-Cjc9jRT48MqxIZSQG3ypl. - orchestrator.ts: adopted tryQualityReview() helper (incoming) over inline shouldRunQualityReview() call (HEAD); fixed duplicate agentManager constructor param; reordered optional params to agentRepository?, agentManager? - orchestrator.test.ts: merged import blocks, used incoming mock defaults with .mockResolvedValue(), removed duplicate agentManager/agentRepository in createMocks(), used incoming createOrchestrator opts pattern; added missing project mock so HEAD's integration test works with tryQualityReview() flow - docs/dispatch-events.md: kept HEAD's more explicit agent:stopped description - container.ts: removed duplicate agentManager arg; reordered to match new constructor signature (agentRepository, agentManager) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -250,8 +250,8 @@ export async function createContainer(options?: ContainerOptions): Promise<Conta
|
||||
conflictResolutionService,
|
||||
eventBus,
|
||||
workspaceRoot,
|
||||
agentManager,
|
||||
repos.agentRepository,
|
||||
agentManager,
|
||||
);
|
||||
executionOrchestrator.start();
|
||||
log.info('execution orchestrator started');
|
||||
|
||||
@@ -8,16 +8,17 @@ import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
import { ExecutionOrchestrator } from './orchestrator.js';
|
||||
import { ensureProjectClone } from '../git/project-clones.js';
|
||||
import type { BranchManager } from '../git/branch-manager.js';
|
||||
import { shouldRunQualityReview, runQualityReview } from './quality-review.js';
|
||||
import type { AgentManager } from '../agent/types.js';
|
||||
import type { AgentRepository } from '../db/repositories/agent-repository.js';
|
||||
|
||||
vi.mock('../git/project-clones.js', () => ({
|
||||
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<string, Function[]>; emitted: DomainEvent[] } {
|
||||
const handlers = new Map<string, Function[]>();
|
||||
@@ -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<typeof createMocks>) {
|
||||
function createOrchestrator(mocks: ReturnType<typeof createMocks>, opts: { withAgentManager?: boolean; withAgentRepository?: boolean } = {}) {
|
||||
const orchestrator = new ExecutionOrchestrator(
|
||||
mocks.branchManager,
|
||||
mocks.phaseRepository,
|
||||
@@ -161,8 +173,8 @@ function createOrchestrator(mocks: ReturnType<typeof createMocks>) {
|
||||
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<typeof createMockEventBus>, 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();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string> {
|
||||
/**
|
||||
* 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<void> {
|
||||
|
||||
Reference in New Issue
Block a user