feat(groom): GROOM_CONFIG — tune the groom loop from a repo variable (BE-5227) - #93
Conversation
…5227) Adds .github/groom/config.py: resolves groom's operational knobs from a GROOM_CONFIG JSON Actions variable layered over the caller's `with:` values, so retuning cadence/ceilings/scope no longer needs a workflow-file PR in the consumer repo. Generalizes the GROOM_INTERVAL_DAYS one-off from BE-4004. Precedence: workflow_dispatch inputs > GROOM_CONFIG > caller with: > defaults, so an existing caller that sets nothing is byte-for-byte unchanged. The variable deliberately CANNOT set builder, pr_size_limit, sink, bot_app_id or workflows_ref: an Actions variable is repo-write to edit but bypasses PR review and leaves no diff on a branch, so the auto-builder's security boundary stays in the reviewed file. Those keys are ignored with a loud warning rather than silently dropped. Fail-open throughout — malformed JSON warns and falls back to the caller's values, because a typo'd variable must not red out a daily scheduled run. Output is one compact JSON line for a single `resolved` job output; consumers read knobs via fromJSON(). One output beats eleven when job outputs are strings and two knobs are multi-line prose. This commit is the resolution layer + its tests only; the groom.yml wiring that consumes it lands next. 45 new tests, 132 green across the groom suite.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Comment |
…b (BE-5227) Makes config.py load-bearing. The gate job resolves every operational knob once and publishes them as a single `resolved` JSON output; all six downstream jobs now read `fromJSON(needs.gate.outputs.resolved).<knob>` instead of `inputs.<knob>`, so the merge happens in exactly one place. NO CALLER CHANGE IS NEEDED. The gate job reads `vars.GROOM_CONFIG` itself: the `vars` context inside a called workflow resolves against the CALLER's repository. Verified in-org rather than assumed — assign-reviewers.yml (also a workflow_call reusable) reads vars.REVIEWER_LOAD_CAP, which is set on Comfy-Org/cloud and NOT on this repo. So a consumer opts in by setting one variable, and picks up the capability on its next pin bump, which bump-groom-callers already automates. Precedence, lowest first: per-knob `with:` inputs < vars.GROOM_CONFIG < the new optional `config` input. There is deliberately NO workflow_dispatch layer, because a reusable CANNOT see one — inside a called workflow `inputs` is the reusable's own inputs. An earlier draft of this commit used toJSON(inputs) as a 'dispatch layer', which would have silently overridden every other layer at top precedence. Callers that forward a dispatch value per-knob now rank BELOW the variable; the `config` input is the documented slot for a one-run override that must still win. Called out in the input description and the step comment because it is a real behavior change for the two callers that forward max_prs/dry_run. New `paths` knob narrows the finder to a subset of the repo. Deliberately does not touch scope_label, which is field 2 of every dedup signature — narrowing scope must not re-key the ledger and re-propose the backlog. Absent, empty, blank and malformed all degrade to no narrowing rather than an empty scope that would silently starve the finder. Verified: yaml parses; all 6 resolved-consuming jobs confirmed to declare `needs: gate` (a missing one would evaluate to null and break silently); the two-pass merge exercised for back-compat, variable-over-baseline, config-over-variable and malformed-degrades-to-baseline; paths rendering exercised across all five knob states. 132 groom tests green.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 6 |
| 🟢 Low | 2 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…les (BE-5227) Resolves the cursor-review panel findings on #93. All nine were reproduced locally before fixing; each is a case where config.py either crashed (breaking the module's central fail-open contract) or silently accepted a value whose downstream effect was the inverse of what the operator asked for. * max_findings: `int(float(value))` raised OverflowError — an ArithmeticError, not covered by `(TypeError, ValueError)` — for `inf`, `"inf"`, the bare `Infinity` literal json accepts, `1e999`, and any 310+-digit integer, so a single typo'd variable exited non-zero and turned the daily gate red. Validate the float (finite, non-negative) BEFORE flooring, which also stops `int()`'s truncate-toward-zero from turning `-0.5` into an accepted `0` (= file nothing). * parse_layer: `json.loads` also raises a plain ValueError past CPython's 4300-digit int/str limit and RecursionError on a deeply nested array, both of which fit in a 48 KB Actions variable. Catch Exception, as interval.py does. * max_prs/interval_days/cadence: `inf`/`nan`/negatives passed `float()` and then degraded downstream in opposite, silent directions (zero builder PRs vs revert to weekly). Apply interval.py's `math.isfinite`/`< 0` guard so the warning names the key instead of the value vanishing into a clamp. * paths: an all-rejected list returned `[]`, which the consumer reads the same as unset — the exact whole-repo widening per-entry dropping exists to prevent. Return None instead. Traversal is now checked per path COMPONENT (a substring test rejected legitimate `src/v1..v2/x.py`), and the cap warning fires only when entries actually remain unprocessed. * scope_desc: blank prose returned `""` and overwrote the caller's value, leaving the finder brief's "Scan {{SCOPE_DESC}}." sentence truncated. Blank now means "keep the caller's". `themes` deliberately keeps `""` — clearing a pinned theme list is meaningful there, and the finder's `if themes:` guard reads it. * coerce_layer: the caller-defaults layer is the reviewed workflow file, i.e. the thing _LOCKED_KEYS protects — not a bypass of it. Gate locked keys on the variable/`config` layers only, so a reviewed `with:` value is not stripped and the bypass warning does not fire on every normal run. Also bounds the value echoed into warnings (run logs here are public) and fixes test_control_characters_stripped, which was passing vacuously over an empty list.
This branch carried a `paths` config knob that reimplemented what #83 already does with a `path` input backed by .github/groom/scope.py. #83 is the older PR and the more complete one: it also teaches interval.py not to reset the cadence when the scope narrows, which this knob did not do. Shipping both would have merged two implementations of one feature into the same workflow, and they conflicted on groom.yml regardless. Dropped here rather than in #83, since scope.py is where path validation belongs — _coerce_path_list would have duplicated it. Removes: the allowlist entry, _coerce_path_list and its caps/regex, the PATHS env plumbing in the finder-prompt step, and the brief-rendering block. Adds a guard test asserting `paths` is NOT an operational key, so re-adding it has to be deliberate rather than a merge accident. Once #83 lands, exposing it is two lines: add `path` to _OPERATIONAL_KEYS and to the DEFAULTS block in `gate`. Noted at the drop site. 137 tests green (was 151; 67 lines of paths tests removed, one guard added).
mattmillerai
changed the title
|
Rebased — scope reduced, and one thing worth knowing before you review. Dropped the What is left is only the config layer, which does not overlap any other open PR:
No caller changes needed. Reviewer attention here, please — there is a real behavior change. There is deliberately no Precedence, lowest first: per-knob Locked keys. Also folded in |
3 participants
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.ymlwiring that consumes it..github/groom/config.py— resolves the knobs. Precedence, lowest first: reusable defaults < callerwith:<vars.GROOM_CONFIG< theconfiginput..github/groom/tests/test_config.py— 50 tests. Full groom suite green on the CI matrix..github/workflows/groom.yml— theconfiginput, theresolvestep ingate, and theresolvedoutput threaded through the downstream jobs. Every consuming job now reads its knobs viafromJSON(needs.gate.outputs.resolved).<knob>instead ofinputs.<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_dispatchlayer. A reusable workflow cannot see the caller's dispatch inputs: inside a called workflowinputsis that workflow's own inputs, andgithub.event.inputsis 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: onceGROOM_CONFIGsets 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 intoconfig, which ranks above the variable.vars.GROOM_CONFIGis read inside the reusable, not passed in. Thevarscontext in a called workflow resolves against the caller's repository, so every consumer inherits variable-driven config with nowith:change — only a pin bump, which thebump-groom-callersfleet already automates. (Verified in-org:assign-reviewers.ymlreadsvars.REVIEWER_LOAD_CAP, set oncloudand 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_refandconfigitself are refused with a loud warning rather than silently dropped.configis 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",1and"on"are all rejected for a boolean rather than coerced — reading"yes"as false would quietly re-enable live filing. Same forboolwhere a number is expected, sinceboolis anintsubclass in Python.scope_labelis 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/cadencestay strings on purpose. Each already has a downstream parser that owns clamping and fail-open degradation (build_selectandinterval.py). Re-clamping here would create two places to disagree.Known leftover, flagged for this review
config.pystill exposes a--dispatch-jsonargument, andresolve()still takes adispatch_rawlayer labelledworkflow_dispatch inputs. It is a vestige of the original design and the shipped wiring never passes it —groom.ymlinstead makes two--config-jsonpasses 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: feat(groom):pathinput scopes an audit to one directory, without resetting the cadence clock (BE-4757) #83 owns path scoping via apathinput backed byscope.py, and it also teachesinterval.pynot to reset the cadence when scope narrows. There is a test assertingpathsis not a config knob. Once feat(groom):pathinput scopes an audit to one directory, without resetting the cadence clock (BE-4757) #83 lands, addingpathto_OPERATIONAL_KEYSand to theDEFAULTSblock makes it variable-tunable with no further wiring.setup_commandinput, 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.