diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index 87adde0..a4a1679 100644 --- a/.github/bump-callers/README.md +++ b/.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 — 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 diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index 64a9bb1..f8dedcb 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.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." diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index 98d7903..739c632 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.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 ()' 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 ()` marker, every later bump used to advance diff --git a/AGENTS.md b/AGENTS.md index 568bc36..b2e93c8 100644 --- a/AGENTS.md +++ b/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/.md` setup guide and a row in the README table (see