From 4455be201228c4fb017bbfa4e55f8816b6dca5a5 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 20 Jul 2026 23:59:46 -0700 Subject: [PATCH 1/2] 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 2/2] 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 ]]