fix: prevent savePlan from overwriting job status set by accept/retry paths

nigel-dev · nigel-dev · commit 84e5a7f2ced2 · 2026-02-14T06:23:47.000-06:00
The accept and retry paths in mc_plan_approve called updatePlanJob()
to set the job to ready_to_merge, then immediately called savePlan()
with the stale in-memory plan object — overwriting the update and
leaving the job stuck in failed state.

Fix by updating the in-memory plan.jobs directly before savePlan(),
removing the now-unnecessary updatePlanJob import. Also adds
checkpointContext assertions to the orchestrator touchSet test.
diff --git 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, updatePlanJob } from '../lib/plan-state';
+import { loadPlan, savePlan } from '../lib/plan-state';
 import { Orchestrator } from '../lib/orchestrator';
 import { getSharedMonitor, getSharedNotifyCallback, setSharedOrchestrator } from '../lib/orchestrator-singleton';
 import type { CheckpointType } from '../lib/plan-types';
@@ -105,7 +105,9 @@ export const mc_plan_approve: ToolDefinition = tool({
           }
         }
 
-        await updatePlanJob(plan.id, args.retry, { status: 'ready_to_merge', error: undefined });
+        const retryJob = plan.jobs.find(j => j.name === args.retry)!;
+        retryJob.status = 'ready_to_merge';
+        retryJob.error = undefined;
 
         plan.status = 'running';
         plan.checkpoint = null;
