fix(land): merge with workflow write when the prepared head brings a workflow change

README.md

@@ -671,8 +671,10 @@ fixer repositories remain credential-free. Write installation tokens are scoped
 to this repository, with contents and pull requests write.
 Tokens are minted for each push or API merge and passed only to trusted git
 and GitHub clients, never the sandbox or fixer. A PR-branch push also requests
-workflow write when the head-to-merge range carries workflow changes from the base. Workflow write is requested only when that PR's squash changes
-`.github/workflows`. Queue API tokens, which disable auto-merge on ejection and
+workflow write when the head-to-merge range carries workflow changes from the base. An API
+merge requests workflow write when that PR's squash changes `.github/workflows`, or
+when any commit the PR brings does (including a merge of a base that changed one),
+since GitHub refuses an App's merge of a PR whose commits touch workflows. Queue API tokens, which disable auto-merge on ejection and
 so need contents write, stay in the daemon, refresh before expiry, and are
 replaced on a 401. Batch errors are logged and retried with bounded exponential
 backoff. Grant the App the base ruleset's bypass and the read permissions needed

src/cli/github.ts

@@ -136,6 +136,17 @@ export function gitHubDependencies(warn: (message: string) => void): GitHubDepen
 	return { fetch, now: Date.now, sleep, token, warn };
 }
 
