diff --git a/.changes/pr-pipeline/fix-issue-patch-exact-renames-7c1e2b90.md b/.changes/pr-pipeline/fix-issue-patch-exact-renames-7c1e2b90.md new file mode 100644 index 0000000000..7089571bcb --- /dev/null +++ b/.changes/pr-pipeline/fix-issue-patch-exact-renames-7c1e2b90.md @@ -0,0 +1,11 @@ +--- +bump: patch +--- + +### Fixed + +- `collect-patch.sh` writes an exact rename (100% similarity) as a rename header, so a moved file + no longer counts its whole content twice against the 256 KiB patch cap; a rename with edits is + still a deletion and an addition. `apply-patch.mjs` already checks the tree the patch builds with + rename detection off, so a rename's old and new path each meet every refusal, the path + predicates, `max-files` and the blob size and credential checks. diff --git a/.github/scripts/apply-patch/apply-patch.test.mjs b/.github/scripts/apply-patch/apply-patch.test.mjs index 787b5edf8b..49b67bd4ab 100644 --- a/.github/scripts/apply-patch/apply-patch.test.mjs +++ b/.github/scripts/apply-patch/apply-patch.test.mjs @@ -150,7 +150,8 @@ function commitAll(repo, message) { return git(repo, ["rev-parse", "HEAD"]); } -// A repository at a head commit, and a patch the model's edits make against it. +// A repository at a head commit, and a patch the model's edits make against it; +// `renames: "exact"` writes renames as collect-patch.sh does. function repoWithPatch(t, edit, { renames = false, head: seed = () => {} } = {}) { const repo = scratch(t); git(repo, ["init", "-q", "-b", "main"]); @@ -164,7 +165,7 @@ function repoWithPatch(t, edit, { renames = false, head: seed = () => {} } = {}) const head = commitAll(repo, "head"); edit(repo); git(repo, ["add", "-A"]); - const text = execFileSync("git", ["diff", "--cached", "--binary", "--full-index", renames ? "-M" : "--no-renames", head], { cwd: repo, encoding: "utf8" }); + const text = execFileSync("git", ["diff", "--cached", "--binary", "--full-index", { true: "-M", exact: "-M100%" }[renames] ?? "--no-renames", head], { cwd: repo, encoding: "utf8" }); git(repo, ["reset", "-q", "--hard", head]); const patchFile = join(scratch(t), "patch.diff"); writeFileSync(patchFile, text); @@ -1687,6 +1688,56 @@ test("issue target: the issue row refuses the universal set, each refusal before } }); +// collect-patch.sh writes an exact move as a rename header; the refusals read +// the tree the patch builds, so the old and the new path each face them. +test("issue target: an exact move pushes, the old path gone and the new one in place", async (t) => { + const fix = repoWithPatch( + t, + (repo) => { + mkdirSync(join(repo, "docs")); + renameSync(join(repo, "notes.md"), join(repo, "docs/notes.md")); + }, + { renames: "exact" }, + ); + assert.match(readFileSync(fix.patchFile, "utf8"), /^similarity index 100%\nrename from notes\.md\nrename to docs\/notes\.md$/m); + const api = fakeApi(fix.repo, fix.head, { labels: READY }); + assert.equal((await pushIssue(fix, api)).outcome, "pushed"); + const { createCommit } = api.calls.find((call) => call.createCommit); + assert.equal(git(fix.repo, ["cat-file", "blob", `${createCommit.tree}:docs/notes.md`]), "notes"); + assert.equal(git(fix.repo, ["ls-tree", "--name-only", createCommit.tree, "--", "notes.md"]), ""); +}); + +test("issue target: an exact move out of or into a refused path is refused before any API call", async (t) => { + const workflow = (repo) => (mkdirSync(join(repo, ".github/workflows"), { recursive: true }), writeFileSync(join(repo, ".github/workflows/x.yml"), "on: push\n")); + const cases = [ + ["out of .github/workflows/", workflow, (repo) => renameSync(join(repo, ".github/workflows/x.yml"), join(repo, "x.yml")), /^rename from \.github\/workflows\/x\.yml$/m, "refused (github-workflows): .github/workflows/x.yml"], + ["out of CODEOWNERS", (repo) => writeFileSync(join(repo, "CODEOWNERS"), "* @o\n"), (repo) => renameSync(join(repo, "CODEOWNERS"), join(repo, "owners.txt")), /^rename from CODEOWNERS$/m, "refused (codeowners): CODEOWNERS"], + [ + "into .github/workflows/", + () => {}, + (repo) => (mkdirSync(join(repo, ".github/workflows"), { recursive: true }), renameSync(join(repo, "notes.md"), join(repo, ".github/workflows/notes.md"))), + /^rename to \.github\/workflows\/notes\.md$/m, + "refused (rename-into-refused): .github/workflows/notes.md", + ], + [ + "into the central lane grant file", + () => {}, + (repo) => (mkdirSync(join(repo, ".github/actions/resolve-config"), { recursive: true }), renameSync(join(repo, "notes.md"), join(repo, ".github/actions/resolve-config/lane-grants.json"))), + /^rename to \.github\/actions\/resolve-config\/lane-grants\.json$/m, + "refused (rename-into-refused): .github/actions/resolve-config/lane-grants.json", + ], + ]; + for (const [what, seed, edit, header, refusal] of cases) { + const fix = repoWithPatch(t, edit, { renames: "exact", head: seed }); + assert.match(readFileSync(fix.patchFile, "utf8"), header, what); + const api = fakeApi(fix.repo, fix.head, { labels: READY }); + const lines = []; + assert.deepEqual(await pushIssue(fix, api, { log: (line) => lines.push(line) }), { outcome: "escalate", escalationReason: "patch-protected-path" }, what); + assert.deepEqual(lines, [refusal], what); + assert.deepEqual(api.calls, [], what); + } +}); + const modeIn = (repo, tree, path) => git(repo, ["ls-tree", tree, "--", path]).split(" ")[0]; test("issue target: a new or changed file that ends in .sh or starts with #! lands 100755; any other file keeps 100644", async (t) => { diff --git a/.github/scripts/read-patch/collect-patch.sh b/.github/scripts/read-patch/collect-patch.sh index db46eb9366..87195f77b7 100755 --- a/.github/scripts/read-patch/collect-patch.sh +++ b/.github/scripts/read-patch/collect-patch.sh @@ -1,7 +1,11 @@ #!/usr/bin/env bash # Writes everything a read activity changed against the gated head, committed, # staged, unstaged or untracked, as one `git diff --binary` patch, for the -# patch output mode of pr-run-activity-read.yml. Runs from the checkout after +# patch output mode of pr-run-activity-read.yml. An exact rename (100% +# similarity) is a rename header with no content, so a moved file costs a few +# lines of the patch cap; a rename with edits is a deletion and an addition. +# The apply side checks each rename's old and new path as a deletion and an +# addition of the tree it builds, whatever the patch's form. Runs from the checkout after # the activity, through a temporary index, so the checkout's own index is # untouched. Ignored files, .lane/ and the CLI's empty scrub stubs are left # out. @@ -82,7 +86,7 @@ done "${g[@]}" read-tree "$from" || fail 'could not read the start tree into a scratch index' "${g[@]}" add -A -- "${paths[@]}" || fail 'could not stage the working tree in the scratch index' "${g[@]}" diff --cached --binary --full-index --no-color --no-ext-diff --no-textconv \ - --no-renames --no-relative --submodule=short --src-prefix=a/ --dst-prefix=b/ -U3 "$from" -- "${paths[@]}" >"$patch" || + -M100% --no-relative --submodule=short --src-prefix=a/ --dst-prefix=b/ -U3 "$from" -- "${paths[@]}" >"$patch" || fail 'could not write the patch' if [[ ! -s $patch ]]; then diff --git a/.github/scripts/read-patch/collect-patch.test.sh b/.github/scripts/read-patch/collect-patch.test.sh index 6be535a4f2..7604949dd4 100755 --- a/.github/scripts/read-patch/collect-patch.test.sh +++ b/.github/scripts/read-patch/collect-patch.test.sh @@ -133,6 +133,49 @@ put edit.txt 'one\ntwo\n' run_collect check 'undone: state is empty' empty "$(field state)" +echo '# an exact move' +# A moved file is a rename header with no content, so a large move stays +# well under the patch cap. +new_repo +big=$(head -c 200000 /dev/zero | tr '\0' 'x') +mkdir -p "$R/skills/old" +printf '%s\n' "$big" >"$R/skills/old/SKILL.md" +put skills/old/run.sh '#!/bin/sh\necho run\n' +chmod +x "$R/skills/old/run.sh" +tg add -A +tg commit -q -m 'a skill' +HEAD_SHA=$(tg rev-parse HEAD) +mkdir -p "$R/skills/new" +git -C "$R" mv skills/old/SKILL.md skills/new/SKILL.md +git -C "$R" mv skills/old/run.sh skills/new/run.sh +rmdir "$R/skills/old" 2>/dev/null || true +run_collect +check 'move: exit 0' 0 "$RC" +check 'move: state is written' written "$(field state)" +check 'move: two exact renames' 2 "$(grep -c '^similarity index 100%$' "$PATCH" || true)" +check 'move: rename headers name both paths' $'rename from skills/old/SKILL.md\nrename to skills/new/SKILL.md\nrename from skills/old/run.sh\nrename to skills/new/run.sh' "$(grep '^rename ' "$PATCH")" +check 'move: no deleted or new file' 0 "$(grep -cE '^(deleted|new) file mode' "$PATCH" || true)" +check 'move: no content hunk' 0 "$(grep -c '^@@' "$PATCH" || true)" +check 'move: the patch is small' yes "$( (($(wc -c <"$PATCH") < 1024)) && echo yes || echo no)" +apply_to_fresh +check 'move: the patch applies' 0 "$APPLY_RC" +check 'move: the applied tree matches' yes "$SAME" + +echo '# a move with an edit' +# Below 100% similarity a move stays a deletion and an addition. +new_repo +mkdir -p "$R/docs" +git -C "$R" mv edit.txt docs/edit.txt +put docs/edit.txt 'one\ntwo\nthree\n' +run_collect +check 'edited move: exit 0' 0 "$RC" +check 'edited move: no rename header' 0 "$(grep -c '^rename ' "$PATCH" || true)" +check 'edited move: the old path is deleted' 1 "$(grep -c '^deleted file mode' "$PATCH" || true)" +check 'edited move: the new path is added' 1 "$(grep -c '^new file mode' "$PATCH" || true)" +apply_to_fresh +check 'edited move: the patch applies' 0 "$APPLY_RC" +check 'edited move: the applied tree matches' yes "$SAME" + echo '# repository config' # Config the activity could set does not change the patch's form. new_repo @@ -147,7 +190,8 @@ put edit.txt 'one\nTWO\n' run_collect check 'config: exit 0' 0 "$RC" check 'config: no color codes' 0 "$(grep -c $'\e' "$PATCH" || true)" -check 'config: no rename headers' 0 "$(grep -c '^rename ' "$PATCH" || true)" +check 'config: diff.renames finds no copies' 0 "$(grep -c '^copy ' "$PATCH" || true)" +check 'config: the exact move is one rename' $'rename from keep.txt\nrename to moved.txt' "$(grep '^rename ' "$PATCH")" check 'config: a/ and b/ prefixes' 'diff --git a/edit.txt b/edit.txt' "$(grep '^diff --git a/edit.txt' "$PATCH")" apply_to_fresh check 'config: the patch applies' 0 "$APPLY_RC" diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 04df3cc98a..b0efa02a37 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -622,7 +622,9 @@ set `applied-by`, and an issue-mode activity must set it. The steps: 2. After an act step that succeeded, in place of the clean-tree check, the collector writes everything the activity changed against the gated head (committed, staged, unstaged and untracked, ignored files and `.lane/` aside) as one `git diff --binary` patch, through a scratch index, with - every diff option that changes the patch's form passed explicitly. Its git runs against a fresh + every diff option that changes the patch's form passed explicitly. An exact rename (100% + similarity) is a rename header with no content, so a moved file adds a few lines to the patch + cap; a rename with edits is a deletion and an addition. Its git runs against a fresh repository that borrows only the checkout's objects, with no global or system config, so no filter, hook, fsmonitor or include the activity wrote into `.git` runs. A failed, cancelled or turnless act step collects nothing and the check fails as usual. The collector also leaves out