From eec9b380a3340ccd6564f45c34740dd45048350f Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 20 Jul 2026 20:15:05 -0700 Subject: [PATCH 1/6] fix(bump-callers): reuse one open PR per repo via a stable branch (BE-3882) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dispatcher embedded the new short-SHA in the branch name (ci/bump--), so every bump minted a unique branch and thus a brand-new PR in each caller repo, leaving the prior bump PRs open. The stack made it unclear which pin was current. Make the branch stable per (repo, TAG) — ci/bump- — and update it in place: each run rebuilds the branch from the caller's current default-branch tip (a clean single-commit "bump to @SHORT" diff), then, if a bump PR is already open for that branch, refreshes its title/body to the new SHA instead of opening another; a fresh PR is opened only when none is open (first bump, or the prior one merged/closed since the last run). Result: at most one open bump PR per (repo, workflow) at any time. Applies to both the cursor-review and agents-md fleets via the shared script. Adds test coverage for the update-in-place path (open PR is edited, not re-opened) and the create path (no edit when no PR is open). --- .github/bump-callers/README.md | 14 +++- .github/bump-callers/bump-callers.sh | 76 ++++++++++++++----- .../bump-callers/tests/test_bump_callers.sh | 26 ++++++- 3 files changed, 93 insertions(+), 23 deletions(-) diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index fb0bd1b..5700d1c 100644 --- a/.github/bump-callers/README.md +++ b/.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-`, 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. diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index 1b1cf4d..ddf892a 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.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:]]/}" @@ -126,21 +130,34 @@ bump_one() { 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") - # Create the branch from the default-branch tip (ignore 422 if it exists). + # Rebuild the stable bump branch from the caller's CURRENT default-branch tip + # every run, so the PR is always a clean single-commit "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). 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). + # 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 + + # Commit the updated file as the single commit on top of the tip. `$(...)` + # 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). BLOB_SHA is the file's blob at the tip we just + # reset to, so the update applies cleanly regardless of any prior branch state. local ENCODED ENCODED=$(printf '%s\n' "$NEW_CONTENT" | base64 | tr -d '\n') gh api --method PUT "repos/${REPO}/contents/${FILE_ENC}" \ @@ -153,22 +170,45 @@ bump_one() { return 1 } + # 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._" + + # 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). + local EXISTING_PR + EXISTING_PR=$(gh pr list --repo "${REPO}" --head "${BRANCH}" --state open --json number --jq '.[0].number' 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). local LABEL_ARGS=() [[ -n "$LABEL" ]] && LABEL_ARGS=(--label "$LABEL") - - # 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. 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" else - echo "::warning::${REPO}: PR may already exist for ${BRANCH}" + echo "::warning::${REPO}: PR create failed for ${BRANCH}" fi return 0 } diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index 98e25b1..bddd42a 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -46,7 +46,14 @@ cat > "${STUB_BIN}/gh" <<'STUB' # value so bump-callers.sh runs end to end offline. sub="$1"; shift || true if [[ "$sub" == "pr" ]]; then - echo "pr-create $*" >> "$STUB_PUT_DIR/pr.log" + action="$1"; shift || true + echo "pr-$action $*" >> "$STUB_PUT_DIR/pr.log" + # `pr list --json number --jq '.[0].number'` returns the open PR number, if + # any. STUB_OPEN_PR toggles whether an open bump PR already exists for the + # branch (set = update-in-place path; unset = create path). + if [[ "$action" == "list" ]]; then + [[ -n "${STUB_OPEN_PR:-}" ]] && echo "$STUB_OPEN_PR" + fi exit 0 fi [[ "$sub" == "api" ]] || exit 0 @@ -68,6 +75,7 @@ done case "$method" in POST) exit 0;; + PATCH) exit 0;; # force-reset of the bump branch ref PUT) n=$(( $(cat "$STUB_PUT_DIR/count" 2>/dev/null || echo 0) + 1 )) echo "$n" > "$STUB_PUT_DIR/count" @@ -132,6 +140,22 @@ check "stale pin comment removed" "! grep -qF '# github-workflows#27 # exactly one trailing newline (#23): last byte is \n (tail -c1 strips to empty), # and the last two bytes are not both \n (tail -c2 keeps a non-newline byte). check "single trailing newline" "[[ -z \"\$(tail -c1 \"$PUT\")\" && -n \"\$(tail -c2 \"$PUT\")\" ]]" +# No open PR for the stable branch → the create path runs, not the edit path. +check "opened a new PR (pr create called)" "grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" +check "did not edit (no open PR existed)" "! grep -q '^pr-edit' \"\$STUB_PUT_DIR/pr.log\"" + +echo "== cursor-review fleet: an open bump PR is UPDATED IN PLACE, not re-opened (BE-3882) ==" +new_case reuse +STUB_CONTENT_FILE="$CR_FIXTURE" run_bump \ + STUB_OPEN_PR=42 \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-alpha","file":".github/workflows/ci-cursor-review.yml","label":""}]' +check "exit 0" "[[ $RC -eq 0 ]]" +check "updated the existing PR in place" "grep -q 'PR #42 updated to $SHORT' <<<\"\$OUT\"" +check "did NOT report a new PR opened" "! grep -q 'PR opened' <<<\"\$OUT\"" +check "called pr edit on the open PR" "grep -q '^pr-edit 42 ' \"\$STUB_PUT_DIR/pr.log\"" +check "did NOT open a second PR" "! grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" +check "branch still refreshed to the new SHA" "grep -qF '$NEW_SHA' \"\${STUB_PUT_DIR}/put.last.txt\"" echo "== agents-md fleet: two callers, two SHA refs, '# v1' preserved ==" new_case amd From 92330d88f95b137d2c1ca6430bf663b0d68eb1dd Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 20 Jul 2026 23:13:09 -0700 Subject: [PATCH 2/6] fix(bump-callers): guard existing-PR lookup + fail loud on create error (BE-3882) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Harden the stable-branch bump PR reuse from the review panel: - Existing-PR lookup: use `.[0].number // empty` (a bare `.[0].number` prints the literal `null` on an empty list, so the first bump for a repo — and every bump after the prior PR merged/closed — ran `gh pr edit null` and failed the caller). Also exclude cross-repository PRs (`isCrossRepository == false`): `--head` matches by branch name across forks, so with the now-predictable branch name an attacker could pre-open a fork PR the bot would stamp instead of bumping the real caller. - Create path: `return 1` on a genuine `gh pr create` failure. The update-in-place path already handles an open PR, so reaching create means none exists and a failure is a real miss — record it in FAILED instead of reporting success. - Serialize each fleet with a `concurrency:` group (cancel-in-progress: false) so overlapping runs can't race the shared stable branch; the newest pending run wins so the latest SHA is committed. - Tests: the gh stub now faithfully models `gh pr list --json --jq` by running the real jq over the JSON gh would return, so the no-open-PR case reproduces production instead of masking it; add a fork-PR decoy regression case. Co-Authored-By: Claude Opus 4.8 --- .github/bump-callers/bump-callers.sh | 20 +++++++++- .../bump-callers/tests/test_bump_callers.sh | 37 +++++++++++++++++-- .github/workflows/bump-agents-md-callers.yml | 11 ++++++ .../workflows/bump-cursor-review-callers.yml | 11 ++++++ 4 files changed, 73 insertions(+), 6 deletions(-) diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index ddf892a..3012fde 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -182,8 +182,18 @@ bump_one() { # 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-`), 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 --jq '.[0].number' 2>/dev/null) + 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 @@ -207,10 +217,16 @@ bump_one() { --body "${PR_BODY}" \ "${LABEL_ARGS[@]}" 2>/dev/null; then echo "${REPO}: PR opened" + return 0 else + # 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=() diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index bddd42a..4564da0 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -48,11 +48,25 @@ sub="$1"; shift || true if [[ "$sub" == "pr" ]]; then action="$1"; shift || true echo "pr-$action $*" >> "$STUB_PUT_DIR/pr.log" - # `pr list --json number --jq '.[0].number'` returns the open PR number, if - # any. STUB_OPEN_PR toggles whether an open bump PR already exists for the - # branch (set = update-in-place path; unset = create path). + # Faithfully model `gh pr list --json --jq `: build the JSON + # array real gh would return for the query, then run the caller's ACTUAL --jq + # over it. Modeling the post-jq output honestly (rather than echoing a bare + # number, or nothing) is what makes the no-open-PR case emit exactly what real + # gh emits — so an empty list can't silently mask a `gh pr edit null` + # regression, and a decoy fork PR is actually exercised. + # STUB_OPEN_PR — number of an open bump PR on the repo's OWN branch. + # STUB_FORK_PR — number of a cross-repository (fork) PR on the same branch + # name; the script must ignore it. if [[ "$action" == "list" ]]; then - [[ -n "${STUB_OPEN_PR:-}" ]] && echo "$STUB_OPEN_PR" + jqexpr=""; a=("$@") + for ((j=0; j<${#a[@]}; j++)); do + [[ "${a[$j]}" == "--jq" ]] && jqexpr="${a[$((j+1))]}" + done + entries=() + [[ -n "${STUB_FORK_PR:-}" ]] && entries+=("{\"number\":${STUB_FORK_PR},\"isCrossRepository\":true}") + [[ -n "${STUB_OPEN_PR:-}" ]] && entries+=("{\"number\":${STUB_OPEN_PR},\"isCrossRepository\":false}") + json="[$(IFS=,; echo "${entries[*]}")]" + if [[ -n "$jqexpr" ]]; then jq -r "$jqexpr" <<<"$json"; fi fi exit 0 fi @@ -157,6 +171,21 @@ check "called pr edit on the open PR" "grep -q '^pr-edit 42 ' \"\$STUB_P check "did NOT open a second PR" "! grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" check "branch still refreshed to the new SHA" "grep -qF '$NEW_SHA' \"\${STUB_PUT_DIR}/put.last.txt\"" +echo "== cursor-review fleet: a decoy fork PR on the stable branch is IGNORED ==" +# An attacker pre-opens a fork PR whose head branch NAME collides with the +# predictable stable branch (ci/bump-). `gh pr list --head` matches by name +# across forks, so without the isCrossRepository filter the bot would edit the +# attacker's PR and skip the real bump. The real caller has NO open bump PR here. +new_case fork +STUB_CONTENT_FILE="$CR_FIXTURE" run_bump \ + STUB_FORK_PR=1337 \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-alpha","file":".github/workflows/ci-cursor-review.yml","label":""}]' +check "exit 0" "[[ $RC -eq 0 ]]" +check "ignored the fork PR, opened the real one" "grep -q 'PR opened' <<<\"\$OUT\"" +check "did NOT edit the attacker's fork PR" "! grep -q '^pr-edit 1337' \"\$STUB_PUT_DIR/pr.log\"" +check "opened a fresh PR via create" "grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" + echo "== agents-md fleet: two callers, two SHA refs, '# v1' preserved ==" new_case amd AMD_FIXTURE="${WORK}/amd_caller.yml" diff --git a/.github/workflows/bump-agents-md-callers.yml b/.github/workflows/bump-agents-md-callers.yml index 772fa61..eadf825 100644 --- a/.github/workflows/bump-agents-md-callers.yml +++ b/.github/workflows/bump-agents-md-callers.yml @@ -39,6 +39,17 @@ on: - .github/workflows/agents-md-integrity.yml - .github/agents-md-integrity/** +# Serialize runs of this fleet. The bumper now pushes to a STABLE branch +# (ci/bump-agents-md-integrity) shared across runs, so two overlapping runs (a +# rapid second main push, or a push racing a manual re-run) would force-reset +# that branch and race the PR update — an older run finishing last could leave +# the committed diff pinned to a stale SHA. cancel-in-progress: false lets the +# running bump finish; GitHub keeps only the newest pending run, so the latest +# SHA always wins (BE-3882). +concurrency: + group: bump-agents-md-callers + cancel-in-progress: false + jobs: bump: runs-on: ubuntu-latest diff --git a/.github/workflows/bump-cursor-review-callers.yml b/.github/workflows/bump-cursor-review-callers.yml index 489946f..0524c6a 100644 --- a/.github/workflows/bump-cursor-review-callers.yml +++ b/.github/workflows/bump-cursor-review-callers.yml @@ -33,6 +33,17 @@ on: paths: - .github/workflows/cursor-review.yml +# Serialize runs of this fleet. The bumper now pushes to a STABLE branch +# (ci/bump-cursor-review) shared across runs, so two overlapping runs (a rapid +# second main push, or a push racing a manual re-run) would force-reset that +# branch and race the PR update — an older run finishing last could leave the +# committed diff pinned to a stale SHA. cancel-in-progress: false lets the +# running bump finish; GitHub keeps only the newest pending run, so the latest +# SHA always wins (BE-3882). +concurrency: + group: bump-cursor-review-callers + cancel-in-progress: false + jobs: bump: runs-on: ubuntu-latest From 4455be201228c4fb017bbfa4e55f8816b6dca5a5 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 20 Jul 2026 23:59:46 -0700 Subject: [PATCH 3/6] fix(bump-callers): commit all of a repo's files onto one bump branch (BE-3896) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Group CALLERS entries by repo before the bump loop and reset the stable branch ONCE per repo, then commit each of that repo's files onto it with successive PUTs (each carrying that file's own blob SHA). The old per-entry loop reset the branch before every file, so a second same-repo entry's reset discarded the first entry's commit and the PR shipped only the last file while every entry reported success — a silent partial bump. Only affects repos that appear more than once in a caller list; single-file repos are unchanged. The test stub now models the one bump branch's committed file set (a ref reset truncates it, a PUT appends), and a new same-repo two-file case asserts BOTH files land on the branch — it fails against the pre-fix script. --- .github/bump-callers/bump-callers.sh | 158 ++++++++++++------ .../bump-callers/tests/test_bump_callers.sh | 37 +++- 2 files changed, 141 insertions(+), 54 deletions(-) diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index 3012fde..a2d9d17 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -89,15 +89,21 @@ 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') || { @@ -105,34 +111,57 @@ bump_one() { 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 - } + # 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}" + [[ -n "$LABEL" ]] && LABELS+=("$LABEL") + + # Fetch current file; a missing file is an expected skip (this file only). + CURRENT=$(gh api "repos/${REPO}/contents/${FILE_ENC}?ref=${DEFAULT_BRANCH}" 2>/dev/null) || { + echo "::warning::${REPO}: ${FILE} not found — skipping" + continue + } + + BLOB_SHA=$(jq -r '.sha' <<<"$CURRENT") + OLD_CONTENT=$(jq -r '.content' <<<"$CURRENT" | base64 -d) + + # Already pinned to this SHA → nothing to do for this file. + if grep -qF "$NEW_SHA" <<<"$OLD_CONTENT"; then + echo "${REPO}: ${FILE} already at ${SHORT} — skipping" + continue + fi + + # Replace every 40-char hex SHA (the caller's pin — cursor-review has one, + # agents-md-integrity has two: the `uses: ...@` 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. + NEW_CONTENT=$(sed -E "s/[0-9a-f]{40}/${NEW_SHA}/g; s|# github-workflows#[0-9]+|# github-workflows main (${SHORT})|g" <<<"$OLD_CONTENT") - local BLOB_SHA OLD_CONTENT - BLOB_SHA=$(jq -r '.sha' <<<"$CURRENT") - OLD_CONTENT=$(jq -r '.content' <<<"$CURRENT" | base64 -d) + # `$(...)` 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 - # Already pinned to this SHA → nothing to do (success). - if grep -qF "$NEW_SHA" <<<"$OLD_CONTENT"; then - echo "${REPO}: already at ${SHORT} — skipping" + # 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: ...@` 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 caller's CURRENT default-branch tip - # every run, so the PR is always a clean single-commit "bump to @SHORT" diff — - # it never accumulates stale commits across bumps and never drifts if the + # 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). local MAIN_SHA MAIN_SHA=$(gh api "repos/${REPO}/git/refs/heads/${DEFAULT_BRANCH}" --jq '.object.sha') || { @@ -152,23 +181,22 @@ bump_one() { } fi - # Commit the updated file as the single commit on top of the tip. `$(...)` - # 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). BLOB_SHA is the file's blob at the tip we just - # reset to, so the update applies cleanly regardless of any prior branch state. - 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 # 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 @@ -207,8 +235,16 @@ bump_one() { # 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). - local LABEL_ARGS=() - [[ -n "$LABEL" ]] && LABEL_ARGS=(--label "$LABEL") + # 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). + local -a LABEL_ARGS=() + local L SEEN_LABELS="" + for L in ${LABELS[@]+"${LABELS[@]}"}; do + case "$SEEN_LABELS" in + *"|${L}|"*) ;; + *) LABEL_ARGS+=(--label "$L"); SEEN_LABELS="${SEEN_LABELS}|${L}|" ;; + esac + done if gh pr create \ --repo "${REPO}" \ --head "${BRANCH}" \ @@ -229,9 +265,27 @@ bump_one() { fi } -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 diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index 4564da0..2ff7e08 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -87,14 +87,26 @@ while (( i < ${#args[@]} )); do esac done +# Model the ONE bump branch's committed file set in $STUB_PUT_DIR/branch_files: +# a ref create/reset (POST/PATCH on git/refs) rebuilds the branch at the tip and +# so DROPS every prior bump commit (truncate), while a contents PUT commits a +# file onto it (append its path). This is what lets a test assert the branch's +# final contents — and catch the BE-3896 regression where resetting the branch +# per file left only the last file on it. (One branch is modeled; a same-repo +# test drives a single repo, so branch_files reflects exactly that repo's PR.) case "$method" in - POST) exit 0;; - PATCH) exit 0;; # force-reset of the bump branch ref + POST) # branch create at the tip — starts a fresh (empty) bump branch + [[ "$path" == *"/git/refs"* ]] && : > "$STUB_PUT_DIR/branch_files" + exit 0;; + PATCH) # force-reset of the bump branch ref — discards prior bump commits + [[ "$path" == *"/git/refs"* ]] && : > "$STUB_PUT_DIR/branch_files" + exit 0;; PUT) n=$(( $(cat "$STUB_PUT_DIR/count" 2>/dev/null || echo 0) + 1 )) echo "$n" > "$STUB_PUT_DIR/count" printf '%s' "$content" | { base64 -d 2>/dev/null || base64 -D; } > "$STUB_PUT_DIR/put.$n.txt" cp "$STUB_PUT_DIR/put.$n.txt" "$STUB_PUT_DIR/put.last.txt" + echo "${path##*/contents/}" >> "$STUB_PUT_DIR/branch_files" # file now on the branch exit 0;; esac @@ -210,6 +222,27 @@ check "both SHA refs rewritten (2 occurrences)" "[[ \$(grep -cF '$NEW_SHA' \"$PU check "old agents-md pin removed" "! grep -qF '2222222222222222222222222222222222222222' \"$PUT\"" check "'# v1' comment left intact" "grep -qF '# v1' \"$PUT\"" +echo "== monorepo: TWO files in the SAME repo BOTH land on the one branch (BE-3896) ==" +# A repo listed more than once (a monorepo pinning the reusable workflow from +# two workflow files) must land BOTH files on its single stable branch. The old +# per-entry loop reset the branch before each file, so the second file's reset +# discarded the first file's commit and the PR shipped only the last file — a +# silent partial bump. The stub now models the branch's file set (a reset +# truncates it, a PUT appends), so this asserts the branch keeps BOTH files. +new_case mono +STUB_CONTENT_FILE="$CR_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-mono","file":".github/workflows/ci-a.yml","label":""},{"repo":"Comfy-Org/secret-mono","file":".github/workflows/ci-b.yml","label":""}]' +BF="${STUB_PUT_DIR}/branch_files" +check "exit 0" "[[ $RC -eq 0 ]]" +check "committed both files (2 PUTs)" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 2 ]]" +check "branch holds exactly two files" "[[ \$(wc -l < \"$BF\") -eq 2 ]]" +check "first file present on the branch" "grep -q 'ci-a.yml' \"$BF\"" # the file the old code dropped +check "second file present on the branch" "grep -q 'ci-b.yml' \"$BF\"" +check "opened exactly ONE PR for the repo" "[[ \$(grep -c '^pr-create' \"\$STUB_PUT_DIR/pr.log\") -eq 1 ]]" +check "masked the repo name once" "grep -q '::add-mask::Comfy-Org/secret-mono' <<<\"\$OUT\"" +check "reported fleet complete" "grep -q 'cursor-review bump complete' <<<\"\$OUT\"" + echo "== agents-md fleet: empty list is a clean no-op (ALLOW_EMPTY) ==" new_case empty STUB_CONTENT_FILE="$CR_FIXTURE" run_bump \ From 214a1a7bd843fd97848495cd31c4c8c1fe95d722 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 21 Jul 2026 00:31:20 -0700 Subject: [PATCH 4/6] fix(bump-callers): harden per-file staging against partial/erroneous bumps (BE-3896) Address cursor-review panel findings on the multi-file-per-repo bumper: - Distinguish a genuine 404 (per-file skip) from any other fetch error (auth/rate-limit/5xx/network); a transient error now fails the whole repo instead of silently shipping a PR that omits the un-fetched file. - Resolve the default-branch tip (MAIN_SHA) up front and pin every Pass-1 blob fetch to that immutable SHA, closing a TOCTOU race with a moving default branch. - De-duplicate staged files by path so a repo listed twice for the same file commits it once (a second PUT with a now-stale blob sha would 409 the repo). - Skip an already-pinned file by comparing rewritten-vs-original content instead of grepping for NEW_SHA anywhere, which also repairs a half-bumped file. - Anchor the 40-hex SHA rewrite to the github-workflows / workflows_ref pin contexts so a full-SHA pin of another action (actions/checkout@) is not clobbered. - Collect a caller entry's label only once its file is confirmed staged, so a skipped entry's label never lands on the real bump PR. - Exact-match label de-dup (GitHub label names may contain the `|` sentinel). Adds functional tests for each: 404-skip vs transient-fail, same-file de-dup, non-github-workflows pin preservation, half-bump repair, and already-pinned skip. Co-Authored-By: Claude Opus 4.8 --- .github/bump-callers/bump-callers.sh | 110 ++++++++++++----- .../bump-callers/tests/test_bump_callers.sh | 116 ++++++++++++++++++ 2 files changed, 198 insertions(+), 28 deletions(-) diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index a2d9d17..bd86b06 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -111,6 +111,18 @@ bump_repo() { return 1 } + # Resolve the default-branch tip ONCE, up front, and pin every Pass-1 blob + # fetch to this immutable SHA (not the mutable `?ref=`). 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 + } + # 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 @@ -121,29 +133,71 @@ bump_repo() { FILE=$(cut -d'|' -f2 <<<"$ENTRY") LABEL=$(cut -d'|' -f3 <<<"$ENTRY") FILE_ENC="${FILE//\//%2F}" - [[ -n "$LABEL" ]] && LABELS+=("$LABEL") - # Fetch current file; a missing file is an expected skip (this file only). - CURRENT=$(gh api "repos/${REPO}/contents/${FILE_ENC}?ref=${DEFAULT_BRANCH}" 2>/dev/null) || { - echo "::warning::${REPO}: ${FILE} not found — skipping" + # 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" BLOB_SHA=$(jq -r '.sha' <<<"$CURRENT") OLD_CONTENT=$(jq -r '.content' <<<"$CURRENT" | base64 -d) - # Already pinned to this SHA → nothing to do for this file. - if grep -qF "$NEW_SHA" <<<"$OLD_CONTENT"; then + # 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…@` + # line and agents-md-integrity's bare `workflows_ref: ` line — so a + # full-SHA pin of ANOTHER action in the same file (`actions/checkout@`, + # 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 - # Replace every 40-char hex SHA (the caller's pin — cursor-review has one, - # agents-md-integrity has two: the `uses: ...@` 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. - NEW_CONTENT=$(sed -E "s/[0-9a-f]{40}/${NEW_SHA}/g; s|# github-workflows#[0-9]+|# github-workflows main (${SHORT})|g" <<<"$OLD_CONTENT") + # 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 @@ -159,15 +213,10 @@ bump_repo() { return 0 fi - # Rebuild the stable bump branch from the caller's CURRENT default-branch tip - # 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). - 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 - } + # 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. @@ -237,13 +286,18 @@ bump_repo() { # 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). - local -a LABEL_ARGS=() - local L SEEN_LABELS="" + # 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 - case "$SEEN_LABELS" in - *"|${L}|"*) ;; - *) LABEL_ARGS+=(--label "$L"); SEEN_LABELS="${SEEN_LABELS}|${L}|" ;; - esac + 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}" \ diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index 2ff7e08..0bf37fe 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -112,6 +112,18 @@ esac # GET dispatch by resource path. if [[ "$path" == *"/contents/"* ]]; then + # Simulate content-fetch failures so the script's 404-vs-transient handling is + # exercised. STUB_404_FILE: a contents GET whose (decoded-ish) path contains + # this substring returns a genuine 404 (an expected per-file skip). + # STUB_FETCH_FAIL: EVERY contents GET returns a transient non-404 error (the + # script must fail the repo, never ship a partial bump). + base="${path##*/contents/}"; base="${base%%\?*}" + if [[ -n "${STUB_404_FILE:-}" && "$base" == *"${STUB_404_FILE}"* ]]; then + echo "gh: Not Found (HTTP 404)" >&2; exit 1 + fi + if [[ -n "${STUB_FETCH_FAIL:-}" ]]; then + echo "gh: Internal Server Error (HTTP 500)" >&2; exit 1 + fi b64=$(base64 < "$STUB_CONTENT_FILE" | tr -d '\n') printf '{"sha":"blobsha123","content":"%s"}' "$b64" elif [[ "$path" == *"/git/refs/heads/"* ]]; then @@ -268,6 +280,110 @@ STUB_CONTENT_FILE="$CR_FIXTURE" run_bump \ check "exit 1 on malformed" "[[ $RC -eq 1 ]]" check "error explains shape" "grep -q 'not a non-empty JSON array' <<<\"\$OUT\"" +echo "== monorepo: a genuinely-missing (404) file is skipped, the present one still bumps ==" +# One file 404s (expected per-file skip), the other bumps. The repo must still +# succeed and open its PR with the file that WAS present — a 404 is not a repo +# failure. +new_case miss404 +STUB_CONTENT_FILE="$CR_FIXTURE" STUB_404_FILE="ci-b.yml" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-mono","file":".github/workflows/ci-a.yml","label":""},{"repo":"Comfy-Org/secret-mono","file":".github/workflows/ci-b.yml","label":""}]' +check "exit 0 (404 is a skip, not a failure)" "[[ $RC -eq 0 ]]" +check "reported the 404 file as not found" "grep -q 'ci-b.yml not found' <<<\"\$OUT\"" +check "committed only the present file" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "still opened the PR" "grep -q 'PR opened' <<<\"\$OUT\"" + +echo "== transient fetch error fails the repo — NEVER a silent partial bump ==" +# A non-404 fetch error (auth/rate-limit/5xx/network) must fail the whole repo: +# skipping it and opening a PR with only the files that DID fetch is the exact +# partial-bump this refactor exists to prevent (BE-3896). +new_case transient +STUB_CONTENT_FILE="$CR_FIXTURE" STUB_FETCH_FAIL=1 run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-alpha","file":".github/workflows/ci-cursor-review.yml","label":""}]' +check "exit 1 on transient fetch error" "[[ $RC -eq 1 ]]" +check "warned about avoiding a partial bump" "grep -q 'failing repo to avoid a partial bump' <<<\"\$OUT\"" +check "committed NOTHING" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]" +check "opened NO PR" "[[ ! -f \"\$STUB_PUT_DIR/pr.log\" ]] || ! grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" +check "job failed for the repo" "grep -q 'bump failed for 1 repo' <<<\"\$OUT\"" + +echo "== same repo+file listed twice is de-duped to ONE commit (no stale-sha 409) ==" +# A repo listed twice for the same path must stage that file once; a second PUT +# with the now-stale pre-bump blob sha would 409 and fail the repo. +new_case dedup +STUB_CONTENT_FILE="$CR_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-dup","file":".github/workflows/ci.yml","label":"ci"},{"repo":"Comfy-Org/secret-dup","file":".github/workflows/ci.yml","label":"ci"}]' +check "exit 0" "[[ $RC -eq 0 ]]" +check "committed the file exactly once" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "opened exactly one PR" "[[ \$(grep -c '^pr-create' \"\$STUB_PUT_DIR/pr.log\") -eq 1 ]]" + +echo "== a full-SHA pin of ANOTHER action is NOT clobbered to github-workflows' SHA ==" +# The caller also pins actions/checkout by full SHA (the org's mandated +# practice). The 40-hex rewrite must touch only the github-workflows pin, not +# every hex token in the file. +new_case anchor +ANCHOR_FIXTURE="${WORK}/anchor_caller.yml" +printf '%s\n' \ + 'name: CI cursor-review' \ + 'jobs:' \ + ' review:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@1111111111111111111111111111111111111111 # github-workflows#27' \ + ' build:' \ + ' runs-on: ubuntu-latest' \ + ' steps:' \ + ' - uses: actions/checkout@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb # v4' \ + > "$ANCHOR_FIXTURE" +STUB_CONTENT_FILE="$ANCHOR_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-anchor","file":".github/workflows/ci.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 0" "[[ $RC -eq 0 ]]" +check "github-workflows pin bumped" "grep -qF '$NEW_SHA' \"$PUT\"" +check "old github-workflows pin removed" "! grep -qF '1111111111111111111111111111111111111111' \"$PUT\"" +check "actions/checkout SHA left intact" "grep -qF 'actions/checkout@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' \"$PUT\"" + +echo "== half-bumped file (one ref at NEW_SHA, one stale) is REPAIRED, not skipped ==" +# The already-pinned check compares rewritten-vs-original content, so a file +# where only one of two refs reached NEW_SHA still differs and is re-staged — +# the old 'NEW_SHA appears anywhere' grep would have skipped it, stranding the +# stale ref. +new_case halfbump +HALF_FIXTURE="${WORK}/half_caller.yml" +printf '%s\n' \ + 'name: AGENTS.md Integrity' \ + 'jobs:' \ + ' agents-md:' \ + " uses: Comfy-Org/github-workflows/.github/workflows/agents-md-integrity.yml@${NEW_SHA} # v1" \ + ' with:' \ + ' workflows_ref: 2222222222222222222222222222222222222222' \ + > "$HALF_FIXTURE" +STUB_CONTENT_FILE="$HALF_FIXTURE" run_bump \ + VAR_NAME=AGENTS_MD_CALLERS TAG=agents-md-integrity WORKFLOW_FILE=agents-md-integrity.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-half","file":".github/workflows/agents-md-integrity.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 0" "[[ $RC -eq 0 ]]" +check "re-staged the half-bumped file" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "both refs now at NEW_SHA" "[[ \$(grep -cF '$NEW_SHA' \"$PUT\") -eq 2 ]]" +check "stale second ref repaired" "! grep -qF '2222222222222222222222222222222222222222' \"$PUT\"" + +echo "== a fully already-pinned file is a clean skip (no commit, no PR) ==" +new_case pinned +PINNED_FIXTURE="${WORK}/pinned_caller.yml" +printf '%s\n' \ + 'name: CI cursor-review' \ + 'jobs:' \ + ' review:' \ + " uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@${NEW_SHA} # github-workflows main (${SHORT})" \ + > "$PINNED_FIXTURE" +STUB_CONTENT_FILE="$PINNED_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-pinned","file":".github/workflows/ci.yml","label":""}]' +check "exit 0" "[[ $RC -eq 0 ]]" +check "reported already at SHORT" "grep -q 'already at $SHORT' <<<\"\$OUT\"" +check "committed nothing" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]" +check "opened no PR" "[[ ! -f \"\$STUB_PUT_DIR/pr.log\" ]] || ! grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" + echo echo "== $PASS passed, $FAIL failed ==" [[ $FAIL -eq 0 ]] From cc45841c910db0edaaebbc22af77d744deb83edf Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 21 Jul 2026 00:46:18 -0700 Subject: [PATCH 5/6] fix(bump-callers): commit each repo's files atomically via the Git Data API (BE-3902) --- .github/bump-callers/bump-callers.sh | 124 +++++++++++------- .../bump-callers/tests/test_bump_callers.sh | 70 ++++++---- 2 files changed, 120 insertions(+), 74 deletions(-) diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index bd86b06..0b4682c 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -97,11 +97,14 @@ fi # 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). +# Grouping by repo is load-bearing: all of a repo's files are built into ONE +# atomic commit (Git Data API) off the default-branch tip and the bump branch is +# moved to it in a single step, so the PR either bumps every file or none — never +# a partial-yet-mergeable bump. The earlier per-file Contents PUT loop committed +# files one at a time with no rollback, so a mid-sequence failure left the +# earlier files already committed on the branch (BE-3902); and before that a +# per-ENTRY branch reset discarded earlier commits so only the last file shipped +# (BE-3896). One commit for the whole repo closes both. bump_repo() { local REPO="$1"; shift # remaining args: this repo's "repo|file|label" entries @@ -111,12 +114,12 @@ bump_repo() { return 1 } - # Resolve the default-branch tip ONCE, up front, and pin every Pass-1 blob - # fetch to this immutable SHA (not the mutable `?ref=`). 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. + # Resolve the default-branch tip ONCE, up front. Every Pass-1 fetch is pinned + # to this immutable SHA (not the mutable `?ref=`), and Pass 2 uses the + # same SHA as both the tree's `base_tree` and the commit's parent — so the + # whole bump is built against one consistent snapshot of the caller repo, + # never a moving tip. 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" @@ -127,8 +130,8 @@ bump_repo() { # 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 + local -a PEND_FILE=() PEND_CONTENT=() LABELS=() + local ENTRY FILE LABEL FILE_ENC CURRENT OLD_CONTENT NEW_CONTENT for ENTRY in "$@"; do FILE=$(cut -d'|' -f2 <<<"$ENTRY") LABEL=$(cut -d'|' -f3 <<<"$ENTRY") @@ -141,8 +144,8 @@ bump_repo() { # 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; } + for P in ${PEND_FILE[@]+"${PEND_FILE[@]}"}; do + [[ "$P" == "$FILE" ]] && { SEEN_FILE=1; break; } done if (( SEEN_FILE )); then [[ -n "$LABEL" ]] && LABELS+=("$LABEL") @@ -170,7 +173,6 @@ bump_repo() { fi rm -f "$ERRFILE" - 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 @@ -203,50 +205,80 @@ bump_repo() { # 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_FILE+=("$FILE") 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 + if (( ${#PEND_FILE[@]} == 0 )); then return 0 fi - # 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. + # Pass 2 — land EVERY staged file in ONE atomic commit via the Git Data API, + # then point the stable bump branch at it. The previous per-file Contents PUT + # loop committed files one at a time with no rollback, so a mid-sequence + # failure left the earlier files already committed on the branch — and if an + # open bump PR pointed at it, a partial-yet-mergeable bump (BE-3902). Building + # one commit off the tip and moving the ref in a single step makes the bump + # all-or-nothing: either every file lands or the branch is untouched. + + # 1. Create a blob for each staged file's new content. + local -a TREE_ENTRIES=() + local i BLOB_NEW + for (( i = 0; i < ${#PEND_FILE[@]}; i++ )); do + BLOB_NEW=$(gh api --method POST "repos/${REPO}/git/blobs" \ + --field content="${PEND_CONTENT[$i]}" \ + --field encoding=base64 \ + --jq '.sha') || { + echo "::warning::${REPO}: blob create for ${PEND_FILE[$i]} failed — skipping" + return 1 + } + # Callers are regular files (mode 100644 — the `.github/workflows/*.yml` + # they pin from is never executable). `base_tree` below carries every OTHER + # path in the repo unchanged, so the tree lists only the bumped files. + TREE_ENTRIES+=("$(jq -n --arg path "${PEND_FILE[$i]}" --arg sha "$BLOB_NEW" \ + '{path: $path, mode: "100644", type: "blob", sha: $sha}')") + done + + # 2. Build ONE tree off the default-branch tip carrying all staged blobs. + local TREE_SHA + TREE_SHA=$(printf '%s\n' "${TREE_ENTRIES[@]}" \ + | jq -s --arg base "$MAIN_SHA" '{base_tree: $base, tree: .}' \ + | gh api --method POST "repos/${REPO}/git/trees" --input - --jq '.sha') || { + echo "::warning::${REPO}: tree create failed — skipping" + return 1 + } + + # 3. Create ONE commit with that tree, parented on the tip so the PR is always + # a clean "bump to @SHORT" diff against the default branch (never stale + # commits accumulated across bumps, never drift if the caller's default + # branch moved since the last bump — BE-3882). + local COMMIT_SHA + COMMIT_SHA=$(jq -n \ + --arg msg "ci: bump ${TAG} to github-workflows@${SHORT}" \ + --arg tree "$TREE_SHA" --arg parent "$MAIN_SHA" \ + '{message: $msg, tree: $tree, parents: [$parent]}' \ + | gh api --method POST "repos/${REPO}/git/commits" --input - --jq '.sha') || { + echo "::warning::${REPO}: commit create failed — skipping" + return 1 + } + + # 4. Point the stable bump branch at the finished commit. Create it there if it + # is new; otherwise force it there so the diff is rebuilt from scratch (an + # open bump PR's branch, or residue from a prior run) rather than committed + # on top of the old bump (BE-3882). The ref moves in ONE step from its old + # state to the complete new commit — it never transiently sits at an empty + # or partial tree. if ! gh api --method POST "repos/${REPO}/git/refs" \ --field ref="refs/heads/${BRANCH}" \ - --field sha="${MAIN_SHA}" >/dev/null 2>&1; then + --field sha="${COMMIT_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" + --field sha="${COMMIT_SHA}" --field force=true >/dev/null 2>&1 || { + echo "::warning::${REPO}: cannot point ${BRANCH} at the bump commit — skipping" return 1 } fi - # 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 - # 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 diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index 0bf37fe..3bcffd2 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -14,7 +14,8 @@ # fleet still hard-fails, and a malformed variable hard-fails. # # No network: `gh` is a PATH stub that serves a fixture file and captures the -# contents PUT so we can inspect exactly what would be committed. +# Git Data API calls (the blob content + the tree's file list) so we can inspect +# exactly what would be committed. set -uo pipefail @@ -42,8 +43,9 @@ mkdir -p "$STUB_BIN" cat > "${STUB_BIN}/gh" <<'STUB' #!/usr/bin/env bash # Minimal `gh` stub. Serves $STUB_CONTENT_FILE for a contents GET and captures -# the decoded contents PUT to $STUB_PUT_DIR; everything else returns a canned -# value so bump-callers.sh runs end to end offline. +# the atomic Git Data API commit (blobs/tree/commit/ref) to $STUB_PUT_DIR; +# everything else returns a canned value so bump-callers.sh runs end to end +# offline. sub="$1"; shift || true if [[ "$sub" == "pr" ]]; then action="$1"; shift || true @@ -87,26 +89,35 @@ while (( i < ${#args[@]} )); do esac done -# Model the ONE bump branch's committed file set in $STUB_PUT_DIR/branch_files: -# a ref create/reset (POST/PATCH on git/refs) rebuilds the branch at the tip and -# so DROPS every prior bump commit (truncate), while a contents PUT commits a -# file onto it (append its path). This is what lets a test assert the branch's -# final contents — and catch the BE-3896 regression where resetting the branch -# per file left only the last file on it. (One branch is modeled; a same-repo -# test drives a single repo, so branch_files reflects exactly that repo's PR.) -case "$method" in - POST) # branch create at the tip — starts a fresh (empty) bump branch - [[ "$path" == *"/git/refs"* ]] && : > "$STUB_PUT_DIR/branch_files" - exit 0;; - PATCH) # force-reset of the bump branch ref — discards prior bump commits - [[ "$path" == *"/git/refs"* ]] && : > "$STUB_PUT_DIR/branch_files" - exit 0;; - PUT) +# Model the ONE atomic bump commit in $STUB_PUT_DIR. The script builds a blob +# per staged file (POST git/blobs), one tree carrying all of them off the tip +# (POST git/trees, body on stdin), one commit (POST git/commits), then points +# the bump branch at that commit (POST/PATCH git/refs). We record each blob's +# decoded content (put.$n.txt / put.last.txt; count = number of blobs = files +# committed) and the tree's path list as $STUB_PUT_DIR/branch_files — the +# atomic branch's final file set. Because the whole commit is built BEFORE the +# ref moves, an earlier failure (e.g. a Pass-1 fetch error) leaves NO blobs, NO +# tree, and the ref untouched — the all-or-nothing property this asserts +# (BE-3902) — while the tree still lists BOTH files of a monorepo caller on the +# one branch (BE-3896). (One branch/commit is modeled; a same-repo test drives a +# single repo, so branch_files reflects exactly that repo's PR.) +case "$method:$path" in + POST:*/git/blobs*) # blob create — capture the new file content, count it n=$(( $(cat "$STUB_PUT_DIR/count" 2>/dev/null || echo 0) + 1 )) echo "$n" > "$STUB_PUT_DIR/count" printf '%s' "$content" | { base64 -d 2>/dev/null || base64 -D; } > "$STUB_PUT_DIR/put.$n.txt" cp "$STUB_PUT_DIR/put.$n.txt" "$STUB_PUT_DIR/put.last.txt" - echo "${path##*/contents/}" >> "$STUB_PUT_DIR/branch_files" # file now on the branch + echo "blobsha${n}" + exit 0;; + POST:*/git/trees*) # tree create — the stdin body lists every bumped path + jq -r '.tree[].path' > "$STUB_PUT_DIR/branch_files" + echo "treesha1" + exit 0;; + POST:*/git/commits*) # commit create — drain the body, return a commit sha + cat >/dev/null + echo "commitsha1" + exit 0;; + POST:*/git/refs*|PATCH:*/git/refs*) # point the bump branch at the commit exit 0;; esac @@ -236,18 +247,20 @@ check "'# v1' comment left intact" "grep -qF '# v1' \"$PUT\"" echo "== monorepo: TWO files in the SAME repo BOTH land on the one branch (BE-3896) ==" # A repo listed more than once (a monorepo pinning the reusable workflow from -# two workflow files) must land BOTH files on its single stable branch. The old -# per-entry loop reset the branch before each file, so the second file's reset -# discarded the first file's commit and the PR shipped only the last file — a -# silent partial bump. The stub now models the branch's file set (a reset -# truncates it, a PUT appends), so this asserts the branch keeps BOTH files. +# two workflow files) must land BOTH files on its single stable branch. Both are +# now built into ONE atomic commit (one tree carrying both blobs), so the branch +# holds them together or not at all. The stub records the tree's path list as +# the branch's file set, so this asserts the branch keeps BOTH files — the old +# per-entry loop reset the branch before each file and shipped only the last one +# (BE-3896), and the per-file PUT loop that replaced it could still leave a +# partial commit on failure (BE-3902). new_case mono STUB_CONTENT_FILE="$CR_FIXTURE" run_bump \ VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ CALLERS_JSON='[{"repo":"Comfy-Org/secret-mono","file":".github/workflows/ci-a.yml","label":""},{"repo":"Comfy-Org/secret-mono","file":".github/workflows/ci-b.yml","label":""}]' BF="${STUB_PUT_DIR}/branch_files" check "exit 0" "[[ $RC -eq 0 ]]" -check "committed both files (2 PUTs)" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 2 ]]" +check "committed both files (2 blobs)" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 2 ]]" check "branch holds exactly two files" "[[ \$(wc -l < \"$BF\") -eq 2 ]]" check "first file present on the branch" "grep -q 'ci-a.yml' \"$BF\"" # the file the old code dropped check "second file present on the branch" "grep -q 'ci-b.yml' \"$BF\"" @@ -307,9 +320,10 @@ check "committed NOTHING" "[[ ! -f \"\$STUB_PUT_DIR/count\ check "opened NO PR" "[[ ! -f \"\$STUB_PUT_DIR/pr.log\" ]] || ! grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" check "job failed for the repo" "grep -q 'bump failed for 1 repo' <<<\"\$OUT\"" -echo "== same repo+file listed twice is de-duped to ONE commit (no stale-sha 409) ==" -# A repo listed twice for the same path must stage that file once; a second PUT -# with the now-stale pre-bump blob sha would 409 and fail the repo. +echo "== same repo+file listed twice is de-duped to ONE blob/tree entry ==" +# A repo listed twice for the same path must stage that file once; a duplicate +# tree entry for the same path is ambiguous (the atomic commit must carry each +# path exactly once), so the dedup keeps the commit well-formed. new_case dedup STUB_CONTENT_FILE="$CR_FIXTURE" run_bump \ VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ From 88f3294ddcc2503b22f530c28032d28de9f86a0c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 21 Jul 2026 01:08:10 -0700 Subject: [PATCH 6/6] fix(bump-callers): pass a tree SHA (not commit SHA) as base_tree (BE-3902) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Git Data Create-a-tree call was passing $MAIN_SHA — the tip *commit* SHA — as base_tree, but that API requires a *tree* SHA. A commit SHA either 422s the call (every bump fails) or yields a tree carrying ONLY the bumped files, so merging the PR would delete every other path in the caller repo. Resolve the tip commit's tree via GET git/commits and pass that as base_tree; $MAIN_SHA stays correct as the new commit's parent. Also send the blob create body on stdin (--input -) instead of --field on argv, matching the adjacent tree/commit calls and keeping content off cmdline/ARG_MAX. Test stub now models GET git/commits and asserts base_tree is the resolved tree SHA, not the commit SHA, so the offline suite catches this class of bug. Co-Authored-By: Claude Opus 4.8 --- .github/bump-callers/bump-callers.sh | 30 +++++++++++++------ .../bump-callers/tests/test_bump_callers.sh | 22 +++++++++++++- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index 0b4682c..565c2cc 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -116,10 +116,10 @@ bump_repo() { # Resolve the default-branch tip ONCE, up front. Every Pass-1 fetch is pinned # to this immutable SHA (not the mutable `?ref=`), and Pass 2 uses the - # same SHA as both the tree's `base_tree` and the commit's parent — so the - # whole bump is built against one consistent snapshot of the caller repo, - # never a moving tip. Reading the tip is a GET (no branch churn), so it is safe - # before the all-skip early return. + # same commit as the new commit's parent AND (via its resolved tree) as the + # tree's `base_tree` — so the whole bump is built against one consistent + # snapshot of the caller repo, never a moving tip. 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" @@ -226,10 +226,12 @@ bump_repo() { local -a TREE_ENTRIES=() local i BLOB_NEW for (( i = 0; i < ${#PEND_FILE[@]}; i++ )); do - BLOB_NEW=$(gh api --method POST "repos/${REPO}/git/blobs" \ - --field content="${PEND_CONTENT[$i]}" \ - --field encoding=base64 \ - --jq '.sha') || { + # Pass the base64 body on stdin (like the tree/commit calls below), not via + # --field on argv — this keeps the content off /proc//cmdline and out of + # the ARG_MAX bound, and matches the surrounding Git Data API calls. + BLOB_NEW=$(jq -n --arg content "${PEND_CONTENT[$i]}" \ + '{content: $content, encoding: "base64"}' \ + | gh api --method POST "repos/${REPO}/git/blobs" --input - --jq '.sha') || { echo "::warning::${REPO}: blob create for ${PEND_FILE[$i]} failed — skipping" return 1 } @@ -241,9 +243,19 @@ bump_repo() { done # 2. Build ONE tree off the default-branch tip carrying all staged blobs. + # `base_tree` must be a TREE SHA, not a commit SHA — the Create-a-tree API + # rejects a commit SHA (422), and a tree built with no valid `base_tree` + # would carry ONLY the bumped files, so merging its PR would delete every + # other path in the caller repo. `MAIN_SHA` is the tip *commit* SHA (still + # correct as the commit's parent below), so resolve its tree first. + local MAIN_TREE_SHA + MAIN_TREE_SHA=$(gh api "repos/${REPO}/git/commits/${MAIN_SHA}" --jq '.tree.sha') || { + echo "::warning::${REPO}: cannot resolve base tree — skipping" + return 1 + } local TREE_SHA TREE_SHA=$(printf '%s\n' "${TREE_ENTRIES[@]}" \ - | jq -s --arg base "$MAIN_SHA" '{base_tree: $base, tree: .}' \ + | jq -s --arg base "$MAIN_TREE_SHA" '{base_tree: $base, tree: .}' \ | gh api --method POST "repos/${REPO}/git/trees" --input - --jq '.sha') || { echo "::warning::${REPO}: tree create failed — skipping" return 1 diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index 3bcffd2..174ddaf 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -103,6 +103,9 @@ done # single repo, so branch_files reflects exactly that repo's PR.) case "$method:$path" in POST:*/git/blobs*) # blob create — capture the new file content, count it + # The script now sends the base64 body on stdin (--input -), so read the + # content out of the JSON body rather than the old --field content= on argv. + content=$(jq -r '.content' <(cat)) n=$(( $(cat "$STUB_PUT_DIR/count" 2>/dev/null || echo 0) + 1 )) echo "$n" > "$STUB_PUT_DIR/count" printf '%s' "$content" | { base64 -d 2>/dev/null || base64 -D; } > "$STUB_PUT_DIR/put.$n.txt" @@ -110,7 +113,13 @@ case "$method:$path" in echo "blobsha${n}" exit 0;; POST:*/git/trees*) # tree create — the stdin body lists every bumped path - jq -r '.tree[].path' > "$STUB_PUT_DIR/branch_files" + body=$(cat) + jq -r '.tree[].path' <<<"$body" > "$STUB_PUT_DIR/branch_files" + # Record base_tree so the suite can assert it is the TIP's TREE sha (resolved + # via GET git/commits below), NOT the tip COMMIT sha — the real Create-a-tree + # API rejects a commit sha, and a missing/invalid base_tree drops every other + # file in the caller repo. + jq -r '.base_tree // ""' <<<"$body" > "$STUB_PUT_DIR/branch_base_tree" echo "treesha1" exit 0;; POST:*/git/commits*) # commit create — drain the body, return a commit sha @@ -137,6 +146,11 @@ if [[ "$path" == *"/contents/"* ]]; then fi b64=$(base64 < "$STUB_CONTENT_FILE" | tr -d '\n') printf '{"sha":"blobsha123","content":"%s"}' "$b64" +elif [[ "$path" == *"/git/commits/"* ]]; then + # Resolve the tip commit's TREE sha (distinct from the commit sha) — the script + # must pass THIS as base_tree, not the commit sha it was parented on. The stub + # discards --jq (see arg loop), so emit the post-`.tree.sha` value directly. + echo "maintreesha1" elif [[ "$path" == *"/git/refs/heads/"* ]]; then echo "1234567890abcdef1234567890abcdef12345678" else @@ -189,6 +203,12 @@ check "stale pin comment removed" "! grep -qF '# github-workflows#27 # exactly one trailing newline (#23): last byte is \n (tail -c1 strips to empty), # and the last two bytes are not both \n (tail -c2 keeps a non-newline byte). check "single trailing newline" "[[ -z \"\$(tail -c1 \"$PUT\")\" && -n \"\$(tail -c2 \"$PUT\")\" ]]" +# base_tree must be the tip's TREE sha (resolved via GET git/commits), NOT the +# tip COMMIT sha — a commit sha 422s the real Create-a-tree API, and a bad +# base_tree drops every other file in the caller repo (BE-3902). +BBT="${STUB_PUT_DIR}/branch_base_tree" +check "base_tree is the resolved tree sha" "[[ \"\$(cat \"$BBT\")\" == 'maintreesha1' ]]" +check "base_tree is NOT the commit sha" "! grep -qF '1234567890abcdef1234567890abcdef12345678' \"$BBT\"" # No open PR for the stable branch → the create path runs, not the edit path. check "opened a new PR (pr create called)" "grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" check "did not edit (no open PR existed)" "! grep -q '^pr-edit' \"\$STUB_PUT_DIR/pr.log\""