[PR #121] [MERGED] feat(groom): make pr_size_limit settable from GROOM_CONFIG #133

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-6345-groom-pr-size-limit-config

📝 Commits (1)

a420023 feat(groom): make pr_size_limit settable from GROOM_CONFIG

📊 Changes

3 files changed (+191 additions, -13 deletions)

View changed files

📝 .github/groom/config.py (+48 -6)
📝 .github/groom/tests/test_config.py (+96 -3)
📝 .github/workflows/groom.yml (+47 -4)

📄 Description

ELI-5

groom's auto-builder refuses to open a PR when the patch it wrote is bigger than pr_size_limit lines — it files an issue instead. Until now that number could only be changed by editing a workflow file. This makes it a normal GROOM_CONFIG knob, so each repo can tune its own ceiling without a PR, the same way it already tunes cadence, max_prs, scope and the rest.

What changed

pr_size_limit moves out of _LOCKED_KEYS and into _OPERATIONAL_KEYS in .github/groom/config.py.
It is wired end-to-end: it joins the gate job's DEFAULTS blob, and the Capture patch step now reads fromJSON(needs.gate.outputs.resolved).pr_size_limit instead of inputs.pr_size_limit. Without that second half the variable would resolve and then be ignored.
The module docstring is rewritten, not just trimmed (see "The honesty fix" below).
Both prose sites in groom.yml that described the knob are updated.
test_pr_size_limit_is_locked is inverted rather than deleted, plus a new TestPrSizeLimit class covering the coercion decision. test_config.py goes 50 → 62 tests.

Why `nonneg_int` and not `numeric_string`

The neighbouring max_prs is deliberately kept as a string on the principle that a downstream parser is the normalization authority. That principle does not transfer here. pr_size_limit's downstream is PR_SIZE_LIMIT=$(awk -v v="$PR_SIZE_LIMIT" 'BEGIN{printf "%d", v}'), and awk's printf "%d" renders any non-numeric value as 0 with no diagnostic. A limit of 0 makes [ "$CHANGED" -gt "$PR_SIZE_LIMIT" ] true for every patch, so every build bails to an issue and the builder silently stops opening PRs entirely. awk cannot tell a typo from an intent, so it is not an authority worth deferring to. Rejecting in config.py means a typo emits a ::warning:: naming the key and falls back to the caller's reviewed default — the module's documented fail-open behaviour. 0 remains a legal explicit value meaning "always file issues, never open PRs".

The honesty fix

config.py's docstring claimed of _OPERATIONAL_KEYS: "it can only make groom scan less, propose less, or file less — never grant it more privilege." That sentence becomes false the moment this knob is settable — raising it admits a larger machine-written patch into a PR. Deleting the pr_size_limit bullet from the locked list without fixing it would leave the file asserting an invariant it no longer holds, so the docstring now says plainly that this is the one operational knob that can widen behaviour, and carries the reasoning for why that is accepted.

Verified against the code, not assumed: groom builder PRs are never auto-merged. The build_pr job opens them with a plain gh pr create -R … --label groom --label groom-pr --label cursor-review (groom.yml:2156). There is no gh pr merge, no --auto, no enablePullRequestAutoMerge and no merge-enabling GraphQL call anywhere in groom.yml or the groom scripts — the only other merge matches in the file are the volume gate's "PRs merged since" activity search and the config-layer merge. So pr_size_limit is a convenience backstop that keeps an unreviewably-large diff out of the review queue, not the thing that stops it merging. Raising it means a bigger diff reaches a human, not that a bigger diff lands unreviewed.

Visibility (the compensating control)

A variable-set value leaves no diff and no branch, so the run itself has to show which ceiling was in force. The gate job's resolve step already echoes the entire resolved knob set (Resolved knobs: {…}), and pr_size_limit is now part of it — so no second echo was added. There is an inline comment at the enforcement site saying so, to stop a future reader adding a per-build repeat of the same fact.

Judgment calls

An empty-value guard in the Capture patch step. config.py only ever drops pr_size_limit from the resolved set if the caller's own reviewed with: value is unusable (a negative literal — type: number permits it). If that happened, the expression would render empty, awk would make it 0, and the builder would silently stop opening PRs. The guard falls back to inputs.pr_size_limit with a ::warning::, which reproduces today's behaviour exactly for that input. Arguably unreachable in practice; it is three lines and it guards precisely the silent-disable failure this PR is otherwise designed to prevent.
test_reviewed_caller_defaults_keep_their_locked_values kept as-is (it still asserts pr_size_limit: 800 passes through from the defaults layer). The property it pins is unchanged — a caller's reviewed with: value reaches resolved untouched — so only its docstring gained a note that the knob is no longer locked.
No clamp, ceiling, or ratchet, per the explicit scope of this change. The value is settable outright.

Interaction with #108

Open PR #108 (matt/be-6161-groom-pr-size-limit-600) raises the shared default from 400 to 600 and rewrites the same pr_size_limit input description block, so the two will conflict textually. They are independent — #108 moves the shared default, this moves where the knob can be set — and neither depends on the other. Whichever merges second rebases. This branch was cut from main (e4a8f7c) with #108 still open.

Testing

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'   # 182 tests, OK (test_config.py: 62)
python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'      # 40, OK
python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py' # 18, OK
python3 .github/agents-md-integrity/check_agents_md.py --root .                  # passed

groom.yml parses as YAML. The resolution path was also exercised end-to-end against config.py's CLI with the caller default 400: {"pr_size_limit": 900} → 900 with no warning; {"pr_size_limit": "lots"} → 400 + ::warning::groom config: pr_size_limit='lots' is not a number; {"pr_size_limit": -5} → 400 + a non-negative warning; {"pr_size_limit": 0} → 0; empty variable → 400. The shell guard + awk floor were run standalone over "", " ", "900" and "0".

Not done

No caller repo changes are needed — this is backwards compatible, and a consumer opts in purely by setting GROOM_CONFIG (the vars context inside a reusable resolves against the caller's repo). Callers still need the usual pin bump to pick it up, which the bump-groom-callers fleet handles.

_{🔄 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/121 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/4/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6345-groom-pr-size-limit-config` --- ### 📝 Commits (1) - [`a420023`](https://github.com/Comfy-Org/github-workflows/commit/a42002383e667e5b103c8e30202de9b788a77f93) feat(groom): make pr_size_limit settable from GROOM_CONFIG ### 📊 Changes **3 files changed** (+191 additions, -13 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/config.py` (+48 -6) 📝 `.github/groom/tests/test_config.py` (+96 -3) 📝 `.github/workflows/groom.yml` (+47 -4) </details> ### 📄 Description ## ELI-5 `groom`'s auto-builder refuses to open a PR when the patch it wrote is bigger than `pr_size_limit` lines — it files an issue instead. Until now that number could only be changed by editing a workflow file. This makes it a normal `GROOM_CONFIG` knob, so each repo can tune its own ceiling without a PR, the same way it already tunes cadence, `max_prs`, scope and the rest. ## What changed - `pr_size_limit` moves out of `_LOCKED_KEYS` and into `_OPERATIONAL_KEYS` in `.github/groom/config.py`. - It is wired end-to-end: it joins the gate job's `DEFAULTS` blob, and the `Capture patch` step now reads `fromJSON(needs.gate.outputs.resolved).pr_size_limit` instead of `inputs.pr_size_limit`. Without that second half the variable would resolve and then be ignored. - The module docstring is rewritten, not just trimmed (see "The honesty fix" below). - Both prose sites in `groom.yml` that described the knob are updated. - `test_pr_size_limit_is_locked` is inverted rather than deleted, plus a new `TestPrSizeLimit` class covering the coercion decision. `test_config.py` goes 50 → 62 tests. ## Why `nonneg_int` and not `numeric_string` The neighbouring `max_prs` is deliberately kept as a string on the principle that a downstream parser is the normalization authority. That principle does not transfer here. `pr_size_limit`'s downstream is `PR_SIZE_LIMIT=$(awk -v v="$PR_SIZE_LIMIT" 'BEGIN{printf "%d", v}')`, and `awk`'s `printf "%d"` renders **any** non-numeric value as `0` with no diagnostic. A limit of `0` makes `[ "$CHANGED" -gt "$PR_SIZE_LIMIT" ]` true for every patch, so every build bails to an issue and the builder silently stops opening PRs entirely. `awk` cannot tell a typo from an intent, so it is not an authority worth deferring to. Rejecting in `config.py` means a typo emits a `::warning::` naming the key and falls back to the caller's reviewed default — the module's documented fail-open behaviour. `0` remains a legal explicit value meaning "always file issues, never open PRs". ## The honesty fix `config.py`'s docstring claimed of `_OPERATIONAL_KEYS`: *"it can only make groom scan less, propose less, or file less — never grant it more privilege."* That sentence becomes false the moment this knob is settable — raising it admits a larger machine-written patch into a PR. Deleting the `pr_size_limit` bullet from the locked list without fixing it would leave the file asserting an invariant it no longer holds, so the docstring now says plainly that this is the one operational knob that can widen behaviour, and carries the reasoning for why that is accepted. **Verified against the code, not assumed:** groom builder PRs are **never auto-merged**. The `build_pr` job opens them with a plain `gh pr create -R … --label groom --label groom-pr --label cursor-review` (`groom.yml:2156`). There is no `gh pr merge`, no `--auto`, no `enablePullRequestAutoMerge` and no merge-enabling GraphQL call anywhere in `groom.yml` or the groom scripts — the only other `merge` matches in the file are the volume gate's "PRs merged since" activity search and the config-layer merge. So `pr_size_limit` is a convenience backstop that keeps an unreviewably-large diff out of the review queue, not the thing that stops it merging. Raising it means a bigger diff reaches a human, not that a bigger diff lands unreviewed. ## Visibility (the compensating control) A variable-set value leaves no diff and no branch, so the run itself has to show which ceiling was in force. The gate job's resolve step already echoes the entire resolved knob set (`Resolved knobs: {…}`), and `pr_size_limit` is now part of it — so no second echo was added. There is an inline comment at the enforcement site saying so, to stop a future reader adding a per-build repeat of the same fact. ## Judgment calls 1. **An empty-value guard in the `Capture patch` step.** `config.py` only ever drops `pr_size_limit` from the resolved set if the *caller's own reviewed* `with:` value is unusable (a negative literal — `type: number` permits it). If that happened, the expression would render empty, `awk` would make it `0`, and the builder would silently stop opening PRs. The guard falls back to `inputs.pr_size_limit` with a `::warning::`, which reproduces today's behaviour exactly for that input. Arguably unreachable in practice; it is three lines and it guards precisely the silent-disable failure this PR is otherwise designed to prevent. 2. **`test_reviewed_caller_defaults_keep_their_locked_values` kept as-is** (it still asserts `pr_size_limit: 800` passes through from the defaults layer). The property it pins is unchanged — a caller's reviewed `with:` value reaches `resolved` untouched — so only its docstring gained a note that the knob is no longer locked. 3. **No clamp, ceiling, or ratchet**, per the explicit scope of this change. The value is settable outright. ## Interaction with #108 Open PR #108 (`matt/be-6161-groom-pr-size-limit-600`) raises the shared **default** from 400 to 600 and rewrites the same `pr_size_limit` input description block, so the two will conflict textually. They are independent — #108 moves the shared default, this moves *where the knob can be set* — and neither depends on the other. Whichever merges second rebases. This branch was cut from `main` (`e4a8f7c`) with #108 still open. ## Testing ``` python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' # 182 tests, OK (test_config.py: 62) python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' # 40, OK python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py' # 18, OK python3 .github/agents-md-integrity/check_agents_md.py --root . # passed ``` `groom.yml` parses as YAML. The resolution path was also exercised end-to-end against `config.py`'s CLI with the caller default `400`: `{"pr_size_limit": 900}` → `900` with no warning; `{"pr_size_limit": "lots"}` → `400` + `::warning::groom config: pr_size_limit='lots' is not a number`; `{"pr_size_limit": -5}` → `400` + a non-negative warning; `{"pr_size_limit": 0}` → `0`; empty variable → `400`. The shell guard + `awk` floor were run standalone over `""`, `" "`, `"900"` and `"0"`. ## Not done No caller repo changes are needed — this is backwards compatible, and a consumer opts in purely by setting `GROOM_CONFIG` (the `vars` context inside a reusable resolves against the caller's repo). Callers still need the usual pin bump to pick it up, which the `bump-groom-callers` fleet handles. --- <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:35 +00:00