diff --git a/apps/server/execution/quality-review.test.ts b/apps/server/execution/quality-review.test.ts new file mode 100644 index 0000000..ed8f780 --- /dev/null +++ b/apps/server/execution/quality-review.test.ts @@ -0,0 +1,582 @@ +/** + * Quality Review Service Tests + */ + +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 { AgentManager } from '../agent/types.js'; + +function makeBranchManager(overrides: Partial = {}): 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(), + ...overrides, + }; +} + +function makeAgentRepository(overrides: Partial = {}): AgentRepository { + return { + 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(), + ...overrides, + } as unknown as AgentRepository; +} + +function makeTaskRepository(overrides: Partial = {}): TaskRepository { + return { + create: vi.fn(), + findById: vi.fn().mockResolvedValue(null), + findByParentTaskId: vi.fn(), + findByInitiativeId: vi.fn(), + findByPhaseId: vi.fn(), + update: vi.fn().mockResolvedValue({}), + delete: vi.fn(), + createDependency: vi.fn(), + getDependencies: vi.fn(), + ...overrides, + } as unknown as TaskRepository; +} + +function makeInitiativeRepository(overrides: Partial = {}): InitiativeRepository { + return { + create: vi.fn(), + findById: vi.fn().mockResolvedValue(null), + findAll: vi.fn(), + findByStatus: vi.fn(), + update: vi.fn(), + findByProjectId: vi.fn(), + delete: vi.fn(), + ...overrides, + } as unknown as InitiativeRepository; +} + +function makeAgentManager(overrides: Partial = {}): AgentManager { + return { + spawn: vi.fn().mockResolvedValue({ id: 'review-agent-1' }), + stop: vi.fn(), + list: vi.fn(), + 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(), + ...overrides, + } as unknown as AgentManager; +} + +// --------------------------------------------------------------------------- +// computeQualifyingFiles +// --------------------------------------------------------------------------- + +describe('computeQualifyingFiles', () => { + it('includes .ts files', async () => { + const branchManager = makeBranchManager({ + diffBranchesStat: vi.fn().mockResolvedValue([ + { path: 'src/foo.ts', status: 'modified', additions: 5, deletions: 2 }, + ]), + }); + const result = await computeQualifyingFiles('/repo', 'task-branch', 'main', branchManager); + expect(result).toEqual(['src/foo.ts']); + }); + + it('includes .tsx, .js, .css, .json and other non-excluded types', async () => { + const branchManager = makeBranchManager({ + diffBranchesStat: vi.fn().mockResolvedValue([ + { path: 'src/App.tsx', status: 'modified', additions: 1, deletions: 0 }, + { path: 'src/utils.js', status: 'added', additions: 10, deletions: 0 }, + { path: 'src/style.css', status: 'modified', additions: 3, deletions: 1 }, + { path: 'config.json', status: 'modified', additions: 2, deletions: 2 }, + ]), + }); + const result = await computeQualifyingFiles('/repo', 'task-branch', 'main', branchManager); + expect(result).toEqual(['src/App.tsx', 'src/utils.js', 'src/style.css', 'config.json']); + }); + + it('excludes files ending with .gen.ts', async () => { + const branchManager = makeBranchManager({ + diffBranchesStat: vi.fn().mockResolvedValue([ + { path: 'src/foo.ts', status: 'modified', additions: 5, deletions: 2 }, + { path: 'src/routes.gen.ts', status: 'modified', additions: 100, deletions: 0 }, + { path: 'types.gen.ts', status: 'added', additions: 50, deletions: 0 }, + ]), + }); + const result = await computeQualifyingFiles('/repo', 'task-branch', 'main', branchManager); + expect(result).toEqual(['src/foo.ts']); + }); + + it('excludes files under dist/', async () => { + const branchManager = makeBranchManager({ + diffBranchesStat: vi.fn().mockResolvedValue([ + { path: 'src/foo.ts', status: 'modified', additions: 5, deletions: 2 }, + { path: 'dist/bundle.js', status: 'modified', additions: 1, deletions: 0 }, + { path: 'apps/server/dist/index.js', status: 'added', additions: 10, deletions: 0 }, + { path: 'packages/foo/dist/foo.js', status: 'modified', additions: 3, deletions: 0 }, + ]), + }); + const result = await computeQualifyingFiles('/repo', 'task-branch', 'main', branchManager); + expect(result).toEqual(['src/foo.ts']); + }); + + it('returns empty array when diff throws', async () => { + const branchManager = makeBranchManager({ + diffBranchesStat: vi.fn().mockRejectedValue(new Error('branch not found')), + }); + const result = await computeQualifyingFiles('/repo', 'task-branch', 'main', branchManager); + expect(result).toEqual([]); + }); + + it('passes baseBranch as first branch arg and taskBranch as second to diffBranchesStat', async () => { + const diffSpy = vi.fn().mockResolvedValue([]); + const branchManager = makeBranchManager({ diffBranchesStat: diffSpy }); + await computeQualifyingFiles('/repo', 'task-branch', 'main', branchManager); + expect(diffSpy).toHaveBeenCalledWith('/repo', 'main', 'task-branch'); + }); +}); + +// --------------------------------------------------------------------------- +// shouldRunQualityReview +// --------------------------------------------------------------------------- + +describe('shouldRunQualityReview', () => { + const BASE_PARAMS = { + agentId: 'agent-1', + taskId: 'task-1', + stopReason: 'task_complete', + repoPath: '/repo', + taskBranch: 'cw/init-task-task-1', + baseBranch: 'main', + }; + + it('returns false when stopReason is not task_complete', async () => { + const agentRepository = makeAgentRepository(); + const taskRepository = makeTaskRepository(); + const initiativeRepository = makeInitiativeRepository(); + const branchManager = makeBranchManager(); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + stopReason: 'error', + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: false, qualifyingFiles: [] }); + // Should short-circuit: no repo lookups + expect(agentRepository.findById).not.toHaveBeenCalled(); + }); + + it('returns false when agent mode is errand', async () => { + const agentRepository = makeAgentRepository({ + findById: vi.fn().mockResolvedValue({ id: 'agent-1', mode: 'errand' }), + }); + const taskRepository = makeTaskRepository(); + const initiativeRepository = makeInitiativeRepository(); + const branchManager = makeBranchManager(); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: false, qualifyingFiles: [] }); + expect(taskRepository.findById).not.toHaveBeenCalled(); + }); + + it('returns false when agent is not found', async () => { + const agentRepository = makeAgentRepository({ + findById: vi.fn().mockResolvedValue(null), + }); + const taskRepository = makeTaskRepository(); + const initiativeRepository = makeInitiativeRepository(); + const branchManager = makeBranchManager(); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: false, qualifyingFiles: [] }); + }); + + it('returns false when task status is quality_review (recursion guard)', async () => { + const agentRepository = makeAgentRepository({ + findById: vi.fn().mockResolvedValue({ id: 'agent-1', mode: 'execute' }), + }); + const taskRepository = makeTaskRepository({ + findById: vi.fn().mockResolvedValue({ + id: 'task-1', + status: 'quality_review', + initiativeId: 'init-1', + }), + }); + const initiativeRepository = makeInitiativeRepository(); + const branchManager = makeBranchManager(); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: false, qualifyingFiles: [] }); + expect(initiativeRepository.findById).not.toHaveBeenCalled(); + }); + + it('returns false when task status is not in_progress and not quality_review', async () => { + const agentRepository = makeAgentRepository({ + findById: vi.fn().mockResolvedValue({ id: 'agent-1', mode: 'execute' }), + }); + const taskRepository = makeTaskRepository({ + findById: vi.fn().mockResolvedValue({ + id: 'task-1', + status: 'completed', + initiativeId: 'init-1', + }), + }); + const initiativeRepository = makeInitiativeRepository(); + const branchManager = makeBranchManager(); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: false, qualifyingFiles: [] }); + }); + + it('returns false when task is not found', async () => { + const agentRepository = makeAgentRepository({ + findById: vi.fn().mockResolvedValue({ id: 'agent-1', mode: 'execute' }), + }); + const taskRepository = makeTaskRepository({ + findById: vi.fn().mockResolvedValue(null), + }); + const initiativeRepository = makeInitiativeRepository(); + const branchManager = makeBranchManager(); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: false, qualifyingFiles: [] }); + }); + + it('returns false when task has no initiativeId', async () => { + const agentRepository = makeAgentRepository({ + findById: vi.fn().mockResolvedValue({ id: 'agent-1', mode: 'execute' }), + }); + const taskRepository = makeTaskRepository({ + findById: vi.fn().mockResolvedValue({ + id: 'task-1', + status: 'in_progress', + initiativeId: null, + }), + }); + const initiativeRepository = makeInitiativeRepository(); + const branchManager = makeBranchManager(); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: false, qualifyingFiles: [] }); + expect(initiativeRepository.findById).not.toHaveBeenCalled(); + }); + + it('returns false when initiative.qualityReview is false', async () => { + const agentRepository = makeAgentRepository({ + findById: vi.fn().mockResolvedValue({ id: 'agent-1', mode: 'execute' }), + }); + const taskRepository = makeTaskRepository({ + findById: vi.fn().mockResolvedValue({ + id: 'task-1', + status: 'in_progress', + initiativeId: 'init-1', + }), + }); + const initiativeRepository = makeInitiativeRepository({ + findById: vi.fn().mockResolvedValue({ id: 'init-1', qualityReview: false }), + }); + const branchManager = makeBranchManager({ + diffBranchesStat: vi.fn().mockResolvedValue([ + { path: 'src/foo.ts', status: 'modified', additions: 5, deletions: 2 }, + ]), + }); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: false, qualifyingFiles: [] }); + expect(branchManager.diffBranchesStat).not.toHaveBeenCalled(); + }); + + it('returns false when initiative is not found', async () => { + const agentRepository = makeAgentRepository({ + findById: vi.fn().mockResolvedValue({ id: 'agent-1', mode: 'execute' }), + }); + const taskRepository = makeTaskRepository({ + findById: vi.fn().mockResolvedValue({ + id: 'task-1', + status: 'in_progress', + initiativeId: 'init-1', + }), + }); + const initiativeRepository = makeInitiativeRepository({ + findById: vi.fn().mockResolvedValue(null), + }); + const branchManager = makeBranchManager(); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: false, qualifyingFiles: [] }); + }); + + it('returns false when no qualifying files in changeset', async () => { + const agentRepository = makeAgentRepository({ + findById: vi.fn().mockResolvedValue({ id: 'agent-1', mode: 'execute' }), + }); + const taskRepository = makeTaskRepository({ + findById: vi.fn().mockResolvedValue({ + id: 'task-1', + status: 'in_progress', + initiativeId: 'init-1', + }), + }); + const initiativeRepository = makeInitiativeRepository({ + findById: vi.fn().mockResolvedValue({ id: 'init-1', qualityReview: true }), + }); + const branchManager = makeBranchManager({ + diffBranchesStat: vi.fn().mockResolvedValue([ + { path: 'dist/bundle.js', status: 'modified', additions: 10, deletions: 0 }, + { path: 'src/routes.gen.ts', status: 'added', additions: 50, deletions: 0 }, + ]), + }); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: false, qualifyingFiles: [] }); + }); + + it('returns true with qualifying files when all conditions pass', async () => { + const agentRepository = makeAgentRepository({ + findById: vi.fn().mockResolvedValue({ id: 'agent-1', mode: 'execute' }), + }); + const taskRepository = makeTaskRepository({ + findById: vi.fn().mockResolvedValue({ + id: 'task-1', + status: 'in_progress', + initiativeId: 'init-1', + }), + }); + const initiativeRepository = makeInitiativeRepository({ + findById: vi.fn().mockResolvedValue({ id: 'init-1', qualityReview: true }), + }); + const branchManager = makeBranchManager({ + diffBranchesStat: vi.fn().mockResolvedValue([ + { path: 'src/foo.ts', status: 'modified', additions: 5, deletions: 2 }, + { path: 'src/bar.ts', status: 'added', additions: 20, deletions: 0 }, + ]), + }); + + const result = await shouldRunQualityReview({ + ...BASE_PARAMS, + agentRepository, + taskRepository, + initiativeRepository, + branchManager, + }); + + expect(result).toEqual({ run: true, qualifyingFiles: ['src/foo.ts', 'src/bar.ts'] }); + }); +}); + +// --------------------------------------------------------------------------- +// runQualityReview +// --------------------------------------------------------------------------- + +describe('runQualityReview', () => { + const BASE_RUN_PARAMS = { + taskId: 'task-1', + taskBranch: 'cw/init-task-task-1', + baseBranch: 'main', + initiativeId: 'init-1', + qualifyingFiles: ['src/foo.ts', 'src/bar.ts'], + }; + + const makeLog = () => ({ + info: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + debug: vi.fn(), + trace: vi.fn(), + fatal: vi.fn(), + child: vi.fn(), + }); + + it('transitions task status to quality_review before spawning', async () => { + const taskRepository = makeTaskRepository(); + const agentManager = makeAgentManager(); + const log = makeLog(); + + await runQualityReview({ + ...BASE_RUN_PARAMS, + taskRepository, + agentManager, + log: log as any, + }); + + const updateCalls = vi.mocked(taskRepository.update).mock.calls; + expect(updateCalls[0]).toEqual(['task-1', { status: 'quality_review' }]); + // spawn should come after the update + expect(agentManager.spawn).toHaveBeenCalled(); + }); + + it('calls agentManager.spawn with mode execute and correct branchName', async () => { + const taskRepository = makeTaskRepository(); + const agentManager = makeAgentManager(); + const log = makeLog(); + + await runQualityReview({ + ...BASE_RUN_PARAMS, + taskRepository, + agentManager, + log: log as any, + }); + + expect(agentManager.spawn).toHaveBeenCalledWith( + expect.objectContaining({ + taskId: 'task-1', + initiativeId: 'init-1', + mode: 'execute', + baseBranch: 'main', + branchName: 'cw/init-task-task-1', + }), + ); + }); + + it('prompt includes /simplify instruction and qualifying files', async () => { + const taskRepository = makeTaskRepository(); + const agentManager = makeAgentManager(); + const log = makeLog(); + + await runQualityReview({ + ...BASE_RUN_PARAMS, + taskRepository, + agentManager, + log: log as any, + }); + + const spawnCall = vi.mocked(agentManager.spawn).mock.calls[0][0]; + expect(spawnCall.prompt).toContain('/simplify'); + expect(spawnCall.prompt).toContain('src/foo.ts'); + expect(spawnCall.prompt).toContain('src/bar.ts'); + }); + + it('logs reviewAgentId at info level after spawn', async () => { + const taskRepository = makeTaskRepository(); + const agentManager = makeAgentManager({ + spawn: vi.fn().mockResolvedValue({ id: 'review-agent-42' }), + }); + const log = makeLog(); + + await runQualityReview({ + ...BASE_RUN_PARAMS, + taskRepository, + agentManager, + log: log as any, + }); + + expect(log.info).toHaveBeenCalledWith( + expect.objectContaining({ taskId: 'task-1', reviewAgentId: 'review-agent-42' }), + expect.any(String), + ); + }); + + it('on spawn failure: transitions task to completed and does not throw', async () => { + const taskRepository = makeTaskRepository(); + const agentManager = makeAgentManager({ + spawn: vi.fn().mockRejectedValue(new Error('spawn failed')), + }); + const log = makeLog(); + + await expect( + runQualityReview({ + ...BASE_RUN_PARAMS, + taskRepository, + agentManager, + log: log as any, + }), + ).resolves.toBeUndefined(); + + expect(log.error).toHaveBeenCalled(); + expect(taskRepository.update).toHaveBeenCalledWith('task-1', { status: 'completed' }); + }); +}); diff --git a/apps/server/execution/quality-review.ts b/apps/server/execution/quality-review.ts new file mode 100644 index 0000000..f54eb35 --- /dev/null +++ b/apps/server/execution/quality-review.ts @@ -0,0 +1,152 @@ +/** + * Quality Review Service + * + * Decides whether to run a quality review after a task agent completes, + * and orchestrates spawning the review agent. + * + * All dependencies are passed as function parameters (hexagonal DI pattern). + */ + +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 { AgentManager } from '../agent/types.js'; +import type { Logger } from 'pino'; + +// --------------------------------------------------------------------------- +// computeQualifyingFiles +// --------------------------------------------------------------------------- + +/** + * Compute source files in the diff between taskBranch and baseBranch that + * qualify for a quality review (excludes *.gen.ts and dist/ paths). + * + * Returns [] if the diff throws (treated as no qualifying files). + */ +export async function computeQualifyingFiles( + repoPath: string, + taskBranch: string, + baseBranch: string, + branchManager: BranchManager, +): Promise { + try { + const entries = await branchManager.diffBranchesStat(repoPath, baseBranch, taskBranch); + return entries + .map((e) => e.path) + .filter((p) => !p.endsWith('.gen.ts')) + .filter((p) => !p.startsWith('dist/') && !p.includes('/dist/')); + } catch { + return []; + } +} + +// --------------------------------------------------------------------------- +// shouldRunQualityReview +// --------------------------------------------------------------------------- + +export interface QualityReviewCheckParams { + agentId: string; + taskId: string; + stopReason: string; + repoPath: string; + taskBranch: string; + baseBranch: string; + agentRepository: AgentRepository; + taskRepository: TaskRepository; + initiativeRepository: InitiativeRepository; + branchManager: BranchManager; +} + +/** + * Determine whether a quality review should be run for the given agent stop event. + * + * Returns { run: true, qualifyingFiles } only when all six conditions pass: + * 1. stopReason === 'task_complete' + * 2. Agent mode is 'execute' + * 3. Task status is 'in_progress' (not 'quality_review' — recursion guard) + * 4. task.initiativeId is non-null + * 5. initiative.qualityReview === true + * 6. computeQualifyingFiles() returns a non-empty array + */ +export async function shouldRunQualityReview( + params: QualityReviewCheckParams, +): Promise<{ run: boolean; qualifyingFiles: string[] }> { + const { agentId, taskId, stopReason, repoPath, taskBranch, baseBranch, agentRepository, taskRepository, initiativeRepository, branchManager } = params; + const NO = { run: false, qualifyingFiles: [] }; + + // 1. Only fire on task_complete + if (stopReason !== 'task_complete') return NO; + + // 2. Agent mode must be 'execute' + const agent = await agentRepository.findById(agentId); + if (!agent || agent.mode !== 'execute') return NO; + + // 3. Task status must be 'in_progress' (recursion guard: skip if already quality_review) + 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) return NO; + + // 6. Must have qualifying files in the changeset + const qualifyingFiles = await computeQualifyingFiles(repoPath, taskBranch, baseBranch, branchManager); + if (qualifyingFiles.length === 0) return NO; + + return { run: true, qualifyingFiles }; +} + +// --------------------------------------------------------------------------- +// runQualityReview +// --------------------------------------------------------------------------- + +export interface QualityReviewRunParams { + taskId: string; + taskBranch: string; + baseBranch: string; + initiativeId: string; + qualifyingFiles: string[]; + taskRepository: TaskRepository; + agentManager: AgentManager; + log: Logger; +} + +/** + * Spawn a quality review agent on the task branch. + * + * 1. Transitions task to 'quality_review' + * 2. Builds /simplify prompt with qualifying files + * 3. Spawns execute-mode agent on the same task branch + * 4. Logs the review agent ID + * 5. On spawn error: logs and transitions task to 'completed' — never throws + */ +export async function runQualityReview(params: QualityReviewRunParams): Promise { + const { taskId, taskBranch, baseBranch, initiativeId, qualifyingFiles, taskRepository, agentManager, log } = params; + + await taskRepository.update(taskId, { status: 'quality_review' }); + + const fileList = qualifyingFiles.join('\n'); + const prompt = `Run /simplify to review and fix code quality in this branch.\n\n${fileList}`; + + try { + const reviewAgent = await agentManager.spawn({ + taskId, + initiativeId, + prompt, + mode: 'execute', + baseBranch, + branchName: taskBranch, + }); + + log.info({ taskId, reviewAgentId: reviewAgent.id }, 'quality review agent spawned'); + } catch (err) { + log.error({ taskId, err: err instanceof Error ? err.message : String(err) }, 'quality review agent spawn failed'); + await taskRepository.update(taskId, { status: 'completed' }); + } +}