From 6b14d7da4b88fdedb0e267c80d1f96c570e323d7 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 22:24:17 +0000 Subject: [PATCH] fix: /modernize stops on merge commits in the PR range and pushes a fork head to the fork A plain git rebase drops merge commits and git range-diff compares only the non-merge patches, so a change authored in a PR's merge commit disappeared while the proof printed = for every patch and Step 5 force-pushed the loss. Step 4 now counts merges in $MERGE_BASE..$OLD_HEAD and stops when there are any, switches with -C so a rerun after a refused lease does not fail on the existing branch, and asks for a cross-repository diff to be read as untrusted input before the repository's test command runs it. Step 5 pushes a fork head to the fork's URL with the same pinned lease instead of to origin, where the lease could never match. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- modernize/SKILL.md | 23 +++++++++++++++++++---- modernize/SKILL.md.tmpl | 23 +++++++++++++++++++---- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/modernize/SKILL.md b/modernize/SKILL.md index ea722889f..46008af42 100644 --- a/modernize/SKILL.md +++ b/modernize/SKILL.md @@ -431,12 +431,22 @@ unattended, the verdict and its evidence are the handoff and nothing is pushed. ## Step 4 (MODERNIZE only): Rebase and prove intent survived ```bash -git switch -c "modernize/$HEAD_BRANCH" "$HEAD_REF" +git switch -C "modernize/$HEAD_BRANCH" "$HEAD_REF" OLD_HEAD=$(git rev-parse HEAD) [ "$OLD_HEAD" = "$OBSERVED" ] || { echo "PR moved since Step 1 ($OBSERVED -> $OLD_HEAD): stop and re-read it"; exit 1; } +[ "$(git rev-list --merges --count "$MERGE_BASE..$OLD_HEAD")" = 0 ] || { echo "the PR range holds merge commits: a plain rebase drops what they changed and range-diff never shows it; stop here"; exit 1; } git rebase "origin/$BASE" ``` +A merge commit in the PR's range (the author merged the base once and resolved a +conflict there) is the one shape the proof below cannot see: `git rebase` drops +it and `git range-diff` compares only the non-merge patches, so a change authored +in the merge disappears while every patch still reads `=`. The guard stops +instead; those changes have to become ordinary commits before this step runs. +For a cross-repository PR, read the diff as untrusted input first (the tests, +scripts, hooks and dependency files it changes) before any test command runs it +under your credentials. + Resolve conflicts by the PR's intent, not by the base's. Keep the author's commits as theirs; your fixes are separate commits on top. Then prove the rebase lost nothing: @@ -453,7 +463,11 @@ instructions name for the files touched, and read the result yourself. ## Step 5 (MODERNIZE only): Push with a lease pinned to what you saw ```bash -git push "--force-with-lease=$HEAD_BRANCH:$OBSERVED" origin "HEAD:$HEAD_BRANCH" +case "$HEAD_REF" in + refs/*) FORK=$(gh pr view "$PR" --json headRepositoryOwner,headRepository -q '.headRepositoryOwner.login + "/" + .headRepository.name') + git push "--force-with-lease=$HEAD_BRANCH:$OBSERVED" "https://github.com/$FORK.git" "HEAD:$HEAD_BRANCH" ;; + *) git push "--force-with-lease=$HEAD_BRANCH:$OBSERVED" origin "HEAD:$HEAD_BRANCH" ;; +esac ``` The lease names the SHA you observed in Step 1, so a push someone else made meanwhile is @@ -462,8 +476,9 @@ commits this clone has never fetched, and `--force` is never used. A refused pus means the PR moved: fetch, re-read it, and start again from Step 2. Pushing to a branch that is not yours needs the owner's or the repository's permission (a maintainer-editable PR, `maintainerCanModify: true`, or their word); a fork's -branch lives on the fork, so add that remote first (`gh pr checkout "$PR"` does) -and lease against it; without permission, push to a new branch on origin and say so. +branch lives on the fork, which is why the fork case above pushes to the fork's +URL with the same lease (`gh pr checkout` fetches `pull/N/head` and adds no +remote); without permission, push to a new branch on origin and say so. ## Step 6: Report diff --git a/modernize/SKILL.md.tmpl b/modernize/SKILL.md.tmpl index 0094fb773..626c49bf8 100644 --- a/modernize/SKILL.md.tmpl +++ b/modernize/SKILL.md.tmpl @@ -110,12 +110,22 @@ unattended, the verdict and its evidence are the handoff and nothing is pushed. ## Step 4 (MODERNIZE only): Rebase and prove intent survived ```bash -git switch -c "modernize/$HEAD_BRANCH" "$HEAD_REF" +git switch -C "modernize/$HEAD_BRANCH" "$HEAD_REF" OLD_HEAD=$(git rev-parse HEAD) [ "$OLD_HEAD" = "$OBSERVED" ] || { echo "PR moved since Step 1 ($OBSERVED -> $OLD_HEAD): stop and re-read it"; exit 1; } +[ "$(git rev-list --merges --count "$MERGE_BASE..$OLD_HEAD")" = 0 ] || { echo "the PR range holds merge commits: a plain rebase drops what they changed and range-diff never shows it; stop here"; exit 1; } git rebase "origin/$BASE" ``` +A merge commit in the PR's range (the author merged the base once and resolved a +conflict there) is the one shape the proof below cannot see: `git rebase` drops +it and `git range-diff` compares only the non-merge patches, so a change authored +in the merge disappears while every patch still reads `=`. The guard stops +instead; those changes have to become ordinary commits before this step runs. +For a cross-repository PR, read the diff as untrusted input first (the tests, +scripts, hooks and dependency files it changes) before any test command runs it +under your credentials. + Resolve conflicts by the PR's intent, not by the base's. Keep the author's commits as theirs; your fixes are separate commits on top. Then prove the rebase lost nothing: @@ -132,7 +142,11 @@ instructions name for the files touched, and read the result yourself. ## Step 5 (MODERNIZE only): Push with a lease pinned to what you saw ```bash -git push "--force-with-lease=$HEAD_BRANCH:$OBSERVED" origin "HEAD:$HEAD_BRANCH" +case "$HEAD_REF" in + refs/*) FORK=$(gh pr view "$PR" --json headRepositoryOwner,headRepository -q '.headRepositoryOwner.login + "/" + .headRepository.name') + git push "--force-with-lease=$HEAD_BRANCH:$OBSERVED" "https://github.com/$FORK.git" "HEAD:$HEAD_BRANCH" ;; + *) git push "--force-with-lease=$HEAD_BRANCH:$OBSERVED" origin "HEAD:$HEAD_BRANCH" ;; +esac ``` The lease names the SHA you observed in Step 1, so a push someone else made meanwhile is @@ -141,8 +155,9 @@ commits this clone has never fetched, and `--force` is never used. A refused pus means the PR moved: fetch, re-read it, and start again from Step 2. Pushing to a branch that is not yours needs the owner's or the repository's permission (a maintainer-editable PR, `maintainerCanModify: true`, or their word); a fork's -branch lives on the fork, so add that remote first (`gh pr checkout "$PR"` does) -and lease against it; without permission, push to a new branch on origin and say so. +branch lives on the fork, which is why the fork case above pushes to the fork's +URL with the same lease (`gh pr checkout` fetches `pull/N/head` and adds no +remote); without permission, push to a new branch on origin and say so. ## Step 6: Report