feat(groom): reusable groom.yml — two-phase finder→verifier, GitHub-issue sink (BE-3872)#49
Conversation
…ssue sink (BE-3872) Build the split-C groomer as a workflow_call reusable, cloning the cursor-review topology: a read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against the durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent → `groom-security`, investigate-only). Finds only — no commits, no PRs, never merges. Briefs + ledger under .github/groom/ are the single source of truth (Phase 1). The agent step holds no write credentials: the `audit` job is contents:read only, so filing runs in a separate `file` job as the configured bot (cloud-code-bot), per model-gap-detector.yml. dry_run reports intended issues without opening them (parity spot-checks). Third-party actions SHA-pinned per AGENTS.md; documented in README + AGENTS.md.
mattmillerai
added
agent-coded
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 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)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![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 | 2 |
| 🟡 Medium | 6 |
| 🟢 Low | 1 |
Panel: 8/8 reviewers contributed findings.
…n allowlist + filing (BE-3872)
Resolves the review-panel findings on the reusable groom workflow:
- Security: drop Bash(sed|awk|find|rg) from both agent allowlists — each can
shell out (awk BEGIN{system}, find -exec, sed s///e, rg --pre), which would
let a prompt-injected finder exfiltrate ANTHROPIC_API_KEY. Agents keep
Read/Glob/Grep + non-executing git/cat/ls/head/tail/wc.
- Independence: split the single audit job into separate audit_find and
audit_verify jobs. The verifier now runs on its OWN fresh checkout with the
finder's JSON passed as an artifact, so the finder's Write can no longer
tamper with the code the verifier reads or the brief it follows.
- Filing: track per-issue failures and exit non-zero so a filing outage
surfaces instead of a silent green check; also catch subprocess.TimeoutExpired
(does not inherit from CalledProcessError) as a per-issue failure.
- Robustness: guard `body: null` (.strip on None) and missing signature; parse
max_findings via float+floor and reject negatives (a negative cap sliced from
the end); normalize the security flag so string "false"/"0" isn't truthy.
- Contract: security/auth findings are always kept regardless of theme (the
appended themes instruction now says so); document that the github.token
filing fallback needs the caller to grant issues:write (a reusable workflow
can't elevate the caller's token).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
ELI-5
We already have a robot that reviews pull requests (
cursor-review.yml). This adds a second robot, groom, that goes the other way: on a schedule it reads a whole repo (not a diff) and looks for cleanups worth doing — duplicated code, dead code, inconsistent patterns. One agent proposes candidates, a second, independent agent plays skeptic and confirms/rejects each, and the survivors are filed as GitHub issues (labeledgroom) for humans to pick up. It never writes code, opens a PR, or merges anything — it only files issues. A durable ledger (built in Phase 1) remembers what it already filed or what a human rejected, so it never nags you twice.What this does
Adds
.github/workflows/groom.ymlas aworkflow_callreusable, cloning the cursor-review topology and reusing the Phase-1 briefs + ledger under.github/groom/as the single source of truth:gatejob — validatessinkand applies the volume gate (skip a repo with no merges incadencedays; fail-open).auditjob (contents: readonly) — checks out a clean default-branch copy, substitutes the{{…}}placeholders infinder.md/verifier.md, runs the finder then a fresh independent verifier viaanthropics/claude-code-action, and uploads the verified findings. This job holds no write credentials — it is structurally incapable of writing to GitHub.filejob — dedups survivors (CONFIRM/DOWNGRADE only) against the durable ledger and opensgroom-labeled GitHub issues as the configured bot (Comfy: cloud-code-bot viabot_app_id+BOT_APP_PRIVATE_KEY), embedding the ledger's signature marker so the next run won't re-file. Security-adjacent findings getgroom-security(investigate, don't auto-implement).Triggers (
schedule+workflow_dispatch, never on-PR) and the requiredconcurrency:group live in the caller (documented in the header + README), matching how cursor-review's triggers live in its caller. A workflow-levelconcurrencygroup also serializes runs per target repo as a backstop against the ledger's TOCTOU race.Inputs
themes,cadence,volume_gate,sink(defaultgithub),max_findings,scope_label/scope_desc,model,workflows_ref,bot_app_id,dry_run. Secrets:ANTHROPIC_API_KEY(required),BOT_APP_PRIVATE_KEY(whenbot_app_idset).Acceptance criteria
groom.ymlreusable exists; files verified findings as GitHub issues, acted as cloud-code-bot, no credentials in the agent step (theauditjob iscontents: read-only and the agent gets onlyANTHROPIC_API_KEY)..github/groom/ledger.py) before filing, reusing its exact signature marker.supervisor-groom.shbriefs verbatim (same.github/groom/*.md), anddry_runprints exactly what it would file for a spot-check. Live parity against a studio run on a test repo is the remaining manual verification step — see below.agent-oksink is deferred (needs orgLINEAR_API_KEY), per the ticket —sink: linearfails the run loudly rather than silently filing nothing.Verification done
actionlintclean ongroom.yml..github/groomledger tests: 37 passing (dep unchanged).agents-md-integritychecker passes (1 pre-existing CODEOWNERS warning, unrelated).signature_markerreuse round-trips; the filing logic verified for dry-run, live-command shape,max_findingscapping (with deferral warning), invalid-signature skip, andgroom-securityrouting.workflow_dispatchon a test repo requires the orgANTHROPIC_API_KEY+ cloud-code-bot install on that repo — that end-to-end run (and the studio-parity spot-check) is the reviewer/operator step before moving thev1tag.Judgment calls (flagged)
themesdefault: I could not read epic BE-3870, so I set the default to the finder brief's own five dimensions verbatim — a no-op that preserves studio parity by default while letting a caller narrow it. If the epic's "safe starter set" is a specific narrower list, adjust the default.bot_app_id/BOT_APP_PRIVATE_KEY(consumer supplies its own App). Comfy uses cloud-code-bot.sink != github) path: this is a not-yet-built feature with a clear error, not a regression removing an existing capability — there is no existing linear-sink path in this new workflow to redirect to, and the ticket explicitly defers Linear. So it ships as a loud guard.schedule); aworkflow_dispatchfrom a non-main branch would groom that branch. Acceptable and arguably desirable; noted for awareness.