diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2a84acbc..6c3aee43 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -55,8 +55,9 @@ configured sandbox. Before each landing, the daemon merges the base into the PR head with the quest merge driver and verifies the tested tree, then pushes that head with a lease and squash-merges it through GitHub. If branch permissions refuse the push, it fast-forwards the base to the tested squash with a lease and -closes the PR with a linked-commit comment instead. Confirmed conflicts, moved heads, or a tree mismatch eject the -remaining batch. Transient refusals are retried; a racing base rebuilds the queue; already merged PRs stay merged. Failed batches are ejected together, with +closes the PR with a linked-commit comment instead. A confirmed conflict parks only +that PR for the fixer, and a moved head or a prepared tree mismatch ejects only that PR; the +rest of the batch stays queued. Transient refusals are retried; a racing base rebuilds the queue; already merged PRs stay merged. Failed batches are ejected together, with a log tail in each comment and the full log retained on the host. The optional credential-free `quest land fix` container makes one mechanical attempt per maintainer pick. The daemon validates and lease-pushes fixes, records the new diff --git a/README.md b/README.md index 565ebc15..01f6bf5e 100644 --- a/README.md +++ b/README.md @@ -637,11 +637,16 @@ tested squash to the base as a fast-forward, leased on the expected parent. It closes the PR with a comment linking the landed commit and removes `quest:merge`. Such a fork PR is closed rather than shown as merged by GitHub. A PR branch lease failure ejects `head_moved`. A locally prepared or API squash tree mismatch logs -CRITICAL and ejects the remaining batch as `tree_mismatch`. +CRITICAL. A PR whose prepared tree differs ejects alone as `tree_mismatch`; an +already-squashed one stays merged. Either way, the rest of the batch stays queued +and is rebuilt and checked on the new base. After an API refusal, the daemon re-reads mergeability and briefly retries -pending mergeability or a base refresh. A confirmed conflict parks only that PR for -the fixer and a moved head ejects only that PR; the rest stays queued. A persistent policy refusal ejects as `merge_refused` without the +pending mergeability or a base refresh. It also retries while GitHub still +reports the pin as the head of a PR it just prepared, since GitHub can refuse the +merge for that stale head; only another writer's head ejects `head_moved`. +A confirmed conflict parks only that PR for the fixer and a moved head ejects +only that PR; the rest stays queued. A persistent policy refusal ejects as `merge_refused` without the fixer. Already-merged PRs are skipped. An unexpected API squash parent means the base moved: the remainder is rebuilt without ejection. diff --git a/quest/m0/README.md b/quest/m0/README.md index 8090a0a0..f324ae5d 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -42,7 +42,6 @@ ownership-manifest design: - [Merge through a host gate](/quest/m0/host-land/README.md) - inline merge picks, small batches landed from the maintainer's host, and a Codex fixer instead of bisection - [The land fixer handles every mechanical failure](/quest/m0/land-fixer/README.md) - attempts, registry network, triage, flaky tests, local mergeability, protected edits, and fork commits - [Landing priority](/quest/m0/land-priority.md) - cheap checks wait a bounded time and the land daemon's check runs ahead of agent builds -- [Accurate head_moved](/quest/m0/land-head-moved.md) - the gate's own base-merge push and batch-mate failures are not reported as a moved head - [Drop emptied Required headings](/quest/m0/required-heading.md) - a clean driver merge that empties a `Required` list passes `quest check` - [GitHub retries](/quest/m0/github-retry.md) - one 5xx or dropped connection no longer fails a command or land step - [Current conflict reports](/quest/m0/land-conflicts-refresh.md) - a queued PR's conflict report updates when the queue ahead of it changes diff --git a/quest/m0/land-head-moved.md b/quest/m0/land-head-moved.md deleted file mode 100644 index ef2e0e2f..00000000 --- a/quest/m0/land-head-moved.md +++ /dev/null @@ -1,29 +0,0 @@ -# [S] quest land reports head_moved only when a writer moved the head - -## Goal - -`quest land` ejects a PR as `head_moved` only when someone other than the -gate pushed to it after it was queued. The gate's own base-merge push is -never counted as a moved head. A PR stopped because a batch-mate conflicted -or moved is ejected with that reason, naming the batch-mate. - -## Plan - -Decided 2026-10-31 (user, /quest-plan of the follow-ups from a moq-dev/moq -`/quest-complete` run). PRs were wrongly ejected as `head_moved`: -[moq-dev/moq#7407](https://github.com/moq-dev/moq/pull/7407), -[moq-dev/moq#7416](https://github.com/moq-dev/moq/pull/7416), -[moq-dev/moq#7353](https://github.com/moq-dev/moq/pull/7353), -[moq-dev/moq#7413](https://github.com/moq-dev/moq/pull/7413). - -- Own push. `prepareHead` (`src/cli/land-head.ts`) pushes a base merge to a - PR's head, and `isPrepared` recognises that head from a private record. Land - still reported `head_moved` after that push, so one of the checks in - `resolveQueue` (`src/cli/land.ts`), `prepareHead`, or `mergePinned` - (`src/cli/land-serve.ts`) missed it. - Find the case the record misses, such as GitHub lagging, a restart, or - `forgetHeads` running too early, and cover it with a test. -- Batch-mates. When one link fails to merge, `serveBatch` - (`src/cli/land-serve.ts`) ejects every remaining link with that link's - reason and detail. Give the other PRs their own reason, one that names - the PR that stopped the batch. diff --git a/src/cli/land-serve.ts b/src/cli/land-serve.ts index 82dcf10e..b7969684 100644 --- a/src/cli/land-serve.ts +++ b/src/cli/land-serve.ts @@ -347,16 +347,15 @@ async function mergePinned(github: GitHub, path: string, head: string, previous if (pull.merged === true) return null; const detail = `GitHub refused ${path}: ${String(result.message)}`; if (pull.state !== "open") return { reason: "merge_refused", detail }; - if ( - previous !== head && - response.status === 409 && - (object(pull.head).sha === previous || object(pull.head).sha === head) - ) { + // Right after the gate's own push, GitHub can still report the pin as the head and refuse the merge + // for it: a 409, or a 405 when the pin conflicts with the base that push merged. Neither is a writer. + const current = object(pull.head).sha; + if (previous !== head && (current === previous || (response.status === 409 && current === head))) { if (attempt >= 3) throw new Error(`${detail}; prepared head has not propagated`); await github.pause(500 * 2 ** attempt); continue; } - if (object(pull.head).sha !== head) return { reason: "head_moved", detail }; + if (current !== head) return { reason: "head_moved", detail }; if (pull.mergeable === false || pull.mergeable_state === "dirty") return { reason: "merge_conflict", detail }; if (pull.mergeable === null || /base branch was modified/i.test(String(result.message))) { if (attempt >= 3) throw new Error(`${detail}; mergeability did not settle`); @@ -647,8 +646,8 @@ export async function serveBatch( } if ("reason" in result) { // Only this PR failed to land. The rest of the batch passed its check and stays queued, - // to be rebuilt on the new base, except after a tree mismatch, which stops them all. - const detail = `${result.detail}; the rest of the batch was not merged`; + // to be rebuilt on the new base. + const detail = `${result.detail}; the rest of the batch stays queued`; appendFileSync(log, `${detail}\n`); if (result.reason === "merge_conflict") { output.push( @@ -664,22 +663,11 @@ export async function serveBatch( ); return output; } - const remaining = result.reason === "tree_mismatch" ? chain.links.slice(index) : [link]; - if (result.reason === "tree_mismatch") deps.warn(`CRITICAL: ${detail}`); + // A prepared tree that differs from the tested one is the gate's own fault, for a maintainer to judge. + const reported = result.reason === "tree_mismatch" ? `CRITICAL: ${detail}` : detail; + if (reported !== detail) deps.warn(reported); return output.concat( - await stop( - root, - options, - deps, - batch, - remaining.map(({ entry }) => - ejection( - entry, - result.reason, - excerpt(result.reason === "tree_mismatch" ? `CRITICAL: ${detail}` : detail), - ), - ), - ), + await stop(root, options, deps, batch, [ejection(link.entry, result.reason, excerpt(reported))]), ); } const sha = result.sha; @@ -697,20 +685,11 @@ export async function serveBatch( expectedParent = sha; const tree = string(object(commit.tree).sha); if (tree === link.tree) continue; - const detail = `CRITICAL: GitHub squash #${link.entry.number} at ${sha} has tree ${tree}, but the tested chain has ${link.tree}. Already merged PRs stay merged; the remaining batch was stopped`; + // The squash is already merged, so there is nothing to eject. The rest of the batch stays queued and + // is rebuilt and checked on the base as it now is, like after a moved base. + const detail = `CRITICAL: GitHub squash #${link.entry.number} at ${sha} has tree ${tree}, but the tested chain has ${link.tree}. It stays merged; the rest of the batch stays queued, to be rebuilt on the new base`; deps.warn(detail); appendFileSync(log, `${detail}\n`); - const remaining = chain.links.slice(index + 1); - if (remaining.length) - output.push( - ...(await stop( - root, - options, - deps, - batch, - remaining.map(({ entry }) => ejection(entry, "tree_mismatch", excerpt(detail))), - )), - ); return output; } return output; diff --git a/src/cli/main.ts b/src/cli/main.ts index 42c0e5af..e52f8c45 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -551,7 +551,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 is 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 and ejects the rest as tree_mismatch. Merged PRs drop out of the queue after restart.", + "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.", ) .option("--base ", "Base branch (default: repository default)") .option("--state ", "Private daemon state directory (default: $STATE_DIRECTORY or the XDG state home)") diff --git a/test/land.test.ts b/test/land.test.ts index faffb928..b30b973e 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -738,6 +738,38 @@ describe.skipIf(process.platform === "win32")("quest land", () => { expect(forge.comment(1)).toBeUndefined(); }); + // moq-dev/moq#7407: GitHub still showed the pin after the gate's base-merge push and refused the + // merge as conflicting, since that pin conflicts with the base. It was ejected as head_moved. + test("a pushed head's 405 conflict on the stale pin is retried rather than ejected as head_moved", async () => { + const forge = new Forge(); + const pull = forge.pull("Old", { "old.txt": "yes" }); + const pin = pull.head; + forge.advance({ "new.txt": "yes" }); + const deps = await forge.deps(); + const plan = await resolveQueue(REPOSITORY, undefined, deps); + pull.mergeRefusals = [{ status: 405, message: "Pull Request has merge conflicts" }]; + const seen: (string | undefined)[] = []; + forge.beforeMerge = () => { + seen.push(pull.head); + }; + const pauses: number[] = []; + deps.sleep = async (delay) => { + pauses.push(delay); + }; + let pushed = false; + expect( + await serveBatch(forge.clone(), plan, sandbox(), deps, async () => { + // The first token is the base-merge push; GitHub misses it for the merge and the re-read. + if (!pushed) forge.lag = 2; + pushed = true; + return deps.token; + }), + ).toEqual([`landed 1 ${pull.squash}`]); + expect(seen[1]).toBe(pin); + expect(pauses).toEqual([500]); + expect(forge.comment(1)).toBeUndefined(); + }); + test("restart rejects a writer's commit after a recorded merge", async () => { const forge = new Forge(); forge.pull("Old", { "old.txt": "yes" }); @@ -1654,7 +1686,7 @@ mv '${result}.new' '${result}' }, ); - test("a tree mismatch logs CRITICAL, keeps landed PRs, and ejects only the unmerged batch suffix", async () => { + test("a squash tree mismatch logs CRITICAL, stays merged, and the next round rebuilds the rest on it", async () => { const forge = new Forge(); for (let i = 1; i <= 4; i++) forge.pull(`PR ${i}`, { [`${i}.txt`]: "yes" }); forge.mismatch = 2; @@ -1664,18 +1696,94 @@ mv '${result}.new' '${result}' warnings.push(message); }; const plan = await resolveQueue(REPOSITORY, undefined, deps); - const options = sandbox(); - const output = await serveBatch(forge.clone(), plan, options, deps, async () => deps.token); - expect(output).toEqual([ + const bases = join(tempDir(), "bases"); + const options = sandbox(`echo "$2" >> '${bases}'`); + expect(await serveBatch(forge.clone(), plan, options, deps, async () => deps.token)).toEqual([ `landed 1 ${forge.repo.pulls[0].squash}`, `landed 2 ${forge.repo.pulls[1].squash}`, - "ejected 3 tree_mismatch", ]); expect(forge.merges.map(({ number }) => number)).toEqual([1, 2]); + expect(warnings).toHaveLength(1); expect(warnings[0]).toContain("CRITICAL"); - expect(forge.comment(3)).toContain("tested chain"); + expect(warnings[0]).toContain("tested chain"); expect(forge.repo.pulls.slice(0, 2).every((pull) => pull.merged)).toBe(true); - expect(forge.repo.pulls[3].landing).toBe(true); + // PR 3 passed the check in the batch, and PR 4 was not in it: both stay queued, without a comment. + expect(forge.repo.pulls.slice(2).every((pull) => pull.landing)).toBe(true); + expect(forge.repo.pulls.every((pull) => !pull.comments?.length)).toBe(true); + + // The next round, based on the newest PR, checks and lands the rest on the base as it is, the mismatched squash included. + forge.mismatch = undefined; + forge.settle(); + const base = forge.main(); + expect(base).toBe(forge.repo.pulls[1].squash); + const next = await resolveQueue(REPOSITORY, undefined, deps); + expect(next.base.sha).toBe(base); + expect(await serveBatch(forge.clone(), next, options, deps, async () => deps.token)).toEqual([ + `landed 4 ${forge.repo.pulls[3].squash}`, + `landed 3 ${forge.repo.pulls[2].squash}`, + ]); + expect(readFileSync(bases, "utf8").trim().split("\n")).toEqual([plan.base.sha, base]); + expect(git(forge.origin, "rev-parse", `${forge.repo.pulls[3].squash}^`)).toBe(base); + expect(warnings).toHaveLength(1); + }); + + test("a prepared tree mismatch ejects only that PR, and the next round rebuilds the rest on the new base", async () => { + const forge = new Forge(); + // Both PRs edit the epic, so preparing PR 2 on PR 1's squash runs the quest merge driver. + forge.pull("Add b", { "quest/m0/README.md": EPIC(["a", "b"]), "quest/m0/b.md": QUEST("B") }); + forge.pull("Add c", { "quest/m0/README.md": EPIC(["a", "c"]), "quest/m0/c.md": QUEST("C") }); + forge.pull("Three", { "3.txt": "yes" }); + forge.pull("Four", { "4.txt": "yes" }); + const deps = await forge.deps(); + const warnings: string[] = []; + deps.warn = (message) => { + warnings.push(message); + }; + const plan = await resolveQueue(REPOSITORY, undefined, deps); + const pin = forge.repo.pulls[1].head; + // After the batch's check, the driver resolves differently than it did for the check. + const quest = join(process.env.PATH!.split(":")[0], "quest"); + forge.beforeMerge = (number) => { + if (number !== 1) return; + renameSync(quest, `${quest}.real`); + writeFileSync( + quest, + `#!/bin/sh\n'${quest}.real' "$@" || exit\nif [ "$1" = resolve ]; then echo drift >> "$2"; fi\n`, + { mode: 0o755 }, + ); + }; + const bases = join(tempDir(), "bases"); + const options = sandbox(`echo "$2" >> '${bases}'`); + expect(await serveBatch(forge.clone(), plan, options, deps, async () => deps.token)).toEqual([ + `landed 1 ${forge.repo.pulls[0].squash}`, + "ejected 2 tree_mismatch", + ]); + expect(forge.merges.map(({ number }) => number)).toEqual([1]); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("CRITICAL"); + expect(forge.comment(2)).toContain("differs from tested"); + expect(forge.comment(2)).toContain(""); + expect(forge.repo.pulls[1].head).toBe(pin); + expect(forge.repo.pulls[1].landing).toBeFalsy(); + // PR 3 passed the check in the batch, and PR 4 was not in it: both stay queued, without a comment. + expect(forge.repo.pulls.slice(2).every((pull) => pull.landing)).toBe(true); + expect(forge.comment(3)).toBeUndefined(); + expect(forge.comment(4)).toBeUndefined(); + + // The next round, based on the newest PR, leaves PR 2 out and checks and lands the rest on PR 1's squash. + renameSync(`${quest}.real`, quest); + forge.beforeMerge = undefined; + forge.settle(); + const base = forge.main(); + expect(base).toBe(forge.repo.pulls[0].squash); + const next = await resolveQueue(REPOSITORY, undefined, deps); + expect(next.queue.map(({ number }) => number)).toEqual([3, 4]); + expect(await serveBatch(forge.clone(), next, options, deps, async () => deps.token)).toEqual([ + `landed 4 ${forge.repo.pulls[3].squash}`, + `landed 3 ${forge.repo.pulls[2].squash}`, + ]); + expect(readFileSync(bases, "utf8").trim().split("\n")).toEqual([plan.base.sha, base]); + expect(git(forge.origin, "rev-parse", `${forge.repo.pulls[3].squash}^`)).toBe(base); }); test("scopes each merge token to that PR's squash, preserving it through the queue token cache", async () => {