diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index 87adde0..4cb74f1 100644 --- a/.github/bump-callers/README.md +++ b/.github/bump-callers/README.md @@ -126,6 +126,11 @@ unrelated full SHA that merely *shares* a line with the words `github-workflows` or `workflows_ref` is now unreachable, and a prose comment mentioning `workflows_ref:` is left as prose. +The ref pattern stops at whitespace *and* at YAML's flow-style delimiters (`,`, +`}`), so the ref in `{uses: …@v1, secrets: inherit}` does not swallow the mapping's +comma. Flow style is not rewritten — it is failed, by the assertion below — but it +is never silently *corrupted*. + Precision cuts both ways, though — a pin form the patterns don't know how to move would be silently left behind. So before a rewritten file can be staged, the script re-reads it with a deliberately **broader** reader (any non-whitespace @@ -171,3 +176,81 @@ gh variable set AGENTS_MD_CALLERS --repo Comfy-Org/github-workflows \ Keep the canonical `callers.json` in a private infra/ops repo so variable edits have a reviewed source of truth (the org audit log records each edit). + +### Entry format — validated, and why + +Every field reaches a privileged sink: `repo` is interpolated into each +`gh api repos//…` write and into `gh pr create --repo`, `file` becomes the +path committed into the caller's tree, and `label` is passed to +`gh pr create --label`. They run under an org-wide app token (`owner: Comfy-Org`, +contents + pull-requests + issues write, no `repositories:` narrowing), and the +roster is a **variable** — editable outside code review. So `bump-callers.sh` +constrains the values before it makes a single API call: + +| Field | Rule | +|---|---| +| `repo` | must match `^Comfy-Org/[A-Za-z0-9._-]+$` (the owner case-insensitively, as GitHub itself resolves it), and the name may not consist *only* of dots — `\A[.]+\z`, so `..` and `...` alike (a dot-leading or dot-bearing name such as `.github` or `a.b.c` is fine; an all-dots name is a path segment, not a repo) | +| `file` | must match `^\.github/workflows/[A-Za-z0-9._-]+\.ya?ml$` (the class excludes `/`, so `../` traversal cannot appear) | +| `label` | optional; when present must be a string containing no `\|`, no comma, and no control character | + +Each pattern is anchored `\A…\z`, not `^…$`: jq matches with Oniguruma, where `$` +also matches *before a trailing newline*, so `"Comfy-Org/legit\n"` would otherwise +pass and then split into two tuples — one of them never validated, with an empty +`repo`. + +A violation is a **hard fail before the fan-out**, reported as the entry's +zero-based **index** and the rule it broke — never the value, because masking has +not been applied at that point and this repo's run logs are public. The `label` +rule is not cosmetic. Entries are carried internally as `repo|file|label|wire_bot` +tuples, so a pipe-bearing label would truncate and bleed its tail into the +`wire_bot` field; a control character is dropped or mangled somewhere between +`jq` and the flag (bash's `read` silently discards a NUL); and `--label` is a +cobra StringSlice, which CSV-splits, so `ci,do-not-merge` would quietly apply a +second, potentially blocking label the entry does not appear to name. Spaces, +`:` and `/` are all still fine — this bars what is structurally unsafe, not what +GitHub disallows. + +The owner is normalised to `Comfy-Org` once validated, and repos are grouped +case-insensitively. Accepting `comfy-org/x` without folding it would make two +spellings of one repo into two bump runs against it, the second force-moving the +shared bump branch off the first's commit — a green run shipping a partial bump. + +### Un-bumpable entries fail the run + +Being in the variable is necessary, not sufficient. The bumper can only move a +pin it can *find*, and a file carrying no pin this fleet can address rewrites to +itself — which the content-equality check then reports as the reassuring +`already at — skipping`. That is how a wrong roster entry drifts forever +behind a green run. Each caller file is now checked for such a pin *before* the +rewrite; if there is none, the run warns per file and then **fails** with an +aggregate error naming how many caller files were affected. Three shapes trip it: + +- the file **does not exist** on the caller's default branch (a renamed or + typo'd path) — previously a silent `not found — skipping`; +- the file carries no `uses:` pin of `Comfy-Org/github-workflows` at all (wrong + file, or not a caller); +- its only `github-workflows` `uses:` names a **sibling** fleet's reusable, so + this fleet has nothing to move — the stale entry, not the file, is the bug. + +A `uses:` pin is required in all three cases: a bare `workflows_ref:` never +rescues a file. That input carries no workflow name, so it cannot vouch for +*this* fleet, and admitting it would let the run stamp this fleet's SHA onto a +sibling caller's assets ref. Every caller of every seeded fleet carries a `uses:` +pin today, so nothing legitimate is caught by this. + +"`uses:` pin" is meant literally: the check is anchored to the `uses:` key +(optionally quoted value), not merely to a `Comfy-Org/github-workflows@` +token somewhere on the line. A `run:` step that curls this repo, or a repo@ref +passed as an input to some *other* action, is not a caller — and since the pin +rewrite keys on the same token, admitting one would mean opening a bump PR +against a file that never called us. + +What does **not** trip it is a pin that is merely not a full SHA. The rewrite +matches a ref by position rather than shape, so a caller on `@v1` is *self-healed* +to the new SHA — dragging floating pins back onto immutable ones is the point of +the fleet, not an error. Nor does a pin the rewrite knows about but cannot move +(a `uses:` ref fed by a `${{ … }}` expression): that is admitted here and then +fails loudly in the post-rewrite assertion, which is the check that owns it. The +failure lands after every caller has been processed, +so one bad entry never blocks the rest of the fleet's bumps; what it refuses to do +is report success. diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index 64a9bb1..d452990 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -114,8 +114,19 @@ INPUT_KEY_RE='[[:space:],{]workflows_ref:' # is never half-rewritten into a broken value — an expression matches nothing # here, is left untouched, and is then caught by the assertion, which fails the # repo rather than shipping a caller whose two pins disagree. +# +# `,` is excluded for the same reason as `}`: it is a YAML FLOW-STYLE delimiter, +# not part of a ref. In `{uses: …/cursor-review.yml@v1, secrets: inherit}` the +# token after `@` is `v1,` — whitespace being the only other stop — so admitting +# the comma would replace the delimiter along with the ref and commit YAML with +# the mapping's separator deleted, which the post-rewrite assertion (it only asks +# whether NEW_SHA is there) would wave through. Excluding it leaves the comma +# behind, so the assertion reads back `,`, finds it unequal to NEW_SHA and +# fails the repo — the same loud outcome flow style already gets on the +# `workflows_ref:` side (see INPUT_PIN_RE above). A ref never legitimately +# contains a comma, so nothing bumpable is lost. # shellcheck disable=SC2016 # `$ { }` here are regex literals, not an expansion -REF_RE='[^[:space:]'\''"#$(){}]+' +REF_RE='[^[:space:],'\''"#$(){}]+' STRIPPED="${CALLERS_JSON//[[:space:]]/}" @@ -139,16 +150,120 @@ if ! jq -e 'type == "array" and length > 0 and all(.[]; (.repo | type == "string exit 1 fi +# Constrain the field VALUES, not just the JSON shape (BE-6471). Every one of +# these reaches a privileged sink: `repo` is interpolated into each +# `gh api repos/${REPO}/…` write and `gh pr create --repo`, `file` becomes the +# path committed into the caller's tree, and `label` is handed to +# `gh pr create --label`. They run under an org-wide app token +# (`owner: Comfy-Org`, contents + pull-requests + issues write, no +# `repositories:` narrowing) and the roster is an Actions VARIABLE, which is +# editable outside code review — so without this, roster-edit access converts +# into bot-authored commits, branch force-moves and labelled PRs in any org repo +# the app is installed on. The `file` rule's character class excludes `/`, which +# also rules out a `.github/workflows/../…` traversal out of the directory. The +# `repo` rule keeps `.` (a repo may legitimately be named `.github`, or carry a +# dot at all) but rejects a name that is ONLY dots, since `Comfy-Org/..` is a path +# segment rather than a repository and every use of it is a URL built by string +# interpolation. The test is `\A[.]+\z`, i.e. the general case rather than the two +# literals `.` and `..`: `Comfy-Org/...` is no more a repository than `..` is, and +# enumerating only the two would let it through the named pre-flight rule to 404 +# later as an unexplained whole-repo failure. +# +# `label` additionally may not contain `|`: the tuple encoding just below joins +# the four fields with `|` and `cut -d'|' -f3` reads the label back, so a +# pipe-bearing label truncates at the pipe and bleeds its tail into the +# `wire_bot` field — silently flagging for identity wiring an entry that never +# asked for it. Every C0/C7F CONTROL character is barred for the same class of +# reason, `[[:cntrl:]]` rather than an enumeration of `\n`/`\r` so the class is +# closed rather than sampled: the tuples are read line-wise (LF, CR), a NUL is +# emitted by `jq -r` and then silently DISCARDED by bash's `read`, so the label +# that reaches `gh pr create --label` differs from the one the roster names, and +# any surviving control reaches that flag verbatim, where the API rejects it as +# an opaque per-repo failure rather than a named rule. +# +# `,` is barred too, and for a reason that is not encoding at all: `--label` is a +# cobra StringSlice, which CSV-SPLITS its value. `ci,do-not-merge` therefore +# applies a SECOND, potentially blocking label that the entry does not appear to +# name — the one shape here that fails by succeeding quietly. GitHub label names +# may legitimately contain spaces, `:` and `/`, so this stays a denylist of the +# characters that are structurally unsafe on the path to the flag, not an +# allowlist of the ones GitHub happens to permit. Verified against the live +# rosters: every label in use today is `""`, `ci` or `Core`. +# +# Every pattern is anchored with `\A`/`\z`, NOT `^`/`$`. jq matches with +# Oniguruma, where `$` also matches immediately BEFORE a trailing newline — so +# `^Comfy-Org/[A-Za-z0-9._-]+$` accepts `"Comfy-Org/legit\n"`, whose tuple then +# splits across two lines in the `while IFS= read -r ENTRY` loop below. The +# fragment after the split is a never-validated tuple: its `REPO` is empty and +# reaches `::add-mask::` and `gh api repos/`, and the fragment before it has no +# `|` at all, so `cut -d'|' -f4` returns the whole line and silently flags the +# entry for identity wiring. `\A`/`\z` are strict whole-string anchors and close +# that off. (`^`/`$` are NOT line anchors here — an embedded newline mid-value is +# already rejected — but the trailing-newline case is real.) +# +# The `Comfy-Org/` owner is matched case-INSENSITIVELY, for the same reason +# REPO_RE above is spelled that way: GitHub resolves an owner case-insensitively, +# so `comfy-org/foo` is a working roster entry today and must not hard-fail the +# whole fleet before a single repo is bumped. +# +# NEVER echo the offending value. Masking has not happened yet at this point +# (the `::add-mask::` loop is below), this repo's run logs are public, and roster +# values are private repo names — so report the INDEX and the violated RULE only, +# which is all anyone needs to find the entry in the variable. One jq pass emits +# `indexrule` for every violation, so an entry breaking two rules reports +# both rather than hiding the second behind a fix for the first. +if ! INVALID=$(jq -r ' + to_entries[] + | .key as $i + | .value as $e + | [ (if ($e.repo | test("\\AComfy-Org/[A-Za-z0-9._-]+\\z"; "i")) + and (($e.repo | sub("\\A[Cc][Oo][Mm][Ff][Yy]-[Oo][Rr][Gg]/"; "")) | test("\\A[.]+\\z") | not) + then empty + else "repo must match ^Comfy-Org/[A-Za-z0-9._-]+$ (owner case-insensitive) and not be a dot segment" end), + (if ($e.file | test("\\A[.]github/workflows/[A-Za-z0-9._-]+[.]ya?ml\\z")) then empty + else "file must be a .github/workflows/.yml path" end), + (if ($e.label == null) + or ((($e.label | type) == "string") and (($e.label | test("[|,]|[[:cntrl:]]")) | not)) + then empty else "label must be a string containing no |, comma or control character" end) + ][] + | "\($i)\t\(.)"' <<<"$CALLERS_JSON"); then + echo "::error::${VAR_NAME} could not be validated (jq failed). Fix the variable — see the workflow header." + exit 1 +fi +if [[ -n "$INVALID" ]]; then + while IFS=$'\t' read -r IDX RULE; do + echo "::error::${VAR_NAME} entry at index ${IDX} is invalid (${RULE}). Fix the variable — see the workflow header." + done <<<"$INVALID" + exit 1 +fi + # Parse the JSON into repo|file|label|wire_bot tuples, and mask every repo name # in the (publicly viewable) run logs BEFORE the loop that echoes it, so all # per-repo output shows *** instead of a private repo name. jq's `//` treats # both `false` and `null` as falsy, so an absent/false/null wire_bot all print # the empty string here — exactly the "not flagged" case. +# +# The OWNER segment is CANONICALIZED to `Comfy-Org` on the way in. The rule above +# accepts it case-insensitively (GitHub resolves an owner that way, so +# `comfy-org/foo` is a working entry today and must not hard-fail the fleet), but +# accepting two spellings without normalizing one produces two DISTINCT entries +# for a single repo: the grouping below is byte-exact, so `Comfy-Org/foo` and +# `comfy-org/foo` become two `REPOS` entries and `bump_repo` runs TWICE against +# the same repo. Each run builds its commit off `MAIN_SHA` carrying only its own +# files and force-PATCHes the shared `refs/heads/ci/bump-`, so the second +# wipes the first's bumped files off the branch and then merely edits the +# already-open PR — a green run reporting a partial bump, which is the BE-3896 +# class this refactor exists to kill. Canonicalizing here (rather than at the +# grouping) means the masked value, every log line and every API path use ONE +# spelling. The repo NAME's case is left as written: it is only ever consumed by +# GitHub, which resolves it case-insensitively, and the grouping below matches it +# case-insensitively too, so a `Foo`/`foo` pair collapses without the log having +# to show a name nobody typed. CALLERS=() while IFS= read -r ENTRY; do echo "::add-mask::${ENTRY%%|*}" CALLERS+=("$ENTRY") -done < <(jq -r '.[] | "\(.repo)|\(.file)|\(.label // "")|\(.wire_bot // "")"' <<<"$CALLERS_JSON") +done < <(jq -r '.[] | "Comfy-Org/\(.repo | sub("\\A[^/]+/"; ""))|\(.file)|\(.label // "")|\(.wire_bot // "")"' <<<"$CALLERS_JSON") if (( ${#CALLERS[@]} == 0 )); then echo "::error::${VAR_NAME} parsed to zero callers — refusing to run a no-op dispatcher." @@ -188,7 +303,13 @@ bump_repo() { # 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" + # The branch NAME is deliberately not interpolated into any per-file message. + # It comes from the private caller's repo metadata and is never `::add-mask::`ed + # (only repo names are), so a branch named after an internal project or codename + # would be printed verbatim into this public repo's run logs. Masking it instead + # is worse: the value is usually `main`, and masking `main` would redact the + # word everywhere it legitimately appears (`# main @ `, MAIN_SHA reports). + echo "::warning::${REPO}: cannot read the default-branch ref — skipping" return 1 } @@ -196,13 +317,30 @@ bump_repo() { # anything yet. This keeps the reset/commit path off entirely for a repo whose # files are all missing or already pinned (the old per-entry skips), so it # never resets a branch or opens a PR it doesn't need. - local -a PEND_FILE=() PEND_CONTENT=() LABELS=() + local -a PEND_FILE=() PEND_CONTENT=() LABELS=() SKIP_FILE=() local WIRING_ADDED_ANY="" - local ENTRY FILE LABEL WIRE_BOT FILE_ENC CURRENT OLD_CONTENT NEW_CONTENT + + # `wire_bot` is unioned PER FILE, up front, rather than read off whichever entry + # happens to reach the staging code first. A repo may list one path twice (a + # structurally valid config the dedup below already handles), and only the FIRST + # of those entries is ever staged — so reading the flag per entry made the + # outcome depend on roster ORDER: `[{ci.yml, wire_bot: true}, {ci.yml}]` wired + # the identity while the reverse order silently did not. Labels are already + # unioned across duplicates for exactly this reason; this is the same rule for + # the other per-entry field. A file flagged by ANY of its entries is wired. + local -a WIRE_FILES=() + local ENTRY FILE LABEL WIRE_BOT FILE_ENC CURRENT OLD_CONTENT NEW_CONTENT P + for ENTRY in "$@"; do + [[ -n "$(cut -d'|' -f4 <<<"$ENTRY")" ]] && WIRE_FILES+=("$(cut -d'|' -f2 <<<"$ENTRY")") + done + for ENTRY in "$@"; do FILE=$(cut -d'|' -f2 <<<"$ENTRY") LABEL=$(cut -d'|' -f3 <<<"$ENTRY") - WIRE_BOT=$(cut -d'|' -f4 <<<"$ENTRY") + WIRE_BOT="" + for P in ${WIRE_FILES[@]+"${WIRE_FILES[@]}"}; do + [[ "$P" == "$FILE" ]] && { WIRE_BOT=1; break; } + done FILE_ENC="${FILE//\//%2F}" # De-duplicate by file: a repo listed twice for the SAME path (a structurally @@ -211,27 +349,60 @@ bump_repo() { # 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 + # + # SKIP_FILE is consulted alongside PEND_FILE so the dedup also covers every + # path that was HANDLED but not staged — rejected (absent, or carrying no + # movable pin) and already-converged alike, i.e. all three of the early + # `continue`s below. Without it a doubly-listed broken path is re-fetched, + # warned about twice, and counted twice in the NOPIN tally — reporting more + # broken files than there are distinct broken roster entries — and a + # doubly-listed already-current path is re-fetched and skip-logged twice. + # + # The two sets are scanned SEPARATELY because only a PENDING match may fold + # its label. A duplicate of a SKIPPED path is an entry whose file is not in + # this PR at all, and the label-collection site below states the invariant: + # labels from skipped entries must not land on the repo's real bump PR, or a + # monorepo that lists a 404'd path twice — the second entry carrying, say, + # `do-not-merge` — would stamp that blocking label onto the PR built from its + # OTHER, healthy file. + local SEEN_FILE=0 SEEN_PENDING=0 for P in ${PEND_FILE[@]+"${PEND_FILE[@]}"}; do - [[ "$P" == "$FILE" ]] && { SEEN_FILE=1; break; } + [[ "$P" == "$FILE" ]] && { SEEN_FILE=1; SEEN_PENDING=1; break; } done + if (( ! SEEN_FILE )); then + for P in ${SKIP_FILE[@]+"${SKIP_FILE[@]}"}; do + [[ "$P" == "$FILE" ]] && { SEEN_FILE=1; break; } + done + fi if (( SEEN_FILE )); then - [[ -n "$LABEL" ]] && LABELS+=("$LABEL") + if (( SEEN_PENDING )) && [[ -n "$LABEL" ]]; then LABELS+=("$LABEL"); fi 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. + # 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. + # + # A 404 is NOT a benign skip (BE-6471): the roster named a path that does not + # exist, so this entry can never be bumped — the renamed- or typo'd-caller case + # that is the whole reason for the un-bumpable tally below, and strictly worse + # than the no-movable-pin case since not even the file is there. It is recorded + # in NOPIN and fails the job at the END (never `return 1`, which would punish + # this repo's other, perfectly bumpable files). This deliberately turns a + # previously green run red when a roster entry has gone stale; that is the + # point — this repo's own `ci-groom.yml` drifted for weeks behind exactly such + # a silent skip. 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" + echo "::warning::${REPO}: ${FILE} not found on the default branch — fix or remove its ${VAR_NAME} entry" + NOPIN+=("${REPO}|${FILE}") + SKIP_FILE+=("$FILE") rm -f "$ERRFILE" continue fi @@ -303,9 +474,139 @@ bump_repo() { # un-bumped (a `workflows_ref` disagreeing with its own `uses:` pin) is the # worse failure. Unreachable while no caller calls two reusables; if one ever # does, parse the YAML instead of extending this rule. - local SHA_ADDR='github-workflows|workflows_ref' + # The address's filename half needs a LEFT delimiter. `github-workflows[^ + # [:space:]]*groom\.yml` is satisfied by a sibling caller pinning + # `…/workflows/legacy-groom.yml` — the target name merely ENDS the token — so + # that caller would be treated as ours, repinned to this fleet's SHA by rules + # 1/4-6, and pass the address-filtered assertion. Requiring the `/` that + # starts the filename scopes it to the intended reusable. A right delimiter is + # unnecessary: `[^[:space:]]*` cannot cross whitespace, so the match is + # confined to the single `uses:` token, where `.yml` is followed by `@`. + # + # SHA_ADDR is consumed two ways and needs two spellings. `grep -E` takes it + # raw; sed takes it as a `/…/` ADDRESS, where an unescaped `/` would close the + # address early and silently change what the rules match — so the sed form + # escapes it, which is the delimiter-escape POSIX defines for exactly this. + # (The non-sibling address contains no `/`, so the two forms coincide there.) + local SHA_ADDR SHA_ADDR_SED + SHA_ADDR='github-workflows|workflows_ref' if (( GW_HAS_SIBLING )); then - SHA_ADDR="github-workflows[^[:space:]]*${WORKFLOW_FILE//./\\.}|workflows_ref" + SHA_ADDR="github-workflows[^[:space:]]*/${WORKFLOW_FILE//./\\.}|workflows_ref" + fi + SHA_ADDR_SED="${SHA_ADDR//\//\\/}" + + # ASSERT, BEFORE the rewrite, that this file carries a pin THIS fleet can + # address at all (BE-6471). The post-rewrite assertion further down answers + # "did every pin we found move?"; it cannot answer "was there a pin to find?" + # — and neither can the content-equality skip below, because a file with no + # addressable pin rewrites to itself and is reported as `already at — + # skipping`. That reads as convergence and is why a misconfigured roster entry + # can drift forever behind a green run: this repo's own `ci-groom.yml` was + # absent from `GROOM_CALLERS` for weeks while nothing in the log said so. + # + # "Addressable" is read on the SAME SHA_ADDR-eligible lines as the rewrite, + # with the ASSERTION's REF reader (`[^[:space:]]+`, hung off a `uses:`-anchored + # pin token — see ADDR_RE below) rather than the + # rewrite's narrower `REF_RE`. That pairing is deliberate: this gate asks "is + # there a pin here at all?", and anything the assertion below would judge must + # count as one. Reading with REF_RE instead would classify a pin the rewrite + # cannot move — `uses: …@${{ inputs.ref }}`, whose ref begins with a character + # REF_RE excludes — as "no pin at all", skipping the file and letting the + # repo's OTHER files ship a PR, where the assertion previously failed the whole + # repo. Broad here, narrow there: the gate admits it, the assertion fails it. + # + # Note what this does NOT require: a 40-hex ref. Rules 1-2 move a ref by + # POSITION, not by shape (BE-4662), so a caller pinned to `@v1` is self-healed + # to NEW_SHA rather than reported — demanding a full SHA here would fail + # exactly the callers the bumper exists to fix. What is left is the genuinely + # un-bumpable roster entry: a file that names no `Comfy-Org/github-workflows` + # pin (wrong file, or not a caller at all), or one whose only github-workflows + # `uses:` belongs to a DIFFERENT fleet's reusable, where the stale entry rather + # than the file is the thing to fix. + # + # That second case is why a bare `workflows_ref:` does NOT vouch for the file — + # UNCONDITIONALLY, not merely when a sibling `uses:` was detected. SHA_ADDR's + # `workflows_ref` alternative is not fleet-scoped (it cannot be — see rule 2's + # comment above), so a sibling fleet's DOUBLE-pinning caller (groom and pr-risk + # callers carry both `uses:` and `workflows_ref:`) would otherwise pass this + # gate on its `workflows_ref:` line alone: rule 1 correctly declines its `uses:` + # pin, but rule 2 — which is unaddressed — would still stamp THIS fleet's + # NEW_SHA onto that caller's `workflows_ref`, and the post-rewrite assertion is + # address-filtered, so it stays silent. A green run would ship a split-pin, + # cross-fleet PR: precisely the misfiled roster entry this gate exists to catch. + # + # Gating that requirement on GW_HAS_SIBLING left the hole open, because the + # flag is derived from a CASE-SENSITIVE grep (`github-workflows/\.github/…`, + # see GW_USES above) while a `uses:` pin is legal in any case. A caller spelled + # `comfy-org/GitHub-Workflows/…/groom.yml@…` — or one whose `uses:` of this + # repo is commented out or deleted, leaving the input behind — yields an EMPTY + # GW_USES, so GW_HAS_SIBLING is 0, so the relaxed address applied and the bare + # `workflows_ref:` vouched for the file after all. Requiring the `uses:` pin in + # both branches closes it, and costs nothing: a caller of this fleet always has + # one (`workflows_ref` is an input to a job whose `uses:` is the pin), which was + # checked rather than assumed — all 51 caller files across the six seeded + # rosters carry an addressable `uses:` pin today, so no live caller changes + # behaviour. It also makes the second bullet of the README's un-bumpable list + # true as written. + # + # Two known blind spots, called out so the warning is not read as gospel. + # SHA_ADDR's `github-workflows` literal is case-SENSITIVE while USES_PIN_RE is + # deliberately case-insensitive, so a caller spelled + # `comfy-org/GitHub-Workflows/…` yields an empty PIN_SCAN and is reported here + # as un-bumpable. That report is not wrong — the rewrite cannot move that pin + # either, and before this gate it was mis-logged as `already at ` — but + # the fix is the pin's spelling, not the roster entry, hence the hint in the + # warning text. No caller spells it that way today. + # + # The second is the price of the `uses:` anchoring below: a `uses:` whose VALUE + # sits on the next line (legal YAML, never written that way for `uses:`) leaves + # the key and the pin token on different lines, so this gate reports a file the + # line-wise rewrite could in fact have moved. That direction is the safe one — + # a false negative here is a red run naming the file, where a false POSITIVE is + # a bump PR opened against something that never called us — and it is the same + # trade every other rule in this block makes. Both spellings are named in the + # warning so the operator is not sent hunting the roster for a file problem. + # + # Comments are stripped first, by YAML's own rule (a `#` preceded by + # whitespace, every line pre-padded with a space so a `#` at column 0 counts), + # for the same reason the post-rewrite assertion strips them and for the same + # reason GW_USES is anchored to `^[^#]*uses:`: a commented-out old pin or a + # docs URL must not vouch for a live pin that is not there. + # + # The reader is anchored to the `uses:` KEY, not just to the pin token, so the + # code matches the contract the comment and README state ("carries a `uses:` + # pin of Comfy-Org/github-workflows"). Unanchored, `USES_PIN_RE[^[:space:]]+` + # is satisfied by any `Comfy-Org/github-workflows@` token anywhere on an + # eligible line — a `run:` curl of this repo, an `env:` scalar, a repo@ref + # handed to some OTHER action — none of which make the file a caller. Rule 1 + # is addressed by the same unscoped token, so such a file cleared the gate and + # was then rewritten and PR'd as a bump of something that never called us. + # The optional quote covers the legal `uses: "…@v1"` spelling; `[[:space:]]*` + # rather than a single space covers `uses: X` and the `- uses:` list form + # (the `-` sits left of the key, outside the match). + local PIN_SCAN ADDR_RE + PIN_SCAN=$(sed -E 's|^| |; s|[[:space:]]#.*$||' <<<"$OLD_CONTENT" | grep -E "$SHA_ADDR") || true + ADDR_RE="uses:[[:space:]]*['\"]?${USES_PIN_RE}[^[:space:]]+" + if ! grep -qE "$ADDR_RE" <<<"$PIN_SCAN"; then + # ${REPO} is safe to print — it was `::add-mask::`ed at parse time, so it + # renders as *** in the public log; ${FILE} is a `.github/workflows/` path, + # which the roster validation above already constrained to that shape. + echo "::warning::${REPO}: ${FILE} carries no ${WORKFLOW_FILE} pin this bumper can move (wrong file, a sibling fleet's reusable, a non-canonically-cased pin, a uses: value on its own line, or not a caller at all?) — fix or remove its ${VAR_NAME} entry" + # Recorded, NOT returned: the per-repo loop turns a non-zero return into a + # whole-repo FAILED entry, which would punish this repo's OTHER, perfectly + # bumpable files. Skip just this file and let the end-of-run accounting fail + # the job once every caller has had its turn. SKIP_FILE so a repo listed + # TWICE for this same path is warned and tallied ONCE — the PEND_FILE dedup + # above cannot see a file that never reaches the pending set. + # + # Skipping the file also skips the `wire_bot` block further down, so say so + # rather than letting a flagged entry quietly lose its identity wiring: the + # entry is being declared broken, and wiring a bot identity into a file we + # have just decided we cannot address would be the wrong half of a bump. + [[ -n "$WIRE_BOT" ]] && echo "::warning::${REPO}: ${FILE} is also flagged wire_bot — its identity wiring was NOT applied either" + NOPIN+=("${REPO}|${FILE}") + SKIP_FILE+=("$FILE") + continue fi # # The `# github-workflows#NN` legacy marker (and its already-converted @@ -332,11 +633,11 @@ bump_repo() { # matches neither. A hex-boundary assertion (`\b`, `[[:>:]]`) would be simpler # but spells differently in GNU and BSD sed; this is portable ERE. NEW_CONTENT=$(sed -E " - /${SHA_ADDR}/ s|(${USES_PIN_RE})${REF_RE}|\1${NEW_SHA}|g + /${SHA_ADDR_SED}/ s|(${USES_PIN_RE})${REF_RE}|\1${NEW_SHA}|g s|^(${INPUT_PIN_RE})${REF_RE}|\1${NEW_SHA}| - /${SHA_ADDR}/ s|# main @ [0-9a-f]{40}|# main @ ${NEW_SHA}|g - /${SHA_ADDR}/ s|# main @ [0-9a-f]{7,12}([^0-9a-f])|# main @ ${SHORT}\1|g - /${SHA_ADDR}/ s|# main @ [0-9a-f]{7,12}\$|# main @ ${SHORT}|g + /${SHA_ADDR_SED}/ s|# main @ [0-9a-f]{40}|# main @ ${NEW_SHA}|g + /${SHA_ADDR_SED}/ s|# main @ [0-9a-f]{7,12}([^0-9a-f])|# main @ ${SHORT}\1|g + /${SHA_ADDR_SED}/ s|# main @ [0-9a-f]{7,12}\$|# main @ ${SHORT}|g " <<<"$OLD_CONTENT") # Normalize the human-readable pin annotation so it never disagrees with the @@ -463,6 +764,12 @@ bump_repo() { # 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" + # SKIP_FILE here too, so this third early-`continue` gets the same + # fetched-once/reported-once property as the other two: a repo listing the + # same already-converged path twice would otherwise re-fetch it and print + # the skip line twice. Recording it in SKIP_FILE (not PEND_FILE) is also + # what keeps a duplicate entry's label off the PR — this file is not in it. + SKIP_FILE+=("$FILE") continue fi @@ -607,9 +914,14 @@ 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). - # 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`). + # Exact-match membership, not a `|${L}|` substring sentinel: a substring test + # drops a distinct label whose name merely CONTAINS an already-seen one + # (`bug` vs `bugfix`). The original reasoning here cited `bug` vs `bug|ui` — a + # pipe-bearing label — which is no longer representable: the roster validation + # rejects `|` in a label outright, because this list is reconstructed from the + # `|`-joined tuples and such a label would have been truncated long before + # reaching here (BE-6471). Exact matching is still the right test; only the + # example changed. local -a LABEL_ARGS=() SEEN_LABELS=() local L S DUP for L in ${LABELS[@]+"${LABELS[@]}"}; do @@ -620,13 +932,29 @@ bump_repo() { (( DUP )) && continue SEEN_LABELS+=("$L"); LABEL_ARGS+=(--label "$L") done + # `${arr[@]+"${arr[@]}"}` like every other possibly-empty array here, NOT a bare + # `"${LABEL_ARGS[@]}"`: under `set -u` on bash < 4.4 — the 3.2 a macOS checkout + # still has, which the case-folding below is deliberately written for — + # expanding an empty array is an unbound-variable error that aborts the shell. + # An entry with no label is the COMMON case, so the bare form would have taken + # out the whole run on the first unlabelled caller. + # + # stdout is dropped as well as stderr. On success `gh pr create` prints the new + # PR's URL, and that URL is built from GitHub's CANONICAL `html_url` — i.e. the + # repo name as GitHub spells it, which need not be the spelling the roster used. + # `::add-mask::` is a case-sensitive literal match on the roster's spelling, so a + # roster entry naming `Comfy-Org/Secret-Alpha` for a repo GitHub calls + # `secret-alpha` would mask one string and print the other into this public + # repo's run log — an irreversible disclosure of a private caller. The masked + # `PR opened` line below carries the same information for an operator, and the + # update-in-place path already discards `gh pr edit`'s output for this reason. if gh pr create \ --repo "${REPO}" \ --head "${BRANCH}" \ --base "${DEFAULT_BRANCH}" \ --title "${PR_TITLE}" \ --body "${PR_BODY}" \ - "${LABEL_ARGS[@]}" 2>/dev/null; then + ${LABEL_ARGS[@]+"${LABEL_ARGS[@]}"} >/dev/null 2>&1; then echo "${REPO}: PR opened" return 0 else @@ -643,28 +971,92 @@ bump_repo() { # 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. +# +# Matched on a CASE-FOLDED key, because GitHub does too: it will not host two +# repos in one owner differing only in case, so `Comfy-Org/Foo` and +# `Comfy-Org/foo` are two spellings of ONE repo. Grouping them byte-exactly would +# run `bump_repo` twice against it, and the second run's force-PATCH of the shared +# bump branch would discard the first's files (see the canonicalization comment at +# the tuple build). The owner half is already canonical by then; this closes the +# name half. `tr` rather than `${VAR,,}` so the script keeps running under the +# bash 3.2 a macOS checkout still has. REPOS=() SEEN_REPOS="" for ENTRY in "${CALLERS[@]}"; do REPO="${ENTRY%%|*}" + REPO_KEY=$(tr '[:upper:]' '[:lower:]' <<<"$REPO") case "$SEEN_REPOS" in - *"|${REPO}|"*) ;; - *) REPOS+=("$REPO"); SEEN_REPOS="${SEEN_REPOS}|${REPO}|" ;; + *"|${REPO_KEY}|"*) ;; + *) REPOS+=("$REPO"); SEEN_REPOS="${SEEN_REPOS}|${REPO_KEY}|" ;; esac done FAILED=() +# Caller files listed in the roster that this fleet can never bump — absent on the +# caller's default branch, or present but carrying no pin this fleet can move +# (BE-6471). Appended to from inside bump_repo — a plain global on purpose, since +# the loop below runs the function in THIS shell (no subshell, no pipe), so the +# appends survive. Declared here, before the first call, so `set -u` sees it. +NOPIN=() for REPO in "${REPOS[@]}"; do # Collect this repo's entries (in their original order) and bump them together. + # Case-folded on both sides, matching the grouping above — otherwise a + # `Foo`/`foo` pair would collapse into ONE `REPOS` entry (so the repo is bumped + # once, correctly) while the second spelling's files failed to match here and + # were silently dropped from the bump: the partial bump again, by the other door. + REPO_KEY=$(tr '[:upper:]' '[:lower:]' <<<"$REPO") ENTRIES=() for ENTRY in "${CALLERS[@]}"; do - [[ "${ENTRY%%|*}" == "$REPO" ]] && ENTRIES+=("$ENTRY") + [[ "$(tr '[:upper:]' '[:lower:]' <<<"${ENTRY%%|*}")" == "$REPO_KEY" ]] && ENTRIES+=("$ENTRY") done bump_repo "$REPO" "${ENTRIES[@]}" || FAILED+=("$REPO") done if (( ${#FAILED[@]} )); then printf '::error::bump failed for %d repo(s): %s\n' "${#FAILED[@]}" "${FAILED[*]}" +fi +# An un-bumpable caller file fails the job, for the same reason an empty roster +# does (see the ALLOW_EMPTY comment above): a silent no-op dispatcher leaves +# callers un-bumped with nobody noticing, and a per-entry silent no-op is that +# same failure one level down. Failing HERE rather than at the offending file +# preserves the fan-out — one bad entry never blocks another repo's bump, which is +# how FAILED already behaves. Only the COUNT is printed: the per-file warnings +# above carry the (masked) repo and the path, and the roster is private. +# +# Counted in caller FILES, not roster entries: the SKIP_FILE dedup records one +# element per distinct broken path per repo, so two entries naming the same +# missing path are one element here. Reporting that as a count of entries would +# tell an operator who removed one of the two to expect the number to fall, and +# it would not. +# +# Repos in FAILED are excluded. A `return 1` from bump_repo (transient fetch +# error, blob/tree/commit failure, the post-rewrite assertion) abandons that +# repo's bump WHOLESALE, but any NOPIN element already appended for an earlier +# file of the same repo survives it — so the repo would be tallied in both lists +# and the aggregate would tell the operator to "fix or remove" roster entries for +# a bump that was dropped for an unrelated, possibly transient reason. The +# per-file warning above still stands on its own; only the actionable tally is +# filtered. `|` is the tuple delimiter and cannot occur in a repo name, so it is +# a safe sentinel here as it is in the grouping above. +NOPIN_ACTIONABLE=0 +if (( ${#NOPIN[@]} )); then + FAILED_KEYS="" + for REPO in ${FAILED[@]+"${FAILED[@]}"}; do + FAILED_KEYS="${FAILED_KEYS}|$(tr '[:upper:]' '[:lower:]' <<<"$REPO")|" + done + for NP in "${NOPIN[@]}"; do + NP_KEY=$(tr '[:upper:]' '[:lower:]' <<<"${NP%%|*}") + case "$FAILED_KEYS" in + *"|${NP_KEY}|"*) ;; + *) NOPIN_ACTIONABLE=$((NOPIN_ACTIONABLE+1)) ;; + esac + done +fi +if (( NOPIN_ACTIONABLE )); then + printf '::error::%d caller file(s) cannot be bumped by this fleet (missing file, or no movable %s pin) — see the warnings above; fix or remove those %s entries\n' \ + "${NOPIN_ACTIONABLE}" "${WORKFLOW_FILE}" "${VAR_NAME}" +fi +if (( ${#FAILED[@]} || ${#NOPIN[@]} )); 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..38cb319 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -153,7 +153,15 @@ if [[ "$path" == *"/contents/"* ]]; then 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') + # STUB_ALT_REPO/STUB_ALT_CONTENT_FILE: serve a DIFFERENT fixture for one named + # repo, so a single run can carry a healthy caller and a broken one at once — + # which is what proves the fan-out continues past a bad entry instead of + # stranding the rest of the fleet. + src="$STUB_CONTENT_FILE" + if [[ -n "${STUB_ALT_REPO:-}" && "$path" == "repos/${STUB_ALT_REPO}/"* ]]; then + src="$STUB_ALT_CONTENT_FILE" + fi + b64=$(base64 < "$src" | tr -d '\n') printf '{"sha":"blobsha123","content":"%s"}' "$b64" elif [[ "$path" == *"/git/commits/"* ]]; then # Resolve the tip commit's TREE sha (distinct from the commit sha) — the script @@ -454,18 +462,187 @@ 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. +echo "== a roster entry naming a repo outside Comfy-Org is rejected before any API call (BE-6471) ==" +# `repo` is interpolated into every `gh api repos/${REPO}/…` write and into +# `gh pr create --repo`, under an org-wide app token with contents + +# pull-requests + issues write. The roster is an Actions VARIABLE — editable +# outside code review — so an unvalidated `repo` turns roster-edit access into +# bot-authored commits, branch force-moves and labelled PRs in any org repo the +# app is installed on. The rejection must land BEFORE the fan-out (nothing is +# written) and must name the INDEX and the RULE, never the value: masking has not +# been applied at that point and this repo's run logs are public. +new_case badrepo +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-alpha","file":".github/workflows/ci.yml","label":""},{"repo":"Evil-Org/somerepo","file":".github/workflows/ci.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "error names the offending index" "grep -q '::error::CURSOR_REVIEW_CALLERS entry at index 1 is invalid' <<<\"\$OUT\"" +check "error names the repo rule" "grep -q 'repo must match \\^Comfy-Org/' <<<\"\$OUT\"" +check "the value itself was NEVER echoed" "! grep -q 'Evil-Org' <<<\"\$OUT\"" +check "wrote nothing anywhere" "[[ -z \"\$(ls -A \"\$STUB_PUT_DIR\")\" ]]" +check "bailed BEFORE the parse/mask loop" "! grep -q '::add-mask::' <<<\"\$OUT\"" + +# `.` is legal in a repo name (`Comfy-Org/.github` is a real shape), so the class +# keeps it — but a name that is ONLY dots is a path segment, not a repository, and +# every use of `repo` is a URL built by string interpolation. +new_case dotrepo +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/..","file":".github/workflows/ci.yml","label":""}]' +check "exit 1 on a dot-segment repo name" "[[ $RC -eq 1 ]]" +check "error names the repo rule" "grep -q 'index 0 is invalid (repo must match' <<<\"\$OUT\"" +check "wrote nothing anywhere" "[[ -z \"\$(ls -A \"\$STUB_PUT_DIR\")\" ]]" +check "bailed BEFORE the parse/mask loop" "! grep -q '::add-mask::' <<<\"\$OUT\"" + +new_case dotgithub +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/.github","file":".github/workflows/ci.yml","label":""}]' +check "a dot-LEADING repo name is still valid" "[[ $RC -eq 0 ]]" +check "and it was actually bumped" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" + +echo "== a roster entry whose file is not a workflow path is rejected (BE-6471) ==" +# `file` becomes the path committed into the caller repo's tree, so an +# unconstrained value lets a roster edit write anywhere in the repo. Both shapes +# are covered: a plainly-unrelated path, and a traversal out of the workflows +# directory — the rule's character class excludes `/`, so `../` cannot appear. +new_case badfile +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-alpha","file":"package.json","label":""}]' +check "exit 1 on a non-workflow path" "[[ $RC -eq 1 ]]" +check "error names index 0 + the file rule" \ + "grep -q '::error::CURSOR_REVIEW_CALLERS entry at index 0 is invalid (file must be a .github/workflows/' <<<\"\$OUT\"" +check "the value itself was NEVER echoed" "! grep -q 'package.json' <<<\"\$OUT\"" +check "wrote nothing anywhere" "[[ -z \"\$(ls -A \"\$STUB_PUT_DIR\")\" ]]" +check "bailed BEFORE the parse/mask loop" "! grep -q '::add-mask::' <<<\"\$OUT\"" + +new_case traversal +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-alpha","file":".github/workflows/../evil.yml","label":""}]' +check "exit 1 on a traversal path" "[[ $RC -eq 1 ]]" +check "error names the file rule" "grep -q 'file must be a .github/workflows/' <<<\"\$OUT\"" +check "the traversal was NEVER echoed" "! grep -q 'evil.yml' <<<\"\$OUT\"" +check "wrote nothing anywhere" "[[ -z \"\$(ls -A \"\$STUB_PUT_DIR\")\" ]]" +check "bailed BEFORE the parse/mask loop" "! grep -q '::add-mask::' <<<\"\$OUT\"" + +echo "== a label containing the tuple delimiter is rejected, not silently truncated (BE-6471) ==" +# The entries are carried as `repo|file|label|wire_bot` tuples and read back with +# `cut -d'|' -f3`, so a label containing `|` truncates at the pipe and its tail +# lands in the `wire_bot` field — silently flagging an entry for identity wiring +# that never asked for it. The label also reaches `gh pr create --label`. Reject +# it at the door rather than shipping the truncation. +new_case badlabel +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-alpha","file":".github/workflows/ci.yml","label":"bug|ui"}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "error names the label rule" "grep -q 'label must be a string containing no |, comma or control character' <<<\"\$OUT\"" +check "the label itself was NEVER echoed" "! grep -q 'bug|ui' <<<\"\$OUT\"" +check "wrote nothing anywhere" "[[ -z \"\$(ls -A \"\$STUB_PUT_DIR\")\" ]]" +check "bailed BEFORE the parse/mask loop" "! grep -q '::add-mask::' <<<\"\$OUT\"" + +echo "== an entry breaking two rules reports BOTH, and a valid roster still passes (BE-6471) ==" +# One jq pass emits every violation, so fixing the first does not merely reveal +# the second on the next run. The second half is the regression guard that +# matters most: the shapes every real roster uses — an absent label, a plain +# label, `wire_bot` — must all still validate. +new_case tworules +STUB_CONTENT_FILE="$CR_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"nope/x","file":"nope.txt","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "reported the repo rule" "grep -q 'index 0 is invalid (repo must match' <<<\"\$OUT\"" +check "reported the file rule too" "grep -q 'index 0 is invalid (file must be' <<<\"\$OUT\"" + +new_case validshapes +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_a.b-c","file":".github/workflows/ci-cursor-review.yaml"},{"repo":"Comfy-Org/secret-b","file":".github/workflows/ci.yml","label":"ci"},{"repo":"Comfy-Org/secret-c","file":".github/workflows/ci.yml","label":null}]' +check "exit 0 — every legitimate shape validates" "[[ $RC -eq 0 ]]" +check "no validation error raised" "! grep -q 'is invalid' <<<\"\$OUT\"" +check "all three callers were bumped" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 3 ]]" + +echo "== a TRAILING NEWLINE cannot smuggle a second, never-validated tuple (BE-6471) ==" +# jq matches with Oniguruma, where `$` also matches immediately BEFORE a trailing +# newline — so an `^…$`-anchored rule accepts "Comfy-Org/legit\n". The tuple +# encoding is read back line-wise, so that one entry splits in two: a fragment +# with no `|` at all (every `cut -d'|' -fN` returns the whole line, so `wire_bot` +# comes back non-empty and the entry is silently flagged for identity wiring) and +# a fragment whose `repo` is EMPTY, which would reach `::add-mask::` and +# `gh api repos/`. `\A`/`\z` are strict whole-string anchors; this is their guard. +new_case trailnl +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-a\n","file":".github/workflows/ci.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "rejected by the repo rule" "grep -q 'index 0 is invalid (repo must match' <<<\"\$OUT\"" +check "bailed BEFORE the parse/mask loop" "! grep -q '::add-mask::' <<<\"\$OUT\"" +check "wrote nothing anywhere" "[[ -z \"\$(ls -A \"\$STUB_PUT_DIR\")\" ]]" + +new_case trailnlfile +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-a","file":".github/workflows/ci.yml\n","label":""}]' +check "a trailing newline in file is rejected too" "[[ $RC -eq 1 ]]" +check "rejected by the file rule" "grep -q 'index 0 is invalid (file must be' <<<\"\$OUT\"" + +echo "== a CR-bearing label is a named rule violation, not an opaque gh failure (BE-6471) ==" +# `\r` survives the line-wise tuple reads (only `\n` splits them) and would reach +# `gh pr create --label` verbatim, where the API rejects it — turning a screenable +# roster value into a per-repo FAILED entry with no hint of the real cause. Both +# `repo` and `file` already exclude CR via their character classes; `label` bars +# it explicitly. +new_case labelcr +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-a","file":".github/workflows/ci.yml","label":"ci\r"}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "error names the label rule" "grep -q 'label must be a string containing no |, comma or control character' <<<\"\$OUT\"" +check "wrote nothing anywhere" "[[ -z \"\$(ls -A \"\$STUB_PUT_DIR\")\" ]]" + +echo "== a lower-cased owner is a WORKING roster entry, not a fleet-wide hard fail (BE-6471) ==" +# GitHub resolves an owner case-insensitively — which is why REPO_RE is spelled +# case-insensitively for `uses:` pins. The roster rule must agree: `comfy-org/x` +# addresses the same repo and bumps fine today, so failing the whole fleet before +# a single repo is touched (with an error that deliberately omits the value) would +# be a self-inflicted outage. The dot-segment rule still has to bite in that +# spelling. +new_case ownercase +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-a","file":".github/workflows/ci.yml","label":""}]' +check "exit 0 — a lower-cased owner validates" "[[ $RC -eq 0 ]]" +check "no validation error raised" "! grep -q 'is invalid' <<<\"\$OUT\"" +check "the caller was bumped" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" + +new_case ownercasedots +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/..","file":".github/workflows/ci.yml","label":""}]' +check "the dot-segment rule still bites in that spelling" "[[ $RC -eq 1 ]]" +check "reported the repo rule" "grep -q 'index 0 is invalid (repo must match' <<<\"\$OUT\"" + +echo "== monorepo: a 404 file does not block its sibling, but DOES fail the run (BE-6471) ==" +# One file 404s, the other bumps. The 404 must not be a REPO failure — the file +# that WAS present is still committed and PR'd, which is the fan-out property. +# But a roster entry naming a path that does not exist can never be bumped (the +# renamed/typo'd caller), so it is tallied like any other un-bumpable entry and +# the JOB fails at the end. It used to be a silent `not found — skipping` on a +# green run: exactly the drift BE-6471 exists to surface. 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\"" +check "exit 1 — a missing caller file is un-bumpable, not a benign skip" "[[ $RC -eq 1 ]]" +check "named the missing file and its variable" \ + "grep -q 'ci-b.yml not found on the default branch — fix or remove its CURSOR_REVIEW_CALLERS entry' <<<\"\$OUT\"" +check "the 404 was NOT a repo failure" "! grep -q 'bump failed for' <<<\"\$OUT\"" +check "committed only the present file" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "still opened the PR for the sibling" "grep -q 'PR opened' <<<\"\$OUT\"" +check "tallied exactly one un-bumpable entry" "grep -q '::error::1 caller file(s) cannot be bumped by this fleet' <<<\"\$OUT\"" +check "did NOT report the fleet complete" "! grep -q 'cursor-review bump complete' <<<\"\$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: @@ -558,6 +735,178 @@ 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\"" +# A genuinely-converged fleet must stay GREEN now that an un-bumpable entry fails +# the job (BE-6471): the un-bumpable tally is about files with no pin to move, not +# files whose pin is already where it should be. +check "raised no un-bumpable warning" "! grep -q 'carries no cursor-review.yml pin' <<<\"\$OUT\"" +check "raised no un-bumpable tally" "! grep -q 'caller file(s) carry no' <<<\"\$OUT\"" +check "reported the fleet complete" "grep -q 'cursor-review bump complete' <<<\"\$OUT\"" + +echo "== a caller pinned to a floating TAG is self-healed, not reported (BE-4662 x BE-6471) ==" +# The un-bumpable check must not demand a 40-hex ref. Rules 1-2 move a ref by +# POSITION, not by shape, so `uses: …/cursor-review.yml@v1` IS bumpable — and a +# floating-tag caller is precisely the caller this fleet exists to drag back onto +# an immutable pin. A "must already be a full SHA" rule would fail those callers +# forever instead of fixing them, so this asserts the healing, not a warning. +new_case floatingtag +FLOAT_FIXTURE="${WORK}/floating_caller.yml" +printf '%s\n' \ + 'name: CI cursor-review' \ + 'jobs:' \ + ' review:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@v1' \ + > "$FLOAT_FIXTURE" +STUB_CONTENT_FILE="$FLOAT_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-floating","file":".github/workflows/ci.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 0" "[[ $RC -eq 0 ]]" +check "the tag was replaced by the new SHA" "grep -qF 'cursor-review.yml@$NEW_SHA' \"$PUT\"" +check "no floating tag survives" "! grep -qF 'cursor-review.yml@v1' \"$PUT\"" +check "not reported as un-bumpable" "! grep -q 'carries no cursor-review.yml pin' <<<\"\$OUT\"" + +echo "== a roster entry pointing at a file with NO pin of ours FAILS the run (BE-6471) ==" +# The silent-drift case the `already at — skipping` line used to hide. A +# file that carries no `Comfy-Org/github-workflows` pin at all rewrites to itself, +# so content-equality reports it as converged and the job exits green — forever, +# for a roster entry that is simply wrong. Two callers, both un-bumpable, so this +# also proves the failure is aggregated at the END: the second is still reached +# rather than the first aborting the fan-out. +new_case nopin +NOPIN_FIXTURE="${WORK}/nopin_caller.yml" +printf '%s\n' \ + 'name: CI something else' \ + 'jobs:' \ + ' build:' \ + ' runs-on: ubuntu-latest' \ + ' steps:' \ + ' - uses: actions/checkout@bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb # v4' \ + ' # uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@1111111111111111111111111111111111111111' \ + > "$NOPIN_FIXTURE" +STUB_CONTENT_FILE="$NOPIN_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-nopin","file":".github/workflows/ci.yml","label":""},{"repo":"Comfy-Org/secret-nopin-two","file":".github/workflows/ci.yml","label":""}]' +check "exit 1 — a no-op entry must not report success" "[[ $RC -eq 1 ]]" +check "warned, naming the file and the variable" \ + "grep -q '::warning::.*carries no cursor-review.yml pin this bumper can move.*CURSOR_REVIEW_CALLERS entry' <<<\"\$OUT\"" +check "fan-out continued — BOTH entries reached" \ + "[[ \$(grep -c 'carries no cursor-review.yml pin' <<<\"\$OUT\") -eq 2 ]]" +check "aggregate error tallies both files" \ + "grep -q '::error::2 caller file(s) cannot be bumped by this fleet' <<<\"\$OUT\"" +check "did NOT claim the file was already current" "! grep -q 'already at $SHORT' <<<\"\$OUT\"" +check "did NOT report the fleet complete" "! grep -q 'cursor-review bump complete' <<<\"\$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 "== a file calling ONLY a sibling fleet's reusable is the roster's bug (BE-6471) ==" +# The entry names a real caller — of a DIFFERENT fleet. This fleet's rewrite is +# address-restricted to its own reusable, so there is nothing here for it to move +# and the file would otherwise be reported as converged on every run. The stale +# roster entry, not the file, is what needs fixing. +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' \ + > "$WRONGFLEET_FIXTURE" +STUB_CONTENT_FILE="$WRONGFLEET_FIXTURE" run_bump \ + VAR_NAME=GROOM_CALLERS TAG=groom WORKFLOW_FILE=groom.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-wrongfleet","file":".github/workflows/ci-groom.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "warned about the missing groom pin" "grep -q 'carries no groom.yml pin this bumper can move' <<<\"\$OUT\"" +check "pointed at the GROOM_CALLERS entry" "grep -q 'GROOM_CALLERS entry' <<<\"\$OUT\"" +check "did NOT bump the sibling fleet's pin" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]" +check "did NOT claim it was already current" "! grep -q 'already at $SHORT' <<<\"\$OUT\"" + +echo "== a sibling fleet's DOUBLE-pinning caller is not rescued by its workflows_ref (BE-6471) ==" +# The sharp edge of the case above. Groom and pr-risk callers pin this repo TWICE +# — `uses:` AND the `workflows_ref` input — and rule 2 (workflows_ref) is +# deliberately UNADDRESSED, because the input carries no workflow name. So if a +# bare `workflows_ref:` were allowed to vouch for the file, a groom caller +# misfiled into another fleet's roster would sail through this gate, rule 1 would +# correctly decline its `uses:` pin, and rule 2 would still stamp THIS fleet's SHA +# onto the groom caller's assets ref — a split-pin, cross-fleet PR under a green +# run. When the file calls a sibling reusable, an addressable `uses:` pin of OUR +# reusable is required. +new_case siblingref +SIBREF_FIXTURE="${WORK}/sibling_double_pin.yml" +printf '%s\n' \ + 'name: CI groom' \ + 'jobs:' \ + ' groom:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/groom.yml@1111111111111111111111111111111111111111 # v1' \ + ' with:' \ + ' workflows_ref: 1111111111111111111111111111111111111111' \ + > "$SIBREF_FIXTURE" +STUB_CONTENT_FILE="$SIBREF_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-sibref","file":".github/workflows/ci-groom.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "flagged as carrying no cursor-review pin" "grep -q 'carries no cursor-review.yml pin this bumper can move' <<<\"\$OUT\"" +check "committed NOTHING" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]" +check "did NOT stamp our SHA on its workflows_ref" \ + "[[ ! -f \"\$STUB_PUT_DIR/put.last.txt\" ]] || ! grep -qF 'workflows_ref: $NEW_SHA' \"\$STUB_PUT_DIR/put.last.txt\"" +check "opened no PR" "[[ ! -f \"\$STUB_PUT_DIR/pr.log\" ]] || ! grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" + +echo "== an EXPRESSION-pinned uses: is a pin the assertion owns, not a missing one (BE-6471) ==" +# The gate reads `uses:` with the ASSERTION's broad reader, not the rewrite's +# narrower REF_RE. A ref fed by `${{ … }}` is deliberately never rewritten, so +# under REF_RE this file would look like it carried no pin at all: the gate would +# skip it and the repo's OTHER files would still ship a PR, where the post-rewrite +# assertion used to fail the whole repo. Broad here, narrow there — the pin is +# admitted, and the assertion fails the repo as it always did. +new_case exprpin +EXPR_FIXTURE="${WORK}/expr_pin_caller.yml" +# shellcheck disable=SC2016 # the `${{ … }}` is Actions-expression YAML, not shell +printf '%s\n' \ + 'name: CI cursor-review' \ + 'jobs:' \ + ' review:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@${{ inputs.ref }}' \ + > "$EXPR_FIXTURE" +STUB_CONTENT_FILE="$EXPR_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-expr","file":".github/workflows/ci.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "NOT misreported as carrying no pin" "! grep -q 'carries no cursor-review.yml pin' <<<\"\$OUT\"" +check "failed the whole repo, as the assertion does" "grep -q 'bump failed for 1 repo' <<<\"\$OUT\"" +check "committed nothing" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]" + +echo "== a doubly-listed broken path is warned and tallied ONCE (BE-6471) ==" +# The PEND_FILE dedup only sees files that were STAGED, so a path rejected before +# staging (absent, or no movable pin) escaped it: the file was re-fetched, warned +# about twice, and counted twice — reporting more broken files than there are +# distinct broken roster entries. The reject paths record into SKIP_FILE, which +# the same dedup now consults. +new_case dupnopin +STUB_CONTENT_FILE="$NOPIN_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-dupnopin","file":".github/workflows/ci.yml","label":""},{"repo":"Comfy-Org/secret-dupnopin","file":".github/workflows/ci.yml","label":"ci"}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "warned exactly once" "[[ \$(grep -c 'carries no cursor-review.yml pin' <<<\"\$OUT\") -eq 1 ]]" +check "tallied exactly one entry, not two" "grep -q '::error::1 caller file(s) cannot be bumped by this fleet' <<<\"\$OUT\"" + +echo "== one un-bumpable entry does not block another repo's bump (BE-6471) ==" +# The fan-out property, and the reason the tally fires at the END rather than at +# the offending file: a single bad roster entry must not strand every other +# caller. The healthy repo is still committed and PR'd; the job still refuses to +# report success. (STUB_ALT_REPO serves the no-pin fixture for just one of the two +# repos, so a single run carries both a healthy and an un-bumpable caller.) +new_case mixednopin +STUB_CONTENT_FILE="$CR_FIXTURE" \ +STUB_ALT_REPO="Comfy-Org/secret-bad" STUB_ALT_CONTENT_FILE="$NOPIN_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-bad","file":".github/workflows/ci.yml","label":""},{"repo":"Comfy-Org/secret-good","file":".github/workflows/ci.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 1 — the run still refuses to pass" "[[ $RC -eq 1 ]]" +check "the un-bumpable caller was named" "grep -q 'carries no cursor-review.yml pin' <<<\"\$OUT\"" +check "exactly ONE caller was reported so" "[[ \$(grep -c 'carries no cursor-review.yml pin' <<<\"\$OUT\") -eq 1 ]]" +check "the HEALTHY caller was still committed" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "the healthy caller's pin moved" "grep -qF 'cursor-review.yml@$NEW_SHA' \"$PUT\"" +check "the healthy caller still got its PR" "grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" +check "the tally counts exactly one file" "grep -q '::error::1 caller file(s) cannot be bumped by this fleet' <<<\"\$OUT\"" 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:` @@ -936,6 +1285,278 @@ check "no spurious attribution warning" "! grep -q 'pin comments untouc check "the marker WAS refreshed" "grep -qF 'github-workflows main ($SHORT)' \"$PUT\"" check "no stale short SHA left behind" "! grep -qF 'main (1111111)' \"$PUT\"" +echo "== two CASE-VARIANT spellings of one repo are ONE bump, not two (BE-6471) ==" +# The roster rule accepts the owner case-insensitively, because GitHub resolves it +# that way and `comfy-org/x` is a working entry. Accepting two spellings without +# folding them is a partial bump: the repo grouping was byte-exact, so each +# spelling became its own REPOS entry and bump_repo ran TWICE against the same +# repo — each run building a commit off MAIN_SHA carrying only ITS files and +# force-PATCHing the SHARED `ci/bump-` ref, so the second discarded the +# first's file and then merely edited the open PR. Green run, half a bump +# (BE-3896). The owner is canonicalized on the way in and the grouping folds case, +# so both entries land in ONE commit carrying BOTH files. +new_case ownercasedup +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-case","file":".github/workflows/ci-a.yml","label":""},{"repo":"comfy-org/secret-case","file":".github/workflows/ci-b.yml","label":""}]' +check "exit 0" "[[ $RC -eq 0 ]]" +check "ONE commit carrying BOTH files" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 2 ]]" +check "both paths are on the one branch" "[[ \$(sort -u \"\$STUB_PUT_DIR/branch_files\" | wc -l) -eq 2 ]]" +check "opened exactly one PR" "[[ \$(grep -c '^pr-create' \"\$STUB_PUT_DIR/pr.log\") -eq 1 ]]" +check "never edited a PR it had just created" "! grep -q '^pr-edit' \"\$STUB_PUT_DIR/pr.log\"" +# Both spellings are masked under the ONE canonical form, so neither can reach the +# public log — the canonicalization must not create an unmasked spelling. +check "the canonical spelling was masked" "grep -q '::add-mask::Comfy-Org/secret-case' <<<\"\$OUT\"" +check "the raw lower-cased spelling never printed" "! grep -q 'comfy-org/secret-case' <<<\"\$OUT\"" + +echo "== a comma-bearing label is rejected — cobra CSV-splits --label (BE-6471) ==" +# `--label` is a StringSlice: `ci,do-not-merge` applies TWO labels, the second +# potentially blocking, from an entry that appears to name one. The denylist +# covers it alongside `|` and the control characters, while the characters GitHub +# labels legitimately use (space, `:`, `/`) still validate. +new_case labelcomma +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-a","file":".github/workflows/ci.yml","label":"ci,do-not-merge"}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "error names the label rule" "grep -q 'label must be a string containing no |, comma or control character' <<<\"\$OUT\"" +check "never echoed the offending label" "! grep -q 'do-not-merge' <<<\"\$OUT\"" +check "wrote nothing anywhere" "[[ -z \"\$(ls -A \"\$STUB_PUT_DIR\")\" ]]" + +new_case labelok +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-a","file":".github/workflows/ci.yml","label":"needs review: ui/ux"}]' +check "a label with a space, colon and slash still validates" "[[ $RC -eq 0 ]]" +check "no validation error raised" "! grep -q 'is invalid' <<<\"\$OUT\"" + +echo "== a bare workflows_ref never vouches for a file, sibling flag or not (BE-6471) ==" +# The gate used to require an addressable `uses:` pin only when GW_HAS_SIBLING was +# set — but that flag comes from a CASE-SENSITIVE grep, so a caller spelling this +# repo `GitHub-Workflows` (or one whose `uses:` of it was deleted, leaving the +# input behind) left the flag at 0 and the relaxed address applied. Rule 2 is +# unaddressed and `^`-anchored, so it would stamp THIS fleet's SHA onto that +# caller's assets ref while the address-filtered assertion stayed silent: a green, +# mutating, cross-fleet PR. The `uses:` requirement is unconditional now. +new_case wrefonly +WREF_FIXTURE="${WORK}/wref_only_caller.yml" +printf '%s\n' \ + 'name: CI groom' \ + 'jobs:' \ + ' groom:' \ + ' uses: comfy-org/GitHub-Workflows/.github/workflows/groom.yml@1111111111111111111111111111111111111111' \ + ' with:' \ + ' workflows_ref: 1111111111111111111111111111111111111111' \ + > "$WREF_FIXTURE" +STUB_CONTENT_FILE="$WREF_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-wref","file":".github/workflows/ci-groom.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "flagged as carrying no pin we can move" "grep -q 'carries no cursor-review.yml pin this bumper can move' <<<\"\$OUT\"" +check "did NOT stamp our SHA on its workflows_ref" \ + "[[ ! -f \"\$STUB_PUT_DIR/put.last.txt\" ]] || ! grep -qF 'workflows_ref: $NEW_SHA' \"\$STUB_PUT_DIR/put.last.txt\"" +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 sibling whose filename merely ENDS with ours is not ours (BE-6471) ==" +# SHA_ADDR's sibling form had no left delimiter before the target filename, so for +# target `groom.yml` a caller pinning `legacy-groom.yml` satisfied the address: +# rules 1/4-6 repinned that SIBLING reusable to this fleet's SHA and the +# address-filtered assertion saw nothing un-moved. Requiring the `/` that starts +# the filename scopes the address to the intended reusable, and the file then +# correctly reads as un-bumpable BY THIS FLEET. +new_case siblingsuffix +LEGACY_FIXTURE="${WORK}/legacy_sibling_caller.yml" +printf '%s\n' \ + 'name: CI legacy groom' \ + 'jobs:' \ + ' groom:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/legacy-groom.yml@1111111111111111111111111111111111111111' \ + > "$LEGACY_FIXTURE" +STUB_CONTENT_FILE="$LEGACY_FIXTURE" run_bump \ + VAR_NAME=GROOM_CALLERS TAG=groom WORKFLOW_FILE=groom.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-legacy","file":".github/workflows/ci-groom.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "reported as carrying no groom.yml pin" "grep -q 'carries no groom.yml pin this bumper can move' <<<\"\$OUT\"" +check "did NOT repin the sibling reusable" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]" +check "the sibling's SHA was never moved" \ + "[[ ! -f \"\$STUB_PUT_DIR/put.last.txt\" ]] || ! grep -qF 'legacy-groom.yml@$NEW_SHA' \"\$STUB_PUT_DIR/put.last.txt\"" + +echo "== a skipped entry's label never reaches the healthy file's PR (BE-6471) ==" +# The SKIP_FILE dedup must not fold a REJECTED path's label. The label-collection +# site's invariant is that a skipped entry's label must stay off the repo's real +# bump PR — a monorepo listing a 404'd path twice, the second entry carrying +# `do-not-merge`, would otherwise apply that blocking label to the PR built from +# its OTHER, healthy file. (ci-b.yml 404s twice; ci-a.yml is healthy and PR'd.) +new_case dupskiplabel +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-lbl","file":".github/workflows/ci-a.yml","label":"ci"},{"repo":"Comfy-Org/secret-lbl","file":".github/workflows/ci-b.yml","label":""},{"repo":"Comfy-Org/secret-lbl","file":".github/workflows/ci-b.yml","label":"do-not-merge"}]' +check "exit 1 — the 404'd entry still fails the run" "[[ $RC -eq 1 ]]" +check "the healthy file was still committed" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "the healthy file's PR was opened" "grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" +check "the skipped entry's label did NOT land" "! grep -qe '--label do-not-merge' \"\$STUB_PUT_DIR/pr.log\"" +check "the healthy entry's own label DID land" "grep -qe '--label ci' \"\$STUB_PUT_DIR/pr.log\"" +check "the doubly-listed 404 was tallied once" "grep -q '::error::1 caller file(s) cannot be bumped by this fleet' <<<\"\$OUT\"" + +echo "== a repo abandoned wholesale is not ALSO billed as un-bumpable (BE-6471) ==" +# NOPIN elements recorded for earlier files survive a later `return 1` from the +# same bump_repo call (transient fetch error, blob/tree/commit failure, the +# post-rewrite assertion), so the repo appeared in BOTH tallies and the aggregate +# told the operator to fix roster entries for a bump that was dropped for an +# unrelated, possibly transient reason. The per-file warning still stands; only +# the actionable count excludes repos that failed outright. +# (ci-b.yml 404s → NOPIN; ci-a.yml hits the transient 500 → the repo FAILS.) +new_case nopinandfailed +STUB_CONTENT_FILE="$CR_FIXTURE" STUB_404_FILE="ci-b.yml" 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-both","file":".github/workflows/ci-b.yml","label":""},{"repo":"Comfy-Org/secret-both","file":".github/workflows/ci-a.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "the repo is reported as FAILED" "grep -q 'bump failed for 1 repo' <<<\"\$OUT\"" +check "NOT also billed in the un-bumpable tally" "! grep -q 'caller file(s) cannot be bumped by this fleet' <<<\"\$OUT\"" +check "the per-file warning still stands" "grep -q 'ci-b.yml not found on the default branch' <<<\"\$OUT\"" + +echo "== a private default-branch name never reaches the public run log (BE-6471) ==" +# DEFAULT_BRANCH is read from the caller's repo metadata and is never masked (only +# repo names are), so interpolating it into a per-file warning would print a branch +# named after an internal project verbatim into this public repo's logs. The +# messages name "the default branch" instead. (The stub serves `main`, so this +# asserts the SHAPE of the message rather than a secret value.) +new_case branchname +STUB_CONTENT_FILE="$CR_FIXTURE" STUB_404_FILE="ci.yml" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-branch","file":".github/workflows/ci.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "the warning does not interpolate the branch" "! grep -q 'not found on main' <<<\"\$OUT\"" +check "it names the default branch generically" "grep -q 'not found on the default branch' <<<\"\$OUT\"" + +echo "== a repo name that is ONLY dots is rejected in the general case, not just '.'/'..' (BE-6471) ==" +# The rule reads `\A[.]+\z`, so `...` is caught by the NAMED pre-flight rule like +# `..` is. Enumerating the two literals instead let a longer dot run through to a +# 404 at the metadata fetch, surfacing as a generic whole-repo FAILED with nothing +# pointing at the roster — and it made the comment and README ("a name that is +# *only* dots") false as written. +new_case dotsmany +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/...","file":".github/workflows/ci.yml","label":""}]' +check "exit 1 on a longer dot run" "[[ $RC -eq 1 ]]" +check "reported the named repo rule" "grep -q 'index 0 is invalid (repo must match' <<<\"\$OUT\"" +check "wrote nothing anywhere" "[[ -z \"\$(ls -A \"\$STUB_PUT_DIR\")\" ]]" +check "bailed BEFORE the parse/mask loop" "! grep -q '::add-mask::' <<<\"\$OUT\"" + +new_case dotname +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/a.b.c","file":".github/workflows/ci.yml","label":""}]' +check "a name merely CONTAINING dots is still valid" "[[ $RC -eq 0 ]]" +check "and it was actually bumped" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" + +echo "== a FLOW-STYLE uses: keeps its mapping delimiter — loud failure, never corrupted YAML ==" +# REF_RE stops at whitespace, so in `{uses: …@v1, secrets: inherit}` the token +# after `@` used to be `v1,` — comma included — and the rewrite replaced the +# delimiter along with the ref, committing `@ secrets: inherit}`. The +# post-rewrite assertion only asks whether NEW_SHA is present, so the malformed +# YAML shipped. Excluding `,` from REF_RE leaves the comma in place, so the +# assertion reads back `,`, finds it unequal to NEW_SHA and fails the repo — +# the same loud outcome flow style already gets on the `workflows_ref:` side. +new_case flowstyle +FLOW_FIXTURE="${WORK}/flow_caller.yml" +printf '%s\n' \ + 'name: CI cursor-review' \ + 'jobs:' \ + ' review: {uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@v1, secrets: inherit}' \ + > "$FLOW_FIXTURE" +STUB_CONTENT_FILE="$FLOW_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-flow","file":".github/workflows/ci.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "the assertion named the un-moved value" "grep -q 'still pins github-workflows at' <<<\"\$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 "== a github-workflows@ref token that is not a uses: pin does not make a file a caller (BE-6471) ==" +# The gate's reader is anchored to the `uses:` KEY, not just to the pin token. +# Unanchored, ANY `Comfy-Org/github-workflows@` on a SHA_ADDR-eligible line +# satisfied it — a `run:` curl of this repo, an `env:` scalar, a repo@ref handed +# to some other action — none of which call us. Rule 1 keys on the same unscoped +# token, so such a file cleared the gate and was then rewritten and PR'd as a bump +# of something that never was a caller. It is an un-bumpable roster entry instead. +new_case notauses +NOTUSES_FIXTURE="${WORK}/notauses_caller.yml" +printf '%s\n' \ + 'name: CI' \ + 'jobs:' \ + ' build:' \ + ' runs-on: ubuntu-latest' \ + ' steps:' \ + ' - run: curl -sSL https://example.invalid/Comfy-Org/github-workflows@v1/thing.sh' \ + > "$NOTUSES_FIXTURE" +STUB_CONTENT_FILE="$NOTUSES_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-notuses","file":".github/workflows/ci.yml","label":""}]' +check "exit 1" "[[ $RC -eq 1 ]]" +check "reported as carrying no movable pin" "grep -q 'carries no cursor-review.yml pin this bumper can move' <<<\"\$OUT\"" +check "did NOT rewrite the run: token" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]" +check "opened no PR" "[[ ! -f \"\$STUB_PUT_DIR/pr.log\" ]] || ! grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" + +new_case quoteduses +QUOTED_FIXTURE="${WORK}/quoted_caller.yml" +printf '%s\n' \ + 'name: CI cursor-review' \ + 'jobs:' \ + ' review:' \ + ' uses: "Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@v1"' \ + > "$QUOTED_FIXTURE" +STUB_CONTENT_FILE="$QUOTED_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-quoted","file":".github/workflows/ci.yml","label":""}]' +check "a QUOTED uses: still clears the anchored gate" "[[ $RC -eq 0 ]]" +check "and its pin was bumped" "grep -qF \"cursor-review.yml@$NEW_SHA\" \"\$STUB_PUT_DIR/put.last.txt\"" + +echo "== wire_bot is unioned per FILE, so the roster's ORDER cannot decide the wiring (BE-6471) ==" +# Only the FIRST entry naming a path is ever staged, so reading `wire_bot` off that +# entry made the outcome order-dependent: `[{ci.yml, wire_bot: true}, {ci.yml}]` +# wired the identity while the reverse order silently did not — a duplicate entry +# quietly dropping the wiring is the wrong half of a bump. Labels are already +# unioned across duplicates; this is the same rule for the other per-entry field. +new_case wiredupflip +STUB_CONTENT_FILE="$WIRE_FIXTURE" WIRE_BOT_SCRIPT="$WIRE_SCRIPT" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-wdup","file":".github/workflows/ci.yml","label":""},{"repo":"Comfy-Org/secret-wdup","file":".github/workflows/ci.yml","label":"","wire_bot":true}]' +check "exit 0" "[[ $RC -eq 0 ]]" +check "staged the file exactly once" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "the LATER entry's wire_bot still applied" \ + "grep -q 'bot_app_id: \${{ vars.APP_ID }}' \"\$STUB_PUT_DIR/put.last.txt\"" + +new_case wiredupfirst +STUB_CONTENT_FILE="$WIRE_FIXTURE" WIRE_BOT_SCRIPT="$WIRE_SCRIPT" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-wdup","file":".github/workflows/ci.yml","label":"","wire_bot":true},{"repo":"Comfy-Org/secret-wdup","file":".github/workflows/ci.yml","label":""}]' +check "the reverse order gives the SAME result" "[[ $RC -eq 0 ]]" +check "still wired" "grep -q 'bot_app_id: \${{ vars.APP_ID }}' \"\$STUB_PUT_DIR/put.last.txt\"" + +echo "== a doubly-listed ALREADY-CURRENT path is fetched and skip-logged ONCE (BE-6471) ==" +# The third early-`continue` (the content-equality no-op) did not record its file, +# so a repo listing the same converged path twice re-fetched it and printed the +# skip line twice. SKIP_FILE now covers all three skip paths, giving the same +# fetched-once/reported-once property the other two already had. +new_case dupconverged +CONVERGED_FIXTURE="${WORK}/converged_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})" \ + > "$CONVERGED_FIXTURE" +STUB_CONTENT_FILE="$CONVERGED_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-conv","file":".github/workflows/ci.yml","label":""},{"repo":"Comfy-Org/secret-conv","file":".github/workflows/ci.yml","label":""}]' +check "exit 0 — a converged caller is still a clean skip" "[[ $RC -eq 0 ]]" +check "the skip line was printed exactly once" \ + "[[ \$(grep -c 'already at $SHORT' <<<\"\$OUT\") -eq 1 ]]" +check "committed nothing" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]" + echo echo "== $PASS passed, $FAIL failed ==" [[ $FAIL -eq 0 ]]