diff --git a/src/lib/merge-train.ts b/src/lib/merge-train.ts index 496d8ad..42c7e0d 100644 --- a/src/lib/merge-train.ts +++ b/src/lib/merge-train.ts @@ -50,6 +50,71 @@ const INSTALL_COMMAND_BY_LOCKFILE = [ +export async function validateTouchSet( + jobBranch: string, + baseBranch: string, + touchSet: string[], + opts?: { cwd?: string }, +): Promise<{ valid: boolean; violations?: string[]; changedFiles?: string[] }> { + if (touchSet.length === 0) { + return { valid: true }; + } + + const diffResult = await gitCommand(['diff', '--name-only', `${baseBranch}...${jobBranch}`], opts); + if (diffResult.exitCode !== 0) { + return { valid: false, violations: [`Failed to diff: ${diffResult.stderr}`] }; + } + + const changedFiles = diffResult.stdout.split('\n').map(f => f.trim()).filter(Boolean); + if (changedFiles.length === 0) { + return { valid: true, changedFiles: [] }; + } + + const violations: string[] = []; + for (const file of changedFiles) { + const matchesAny = touchSet.some(pattern => { + const glob = new Bun.Glob(pattern); + return glob.match(file); + }); + if (!matchesAny) { + violations.push(file); + } + } + + return { + valid: violations.length === 0, + violations: violations.length > 0 ? violations : undefined, + changedFiles, + }; +} + +export async function checkMergeability( + integrationWorktree: string, + jobBranch: string, +): Promise<{ canMerge: boolean; conflicts?: string[] }> { + const testMerge = await gitCommand([ + '-C', integrationWorktree, + 'merge', '--no-commit', '--no-ff', jobBranch, + ]); + + // Always clean up — leave worktree pristine + await gitCommand(['-C', integrationWorktree, 'merge', '--abort']).catch(() => {}); + await gitCommand(['-C', integrationWorktree, 'reset', '--hard', 'HEAD']).catch(() => {}); + await gitCommand(['-C', integrationWorktree, 'clean', '-fd']).catch(() => {}); + + if (testMerge.exitCode !== 0) { + const conflicts = extractConflicts( + [testMerge.stdout, testMerge.stderr].filter(Boolean).join('\n'), + ); + return { + canMerge: false, + conflicts: conflicts.length > 0 ? conflicts : undefined, + }; + } + + return { canMerge: true }; +} + async function rollbackMerge(worktreePath: string): Promise { // Try merge --abort first await gitCommand(['-C', worktreePath, 'merge', '--abort']).catch(() => {}); diff --git a/src/lib/orchestrator.ts b/src/lib/orchestrator.ts index cda54c6..6553148 100644 --- a/src/lib/orchestrator.ts +++ b/src/lib/orchestrator.ts @@ -4,7 +4,7 @@ import type { PlanSpec, JobSpec, PlanStatus, CheckpointType } from './plan-types import { loadPlan, savePlan, updatePlanJob, clearPlan, validateGhAuth } from './plan-state'; import { getDefaultBranch } from './git'; import { createIntegrationBranch, deleteIntegrationBranch } from './integration'; -import { MergeTrain, type MergeTestReport } from './merge-train'; +import { MergeTrain, checkMergeability, type MergeTestReport, validateTouchSet } from './merge-train'; import { addJob, getRunningJobs, updateJob, loadJobState, removeJob, type Job } from './job-state'; import { JobMonitor } from './monitor'; import { removeReport } from './reports'; @@ -473,6 +473,22 @@ export class Orchestrator { for (const job of mergeOrder) { if (job.status === 'completed') { + if (job.touchSet && job.touchSet.length > 0 && job.branch && plan.integrationBranch) { + const validation = await validateTouchSet(job.branch, plan.integrationBranch, job.touchSet); + if (!validation.valid && validation.violations) { + await updatePlanJob(plan.id, job.name, { + status: 'failed', + error: `Modified files outside touchSet: ${validation.violations.join(', ')}. Expected only: ${job.touchSet.join(', ')}`, + }); + job.status = 'failed'; + + this.showToast('Mission Control', `Job "${job.name}" touched files outside its touchSet. Plan paused.`, 'error'); + this.notify(`❌ Job "${job.name}" modified files outside its touchSet:\n Violations: ${validation.violations.join(', ')}\n Allowed: ${job.touchSet.join(', ')}\nFix the branch and retry with mc_plan_approve(checkpoint: "on_error", retry: "${job.name}").`); + await this.setCheckpoint('on_error', plan); + return; + } + } + await updatePlanJob(plan.id, job.name, { status: 'ready_to_merge' }); job.status = 'ready_to_merge'; } @@ -494,6 +510,22 @@ export class Orchestrator { continue; } + if (job.branch && plan.integrationWorktree) { + const mergeCheck = await checkMergeability(plan.integrationWorktree, job.branch); + if (!mergeCheck.canMerge) { + await updatePlanJob(plan.id, job.name, { + status: 'needs_rebase', + error: mergeCheck.conflicts?.join(', ') ?? 'merge conflict detected in trial merge', + }); + job.status = 'needs_rebase'; + + this.showToast('Mission Control', `Job "${job.name}" has merge conflicts. Plan paused.`, 'error'); + this.notify(`❌ Job "${job.name}" would conflict with the integration branch.\n Files: ${mergeCheck.conflicts?.join(', ') ?? 'unknown'}\nRebase the job branch and retry with mc_plan_approve(checkpoint: "on_error", retry: "${job.name}").`); + await this.setCheckpoint('on_error', plan); + return; + } + } + if (this.isSupervisor(plan) && !this.approvedForMerge.has(job.name)) { await this.setCheckpoint('pre_merge', plan); return; @@ -538,14 +570,10 @@ export class Orchestrator { error: mergeResult.files?.join(', ') ?? 'merge conflict', }); - if (this.isSupervisor(plan)) { - await this.setCheckpoint('on_error', plan); - return; - } - - plan.status = 'failed'; - this.showToast('Mission Control', `Merge conflict in job "${nextJob.name}".`, 'error'); - this.notify(`❌ Merge conflict in job "${nextJob.name}". Files: ${mergeResult.files?.join(', ') ?? 'unknown'}. Plan failed.`); + this.showToast('Mission Control', `Merge conflict in job "${nextJob.name}". Plan paused.`, 'error'); + this.notify(`❌ Merge conflict in job "${nextJob.name}". Files: ${mergeResult.files?.join(', ') ?? 'unknown'}. Fix the branch and retry with mc_plan_approve(checkpoint: "on_error", retry: "${nextJob.name}").`); + await this.setCheckpoint('on_error', plan); + return; } else { await updatePlanJob(plan.id, nextJob.name, { status: 'failed', @@ -557,14 +585,10 @@ export class Orchestrator { this.notify(`🧪 ${nextJob.name}: ${testSummary}`); } - if (this.isSupervisor(plan)) { - await this.setCheckpoint('on_error', plan); - return; - } - - plan.status = 'failed'; - this.showToast('Mission Control', `Job "${nextJob.name}" failed during merge.`, 'error'); - this.notify(`❌ Job "${nextJob.name}" failed merge tests. Plan failed.`); + this.showToast('Mission Control', `Job "${nextJob.name}" failed merge tests. Plan paused.`, 'error'); + this.notify(`❌ Job "${nextJob.name}" failed merge tests. Fix the branch and retry with mc_plan_approve(checkpoint: "on_error", retry: "${nextJob.name}").`); + await this.setCheckpoint('on_error', plan); + return; } } @@ -763,22 +787,24 @@ If your work needs human review before it can proceed: mc_report(status: "needs_ } private handleJobComplete = (job: Job): void => { - if (job.planId && this.activePlanId && job.planId === this.activePlanId) { - if (!this.firstJobCompleted) { - this.firstJobCompleted = true; - this.showToast('Mission Control', `First job completed: "${job.name}".`, 'success'); - } + if (!job.planId || !this.activePlanId || job.planId !== this.activePlanId) { + return; + } - updatePlanJob(job.planId, job.name, { - status: 'completed', - }).catch((error) => { - console.error('Failed to update completed job state:', error); - }); + if (!this.firstJobCompleted) { + this.firstJobCompleted = true; + this.showToast('Mission Control', `First job completed: "${job.name}".`, 'success'); + } - this.reconcile().catch((error) => { - console.error('Reconcile after completion failed:', error); + const planId = job.planId; + (async () => { + await updatePlanJob(planId, job.name, { + status: 'completed', }); - } + await this.reconcile(); + })().catch((error) => { + console.error('Failed to reconcile completed job state:', error); + }); } private handleJobFailed = (job: Job): void => { @@ -794,16 +820,9 @@ If your work needs human review before it can proceed: mc_report(status: "needs_ return; } - if (this.isSupervisor(plan)) { - await this.setCheckpoint('on_error', plan); - return; - } - - plan.status = 'failed'; - plan.completedAt = new Date().toISOString(); - await savePlan(plan); - this.showToast('Mission Control', `Plan failed: job "${job.name}" failed.`, 'error'); - this.notify(`❌ Plan failed: job "${job.name}" failed.`); + this.showToast('Mission Control', `Job "${job.name}" failed. Plan paused.`, 'error'); + this.notify(`❌ Job "${job.name}" failed. Fix and retry with mc_plan_approve(checkpoint: "on_error", retry: "${job.name}").`); + await this.setCheckpoint('on_error', plan); }) .catch(() => {}) .finally(() => { diff --git a/src/lib/plan-types.ts b/src/lib/plan-types.ts index ee03af9..3488afb 100644 --- a/src/lib/plan-types.ts +++ b/src/lib/plan-types.ts @@ -75,8 +75,8 @@ export const VALID_JOB_TRANSITIONS: Record = { waiting_deps: ['running', 'stopped', 'canceled'], running: ['completed', 'failed', 'stopped', 'canceled'], completed: ['ready_to_merge', 'stopped', 'canceled'], - failed: ['stopped', 'canceled'], - ready_to_merge: ['merging', 'stopped', 'canceled'], + failed: ['ready_to_merge', 'stopped', 'canceled'], + ready_to_merge: ['merging', 'needs_rebase', 'stopped', 'canceled'], merging: ['merged', 'conflict', 'stopped', 'canceled'], merged: ['needs_rebase'], conflict: ['ready_to_merge', 'stopped', 'canceled'], diff --git a/src/tools/plan-approve.ts b/src/tools/plan-approve.ts index d3e316d..970da90 100644 --- a/src/tools/plan-approve.ts +++ b/src/tools/plan-approve.ts @@ -1,5 +1,5 @@ import { tool, type ToolDefinition } from '@opencode-ai/plugin'; -import { loadPlan, savePlan } from '../lib/plan-state'; +import { loadPlan, savePlan, updatePlanJob } from '../lib/plan-state'; import { Orchestrator } from '../lib/orchestrator'; import { getSharedMonitor, getSharedNotifyCallback, setSharedOrchestrator } from '../lib/orchestrator-singleton'; import type { CheckpointType } from '../lib/plan-types'; @@ -10,12 +10,16 @@ import { resolvePostCreateHook } from '../lib/worktree-setup'; export const mc_plan_approve: ToolDefinition = tool({ description: - 'Approve a pending copilot plan or clear a supervisor checkpoint to continue execution', + 'Approve a pending copilot plan, clear a supervisor checkpoint, or retry a failed job to continue execution', args: { checkpoint: tool.schema .enum(['pre_merge', 'on_error', 'pre_pr']) .optional() .describe('Specific checkpoint to clear (for supervisor mode)'), + retry: tool.schema + .string() + .optional() + .describe('Name of a failed, conflict, or needs_rebase job to retry'), }, async execute(args) { const plan = await loadPlan(); @@ -23,8 +27,30 @@ export const mc_plan_approve: ToolDefinition = tool({ throw new Error('No active plan to approve'); } + if (args.retry) { + const job = plan.jobs.find((j) => j.name === args.retry); + if (!job) { + throw new Error(`Job "${args.retry}" not found in plan`); + } + if (job.status !== 'failed' && job.status !== 'conflict' && job.status !== 'needs_rebase') { + throw new Error(`Job "${args.retry}" is not in a retryable state (current: ${job.status}). Only failed, conflict, or needs_rebase jobs can be retried.`); + } + } + if (plan.status === 'paused' && plan.checkpoint) { const checkpoint = (args.checkpoint ?? plan.checkpoint) as CheckpointType; + + if (args.retry) { + const job = plan.jobs.find(j => j.name === args.retry); + if (!job) { + throw new Error(`Job "${args.retry}" not found in plan`); + } + if (job.status !== 'failed' && job.status !== 'conflict' && job.status !== 'needs_rebase') { + throw new Error(`Job "${args.retry}" is not in a retryable state (current: ${job.status}). Only failed, conflict, or needs_rebase jobs can be retried.`); + } + await updatePlanJob(plan.id, args.retry, { status: 'ready_to_merge', error: undefined }); + } + plan.status = 'running'; plan.checkpoint = null; await savePlan(plan); @@ -35,8 +61,9 @@ export const mc_plan_approve: ToolDefinition = tool({ orchestrator.setPlanModelSnapshot(getCurrentModel()); await orchestrator.resumePlan(); + const retryMsg = args.retry ? ` Job "${args.retry}" reset to ready_to_merge.` : ''; return [ - `Checkpoint "${checkpoint}" cleared. Plan "${plan.name}" resuming.`, + `Checkpoint "${checkpoint}" cleared.${retryMsg} Plan "${plan.name}" resuming.`, '', ` ID: ${plan.id}`, ` Mode: ${plan.mode}`, diff --git a/tests/integration/orchestration.test.ts b/tests/integration/orchestration.test.ts index e4cc511..d6a692e 100644 --- a/tests/integration/orchestration.test.ts +++ b/tests/integration/orchestration.test.ts @@ -447,7 +447,7 @@ describe('orchestration integration', () => { expect(tmux.sendKeys).toHaveBeenCalledTimes(0); }, 30000); - it('marks plan failed when a merge conflict occurs and leaves integration clean', async () => { + it('pauses plan on merge conflict for retry instead of failing', async () => { const monitor = new FakeMonitor(); mockTmux(); const orchestrator = new Orchestrator(monitor as any, { @@ -497,22 +497,23 @@ describe('orchestration integration', () => { await waitForCondition(async () => { const currentPlan = await loadPlan(); const second = currentPlan?.jobs.find((job) => job.id === 'job-2'); - return currentPlan?.status === 'failed' && second?.status === 'conflict'; + return currentPlan?.status === 'paused' && second?.status === 'needs_rebase'; }, 8000); - const failedPlan = await loadPlan(); - expect(failedPlan?.status).toBe('failed'); - expect(failedPlan?.jobs.find((job) => job.id === 'job-2')?.status).toBe('conflict'); + const pausedPlan = await loadPlan(); + expect(pausedPlan?.status).toBe('paused'); + expect(pausedPlan?.checkpoint).toBe('on_error'); + expect(pausedPlan?.jobs.find((job) => job.id === 'job-2')?.status).toBe('needs_rebase'); const status = await mustExec( ['git', 'status', '--porcelain'], - failedPlan!.integrationWorktree!, + pausedPlan!.integrationWorktree!, ); expect(status).toBe(''); const mergeHead = await exec( ['git', 'rev-parse', '-q', '--verify', 'MERGE_HEAD'], - failedPlan!.integrationWorktree!, + pausedPlan!.integrationWorktree!, ); expect(mergeHead.exitCode).not.toBe(0); }, 30000); diff --git a/tests/lib/merge-train.test.ts b/tests/lib/merge-train.test.ts index b303e1d..d59e695 100644 --- a/tests/lib/merge-train.test.ts +++ b/tests/lib/merge-train.test.ts @@ -3,7 +3,7 @@ import { join } from 'path'; import { existsSync, mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'fs'; import { tmpdir } from 'os'; import type { JobSpec } from '../../src/lib/plan-types'; -import { MergeTrain, detectInstallCommand, detectTestCommand } from '../../src/lib/merge-train'; +import { MergeTrain, checkMergeability, detectInstallCommand, detectTestCommand, validateTouchSet } from '../../src/lib/merge-train'; type TestRepo = { rootDir: string; @@ -412,3 +412,169 @@ describe('MergeTrain', () => { expect(train.getQueue().length).toBe(1); }); }); + +describe('validateTouchSet', () => { + let testRepo: TestRepo; + + beforeEach(async () => { + testRepo = await setupRepo(); + }); + + afterEach(() => { + rmSync(testRepo.rootDir, { recursive: true, force: true }); + }); + + it('should return valid when touchSet is empty', async () => { + await createBranchCommit(testRepo.repoDir, 'feature-empty-ts', 'foo.txt', 'foo\n'); + + const result = await validateTouchSet('feature-empty-ts', 'main', [], { cwd: testRepo.repoDir }); + expect(result.valid).toBe(true); + }); + + it('should return valid when all changed files match touchSet globs', async () => { + await mustExec(['git', 'checkout', '-b', 'feature-match', 'main'], testRepo.repoDir); + mkdirSync(join(testRepo.repoDir, 'src'), { recursive: true }); + writeFileSync(join(testRepo.repoDir, 'src/auth.ts'), 'auth\n'); + await mustExec(['git', 'add', '.'], testRepo.repoDir); + await mustExec(['git', 'commit', '-m', 'add auth'], testRepo.repoDir); + await mustExec(['git', 'checkout', 'main'], testRepo.repoDir); + + const result = await validateTouchSet('feature-match', 'main', ['src/**'], { cwd: testRepo.repoDir }); + expect(result.valid).toBe(true); + expect(result.violations).toBeUndefined(); + expect(result.changedFiles).toContain('src/auth.ts'); + }); + + it('should return violations for files outside touchSet', async () => { + await mustExec(['git', 'checkout', '-b', 'feature-violation', 'main'], testRepo.repoDir); + mkdirSync(join(testRepo.repoDir, 'src'), { recursive: true }); + writeFileSync(join(testRepo.repoDir, 'src/ok.ts'), 'ok\n'); + writeFileSync(join(testRepo.repoDir, 'README.md'), 'readme\n'); + await mustExec(['git', 'add', '.'], testRepo.repoDir); + await mustExec(['git', 'commit', '-m', 'add files'], testRepo.repoDir); + await mustExec(['git', 'checkout', 'main'], testRepo.repoDir); + + const result = await validateTouchSet('feature-violation', 'main', ['src/**'], { cwd: testRepo.repoDir }); + expect(result.valid).toBe(false); + expect(result.violations).toContain('README.md'); + expect(result.violations).not.toContain('src/ok.ts'); + }); + + it('should handle multiple touchSet patterns', async () => { + await mustExec(['git', 'checkout', '-b', 'feature-multi', 'main'], testRepo.repoDir); + mkdirSync(join(testRepo.repoDir, 'src'), { recursive: true }); + mkdirSync(join(testRepo.repoDir, 'tests'), { recursive: true }); + writeFileSync(join(testRepo.repoDir, 'src/app.ts'), 'app\n'); + writeFileSync(join(testRepo.repoDir, 'tests/app.test.ts'), 'test\n'); + await mustExec(['git', 'add', '.'], testRepo.repoDir); + await mustExec(['git', 'commit', '-m', 'add files'], testRepo.repoDir); + await mustExec(['git', 'checkout', 'main'], testRepo.repoDir); + + const result = await validateTouchSet('feature-multi', 'main', ['src/**', 'tests/**'], { cwd: testRepo.repoDir }); + expect(result.valid).toBe(true); + expect(result.changedFiles).toContain('src/app.ts'); + expect(result.changedFiles).toContain('tests/app.test.ts'); + }); + + it('should handle ** glob wildcards', async () => { + await mustExec(['git', 'checkout', '-b', 'feature-glob', 'main'], testRepo.repoDir); + mkdirSync(join(testRepo.repoDir, 'src/lib/deep'), { recursive: true }); + writeFileSync(join(testRepo.repoDir, 'src/lib/deep/nested.ts'), 'nested\n'); + await mustExec(['git', 'add', '.'], testRepo.repoDir); + await mustExec(['git', 'commit', '-m', 'add deep file'], testRepo.repoDir); + await mustExec(['git', 'checkout', 'main'], testRepo.repoDir); + + const result = await validateTouchSet('feature-glob', 'main', ['src/**/*.ts'], { cwd: testRepo.repoDir }); + expect(result.valid).toBe(true); + expect(result.changedFiles).toContain('src/lib/deep/nested.ts'); + }); + + it('should return valid when job has no changes', async () => { + await mustExec(['git', 'checkout', '-b', 'feature-no-changes', 'main'], testRepo.repoDir); + await mustExec(['git', 'commit', '--allow-empty', '-m', 'empty'], testRepo.repoDir); + await mustExec(['git', 'checkout', 'main'], testRepo.repoDir); + + const result = await validateTouchSet('feature-no-changes', 'main', ['src/**'], { cwd: testRepo.repoDir }); + expect(result.valid).toBe(true); + expect(result.changedFiles).toEqual([]); + }); + + it('should return invalid when git diff fails', async () => { + const result = await validateTouchSet('nonexistent-branch', 'main', ['src/**'], { cwd: testRepo.repoDir }); + expect(result.valid).toBe(false); + expect(result.violations).toBeDefined(); + expect(result.violations![0]).toContain('Failed to diff'); + }); +}); + +describe('checkMergeability', () => { + let testRepo: TestRepo; + + beforeEach(async () => { + testRepo = await setupRepo(); + }); + + afterEach(() => { + rmSync(testRepo.rootDir, { recursive: true, force: true }); + }); + + it('should return canMerge true when merge would succeed', async () => { + await createBranchCommit(testRepo.repoDir, 'clean-feature', 'clean.txt', 'clean\n'); + + const result = await checkMergeability(testRepo.integrationWorktree, 'clean-feature'); + + expect(result.canMerge).toBe(true); + expect(result.conflicts).toBeUndefined(); + }); + + it('should return canMerge false with conflicts when merge would fail', async () => { + writeFileSync(join(testRepo.repoDir, 'shared.txt'), 'base\n'); + await mustExec(['git', 'add', 'shared.txt'], testRepo.repoDir); + await mustExec(['git', 'commit', '-m', 'add shared'], testRepo.repoDir); + + await mustExec(['git', 'checkout', '-b', 'int-change', 'main'], testRepo.repoDir); + writeFileSync(join(testRepo.repoDir, 'shared.txt'), 'integration side\n'); + await mustExec(['git', 'add', 'shared.txt'], testRepo.repoDir); + await mustExec(['git', 'commit', '-m', 'int change'], testRepo.repoDir); + await mustExec(['git', 'checkout', 'main'], testRepo.repoDir); + + await mustExec(['git', '-C', testRepo.integrationWorktree, 'merge', 'int-change'], testRepo.integrationWorktree); + + await mustExec(['git', 'checkout', '-b', 'conflict-branch', 'main'], testRepo.repoDir); + writeFileSync(join(testRepo.repoDir, 'shared.txt'), 'conflicting side\n'); + await mustExec(['git', 'add', 'shared.txt'], testRepo.repoDir); + await mustExec(['git', 'commit', '-m', 'conflict change'], testRepo.repoDir); + await mustExec(['git', 'checkout', 'main'], testRepo.repoDir); + + const result = await checkMergeability(testRepo.integrationWorktree, 'conflict-branch'); + + expect(result.canMerge).toBe(false); + expect(result.conflicts).toBeDefined(); + expect(result.conflicts!.length).toBeGreaterThan(0); + }); + + it('should always clean up worktree state after check', async () => { + await createBranchCommit(testRepo.repoDir, 'cleanup-test', 'cleanup.txt', 'cleanup\n'); + + await checkMergeability(testRepo.integrationWorktree, 'cleanup-test'); + + const status = await mustExec(['git', 'status', '--porcelain'], testRepo.integrationWorktree); + expect(status).toBe(''); + + writeFileSync(join(testRepo.repoDir, 'shared2.txt'), 'base\n'); + await mustExec(['git', 'add', 'shared2.txt'], testRepo.repoDir); + await mustExec(['git', 'commit', '-m', 'add shared2'], testRepo.repoDir); + await mustExec(['git', '-C', testRepo.integrationWorktree, 'merge', 'main'], testRepo.integrationWorktree); + + await mustExec(['git', 'checkout', '-b', 'conflict2', 'main'], testRepo.repoDir); + writeFileSync(join(testRepo.repoDir, 'shared2.txt'), 'conflict\n'); + await mustExec(['git', 'add', 'shared2.txt'], testRepo.repoDir); + await mustExec(['git', 'commit', '-m', 'conflict2'], testRepo.repoDir); + await mustExec(['git', 'checkout', 'main'], testRepo.repoDir); + + await checkMergeability(testRepo.integrationWorktree, 'conflict2'); + + const statusAfter = await mustExec(['git', 'status', '--porcelain'], testRepo.integrationWorktree); + expect(statusAfter).toBe(''); + }); +}); diff --git a/tests/lib/orchestrator-modes.test.ts b/tests/lib/orchestrator-modes.test.ts index ec4f4e3..3d2675b 100644 --- a/tests/lib/orchestrator-modes.test.ts +++ b/tests/lib/orchestrator-modes.test.ts @@ -5,6 +5,7 @@ import type { JobSpec, PlanSpec } from '../../src/lib/plan-types'; import * as integrationMod from '../../src/lib/integration'; import * as jobStateMod from '../../src/lib/job-state'; import { Orchestrator, type ToastCallback } from '../../src/lib/orchestrator'; +import * as mergeTrainMod from '../../src/lib/merge-train'; import * as planStateMod from '../../src/lib/plan-state'; import * as tmuxMod from '../../src/lib/tmux'; import * as worktreeMod from '../../src/lib/worktree'; @@ -114,6 +115,8 @@ describe('orchestrator modes', () => { spyOn(tmuxMod, 'killWindow').mockResolvedValue(); spyOn(tmuxMod, 'sendKeys').mockResolvedValue(); spyOn(tmuxMod, 'setPaneDiedHook').mockResolvedValue(); + + spyOn(mergeTrainMod, 'checkMergeability').mockResolvedValue({ canMerge: true }); }); afterEach(() => { diff --git a/tests/lib/orchestrator.test.ts b/tests/lib/orchestrator.test.ts index 02815d3..d4d90db 100644 --- a/tests/lib/orchestrator.test.ts +++ b/tests/lib/orchestrator.test.ts @@ -4,6 +4,7 @@ import type { Job, JobState } from '../../src/lib/job-state'; import type { JobSpec, PlanSpec } from '../../src/lib/plan-types'; import * as integrationMod from '../../src/lib/integration'; import * as jobStateMod from '../../src/lib/job-state'; +import * as mergeTrainMod from '../../src/lib/merge-train'; import { Orchestrator, hasCircularDependency, topologicalSort } from '../../src/lib/orchestrator'; import * as planStateMod from '../../src/lib/plan-state'; import * as tmuxMod from '../../src/lib/tmux'; @@ -99,6 +100,8 @@ describe('orchestrator', () => { spyOn(tmuxMod, 'killWindow').mockResolvedValue(); spyOn(tmuxMod, 'sendKeys').mockResolvedValue(); spyOn(tmuxMod, 'setPaneDiedHook').mockResolvedValue(); + + spyOn(mergeTrainMod, 'checkMergeability').mockResolvedValue({ canMerge: true }); }); afterEach(() => { @@ -287,7 +290,7 @@ describe('orchestrator', () => { expect(planState?.prUrl).toBe('https://example.com/pr/1'); }); - it('failed job event stops the plan', async () => { + it('failed job event pauses the plan', async () => { const orchestrator = new Orchestrator(monitor as any, { defaultPlacement: 'session', pollInterval: 10000, @@ -312,7 +315,121 @@ describe('orchestrator', () => { await new Promise((resolve) => setTimeout(resolve, 0)); - expect(planState?.status).toBe('failed'); + expect(planState?.status).toBe('paused'); + expect(planState?.checkpoint).toBe('on_error'); + }); + + it('autopilot plan should pause on merge conflict instead of failing', async () => { + planState = makePlan({ + status: 'running', + jobs: [ + makeJob('conflict-job', { status: 'ready_to_merge', mergeOrder: 0, branch: 'mc/conflict-job' }), + ], + }); + + const fakeTrain = { + queue: [] as JobSpec[], + enqueue(job: JobSpec) { + this.queue.push(job); + }, + getQueue() { + return [...this.queue]; + }, + async processNext() { + this.queue.shift(); + return { success: false, type: 'conflict', files: ['src/index.ts'] }; + }, + }; + + const orchestrator = new Orchestrator(monitor as any, { + defaultPlacement: 'session', + pollInterval: 10000, + idleThreshold: 300000, + worktreeBasePath: '/tmp', + omo: { enabled: false, defaultMode: 'vanilla' }, + } as any); + (orchestrator as any).mergeTrain = fakeTrain; + + await (orchestrator as any).reconcile(); + + expect(planState?.status).toBe('paused'); + expect(planState?.checkpoint).toBe('on_error'); + // Verify updatePlanJob was called with conflict status + expect(planStateMod.updatePlanJob).toHaveBeenCalledWith('plan-1', 'conflict-job', { + status: 'conflict', + error: 'src/index.ts', + }); + }); + + it('autopilot plan should pause on test failure instead of failing', async () => { + planState = makePlan({ + status: 'running', + jobs: [ + makeJob('test-fail-job', { status: 'ready_to_merge', mergeOrder: 0, branch: 'mc/test-fail-job' }), + ], + }); + + const fakeTrain = { + queue: [] as JobSpec[], + enqueue(job: JobSpec) { + this.queue.push(job); + }, + getQueue() { + return [...this.queue]; + }, + async processNext() { + this.queue.shift(); + return { success: false, type: 'test_failure', output: 'tests failed' }; + }, + }; + + const orchestrator = new Orchestrator(monitor as any, { + defaultPlacement: 'session', + pollInterval: 10000, + idleThreshold: 300000, + worktreeBasePath: '/tmp', + omo: { enabled: false, defaultMode: 'vanilla' }, + } as any); + (orchestrator as any).mergeTrain = fakeTrain; + + await (orchestrator as any).reconcile(); + + expect(planState?.status).toBe('paused'); + expect(planState?.checkpoint).toBe('on_error'); + expect(planStateMod.updatePlanJob).toHaveBeenCalledWith('plan-1', 'test-fail-job', { + status: 'failed', + error: 'tests failed', + }); + }); + + it('autopilot plan should pause on job monitor failure', async () => { + const orchestrator = new Orchestrator(monitor as any, { + defaultPlacement: 'session', + pollInterval: 10000, + idleThreshold: 300000, + worktreeBasePath: '/tmp', + omo: { enabled: false, defaultMode: 'vanilla' }, + } as any); + spyOn(orchestrator as any, 'startReconciler').mockImplementation(() => {}); + + await orchestrator.startPlan( + makePlan({ + status: 'pending', + mode: 'autopilot', + jobs: [makeJob('monitor-fail', { status: 'queued' })], + }), + ); + + monitor.emit('failed', { + id: 'j1', + name: 'monitor-fail', + planId: 'plan-1', + } as Job); + + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(planState?.status).toBe('paused'); + expect(planState?.checkpoint).toBe('on_error'); }); it('cancelPlan stops plan jobs and cleans up integration branch', async () => { @@ -362,6 +479,79 @@ describe('orchestrator', () => { expect(planStateMod.clearPlan).toHaveBeenCalled(); }); + it('should mark job needs_rebase and pause plan when trial merge detects conflicts', async () => { + planState = makePlan({ + status: 'running', + jobs: [ + makeJob('merge-conflict', { status: 'completed', mergeOrder: 0, branch: 'mc/merge-conflict' }), + ], + }); + + spyOn(mergeTrainMod, 'checkMergeability').mockResolvedValue({ + canMerge: false, + conflicts: ['src/index.ts'], + }); + + const orchestrator = new Orchestrator(monitor as any, { + defaultPlacement: 'session', + pollInterval: 10000, + idleThreshold: 300000, + worktreeBasePath: '/tmp', + omo: { enabled: false, defaultMode: 'vanilla' }, + } as any); + + await (orchestrator as any).reconcile(); + + expect(planStateMod.updatePlanJob).toHaveBeenCalledWith('plan-1', 'merge-conflict', { + status: 'needs_rebase', + error: 'src/index.ts', + }); + expect(planState?.status).toBe('paused'); + expect(planState?.checkpoint).toBe('on_error'); + }); + + it('should proceed to merge when trial merge succeeds', async () => { + planState = makePlan({ + status: 'running', + jobs: [ + makeJob('clean-merge', { status: 'completed', mergeOrder: 0, branch: 'mc/clean-merge' }), + ], + }); + + spyOn(mergeTrainMod, 'checkMergeability').mockResolvedValue({ canMerge: true }); + + const fakeTrain = { + queue: [] as JobSpec[], + enqueue(job: JobSpec) { + this.queue.push(job); + }, + getQueue() { + return [...this.queue]; + }, + async processNext() { + this.queue.shift(); + return { success: true, mergedAt: '2026-01-02T00:00:00.000Z' }; + }, + }; + + const orchestrator = new Orchestrator(monitor as any, { + defaultPlacement: 'session', + pollInterval: 10000, + idleThreshold: 300000, + worktreeBasePath: '/tmp', + omo: { enabled: false, defaultMode: 'vanilla' }, + } as any); + (orchestrator as any).mergeTrain = fakeTrain; + + await (orchestrator as any).reconcile(); + + expect(planStateMod.updatePlanJob).toHaveBeenCalledWith('plan-1', 'clean-merge', { status: 'merging' }); + expect(planStateMod.updatePlanJob).toHaveBeenCalledWith('plan-1', 'clean-merge', { + status: 'merged', + mergedAt: '2026-01-02T00:00:00.000Z', + }); + }); + it('resumePlan reconstructs state and marks dead running panes failed', async () => { planState = makePlan({ status: 'running', @@ -402,6 +592,114 @@ describe('orchestrator', () => { expect(planState?.status).toBe('failed'); expect(startSpy).not.toHaveBeenCalled(); }); + + it('should fail job and pause plan when touchSet is violated', async () => { + planState = makePlan({ + status: 'running', + jobs: [ + makeJob('touch-violator', { + status: 'completed', + mergeOrder: 0, + branch: 'mc/touch-violator', + touchSet: ['src/**'], + }), + ], + }); + + spyOn(mergeTrainMod, 'validateTouchSet').mockResolvedValue({ + valid: false, + violations: ['README.md'], + changedFiles: ['src/app.ts', 'README.md'], + }); + + const orchestrator = new Orchestrator(monitor as any, { + defaultPlacement: 'session', + pollInterval: 10000, + idleThreshold: 300000, + worktreeBasePath: '/tmp', + omo: { enabled: false, defaultMode: 'vanilla' }, + } as any); + + await (orchestrator as any).reconcile(); + + expect(planStateMod.updatePlanJob).toHaveBeenCalledWith( + 'plan-1', + 'touch-violator', + expect.objectContaining({ + status: 'failed', + error: expect.stringContaining('Modified files outside touchSet'), + }), + ); + expect(planState?.status).toBe('paused'); + expect(planState?.checkpoint).toBe('on_error'); + }); + + it('should allow transition to ready_to_merge when touchSet is satisfied', async () => { + planState = makePlan({ + status: 'running', + jobs: [ + makeJob('touch-ok', { + status: 'completed', + mergeOrder: 0, + branch: 'mc/touch-ok', + touchSet: ['src/**'], + }), + ], + }); + + spyOn(mergeTrainMod, 'validateTouchSet').mockResolvedValue({ + valid: true, + changedFiles: ['src/app.ts'], + }); + + const orchestrator = new Orchestrator(monitor as any, { + defaultPlacement: 'session', + pollInterval: 10000, + idleThreshold: 300000, + worktreeBasePath: '/tmp', + omo: { enabled: false, defaultMode: 'vanilla' }, + } as any); + + await (orchestrator as any).reconcile(); + + expect(planStateMod.updatePlanJob).toHaveBeenCalledWith( + 'plan-1', + 'touch-ok', + { status: 'ready_to_merge' }, + ); + }); + + it('should skip touchSet validation when touchSet is not defined', async () => { + planState = makePlan({ + status: 'running', + jobs: [ + makeJob('no-touchset', { + status: 'completed', + mergeOrder: 0, + branch: 'mc/no-touchset', + }), + ], + }); + + const validateSpy = spyOn(mergeTrainMod, 'validateTouchSet'); + + const orchestrator = new Orchestrator(monitor as any, { + defaultPlacement: 'session', + pollInterval: 10000, + idleThreshold: 300000, + worktreeBasePath: '/tmp', + omo: { enabled: false, defaultMode: 'vanilla' }, + } as any); + + await (orchestrator as any).reconcile(); + + expect(validateSpy).not.toHaveBeenCalled(); + expect(planStateMod.updatePlanJob).toHaveBeenCalledWith( + 'plan-1', + 'no-touchset', + { status: 'ready_to_merge' }, + ); + }); }); describe('orchestrator DAG helpers', () => { diff --git a/tests/lib/plan-types.test.ts b/tests/lib/plan-types.test.ts index 09b299f..dcd509c 100644 --- a/tests/lib/plan-types.test.ts +++ b/tests/lib/plan-types.test.ts @@ -171,6 +171,10 @@ describe('plan-types', () => { expect(isValidJobTransition('merged', 'needs_rebase')).toBe(true); expect(isValidJobTransition('needs_rebase', 'ready_to_merge')).toBe(true); }); + + it('should support failed job retry: failed -> ready_to_merge', () => { + expect(isValidJobTransition('failed', 'ready_to_merge')).toBe(true); + }); }); describe('isValidPlanTransition', () => { diff --git a/tests/tools/plan-approve.test.ts b/tests/tools/plan-approve.test.ts index e87a6b7..f479a0f 100644 --- a/tests/tools/plan-approve.test.ts +++ b/tests/tools/plan-approve.test.ts @@ -1,10 +1,8 @@ -import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { beforeEach, describe, expect, it, mock, spyOn } from 'bun:test'; import * as planState from '../../src/lib/plan-state'; import * as orchestrator from '../../src/lib/orchestrator'; import * as config from '../../src/lib/config'; -import * as monitor from '../../src/lib/monitor'; import * as integration from '../../src/lib/integration'; -import * as worktreeSetup from '../../src/lib/worktree-setup'; const { mc_plan_approve } = await import('../../src/tools/plan-approve'); @@ -15,23 +13,20 @@ const mockContext = { directory: '/test/dir', worktree: '/test/worktree', abort: new AbortController().signal, - metadata: vi.fn(), - ask: vi.fn(), + metadata: mock(), + ask: mock(), } as any; describe('mc_plan_approve', () => { beforeEach(() => { - vi.clearAllMocks(); - vi.spyOn(config, 'loadConfig').mockResolvedValue({ + mock.restore(); + spyOn(config, 'loadConfig').mockResolvedValue({ defaultPlacement: 'session', pollInterval: 10000, idleThreshold: 300000, worktreeBasePath: '/tmp', omo: { enabled: false, defaultMode: 'vanilla' }, }); - vi.spyOn(monitor, 'JobMonitor').mockImplementation( - () => ({ start: vi.fn(), on: vi.fn(), off: vi.fn() }) as any, - ); }); describe('tool definition', () => { @@ -42,9 +37,9 @@ describe('mc_plan_approve', () => { describe('no active plan', () => { it('should throw error when no plan exists', async () => { - vi.spyOn(planState, 'loadPlan').mockResolvedValue(null); + spyOn(planState, 'loadPlan').mockResolvedValue(null); - await expect( + expect( mc_plan_approve.execute({}, mockContext), ).rejects.toThrow('No active plan to approve'); }); @@ -52,7 +47,7 @@ describe('mc_plan_approve', () => { describe('non-pending plan', () => { it('should throw error when plan is not pending', async () => { - vi.spyOn(planState, 'loadPlan').mockResolvedValue({ + spyOn(planState, 'loadPlan').mockResolvedValue({ id: 'plan-1', name: 'Test Plan', mode: 'copilot', @@ -63,15 +58,142 @@ describe('mc_plan_approve', () => { createdAt: new Date().toISOString(), }); - await expect( + expect( mc_plan_approve.execute({}, mockContext), ).rejects.toThrow('not pending'); }); }); + describe('retry validation', () => { + it('should reset a failed job to ready_to_merge when retry is provided', async () => { + spyOn(planState, 'loadPlan').mockResolvedValue({ + id: 'plan-1', + name: 'Retry Plan', + mode: 'autopilot', + status: 'paused', + checkpoint: 'on_error', + jobs: [ + { id: 'j1', name: 'good-job', prompt: 'do good', status: 'merged' }, + { id: 'j2', name: 'bad-job', prompt: 'do bad', status: 'failed', error: 'test failure' }, + ], + integrationBranch: 'mc/integration/plan-1', + baseCommit: 'abc123', + createdAt: new Date().toISOString(), + }); + + const mockSavePlan = spyOn(planState, 'savePlan').mockResolvedValue(undefined); + const mockUpdatePlanJob = spyOn(planState, 'updatePlanJob').mockResolvedValue(undefined); + const mockResumePlan = mock().mockResolvedValue(undefined); + spyOn(orchestrator.Orchestrator.prototype, 'resumePlan').mockImplementation(mockResumePlan); + spyOn(orchestrator.Orchestrator.prototype, 'setPlanModelSnapshot').mockImplementation(() => {}); + + const result = await mc_plan_approve.execute({ checkpoint: 'on_error', retry: 'bad-job' }, mockContext); + + expect(mockUpdatePlanJob).toHaveBeenCalledWith('plan-1', 'bad-job', { status: 'ready_to_merge', error: undefined }); + expect(result).toContain('bad-job'); + expect(result).toContain('ready_to_merge'); + expect(result).toContain('resuming'); + expect(mockSavePlan).toHaveBeenCalled(); + expect(mockResumePlan).toHaveBeenCalled(); + }); + + it('should allow retry of needs_rebase jobs', async () => { + spyOn(planState, 'loadPlan').mockResolvedValue({ + id: 'plan-1', + name: 'Test Plan', + mode: 'supervisor', + status: 'paused', + checkpoint: 'on_error', + jobs: [ + { id: 'j1', name: 'conflicting-job', prompt: 'do stuff', status: 'needs_rebase', error: 'merge conflict detected' }, + ], + integrationBranch: 'mc/integration/plan-1', + baseCommit: 'abc', + createdAt: new Date().toISOString(), + }); + + const mockUpdatePlanJob = spyOn(planState, 'updatePlanJob').mockResolvedValue(undefined); + const mockSavePlan = spyOn(planState, 'savePlan').mockResolvedValue(undefined); + const mockResumePlan = mock().mockResolvedValue(undefined); + spyOn(orchestrator.Orchestrator.prototype, 'resumePlan').mockImplementation(mockResumePlan); + spyOn(orchestrator.Orchestrator.prototype, 'setPlanModelSnapshot').mockImplementation(() => {}); + + const result = await mc_plan_approve.execute({ checkpoint: 'on_error', retry: 'conflicting-job' }, mockContext); + + expect(mockUpdatePlanJob).toHaveBeenCalledWith('plan-1', 'conflicting-job', { status: 'ready_to_merge', error: undefined }); + expect(result).toContain('Checkpoint'); + expect(result).toContain('resuming'); + expect(mockSavePlan).toHaveBeenCalled(); + expect(mockResumePlan).toHaveBeenCalled(); + }); + + it('should throw if retry job name is not found in plan', async () => { + spyOn(planState, 'loadPlan').mockResolvedValue({ + id: 'plan-1', + name: 'Retry Plan', + mode: 'autopilot', + status: 'paused', + checkpoint: 'on_error', + jobs: [{ id: 'j1', name: 'existing-job', prompt: 'do stuff', status: 'failed' }], + integrationBranch: 'mc/integration/plan-1', + baseCommit: 'abc123', + createdAt: new Date().toISOString(), + }); + + expect( + mc_plan_approve.execute({ checkpoint: 'on_error', retry: 'nonexistent' }, mockContext), + ).rejects.toThrow('Job "nonexistent" not found in plan'); + }); + + it('should reject retry of non-retryable jobs', async () => { + spyOn(planState, 'loadPlan').mockResolvedValue({ + id: 'plan-1', + name: 'Test Plan', + mode: 'supervisor', + status: 'paused', + checkpoint: 'on_error', + jobs: [{ id: 'j1', name: 'running-job', prompt: 'do stuff', status: 'running' }], + integrationBranch: 'mc/integration/plan-1', + baseCommit: 'abc', + createdAt: new Date().toISOString(), + }); + + expect( + mc_plan_approve.execute({ retry: 'running-job' }, mockContext), + ).rejects.toThrow('not in a retryable state'); + }); + + it('should clear checkpoint without retry (backward compatible)', async () => { + spyOn(planState, 'loadPlan').mockResolvedValue({ + id: 'plan-1', + name: 'Checkpoint Plan', + mode: 'supervisor', + status: 'paused', + checkpoint: 'pre_merge', + jobs: [{ id: 'j1', name: 'merge-job', prompt: 'do merge', status: 'ready_to_merge' }], + integrationBranch: 'mc/integration/plan-1', + baseCommit: 'abc123', + createdAt: new Date().toISOString(), + }); + + const mockSavePlan = spyOn(planState, 'savePlan').mockResolvedValue(undefined); + const mockResumePlan = mock().mockResolvedValue(undefined); + spyOn(orchestrator.Orchestrator.prototype, 'resumePlan').mockImplementation(mockResumePlan); + spyOn(orchestrator.Orchestrator.prototype, 'setPlanModelSnapshot').mockImplementation(() => {}); + + const result = await mc_plan_approve.execute({ checkpoint: 'pre_merge' }, mockContext); + + expect(result).toContain('Checkpoint "pre_merge" cleared'); + expect(result).toContain('resuming'); + expect(result).not.toContain('ready_to_merge'); + expect(mockSavePlan).toHaveBeenCalled(); + expect(mockResumePlan).toHaveBeenCalled(); + }); + }); + describe('approve pending plan', () => { it('should transition plan to running and resume via orchestrator', async () => { - vi.spyOn(planState, 'loadPlan').mockResolvedValue({ + spyOn(planState, 'loadPlan').mockResolvedValue({ id: 'plan-1', name: 'Feature Sprint', mode: 'copilot', @@ -80,25 +202,19 @@ describe('mc_plan_approve', () => { { id: 'j1', name: 'auth', prompt: 'do auth', status: 'queued' }, { id: 'j2', name: 'api', prompt: 'do api', status: 'queued' }, ], - integrationBranch: 'mc/integration-plan-1', + integrationBranch: 'mc/integration/plan-1', baseCommit: 'abc123', createdAt: new Date().toISOString(), }); - const mockSavePlan = vi.spyOn(planState, 'savePlan').mockResolvedValue(undefined); - const mockResumePlan = vi.fn().mockResolvedValue(undefined); - vi.spyOn(orchestrator, 'Orchestrator').mockImplementation( - () => - ({ - resumePlan: mockResumePlan, - setPlanModelSnapshot: vi.fn(), - }) as any, - ); - vi.spyOn(integration, 'createIntegrationBranch').mockResolvedValue({ + const mockSavePlan = spyOn(planState, 'savePlan').mockResolvedValue(undefined); + const mockResumePlan = mock().mockResolvedValue(undefined); + spyOn(orchestrator.Orchestrator.prototype, 'resumePlan').mockImplementation(mockResumePlan); + spyOn(orchestrator.Orchestrator.prototype, 'setPlanModelSnapshot').mockImplementation(() => {}); + spyOn(integration, 'createIntegrationBranch').mockResolvedValue({ branch: 'mc/integration-plan-1', - worktreePath: '/tmp/integration-plan-1', + worktreePath: '/tmp/mc-integration-plan-1', }); - vi.spyOn(worktreeSetup, 'resolvePostCreateHook').mockReturnValue(undefined as any); const result = await mc_plan_approve.execute({}, mockContext);