From 637a1e4634936f1604d35e9c117758aff21d5364 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 14:45:55 -0800 Subject: [PATCH 1/3] fix(land): merge with workflow write when the prepared head brings a workflow change GitHub refuses an App's merge of a PR whose commits touch .github/workflows. The gate's own merge of a base that changed a workflow is such a commit, but the merge token asked for workflow write only when the squash itself changed one. moq-dev/quest#286 hit this, and the 403 failed the whole batch on every retry, stalling the queue. The merge token now also asks for workflow write when the prepared head changes a workflow since the pinned head. Any other 403 from the merge endpoint, except a rate limit, stops only that PR as merge_refused, so the rest of the queue keeps landing. Co-Authored-By: Claude Opus 5.5 --- README.md | 6 ++++-- src/cli/land-serve.ts | 25 +++++++++++++++++++----- test/land.test.ts | 45 ++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 66 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 4e467779..7b6d29ea 100644 --- a/README.md +++ b/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 the prepared head does since the pinned 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 diff --git a/src/cli/land-serve.ts b/src/cli/land-serve.ts index b7969684..ea4de196 100644 --- a/src/cli/land-serve.ts +++ b/src/cli/land-serve.ts @@ -89,9 +89,15 @@ 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 the head being + * merged changes one since the pinned head: GitHub refuses an App's merge of a PR whose commits touch + * workflows, such as the gate's own merge of a base that changed one. + */ +export function touchesWorkflows(root: string, parent: string, link: Chain["links"][number], head: string): boolean { + const changed = (from: string, to: string) => + git(root, ["diff", "--name-only", from, to, "--", ".github/workflows"]) !== ""; + return changed(parent, link.commit) || changed(link.entry.head, head); } /** A fresh bare repository with no config, objects, or alternates inherited from the user's checkout. */ @@ -338,9 +344,18 @@ 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], }); const result = object(await response.json()); + // A rate limit is transient, but any other 403 is GitHub refusing this PR: it stops this PR alone, + // rather than failing the whole batch on every retry. + if ( + response.status === 403 && + (response.headers.has("retry-after") || + response.headers.get("x-ratelimit-remaining") === "0" || + /rate limit/i.test(String(result.message))) + ) + throw new Error(`GitHub rate limited ${path}: ${String(result.message)}`); if (response.ok && result.merged === true) return { sha: string(result.sha) }; const pull = object(await (await github.request(path)).json()); // A lost response or a manual merge is reconciled by rebuilding the remaining queue. @@ -638,7 +653,7 @@ 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, 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); diff --git a/test/land.test.ts b/test/land.test.ts index ef4cde9b..c605ec68 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1786,10 +1786,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 +1803,51 @@ 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(); + 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("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" }); From 4d943a9408dc5ac69ac70a1de6a009f9ffd8fbdd Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 14:51:18 -0800 Subject: [PATCH 2/3] fix(github): wait out a rate limit even when the caller allows its status Co-Authored-By: Claude Opus 5.5 --- src/cli/github.ts | 26 ++++++++++++++++++-------- src/cli/land-serve.ts | 11 ++--------- test/github.test.ts | 10 ++++++++++ 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/cli/github.ts b/src/cli/github.ts index dfe18754..b2cce3d5 100644 --- a/src/cli/github.ts +++ b/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, diff --git a/src/cli/land-serve.ts b/src/cli/land-serve.ts index ea4de196..562c6437 100644 --- a/src/cli/land-serve.ts +++ b/src/cli/land-serve.ts @@ -346,16 +346,9 @@ async function mergePinned(github: GitHub, path: string, head: string, previous body: { merge_method: "squash", sha: head }, 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()); - // A rate limit is transient, but any other 403 is GitHub refusing this PR: it stops this PR alone, - // rather than failing the whole batch on every retry. - if ( - response.status === 403 && - (response.headers.has("retry-after") || - response.headers.get("x-ratelimit-remaining") === "0" || - /rate limit/i.test(String(result.message))) - ) - throw new Error(`GitHub rate limited ${path}: ${String(result.message)}`); if (response.ok && result.merged === true) return { sha: string(result.sha) }; const pull = object(await (await github.request(path)).json()); // A lost response or a manual merge is reconciled by rebuilding the remaining queue. diff --git a/test/github.test.ts b/test/github.test.ts index 11a1c9cd..0260baa8 100644 --- a/test/github.test.ts +++ b/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]); From bbf8ce96946d1f3a99c6f4f90e97a6af5d667aa0 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 14:54:48 -0800 Subject: [PATCH 3/3] fix(land): count workflow changes in any commit the PR brings, not only the gate's merge Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++-- src/cli/land-serve.ts | 25 +++++++++++++++++-------- test/land.test.ts | 43 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 7b6d29ea..53b88a54 100644 --- a/README.md +++ b/README.md @@ -673,8 +673,8 @@ 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. An API merge requests workflow write when that PR's squash changes `.github/workflows`, or -when the prepared head does since the pinned 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 +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 diff --git a/src/cli/land-serve.ts b/src/cli/land-serve.ts index 562c6437..13471f5e 100644 --- a/src/cli/land-serve.ts +++ b/src/cli/land-serve.ts @@ -90,14 +90,20 @@ function save(path: string, value: unknown): void { } /** - * The merge needs workflow write when this PR's squash changes a workflow, or when the head being - * merged changes one since the pinned head: GitHub refuses an App's merge of a PR whose commits touch - * workflows, such as the gate's own merge of a base that changed one. + * 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], head: string): boolean { - const changed = (from: string, to: string) => - git(root, ["diff", "--name-only", from, to, "--", ".github/workflows"]) !== ""; - return changed(parent, link.commit) || changed(link.entry.head, head); +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. */ @@ -646,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, prepared.head)) }); + : 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); diff --git a/test/land.test.ts b/test/land.test.ts index c605ec68..ded59004 100644 --- a/test/land.test.ts +++ b/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"); } @@ -1829,6 +1852,26 @@ mv '${result}.new' '${result}' ]); }); + 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(); + 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" });