fix(bump-callers): reuse one open PR per repo via a stable branch (BE-3882)

.github/bump-callers/README.md

@@ -6,10 +6,16 @@ a SHA-bump PR in every repo that pins a caller against it — so consumers move
 forward automatically instead of silently drifting commits behind.
 
 - **`bump-callers.sh`** — the one, fleet-agnostic bump script (parse the caller
-  list, mask private repo names, rewrite the pin, open one PR per caller). It is
-  the single source of truth; the two workflow entrypoints are thin wrappers
-  that only supply per-fleet parameters. A forked copy is how other shared
-  machinery in the org has drifted — this stays one file on purpose.
+  list, mask private repo names, rewrite the pin, keep one bump PR per caller
+  current). It is the single source of truth; the two workflow entrypoints are
+  thin wrappers that only supply per-fleet parameters. A forked copy is how other
+  shared machinery in the org has drifted — this stays one file on purpose.
+  - **One open bump PR per (repo, fleet), updated in place.** The head branch is
+    stable (`ci/bump-<tag>`, not SHA-stamped), so each bump rebuilds that branch
+    from the caller's current default-branch tip (a clean single-commit "bump to
+    @SHORT" diff) and, if a bump PR is already open, refreshes its title/body to
+    the new SHA rather than opening another. A fresh PR is opened only when none
+    is open (first bump, or the prior one merged/closed since the last run).
 - **`tests/`** — a `bash` functional suite (stubs `gh`, no network), run by
   [`test-bump-callers.yml`](../workflows/test-bump-callers.yml) plus shellcheck.
 

.github/bump-callers/bump-callers.sh

@@ -47,7 +47,11 @@ CALLERS_JSON="${CALLERS_JSON-}"
 ALLOW_EMPTY="${ALLOW_EMPTY:-false}"
 
 SHORT="${NEW_SHA:0:7}"
-BRANCH="ci/bump-${TAG}-${SHORT}"
+# Stable branch per (repo, TAG) — deliberately NOT SHA-stamped. A fixed head
+# branch is what lets a subsequent bump reuse (and update in place) the one open
+# bump PR instead of opening a fresh PR on every SHA (BE-3882). The SHA lives in
+# the commit/title/body, not the branch name.
+BRANCH="ci/bump-${TAG}"
 
 STRIPPED="${CALLERS_JSON//[[:space:]]/}"
 
