From a286bb6182f5563d15a4b040fab8600c3717b263 Mon Sep 17 00:00:00 2001 From: "Jason (Json)" <263060202+fuller-stack-dev@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:24:19 -0600 Subject: [PATCH] fix: correction review fails on large changed-path sets (#156043) --- scripts/pr-lib/correction-review.mjs | 2 + test/scripts/pr-review-artifacts.node.mjs | 164 ++++++++++++++++++++++ 2 files changed, 166 insertions(+) diff --git a/scripts/pr-lib/correction-review.mjs b/scripts/pr-lib/correction-review.mjs index c891465e077e..3e63548e6059 100644 --- a/scripts/pr-lib/correction-review.mjs +++ b/scripts/pr-lib/correction-review.mjs @@ -61,6 +61,8 @@ function candidateMetadata(prMeta, incoming, head) { // derived in memory; the live incoming-head metadata is never rewritten. const changed = execFileSync(gitExecutable, ["diff", "--name-only", "-z", incoming, head], { encoding: "utf8", + // The complete path set is required; a fixed capture cap rejects large corrections. + maxBuffer: Infinity, }) .split("\0") .filter(Boolean); diff --git a/test/scripts/pr-review-artifacts.node.mjs b/test/scripts/pr-review-artifacts.node.mjs index 613da6aa158b..0789a6c9300e 100644 --- a/test/scripts/pr-review-artifacts.node.mjs +++ b/test/scripts/pr-review-artifacts.node.mjs @@ -328,3 +328,167 @@ ${invocation} || exit 1 } }, ); + +function correctionFixture(t, incomingPaths = ["docs/incoming.md"]) { + const root = mkdtempSync(join(tmpdir(), "pr-correction-paths-")); + t.after(() => rmSync(root, { recursive: true, force: true })); + const env = { + ...process.env, + GIT_CONFIG_GLOBAL: process.platform === "win32" ? "NUL" : "/dev/null", + GIT_CONFIG_NOSYSTEM: "1", + GIT_AUTHOR_NAME: "Fixture", + GIT_AUTHOR_EMAIL: "fixture@example.invalid", + GIT_COMMITTER_NAME: "Fixture", + GIT_COMMITTER_EMAIL: "fixture@example.invalid", + OPENCLAW_PR_GIT: "", + GIT_EXEC: "", + }; + const git = (args, input) => + execFileSync("git", args, { cwd: root, env, input, encoding: "utf8" }); + git(["init", "-q", "-b", "topic"]); + git(["config", "commit.gpgSign", "false"]); + // Synthetic index-only names need not be representable on the host filesystem. + git(["config", "core.protectNTFS", "false"]); + git(["config", "core.hooksPath", process.platform === "win32" ? "NUL" : "/dev/null"]); + mkdirSync(join(root, ".local")); + writeFileSync(join(root, ".gitignore"), ".local/\n"); + git(["add", ".gitignore"]); + git(["commit", "-qm", "incoming"]); + const incoming = git(["rev-parse", "HEAD"]).trim(); + const review = JSON.parse(cli("template", "42", incoming).stdout); + review.findings = [ + { id: "F1", severity: "IMPORTANT", title: "Fix behavior", area: "paths", fix: "Correct it" }, + ]; + const incomingFiles = { + "review.json": JSON.stringify(review), + "pr-meta.json": JSON.stringify({ + number: 42, + headRefOid: incoming, + files: incomingPaths.map((path) => ({ path })), + }), + }; + for (const [name, bytes] of Object.entries(incomingFiles)) { + writeFileSync(join(root, ".local", name), bytes); + } + const jsonOid = git(["hash-object", "--no-filters", ".local/review.json"]).trim(); + const commitPaths = (paths) => { + const blob = git(["hash-object", "-w", "--stdin"], "fixture\n").trim(); + git( + ["update-index", "-z", "--index-info"], + paths.map((path) => `100644 ${blob}\t${path}\0`).join(""), + ); + git(["commit", "-qm", "correction"]); + // Let Git mark exact index names without host-specific path normalization. + // Only .gitignore is materialized; the synthetic paths stay in the index. + git(["sparse-checkout", "set", "--no-cone", "/.gitignore"]); + }; + const run = (command, envOverrides = {}) => + spawnSync( + process.execPath, + [ + join(scripts, "pr-lib/correction-review.mjs"), + command, + "42", + incoming, + git(["rev-parse", "HEAD"]).trim(), + jsonOid, + ], + { cwd: root, env: { ...env, ...envOverrides }, encoding: "utf8" }, + ); + const approve = (runtime = false) => { + const path = join(root, ".local/correction-review.json"); + const correction = JSON.parse(readFileSync(path, "utf8")); + correction.recommendation = "READY FOR /prepare-pr"; + correction.findings = []; + Object.assign(correction.issueValidation, { performed: true, status: "valid" }); + Object.assign(correction.behavioralSweep, { + performed: true, + status: runtime ? "pass" : "not_applicable", + silentDropRisk: "none", + branches: runtime + ? [{ path: "ui/last-雪\n\tfile.ts", decision: "changed", outcome: "verified" }] + : [], + }); + correction.tests = { result: "pass", ran: ["regression fixture"], gaps: [] }; + correction.correction.resolvedFindings[0].resolution = "Verified corrected behavior."; + writeFileSync(path, JSON.stringify(correction)); + }; + const assertIncomingUnchanged = () => { + for (const [name, bytes] of Object.entries(incomingFiles)) { + assert.equal(readFileSync(join(root, ".local", name), "utf8"), bytes); + } + }; + return { root, git, incoming, commitPaths, run, approve, assertIncomingUnchanged }; +} + +test("correction review accepts Git path output above 1 MiB without dropping the final runtime path", (t) => { + const f = correctionFixture(t); + const directory = "d".repeat(80); + const paths = Array.from({ length: 12000 }, (_, i) => `docs/${directory}/${i}.md`); + paths.push("ui/last-雪\n\tfile.ts"); + f.commitPaths(paths); + assert.ok(Buffer.byteLength(`${paths.join("\0")}\0`) > 1024 * 1024); + assert.throws(() => f.git(["diff", "--name-only", "-z", f.incoming, "HEAD"]), { + code: "ENOBUFS", + }); + const initialized = f.run("init"); + assert.equal(initialized.status, 0, initialized.stdout + initialized.stderr); + // A runtime path after the former capture ceiling still requires runtime proof. + f.approve(); + const validated = f.run("validate"); + assert.equal(validated.status, 1, validated.stdout + validated.stderr); + assert.match(validated.stderr, /runtime file changes require/); + f.approve(true); + const accepted = f.run("validate"); + assert.equal(accepted.status, 0, accepted.stdout + accepted.stderr); + assert.match(accepted.stdout, /READY FOR \/prepare-pr/); + f.assertIncomingUnchanged(); +}); + +for (const incomingRuntime of [false, true]) { + test(`correction review preserves NUL path boundaries and incoming scope, runtime=${incomingRuntime}`, (t) => { + const f = correctionFixture(t, [incomingRuntime ? "src/incoming.ts" : "docs/incoming.md"]); + f.commitPaths(["docs/line\nsrc/not-a-separate-path.ts", 'docs/ space\t"\\雪.md']); + const initialized = f.run("init"); + assert.equal(initialized.status, 0, initialized.stdout + initialized.stderr); + f.approve(); + const result = f.run("validate"); + assert.equal(result.status, incomingRuntime ? 1 : 0, result.stdout + result.stderr); + if (incomingRuntime) { + assert.match(result.stderr, /runtime file changes require/); + } else { + assert.match(result.stdout, /READY FOR \/prepare-pr/); + } + f.assertIncomingUnchanged(); + }); +} + +test( + "correction review rejects a failed path query without writing review artifacts", + { + skip: process.platform === "win32", + }, + (t) => { + const f = correctionFixture(t); + f.commitPaths(["docs/fix.md"]); + const selectedGit = join(f.root, ".local", "git-query-error"); + writeFileSync( + selectedGit, + `#!/bin/sh +if [ "$1" = diff ] && [ "$2" = --name-only ]; then + printf 'docs/partial.md\\0' + echo 'fixture changed-path failure' >&2 + exit 73 +fi +exec git "$@" +`, + { mode: 0o755 }, + ); + const result = f.run("init", { OPENCLAW_PR_GIT: selectedGit }); + assert.equal(result.status, 1, result.stdout + result.stderr); + assert.match(result.stderr, /fixture changed-path failure/); + assert.equal(existsSync(join(f.root, ".local/correction-review.json")), false); + assert.equal(existsSync(join(f.root, ".local/correction-incoming-review.json")), false); + f.assertIncomingUnchanged(); + }, +);