fix(groom): run the three agent phases via the Claude CLI directly (BE-4214)#65
Merged
Merged
Conversation
…E-4214) The finder/verifier/builder steps ran anthropics/claude-code-action at the non-git workspace root with --max-turns 40. Two failures compounded there: the whole-repo finder brief needs ~82 opus turns, and the `repo/`-subdir layout made every allowlisted git command unusable (`git log` fails at the non-git root; `git -C repo …` / `cd repo && …` miss the `Bash(git log:*)` prefix match and get denied). The agent starved and never wrote $FINDER_OUT. The action cannot set a working directory (no such input; composite actions can't take `working-directory:`), so invoke the CLI directly — exactly like the production-proven studio groom — with cwd = the clone, the SAME restricted allowlists, and the `repo/` layout unchanged. Turn caps: finder/verifier 150, builder 100 (was 60), all comfortably inside the existing timeout-minutes. Also: - Upload the CLI result JSON (`subtype`, `num_turns`, `permission_denials`) as an artifact before each assert/validate/capture gate, so a starved run is diagnosable from the run page instead of needing a local reproduction. - Drop `id-token: write` from the three agent jobs, the header comment, the ci-groom.yml caller and the README: the direct CLI mints no GitHub token, so those jobs now hold `contents: read` ONLY. Added in #64 (BE-3873) solely for the action's OIDC token; that reason is gone with the action. - Pass `inputs.model` through env rather than interpolating it into the run script, matching how every other input in this workflow reaches a shell. Refs BE-4202 (spike; this is layer 3 after startup-perms L1 and OIDC L2).
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: 40 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 (1)
📝 WalkthroughWalkthroughThe grooming workflows replace the Anthropic composite action with a pinned Claude CLI, remove OIDC permissions, lock agent workspaces, harden environments, redact diagnostics, scan outputs for API keys, and block patches touching workflow or action paths. ChangesGroom CLI migration
Sequence Diagram(s)sequenceDiagram
participant GroomWorkflow
participant ClaudeCLI
participant LockedClone
participant PatchGate
GroomWorkflow->>ClaudeCLI: install pinned CLI and run agent
ClaudeCLI->>LockedClone: inspect or modify permitted workspace
ClaudeCLI-->>GroomWorkflow: produce JSON or patch output
GroomWorkflow->>PatchGate: scan output for API key and privileged paths
PatchGate-->>GroomWorkflow: allow diagnostics or bail before PR creation
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
mattmillerai
marked this pull request as ready for review
July 23, 2026 21:45
![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 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 5 |
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…BE-4214)
Addresses the cursor-review findings on the CLI migration. All three agent
jobs (finder/verifier/builder) get the same four changes:
- `--bare`: cwd is the UNTRUSTED clone, and `--setting-sources ""` only
governs settings.json — it does NOT stop the target repo's
CLAUDE.md/AGENTS.md auto-loading as TRUSTED project memory, which would
let a malicious repo override the "untrusted data, never instructions"
guardrail. `--bare` skips CLAUDE.md auto-discovery; auth stays on
ANTHROPIC_API_KEY, the step's only credential.
- Drop `Bash(git grep:*)` + force `GIT_PAGER`/`PAGER` to `cat`:
`git grep --open-files-in-pager=<cmd>` executes an arbitrary command from
a single allowlisted invocation, with the model key in the step env. The
briefs never call `git grep` ("grep/cat/git-log"); Grep covers the search.
- Stop swallowing the CLI's non-zero exit. `|| echo "::warning::"` made a
rate-limit/network/turn-exhaustion failure look green while the downstream
validate step read the missing output as "no findings" and silently
discarded real candidates. `build_pr` already tolerates a failed build leg
via `!cancelled()`.
- Upload a FIXED-SHAPE projection of the exec JSON, not the raw file.
`result` is free-form assistant text and `permission_denials[].tool_input`
is a model-authored command string; neither is secret-masked, and
artifacts are publicly downloadable on public callers, so a prompt-injected
agent could read the key (`cat /proc/self/environ`) and exfiltrate it with
no network egress. Denials reduce to per-tool counts. The projection is
written only when there is something to report, so `if-no-files-found:
warn` can finally fire on a starved run, and the uploads are `if: always()`
so diagnostics survive the now-hard failure.
ci-groom.yml keeps the `id-token: write` removal — the pinned SHA predates
the agent jobs entirely and declares no `id-token` — plus a note to bump that
pin to a SHA at or after this migration, since the intermediate main SHAs do
declare it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 High | 2 |
| 🟡 Medium | 2 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
…214) Second cursor-review round on the direct-CLI migration. Addresses the unresolved threads: - CRITICAL: the agents kept an unscoped `Write`/`Edit` while cwd is the untrusted clone, so a prompt-injected agent could write `.git/config` to register a `diff.external`/`core.fsmonitor` driver and get arbitrary execution out of the *allowlisted* `git show`/`git diff` — with the model key in env. Locked at the OS level instead of via permission rules: finder/verifier chmod the WHOLE clone a-w (their briefs already promise "NO repo edits, only the /tmp result file"); the builder, which must edit the worktree, freezes only `.git`. Paired `if: always()` unlock steps run before capture (`git add -A`) and checkout's post-job cleanup. - Same thread: `$GITHUB_ENV` was reachable as a write-to-execute primitive (BASH_ENV => arbitrary shell in a later step, outside the allowlist). The runner file-command paths are now stripped from the agent's env via `env -u GITHUB_ENV -u GITHUB_PATH -u GITHUB_OUTPUT -u GITHUB_STATE -u GITHUB_STEP_SUMMARY`. - MEDIUM: the verifier had no equivalent of the finder's assert — a CLI that exited 0 without writing $VERIFIER_OUT was read as have_findings=false and the run went green, silently discarding every confirmed candidate. Now a hard error. Zero survivors on a VALID document is still a legitimate exit-0. - MEDIUM: the redact step never removed a stale diag file, so an agent that pre-created /tmp/groom-<phase>-diag.json (its Write reaches /tmp) would have it published verbatim as a public artifact when the CLI produced no valid output. `rm -f` before the projection, all three phases. - LOW: diagnostic redact/upload steps are `continue-on-error: true` so telemetry cannot sink an otherwise-successful agent job. Verified locally against real git: under the lockdown the entire allowlist (`git log`/`show`/`status`/`diff`, grep/cat/ls) still succeeds and tracked-file edits still work, while `.git/config` writes, new files in the tree, and mkdir under `.git` all fail EACCES. Flag set re-parsed under `env -u` on the pinned CLI 2.1.217 (reaches the API, 401 on a bogus key). actionlint clean; groom 48 / cursor-review 40 / agents-md 18 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 High | 2 |
| 🟡 Medium | 3 |
| 🟢 Low | 1 |
Panel: 8/8 reviewers contributed findings.
… steps (BE-4214) Addresses the six unresolved cursor-review threads on #65: - Critical/High (global git-config + RUNNER_TEMP write->exec): confine the agent env — GIT_CONFIG_GLOBAL=/dev/null + GIT_CONFIG_SYSTEM=/dev/null make an injected ~/.gitconfig (diff.external / core.fsmonitor) inert, and -u RUNNER_TEMP hides the runner's file-command files from the allowlisted ls/Glob. All three agent jobs. - High (key exfil via published output): pre-publish secret-scan of the finder/ verifier JSON and the builder patch for the literal ANTHROPIC_API_KEY; fail closed. Deterministic interim only — the structural close (key out of the agent env via a sandboxed step/broker) is tracked separately and gates untrusted-repo use. - Medium (forgeable diagnostics): type/format-guard every field copied into the diag projection so a forged /tmp exec file cannot smuggle the key through session_id or a *_reason field; rm -rf the pre-delete so a planted directory cannot survive. - Medium (verifier schema): hard-fail when the verifier output lacks a findings array ({} no longer silently discards every candidate while the run stays green). - Medium (builder path guard): core.quotePath=false (a quoted special-char path no longer evades the anchor) and also block .github/actions/. - Low (npm --ignore-scripts): breaks the install (postinstall fetches the binary) — already answered; no change. actionlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Round-3 review findings addressed in 9b63c5e (
Resolving these threads. The structural close — a sandboxed agent step (confined FS + scrubbed env + key-broker) that collapses the whole read-the-runner-FS / key-in-env class by construction — is tracked separately as the gate before groom runs on any repo with untrusted contributor content. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 429-432: Update the explanatory parenthetical near the git
configuration step to accurately state that global and system Git config are
cleared via GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM, removing the stale
safe.directory concern and any contradictory wording.
🪄 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: 66374530-a194-43a2-94c1-c502e1e8bc25
📒 Files selected for processing (3)
.github/workflows/ci-groom.yml.github/workflows/groom.ymlREADME.md
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 5 |
| 🟡 Medium | 1 |
| 🟢 Low | 4 |
Panel: 8/8 reviewers contributed findings.
…m write class (BE-4214) Round-4 review findings on the direct-CLI agent steps. - Scope the write tools with `Edit(//<abs>)` / `Edit(//<abs>/**)` instead of granting a bare `Write`(/`Edit`). An unscoped write reached the WHOLE runner filesystem — $RUNNER_TEMP/_runner_file_commands/set_env_* (BASH_ENV = arbitrary shell in every later step), a checked-out action's dist/index.js, ~/.gitconfig. `env -u` never closed that: it drops the pointer vars, not the files, whose paths are fixed and derivable. Finder/verifier are scoped to their one output file; the builder to the clone tree + $BUILDER_OUT. Verified empirically against the pinned 2.1.217 (both Write and Edit obey the rule; `Write(<path>)` does not bind at all; Bash refuses redirection). - Clear global/system git config on the builder's Capture-patch step too — it is the step that actually runs git over the agent's output, with the key in env. - Re-supply safe.directory via GIT_CONFIG_COUNT wherever global config is cleared, so the masking cannot re-starve the agent on a non-hosted runner. - Scan $SUMMARY for the key, not just the patch — it is published in result.json and the bail issue. On a hit, bail with a FIXED reason instead of `exit 1`, so the finding lands in the ledger rather than being re-proposed every run. - Compare paths NUL-delimited: git C-quotes `.github/workflows/ev"il.yml`, which slipped past the anchored grep (`core.quotePath=false` only covers high bytes). - Widen the CI-privileged deny-list past the comment-guarded KNOWN GAP to the build/test config that also executes in pre-review CI. - Validate each verified finding's fields, not just that `findings` is an array. - Emit only granted tool names in denials_by_tool — a shape guard still let the key be split across forged tool_name entries. - Fix the stale git-config parenthetical that contradicted the run script.
mattmillerai
added
cursor-review
Contributor
Author
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
luke-mino-altherr
approved these changes
Jul 24, 2026
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
2 participants
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
Groom's three AI phases (finder, verifier, builder) were being run from the
wrong folder. The repo they're supposed to read gets checked out into a
repo/subfolder, but the agent was started one level above it — and up theregit logdoesn't work at all, because that spot isn't a git repo. Every gitcommand the agent was allowed to run therefore failed, and it burned through
its whole 40-turn budget flailing without ever writing its results file.
We couldn't just tell the old tool "start in
repo/" — the action we wereusing has no setting for that. So this PR runs the Claude CLI directly instead,
started inside the clone, with the exact same restricted tool allowlist. It
also raises the turn budget to what a real run actually needs, and saves the
agent's execution log as a downloadable artifact so next time something goes
wrong you can just read it off the run page.
What changed
Replaces
anthropics/claude-code-actionwith a directclaude -pinvocation inaudit_find→ "Run finder",audit_verify→ "Run verifier", andbuild→"Run builder".
working-directory: ${{ env.GROOM_CLONE }}— the actual fix. cwd is nowthe clone, so
Bash(git log:*)/Bash(git grep:*)etc. resolve. Theallowlists, the briefs (
.github/groom/*.md), the prompt-build steps,GROOM_CLONE, therepo/checkout layout,FINDER_OUT/VERIFIER_OUT/BUILDER_OUT, and all assert/validate/capture logic are unchanged.healthy finder run measured ~82 turns ≈ 12.3 min; 150 turns ≈ 22 min fits the
40-min job timeout, and the builder's 100 ≈ 15 min fits its 30-min timeout.
npm install -g @anthropic-ai/claude-code@2.1.217, once peragent job. Pinned (not floating) because the CLI is executable supply chain
for a step that reads untrusted repo content.
groom-{finder,verifier,builder}-exec, uploadedbefore each assert/validate/capture gate so they survive a starved run.
id-token: writedropped from the three agent jobs, the header comment,the
ci-groom.ymlcaller, and the README. Each agent job is nowcontents: readonly — a security improvement: the direct CLI mints noGitHub token into the agent env at all.
--setting-sources "" --strict-mcp-config— security-critical now thatcwd is the untrusted clone: without them the CLI would load the target
repo's
.claude/settings.json(hooks = arbitrary shell, outside thepermission engine) and its
.mcp.json.inputs.modelrouted throughenv:instead of interpolated into the runscript — see "judgment calls" below.
The
|| echo "::warning::…"on each invocation is deliberate:claude -pexitsnon-zero on
error_max_turnsand friends, and the existing assert/validate/capture steps are the intended gates (matching current behavior, where the
action step always "succeeded" and the assert step decided).
Verification
Local (done):
actionlintclean across all workflows.test-groom-scripts.ymlis path-filtered to.github/groom/**, untouched here — ran it anyway.) cursor-review (40) andagents-md-integrity (18) suites also green; AGENTS.md integrity check passes.
groom.yml; exactly 3npm install -g @anthropic-ai/claude-code@pins; noid-tokengrant anywhere.Premise falsification (rather than trusting the ticket):
action.ymlat the pinned SHA
b76a0776; its full input list has noworking-directory/cwd/ repo-path input (onlypath_to_claude_code_executableandpath_to_bun_executable, both unrelated).--max-turnsis not inthat version's
--helpoutput, so I tested it rather than assuming: ran theexact flag set against CLI 2.1.217 with a deliberately invalid API key. All
flags parsed (no "unknown option"), execution reached the API and returned a
401 — and the emitted result JSON contained
subtype,num_turns, andpermission_denials, confirming the artifact carries the promiseddiagnostics.
--setting-sources ""is accepted (help documents it as acomma-separated list of
user,project,local; empty ⇒ none loaded).Not done — needs a maintainer (the one remaining acceptance criterion):
E2E
workflow_dispatchgroom againstComfy-Org/comfy-cloud-mcp-serverwiththat caller's
uses:pointed at this branch. The pilot caller pins the reusableby ref, so I can't trigger it from here. Expected: "Run finder" completes in
~10–15 min, "Assert finder produced JSON" passes with a non-zero candidate
count, the
groom-finder-execartifact is present, verifier proceeds.Judgment calls
2.1.217, not the current-latest2.1.218. The ticket says both"pin the version current at implementation time" and "2.1.217 is what the fix
was validated on". I took the validated one — 2.1.218 shipped without being
exercised against this recipe, and this is the version I verified the flags
against above.
inputs.modelmoved intoenv:(MODEL: ${{ inputs.model }}+--model "$MODEL") rather than the ticket's literal--model "${{ inputs.model }}". Direct interpolation puts a caller-suppliedstring into a shell command; every other input in this workflow (
SINK,CADENCE,THEMES,PR_SIZE_LIMIT, …) already reachesrun:viaenv:, sothe literal version would have been the only script-interpolation in the file.
ci-groom.yml'suses:SHA pin is deliberately left alone. Dropping thecaller's
id-token: writeis safe in every merge ordering: the reusable atthe currently-pinned SHA (
07154fb) declares noid-tokenat all, so thereduced grant cannot trip GitHub's startup validation, and that version's
agent job declares
contents: readonly — so no OIDC token was reachablethere regardless of the caller's grant. Bumping the pin stays a separate step.
Notes
opus turns ≈ $7.5/repo, versus ~$2.80 for the previous truncated-and-useless
runs. Existing
timeout-minutes(40/40/30) already cover the new caps.it restores the git capability the agents were supposed to have, so the
negative-claim falsification rule doesn't apply. The premise-falsification
work above was done anyway, since the whole rewrite rests on two claims.
id-token: writeremoval: it was added by fix(groom): correct required caller permissions in ci-groom, header, README (BE-3873) #64(BE-3873) explicitly and only because
claude-code-actionmints its GitHubtoken via OIDC. That action is gone here, so the reason is gone with it.