diff --git a/README.md b/README.md index a6bb9307..4b89051e 100644 --- a/README.md +++ b/README.md @@ -611,10 +611,17 @@ stays queued. `quest status` reads the hidden reason marker. Full logs stay in t `$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 parked alone for the fixer, blamed +a mistyped link, is parked alone for the fixer, blamed for those findings, so it never fails a batch; the fixer starts from that squash. Without a fixer it is ejected as `failed_checks` with those findings. 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 6dbd88a1..a980ccdb 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -72,5 +72,4 @@ commands from Sep-Oct. Session counts in each child come from that window. - [quest merge --land gaps](/quest/m0/helpers/merge-land.md) - squash on squash-only repositories, PR URLs outside a checkout, labels released after direct merges, and a warning when auto-merge outlives its head - [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 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/quest/m0/merge-driver-upkeep.md b/quest/m0/merge-driver-upkeep.md index 5440803c..9453e6da 100644 --- a/quest/m0/merge-driver-upkeep.md +++ b/quest/m0/merge-driver-upkeep.md @@ -32,7 +32,3 @@ Decided 2026-11-01 (user, /quest-plan of the follow-ups from the added the `quest-pinned` input, which only a manual `nix flake update quest-pinned` bumps. Pick how it stays current, such as Dependabot's nix updates, a step in the release checklist, or a recurring quest, and do it. - -## Related - -- [Merge queue siblings](/quest/m0/helpers/queue-siblings.md) - the landing-time drop this quest brings to local merges diff --git a/src/cli/land.ts b/src/cli/land.ts index 125b347c..56a9c2de 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"; @@ -575,10 +575,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; @@ -680,9 +726,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 da00f110..ff5454bc 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -554,7 +554,7 @@ export function main(argv: string[], out: Output): number | Promise { landCommand .command("serve") .description( - "Run one daemon per repository. As a batch is filled, park alone for the fixer (eject alone, without one) 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. A failed batch is never bisected: its PRs are parked for the fixer, which blames the ones at fault, or ejected together without a fixer. It halves the next batch's size down to one PR; each landed batch grows it back by one. When the fixer blames none and the base fails its check alone, the queue halts until a moved base passes, and the fixer proposes a fix of the base as a PR. 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, or none for a base that fails alone. 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, and the comment a result carries for its PR is posted there; invalid results, crashes, and timeouts are handed back. A validated fix whose push GitHub refuses is handed back with its resolution for `quest land apply`. 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, and GitHub still showing the pin after the daemon's own push, are retried briefly; a confirmed conflict parks only that PR for the fixer and a moved head ejects only that PR, and the rest of the batch stays queued. 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, ejects only a PR whose prepared tree differs as tree_mismatch (an already squashed PR stays merged), and leaves the rest of the batch queued to be rebuilt on the new base. 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, park alone for the fixer (eject alone, without one) 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. A failed batch is never bisected: its PRs are parked for the fixer, which blames the ones at fault, or ejected together without a fixer. It halves the next batch's size down to one PR; each landed batch grows it back by one. When the fixer blames none and the base fails its check alone, the queue halts until a moved base passes, and the fixer proposes a fix of the base as a PR. 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, or none for a base that fails alone. 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, and the comment a result carries for its PR is posted there; invalid results, crashes, and timeouts are handed back. A validated fix whose push GitHub refuses is handed back with its resolution for `quest land apply`. 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, and GitHub still showing the pin after the daemon's own push, are retried briefly; a confirmed conflict parks only that PR for the fixer and a moved head ejects only that PR, and the rest of the batch stays queued. 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, ejects only a PR whose prepared tree differs as tree_mismatch (an already squashed PR stays merged), and leaves the rest of the batch queued to be rebuilt on the new base. 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 baf585d1..b31cf584 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1232,21 +1232,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") }); @@ -1588,9 +1659,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") };