Skip to content

Commit 51d44f0

Browse files
nigel-devnigel-dev
committed
feat: implement accept, relaunch, and fixed retry paths in mc_plan_approve
Three distinct actions for touchSet violations: - Accept (no params): moves checkpoint job to ready_to_merge - Relaunch (relaunch param): spawns agent in existing worktree with correction prompt to fix violations - Retry (retry param): re-validates touchSet before proceeding, fixing the bug where retry silently skipped validation Update test assertions to match new output format.
1 parent c8dae75 commit 51d44f0

2 files changed

Lines changed: 102 additions & 18 deletions

File tree

‎src/tools/plan-approve.ts‎

Lines changed: 101 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,11 @@ import { loadConfig } from '../lib/config';
77
import { getCurrentModel } from '../lib/model-tracker';
88
import { createIntegrationBranch } from '../lib/integration';
99
import { resolvePostCreateHook } from '../lib/worktree-setup';
10+
import { validateTouchSet } from '../lib/merge-train';
1011

1112
export const mc_plan_approve: ToolDefinition = tool({
1213
description:
13-
'Approve a pending copilot plan, clear a supervisor checkpoint, or retry a failed job to continue execution',
14+
'Approve a pending copilot plan, clear a supervisor checkpoint, or retry/relaunch a failed job to continue execution',
1415
args: {
1516
checkpoint: tool.schema
1617
.enum(['pre_merge', 'on_error', 'pre_pr'])
@@ -19,51 +20,136 @@ export const mc_plan_approve: ToolDefinition = tool({
1920
retry: tool.schema
2021
.string()
2122
.optional()
22-
.describe('Name of a failed, conflict, or needs_rebase job to retry'),
23+
.describe('Name of a failed, conflict, or needs_rebase job to retry after manual fix (re-validates touchSet)'),
24+
relaunch: tool.schema
25+
.string()
26+
.optional()
27+
.describe('Name of a touchSet-failed job to relaunch — spawns agent in existing worktree with correction prompt'),
2328
},
2429
async execute(args) {
30+
if (args.retry && args.relaunch) {
31+
throw new Error('Cannot specify both "retry" and "relaunch". Use "retry" for manual fixes (re-validates) or "relaunch" to spawn an agent to fix violations.');
32+
}
33+
2534
const plan = await loadPlan();
2635
if (!plan) {
2736
throw new Error('No active plan to approve');
2837
}
2938

30-
if (args.retry) {
31-
const job = plan.jobs.find((j) => j.name === args.retry);
39+
const targetJobName = args.retry ?? args.relaunch;
40+
if (targetJobName) {
41+
const job = plan.jobs.find((j) => j.name === targetJobName);
3242
if (!job) {
33-
throw new Error(`Job "${args.retry}" not found in plan`);
43+
throw new Error(`Job "${targetJobName}" not found in plan`);
3444
}
3545
if (job.status !== 'failed' && job.status !== 'conflict' && job.status !== 'needs_rebase') {
36-
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.`);
46+
throw new Error(`Job "${targetJobName}" is not in a retryable state (current: ${job.status}). Only failed, conflict, or needs_rebase jobs can be retried.`);
3747
}
3848
}
3949

4050
if (plan.status === 'paused' && plan.checkpoint) {
4151
const checkpoint = (args.checkpoint ?? plan.checkpoint) as CheckpointType;
52+
const config = await loadConfig();
4253

43-
if (args.retry) {
44-
const job = plan.jobs.find(j => j.name === args.retry);
45-
if (!job) {
46-
throw new Error(`Job "${args.retry}" not found in plan`);
54+
if (args.relaunch) {
55+
const ctx = plan.checkpointContext;
56+
if (!ctx || ctx.failureKind !== 'touchset') {
57+
throw new Error(`Job "${args.relaunch}" was not failed due to a touchSet violation — use "retry" instead.`);
4758
}
48-
if (job.status !== 'failed' && job.status !== 'conflict' && job.status !== 'needs_rebase') {
49-
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.`);
59+
if (ctx.jobName !== args.relaunch) {
60+
throw new Error(`Checkpoint was set for job "${ctx.jobName}", not "${args.relaunch}".`);
5061
}
62+
63+
plan.status = 'running';
64+
plan.checkpoint = null;
65+
plan.checkpointContext = null;
66+
await savePlan(plan);
67+
68+
const orchestrator = new Orchestrator(getSharedMonitor(), config, { notify: getSharedNotifyCallback() ?? undefined });
69+
setSharedOrchestrator(orchestrator);
70+
orchestrator.setPlanModelSnapshot(getCurrentModel());
71+
72+
await orchestrator.relaunchJobForCorrection(
73+
args.relaunch,
74+
ctx.touchSetViolations ?? [],
75+
ctx.touchSetPatterns ?? [],
76+
);
77+
await orchestrator.resumePlan();
78+
79+
return [
80+
`Checkpoint "${checkpoint}" cleared. Job "${args.relaunch}" relaunched with correction prompt.`,
81+
'',
82+
` ID: ${plan.id}`,
83+
` Mode: ${plan.mode}`,
84+
'',
85+
'The agent will fix touchSet violations in the existing worktree.',
86+
'Use mc_plan_status to monitor progress.',
87+
].join('\n');
88+
}
89+
90+
if (args.retry) {
91+
const job = plan.jobs.find(j => j.name === args.retry)!;
92+
93+
const ctx = plan.checkpointContext;
94+
if (ctx?.failureKind === 'touchset' && ctx.jobName === args.retry) {
95+
if (job.touchSet && job.touchSet.length > 0 && job.branch && plan.integrationBranch) {
96+
const validation = await validateTouchSet(job.branch, plan.integrationBranch, job.touchSet);
97+
if (!validation.valid && validation.violations) {
98+
throw new Error(
99+
`Job "${args.retry}" still has touchSet violations after manual fix:\n` +
100+
` Violations: ${validation.violations.join(', ')}\n` +
101+
` Allowed: ${job.touchSet.join(', ')}\n` +
102+
`Fix the remaining violations and retry again.`,
103+
);
104+
}
105+
}
106+
}
107+
51108
await updatePlanJob(plan.id, args.retry, { status: 'ready_to_merge', error: undefined });
109+
110+
plan.status = 'running';
111+
plan.checkpoint = null;
112+
plan.checkpointContext = null;
113+
await savePlan(plan);
114+
115+
const orchestrator = new Orchestrator(getSharedMonitor(), config, { notify: getSharedNotifyCallback() ?? undefined });
116+
setSharedOrchestrator(orchestrator);
117+
orchestrator.setPlanModelSnapshot(getCurrentModel());
118+
await orchestrator.resumePlan();
119+
120+
return [
121+
`Checkpoint "${checkpoint}" cleared. Job "${args.retry}" reset to ready_to_merge.`,
122+
'',
123+
` ID: ${plan.id}`,
124+
` Mode: ${plan.mode}`,
125+
'',
126+
'Use mc_plan_status to monitor progress.',
127+
].join('\n');
128+
}
129+
130+
const ctx = plan.checkpointContext;
131+
if (ctx?.failureKind === 'touchset') {
132+
const job = plan.jobs.find(j => j.name === ctx.jobName);
133+
if (job && job.status === 'failed') {
134+
await updatePlanJob(plan.id, ctx.jobName, { status: 'ready_to_merge', error: undefined });
135+
}
52136
}
53137

54138
plan.status = 'running';
55139
plan.checkpoint = null;
140+
plan.checkpointContext = null;
56141
await savePlan(plan);
57142

58-
const config = await loadConfig();
59143
const orchestrator = new Orchestrator(getSharedMonitor(), config, { notify: getSharedNotifyCallback() ?? undefined });
60144
setSharedOrchestrator(orchestrator);
61145
orchestrator.setPlanModelSnapshot(getCurrentModel());
62146
await orchestrator.resumePlan();
63147

64-
const retryMsg = args.retry ? ` Job "${args.retry}" reset to ready_to_merge.` : '';
148+
const acceptMsg = ctx?.failureKind === 'touchset'
149+
? ` TouchSet violations for job "${ctx.jobName}" accepted.`
150+
: '';
65151
return [
66-
`Checkpoint "${checkpoint}" cleared.${retryMsg} Plan "${plan.name}" resuming.`,
152+
`Checkpoint "${checkpoint}" cleared.${acceptMsg} Plan "${plan.name}" resuming.`,
67153
'',
68154
` ID: ${plan.id}`,
69155
` Mode: ${plan.mode}`,
@@ -78,7 +164,6 @@ export const mc_plan_approve: ToolDefinition = tool({
78164
);
79165
}
80166

81-
// Create integration infrastructure that copilot mode skipped
82167
const config = await loadConfig();
83168
const integrationPostCreate = resolvePostCreateHook(config.worktreeSetup);
84169
const integration = plan.baseBranch

‎tests/tools/plan-approve.test.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,6 @@ describe('mc_plan_approve', () => {
9292
expect(mockUpdatePlanJob).toHaveBeenCalledWith('plan-1', 'bad-job', { status: 'ready_to_merge', error: undefined });
9393
expect(result).toContain('bad-job');
9494
expect(result).toContain('ready_to_merge');
95-
expect(result).toContain('resuming');
9695
expect(mockSavePlan).toHaveBeenCalled();
9796
expect(mockResumePlan).toHaveBeenCalled();
9897
});
@@ -122,7 +121,7 @@ describe('mc_plan_approve', () => {
122121

123122
expect(mockUpdatePlanJob).toHaveBeenCalledWith('plan-1', 'conflicting-job', { status: 'ready_to_merge', error: undefined });
124123
expect(result).toContain('Checkpoint');
125-
expect(result).toContain('resuming');
124+
expect(result).toContain('ready_to_merge');
126125
expect(mockSavePlan).toHaveBeenCalled();
127126
expect(mockResumePlan).toHaveBeenCalled();
128127
});

0 commit comments

Comments
 (0)