Skip to content

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

Open
mattmillerai wants to merge 2 commits into
mainfrom
matt/be-6851-ci-groom-builder
Open

ci(groom): enable the auto-builder on this repo's own groom caller (BE-6851)#143
mattmillerai wants to merge 2 commits into
mainfrom
matt/be-6851-ci-groom-builder

Conversation

@mattmillerai

Copy link
Copy Markdown
Contributor

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/tests287 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.

…E-6851)

Flip ci-groom.yml from finds-only to the fleet-wide default: the top
CONFIRMED, non-security findings become review-gated PRs instead of
`groom` issues. Never auto-merged.

- `builder: true` — locked against vars.GROOM_CONFIG (config.py
  _LOCKED_KEYS), so turning it on is only ever a reviewed diff.
- `max_prs` reviewed fallback of 1 (BE-6017 sizing) plus a `choice`
  dispatch input, mirroring the reusable's documented caller pattern.
  The live ceiling stays vars.GROOM_CONFIG.max_prs, which outranks it.
- Header documents the App-identity precondition (satisfied and verified
  by behaviour), the pre-review-CI credential audit that makes a
  machine-authored PR safe here, and the patch-path policy's effect on a
  repo whose deliverable is .github/workflows/.

bot_app_id, the permission grant and both SHA pins are unchanged.
@mattmillerai mattmillerai added cursor-review Multi-model cursor review agent-coded Authored by the agent-work loop labels Aug 9, 2026
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 14 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e66470a6-e724-4ca7-a0fe-476c25a99455

📥 Commits

Reviewing files that changed from the base of the PR and between 46a96b9 and 8bffc71.

📒 Files selected for processing (1)
  • .github/workflows/ci-groom.yml

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 10 finding(s).

Severity Count
🟠 High 2
🟡 Medium 6
🟢 Low 1
⚪ Nit 1

Panel: 8/8 reviewers contributed findings.

Comment thread .github/workflows/ci-groom.yml
Comment thread .github/workflows/ci-groom.yml
Comment thread .github/workflows/ci-groom.yml
Comment thread .github/workflows/ci-groom.yml Outdated
Comment thread .github/workflows/ci-groom.yml Outdated
Comment thread .github/workflows/ci-groom.yml
Comment thread .github/workflows/ci-groom.yml
Comment thread .github/workflows/ci-groom.yml
Comment thread .github/workflows/ci-groom.yml
Comment thread .github/workflows/ci-groom.yml Outdated
Addresses the cursor-review panel on #143. Every fix is to the header's
security argument — the diff's actual behaviour is unchanged apart from
adding '0' to the max_prs options list.

Corrected, each verified against the pinned reusable at 0b6ebf5:

* cursor-review is NOT a "reads the diff as DATA" job. `build_pr` opens
  builder PRs with `--label cursor-review`, and BOTH the panel cells and
  the `consolidate` judge check out the PR and run `cursor-agent --print
  --trust` with CURSOR_API_KEY in env — the judge while holding
  `pull-requests: write`. Stated with its real blast radius, and with the
  honest delta: the builder removes the human who applied that label.
* `pr_size_limit` is NOT in `_LOCKED_KEYS` — it is the one operational
  knob that WIDENS by variable (BE-6345). Claim dropped.
* the "only one sink degrades" story is false under `builder: true`:
  `build_select`'s validate step exits 1 on an empty bot_app_id and
  `file` needs it, so an absent App token discards the whole run.
* `max_findings: 0` discards every non-built finding, security ones
  included, with no ledger marker — so they are re-billed daily at
  `interval_days: 1`. Spelled out rather than implied.
* noted that the patch policy's `.github/workflows/` deny still leaves
  the script dirs that pre-review CI executes, groom's own test suites
  among them.
* noted that `dry_run` is an operational key, so GROOM_CONFIG outranks
  the dispatch toggle.

Refuted, with evidence recorded inline so it is not re-raised:

* `github.event.inputs.dry_run == 'true'` is correct — that context
  carries a boolean input as a string. Run 30956077668 (a real dry_run
  dispatch) resolved `"dry_run": true` and filed nothing.
* the pin need not move: 0b6ebf5 is still main's tip for groom.yml and
  .github/groom/**, and carries BE-4404, BE-5227 and BE-6157.
* the max_prs expression cannot yield an empty string; the empty ->
  MAX_PRS_DEFAULT=5 path is real, so it is documented as a property to
  preserve. Its comment's `||`-truthiness rationale was wrong (a
  non-empty string is truthy, so '0' survives `||`) and is rewritten.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded Authored by the agent-work loop cursor-review Multi-model cursor review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants