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..24057bc4 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,18 @@ 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"))) { + // 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, [ "-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..35611921 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})` }, }); } @@ -251,15 +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 { - const line = `mechanical fix by Codex: ${fixerText(reason)} (head ${head}; ${attribution})`; + const line = `mechanical fix by ${attribution}: ${fixerText(reason)} (head ${head})`; if (body.includes(line)) return body; - const section = /^## Review[ \t]*$/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})`; + const section = /^## Review[ \t]*\r?$/m.exec(body); + 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/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..9e8afe6e 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})`, ); expect(fixture.pull.autoMerge?.commitBody).toBe(pin(fixture.pull.head!)); expect(fixture.pull.labels).toContain("quest:merge"); @@ -1972,6 +1979,47 @@ ${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 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")); + 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"); + // 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", () => { for (const value of [ [],