[PR #65] [MERGED] fix(groom): run the three agent phases via the Claude CLI directly (BE-4214) #74

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/65
Author: @mattmillerai
Created: 7/23/2026
Status: ✅ Merged
Merged: 7/24/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4214-groom-claude-cli

📝 Commits (5)

70098ae fix(groom): run the three agent phases via the Claude CLI directly (BE-4214)
d938c34 fix(groom): harden the direct-CLI agent steps from the review panel (BE-4214)
ada848b fix(groom): close the agent-write escalation paths from round 2 (BE-4214)
9b63c5e fix(groom): close the round-3 review findings on the direct-CLI agent steps (BE-4214)
eab3fa1 fix(groom): scope the agent write tools to close the runner-filesystem write class (BE-4214)

📊 Changes

3 files changed (+623 additions, -59 deletions)

View changed files

📝 .github/workflows/ci-groom.yml (+9 -4)
📝 .github/workflows/groom.yml (+613 -54)
📝 README.md (+1 -1)

📄 Description

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 there
git log doesn't work at all, because that spot isn't a git repo. Every git
command 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 were
using 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-action with a direct claude -p invocation in
audit_find → "Run finder", audit_verify → "Run verifier", and build →
"Run builder".

working-directory: ${{ env.GROOM_CLONE }} — the actual fix. cwd is now
the clone, so Bash(git log:*) / Bash(git grep:*) etc. resolve. The
allowlists, the briefs (.github/groom/*.md), the prompt-build steps,
GROOM_CLONE, the repo/ checkout layout, FINDER_OUT/VERIFIER_OUT/
BUILDER_OUT, and all assert/validate/capture logic are unchanged.
Turn caps — finder 150, verifier 150, builder 100 (was 40/40/60). A
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.
Pinned CLI — npm install -g @anthropic-ai/claude-code@2.1.217, once per
agent job. Pinned (not floating) because the CLI is executable supply chain
for a step that reads untrusted repo content.
Exec-log artifacts — groom-{finder,verifier,builder}-exec, uploaded
before each assert/validate/capture gate so they survive a starved run.
id-token: write dropped from the three agent jobs, the header comment,
the ci-groom.yml caller, and the README. Each agent job is now
contents: read only — a security improvement: the direct CLI mints no
GitHub token into the agent env at all.
--setting-sources "" --strict-mcp-config — security-critical now that
cwd is the untrusted clone: without them the CLI would load the target
repo's .claude/settings.json (hooks = arbitrary shell, outside the
permission engine) and its .mcp.json.
inputs.model routed through env: instead of interpolated into the run
script — see "judgment calls" below.

The || echo "::warning::…" on each invocation is deliberate: claude -p exits
non-zero on error_max_turns and 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):

actionlint clean across all workflows.
Groom ledger tests: 48 passed. (test-groom-scripts.yml is path-filtered to
.github/groom/**, untouched here — ran it anyway.) cursor-review (40) and
agents-md-integrity (18) suites also green; AGENTS.md integrity check passes.
Greps: 0 occurrences of the removed action in groom.yml; exactly 3
npm install -g @anthropic-ai/claude-code@ pins; no id-token grant anywhere.

Premise falsification (rather than trusting the ticket):

"The action cannot set a working directory." Confirmed — dumped action.yml
at the pinned SHA b76a0776; its full input list has no working-directory /
cwd / repo-path input (only path_to_claude_code_executable and
path_to_bun_executable, both unrelated).
"These CLI flags exist and parse on 2.1.217." --max-turns is not in
that version's --help output, so I tested it rather than assuming: ran the
exact 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, and
permission_denials, confirming the artifact carries the promised
diagnostics. --setting-sources "" is accepted (help documents it as a
comma-separated list of user,project,local; empty ⇒ none loaded).

Not done — needs a maintainer (the one remaining acceptance criterion):
E2E workflow_dispatch groom against Comfy-Org/comfy-cloud-mcp-server with
that caller's uses: pointed at this branch. The pilot caller pins the reusable
by 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-exec artifact is present, verifier proceeds.

Judgment calls

Pinned 2.1.217, not the current-latest 2.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.model moved into env: (MODEL: ${{ inputs.model }} +
--model "$MODEL") rather than the ticket's literal
--model "${{ inputs.model }}". Direct interpolation puts a caller-supplied
string into a shell command; every other input in this workflow (SINK,
CADENCE, THEMES, PR_SIZE_LIMIT, …) already reaches run: via env:, so
the literal version would have been the only script-interpolation in the file.
ci-groom.yml's uses: SHA pin is deliberately left alone. Dropping the
caller's id-token: write is safe in every merge ordering: the reusable at
the currently-pinned SHA (07154fb) declares no id-token at all, so the
reduced grant cannot trip GitHub's startup validation, and that version's
agent job declares contents: read only — so no OIDC token was reachable
there regardless of the caller's grant. Bumping the pin stays a separate step.

Notes

Cost/runtime shift is expected and intended: a completed finder run is ~82
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.
Not a capability-denying change (no deny/throw/"unsupported" dead-end added) —
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.
Chesterton's fence on the id-token: write removal: it was added by #64
(BE-3873) explicitly and only because claude-code-action mints its GitHub
token via OIDC. That action is gone here, so the reason is gone with it.

_{🔄 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/65 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/23/2026 **Status:** ✅ Merged **Merged:** 7/24/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4214-groom-claude-cli` --- ### 📝 Commits (5) - [`70098ae`](https://github.com/Comfy-Org/github-workflows/commit/70098ae851d7a11b434f85161ef2a5f68206c5b9) fix(groom): run the three agent phases via the Claude CLI directly (BE-4214) - [`d938c34`](https://github.com/Comfy-Org/github-workflows/commit/d938c342380a25d6339b37a63e614315ff78f9f2) fix(groom): harden the direct-CLI agent steps from the review panel (BE-4214) - [`ada848b`](https://github.com/Comfy-Org/github-workflows/commit/ada848bdd57c5bd044ed0d62555589e1e5918255) fix(groom): close the agent-write escalation paths from round 2 (BE-4214) - [`9b63c5e`](https://github.com/Comfy-Org/github-workflows/commit/9b63c5e99aab39714a9d5f3cabfa02c6280eb0bb) fix(groom): close the round-3 review findings on the direct-CLI agent steps (BE-4214) - [`eab3fa1`](https://github.com/Comfy-Org/github-workflows/commit/eab3fa161305c92b391573483caaf501bae1e1f5) fix(groom): scope the agent write tools to close the runner-filesystem write class (BE-4214) ### 📊 Changes **3 files changed** (+623 additions, -59 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci-groom.yml` (+9 -4) 📝 `.github/workflows/groom.yml` (+613 -54) 📝 `README.md` (+1 -1) </details> ### 📄 Description ## 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 there `git log` doesn't work at all, because that spot isn't a git repo. Every git command 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 were using 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-action` with a direct `claude -p` invocation in `audit_find` → "Run finder", `audit_verify` → "Run verifier", and `build` → "Run builder". 1. **`working-directory: ${{ env.GROOM_CLONE }}`** — the actual fix. cwd is now the clone, so `Bash(git log:*)` / `Bash(git grep:*)` etc. resolve. The allowlists, the briefs (`.github/groom/*.md`), the prompt-build steps, `GROOM_CLONE`, the `repo/` checkout layout, `FINDER_OUT`/`VERIFIER_OUT`/ `BUILDER_OUT`, and all assert/validate/capture logic are **unchanged**. 2. **Turn caps** — finder 150, verifier 150, builder 100 (was 40/40/60). A 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. 3. **Pinned CLI** — `npm install -g @anthropic-ai/claude-code@2.1.217`, once per agent job. Pinned (not floating) because the CLI is executable supply chain for a step that reads untrusted repo content. 4. **Exec-log artifacts** — `groom-{finder,verifier,builder}-exec`, uploaded *before* each assert/validate/capture gate so they survive a starved run. 5. **`id-token: write` dropped** from the three agent jobs, the header comment, the `ci-groom.yml` caller, and the README. Each agent job is now `contents: read` **only** — a security improvement: the direct CLI mints no GitHub token into the agent env at all. 6. **`--setting-sources "" --strict-mcp-config`** — security-critical now that cwd is the untrusted clone: without them the CLI would load the *target* repo's `.claude/settings.json` (hooks = arbitrary shell, outside the permission engine) and its `.mcp.json`. 7. **`inputs.model` routed through `env:`** instead of interpolated into the run script — see "judgment calls" below. The `|| echo "::warning::…"` on each invocation is deliberate: `claude -p` exits non-zero on `error_max_turns` and 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):** - `actionlint` clean across all workflows. - Groom ledger tests: 48 passed. (`test-groom-scripts.yml` is path-filtered to `.github/groom/**`, untouched here — ran it anyway.) cursor-review (40) and agents-md-integrity (18) suites also green; AGENTS.md integrity check passes. - Greps: **0** occurrences of the removed action in `groom.yml`; exactly **3** `npm install -g @anthropic-ai/claude-code@` pins; no `id-token` grant anywhere. **Premise falsification (rather than trusting the ticket):** - *"The action cannot set a working directory."* Confirmed — dumped `action.yml` at the pinned SHA `b76a0776`; its full input list has no `working-directory` / `cwd` / repo-path input (only `path_to_claude_code_executable` and `path_to_bun_executable`, both unrelated). - *"These CLI flags exist and parse on 2.1.217."* `--max-turns` is **not** in that version's `--help` output, so I tested it rather than assuming: ran the exact 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`, and `permission_denials`, confirming the artifact carries the promised diagnostics. `--setting-sources ""` is accepted (help documents it as a comma-separated list of `user,project,local`; empty ⇒ none loaded). **Not done — needs a maintainer (the one remaining acceptance criterion):** E2E `workflow_dispatch` groom against `Comfy-Org/comfy-cloud-mcp-server` with that caller's `uses:` pointed at this branch. The pilot caller pins the reusable by 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-exec` artifact is present, verifier proceeds. ## Judgment calls - **Pinned `2.1.217`, not the current-latest `2.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.model` moved into `env:`** (`MODEL: ${{ inputs.model }}` + `--model "$MODEL"`) rather than the ticket's literal `--model "${{ inputs.model }}"`. Direct interpolation puts a caller-supplied string into a shell command; every other input in this workflow (`SINK`, `CADENCE`, `THEMES`, `PR_SIZE_LIMIT`, …) already reaches `run:` via `env:`, so the literal version would have been the only script-interpolation in the file. - **`ci-groom.yml`'s `uses:` SHA pin is deliberately left alone.** Dropping the caller's `id-token: write` is safe in every merge ordering: the reusable at the currently-pinned SHA (`07154fb`) declares no `id-token` at all, so the reduced grant cannot trip GitHub's startup validation, and that version's agent job declares `contents: read` only — so no OIDC token was reachable there regardless of the caller's grant. Bumping the pin stays a separate step. ## Notes - Cost/runtime shift is expected and intended: a *completed* finder run is ~82 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. - Not a capability-denying change (no deny/throw/"unsupported" dead-end added) — 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. - Chesterton's fence on the `id-token: write` removal: it was added by #64 (BE-3873) explicitly and only because `claude-code-action` mints its GitHub token via OIDC. That action is gone here, so the reason is gone with it. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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