[PR #93] [MERGED] feat(groom): GROOM_CONFIG — tune the groom loop from a repo variable (BE-5227) #102

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/93
Author: @mattmillerai
Created: 7/30/2026
Status: ✅ Merged
Merged: 7/31/2026
Merged by: @mattmillerai

Base: main ← Head: matt/groom-config-and-dedup

📝 Commits (4)

4aba6df feat(groom): GROOM_CONFIG resolution layer for operational knobs (BE-5227)
f9fe00d feat(groom): wire GROOM_CONFIG through the reusable + add a paths knob (BE-5227)
9ac79d8 fix(groom): harden the GROOM_CONFIG coercers against the fail-open holes (BE-5227)
a0ae5db refactor(groom): drop the competing paths knob — #83 owns path scoping

📊 Changes

3 files changed (+870 additions, -21 deletions)

View changed files

➕ .github/groom/config.py (+440 -0)
➕ .github/groom/tests/test_config.py (+309 -0)
📝 .github/workflows/groom.yml (+121 -21)

📄 Description

ELI-5

Groom has about a dozen dials — how often it runs, how many PRs it may open, what it looks at, whether it files anything at all. Today every one of those lives in a workflow file inside each consuming repo, so turning a dial means opening a PR in that repo and waiting for review. Turning a dial should not need a code review.

This reads those dials from a single JSON repo variable instead, layered on top of whatever the workflow file already says. A repo that sets nothing keeps behaving exactly as it does today. A repo that sets the variable gets its new values on the next run, with no PR.

What is here

Both halves — the resolver and the groom.yml wiring that consumes it.

.github/groom/config.py — resolves the knobs. Precedence, lowest first: reusable defaults < caller with: < vars.GROOM_CONFIG < the config input.
.github/groom/tests/test_config.py — 50 tests. Full groom suite green on the CI matrix.
.github/workflows/groom.yml — the config input, the resolve step in gate, and the resolved output threaded through the downstream jobs. Every consuming job now reads its knobs via fromJSON(needs.gate.outputs.resolved).<knob> instead of inputs.<knob>, so the merge happens in exactly one place.

Merging this does change behavior for existing callers — every knob read is rerouted through the resolver. A caller that sets no variable resolves to its current with: values, so the intended net effect is nil, but the code path is new and that is what review should be aimed at.

Design notes worth reviewing

There is deliberately no workflow_dispatch layer. A reusable workflow cannot see the caller's dispatch inputs: inside a called workflow inputs is that workflow's own inputs, and github.event.inputs is the caller's payload only for the caller's own jobs. A caller forwarding a dispatch input does it per-knob (max_prs: ${{ github.event.inputs.max_prs || '3' }}), which lands in the baseline below the variable. Consequence worth stating plainly: once GROOM_CONFIG sets a knob, that variable wins and a per-knob dispatch override of the same knob silently stops taking effect. A caller that wants one-run overrides to keep winning forwards a JSON blob into config, which ranks above the variable.

vars.GROOM_CONFIG is read inside the reusable, not passed in. The vars context in a called workflow resolves against the caller's repository, so every consumer inherits variable-driven config with no with: change — only a pin bump, which the bump-groom-callers fleet already automates. (Verified in-org: assign-reviewers.yml reads vars.REVIEWER_LOAD_CAP, set on cloud and not on this repo.)

It cannot touch the security boundary. An Actions variable is editable by anyone with repo write — the same principals who could edit the workflow file — but editing it bypasses PR review and leaves no diff on a branch. So builder, pr_size_limit, sink, bot_app_id, workflows_ref and config itself are refused with a loud warning rather than silently dropped. config is in that list so a variable cannot nominate its own higher-precedence layer. There is a test asserting every one of them is unreachable, so this cannot regress quietly.

Fail-open, everywhere. Malformed JSON warns and falls back to the layer below. A typo in a variable must not turn a daily scheduled sweep red until somebody notices. Each individual drop is warned about with the key named.

Refuses to guess. "yes", 1 and "on" are all rejected for a boolean rather than coerced — reading "yes" as false would quietly re-enable live filing. Same for bool where a number is expected, since bool is an int subclass in Python.

scope_label is settable but announces itself. It is field 2 of every dedup signature, so changing it re-keys the entire ledger and groom re-proposes its whole backlog once. Legitimate for a genuine re-scope, never something you want by accident, so it warns every time.

One output, not eleven. Job outputs are strings, and two knobs are multi-line prose — eleven separate outputs would mean heredoc delimiter plumbing per knob plus an env: line in every consuming job. The resolved set is emitted as one compact JSON line.

max_prs / interval_days / cadence stay strings on purpose. Each already has a downstream parser that owns clamping and fail-open degradation (build_select and interval.py). Re-clamping here would create two places to disagree.

Known leftover, flagged for this review

config.py still exposes a --dispatch-json argument, and resolve() still takes a dispatch_raw layer labelled workflow_dispatch inputs. It is a vestige of the original design and the shipped wiring never passes it — groom.yml instead makes two --config-json passes to order variable-then-config. It is inert (defaults to ""), but the label names a layer that architecturally cannot exist, which is exactly the sort of thing that misleads the next reader. Worth deciding in review whether to drop it or keep it as the seam a non-reusable caller could use.

Not in this PR

paths — an earlier revision carried a competing path-scoping knob. It was dropped rather than merged into a second implementation: #83 owns path scoping via a path input backed by scope.py, and it also teaches interval.py not to reset the cadence when scope narrows. There is a test asserting paths is not a config knob. Once #83 lands, adding path to _OPERATIONAL_KEYS and to the DEFAULTS block makes it variable-tunable with no further wiring.
BE-5228: the dedup signature is derived from the LLM prose title, so groom re-proposes its own findings — confirmed 3x on comfy-cloud-mcp-server and 2x on cloud.
a setup_command input, so the builder job has a toolchain and can verify its patch instead of bailing.

Testing

python3 -m unittest discover -s .github/groom/tests -p "test_*.py" — green locally on 3.14, and the CI matrix job (3.12) is green on this PR, which is the real check for version skew. Nothing in either file uses post-3.12 syntax.

_{🔄 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/93 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/30/2026 **Status:** ✅ Merged **Merged:** 7/31/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/groom-config-and-dedup` --- ### 📝 Commits (4) - [`4aba6df`](https://github.com/Comfy-Org/github-workflows/commit/4aba6df925337bcea86da1ea7627921aa1d9920a) feat(groom): GROOM_CONFIG resolution layer for operational knobs (BE-5227) - [`f9fe00d`](https://github.com/Comfy-Org/github-workflows/commit/f9fe00d4c3033f241f320ce3aa23f5de8b12cc0a) feat(groom): wire GROOM_CONFIG through the reusable + add a paths knob (BE-5227) - [`9ac79d8`](https://github.com/Comfy-Org/github-workflows/commit/9ac79d84a4b10e47cb3a7402162af3a29f291125) fix(groom): harden the GROOM_CONFIG coercers against the fail-open holes (BE-5227) - [`a0ae5db`](https://github.com/Comfy-Org/github-workflows/commit/a0ae5dbf530aca73906a023497d4d8debd82335f) refactor(groom): drop the competing paths knob — #83 owns path scoping ### 📊 Changes **3 files changed** (+870 additions, -21 deletions) <details> <summary>View changed files</summary> ➕ `.github/groom/config.py` (+440 -0) ➕ `.github/groom/tests/test_config.py` (+309 -0) 📝 `.github/workflows/groom.yml` (+121 -21) </details> ### 📄 Description ## ELI-5 Groom has about a dozen dials — how often it runs, how many PRs it may open, what it looks at, whether it files anything at all. Today every one of those lives in a workflow file inside each consuming repo, so turning a dial means opening a PR in that repo and waiting for review. Turning a dial should not need a code review. This reads those dials from a single JSON repo variable instead, layered on top of whatever the workflow file already says. A repo that sets nothing keeps behaving exactly as it does today. A repo that sets the variable gets its new values on the next run, with no PR. ## What is here Both halves — the resolver **and** the `groom.yml` wiring that consumes it. - `.github/groom/config.py` — resolves the knobs. Precedence, lowest first: reusable defaults < caller `with:` < `vars.GROOM_CONFIG` < the `config` input. - `.github/groom/tests/test_config.py` — 50 tests. Full groom suite green on the CI matrix. - `.github/workflows/groom.yml` — the `config` input, the `resolve` step in `gate`, and the `resolved` output threaded through the downstream jobs. Every consuming job now reads its knobs via `fromJSON(needs.gate.outputs.resolved).<knob>` instead of `inputs.<knob>`, so the merge happens in exactly one place. **Merging this does change behavior for existing callers** — every knob read is rerouted through the resolver. A caller that sets no variable resolves to its current `with:` values, so the intended net effect is nil, but the code path is new and that is what review should be aimed at. ## Design notes worth reviewing **There is deliberately no `workflow_dispatch` layer.** A reusable workflow cannot see the caller's dispatch inputs: inside a called workflow `inputs` is that workflow's own inputs, and `github.event.inputs` is the caller's payload only for the caller's own jobs. A caller forwarding a dispatch input does it per-knob (`max_prs: ${{ github.event.inputs.max_prs || '3' }}`), which lands in the baseline **below** the variable. Consequence worth stating plainly: **once `GROOM_CONFIG` sets a knob, that variable wins and a per-knob dispatch override of the same knob silently stops taking effect.** A caller that wants one-run overrides to keep winning forwards a JSON blob into `config`, which ranks above the variable. **`vars.GROOM_CONFIG` is read inside the reusable, not passed in.** The `vars` context in a called workflow resolves against the *caller's* repository, so every consumer inherits variable-driven config with no `with:` change — only a pin bump, which the `bump-groom-callers` fleet already automates. (Verified in-org: `assign-reviewers.yml` reads `vars.REVIEWER_LOAD_CAP`, set on `cloud` and not on this repo.) **It cannot touch the security boundary.** An Actions variable is editable by anyone with repo write — the same principals who could edit the workflow file — but editing it bypasses PR review and leaves no diff on a branch. So `builder`, `pr_size_limit`, `sink`, `bot_app_id`, `workflows_ref` and `config` itself are refused with a loud warning rather than silently dropped. `config` is in that list so a variable cannot nominate its own higher-precedence layer. There is a test asserting every one of them is unreachable, so this cannot regress quietly. **Fail-open, everywhere.** Malformed JSON warns and falls back to the layer below. A typo in a variable must not turn a daily scheduled sweep red until somebody notices. Each individual drop is warned about with the key named. **Refuses to guess.** `"yes"`, `1` and `"on"` are all rejected for a boolean rather than coerced — reading `"yes"` as false would quietly re-enable live filing. Same for `bool` where a number is expected, since `bool` is an `int` subclass in Python. **`scope_label` is settable but announces itself.** It is field 2 of every dedup signature, so changing it re-keys the entire ledger and groom re-proposes its whole backlog once. Legitimate for a genuine re-scope, never something you want by accident, so it warns every time. **One output, not eleven.** Job outputs are strings, and two knobs are multi-line prose — eleven separate outputs would mean heredoc delimiter plumbing per knob plus an `env:` line in every consuming job. The resolved set is emitted as one compact JSON line. **`max_prs` / `interval_days` / `cadence` stay strings on purpose.** Each already has a downstream parser that owns clamping and fail-open degradation (`build_select` and `interval.py`). Re-clamping here would create two places to disagree. ## Known leftover, flagged for this review `config.py` still exposes a `--dispatch-json` argument, and `resolve()` still takes a `dispatch_raw` layer labelled `workflow_dispatch inputs`. It is a vestige of the original design and the shipped wiring never passes it — `groom.yml` instead makes two `--config-json` passes to order variable-then-`config`. It is inert (defaults to `""`), but the label names a layer that architecturally cannot exist, which is exactly the sort of thing that misleads the next reader. Worth deciding in review whether to drop it or keep it as the seam a non-reusable caller could use. ## Not in this PR - `paths` — an earlier revision carried a competing path-scoping knob. It was **dropped** rather than merged into a second implementation: #83 owns path scoping via a `path` input backed by `scope.py`, and it also teaches `interval.py` not to reset the cadence when scope narrows. There is a test asserting `paths` is not a config knob. Once #83 lands, adding `path` to `_OPERATIONAL_KEYS` and to the `DEFAULTS` block makes it variable-tunable with no further wiring. - BE-5228: the dedup signature is derived from the LLM prose title, so groom re-proposes its own findings — confirmed 3x on comfy-cloud-mcp-server and 2x on cloud. - a `setup_command` input, so the builder job has a toolchain and can verify its patch instead of bailing. ## Testing `python3 -m unittest discover -s .github/groom/tests -p "test_*.py"` — green locally on 3.14, and the CI matrix job (3.12) is green on this PR, which is the real check for version skew. Nothing in either file uses post-3.12 syntax. --- <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:15 +00:00