From 558a0f476b107bf125a17c04203c46c33321e341 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 3 Nov 2026 22:57:43 -0800 Subject: [PATCH 1/9] chore(quest): claim quest/m0/land-fixer/holds.md Quest-Claim: fc9dee9c610f484b From 1424d182fbe5444f16a2d09787862796f68b6674 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 3 Nov 2026 23:21:01 -0800 Subject: [PATCH 2/9] fix(land-watch): leave a PR another session holds to that session The watcher read no hold labels, so it fixed, pushed to, and re-enqueued a PR while a /quest-iterate session held it. It now checks for a hold when it handles the removal, before it starts the fixer, and again before it pushes a result or re-enqueues. A hold ends the pick, and the PR gets one comment. Co-Authored-By: Claude Opus 5.5 --- README.md | 12 +++++- quest/m0/land-fixer/README.md | 1 - quest/m0/land-fixer/holds.md | 47 --------------------- src/cli/land-watch-fix.ts | 68 +++++++++++++++++++++++++++--- src/cli/land-watch.ts | 31 ++++++++++---- test/land-watch.test.ts | 79 +++++++++++++++++++++++++++++++++++ 6 files changed, 175 insertions(+), 63 deletions(-) delete mode 100644 quest/m0/land-fixer/holds.md diff --git a/README.md b/README.md index b5a59f7c..b58c652d 100644 --- a/README.md +++ b/README.md @@ -817,8 +817,8 @@ head the watcher itself put back in the queue continues the watcher's pick instead, until a handback ends it: the watcher's re-enqueue, GitHub's auto-merge of it, or such a workflow picking the fixer's push. A failed build or a conflict is recorded for the land fixer in `removal-.json`, with the -full log beside it. A push since the removal, a person's removal, or a reason -the watcher does not know ends the pick, and the comment says the PR waits for +full log beside it. A push since the removal, a person's removal, a hold, or a +reason the watcher does not know ends the pick, and the comment says the PR waits for a maintainer to enqueue it again. The state directory remembers each removal handled, so a restart neither misses nor repeats one; a crash between commenting and recording finds its own comment's marker and posts nothing. The @@ -874,6 +874,14 @@ handback, from a stop rule, the fixer, a refusal, a crash, or a timeout, is posted with its reason and a recommendation, and the PR leaves the watcher's care until a maintainer enqueues it again. A round whose pick ended while it ran, by a writer's push or enqueue, is not applied, and its comment says so. + +The watcher leaves a PR another session holds to that session: one with a hold +label, such as the `quest:active` that `/quest-iterate` takes. It reads the +PR's labels when it handles the removal, before it starts the fixer, and again +before it pushes a result or re-enqueues the PR, so a hold taken while the +fixer runs counts too. A hold ends the pick, as a person's removal does: the +PR gets one comment saying so, and a maintainer enqueues it again once its +session is done. A round whose collection fails three passes in a row, as when GitHub refuses its handback comment, is set aside with a logged warning, freeing its fixer slot. diff --git a/quest/m0/land-fixer/README.md b/quest/m0/land-fixer/README.md index 05ee54f6..db056ea4 100644 --- a/quest/m0/land-fixer/README.md +++ b/quest/m0/land-fixer/README.md @@ -153,7 +153,6 @@ work, done once the Required quests below have merged. ## Required -- [Leave held PRs alone](/quest/m0/land-fixer/holds.md) - the watcher never fixes, pushes to, or re-enqueues a PR another session holds - [Run the repository's checks](/quest/m0/land-fixer/checks.md) - the fixer runs the repository's own lint, type, build, and test checks before it requeues - [Workflow permission](/quest/m0/land-fixer/workflows-permission.md) - waits on the maintainer to grant moq-bot `workflows`, so pushes touching workflows are accepted diff --git a/quest/m0/land-fixer/holds.md b/quest/m0/land-fixer/holds.md deleted file mode 100644 index 46e951c5..00000000 --- a/quest/m0/land-fixer/holds.md +++ /dev/null @@ -1,47 +0,0 @@ -# [S] The land fixer leaves a held PR alone - -## Goal - -`quest land watch` neither runs the fixer on a PR another session holds, nor -pushes a fixer result to it or re-enqueues it. A hold is any hold label -`isHoldLabel` accepts (`src/core/hold.ts`), such as the `quest:active` that -`/quest-iterate` takes, or an older pin's `quest:iterate`. The watcher -checks for a hold before it starts a job and again before it pushes or -re-enqueues. A held PR gets one comment saying the watcher left it to the -session that holds it. - -Non-goals: the frozen batching gate (`quest land serve`), which only -moq-dev/moq.pro still uses until it is -[retired](/quest/m0/queue-watcher/retire.md). - -## Plan - -Decided 2026-11-03 (maintainer, /quest-complete on moq-dev/moq, planned -through moq-dev/moq's coordinator): plan this follow-up. Milestone: m0, -under the land fixer epic. moq-dev/moq lands through this fixer every day, -and m0's Plan ranks the land work first. - -What happened on -[moq-dev/moq#6949](https://github.com/moq-dev/moq/pull/6949): the -maintainer added `quest:iterate` at 2026-11-01 16:41Z for a `/quest-iterate` -session. The fixer still pushed its merge `7c10f33cbc` at 16:52Z, and moq-bot -re-armed `quest:merge` at 16:56Z, while the session was pushing its own -fixes on the same branch. The hold was lifted at 17:22Z. That was the host -gate. `quest land watch`, which replaced the gate on moq-dev/moq, has the -same gap: `src/cli/land-watch.ts` and `src/cli/land-watch-fix.ts` never read -the PR's labels for holds. - -Implementation guidance: - -- Read the hold labels with the snapshot the watcher already takes for each - PR the queue drops, and again right before a result is pushed or the PR is - re-enqueued, so a hold taken while the fixer runs is respected too. -- Open question for the starter: what happens to the pick of a held PR? - Recommended: the hold ends it, as a person's removal from the queue does - (`endPick`). A maintainer enqueues the PR again once its session finishes, - as `/quest-iterate`'s `/quest-merge` hand-off already does. The other - option is to wait and fix it once the hold clears, but by then the session - has usually pushed, and a push by anyone but the fixer already ends the - pick. -- Add a regression test: a PR the queue drops while it holds `quest:active` - is neither fixed nor re-enqueued. diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index 4f2a07c2..8480f07d 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -2,7 +2,8 @@ // the background and re-enqueued at the back of the queue, or handed back to a person. import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { basename, join } from "node:path"; -import { GitHub, type GitHubDependencies, GitHubGraphQLError, object, type ObjectValue, string } from "./github"; +import { isHoldLabel } from "../core"; +import { array, GitHub, type GitHubDependencies, GitHubGraphQLError, object, type ObjectValue, string } from "./github"; import { type Queued, queueHead } from "../github/merge"; import { UNREFUSED } from "./hold"; import { baseRemote, type Entry, mergeBase, type RemoteRead } from "./land"; @@ -107,12 +108,30 @@ const MERGER = "quest land watch"; const PULL = `query($id: ID!) { node(id: $id) { ... on PullRequest { state baseRefName headRefOid mergeStateStatus mergeQueueEntry { enqueuedAt } autoMergeRequest { enabledAt } + labels(first: 100) { nodes { name } } } } }`; export async function readPull(github: GitHub, id: string): Promise { return object((await github.graphql(PULL, { id })).node); } +/** The hold label another session keeps on the PR, such as `/quest-iterate`'s `quest:active`, if any. */ +export const holdOf = (pull: ObjectValue): string | undefined => + array(object(pull.labels).nodes) + .map((label) => string(label.name)) + .find(isHoldLabel); + +/** Why the watcher leaves a held PR alone, as a comment's closing sentence. */ +export const heldText = (label: string) => + `It is held by ${fixerText(label)}, so the watcher leaves it to the session that holds it: the hold ends the pick, and a maintainer enqueues it again once that session is done.`; + +/** A hold found right before a push or a re-enqueue. */ +class Held extends Error { + constructor(readonly label: string) { + super(`held by ${label}`); + } +} + /** * Whether the PR is in the merge queue, or armed since `since` to join it once its required checks * pass. An older request is not a pick of the head the queue dropped. @@ -248,7 +267,7 @@ async function collectAll( } /** - * Take a removal waiting for the fixer, if its PR still waits as the queue dropped it: merge a + * Take a removal waiting for the fixer, if its PR still waits as the queue dropped it, unheld: merge a * conflict's base with the quest merge driver, or start the fixer when `slot` allows. Returns what it * did, if anything. */ @@ -290,6 +309,20 @@ async function take( mark(`${started[1].merged ? "merged" : "fixer"} ${started[0]}`); return undefined; } + const hold = holdOf(pull); + if (hold !== undefined) { + await post( + github, + { repository }, + entry, + `quest land watch: the land fixer did not start on ${removal.head}. ${heldText(hold)}`, + "held", + MERGER, + ); + endPick(state, number, entry.enabledAt); + mark("held"); + return `dropped ${number} held`; + } const conflict = removal.reason === "merge_conflict"; // A conflict is merged with the current base; a failed build is fixed on the base it was built on. const base = conflict ? current : failure.plan.base; @@ -407,7 +440,8 @@ function finish(path: string, job: WatchJob): void { /** * Collect a finished round: record it in its pick's history, and either put the PR back in the queue, * at a fixed head or unchanged, or hand it back, as a stop rule, the fixer, or a refusal says. A - * round whose pick ended while it ran is dropped, and the PR says so. Returns what it did. + * round whose pick ended while it ran, or whose PR another session holds, is dropped, and the PR says + * so. Returns what it did. */ async function collect( github: GitHub, @@ -492,6 +526,23 @@ async function collect( : queued(pull, job.dropped) && !ours && job.requeuing === undefined ? "a maintainer enqueued it again while the fixer ran, and that pick stands" : undefined; + // Leave a held PR to its session, unapplied: its own comment replaces the fixer's note. + const leave = async (hold: string): Promise => { + await post( + github, + plan, + marked, + `quest land watch: the land fixer's result for ${entry.head} was not applied. ${heldText(hold)}`, + "held", + MERGER, + ); + endPick(state, number, entry.enabledAt); + finish(path, job); + return [`dropped ${number} held`]; + }; + // A head this round already asked GitHub to queue, and GitHub took, stays: the hold came after it. + const hold = holdOf(pull); + if (hold !== undefined && !(job.requeuing !== undefined && queued(pull, job.dropped))) return leave(hold); if (ended) { await note(); await post( @@ -572,6 +623,9 @@ async function collect( job.published = head; save(path, job); } + // Validating the fix takes a while, and a session may have taken the PR since. + const hold = holdOf(await readPull(github, entry.id)); + if (hold !== undefined) throw new Held(hold); await publishHead(github, context.root, job, entry, entry.head, current, head, deps, context.token, { reason: result.reason, attribution, @@ -584,6 +638,7 @@ async function collect( job.requeued = await requeue(github, entry.id, head, job.base.ref, job.dropped); save(path, job); } catch (failure) { + if (failure instanceof Held) return [...said, ...(await leave(failure.label))]; if (!(failure instanceof FixerRefusal)) throw failure; handback = failure.message; if (failure.resolution) job.resolution = failure.resolution; @@ -626,7 +681,8 @@ async function collect( /** * Put `head` back in `base`'s merge queue, at the back. A PR queued or armed at it since the queue * dropped it, at `dropped`, needs nothing more: a restart finds its own request, and a workflow may pick - * the fixer's push first. A refusal, or a PR retargeted from `base`, hands the PR back. + * the fixer's push first. A refusal, or a PR retargeted from `base`, hands the PR back; a hold leaves + * it to the session that holds it. */ async function requeue(github: GitHub, id: string, head: string, base: string, dropped: string): Promise { const how = (pull: ObjectValue): Queued => (pull.mergeQueueEntry != null ? "enqueued" : "armed"); @@ -634,6 +690,8 @@ async function requeue(github: GitHub, id: string, head: string, base: string, d if (string(pull.baseRefName) !== base) throw new FixerRefusal(`retargeted from ${base} before it was re-enqueued`); if (string(pull.headRefOid) !== head) throw new FixerRefusal("head moved before it was re-enqueued"); if (!queued(pull, dropped)) { + const hold = holdOf(pull); + if (hold !== undefined) throw new Held(hold); try { await queueHead( (query, variables) => github.graphql(query, variables), @@ -741,7 +799,7 @@ async function openDisable( try { disable.done = await requeue(github, disable.id, disable.head, disable.base.ref, ""); } catch (error) { - if (!(error instanceof FixerRefusal)) throw error; + if (!(error instanceof FixerRefusal || error instanceof Held)) throw error; await post( github, { repository }, diff --git a/src/cli/land-watch.ts b/src/cli/land-watch.ts index b3334979..01461bb2 100644 --- a/src/cli/land-watch.ts +++ b/src/cli/land-watch.ts @@ -28,7 +28,16 @@ import { save, startDaemon, } from "./land-serve"; -import { advanceFixes, endPick, type FixContext, readPull, readRemoval, requeuedHeads } from "./land-watch-fix"; +import { + advanceFixes, + endPick, + type FixContext, + heldText, + holdOf, + readPull, + readRemoval, + requeuedHeads, +} from "./land-watch-fix"; import { enabledHead, headBefore } from "./status"; export { readRemoval } from "./land-watch-fix"; @@ -49,9 +58,10 @@ export interface WatchOptions { /** * What a removal means for its pick: the fixer's to take (`fix`), or the end of the pick, left alone * until a writer enqueues the PR again: a push since the queue dropped it (`pushed`), a person's - * removal (`removed`), or a reason the watcher does not know (`unrecognized`). + * removal (`removed`), a hold label another session keeps on it (`held`), or a reason the watcher + * does not know (`unrecognized`). */ -export type Verdict = "fix" | "pushed" | "removed" | "unrecognized" | "unknown"; +export type Verdict = "fix" | "pushed" | "removed" | "held" | "unrecognized" | "unknown"; /** A `merge_group` job that failed, or was still running when its checks timed out. */ export interface FailedJob { @@ -136,6 +146,7 @@ const OPEN = `query($owner: String!, $name: String!, $base: String!, $qualified: nodes { id number url title headRefOid headRefName maintainerCanModify mergeStateStatus headRepository { nameWithOwner } + labels(first: 100) { nodes { name } } mergeQueueEntry { id } autoMergeRequest { enabledAt commitBody } removals: timelineItems(since: $since, first: 20, itemTypes: [REMOVED_FROM_MERGE_QUEUE_EVENT]) { ${REMOVALS} } @@ -377,7 +388,7 @@ export function jobsSection(blocks: string[], room: number): string { } /** What the watcher does about it, if anything it needs to say; `fixing` when a land fixer is configured. */ -const AFTER: Record string> = { +const AFTER: Record string> = { fix: (_, fixing) => fixing ? "\n\nThe land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs." @@ -386,6 +397,7 @@ const AFTER: Record string> = { `\n\nIts head has moved to ${current} since, and a push by anyone but the land fixer ends the pick: the watcher leaves this pull request alone until a maintainer enqueues it again.`, removed: () => "\n\nA person's removal ends the pick: the watcher leaves this pull request alone until a maintainer enqueues it again.", + held: (_, __, hold) => `\n\n${heldText(hold)}`, unrecognized: () => "\n\nThe watcher does not recognize this reason, so it leaves this pull request alone until a maintainer enqueues it again.", unknown: () => @@ -431,6 +443,7 @@ async function handle( // An unknown head never authorizes the fixer: wait for GitHub to list the push, then end the pick. if (known === null && now - Date.parse(at) < UNLISTED) return null; const head = known ?? current; + const hold = holdOf(pull); const { pick, queued } = head === current ? first : await pickOf(github, string(pull.id), event, head, app, own); const verdict: Verdict = known === null @@ -439,9 +452,11 @@ async function handle( ? "pushed" : reason === "manual" ? "removed" - : FIXABLE.includes(reason) - ? "fix" - : "unrecognized"; + : !FIXABLE.includes(reason) + ? "unrecognized" + : hold !== undefined + ? "held" + : "fix"; const build = reason === "failed_checks" || reason === "checks_timed_out" ? await failedBuild( @@ -484,7 +499,7 @@ async function handle( github, { repository }, { number, enabledAt: pick.at }, - `quest land watch: the merge queue removed this pull request at ${head} because ${because(reason, actor, base.ref)} (\`${reason.replace(/[^a-z_]/g, "")}\`).${AFTER[verdict](current, fixing)}${missing}`, + `quest land watch: the merge queue removed this pull request at ${head} because ${because(reason, actor, base.ref)} (\`${reason.replace(/[^a-z_]/g, "")}\`).${AFTER[verdict](current, fixing, hold ?? "")}${missing}`, `removed ${string(event.id)}`, "quest land watch", undefined, diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index d36312bb..9aa57841 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -66,6 +66,7 @@ interface Pull { branch?: string; /** Its head branch's pushes, oldest first, in place of those its removals imply. */ pushes?: { sha: string; at: string }[]; + labels?: string[]; } interface Job { id: number; @@ -108,6 +109,8 @@ interface World { onActivity?: (pull: Pull) => void; /** GitHub's refusal of an enqueue of `pull`, if it refuses one. */ refuseEnqueue?: (pull: Pull) => string | undefined; + /** Called on each read of a PR's queue state, as a change GitHub makes while the watcher reads it. */ + onPull?: (pull: Pull) => void; } const at = (minutes: number) => new Date(Date.UTC(2026, 10, 1, 12, minutes)).toISOString().replace(".000", ""); @@ -122,6 +125,7 @@ function headOf(world: World, pull: Pull): string { } const branchOf = (pull: Pull) => pull.branch ?? `branch-${pull.number}`; +const labelsOf = (pull: Pull) => ({ nodes: (pull.labels ?? []).map((name) => ({ name })) }); function git(cwd: string, ...args: string[]): string { return execFileSync("git", args, { cwd, encoding: "utf8" }).trim(); @@ -226,6 +230,7 @@ function deps(world: World): GitHubDependencies { } if (query.includes("mergeStateStatus") && !query.includes("pullRequests(states: OPEN")) { const pull = pullOf(); + world.onPull?.(pull); if (world.unreadable?.(pull)) { world.unreadable = undefined; return Response.json({ errors: [{ type: "INTERNAL", message: "unavailable" }] }); @@ -239,6 +244,7 @@ function deps(world: World): GitHubDependencies { mergeStateStatus: pull.mergeState ?? "CLEAN", mergeQueueEntry: pull.queued ? { enqueuedAt: pull.queued } : null, autoMergeRequest: pull.armed ? { enabledAt: pull.armed } : null, + labels: labelsOf(pull), }, }, }); @@ -281,6 +287,7 @@ function deps(world: World): GitHubDependencies { mergeStateStatus: pull.mergeState ?? "CLEAN", mergeQueueEntry: pull.queued ? { id: `MQE_${pull.number}` } : null, autoMergeRequest: pull.armed ? { enabledAt: pull.armed, commitBody: "" } : null, + labels: labelsOf(pull), removals: removalPage(pull, variables.since, null), })), pageInfo: { hasNextPage: false, endCursor: null }, @@ -1683,4 +1690,76 @@ git commit -qm 'disable render_timeout'`; github.now = Date.parse(at(27)); expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([`requeued 12 ${head}`]); }); + test("a held PR is neither fixed nor re-enqueued, whenever the hold is taken", async () => { + const { repo, state, fix, context } = setup(); + const base = repo.main(); + const heads = new Map( + [5, 6, 7, 8].map((number) => [number, repo.pull(number, { [`f-${number}.txt`]: "new\n" })]), + ); + const removed = (number: number, labels?: string[]): Pull => ({ + number, + head: heads.get(number)!, + labels, + enqueues: [{ createdAt: at(1), actor: "maintainer" }], + removals: [ + { + id: `R${number}`, + createdAt: at(5), + reason: "failed_checks", + actor: "github-merge-queue", + before: heads.get(number), + }, + ], + }); + // 5 is held when the queue drops it; `quest:merge`, an older pin's leftover, holds nothing. + const pulls = [removed(5, ["quest:active"]), removed(6, ["quest:merge"]), removed(7), removed(8)]; + const github = { + ...world( + pulls, + pulls.map((pull) => ({ ...failedRun(pull.number, at(3), base), id: pull.number })), + ), + origin: repo.origin, + }; + github.now = Date.parse(at(6)); + fix.round(6, fixScript("fixed.txt")); + fix.round(7, flakyScript); + // Two fixer slots: 8 waits. + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + "removed 5 failed_checks held", + "removed 6 failed_checks fix", + "removed 7 failed_checks fix", + "removed 8 failed_checks fix", + "fixing 6", + "fixing 7", + ]); + await exited(state, 2); + // A session takes 6 while the watcher validates its fix, 7 while its fixer runs, and 8 while it waits. + let reads = 0; + github.onPull = (pull) => { + if (pull.number === 6 && ++reads === 2) pull.labels = ["quest:merge", "quest:active"]; + }; + pulls[2].labels = ["quest:iterate"]; + pulls[3].labels = ["quest:active"]; + github.now = Date.parse(at(7)); + expect((await watchPass(REPOSITORY, { state }, deps(github), context)).sort()).toEqual([ + "dropped 6 held", + "dropped 7 held", + "dropped 8 held", + ]); + expect(github.mutations).toEqual([]); + expect(git(repo.origin, "rev-parse", "refs/heads/branch-6")).toBe(heads.get(6)); + expect(fix.calls().map((call) => call.split(" ").at(-1))).toEqual(["6", "7"]); + for (const pull of pulls) { + const held = github.comments[pull.number].filter((comment) => + comment.body.includes("It is held by `quest:"), + ); + expect(held).toHaveLength(1); + expect(history(state, pull.number)).toBeUndefined(); + } + // The fixer's note on 7 is not posted: the held comment is the only one after the removal's. + expect(github.comments[7]).toHaveLength(2); + expect(readRemoval(state, 5)!.verdict).toBe("held"); + expect(readRemoval(state, 8)!.taken).toBe("held"); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); + }); }); From bd6deef7df29ac2425eca9f1f244ee1cad41e19f Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 08:25:46 -0800 Subject: [PATCH 3/9] fix(land-watch): leave a held PR's armed pick in place The watcher enqueues an armed pick GitHub left out of the queue, including its own re-enqueue that only armed auto-merge. That path never checked holds, so a session's hold taken while the checks ran did not stop the watcher from enqueuing the PR. It now skips a held PR, keeping the pick armed for the pass after the hold clears. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++-- src/cli/land-watch.ts | 6 ++++-- test/land-watch.test.ts | 15 ++++++++++++++- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index b58c652d..79eb79a4 100644 --- a/README.md +++ b/README.md @@ -834,8 +834,8 @@ enqueues only the head auto-merge was armed on, with `expectedHeadOid` bound to it: GitHub keeps auto-merge across a push, but a push by anyone but the land fixer ends the pick, and a head it cannot tell yet waits for the next pass. An armed PR whose required checks fail, or that conflicts, is left alone for -`/quest-iterate` and a new pick. The watcher's own re-enqueues are among these -picks. A refusal is logged once per pick and retried each pass. +`/quest-iterate` and a new pick. One another session holds waits, armed, until +its hold clears. The watcher's own re-enqueues are among these picks. A refusal is logged once per pick and retried each pass. With `--fixer`, the watcher takes each recorded removal, oldest first, while its PR still waits as the queue dropped it: diff --git a/src/cli/land-watch.ts b/src/cli/land-watch.ts index 01461bb2..ea887092 100644 --- a/src/cli/land-watch.ts +++ b/src/cli/land-watch.ts @@ -235,7 +235,7 @@ async function pickOf( * `expectedHeadOid` bound to it: GitHub keeps auto-merge across a writer's push, but that push ends the * pick, and a head `enabledHead` cannot tell yet waits for the next pass. The PR is read again just * before the enqueue, so a request disabled or a PR retargeted since the pass read it is not enqueued, - * which `expectedHeadOid` alone would not catch. An armed pick whose required + * which `expectedHeadOid` alone would not catch, and neither is a PR another session holds. An armed pick whose required * checks fail, or that conflicts, is not ready and is left alone. A refusal is reported once per pick * and retried each pass; one because GitHub queued the PR first says nothing. */ @@ -268,7 +268,9 @@ async function enqueueArmed( string(now.headRefOid) !== head || now.mergeQueueEntry != null || request === null || - string(request.enabledAt) !== armed + string(request.enabledAt) !== armed || + // Another session holds it: leave the pick armed, for the next pass once the hold clears. + holdOf(now) !== undefined ) continue; try { diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index 9aa57841..2138a292 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -764,12 +764,15 @@ test("a refused enqueue of an armed pick is reported once and retried, and says expect(JSON.parse(readFileSync(join(state, "watch.json"), "utf8")).refused).toEqual({}); }); -test("an armed pick withdrawn or retargeted while the pass reads it is not enqueued", async () => { +test("an armed pick withdrawn, retargeted, or held while the pass reads it is not enqueued", async () => { const state = tempDir(); const github = world([ { number: 32, head: HEAD, enqueues: [], removals: [], armed: at(2) }, { number: 33, head: HEAD, enqueues: [], removals: [], armed: at(2) }, { number: 34, head: HEAD, enqueues: [], removals: [], armed: at(2) }, + // Held when the pass reads it, or once it does. + { number: 35, head: HEAD, enqueues: [], removals: [], armed: at(2), labels: ["quest:active"] }, + { number: 36, head: HEAD, enqueues: [], removals: [], armed: at(2) }, ]); github.now = Date.parse(at(4)); github.onActivity = (pull) => { @@ -777,9 +780,19 @@ test("an armed pick withdrawn or retargeted while the pass reads it is not enque if (pull.number === 33) pull.base = "release"; // Disabled and armed again: a new pick, which the next pass weighs on its own. if (pull.number === 34) pull.armed = at(3); + if (pull.number === 36) pull.labels = ["quest:iterate"]; }; expect(await watchPass(REPOSITORY, { state }, deps(github))).toEqual([]); expect(github.mutations).toEqual([]); + + // The holds clear with the pick still armed at its head: the next pass enqueues it, with 34's new pick. + github.onActivity = undefined; + for (const pull of github.pulls.slice(3)) pull.labels = []; + expect(await watchPass(REPOSITORY, { state }, deps(github))).toEqual([ + `enqueued 34 ${HEAD}`, + `enqueued 35 ${HEAD}`, + `enqueued 36 ${HEAD}`, + ]); }); test("the watcher's tokens ask for writes only with a fixer, and pushes for workflows only when they carry one", () => { From 411c13ba2356f6bbff313916aaa468e98259c325 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 09:26:52 -0800 Subject: [PATCH 4/9] fix(land-watch): say a held PR's fix was pushed when it was A hold found by the re-enqueue, after the fixer's push, or on a restart whose head is already the fix, posted that the result "was not applied". The comment now names the pushed head, so the holding session knows its branch moved. Co-Authored-By: Claude Opus 5.5 --- README.md | 11 +++++----- src/cli/land-watch-fix.ts | 7 ++++-- test/land-watch.test.ts | 46 ++++++++++++++++++++++++++------------- 3 files changed, 42 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 8607bcd1..70d666a0 100644 --- a/README.md +++ b/README.md @@ -819,8 +819,8 @@ instead, until a handback ends it: the watcher's re-enqueue, GitHub's auto-merge of it, or such a workflow picking the fixer's push. A failed build or a conflict is recorded for the land fixer in `removal-.json`, with the full log beside it. A push since the removal, a person's removal, a hold, or a -reason the watcher does not know ends the pick, and the comment says the PR waits for -a maintainer to enqueue it again. The state directory remembers each removal +reason the watcher does not know ends the pick, and the comment says the PR +waits for a maintainer to enqueue it again. The state directory remembers each removal handled, so a restart neither misses nor repeats one; a crash between commenting and recording finds its own comment's marker and posts nothing. The first pass reads the ten minutes before it. @@ -836,7 +836,8 @@ it: GitHub keeps auto-merge across a push, but a push by anyone but the land fixer ends the pick, and a head it cannot tell yet waits for the next pass. An armed PR whose required checks fail, or that conflicts, is left alone for `/quest-iterate` and a new pick. One another session holds waits, armed, until -its hold clears. The watcher's own re-enqueues are among these picks. A refusal is logged once per pick and retried each pass. +its hold clears. The watcher's own re-enqueues are among these picks. A refusal +is logged once per pick and retried each pass. With `--fixer`, the watcher takes each recorded removal, oldest first, while its PR still waits as the queue dropped it: @@ -881,8 +882,8 @@ label, such as the `quest:active` that `/quest-iterate` takes. It reads the PR's labels when it handles the removal, before it starts the fixer, and again before it pushes a result or re-enqueues the PR, so a hold taken while the fixer runs counts too. A hold ends the pick, as a person's removal does: the -PR gets one comment saying so, and a maintainer enqueues it again once its -session is done. +PR gets one comment saying so, naming the fixer's head if it was already pushed, +and a maintainer enqueues it again once its session is done. A round whose collection fails three passes in a row, as when GitHub refuses its handback comment, is set aside with a logged warning, freeing its fixer slot. diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index 8480f07d..2891f759 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -526,13 +526,15 @@ async function collect( : queued(pull, job.dropped) && !ours && job.requeuing === undefined ? "a maintainer enqueued it again while the fixer ran, and that pick stands" : undefined; - // Leave a held PR to its session, unapplied: its own comment replaces the fixer's note. + // Whether the PR's head is this round's fix, so the holding session hears its branch moved. + let pushed = ours; + // Leave a held PR to its session: its own comment replaces the fixer's note. const leave = async (hold: string): Promise => { await post( github, plan, marked, - `quest land watch: the land fixer's result for ${entry.head} was not applied. ${heldText(hold)}`, + `quest land watch: the land fixer's result for ${entry.head} ${pushed ? `was pushed as ${head}, but not re-enqueued` : "was not applied"}. ${heldText(hold)}`, "held", MERGER, ); @@ -630,6 +632,7 @@ async function collect( reason: result.reason, attribution, }); + pushed = true; } if (job.requeuing === undefined) { job.requeuing = head; diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index 2138a292..b98030e8 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -1704,10 +1704,10 @@ git commit -qm 'disable render_timeout'`; expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([`requeued 12 ${head}`]); }); test("a held PR is neither fixed nor re-enqueued, whenever the hold is taken", async () => { - const { repo, state, fix, context } = setup(); + const { repo, state, fix, context } = setup(3); const base = repo.main(); const heads = new Map( - [5, 6, 7, 8].map((number) => [number, repo.pull(number, { [`f-${number}.txt`]: "new\n" })]), + [5, 6, 7, 8, 9].map((number) => [number, repo.pull(number, { [`f-${number}.txt`]: "new\n" })]), ); const removed = (number: number, labels?: string[]): Pull => ({ number, @@ -1725,7 +1725,7 @@ git commit -qm 'disable render_timeout'`; ], }); // 5 is held when the queue drops it; `quest:merge`, an older pin's leftover, holds nothing. - const pulls = [removed(5, ["quest:active"]), removed(6, ["quest:merge"]), removed(7), removed(8)]; + const pulls = [removed(5, ["quest:active"]), removed(6, ["quest:merge"]), removed(7), removed(8), removed(9)]; const github = { ...world( pulls, @@ -1735,33 +1735,46 @@ git commit -qm 'disable render_timeout'`; }; github.now = Date.parse(at(6)); fix.round(6, fixScript("fixed.txt")); - fix.round(7, flakyScript); - // Two fixer slots: 8 waits. + fix.round(7, fixScript("fixed.txt")); + fix.round(8, flakyScript); + // Three fixer slots: 9 waits. expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ "removed 5 failed_checks held", "removed 6 failed_checks fix", "removed 7 failed_checks fix", "removed 8 failed_checks fix", + "removed 9 failed_checks fix", "fixing 6", "fixing 7", + "fixing 8", + ]); + await exited(state, 3); + // A session takes 6 while the watcher validates its fix, 7 right after the watcher pushes its fix, + // 8 while its fixer runs, and 9 while it waits. + const reads = new Map(); + const takeAt = new Map([ + [6, 2], + [7, 3], ]); - await exited(state, 2); - // A session takes 6 while the watcher validates its fix, 7 while its fixer runs, and 8 while it waits. - let reads = 0; github.onPull = (pull) => { - if (pull.number === 6 && ++reads === 2) pull.labels = ["quest:merge", "quest:active"]; + const read = (reads.get(pull.number) ?? 0) + 1; + reads.set(pull.number, read); + if (takeAt.get(pull.number) === read) pull.labels = [...(pull.labels ?? []), "quest:active"]; }; - pulls[2].labels = ["quest:iterate"]; - pulls[3].labels = ["quest:active"]; + pulls[3].labels = ["quest:iterate"]; + pulls[4].labels = ["quest:active"]; github.now = Date.parse(at(7)); expect((await watchPass(REPOSITORY, { state }, deps(github), context)).sort()).toEqual([ "dropped 6 held", "dropped 7 held", "dropped 8 held", + "dropped 9 held", ]); expect(github.mutations).toEqual([]); expect(git(repo.origin, "rev-parse", "refs/heads/branch-6")).toBe(heads.get(6)); - expect(fix.calls().map((call) => call.split(" ").at(-1))).toEqual(["6", "7"]); + const pushed = git(repo.origin, "rev-parse", "refs/heads/branch-7"); + expect(pushed).not.toBe(heads.get(7)); + expect(fix.calls().map((call) => call.split(" ").at(-1))).toEqual(["6", "7", "8"]); for (const pull of pulls) { const held = github.comments[pull.number].filter((comment) => comment.body.includes("It is held by `quest:"), @@ -1769,10 +1782,13 @@ git commit -qm 'disable render_timeout'`; expect(held).toHaveLength(1); expect(history(state, pull.number)).toBeUndefined(); } - // The fixer's note on 7 is not posted: the held comment is the only one after the removal's. - expect(github.comments[7]).toHaveLength(2); + // Only 7's branch moved, and its session hears where to. + expect(github.comments[6].at(-1)!.body).toContain("was not applied"); + expect(github.comments[7].at(-1)!.body).toContain(`was pushed as ${pushed}, but not re-enqueued`); + // The fixer's note on 8 is not posted: the held comment is the only one after the removal's. + expect(github.comments[8]).toHaveLength(2); expect(readRemoval(state, 5)!.verdict).toBe("held"); - expect(readRemoval(state, 8)!.taken).toBe("held"); + expect(readRemoval(state, 9)!.taken).toBe("held"); expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); }); }); From 58823befb7305d152eb6a9fa1762cf130e380fb3 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 09:43:52 -0800 Subject: [PATCH 5/9] fix(land-watch): record a round's hold before commenting on it A held round whose comment GitHub refused stayed pending once it had asked to re-enqueue, so if the hold cleared before the next pass, that pass re-enqueued it without a maintainer's pick. The job now records the hold first, and every retry leaves the PR. Co-Authored-By: Claude Opus 5.5 --- src/cli/land-watch-fix.ts | 5 +++++ test/land-watch.test.ts | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index 2891f759..ba435a13 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -95,6 +95,8 @@ interface WatchJob extends Process { requeued?: Queued; /** Why the round ended in a handback, recorded before posting it, so a restart keeps the reason. */ handback?: string; + /** The hold that ended the round, recorded before posting it, so a retry never applies the round once it clears. */ + held?: string; /** A validated fix whose push GitHub refused, handed back for `quest land apply`. */ resolution?: Resolution; /** Passes that failed to collect it, so one that keeps failing is set aside rather than holding a fixer slot. */ @@ -530,6 +532,8 @@ async function collect( let pushed = ours; // Leave a held PR to its session: its own comment replaces the fixer's note. const leave = async (hold: string): Promise => { + job.held = hold; + save(path, job); await post( github, plan, @@ -543,6 +547,7 @@ async function collect( return [`dropped ${number} held`]; }; // A head this round already asked GitHub to queue, and GitHub took, stays: the hold came after it. + if (job.held !== undefined) return leave(job.held); const hold = holdOf(pull); if (hold !== undefined && !(job.requeuing !== undefined && queued(pull, job.dropped))) return leave(hold); if (ended) { diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index b98030e8..32bf2fe5 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -1763,13 +1763,17 @@ git commit -qm 'disable render_timeout'`; }; pulls[3].labels = ["quest:iterate"]; pulls[4].labels = ["quest:active"]; + // GitHub refuses 7's held comment, and its session lets go before the next pass: the hold still ends 7's round. + github.refuse = (body) => body.includes("but not re-enqueued"); github.now = Date.parse(at(7)); expect((await watchPass(REPOSITORY, { state }, deps(github), context)).sort()).toEqual([ "dropped 6 held", - "dropped 7 held", "dropped 8 held", "dropped 9 held", ]); + pulls[2].labels = []; + github.now = Date.parse(at(8)); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual(["dropped 7 held"]); expect(github.mutations).toEqual([]); expect(git(repo.origin, "rev-parse", "refs/heads/branch-6")).toBe(heads.get(6)); const pushed = git(repo.origin, "rev-parse", "refs/heads/branch-7"); From fa70c6b963a90d1cf39915a08f909214d511da06 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 09:54:30 -0800 Subject: [PATCH 6/9] fix(land-watch): keep a hold seen at the removal or the fixer's start A removal's or a waiting round's held comment that GitHub refused left nothing recorded, so if the hold cleared before the retry, the watcher fixed the PR as if it had never been held. The removal record, with its hold, is now saved before the removal's comment, and take records the hold on the removal before commenting; a retry reuses either. Co-Authored-By: Claude Opus 5.5 --- src/cli/land-watch-fix.ts | 4 +++- src/cli/land-watch.ts | 33 +++++++++++++++++++-------------- test/land-watch.test.ts | 29 +++++++++++++++++------------ 3 files changed, 39 insertions(+), 27 deletions(-) diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index ba435a13..14e17f75 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -311,8 +311,10 @@ async function take( mark(`${started[1].merged ? "merged" : "fixer"} ${started[0]}`); return undefined; } - const hold = holdOf(pull); + // A hold recorded before GitHub refused its comment still ends the pick once it clears. + const hold = removal.hold ?? holdOf(pull); if (hold !== undefined) { + save(removalPath(state, number), { ...removal, hold } satisfies Removal); await post( github, { repository }, diff --git a/src/cli/land-watch.ts b/src/cli/land-watch.ts index ea887092..180c3b50 100644 --- a/src/cli/land-watch.ts +++ b/src/cli/land-watch.ts @@ -103,6 +103,8 @@ export interface Removal { failure?: Failure; /** What the fixer step made of a `fix` verdict; unset while it waits for one. */ taken?: string; + /** The hold label that ends the pick, recorded before its comment, so a retry honors it once it clears. */ + hold?: string; /** For a conflict: the base the quest merge driver could not merge, retried only once the base moves. */ unmerged?: string; } @@ -445,7 +447,9 @@ async function handle( // An unknown head never authorizes the fixer: wait for GitHub to list the push, then end the pick. if (known === null && now - Date.parse(at) < UNLISTED) return null; const head = known ?? current; - const hold = holdOf(pull); + // A hold recorded for this removal, before GitHub refused its comment, still ends the pick. + const prior = readRemoval(state, number); + const hold = (prior?.event === string(event.id) ? prior.hold : undefined) ?? holdOf(pull); const { pick, queued } = head === current ? first : await pickOf(github, string(pull.id), event, head, app, own); const verdict: Verdict = known === null @@ -497,17 +501,6 @@ async function handle( build.jobs.length === 0 && reason !== "merge_conflict" && FIXABLE.includes(reason) ? `\n\nThe watcher found no failing \`merge_group\` job for it before ${at}.` : ""; - await post( - github, - { repository }, - { number, enabledAt: pick.at }, - `quest land watch: the merge queue removed this pull request at ${head} because ${because(reason, actor, base.ref)} (\`${reason.replace(/[^a-z_]/g, "")}\`).${AFTER[verdict](current, fixing, hold ?? "")}${missing}`, - `removed ${string(event.id)}`, - "quest land watch", - undefined, - (room) => jobsSection(blocks, room), - ); - const entry: Entry = { id: string(pull.id), number, @@ -533,6 +526,7 @@ async function handle( verdict, jobs: build.jobs.map(({ log: _, ...job }) => job), log, + ...(verdict === "held" ? { hold } : {}), ...(verdict === "fix" ? { failure: { @@ -547,12 +541,23 @@ async function handle( } : {}), }; - // A removal GitHub showed late never replaces the record of a newer one. - if ((readRemoval(state, number)?.at ?? "") <= at) { + // Recorded before its comment, so a pass retrying a refused comment keeps a hold that cleared since. A + // removal GitHub showed late never replaces the record of a newer one. + if ((prior?.at ?? "") <= at) { save(join(state, `removal-${number}.json`), removal); // Any other verdict ends the pick: its fixer history goes, so a later enqueue starts afresh. if (verdict !== "fix") endPick(state, number, pick.at); } + await post( + github, + { repository }, + { number, enabledAt: pick.at }, + `quest land watch: the merge queue removed this pull request at ${head} because ${because(reason, actor, base.ref)} (\`${reason.replace(/[^a-z_]/g, "")}\`).${AFTER[verdict](current, fixing, hold ?? "")}${missing}`, + `removed ${string(event.id)}`, + "quest land watch", + undefined, + (room) => jobsSection(blocks, room), + ); return `removed ${number} ${reason} ${verdict}`; } diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index 32bf2fe5..a3e5f838 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -96,8 +96,8 @@ interface World { now: number; /** The upstream repository that holds each PR's branch, `branch-`. */ origin?: string; - /** A comment GitHub refuses to post, once. */ - refuse?: (body: string) => boolean; + /** Texts whose comment GitHub refuses to post, each once. */ + refuse?: string[]; /** A read of a PR's queue state that fails, once. */ unreadable?: (pull: Pull) => boolean; /** PRs whose push activity GitHub has not listed yet. */ @@ -368,8 +368,9 @@ function deps(world: World): GitHubDependencies { const list = (world.comments[Number(comments[1])] ??= []); if (init?.method === "POST") { const comment = { user: { login: `${APP}[bot]` }, body: JSON.parse(String(init.body)).body }; - if (world.refuse?.(comment.body)) { - world.refuse = undefined; + const refused = world.refuse?.findIndex((text) => comment.body.includes(text)) ?? -1; + if (refused >= 0) { + world.refuse!.splice(refused, 1); return Response.json({ message: "refused" }, { status: 422 }); } list.push(comment); @@ -1109,7 +1110,7 @@ describe.skipIf(process.platform === "win32")("the watcher's land fixer", () => github.unreadable = (pull) => pull.queued !== undefined; expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); // Then GitHub refuses the outcome comment once: the round stays requeued, and only its comment is retried. - github.refuse = (body) => body.includes("re-enqueued this pull request"); + github.refuse = ["re-enqueued this pull request"]; expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); const [line] = await watchPass(REPOSITORY, { state }, deps(github), context); const fixed = git(repo.origin, "rev-parse", "refs/heads/branch-5"); @@ -1737,6 +1738,11 @@ git commit -qm 'disable render_timeout'`; fix.round(6, fixScript("fixed.txt")); fix.round(7, fixScript("fixed.txt")); fix.round(8, flakyScript); + // Each hold the watcher sees ends the pick, even when GitHub refuses its comment and the hold clears + // before the retry: here 5's at the removal, and below 7's after the push and 9's while it waits. + github.refuse = ["It is held by"]; + await expect(watchPass(REPOSITORY, { state }, deps(github), context)).rejects.toThrow(); + pulls[0].labels = []; // Three fixer slots: 9 waits. expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ "removed 5 failed_checks held", @@ -1763,17 +1769,16 @@ git commit -qm 'disable render_timeout'`; }; pulls[3].labels = ["quest:iterate"]; pulls[4].labels = ["quest:active"]; - // GitHub refuses 7's held comment, and its session lets go before the next pass: the hold still ends 7's round. - github.refuse = (body) => body.includes("but not re-enqueued"); + github.refuse = ["but not re-enqueued", "did not start on"]; github.now = Date.parse(at(7)); + await expect(watchPass(REPOSITORY, { state }, deps(github), context)).rejects.toThrow(); + pulls[2].labels = []; + pulls[4].labels = []; + github.now = Date.parse(at(8)); expect((await watchPass(REPOSITORY, { state }, deps(github), context)).sort()).toEqual([ - "dropped 6 held", - "dropped 8 held", + "dropped 7 held", "dropped 9 held", ]); - pulls[2].labels = []; - github.now = Date.parse(at(8)); - expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual(["dropped 7 held"]); expect(github.mutations).toEqual([]); expect(git(repo.origin, "rev-parse", "refs/heads/branch-6")).toBe(heads.get(6)); const pushed = git(repo.origin, "rev-parse", "refs/heads/branch-7"); From bb7474037ca92945c72c0aec9503b68bba90ff72 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 10:06:22 -0800 Subject: [PATCH 7/9] fix(land-watch): keep a flaky-test disable PR's hold when its comment fails A disable PR found held, or refused, at its enqueue posted before it recorded the outcome, so a refused comment left it to be enqueued once the hold cleared. The refusal is now recorded first, and a retry only posts the comment. Co-Authored-By: Claude Opus 5.5 --- src/cli/land-watch-fix.ts | 36 +++---- src/cli/land-watch-flaky.ts | 2 + test/land-watch.test.ts | 184 +++++++++++++++++++++--------------- 3 files changed, 128 insertions(+), 94 deletions(-) diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index 14e17f75..f623a539 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -806,22 +806,26 @@ async function openDisable( disable.id = string(pull.node_id); record(); } - try { - disable.done = await requeue(github, disable.id, disable.head, disable.base.ref, ""); - } catch (error) { - if (!(error instanceof FixerRefusal || error instanceof Held)) throw error; - await post( - github, - { repository }, - { number: disable.number!, enabledAt: disable.head }, - `quest land watch: ${fixerText(error.message)}, so this pull request waits for a maintainer to enqueue it.`, - "refused", - MERGER, - ); - disable.done = error.message; - record(); - return `disable refused ${name}: ${error.message}`; + if (disable.refused === undefined) { + try { + disable.done = await requeue(github, disable.id, disable.head, disable.base.ref, ""); + record(); + return `disabled ${name} ${disable.number}`; + } catch (error) { + if (!(error instanceof FixerRefusal || error instanceof Held)) throw error; + disable.refused = error.message; + record(); + } } + await post( + github, + { repository }, + { number: disable.number!, enabledAt: disable.head }, + `quest land watch: ${fixerText(disable.refused)}, so this pull request waits for a maintainer to enqueue it.`, + "refused", + MERGER, + ); + disable.done = disable.refused; record(); - return `disabled ${name} ${disable.number}`; + return `disable refused ${name}: ${disable.refused}`; } diff --git a/src/cli/land-watch-flaky.ts b/src/cli/land-watch-flaky.ts index 01de9b31..203164aa 100644 --- a/src/cli/land-watch-flaky.ts +++ b/src/cli/land-watch-flaky.ts @@ -31,6 +31,8 @@ export interface Disable { /** Its PR, once opened. */ number?: number; id?: string; + /** Why it could not be queued, such as a hold, recorded before its comment, so a retry only comments. */ + refused?: string; /** How it ended: queued (`enqueued` or `armed`), or why it could not be. */ done?: string; } diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index a3e5f838..846ba118 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -1371,17 +1371,19 @@ printf '[{"number":%s,"action":"fixed","head":"%s","reason":"preserve both confl await exited(state); }); - test("a test seen flaking twice is disabled in its own pull request, which the watcher enqueues", async () => { - const { repo, state, fix, context } = setup(); - const base = repo.main(); - const heads = [5, 6, 7].map((number) => repo.pull(number, { [`feature-${number}.txt`]: "new\n" })); - const github = { ...world([], []), origin: repo.origin }; - /** A flaky requeue naming render_timeout, disabling it at the checkout's HEAD after `script`. */ - const flaky = (script: string) => `${script} + test.each([false, true])( + "a test seen flaking twice is disabled in its own pull request, which the watcher enqueues unless it is held (held: %s)", + async (held) => { + const { repo, state, fix, context } = setup(); + const base = repo.main(); + const heads = [5, 6, 7].map((number) => repo.pull(number, { [`feature-${number}.txt`]: "new\n" })); + const github = { ...world([], []), origin: repo.origin }; + /** A flaky requeue naming render_timeout, disabling it at the checkout's HEAD after `script`. */ + const flaky = (script: string) => `${script} printf '[{"number":%s,"action":"requeue","reason":"healthy; flaky test","failure":"just test: render_timeout","disable":"%s"}]' "$2" "$(git rev-parse HEAD)" > quest-land-result.json `; - /** A disabling commit on base that skips the test, adds its follow-up quest, and writes `extra`. */ - const disabling = (extra: string) => `grep -q 'just test: render_timeout' .git/quest-land-flaky.json + /** A disabling commit on base that skips the test, adds its follow-up quest, and writes `extra`. */ + const disabling = (extra: string) => `grep -q 'just test: render_timeout' .git/quest-land-flaky.json git checkout -q -b disable base mkdir -p quest/m0/flaky printf '# [S] Fix or delete render_timeout\\n\\n## Goal\\n\\nFix the flaky test or delete it.\\n' > quest/m0/flaky/render-timeout.md @@ -1390,78 +1392,104 @@ printf 'skip render_timeout\\n' > skip.txt ${extra} git add quest skip.txt ${extra ? "flake.nix" : ""} git commit -qm 'disable render_timeout'`; - // The first verdict's disable is ignored: one judgment alone never disables a test. The second's edits a - // protected path and is refused, and the third's is opened as its own pull request. - fix.round(5, flaky("[ ! -e .git/quest-land-flaky.json ]")); - fix.round(6, flaky(disabling("printf 'changed\\n' > flake.nix"))); - fix.round(7, flaky(disabling(""))); - const said: string[][] = []; - for (const [index, number] of [5, 6, 7].entries()) { - const minute = (index + 1) * 10; - github.pulls.push({ - number, - head: heads[index], - enqueues: [{ createdAt: at(minute - 5), actor: "maintainer" }], - removals: [ - { - id: `R${number}`, - createdAt: at(minute), - reason: "failed_checks", - actor: "github-merge-queue", - before: heads[index], + // The first verdict's disable is ignored: one judgment alone never disables a test. The second's edits a + // protected path and is refused, and the third's is opened as its own pull request. + fix.round(5, flaky("[ ! -e .git/quest-land-flaky.json ]")); + fix.round(6, flaky(disabling("printf 'changed\\n' > flake.nix"))); + fix.round(7, flaky(disabling(""))); + const said: string[][] = []; + for (const [index, number] of [5, 6, 7].entries()) { + const minute = (index + 1) * 10; + github.pulls.push({ + number, + head: heads[index], + enqueues: [{ createdAt: at(minute - 5), actor: "maintainer" }], + removals: [ + { + id: `R${number}`, + createdAt: at(minute), + reason: "failed_checks", + actor: "github-merge-queue", + before: heads[index], + }, + ], + }); + github.runs.push({ ...failedRun(number, at(minute - 2), base), id: number * 10 }); + github.now = Date.parse(at(minute + 1)); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + `removed ${number} failed_checks fix`, + `fixing ${number}`, + ]); + await exited(state); + github.now = Date.parse(at(minute + 2)); + // A session takes the disable's pull request as soon as it opens, and GitHub refuses its comment. + if (held && number === 7) { + github.onPull = (pull) => { + if (pull.number === 8) pull.labels = ["quest:active"]; + }; + github.refuse = ["waits for a maintainer"]; + } + said.push(await watchPass(REPOSITORY, { state }, deps(github), context)); + } + const branch = github.pulls.find((pull) => pull.number === 8)?.branch; + expect(branch).toMatch(/^quest-land\/flaky-[0-9a-f]{12}$/); + const head = git(repo.origin, "rev-parse", `refs/heads/${branch}`); + expect(said).toEqual([ + ["disable ignored 5: first flaky verdict", `requeued 5 ${heads[0]}`], + [expect.stringMatching(/^disable refused 6: Error: off-limits fixer edit/), `requeued 6 ${heads[1]}`], + [`disable 7 ${head}`, `requeued 7 ${heads[2]}`, ...(held ? [] : ["disabled render-timeout 8"])], + ]); + if (held) { + // The hold clears before the retry, which still leaves the pull request for a maintainer. + github.onPull = undefined; + github.pulls.find((pull) => pull.number === 8)!.labels = []; + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + "disable refused render-timeout: held by quest:active", + ]); + expect(github.comments[8].map((comment) => comment.body)).toEqual([ + expect.stringContaining("`held by quest:active`, so this pull request waits for a maintainer"), + ]); + } + // The disable is the fixer's commit on the base, enqueued like any other pull request with no pick. + expect(git(repo.origin, "rev-parse", `${head}^`)).toBe(base); + expect(git(repo.origin, "show", `${head}:quest/m0/flaky/render-timeout.md`)).toContain( + "Fix the flaky test", + ); + expect(github.mutations).toEqual([ + `enqueuePullRequest 5 ${heads[0]}`, + `enqueuePullRequest 6 ${heads[1]}`, + `enqueuePullRequest 7 ${heads[2]}`, + ...(held ? [] : [`enqueuePullRequest 8 ${head}`]), + ]); + const opened = github.pulls.find((pull) => pull.number === 8)!; + expect(opened.title).toBe("quest land: disable the flaky test render-timeout"); + expect(opened.body).toContain( + "The land fixer judged `just test: render_timeout` flaky 3 times, most recently in", + ); + expect(opened.body).toContain( + "#5 ([job 1](https://github.com/upstream/project/actions/runs/50/job/71)), #6 ([job 1](https://github.com/upstream/project/actions/runs/60/job/71))", + ); + expect(opened.body).toContain("adds its follow-up quest, `quest/m0/flaky/render-timeout.md`"); + expect(opened.body).toContain(`## Review\n\nmechanical fix by Codex: \``); + expect(opened.body).toContain(`(head ${head})`); + expect(readFlaky(state)).toMatchObject([ + { + failure: "just test: render_timeout", + verdicts: [{ number: 5 }, { number: 6 }, { number: 7 }], + disable: { + head, + quest: "quest/m0/flaky/render-timeout.md", + number: 8, + done: held ? "held by quest:active" : "enqueued", }, - ], - }); - github.runs.push({ ...failedRun(number, at(minute - 2), base), id: number * 10 }); - github.now = Date.parse(at(minute + 1)); - expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ - `removed ${number} failed_checks fix`, - `fixing ${number}`, + }, ]); - await exited(state); - github.now = Date.parse(at(minute + 2)); - said.push(await watchPass(REPOSITORY, { state }, deps(github), context)); - } - const branch = github.pulls.find((pull) => pull.number === 8)?.branch; - expect(branch).toMatch(/^quest-land\/flaky-[0-9a-f]{12}$/); - const head = git(repo.origin, "rev-parse", `refs/heads/${branch}`); - expect(said).toEqual([ - ["disable ignored 5: first flaky verdict", `requeued 5 ${heads[0]}`], - [expect.stringMatching(/^disable refused 6: Error: off-limits fixer edit/), `requeued 6 ${heads[1]}`], - [`disable 7 ${head}`, `requeued 7 ${heads[2]}`, "disabled render-timeout 8"], - ]); - // The disable is the fixer's commit on the base, enqueued like any other pull request with no pick. - expect(git(repo.origin, "rev-parse", `${head}^`)).toBe(base); - expect(git(repo.origin, "show", `${head}:quest/m0/flaky/render-timeout.md`)).toContain("Fix the flaky test"); - expect(github.mutations).toEqual([ - `enqueuePullRequest 5 ${heads[0]}`, - `enqueuePullRequest 6 ${heads[1]}`, - `enqueuePullRequest 7 ${heads[2]}`, - `enqueuePullRequest 8 ${head}`, - ]); - const opened = github.pulls.find((pull) => pull.number === 8)!; - expect(opened.title).toBe("quest land: disable the flaky test render-timeout"); - expect(opened.body).toContain( - "The land fixer judged `just test: render_timeout` flaky 3 times, most recently in", - ); - expect(opened.body).toContain( - "#5 ([job 1](https://github.com/upstream/project/actions/runs/50/job/71)), #6 ([job 1](https://github.com/upstream/project/actions/runs/60/job/71))", - ); - expect(opened.body).toContain("adds its follow-up quest, `quest/m0/flaky/render-timeout.md`"); - expect(opened.body).toContain(`## Review\n\nmechanical fix by Codex: \``); - expect(opened.body).toContain(`(head ${head})`); - expect(readFlaky(state)).toMatchObject([ - { - failure: "just test: render_timeout", - verdicts: [{ number: 5 }, { number: 6 }, { number: 7 }], - disable: { head, quest: "quest/m0/flaky/render-timeout.md", number: 8, done: "enqueued" }, - }, - ]); - // A restart neither opens nor enqueues it again, and the disabled test is no longer offered as known. - expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); - expect(github.mutations).toHaveLength(4); - }); + // A restart neither opens nor enqueues it again, and the disabled test is no longer offered as known. + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); + expect(github.mutations).toHaveLength(held ? 3 : 4); + }, + ); test("a pick that ends while the fixer runs is left alone, and every handback ends the pick", async () => { const { repo, state, fix, context } = setup(5); From 1ec89e3c186d7b97ad459b8e286d9549178d76d6 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 11:25:15 -0800 Subject: [PATCH 8/9] fix(land-watch): end a held pick when the hold is recorded, and keep a taken requeue A held round whose comment GitHub kept refusing was set aside without ending its pick, so a later App enqueue continued it. The pick now ends when the hold is recorded, before the comment, in collect and take. After a crash that lost a round's requeue result, a hold taken since tripped the pre-push check even though the head was already pushed and queued. That check now runs only when a push is still needed; requeue still checks the hold before any enqueue. Co-Authored-By: Claude Opus 5.5 --- src/cli/land-watch-fix.ts | 15 +++++++++------ test/land-watch.test.ts | 6 ++++++ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index c5521e15..a0dc2952 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -321,6 +321,7 @@ async function take( const hold = removal.hold ?? holdOf(pull); if (hold !== undefined) { save(removalPath(state, number), { ...removal, hold } satisfies Removal); + endPick(state, number, entry.enabledAt); await post( github, { repository }, @@ -329,7 +330,6 @@ async function take( "held", MERGER, ); - endPick(state, number, entry.enabledAt); mark("held"); return `dropped ${number} held`; } @@ -542,6 +542,7 @@ async function collect( const leave = async (hold: string): Promise => { job.held = hold; save(path, job); + endPick(state, number, entry.enabledAt); await post( github, plan, @@ -550,12 +551,11 @@ async function collect( "held", MERGER, ); - endPick(state, number, entry.enabledAt); finish(path, job); return [`dropped ${number} held`]; }; - // A head this round already asked GitHub to queue, and GitHub took, stays: the hold came after it. if (job.held !== undefined) return leave(job.held); + // A head this round already asked GitHub to queue, and GitHub took, stays: the hold came after it. const hold = holdOf(pull); if (hold !== undefined && !(job.requeuing !== undefined && queued(pull, job.dropped))) return leave(hold); if (ended) { @@ -639,9 +639,12 @@ async function collect( job.published = head; save(path, job); } - // Validating the fix takes a while, and a session may have taken the PR since. - const hold = holdOf(await readPull(github, entry.id)); - if (hold !== undefined) throw new Held(hold); + // Validating the fix takes a while, and a session may have taken the PR since. A head already + // pushed needs no push, and `requeue` checks the hold before any enqueue. + if (!pushed) { + const hold = holdOf(await readPull(github, entry.id)); + if (hold !== undefined) throw new Held(hold); + } await publishHead(github, context.root, job, entry, entry.head, current, head, deps, context.token, { reason: result.reason, attribution, diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index b4b9832e..7eedb51d 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -1112,12 +1112,15 @@ describe.skipIf(process.platform === "win32")("the watcher's land fixer", () => // retried rather than handed back, and finds its own enqueue. github.unreadable = (pull) => pull.queued !== undefined; expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); + // A session takes it now: the head GitHub already took back stays queued. + pull.labels = ["quest:active"]; // Then GitHub refuses the outcome comment once: the round stays requeued, and only its comment is retried. github.refuse = ["re-enqueued this pull request"]; expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); const [line] = await watchPass(REPOSITORY, { state }, deps(github), context); const fixed = git(repo.origin, "rev-parse", "refs/heads/branch-5"); expect(line).toBe(`requeued 5 ${fixed}`); + pull.labels = undefined; expect(git(repo.origin, "rev-parse", `${fixed}^`)).toBe(head); expect(github.mutations).toEqual([`enqueuePullRequest 5 ${fixed}`]); expect(pull.body).toContain(`mechanical fix by Codex: \`add the missing file\` (head ${fixed})`); @@ -1866,6 +1869,9 @@ git commit -qm 'disable render_timeout'`; github.refuse = ["but not re-enqueued", "did not start on"]; github.now = Date.parse(at(7)); await expect(watchPass(REPOSITORY, { state }, deps(github), context)).rejects.toThrow(); + // The hold ended 7's pick when it was recorded, so even a round set aside for its refused comment + // leaves no pick for a later App enqueue to continue. + expect(history(state, 7)).toBeUndefined(); pulls[2].labels = []; pulls[4].labels = []; github.now = Date.parse(at(8)); From 2ccd142c4dff5548cf362690a04150affb054422 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 4 Nov 2026 12:39:42 -0800 Subject: [PATCH 9/9] fix(land-watch): end a held pick in state first, and comment on it best-effort Per the design checkpoint settled on 2026-11-04: every step that sees a hold ends the pick in its state first and stops acting on the PR, then posts one comment best-effort, with one wording whether or not a fix was pushed. Drop job.held, Disable.refused, Removal.hold, and the comment retry paths. A removal recorded held still ends the pick if a crash makes the pass handle it again. Co-Authored-By: Claude Opus 5.5 --- README.md | 5 +- src/cli/land-watch-fix.ts | 139 ++++++++++++++++++------------------ src/cli/land-watch-flaky.ts | 2 - src/cli/land-watch.ts | 33 +++++---- test/land-watch.test.ts | 87 +++++++++++----------- 5 files changed, 134 insertions(+), 132 deletions(-) diff --git a/README.md b/README.md index 5347591a..cd3942c0 100644 --- a/README.md +++ b/README.md @@ -910,8 +910,9 @@ label, such as the `quest:active` that `/quest-iterate` takes. It reads the PR's labels when it handles the removal, before it starts the fixer, and again before it pushes a result or re-enqueues the PR, so a hold taken while the fixer runs counts too. A hold ends the pick, as a person's removal does: the -PR gets one comment saying so, naming the fixer's head if it was already pushed, -and a maintainer enqueues it again once its session is done. +watcher records that first and stops acting on the PR, then posts one comment +saying so, best-effort and never retried. A maintainer enqueues it again once +its session is done. A held flaky-test disable PR is left for a maintainer too. A round whose collection fails three passes in a row, as when GitHub refuses its handback comment, is set aside with a logged warning, freeing its fixer slot. diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index a0dc2952..744ef2cb 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -95,8 +95,6 @@ interface WatchJob extends Process { requeued?: Queued; /** Why the round ended in a handback, recorded before posting it, so a restart keeps the reason. */ handback?: string; - /** The hold that ended the round, recorded before posting it, so a retry never applies the round once it clears. */ - held?: string; /** A validated fix whose push GitHub refused, handed back for `quest land apply`. */ resolution?: Resolution; /** Passes that failed to collect it, so one that keeps failing is set aside rather than holding a fixer slot. */ @@ -123,20 +121,35 @@ export async function readPull(github: GitHub, id: string): Promise return object((await github.graphql(PULL, { id })).node); } -/** The hold label another session keeps on the PR, such as `/quest-iterate`'s `quest:active`, if any. */ -export const holdOf = (pull: ObjectValue): string | undefined => - array(object(pull.labels).nodes) - .map((label) => string(label.name)) - .find(isHoldLabel); +/** Whether another session holds the PR with a hold label, such as `/quest-iterate`'s `quest:active`. */ +export const isHeld = (pull: ObjectValue): boolean => + array(object(pull.labels).nodes).some((label) => isHoldLabel(string(label.name))); -/** Why the watcher leaves a held PR alone, as a comment's closing sentence. */ -export const heldText = (label: string) => - `It is held by ${fixerText(label)}, so the watcher leaves it to the session that holds it: the hold ends the pick, and a maintainer enqueues it again once that session is done.`; +/** What the watcher says on a held PR, whatever step found the hold and whether a fix was pushed. */ +export const HELD = + "Another session holds this pull request, so the watcher stops acting on it and leaves it to that session: the hold ends the pick, and a maintainer enqueues it again once that session is done."; + +/** + * Say once, best-effort, that the watcher left a held PR to its session. The caller has already ended + * the pick in its state, so a comment GitHub refuses is only warned about and never retried. + */ +async function sayHeld( + github: GitHub, + repository: string, + entry: Pick, + warn: (text: string) => void, +): Promise { + try { + await post(github, { repository }, entry, `quest land watch: ${HELD}`, "held", MERGER); + } catch (error) { + warn(`quest land watch: hold comment on ${entry.number} not posted: ${String(error)}`); + } +} /** A hold found right before a push or a re-enqueue. */ class Held extends Error { - constructor(readonly label: string) { - super(`held by ${label}`); + constructor() { + super("held by another session"); } } @@ -317,20 +330,10 @@ async function take( mark(`${started[1].merged ? "merged" : "fixer"} ${started[0]}`); return undefined; } - // A hold recorded before GitHub refused its comment still ends the pick once it clears. - const hold = removal.hold ?? holdOf(pull); - if (hold !== undefined) { - save(removalPath(state, number), { ...removal, hold } satisfies Removal); - endPick(state, number, entry.enabledAt); - await post( - github, - { repository }, - entry, - `quest land watch: the land fixer did not start on ${removal.head}. ${heldText(hold)}`, - "held", - MERGER, - ); + if (isHeld(pull)) { mark("held"); + endPick(state, number, entry.enabledAt); + await sayHeld(github, repository, entry, deps.warn); return `dropped ${number} held`; } const conflict = removal.reason === "merge_conflict"; @@ -536,28 +539,16 @@ async function collect( : queued(pull, job.dropped) && !ours && job.requeuing === undefined ? "a maintainer enqueued it again while the fixer ran, and that pick stands" : undefined; - // Whether the PR's head is this round's fix, so the holding session hears its branch moved. - let pushed = ours; - // Leave a held PR to its session: its own comment replaces the fixer's note. - const leave = async (hold: string): Promise => { - job.held = hold; - save(path, job); + // Leave a held PR to its session: the pick and the round end first, and the hold's comment replaces + // the fixer's note. + const leave = async (): Promise => { endPick(state, number, entry.enabledAt); - await post( - github, - plan, - marked, - `quest land watch: the land fixer's result for ${entry.head} ${pushed ? `was pushed as ${head}, but not re-enqueued` : "was not applied"}. ${heldText(hold)}`, - "held", - MERGER, - ); finish(path, job); + await sayHeld(github, job.repository, marked, deps.warn); return [`dropped ${number} held`]; }; - if (job.held !== undefined) return leave(job.held); // A head this round already asked GitHub to queue, and GitHub took, stays: the hold came after it. - const hold = holdOf(pull); - if (hold !== undefined && !(job.requeuing !== undefined && queued(pull, job.dropped))) return leave(hold); + if (isHeld(pull) && !(job.requeuing !== undefined && queued(pull, job.dropped))) return leave(); if (ended) { await note(); await post( @@ -641,15 +632,11 @@ async function collect( } // Validating the fix takes a while, and a session may have taken the PR since. A head already // pushed needs no push, and `requeue` checks the hold before any enqueue. - if (!pushed) { - const hold = holdOf(await readPull(github, entry.id)); - if (hold !== undefined) throw new Held(hold); - } + if (!ours && isHeld(await readPull(github, entry.id))) throw new Held(); await publishHead(github, context.root, job, entry, entry.head, current, head, deps, context.token, { reason: result.reason, attribution, }); - pushed = true; } if (job.requeuing === undefined) { job.requeuing = head; @@ -658,7 +645,7 @@ async function collect( job.requeued = await requeue(github, entry.id, head, job.base.ref, job.dropped); save(path, job); } catch (failure) { - if (failure instanceof Held) return [...said, ...(await leave(failure.label))]; + if (failure instanceof Held) return [...said, ...(await leave())]; if (!(failure instanceof FixerRefusal)) throw failure; handback = failure.message; if (failure.resolution) job.resolution = failure.resolution; @@ -710,8 +697,7 @@ async function requeue(github: GitHub, id: string, head: string, base: string, d if (string(pull.baseRefName) !== base) throw new FixerRefusal(`retargeted from ${base} before it was re-enqueued`); if (string(pull.headRefOid) !== head) throw new FixerRefusal("head moved before it was re-enqueued"); if (!queued(pull, dropped)) { - const hold = holdOf(pull); - if (hold !== undefined) throw new Held(hold); + if (isHeld(pull)) throw new Held(); try { await queueHead( (query, variables) => github.graphql(query, variables), @@ -736,7 +722,7 @@ async function requeue(github: GitHub, id: string, head: string, base: string, d /** * Open each validated disable as a PR of its own and enqueue it like any other, with no maintainer's * pick: its Review records the fixer's change. A disable that fails to advance is retried on the next - * pass; one GitHub refuses is left for a maintainer. + * pass; one GitHub refuses, or another session holds, is left for a maintainer. */ async function advanceDisables( github: GitHub, @@ -751,7 +737,15 @@ async function advanceDisables( if (!test.disable || test.disable.done !== undefined) continue; try { output.push( - await openDisable(github, repository, context, test, test.disable, () => saveFlaky(state, tests)), + await openDisable( + github, + repository, + context, + test, + test.disable, + () => saveFlaky(state, tests), + deps.warn, + ), ); } catch (error) { deps.warn(`quest land watch: disable of ${test.disable.quest} pending: ${String(error)}`); @@ -779,6 +773,7 @@ async function openDisable( test: FlakyTest, disable: Disable, record: () => void, + warn: (text: string) => void, ): Promise { const name = questName(disable.quest); if (disable.id === undefined) { @@ -816,26 +811,30 @@ async function openDisable( disable.id = string(pull.node_id); record(); } - if (disable.refused === undefined) { - try { - disable.done = await requeue(github, disable.id, disable.head, disable.base.ref, ""); - record(); - return `disabled ${name} ${disable.number}`; - } catch (error) { - if (!(error instanceof FixerRefusal || error instanceof Held)) throw error; - disable.refused = error.message; + const entry = { number: disable.number!, enabledAt: disable.head }; + try { + disable.done = await requeue(github, disable.id, disable.head, disable.base.ref, ""); + } catch (error) { + if (error instanceof Held) { + // Left for a maintainer, as a refused one is, once recorded. + disable.done = "held"; record(); + await sayHeld(github, repository, entry, warn); + return `disable held ${name} ${disable.number}`; } + if (!(error instanceof FixerRefusal)) throw error; + await post( + github, + { repository }, + entry, + `quest land watch: ${fixerText(error.message)}, so this pull request waits for a maintainer to enqueue it.`, + "refused", + MERGER, + ); + disable.done = error.message; + record(); + return `disable refused ${name}: ${error.message}`; } - await post( - github, - { repository }, - { number: disable.number!, enabledAt: disable.head }, - `quest land watch: ${fixerText(disable.refused)}, so this pull request waits for a maintainer to enqueue it.`, - "refused", - MERGER, - ); - disable.done = disable.refused; record(); - return `disable refused ${name}: ${disable.refused}`; + return `disabled ${name} ${disable.number}`; } diff --git a/src/cli/land-watch-flaky.ts b/src/cli/land-watch-flaky.ts index ee1cd153..b3d0e961 100644 --- a/src/cli/land-watch-flaky.ts +++ b/src/cli/land-watch-flaky.ts @@ -31,8 +31,6 @@ export interface Disable { /** Its PR, once opened. */ number?: number; id?: string; - /** Why it could not be queued, such as a hold, recorded before its comment, so a retry only comments. */ - refused?: string; /** How it ended: queued (`enqueued` or `armed`), or why it could not be. */ done?: string; } diff --git a/src/cli/land-watch.ts b/src/cli/land-watch.ts index 5aacc7f1..7f8cf28a 100644 --- a/src/cli/land-watch.ts +++ b/src/cli/land-watch.ts @@ -32,8 +32,8 @@ import { advanceFixes, endPick, type FixContext, - heldText, - holdOf, + HELD, + isHeld, readPull, readRemoval, requeuedHeads, @@ -103,8 +103,6 @@ export interface Removal { failure?: QueueFailure; /** What the fixer step made of a `fix` verdict; unset while it waits for one. */ taken?: string; - /** The hold label that ends the pick, recorded before its comment, so a retry honors it once it clears. */ - hold?: string; /** For a conflict: the base the quest merge driver could not merge, retried only once the base moves. */ unmerged?: string; } @@ -275,7 +273,7 @@ async function enqueueArmed( request === null || string(request.enabledAt) !== armed || // Another session holds it: leave the pick armed, for the next pass once the hold clears. - holdOf(now) !== undefined + isHeld(now) ) continue; try { @@ -395,7 +393,7 @@ export function jobsSection(blocks: string[], room: number): string { } /** What the watcher does about it, if anything it needs to say; `fixing` when a land fixer is configured. */ -const AFTER: Record string> = { +const AFTER: Record string> = { fix: (_, fixing) => fixing ? "\n\nThe land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs." @@ -404,7 +402,7 @@ const AFTER: Record `\n\nIts head has moved to ${current} since, and a push by anyone but the land fixer ends the pick: the watcher leaves this pull request alone until a maintainer enqueues it again.`, removed: () => "\n\nA person's removal ends the pick: the watcher leaves this pull request alone until a maintainer enqueues it again.", - held: (_, __, hold) => `\n\n${heldText(hold)}`, + held: () => `\n\n${HELD}`, unrecognized: () => "\n\nThe watcher does not recognize this reason, so it leaves this pull request alone until a maintainer enqueues it again.", unknown: () => @@ -450,9 +448,9 @@ async function handle( // An unknown head never authorizes the fixer: wait for GitHub to list the push, then end the pick. if (known === null && now - Date.parse(at) < UNLISTED) return null; const head = known ?? current; - // A hold recorded for this removal, before GitHub refused its comment, still ends the pick. + // A hold recorded for this removal, as before a crash, ends the pick even once it clears. const prior = readRemoval(state, number); - const hold = (prior?.event === string(event.id) ? prior.hold : undefined) ?? holdOf(pull); + const held = (prior?.event === string(event.id) && prior.verdict === "held") || isHeld(pull); const { pick, queued } = head === current ? first : await pickOf(github, string(pull.id), event, head, app, own); const verdict: Verdict = known === null @@ -463,7 +461,7 @@ async function handle( ? "removed" : !FIXABLE.includes(reason) ? "unrecognized" - : hold !== undefined + : held ? "held" : "fix"; const build = @@ -529,7 +527,6 @@ async function handle( verdict, jobs: build.jobs.map(({ log: _, ...job }) => job), log, - ...(verdict === "held" ? { hold } : {}), ...(verdict === "fix" ? { failure: { @@ -544,23 +541,29 @@ async function handle( } : {}), }; - // Recorded before its comment, so a pass retrying a refused comment keeps a hold that cleared since. A - // removal GitHub showed late never replaces the record of a newer one. + // Recorded before its comment, so a hold ends the pick whatever becomes of the comment. A removal + // GitHub showed late never replaces the record of a newer one. if ((prior?.at ?? "") <= at) { save(join(state, `removal-${number}.json`), removal); // Any other verdict ends the pick: its fixer history goes, so a later enqueue starts afresh. if (verdict !== "fix") endPick(state, number, pick.at); } - await post( + const said = post( github, { repository }, { number, enabledAt: pick.at }, - `quest land watch: the merge queue removed this pull request at ${head} because ${because(reason, actor, base.ref)} (\`${reason.replace(/[^a-z_]/g, "")}\`).${AFTER[verdict](current, fixing, hold ?? "")}${missing}`, + `quest land watch: the merge queue removed this pull request at ${head} because ${because(reason, actor, base.ref)} (\`${reason.replace(/[^a-z_]/g, "")}\`).${AFTER[verdict](current, fixing)}${missing}`, `removed ${string(event.id)}`, "quest land watch", undefined, (room) => jobsSection(blocks, room), ); + // A held removal's comment is the hold's, so it is best-effort: a refusal fails no pass and is not retried. + if (verdict === "held") + await said.catch((error: unknown) => + note(`quest land watch: hold comment on ${number} not posted: ${String(error)}`), + ); + else await said; return `removed ${number} ${reason} ${verdict}`; } diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index 7eedb51d..1faac133 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -15,7 +15,7 @@ import { CHECK_LOG } from "../src/cli/land-checks"; import { COLLECTION_ATTEMPTS, REQUEUE_LIMIT, history } from "../src/cli/land-fixer"; import { privateRepository } from "../src/cli/land-serve"; import { jobsSection, readRemoval, SETTLE, watchPass, watchPermissions } from "../src/cli/land-watch"; -import type { FixContext } from "../src/cli/land-watch-fix"; +import { type FixContext, HELD } from "../src/cli/land-watch-fix"; import { type FlakyTest, RECENT, readFlaky, saveFlaky, writeKnownFlaky } from "../src/cli/land-watch-flaky"; import { FLAKY_INPUT, type KnownFlaky } from "../src/cli/land-fix"; import { configure, GIT_SETTINGS, initRepository, tempDir } from "./fixture"; @@ -96,7 +96,7 @@ interface World { now: number; /** The upstream repository that holds each PR's branch, `branch-`. */ origin?: string; - /** Texts whose comment GitHub refuses to post, each once. */ + /** Texts whose comment GitHub refuses to post, each once, found in `# `. */ refuse?: string[]; /** A read of a PR's queue state that fails, once. */ unreadable?: (pull: Pull) => boolean; @@ -368,7 +368,8 @@ function deps(world: World): GitHubDependencies { const list = (world.comments[Number(comments[1])] ??= []); if (init?.method === "POST") { const comment = { user: { login: `${APP}[bot]` }, body: JSON.parse(String(init.body)).body }; - const refused = world.refuse?.findIndex((text) => comment.body.includes(text)) ?? -1; + const refused = + world.refuse?.findIndex((text) => `#${comments[1]} ${comment.body}`.includes(text)) ?? -1; if (refused >= 0) { world.refuse!.splice(refused, 1); return Response.json({ message: "refused" }, { status: 422 }); @@ -1491,13 +1492,11 @@ git commit -qm 'disable render_timeout'`; ]); await exited(state); github.now = Date.parse(at(minute + 2)); - // A session takes the disable's pull request as soon as it opens, and GitHub refuses its comment. - if (held && number === 7) { + // A session takes the disable's pull request as soon as it opens. + if (held && number === 7) github.onPull = (pull) => { if (pull.number === 8) pull.labels = ["quest:active"]; }; - github.refuse = ["waits for a maintainer"]; - } said.push(await watchPass(REPOSITORY, { state }, deps(github), context)); } const branch = github.pulls.find((pull) => pull.number === 8)?.branch; @@ -1506,18 +1505,17 @@ git commit -qm 'disable render_timeout'`; expect(said).toEqual([ ["disable ignored 5: first flaky verdict", `requeued 5 ${heads[0]}`], [expect.stringMatching(/^disable refused 6: Error: off-limits fixer edit/), `requeued 6 ${heads[1]}`], - [`disable 7 ${head}`, `requeued 7 ${heads[2]}`, ...(held ? [] : ["disabled render-timeout 8"])], + [ + `disable 7 ${head}`, + `requeued 7 ${heads[2]}`, + held ? "disable held render-timeout 8" : "disabled render-timeout 8", + ], ]); if (held) { - // The hold clears before the retry, which still leaves the pull request for a maintainer. + // Left for a maintainer, even once the hold clears. github.onPull = undefined; github.pulls.find((pull) => pull.number === 8)!.labels = []; - expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ - "disable refused render-timeout: held by quest:active", - ]); - expect(github.comments[8].map((comment) => comment.body)).toEqual([ - expect.stringContaining("`held by quest:active`, so this pull request waits for a maintainer"), - ]); + expect(github.comments[8].map((comment) => comment.body)).toEqual([expect.stringContaining(HELD)]); } // The disable is the fixer's commit on the base, enqueued like any other pull request with no pick. expect(git(repo.origin, "rev-parse", `${head}^`)).toBe(base); @@ -1549,7 +1547,7 @@ git commit -qm 'disable render_timeout'`; head, quest: "quest/m0/flaky/render-timeout.md", number: 8, - done: held ? "held by quest:active" : "enqueued", + done: held ? "held" : "enqueued", }, }, ]); @@ -1835,13 +1833,12 @@ git commit -qm 'disable render_timeout'`; fix.round(6, fixScript("fixed.txt")); fix.round(7, fixScript("fixed.txt")); fix.round(8, flakyScript); - // Each hold the watcher sees ends the pick, even when GitHub refuses its comment and the hold clears - // before the retry: here 5's at the removal, and below 7's after the push and 9's while it waits. - github.refuse = ["It is held by"]; - await expect(watchPass(REPOSITORY, { state }, deps(github), context)).rejects.toThrow(); - pulls[0].labels = []; + // Each hold the watcher sees ends the pick, and its comment is best-effort: GitHub refuses 5's here, + // and 7's below, and neither is retried once the hold clears. + github.refuse = ["Another session holds"]; // Three fixer slots: 9 waits. expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + expect.stringMatching(/^quest land watch: hold comment on 5 not posted: /), "removed 5 failed_checks held", "removed 6 failed_checks fix", "removed 7 failed_checks fix", @@ -1851,6 +1848,12 @@ git commit -qm 'disable render_timeout'`; "fixing 7", "fixing 8", ]); + // A crash before the pass marked 5's removal handled: its recorded hold still ends the pick once it + // clears, and the pass that handles it again posts the removal's comment. + pulls[0].labels = []; + const progress = join(state, "watch.json"); + const { handled, ...rest } = JSON.parse(readFileSync(progress, "utf8")); + writeFileSync(progress, JSON.stringify({ ...rest, handled: { ...handled, 5: {} } })); await exited(state, 3); // A session takes 6 while the watcher validates its fix, 7 right after the watcher pushes its fix, // 8 while its fixer runs, and 9 while it waits. @@ -1866,38 +1869,36 @@ git commit -qm 'disable render_timeout'`; }; pulls[3].labels = ["quest:iterate"]; pulls[4].labels = ["quest:active"]; - github.refuse = ["but not re-enqueued", "did not start on"]; + github.refuse = ["#7 quest land watch: Another session holds"]; github.now = Date.parse(at(7)); - await expect(watchPass(REPOSITORY, { state }, deps(github), context)).rejects.toThrow(); - // The hold ended 7's pick when it was recorded, so even a round set aside for its refused comment - // leaves no pick for a later App enqueue to continue. - expect(history(state, 7)).toBeUndefined(); - pulls[2].labels = []; - pulls[4].labels = []; - github.now = Date.parse(at(8)); expect((await watchPass(REPOSITORY, { state }, deps(github), context)).sort()).toEqual([ + "dropped 6 held", "dropped 7 held", + "dropped 8 held", "dropped 9 held", + "removed 5 failed_checks held", ]); expect(github.mutations).toEqual([]); expect(git(repo.origin, "rev-parse", "refs/heads/branch-6")).toBe(heads.get(6)); - const pushed = git(repo.origin, "rev-parse", "refs/heads/branch-7"); - expect(pushed).not.toBe(heads.get(7)); + expect(git(repo.origin, "rev-parse", "refs/heads/branch-7")).not.toBe(heads.get(7)); expect(fix.calls().map((call) => call.split(" ").at(-1))).toEqual(["6", "7", "8"]); - for (const pull of pulls) { - const held = github.comments[pull.number].filter((comment) => - comment.body.includes("It is held by `quest:"), - ); - expect(held).toHaveLength(1); - expect(history(state, pull.number)).toBeUndefined(); - } - // Only 7's branch moved, and its session hears where to. - expect(github.comments[6].at(-1)!.body).toContain("was not applied"); - expect(github.comments[7].at(-1)!.body).toContain(`was pushed as ${pushed}, but not re-enqueued`); - // The fixer's note on 8 is not posted: the held comment is the only one after the removal's. - expect(github.comments[8]).toHaveLength(2); + for (const pull of pulls) expect(history(state, pull.number)).toBeUndefined(); expect(readRemoval(state, 5)!.verdict).toBe("held"); expect(readRemoval(state, 9)!.taken).toBe("held"); + + // The holds clear: nothing is fixed, re-enqueued, or commented again. + github.onPull = undefined; + for (const pull of pulls) pull.labels = []; + github.now = Date.parse(at(8)); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); + expect(github.mutations).toEqual([]); + const held = (number: number) => + (github.comments[number] ?? []).filter((comment) => comment.body.includes(HELD)).length; + expect([5, 6, 7, 8, 9].map(held)).toEqual([1, 1, 0, 1, 1]); + // 8's round is not applied, as one whose pick ended is not: its flaky verdict does not count, and its + // fixer's note is not posted, so the hold's comment is the only one after the removal's. + expect(readFlaky(state)).toEqual([]); + expect(github.comments[8]).toHaveLength(2); expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([]); }); });