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 91146a8..a9e51c9 100755 --- a/.github/bump-callers/bump-callers.sh +++ b/.github/bump-callers/bump-callers.sh @@ -234,7 +234,39 @@ bump_repo() { if (( GW_HAS_SIBLING )); then SHA_ADDR="/github-workflows[^[:space:]]*${WORKFLOW_FILE//./\\.}|workflows_ref/" fi - NEW_CONTENT=$(sed -E "${SHA_ADDR} s/[0-9a-f]{40}/${NEW_SHA}/g" <<<"$OLD_CONTENT") + # + # The groom callers' `# main @ ` annotation rides on the pin line + # itself, so it moves HERE, under the SAME `${SHA_ADDR}` — the address is the + # exact set of lines whose pin this rule rewrites, which makes it honest + # attribution for that form (the `main ()` form below cannot use it: it + # also appears in prose ABOVE the `uses:` line, naming no pin context of its + # own, so it needs the caller-level guard instead). The two MUST stay + # identical: a groom caller pins TWICE and its `workflows_ref: # main @ + # ` line is rewritten by the 40-hex rule, 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 rewrites exist to kill. Sharing the address also means the sibling + # tightening covers this form for free: a sibling fleet's `uses: …/other.yml@… + # # main @ ` line is outside the tightened address, so neither its pin + # nor its annotation is stamped with our SHA. + # + # The short form is bounded to {7,12} hex so a deliberate FULL-sha annotation + # keeps its full form (the 40-hex rule has already corrected it in the same + # pass) instead of being shortened — and that bound only holds if the hex run + # ENDS there, hence 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 it would swap 12 hex for the 7-char SHORT and strand the remaining 28 + # — 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 2 catches the short form mid-line, rule 3 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 " + ${SHA_ADDR} s/[0-9a-f]{40}/${NEW_SHA}/g + ${SHA_ADDR} s|# main @ [0-9a-f]{7,12}([^0-9a-f])|# main @ ${SHORT}\1|g + ${SHA_ADDR} s|# main @ [0-9a-f]{7,12}\$|# main @ ${SHORT}|g + " <<<"$OLD_CONTENT") # Normalize the human-readable pin annotation so it never disagrees with the # pin above. Two spellings, both handled here: @@ -254,13 +286,16 @@ bump_repo() { # clean-skip path still skips). A caller using some other comment form # (agents-md-integrity's `# v1`) matches neither rule and is untouched. # - # Attribution guard: an annotation names a SHA but not WHICH reusable it - # annotates, so in a file that calls several github-workflows reusables there - # is no honest way to tell whose is whose — rewriting them all would stamp - # this fleet's SHA onto a sibling fleet's annotation. So rewrite only when the - # file is provably ours alone; otherwise leave every annotation as found and - # say so. (No caller does this today; the guard is why the new rule cannot - # regress one that starts to.) + # Attribution guard: an annotation of these two forms names a SHA but not + # WHICH reusable it annotates — and, appearing in prose above the pin as often + # as beside it, it carries no line context to attribute it by either. So in a + # file that calls several github-workflows reusables there is no honest way to + # tell whose is whose, and rewriting them all would stamp this fleet's SHA + # onto a sibling fleet's annotation. Rewrite only when the file is provably + # ours alone; otherwise leave every annotation as found and say so. (No caller + # does this today; the guard is why the new rule cannot regress one that starts + # to.) The `# main @` form needs no guard here — it is bound to the pin line it + # annotates and moved above, under the same address as the pin itself. if (( GW_ONLY_OURS )); then NEW_CONTENT=$(sed -E "s|# github-workflows#[0-9]+|# github-workflows main (${SHORT})|g; s|github-workflows main \([0-9a-f]{7,40}\)|github-workflows main (${SHORT})|g" <<<"$NEW_CONTENT") elif grep -qE '# github-workflows#[0-9]+|github-workflows main \([0-9a-f]{7,40}\)' <<<"$OLD_CONTENT"; then diff --git a/.github/bump-callers/tests/test_bump_callers.sh b/.github/bump-callers/tests/test_bump_callers.sh index 79f25a8..a63e6e8 100755 --- a/.github/bump-callers/tests/test_bump_callers.sh +++ b/.github/bump-callers/tests/test_bump_callers.sh @@ -213,6 +213,64 @@ 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 # 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 \ + 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 `# 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 `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). +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 \ @@ -548,7 +606,9 @@ printf '%s\n' \ ' review:' \ ' uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@1111111111111111111111111111111111111111 # github-workflows main (1111111)' \ ' auto-label:' \ - ' uses: Comfy-Org/github-workflows/.github/workflows/cursor-review-auto-label.yml@2222222222222222222222222222222222222222 # github-workflows#27' \ + ' uses: Comfy-Org/github-workflows/.github/workflows/cursor-review-auto-label.yml@2222222222222222222222222222222222222222 # github-workflows#27 # main @ 2222222' \ + ' with:' \ + ' workflows_ref: 1111111111111111111111111111111111111111 # main @ 1111111' \ > "$MULTI_FIXTURE" STUB_CONTENT_FILE="$MULTI_FIXTURE" run_bump \ VAR_NAME=CURSOR_REVIEW_CALLERS TAG=cursor-review WORKFLOW_FILE=cursor-review.yml \ @@ -561,6 +621,22 @@ check "the SIBLING's 40-hex pin is UNCHANGED" "grep -qF 'cursor-review-auto-l check "the sibling's legacy marker is untouched" "grep -qF '# github-workflows#27' \"$PUT\"" check "our own marker left alone (ambiguous)" "grep -qF 'github-workflows main (1111111)' \"$PUT\"" check "this fleet's short SHA was NOT stamped in" "! grep -qF 'github-workflows main ($SHORT)' \"$PUT\"" +# The `# main @ ` form is attributed differently from the two markers above: +# it rides ON the pin line, so it moves with whatever the 40-hex rule moves — same +# address, both regimes. That makes the multi-reusable case a two-sided assertion: +# * the SIBLING's `# main @` note shares its line with a pin the tightened address +# deliberately does NOT bump, so stamping the note would be the same cross-fleet +# lie as stamping the pin, and +# * a `workflows_ref:` pin IS bumped even here (that context stays broad — an +# un-bumped workflows_ref disagreeing with its own `uses:` is the worse +# failure), so its own note MUST move with it or the guard manufactures exactly +# the stale comment BE-4346 removed. +# Unreachable today (no caller calls two reusables); asserted so the guard and the +# groom fleet's lock-step cannot silently trade one for the other. +check "sibling's '# main @' note NOT stamped" "grep -qF '# main @ 2222222' \"$PUT\"" +check "workflows_ref pin bumped even here" "grep -qE \"workflows_ref: ${NEW_SHA}\" \"$PUT\"" +check "workflows_ref's '# main @' moved with it" \ + "grep -qE \"workflows_ref: ${NEW_SHA} +# main @ ${SHORT}\\\$\" \"$PUT\"" echo "== merely NAMING a sibling workflow in a comment is not a second caller (BE-4523) ==" # The multi-reusable check reads `uses:` lines only. A single-reusable caller that diff --git a/.github/workflows/bump-groom-callers.yml b/.github/workflows/bump-groom-callers.yml new file mode 100644 index 0000000..c23d8c8 --- /dev/null +++ b/.github/workflows/bump-groom-callers.yml @@ -0,0 +1,145 @@ +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: + # Guard against a workflow_dispatch run from a non-main ref: github.sha is + # the tip of whatever ref was selected, and the bumper force-resets a stable + # shared branch — a manual run from an older ref would pin every caller to a + # stale SHA (and run this job's script, with the App token minted below, + # from an unreviewed commit). The push trigger is already main-only, so this + # only ever skips stray manual runs. + if: github.ref == 'refs/heads/main' + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + 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:` 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: + 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: | + # The main-only ref guard above cannot catch a manual RE-RUN of an + # older main run: github.ref is refs/heads/main but github.sha is + # that run's original (now stale) commit, and the bumper would + # force-repin every caller to it. Only act from the current tip — + # any newer push has its own queued run that does the bump. + main_tip=$(git ls-remote origin refs/heads/main | cut -f1) + if [[ "$main_tip" != "$GITHUB_SHA" ]]; then + echo "github.sha $GITHUB_SHA is not the current main tip ($main_tip) — stale run/re-run; nothing to bump" + exit 0 + fi + # The push path filter also matches a commit that DELETES the reusable + # workflow — or the briefs/ledger callers load at run time from the + # pinned SHA (.github/groom/**); bumping callers to a SHA where either + # is gone would break every caller. Deletion means decommissioning — + # no-op. Both are checked because a groom caller pins BOTH (`uses:` and + # `workflows_ref:`), so either one missing is a broken pin. + if [[ ! -f .github/workflows/groom.yml || ! -d .github/groom ]]; then + echo "groom.yml or .github/groom/ absent at this SHA — decommissioned; nothing to bump" + exit 0 + fi + bash .github/bump-callers/bump-callers.sh diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index fdbfd3b..4284f98 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 }} @@ -206,9 +223,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 @@ -1007,13 +1057,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 4a41c18..42dc18d 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