fix(cli): harden dormant PR merge path against stale-base dead-ends
Two failure modes wedged the PR-based merge flow (dormant under the
current direct-merge config, but the rollback target):
1. Empty-diff branches ("No commits between ...") were marked `failed`,
pinning the serial merge slot + file leases on a task that is a
legitimate no-op. Now finalized as a terminal DONE via
`finalizeNoOpMergeTask`, mirroring the engine's canonical
`noOpResult` decision (merger-ai.ts).
2. A stale-base PR that GitHub reports CONFLICTING never becomes
mergeable on its own (nothing in the PR path rebases the head), so
`awaiting-pr-checks` waited unbounded — no escape, because the PR
path never incremented `mergeRetries`. Now each conflicting poll
counts against `mergeRetries`, so the existing
`getInReviewStallReason` "merge-retries-exhausted" escape disposes
the task after `maxAutoMergeRetries` cycles. Pending/behind PRs
still wait (checks legitimately run; "behind" is fixed by rebase).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
committed by
gsxdsm
parent
3ef90bc6fe
commit
87b787319f
@@ -668,6 +668,73 @@ describe("processPullRequestMergeTask", () => {
|
||||
expect(github.getPrMergeStatus).toHaveBeenCalledWith("owner", "repo", 7);
|
||||
});
|
||||
|
||||
it("counts a conflicting stale-base PR against mergeRetries so the stall escape fires", async () => {
|
||||
const task: MockTask = {
|
||||
id: "FN-9020",
|
||||
title: "test",
|
||||
description: "desc",
|
||||
column: "in-review",
|
||||
mergeRetries: 1,
|
||||
};
|
||||
const branch = getTaskBranchName(task.id);
|
||||
const store = makeStore(task);
|
||||
execMock.mockImplementation(() => "");
|
||||
|
||||
const existingPr = { number: 9, url: "https://github.com/x/y/pull/9", status: "open" as const, headBranch: branch, baseBranch: "main" };
|
||||
const github = {
|
||||
findPrForBranch: vi.fn(async () => existingPr),
|
||||
createPr: vi.fn(),
|
||||
getPrMergeStatus: vi.fn(async () => ({
|
||||
prInfo: { ...existingPr, mergeable: "conflicting" as const },
|
||||
reviewDecision: null,
|
||||
checks: [],
|
||||
mergeReady: false,
|
||||
blockingReasons: [],
|
||||
})),
|
||||
mergePr: vi.fn(),
|
||||
};
|
||||
|
||||
const result = await processPullRequestMergeTask(store as never, "/repo", task.id, github as never, () => undefined);
|
||||
|
||||
expect(result).toBe("waiting");
|
||||
expect(store.updateTask).toHaveBeenCalledWith(task.id, {
|
||||
status: "awaiting-pr-checks",
|
||||
mergeRetries: 2,
|
||||
});
|
||||
});
|
||||
|
||||
it("does not bump mergeRetries for a non-conflicting not-ready PR", async () => {
|
||||
const task: MockTask = {
|
||||
id: "FN-9021",
|
||||
title: "test",
|
||||
description: "desc",
|
||||
column: "in-review",
|
||||
mergeRetries: 1,
|
||||
};
|
||||
const branch = getTaskBranchName(task.id);
|
||||
const store = makeStore(task);
|
||||
execMock.mockImplementation(() => "");
|
||||
|
||||
const existingPr = { number: 10, url: "https://github.com/x/y/pull/10", status: "open" as const, headBranch: branch, baseBranch: "main" };
|
||||
const github = {
|
||||
findPrForBranch: vi.fn(async () => existingPr),
|
||||
createPr: vi.fn(),
|
||||
getPrMergeStatus: vi.fn(async () => ({
|
||||
prInfo: { ...existingPr, mergeable: "unknown" as const },
|
||||
reviewDecision: null,
|
||||
checks: [],
|
||||
mergeReady: false,
|
||||
blockingReasons: [],
|
||||
})),
|
||||
mergePr: vi.fn(),
|
||||
};
|
||||
|
||||
const result = await processPullRequestMergeTask(store as never, "/repo", task.id, github as never, () => undefined);
|
||||
|
||||
expect(result).toBe("waiting");
|
||||
expect(store.updateTask).toHaveBeenCalledWith(task.id, { status: "awaiting-pr-checks" });
|
||||
});
|
||||
|
||||
it("skips the push when an existing PR already covers the branch", async () => {
|
||||
const task: MockTask = {
|
||||
id: "FN-9002",
|
||||
@@ -883,7 +950,7 @@ describe("processPullRequestMergeTask", () => {
|
||||
expect(github.createPr).toHaveBeenCalledWith(expect.objectContaining({ head: branch }));
|
||||
});
|
||||
|
||||
it("parks no-delta branches instead of retrying into branch push failures", async () => {
|
||||
it("finalizes no-delta branches as a no-op done instead of failing", async () => {
|
||||
const task: MockTask = {
|
||||
id: "FN-9012",
|
||||
title: "test",
|
||||
@@ -913,13 +980,13 @@ describe("processPullRequestMergeTask", () => {
|
||||
|
||||
expect(result).toBe("skipped");
|
||||
expect(store.updateTask).toHaveBeenCalledWith(task.id, {
|
||||
status: "failed",
|
||||
error: `No pull request created for ${branch}: the branch has no commits relative to the base branch.`,
|
||||
status: null,
|
||||
mergeRetries: 0,
|
||||
});
|
||||
expect(store.logEntry).toHaveBeenCalledWith(
|
||||
expect(store.moveTask).toHaveBeenCalledWith(task.id, "done");
|
||||
expect(store.updateTask).not.toHaveBeenCalledWith(
|
||||
task.id,
|
||||
`No pull request created for ${branch}: the branch has no commits relative to the base branch.`,
|
||||
expect.stringContaining("No commits between"),
|
||||
expect.objectContaining({ status: "failed" }),
|
||||
);
|
||||
});
|
||||
|
||||
|
||||
@@ -640,6 +640,40 @@ async function finalizePullRequestMerge(
|
||||
} as MergeResult);
|
||||
}
|
||||
|
||||
/**
|
||||
* Finalize a task whose branch has no commits relative to the base branch
|
||||
* ("No commits between ...") as a terminal no-op DONE, mirroring the engine's
|
||||
* canonical zero-commits decision (merger-ai.ts `noOpResult` → done, PR #1920).
|
||||
* Marking it `failed` here left an empty-diff follow-up task pinning the merge
|
||||
* slot + file leases indefinitely; a no-op branch is not a failure — there was
|
||||
* simply nothing to merge.
|
||||
*/
|
||||
async function finalizeNoOpMergeTask(
|
||||
store: TaskStore,
|
||||
cwd: string,
|
||||
task: TaskDetail,
|
||||
reason: string,
|
||||
pool?: WorktreePool,
|
||||
): Promise<void> {
|
||||
const branch = task.branch ?? getTaskBranchName(task.id);
|
||||
await cleanupMergedTaskArtifacts(cwd, task, { pool });
|
||||
await store.updateTask(task.id, { status: null, mergeRetries: 0 });
|
||||
const movedTask = await store.moveTask(task.id, "done");
|
||||
const mergedTask = movedTask ?? (await store.getTask(task.id));
|
||||
await store.logEntry(task.id, reason, `Branch ${branch} has no commits relative to the base branch; nothing to merge.`);
|
||||
store.emit("task:merged", {
|
||||
task: mergedTask,
|
||||
branch: mergedTask.branch ?? branch,
|
||||
merged: false,
|
||||
noOp: true,
|
||||
ok: true,
|
||||
reason,
|
||||
mergeConfirmed: true,
|
||||
worktreeRemoved: false,
|
||||
branchDeleted: false,
|
||||
} as MergeResult);
|
||||
}
|
||||
|
||||
/**
|
||||
* Result of processing a PR merge task.
|
||||
* - "waiting": PR exists but not ready to merge (checks pending, reviews needed)
|
||||
@@ -774,8 +808,7 @@ export async function processPullRequestMergeTask(
|
||||
const message = err instanceof Error ? err.message : String(err);
|
||||
if (message.includes("No commits between")) {
|
||||
await store.updateBranchGroup(branchGroup.id, { prState: "none", prNumber: null, prUrl: null });
|
||||
await store.updateTask(task.id, { status: "failed", error: `No pull request created for ${branchGroup.branchName}: no commits relative to ${projectDefaultBranch}.` });
|
||||
await store.logEntry(task.id, "No group pull request created", message);
|
||||
await finalizeNoOpMergeTask(store, cwd, task, "No group pull request created (no commits vs base) — finalizing as no-op", pool);
|
||||
return "skipped";
|
||||
}
|
||||
throw err;
|
||||
@@ -878,9 +911,7 @@ export async function processPullRequestMergeTask(
|
||||
} catch (err: unknown) {
|
||||
const message = err instanceof Error ? err.message : String(err);
|
||||
if (message.includes("No commits between")) {
|
||||
const error = `No pull request created for ${branch}: the branch has no commits relative to the base branch.`;
|
||||
await store.updateTask(task.id, { status: "failed", error });
|
||||
await store.logEntry(task.id, error, message);
|
||||
await finalizeNoOpMergeTask(store, cwd, task, "No pull request created (no commits vs base) — finalizing as no-op", pool);
|
||||
return "skipped";
|
||||
}
|
||||
throw err;
|
||||
@@ -924,7 +955,23 @@ export async function processPullRequestMergeTask(
|
||||
|
||||
if (!mergeStatus.mergeReady) {
|
||||
if (mergeStatus.prInfo.status === "open") {
|
||||
await store.updateTask(task.id, { status: "awaiting-pr-checks" });
|
||||
// A stale-base PR that GitHub reports as CONFLICTING never becomes
|
||||
// mergeable on its own — nothing in the PR path rebases the head branch —
|
||||
// so an unbounded `awaiting-pr-checks` wait pins the merge slot + file
|
||||
// leases forever (FN-485). Count each conflicting poll against
|
||||
// `mergeRetries` so the existing `getInReviewStallReason`
|
||||
// "merge-retries-exhausted" escape disposes the task after
|
||||
// `maxAutoMergeRetries` cycles. Pending/behind PRs still wait indefinitely
|
||||
// (checks legitimately run; "behind" is resolved by the pre-merge rebase).
|
||||
// ponytail: bounded escape via the existing stall counter, not an in-path
|
||||
// rebase. Upgrade path: rebase the head branch onto base + force-push here
|
||||
// before giving up, then reset mergeRetries.
|
||||
await store.updateTask(task.id, {
|
||||
status: "awaiting-pr-checks",
|
||||
...(mergeStatus.prInfo.mergeable === "conflicting"
|
||||
? { mergeRetries: (task.mergeRetries ?? 0) + 1 }
|
||||
: {}),
|
||||
});
|
||||
} else {
|
||||
await store.updateTask(task.id, { status: null });
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user