From 023a04fa0ec1d795b79ecf941297f8eeca2e930d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 05:19:56 -0800 Subject: [PATCH 1/3] chore(quest): claim quest/m0/land-fixer/fork-commit.md Quest-Claim: 9ea8bdab7d593b4d From b89cc078e0fb94543ff87467548388732713d67b Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 05:30:06 -0800 Subject: [PATCH 2/3] feat(land): hand back a refused fix push as a ready commit When GitHub refuses the fixer's push of a validated fix, such as a merged base's workflow changes the App lacks the `workflows` permission for, the handback comment carries the resolution: the PR head, the base, the resolved tree, and the fixer's edits as a gzipped patch against the merge's conflicted result, sized to GitHub's comment limit. `quest land apply ` refuses a moved head, recomputes that conflicted result with pinned merge settings, applies the patch, checks the tree, and prints an unpushed commit with its push command. Co-Authored-By: Claude Opus 5.5 --- CONTRIBUTING.md | 4 +- README.md | 9 +- quest/m0/land-fixer/README.md | 6 +- quest/m0/land-fixer/fork-commit.md | 38 ------ quest/m0/land-fixer/protected.md | 4 - src/cli/land-apply.ts | 212 +++++++++++++++++++++++++++++ src/cli/land-fixer.ts | 28 +++- src/cli/land.ts | 18 ++- src/cli/main.ts | 22 ++- test/land.test.ts | 96 +++++++++++++ 10 files changed, 376 insertions(+), 61 deletions(-) delete mode 100644 quest/m0/land-fixer/fork-commit.md create mode 100644 src/cli/land-apply.ts diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 75fb5e77..2a84acbc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -60,7 +60,9 @@ remaining batch. Transient refusals are retried; a racing base rebuilds the queu 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 -head in Review, and requeues healthy PRs; other results are handed back. The admin bypass of `main`'s +head in Review, and requeues healthy PRs; other results are handed back. A fix +whose push GitHub refuses is handed back with its resolution, which +`quest land apply ` rebuilds as a commit for a maintainer to push. The admin bypass of `main`'s ruleset is for emergencies. Every AI-authored GitHub post must end with `(written by )`, naming the diff --git a/README.md b/README.md index ed9557bd..9cd945f6 100644 --- a/README.md +++ b/README.md @@ -540,7 +540,14 @@ checks every object and link, since the import enables `fetch.fsckObjects=true`. `## Review`, enables head-pinned auto-merge, and adds `quest:merge`. Healthy PRs requeue unchanged. Forks refusing maintainer edits, refused pushes, invalid results, crashes, or the 60-minute fixer deadline hand back with a marked comment naming the exit -status or signal, with the tail of the job's retained log. A flaky +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 +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 flaky requeue suggests a follow-up. Each maintainer pick gets one attempt; the daemon records the fixer's new auto-merge `enabledAt` so its second ejection hands back. A new maintainer pick resets the attempt. Host provisioning and isolation diff --git a/quest/m0/land-fixer/README.md b/quest/m0/land-fixer/README.md index 73786284..3d5e9a9b 100644 --- a/quest/m0/land-fixer/README.md +++ b/quest/m0/land-fixer/README.md @@ -35,7 +35,7 @@ moq-dev/rust2, moq-dev/rind and moq-dev/quest. | 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 | [Protected edits](/quest/m0/land-fixer/protected.md) | | 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 | [Registry network](/quest/m0/land-fixer/network.md) | -| 2 | GitHub refused the push to a fork because the merge touches workflows: moq-dev/moq#7439, #7467 | [Fork commits](/quest/m0/land-fixer/fork-commit.md) | +| 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 tests](/quest/m0/land-fixer/flaky.md) | | ~15 | design conflicts, correctly handed back | none | @@ -50,7 +50,8 @@ Decision prompts, selected options marked ✅: - How attempts count: see [Attempt budget](/quest/m0/land-fixer/attempts.md). - How the fixer gets dependencies: see [Registry network](/quest/m0/land-fixer/network.md). - Protected paths: see [Protected edits](/quest/m0/land-fixer/protected.md). -- Fork pushes GitHub refuses: see [Fork commits](/quest/m0/land-fixer/fork-commit.md). +- Fork pushes GitHub refuses: ✅ hand back a ready commit, which `quest land apply` rebuilds + for a maintainer to push (Grant moq-bot the `workflows` permission · Leave it manual). - Flaky tests: see [Flaky tests](/quest/m0/land-fixer/flaky.md). - Milestone for the new quests: ✅ m0 · m1 - The user, starting the work: "The fixer is meant to handle conflicts and @@ -72,7 +73,6 @@ resolves itself and what it hands back; that docs update finishes the epic. - [Flaky tests](/quest/m0/land-fixer/flaky.md) - the gate reruns a failed module, disables a flaky test in the next batch, and files a follow-up - [Local mergeability](/quest/m0/land-fixer/mergeability.md) - `quest land` merges with the quest driver instead of ejecting on GitHub's conflict flag - [Protected edits](/quest/m0/land-fixer/protected.md) - the fixer edits protected paths when it judges the edit mechanical, and the fix lands -- [Fork commits](/quest/m0/land-fixer/fork-commit.md) - a fork push GitHub refuses hands back a commit a maintainer session applies as is ## Related diff --git a/quest/m0/land-fixer/fork-commit.md b/quest/m0/land-fixer/fork-commit.md deleted file mode 100644 index accf2d79..00000000 --- a/quest/m0/land-fixer/fork-commit.md +++ /dev/null @@ -1,38 +0,0 @@ -# [S] A refused fork push hands back a ready commit - -## Goal - -When GitHub refuses the fixer's push to a fork because the commit touches -`.github/workflows` (the App has no `workflows` permission), the handback -carries the fixer's validated resolution. A maintainer session applies and -pushes it without redoing the merge. - -## Plan - -Decided 2026-11-01 (user, /quest-plan of the land fixer audit; see the -[epic](/quest/m0/land-fixer/README.md)). On -[moq-dev/moq#7439](https://github.com/moq-dev/moq/pull/7439) and -[#7467](https://github.com/moq-dev/moq/pull/7467) the fixer's merge of the -base brought in the base's workflow changes, and GitHub refused the push. A -maintainer session recovered #7439's commit from the daemon's -`repository.git` through a temporary ref and a bundle over SSH. - -- What happens on a refused fork push: - - Grant moq-bot the `workflows` permission - - ✅ Hand back a ready commit - - Leave it manual - -In any repository where the App lacks the `workflows` permission, GitHub -refuses the App's push of such a commit, so the App cannot publish it on a -fork or an upstream ref. The commit itself can still travel through Git, as -the bundle recovery showed. The handback comment records the PR head SHA, -the base SHA, the expected tree hash, and the fixer's resolution as a patch -against the merge's conflicted result. A command such as `quest land apply -` stops if the PR's head is no longer the recorded one, then merges that -base, applies the patch, checks the tree hash, and leaves a commit for the -maintainer to push. Size the comment to GitHub's limit and -hand back plainly when the patch does not fit. - -## Related - -- [Protected edits](/quest/m0/land-fixer/protected.md) - the fixer's own workflow edits diff --git a/quest/m0/land-fixer/protected.md b/quest/m0/land-fixer/protected.md index 6107cfe6..d4cde982 100644 --- a/quest/m0/land-fixer/protected.md +++ b/quest/m0/land-fixer/protected.md @@ -38,7 +38,3 @@ protected-path rule in `assets/land-fix.md` with the same mechanical bar every other path has. Keep the rule that the fixer hands back any attempt to redirect its task, and update the decision line in [Fixer proxy access](/quest/m0/host-land/fixer.md). - -## Related - -- [Fork commits](/quest/m0/land-fixer/fork-commit.md) - workflow edits GitHub refuses to take from the App on forks diff --git a/src/cli/land-apply.ts b/src/cli/land-apply.ts new file mode 100644 index 00000000..89e026da --- /dev/null +++ b/src/cli/land-apply.ts @@ -0,0 +1,212 @@ +// A fixer resolution GitHub refused to 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"; +import { gunzipSync, gzipSync } from "node:zlib"; +import { GitHub, type GitHubDependencies, type PullRequest, object, remoteRepository, string } from "./github"; +import { contains, git, gitEnvironment, tryGit } from "./land"; +import { Refusal } from "./start"; + +/** GitHub refuses a comment body longer than this many characters. */ +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. + */ +export interface Resolution { + head: string; + base: string; + tree: string; + patch: string; +} + +const SHA = "[0-9a-f]{40}(?:[0-9a-f]{24})?"; +const DATA = new RegExp( + ``, +); + +/** + * Settings the merge's conflicted result depends on, pinned so the daemon's bare repository and a maintainer's + * checkout write the same tree. Attributes would pick merge drivers and conversions, so they come from the empty tree. + */ +function pinned(root: string): string[] { + const empty = git(root, ["mktree"], { input: "" }); + return [ + `--attr-source=${empty}`, + "-c", + "merge.conflictStyle=merge", + "-c", + "merge.renames=true", + "-c", + "merge.directoryRenames=conflict", + "-c", + "merge.renormalize=false", + ]; +} + +/** The tree `git merge-tree` writes for merging `base` into `head`, conflict markers included. */ +export function conflicted(root: string, head: string, base: string): string { + const merged = tryGit(root, [ + ...pinned(root), + "merge-tree", + "--write-tree", + "--name-only", + "--no-messages", + head, + base, + ]); + const tree = merged.stdout.split("\n")[0]; + if ((merged.status !== 0 && merged.status !== 1) || !new RegExp(`^${SHA}$`).test(tree)) + throw new Error(`cannot merge ${base} into ${head}: ${merged.stderr.trim()}`); + return tree; +} + +/** Run trusted git on bytes, since a patch need not be UTF-8. */ +function bytes(root: string, args: string[], input?: Buffer, env: Record = {}): Buffer { + const result = spawnSync("git", ["-c", "core.hooksPath=/dev/null", ...args], { + cwd: root, + input, + env: { ...gitEnvironment(), ...env }, + maxBuffer: 1 << 30, + }); + if (result.error) throw result.error; + if (result.status !== 0) throw new Error(`git ${args[0]} failed: ${result.stderr.toString().trim()}`); + return result.stdout; +} + +/** The fixed head as a resolution of `base` merged into `head`, or nothing when it does not contain that base. */ +export function resolution(root: string, head: string, base: string, fixed: string): Resolution | undefined { + if (!contains(root, base, fixed)) return undefined; + const patch = bytes(root, [ + "diff", + "--binary", + "--full-index", + "--no-renames", + "--no-ext-diff", + "--no-textconv", + "--no-color", + "--no-relative", + "--src-prefix=a/", + "--dst-prefix=b/", + conflicted(root, head, base), + fixed, + ]); + return { head, base, tree: git(root, ["rev-parse", `${fixed}^{tree}`]), patch: gzipSync(patch).toString("base64") }; +} + +/** 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 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`; + 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.`; +} + +/** The resolution a handback comment carries. */ +export function parseResolution(body: string): Resolution | undefined { + const match = DATA.exec(body); + return match ? { head: match[1], base: match[2], tree: match[3], patch: match[4] } : undefined; +} + +/** Merge the resolution's base into its head, apply its patch, and return the tree, refusing any other tree. */ +export function rebuild(root: string, resolution: Resolution): string { + const index = resolve(root, git(root, ["rev-parse", "--git-path", `quest-land-apply-${process.pid}.index`])); + const env = { GIT_INDEX_FILE: index }; + try { + git(root, ["read-tree", conflicted(root, resolution.head, resolution.base)], { env }); + const patch = gunzipSync(Buffer.from(resolution.patch, "base64")); + if (patch.length) + bytes(root, ["-c", "apply.whitespace=nowarn", "apply", "--cached", "--binary", "-"], patch, env); + const tree = git(root, ["write-tree"], { env }); + if (tree !== resolution.tree) + throw new Refusal( + `the rebuilt tree ${tree} differs from the fixer's ${resolution.tree}; this Git's merge differs from the daemon's, so redo the merge by hand`, + ); + return tree; + } finally { + rmSync(index, { force: true }); + } +} + +/** Git as the maintainer runs it, with their credentials, identity, and signing; hooks stay off. */ +function userGit(root: string, args: string[], input?: string): string { + const result = spawnSync("git", ["-c", "core.hooksPath=/dev/null", ...args], { + cwd: root, + input, + encoding: "utf8", + }); + if (result.error) throw result.error; + if (result.status !== 0) throw new Error(`git ${args[0]} failed: ${result.stderr.trim()}`); + return result.stdout.trim(); +} + +export interface Applied { + commit: string; + /** Where the maintainer pushes the commit: the PR's head repository and branch. */ + remote: string; + branch: string; +} + +/** + * Rebuild the commit a quest land fixer handed back because GitHub refused its push: 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 { + const github = new GitHub(deps); + const path = `/repos/${pr.owner}/${pr.repo}`; + const url = `https://github.com/${pr.owner}/${pr.repo}/pull/${pr.number}`; + // Only an App can post as a `[bot]` login, so a contributor cannot plant a resolution; the fixer's own + // handback marker sets it apart from another App quoting one. + const found = (await github.all(`${path}/issues/${pr.number}/comments?per_page=100`)) + .filter( + (item) => + item.user != null && + String(object(item.user).login).endsWith("[bot]") && + String(item.body ?? "").includes("`; @@ -110,11 +114,10 @@ async function comment( ) ) return; - await github.request(path, { - body: { - body: `${attribution} land fixer: ${fixerText(text)}${log ? `\n\n
${excerpt(log)}
` : ""}\n\n${marker}\n\n(written by ${attribution})`, - }, - }); + const lead = `${attribution} land fixer: ${fixerText(text)}${log ? `\n\n
${excerpt(log)}
` : ""}`; + const tail = `\n\n${marker}\n\n(written by ${attribution})`; + const extra = section?.(COMMENT_LIMIT - lead.length - tail.length) ?? ""; + await github.request(path, { body: { body: `${lead}${extra}${tail}` } }); } /** Queue eligibility can eject a previously requeued PR before building another batch. */ @@ -465,7 +468,17 @@ async function collectJob( `refs/heads/${entry.branch}`, token, ); - if (pushed.status !== 0) throw new FixerRefusal(`fixer push refused: ${pushed.stderr.trim()}`); + 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. + try { + const ready = resolution(root, entry.head, job.plan.base.sha, head); + if (ready) (job.resolutions ??= {})[number] = ready; + } catch (error) { + deps.warn(`quest land fixer: cannot hand back #${number}'s resolution: ${String(error)}`); + } + throw new FixerRefusal(`fixer push refused: ${pushed.stderr.trim()}`); + } } if (result?.action === "fixed") { @@ -555,6 +568,9 @@ async function collectJob( "handback", author(result), fixerLog(path), + error && job.resolutions?.[number] + ? (room) => resolutionSection(number, job.resolutions![number], room) + : undefined, ); output.push(`${error || result?.action === "handback" ? "handback" : "requeued"} ${number}`); job.numbers.shift(); diff --git a/src/cli/land.ts b/src/cli/land.ts index fc19f542..b7750f06 100644 --- a/src/cli/land.ts +++ b/src/cli/land.ts @@ -729,6 +729,16 @@ export function commandEnvironment(): NodeJS.ProcessEnv { return { PATH: process.env.PATH, LANG: "C.UTF-8" }; } +/** Git's environment for trusted work: no system or global configuration, and no prompts. */ +export function gitEnvironment(): NodeJS.ProcessEnv { + return { + ...commandEnvironment(), + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: "/dev/null", + GIT_TERMINAL_PROMPT: "0", + }; +} + /** Run git without the repository's hooks, returning its status and output. */ export function tryGit( cwd: string, @@ -739,13 +749,7 @@ export function tryGit( cwd, encoding: "utf8", input: options.input, - env: { - ...commandEnvironment(), - GIT_CONFIG_NOSYSTEM: "1", - GIT_CONFIG_GLOBAL: "/dev/null", - GIT_TERMINAL_PROMPT: "0", - ...options.env, - }, + env: { ...gitEnvironment(), ...options.env }, maxBuffer: 1 << 30, }); if (result.error) throw result.error; diff --git a/src/cli/main.ts b/src/cli/main.ts index ee0a587e..d60b29bf 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -43,6 +43,7 @@ import { gate } from "./gate"; import { gitHubDependencies, GitHubInputError, mergedHead, pullRequest, remoteRepository, remoteUrl } from "./github"; import { type Hold, type State, finish, land, release, take } from "./hold"; import { LAND_CHECK } from "./land"; +import { apply } from "./land-apply"; import { fix, FixerAgentError } from "./land-fix"; import { serve, type ServeOptions } from "./land-serve"; import { landed } from "./landed"; @@ -531,10 +532,29 @@ export function main(argv: string[], out: Output): number | Promise { () => print("fixer result written"), ); }); + 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 " + + "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; " + + "nothing is pushed and the working tree is untouched.", + ) + .argument("", "GitHub PR URL, or a PR number in origin") + .action((value: string) => { + settle(apply(root(), pullRequest(value, root()), gitHubDependencies(warn)), (applied) => { + print(applied.commit); + print( + `review it, then push: git push ${applied.remote} ${applied.commit}:refs/heads/${applied.branch}`, + ); + }); + }); 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. 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. 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; 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/test/land.test.ts b/test/land.test.ts index f2199028..4156bad7 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -25,6 +25,7 @@ import { ACTIONS_APP, type Pull, type World, fakeGitHub } from "../cloudflare/de import { authenticatedRead, gitAuthentication } from "../src/cli/land-git"; import { collectFixers, validateFix, fixerText, handbackRepeated } from "../src/cli/land-fixer"; import { importFixer } from "../src/cli/land-fixer-repository"; +import { apply, parseResolution, resolutionSection } from "../src/cli/land-apply"; import fixerPrompt from "../assets/land-fix.md"; import { fix, parseFixResult } from "../src/cli/land-fix"; import { resolvePreparedQueue, prepareHead, finishFallback } from "../src/cli/land-head"; @@ -1781,6 +1782,8 @@ async function fixerFixture( collateral?: boolean; // The base the fixer merges already changed a protected file. protectedBase?: boolean; + // The base the fixer merges already changed a workflow. + workflowBase?: boolean; } = {}, ) { const forge = new Forge(); @@ -1792,6 +1795,7 @@ async function fixerFixture( 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" }); const deps = await forge.deps(); const plan = await resolveQueue(REPOSITORY, undefined, deps); const state = sandbox(); @@ -2501,3 +2505,95 @@ describe.skipIf(process.platform === "win32")("private repository reads", () => await expect(read(root, ["fetch", "file:///local/repository", "main"])).rejects.toThrow("GitHub HTTPS"); }); }); + +/** GitHub's refusal of an App push that changes a workflow, as a pre-receive hook on every local remote. */ +function refuseWorkflows(forge: Forge): void { + const hook = join(forge.origin, "hooks", "pre-receive"); + writeFileSync( + hook, + `#!/bin/sh +while read -r old new ref; do + if [ -n "$(git diff --name-only "$old" "$new" -- .github/workflows)" ]; then + echo "refusing to allow a GitHub App to create or update workflow without workflows permission" >&2 + exit 1 + fi +done +`, + { mode: 0o755 }, + ); +} + +describe.skipIf(process.platform === "win32")("quest land apply", () => { + const PR = { owner: "upstream", repo: "project", number: 1 }; + async function refused() { + const fixture = await fixerFixture(fixedScript(), { conflict: true, workflowBase: true }); + await fixture.finished; + refuseWorkflows(fixture.forge); + expect(await fixture.collect()).toEqual(["handback 1"]); + const comment = fixture.forge.comment(1)!; + expect(comment).toContain("without workflows permission"); + expect(fixture.pull.head).toBe(fixture.plan.queue[0].head); + return { ...fixture, comment }; + } + + test("rebuilds the fixer's commit from a refused workflow push, for the maintainer to push", async () => { + const fixture = await refused(); + expect(fixture.comment).toContain("`quest land apply 1`"); + const resolution = parseResolution(fixture.comment)!; + expect(resolution).toMatchObject({ head: fixture.plan.queue[0].head, base: fixture.plan.base.sha }); + + // A maintainer's checkout whose own merge settings differ from the daemon's. + const checkout = fixture.forge.clone(); + git(checkout, "config", "merge.conflictStyle", "zdiff3"); + const applied = await apply(checkout, PR, await fixture.forge.deps()); + expect(git(checkout, "rev-parse", `${applied.commit}^{tree}`)).toBe(resolution.tree); + expect(git(checkout, "show", "-s", "--format=%P", applied.commit)).toBe( + `${resolution.head} ${resolution.base}`, + ); + expect(git(checkout, "show", `${applied.commit}:code.txt`)).toBe("one\nII and 2\nthree"); + expect(git(checkout, "show", `${applied.commit}:.github/workflows/alert.yml`)).toBe("on: push"); + expect(applied).toEqual({ + commit: applied.commit, + remote: "https://github.com/upstream/project.git", + branch: "topic-1", + }); + expect(git(checkout, "status", "--porcelain")).toBe(""); + // The maintainer's push is no App's, so GitHub takes it as a fast-forward of the PR branch. + rmSync(join(fixture.forge.origin, "hooks", "pre-receive")); + git(checkout, "push", "--quiet", applied.remote, `${applied.commit}:refs/heads/${applied.branch}`); + expect(fixture.forge.ref("upstream/project", "refs/heads/topic-1")).toBe(applied.commit); + }); + + 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); + await expect(apply(fixture.forge.clone(), PR, await fixture.forge.deps())).rejects.toThrow("head moved"); + }); + + test("trusts only an App's resolution", async () => { + const fixture = await refused(); + fixture.pull.comments = fixture.pull.comments!.map((item) => ({ ...item, author: "outsider" })); + await expect(apply(fixture.forge.clone(), PR, await fixture.forge.deps())).rejects.toThrow( + "carries no quest land fixer resolution", + ); + }); + + test("refuses a resolution whose tree the rebuild does not reach", async () => { + const fixture = await refused(); + const resolution = parseResolution(fixture.comment)!; + const forged = fixture.comment.replace(`tree=${resolution.tree}`, `tree=${"0".repeat(40)}`); + fixture.pull.comments!.at(-1)!.body = forged; + await expect(apply(fixture.forge.clone(), PR, await fixture.forge.deps())).rejects.toThrow( + "differs from the fixer's", + ); + }); + + test("hands back plainly when the resolution does not fit in a comment", () => { + const resolution = { head: "a".repeat(40), base: "b".repeat(40), tree: "c".repeat(40), patch: "x".repeat(100) }; + const fits = resolutionSection(1, resolution, 1_000); + expect(parseResolution(fits)).toEqual(resolution); + const plain = resolutionSection(1, resolution, 300); + expect(parseResolution(plain)).toBeUndefined(); + expect(plain).toContain("too large for a comment"); + }); +}); From 99a1c914d72e6a3a2eee335118838e3791e711b9 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 05:33:33 -0800 Subject: [PATCH 3/3] fix(land): shell-quote the printed push command A contributor names the PR branch, so quote every argument of the push command quest land apply prints. Co-Authored-By: Claude Opus 5.5 --- src/cli/land-apply.ts | 6 ++++++ src/cli/main.ts | 6 ++---- test/land.test.ts | 11 ++++++++++- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/cli/land-apply.ts b/src/cli/land-apply.ts index 89e026da..33b840ff 100644 --- a/src/cli/land-apply.ts +++ b/src/cli/land-apply.ts @@ -145,6 +145,12 @@ function userGit(root: string, args: string[], input?: string): string { return result.stdout.trim(); } +/** The command that pushes `applied`, each argument shell-quoted: a contributor names the branch. */ +export function pushCommand(applied: Applied): string { + const quote = (value: string) => `'${value.replaceAll("'", `'\\''`)}'`; + return ["git", "push", applied.remote, `${applied.commit}:refs/heads/${applied.branch}`].map(quote).join(" "); +} + export interface Applied { commit: string; /** Where the maintainer pushes the commit: the PR's head repository and branch. */ diff --git a/src/cli/main.ts b/src/cli/main.ts index d60b29bf..06c38a5a 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -43,7 +43,7 @@ import { gate } from "./gate"; import { gitHubDependencies, GitHubInputError, mergedHead, pullRequest, remoteRepository, remoteUrl } from "./github"; import { type Hold, type State, finish, land, release, take } from "./hold"; import { LAND_CHECK } from "./land"; -import { apply } from "./land-apply"; +import { apply, pushCommand } from "./land-apply"; import { fix, FixerAgentError } from "./land-fix"; import { serve, type ServeOptions } from "./land-serve"; import { landed } from "./landed"; @@ -546,9 +546,7 @@ export function main(argv: string[], out: Output): number | Promise { .action((value: string) => { settle(apply(root(), pullRequest(value, root()), gitHubDependencies(warn)), (applied) => { print(applied.commit); - print( - `review it, then push: git push ${applied.remote} ${applied.commit}:refs/heads/${applied.branch}`, - ); + print(`review it, then push: ${pushCommand(applied)}`); }); }); landCommand diff --git a/test/land.test.ts b/test/land.test.ts index 4156bad7..c147a2e4 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -25,7 +25,7 @@ import { ACTIONS_APP, type Pull, type World, fakeGitHub } from "../cloudflare/de import { authenticatedRead, gitAuthentication } from "../src/cli/land-git"; import { collectFixers, validateFix, fixerText, handbackRepeated } from "../src/cli/land-fixer"; import { importFixer } from "../src/cli/land-fixer-repository"; -import { apply, parseResolution, resolutionSection } from "../src/cli/land-apply"; +import { apply, parseResolution, pushCommand, resolutionSection } from "../src/cli/land-apply"; import fixerPrompt from "../assets/land-fix.md"; import { fix, parseFixResult } from "../src/cli/land-fix"; import { resolvePreparedQueue, prepareHead, finishFallback } from "../src/cli/land-head"; @@ -2588,6 +2588,15 @@ describe.skipIf(process.platform === "win32")("quest land apply", () => { ); }); + test("quotes a contributor's branch in the push command", () => { + const branch = "topic;echo${IFS}injected'x"; + const command = pushCommand({ commit: "a".repeat(40), remote: "https://github.com/o/r.git", branch }); + const echoed = execFileSync("sh", ["-c", `eval "set -- $1"; printf '%s\\n' "$@"`, "sh", command], { + encoding: "utf8", + }); + expect(echoed).toBe(`git\npush\nhttps://github.com/o/r.git\n${"a".repeat(40)}:refs/heads/${branch}\n`); + }); + test("hands back plainly when the resolution does not fit in a comment", () => { const resolution = { head: "a".repeat(40), base: "b".repeat(40), tree: "c".repeat(40), patch: "x".repeat(100) }; const fits = resolutionSection(1, resolution, 1_000);