+/** Whether GitHub turned a request away for a rate limit, which any request may wait out and repeat. */
+function limited(response: Response, body: string): boolean {
+	return (
+		response.status === 429 ||
+		(response.status === 403 &&
+			(response.headers.has("retry-after") ||
+				response.headers.get("x-ratelimit-remaining") === "0" ||
+				/rate limit/i.test(body)))
+	);
+}
+
 /** Authenticated REST and GraphQL requests that wait out rate limits, bounded by an optional deadline. */
 export class GitHub {
 	/** The longest X-Poll-Interval GitHub asked for since the caller last set this, in milliseconds. */
@@ -200,7 +211,12 @@ export class GitHub {
 					(text) => ({ text }),
 					(error: unknown) => ({ error }),
 				);
-				if (response.ok || response.status === 304 || options.allow?.includes(response.status)) {
+				// A rate limit is waited out even when the caller allows its status, such as a 403 refusal.
+				if (
+					response.ok ||
+					response.status === 304 ||
+					(options.allow?.includes(response.status) && !limited(response, "text" in read ? read.text : ""))
+				) {
 					const { status, statusText, headers } = response;
 					if ("text" in read)
 						return new Response(NULL_BODY.includes(status) ? null : read.text, {
@@ -213,13 +229,7 @@ export class GitHub {
 				} else {
 					// A refusal's status still says what to do when its body was cut off.
 					const body = "text" in read ? read.text : "";
-					const limited =
-						response.status === 429 ||
-						(response.status === 403 &&
-							(response.headers.has("retry-after") ||
-								response.headers.get("x-ratelimit-remaining") === "0" ||
-								/rate limit/i.test(body)));
-					if (limited) {
+					if (limited(response, body)) {
 						// GitHub turned the request away unprocessed, so any request repeats.
 						await this.waitOut(
 							response.headers,

src/cli/land-serve.ts

@@ -89,9 +89,21 @@ function save(path: string, value: unknown): void {
 	renameSync(`${path}.new`, path);
 }
 
-/** Only this PR's squash changes need workflow write; no PR branch is moved. */
-export function touchesWorkflows(root: string, parent: string, link: Chain["links"][number]): boolean {
-	return git(root, ["diff", "--name-only", parent, link.commit, "--", ".github/workflows"]) !== "";
+/**
+ * The merge needs workflow write when this PR's squash changes a workflow, or when any commit the PR
+ * brings changes one: GitHub refuses an App's merge of a PR whose commits touch workflows, such as a
+ * merge of a base that changed one, whether the gate or the author made it.
+ */
+export function touchesWorkflows(
+	root: string,
+	parent: string,
+	link: Chain["links"][number],
+	base: string,
+	head: string,
+): boolean {
+	if (git(root, ["diff", "--name-only", parent, link.commit, "--", ".github/workflows"]) !== "") return true;
+	// `-m` also lists a merge commit's changes against each parent, so a merged-in workflow change counts.
+	return git(root, ["log", "-m", "--format=", "--name-only", `${base}..${head}`, "--", ".github/workflows"]) !== "";
 }
 
 /** A fresh bare repository with no config, objects, or alternates inherited from the user's checkout. */
@@ -338,8 +350,10 @@ async function mergePinned(github: GitHub, path: string, head: string, previous
 		const response = await github.request(`${path}/merge`, {
 			method: "PUT",
 			body: { merge_method: "squash", sha: head },
-			allow: [405, 409],
+			allow: [403, 405, 409],
 		});
+		// The client waits out a rate limit; any other 403 is GitHub refusing this PR, which stops this PR
+		// alone rather than failing the whole batch on every retry.
 		const result = object(await response.json());
 		if (response.ok && result.merged === true) return { sha: string(result.sha) };
 		const pull = object(await (await github.request(path)).json());
@@ -638,7 +652,10 @@ export async function serveBatch(
 		const merge =
 			"reason" in prepared
 				? new GitHub(deps)
-				: new GitHub({ ...deps, token: await token(touchesWorkflows(root, parent, link)) });
+				: new GitHub({
+						...deps,
+						token: await token(touchesWorkflows(root, parent, link, plan.base.sha, prepared.head)),
+					});
 		const result = "reason" in prepared ? prepared : await mergePinned(merge, path, prepared.head, link.entry.head);
 		if (result === null) {
 			forgetHeads(options.state, link.entry.number);

test/github.test.ts

@@ -176,6 +176,16 @@ describe("transient failures", () => {
 		expect(calls).toHaveLength(2);
 	});
 
+	test("a rate limit is waited out even when the caller allows its status", async () => {
+		const { github, calls } = fixture([
+			json({ message: "secondary rate limit" }, 403, { "retry-after": "60" }),
+			json({ message: "Resource not accessible by integration" }, 403),
+		]);
+		const response = await github.request(`${repo}/pulls/1/merge`, { method: "PUT", body: {}, allow: [403] });
+		expect(calls).toHaveLength(2);
+		expect(((await response.json()) as { message: string }).message).toBe("Resource not accessible by integration");
+	});
+
 	test("any other POST never repeats after a 5xx or a dropped connection", async () => {
 		for (const failure of [bad(502), dropped()]) {
 			const { github, calls, sleeps } = fixture([failure]);

test/land.test.ts

@@ -182,6 +182,29 @@ class Forge {
 		this.repo.branches.main = this.main();
 	}
 
+	/** The author merges the current base into a PR's branch, which moves its head and pin. */
+	mergeMain(number: number): string {
+		const pull = this.repo.pulls.find((pull) => pull.number === number)!;
+		git(this.work, "checkout", "--quiet", "-B", pull.branch, pull.head!);
+		git(
+			this.work,
+			"-c",
+			"user.name=Author",
+			"-c",
+			"user.email=author@example.test",
+			"merge",
+			"--quiet",
+			"--no-edit",
+			this.main(),
+		);
+		const head = git(this.work, "rev-parse", "HEAD");
+		git(this.work, "push", "--quiet", "--force", "origin", `${head}:refs/heads/${pull.branch}`);
+		git(this.work, "push", "--quiet", "--force", "origin", `${head}:refs/pull/${number}/head`);
+		pull.head = head;
+		pull.autoMerge = { ...pull.autoMerge!, commitBody: pin(head) };
+		return head;
+	}
+
 	main(): string {
 		return git(this.origin, "rev-parse", "refs/heads/main");
 	}
@@ -1786,10 +1809,10 @@ mv '${result}.new' '${result}'
 		expect(git(forge.origin, "rev-parse", `${forge.repo.pulls[3].squash}^`)).toBe(base);
 	});
 
-	test("scopes each merge token to that PR's squash, preserving it through the queue token cache", async () => {
+	test("scopes each merge token to the workflows its PR changes, preserving it through the queue token cache", async () => {
 		const forge = new Forge();
-		forge.pull("Workflow", { ".github/workflows/one.yml": "one" });
 		forge.pull("Code", { "code-two.txt": "two" });
+		forge.pull("Workflow", { ".github/workflows/one.yml": "one" });
 		forge.pull("Workflow again", { ".github/workflows/three.yml": "three" });
 		forge.pull("Waiting", { "four.txt": "four" });
 		const deps = await forge.deps();
@@ -1803,12 +1826,71 @@ mv '${result}.new' '${result}'
 			tokens.push(minted.token);
 			return minted.token;
 		});
-		expect(scopes).toEqual([true, true, false, true, true]);
+		expect(scopes).toEqual([false, false, true, true, true]);
 		expect(forge.merges.map(({ authorization }) => authorization)).toEqual(
 			[tokens[0], tokens[2], tokens[4]].map((token) => `Bearer ${token}`),
 		);
 	});
 
+	test("scopes the merge token to workflows when the gate's own base merge brings a workflow change", async () => {
+		const forge = new Forge();
+		forge.pull("Behind", { "code.txt": "one" });
+		// The base changes a workflow after the PR branched, so the gate merges it into the PR's head.
+		forge.advance({ ".github/workflows/ci.yml": "on: push\n" });
+		const deps = await forge.deps();
+		const plan = await resolveQueue(REPOSITORY, undefined, deps);
+		const tokens = new Map<string, boolean>();
+		const output = await serveBatch(forge.clone(), plan, sandbox(), deps, async (workflows) => {
+			const minted = await forge.deps(workflows ? { workflows: "write" } : {});
+			tokens.set(minted.token, workflows);
+			return minted.token;
+		});
+		expect(output).toEqual([`landed 1 ${forge.repo.pulls[0].squash}`]);
+		// The squash changes no workflow, but GitHub refuses an App's merge of a PR whose commits do.
+		expect(forge.merges.map(({ authorization }) => tokens.get(authorization!.replace("Bearer ", "")))).toEqual([
+			true,
+		]);
+	});
+
+	test("scopes the merge token to workflows when the author's own base merge brought a workflow change", async () => {
+		const forge = new Forge();
+		forge.pull("Merged main", { "code.txt": "one" });
+		forge.advance({ ".github/workflows/ci.yml": "on: push\n" });
+		// The head is up to date, so the gate merges nothing; the workflow change is in the PR's own commits.
+		forge.mergeMain(1);
+		const deps = await forge.deps();
+		const plan = await resolveQueue(REPOSITORY, undefined, deps);
+		const tokens = new Map<string, boolean>();
+		const output = await serveBatch(forge.clone(), plan, sandbox(), deps, async (workflows) => {
+			const minted = await forge.deps(workflows ? { workflows: "write" } : {});
+			tokens.set(minted.token, workflows);
+			return minted.token;
+		});
+		expect(output).toEqual([`landed 1 ${forge.repo.pulls[0].squash}`]);
+		expect(forge.merges.map(({ authorization }) => tokens.get(authorization!.replace("Bearer ", "")))).toEqual([
+			true,
+		]);
+	});
+
+	test("a merge GitHub refuses with a 403 stops only that PR, instead of failing every retry", async () => {
+		const forge = new Forge();
+		for (let i = 1; i <= 3; i++) forge.pull(`PR ${i}`, { [`${i}.txt`]: "yes" });
+		const deps = await forge.deps();
+		const plan = await resolveQueue(REPOSITORY, undefined, deps);
+		const original = deps.fetch;
+		deps.fetch = (async (input, init) => {
+			if (String(input).endsWith("/pulls/1/merge"))
+				return Response.json({ message: "Resource not accessible by integration" }, { status: 403 });
+			return original(input, init);
+		}) as typeof fetch;
+		const output = await serveBatch(forge.clone(), plan, sandbox(), deps, async () => deps.token);
+		expect(output).toEqual(["ejected 1 merge_refused"]);
+		expect(forge.merges).toEqual([]);
+		expect(forge.comment(1)).toContain("Resource not accessible by integration");
+		// PR 2, which passed the batch check with it, stays queued.
+		expect(forge.repo.pulls[1].autoMerge).not.toBeNull();
+	});
+
 	test("a lost merge response is confirmed from the PR, never merged again, and the batch goes on", async () => {
 		const forge = new Forge();
 		for (let i = 1; i <= 3; i++) forge.pull(`PR ${i}`, { [`${i}.txt`]: "yes" });