From 3cd3477eb229a4955113e8359014232dffddf0cb Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 00:17:45 -0700 Subject: [PATCH 1/2] chore(quest): claim quest/m0/helpers/queue-siblings.md Quest-Claim: 9d92f0bee49acdd0 From fc9a2938a86f66bbe88bea674447365db0eb6b83 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 00:29:47 -0700 Subject: [PATCH 2/2] feat(land): drop links to quests the base finished A queued PR that links a quest the base finishes after it branched used to fail `quest check` on its squash and be ejected for a fixer, even though the PR wrote the link while the quest was open. Each squash quest land builds, and each base it merges into a PR head, now drops the Required, Related, and Closes entries linking to quests the base deleted since the PR branched, with `quest rm`'s edit. Deletion since the merge base tells a finished quest from a mistyped link; a moved quest is a rename, and prose links still fail the check. Finishes quest/m0/helpers/queue-siblings.md. Co-Authored-By: Claude Opus 5.5 --- README.md | 9 +++- quest/m0/helpers/README.md | 1 - quest/m0/helpers/merge-driver.md | 4 -- quest/m0/helpers/queue-siblings.md | 57 --------------------- src/cli/land.ts | 56 +++++++++++++++++++-- src/cli/main.ts | 2 +- src/cli/tree.ts | 27 +++++++--- test/land.test.ts | 81 +++++++++++++++++++++++++++--- 8 files changed, 158 insertions(+), 79 deletions(-) delete mode 100644 quest/m0/helpers/queue-siblings.md diff --git a/README.md b/README.md index ed9557bd..a124306e 100644 --- a/README.md +++ b/README.md @@ -561,10 +561,17 @@ PR in that batch, without bisection. Ejection disables auto-merge, removes `$STATE_DIRECTORY` or the XDG state home, never inside the checkout. A check that runs past `--timeout` minutes (60 by default) is killed and fails its batch. +Every squash the daemon builds, and every base it merges into a PR head, drops +each `Required`, `Related`, or `Closes` entry linking to a quest the base +deleted since the PR branched, as `quest rm` would have. The PR wrote that link +while the quest was open, so the base finishing it is no fault of the PR's: a +sibling PR that links a quest another sibling finishes lands in the same batch. +A quest the base moved is a rename, not a deletion, and a link in prose stays. + As a batch is filled, each PR it reaches is squashed alone onto the current base, and `quest check` runs on the result; PRs the batch does not reach wait for a later round, so a long queue costs only each batch's candidates. A PR with findings the base lacks, such as -a link to a quest the base has since finished, is ejected alone as +a mistyped link, is ejected alone as `failed_checks` with those findings, so it never fails a batch; a configured fixer receives it as it would a failed batch, starting from that squash. This check runs in the daemon, never the sandbox: it parses quest files read from git objects diff --git a/quest/m0/helpers/README.md b/quest/m0/helpers/README.md index dbf75eed..988f5bd8 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -76,7 +76,6 @@ commands from Sep-Oct. Session counts in each child come from that window. - [Plan delegated follow-ups](/quest/m0/helpers/follow-up-plan.md) - a caller plans a background merge's follow-ups without asking - [Drop review-rounds](/quest/m0/helpers/review-rounds.md) - `quest status` stops counting GitHub reviews now that `quest review` numbers rounds - [Headless agents](/quest/m0/helpers/headless-agents.md) - `/quest-spawn api` launches, resumes, and stops billed agents through one command auto mode allows -- [Sibling PRs through a server-side merge](/quest/m0/helpers/queue-siblings.md) - children of one epic land back to back through a merge queue without conflicting on its link list - [moq-dev/quest lands through quest land](/quest/m0/helpers/land-adopt.md) - this repository's `main` changes only through its own gate ## Related diff --git a/quest/m0/helpers/merge-driver.md b/quest/m0/helpers/merge-driver.md index 998472f5..e2a6fc4e 100644 --- a/quest/m0/helpers/merge-driver.md +++ b/quest/m0/helpers/merge-driver.md @@ -22,7 +22,3 @@ once `quest` was put on PATH. head's code. - Rejected: `quest init` writing an absolute binary path, which breaks on upgrades and Nix store paths. - -## Related - -- [Sibling PRs through a server-side merge](/quest/m0/helpers/queue-siblings.md) - the same link-list conflict, but in GitHub's merges, which run no driver diff --git a/quest/m0/helpers/queue-siblings.md b/quest/m0/helpers/queue-siblings.md deleted file mode 100644 index cfc71eb5..00000000 --- a/quest/m0/helpers/queue-siblings.md +++ /dev/null @@ -1,57 +0,0 @@ -# [S] Sibling quest PRs land through a server-side merge - -## Goal - -Several PRs that each finish a child of the same epic land back to back -through a server-side merge, such as a GitHub merge queue, without each -landing knocking the next one out on a conflict in the epic's link list, or -on a `quest check` failure over a quest the base just finished. - -## Plan - -Decided 2026-10-25 (user), from a moq-dev/moq merge wave. Nine -test-flake PRs each deleted their own line from one epic README's -`Required` list. The local `quest` merge driver resolves those edits, but -GitHub's merge queue builds its merge commit without it. Each landing -conflicted with the next queued sibling, which cost four extra -drop, merge, and re-land cycles in one batch. - -A second symptom: a quest-only PR waiting in the queue fails `quest check` -once main finishes a quest it links. moq-dev/moq#6707 was dropped 3 times -and moq-dev/moq#6790 4 times. Options: run `quest check` against current -main before queueing, or have the check treat a link to a deleted quest as -resolvable. A pre-queue check alone misses a base that moves after queueing, -and a tolerant check must tell a finished quest from a mistyped link. - -The fix belongs in quest, not in each repository's process. Rejected: -landing one sibling at a time, which serializes batches. - -Narrowed 2026-10-26 (user): `quest land` runs -the merge driver while it builds its chain, so a repository on that gate no -longer hits the link-list conflict. This quest keeps the dead-link -`quest check` failure on every gate, which caused 6 of moq's queue failures -from 2026-10-23 to 10-26, and the link-list conflict for repositories that -still merge through GitHub's merge queue. Its third option below is what -`quest land` does. - -Narrowed 2026-10-31: before batching, `quest land` runs `quest check` on -each queued PR squashed alone onto the current base, and ejects a PR whose -new findings, such as a link to a quest the base finished, fail it, with that -output. It no longer fails a whole batch. That is the pre-queue check above, -run against every base the PR waits on. The PR is still ejected and needs a -fix, so this quest keeps the tolerant check or another way to spare it, on -every gate. - -Open (this quest decides, with a recommendation to the user): - -- Change the link-list format so deleting one entry never touches its - neighbors' lines in a 3-way merge. -- Or keep the finishing PR's diff off the shared parent list. Example: drop - the link in a later pass, or have `quest check` treat a link to a deleted - child as done rather than broken. -- Or have `quest merge --land` merge the base locally with the driver and - re-land when the queue drops a PR only for a link-list conflict. - -Whatever it picks must keep `quest check`'s rule that a finished quest leaves -no dangling reference once all siblings land. Test it with two sibling PRs -landing through a merge that does not run the driver. diff --git a/src/cli/land.ts b/src/cli/land.ts index fc19f542..24cd4ce6 100644 --- a/src/cli/land.ts +++ b/src/cli/land.ts @@ -1,7 +1,7 @@ import { spawnSync } from "node:child_process"; import { resolve } from "node:path"; -import { type Finding, MERGE_LABEL, check, formatFinding } from "../core"; +import { type Finding, MERGE_LABEL, check, formatFinding, isQuest, remove } from "../core"; import { repositoryPath } from "./claims"; import { GitHub, @@ -16,7 +16,7 @@ import { string, } from "./github"; import { classifyCheck } from "./status"; -import { snapshot } from "./tree"; +import { committedSources, snapshot } from "./tree"; /** The check a gated base requires. Nothing reports it on a PR head, so GitHub's auto-merge only waits. */ export const LAND_CHECK = "quest land"; @@ -560,10 +560,56 @@ function squash(dir: string, driver: string, entry: Entry): string[] | null { git(dir, ["reset", "--quiet", "--hard", "HEAD"]); return files; } + prune(dir, entry.head); git(dir, ["commit", "--quiet", "--allow-empty", "-m", `${entry.title} (#${entry.number})`]); return null; } +/** + * Finish a clean merge of `head` into `HEAD`, staged in the index, by dropping each list entry + * that links to a quest `HEAD` finished since `head` branched, as `quest rm` drops links to a + * quest it deletes. The PR wrote that link while the quest was open, so the base finishing it + * leaves the link dangling through no fault of the PR, and `quest check` would reject it. + * Deletion since the merge base tells a finished quest from a mistyped link, and a quest the base + * moved is a rename, not a deletion. A link in prose stays, for `quest check` to report. Both the + * chain and a prepared head merge through here, so the head lands the tree the chain tested. + * Returns the documents it changed. + */ +function prune(dir: string, head: string): string[] { + const since = git(dir, ["merge-base", "HEAD", head]); + const finished = new Set( + git(dir, [ + "diff", + "--no-ext-diff", + "--find-renames", + "--diff-filter=D", + "--name-only", + "-z", + since, + "HEAD", + "--", + "quest", + ]) + .split("\0") + .filter(isQuest), + ); + if (finished.size === 0) return []; + let tree: ReturnType; + try { + tree = committedSources(dir, git(dir, ["write-tree"]), false); + } catch { + // A tree `quest check` cannot read is its finding to report, not this edit's. + return []; + } + const modes = new Map(tree.sources.map((source) => [source.path, source.mode])); + const texts = remove(tree.sources, (path) => finished.has(path) && !tree.exists(path)); + for (const [path, text] of texts) { + const oid = git(dir, ["hash-object", "-w", "--no-filters", "--stdin"], { input: text }); + git(dir, ["update-index", "--cacheinfo", `${modes.get(path)},${oid},${path}`]); + } + return [...texts.keys()]; +} + /** How many queued PRs ahead of a newly queued one its conflict report covers: those landing soonest. */ const OVERLAP_AHEAD = 4; @@ -665,9 +711,13 @@ export function mergeBase( const result = tryGit(dir, ["-c", `merge.quest.driver=${driver}`, "merge", "--no-ff", "--no-commit", head]); if (result.status !== 0) return { reason: "merge_conflict", detail: `${result.stdout}\n${result.stderr}`.trim() }; + const pruned = prune(dir, head); const tree = git(dir, ["write-tree"]); + const message = pruned.length + ? `quest land: merge tested base\n\nDrop links to quests the base finished from ${pruned.join(", ")}.\n` + : "quest land: merge tested base\n"; const commit = git(dir, ["commit-tree", tree, "-p", head, "-p", base], { - input: "quest land: merge tested base\n", + input: message, env: { GIT_AUTHOR_NAME: "moq-bot", GIT_AUTHOR_EMAIL: "moq-bot@users.noreply.github.com", diff --git a/src/cli/main.ts b/src/cli/main.ts index ee0a587e..b7e83e31 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -534,7 +534,7 @@ export function main(argv: string[], out: Output): number | Promise { landCommand .command("serve") .description( - "Run one daemon per repository. As a batch is filled, eject alone each PR it reaches whose squash onto the base fails quest check with findings the base lacks, and comment once on each newly queued PR that conflicts with the four PRs landing soonest ahead of it. Check a base PR plus up to floor(log2 n) cleanly merging PRs once through the sandbox, alternating the oldest and the newest queued PR as the base. Failed batches are ejected together, never bisected, and halve the next batch's size down to one PR; each landed batch grows it back by one. Requires quest on PATH and flock. Sandbox and fixer wrappers MUST run code as a different user from the App key owner (for example in nspawn containers); environment filtering alone cannot isolate /proc or host files. The sandbox receives , with the base also in QUEST_LAND_BASE and present in the standalone shallow clone; the optional tracked background fixer receives the log path followed by PR numbers. Neither receives credentials. State retains logs, fixer jobs, and one mechanical attempt per maintainer pick across restarts. Fixer results are validated, lease-pushed, recorded in Review, and pinned for requeue; invalid results, crashes, and timeouts are handed back. After revalidating the queue, merge the base into each behind-base PR head with quest resolve, verify the tested tree, and lease-push that head before the pinned GitHub squash API merge. A permissions-refused branch push falls back to a leased fast-forward of the tested squash to the base, closing the PR with a linked-commit comment and removing quest:merge. A recorded daemon merge atop the original pin is recognized after restart; other moved heads require maintainer requeueing. Pending mergeability is retried briefly; confirmed conflicts or moved heads eject the remaining batch. A refusal that would repeat, such as squash being disabled, ejects as merge_refused without the fixer. An unexpected squash parent rebuilds the queue without ejecting; a tree mismatch logs CRITICAL and ejects the rest as tree_mismatch. Merged PRs drop out of the queue after restart.", + "Run one daemon per repository. Every squash, and every base merged into a PR head, drops each list entry linking to a quest the base deleted since the PR branched, as quest rm would. As a batch is filled, eject alone each PR it reaches whose squash onto the base fails quest check with findings the base lacks, and comment once on each newly queued PR that conflicts with the four PRs landing soonest ahead of it. Check a base PR plus up to floor(log2 n) cleanly merging PRs once through the sandbox, alternating the oldest and the newest queued PR as the base. Failed batches are ejected together, never bisected, and halve the next batch's size down to one PR; each landed batch grows it back by one. Requires quest on PATH and flock. Sandbox and fixer wrappers MUST run code as a different user from the App key owner (for example in nspawn containers); environment filtering alone cannot isolate /proc or host files. The sandbox receives , with the base also in QUEST_LAND_BASE and present in the standalone shallow clone; the optional tracked background fixer receives the log path followed by PR numbers. Neither receives credentials. State retains logs, fixer jobs, and one mechanical attempt per maintainer pick across restarts. Fixer results are validated, lease-pushed, recorded in Review, and pinned for requeue; invalid results, crashes, and timeouts are handed back. After revalidating the queue, merge the base into each behind-base PR head with quest resolve, verify the tested tree, and lease-push that head before the pinned GitHub squash API merge. A permissions-refused branch push falls back to a leased fast-forward of the tested squash to the base, closing the PR with a linked-commit comment and removing quest:merge. A recorded daemon merge atop the original pin is recognized after restart; other moved heads require maintainer requeueing. Pending mergeability is retried briefly; confirmed conflicts or moved heads eject the remaining batch. A refusal that would repeat, such as squash being disabled, ejects as merge_refused without the fixer. An unexpected squash parent rebuilds the queue without ejecting; a tree mismatch logs CRITICAL and ejects the rest as tree_mismatch. Merged PRs drop out of the queue after restart.", ) .option("--base ", "Base branch (default: repository default)") .option("--state ", "Private daemon state directory (default: $STATE_DIRECTORY or the XDG state home)") diff --git a/src/cli/tree.ts b/src/cli/tree.ts index 768ae6ee..ae822dc6 100644 --- a/src/cli/tree.ts +++ b/src/cli/tree.ts @@ -73,6 +73,24 @@ export function checkout(root: string): Snapshot { * every path inside a submodule exists rather than being read from it. */ export function snapshot(root: string, commit: string, submodules = true): Snapshot { + const tree = committedSources(root, commit, submodules); + return { docs: tree.sources.map((source) => parse(source.path, source.text)), exists: tree.exists }; +} + +/** A quest document as committed: its source text and its file mode. */ +export interface CommittedSource extends Source { + mode: string; +} + +/** + * Every quest document's source at `commit`, a commit or a tree, in tree order, read from git + * objects alone, as `snapshot` reads them, and whether a repository-relative path exists there. + */ +export function committedSources( + root: string, + commit: string, + submodules = true, +): { sources: CommittedSource[]; exists(path: string): boolean } { const tree = committed(root, commit, submodules); const quests = tree.entries.filter( ({ mode, type, path }) => type === "blob" && mode !== "120000" && path.startsWith("quest/") && isQuest(path), @@ -82,17 +100,14 @@ export function snapshot(root: string, commit: string, submodules = true): Snaps // One `cat-file --batch` reads every document: ` blob \n\n` each, in input order. const batch = git(root, ["cat-file", "--batch"], `${quests.map((quest) => quest.oid).join("\n")}\n`); let offset = 0; - const docs = quests.map(({ path }) => { + const sources = quests.map(({ path, mode }) => { const header = batch.indexOf(10, offset); const size = Number(batch.toString("utf8", offset, header).split(" ")[2]); const text = decode(batch.subarray(header + 1, header + 1 + size), path); offset = header + size + 2; - return { path, text }; + return { path, mode, text }; }); - return { - docs: docs.sort((a, b) => comparePaths(a.path, b.path)).map((source) => parse(source.path, source.text)), - exists: tree.exists, - }; + return { sources: sources.sort((a, b) => comparePaths(a.path, b.path)), exists: tree.exists }; } interface Entry { diff --git a/test/land.test.ts b/test/land.test.ts index f2199028..6422a768 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1061,21 +1061,92 @@ describe.skipIf(process.platform === "win32")("host daemon", () => { test("ejects alone a PR that fails quest check on the current base, before batching", async () => { const forge = new Forge(); forge.advance({ "quest/m0/README.md": EPIC(["a", "b"]), "quest/m0/b.md": QUEST("B") }); + // A mistyped link never named a quest, so nothing the base did explains it. const linking = forge.pull("Link b", { - "quest/m0/a.md": `${QUEST("A")}\n## Related\n\n- [B](/quest/m0/b.md) - b\n`, + "quest/m0/a.md": `${QUEST("A")}\n## Related\n\n- [B](/quest/m0/bb.md) - b\n`, }); forge.pull("Clean", { "clean.txt": "ok" }); - // The base finishes b after the PR was queued, leaving its link dangling. - forge.advance({ "quest/m0/README.md": EPIC(["a"]), "quest/m0/b.md": null }); const { checked, landing } = await forge.round(); expect(landing).toEqual(["ejected 1 failed_checks", `landed 2 ${forge.repo.pulls[1].squash}`]); expect(checked).toEqual([[2]]); expect(forge.comment(1)).toContain("`quest check` fails with it squashed alone onto the base"); - expect(forge.comment(1)).toMatch(/quest\/m0\/a\.md:\d+: .*\/quest\/m0\/b\.md/); + expect(forge.comment(1)).toMatch(/quest\/m0\/a\.md:\d+: .*\/quest\/m0\/bb\.md/); expect(forge.comment(1)).toContain(""); expect(linking.labels).toEqual([]); }); + test("drops a queued PR's list links to a quest the base finished, in the chain and the landed head", async () => { + const forge = new Forge(); + forge.advance({ "quest/m0/README.md": EPIC(["a", "b"]), "quest/m0/b.md": QUEST("B") }); + const pull = forge.pull("Plan c", { + "quest/m0/README.md": EPIC(["a", "b", "c"]), + "quest/m0/c.md": `${QUEST("C")}\n## Required\n\n- [B](/quest/m0/b.md) - b first\n`, + "quest/m0/a.md": `${QUEST("A")}\n## Related\n\n- [B](/quest/m0/b.md) - b\n- [C](/quest/m0/c.md) - c\n`, + }); + const pin = pull.head!; + // The base finishes b after the PR was queued, leaving both of its links to b dangling. + forge.advance({ "quest/m0/README.md": EPIC(["a"]), "quest/m0/b.md": null }); + const base = forge.main(); + const { checked, landing } = await forge.round(); + expect(checked).toEqual([[1]]); + expect(landing).toEqual([`landed 1 ${pull.squash}`]); + // c is no longer blocked, and a keeps its other link. + expect(git(forge.origin, "show", "main:quest/m0/c.md")).toBe(QUEST("C").trim()); + expect(git(forge.origin, "show", "main:quest/m0/a.md")).toBe( + `${QUEST("A")}\n## Related\n\n- [C](/quest/m0/c.md) - c`, + ); + expect(git(forge.origin, "show", "-s", "--format=%P", pull.head!)).toBe(`${pin} ${base}`); + expect(git(forge.origin, "show", "-s", "--format=%B", pull.head!)).toContain( + "Drop links to quests the base finished from quest/m0/a.md, quest/m0/c.md.", + ); + }); + + test("two sibling PRs land together when one finishes a quest the other links", async () => { + const forge = new Forge(); + forge.advance({ + "quest/m0/README.md": EPIC(["a", "b", "c"]), + "quest/m0/b.md": QUEST("B"), + "quest/m0/c.md": QUEST("C"), + }); + // Each passes `quest check` alone on the base; chained, the first leaves the second's link dangling. + const one = forge.pull("Finish b", { "quest/m0/README.md": EPIC(["a", "c"]), "quest/m0/b.md": null }); + const two = forge.pull("Finish c", { + "quest/m0/README.md": EPIC(["a", "b"]), + "quest/m0/c.md": null, + "quest/m0/a.md": `${QUEST("A")}\n## Related\n\n- [B](/quest/m0/b.md) - b\n`, + }); + const { checked, landing } = await forge.round(); + expect(checked).toEqual([[1, 2]]); + expect(landing).toEqual([`landed 1 ${one.squash}`, `landed 2 ${two.squash}`]); + expect(git(forge.origin, "show", "main:quest/m0/README.md")).toBe(EPIC(["a"]).trim()); + expect(git(forge.origin, "show", "main:quest/m0/a.md")).toBe(QUEST("A").trim()); + }); + + test("ejects a PR whose prose links a quest the base finished, and keeps links to a quest the base moved", async () => { + const forge = new Forge(); + forge.advance({ + "quest/m0/README.md": EPIC(["a", "b", "c"]), + "quest/m0/b.md": QUEST("B"), + "quest/m0/c.md": `${QUEST("C")}\nMore about c, so a move keeps enough of it to read as a rename.\n`, + }); + const prose = forge.pull("Mention b", { "quest/m0/a.md": `${QUEST("A")}\nSee [B](/quest/m0/b.md).\n` }); + const moving = forge.pull("Link c", { + "quest/m0/a.md": `${QUEST("A")}\n## Related\n\n- [C](/quest/m0/c.md) - c\n`, + }); + // The base finishes b and moves c to d. + forge.advance({ + "quest/m0/README.md": EPIC(["a", "d"]), + "quest/m0/b.md": null, + "quest/m0/c.md": null, + "quest/m0/d.md": `${QUEST("C")}\nMore about c, so a move keeps enough of it to read as a rename.\n`, + }); + const { landing } = await forge.round(); + expect(landing).toEqual(["ejected 1 failed_checks", "ejected 2 failed_checks"]); + expect(forge.comment(1)).toMatch(/quest\/m0\/a\.md:\d+: .*\/quest\/m0\/b\.md/); + expect(forge.comment(2)).toMatch(/quest\/m0\/a\.md:\d+: .*\/quest\/m0\/c\.md/); + expect([prose.labels, moving.labels]).toEqual([[], []]); + }); + test("screens only the PRs a batch reaches, leaving the rest of a long queue for later", async () => { const forge = new Forge(); forge.advance({ "quest/m0/README.md": EPIC(["a", "b"]), "quest/m0/b.md": QUEST("B") }); @@ -1235,9 +1306,7 @@ mv '${result}.new' '${result}' test("starts the fixer on a PR that fails quest check alone, from its squash onto the base", async () => { const forge = new Forge(); - forge.advance({ "quest/m0/README.md": EPIC(["a", "b"]), "quest/m0/b.md": QUEST("B") }); forge.pull("Link b", { "quest/m0/a.md": `${QUEST("A")}\n## Related\n\n- [B](/quest/m0/b.md) - b\n` }); - forge.advance({ "quest/m0/README.md": EPIC(["a"]), "quest/m0/b.md": null }); const deps = await forge.deps(); const plan = await resolveQueue(REPOSITORY, undefined, deps); const options = { ...sandbox(), fixer: join(tempDir(), "fixer") };