fix(groom): pin ci-groom.yml by SHA and stop the bumper reporting unpinnable callers as converged (BE-6015)

.github/bump-callers/README.md

@@ -68,6 +68,31 @@ ended up failing at startup against a reusable it had drifted away from. Before
 adding a caller anywhere, check that its fleet exists **and** that the repo is in
 the variable; the second half is the one people skip.
 
+Being in the variable is necessary, not sufficient, in two different ways:
+
+- **A pin's SHAPE doesn't matter — the rewrite self-heals it.** The
+  substitution is anchored to the pin token, not to 40-hex-ness (BE-4662), so a
+  `uses:` or `workflows_ref:` pinned to a placeholder, a tag, a branch, or a
+  short SHA all move to `NEW_SHA` on the next bump regardless of what they
+  carried before. A shape the rewrite genuinely cannot move — today, only a
+  `workflows_ref` fed by a `${{ … }}` expression — is asserted against
+  post-rewrite and **fails the run** rather than shipping a half-bumped caller
+  (a partial bump is worse than no bump). That assertion is what actually
+  caught the second way `ci-groom.yml` broke (BE-6015): registered, but pinned
+  to a `REPLACE_AT_MERGE_…` placeholder a landed PR never replaced — the
+  rewrite moves a placeholder like any other shape, so this specific case is
+  now a normal, self-healing bump, not a failure.
+- **A roster entry can point at a file with nothing of ours to bump.** If the
+  file names some *other* github-workflows reusable and none of ours, that is
+  a stale roster entry, not a movable pin — the variable is the thing to fix,
+  not the file. `bump-callers.sh` checks this against the ORIGINAL content
+  before the no-op test, warns per file, then **fails the run** with an
+  aggregate error, instead of logging the reassuring
+  `already at <short> — skipping` that hid it for `ci-groom.yml`.
+
+Both failure modes land after the whole fleet is processed, so every other
+caller still gets its bump; what they refuse to do is report success.
+
 They stay as thin entrypoints rather than one matrix because their triggers
 differ: a `cursor-review.yml` change must not spuriously bump agents-md or
 pr-size callers, and vice versa. Everything else (masking, the PR-per-caller

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

@@ -394,6 +394,62 @@ bump_repo() {
       fi
     fi
 
+    # ROSTER-ENTRY CHECK (BE-6015) — asked of the ORIGINAL content, before the
+    # no-op test below. A stale roster entry can send this function to a file
+    # that calls no github-workflows reusable in a spelling this script can
+    # parse at all ("not provably ours" — treated exactly like the rewrite
+    # address above: a quiet skip) OR to a file that DOES call some
+    # github-workflows reusable, but only a SIBLING fleet's, never ours. That
+    # second case is not something the rewrite below can ever fix — there is
+    # nothing of ours in the file to bump — so it is caught here, before the
+    # no-op branch, and blamed on the roster entry rather than the file.
+    #
+    # This is narrower than it first was: two more checks used to sit here —
+    # "is `uses:` pinned by 40-hex" and "is `workflows_ref:` pinned by
+    # 40-hex" — but BE-4662 (landed on main after this PR was opened) anchored
+    # the rewrite to the pin TOKEN rather than to 40-hex-ness, so a
+    # placeholder, tag, or short-SHA pin is now self-healed by the sed above
+    # regardless of its prior shape. Keeping those two checks here would warn
+    # on — and, worse, refuse to stage — callers the bumper can in fact bump,
+    # contradicting the STALE_PINS assertion below on the very same file. The
+    # one shape STALE_PINS still cannot move, a `workflows_ref` fed by a
+    # `${{ … }}` expression, is a value the rewrite has never touched and that
+    # assertion already fails the run for.
+    local PIN_OK=1
+    if ! grep -qxF "$WORKFLOW_FILE" <<<"$GW_USES"; then
+      # `GW_USES` empty means no `uses:` line here names a github-workflows
+      # reusable in a spelling this script can parse — "not provably ours", which
+      # stays silent exactly as the rewrite address above does. Non-empty but
+      # WITHOUT our file is a different animal: the roster sent us to a file that
+      # calls only a SIBLING fleet's reusable, so there is nothing here for this
+      # fleet to bump and the stale entry — not the file — is the bug.
+      if [[ -n "$GW_USES" ]]; then
+        PIN_OK=0
+        echo "::warning::${REPO}: ${FILE} has no \`uses:\` calling ${WORKFLOW_FILE} — it is listed in ${VAR_NAME} but this fleet has nothing to bump in it; fix the roster entry"
+      fi
+    fi
+    # Accumulate AND skip rather than `return 1`: a roster entry with nothing
+    # of ours to bump is broken by definition, so the run must not report
+    # success — but it must also not abort the fan-out, since every OTHER
+    # caller in the fleet still deserves its bump. The aggregate `::error::`
+    # after the loop is what makes this unskimmable; BE-6015 persisted for
+    # weeks precisely because a warning in a green run reads as noise.
+    #
+    # The `continue` is load-bearing, not cosmetic: rule 2's `workflows_ref:`
+    # rewrite (line 336, below) is deliberately UNADDRESSED — it fires on any
+    # `workflows_ref:` key in the file regardless of SHA_ADDR, because in the
+    # legitimate multi-reusable case there is no cheap way to tell "our" input
+    # from a sibling job's. A sibling-only file (this branch) has no `uses:`
+    # of ours at all, so a bare `workflows_ref:` in it belongs entirely to the
+    # sibling — falling through to the rewrite/stage below would silently
+    # repoint that sibling's asset ref at THIS fleet's SHA and open a PR for
+    # it. Stopping here, before the rewrite, is the only way to keep the file
+    # untouched.
+    if (( ! PIN_OK )); then
+      UNPINNABLE+=("${REPO}:${FILE}")
+      continue
+    fi
+
     # ASSERT that the rewrite actually moved EVERY github-workflows pin in this
     # file, before it can be staged (BE-4662). The patterns above are precise by
     # design, and precision cuts both ways: a pin form they do not know how to
@@ -461,8 +517,10 @@ bump_repo() {
     # 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.
+    # The "already at" line is claimed only when the pin check above vouched for
+    # the file; otherwise the warning it just emitted is the whole story.
     if [[ "$NEW_CONTENT" == "$OLD_CONTENT" ]]; then
-      echo "${REPO}: ${FILE} already at ${SHORT} — skipping"
+      (( PIN_OK )) && echo "${REPO}: ${FILE} already at ${SHORT} — skipping"
       continue
     fi
 
@@ -654,6 +712,10 @@ for ENTRY in "${CALLERS[@]}"; do
 done
 
 FAILED=()
+# Caller files that carry no pin this fleet can move (BE-6015). Appended to from
+# inside bump_repo — deliberately a plain global, since the bump loop below runs
+# the function in this shell (no subshell/pipe), so the accumulation survives.
+UNPINNABLE=()
 for REPO in "${REPOS[@]}"; do
   # Collect this repo's entries (in their original order) and bump them together.
   ENTRIES=()
@@ -665,6 +727,17 @@ done
 
 if (( ${#FAILED[@]} )); then
   printf '::error::bump failed for %d repo(s): %s\n' "${#FAILED[@]}" "${FAILED[*]}"
+fi
+# Every other caller has been bumped by now; failing here (rather than at the
+# first offender) keeps the fan-out complete while still refusing to report
+# success for a fleet that contains a caller the bumper cannot keep current.
+# Repo names are `::add-mask::`ed at parse time, so this prints *** in the public
+# log while the file path stays readable enough to act on.
+if (( ${#UNPINNABLE[@]} )); then
+  printf '::error::%d caller file(s) this bumper cannot keep current for %s — see the per-file warnings above: %s\n' \
+    "${#UNPINNABLE[@]}" "${WORKFLOW_FILE}" "${UNPINNABLE[*]}"
+fi
+if (( ${#FAILED[@]} || ${#UNPINNABLE[@]} )); then
   exit 1
 fi
 echo "${TAG} bump complete for all callers."

.github/bump-callers/tests/test_bump_callers.sh

@@ -559,6 +559,61 @@ check "reported already at SHORT"              "grep -q 'already at $SHORT' <<<\
 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 "== a roster entry pointing at a file that calls ONLY a sibling reusable is the roster's bug =="
+# `GW_USES` non-empty proves the file calls SOME github-workflows reusable, not
+# ours. Telling a human to hand-pin a workflow this file does not use hides the
+# real problem — the stale entry in the fleet variable — so the warning names
+# that instead. (Silently skipping is not an option either: this file also
+# produces a no-op rewrite, i.e. the "already at" lie.) The fixture also carries
+# a bare `workflows_ref:` — rule 2's rewrite is unaddressed by design (it can't
+# tell "ours" from a sibling job's `with:` input) and would otherwise happily
+# repoint the SIBLING's asset ref at this fleet's SHA and stage the file; the
+# roster check must `continue` before that rewrite runs, not just warn.
+new_case wrongfleet
+WRONGFLEET_FIXTURE="${WORK}/wrongfleet_caller.yml"
+printf '%s\n' \
+  'name: CI cursor-review' \
+  'jobs:' \
+  '  review:' \
+  '    uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@1111111111111111111111111111111111111111' \
+  '    with:' \
+  '      workflows_ref: 2222222222222222222222222222222222222222' \
+  > "$WRONGFLEET_FIXTURE"
+STUB_CONTENT_FILE="$WRONGFLEET_FIXTURE" run_bump \
+  VAR_NAME=GROOM_CALLERS TAG=groom WORKFLOW_FILE=groom.yml \
+  CALLERS_JSON='[{"repo":"Comfy-Org/secret-wrongfleet","file":".github/workflows/ci-groom.yml","label":""}]'
+check "exit 1" "[[ $RC -eq 1 ]]"
+check "blamed the roster entry, not the file" \
+  "grep -q '::warning::.*has no \`uses:\` calling groom.yml.*GROOM_CALLERS.*fix the roster entry' <<<\"\$OUT\""
+check "did NOT tell a human to hand-pin groom.yml"  "! grep -q 'pin it by full SHA by hand' <<<\"\$OUT\""
+check "did NOT claim the file was already current"  "! grep -q 'already at $SHORT' <<<\"\$OUT\""
+check "committed nothing (uses: AND workflows_ref both left alone)" \
+  "[[ ! -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 "== a file naming NO github-workflows reusable stays silent (no false positives) =="
+# The unusual-spelling escape hatch, now that a bad roster entry can fail the run:
+# `GW_USES` empty means no `uses:` line names a github-workflows reusable in a
+# spelling this script parses. "Not provably ours" must stay a quiet skip exactly
+# as the rewrite address treats it — a check that turns every unparsed caller red
+# is a check people disable.
+new_case nogw
+NOGW_FIXTURE="${WORK}/nogw_caller.yml"
+printf '%s\n' \
+  'name: CI something else' \
+  'jobs:' \
+  '  build:' \
+  '    steps:' \
+  '      - uses: actions/checkout@1111111111111111111111111111111111111111' \
+  > "$NOGW_FIXTURE"
+STUB_CONTENT_FILE="$NOGW_FIXTURE" run_bump \
+  VAR_NAME=GROOM_CALLERS TAG=groom WORKFLOW_FILE=groom.yml \
+  CALLERS_JSON='[{"repo":"Comfy-Org/secret-nogw","file":".github/workflows/ci-groom.yml","label":""}]'
+check "exit 0" "[[ $RC -eq 0 ]]"
+check "emitted no pin warning"        "! grep -q 'pin it by full SHA by hand' <<<\"\$OUT\""
+check "emitted no roster warning"     "! grep -q 'fix the roster entry' <<<\"\$OUT\""
+check "left the unrelated action pin alone" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]"
+
 echo "== a TAG-pinned workflows_ref moves in lock-step with uses: (BE-4662) =="
 # The under-rewrite half of BE-4662. A caller pins this repo TWICE — the `uses:`
 # sha and the `workflows_ref` input that loads the briefs/prompts/scripts. The
@@ -823,6 +878,7 @@ check "exit 1 — the repo failed"                 "[[ $RC -eq 1 ]]"
 check "warning names the empty pin"              "grep -qF '(empty)' <<<\"\$OUT\""
 check "committed NOTHING"                        "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]"
 
+
 echo "== an ALREADY-CONVERTED 'main (<short>)' marker is refreshed, not frozen (BE-4523) =="
 # The legacy `# github-workflows#NN` rule only fires once. After a caller has
 # been migrated to the `main (<short>)` marker, every later bump used to advance

AGENTS.md

@@ -148,8 +148,19 @@ tests — run the matching command above for whatever you touched.
   its `vars.*_CALLERS` roster. Skipping the second is the most repeated mistake
   here: the pin then never moves, the caller drifts behind the reusable, and it
   fails at startup much later with no obvious cause. This repo did it to its own
-  `ci-groom.yml`. When auditing, compare the roster against reality in both
-  directions — a roster entry whose caller file does not exist is equally broken.
+  `ci-groom.yml`. Being listed is necessary, not sufficient: a caller's pin
+  shape (placeholder, tag, branch, short SHA) no longer matters — the rewrite
+  is anchored to the pin token, not to 40-hex-ness, so it self-heals any of
+  those on the next bump (BE-4662), and a shape it truly cannot move (e.g. a
+  `workflows_ref` fed by a `${{ … }}` expression) fails the run rather than
+  shipping a half-bumped caller. What the bumper *can't* fix is a roster entry
+  pointing at a file that names some *other* github-workflows reusable but
+  never ours — that is the roster entry being wrong, not a pin to move, and it
+  fails the run naming it (BE-6015); a file naming no github-workflows
+  reusable at all in a spelling the bumper can parse stays a quiet skip, same
+  as an untouched line. When auditing, compare the roster against reality in
+  both directions — a roster entry whose caller file does not exist is equally
+  broken.
 - **New reusable workflow?** `on: workflow_call` + a header comment documenting
   inputs/secrets/triggers + a caller-pattern example, then a
   `docs/callers/<name>.md` setup guide and a row in the README table (see