@@ -131,7 +133,8 @@ export const mc_plan_approve: ToolDefinition = tool({
       if (ctx?.failureKind === 'touchset') {
         const job = plan.jobs.find(j => j.name === ctx.jobName);
         if (job && job.status === 'failed') {
-          await updatePlanJob(plan.id, ctx.jobName, { status: 'ready_to_merge', error: undefined });
+          job.status = 'ready_to_merge';
+          job.error = undefined;
         }
       }
 
diff --git a/tests/lib/orchestrator.test.ts b/tests/lib/orchestrator.test.ts
@@ -636,6 +636,14 @@ describe('orchestrator', () => {
     );
     expect(planState?.status).toBe('paused');
     expect(planState?.checkpoint).toBe('on_error');
+    expect(planState?.checkpointContext).toEqual(
+      expect.objectContaining({
+        jobName: 'touch-violator',
+        failureKind: 'touchset',
+        touchSetViolations: expect.arrayContaining(['README.md']),
+        touchSetPatterns: expect.arrayContaining(['src/**']),
+      }),
+    );
   });
 
   it('should allow transition to ready_to_merge when touchSet is satisfied', async () => {
diff --git a/tests/tools/plan-approve.test.ts b/tests/tools/plan-approve.test.ts
@@ -3,6 +3,7 @@ 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';
+import * as mergeTrain from '../../src/lib/merge-train';
 
 const { mc_plan_approve } = await import('../../src/tools/plan-approve');
 
@@ -65,6 +66,12 @@ describe('mc_plan_approve', () => {
   });
 
   describe('retry validation', () => {
+    it('should reject when both retry and relaunch are provided', async () => {
+      expect(
+        mc_plan_approve.execute({ checkpoint: 'on_error', retry: 'bad-job', relaunch: 'bad-job' }, mockContext),
+      ).rejects.toThrow('Cannot specify both "retry" and "relaunch"');
+    });
+
     it('should reset a failed job to ready_to_merge when retry is provided', async () => {
       spyOn(planState, 'loadPlan').mockResolvedValue({
         id: 'plan-1',
@@ -82,20 +89,224 @@ describe('mc_plan_approve', () => {
       });
 
       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(mockSavePlan).toHaveBeenCalledWith(expect.objectContaining({
+        status: 'running',
+        jobs: expect.arrayContaining([
+          expect.objectContaining({ name: 'bad-job', status: 'ready_to_merge' }),
+        ]),
+      }));
       expect(result).toContain('bad-job');
       expect(result).toContain('ready_to_merge');
-      expect(mockSavePlan).toHaveBeenCalled();
       expect(mockResumePlan).toHaveBeenCalled();
     });
 
+    it('should accept touchSet violations and move failed job to ready_to_merge', async () => {
+      spyOn(planState, 'loadPlan').mockResolvedValue({
+        id: 'plan-1',
+        name: 'TouchSet Plan',
+        mode: 'supervisor',
+        status: 'paused',
+        checkpoint: 'on_error',
+        checkpointContext: {
+          jobName: 'touch-job',
+          failureKind: 'touchset',
+          touchSetViolations: ['README.md'],
+          touchSetPatterns: ['src/**'],
+        },
+        jobs: [
+          { id: 'j1', name: 'touch-job', prompt: 'fix files', status: 'failed', error: 'touchSet violation' },
+        ],
+        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: 'on_error' }, mockContext);
+
+      expect(mockSavePlan).toHaveBeenCalledWith(expect.objectContaining({
+        status: 'running',
+        checkpoint: null,
+        checkpointContext: null,
+        jobs: expect.arrayContaining([
+          expect.objectContaining({ name: 'touch-job', status: 'ready_to_merge' }),
+        ]),
+      }));
+      expect(mockResumePlan).toHaveBeenCalled();
+      expect(result).toContain('TouchSet violations for job "touch-job" accepted');
+    });
+
+    it('should relaunch touchSet-failed job for correction', async () => {
+      spyOn(planState, 'loadPlan').mockResolvedValue({
+        id: 'plan-1',
+        name: 'TouchSet Relaunch Plan',
+        mode: 'supervisor',
+        status: 'paused',
+        checkpoint: 'on_error',
+        checkpointContext: {
+          jobName: 'touch-job',
+          failureKind: 'touchset',
+          touchSetViolations: ['README.md'],
+          touchSetPatterns: ['src/**'],
+        },
+        jobs: [
+          {
+            id: 'j1',
+            name: 'touch-job',
+            prompt: 'fix files',
+            status: 'failed',
+            touchSet: ['src/**'],
+            branch: 'mc/plan/plan-1/touch-job',
+          },
+        ],
+        integrationBranch: 'mc/integration/plan-1',
+        baseCommit: 'abc123',
+        createdAt: new Date().toISOString(),
+      });
+
+      const mockSavePlan = spyOn(planState, 'savePlan').mockResolvedValue(undefined);
+      const mockResumePlan = mock().mockResolvedValue(undefined);
+      const relaunchSpy = mock().mockResolvedValue(undefined);
+      spyOn(orchestrator.Orchestrator.prototype, 'resumePlan').mockImplementation(mockResumePlan);
+      spyOn(orchestrator.Orchestrator.prototype, 'relaunchJobForCorrection').mockImplementation(relaunchSpy);
+      spyOn(orchestrator.Orchestrator.prototype, 'setPlanModelSnapshot').mockImplementation(() => {});
+
+      const result = await mc_plan_approve.execute({ checkpoint: 'on_error', relaunch: 'touch-job' }, mockContext);
+
+      expect(mockSavePlan).toHaveBeenCalledWith(expect.objectContaining({
+        status: 'running',
+        checkpoint: null,
+        checkpointContext: null,
+      }));
+      expect(relaunchSpy).toHaveBeenCalledWith('touch-job', ['README.md'], ['src/**']);
+      expect(mockResumePlan).toHaveBeenCalled();
+      expect(result).toContain('relaunched with correction prompt');
+    });
+
+    it('should re-validate touchSet on retry before proceeding', async () => {
+      spyOn(planState, 'loadPlan').mockResolvedValue({
+        id: 'plan-1',
+        name: 'TouchSet Retry Plan',
+        mode: 'supervisor',
+        status: 'paused',
+        checkpoint: 'on_error',
+        checkpointContext: {
+          jobName: 'touch-job',
+          failureKind: 'touchset',
+          touchSetViolations: ['README.md'],
+          touchSetPatterns: ['src/**'],
+        },
+        jobs: [
+          {
+            id: 'j1',
+            name: 'touch-job',
+            prompt: 'fix files',
+            status: 'failed',
+            touchSet: ['src/**'],
+            branch: 'mc/plan/plan-1/touch-job',
+          },
+        ],
+        integrationBranch: 'mc/integration/plan-1',
+        baseCommit: 'abc123',
+        createdAt: new Date().toISOString(),
+      });
+
+      const validateSpy = spyOn(mergeTrain, 'validateTouchSet').mockResolvedValue({
+        valid: true,
+        changedFiles: ['src/main.ts'],
+      });
+      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: 'touch-job' }, mockContext);
+
+      expect(validateSpy).toHaveBeenCalledWith('mc/plan/plan-1/touch-job', 'mc/integration/plan-1', ['src/**']);
+      expect(mockSavePlan).toHaveBeenCalledWith(expect.objectContaining({
+        status: 'running',
+        jobs: expect.arrayContaining([
+          expect.objectContaining({ name: 'touch-job', status: 'ready_to_merge' }),
+        ]),
+      }));
+      expect(mockResumePlan).toHaveBeenCalled();
+      expect(result).toContain('touch-job');
+      expect(result).toContain('ready_to_merge');
+    });
+
+    it('should reject retry when touchSet remains violated after manual fix', async () => {
+      spyOn(planState, 'loadPlan').mockResolvedValue({
+        id: 'plan-1',
+        name: 'TouchSet Retry Plan',
+        mode: 'supervisor',
+        status: 'paused',
+        checkpoint: 'on_error',
+        checkpointContext: {
+          jobName: 'touch-job',
+          failureKind: 'touchset',
+          touchSetViolations: ['README.md'],
+          touchSetPatterns: ['src/**'],
+        },
+        jobs: [
+          {
+            id: 'j1',
+            name: 'touch-job',
+            prompt: 'fix files',
+            status: 'failed',
+            touchSet: ['src/**'],
+            branch: 'mc/plan/plan-1/touch-job',
+          },
+        ],
+        integrationBranch: 'mc/integration/plan-1',
+        baseCommit: 'abc123',
+        createdAt: new Date().toISOString(),
+      });
+
+      spyOn(mergeTrain, 'validateTouchSet').mockResolvedValue({
+        valid: false,
+        violations: ['README.md'],
+        changedFiles: ['src/main.ts', 'README.md'],
+      });
+
+      expect(
+        mc_plan_approve.execute({ checkpoint: 'on_error', retry: 'touch-job' }, mockContext),
+      ).rejects.toThrow('still has touchSet violations after manual fix');
+    });
+
+    it('should reject relaunch when failure is not touchSet-related', async () => {
+      spyOn(planState, 'loadPlan').mockResolvedValue({
+        id: 'plan-1',
+        name: 'Non TouchSet Plan',
+        mode: 'supervisor',
+        status: 'paused',
+        checkpoint: 'on_error',
+        checkpointContext: {
+          jobName: 'bad-job',
+          failureKind: 'test_failure',
+        },
+        jobs: [
+          { id: 'j1', name: 'bad-job', prompt: 'fix', status: 'failed', error: 'tests failed' },
+        ],
+        integrationBranch: 'mc/integration/plan-1',
+        baseCommit: 'abc123',
+        createdAt: new Date().toISOString(),
+      });
+
+      expect(
+        mc_plan_approve.execute({ checkpoint: 'on_error', relaunch: 'bad-job' }, mockContext),
+      ).rejects.toThrow('was not failed due to a touchSet violation');
+    });
+
     it('should allow retry of needs_rebase jobs', async () => {
       spyOn(planState, 'loadPlan').mockResolvedValue({
         id: 'plan-1',
@@ -111,18 +322,21 @@ describe('mc_plan_approve', () => {
         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(mockSavePlan).toHaveBeenCalledWith(expect.objectContaining({
+        status: 'running',
+        jobs: expect.arrayContaining([
+          expect.objectContaining({ name: 'conflicting-job', status: 'ready_to_merge' }),
+        ]),
+      }));
       expect(result).toContain('Checkpoint');
       expect(result).toContain('ready_to_merge');
-      expect(mockSavePlan).toHaveBeenCalled();
       expect(mockResumePlan).toHaveBeenCalled();
     });
 

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`import { tool, type ToolDefinition } from '@opencode-ai/plugin';`
`2`		`-import { loadPlan, savePlan, updatePlanJob } from '../lib/plan-state';`
	`2`	`+import { loadPlan, savePlan } from '../lib/plan-state';`
`3`	`3`	`import { Orchestrator } from '../lib/orchestrator';`
`4`	`4`	`import { getSharedMonitor, getSharedNotifyCallback, setSharedOrchestrator } from '../lib/orchestrator-singleton';`
`5`	`5`	`import type { CheckpointType } from '../lib/plan-types';`
`@@ -105,7 +105,9 @@ export const mc_plan_approve: ToolDefinition = tool({`
`105`	`105`	`}`
`106`	`106`	`}`
`107`	`107`
`108`		`- await updatePlanJob(plan.id, args.retry, { status: 'ready_to_merge', error: undefined });`
	`108`	`+ const retryJob = plan.jobs.find(j => j.name === args.retry)!;`
	`109`	`+ retryJob.status = 'ready_to_merge';`
	`110`	`+ retryJob.error = undefined;`
`109`	`111`
`110`	`112`	`plan.status = 'running';`
`111`	`113`	`plan.checkpoint = null;`
`@@ -131,7 +133,8 @@ export const mc_plan_approve: ToolDefinition = tool({`
`131`	`133`	`if (ctx?.failureKind === 'touchset') {`
`132`	`134`	`const job = plan.jobs.find(j => j.name === ctx.jobName);`
`133`	`135`	`if (job && job.status === 'failed') {`
`134`		`- await updatePlanJob(plan.id, ctx.jobName, { status: 'ready_to_merge', error: undefined });`
	`136`	`+ job.status = 'ready_to_merge';`
	`137`	`+ job.error = undefined;`
`135`	`138`	`}`
`136`	`139`	`}`
`137`	`140`