From e4af17eaadd0737cca5332f9e0c7647afeb2034b Mon Sep 17 00:00:00 2001 From: Luke Curley <432854+kixelated@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:44:10 -0700 Subject: [PATCH 1/2] fix(land): give sandbox checks their tested base Retain the shallow chain through its base and pass plan.base.sha as the sandbox's second argument and QUEST_LAND_BASE. Document the contract and cover diff visibility, merge-base, and standalone isolation for single- and multi-PR batches. Rebuild fixer pack indexes with index-pack --strict, enable fetch.fsckObjects, use the recorded agent in fixer wording, and replace stacked PR body attributions. --- README.md | 18 ++++++---- src/cli/land-fixer-repository.ts | 8 +++-- src/cli/land-fixer.ts | 14 +++++--- src/cli/land-serve.ts | 12 +++---- src/cli/main.ts | 7 ++-- test/land.test.ts | 59 ++++++++++++++++++++++++++++---- 6 files changed, 92 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 2d9c22ab..c8da6c2b 100644 --- a/README.md +++ b/README.md @@ -480,14 +480,19 @@ QUEST_LAND_KEY=/run/credentials/quest-land/key quest land serve --app-id 1039466 --sandbox /usr/local/bin/check-sandbox --fixer /usr/local/bin/fix-sandbox ``` -The sandbox executable receives the chain's checkout path and must isolate PR -code from the host's credentials and filesystem. Both the sandbox and fixer +The sandbox executable receives ` ` as its two arguments, +with cwd set to the checkout. The SHA is `plan.base.sha`, the commit the tested +chain was built on, and is also exported as `QUEST_LAND_BASE`. Its environment +otherwise contains only `PATH` and `LANG`. The wrapper must isolate PR code from +the host's credentials and filesystem. Both the sandbox and fixer wrappers **must run code as a different user from the App key owner**, for example inside nspawn containers. Environment filtering cannot protect the key or later merge tokens from a process running under the daemon's UID through `/proc`. -The sandbox gets a standalone, depth-1 clone of the tested commit, with no remote -and no alternates, so `HEAD` is the commit being checked and nothing links back to -the daemon's repository; its working directory is that clone. The fixer runs in an independent repository with batch history, no remotes, +The sandbox gets a standalone shallow clone of the tested tip and every commit +through that base (`batch links + 1` commits), with no remote or alternates. +`HEAD` is the tested tip; `git diff "$2"` and `git merge-base "$2" HEAD` work, +and nothing links back to the daemon's repository. moq's host wrapper can run +`nix develop --command just ci "$2"`; quest's wrapper may ignore the extra argument. The fixer runs in an independent repository with batch history, no remotes, no alternates, and no link to the daemon's private bare repository. Fetches and chain construction use only that private repository in the state directory. The sandbox runs the repository's checks @@ -519,7 +524,8 @@ as the bot. The protected set as a whole must match the pinned head or its merge current base, including content and mode. It covers `flake.nix`, `flake.lock`, `shell.nix`, `default.nix`, `nix/`, `.envrc`, `.github/`, `.gitmodules`, and gitlinks anywhere. Collection copies at most 100 MiB of Git metadata into a clean object -repository, ignores fixer configuration, and never follows Git alternates. It records `mechanical fix by Codex: ` and the new head in +repository, ignores fixer configuration, and never follows Git alternates. Pack indexes are rebuilt with +`git index-pack --strict`, and the import enables `fetch.fsckObjects=true`. It records `mechanical fix by : ` and the new head in `## 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. A flaky diff --git a/src/cli/land-fixer-repository.ts b/src/cli/land-fixer-repository.ts index dcc597d3..499fb5f6 100644 --- a/src/cli/land-fixer-repository.ts +++ b/src/cli/land-fixer-repository.ts @@ -38,8 +38,7 @@ export function importFixer(root: string, state: string, checkout: string, numbe else { bytes += stat.size; if (bytes > MAX_FIXER_BYTES) throw new Error("fixer repository exceeds 100 MiB"); - if (/^objects\/(?:[0-9a-f]{2}\/[0-9a-f]{38}|pack\/pack-[0-9a-f]+\.(?:pack|idx|rev))$/.test(rel)) - objects.push(rel); + if (/^objects\/(?:[0-9a-f]{2}\/[0-9a-f]{38}|pack\/pack-[0-9a-f]+\.pack)$/.test(rel)) objects.push(rel); } } }; @@ -81,11 +80,16 @@ export function importFixer(root: string, state: string, checkout: string, numbe mkdirSync(dirname(dest), { recursive: true }); writeFileSync(dest, data); } + for (const path of objects.filter((path) => path.endsWith(".pack"))) { + git(clean, ["index-pack", "--strict", join(clean, path)]); + } git(clean, ["update-ref", ref, head]); git(root, [ "-c", `safe.directory=${clean}`, "-c", + "fetch.fsckObjects=true", + "-c", "protocol.file.allow=always", "fetch", "--quiet", diff --git a/src/cli/land-fixer.ts b/src/cli/land-fixer.ts index 6ef4242e..139b2a0d 100644 --- a/src/cli/land-fixer.ts +++ b/src/cli/land-fixer.ts @@ -77,7 +77,7 @@ async function comment( ) return; await github.request(path, { - body: { body: `Codex land fixer: ${fixerText(text)}\n\n${marker}\n\n(written by ${attribution})` }, + body: { body: `${attribution} land fixer: ${fixerText(text)}\n\n${marker}\n\n(written by ${attribution})` }, }); } @@ -252,9 +252,15 @@ function author(result?: Pick): string { ); } function review(body: string, head: string, reason: string, attribution: string): string { - const line = `mechanical fix by Codex: ${fixerText(reason)} (head ${head}; ${attribution})`; - if (body.includes(line)) return body; - const section = /^## Review[ \t]*$/m.exec(body); + for (;;) { + const stripped = body.replace(/(?:^|\r?\n)[ \t]*\(written by [^\r\n)]+\)[ \t\r\n]*$/, ""); + if (stripped === body) break; + body = stripped; + } + + const line = `mechanical fix by ${attribution}: ${fixerText(reason)} (head ${head}; ${attribution})`; + if (body.includes(line)) return `${body}\n\n(written by ${attribution})`; + const section = /^## Review[ \t]*\r?$/m.exec(body); const start = section ? section.index + section[0].length : body.length; const updated = section ? `${body.slice(0, start)}\n\n${line}${body.slice(start)}` diff --git a/src/cli/land-serve.ts b/src/cli/land-serve.ts index 07f40a04..3bbc77dd 100644 --- a/src/cli/land-serve.ts +++ b/src/cli/land-serve.ts @@ -99,18 +99,18 @@ export function privateRepository(state: string): string { } /** - * A standalone, depth-1 clone of the tip: `HEAD` is the commit being checked, so tools that need a + * A standalone shallow clone from the tested tip through its base: `HEAD` is the commit being checked, so tools that need a * repository work. It has no remote and no alternates, so nothing in it links back to the daemon's * repository, and a checkout ignores `export-ignore` and `export-subst`, so a PR cannot hide files. */ -function sandboxTree(root: string, state: string, tip: string): string { +function sandboxTree(root: string, state: string, tip: string, links: number): string { const checkout = mkdtempSync(join(state, "check-")); // A shallow fetch needs a ref; this one lives only for the fetch. const ref = "refs/quest-land/check"; git(root, ["update-ref", ref, tip]); try { git(checkout, ["init", "--quiet"]); - git(checkout, ["fetch", "--quiet", "--no-tags", "--depth=1", `file://${root}`, ref]); + git(checkout, ["fetch", "--quiet", "--no-tags", `--depth=${links + 1}`, `file://${root}`, ref]); git(checkout, ["checkout", "--quiet", "--detach", "FETCH_HEAD"]); } finally { git(root, ["update-ref", "-d", ref]); @@ -289,15 +289,15 @@ export async function serveBatch( }; } else { const tip = chain.links.at(-1)!; - const checkout = sandboxTree(root, options.state, tip.commit); + const checkout = sandboxTree(root, options.state, tip.commit, chain.links.length); writeFileSync(log, "", { mode: 0o600 }); let code: number; try { code = await new Promise((done, reject) => { - const child = spawn(options.sandbox, [checkout], { + const child = spawn(options.sandbox, [checkout, plan.base.sha], { cwd: checkout, detached: true, - env: commandEnvironment(), + env: { ...commandEnvironment(), QUEST_LAND_BASE: plan.base.sha }, stdio: ["ignore", "pipe", "pipe"], }); // A hung check would stall the queue: kill its whole process group at the deadline. diff --git a/src/cli/main.ts b/src/cli/main.ts index 10a133d7..eb9c0963 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -531,11 +531,14 @@ export function main(argv: string[], out: Output): number | Promise { landCommand .command("serve") .description( - "Run one daemon per repository. Check a base PR plus up to floor(log2 n) cleanly merging PRs once through the sandbox. Failed batches are ejected together, never bisected. 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 the checkout path; 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. Check a base PR plus up to floor(log2 n) cleanly merging PRs once through the sandbox. Failed batches are ejected together, never bisected. Requires quest on PATH and flock. Sandbox and fixer wrappers MUST run code as a different user from the App key owner (for example in nspawn containers); environment filtering alone cannot isolate /proc or host files. The sandbox receives , with the base also in QUEST_LAND_BASE and present in the standalone shallow clone; the optional tracked background fixer receives the log path followed by PR numbers. Neither receives credentials. State retains logs, fixer jobs, and one mechanical attempt per maintainer pick across restarts. Fixer results are validated, lease-pushed, recorded in Review, and pinned for requeue; invalid results, crashes, and timeouts are handed back. After revalidating the queue, merge the base into each behind-base PR head with quest resolve, verify the tested tree, and lease-push that head before the pinned GitHub squash API merge. A permissions-refused branch push falls back to a leased fast-forward of the tested squash to the base, closing the PR with a linked-commit comment and removing quest:merge. A recorded daemon merge atop the original pin is recognized after restart; other moved heads require maintainer requeueing. Pending mergeability is retried briefly; confirmed conflicts or moved heads eject the remaining batch. A refusal that would repeat, such as squash being disabled, ejects as merge_refused without the fixer. An unexpected squash parent rebuilds the queue without ejecting; a tree mismatch logs CRITICAL and ejects the rest as tree_mismatch. Merged PRs drop out of the queue after restart.", ) .option("--base ", "Base branch (default: repository default)") .option("--state ", "Private daemon state directory (default: $STATE_DIRECTORY or the XDG state home)") - .requiredOption("--sandbox ", "Sandbox executable that checks the supplied checkout path") + .requiredOption( + "--sandbox ", + "Sandbox executable called with and QUEST_LAND_BASE", + ) .option("--fixer ", "Credential-free fixer wrapper executable (60-minute deadline)") .requiredOption("--app-id ", "GitHub App id") .option("--timeout ", "Fail a check that runs longer than this", positiveInteger("timeout"), 60) diff --git a/test/land.test.ts b/test/land.test.ts index 43aee73d..3a1db5b4 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1014,15 +1014,22 @@ describe.skipIf(process.platform === "win32")("host daemon", () => { expect(forge.main()).toBe(old); }); - test("gives the sandbox a standalone shallow clone, separate from the private bare repository", async () => { + test.each([1, 3])("gives the sandbox its base and a standalone shallow chain (%s queued PRs)", async (count) => { const forge = new Forge(); - forge.pull("One", { "one.txt": "yes" }); + forge.advance({ "base.txt": "base history" }); + for (let number = 1; number <= count; number++) forge.pull(`PR ${number}`, { [`file-${number}.txt`]: "yes" }); const deps = await forge.deps(); const plan = await resolveQueue(REPOSITORY, undefined, deps); + const links = 1 + Math.floor(Math.log2(count)); const options = sandbox( - 'test "$PWD" = "$1" && test -d .git && test -z "$(git remote)" && test ! -e .git/objects/info/alternates && ' + - 'test "$(git rev-parse --is-shallow-repository)" = true && test "$(git log --format=%s)" = "One (#1)" && ' + - "git diff --quiet HEAD && test ! -e key.pem && test ! -e repository.git && test -f one.txt", + 'test "$PWD" = "$1" && test "$#" = 2 && test "$QUEST_LAND_BASE" = "$2" && ' + + `test "$2" = '${plan.base.sha}' && ` + + 'test -d .git && test -z "$(git remote)" && test ! -e .git/objects/info/alternates && ' + + 'test "$(git rev-parse --is-shallow-repository)" = true && ' + + `test "$(git rev-list --count HEAD)" = '${links + 1}' && ` + + 'test "$(git merge-base "$2" HEAD)" = "$2" && ' + + "git diff --quiet HEAD && test ! -e key.pem && test ! -e repository.git || exit 1;\n" + + 'git diff --quiet "$2"..HEAD -- file-1.txt; code=$?; test "$code" = 1', ); const trusted = privateRepository(options.state); forge.configure(trusted); @@ -1705,7 +1712,7 @@ describe("asynchronous land fixer", () => { expect(fixture.pull.head).not.toBe(fixture.plan.queue[0].head); expect(git(fixture.root, "show", "-s", "--format=%P", fixture.pull.head!)).toContain(fixture.plan.base.sha); expect(fixture.pull.body).toContain( - `mechanical fix by Codex: \`preserve both conflict sides\` (head ${fixture.pull.head}; agent / Codex)`, + `mechanical fix by agent / Codex: \`preserve both conflict sides\` (head ${fixture.pull.head}; agent / Codex)`, ); expect(fixture.pull.autoMerge?.commitBody).toBe(pin(fixture.pull.head!)); expect(fixture.pull.labels).toContain("quest:merge"); @@ -1972,6 +1979,46 @@ ${requeueScript} expect(() => validateFix(root, entry, git(root, "rev-parse", "HEAD"), forge.main())).toThrow("protected set"); }); + test.each([false, true])( + "rebuilds pack indexes and strictly validates packs (corrupt pack: %s)", + async (corrupt) => { + const fixture = await fixerFixture(fixedScript()); + await fixture.finished; + const job = JSON.parse(readFileSync(join(fixture.state.state, fixture.jobs()[0]), "utf8")); + git(job.checkout, "gc", "--prune=now"); + const packs = join(job.checkout, ".git/objects/pack"); + const pack = readdirSync(packs).find((name) => name.endsWith(".pack"))!; + expect(pack).toBeTruthy(); + for (const name of readdirSync(packs).filter((name) => /\.(idx|rev)$/.test(name))) { + rmSync(join(packs, name)); + writeFileSync(join(packs, name), "untrusted index"); + } + if (corrupt) { + rmSync(join(packs, pack)); + writeFileSync(join(packs, pack), "corrupt pack"); + } + expect(await fixture.collect()).toEqual([corrupt ? "handback 1" : "requeued 1"]); + }, + ); + test("uses the recorded agent and replaces stacked body attributions", async () => { + const fixture = await fixerFixture(fixedScript()); + await fixture.finished; + const job = JSON.parse(readFileSync(join(fixture.state.state, fixture.jobs()[0]), "utf8")); + const resultPath = join(job.checkout, "quest-land-result.json"); + const results = JSON.parse(readFileSync(resultPath, "utf8")); + results[0].agent = "Claude"; + results[0].model = "actual-model"; + writeFileSync(resultPath, JSON.stringify(results)); + fixture.pull.body = + "Author content\r\n\r\n## Review\r\n\r\nPrevious review\r\n\r\n(written by old-model)\n\n(written by older-model)"; + expect(await fixture.collect()).toEqual(["requeued 1"]); + expect(fixture.pull.body).toContain("mechanical fix by Claude / actual-model:"); + expect(fixture.pull.body).toContain("Author content"); + expect(fixture.pull.body).toContain("Previous review"); + expect(fixture.pull.body?.match(/\(written by /g)).toHaveLength(1); + expect(fixture.pull.body).toMatch(/\(written by Claude \/ actual-model\)$/); + }); + test("strictly rejects duplicate, missing, extra, and multiline results", () => { for (const value of [ [], From 0175469cec70b7161e491d3d89fbe18086a7bb84 Mon Sep 17 00:00:00 2001 From: Luke Curley <432854+kixelated@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:54:55 -0700 Subject: [PATCH 2/2] fix(land): index the fixer's packs in any order and keep the author's attribution Strict index-pack failed when one pack refers to another not yet indexed, which would hand back valid fixes; fetch.fsckObjects already checks every object and link on import. The fix is credited only in its Review line, so the author's own (written by ...) lines stay. Co-Authored-By: Claude Opus 5.5 --- src/cli/land-fixer-repository.ts | 4 +++- src/cli/land-fixer.ts | 19 ++++++------------- test/land.test.ts | 9 +++++---- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/cli/land-fixer-repository.ts b/src/cli/land-fixer-repository.ts index 499fb5f6..24057bc4 100644 --- a/src/cli/land-fixer-repository.ts +++ b/src/cli/land-fixer-repository.ts @@ -81,7 +81,9 @@ export function importFixer(root: string, state: string, checkout: string, numbe writeFileSync(dest, data); } for (const path of objects.filter((path) => path.endsWith(".pack"))) { - git(clean, ["index-pack", "--strict", join(clean, path)]); + // Rebuild the index rather than trust the fixer's. Not --strict: one pack may refer to another not yet + // indexed; fetch.fsckObjects checks every object and link when they reach the daemon's repository. + git(clean, ["index-pack", join(clean, path)]); } git(clean, ["update-ref", ref, head]); git(root, [ diff --git a/src/cli/land-fixer.ts b/src/cli/land-fixer.ts index 139b2a0d..35611921 100644 --- a/src/cli/land-fixer.ts +++ b/src/cli/land-fixer.ts @@ -251,21 +251,14 @@ function author(result?: Pick): string { .slice(0, 100) || "Codex" ); } +/** The PR body with the fix recorded in its `## Review` section; the line names the agent, and the author's text stays as written. */ function review(body: string, head: string, reason: string, attribution: string): string { - for (;;) { - const stripped = body.replace(/(?:^|\r?\n)[ \t]*\(written by [^\r\n)]+\)[ \t\r\n]*$/, ""); - if (stripped === body) break; - body = stripped; - } - - const line = `mechanical fix by ${attribution}: ${fixerText(reason)} (head ${head}; ${attribution})`; - if (body.includes(line)) return `${body}\n\n(written by ${attribution})`; + const line = `mechanical fix by ${attribution}: ${fixerText(reason)} (head ${head})`; + if (body.includes(line)) return body; const section = /^## Review[ \t]*\r?$/m.exec(body); - const start = section ? section.index + section[0].length : body.length; - const updated = section - ? `${body.slice(0, start)}\n\n${line}${body.slice(start)}` - : `${body}\n\n## Review\n\n${line}`; - return `${updated}\n\n(written by ${attribution})`; + if (!section) return `${body}\n\n## Review\n\n${line}`; + const start = section.index + section[0].length; + return `${body.slice(0, start)}\n\n${line}${body.slice(start)}`; } export async function collectFixers( diff --git a/test/land.test.ts b/test/land.test.ts index 3a1db5b4..9e8afe6e 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1712,7 +1712,7 @@ describe("asynchronous land fixer", () => { expect(fixture.pull.head).not.toBe(fixture.plan.queue[0].head); expect(git(fixture.root, "show", "-s", "--format=%P", fixture.pull.head!)).toContain(fixture.plan.base.sha); expect(fixture.pull.body).toContain( - `mechanical fix by agent / Codex: \`preserve both conflict sides\` (head ${fixture.pull.head}; agent / Codex)`, + `mechanical fix by agent / Codex: \`preserve both conflict sides\` (head ${fixture.pull.head})`, ); expect(fixture.pull.autoMerge?.commitBody).toBe(pin(fixture.pull.head!)); expect(fixture.pull.labels).toContain("quest:merge"); @@ -2000,7 +2000,7 @@ ${requeueScript} expect(await fixture.collect()).toEqual([corrupt ? "handback 1" : "requeued 1"]); }, ); - test("uses the recorded agent and replaces stacked body attributions", async () => { + test("uses the recorded agent and leaves the author's attribution alone", async () => { const fixture = await fixerFixture(fixedScript()); await fixture.finished; const job = JSON.parse(readFileSync(join(fixture.state.state, fixture.jobs()[0]), "utf8")); @@ -2015,8 +2015,9 @@ ${requeueScript} expect(fixture.pull.body).toContain("mechanical fix by Claude / actual-model:"); expect(fixture.pull.body).toContain("Author content"); expect(fixture.pull.body).toContain("Previous review"); - expect(fixture.pull.body?.match(/\(written by /g)).toHaveLength(1); - expect(fixture.pull.body).toMatch(/\(written by Claude \/ actual-model\)$/); + // The author's attributions stay; the fix is credited only in its own Review line. + expect(fixture.pull.body).toMatch(/\(written by old-model\)\n\n\(written by older-model\)$/); + expect(fixture.pull.body).not.toContain("(written by Claude"); }); test("strictly rejects duplicate, missing, extra, and multiline results", () => {