@@ -85,97 +89,257 @@ if (( ${#CALLERS[@]} == 0 )); then
   exit 1
 fi
 
-# Bump a single caller repo. Every external call is guarded, so a bad repo
-# returns non-zero here (caught by the loop) instead of killing the run. An
-# expected skip (file missing / already pinned) is a successful `return 0`.
-bump_one() {
-  local ENTRY="$1" REPO FILE LABEL FILE_ENC
-  REPO=$(cut -d'|' -f1 <<<"$ENTRY")
-  FILE=$(cut -d'|' -f2 <<<"$ENTRY")
-  LABEL=$(cut -d'|' -f3 <<<"$ENTRY")
-  FILE_ENC="${FILE//\//%2F}"
+# Bump ONE caller repo, committing EVERY file that repo pins onto a single
+# stable bump branch. Called once per repo with that repo's entries, so a repo
+# listed more than once in the caller variable (a monorepo pinning the reusable
+# workflow from several workflow files) lands ALL its files on ONE branch/PR.
+# Every external call is guarded, so a bad repo returns non-zero here (caught by
+# the loop) instead of killing the run. A repo whose files are all missing or
+# already pinned commits nothing and is a successful skip (`return 0`).
+#
+# Grouping by repo is load-bearing: the branch is reset to the default-branch
+# tip ONCE per repo, then each file is committed onto it. The previous per-entry
+# loop reset the branch before EACH file, so a second same-repo file's reset
+# discarded the first file's commit and the PR shipped only the last file — a
+# silent partial bump (BE-3896).
+bump_repo() {
+  local REPO="$1"; shift   # remaining args: this repo's "repo|file|label" entries
 
   local DEFAULT_BRANCH
   DEFAULT_BRANCH=$(gh api "repos/${REPO}" --jq '.default_branch') || {
     echo "::warning::${REPO}: cannot read repo metadata — skipping"
     return 1
   }
 
-  # Fetch current file; a missing file is an expected skip (success).
-  local CURRENT
-  CURRENT=$(gh api "repos/${REPO}/contents/${FILE_ENC}?ref=${DEFAULT_BRANCH}" 2>/dev/null) || {
-    echo "::warning::${REPO}: ${FILE} not found — skipping"
-    return 0
+  # Resolve the default-branch tip ONCE, up front, and pin every Pass-1 blob
+  # fetch to this immutable SHA (not the mutable `?ref=<branch>`). This closes a
+  # TOCTOU race: if the caller's default branch moved between the fetches and the
+  # reset below, a staged blob `sha` would be stale against the new tip and its
+  # Pass-2 PUT would 409 after earlier files already committed. Reading the tip
+  # is a GET (no branch churn), so it is safe before the all-skip early return.
+  local MAIN_SHA
+  MAIN_SHA=$(gh api "repos/${REPO}/git/refs/heads/${DEFAULT_BRANCH}" --jq '.object.sha') || {
+    echo "::warning::${REPO}: cannot read ${DEFAULT_BRANCH} ref — skipping"
+    return 1
   }
 
-  local BLOB_SHA OLD_CONTENT
-  BLOB_SHA=$(jq -r '.sha' <<<"$CURRENT")
-  OLD_CONTENT=$(jq -r '.content' <<<"$CURRENT" | base64 -d)
+  # Pass 1 — stage every file that actually needs a bump WITHOUT writing
+  # anything yet. This keeps the reset/commit path off entirely for a repo whose
+  # files are all missing or already pinned (the old per-entry skips), so it
+  # never resets a branch or opens a PR it doesn't need.
+  local -a PEND_FILE_ENC=() PEND_CONTENT=() PEND_BLOB=() LABELS=()
+  local ENTRY FILE LABEL FILE_ENC CURRENT BLOB_SHA OLD_CONTENT NEW_CONTENT
+  for ENTRY in "$@"; do
+    FILE=$(cut -d'|' -f2 <<<"$ENTRY")
+    LABEL=$(cut -d'|' -f3 <<<"$ENTRY")
+    FILE_ENC="${FILE//\//%2F}"
+
+    # De-duplicate by file: a repo listed twice for the SAME path (a structurally
+    # valid config) must stage that file ONCE. A second PUT carrying the same
+    # pre-bump blob `sha` would 409 — the first PUT already moved the blob — and
+    # fail the whole repo even though the file was actually bumped. Fold the
+    # duplicate entry's label into the set (it still applies to the one PR) but
+    # do not re-stage the file.
+    local SEEN_FILE=0 P
+    for P in ${PEND_FILE_ENC[@]+"${PEND_FILE_ENC[@]}"}; do
+      [[ "$P" == "$FILE_ENC" ]] && { SEEN_FILE=1; break; }
+    done
+    if (( SEEN_FILE )); then
+      [[ -n "$LABEL" ]] && LABELS+=("$LABEL")
+      continue
+    fi
+
+    # Fetch current file, pinned to the resolved tip. Distinguish a genuine 404
+    # (this file is actually absent → an expected per-file skip) from ANY other
+    # failure (auth, rate limit, 5xx, network). Treating a transient error as
+    # "not found" and continuing would open a PR that silently OMITS this file
+    # while the job still reports success — the partial-bump class this refactor
+    # exists to kill (BE-3896). So a non-404 failure fails the whole repo.
+    local ERRFILE RC
+    ERRFILE=$(mktemp)
+    CURRENT=$(gh api "repos/${REPO}/contents/${FILE_ENC}?ref=${MAIN_SHA}" 2>"$ERRFILE"); RC=$?
+    if (( RC != 0 )); then
+      if grep -qi 'HTTP 404' "$ERRFILE"; then
+        echo "::warning::${REPO}: ${FILE} not found — skipping"
+        rm -f "$ERRFILE"
+        continue
+      fi
+      echo "::warning::${REPO}: ${FILE} fetch failed (HTTP error, not 404) — failing repo to avoid a partial bump"
+      rm -f "$ERRFILE"
+      return 1
+    fi
+    rm -f "$ERRFILE"
 
-  # Already pinned to this SHA → nothing to do (success).
-  if grep -qF "$NEW_SHA" <<<"$OLD_CONTENT"; then
-    echo "${REPO}: already at ${SHORT} — skipping"
+    BLOB_SHA=$(jq -r '.sha' <<<"$CURRENT")
+    OLD_CONTENT=$(jq -r '.content' <<<"$CURRENT" | base64 -d)
+
+    # Rewrite the github-workflows pin(s) to NEW_SHA and normalize the stale
+    # `# github-workflows#NN` pin comment. Anchor the 40-hex substitution to the
+    # two known pin contexts — the `uses: …Comfy-Org/github-workflows…@<sha>`
+    # line and agents-md-integrity's bare `workflows_ref: <sha>` line — so a
+    # full-SHA pin of ANOTHER action in the same file (`actions/checkout@<sha>`,
+    # the org's mandated practice) is never clobbered to github-workflows' SHA.
+    # The comment rewrite is a no-op for callers that use a different comment
+    # form (e.g. agents-md-integrity's `# v1`), so it is safe to share.
+    NEW_CONTENT=$(sed -E "/github-workflows|workflows_ref/ s/[0-9a-f]{40}/${NEW_SHA}/g; s|# github-workflows#[0-9]+|# github-workflows main (${SHORT})|g" <<<"$OLD_CONTENT")
+
+    # Already fully pinned → the rewrite is a no-op → nothing to do for this file.
+    # Comparing the rewritten content to the original (rather than grepping for
+    # NEW_SHA appearing *anywhere*) also repairs a half-bumped file: if a prior
+    # run left one of two refs at NEW_SHA and the other stale, the content still
+    # differs here, so the file is re-staged and repaired instead of skipped.
+    if [[ "$NEW_CONTENT" == "$OLD_CONTENT" ]]; then
+      echo "${REPO}: ${FILE} already at ${SHORT} — skipping"
+      continue
+    fi
+
+    # Collect the label ONLY now that the file is confirmed staged. Labels from
+    # entries that were skipped (missing / already-pinned) must not land on the
+    # repo's real bump PR — a stray blocking label (e.g. `do-not-merge`) on a
+    # skipped entry would else be applied to the actual bump.
+    [[ -n "$LABEL" ]] && LABELS+=("$LABEL")
+
+    # `$(...)` capture above (base64 -d, then sed) strips the file's trailing
+    # newline, so printf '%s\n' restores exactly one — otherwise the committed
+    # caller loses its EOF newline and trips the receiver repo's formatter
+    # (oxfmt/prettier both require a trailing newline).
+    PEND_FILE_ENC+=("$FILE_ENC")
+    PEND_CONTENT+=("$(printf '%s\n' "$NEW_CONTENT" | base64 | tr -d '\n')")
+    PEND_BLOB+=("$BLOB_SHA")
+  done
+
+  # Nothing to bump for this repo → clean skip: no branch churn, no PR.
+  if (( ${#PEND_FILE_ENC[@]} == 0 )); then
     return 0
   fi
 
-  # Replace every 40-char hex SHA (the caller's pin — cursor-review has one,
-  # agents-md-integrity has two: the `uses: ...@<sha>` and its `workflows_ref`)
-  # and normalize the stale `# github-workflows#NN` pin comment. The comment
-  # rewrite is a no-op for callers that use a different comment form (e.g.
-  # agents-md-integrity's `# v1`), so it is safe to share across fleets.
-  local NEW_CONTENT
-  NEW_CONTENT=$(sed -E "s/[0-9a-f]{40}/${NEW_SHA}/g; s|# github-workflows#[0-9]+|# github-workflows main (${SHORT})|g" <<<"$OLD_CONTENT")
+  # Rebuild the stable bump branch from the resolved default-branch tip (MAIN_SHA,
+  # captured up front) ONCE for the repo, so the PR is always a clean "bump to
+  # @SHORT" diff — it never accumulates stale commits across bumps and never
+  # drifts if the caller's default branch moved since the last bump (BE-3882).
+  # Create the branch at the tip; if it already exists (an open bump PR, or
+  # residue from a prior run) force it back to the tip so the diff is rebuilt
+  # from scratch rather than committed on top of the old bump.
+  if ! gh api --method POST "repos/${REPO}/git/refs" \
+        --field ref="refs/heads/${BRANCH}" \
+        --field sha="${MAIN_SHA}" >/dev/null 2>&1; then
+    gh api --method PATCH "repos/${REPO}/git/refs/heads/${BRANCH}" \
+      --field sha="${MAIN_SHA}" --field force=true >/dev/null 2>&1 || {
+      echo "::warning::${REPO}: cannot reset ${BRANCH} to ${DEFAULT_BRANCH} tip — skipping"
+      return 1
+    }
+  fi
 
-  # Create the branch from the default-branch tip (ignore 422 if it exists).
-  local MAIN_SHA
-  MAIN_SHA=$(gh api "repos/${REPO}/git/refs/heads/${DEFAULT_BRANCH}" --jq '.object.sha') || {
-    echo "::warning::${REPO}: cannot read ${DEFAULT_BRANCH} ref — skipping"
-    return 1
-  }
-  gh api --method POST "repos/${REPO}/git/refs" \
-    --field ref="refs/heads/${BRANCH}" \
-    --field sha="${MAIN_SHA}" 2>/dev/null || true
-
-  # Commit the updated file onto the branch. `$(...)` capture above (base64 -d,
-  # then sed) strips the file's trailing newline, so printf '%s\n' restores
-  # exactly one — otherwise the committed caller loses its EOF newline and trips
-  # the receiver repo's formatter (oxfmt/prettier both require a trailing
-  # newline).
-  local ENCODED
-  ENCODED=$(printf '%s\n' "$NEW_CONTENT" | base64 | tr -d '\n')
-  gh api --method PUT "repos/${REPO}/contents/${FILE_ENC}" \
-    --field message="ci: bump ${TAG} to github-workflows@${SHORT}" \
-    --field content="${ENCODED}" \
-    --field sha="${BLOB_SHA}" \
-    --field branch="${BRANCH}" \
-    > /dev/null || {
-    echo "::warning::${REPO}: commit to ${BRANCH} failed — skipping"
-    return 1
-  }
+  # Pass 2 — commit each staged file onto the (single) branch in turn. Each PUT
+  # carries that file's own blob SHA from the tip; because the files are
+  # distinct, committing one never changes another's blob, so every PUT applies
+  # cleanly on top of the previous one and the branch accumulates ALL of them.
+  local i
+  for (( i = 0; i < ${#PEND_FILE_ENC[@]}; i++ )); do
+    gh api --method PUT "repos/${REPO}/contents/${PEND_FILE_ENC[$i]}" \
+      --field message="ci: bump ${TAG} to github-workflows@${SHORT}" \
+      --field content="${PEND_CONTENT[$i]}" \
+      --field sha="${PEND_BLOB[$i]}" \
+      --field branch="${BRANCH}" \
+      > /dev/null || {
+      echo "::warning::${REPO}: commit to ${BRANCH} failed — skipping"
+      return 1
+    }
+  done
 
-  local LABEL_ARGS=()
-  [[ -n "$LABEL" ]] && LABEL_ARGS=(--label "$LABEL")
+  # The title/body embed ${SHORT}. Build them once so the create and the
+  # update-in-place paths post the identical, current text. The body is a single
+  # line so the workflow parses it — a multi-line --body collapsed the step's
+  # YAML in an earlier revision.
+  local PR_TITLE PR_BODY
+  PR_TITLE="ci: bump ${TAG} to github-workflows@${SHORT}"
+  PR_BODY="Automatic SHA bump — \`${WORKFLOW_FILE}\` was updated in \`Comfy-Org/github-workflows\` at [\`${SHORT}\`](https://github.com/Comfy-Org/github-workflows/commit/${NEW_SHA}). _Opened by the \`bump-${TAG}-callers\` workflow._"
 
-  # Open the PR (a pre-existing PR for this branch is not an error). The body is
-  # a single line so the workflow parses it — a multi-line --body collapsed the
-  # step's YAML in an earlier revision.
+  # Reuse the one open bump PR for this branch if there is one: the branch push
+  # above already refreshed its diff to the new SHA, so just refresh its
+  # title/body. The stable head branch guarantees at most ONE open bump PR per
+  # (repo, TAG) at any time — never open a second (BE-3882).
+  #
+  # `.[0].number // empty` (not a bare `.[0].number`, which prints the literal
+  # `null` on an empty list and would send us down `gh pr edit null`). And
+  # because `--head` matches by branch *name* across forks, exclude
+  # cross-repository PRs (`isCrossRepository == false`): the branch name is now
+  # predictable (`ci/bump-<tag>`), so an attacker could pre-open a fork PR on it
+  # and have the bot stamp their PR with the official title/body instead of
+  # bumping the real caller. Only the caller repo's own bump branch counts.
+  local EXISTING_PR
+  EXISTING_PR=$(gh pr list --repo "${REPO}" --head "${BRANCH}" --state open \
+    --json number,isCrossRepository \
+    --jq 'map(select(.isCrossRepository == false)) | .[0].number // empty' 2>/dev/null)
+  if [[ -n "$EXISTING_PR" ]]; then
+    if gh pr edit "${EXISTING_PR}" --repo "${REPO}" \
+        --title "${PR_TITLE}" --body "${PR_BODY}" >/dev/null 2>&1; then
+      echo "${REPO}: PR #${EXISTING_PR} updated to ${SHORT}"
+    else
+      echo "::warning::${REPO}: failed to update open PR #${EXISTING_PR}"
+      return 1
+    fi
+    return 0
+  fi
+
+  # No open PR for this branch → open one (first bump, or the prior PR was
+  # merged/closed — and its branch possibly auto-deleted — since the last run).
+  # Apply every distinct non-empty label the repo's entries carried (they need
+  # not agree; dedup so `gh pr create` isn't passed the same label twice).
+  # Exact-match membership (not a `|${L}|` substring sentinel): GitHub label
+  # names may themselves contain `|`, so a substring test could drop a distinct
+  # label whose name is a substring of an already-seen one (e.g. `bug` vs `bug|ui`).
+  local -a LABEL_ARGS=() SEEN_LABELS=()
+  local L S DUP
+  for L in ${LABELS[@]+"${LABELS[@]}"}; do
+    DUP=0
+    for S in ${SEEN_LABELS[@]+"${SEEN_LABELS[@]}"}; do
+      [[ "$S" == "$L" ]] && { DUP=1; break; }
+    done
+    (( DUP )) && continue
+    SEEN_LABELS+=("$L"); LABEL_ARGS+=(--label "$L")
+  done
   if gh pr create \
     --repo "${REPO}" \
     --head "${BRANCH}" \
     --base "${DEFAULT_BRANCH}" \
-    --title "ci: bump ${TAG} to github-workflows@${SHORT}" \
-    --body "Automatic SHA bump — \`${WORKFLOW_FILE}\` was updated in \`Comfy-Org/github-workflows\` at [\`${SHORT}\`](https://github.com/Comfy-Org/github-workflows/commit/${NEW_SHA}). _Opened by the \`bump-${TAG}-callers\` workflow._" \
+    --title "${PR_TITLE}" \
+    --body "${PR_BODY}" \
     "${LABEL_ARGS[@]}" 2>/dev/null; then
     echo "${REPO}: PR opened"
+    return 0
   else
-    echo "::warning::${REPO}: PR may already exist for ${BRANCH}"
+    # Reaching here means no open bump PR existed (the update-in-place path
+    # above already handled that case), so a create failure is a real error
+    # — auth, branch protection, an invalid label, a transient API fault — not
+    # a benign "PR already exists". Fail the caller so it lands in FAILED and
+    # the job reports the miss instead of a silent success.
+    echo "::warning::${REPO}: PR create failed for ${BRANCH}"
+    return 1
   fi
-  return 0
 }
 
-FAILED=()
+# Group the flat entry list by repo, preserving first-seen order, so each repo
+# is bumped exactly once with ALL of its files (BE-3896). `|` can't appear in a
+# repo name (it is the field delimiter), so it is a safe membership sentinel.
+REPOS=()
+SEEN_REPOS=""
 for ENTRY in "${CALLERS[@]}"; do
-  bump_one "$ENTRY" || FAILED+=("${ENTRY%%|*}")
+  REPO="${ENTRY%%|*}"
+  case "$SEEN_REPOS" in
+    *"|${REPO}|"*) ;;
+    *) REPOS+=("$REPO"); SEEN_REPOS="${SEEN_REPOS}|${REPO}|" ;;
+  esac
+done
+
+FAILED=()
+for REPO in "${REPOS[@]}"; do
+  # Collect this repo's entries (in their original order) and bump them together.
+  ENTRIES=()
+  for ENTRY in "${CALLERS[@]}"; do
+    [[ "${ENTRY%%|*}" == "$REPO" ]] && ENTRIES+=("$ENTRY")
+  done
+  bump_repo "$REPO" "${ENTRIES[@]}" || FAILED+=("$REPO")
 done
 
 if (( ${#FAILED[@]} )); then