From 62fa54cf9c4546690f3dedc852d159227c56a6bf Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 26 Jul 2026 00:23:42 -0700 Subject: [PATCH 1/5] feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comfy-Org/cloud#5572 added a `max_prs` workflow_dispatch input to ONE caller, and it cost ~40 lines of expression gymnastics to do it: max_prs: ${{ fromJSON(contains(fromJSON('["1","2","3","5"]'), github.event.inputs.max_prs) && github.event.inputs.max_prs || '1') }} All of that exists because the reusable declared `max_prs` as `type: number`. A workflow_dispatch input is always a string, GitHub rejects a string expression assigned to a `type: number` reusable input at startup, so the caller must cast with `fromJSON()` — and `fromJSON()` on a non-numeric string throws during expression evaluation, killing the run before any job starts. Hence the mirrored `contains()` allowlist guarding the cast. Every caller that wanted the knob would have carried its own copy of that, which is exactly the drift this repo exists to prevent: the logic belongs here, once. So `max_prs` becomes `type: string` and the reusable does the parse. Verified with actionlint that the asymmetry is real: a string expression into a `type: number` input is rejected, while a plain `max_prs: 1` into a `type: string` input is accepted — so existing callers need no change. The caller collapses to: max_prs: ${{ github.event.inputs.max_prs || '1' }} Parsing lives in build_select, and none of its cases can abort a run: empty (what a SCHEDULE yields, carrying no inputs) takes the default; a number is clamped to >= 0; anything else opens ZERO PRs with a loud warning. That last case is the one judgement call — falling back to the DEFAULT would be actively wrong, since a caller piloting at `max_prs: 1` would silently get 5, and exiting non-zero would throw away a finder+verifier run already paid for while the findings still file as issues on the 0-PR path. No upper bound is enforced: a caller's `options:` list is a UI convention and a typo guard, not a security boundary — anyone who can API-dispatch already has write access and could edit `options:` anyway. Also adds the groom caller fleet, which did not exist. It is the fleet that most needs one: a groom caller pins the reusable TWICE (`uses:` and the `workflows_ref:` that loads the briefs + ledger), and those must move in lock-step or a run executes one version's workflow against another version's briefs. bump-callers.sh already rewrote both; it now also re-points the `# main @ ` pin comment those callers carry, because a comment still naming the old commit after the pin moved is worse than no comment. Anchored to the github-workflows line and bounded to {7,12} hex so an unrelated note and a deliberate full-SHA comment are both left alone. - .github/workflows/bump-groom-callers.yml — thin entrypoint, GROOM_CALLERS, ALLOW_EMPTY=true (the fleet grows as callers land), no WIRE_BOT_SCRIPT. - test_bump_callers.sh — a groom case covering both pins, the comment rewrite, an untouched actions/checkout pin, and an intact max_prs forward expression. Verified: actionlint clean; shellcheck clean; 97/97 bump-callers tests (9 new); 59/59 groom ledger tests; agents-md-integrity passes. Parse exercised over '', ' ', '5.0', '-2', 'abc', '1e3', 'inf', 'nan', '01', ' 2 '. Co-Authored-By: Claude Opus 5 --- .github/bump-callers/README.md | 11 ++ .github/bump-callers/bump-callers.sh | 20 +++- .../bump-callers/tests/test_bump_callers.sh | 38 +++++++ .github/workflows/bump-groom-callers.yml | 102 ++++++++++++++++++ .github/workflows/groom.yml | 84 +++++++++++++-- .github/workflows/test-bump-callers.yml | 13 +-- AGENTS.md | 6 +- README.md | 2 +- 8 files changed, 256 insertions(+), 20 deletions(-) create mode 100644 .github/workflows/bump-groom-callers.yml diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index 1c4e1aa..d3544c3 100644 --- a/.github/bump-callers/README.md +++ b/.github/bump-callers/README.md @@ -27,6 +27,7 @@ forward automatically instead of silently drifting commits behind. | [`bump-agents-md-callers.yml`](../workflows/bump-agents-md-callers.yml) | `agents-md-integrity.yml` or `agents-md-integrity/**` | `AGENTS_MD_CALLERS` | empty `[]` (grows as callers land) | | [`bump-pr-size-callers.yml`](../workflows/bump-pr-size-callers.yml) | `pr-size.yml` or `scripts/check-pr-size/**` | `PR_SIZE_CALLERS` | empty `[]` (grows as callers land) | | [`bump-assign-reviewers-callers.yml`](../workflows/bump-assign-reviewers-callers.yml) | `assign-reviewers.yml` | `ASSIGN_REVIEWERS_CALLERS` | empty `[]` (grows as callers land) | +| [`bump-groom-callers.yml`](../workflows/bump-groom-callers.yml) | `groom.yml` or `groom/**` | `GROOM_CALLERS` | empty `[]` (grows as callers land) | 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 @@ -37,6 +38,16 @@ existing one, swap the path filter + `VAR_NAME`/`TAG`/`WORKFLOW_FILE`/ `ALLOW_EMPTY`), seed its variable, and add a row to this table + the paths in `test-bump-callers.yml`. +The **groom** fleet is the one that most needs this: a groom caller pins the +reusable **twice** — the `uses:` SHA *and* the `workflows_ref:` input that loads +the finder/verifier/builder briefs plus the dedup ledger. Those must stay in +lock-step or a run executes one version's workflow against another version's +briefs. `bump-callers.sh`'s pin rewrite moves both (it matches the `uses:` line +and any bare `workflows_ref:` line), so the fleet cannot drift into that split +state through a hand-bump of only one. It also re-points the `# main @ ` +pin comment those callers carry — a comment still naming the old commit after the +pin moved is worse than no comment. + ## The caller variables This repo is **public** — the workflow files and Actions run logs are both diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index d758184..c3d9493 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -196,9 +196,23 @@ bump_repo() { # line and agents-md-integrity's bare `workflows_ref: ` line — so a # full-SHA pin of ANOTHER action in the same file (`actions/checkout@`, # the org's mandated practice) is never clobbered to github-workflows' SHA. - # The comment rewrite is a no-op for callers that use a different comment - # form (e.g. agents-md-integrity's `# v1`), so it is safe to share. - NEW_CONTENT=$(sed -E "/github-workflows|workflows_ref/ s/[0-9a-f]{40}/${NEW_SHA}/g; s|# github-workflows#[0-9]+|# github-workflows main (${SHORT})|g" <<<"$OLD_CONTENT") + # The comment rewrites are a no-op for callers that use a different comment + # form (e.g. agents-md-integrity's `# v1`), so they are safe to share. + # + # Two comment forms are normalized, because a pin comment that still names the + # OLD commit after the pin moved is worse than no comment — it is a confident + # lie in the one file where the pin is the whole point: + # `# github-workflows#27` -> `# github-workflows main ()` + # `# main @ 29a81ca …` -> `# main @ …` (the groom callers' form) + # The second is anchored to the github-workflows line so an unrelated `# main @ + # ` note elsewhere in the caller is untouched, and bounded to {7,12} hex + # so a deliberate FULL-sha comment keeps its full form (the 40-hex rewrite + # above has already corrected it) instead of being shortened. + NEW_CONTENT=$(sed -E " + /github-workflows|workflows_ref/ s/[0-9a-f]{40}/${NEW_SHA}/g + s|# github-workflows#[0-9]+|# github-workflows main (${SHORT})|g + /github-workflows/ s|# main @ [0-9a-f]{7,12}|# main @ ${SHORT}|g + " <<<"$OLD_CONTENT") # Wire an extra identity/config into this file when its entry is flagged # (BE-1814's cloud-code-bot review identity is the first user). Idempotent — diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index ec6e980..0797be4 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -213,6 +213,44 @@ check "base_tree is NOT the commit sha" "! grep -qF '1234567890abcdef12345 check "opened a new PR (pr create called)" "grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" check "did not edit (no open PR existed)" "! grep -q '^pr-edit' \"\$STUB_PUT_DIR/pr.log\"" +echo "== groom fleet: BOTH pins (uses: + workflows_ref) and the '# main @' comment move together ==" +# A groom caller pins the reusable TWICE — the `uses:` SHA and the `workflows_ref:` +# input that loads the finder/verifier briefs + dedup ledger. They must move in +# lock-step or a run executes one version's workflow against another version's +# briefs. The fixture also carries an `actions/checkout@<40hex>` pin, which must +# NOT be clobbered to github-workflows' SHA. +new_case groom +GROOM_FIXTURE="${WORK}/groom_caller.yml" +printf '%s\n' \ + 'name: Groom' \ + 'jobs:' \ + ' groom:' \ + ' steps:' \ + ' - uses: actions/checkout@abcdefabcdefabcdefabcdefabcdefabcdefabcd # v6' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/groom.yml@1111111111111111111111111111111111111111 # main @ 1111111 — groom.yml not on the v1 tag yet' \ + ' with:' \ + ' workflows_ref: 1111111111111111111111111111111111111111' \ + " max_prs: \${{ github.event.inputs.max_prs || '1' }}" \ + > "$GROOM_FIXTURE" +STUB_CONTENT_FILE="$GROOM_FIXTURE" run_bump \ + VAR_NAME=GROOM_CALLERS TAG=groom WORKFLOW_FILE=groom.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-groomed","file":".github/workflows/groom.yml","label":""}]' +check "exit 0" "[[ $RC -eq 0 ]]" +check "masked the private repo name" "grep -q '::add-mask::Comfy-Org/secret-groomed' <<<\"\$OUT\"" +check "reported groom fleet complete" "grep -q 'groom bump complete' <<<\"\$OUT\"" +PUT="${STUB_PUT_DIR}/put.last.txt" +check "uses: pin moved" "grep -qE \"groom.yml@${NEW_SHA}\" \"$PUT\"" +check "workflows_ref pin moved" "grep -qE \"workflows_ref: ${NEW_SHA}\" \"$PUT\"" +check "no stale 40-hex pin anywhere" "! grep -qF '1111111111111111111111111111111111111111' \"$PUT\"" +check "'# main @' comment moved to new short" "grep -qF '# main @ $SHORT' \"$PUT\"" +# The third-party action pin is a full 40-hex SHA on a line that does NOT mention +# github-workflows — the address anchor is what keeps it intact (the org mandates +# SHA-pinning every action, so clobbering it would break the caller's CI). +check "actions/checkout pin untouched" "grep -qF 'actions/checkout@abcdefabcdefabcdefabcdefabcdefabcdefabcd' \"$PUT\"" +# max_prs forwards a workflow_dispatch string (see groom.yml's input docs); the +# bumper must not mangle the expression while rewriting the pins around it. +check "max_prs forward expression intact" "grep -qF \"github.event.inputs.max_prs || '1'\" \"$PUT\"" + echo "== cursor-review fleet: an open bump PR is UPDATED IN PLACE, not re-opened (BE-3882) ==" new_case reuse STUB_CONTENT_FILE="$CR_FIXTURE" run_bump \ diff --git a/.github/workflows/bump-groom-callers.yml b/.github/workflows/bump-groom-callers.yml new file mode 100644 index 0000000..f386d0d --- /dev/null +++ b/.github/workflows/bump-groom-callers.yml @@ -0,0 +1,102 @@ +name: Bump groom callers + +# When groom.yml (or the briefs/ledger it loads) is updated on main, open a +# SHA-bump PR in every repo that pins a groom caller against it. PRs are opened +# by Cloud Code Bot so they are easy to filter and merge. +# +# This is a thin entrypoint over the shared bumper at +# .github/bump-callers/bump-callers.sh — the same script drives the +# cursor-review, agents-md-integrity, pr-size and assign-reviewers fleets. +# Keeping ONE implementation is deliberate: a forked copy is how other shared +# machinery in the org has drifted. The entrypoints differ only in their +# path-filter trigger and the parameters passed below. +# +# WHY groom needs a bumper at all, more than most fleets: a groom caller pins the +# reusable TWICE — the `uses:` SHA and the `workflows_ref:` input that loads the +# finder/verifier/builder briefs + the dedup ledger. Those two must stay in +# lock-step or a run executes one version's workflow against another version's +# briefs. bump-callers.sh rewrites both (its rewrite matches the `uses:` line and +# any bare `workflows_ref:` line), so the fleet cannot drift into that split +# state by hand-bumping only one of them. +# +# The caller list is NOT hardcoded here. This repo is PUBLIC (workflow file and +# Actions run logs are both publicly viewable) and callers may be private, so +# their names must never appear in this file or its logs. The list lives in the +# repo-level Actions variable `GROOM_CALLERS` (config, not a credential — a +# variable, not a secret, since secrets are write-only via the API) as a JSON +# array of {"repo","file","label"} objects. Every repo name is `::add-mask::`ed +# out of the (public) run logs before it is ever echoed. +# +# No WIRE_BOT_SCRIPT here: bot-identity injection is a cursor-review-only concern +# (BE-1814). A groom caller must set `bot_app_id` itself — the reusable requires +# it for `builder: true` — so there is nothing for the bumper to wire. +# +# Update flow — adding/removing a caller needs NO public commit: +# gh variable set GROOM_CALLERS --repo Comfy-Org/github-workflows \ +# --body "$(jq -c . callers.json)" +# 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. (The +# specific home repo is intentionally not named here — this file is public.) + +on: + workflow_dispatch: {} # allow on-demand runs (e.g. to re-bump callers) + push: + branches: [main] + paths: + - .github/workflows/groom.yml + # The briefs + ledger consumers actually run live here, loaded at run time + # from the `workflows_ref` SHA a caller pins (see groom.yml's header) — a + # behavior change here (a finder/verifier/builder brief edit, or ledger.py + # dedup logic) needs the same caller bump as a workflow-file change, or + # pinned callers silently drift onto stale audit behavior. + - .github/groom/** + +permissions: + contents: read + +# Serialize runs of this fleet. The bumper pushes to a STABLE branch +# (ci/bump-groom) shared across runs, so two overlapping runs (a rapid second +# main push, or a push racing a manual re-run) would force-reset that branch and +# race the PR update — an older run finishing last could leave the committed diff +# pinned to a stale SHA. cancel-in-progress: false lets the running bump finish; +# GitHub keeps only the newest pending run, so the latest SHA always wins +# (BE-3882). +concurrency: + group: bump-groom-callers + cancel-in-progress: false + +jobs: + bump: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v6 + with: + persist-credentials: false + + - name: Generate Cloud Code Bot token + uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547 # v1.12.0 + id: token + with: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} + owner: Comfy-Org + + - name: Bump SHA in caller repos + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + NEW_SHA: ${{ github.sha }} + VAR_NAME: GROOM_CALLERS + TAG: groom + WORKFLOW_FILE: groom.yml + # JSON array of {"repo","file","label"} — see the header comment for the + # update flow. Kept in a variable (not the file) so caller names never + # land in this public repo or its logs. + CALLERS_JSON: ${{ vars.GROOM_CALLERS }} + # groom is still rolling out repo by repo, so an empty list is a clean + # no-op rather than a hard failure — same posture as the agents-md / + # pr-size / assign-reviewers fleets. Flip to the default (unset) once + # the fleet is expected to always have callers, so a silently empty + # variable can never leave every caller un-bumped unnoticed. + ALLOW_EMPTY: 'true' + run: bash .github/bump-callers/bump-callers.sh diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index c846955..7e25792 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -45,6 +45,16 @@ name: Groom (reusable) # description: 'Run the audit but do NOT file issues' # type: boolean # default: false +# # OPTIONAL, builder callers only: let an operator raise the builder PR +# # ceiling for ONE manual run without editing the workflow. `choice` (not +# # `string`) so the UI offers a sanctioned list; the reusable clamps and +# # warns on anything else, so a value posted past this list by an API +# # dispatch fails closed at 0 PRs rather than aborting the run. +# max_prs: +# description: 'Builder PR ceiling for THIS dispatch (schedule uses the `with:` value)' +# type: choice +# default: '1' +# options: ['1', '2', '3', '5'] # permissions: # # REQUIRED union — a reusable's nested jobs cannot request more GITHUB_TOKEN # # scope than the caller grants, and GitHub validates this at STARTUP (a short @@ -83,7 +93,14 @@ name: Groom (reusable) # # so the file / build_pr-bail paths can open groom issues. Pilot with # # dry_run:true first to preview patches without opening PRs. # # builder: true -# # max_prs: 5 # throttle: at most N builder PRs per run +# # Throttle: at most N builder PRs per run. A bare number pins every run +# # (`max_prs: 1`). To let the optional dispatch input above override it for +# # one manual run, forward the input with a fallback for the SCHEDULE (which +# # carries no inputs, so the expression yields ''): +# # max_prs: ${{ github.event.inputs.max_prs || '1' }} +# # No fromJSON() and no allowlist re-check are needed — max_prs is a +# # `string` input and the reusable does the parse/clamp (see its +# # description). That is the whole reason it is not `type: number`. # secrets: # ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} # BOT_APP_PRIVATE_KEY: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} @@ -202,9 +219,42 @@ on: the top `max_prs` CONFIRM, non-security findings (ranked by the verifier) become PRs; the rest are filed as issues. Ignored unless `builder` is true. - type: number + + + Typed `string`, not `number`, ON PURPOSE — so a caller can forward its + own `workflow_dispatch` input straight through and let an operator raise + the ceiling for one manual run (see the caller pattern in the header). + A `workflow_dispatch` input is ALWAYS a string, and GitHub rejects a + string expression assigned to a `type: number` reusable input at + startup; the workaround is a `fromJSON()` cast in the caller, and + `fromJSON()` on a non-numeric string throws during expression + evaluation — failing the run before any job starts, with an error far + less obvious than "you typo'd a number". Taking a string here keeps + that cast, and its failure mode, in ONE place (the `build_select` step + below) instead of copy-pasted into every caller. A plain YAML number + (`max_prs: 1`) still assigns cleanly to a string input, so existing + callers need no change. + + + Parsing, all of which is non-fatal: empty/whitespace — what a SCHEDULED + run yields when a caller forwards `github.event.inputs.max_prs`, since a + schedule event carries no inputs — means "no override asked for" and + takes this default; a number is floor-clamped to >= 0; anything else + opens ZERO PRs with a loud warning, so a typo'd dispatch fails CLOSED + and every finding still files as an issue rather than throwing away a + completed audit. + + + No upper bound is enforced here, deliberately. A caller's `options:` + list is a UI convention and a typo guard, NOT a security boundary — the + only principals who can post a value past it (an API/CLI + `workflow_dispatch`) already have repo write access and could just edit + `options:`. The real backstops are structural: builder PRs are review- + gated and NEVER auto-merged, the finder emits only ~6-12 findings a run, + and `pr_size_limit` bails any oversized patch to an issue. + type: string required: false - default: 5 + default: '5' pr_size_limit: description: >- Patch-size bail-out. If a finding's generated patch changes more than @@ -1003,13 +1053,31 @@ jobs: print(f"::warning::{len(invalid)} verified finding(s) had no usable signature and were NOT filed (producer error).") builder = os.environ.get("BUILDER", "false").lower() == "true" - # `type: number` can arrive as "5" or "5.0"; parse leniently, clamp >=0. - raw = os.environ.get("MAX_PRS", "5").strip() + # max_prs is a `string` input so a caller can forward a raw + # workflow_dispatch value without a fromJSON() cast (see the input's + # description). This is the ONE place it is parsed. Three cases, none of + # which may abort the run: + # empty -> a SCHEDULED run of a caller that forwards + # `github.event.inputs.max_prs`; a schedule event carries no + # inputs, so no override was asked for -> take the default. + # numeric -> accept "5" and "5.0" alike, floor-clamped to >= 0. + # garbage -> `type: choice` constrains only the WEB UI; an API/CLI + # workflow_dispatch can post any string. Fail CLOSED at 0 + # PRs with a loud warning instead of exiting non-zero: the + # finder and verifier have already run and been paid for, + # and every finding still files as an issue on the 0-PR + # path. Falling back to MAX_PRS_DEFAULT here would be + # actively wrong — a caller piloting at `max_prs: 1` would + # silently get 5. + # OverflowError is caught alongside ValueError because float() accepts + # "inf"/"-inf" happily and it is int() that then blows up. + MAX_PRS_DEFAULT = "5" # keep in sync with the input's `default:` above + raw = os.environ.get("MAX_PRS", MAX_PRS_DEFAULT).strip() or MAX_PRS_DEFAULT try: max_prs = max(0, int(float(raw))) - except ValueError: - print(f"::error::max_prs={raw!r} is not a number.") - raise SystemExit(1) + except (ValueError, OverflowError): + print(f"::warning::max_prs={raw!r} is not a number — opening NO builder PRs this run; every finding files as an issue instead. Re-dispatch with a numeric max_prs.") + max_prs = 0 to_build, built_positions = [], set() if builder: diff --git a/.github/workflows/test-bump-callers.yml b/.github/workflows/test-bump-callers.yml index d82e981..6b63502 100644 --- a/.github/workflows/test-bump-callers.yml +++ b/.github/workflows/test-bump-callers.yml @@ -2,12 +2,11 @@ name: Test bump-callers script # Runs the functional tests + shellcheck for the shared caller-bump script # (.github/bump-callers/bump-callers.sh). That one script drives the SHA-bump -# fan-out for the cursor-review, agents-md-integrity, pr-size, AND -# assign-reviewers caller fleets, -# so a regression here silently breaks every consumer repo's pin auto-bump (or, -# worse, leaks a private caller name into this public repo's run logs) — cheap -# to guard with a unit run on change. Mirrors test-cursor-review-scripts.yml / -# test-agents-md-integrity.yml. +# fan-out for the cursor-review, agents-md-integrity, pr-size, assign-reviewers +# AND groom caller fleets, so a regression here silently breaks every consumer +# repo's pin auto-bump (or, worse, leaks a private caller name into this public +# repo's run logs) — cheap to guard with a unit run on change. Mirrors +# test-cursor-review-scripts.yml / test-agents-md-integrity.yml. on: pull_request: @@ -17,6 +16,7 @@ on: - '.github/workflows/bump-agents-md-callers.yml' - '.github/workflows/bump-pr-size-callers.yml' - '.github/workflows/bump-assign-reviewers-callers.yml' + - '.github/workflows/bump-groom-callers.yml' - '.github/workflows/test-bump-callers.yml' push: branches: [main] @@ -26,6 +26,7 @@ on: - '.github/workflows/bump-agents-md-callers.yml' - '.github/workflows/bump-pr-size-callers.yml' - '.github/workflows/bump-assign-reviewers-callers.yml' + - '.github/workflows/bump-groom-callers.yml' - '.github/workflows/test-bump-callers.yml' permissions: diff --git a/AGENTS.md b/AGENTS.md index 134ae61..5e08853 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -79,8 +79,10 @@ tests — run the matching command above for whatever you touched. - `assign-prs-to-author.yml` — assigns unassigned open PRs to their author. - `detect-unreviewed-merge.yml` — SOC 2: flags PRs merged without approval. - `bump-cursor-review-callers.yml` / `bump-agents-md-callers.yml` / - `bump-pr-size-callers.yml` / `bump-assign-reviewers-callers.yml` — thin - entrypoints over `bump-callers.sh` that fan SHA bumps out to consumers. + `bump-pr-size-callers.yml` / `bump-assign-reviewers-callers.yml` / + `bump-groom-callers.yml` — thin entrypoints over `bump-callers.sh` that fan SHA + bumps out to consumers. A groom caller pins TWICE (`uses:` + `workflows_ref:`); + the shared rewrite moves both, so never hand-bump one alone. ## Conventions & gotchas diff --git a/README.md b/README.md index 9687148..faf8974 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ This repo is **public** so any repo — public or private, inside or outside the | [`assign-prs-to-author.yml`](.github/workflows/assign-prs-to-author.yml) | Housekeeping — assigns every open PR with no assignees to its author (bot-authored PRs skipped by default). Run on a schedule from a thin caller; useful when a team tracks PR ownership via assignees. The calling job needs `pull-requests: write` and `issues: write`. | | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | -| [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The agent step holds no write credentials (the `audit` job is `contents: read` only, per `model-gap-detector.yml`); filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` — declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set); GitHub rejects a shorter grant at startup. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read` (`id-token: write` is no longer required; a caller that still grants it is harmless). Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. Requires `bot_app_id`. | +| [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The agent step holds no write credentials (the `audit` job is `contents: read` only, per `model-gap-detector.yml`); filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` — declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set); GitHub rejects a shorter grant at startup. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read` (`id-token: write` is no longer required; a caller that still grants it is harmless). Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. | | [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | ## Usage From ac575f8c59bd10301b3cec0d12694e91b2d52d26 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 26 Jul 2026 00:56:20 -0700 Subject: [PATCH 2/5] fix(bump-callers): SHA-pin checkout, scope the bot token, anchor the pin-comment rewrite (BE-4346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the cursor-review panel on #77. - bump-groom-callers.yml checked out with the mutable tag `actions/checkout@v6` while holding an org-scoped bot token. AGENTS.md mandates a full-SHA pin for every third-party action; pinned to the same SHA groom.yml already carries. - The minted token requested no permissions, so it carried everything Cloud Code Bot holds on every repo it is installed on. Narrowed to the trio this bumper actually uses — contents (Git Data commit), pull-requests (open/update the bump PR), issues (`gh pr create --label`) — matching groom.yml's own PR job. `owner:` with no `repositories:` stays: the caller list is a runtime variable, and naming those repos here would leak private names into a public file. - The `# main @ ` comment rewrite was unanchored at its right edge, so `[0-9a-f]{7,12}` would match the first 12 characters of a longer hex run. On a deliberate `# main @ <40hex>` comment — which rule 1 has just rewritten to NEW_SHA — it swapped those 12 for the 7-char SHORT and stranded the other 28, mangling the exact full-form comment the {7,12} bound was there to protect. Split into two rules requiring a non-hex character or EOL after the run, so the match is a whole token; a 13+ hex run now matches neither and is left intact. Portable ERE rather than a `\b`/`[[:>:]]` assertion, which spells differently in GNU and BSD sed. 99/99 bump-callers functional tests (2 new; the full-form case fails against the old rewrite), shellcheck -x clean, actionlint clean, 59/59 groom ledger tests, agents-md-integrity passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- .github/bump-callers/bump-callers.sh | 14 +++++++++++++- .github/bump-callers/tests/test_bump_callers.sh | 9 +++++++++ .github/workflows/bump-groom-callers.yml | 17 ++++++++++++++++- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index c3d9493..62530cb 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -208,10 +208,22 @@ bump_repo() { # ` note elsewhere in the caller is untouched, and bounded to {7,12} hex # so a deliberate FULL-sha comment keeps its full form (the 40-hex rewrite # above has already corrected it) instead of being shortened. + # + # That bound only holds if the hex run ENDS there, hence the two rules rather + # than one: `[0-9a-f]{7,12}` alone is happy to match the first 12 characters of + # a longer run, so on a `# main @ <40hex>` comment (already rewritten to + # NEW_SHA by rule 1) it would swap 12 hex for the 7-char SHORT and leave the + # remaining 28 dangling — mangling the very full-form comment the bound exists + # to protect. Requiring a non-hex character (or end of line) after the run + # makes the match a whole token: rule 3 catches the short form mid-line, rule 4 + # the same at EOL, and a 13+ hex run matches neither and is left intact. A + # hex-boundary assertion (`\b`, `[[:>:]]`) would be simpler but spells + # differently in GNU and BSD sed; this is portable ERE. NEW_CONTENT=$(sed -E " /github-workflows|workflows_ref/ s/[0-9a-f]{40}/${NEW_SHA}/g s|# github-workflows#[0-9]+|# github-workflows main (${SHORT})|g - /github-workflows/ s|# main @ [0-9a-f]{7,12}|# main @ ${SHORT}|g + /github-workflows/ s|# main @ [0-9a-f]{7,12}([^0-9a-f])|# main @ ${SHORT}\1|g + /github-workflows/ s|# main @ [0-9a-f]{7,12}\$|# main @ ${SHORT}|g " <<<"$OLD_CONTENT") # Wire an extra identity/config into this file when its entry is flagged diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index 0797be4..81c3f1a 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -231,6 +231,8 @@ printf '%s\n' \ ' with:' \ ' workflows_ref: 1111111111111111111111111111111111111111' \ " max_prs: \${{ github.event.inputs.max_prs || '1' }}" \ + ' # github-workflows pin note, short form, hex run ends at EOL: # main @ 1111111' \ + ' # github-workflows pin note, deliberate FULL sha: # main @ 1111111111111111111111111111111111111111' \ > "$GROOM_FIXTURE" STUB_CONTENT_FILE="$GROOM_FIXTURE" run_bump \ VAR_NAME=GROOM_CALLERS TAG=groom WORKFLOW_FILE=groom.yml \ @@ -243,6 +245,13 @@ check "uses: pin moved" "grep -qE \"groom.yml@${NEW_SHA}\" check "workflows_ref pin moved" "grep -qE \"workflows_ref: ${NEW_SHA}\" \"$PUT\"" check "no stale 40-hex pin anywhere" "! grep -qF '1111111111111111111111111111111111111111' \"$PUT\"" check "'# main @' comment moved to new short" "grep -qF '# main @ $SHORT' \"$PUT\"" +# The `# main @` rewrite is bounded to a 7-12 hex SHORT sha, and that bound has to +# hold at BOTH ends of the run or it mangles what it claims to protect. +check "'# main @ ' at EOL rewritten" "grep -qE '# main @ ${SHORT}\$' \"$PUT\"" +# A deliberate FULL-sha comment is corrected by the 40-hex rule and must then be +# left alone — an unbounded {7,12} match would eat its first 12 characters, swap in +# the 7-char SHORT and strand the other 28 as a nonsense suffix. +check "full-sha '# main @' keeps full form" "grep -qF '# main @ ${NEW_SHA}' \"$PUT\"" # The third-party action pin is a full 40-hex SHA on a line that does NOT mention # github-workflows — the address anchor is what keeps it intact (the org mandates # SHA-pinning every action, so clobbering it would break the caller's CI). diff --git a/.github/workflows/bump-groom-callers.yml b/.github/workflows/bump-groom-callers.yml index f386d0d..3971255 100644 --- a/.github/workflows/bump-groom-callers.yml +++ b/.github/workflows/bump-groom-callers.yml @@ -70,7 +70,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v6 + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: persist-credentials: false @@ -80,7 +80,22 @@ jobs: with: app-id: ${{ vars.APP_ID }} private-key: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} + # `owner:` with no `repositories:` is deliberate and cannot be narrowed: + # the caller list is a runtime variable (see the header), so the repos + # this token must reach are not knowable at authoring time — and naming + # them here would leak private caller names into a public file. + # + # The PERMISSIONS can be narrowed without naming anything, so they are: + # unset, the minted token carries every permission the app holds on every + # repo it is installed on, far past what this bumper needs. It commits + # the rewritten caller file via the Git Data API (contents) and opens or + # updates the bump PR (pull-requests); `gh pr create --label` stamps the + # caller's label through the issues API (issues) — same trio, and the + # same reasoning, as groom.yml's own PR job. owner: Comfy-Org + permission-contents: write + permission-pull-requests: write + permission-issues: write - name: Bump SHA in caller repos env: From 46abc9f61f54c979d63cdc34a7c6328dc0ab83bf Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 26 Jul 2026 01:28:16 -0700 Subject: [PATCH 3/5] fix(bump-callers): anchor the pin-comment rewrite to workflows_ref too (BE-4346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 40-hex pin rewrite fires on `/github-workflows|workflows_ref/`, but the two `# main @ ` comment rewrites were anchored to `/github-workflows/` alone. A groom caller that annotates its `workflows_ref:` pin — the second of the two pins groom callers carry — got that line's SHA bumped while the comment kept naming the old commit: the stale "confident lie" pin comment these rules exist to kill, reintroduced on the pin the groom fleet was built to keep in lock-step. Widen the comment anchors to match rule 1's exactly, and cover both directions in the groom fixture: the `workflows_ref:` line now carries its own `# main @` note (must move), plus an unanchored note on a line naming neither pin context (must NOT move, proving the anchor still bounds the rewrite). The first check fails against the pre-fix script. Co-Authored-By: Claude Opus 5 --- .github/bump-callers/bump-callers.sh | 17 +++++++++++------ .github/bump-callers/tests/test_bump_callers.sh | 13 ++++++++++++- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index 62530cb..a980078 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -204,10 +204,15 @@ bump_repo() { # lie in the one file where the pin is the whole point: # `# github-workflows#27` -> `# github-workflows main ()` # `# main @ 29a81ca …` -> `# main @ …` (the groom callers' form) - # The second is anchored to the github-workflows line so an unrelated `# main @ - # ` note elsewhere in the caller is untouched, and bounded to {7,12} hex - # so a deliberate FULL-sha comment keeps its full form (the 40-hex rewrite - # above has already corrected it) instead of being shortened. + # The second is anchored to the SAME two pin contexts as the 40-hex rule above + # (`github-workflows` or `workflows_ref`) so an unrelated `# main @ ` note + # elsewhere in the caller is untouched, and bounded to {7,12} hex so a + # deliberate FULL-sha comment keeps its full form (the 40-hex rewrite above has + # already corrected it) instead of being shortened. The two anchors MUST stay + # identical: a groom caller's `workflows_ref: # main @ ` line is + # rewritten by rule 1, so a narrower anchor here would bump that pin while + # leaving its comment naming the old commit — reintroducing, on the second of + # the two groom pins, exactly the confident lie these rules exist to kill. # # That bound only holds if the hex run ENDS there, hence the two rules rather # than one: `[0-9a-f]{7,12}` alone is happy to match the first 12 characters of @@ -222,8 +227,8 @@ bump_repo() { NEW_CONTENT=$(sed -E " /github-workflows|workflows_ref/ s/[0-9a-f]{40}/${NEW_SHA}/g s|# github-workflows#[0-9]+|# github-workflows main (${SHORT})|g - /github-workflows/ s|# main @ [0-9a-f]{7,12}([^0-9a-f])|# main @ ${SHORT}\1|g - /github-workflows/ s|# main @ [0-9a-f]{7,12}\$|# main @ ${SHORT}|g + /github-workflows|workflows_ref/ s|# main @ [0-9a-f]{7,12}([^0-9a-f])|# main @ ${SHORT}\1|g + /github-workflows|workflows_ref/ s|# main @ [0-9a-f]{7,12}\$|# main @ ${SHORT}|g " <<<"$OLD_CONTENT") # Wire an extra identity/config into this file when its entry is flagged diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index 81c3f1a..fbcf9cb 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -229,10 +229,11 @@ printf '%s\n' \ ' - uses: actions/checkout@abcdefabcdefabcdefabcdefabcdefabcdefabcd # v6' \ ' uses: Comfy-Org/github-workflows/.github/workflows/groom.yml@1111111111111111111111111111111111111111 # main @ 1111111 — groom.yml not on the v1 tag yet' \ ' with:' \ - ' workflows_ref: 1111111111111111111111111111111111111111' \ + ' workflows_ref: 1111111111111111111111111111111111111111 # main @ 1111111' \ " max_prs: \${{ github.event.inputs.max_prs || '1' }}" \ ' # github-workflows pin note, short form, hex run ends at EOL: # main @ 1111111' \ ' # github-workflows pin note, deliberate FULL sha: # main @ 1111111111111111111111111111111111111111' \ + ' # unrelated third-party note, neither anchor on the line: # main @ 2222222' \ > "$GROOM_FIXTURE" STUB_CONTENT_FILE="$GROOM_FIXTURE" run_bump \ VAR_NAME=GROOM_CALLERS TAG=groom WORKFLOW_FILE=groom.yml \ @@ -252,6 +253,16 @@ check "'# main @ ' at EOL rewritten" "grep -qE '# main @ ${SHORT}\$' \" # left alone — an unbounded {7,12} match would eat its first 12 characters, swap in # the 7-char SHORT and strand the other 28 as a nonsense suffix. check "full-sha '# main @' keeps full form" "grep -qF '# main @ ${NEW_SHA}' \"$PUT\"" +# The `workflows_ref:` pin is bumped by the 40-hex rule, so its OWN `# main @ +# ` note has to move with it — the comment rules are anchored to the same +# two pin contexts as that rule for exactly this line. A narrower anchor bumps the +# pin and leaves the comment naming the old commit: a confident lie on the second +# of groom's two pins. +check "workflows_ref's own pin comment moved" \ + "grep -qE \"workflows_ref: ${NEW_SHA} +# main @ ${SHORT}\$\" \"$PUT\"" +# ...and the anchor still BOUNDS the rewrite: a `# main @ ` note on a line +# naming neither pin context belongs to some other pin and must be left alone. +check "unanchored '# main @' note untouched" "grep -qF '# main @ 2222222' \"$PUT\"" # The third-party action pin is a full 40-hex SHA on a line that does NOT mention # github-workflows — the address anchor is what keeps it intact (the org mandates # SHA-pinning every action, so clobbering it would break the caller's CI). From d4d807dfe8f46905ae4beb3ebe81a29283c69737 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 26 Jul 2026 04:15:52 -0700 Subject: [PATCH 4/5] fix(bump-callers): target the github-workflows pin token and assert both pins moved (BE-4662) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The caller pin rewrite keyed on "any 40-hex on a line that mentions github-workflows or workflows_ref" and never checked its own outcome, which failed silently in both directions: * Under-rewrite. A caller pins this repo TWICE — the `uses:` sha and the `workflows_ref:` input that loads the briefs/prompts/scripts at run time. A `workflows_ref` pinned to a tag (`v1`) or a short sha is not 40 hex, so it was left behind while `uses:` moved. The content-equality check still saw a difference, so the file was staged and a green-looking bump PR opened on a caller now running one version's workflow against another version's assets — the exact split this fleet exists to prevent. * Over-rewrite. An unrelated 40-hex value sharing such a line was clobbered. Anchor the substitution to the pin TOKEN instead — `Comfy-Org/github-workflows…@` and the `workflows_ref:` key — and take whatever ref follows by position, so any literal shape (full sha, short sha, tag) moves and a co-located SHA is unreachable. The `workflows_ref` rule is `^`-anchored to a block-mapping key so prose that merely mentions the input is left alone. Precision cuts both ways, so add the guard it needs: before a rewritten file can be staged, re-read it with a deliberately broader reader and assert every github-workflows pin now equals the new SHA. If one does not (today: a `workflows_ref` fed by a `${{ … }}` expression, which is never rewritten), warn with the file + the stale value and fail that repo — the same posture a transient fetch error already takes, because a partial bump is worse than no bump (BE-3896). The full-sha `# main @ <40hex>` comment form gets its own rule now that rule 1 no longer sprays every 40-hex on the line; it used to be corrected as collateral. Tests: +31 cases (130 total) — tag-pinned and short-sha `workflows_ref`, an unrelated 40-hex sharing a github-workflows line, the assertion firing (no commit, no PR, non-zero for that repo), and a prose comment not tripping it. shellcheck -x clean. --- .github/bump-callers/README.md | 37 ++++- .github/bump-callers/bump-callers.sh | 141 +++++++++++++---- .../bump-callers/tests/test_bump_callers.sh | 144 ++++++++++++++++++ .github/workflows/bump-groom-callers.yml | 9 +- 4 files changed, 292 insertions(+), 39 deletions(-) diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index d3544c3..70c16ed 100644 --- a/.github/bump-callers/README.md +++ b/.github/bump-callers/README.md @@ -42,11 +42,38 @@ The **groom** fleet is the one that most needs this: a groom caller pins the reusable **twice** — the `uses:` SHA *and* the `workflows_ref:` input that loads the finder/verifier/builder briefs plus the dedup ledger. Those must stay in lock-step or a run executes one version's workflow against another version's -briefs. `bump-callers.sh`'s pin rewrite moves both (it matches the `uses:` line -and any bare `workflows_ref:` line), so the fleet cannot drift into that split -state through a hand-bump of only one. It also re-points the `# main @ ` -pin comment those callers carry — a comment still naming the old commit after the -pin moved is worse than no comment. +briefs. `bump-callers.sh`'s pin rewrite moves both, so the fleet cannot drift +into that split state through a hand-bump of only one. It also re-points the +`# main @ ` pin comment those callers carry — a comment still naming the +old commit after the pin moved is worse than no comment. + +## How the pin rewrite is scoped (and why it asserts afterwards) + +The rewrite targets the **pin token**, not "any 40-hex on a line that mentions +`github-workflows`" (BE-4662). Two patterns, matched by position rather than by +what the ref *looks like*: + +- `Comfy-Org/github-workflows…@` — the `uses:` pin; +- `workflows_ref: ` as a block-mapping key, optionally quoted — the input pin. + +Whatever sits right after the token is the ref, so **any literal ref shape moves** +— full sha, short sha, or a tag like `v1`. That matters because a caller whose +`workflows_ref` is a tag used to be skipped by the old 40-hex rule while its +`uses:` pin moved: a green-looking bump PR on a caller that is now running one +version's workflow against another version's briefs. In the other direction, an +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. + +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 +value sitting where a ref belongs, comments excluded) and **asserts every +github-workflows pin now equals the new SHA**. If one does not — today the one +known case is a `workflows_ref` fed by a `${{ … }}` expression, which is +intentionally never rewritten — it emits a `::warning::` naming the file and the +stale value and **fails that repo**, exactly as it does for a transient fetch +error. A partial bump is worse than no bump (BE-3896). ## The caller variables diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index a980078..c11eba1 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -64,6 +64,31 @@ SHORT="${NEW_SHA:0:7}" # the commit/title/body, not the branch name. BRANCH="ci/bump-${TAG}" +# The two forms a caller uses to pin THIS repo, written as regex prefixes that +# end exactly where the ref begins. They live here, together, because the +# rewrite below and the post-rewrite assertion that guards it must never drift +# apart: whatever the rewrite is expected to move, the assertion re-reads. +# 1. the `uses:` pin — `Comfy-Org/github-workflows/@` +# 2. the input pin — `workflows_ref: ` (optionally quoted), which loads +# this repo's prompts/briefs/scripts at run time +# INPUT_PIN_RE is applied `^`-anchored (see the rewrite), i.e. only where +# `workflows_ref:` is a block-mapping KEY. That is what keeps the rewrite off a +# prose comment that merely mentions the input — `# workflows_ref: keep in sync +# with uses:` must not come back with a SHA spliced into the middle of the +# sentence. The assertion below is deliberately NOT anchored that way, so the +# one shape this misses (flow style, `with: {workflows_ref: v1}`) fails the repo +# loudly instead of silently half-bumping it. +USES_PIN_RE='Comfy-Org/github-workflows[^@[:space:]]*@' +INPUT_PIN_RE='[[:space:]]*workflows_ref:[[:space:]]*['\''"]?' +# A LITERAL ref: a full sha, a short sha, or a tag (`v1`). Deliberately excludes +# quotes, `#` and `$ { } ( )` so a closing quote or a trailing comment is never +# swallowed, and a GitHub expression (`workflows_ref: ${{ inputs.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. +# shellcheck disable=SC2016 # `$ { }` here are regex literals, not an expansion +REF_RE='[^[:space:]'\''"#$(){}]+' + STRIPPED="${CALLERS_JSON//[[:space:]]/}" # Empty handling is fleet-specific. A fleet seeded empty (ALLOW_EMPTY=true) @@ -190,43 +215,61 @@ bump_repo() { OLD_CONTENT=$(jq -r '.content' <<<"$CURRENT" | base64 -d) - # Rewrite the github-workflows pin(s) to NEW_SHA and normalize the stale - # `# github-workflows#NN` pin comment. Anchor the 40-hex substitution to the - # two known pin contexts — the `uses: …Comfy-Org/github-workflows…@` - # line and agents-md-integrity's bare `workflows_ref: ` line — so a - # full-SHA pin of ANOTHER action in the same file (`actions/checkout@`, - # the org's mandated practice) is never clobbered to github-workflows' SHA. - # The comment rewrites are a no-op for callers that use a different comment - # form (e.g. agents-md-integrity's `# v1`), so they are safe to share. + # Rewrite the github-workflows pin(s) to NEW_SHA and normalize the stale pin + # comments. + # + # Rules 1-2 are anchored to the PIN TOKEN itself (BE-4662) — + # `Comfy-Org/github-workflows…@` and `workflows_ref: ` — NOT to + # "any 40-hex on a line that mentions github-workflows or workflows_ref", + # which is what this used to key on. Keying on the line was wrong in both + # directions, and silently so: + # * UNDER-rewrite. A caller pins this repo TWICE — the `uses:` sha and the + # `workflows_ref:` input that loads the briefs/prompts/scripts at run + # time — and the halves must move in lock-step or a run executes one + # version's workflow against another version's assets. A `workflows_ref` + # pinned to a TAG (`v1`) or a short sha is not 40 hex, so it was left + # behind while `uses:` moved, and the content-equality check below still + # saw a difference — a green-looking bump PR on a split caller. + # * OVER-rewrite. An unrelated 40-hex value that merely shared such a line + # was clobbered to NEW_SHA. + # Matching the ref by POSITION (right after the pin token) instead of by + # 40-hex-ness fixes both at once: any literal ref shape is moved, and a + # full-SHA pin of ANOTHER action (`actions/checkout@`, the org's + # mandated practice) or a co-located digest is unreachable by both patterns. + # Rule 2 is additionally `^`-anchored so it only fires where `workflows_ref:` + # is a block-mapping key, never inside a prose comment that mentions it. # - # Two comment forms are normalized, because a pin comment that still names the - # OLD commit after the pin moved is worse than no comment — it is a confident - # lie in the one file where the pin is the whole point: + # Rules 3-5 normalize the pin COMMENTS, because a comment that still names + # the OLD commit after the pin moved is worse than no comment — it is a + # confident lie in the one file where the pin is the whole point: # `# github-workflows#27` -> `# github-workflows main ()` # `# main @ 29a81ca …` -> `# main @ …` (the groom callers' form) - # The second is anchored to the SAME two pin contexts as the 40-hex rule above - # (`github-workflows` or `workflows_ref`) so an unrelated `# main @ ` note - # elsewhere in the caller is untouched, and bounded to {7,12} hex so a - # deliberate FULL-sha comment keeps its full form (the 40-hex rewrite above has - # already corrected it) instead of being shortened. The two anchors MUST stay - # identical: a groom caller's `workflows_ref: # main @ ` line is - # rewritten by rule 1, so a narrower anchor here would bump that pin while - # leaving its comment naming the old commit — reintroducing, on the second of - # the two groom pins, exactly the confident lie these rules exist to kill. + # The `# main @` rules are anchored to that comment token AND to the same two + # pin contexts by line address (`github-workflows` or `workflows_ref`), so an + # unrelated `# main @ ` note elsewhere in the caller is untouched. The + # two line addresses MUST stay identical: a groom caller's + # `workflows_ref: # main @ ` line is rewritten by rule 2, so a + # narrower address here would bump that pin while leaving its comment naming + # the old commit — reintroducing, on the second of the two groom pins, exactly + # the confident lie these rules exist to kill. # - # That bound only holds if the hex run ENDS there, hence the two rules rather - # than one: `[0-9a-f]{7,12}` alone is happy to match the first 12 characters of - # a longer run, so on a `# main @ <40hex>` comment (already rewritten to - # NEW_SHA by rule 1) it would swap 12 hex for the 7-char SHORT and leave the - # remaining 28 dangling — mangling the very full-form comment the bound exists - # to protect. Requiring a non-hex character (or end of line) after the run - # makes the match a whole token: rule 3 catches the short form mid-line, rule 4 - # the same at EOL, and a 13+ hex run matches neither and is left intact. A - # hex-boundary assertion (`\b`, `[[:>:]]`) would be simpler but spells - # differently in GNU and BSD sed; this is portable ERE. + # Rule 3 (the FULL-sha comment form) is load-bearing now that rule 1 no longer + # sprays every 40-hex on the line: a deliberate `# main @ <40hex>` note used to + # be corrected as collateral of the old line-scoped substitution, and would + # otherwise be left naming the old commit. Rules 4-5 handle the SHORT form and + # are bounded to {7,12} hex, which only holds if the hex run ENDS there — + # `[0-9a-f]{7,12}` alone is happy to match the first 12 characters of a longer + # run, so on a 40-hex comment it would swap 12 hex for the 7-char SHORT and + # leave the remaining 28 dangling, mangling the very full-form comment rule 3 + # just corrected. Requiring a non-hex character (rule 4) or end of line + # (rule 5) after the run makes the match a whole token, so a 13+ hex run + # 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 " - /github-workflows|workflows_ref/ s/[0-9a-f]{40}/${NEW_SHA}/g + s|(${USES_PIN_RE})${REF_RE}|\1${NEW_SHA}|g + s|^(${INPUT_PIN_RE})${REF_RE}|\1${NEW_SHA}| s|# github-workflows#[0-9]+|# github-workflows main (${SHORT})|g + /github-workflows|workflows_ref/ s|# main @ [0-9a-f]{40}|# main @ ${NEW_SHA}|g /github-workflows|workflows_ref/ s|# main @ [0-9a-f]{7,12}([^0-9a-f])|# main @ ${SHORT}\1|g /github-workflows|workflows_ref/ s|# main @ [0-9a-f]{7,12}\$|# main @ ${SHORT}|g " <<<"$OLD_CONTENT") @@ -252,6 +295,42 @@ bump_repo() { fi 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 + # move is silently left behind. So re-read the result with a DELIBERATELY + # BROADER reader — any non-whitespace value sitting where a ref belongs — and + # fail the repo if anything but NEW_SHA is still there. The gap between the + # two (today: a `workflows_ref` fed by a `${{ … }}` expression, which is + # deliberately never rewritten) then surfaces as a loud, named failure + # instead of a green-looking PR that bumps `uses:` and leaves the assets ref + # behind. A caller running one version's workflow against another version's + # briefs is exactly the split this fleet exists to prevent, and — as with the + # transient fetch error above — a partial bump is worse than no bump (BE-3896). + # + # Comments are not pins, so drop everything from the first `#` on each line + # before scanning: a prose note that happens to say `workflows_ref:` must not + # fail an otherwise-clean repo. This cannot hide a real pin — a pin and its + # trailing comment are never the same token, and the rewrite's REF_RE stops + # at `#` for the same reason. + local LIVE_CONTENT STALE_PINS + LIVE_CONTENT=$(sed -E 's|#.*$||' <<<"$NEW_CONTENT") + STALE_PINS=$( + { + # A `uses:` ref cannot contain whitespace, so the token after `@` is the + # whole ref. `workflows_ref` is a YAML scalar, so take the rest of the + # line — that keeps an expression value readable in the warning instead + # of truncating it to `${{`. + grep -oE "${USES_PIN_RE}[^[:space:]]+" <<<"$LIVE_CONTENT" | sed -E 's|^.*@||' + grep -E "workflows_ref:[[:space:]]*[^[:space:]]" <<<"$LIVE_CONTENT" \ + | sed -E 's|^.*workflows_ref:[[:space:]]*||; s|[[:space:]]+$||' + } | sed -E "s|^['\"]||; s|['\"]\$||" | grep -v '^$' | grep -vFx "$NEW_SHA" | sort -u + ) + if [[ -n "$STALE_PINS" ]]; then + echo "::warning::${REPO}: ${FILE} still pins github-workflows at $(tr '\n' ' ' <<<"$STALE_PINS")after the rewrite (expected ${NEW_SHA}) — failing repo to avoid a half-bumped caller" + return 1 + fi + # Already fully pinned → the rewrite is a no-op → nothing to do for this file. # Comparing the rewritten content to the original (rather than grepping for # NEW_SHA appearing *anywhere*) also repairs a half-bumped file: if a prior diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index fbcf9cb..9953e6b 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -10,6 +10,11 @@ # * every private repo name is masked out of the public run logs, # * the caller's pinned SHA (and only it) is rewritten, the pin comment is # normalized, and the committed file keeps its single trailing newline, +# * BOTH pin halves move in lock-step — the `uses:` ref and the +# `workflows_ref` input, whatever shape the latter carries (full sha, short +# sha, tag) — while an unrelated 40-hex merely sharing such a line is left +# alone, and a pin the rewrite cannot move fails the repo rather than +# shipping a half-bumped caller, # * an empty seeded-empty fleet is a clean no-op while a must-have-callers # fleet still hard-fails, and a malformed variable hard-fails. # @@ -550,6 +555,145 @@ 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 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 +# old substitution rewrote "any 40-hex on a line mentioning github-workflows", so +# a `workflows_ref` pinned to a TAG was left behind while `uses:` moved: a +# green-looking bump PR running one version's workflow against another version's +# assets. The rewrite is anchored to the pin token now, so ref SHAPE is irrelevant. +new_case reftag +TAG_FIXTURE="${WORK}/tag_ref_caller.yml" +printf '%s\n' \ + 'name: AGENTS.md Integrity' \ + 'jobs:' \ + ' agents-md:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/agents-md-integrity.yml@2222222222222222222222222222222222222222 # v1' \ + ' with:' \ + ' workflows_ref: v1' \ + > "$TAG_FIXTURE" +STUB_CONTENT_FILE="$TAG_FIXTURE" run_bump \ + VAR_NAME=AGENTS_MD_CALLERS TAG=agents-md-integrity WORKFLOW_FILE=agents-md-integrity.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-tagref","file":".github/workflows/agents-md-integrity.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 0" "[[ $RC -eq 0 ]]" +check "staged the caller" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "uses: pin bumped" "grep -qF 'agents-md-integrity.yml@$NEW_SHA' \"$PUT\"" +check "TAG workflows_ref bumped too" "grep -qF 'workflows_ref: $NEW_SHA' \"$PUT\"" +check "no tag left in workflows_ref" "! grep -qE '^[[:space:]]*workflows_ref:[[:space:]]*v1[[:space:]]*\$' \"$PUT\"" +check "both pins at the new SHA" "[[ \$(grep -cF '$NEW_SHA' \"$PUT\") -eq 2 ]]" +check "'# v1' comment left intact" "grep -qF '# v1' \"$PUT\"" + +echo "== a SHORT-SHA (and quoted) workflows_ref also moves in lock-step (BE-4662) ==" +# The other non-40-hex shape a caller can carry. The quotes must survive — only +# the ref token inside them is replaced, so the YAML stays valid. +new_case refshort +SHORT_FIXTURE="${WORK}/short_ref_caller.yml" +printf '%s\n' \ + 'name: AGENTS.md Integrity' \ + 'jobs:' \ + ' agents-md:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/agents-md-integrity.yml@2222222222222222222222222222222222222222 # v1' \ + ' with:' \ + " workflows_ref: '2222222'" \ + > "$SHORT_FIXTURE" +STUB_CONTENT_FILE="$SHORT_FIXTURE" run_bump \ + VAR_NAME=AGENTS_MD_CALLERS TAG=agents-md-integrity WORKFLOW_FILE=agents-md-integrity.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-shortref","file":".github/workflows/agents-md-integrity.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 0" "[[ $RC -eq 0 ]]" +check "staged the caller" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "short-sha workflows_ref bumped" "grep -qF \"workflows_ref: '$NEW_SHA'\" \"$PUT\"" +check "closing quote preserved" "! grep -qF \"workflows_ref: $NEW_SHA'\" \"$PUT\"" +check "short sha gone" "! grep -qF \"workflows_ref: '2222222'\" \"$PUT\"" +check "both pins at the new SHA" "[[ \$(grep -cF '$NEW_SHA' \"$PUT\") -eq 2 ]]" + +echo "== an unrelated 40-hex SHARING a github-workflows line is NOT clobbered (BE-4662) ==" +# The over-rewrite half of BE-4662. The old substitution keyed on the LINE ("any +# 40-hex on a line that mentions github-workflows or workflows_ref"), so an +# unrelated full-SHA value that merely shared such a line — another action pinned +# next to a mention of this repo, a digest documented as tracking workflows_ref — +# was rewritten to github-workflows' SHA. Only the pin TOKEN may move. (The +# sibling `anchor` case above covers the easy version, where the third-party pin +# sits on its own line.) +new_case coloc +COLOC_FIXTURE="${WORK}/coloc_caller.yml" +printf '%s\n' \ + 'name: CI cursor-review' \ + 'jobs:' \ + ' review:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@1111111111111111111111111111111111111111 # github-workflows#27' \ + ' audit:' \ + ' runs-on: ubuntu-latest' \ + ' steps:' \ + ' - uses: some-org/mirror-check@cccccccccccccccccccccccccccccccccccccccc # verifies the github-workflows mirror' \ + ' - name: digest' \ + ' run: echo dddddddddddddddddddddddddddddddddddddddd # kept in sync with workflows_ref' \ + > "$COLOC_FIXTURE" +STUB_CONTENT_FILE="$COLOC_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-coloc","file":".github/workflows/ci.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 0" "[[ $RC -eq 0 ]]" +check "github-workflows pin bumped" "grep -qF 'cursor-review.yml@$NEW_SHA' \"$PUT\"" +check "co-located action SHA left intact" "grep -qF 'some-org/mirror-check@cccccccccccccccccccccccccccccccccccccccc' \"$PUT\"" +check "co-located digest left intact" "grep -qF 'echo dddddddddddddddddddddddddddddddddddddddd' \"$PUT\"" +check "exactly ONE pin was rewritten" "[[ \$(grep -cF '$NEW_SHA' \"$PUT\") -eq 1 ]]" + +echo "== a pin the rewrite cannot move FAILS the repo — no commit, no PR (BE-4662) ==" +# The assertion is the backstop for anything the (deliberately precise) rewrite +# does not know how to move — here a `workflows_ref` fed by a GitHub expression, +# which must never be half-rewritten into a broken value. Staging it would open a +# green-looking PR whose `uses:` moved and whose assets ref did not, so the repo +# fails instead: a partial bump is worse than no bump (BE-3896). +new_case assertfire +EXPR_FIXTURE="${WORK}/expr_ref_caller.yml" +# shellcheck disable=SC2016 # the `${{ }}` must reach the fixture verbatim +printf '%s\n' \ + 'name: AGENTS.md Integrity' \ + 'jobs:' \ + ' agents-md:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/agents-md-integrity.yml@2222222222222222222222222222222222222222 # v1' \ + ' with:' \ + ' workflows_ref: ${{ inputs.workflows_ref }}' \ + > "$EXPR_FIXTURE" +STUB_CONTENT_FILE="$EXPR_FIXTURE" run_bump \ + VAR_NAME=AGENTS_MD_CALLERS TAG=agents-md-integrity WORKFLOW_FILE=agents-md-integrity.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-expr","file":".github/workflows/agents-md-integrity.yml","label":""}]' +check "exit 1 — the repo failed" "[[ $RC -eq 1 ]]" +check "warning names the file" "grep -q 'agents-md-integrity.yml still pins github-workflows' <<<\"\$OUT\"" +check "warning names the un-bumped pin" "grep -qF 'inputs.workflows_ref' <<<\"\$OUT\"" +check "committed NOTHING" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]" +check "opened NO PR" "[[ ! -f \"\$STUB_PUT_DIR/pr.log\" ]] || ! grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" +check "job failed for the repo" "grep -q 'bump failed for 1 repo' <<<\"\$OUT\"" + +echo "== the assertion reads PINS, not prose — a commented workflows_ref is not a stale pin ==" +# The assertion is deliberately broader than the rewrite, which makes it the one +# place a false positive would hard-fail an otherwise-clean repo. Comments are +# stripped before it scans, so a human note that happens to say `workflows_ref:` +# cannot masquerade as an un-bumped pin — while the real pin on the next line is +# still asserted (it is checked before the `#`). +new_case prosecomment +PROSE_FIXTURE="${WORK}/prose_caller.yml" +printf '%s\n' \ + 'name: AGENTS.md Integrity' \ + 'jobs:' \ + ' agents-md:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/agents-md-integrity.yml@2222222222222222222222222222222222222222 # v1' \ + ' with:' \ + ' # workflows_ref: keep this in lock-step with the uses: pin above' \ + ' workflows_ref: 2222222222222222222222222222222222222222 # bumped by CI' \ + > "$PROSE_FIXTURE" +STUB_CONTENT_FILE="$PROSE_FIXTURE" run_bump \ + VAR_NAME=AGENTS_MD_CALLERS TAG=agents-md-integrity WORKFLOW_FILE=agents-md-integrity.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-prose","file":".github/workflows/agents-md-integrity.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 0 — prose did not trip the assert" "[[ $RC -eq 0 ]]" +check "staged the caller" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "both real pins bumped" "[[ \$(grep -cF '$NEW_SHA' \"$PUT\") -eq 2 ]]" +check "old pin gone" "! grep -qF '2222222222222222222222222222222222222222' \"$PUT\"" +check "prose comment still readable" "grep -qF 'keep this in lock-step with the uses: pin above' \"$PUT\"" + echo echo "== $PASS passed, $FAIL failed ==" [[ $FAIL -eq 0 ]] diff --git a/.github/workflows/bump-groom-callers.yml b/.github/workflows/bump-groom-callers.yml index 3971255..729dac6 100644 --- a/.github/workflows/bump-groom-callers.yml +++ b/.github/workflows/bump-groom-callers.yml @@ -15,9 +15,12 @@ name: Bump groom callers # reusable TWICE — the `uses:` SHA and the `workflows_ref:` input that loads the # finder/verifier/builder briefs + the dedup ledger. Those two must stay in # lock-step or a run executes one version's workflow against another version's -# briefs. bump-callers.sh rewrites both (its rewrite matches the `uses:` line and -# any bare `workflows_ref:` line), so the fleet cannot drift into that split -# state by hand-bumping only one of them. +# briefs. bump-callers.sh rewrites both — it targets the pin TOKEN +# (`Comfy-Org/github-workflows…@` and the `workflows_ref:` key), so the ref's +# shape does not matter and both halves move whatever they are pinned to — and it +# then asserts every pin in the file reached the new SHA before staging it. So the +# fleet cannot drift into that split state by hand-bumping only one of them, nor +# ship a half-bumped caller (BE-4662). # # The caller list is NOT hardcoded here. This repo is PUBLIC (workflow file and # Actions run logs are both publicly viewable) and callers may be private, so From b929a0086fead861b4169784fde83877a2444ae5 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 26 Jul 2026 04:49:24 -0700 Subject: [PATCH 5/5] =?UTF-8?q?fix(bump-callers):=20bound=20the=20pin=20to?= =?UTF-8?q?ken's=20edges=20=E2=80=94=20sibling=20repo,=20owner=20case,=20k?= =?UTF-8?q?ey=20boundary=20(BE-4662)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the pin-token rewrite + assertion. All five are edges of the same idea: the token has to end where the ref begins, and the assertion has to read exactly what the rewrite writes. * the `uses:` pattern now requires a DELIMITER after the repo name (`/` or `@`), so a sibling repo whose name merely starts the same (`Comfy-Org/github-workflows-tools/action@v1`) is no longer swallowed and repinned to this repo's SHA — and, because the assertion reuses the pattern, no longer read back as NEW_SHA and staged silently; * the owner/repo is matched case-INSENSITIVELY, because GitHub resolves `uses:` that way. A `comfy-org/…` caller previously had its (repo-agnostic) `workflows_ref` half bumped while `uses:` stayed stale, and the assertion — reading `uses:` with the same pattern — missed the stale half too: precisely the split half-bump this guard exists to prevent; * the assertion's `workflows_ref` reader gained a left boundary, so a longer key like `upstream_workflows_ref: v1` (which the rewrite correctly leaves alone) is no longer read as an un-bumped pin and no longer hard-fails a clean caller's bump on every run; * comments are dropped by YAML's own rule (a `#` preceded by whitespace) instead of at the first `#`, so a `#` INSIDE a ref cannot fake its way past: REF_RE half-moves `'feature#1'` to `'#1'`, which now compares unequal and fails the repo rather than being read back as a clean NEW_SHA; * an empty pin (`workflows_ref: ""`) is named `(empty)` rather than filtered out as a blank — the rewrite cannot move it either, so dropping it let a silent half-bump through. The failure warning is also sanitized before it reaches this public repo's run logs (carriage return stripped, `::` neutralized) so a caller-supplied value cannot inject a workflow command, and the value/prose spacing is now explicit rather than relying on the here-string's trailing newline. Five regression cases added; each fails against the pre-fix script (16 checks). --- .github/bump-callers/README.md | 17 ++- .github/bump-callers/bump-callers.sh | 69 ++++++++-- .../bump-callers/tests/test_bump_callers.sh | 129 ++++++++++++++++++ 3 files changed, 202 insertions(+), 13 deletions(-) diff --git a/.github/bump-callers/README.md b/.github/bump-callers/README.md index 70c16ed..b25cfdf 100644 --- a/.github/bump-callers/README.md +++ b/.github/bump-callers/README.md @@ -53,7 +53,11 @@ The rewrite targets the **pin token**, not "any 40-hex on a line that mentions `github-workflows`" (BE-4662). Two patterns, matched by position rather than by what the ref *looks like*: -- `Comfy-Org/github-workflows…@` — the `uses:` pin; +- `Comfy-Org/github-workflows…@` — the `uses:` pin. The owner/repo is matched + **case-insensitively**, because GitHub resolves `uses:` that way and a caller + written `comfy-org/…` is calling this repo; what follows the repo name must be + the `/` of a path or the `@` of a ref, so a **sibling** repo whose name merely + starts the same (`github-workflows-tools/action@v1`) is out of reach. - `workflows_ref: ` as a block-mapping key, optionally quoted — the input pin. Whatever sits right after the token is the ref, so **any literal ref shape moves** @@ -73,7 +77,16 @@ github-workflows pin now equals the new SHA**. If one does not — today the one known case is a `workflows_ref` fed by a `${{ … }}` expression, which is intentionally never rewritten — it emits a `::warning::` naming the file and the stale value and **fails that repo**, exactly as it does for a transient fetch -error. A partial bump is worse than no bump (BE-3896). +error. A partial bump is worse than no bump (BE-3896). An empty pin +(`workflows_ref: ""`) and a value the rewrite could only half-move (a `#` inside +the ref) fail the same way rather than reading back as clean. + +Because that reader is the one place a false positive would block an otherwise +clean caller's bump on every run, it is bounded on both sides: comments are +dropped by YAML's own rule (a `#` preceded by whitespace, so a `#` *inside* a +value survives to be compared), and the `workflows_ref` key needs a real left +boundary, so a longer key that merely ends in it (`upstream_workflows_ref: v1`) +is not read as this repo's pin. ## The caller variables diff --git a/.github/bump-callers/bump-callers.sh b/.github/bump-callers/bump-callers.sh index c11eba1..2c4026d 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -78,8 +78,36 @@ BRANCH="ci/bump-${TAG}" # sentence. The assertion below is deliberately NOT anchored that way, so the # one shape this misses (flow style, `with: {workflows_ref: v1}`) fails the repo # loudly instead of silently half-bumping it. -USES_PIN_RE='Comfy-Org/github-workflows[^@[:space:]]*@' +# +# The repo name is spelled case-INSENSITIVELY because GitHub resolves a `uses:` +# owner/repo that way — a caller written `comfy-org/github-workflows/…@` is +# calling THIS repo and must be bumped like any other. Missing it would be the +# worst kind of miss: rule 2 below is repo-agnostic, so the caller's +# `workflows_ref:` half would move while `uses:` stayed stale, and the assertion +# (which reuses this same regex to read `uses:` back) would not see the stale +# half either — shipping exactly the split bump this change exists to prevent. +# Character classes rather than a case-insensitive flag: sed's `I` and grep's +# `-i` are not portable/scopable the same way, and this regex is shared by both. +REPO_RE='[Cc][Oo][Mm][Ff][Yy]-[Oo][Rr][Gg]/[Gg][Ii][Tt][Hh][Uu][Bb]-[Ww][Oo][Rr][Kk][Ff][Ll][Oo][Ww][Ss]' +# The path segment is OPTIONAL but the delimiter after the repo name is NOT: +# what follows `github-workflows` must be the `/` that starts the path or the `@` +# that starts the ref. Without that, `[^@[:space:]]*` also swallows a SIBLING +# repo's name — `Comfy-Org/github-workflows-tools/action@v1` would be repinned to +# THIS repo's SHA, and because the assertion reuses this regex it would read the +# corrupted value back as NEW_SHA and stage it silently. +USES_PIN_RE="${REPO_RE}(/[^@[:space:]]*)?@" INPUT_PIN_RE='[[:space:]]*workflows_ref:[[:space:]]*['\''"]?' +# The assertion's reader for the input pin. Deliberately looser than +# INPUT_PIN_RE's `^`-anchored use — it also catches flow style +# (`with: {workflows_ref: v1}`), which the rewrite cannot move — but it still +# needs a LEFT boundary. Unanchored, `workflows_ref:` matches inside a longer key +# like `upstream_workflows_ref: v1`, whose value rule 2 correctly leaves alone; +# the assertion would then read that value as a stale github-workflows pin and +# hard-fail an otherwise-clean repo on every run. The boundary is a plain +# character class, not `(^|…)`: `^` inside a group is a GNU extension, and the +# assertion pads every line with a leading space first (see below) so a key at +# column 0 is covered by the same class. +INPUT_KEY_RE='[[:space:],{]workflows_ref:' # A LITERAL ref: a full sha, a short sha, or a tag (`v1`). Deliberately excludes # quotes, `#` and `$ { } ( )` so a closing quote or a trailing comment is never # swallowed, and a GitHub expression (`workflows_ref: ${{ inputs.workflows_ref }}`) @@ -308,13 +336,19 @@ bump_repo() { # briefs is exactly the split this fleet exists to prevent, and — as with the # transient fetch error above — a partial bump is worse than no bump (BE-3896). # - # Comments are not pins, so drop everything from the first `#` on each line - # before scanning: a prose note that happens to say `workflows_ref:` must not - # fail an otherwise-clean repo. This cannot hide a real pin — a pin and its - # trailing comment are never the same token, and the rewrite's REF_RE stops - # at `#` for the same reason. - local LIVE_CONTENT STALE_PINS - LIVE_CONTENT=$(sed -E 's|#.*$||' <<<"$NEW_CONTENT") + # Comments are not pins, so drop them before scanning: a prose note that + # happens to say `workflows_ref:` must not fail an otherwise-clean repo. A + # comment is stripped by YAML's OWN rule — a `#` preceded by whitespace (every + # line is padded with a leading space first, so that also covers a `#` at + # column 0, and gives INPUT_KEY_RE's boundary class a character to match at + # line start) — NOT at the first `#` anywhere on the line. That distinction is + # what keeps the assertion honest about a `#` INSIDE a value: REF_RE stops at + # `#`, so `workflows_ref: 'feature#1'` is rewritten to `'#1'`, and a + # first-`#` strip would read back a bare NEW_SHA and accept that corrupted + # value. Under YAML's rule the value survives intact, compares unequal, and + # fails the repo — the loud outcome, as designed. + local LIVE_CONTENT STALE_PINS SAFE_PINS + LIVE_CONTENT=$(sed -E 's|^| |; s|[[:space:]]#.*$||' <<<"$NEW_CONTENT") STALE_PINS=$( { # A `uses:` ref cannot contain whitespace, so the token after `@` is the @@ -322,12 +356,25 @@ bump_repo() { # line — that keeps an expression value readable in the warning instead # of truncating it to `${{`. grep -oE "${USES_PIN_RE}[^[:space:]]+" <<<"$LIVE_CONTENT" | sed -E 's|^.*@||' - grep -E "workflows_ref:[[:space:]]*[^[:space:]]" <<<"$LIVE_CONTENT" \ + grep -oE "${INPUT_KEY_RE}[[:space:]]*[^[:space:]].*$" <<<"$LIVE_CONTENT" \ | sed -E 's|^.*workflows_ref:[[:space:]]*||; s|[[:space:]]+$||' - } | sed -E "s|^['\"]||; s|['\"]\$||" | grep -v '^$' | grep -vFx "$NEW_SHA" | sort -u + # An extracted value that is EMPTY after unquoting is named rather than + # dropped: `workflows_ref: ""` is a pin the rewrite cannot move either + # (REF_RE needs ≥1 character), so filtering blanks out here would let it + # slip past the assertion while `uses:` moved — the exact silent half-bump + # this guard exists to catch. It is not a legal ref, so it fails loudly. + } | sed -E "s|^['\"]||; s|['\"]\$||" \ + | sed -E 's|^$|(empty)|' | grep -vFx "$NEW_SHA" | sort -u ) if [[ -n "$STALE_PINS" ]]; then - echo "::warning::${REPO}: ${FILE} still pins github-workflows at $(tr '\n' ' ' <<<"$STALE_PINS")after the rewrite (expected ${NEW_SHA}) — failing repo to avoid a half-bumped caller" + # Sanitize before echoing: these values come from a caller file, and the + # run logs of this public repo are a workflow-command sink. `tr` collapses + # the newline join but not a carriage return, so a value carrying `\r::` + # could inject a command of its own; `::` is neutralized for the same + # reason. (The value itself is a ref of THIS public repo, so there is + # nothing private to redact — only a control character to defang.) + SAFE_PINS=$(tr -d '\r' <<<"$STALE_PINS" | sed -E 's|::|:|g' | tr '\n' ' ' | sed -E 's|[[:space:]]+$||') + echo "::warning::${REPO}: ${FILE} still pins github-workflows at ${SAFE_PINS} after the rewrite (expected ${NEW_SHA}) — failing repo to avoid a half-bumped caller" return 1 fi diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index 9953e6b..7fa789f 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -15,6 +15,10 @@ # sha, tag) — while an unrelated 40-hex merely sharing such a line is left # alone, and a pin the rewrite cannot move fails the repo rather than # shipping a half-bumped caller, +# * the pin token's edges hold in both directions: a differently-cased +# owner/repo is still this repo, while a sibling repo whose name starts the +# same and a longer key ending in `workflows_ref` are neither rewritten nor +# misread as a stale pin, # * an empty seeded-empty fleet is a clean no-op while a must-have-callers # fleet still hard-fails, and a malformed variable hard-fails. # @@ -663,6 +667,7 @@ STUB_CONTENT_FILE="$EXPR_FIXTURE" run_bump \ check "exit 1 — the repo failed" "[[ $RC -eq 1 ]]" check "warning names the file" "grep -q 'agents-md-integrity.yml still pins github-workflows' <<<\"\$OUT\"" check "warning names the un-bumped pin" "grep -qF 'inputs.workflows_ref' <<<\"\$OUT\"" +check "warning reads as a sentence" "grep -qF 'inputs.workflows_ref }} after the rewrite' <<<\"\$OUT\"" check "committed NOTHING" "[[ ! -f \"\$STUB_PUT_DIR/count\" ]]" check "opened NO PR" "[[ ! -f \"\$STUB_PUT_DIR/pr.log\" ]] || ! grep -q '^pr-create' \"\$STUB_PUT_DIR/pr.log\"" check "job failed for the repo" "grep -q 'bump failed for 1 repo' <<<\"\$OUT\"" @@ -694,6 +699,130 @@ check "both real pins bumped" "[[ \$(grep -cF '$NEW_SHA' \"$PU check "old pin gone" "! grep -qF '2222222222222222222222222222222222222222' \"$PUT\"" check "prose comment still readable" "grep -qF 'keep this in lock-step with the uses: pin above' \"$PUT\"" +echo "== a SIBLING repo that merely starts with our name is NOT repinned (BE-4662) ==" +# The pin token ends at a DELIMITER. Without one, the path glob after the repo +# name also swallows a sibling repo's name — `github-workflows-tools/action@v1` +# would be repinned to THIS repo's SHA, and because the assertion reads pins with +# the same pattern it would read the corrupted value back as NEW_SHA and stage it. +new_case sibling +SIB_FIXTURE="${WORK}/sibling_caller.yml" +printf '%s\n' \ + 'name: CI cursor-review' \ + 'jobs:' \ + ' review:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@1111111111111111111111111111111111111111 # github-workflows#27' \ + ' tools:' \ + ' uses: Comfy-Org/github-workflows-tools/.github/workflows/lint.yml@cccccccccccccccccccccccccccccccccccccccc # v3' \ + ' steps:' \ + ' - uses: Comfy-Org/github-workflows-actions/setup@v2' \ + > "$SIB_FIXTURE" +STUB_CONTENT_FILE="$SIB_FIXTURE" run_bump \ + VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-sibling","file":".github/workflows/ci.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 0" "[[ $RC -eq 0 ]]" +check "our pin bumped" "grep -qF 'cursor-review.yml@$NEW_SHA' \"$PUT\"" +check "sibling repo's SHA pin left intact" "grep -qF 'github-workflows-tools/.github/workflows/lint.yml@cccccccccccccccccccccccccccccccccccccccc' \"$PUT\"" +check "sibling repo's tag pin left intact" "grep -qF 'github-workflows-actions/setup@v2' \"$PUT\"" +check "exactly ONE pin was rewritten" "[[ \$(grep -cF '$NEW_SHA' \"$PUT\") -eq 1 ]]" + +echo "== a lowercase owner/repo is still THIS repo — both halves move (BE-4662) ==" +# GitHub resolves `uses:` owner/repo case-insensitively, so a caller written +# `comfy-org/…` is calling this repo. A case-SENSITIVE match would skip its +# `uses:` half while rule 2 (repo-agnostic) bumped `workflows_ref` anyway, and the +# assertion — reading `uses:` with the same pattern — would not see the stale half +# either: a silently half-bumped caller, the split this change exists to prevent. +new_case lowercase +LC_FIXTURE="${WORK}/lowercase_caller.yml" +printf '%s\n' \ + 'name: AGENTS.md Integrity' \ + 'jobs:' \ + ' agents-md:' \ + ' uses: comfy-org/github-workflows/.github/workflows/agents-md-integrity.yml@2222222222222222222222222222222222222222 # v1' \ + ' with:' \ + ' workflows_ref: v1' \ + > "$LC_FIXTURE" +STUB_CONTENT_FILE="$LC_FIXTURE" run_bump \ + VAR_NAME=AGENTS_MD_CALLERS TAG=agents-md-integrity WORKFLOW_FILE=agents-md-integrity.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-lowercase","file":".github/workflows/agents-md-integrity.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 0" "[[ $RC -eq 0 ]]" +check "staged the caller" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "lowercase uses: pin bumped" "grep -qF 'comfy-org/github-workflows/.github/workflows/agents-md-integrity.yml@$NEW_SHA' \"$PUT\"" +check "workflows_ref bumped in lock-step" "grep -qF 'workflows_ref: $NEW_SHA' \"$PUT\"" +check "owner case preserved as written" "! grep -qF 'Comfy-Org/github-workflows/.github' \"$PUT\"" +check "both pins at the new SHA" "[[ \$(grep -cF '$NEW_SHA' \"$PUT\") -eq 2 ]]" + +echo "== a longer key ENDING in workflows_ref is not a stale pin ==" +# The assertion is looser than the rewrite by design, which makes it the one place +# a false positive hard-fails a clean repo. `upstream_workflows_ref: v1` is not +# this repo's input — rule 2 correctly leaves it alone — so reading it as an +# un-bumped github-workflows pin would block this caller's bump on every run. +new_case longkey +LONGKEY_FIXTURE="${WORK}/longkey_caller.yml" +printf '%s\n' \ + 'name: AGENTS.md Integrity' \ + 'jobs:' \ + ' agents-md:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/agents-md-integrity.yml@2222222222222222222222222222222222222222 # v1' \ + ' with:' \ + ' workflows_ref: 2222222222222222222222222222222222222222' \ + ' upstream_workflows_ref: v1' \ + > "$LONGKEY_FIXTURE" +STUB_CONTENT_FILE="$LONGKEY_FIXTURE" run_bump \ + VAR_NAME=AGENTS_MD_CALLERS TAG=agents-md-integrity WORKFLOW_FILE=agents-md-integrity.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-longkey","file":".github/workflows/agents-md-integrity.yml","label":""}]' +PUT="${STUB_PUT_DIR}/put.last.txt" +check "exit 0 — the foreign key did not trip it" "[[ $RC -eq 0 ]]" +check "staged the caller" "[[ \$(cat \"\$STUB_PUT_DIR/count\") -eq 1 ]]" +check "both real pins bumped" "[[ \$(grep -cF '$NEW_SHA' \"$PUT\") -eq 2 ]]" +check "foreign key's value left alone" "grep -qE '^[[:space:]]*upstream_workflows_ref:[[:space:]]*v1\$' \"$PUT\"" + +echo "== a '#' inside the ref cannot fake its way past the assertion ==" +# `#` is legal in a git ref name, and REF_RE stops at it, so rule 2 rewrites only +# the part before: `'feature#1'` -> `'#1'`. Stripping comments at the +# FIRST `#` would read that back as a bare NEW_SHA and accept the corrupted value; +# stripping by YAML's rule (whitespace-preceded `#`) keeps the value whole, so it +# compares unequal and fails the repo instead of shipping broken YAML. +new_case hashref +HASH_FIXTURE="${WORK}/hash_ref_caller.yml" +printf '%s\n' \ + 'name: AGENTS.md Integrity' \ + 'jobs:' \ + ' agents-md:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/agents-md-integrity.yml@2222222222222222222222222222222222222222 # v1' \ + ' with:' \ + " workflows_ref: 'feature#1'" \ + > "$HASH_FIXTURE" +STUB_CONTENT_FILE="$HASH_FIXTURE" run_bump \ + VAR_NAME=AGENTS_MD_CALLERS TAG=agents-md-integrity WORKFLOW_FILE=agents-md-integrity.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-hashref","file":".github/workflows/agents-md-integrity.yml","label":""}]' +check "exit 1 — the repo failed" "[[ $RC -eq 1 ]]" +check "warning names the half-rewritten value" "grep -qF '#1' <<<\"\$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 "== an EMPTY workflows_ref is a stale pin, not a blank to skip ==" +# `workflows_ref: \"\"` is a pin rule 2 cannot move either (REF_RE needs >=1 +# character), so dropping empty extracted values would let it slip past while +# `uses:` moved — the silent half-bump the assertion exists to catch. +new_case emptyref +EMPTY_FIXTURE="${WORK}/empty_ref_caller.yml" +printf '%s\n' \ + 'name: AGENTS.md Integrity' \ + 'jobs:' \ + ' agents-md:' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/agents-md-integrity.yml@2222222222222222222222222222222222222222 # v1' \ + ' with:' \ + ' workflows_ref: ""' \ + > "$EMPTY_FIXTURE" +STUB_CONTENT_FILE="$EMPTY_FIXTURE" run_bump \ + VAR_NAME=AGENTS_MD_CALLERS TAG=agents-md-integrity WORKFLOW_FILE=agents-md-integrity.yml ALLOW_EMPTY=true \ + CALLERS_JSON='[{"repo":"Comfy-Org/secret-emptyref","file":".github/workflows/agents-md-integrity.yml","label":""}]' +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 echo "== $PASS passed, $FAIL failed ==" [[ $FAIL -eq 0 ]]