[PR #143] [MERGED] ci(groom): enable the auto-builder on this repo's own groom caller (BE-6851) #152

New issue

Closed

opened 2026-10-26 22:36:49 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:49 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/143
Author: @mattmillerai
Created: 8/9/2026
Status: ✅ Merged
Merged: 8/12/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-6851-ci-groom-builder

📝 Commits (4)

451c220 ci(groom): enable the auto-builder on this repo's own groom caller (BE-6851)
8bffc71 ci(groom): correct the builder safety audit's false claims (BE-6851)
52aa0d0 Merge branch 'main' into matt/be-6851-ci-groom-builder
f410e62 ci: nudge — retrigger checks after CI apparently never queued for 52aa0d0

📊 Changes

1 file changed (+269 additions, -34 deletions)

View changed files

📝 .github/workflows/ci-groom.yml (+269 -34)

📄 Description

ELI-5

Groom already scans this repo every day and writes up the cleanups it finds. Until now it could only tell us about them. This turns on the part that also does them: the best couple of findings each run come back as normal pull requests instead of issues. Nothing merges itself — a machine-written PR sits in review like anyone else's, and this repo's own CI holds no secrets on the checks that run a PR's code, so a bad patch can waste a review and nothing more.

What changed

One file, .github/workflows/ci-groom.yml:

builder: true in the with: block. This knob is in config.py's _LOCKED_KEYS, so it is not settable from vars.GROOM_CONFIG — turning the auto-builder on can only ever be a reviewed diff, which is what this PR is.
max_prs reviewed fallback of 1, plus a workflow_dispatch max_prs choice input (['1','2','3','5'], default '1') mirroring the reusable's documented caller pattern and the comfy-infra caller.
Header comment rewritten to document the App-identity precondition, the credentialing gotchas, the pre-review-CI audit, and the first-tick dry-run instruction.

bot_app_id: ${{ vars.APP_ID }}, the permissions: grant, and both SHA pins (uses: + workflows_ref, 0b6ebf5) are unchanged.

What I verified against the pinned SHA (not `main`)

The pin is groom.yml@0b6ebf5e460ca02499e362aa8d650ac4d6806a7c, so that commit — not origin/main — is what actually runs. Checked there:

builder and max_prs both exist in on.workflow_call.inputs, so neither is an unknown-input startup rejection.
No permission change is needed. Every nested job's declared GITHUB_TOKEN scope at that SHA fits inside the caller's existing grant: build and build_pr are contents: read only, build_select is contents/issues/pull-requests: read, file is issues: write, gate adds actions: read. build_pr does all its writing with the App installation token, which is why it needs no pull-requests: write from the caller. (A reviewer's first instinct here is "the builder opens PRs, surely it needs write" — it does, just not from this token.)
_LOCKED_KEYS = ("builder", "sink", "bot_app_id", "workflows_ref", "config"), so builder genuinely cannot be flipped by a variable edit.
patch_policy.py's _ROOT_PREFIXES denies .github/workflows/ and .github/actions/.
The dry-run path exists in build_pr and the patch artifact is groom-build-<idx>.

Locally: python3 -m unittest discover -s .github/groom/tests → 287 passed (1 skip); python3 .github/agents-md-integrity/check_agents_md.py --root . → passed with the two pre-existing warnings (AGENTS.md 186 lines, no CODEOWNERS); actionlint .github/workflows/ci-groom.yml → clean. No test workflow path-filters on ci-groom.yml, so CI will run no unit job for this PR — that is the repo's path-filtered design, not a gap.

The security question, asked properly

"A human reads the PR first" gates the merge, not CI execution — opening a PR runs this repo's checks against the PR's own checkout before anyone reads the diff. So: does any pre-review check here hold credentials? Swept every workflow's triggers; exactly nine are pull_request-triggered:

the seven test-*.yml suites do execute the PR's checkout (unittest + shellcheck over .github/**) and every one is permissions: contents: read with zero secrets: — the ordinary untrusted-PR-CI posture of any public repo.
the two credentialed ones — ci-cursor-review.yml (CURSOR_API_KEY, SLACK_BOT_TOKEN, pull-requests: write) and ci-assign-reviewers.yml (App key) — never execute the PR's code; both pin the reusable by merged-main SHA and it loads its scripts from that pinned ref, so a PR can't rewrite the logic judging it.

Stated honestly rather than favourably: cursor-review does check out the PR to read the diff, so machine-written text reaches an LLM in a credentialed job as data. That is the same prompt-injection surface any outside contributor's PR already carries here — unchanged in kind by this flag, only raised in volume. The header records the condition that would invalidate this analysis (a credentialed workflow starting to execute the tree) and says the caller reverts to builder: false if it ever holds.

Repo-specific consequence worth knowing before the dry run: patch_policy.py denies .github/workflows/, and this repo's deliverable is .github/workflows/. So a finding in a workflow file cannot become a PR here — it bails to a groom issue. Builder PRs will come from the script directories and the docs. Bail issues are exempt from max_findings (BE-6157), so nothing is discarded.

Judgment calls (please sanity-check these three)

max_prs: 1 is the reviewed fallback, not the live value. vars.GROOM_CONFIG is already set on this repo to {"interval_days":"1","cadence":"1","max_prs":"2","max_findings":0}, and it outranks every with: input. So the effective ceiling on merge is 2, not the 1 this PR writes. I did not edit the variable: it was set deliberately on 2026-08-04, before this ticket, and overriding a human's staged config out-of-band is worse than flagging it. If you want a literal 1, it is one command and no PR: gh variable set GROOM_CONFIG --repo Comfy-Org/github-workflows --body '{"interval_days":"1","cadence":"1","max_prs":"1","max_findings":0}'.
The dispatch max_prs input is inert today, for the same precedence reason (a reusable can't read its caller's dispatch inputs; beating the variable would mean forwarding it through the reusable's config JSON input instead). Added anyway for parity with the other builder callers and so the knob exists the moment the variable stops pinning it. Same call the comfy-infra caller made.
== '' instead of the ticket's || '1'. GitHub casts a string to a number for || truthiness and '0' casts to 0, which is falsy — so the || form silently turns a deliberate "build no PRs this run" dispatch into 1. The empty-test does a same-type string comparison on a dispatch (so '0' survives) and a null-vs-string comparison on the schedule event (both coerce to 0 → the reviewed fallback). Same reasoning the comfy-infra caller wrote down. Worst case if I have GitHub's coercion wrong, it behaves exactly like the || form, so it is not a downside bet.

Deliberately not changed: no max_findings reviewed baseline was added (comfy-infra added one). Clearing GROOM_CONFIG would degrade this caller to the reusable's default of 12 findings issues — which is this caller's pre-existing behaviour, so adding one would be a scope-widening change, not a fix. Separately spotted and left alone: docs/callers/groom.md still lists pr_size_limit's default as 400; it became 600 in BE-6161. Pre-existing doc drift, unrelated to this diff.

Not met by this PR

The ticket's acceptance has a post-merge half that cannot be done from a PR branch — a scheduled/dispatch caller does not run on a PR. After merge, run the dry run before the next 09:17 UTC tick, because the daily cron resolves dry_run to false and would otherwise be the first unattended live builder run:

gh workflow run ci-groom.yml --repo Comfy-Org/github-workflows -f dry_run=true

Expected: a groom-build-<idx> artifact per selected finding and zero PRs opened.

Negative-claim falsification

Not applicable, and checked rather than assumed: this diff denies no capability. It adds no "not supported"/"unavailable"/STOP string, no throw or deny dead-end, and flips no test to assert one — it enables the PR sink. The only path it narrows is documentation of an existing deny (patch_policy.py's workflow-path bail), which is pre-existing reusable behaviour verified at the pinned SHA, not something this diff introduces.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/143 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/9/2026 **Status:** ✅ Merged **Merged:** 8/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6851-ci-groom-builder` --- ### 📝 Commits (4) - [`451c220`](https://github.com/Comfy-Org/github-workflows/commit/451c2200b2e92a5fb73a3dae4a1f9517cf5cb1fa) ci(groom): enable the auto-builder on this repo's own groom caller (BE-6851) - [`8bffc71`](https://github.com/Comfy-Org/github-workflows/commit/8bffc716cefd1f2ceb29c1d1868deb2670128404) ci(groom): correct the builder safety audit's false claims (BE-6851) - [`52aa0d0`](https://github.com/Comfy-Org/github-workflows/commit/52aa0d0a2eee807e057a53549f1dbf3028ea0961) Merge branch 'main' into matt/be-6851-ci-groom-builder - [`f410e62`](https://github.com/Comfy-Org/github-workflows/commit/f410e6236c18be6c6a4c92e2a73f6511a21a6e40) ci: nudge — retrigger checks after CI apparently never queued for 52aa0d0 ### 📊 Changes **1 file changed** (+269 additions, -34 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci-groom.yml` (+269 -34) </details> ### 📄 Description ## ELI-5 Groom already scans this repo every day and writes up the cleanups it finds. Until now it could only *tell* us about them. This turns on the part that also *does* them: the best couple of findings each run come back as normal pull requests instead of issues. Nothing merges itself — a machine-written PR sits in review like anyone else's, and this repo's own CI holds no secrets on the checks that run a PR's code, so a bad patch can waste a review and nothing more. ## What changed One file, `.github/workflows/ci-groom.yml`: - `builder: true` in the `with:` block. This knob is in `config.py`'s `_LOCKED_KEYS`, so it is *not* settable from `vars.GROOM_CONFIG` — turning the auto-builder on can only ever be a reviewed diff, which is what this PR is. - `max_prs` reviewed fallback of `1`, plus a `workflow_dispatch` `max_prs` choice input (`['1','2','3','5']`, default `'1'`) mirroring the reusable's documented caller pattern and the comfy-infra caller. - Header comment rewritten to document the App-identity precondition, the credentialing gotchas, the pre-review-CI audit, and the first-tick dry-run instruction. `bot_app_id: ${{ vars.APP_ID }}`, the `permissions:` grant, and both SHA pins (`uses:` + `workflows_ref`, `0b6ebf5`) are **unchanged**. ## What I verified against the pinned SHA (not `main`) The pin is `groom.yml@0b6ebf5e460ca02499e362aa8d650ac4d6806a7c`, so that commit — not `origin/main` — is what actually runs. Checked there: - `builder` and `max_prs` both exist in `on.workflow_call.inputs`, so neither is an unknown-input startup rejection. - **No permission change is needed.** Every nested job's declared GITHUB_TOKEN scope at that SHA fits inside the caller's existing grant: `build` and `build_pr` are `contents: read` only, `build_select` is `contents/issues/pull-requests: read`, `file` is `issues: write`, `gate` adds `actions: read`. `build_pr` does all its writing with the App installation token, which is why it needs no `pull-requests: write` from the caller. (A reviewer's first instinct here is "the builder opens PRs, surely it needs write" — it does, just not from this token.) - `_LOCKED_KEYS = ("builder", "sink", "bot_app_id", "workflows_ref", "config")`, so `builder` genuinely cannot be flipped by a variable edit. - `patch_policy.py`'s `_ROOT_PREFIXES` denies `.github/workflows/` and `.github/actions/`. - The dry-run path exists in `build_pr` and the patch artifact is `groom-build-<idx>`. Locally: `python3 -m unittest discover -s .github/groom/tests` → **287 passed** (1 skip); `python3 .github/agents-md-integrity/check_agents_md.py --root .` → passed with the two pre-existing warnings (AGENTS.md 186 lines, no CODEOWNERS); `actionlint .github/workflows/ci-groom.yml` → clean. No test workflow path-filters on `ci-groom.yml`, so CI will run no unit job for this PR — that is the repo's path-filtered design, not a gap. ## The security question, asked properly "A human reads the PR first" gates the **merge**, not CI **execution** — opening a PR runs this repo's checks against the PR's own checkout before anyone reads the diff. So: does any pre-review check here hold credentials? Swept every workflow's triggers; exactly nine are `pull_request`-triggered: - the seven `test-*.yml` suites **do** execute the PR's checkout (unittest + shellcheck over `.github/**`) and every one is `permissions: contents: read` with **zero** `secrets:` — the ordinary untrusted-PR-CI posture of any public repo. - the two credentialed ones — `ci-cursor-review.yml` (CURSOR_API_KEY, SLACK_BOT_TOKEN, `pull-requests: write`) and `ci-assign-reviewers.yml` (App key) — never *execute* the PR's code; both pin the reusable by merged-main SHA and it loads its scripts from that pinned ref, so a PR can't rewrite the logic judging it. Stated honestly rather than favourably: cursor-review *does* check out the PR to read the diff, so machine-written text reaches an LLM in a credentialed job as **data**. That is the same prompt-injection surface any outside contributor's PR already carries here — unchanged in kind by this flag, only raised in volume. The header records the condition that would invalidate this analysis (a credentialed workflow starting to *execute* the tree) and says the caller reverts to `builder: false` if it ever holds. Repo-specific consequence worth knowing before the dry run: `patch_policy.py` denies `.github/workflows/`, and this repo's deliverable **is** `.github/workflows/`. So a finding in a workflow file cannot become a PR here — it bails to a `groom` issue. Builder PRs will come from the script directories and the docs. Bail issues are exempt from `max_findings` (BE-6157), so nothing is discarded. ## Judgment calls (please sanity-check these three) 1. **`max_prs: 1` is the reviewed fallback, not the live value.** `vars.GROOM_CONFIG` is already set on this repo to `{"interval_days":"1","cadence":"1","max_prs":"2","max_findings":0}`, and it **outranks** every `with:` input. So the effective ceiling on merge is **2**, not the 1 this PR writes. I did not edit the variable: it was set deliberately on 2026-08-04, before this ticket, and overriding a human's staged config out-of-band is worse than flagging it. If you want a literal 1, it is one command and no PR: `gh variable set GROOM_CONFIG --repo Comfy-Org/github-workflows --body '{"interval_days":"1","cadence":"1","max_prs":"1","max_findings":0}'`. 2. **The dispatch `max_prs` input is inert today**, for the same precedence reason (a reusable can't read its caller's dispatch inputs; beating the variable would mean forwarding it through the reusable's `config` JSON input instead). Added anyway for parity with the other builder callers and so the knob exists the moment the variable stops pinning it. Same call the comfy-infra caller made. 3. **`== ''` instead of the ticket's `|| '1'`.** GitHub casts a string to a number for `||` truthiness and `'0'` casts to `0`, which is falsy — so the `||` form silently turns a deliberate "build no PRs this run" dispatch into 1. The empty-test does a same-type string comparison on a dispatch (so `'0'` survives) and a null-vs-string comparison on the schedule event (both coerce to 0 → the reviewed fallback). Same reasoning the comfy-infra caller wrote down. Worst case if I have GitHub's coercion wrong, it behaves exactly like the `||` form, so it is not a downside bet. Deliberately **not** changed: no `max_findings` reviewed baseline was added (comfy-infra added one). Clearing `GROOM_CONFIG` would degrade this caller to the reusable's default of 12 findings issues — which is this caller's pre-existing behaviour, so adding one would be a scope-widening change, not a fix. Separately spotted and left alone: `docs/callers/groom.md` still lists `pr_size_limit`'s default as 400; it became 600 in BE-6161. Pre-existing doc drift, unrelated to this diff. ## Not met by this PR The ticket's acceptance has a **post-merge** half that cannot be done from a PR branch — a scheduled/dispatch caller does not run on a PR. After merge, run the dry run **before the next 09:17 UTC tick**, because the daily cron resolves `dry_run` to false and would otherwise be the first unattended live builder run: ``` gh workflow run ci-groom.yml --repo Comfy-Org/github-workflows -f dry_run=true ``` Expected: a `groom-build-<idx>` artifact per selected finding and **zero** PRs opened. ## Negative-claim falsification Not applicable, and checked rather than assumed: this diff denies no capability. It adds no "not supported"/"unavailable"/`STOP` string, no throw or deny dead-end, and flips no test to assert one — it *enables* the PR sink. The only path it narrows is documentation of an existing deny (`patch_policy.py`'s workflow-path bail), which is pre-existing reusable behaviour verified at the pinned SHA, not something this diff introduces. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:49 +00:00