From 36a1dbe716750b80a3260fca9517d9043627be6a Mon Sep 17 00:00:00 2001 From: Nigel Bazzeghin Date: Thu, 12 Feb 2026 05:54:53 -0600 Subject: [PATCH 1/6] Merge graceful-retry --- src/lib/orchestrator.ts | 37 +++----- src/lib/plan-types.ts | 2 +- src/tools/plan-approve.ts | 23 ++++- tests/integration/orchestration.test.ts | 15 ++- tests/lib/orchestrator.test.ts | 118 +++++++++++++++++++++++- tests/lib/plan-types.test.ts | 4 + tests/tools/plan-approve.test.ts | 113 +++++++++++++++++++++++ 7 files changed, 272 insertions(+), 40 deletions(-) diff --git a/src/lib/orchestrator.ts b/src/lib/orchestrator.ts index cda54c6..2c092d7 100644 --- a/src/lib/orchestrator.ts +++ b/src/lib/orchestrator.ts @@ -538,14 +538,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 +553,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; } } @@ -794,16 +786,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..00fc607 100644 --- a/src/lib/plan-types.ts +++ b/src/lib/plan-types.ts @@ -75,7 +75,7 @@ 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'], + failed: ['ready_to_merge', 'stopped', 'canceled'], ready_to_merge: ['merging', 'stopped', 'canceled'], merging: ['merged', 'conflict', 'stopped', 'canceled'], merged: ['needs_rebase'], diff --git a/src/tools/plan-approve.ts b/src/tools/plan-approve.ts index d3e316d..3a8c893 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('Job name to retry โ€” resets a failed/conflict job to ready_to_merge before resuming'), }, async execute(args) { const plan = await loadPlan(); @@ -25,6 +29,18 @@ export const mc_plan_approve: ToolDefinition = tool({ 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') { + throw new Error(`Job "${args.retry}" is not in a retryable state (current: ${job.status}). Only failed or conflict 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 +51,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..d21ace8 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, { @@ -496,23 +496,22 @@ describe('orchestration integration', () => { await simulateJobCompletion('job-2', monitor, orchestrator); 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' && currentPlan?.checkpoint === 'on_error'; }, 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'); 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/orchestrator.test.ts b/tests/lib/orchestrator.test.ts index 02815d3..f792e63 100644 --- a/tests/lib/orchestrator.test.ts +++ b/tests/lib/orchestrator.test.ts @@ -287,7 +287,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 +312,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 () => { 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..3007a67 100644 --- a/tests/tools/plan-approve.test.ts +++ b/tests/tools/plan-approve.test.ts @@ -69,6 +69,119 @@ describe('mc_plan_approve', () => { }); }); + describe('retry failed job', () => { + it('should reset a failed job to ready_to_merge when retry is provided', async () => { + vi.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 = vi.spyOn(planState, 'savePlan').mockResolvedValue(undefined); + const mockUpdatePlanJob = vi.spyOn(planState, 'updatePlanJob').mockResolvedValue(undefined); + const mockResumePlan = vi.fn().mockResolvedValue(undefined); + vi.spyOn(orchestrator, 'Orchestrator').mockImplementation( + () => + ({ + resumePlan: mockResumePlan, + setPlanModelSnapshot: vi.fn(), + }) as any, + ); + + 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 throw if retry job name is not found in plan', async () => { + vi.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(), + }); + + await expect( + mc_plan_approve.execute({ checkpoint: 'on_error', retry: 'nonexistent' }, mockContext), + ).rejects.toThrow('Job "nonexistent" not found in plan'); + }); + + it('should throw if retry job is not in a retryable state', async () => { + vi.spyOn(planState, 'loadPlan').mockResolvedValue({ + id: 'plan-1', + name: 'Retry Plan', + mode: 'autopilot', + status: 'paused', + checkpoint: 'on_error', + jobs: [ + { id: 'j1', name: 'running-job', prompt: 'do stuff', status: 'running' }, + ], + integrationBranch: 'mc/integration/plan-1', + baseCommit: 'abc123', + createdAt: new Date().toISOString(), + }); + + await expect( + mc_plan_approve.execute({ checkpoint: 'on_error', retry: 'running-job' }, mockContext), + ).rejects.toThrow('not in a retryable state'); + }); + + it('should clear checkpoint without retry (backward compatible)', async () => { + vi.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 = vi.spyOn(planState, 'savePlan').mockResolvedValue(undefined); + const mockResumePlan = vi.fn().mockResolvedValue(undefined); + vi.spyOn(orchestrator, 'Orchestrator').mockImplementation( + () => + ({ + resumePlan: mockResumePlan, + setPlanModelSnapshot: vi.fn(), + }) as any, + ); + + 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({ From ba443b61e4ad28ba283f9466b245e82eb7d66f71 Mon Sep 17 00:00:00 2001 From: Nigel Bazzeghin Date: Thu, 12 Feb 2026 06:04:13 -0600 Subject: [PATCH 2/6] Merge touchset-gate --- src/lib/merge-train.ts | 38 ++++++++++++ src/lib/orchestrator.ts | 18 +++++- tests/lib/merge-train.test.ts | 96 ++++++++++++++++++++++++++++- tests/lib/orchestrator.test.ts | 109 +++++++++++++++++++++++++++++++++ 4 files changed, 259 insertions(+), 2 deletions(-) diff --git a/src/lib/merge-train.ts b/src/lib/merge-train.ts index 496d8ad..7002ef9 100644 --- a/src/lib/merge-train.ts +++ b/src/lib/merge-train.ts @@ -50,6 +50,44 @@ 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, + }; +} + 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 2c092d7..008e8dc 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, 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'; } diff --git a/tests/lib/merge-train.test.ts b/tests/lib/merge-train.test.ts index b303e1d..7845ae2 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, detectInstallCommand, detectTestCommand, validateTouchSet } from '../../src/lib/merge-train'; type TestRepo = { rootDir: string; @@ -412,3 +412,97 @@ 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'); + }); +}); diff --git a/tests/lib/orchestrator.test.ts b/tests/lib/orchestrator.test.ts index f792e63..440a938 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'; @@ -516,6 +517,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', () => { From e355b76f91655f467709ca3f12b8c8c0fd81d0b5 Mon Sep 17 00:00:00 2001 From: Nigel Bazzeghin Date: Thu, 12 Feb 2026 06:12:59 -0600 Subject: [PATCH 3/6] feat: activate needs_rebase job status for early conflict detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add trial merge check before jobs enter the merge train to detect conflicts early. Jobs that would conflict are marked needs_rebase and the plan pauses at an on_error checkpoint. - Add checkMergeability() to merge-train.ts for trial merges - Integrate trial merge in orchestrator reconcile loop - Add retry arg to plan-approve for needs_rebase jobs - Add ready_to_merge โ†’ needs_rebase transition in plan-types - Add tests for all new functionality Closes #36 --- src/lib/merge-train.ts | 27 +++++++++ src/lib/orchestrator.ts | 18 +++++- src/lib/plan-types.ts | 2 +- src/tools/plan-approve.ts | 16 +++++- tests/integration/orchestration.test.ts | 4 +- tests/lib/merge-train.test.ts | 76 ++++++++++++++++++++++++- tests/lib/orchestrator-modes.test.ts | 3 + tests/lib/orchestrator.test.ts | 76 +++++++++++++++++++++++++ tests/tools/plan-approve.test.ts | 59 ++++++++++++++----- 9 files changed, 259 insertions(+), 22 deletions(-) diff --git a/src/lib/merge-train.ts b/src/lib/merge-train.ts index 7002ef9..42c7e0d 100644 --- a/src/lib/merge-train.ts +++ b/src/lib/merge-train.ts @@ -88,6 +88,33 @@ export async function validateTouchSet( }; } +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 008e8dc..e6e7d4d 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, validateTouchSet } 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'; @@ -510,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; diff --git a/src/lib/plan-types.ts b/src/lib/plan-types.ts index 00fc607..3488afb 100644 --- a/src/lib/plan-types.ts +++ b/src/lib/plan-types.ts @@ -76,7 +76,7 @@ export const VALID_JOB_TRANSITIONS: Record = { running: ['completed', 'failed', 'stopped', 'canceled'], completed: ['ready_to_merge', 'stopped', 'canceled'], failed: ['ready_to_merge', 'stopped', 'canceled'], - ready_to_merge: ['merging', '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 3a8c893..970da90 100644 --- a/src/tools/plan-approve.ts +++ b/src/tools/plan-approve.ts @@ -19,7 +19,7 @@ export const mc_plan_approve: ToolDefinition = tool({ retry: tool.schema .string() .optional() - .describe('Job name to retry โ€” resets a failed/conflict job to ready_to_merge before resuming'), + .describe('Name of a failed, conflict, or needs_rebase job to retry'), }, async execute(args) { const plan = await loadPlan(); @@ -27,6 +27,16 @@ 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; @@ -35,8 +45,8 @@ export const mc_plan_approve: ToolDefinition = tool({ if (!job) { throw new Error(`Job "${args.retry}" not found in plan`); } - if (job.status !== 'failed' && job.status !== 'conflict') { - throw new Error(`Job "${args.retry}" is not in a retryable state (current: ${job.status}). Only failed or conflict jobs can be retried.`); + 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 }); } diff --git a/tests/integration/orchestration.test.ts b/tests/integration/orchestration.test.ts index d21ace8..d6a692e 100644 --- a/tests/integration/orchestration.test.ts +++ b/tests/integration/orchestration.test.ts @@ -496,12 +496,14 @@ describe('orchestration integration', () => { await simulateJobCompletion('job-2', monitor, orchestrator); await waitForCondition(async () => { const currentPlan = await loadPlan(); - return currentPlan?.status === 'paused' && currentPlan?.checkpoint === 'on_error'; + const second = currentPlan?.jobs.find((job) => job.id === 'job-2'); + return currentPlan?.status === 'paused' && second?.status === 'needs_rebase'; }, 8000); 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'], diff --git a/tests/lib/merge-train.test.ts b/tests/lib/merge-train.test.ts index 7845ae2..a0811ce 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, validateTouchSet } from '../../src/lib/merge-train'; +import { MergeTrain, checkMergeability, detectInstallCommand, detectTestCommand, validateTouchSet } from '../../src/lib/merge-train'; type TestRepo = { rootDir: string; @@ -506,3 +506,77 @@ describe('validateTouchSet', () => { 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(''); + + const headBefore = await mustExec(['git', 'rev-parse', 'HEAD'], testRepo.integrationWorktree); + + 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 440a938..8de2a4c 100644 --- a/tests/lib/orchestrator.test.ts +++ b/tests/lib/orchestrator.test.ts @@ -6,6 +6,7 @@ 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 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'; @@ -100,6 +101,8 @@ describe('orchestrator', () => { spyOn(tmuxMod, 'killWindow').mockResolvedValue(); spyOn(tmuxMod, 'sendKeys').mockResolvedValue(); spyOn(tmuxMod, 'setPaneDiedHook').mockResolvedValue(); + + spyOn(mergeTrainMod, 'checkMergeability').mockResolvedValue({ canMerge: true }); }); afterEach(() => { @@ -477,6 +480,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', diff --git a/tests/tools/plan-approve.test.ts b/tests/tools/plan-approve.test.ts index 3007a67..3252b12 100644 --- a/tests/tools/plan-approve.test.ts +++ b/tests/tools/plan-approve.test.ts @@ -69,7 +69,7 @@ describe('mc_plan_approve', () => { }); }); - describe('retry failed job', () => { + describe('retry validation', () => { it('should reset a failed job to ready_to_merge when retry is provided', async () => { vi.spyOn(planState, 'loadPlan').mockResolvedValue({ id: 'plan-1', @@ -107,6 +107,41 @@ describe('mc_plan_approve', () => { expect(mockResumePlan).toHaveBeenCalled(); }); + it('should allow retry of needs_rebase jobs', async () => { + vi.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 = vi.spyOn(planState, 'updatePlanJob').mockResolvedValue(undefined); + 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, + ); + + 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 () => { vi.spyOn(planState, 'loadPlan').mockResolvedValue({ id: 'plan-1', @@ -114,9 +149,7 @@ describe('mc_plan_approve', () => { mode: 'autopilot', status: 'paused', checkpoint: 'on_error', - jobs: [ - { id: 'j1', name: 'existing-job', prompt: 'do stuff', status: 'failed' }, - ], + jobs: [{ id: 'j1', name: 'existing-job', prompt: 'do stuff', status: 'failed' }], integrationBranch: 'mc/integration/plan-1', baseCommit: 'abc123', createdAt: new Date().toISOString(), @@ -127,23 +160,21 @@ describe('mc_plan_approve', () => { ).rejects.toThrow('Job "nonexistent" not found in plan'); }); - it('should throw if retry job is not in a retryable state', async () => { + it('should reject retry of non-retryable jobs', async () => { vi.spyOn(planState, 'loadPlan').mockResolvedValue({ id: 'plan-1', - name: 'Retry Plan', - mode: 'autopilot', + name: 'Test Plan', + mode: 'supervisor', status: 'paused', checkpoint: 'on_error', - jobs: [ - { id: 'j1', name: 'running-job', prompt: 'do stuff', status: 'running' }, - ], + jobs: [{ id: 'j1', name: 'running-job', prompt: 'do stuff', status: 'running' }], integrationBranch: 'mc/integration/plan-1', - baseCommit: 'abc123', + baseCommit: 'abc', createdAt: new Date().toISOString(), }); await expect( - mc_plan_approve.execute({ checkpoint: 'on_error', retry: 'running-job' }, mockContext), + mc_plan_approve.execute({ retry: 'running-job' }, mockContext), ).rejects.toThrow('not in a retryable state'); }); @@ -154,9 +185,7 @@ describe('mc_plan_approve', () => { mode: 'supervisor', status: 'paused', checkpoint: 'pre_merge', - jobs: [ - { id: 'j1', name: 'merge-job', prompt: 'do merge', status: 'ready_to_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(), From 0c4aea43ea77add3fd92963793b0e95a94b7dacb Mon Sep 17 00:00:00 2001 From: Nigel Bazzeghin Date: Thu, 12 Feb 2026 06:37:00 -0600 Subject: [PATCH 4/6] test: fix diagnostics in rebased test files --- tests/lib/merge-train.test.ts | 2 - tests/lib/orchestrator.test.ts | 1 - tests/tools/plan-approve.test.ts | 103 +++++++++++-------------------- 3 files changed, 36 insertions(+), 70 deletions(-) diff --git a/tests/lib/merge-train.test.ts b/tests/lib/merge-train.test.ts index a0811ce..d59e695 100644 --- a/tests/lib/merge-train.test.ts +++ b/tests/lib/merge-train.test.ts @@ -561,8 +561,6 @@ describe('checkMergeability', () => { const status = await mustExec(['git', 'status', '--porcelain'], testRepo.integrationWorktree); expect(status).toBe(''); - const headBefore = await mustExec(['git', 'rev-parse', 'HEAD'], testRepo.integrationWorktree); - 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); diff --git a/tests/lib/orchestrator.test.ts b/tests/lib/orchestrator.test.ts index 8de2a4c..d4d90db 100644 --- a/tests/lib/orchestrator.test.ts +++ b/tests/lib/orchestrator.test.ts @@ -6,7 +6,6 @@ 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 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'; diff --git a/tests/tools/plan-approve.test.ts b/tests/tools/plan-approve.test.ts index 3252b12..08a94ad 100644 --- a/tests/tools/plan-approve.test.ts +++ b/tests/tools/plan-approve.test.ts @@ -1,10 +1,7 @@ -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 +12,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 +36,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 +46,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,7 +57,7 @@ describe('mc_plan_approve', () => { createdAt: new Date().toISOString(), }); - await expect( + expect( mc_plan_approve.execute({}, mockContext), ).rejects.toThrow('not pending'); }); @@ -71,7 +65,7 @@ describe('mc_plan_approve', () => { describe('retry validation', () => { it('should reset a failed job to ready_to_merge when retry is provided', async () => { - vi.spyOn(planState, 'loadPlan').mockResolvedValue({ + spyOn(planState, 'loadPlan').mockResolvedValue({ id: 'plan-1', name: 'Retry Plan', mode: 'autopilot', @@ -86,16 +80,11 @@ describe('mc_plan_approve', () => { createdAt: new Date().toISOString(), }); - const mockSavePlan = vi.spyOn(planState, 'savePlan').mockResolvedValue(undefined); - const mockUpdatePlanJob = vi.spyOn(planState, 'updatePlanJob').mockResolvedValue(undefined); - const mockResumePlan = vi.fn().mockResolvedValue(undefined); - vi.spyOn(orchestrator, 'Orchestrator').mockImplementation( - () => - ({ - resumePlan: mockResumePlan, - setPlanModelSnapshot: vi.fn(), - }) as any, - ); + 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); @@ -108,7 +97,7 @@ describe('mc_plan_approve', () => { }); it('should allow retry of needs_rebase jobs', async () => { - vi.spyOn(planState, 'loadPlan').mockResolvedValue({ + spyOn(planState, 'loadPlan').mockResolvedValue({ id: 'plan-1', name: 'Test Plan', mode: 'supervisor', @@ -122,16 +111,11 @@ describe('mc_plan_approve', () => { createdAt: new Date().toISOString(), }); - const mockUpdatePlanJob = vi.spyOn(planState, 'updatePlanJob').mockResolvedValue(undefined); - 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, - ); + 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); @@ -143,7 +127,7 @@ describe('mc_plan_approve', () => { }); it('should throw if retry job name is not found in plan', async () => { - vi.spyOn(planState, 'loadPlan').mockResolvedValue({ + spyOn(planState, 'loadPlan').mockResolvedValue({ id: 'plan-1', name: 'Retry Plan', mode: 'autopilot', @@ -155,13 +139,13 @@ describe('mc_plan_approve', () => { createdAt: new Date().toISOString(), }); - await expect( + 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 () => { - vi.spyOn(planState, 'loadPlan').mockResolvedValue({ + spyOn(planState, 'loadPlan').mockResolvedValue({ id: 'plan-1', name: 'Test Plan', mode: 'supervisor', @@ -173,13 +157,13 @@ describe('mc_plan_approve', () => { createdAt: new Date().toISOString(), }); - await expect( + 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 () => { - vi.spyOn(planState, 'loadPlan').mockResolvedValue({ + spyOn(planState, 'loadPlan').mockResolvedValue({ id: 'plan-1', name: 'Checkpoint Plan', mode: 'supervisor', @@ -191,15 +175,10 @@ describe('mc_plan_approve', () => { 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, - ); + 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); @@ -213,7 +192,7 @@ describe('mc_plan_approve', () => { 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', @@ -222,25 +201,15 @@ 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({ - branch: 'mc/integration-plan-1', - worktreePath: '/tmp/integration-plan-1', - }); - vi.spyOn(worktreeSetup, 'resolvePostCreateHook').mockReturnValue(undefined as any); + 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({}, mockContext); From d38dec7676d6088da2970b563498e10e0a7ade83 Mon Sep 17 00:00:00 2001 From: Nigel Bazzeghin Date: Thu, 12 Feb 2026 06:53:22 -0600 Subject: [PATCH 5/6] fix: update plan job state before reconcile on completion --- src/lib/orchestrator.ts | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/lib/orchestrator.ts b/src/lib/orchestrator.ts index e6e7d4d..6553148 100644 --- a/src/lib/orchestrator.ts +++ b/src/lib/orchestrator.ts @@ -787,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 => { From 7645c7451365dd1715705a48b2c65cb90987dd13 Mon Sep 17 00:00:00 2001 From: Nigel Bazzeghin Date: Thu, 12 Feb 2026 06:56:24 -0600 Subject: [PATCH 6/6] test: mock createIntegrationBranch in plan-approve test for CI --- tests/tools/plan-approve.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/tools/plan-approve.test.ts b/tests/tools/plan-approve.test.ts index 08a94ad..f479a0f 100644 --- a/tests/tools/plan-approve.test.ts +++ b/tests/tools/plan-approve.test.ts @@ -2,6 +2,7 @@ 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 integration from '../../src/lib/integration'; const { mc_plan_approve } = await import('../../src/tools/plan-approve'); @@ -210,6 +211,10 @@ describe('mc_plan_approve', () => { 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/mc-integration-plan-1', + }); const result = await mc_plan_approve.execute({}, mockContext);