From 5265b13f857adcc26ebb317607174a1e4c481aa6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 12:20:09 +0000 Subject: [PATCH] Verify updated PRs' diffs and wake GitHub's recompute when stale After a PR's base is edited, GitHub sometimes keeps serving the diff computed against the old base; only a fresh event on the PR triggers another recompute (see scortexio/gh-stack-mv#37, which works around the same bug). The retarget is the last event this action feeds a PR, so an affected child PR durably shows its parent's changes in "Files changed". Add an advisory pass after the retargets in all three modes: compare the diff GitHub serves for each updated PR against a local git diff of live-fetched refs, and on mismatch re-assert the PR's current base (a same-value edit that feeds the recompute a fresh event), re-checking a bounded number of times. Never fails the run; when the diff stays stale after the nudge, posts a comment on the PR with the manual fix. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019o5g4zMNLzTikiyFEghTVM --- README.md | 1 + tests/mock_gh.sh | 36 +++++- tests/mock_git.sh | 6 +- tests/test_conflict_resolution_resume.sh | 19 ++- tests/test_merge_commit_merge.sh | 12 ++ tests/test_update_pr_stack.sh | 77 ++++++++++++ update-pr-stack.sh | 142 +++++++++++++++++++++++ 7 files changed, 286 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index cf6b223..16c2d9c 100644 --- a/README.md +++ b/README.md @@ -22,6 +22,7 @@ This action tries to fix that in a transparent way. Install it, and hopefully th 4. Pushes the updated branches 5. Updates the direct child PRs to base on trunk now that the bottom change has landed 6. Deletes the merged branch +7. Checks that GitHub actually serves each updated PR's diff consistently with the refs. After a base change, GitHub sometimes keeps serving the diff computed against the old base until a fresh event on the PR triggers a recompute, so the "Files changed" tab would durably show the parent's changes. On a mismatch the action re-asserts the PR's current base (a same-value edit that feeds the recompute a fresh event) and re-checks a few times; if the diff stays stale it posts a comment on the PR with the manual fix. This step is advisory and never fails the run. The re-parent primitive ([git-merge-onto](https://github.com/scortexio/git-merge-onto)) is vendored as a single zero-dependency file and run with `python3`, so the action needs no network download. diff --git a/tests/mock_gh.sh b/tests/mock_gh.sh index cfeafb9..5ee2f06 100755 --- a/tests/mock_gh.sh +++ b/tests/mock_gh.sh @@ -18,10 +18,44 @@ if [[ "$1" == "api" && "$2" == repos/*"/pulls?base="* ]]; then # No other bases have direct children in our test scenario : fi +elif [[ "$1" == "pr" && "$2" == "view" && "$*" == *"--json baseRefName"* ]]; then + # Only the served-diff verification asks; by then the PR has been + # retargeted onto the target branch. + echo "${TARGET_BRANCH:-main}" +elif [[ "$1" == "pr" && "$2" == "diff" ]]; then + # The diff GitHub serves for the PR. While the MOCK_STALE_DIFFS file + # exists, serve garbage instead, simulating the staleness bug the + # verification pass works around; see `pr edit` below for how it heals. + if [[ -n "${MOCK_STALE_DIFFS:-}" && -e "$MOCK_STALE_DIFFS" ]]; then + echo "stale diff from before the base update" + elif [[ "$3" == 2 ]]; then + # PR #2 is feature2 (see the pulls query above). Neutral config so + # the output has the shape GitHub serves, whatever the test machine's + # gitconfig says. + GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null \ + git diff "origin/${TARGET_BRANCH:-main}...origin/feature2" + else + echo "Unknown PR number for pr diff: $3" >&2 + exit 1 + fi elif [[ "$1" == "pr" && "$2" == "edit" ]]; then - # Just log the edit command + # A PR event feeds GitHub's diff recompute a fresh chance to run: count + # down the edits the staleness survives, then heal the served diff. + if [[ -n "${MOCK_STALE_DIFFS:-}" && -e "$MOCK_STALE_DIFFS" ]]; then + N=$(cat "$MOCK_STALE_DIFFS") + if [[ "$N" -gt 0 ]]; then + echo "$((N - 1))" > "$MOCK_STALE_DIFFS" + else + rm "$MOCK_STALE_DIFFS" + fi + fi echo "Mock: gh pr edit $3 --base $5" elif [[ "$1" == "pr" && "$2" == "comment" ]]; then + # A `-F -` body arrives on stdin; consume it so the writer does not die + # on a broken pipe. + if [[ "$*" == *"-F"* ]]; then + cat > /dev/null + fi # Just log the comment command echo "Mock: gh pr comment $3" elif [[ "$1" == "api" && "$2" == repos/*/commits/*/pulls ]]; then diff --git a/tests/mock_git.sh b/tests/mock_git.sh index 136b4a3..e6923f1 100755 --- a/tests/mock_git.sh +++ b/tests/mock_git.sh @@ -1,8 +1,10 @@ #!/bin/bash -if [[ "$1" == "push" ]]; then - # Log the attempt but don't execute, preventing failure +if [[ "$1" == "push" || "$1" == "fetch" ]]; then + # Log the attempt but don't execute: the test repos have no real remote. + # push would fail outright; refs/remotes/origin/* stand for the remote's + # state and are maintained by the tests' simulate_push instead. printf "Executing (mocked):" >&2 printf " %q" "git" "$@" >&2 printf "\n" >&2 diff --git a/tests/test_conflict_resolution_resume.sh b/tests/test_conflict_resolution_resume.sh index 45d37c7..242e0f2 100644 --- a/tests/test_conflict_resolution_resume.sh +++ b/tests/test_conflict_resolution_resume.sh @@ -24,8 +24,10 @@ ok() { echo "✅ $1"; PASS=$((PASS+1)); } # MOCK_COMMENTS_FILE file served as the body of our own PR comments # MOCK_LABELS_FAIL set to 1 to make `pr view --json labels` fail # MOCK_PR_LIST_FAIL set to 1 to make `pr list` fail -# The PR's base branch is not mocked: the script must take it from PR_BASE -# (event payload), so a baseRefName query is an unhandled call and fails. +# The resume decision must take the PR's base from PR_BASE (event payload), +# never from a live baseRefName query; only the served-diff verification at +# the very end of a successful resume may ask, and by then the PR has been +# retargeted onto main. make_mock_gh() { local dir="$1" cat > "$dir/mock_gh.sh" <<'EOF' @@ -37,8 +39,16 @@ if [[ "$1 $2" == "pr view" ]]; then *--json\ labels*) [[ "${MOCK_LABELS_FAIL:-}" == 1 ]] && { echo "mock gh: labels API down" >&2; exit 1; } printf '%s\n' "${MOCK_LABELS:-}";; + *--json\ baseRefName*) + echo main;; *) echo "unhandled pr view: $*" >&2; exit 1;; esac +elif [[ "$1 $2" == "pr diff" ]]; then + # The diff GitHub serves for the resumed PR (#5, branch child). Neutral + # config so the output has the shape GitHub serves, whatever the test + # machine's gitconfig says. + GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null \ + git diff origin/main...origin/child elif [[ "$1 $2" == "api graphql" ]]; then # The comments file stands for our own comments only, so the query must # restrict itself to those. @@ -98,7 +108,7 @@ setup_repo() { run_resume() { env ACTION_MODE=conflict-resolved PR_BRANCH=child PR_NUMBER=5 PR_BASE="$PR_BASE" \ - GITHUB_REPOSITORY=tester/repo \ + GITHUB_REPOSITORY=tester/repo VERIFY_POLL_SECONDS=0 \ GH="$MOCK_DIR/mock_gh.sh" GIT="$MOCK_DIR/mock_git.sh" \ MOCK_LABELS="$MOCK_LABELS" MOCK_LABELS_FAIL="${MOCK_LABELS_FAIL:-}" \ MOCK_COMMENTS_FILE="$MOCK_COMMENTS_FILE" CALLS="$CALLS" \ @@ -161,7 +171,8 @@ base_line=$(grep -n -- "--base main" "$CALLS" | head -1 | cut -d: -f1) label_line=$(grep -n "remove-label" "$CALLS" | head -1 | cut -d: -f1) [[ "$push_line" -lt "$base_line" ]] || fail "C: push must come before base edit" [[ "$base_line" -lt "$label_line" ]] || fail "C: base edit must come before label removal" -ok "C: resume pushes, retargets base, then removes label" +grep -q "✓ GitHub serves the expected diff for PR #5" "$WORK/out.log" || fail "C: served diff not verified" +ok "C: resume pushes, retargets base, removes label, verifies the served diff" # --------------------------------------------------------------------------- echo "### Scenario D: recorded target branch is gone -> give up cleanly" diff --git a/tests/test_merge_commit_merge.sh b/tests/test_merge_commit_merge.sh index af5b57a..b596a58 100755 --- a/tests/test_merge_commit_merge.sh +++ b/tests/test_merge_commit_merge.sh @@ -83,6 +83,18 @@ if [[ "$EDIT_LINE" -gt "$DELETE_LINE" ]]; then exit 1 fi +# The served-diff verification is advisory, so it must come after the +# deletion, the last mutation the run owes the stack. +VERIFY_LINE=$(grep -n "✓ GitHub serves the expected diff for PR #2" <<<"$OUT" | cut -d: -f1 | head -1 || true) +if [[ -z "$VERIFY_LINE" ]]; then + echo "❌ Served diff must be verified after the retarget" + exit 1 +fi +if [[ "$VERIFY_LINE" -lt "$DELETE_LINE" ]]; then + echo "❌ Verification must come after the branch deletion (verify=$VERIFY_LINE delete=$DELETE_LINE)" + exit 1 +fi + # The untouched head keeps a clean diff against the new base ACTUAL_DIFF=$(git diff main...feature2 | grep '^[+-]' | grep -v '^[+-][+-][+-]') if [[ "$ACTUAL_DIFF" != "+f2" ]]; then diff --git a/tests/test_update_pr_stack.sh b/tests/test_update_pr_stack.sh index 7689141..7cbed98 100755 --- a/tests/test_update_pr_stack.sh +++ b/tests/test_update_pr_stack.sh @@ -104,6 +104,16 @@ else exit 1 fi +# The run must end by checking the diff GitHub serves for the retargeted PR +# against the refs (the mock serves a diff computed from the same refs, so +# they match). +if grep -q "✓ GitHub serves the expected diff for PR #2" "$RUN_LOG"; then + echo "✅ Served diff verified against the refs" +else + echo "❌ Served-diff verification did not pass" + exit 1 +fi + # Verify the results cd "$TEST_REPO" @@ -212,6 +222,73 @@ else exit 1 fi +# GitHub sometimes keeps serving a retargeted PR's diff computed against the +# old base until a fresh event triggers a recompute. The mock simulates it +# with a countdown file: `pr diff` serves garbage while the file exists, and +# each `pr edit` decrements it, clearing it at zero. With a count of 1 the +# initial retarget leaves the diff stale and the verification's nudge (a +# same-value base edit) heals it. +echo -e "\nRunning update script with a stale served diff (heals on the nudge)..." +STALE_FILE="$TEST_REPO/stale_diffs" +echo 1 > "$STALE_FILE" +STALE_LOG=$(mktemp) +log_cmd \ + env \ + SQUASH_COMMIT=$SQUASH_COMMIT \ + MERGED_BRANCH=feature1 \ + PR_NUMBER=1 \ + TARGET_BRANCH=main \ + MOCK_STALE_DIFFS="$STALE_FILE" \ + VERIFY_POLL_SECONDS=0 \ + GH="$SCRIPT_DIR/mock_gh.sh" \ + GIT="$SCRIPT_DIR/mock_git.sh" \ + $SCRIPT_DIR/../update-pr-stack.sh 2>&1 | tee "$STALE_LOG" + +NUDGES=$(grep -c "Mock: gh pr edit 2 --base main" "$STALE_LOG" || true) +if [[ "$NUDGES" -eq 2 ]]; then + echo "✅ Stale diff nudged with a same-value base edit" +else + echo "❌ Expected the retarget plus one nudge, saw $NUDGES base edits" + exit 1 +fi +if grep -q "✓ GitHub serves the expected diff for PR #2" "$STALE_LOG"; then + echo "✅ Stale served diff healed after the nudge" +else + echo "❌ Served diff still unverified after the nudge" + exit 1 +fi +if grep -q "Mock: gh pr comment" "$STALE_LOG"; then + echo "❌ No comment must be posted when the nudge heals the diff" + exit 1 +fi + +# When the staleness survives the nudge and every re-check, the run must warn +# the user with a PR comment and still end green: the stack update itself +# succeeded. +echo -e "\nRunning update script with a served diff that never heals..." +echo 99 > "$STALE_FILE" +STUCK_LOG=$(mktemp) +log_cmd \ + env \ + SQUASH_COMMIT=$SQUASH_COMMIT \ + MERGED_BRANCH=feature1 \ + PR_NUMBER=1 \ + TARGET_BRANCH=main \ + MOCK_STALE_DIFFS="$STALE_FILE" \ + VERIFY_POLL_SECONDS=0 \ + GH="$SCRIPT_DIR/mock_gh.sh" \ + GIT="$SCRIPT_DIR/mock_git.sh" \ + $SCRIPT_DIR/../update-pr-stack.sh 2>&1 | tee "$STUCK_LOG" + +if grep -q "still serves a stale diff for PR #2" "$STUCK_LOG" \ + && grep -q "Mock: gh pr comment 2" "$STUCK_LOG"; then + echo "✅ Persistent staleness warns and comments on the PR without failing the run" +else + echo "❌ Persistent staleness must warn and comment on the PR" + exit 1 +fi +rm -f "$STALE_FILE" + # A failed children listing must fail the run before any mutation: silently # treating it as "no children" would delete the merged branch under the # children it never saw. diff --git a/update-pr-stack.sh b/update-pr-stack.sh index ee20077..d3fc9d9 100755 --- a/update-pr-stack.sh +++ b/update-pr-stack.sh @@ -171,6 +171,130 @@ abort_merge_if_in_progress() { fi } +# GitHub's served diff and a local `git diff` of the same trees agree on +# content but not on header noise: blob hashes (the index lines) differ +# between generators, and so can the "diff --git" line's formatting. Drop +# both, keep everything else verbatim. +normalize_diff() { + sed -e '/^index /d' -e '/^diff --git /d' +} + +# Args: PR number, head branch, base branch. Returns 0 when the diff GitHub +# serves for the PR matches one computed locally from the remote-tracking +# refs, 1 when it does not, 2 when a command failed. The local diff pins +# every config knob that shapes the text, so it reproduces GitHub's generator +# no matter what the runner's gitconfig says: diff.algorithm (hunk +# splitting), noprefix/mnemonicPrefix and the explicit --src/dst-prefix (the +# a/ b/ headers), diff.context (hunk sizes), --no-ext-diff and --no-textconv +# (content rewriting), --no-color. +served_diff_matches() { + local NUMBER="$1" BRANCH="$2" BASE="$3" + local SERVED EXPECTED + SERVED=$(try gh pr diff "$NUMBER" | normalize_diff) || return 2 + EXPECTED=$(try git -c diff.algorithm=myers -c diff.noprefix=false \ + -c diff.mnemonicPrefix=false -c diff.context=3 \ + diff --no-color --no-ext-diff --no-textconv \ + --src-prefix=a/ --dst-prefix=b/ \ + "origin/$BASE...origin/$BRANCH" | normalize_diff) || return 2 + [[ "$SERVED" == "$EXPECTED" ]] +} + +# Args: PR number, head branch. A verification step failed; the check is +# advisory, so tell the log and move on. +warn_unverified() { + echo "⚠️ Could not verify the diff GitHub serves for PR #$1 ($2)" >&2 +} + +# Args: PR number, head branch. Advisory check that GitHub actually *serves* +# the PR's diff consistently with the refs, nudging its recompute when it +# does not. Always returns 0: the stack update itself is already complete, so +# a failure here only warns (and comments on the PR when the diff stays +# stale). +# +# Observed repeatedly on real GitHub (scortexio/gh-stack-mv#37 works around +# the same bug): after `gh pr edit --base`, GitHub sometimes keeps serving +# the diff computed against the OLD base. The PR record (baseRefName) is +# correct, only the diff endpoint is stale, and it stays stale until a fresh +# event on the PR triggers another recompute. The retarget is the last event +# this action feeds a PR, so left alone the PR's "Files changed" tab durably +# shows the parent's changes -- the very thing this action exists to prevent. +verify_pr_diff() { + local NUMBER="$1" BRANCH="$2" + local BASE RC + + # The base is read LIVE from the PR, never remembered from this run's own + # retarget: a human retargeting the PR while this check runs must not + # have their edit reverted by the nudge below. + BASE=$(try gh pr view "$NUMBER" --json baseRefName --jq .baseRefName) \ + || { warn_unverified "$NUMBER" "$BRANCH"; return 0; } + # The expected diff is computed from refs fetched at verification time: + # the invariant checked is "the served diff is consistent with the refs + # as they are NOW", which holds under any concurrent push history. + try git fetch origin "refs/heads/$BASE" "refs/heads/$BRANCH" \ + || { warn_unverified "$NUMBER" "$BRANCH"; return 0; } + + RC=0; served_diff_matches "$NUMBER" "$BRANCH" "$BASE" || RC=$? + if [[ "$RC" -eq 1 ]]; then + # A push can land between the fetch above and gh's read, so a single + # mismatch may be a torn read, not staleness. Re-fetch and re-compare + # once before concluding anything. + try git fetch origin "refs/heads/$BASE" "refs/heads/$BRANCH" \ + || { warn_unverified "$NUMBER" "$BRANCH"; return 0; } + RC=0; served_diff_matches "$NUMBER" "$BRANCH" "$BASE" || RC=$? + fi + + if [[ "$RC" -eq 1 ]]; then + # Genuinely stale. Nudge: re-assert the base the PR has RIGHT NOW (a + # same-value edit); its only purpose is to feed the diff recompute a + # fresh event. Re-read rather than reuse: never write a value that + # was not on the PR moments before. + BASE=$(try gh pr view "$NUMBER" --json baseRefName --jq .baseRefName) \ + || { warn_unverified "$NUMBER" "$BRANCH"; return 0; } + try gh pr edit "$NUMBER" --base "$BASE" \ + || { warn_unverified "$NUMBER" "$BRANCH"; return 0; } + for _ in $(seq 1 5); do + sleep "${VERIFY_POLL_SECONDS:-3}" + # A concurrent push during this window would otherwise keep + # reading as "still stale": keep the expected diff in step with + # the refs. + try git fetch origin "refs/heads/$BASE" "refs/heads/$BRANCH" \ + || { warn_unverified "$NUMBER" "$BRANCH"; return 0; } + RC=0; served_diff_matches "$NUMBER" "$BRANCH" "$BASE" || RC=$? + [[ "$RC" -eq 1 ]] || break + done + fi + + case "$RC" in + 0) + echo "✓ GitHub serves the expected diff for PR #$NUMBER" + ;; + 1) + echo "⚠️ GitHub still serves a stale diff for PR #$NUMBER ($BRANCH) after a nudge" >&2 + { + echo "### ⚠️ GitHub may be showing an outdated diff" + echo + echo "After the base branch of this pull request was updated, the diff GitHub serves for it still does not match \`$BASE...$BRANCH\`, and re-asserting the base branch did not refresh it. If the *Files changed* tab looks wrong, nudge the recompute again:" + echo '```bash' + echo "gh pr edit $NUMBER --base $BASE" + echo '```' + } | try gh pr comment "$NUMBER" -F - \ + || echo "⚠️ Could not comment on PR #$NUMBER" >&2 + ;; + *) + warn_unverified "$NUMBER" "$BRANCH" + ;; + esac + return 0 +} + +# Args: PR number and head branch, repeated (N1 BRANCH1 N2 BRANCH2 ...). +verify_pr_diffs() { + while (( $# >= 2 )); do + verify_pr_diff "$1" "$2" + shift 2 + done +} + # Args: head branch, base branch, PR number. git commands use the branch; gh # commands use the number, since a head branch can carry several PRs. update_direct_target() { @@ -385,6 +509,9 @@ continue_after_resolution() { echo "Deleting old base branch '$OLD_BASE' (no other PRs depend on it)" try git push origin ":$OLD_BASE" || echo "⚠️ Could not delete '$OLD_BASE' (may already be deleted)" fi + + # Advisory, so strictly after every mutation the stack is owed. + verify_pr_diffs "$PR_NUMBER" "$PR_BRANCH" } main() { @@ -403,12 +530,18 @@ main() { if git rev-parse --verify --quiet SQUASH_COMMIT^2 >/dev/null; then echo "✓ '$MERGED_BRANCH' was merged with a merge commit, not squashed; retargeting children without touching their heads" CHILDREN=$(list_child_prs) || die "could not list the PRs based on $MERGED_BRANCH" + VERIFY_ARGS=() while read -r NUMBER BRANCH; do [[ -n "$BRANCH" ]] || continue run gh pr edit "$NUMBER" --base "$TARGET_BRANCH" + VERIFY_ARGS+=("$NUMBER" "$BRANCH") done <<<"$CHILDREN" # Deleting a PR's base branch closes the PR, so the retargets come first. run git push origin ":$MERGED_BRANCH" + # Advisory, so strictly after every mutation the stack is owed. + if [[ "${#VERIFY_ARGS[@]}" -gt 0 ]]; then + verify_pr_diffs "${VERIFY_ARGS[@]}" + fi return 0 fi @@ -468,6 +601,15 @@ main() { else echo "⚠️ Keeping branch '$MERGED_BRANCH' - still referenced by conflicted PRs: ${CONFLICTED_TARGETS[*]}" fi + + # Advisory, so strictly after every mutation the stack is owed. + VERIFY_ARGS=() + for i in "${!UPDATED_NUMBERS[@]}"; do + VERIFY_ARGS+=("${UPDATED_NUMBERS[$i]}" "${UPDATED_TARGETS[$i]}") + done + if [[ "${#VERIFY_ARGS[@]}" -gt 0 ]]; then + verify_pr_diffs "${VERIFY_ARGS[@]}" + fi } # Only run if the script is executed directly (not sourced)