From 62fa54cf9c4546690f3dedc852d159227c56a6bf Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 26 Jul 2026 00:23:42 -0700 Subject: [PATCH 1/3] 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/3] 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/3] 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).