feat: add quality-review dispatch hook to intercept agent:stopped events
When an execute-mode agent stops with task_complete and the initiative has qualityReview=true, the orchestrator now spawns a fresh execute-mode agent to run /simplify on changed .ts/.tsx/.js files before marking the task completed. The task transitions through quality_review status as a recursion guard so the review agent's stop event is handled normally. - Add apps/server/execution/quality-review.ts with three exported functions: computeQualifyingFiles, shouldRunQualityReview, runQualityReview - Add apps/server/execution/quality-review.test.ts (28 tests) - Update ExecutionOrchestrator to accept agentManager, replace handleAgentStopped with quality-review-aware logic, add getRepoPathForTask - Update orchestrator.test.ts with 3 quality-review integration tests - Update container.ts to pass agentManager to ExecutionOrchestrator - Update docs/dispatch-events.md to reflect new agent:stopped behavior Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -250,6 +250,7 @@ export async function createContainer(options?: ContainerOptions): Promise<Conta
|
||||
conflictResolutionService,
|
||||
eventBus,
|
||||
workspaceRoot,
|
||||
agentManager,
|
||||
repos.agentRepository,
|
||||
);
|
||||
executionOrchestrator.start();
|
||||
|
||||
@@ -8,10 +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';
|
||||
|
||||
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(),
|
||||
}));
|
||||
import type { PhaseRepository } from '../db/repositories/phase-repository.js';
|
||||
import type { TaskRepository } from '../db/repositories/task-repository.js';
|
||||
import type { InitiativeRepository } from '../db/repositories/initiative-repository.js';
|
||||
@@ -110,6 +117,23 @@ function createMocks() {
|
||||
|
||||
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(),
|
||||
update: vi.fn(),
|
||||
delete: vi.fn(),
|
||||
};
|
||||
|
||||
return {
|
||||
branchManager,
|
||||
phaseRepository,
|
||||
@@ -120,6 +144,8 @@ function createMocks() {
|
||||
dispatchManager,
|
||||
conflictResolutionService,
|
||||
eventBus,
|
||||
agentManager,
|
||||
agentRepository,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -135,6 +161,8 @@ function createOrchestrator(mocks: ReturnType<typeof createMocks>) {
|
||||
mocks.conflictResolutionService,
|
||||
mocks.eventBus,
|
||||
'/tmp/test-workspace',
|
||||
mocks.agentManager as any,
|
||||
mocks.agentRepository as any,
|
||||
);
|
||||
orchestrator.start();
|
||||
return orchestrator;
|
||||
@@ -370,3 +398,87 @@ describe('ExecutionOrchestrator', () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('handleAgentStopped — quality review integration', () => {
|
||||
let mocks: ReturnType<typeof createMocks>;
|
||||
|
||||
beforeEach(() => {
|
||||
mocks = createMocks();
|
||||
vi.mocked(shouldRunQualityReview).mockReset();
|
||||
vi.mocked(runQualityReview).mockReset();
|
||||
});
|
||||
|
||||
it('calls runQualityReview and skips completeTask when shouldRunQualityReview returns run:true', async () => {
|
||||
vi.mocked(shouldRunQualityReview).mockResolvedValue({
|
||||
run: true,
|
||||
qualifyingFiles: ['src/foo.ts'],
|
||||
});
|
||||
vi.mocked(runQualityReview).mockResolvedValue(undefined);
|
||||
|
||||
// Provide task data for re-fetch inside runQualityReview branch
|
||||
vi.mocked(mocks.taskRepository.findById).mockResolvedValue({
|
||||
id: 't1',
|
||||
status: 'in_progress',
|
||||
initiativeId: 'i1',
|
||||
phaseId: 'p1',
|
||||
} as any);
|
||||
vi.mocked(mocks.initiativeRepository.findById).mockResolvedValue({
|
||||
id: 'i1',
|
||||
branch: 'cw/test',
|
||||
qualityReview: true,
|
||||
} as any);
|
||||
vi.mocked(mocks.phaseRepository.findById).mockResolvedValue({
|
||||
id: 'p1',
|
||||
name: 'impl',
|
||||
initiativeId: 'i1',
|
||||
} as any);
|
||||
|
||||
createOrchestrator(mocks);
|
||||
|
||||
mocks.eventBus.emit({
|
||||
type: 'agent:stopped',
|
||||
timestamp: new Date(),
|
||||
payload: { taskId: 't1', reason: 'task_complete', agentId: 'a1' },
|
||||
});
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(runQualityReview).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ taskId: 't1', qualifyingFiles: ['src/foo.ts'] }),
|
||||
);
|
||||
});
|
||||
expect(mocks.dispatchManager.completeTask).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('calls completeTask and skips runQualityReview when shouldRunQualityReview returns run:false', async () => {
|
||||
vi.mocked(shouldRunQualityReview).mockResolvedValue({ run: false, qualifyingFiles: [] });
|
||||
vi.mocked(runQualityReview).mockResolvedValue(undefined);
|
||||
|
||||
createOrchestrator(mocks);
|
||||
|
||||
mocks.eventBus.emit({
|
||||
type: 'agent:stopped',
|
||||
timestamp: new Date(),
|
||||
payload: { taskId: 't1', reason: 'task_complete', agentId: 'a1' },
|
||||
});
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(mocks.dispatchManager.completeTask).toHaveBeenCalledWith('t1', 'a1');
|
||||
});
|
||||
expect(runQualityReview).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('skips both paths for user_requested reason', async () => {
|
||||
createOrchestrator(mocks);
|
||||
|
||||
mocks.eventBus.emit({
|
||||
type: 'agent:stopped',
|
||||
timestamp: new Date(),
|
||||
payload: { taskId: 't1', reason: 'user_requested', agentId: 'a1' },
|
||||
});
|
||||
|
||||
// Wait for scheduleDispatch to be triggered (dispatchNext is called in the cycle)
|
||||
await vi.waitFor(() => expect(mocks.dispatchManager.dispatchNext).toHaveBeenCalled());
|
||||
expect(shouldRunQualityReview).not.toHaveBeenCalled();
|
||||
expect(mocks.dispatchManager.completeTask).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -18,12 +18,14 @@ import type { TaskRepository } from '../db/repositories/task-repository.js';
|
||||
import type { InitiativeRepository } from '../db/repositories/initiative-repository.js';
|
||||
import type { ProjectRepository } from '../db/repositories/project-repository.js';
|
||||
import type { AgentRepository } from '../db/repositories/agent-repository.js';
|
||||
import type { AgentManager } from '../agent/types.js';
|
||||
import type { DispatchManager, PhaseDispatchManager } from '../dispatch/types.js';
|
||||
import type { ConflictResolutionService } from '../coordination/conflict-resolution-service.js';
|
||||
import { phaseBranchName, taskBranchName } from '../git/branch-naming.js';
|
||||
import { ensureProjectClone } from '../git/project-clones.js';
|
||||
import { createModuleLogger } from '../logger/index.js';
|
||||
import { phaseMetaCache, fileDiffCache } from '../review/diff-cache.js';
|
||||
import { shouldRunQualityReview, runQualityReview } from './quality-review.js';
|
||||
|
||||
const log = createModuleLogger('execution-orchestrator');
|
||||
|
||||
@@ -49,6 +51,7 @@ export class ExecutionOrchestrator {
|
||||
private conflictResolutionService: ConflictResolutionService,
|
||||
private eventBus: EventBus,
|
||||
private workspaceRoot: string,
|
||||
private agentManager: AgentManager,
|
||||
private agentRepository?: AgentRepository,
|
||||
) {}
|
||||
|
||||
@@ -108,15 +111,53 @@ export class ExecutionOrchestrator {
|
||||
private async handleAgentStopped(event: AgentStoppedEvent): Promise<void> {
|
||||
const { taskId, reason, agentId } = event.payload;
|
||||
|
||||
// Auto-complete task for successful agent completions, not manual stops
|
||||
if (taskId && reason !== 'user_requested') {
|
||||
try {
|
||||
await this.dispatchManager.completeTask(taskId, agentId);
|
||||
log.info({ taskId, agentId, reason }, 'task auto-completed on agent stop');
|
||||
if (!this.agentRepository) {
|
||||
// No agent repository — skip quality review, complete task directly
|
||||
log.warn({ taskId, agentId }, 'agentRepository not available; skipping quality review');
|
||||
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(
|
||||
{ taskId, agentId, reason, err: err instanceof Error ? err.message : String(err) },
|
||||
'failed to auto-complete task on agent stop',
|
||||
'failed to handle agent stop',
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -124,6 +165,14 @@ export class ExecutionOrchestrator {
|
||||
this.scheduleDispatch();
|
||||
}
|
||||
|
||||
private async getRepoPathForTask(taskId: string): Promise<string> {
|
||||
const task = await this.taskRepository.findById(taskId);
|
||||
if (!task?.initiativeId) return this.workspaceRoot;
|
||||
const projects = await this.projectRepository.findProjectsByInitiativeId(task.initiativeId);
|
||||
if (!projects.length) return this.workspaceRoot;
|
||||
return ensureProjectClone(projects[0], this.workspaceRoot);
|
||||
}
|
||||
|
||||
private async handleAgentCrashed(event: AgentCrashedEvent): Promise<void> {
|
||||
const { taskId, agentId, error } = event.payload;
|
||||
if (!taskId) return;
|
||||
|
||||
434
apps/server/execution/quality-review.test.ts
Normal file
434
apps/server/execution/quality-review.test.ts
Normal file
@@ -0,0 +1,434 @@
|
||||
/**
|
||||
* Quality Review Tests
|
||||
*
|
||||
* Tests for the quality-review dispatch hook that intercepts agent:stopped
|
||||
* events and spawns a review agent when conditions are met.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
import { computeQualifyingFiles, shouldRunQualityReview, runQualityReview } from './quality-review.js';
|
||||
import type { BranchManager } from '../git/branch-manager.js';
|
||||
import type { AgentRepository } from '../db/repositories/agent-repository.js';
|
||||
import type { TaskRepository } from '../db/repositories/task-repository.js';
|
||||
import type { InitiativeRepository } from '../db/repositories/initiative-repository.js';
|
||||
import type { PhaseRepository } from '../db/repositories/phase-repository.js';
|
||||
import type { AgentManager } from '../agent/types.js';
|
||||
import type { createModuleLogger } from '../logger/index.js';
|
||||
|
||||
type Logger = ReturnType<typeof createModuleLogger>;
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Mock helpers
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
function createBranchManagerMock(): BranchManager {
|
||||
return {
|
||||
ensureBranch: vi.fn(),
|
||||
mergeBranch: vi.fn(),
|
||||
diffBranches: vi.fn(),
|
||||
diffBranchesStat: vi.fn().mockResolvedValue([]),
|
||||
diffFileSingle: vi.fn(),
|
||||
getHeadCommitHash: vi.fn(),
|
||||
deleteBranch: vi.fn(),
|
||||
branchExists: vi.fn(),
|
||||
remoteBranchExists: vi.fn(),
|
||||
listCommits: vi.fn(),
|
||||
diffCommit: vi.fn(),
|
||||
getMergeBase: vi.fn(),
|
||||
pushBranch: vi.fn(),
|
||||
checkMergeability: vi.fn(),
|
||||
fetchRemote: vi.fn(),
|
||||
fastForwardBranch: vi.fn(),
|
||||
updateRef: vi.fn(),
|
||||
} as unknown as BranchManager;
|
||||
}
|
||||
|
||||
function createAgentRepositoryMock(): AgentRepository {
|
||||
return {
|
||||
findById: vi.fn().mockResolvedValue({ id: 'a1', mode: 'execute' }),
|
||||
findByTaskId: vi.fn(),
|
||||
findAll: vi.fn(),
|
||||
create: vi.fn(),
|
||||
update: vi.fn(),
|
||||
delete: vi.fn(),
|
||||
} as unknown as AgentRepository;
|
||||
}
|
||||
|
||||
function createTaskRepositoryMock(): TaskRepository {
|
||||
return {
|
||||
findById: vi.fn().mockResolvedValue({
|
||||
id: 't1',
|
||||
status: 'in_progress',
|
||||
initiativeId: 'i1',
|
||||
phaseId: 'p1',
|
||||
}),
|
||||
findByPhaseId: vi.fn(),
|
||||
findByInitiativeId: vi.fn(),
|
||||
create: vi.fn(),
|
||||
update: vi.fn().mockResolvedValue(undefined),
|
||||
delete: vi.fn(),
|
||||
} as unknown as TaskRepository;
|
||||
}
|
||||
|
||||
function createInitiativeRepositoryMock(): InitiativeRepository {
|
||||
return {
|
||||
findById: vi.fn().mockResolvedValue({
|
||||
id: 'i1',
|
||||
qualityReview: true,
|
||||
branch: 'cw/test',
|
||||
}),
|
||||
findAll: vi.fn(),
|
||||
findByStatus: vi.fn(),
|
||||
create: vi.fn(),
|
||||
update: vi.fn(),
|
||||
delete: vi.fn(),
|
||||
} as unknown as InitiativeRepository;
|
||||
}
|
||||
|
||||
function createPhaseRepositoryMock(): PhaseRepository {
|
||||
return {
|
||||
findById: vi.fn().mockResolvedValue({ id: 'p1', name: 'impl', initiativeId: 'i1' }),
|
||||
findByInitiativeId: vi.fn(),
|
||||
create: vi.fn(),
|
||||
update: vi.fn(),
|
||||
delete: vi.fn(),
|
||||
} as unknown as PhaseRepository;
|
||||
}
|
||||
|
||||
function createAgentManagerMock(): AgentManager {
|
||||
return {
|
||||
spawn: vi.fn().mockResolvedValue({ id: 'review-agent-1' }),
|
||||
stop: vi.fn(),
|
||||
list: vi.fn(),
|
||||
resume: vi.fn(),
|
||||
delete: vi.fn(),
|
||||
getStatus: vi.fn(),
|
||||
answerQuestion: vi.fn(),
|
||||
spawnWithLifecycle: vi.fn(),
|
||||
} as unknown as AgentManager;
|
||||
}
|
||||
|
||||
function createLoggerMock(): Logger {
|
||||
return {
|
||||
info: vi.fn(),
|
||||
error: vi.fn(),
|
||||
warn: vi.fn(),
|
||||
debug: vi.fn(),
|
||||
trace: vi.fn(),
|
||||
fatal: vi.fn(),
|
||||
child: vi.fn(),
|
||||
} as unknown as Logger;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// computeQualifyingFiles
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('computeQualifyingFiles', () => {
|
||||
let branchManager: BranchManager;
|
||||
|
||||
beforeEach(() => {
|
||||
branchManager = createBranchManagerMock();
|
||||
});
|
||||
|
||||
it('includes .ts files', async () => {
|
||||
vi.mocked(branchManager.diffBranchesStat).mockResolvedValue([
|
||||
{ path: 'src/foo.ts', status: 'modified', additions: 5, deletions: 2 },
|
||||
]);
|
||||
expect(await computeQualifyingFiles(branchManager, '/repo', 'task-branch', 'base')).toEqual(['src/foo.ts']);
|
||||
});
|
||||
|
||||
it('includes .tsx and .js files', async () => {
|
||||
vi.mocked(branchManager.diffBranchesStat).mockResolvedValue([
|
||||
{ path: 'src/Comp.tsx', status: 'modified', additions: 3, deletions: 1 },
|
||||
{ path: 'src/util.js', status: 'added', additions: 10, deletions: 0 },
|
||||
]);
|
||||
const result = await computeQualifyingFiles(branchManager, '/repo', 'task-branch', 'base');
|
||||
expect(result).toContain('src/Comp.tsx');
|
||||
expect(result).toContain('src/util.js');
|
||||
});
|
||||
|
||||
it('excludes *.gen.ts files', async () => {
|
||||
vi.mocked(branchManager.diffBranchesStat).mockResolvedValue([
|
||||
{ path: 'src/routeTree.gen.ts', status: 'modified', additions: 1, deletions: 0 },
|
||||
]);
|
||||
expect(await computeQualifyingFiles(branchManager, '/repo', 'branch', 'base')).toEqual([]);
|
||||
});
|
||||
|
||||
it('excludes files starting with dist/', async () => {
|
||||
vi.mocked(branchManager.diffBranchesStat).mockResolvedValue([
|
||||
{ path: 'dist/index.js', status: 'modified', additions: 1, deletions: 0 },
|
||||
]);
|
||||
expect(await computeQualifyingFiles(branchManager, '/repo', 'branch', 'base')).toEqual([]);
|
||||
});
|
||||
|
||||
it('excludes files containing /dist/', async () => {
|
||||
vi.mocked(branchManager.diffBranchesStat).mockResolvedValue([
|
||||
{ path: 'packages/foo/dist/bar.js', status: 'modified', additions: 1, deletions: 0 },
|
||||
]);
|
||||
expect(await computeQualifyingFiles(branchManager, '/repo', 'branch', 'base')).toEqual([]);
|
||||
});
|
||||
|
||||
it('returns empty array when diffBranchesStat throws', async () => {
|
||||
vi.mocked(branchManager.diffBranchesStat).mockRejectedValue(new Error('branch not found'));
|
||||
expect(await computeQualifyingFiles(branchManager, '/repo', 'branch', 'base')).toEqual([]);
|
||||
});
|
||||
|
||||
it('returns only qualifying files from a mixed set', async () => {
|
||||
vi.mocked(branchManager.diffBranchesStat).mockResolvedValue([
|
||||
{ path: 'src/foo.ts', status: 'modified', additions: 5, deletions: 2 },
|
||||
{ path: 'src/routeTree.gen.ts', status: 'modified', additions: 1, deletions: 0 },
|
||||
{ path: 'dist/bundle.js', status: 'modified', additions: 1, deletions: 0 },
|
||||
{ path: 'src/bar.tsx', status: 'added', additions: 10, deletions: 0 },
|
||||
{ path: 'README.md', status: 'modified', additions: 2, deletions: 0 },
|
||||
]);
|
||||
const result = await computeQualifyingFiles(branchManager, '/repo', 'branch', 'base');
|
||||
expect(result).toEqual(['src/foo.ts', 'src/bar.tsx']);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// shouldRunQualityReview
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('shouldRunQualityReview', () => {
|
||||
let branchManager: BranchManager;
|
||||
let agentRepository: AgentRepository;
|
||||
let taskRepository: TaskRepository;
|
||||
let initiativeRepository: InitiativeRepository;
|
||||
let phaseRepository: PhaseRepository;
|
||||
|
||||
// Base params where all conditions pass
|
||||
let params: Parameters<typeof shouldRunQualityReview>[0];
|
||||
|
||||
beforeEach(() => {
|
||||
branchManager = createBranchManagerMock();
|
||||
agentRepository = createAgentRepositoryMock();
|
||||
taskRepository = createTaskRepositoryMock();
|
||||
initiativeRepository = createInitiativeRepositoryMock();
|
||||
phaseRepository = createPhaseRepositoryMock();
|
||||
|
||||
// Default diffBranchesStat returns qualifying file
|
||||
vi.mocked(branchManager.diffBranchesStat).mockResolvedValue([
|
||||
{ path: 'src/foo.ts', status: 'modified', additions: 5, deletions: 2 },
|
||||
]);
|
||||
|
||||
params = {
|
||||
agentId: 'a1',
|
||||
taskId: 't1',
|
||||
stopReason: 'task_complete',
|
||||
agentRepository,
|
||||
taskRepository,
|
||||
initiativeRepository,
|
||||
phaseRepository,
|
||||
branchManager,
|
||||
repoPath: '/repo',
|
||||
};
|
||||
});
|
||||
|
||||
it('returns false when stopReason is not task_complete', async () => {
|
||||
const result = await shouldRunQualityReview({ ...params, stopReason: 'error' });
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when agent is not found', async () => {
|
||||
vi.mocked(agentRepository.findById).mockResolvedValue(undefined as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when agent mode is errand', async () => {
|
||||
vi.mocked(agentRepository.findById).mockResolvedValue({ id: 'a1', mode: 'errand' } as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when task is not found', async () => {
|
||||
vi.mocked(taskRepository.findById).mockResolvedValue(undefined as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when task status is quality_review (recursion guard)', async () => {
|
||||
vi.mocked(taskRepository.findById).mockResolvedValue({
|
||||
id: 't1',
|
||||
status: 'quality_review',
|
||||
initiativeId: 'i1',
|
||||
phaseId: 'p1',
|
||||
} as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when task status is not in_progress', async () => {
|
||||
vi.mocked(taskRepository.findById).mockResolvedValue({
|
||||
id: 't1',
|
||||
status: 'pending',
|
||||
initiativeId: 'i1',
|
||||
phaseId: 'p1',
|
||||
} as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when task has no initiativeId', async () => {
|
||||
vi.mocked(taskRepository.findById).mockResolvedValue({
|
||||
id: 't1',
|
||||
status: 'in_progress',
|
||||
initiativeId: null,
|
||||
phaseId: 'p1',
|
||||
} as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when initiative is not found', async () => {
|
||||
vi.mocked(initiativeRepository.findById).mockResolvedValue(undefined as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when initiative qualityReview is false', async () => {
|
||||
vi.mocked(initiativeRepository.findById).mockResolvedValue({
|
||||
id: 'i1',
|
||||
qualityReview: false,
|
||||
branch: 'cw/test',
|
||||
} as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when task has no phaseId', async () => {
|
||||
vi.mocked(taskRepository.findById).mockResolvedValue({
|
||||
id: 't1',
|
||||
status: 'in_progress',
|
||||
initiativeId: 'i1',
|
||||
phaseId: null,
|
||||
} as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when phase is not found', async () => {
|
||||
vi.mocked(phaseRepository.findById).mockResolvedValue(undefined as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when initiative has no branch', async () => {
|
||||
vi.mocked(initiativeRepository.findById).mockResolvedValue({
|
||||
id: 'i1',
|
||||
qualityReview: true,
|
||||
branch: null,
|
||||
} as any);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns false when no qualifying files in changeset', async () => {
|
||||
vi.mocked(branchManager.diffBranchesStat).mockResolvedValue([
|
||||
{ path: 'src/routeTree.gen.ts', status: 'modified', additions: 1, deletions: 0 },
|
||||
]);
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result).toEqual({ run: false, qualifyingFiles: [] });
|
||||
});
|
||||
|
||||
it('returns true with qualifying files when all conditions pass', async () => {
|
||||
const result = await shouldRunQualityReview(params);
|
||||
expect(result.run).toBe(true);
|
||||
expect(result.qualifyingFiles).toContain('src/foo.ts');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// runQualityReview
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('runQualityReview', () => {
|
||||
let taskRepository: TaskRepository;
|
||||
let agentManager: AgentManager;
|
||||
let log: Logger;
|
||||
|
||||
let params: Parameters<typeof runQualityReview>[0];
|
||||
|
||||
beforeEach(() => {
|
||||
taskRepository = createTaskRepositoryMock();
|
||||
agentManager = createAgentManagerMock();
|
||||
log = createLoggerMock();
|
||||
|
||||
params = {
|
||||
taskId: 't1',
|
||||
taskBranch: 'cw/test-task-t1',
|
||||
baseBranch: 'cw/test-phase-impl',
|
||||
initiativeId: 'i1',
|
||||
qualifyingFiles: ['src/foo.ts', 'src/bar.ts'],
|
||||
taskRepository,
|
||||
agentManager,
|
||||
log,
|
||||
};
|
||||
});
|
||||
|
||||
it('transitions task to quality_review before spawning', async () => {
|
||||
await runQualityReview(params);
|
||||
expect(taskRepository.update).toHaveBeenCalledWith('t1', { status: 'quality_review' });
|
||||
// update called BEFORE spawn
|
||||
const updateOrder = vi.mocked(taskRepository.update).mock.invocationCallOrder[0]!;
|
||||
const spawnOrder = vi.mocked(agentManager.spawn).mock.invocationCallOrder[0]!;
|
||||
expect(updateOrder).toBeLessThan(spawnOrder);
|
||||
});
|
||||
|
||||
it('spawns agent with mode execute on the task branch', async () => {
|
||||
await runQualityReview(params);
|
||||
expect(agentManager.spawn).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
mode: 'execute',
|
||||
branchName: 'cw/test-task-t1',
|
||||
baseBranch: 'cw/test-phase-impl',
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('includes qualifying files in the prompt', async () => {
|
||||
await runQualityReview(params);
|
||||
const spawnArgs = vi.mocked(agentManager.spawn).mock.calls[0]![0];
|
||||
expect(spawnArgs.prompt).toContain('src/foo.ts');
|
||||
expect(spawnArgs.prompt).toContain('src/bar.ts');
|
||||
expect(spawnArgs.prompt).toContain('/simplify');
|
||||
});
|
||||
|
||||
it('spawns with taskId and initiativeId', async () => {
|
||||
await runQualityReview(params);
|
||||
expect(agentManager.spawn).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
taskId: 't1',
|
||||
initiativeId: 'i1',
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('on spawn failure: marks task completed and does not throw', async () => {
|
||||
vi.mocked(agentManager.spawn).mockRejectedValue(new Error('spawn failed'));
|
||||
await expect(runQualityReview(params)).resolves.toBeUndefined();
|
||||
// Last call to update should set status to completed
|
||||
const updateCalls = vi.mocked(taskRepository.update).mock.calls;
|
||||
const lastCall = updateCalls[updateCalls.length - 1]!;
|
||||
expect(lastCall).toEqual(['t1', { status: 'completed' }]);
|
||||
});
|
||||
|
||||
it('logs info after successful spawn', async () => {
|
||||
await runQualityReview(params);
|
||||
expect(log.info).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ taskId: 't1', reviewAgentId: 'review-agent-1' }),
|
||||
expect.any(String),
|
||||
);
|
||||
});
|
||||
|
||||
it('logs error on spawn failure', async () => {
|
||||
vi.mocked(agentManager.spawn).mockRejectedValue(new Error('spawn failed'));
|
||||
await runQualityReview(params);
|
||||
expect(log.error).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ taskId: 't1' }),
|
||||
expect.any(String),
|
||||
);
|
||||
});
|
||||
});
|
||||
175
apps/server/execution/quality-review.ts
Normal file
175
apps/server/execution/quality-review.ts
Normal file
@@ -0,0 +1,175 @@
|
||||
/**
|
||||
* Quality Review Dispatch Hook
|
||||
*
|
||||
* Intercepts agent:stopped events and, when conditions are met, spawns
|
||||
* a fresh execute-mode agent to run /simplify on changed files before
|
||||
* the task reaches 'completed' status.
|
||||
*/
|
||||
|
||||
import type { BranchManager } from '../git/branch-manager.js';
|
||||
import type { AgentManager } from '../agent/types.js';
|
||||
import type { AgentRepository } from '../db/repositories/agent-repository.js';
|
||||
import type { TaskRepository } from '../db/repositories/task-repository.js';
|
||||
import type { InitiativeRepository } from '../db/repositories/initiative-repository.js';
|
||||
import type { PhaseRepository } from '../db/repositories/phase-repository.js';
|
||||
import { phaseBranchName, taskBranchName } from '../git/branch-naming.js';
|
||||
import type { createModuleLogger } from '../logger/index.js';
|
||||
|
||||
type Logger = ReturnType<typeof createModuleLogger>;
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// computeQualifyingFiles
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* Returns the list of .ts/.tsx/.js files changed between taskBranch and baseBranch,
|
||||
* excluding generated files and dist artifacts.
|
||||
*/
|
||||
export async function computeQualifyingFiles(
|
||||
branchManager: BranchManager,
|
||||
repoPath: string,
|
||||
taskBranch: string,
|
||||
baseBranch: string,
|
||||
): Promise<string[]> {
|
||||
try {
|
||||
const entries = await branchManager.diffBranchesStat(repoPath, baseBranch, taskBranch);
|
||||
return entries
|
||||
.map((e) => e.path)
|
||||
.filter(
|
||||
(p) =>
|
||||
/\.(ts|tsx|js)$/.test(p) &&
|
||||
!p.endsWith('.gen.ts') &&
|
||||
!p.startsWith('dist/') &&
|
||||
!p.includes('/dist/'),
|
||||
);
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// shouldRunQualityReview
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
interface ShouldRunParams {
|
||||
agentId: string;
|
||||
taskId: string;
|
||||
stopReason: string;
|
||||
agentRepository: AgentRepository;
|
||||
taskRepository: TaskRepository;
|
||||
initiativeRepository: InitiativeRepository;
|
||||
phaseRepository: PhaseRepository;
|
||||
branchManager: BranchManager;
|
||||
repoPath: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Evaluates whether a quality review should be run for the stopped agent.
|
||||
* Returns `{ run: true, qualifyingFiles }` only when all conditions pass.
|
||||
* Short-circuits on first false condition.
|
||||
*/
|
||||
export async function shouldRunQualityReview(
|
||||
params: ShouldRunParams,
|
||||
): Promise<{ run: boolean; qualifyingFiles: string[] }> {
|
||||
const {
|
||||
agentId,
|
||||
taskId,
|
||||
stopReason,
|
||||
agentRepository,
|
||||
taskRepository,
|
||||
initiativeRepository,
|
||||
phaseRepository,
|
||||
branchManager,
|
||||
repoPath,
|
||||
} = params;
|
||||
|
||||
const NO = { run: false, qualifyingFiles: [] };
|
||||
|
||||
// 1. Only act on task_complete stops
|
||||
if (stopReason !== 'task_complete') return NO;
|
||||
|
||||
// 2. Agent must be in execute mode (guards against errand agents)
|
||||
const agent = await agentRepository.findById(agentId);
|
||||
if (!agent || agent.mode !== 'execute') return NO;
|
||||
|
||||
// 3. Task must be in_progress; quality_review is the recursion guard
|
||||
const task = await taskRepository.findById(taskId);
|
||||
if (!task) return NO;
|
||||
if (task.status === 'quality_review') return NO;
|
||||
if (task.status !== 'in_progress') return NO;
|
||||
|
||||
// 4. Task must belong to an initiative
|
||||
if (!task.initiativeId) return NO;
|
||||
|
||||
// 5. Initiative must have qualityReview enabled
|
||||
const initiative = await initiativeRepository.findById(task.initiativeId);
|
||||
if (!initiative || initiative.qualityReview !== true) return NO;
|
||||
|
||||
// 6. Compute branch names from task context
|
||||
if (!task.phaseId) return NO;
|
||||
const phase = await phaseRepository.findById(task.phaseId);
|
||||
if (!phase) return NO;
|
||||
|
||||
const initBranch = initiative.branch;
|
||||
if (!initBranch) return NO;
|
||||
|
||||
const base = phaseBranchName(initBranch, phase.name);
|
||||
const branch = taskBranchName(initBranch, task.id);
|
||||
|
||||
// 7. Must have qualifying files in the changeset
|
||||
const qualifyingFiles = await computeQualifyingFiles(branchManager, repoPath, branch, base);
|
||||
if (qualifyingFiles.length === 0) return NO;
|
||||
|
||||
return { run: true, qualifyingFiles };
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// runQualityReview
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
interface RunQualityReviewParams {
|
||||
taskId: string;
|
||||
taskBranch: string;
|
||||
baseBranch: string;
|
||||
initiativeId: string;
|
||||
qualifyingFiles: string[];
|
||||
taskRepository: TaskRepository;
|
||||
agentManager: AgentManager;
|
||||
log: Logger;
|
||||
}
|
||||
|
||||
/**
|
||||
* Transitions the task to quality_review and spawns a fresh execute-mode
|
||||
* agent to run /simplify on the changed files.
|
||||
*
|
||||
* On spawn failure: marks task completed and returns (never throws).
|
||||
*/
|
||||
export async function runQualityReview(params: RunQualityReviewParams): Promise<void> {
|
||||
const { taskId, taskBranch, baseBranch, initiativeId, qualifyingFiles, taskRepository, agentManager, log } = params;
|
||||
|
||||
// 1. Transition BEFORE spawning
|
||||
await taskRepository.update(taskId, { status: 'quality_review' });
|
||||
|
||||
// 2. Build prompt
|
||||
const fileList = qualifyingFiles.map((f) => `- ${f}`).join('\n');
|
||||
const reviewPrompt = `Run /simplify to review and fix code quality in this branch.\n\nFiles changed in this task:\n${fileList}`;
|
||||
|
||||
// 3. Spawn fresh execute-mode agent on the same task branch
|
||||
try {
|
||||
const reviewAgent = await agentManager.spawn({
|
||||
taskId,
|
||||
initiativeId,
|
||||
prompt: reviewPrompt,
|
||||
mode: 'execute',
|
||||
baseBranch,
|
||||
branchName: taskBranch,
|
||||
});
|
||||
|
||||
// 4. Log success
|
||||
log.info({ taskId, reviewAgentId: reviewAgent.id }, 'quality review agent spawned');
|
||||
} catch (err) {
|
||||
// 5. On spawn failure: mark completed and return — never block task completion
|
||||
log.error({ taskId, err }, 'quality review spawn failed; marking task completed');
|
||||
await taskRepository.update(taskId, { status: 'completed' });
|
||||
}
|
||||
}
|
||||
@@ -113,7 +113,7 @@ InitiativeChangesRequestedEvent { initiativeId, phaseId, taskId }
|
||||
| Event | Action |
|
||||
|-------|--------|
|
||||
| `phase:queued` | Dispatch ready phases → dispatch their tasks to idle agents |
|
||||
| `agent:stopped` | Auto-complete task (unless user_requested), re-dispatch queued tasks (freed agent slot) |
|
||||
| `agent:stopped` | When `task_complete`: check `shouldRunQualityReview()` — if conditions met, spawn quality-review agent and set task to `quality_review`; otherwise auto-complete task. Manual stops (`user_requested`) are skipped. Re-dispatch queued tasks after either path. |
|
||||
| `agent:crashed` | Auto-retry crashed task up to `MAX_TASK_RETRIES` (3). Increments `retryCount`, resets status to `pending`, re-queues. Exceeding retries leaves task `in_progress` for manual intervention. |
|
||||
| `task:completed` | Merge task branch (if branch exists), check phase completion, dispatch next queued task |
|
||||
|
||||
|
||||
Reference in New Issue
Block a user