Skip to content

Commit afd0209

Browse files
jonathanlabjonathanlab
committed
more fixes and safeguards for merging
1 parent 72b5b0e commit afd0209

File tree

4 files changed

+295
-49
lines changed

4 files changed

+295
-49
lines changed

‎apps/cli/src/commands/merge.ts‎

Lines changed: 110 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { GitHub } from "@array/core";
1+
import { GitHub, type PRStatus } from "@array/core";
22
import { cyan, dim, formatError, formatSuccess, yellow } from "../utils/output";
33
import { createJJ, unwrap } from "../utils/run";
44

@@ -8,6 +8,12 @@ interface MergeFlags {
88
merge?: boolean;
99
}
1010

11+
interface PRToMerge {
12+
pr: PRStatus;
13+
bookmarkName: string;
14+
changeId: string | null;
15+
}
16+
1117
export async function merge(flags: MergeFlags = {}): Promise<void> {
1218
const jj = createJJ();
1319
const github = new GitHub(process.cwd());
@@ -40,65 +46,125 @@ export async function merge(flags: MergeFlags = {}): Promise<void> {
4046
);
4147
process.exit(1);
4248
}
43-
const pr = unwrap(await github.getPRForBranch(bookmarkName));
4449

45-
if (!pr) {
46-
console.error(formatError(`No PR found for branch ${cyan(bookmarkName)}`));
47-
console.log(dim(" Submit first with 'arr submit'"));
48-
process.exit(1);
50+
// Build the stack of PRs to merge (from current up to trunk)
51+
const prsToMerge: PRToMerge[] = [];
52+
let currentBookmark: string | null = bookmarkName;
53+
let currentChangeId: string | null = changeId;
54+
55+
while (currentBookmark) {
56+
const prResult = await github.getPRForBranch(currentBookmark);
57+
const pr: PRStatus | null = unwrap(prResult);
58+
59+
if (!pr) {
60+
console.error(
61+
formatError(`No PR found for branch ${cyan(currentBookmark)}`),
62+
);
63+
console.log(dim(" Submit first with 'arr submit'"));
64+
process.exit(1);
65+
}
66+
67+
if (pr.state === "merged") {
68+
// Already merged, skip and continue up the stack
69+
break;
70+
}
71+
72+
if (pr.state === "closed") {
73+
console.error(formatError(`PR #${pr.number} is closed (not merged)`));
74+
process.exit(1);
75+
}
76+
77+
prsToMerge.unshift({
78+
pr,
79+
bookmarkName: currentBookmark,
80+
changeId: currentChangeId,
81+
});
82+
83+
// Follow the base to find parent PRs
84+
if (pr.baseRefName === trunk) {
85+
break; // Reached trunk, we have the full stack
86+
}
87+
88+
currentBookmark = pr.baseRefName;
89+
currentChangeId = null; // We don't track changeIds for parent PRs
4990
}
5091

51-
if (pr.state === "merged") {
52-
console.log(yellow(`PR #${pr.number} is already merged`));
92+
if (prsToMerge.length === 0) {
93+
console.log(yellow("No open PRs to merge"));
5394
console.log(dim(" Running sync to update local state..."));
5495
unwrap(await jj.sync());
5596
console.log(formatSuccess("Synced"));
5697
return;
5798
}
5899

59-
if (pr.state === "closed") {
60-
console.error(formatError(`PR #${pr.number} is closed (not merged)`));
61-
process.exit(1);
62-
}
63-
64-
// Check if this is a stacked PR (base is not trunk)
65-
if (pr.baseRefName !== trunk) {
66-
console.error(
67-
formatError(
68-
`Cannot merge: PR #${pr.number} is stacked on ${cyan(pr.baseRefName)}`,
69-
),
70-
);
71-
console.log(
72-
dim(` Merge the base PR first, or use 'arr merge --stack' to merge all`),
73-
);
74-
process.exit(1);
75-
}
76-
77100
let method: "merge" | "squash" | "rebase" = "squash";
78101
if (flags.merge) method = "merge";
79102
if (flags.rebase) method = "rebase";
80103

81-
console.log(`Merging PR #${cyan(String(pr.number))}: ${pr.title}`);
82-
83-
// Merge and delete the remote branch to prevent stale references
84-
unwrap(
85-
await github.mergePR(pr.number, {
86-
method,
87-
deleteHead: true,
88-
headRef: bookmarkName,
89-
}),
104+
// Merge PRs from bottom to top
105+
console.log(
106+
`Merging ${prsToMerge.length} PR${prsToMerge.length > 1 ? "s" : ""} from stack...`,
90107
);
91-
console.log(formatSuccess(`Merged PR #${pr.number}`));
108+
console.log();
92109

93-
// Clean up local state: delete bookmark and abandon change
94-
if (bookmarkName) {
95-
await jj.deleteBookmark(bookmarkName);
96-
}
97-
if (changeId) {
98-
await jj.abandon(changeId);
110+
for (const { pr, bookmarkName: bookmark, changeId: chgId } of prsToMerge) {
111+
// SAFETY: Validate bookmark is not a protected branch
112+
const protectedBranches = [trunk, "main", "master", "develop"];
113+
if (protectedBranches.includes(bookmark)) {
114+
console.error(
115+
formatError(
116+
`Internal error: Cannot merge with protected branch as head: ${bookmark}`,
117+
),
118+
);
119+
process.exit(1);
120+
}
121+
122+
console.log(`Merging PR #${cyan(String(pr.number))}: ${pr.title}`);
123+
console.log(dim(` Branch: ${bookmark} → ${pr.baseRefName}`));
124+
125+
// Before merging, update the PR base to trunk if it's not already
126+
// (since we're merging bottom-up, each PR should target trunk after its base is merged)
127+
if (pr.baseRefName !== trunk) {
128+
await github.updatePR(pr.number, { base: trunk });
129+
130+
// Wait for GitHub to recalculate merge status after base change
131+
process.stdout.write(dim(" Waiting for GitHub..."));
132+
const mergeableResult = unwrap(
133+
await github.waitForMergeable(pr.number, {
134+
timeoutMs: 60000,
135+
pollIntervalMs: 2000,
136+
}),
137+
);
138+
process.stdout.write(`\r${" ".repeat(30)}\r`);
139+
140+
if (!mergeableResult.mergeable) {
141+
console.error(
142+
formatError(
143+
`PR #${pr.number} is not mergeable: ${mergeableResult.reason}`,
144+
),
145+
);
146+
process.exit(1);
147+
}
148+
}
149+
150+
unwrap(
151+
await github.mergePR(pr.number, {
152+
method,
153+
deleteHead: true,
154+
headRef: bookmark,
155+
}),
156+
);
157+
console.log(formatSuccess(`Merged PR #${pr.number}`));
158+
159+
// Clean up local state
160+
await jj.deleteBookmark(bookmark);
161+
if (chgId) {
162+
await jj.abandon(chgId);
163+
}
99164
}
100165

101-
console.log(dim(" Syncing to update local state..."));
166+
console.log();
167+
console.log(dim("Syncing to update local state..."));
102168
unwrap(await jj.sync());
103-
console.log(formatSuccess("Done! Change has been merged and synced."));
169+
console.log(formatSuccess("Done! All PRs merged and synced."));
104170
}

‎packages/core/src/github.ts‎

Lines changed: 113 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,11 @@ export interface GitHubClient {
7676
batchGetPRsForBranches(
7777
branchNames: string[],
7878
): Promise<Result<Map<string, PRStatus>>>;
79+
deleteBranch(branchName: string): Promise<Result<void>>;
80+
waitForMergeable(
81+
prNumber: number,
82+
options?: { timeoutMs?: number; pollIntervalMs?: number },
83+
): Promise<Result<{ mergeable: boolean; reason?: string }>>;
7984
}
8085

8186
export class GitHub implements GitHubClient {
@@ -375,6 +380,13 @@ export class GitHub implements GitHubClient {
375380
});
376381

377382
if (options?.deleteHead && options?.headRef) {
383+
// SAFETY: Never delete protected branches
384+
const protectedBranches = ["main", "master", "trunk", "develop"];
385+
if (protectedBranches.includes(options.headRef)) {
386+
// Silently skip - this should never happen but is catastrophic if it does
387+
return ok(undefined);
388+
}
389+
378390
try {
379391
await octokit.git.deleteRef({
380392
owner,
@@ -460,7 +472,20 @@ export class GitHub implements GitHubClient {
460472

461473
return ok({ url: pr.html_url, number: pr.number });
462474
} catch (e) {
463-
return err(createError("COMMAND_FAILED", `Failed to create PR: ${e}`));
475+
const error = e as Error & {
476+
status?: number;
477+
response?: {
478+
data?: { message?: string; errors?: Array<{ message?: string }> };
479+
};
480+
};
481+
const ghMessage = error.response?.data?.message || error.message;
482+
const ghErrors = error.response?.data?.errors
483+
?.map((err) => err.message)
484+
.join(", ");
485+
const details = ghErrors ? `${ghMessage} (${ghErrors})` : ghMessage;
486+
return err(
487+
createError("COMMAND_FAILED", `Failed to create PR: ${details}`),
488+
);
464489
}
465490
}
466491

@@ -512,6 +537,93 @@ export class GitHub implements GitHubClient {
512537
return err(createError("COMMAND_FAILED", `Failed to list PRs: ${e}`));
513538
}
514539
}
540+
541+
async deleteBranch(branchName: string): Promise<Result<void>> {
542+
// SAFETY: Never delete protected branches
543+
const protectedBranches = ["main", "master", "trunk", "develop"];
544+
if (protectedBranches.includes(branchName)) {
545+
return err(
546+
createError(
547+
"INVALID_STATE",
548+
`Cannot delete protected branch: ${branchName}`,
549+
),
550+
);
551+
}
552+
553+
const repoResult = await this.getRepoInfo();
554+
if (!repoResult.ok) return repoResult;
555+
556+
const { owner, repo } = repoResult.value;
557+
558+
try {
559+
const octokit = await this.getOctokit();
560+
await octokit.git.deleteRef({
561+
owner,
562+
repo,
563+
ref: `heads/${branchName}`,
564+
});
565+
return ok(undefined);
566+
} catch (e) {
567+
// Branch might not exist, which is fine
568+
const error = e as Error & { status?: number };
569+
if (error.status === 422) {
570+
// Branch doesn't exist or already deleted - that's okay
571+
return ok(undefined);
572+
}
573+
return err(
574+
createError("COMMAND_FAILED", `Failed to delete branch: ${e}`),
575+
);
576+
}
577+
}
578+
579+
async waitForMergeable(
580+
prNumber: number,
581+
options?: { timeoutMs?: number; pollIntervalMs?: number },
582+
): Promise<Result<{ mergeable: boolean; reason?: string }>> {
583+
const repoResult = await this.getRepoInfo();
584+
if (!repoResult.ok) return repoResult;
585+
586+
const { owner, repo } = repoResult.value;
587+
const timeoutMs = options?.timeoutMs ?? 30000; // 30 second default timeout
588+
const pollIntervalMs = options?.pollIntervalMs ?? 2000; // 2 second poll interval
589+
const startTime = Date.now();
590+
591+
try {
592+
const octokit = await this.getOctokit();
593+
594+
while (Date.now() - startTime < timeoutMs) {
595+
const { data: pr } = await octokit.pulls.get({
596+
owner,
597+
repo,
598+
pull_number: prNumber,
599+
});
600+
601+
// mergeable can be true, false, or null (still calculating)
602+
if (pr.mergeable === true) {
603+
return ok({ mergeable: true });
604+
}
605+
606+
if (pr.mergeable === false) {
607+
return ok({
608+
mergeable: false,
609+
reason: pr.mergeable_state || "Has conflicts or other issues",
610+
});
611+
}
612+
613+
// mergeable is null - GitHub is still calculating, wait and retry
614+
await new Promise((resolve) => setTimeout(resolve, pollIntervalMs));
615+
}
616+
617+
return ok({
618+
mergeable: false,
619+
reason: "Timeout waiting for merge status",
620+
});
621+
} catch (e) {
622+
return err(
623+
createError("COMMAND_FAILED", `Failed to check mergeable status: ${e}`),
624+
);
625+
}
626+
}
515627
}
516628

517629
export function createGitHub(cwd: string): GitHub {

‎packages/core/src/stacks.ts‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,8 @@ export class StackManager {
156156

157157
let status: PRSubmitStatus;
158158

159-
if (existingPR) {
159+
// Only update PRs that are still open - merged/closed PRs can't be updated
160+
if (existingPR && existingPR.state === "open") {
160161
await this.github.updatePR(existingPR.number, {
161162
base: previousBookmark,
162163
});
@@ -176,8 +177,27 @@ export class StackManager {
176177
status,
177178
});
178179
} else {
180+
// If there's a merged/closed PR for this branch, we need a new unique branch name
181+
// GitHub doesn't allow creating a new PR with the same branch name as an existing PR
182+
let finalBookmark = bookmark;
183+
if (existingPR && existingPR.state !== "open") {
184+
// Find a unique suffix by incrementing until we find an unused name
185+
let suffix = 2;
186+
let candidateBookmark = `${bookmark}-${suffix}`;
187+
while (prCache.has(candidateBookmark)) {
188+
suffix++;
189+
candidateBookmark = `${bookmark}-${suffix}`;
190+
}
191+
finalBookmark = candidateBookmark;
192+
193+
// Delete the old bookmark and create the new one
194+
await this.jj.deleteBookmark(bookmark);
195+
await this.jj.ensureBookmark(finalBookmark, change.changeId);
196+
await this.jj.push({ bookmark: finalBookmark, allowNew: true });
197+
}
198+
179199
const prResult = await this.github.createPR({
180-
head: bookmark,
200+
head: finalBookmark,
181201
title: change.description || "Untitled",
182202
base: previousBookmark,
183203
draft: options?.draft,
@@ -189,14 +209,17 @@ export class StackManager {
189209
created++;
190210
prs.push({
191211
changeId: change.changeId,
192-
bookmarkName: bookmark,
212+
bookmarkName: finalBookmark,
193213
prNumber: prResult.value.number,
194214
prUrl: prResult.value.url,
195215
base: previousBookmark,
196216
position: i,
197217
status,
198218
});
199219
}
220+
// Use the final bookmark name (possibly with suffix) for the next PR's base
221+
previousBookmark = finalBookmark;
222+
continue;
200223
}
201224

202225
previousBookmark = bookmark;

0 commit comments

Comments
 (0)