feat(groom): opt-in auto-builder — top-N CONFIRMED findings become review-gated PRs (BE-4003)#55
Open
mattmillerai wants to merge 3 commits into
Open
feat(groom): opt-in auto-builder — top-N CONFIRMED findings become review-gated PRs (BE-4003)#55mattmillerai wants to merge 3 commits into
mattmillerai wants to merge 3 commits into
Conversation
…view-gated PRs (BE-4003) Split C of the groom epic (BE-3870). With builder: true, the top max_prs (default 5) CONFIRMED, non-security findings become reviewable PRs instead of issues, throttled and NEVER auto-merged; the remainder stay groom issues (hybrid). Security boundary preserved (mirrors finder/verifier): a credential-free build job (contents: read) writes only a patch artifact; a separate no-agent build_pr job holds the bot token, applies the patch on a branch, and opens the PR. A prompt-injected builder can only produce a bad patch, never push. The durable ledger now recognizes builder PR state (open/merged/closed) via the signature marker in the PR body, so a built finding is never re-proposed (regression-tested). Groomer-only (issues) stays the default; builder requires bot_app_id so PRs are branded and trigger CI + cursor-review.
mattmillerai
added
cursor-review
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 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)
📝 WalkthroughWalkthroughThe groom workflow adds an opt-in auto-builder that converts verified non-security findings into patches and review-gated pull requests, with fallback issue filing. The durable ledger now tracks builder PR lifecycle states to prevent duplicate proposals. ChangesGroom auto-builder
Sequence Diagram(s)sequenceDiagram
participant Groom
participant build_select
participant build
participant build_pr
participant GitHub
Groom->>build_select: verified findings
build_select->>GitHub: read issue and PR ledger state
GitHub-->>build_select: deduplicated findings
build_select->>build: selected findings
build->>build_pr: patch artifact and status
build_pr->>GitHub: open review-gated PR or fallback issue
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/groom.yml:
- Around line 1000-1006: Scope each create-github-app-token invocation to least
privilege: at .github/workflows/groom.yml lines 570-576 (build_select), add
read-only issues and pull-requests permissions; at lines 689-695 (file), add
issues write permission; and at lines 1000-1006 (build_pr), add contents write,
pull-requests write, and issues write permissions. Use the action’s permission-*
inputs for each job and leave unrelated token configuration unchanged.
- Around line 69-77: Update the bot App permission requirements documented near
the builder configuration in the workflow to include issues:write alongside
contents:write and pull-requests:write, covering both File findings issue
creation and the build_pr fallback.
- Around line 941-968: Normalize the PR_SIZE_LIMIT value from the workflow input
to an integer before the size comparison in the patch-capture step. Update the
check around CHANGED and PR_SIZE_LIMIT so fractional values such as 400.0 are
safely converted or clamped, while preserving the existing bail behavior when
CHANGED exceeds the normalized limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f4cadb3d-c944-474d-aa33-e2ab46af4c3e
📒 Files selected for processing (7)
.github/groom/README.md.github/groom/builder.md.github/groom/ledger.py.github/groom/tests/test_ledger.py.github/workflows/groom.ymlAGENTS.mdREADME.md
…issues:write (BE-4003) Address CodeRabbit review on PR #55: - Scope each create-github-app-token mint to least privilege via permission-* inputs: build_select reads issues+PRs (read), file opens issues (write), build_pr pushes+opens PR+bails to issue (contents/pull-requests/issues write). - Floor PR_SIZE_LIMIT to an int before the integer-only `-gt` test so a fractional `pr_size_limit` (e.g. 400.0) can't abort the patch-capture step under `set -euo pipefail`. - Document that the bot App also needs issues:write (file + build_pr-bail paths open groom issues). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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 |
|---|---|
| 🔴 Critical | 1 |
| 🟠 High | 4 |
| 🟡 Medium | 2 |
| 🟢 Low | 3 |
Panel: 8/8 reviewers contributed findings.
Address the cursor-review panel on PR #55: - build_pr: gate on `!cancelled()` so one failed/timed-out build matrix cell (fail-fast:false) no longer skips the PR sink for EVERY index — each successfully-built finding still opens its PR. (High) - builder allowedTools: drop `Bash(find:*)` — `find … -exec` is arbitrary command execution / an egress channel for a prompt-injected builder holding the model key. Glob/Grep/Read cover discovery without exec. (High) - capture: refuse a patch touching `.github/workflows/` (files an issue instead) — such a patch would run in credentialed CI on the same-repo push before any human review, self-authoring CI. (High) - security gate: fail CLOSED in both build_select and build_pr — a finding is auto-buildable only if the verifier PROVABLY marked it `security == false`; missing/malformed values now stay issues, never auto-implemented. (High) - ledger: admit a PR as a builder record only if it carries the bot-applied `groom-pr` label, so a hand-opened `groom` PR pasting a public signature marker cannot suppress a live finding. (Medium) - build_pr base: use `github.event.repository.default_branch` (rev-parse fallback) instead of `git rev-parse HEAD`, which mis-targets on a non-default-branch dispatch or yields "HEAD" on a detached checkout. (Medium) - builder labels step: add the `if: dry_run != true` guard the file job has, so a dry run no longer mutates the target repo's labels. (Low) Also fixes a bug in the initial hardening: rationale comments must not live inside the `claude_args:` block scalar (they would reach the CLI as literal args) — moved to a YAML comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI-5
The groomer finds tech-debt and files GitHub issues, but the team reviews at the PR level and never works the issues — so they're a dead end. This adds an opt-in auto-builder (split C of the groom epic): when a repo turns it on, the top few confirmed findings each run are turned into real, reviewable PRs (throttled to 5), while everything else still files as issues. The PRs run full CI + cursor-review and are never auto-merged — a human still presses the button. Off by default, so nothing changes unless you opt in.
What changed
groom.yml— three new jobs behindbuilder: true(default false):build_select— the single dedup + throttle authority (no agent). Dedups verified findings against the durable ledger, then splits them: topmax_prs(default 5) CONFIRM, non-security findings → build queue; the rest (DOWNGRADE, security, beyond-cap) → issue remainder. In builder-off mode it's a pass-through, so the issue path is byte-identical to before.build— the credential-free builder (matrix, one cell per queued finding).permissions: contents: read, onlyANTHROPIC_API_KEY. An agent writes the code change into its own fresh checkout; a deterministic step captures the working-tree diff as a patch artifact and enforces thepr_size_limitbail-out. Mirrors the finder/verifier isolation exactly.build_pr— the credentialed PR job (no agent). Mints the bot token, applies the patch on a branch, opens a branded, review-gated PR (labelsgroom+groom-pr+cursor-review, a loud "NOT auto-merged" banner, the verifier rationale, and the signature marker). On a bail/oversized/non-applying patch it files agroomissue instead so nothing is dropped. Honorsdry_run(preview, opens nothing).ledger.py— PR-state added. The/issueslisting already returns groom-labeled PRs; builder PRs carry the same signature marker in their body, so the ledger now classifies them: open →pr-open, merged →merged, closed-unmerged →pr-closed. All suppress re-proposing → a merged/closed finding is never re-proposed (RFC point 3). The markerless-record skip still guards against poisoning. (BE-3874 skipped all PRs because groom filed only issues then — cited in the code.)builder.md— new phase-3 brief (scope-disciplined, keep-it-green, never-touch-security, patch-size bail).Acceptance criteria
build_selectcapsto_buildatmax_prs.build(contents: read) → artifact →build_pr(bot token).ledger.py, regression-tested (test_{open,merged,closed}_builder_pr_*intest_ledger.py).groom/groom-pr/cursor-reviewlabels + bot author; opening as the bot App (required) triggers CI + the review panel.groom-pr(NOTagent-coded), so the agent-work merger (which lives in a different repo and keys onauthor == operator && agent-coded) already skips them. The actual merger-side skip rule can't be enforced from here — please confirm the merger excludesauthor == bot/label == groom-prbefore enabling live on a repo the merger watches.builderinput.Judgment calls (open items from the ticket — resolvable in review)
max_prsinput — trivially tunable.groomissues, so nothing is silently dropped (security findings especially). The alternative "defer to next run" is a one-line change if preferred.rank/score field inverifier.mdcan refine it.pr_size_limit(default 400 changed lines); an oversized or non-applying patch files an issue instead of a giant PR.Riskiest lines & why they're safe
build_ledgernow includes PRs (was an explicit skip). Safe because the marker check is unchanged: only PRs carrying a signature marker (= builder PRs) become records; a hand-labeled human PR is still ignored. History (BE-3874) cited inline — the skip predated the builder.git diff --cached→git apply --indexround-trip: verified locally against modify + add + delete; apply-failure falls back to filing an issue.buildagent job iscontents: readwith no GitHub token; the only job with write creds (build_pr) runs no agent and never executes repo code (it onlygit applys a diff). A prompt-injected builder can produce a bad patch, nothing more.Falsification note (negative-claim check)
The one deny path added —
builder=true requires bot_app_id → exit 1— is input validation that tells the operator exactly how to enable the feature, not a capability denial (no "not supported / STOP" dead-end). The bail path preserves the capability by filing an issue. No #581-class dead-end.Testing
python3 -m unittest discover -s .github/groom/tests→ 46 pass (10 new for PR-state).build_selectpartition +git diff/applyround-trip smoke-tested locally.dry_run: truefirst (RFC point 6).Note on size
~700 lines, but a single cohesive feature (the three jobs are one atomic unit) and fully gated behind an opt-in flag — zero behavior change when
builderis off.