Skip to content

fix(cli): harden PR merge path against stale-base dead-ends (empty-diff no-op + conflict escape) #1943

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flexi767 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(cli): harden PR merge path against stale-base dead-ends (empty-diff no-op + conflict escape) #1943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 73 additions & 6 deletions packages/cli/src/commands/__tests__/task-lifecycle.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" });
});
Comment on lines +671 to +736

@coderabbitai coderabbitai Bot Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add regression coverage for the shared-branch-group surface.

Both new tests (and the reworked no-delta test) exercise only the per-task PR creation/merge path. The same two behaviors also have a shared-branch-group surface: no-op finalization on "No commits between" (Line 799-812) and the not-ready/conflicting handling (Line 858-861). Please enumerate and assert the invariant on the shared-group surface too — at minimum a shared-group "No commits between" → no-op done case, and (if the gap flagged in task-lifecycle.ts is addressed) a shared-group conflicting case.

As per coding guidelines: "When fixing a bug, regression tests must assert the general invariant across all known surfaces, and bug-fix work must include symptom verification and surface enumeration rather than only the single reproduction."

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/cli/src/commands/__tests__/task-lifecycle.test.ts` around lines 671
- 736, The current tests cover only the per-task PR path, but the same invariant
must also be verified on the shared-branch-group path. Add regression coverage
in the task lifecycle tests for the shared-group surface by exercising the
shared-group finalization flow that handles the “No commits between” case and
asserting it resolves as a no-op done outcome, and also cover the shared-group
not-ready/conflicting handling if the task-lifecycle logic now routes there. Use
the existing task lifecycle helpers and the shared-group branch-group
entrypoints in task-lifecycle.test.ts to locate the relevant flows.

Source: Coding guidelines


it("skips the push when an existing PR already covers the branch", async () => {
const task: MockTask = {
id: "FN-9002",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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" }),
);
});

Expand Down
59 changes: 53 additions & 6 deletions packages/cli/src/commands/task-lifecycle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 });
}
Expand Down
Loading