From a49428faa19746813e9ff23df9c331185a7a2349 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 2 Nov 2026 18:54:40 -0800 Subject: [PATCH 1/2] chore(quest): claim quest/m0/land-fixer/protected-commit.md Quest-Claim: f897edf676c42097 From be49fcf6fd7a46b3649e95a271d425340f9d922b Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 2 Nov 2026 19:02:43 -0800 Subject: [PATCH 2/2] feat(land): hand back a fixer's protected edit as a ready commit When validateFix rejects a fixer result for editing protected paths beyond what the base merge carries, the handback now carries the validated resolution in the same form as a refused push, for `quest land apply`. The prompt lets the fixer make such an edit when the mechanical fix needs it, knowing quest land never pushes it. A fix that merges no base is carried as a patch on the PR head itself. Co-Authored-By: Claude Opus 5.5 --- README.md | 17 +++-- assets/land-fix.md | 15 ++-- quest/m0/host-land/fixer.md | 2 +- quest/m0/land-fixer/README.md | 5 +- quest/m0/land-fixer/lockfile.md | 9 +-- quest/m0/land-fixer/protected-commit.md | 30 -------- src/cli/land-apply.ts | 61 ++++++++++++---- src/cli/land-fixer.ts | 93 +++++++++++++++++-------- src/cli/land-watch-fix.ts | 13 +++- src/cli/main.ts | 7 +- test/land-watch.test.ts | 31 +++++++++ test/land.test.ts | 77 +++++++++++++++++++- 12 files changed, 260 insertions(+), 100 deletions(-) delete mode 100644 quest/m0/land-fixer/protected-commit.md diff --git a/README.md b/README.md index 31c9c53c..de739e45 100644 --- a/README.md +++ b/README.md @@ -625,12 +625,15 @@ Any other moved head hands back. Forks refusing maintainer edits, refused pushes crashes, or the fixer deadline hand back with a marked comment naming the exit status or signal, with the tail of the job's retained log. A validated fix whose push GitHub refuses, such as a merged base's `.github/workflows` changes that the App lacks -the `workflows` permission to push, is handed back with its resolution: the PR head, -the base, the resolved tree, and the fixer's edits as a patch against the merge's -conflicted result, sized to GitHub's 65,536-character comment limit. A maintainer +the `workflows` permission to push, is handed back with its resolution. So is a fix +that edits protected paths beyond what the base merge carries, such as a conflict in +`flake.nix` the fixer resolved: those never land automatically. The resolution is the +PR head, the base, the resolved tree, and the fixer's edits as a patch against the +merge's conflicted result, or against the head itself for a fix that merges no base, +sized to GitHub's 65,536-character comment limit. A maintainer runs `quest land apply `, which refuses a moved head, rebuilds and checks that -tree, and prints an unpushed commit and its push command. A resolution too large for -the comment hands back plainly. A handback disables +tree, and prints an unpushed commit and its push command to run after reviewing it. +A resolution too large for the comment hands back plainly. A handback disables auto-merge and carries the reason marker `quest status` reads. Each failure a fixer job blames a PR for is a round of its maintainer pick; a PR that only shared a broken batch has none. The fixer keeps going while each round @@ -818,8 +821,8 @@ base it conflicted with. The pick keeps going while each round makes progress: a round with no new commit for a failure that isn't flaky, the failure a fix addressed recurring right after it, or three flaky rounds in a row hand it back. A fix is validated, lease-pushed, and recorded in Review as for the host -gate, and a push GitHub refuses is handed back with its resolution for -`quest land apply`. The watcher then puts the fixed, merged, or flaky head back +gate, and a push GitHub refuses, or a fix that edits protected paths, is handed +back with its resolution for `quest land apply`. The watcher then puts the fixed, merged, or flaky head back in the queue with `expectedHeadOid` bound to it: it enqueues a head whose required checks have passed, and arms auto-merge on one whose checks still run, so GitHub enqueues it once they pass. Either joins the back of the queue. diff --git a/assets/land-fix.md b/assets/land-fix.md index 60ca5033..4c0263d0 100644 --- a/assets/land-fix.md +++ b/assets/land-fix.md @@ -29,11 +29,16 @@ flaky test in its `failure` so it can be followed up), commit a mechanical fix on its own branch, or hand it back with diagnosis and recommendation. Allowed fixes: preserve both sides' intent in conflicts, format/lint, regenerate lockfiles/generated files, or follow a base rename. Merge base where practical. -Never edit flake.nix, flake.lock, shell.nix, default.nix, nix/, .envrc, .github/, -.gitmodules, or gitlinks (submodule entries) anywhere. Merging the base may carry -the base's own changes to those paths: that is allowed when they come out exactly as -a plain `git merge` of the base leaves them, with no conflict there for you to edit. Never push or contact -GitHub. Anything needing behavior changes or review must be handed back. +flake.nix, flake.lock, shell.nix, default.nix, nix/, .envrc, .github/, .gitmodules, +and gitlinks (submodule entries) anywhere are protected. Merging the base may carry +the base's own changes to those paths: that is no edit of yours when they come out +exactly as a plain `git merge` of the base leaves them, with no conflict there for you +to edit. Prefer a fix that leaves them alone. When the mechanical fix itself must +edit one, such as a conflict there that a resolution preserving both sides settles, +make it and report it `fixed`, naming the protected paths it edits in the reason: +quest land never pushes it, but hands the PR back with your commit ready for a +maintainer to review and apply. Never push or contact GitHub. Anything needing +behavior changes or review must be handed back. Build tools may fetch the dependencies the repository's manifests and lockfiles name: from their package registries, such as crates.io, npm, PyPI, and the Go diff --git a/quest/m0/host-land/fixer.md b/quest/m0/host-land/fixer.md index 02081866..917bfe67 100644 --- a/quest/m0/host-land/fixer.md +++ b/quest/m0/host-land/fixer.md @@ -14,7 +14,7 @@ Maintainer decision, 2026-10-30: a `fixed` head lands without further review. prompt-injection risk is accepted. Protected paths stay blocked: on 2026-11-01 the maintainer briefly let the fixer edit them (moq-dev/quest#357), then reverted that after an audit, routing fixer-written protected edits through a ready commit -instead ([Protected edits as ready commits](/quest/m0/land-fixer/protected-commit.md)). The built-in prompt treats contributor content +instead ([moq-dev/quest#409](https://github.com/moq-dev/quest/pull/409)). The built-in prompt treats contributor content as untrusted and ignores instructions in the PRs' diffs, without handing the PR back for them. diff --git a/quest/m0/land-fixer/README.md b/quest/m0/land-fixer/README.md index b539ce2e..e8286e70 100644 --- a/quest/m0/land-fixer/README.md +++ b/quest/m0/land-fixer/README.md @@ -39,7 +39,7 @@ moq-dev/rust2, moq-dev/rind and moq-dev/quest. | 98 | "already used its mechanical fix attempt" after a collateral requeue or an earlier conflict fix; each needed a manual re-pick | stop rules ([moq-dev/quest#384](https://github.com/moq-dev/quest/pull/384)), batch triage ([moq-dev/quest#355](https://github.com/moq-dev/quest/pull/355)) | | ~75 | "requeued unchanged": collateral of a batch another PR broke | batch triage ([moq-dev/quest#355](https://github.com/moq-dev/quest/pull/355)) | | 36 | picked again while fixing; the newer pick is kept, so nothing is lost | not planned | -| 17 | conflicts in protected paths (`flake.nix`, `.github/`), mostly mechanical; resolved by hand on moq-dev/moq#7344, #7315, #7318 | blocked by design; fixer-written protected edits become a ready commit ([Protected edits as ready commits](/quest/m0/land-fixer/protected-commit.md)) | +| 17 | conflicts in protected paths (`flake.nix`, `.github/`), mostly mechanical; resolved by hand on moq-dev/moq#7344, #7315, #7318 | kept blocked; a fixer-written protected edit is handed back with a ready commit for `quest land apply` by [moq-dev/quest#409](https://github.com/moq-dev/quest/pull/409) | | 17 of the last 40 jobs | `cargo` fails offline ("failed to get `uniffi`"), so Rust fixes were requeued uncompiled: moq-dev/moq#7357 sank three batches, and #6985's lockfile could not be regenerated | fixed by [moq-dev/quest#356](https://github.com/moq-dev/quest/pull/356): the prompt allows dependency fetches and requires a build and test | | 2 | GitHub refused the push to a fork because the merge touches workflows: moq-dev/moq#7439, #7467 | handed back with a ready commit for `quest land apply` by [moq-dev/quest#358](https://github.com/moq-dev/quest/pull/358) | | several batches | a flaky ffmpeg timeout ([moq-dev/moq#7480](https://github.com/moq-dev/moq/pull/7480)) ejected whole batches | [Flaky failures](/quest/m0/queue-watcher/flaky.md) | @@ -111,7 +111,7 @@ Decision prompts, selected options marked ✅: PRs broken against the current base, and not fixer capability. Decisions: - Protected paths: ✅ keep `validateFix`'s block and route fixer-written protected edits through moq-dev/quest#358's ready commit - ([Protected edits as ready commits](/quest/m0/land-fixer/protected-commit.md)); moq-dev/quest#357 closed · keep #357 · #357 minus workflows + ([moq-dev/quest#409](https://github.com/moq-dev/quest/pull/409)); moq-dev/quest#357 closed · keep #357 · #357 minus workflows - Fixer comments: ✅ the daemon posts them from the fixer's result, no token in the container (moq-bot comments carry trust: `quest land apply`, the daemon's markers, `quest status`) · keep the token @@ -153,7 +153,6 @@ work, done once the Required quests below have merged. - [Workflow permission](/quest/m0/land-fixer/workflows-permission.md) - waits on the maintainer to grant moq-bot `workflows`, so pushes touching workflows are accepted - [Lockfile merges](/quest/m0/land-fixer/lockfile.md) - a conflicted lockfile is rebuilt from the base's copy, so a fix never reverts the base's dependency changes -- [Protected edits as ready commits](/quest/m0/land-fixer/protected-commit.md) - a fixer edit `validateFix` rejects for touching protected paths comes back as a commit the maintainer applies - [PR CI as a signal](/quest/m0/land-fixer/pr-signal.md) - `/quest-merge` and the fixer read a PR's own CI without gating on it; public repos may run it, private ones need not ## Related diff --git a/quest/m0/land-fixer/lockfile.md b/quest/m0/land-fixer/lockfile.md index f37fd547..f09b7ef7 100644 --- a/quest/m0/land-fixer/lockfile.md +++ b/quest/m0/land-fixer/lockfile.md @@ -56,14 +56,9 @@ The prompt already lets build tools fetch from package registries (from regeneration needs. `flake.lock` stays a protected path in `validateFix`, and this quest does not open it: a fixer-written `flake.lock` comes back as a ready commit through -[Protected edits as ready commits](/quest/m0/land-fixer/protected-commit.md), +[moq-dev/quest#409](https://github.com/moq-dev/quest/pull/409), and that edit follows the same rule, rebuilt from the base's copy with -`nix flake lock`. Both quests edit `assets/land-fix.md`, so whichever lands -second merges the other's wording. +`nix flake lock`. That PR also edited `assets/land-fix.md`: merge its wording. Cover the rule in `test/land.test.ts`, which already asserts the prompt's contents. - -## Related - -- [Protected edits as ready commits](/quest/m0/land-fixer/protected-commit.md) - returns a fixer-written `flake.lock` as a ready commit, built by this quest's rule diff --git a/quest/m0/land-fixer/protected-commit.md b/quest/m0/land-fixer/protected-commit.md deleted file mode 100644 index 729b1fce..00000000 --- a/quest/m0/land-fixer/protected-commit.md +++ /dev/null @@ -1,30 +0,0 @@ -# [S] A protected edit the fixer writes comes back as a ready commit - -## Goal - -When `validateFix` rejects a fixer result because it edits a protected path -(`.github/`, `nix/`, `flake.nix`, `flake.lock`, `shell.nix`, `default.nix`, -`.envrc`, `.gitmodules`, and gitlinks, as `validateFix` in -`src/cli/land-fixer.ts` lists them) beyond what the base -merge carries, the handback carries that result as a ready commit, the same -form as a fork push GitHub refuses -([moq-dev/quest#358](https://github.com/moq-dev/quest/pull/358)). -The maintainer reviews it and applies it with `quest land apply `, then -pushes and re-picks. Protected edits never land automatically. - -## Plan - -Decided 2026-11-01 (maintainer, from the audit of the merge-queue plan; see -the [epic](/quest/m0/land-fixer/README.md)). The maintainer kept -`validateFix`'s protected-path block and closed -[moq-dev/quest#357](https://github.com/moq-dev/quest/pull/357), which would -have let these edits land unreviewed. moq-dev/moq has about 25 release -workflows with publishing secrets, and a merge pick covers the PR's diff, -not the fixer's later resolution. About 17 such handbacks happened in 3 days, -most of them mechanical, so a one-command ready commit keeps the human review -cheap. - -Reuse [moq-dev/quest#358](https://github.com/moq-dev/quest/pull/358)'s -handback format and `quest land apply`. Record the PR head, base, expected -tree hash, and the patch against the merge's conflicted result. Size it to -GitHub's comment limit, and hand back plainly when it does not fit. diff --git a/src/cli/land-apply.ts b/src/cli/land-apply.ts index 2003b19e..a1b746b6 100644 --- a/src/cli/land-apply.ts +++ b/src/cli/land-apply.ts @@ -1,4 +1,4 @@ -// A fixer resolution GitHub refused to publish travels in the handback comment, and a maintainer's checkout rebuilds it. +// A fixer resolution the daemon cannot publish travels in the handback comment, and a maintainer's checkout rebuilds it. import { spawnSync } from "node:child_process"; import { rmSync } from "node:fs"; import { resolve } from "node:path"; @@ -20,13 +20,19 @@ export const COMMENT_LIMIT = 65_536; /** * A validated fix as data: the PR head and base it merges, the tree it must produce, and the fixer's edits as a - * gzipped, base64 patch against the merge's conflicted result, so it stays as small as the resolution itself. + * gzipped, base64 patch against the merge's conflicted result, so it stays as small as the resolution itself. A fix + * that merges no base names the head as its base, so its patch applies to the head's own tree. */ export interface Resolution { head: string; base: string; tree: string; patch: string; + /** + * Why the daemon handed it back rather than pushing it: GitHub refused the push, or the fix edits protected + * paths, which only a maintainer's push lands. The comment says it in prose; its data carries no cause. + */ + cause?: "protected"; } const SHA = "[0-9a-f]{40}(?:[0-9a-f]{24})?"; @@ -83,9 +89,24 @@ function bytes(root: string, args: string[], input?: Buffer, env: Record git(root, ["rev-list", "--count", fixed, ...not.map((ref) => `^${ref}`)]); + if (count(head) !== count(head, base)) return undefined; + base = head; + } const patch = bytes(root, [ "diff", "--binary", @@ -100,16 +121,28 @@ export function resolution(root: string, head: string, base: string, fixed: stri conflicted(root, head, base), fixed, ]); - return { head, base, tree: git(root, ["rev-parse", `${fixed}^{tree}`]), patch: gzipSync(patch).toString("base64") }; + return { + head, + base, + tree: git(root, ["rev-parse", `${fixed}^{tree}`]), + patch: gzipSync(patch).toString("base64"), + ...(cause ? { cause } : {}), + }; } /** The handback's section carrying `resolution`, or a plain handback when it would not fit in `room` characters. */ export function resolutionSection(number: number, resolution: Resolution, room: number): string { - const { head, base, tree, patch } = resolution; + const { head, base, tree, patch, cause } = resolution; + const why = + cause === "protected" + ? "The fixer's resolution edits protected paths beyond what the base merge carries, and only a maintainer's push lands those" + : "GitHub refused the fixer's push"; + const merge = base === head ? "no base merge" : `base \`${base}\``; + const review = cause === "protected" ? "reviews its protected edits" : "reviews it"; const text = - `\n\nGitHub refused the fixer's push, so its validated resolution travels in this comment: PR head \`${head}\`, ` + - `base \`${base}\`, resolved tree \`${tree}\`. A maintainer session rebuilds the commit with ` + - `\`quest land apply ${number}\`, reviews it, pushes it to the PR branch, and picks the PR again.` + + `\n\n${why}, so its validated resolution travels in this comment: PR head \`${head}\`, ` + + `${merge}, resolved tree \`${tree}\`. A maintainer session rebuilds the commit with ` + + `\`quest land apply ${number}\`, ${review}, pushes it to the PR branch, and picks the PR again.` + `\n\n`; if (text.length <= room) return text; return `\n\nThe fixer's resolution is too large for a comment (${patch.length} characters encoded), so redo the merge by hand.`; @@ -167,7 +200,7 @@ export interface Applied { } /** - * Rebuild the commit a quest land fixer handed back because GitHub refused its push: the latest resolution an App + * Rebuild the commit a quest land fixer handed back because GitHub refused its push or it edits protected paths: the latest resolution an App * posted on the PR, while the PR's head is still the one it resolved. The commit is left unpushed. */ export async function apply(root: string, pr: PullRequest, deps: GitHubDependencies): Promise { @@ -212,10 +245,14 @@ export async function apply(root: string, pr: PullRequest, deps: GitHubDependenc const tree = rebuild(root, found); const parents = contains(root, found.base, found.head) ? [found.head] : [found.head, found.base]; const branch = string(head.ref); + const subject = + found.base === found.head + ? `Apply the quest land fixer's fix to ${branch}` + : `Merge ${found.base} into ${branch}`; const commit = userGit( root, ["commit-tree", tree, ...parents.flatMap((parent) => ["-p", parent])], - `Merge ${found.base} into ${branch}\n\nThe quest land fixer's resolution, rebuilt by \`quest land apply\` after GitHub refused the fixer's push.\n`, + `${subject}\n\nThe quest land fixer's resolution, rebuilt by \`quest land apply\` after the daemon handed it back unpushed.\n`, ); const holder = object(head.repo); return { diff --git a/src/cli/land-fixer.ts b/src/cli/land-fixer.ts index 75ec65ee..a434c301 100644 --- a/src/cli/land-fixer.ts +++ b/src/cli/land-fixer.ts @@ -38,8 +38,8 @@ import { isParked, parked } from "./land-park"; import { excerpt, type Failure } from "./land-serve"; /** - * Why a fixer's result cannot be published, which hands its PR back. A push GitHub refused carries the - * validated resolution, for `quest land apply` to rebuild. + * Why a fixer's result cannot be published, which hands its PR back. A push GitHub refused, or a fix + * that edits protected paths, carries the validated resolution, for `quest land apply` to rebuild. */ export class FixerRefusal extends Error { constructor( @@ -50,6 +50,8 @@ export class FixerRefusal extends Error { } } class BaseCheckError extends Error {} +/** A fix that edits protected paths beyond what the base merge carries: only a maintainer's push lands it. */ +export class ProtectedEdit extends Error {} /** The failure log's lines the fixer cited, first to last; the comment bounds them. */ export function cited(log: string, [first, last]: [number, number]): string { @@ -644,7 +646,7 @@ export function validateFix(root: string, entry: Pick, head: stri const sets = maps.map((list) => list.filter(protectedEntry).sort().join("\0")); const protectedConflict = conflicts.some((path) => protectedPath(path) || gitlinks.has(path)); if (sets[0] !== sets[1] && (protectedConflict || sets[0] !== sets[2])) - throw new Error("off-limits fixer edit: protected set must equal pinned head or base merge result"); + throw new ProtectedEdit("off-limits fixer edit: protected set must equal pinned head or base merge result"); } /** Anything the fixer writes may echo a token it read; GitHub tokens never reach a post. */ @@ -701,11 +703,36 @@ function review(body: string, head: string, reason: string, attribution: string) return `${body.slice(0, start)}\n\n${line}${body.slice(start)}`; } +/** + * The ready commit a handback carries for a validated `head` the daemon cannot publish on `entry`'s PR, + * while the PR stands at its pinned head: `quest land apply` rebuilds only on the head the fixer worked + * from. + */ +function ready( + root: string, + plan: Pick, + entry: Pick, + standing: string, + head: string, + warn: (message: string) => void, + cause?: Resolution["cause"], +): Resolution | undefined { + if (standing !== entry.head) return undefined; + try { + return resolution(root, entry.head, plan.base.sha, head, cause); + } catch (error) { + warn(`quest land fixer: cannot hand back #${entry.number}'s resolution: ${String(error)}`); + return undefined; + } +} + /** * Check a fixer's `fixed` head before publishing it on `entry`'s PR: a fork must allow maintainer * edits, the head's objects are imported from the fixer's `checkout`, and it must descend from the * pinned head and keep the protected paths, judged against the base the fixer was given rather than - * the current one, since other PRs may land protected changes while the result waits. + * the current one, since other PRs may land protected changes while the result waits. A fix that edits + * them is refused with its resolution, for a maintainer to review and push with `quest land apply`, + * when the PR stands at `entry`'s pinned head. */ export async function importFix( root: string, @@ -716,6 +743,8 @@ export async function importFix( packs: string[] | undefined, head: string, read: RemoteRead, + standing: string, + warn: (message: string) => void, ): Promise { if (entry.repository !== plan.repository && !entry.maintainerCanModify) throw new FixerRefusal("fork does not allow maintainer edits"); @@ -728,7 +757,10 @@ export async function importFix( try { validateFix(root, entry, head, plan.base.sha); } catch (error) { - throw new FixerRefusal(String(error)); + throw new FixerRefusal( + String(error), + error instanceof ProtectedEdit ? ready(root, plan, entry, standing, head, warn, "protected") : undefined, + ); } } @@ -763,15 +795,11 @@ export async function publishHead( ); if (pushed.status !== 0) { // GitHub refuses an App's push of a workflow change it lacks the permission for, and a fork's - // branch rules may refuse any. The maintainer's own push may still pass, but `quest land apply` - // rebuilds only on the head the fixer worked from. - let ready: Resolution | undefined; - try { - ready = standing === entry.head ? resolution(root, entry.head, plan.base.sha, head) : undefined; - } catch (error) { - deps.warn(`quest land fixer: cannot hand back #${entry.number}'s resolution: ${String(error)}`); - } - throw new FixerRefusal(`fixer push refused: ${pushed.stderr.trim()}`, ready); + // branch rules may refuse any. The maintainer's own push may still pass. + throw new FixerRefusal( + `fixer push refused: ${pushed.stderr.trim()}`, + ready(root, plan, entry, standing, head, deps.warn), + ); } } // GitHub shows a push a moment later. Wait for our own head; only a third head is a real move. @@ -978,26 +1006,30 @@ async function collectJob( } else if (!error && result?.action !== "handback") { try { if (result?.action === "fixed") - await importFix(root, state, job.plan, entry, job.checkout, job.packs, head, read); + await importFix( + root, + state, + job.plan, + entry, + job.checkout, + job.packs, + head, + read, + standing, + deps.warn, + ); if (!job.published.includes(number)) { job.published.push(number); save(path, job); } - if (result?.action === "fixed") { - try { - // The fix descends from the pin, not from a gate merge the PR stands at: it replaces that - // merge, as the gate replaces its own, since building on it would revert the newer base it - // merged. The lease still refuses any writer's commit. - await publishHead(github, root, job.plan, entry, standing, current, head, deps, token, { - reason: result.reason, - attribution: author(result), - }); - } catch (failure) { - if (failure instanceof FixerRefusal && failure.resolution) - (job.resolutions ??= {})[number] = failure.resolution; - throw failure; - } - } + // The fix descends from the pin, not from a gate merge the PR stands at: it replaces that + // merge, as the gate replaces its own, since building on it would revert the newer base it + // merged. The lease still refuses any writer's commit. + if (result?.action === "fixed") + await publishHead(github, root, job.plan, entry, standing, current, head, deps, token, { + reason: result.reason, + attribution: author(result), + }); // A parked PR requeued unchanged is still queued as it was; anything else is pinned anew. if (queuedAs !== entry.enabledAt || head !== entry.head) { @@ -1013,6 +1045,7 @@ async function collectJob( } } catch (failure) { if (!(failure instanceof FixerRefusal)) throw failure; + if (failure.resolution) (job.resolutions ??= {})[number] = failure.resolution; error = `${String(failure)}; run /quest-iterate before picking it again`; } } diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index ea28f0a5..ebe5040b 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -498,7 +498,18 @@ async function collect( if (entry.repository !== job.repository && !entry.maintainerCanModify) throw new FixerRefusal("fork does not allow maintainer edits"); } else - await importFix(context.root, state, job, entry, job.checkout, job.packs, head, context.read); + await importFix( + context.root, + state, + job, + entry, + job.checkout, + job.packs, + head, + context.read, + entry.head, + deps.warn, + ); job.published = head; save(path, job); } diff --git a/src/cli/main.ts b/src/cli/main.ts index e616d616..e61eb358 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -599,8 +599,9 @@ export function main(argv: string[], out: Output): number | Promise { landCommand .command("apply") .description( - "Rebuild the commit a quest land fixer handed back because GitHub refused its push, such as a merge of a " + - "base that changed .github/workflows, which the App lacks the permission to push. Reads the latest " + + "Rebuild the commit a quest land fixer handed back unpushed: GitHub refused its push, such as a merge of a " + + "base that changed .github/workflows, which the App lacks the permission to push, or it edits protected " + + "paths, which only a maintainer's push lands. Reads the latest " + "resolution an App posted on the PR and refuses (exit 3) when the PR's head is no longer the one it " + "resolved. Fetches that head and base, recomputes the merge's conflicted result as the daemon did, " + "applies the fixer's patch, and checks the resolved tree before committing with your identity. Prints the commit and the push command; " + @@ -616,7 +617,7 @@ export function main(argv: string[], out: Output): number | Promise { landCommand .command("serve") .description( - "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. 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, or a fix that edits protected paths beyond the base merge, 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. 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-watch.test.ts b/test/land-watch.test.ts index 3cb0c5ec..def17c63 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -10,6 +10,7 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; import type { GitHubDependencies } from "../src/cli/github"; import { readRemote } from "../src/cli/land"; +import { parseResolution } from "../src/cli/land-apply"; import { FLAKY_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"; @@ -858,6 +859,36 @@ describe.skipIf(process.platform === "win32")("the watcher's land fixer", () => expect(history(state, 5)!.rounds[1].rule).toBe("repeated"); }); + test("a fix that edits a protected path is handed back unpushed, as a ready commit", async () => { + const { repo, state, fix, context } = setup(); + const base = repo.main(); + const head = repo.pull(5, { "feature.txt": "new\n" }); + const pull: Pull = { + number: 5, + head, + enqueues: [{ createdAt: at(1), actor: "maintainer" }], + removals: [ + { id: "R1", createdAt: at(5), reason: "failed_checks", actor: "github-merge-queue", before: head }, + ], + }; + const github = { ...world([pull], [failedRun(5, at(3), base)]), origin: repo.origin }; + github.now = Date.parse(at(6)); + fix.round(5, fixScript("flake.nix")); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + "removed 5 failed_checks fix", + "fixing 5", + ]); + await exited(state); + github.now = Date.parse(at(10)); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual(["handback 5"]); + expect(git(repo.origin, "rev-parse", "refs/heads/branch-5")).toBe(head); + expect(github.mutations).toEqual([]); + const handback = github.comments[5].at(-1)!.body; + expect(handback).toContain("off-limits fixer edit"); + expect(handback).toContain("`quest land apply 5`"); + expect(parseResolution(handback)).toMatchObject({ head, base: head }); + }); + test("a quest-file conflict is merged with the quest driver, and a code conflict goes to the fixer", async () => { const { repo, state, fix, context } = setup(1); const quests = repo.pull(3, { diff --git a/test/land.test.ts b/test/land.test.ts index 6cde2067..cc7b8e65 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -2370,6 +2370,8 @@ async function fixerFixture( protectedBase?: boolean; // The base the fixer merges already changed a workflow. workflowBase?: boolean; + // The PR and the base change `flake.nix` apart, so the merge conflicts there. + protectedConflict?: boolean; // A line the wrapper prints before the agent starts, as the host's does when its launcher admits the job. admitted?: string; } = {}, @@ -2377,13 +2379,14 @@ async function fixerFixture( const forge = new Forge(); const pull = forge.pull( "Fix me", - { "code.txt": "one\nII\nthree\n" }, + { "code.txt": "one\nII\nthree\n", ...(options.protectedConflict ? { "flake.nix": "pr\n" } : {}) }, options.fork ? { contributor: "outside", edits: false } : {}, ); const collateral = options.collateral ? forge.pull("Bystander", { "other.txt": "fine\n" }) : undefined; if (options.conflict) forge.advance({ "code.txt": "one\n2\nthree\n" }); if (options.protectedBase) forge.advance({ "flake.nix": "base\n" }); if (options.workflowBase) forge.advance({ ".github/workflows/alert.yml": "on: push\n" }); + if (options.protectedConflict) forge.advance({ "flake.nix": "base\n" }); const deps = await forge.deps(); const plan = await resolveQueue(REPOSITORY, undefined, deps); const state = sandbox(); @@ -3766,6 +3769,63 @@ describe.skipIf(process.platform === "win32")("quest land apply", () => { expect(fixture.forge.ref("upstream/project", "refs/heads/topic-1")).toBe(applied.commit); }); + test("hands back a fixer's resolution of a protected conflict as a ready commit, never pushing it", async () => { + const fixture = await fixerFixture( + `git checkout -q pr-1 +git merge -q base || true +printf 'pr and base\n' > flake.nix +git add flake.nix +git commit -qm 'preserve both sides of flake.nix' +printf '[{"number":1,"action":"fixed","head":"%s","reason":"preserve both sides; edits flake.nix"}]' "$(git rev-parse HEAD)" > quest-land-result.json`, + { protectedConflict: true }, + ); + await fixture.finished; + expect(await fixture.collect()).toEqual(["handback 1"]); + expect(fixture.pull.head).toBe(fixture.plan.queue[0].head); + expect(fixture.pull.landing).toBe(false); + const comment = fixture.forge.comment(1)!; + expect(comment).toContain("off-limits fixer edit"); + expect(comment).toContain("edits protected paths beyond what the base merge carries"); + expect(comment).toContain("reviews its protected edits"); + const resolution = parseResolution(comment)!; + expect(resolution).toMatchObject({ head: fixture.plan.queue[0].head, base: fixture.plan.base.sha }); + + const checkout = fixture.forge.clone(); + const applied = await apply(checkout, PR, await fixture.forge.deps()); + expect(git(checkout, "show", "-s", "--format=%P", applied.commit)).toBe( + `${resolution.head} ${resolution.base}`, + ); + expect(git(checkout, "show", `${applied.commit}:flake.nix`)).toBe("pr and base"); + expect(git(checkout, "show", `${applied.commit}:code.txt`)).toBe("one\nII\nthree"); + }); + + test("hands back a protected fix that merges no base as a commit on the PR head", async () => { + // The base moved on, and the fix leaves it unmerged. + const fixture = await fixerFixture( + `git checkout -q pr-1 +printf 'pinned\n' > flake.nix +git add flake.nix +git commit -qm 'pin the toolchain' +printf '[{"number":1,"action":"fixed","head":"%s","reason":"pin the toolchain; edits flake.nix"}]' "$(git rev-parse HEAD)" > quest-land-result.json`, + { conflict: true }, + ); + await fixture.finished; + expect(await fixture.collect()).toEqual(["handback 1"]); + expect(fixture.pull.head).toBe(fixture.plan.queue[0].head); + const comment = fixture.forge.comment(1)!; + expect(comment).toContain("no base merge"); + const resolution = parseResolution(comment)!; + expect(resolution).toMatchObject({ head: fixture.plan.queue[0].head, base: fixture.plan.queue[0].head }); + + const checkout = fixture.forge.clone(); + const applied = await apply(checkout, PR, await fixture.forge.deps()); + expect(git(checkout, "show", "-s", "--format=%P %s", applied.commit)).toBe( + `${resolution.head} Apply the quest land fixer's fix to topic-1`, + ); + expect(git(checkout, "show", `${applied.commit}:flake.nix`)).toBe("pinned"); + expect(git(checkout, "rev-parse", `${applied.commit}^{tree}`)).toBe(resolution.tree); + }); + test("refuses a PR whose head moved since the handback", async () => { const fixture = await refused(); git(fixture.forge.origin, "update-ref", "refs/heads/topic-1", fixture.plan.base.sha); @@ -3807,6 +3867,21 @@ describe.skipIf(process.platform === "win32")("quest land apply", () => { expect(parseResolution(plain)).toBeUndefined(); expect(plain).toContain("too large for a comment"); }); + + test("says why a resolution was handed back, and when it merges no base", () => { + const head = "a".repeat(40); + const pushed = resolutionSection(1, { head, base: "b".repeat(40), tree: "c".repeat(40), patch: "" }, 1_000); + expect(pushed).toContain("GitHub refused the fixer's push"); + expect(pushed).toContain(`base \`${"b".repeat(40)}\``); + const edited = resolutionSection( + 1, + { head, base: head, tree: "c".repeat(40), patch: "", cause: "protected" }, + 1_000, + ); + expect(edited).toContain("edits protected paths"); + expect(edited).toContain("no base merge"); + expect(parseResolution(edited)).toEqual({ head, base: head, tree: "c".repeat(40), patch: "" }); + }); }); describe("land fixer stop rules", () => {