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