fix: prevent double mergeAndExit on milestone completion (#2648)
When a milestone completes, phases.ts calls mergeAndExit to merge the worktree branch back to main. It then calls closeoutAndStop → stopAuto, which unconditionally calls mergeAndExit again. The second call fails because the branch was already deleted by the first merge, producing a misleading 'not something we can merge' warning even though the merge succeeded. Add a milestoneMergedInPhases session flag that phases.ts sets after a successful merge. stopAuto checks this flag and skips its own merge when it is already set. The flag is cleared in AutoSession.reset() so it does not leak across sessions. Closes #2645
This commit is contained in:
parent
b03694fb4f
commit
badcaa3152
4 changed files with 111 additions and 1 deletions
|
|
@ -590,8 +590,11 @@ export async function stopAuto(
|
|||
// When the milestone is complete (has a SUMMARY), merge the worktree branch
|
||||
// back to main so code isn't stranded on the worktree branch (#2317).
|
||||
// For incomplete milestones, preserve the branch for later resumption.
|
||||
//
|
||||
// Skip if phases.ts already merged this milestone — avoids the double
|
||||
// mergeAndExit that fails because the branch was already deleted (#2645).
|
||||
try {
|
||||
if (s.currentMilestoneId) {
|
||||
if (s.currentMilestoneId && !s.milestoneMergedInPhases) {
|
||||
const notifyCtx = ctx
|
||||
? { notify: ctx.ui.notify.bind(ctx.ui) }
|
||||
: { notify: () => {} };
|
||||
|
|
|
|||
|
|
@ -333,6 +333,8 @@ export async function runPreDispatch(
|
|||
if (s.currentMilestoneId) {
|
||||
try {
|
||||
deps.resolver.mergeAndExit(s.currentMilestoneId, ctx.ui);
|
||||
// Prevent stopAuto from attempting the same merge (#2645)
|
||||
s.milestoneMergedInPhases = true;
|
||||
} catch (mergeErr) {
|
||||
if (mergeErr instanceof MergeConflictError) {
|
||||
ctx.ui.notify(
|
||||
|
|
@ -428,6 +430,8 @@ export async function runPreDispatch(
|
|||
if (s.currentMilestoneId) {
|
||||
try {
|
||||
deps.resolver.mergeAndExit(s.currentMilestoneId, ctx.ui);
|
||||
// Prevent stopAuto from attempting the same merge (#2645)
|
||||
s.milestoneMergedInPhases = true;
|
||||
} catch (mergeErr) {
|
||||
if (mergeErr instanceof MergeConflictError) {
|
||||
ctx.ui.notify(
|
||||
|
|
|
|||
|
|
@ -122,6 +122,11 @@ export class AutoSession {
|
|||
/** Set to true when worktree creation fails; prevents merge of nonexistent branch. */
|
||||
isolationDegraded = false;
|
||||
|
||||
// ── Merge guard ──────────────────────────────────────────────────────
|
||||
/** Set to true after phases.ts successfully calls mergeAndExit, so that
|
||||
* stopAuto does not attempt the same merge a second time (#2645). */
|
||||
milestoneMergedInPhases = false;
|
||||
|
||||
// ── Dispatch circuit breakers ──────────────────────────────────────
|
||||
rewriteAttemptCount = 0;
|
||||
|
||||
|
|
@ -205,6 +210,7 @@ export class AutoSession {
|
|||
this.sidecarQueue = [];
|
||||
this.rewriteAttemptCount = 0;
|
||||
this.isolationDegraded = false;
|
||||
this.milestoneMergedInPhases = false;
|
||||
|
||||
// Signal handler
|
||||
this.sigtermHandler = null;
|
||||
|
|
|
|||
|
|
@ -0,0 +1,97 @@
|
|||
import { describe, test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { join, dirname } from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { AutoSession } from "../auto/session.ts";
|
||||
|
||||
const __dirname = dirname(fileURLToPath(import.meta.url));
|
||||
|
||||
describe("double mergeAndExit guard (#2645)", () => {
|
||||
test("phases.ts sets milestoneMergedInPhases after mergeAndExit in milestone-complete path", () => {
|
||||
// Source audit: the "complete" phase path must set the guard flag
|
||||
// after calling mergeAndExit so that stopAuto skips the second merge.
|
||||
const phasesSrc = readFileSync(
|
||||
join(__dirname, "..", "auto", "phases.ts"),
|
||||
"utf-8",
|
||||
);
|
||||
|
||||
// Find the "complete" phase block
|
||||
const completeIdx = phasesSrc.indexOf('state.phase === "complete"');
|
||||
assert.ok(completeIdx > 0, "phases.ts should have a 'complete' phase check");
|
||||
|
||||
const afterComplete = phasesSrc.slice(completeIdx, completeIdx + 600);
|
||||
const mergeIdx = afterComplete.indexOf("deps.resolver.mergeAndExit");
|
||||
const flagIdx = afterComplete.indexOf("s.milestoneMergedInPhases = true");
|
||||
|
||||
assert.ok(mergeIdx > 0, "complete path should call mergeAndExit");
|
||||
assert.ok(flagIdx > 0, "complete path should set milestoneMergedInPhases");
|
||||
assert.ok(
|
||||
flagIdx > mergeIdx,
|
||||
"milestoneMergedInPhases must be set AFTER mergeAndExit (not before)",
|
||||
);
|
||||
});
|
||||
|
||||
test("phases.ts sets milestoneMergedInPhases after mergeAndExit in all-milestones-complete path", () => {
|
||||
const phasesSrc = readFileSync(
|
||||
join(__dirname, "..", "auto", "phases.ts"),
|
||||
"utf-8",
|
||||
);
|
||||
|
||||
// The "all milestones complete" block checks incomplete.length === 0
|
||||
const allCompleteIdx = phasesSrc.indexOf("incomplete.length === 0");
|
||||
assert.ok(allCompleteIdx > 0, "phases.ts should have an all-milestones-complete check");
|
||||
|
||||
const afterAllComplete = phasesSrc.slice(allCompleteIdx, allCompleteIdx + 600);
|
||||
const mergeIdx = afterAllComplete.indexOf("deps.resolver.mergeAndExit");
|
||||
const flagIdx = afterAllComplete.indexOf("s.milestoneMergedInPhases = true");
|
||||
|
||||
assert.ok(mergeIdx > 0, "all-complete path should call mergeAndExit");
|
||||
assert.ok(flagIdx > 0, "all-complete path should set milestoneMergedInPhases");
|
||||
assert.ok(
|
||||
flagIdx > mergeIdx,
|
||||
"milestoneMergedInPhases must be set AFTER mergeAndExit (not before)",
|
||||
);
|
||||
});
|
||||
|
||||
test("stopAuto checks milestoneMergedInPhases before calling mergeAndExit", () => {
|
||||
const autoSrc = readFileSync(
|
||||
join(__dirname, "..", "auto.ts"),
|
||||
"utf-8",
|
||||
);
|
||||
|
||||
// The Step 4 worktree exit block must check the guard flag
|
||||
const step4Idx = autoSrc.indexOf("Step 4: Auto-worktree exit");
|
||||
assert.ok(step4Idx > 0, "auto.ts should have Step 4 worktree exit");
|
||||
|
||||
const step4Block = autoSrc.slice(step4Idx, step4Idx + 600);
|
||||
assert.ok(
|
||||
step4Block.includes("milestoneMergedInPhases"),
|
||||
"stopAuto Step 4 must check milestoneMergedInPhases before merging",
|
||||
);
|
||||
assert.ok(
|
||||
step4Block.includes("!s.milestoneMergedInPhases"),
|
||||
"stopAuto should skip merge when milestoneMergedInPhases is true",
|
||||
);
|
||||
});
|
||||
|
||||
test("AutoSession.milestoneMergedInPhases defaults to false", () => {
|
||||
const session = new AutoSession();
|
||||
assert.equal(
|
||||
session.milestoneMergedInPhases,
|
||||
false,
|
||||
"new session should have milestoneMergedInPhases = false",
|
||||
);
|
||||
});
|
||||
|
||||
test("AutoSession.reset() clears milestoneMergedInPhases", () => {
|
||||
const session = new AutoSession();
|
||||
session.milestoneMergedInPhases = true;
|
||||
session.reset();
|
||||
assert.equal(
|
||||
session.milestoneMergedInPhases,
|
||||
false,
|
||||
"reset() should clear milestoneMergedInPhases back to false",
|
||||
);
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue