fix(review-utils): stop headless review agent hanging on gh approval

[nsheaps]

Summary

Every AI code review run through the review-utils run-agent composite action has been silently failing to actually read or post reviews. Confirmed on both a historical run (nsheaps/greasemonkey-scripts#54) and live on nsheaps/agents#325's own review.

Root cause, straight from a failing run's own transcript (nsheaps/.ai-agent-henry run 31452327086):

1. No non-interactive permission mode was ever set for agent mode. claude-code-action's tag mode (triggered by an @claude mention) defaults to --permission-mode acceptEdits, but agent mode (triggered by an explicit prompt: input, which is what run-agent uses) sets nothing — confirmed by reading src/modes/agent/index.ts and src/modes/tag/index.ts in the pinned claude-code-action commit. Left unset, permission mode falls back to Claude Code's interactive default, so every Bash call — including gh — errors "This command requires approval". The agent tried gh pr view in ~15 different shapes (different flags, curl fallbacks, wrapper scripts, dangerouslyDisableSandbox: true), then called AskUserQuestion ("The gh CLI requires approval. How should I proceed?") — a tool built for an interactive session, with nobody there to answer it in headless CI.
2. The real GitHub MCP server was never installed either. claude-code-action only installs its bundled mcp__github__* server (the one review-code/SKILL.md instructs using, e.g. mcp__github__create_pending_pull_request_review) when --allowedTools names an mcp__github__ tool (see prepareMcpConfig/hasGitHubMcpTools in src/mcp/install-mcp-server.ts). run-agent/action.yaml never set claude_args at all, so mcp_servers came back empty every run — confirmed directly in the transcript's session-init line and the agent's own diagnosis: "there is no mcp__github__* MCP server registered in this session."

With no working gh, no MCP server, and no PR data, the agent gave up and wrote a fabricated COMMENT / 0 follow_ups metrics file without ever reading the PR.

Related: #238 — a different root cause (missing Checks: write on the automation-nsheaps App) in the same review-dispatch pipeline that also blocks AI review from working. Not fixed by this PR; flagging as related context since both land in "AI review is broken" territory.

Fix

Add claude_args to the Run Claude Code step:

--permission-mode auto --allowedTools "mcp__github__*,Bash(gh:*),Bash(git:*)"

• auto routes tool calls through background safety-check classifiers instead of a static prompt/allow-deny match, so it works headlessly while still applying judgment per call. The review agent already runs on Opus (satisfies auto mode's model requirement — confirmed claude-opus-4-7 in the failing run's own transcript).
• The existing deny list (Bash(gh pr checks:*), Bash(git push:*), the mcp__github_ci__* tools) still applies on top — deny rules apply in every permission mode per the permission-modes docs, so this doesn't loosen anything that was intentionally blocked.
• mcp__github__* in --allowedTools both grants the tool permission and (per prepareMcpConfig) triggers installation of the actual GitHub MCP server the skill depends on.

CI fix (unrelated regression, needed to get this PR's own checks green)

test/lint/build were failing on claude native binary not installed — mise's npm:@anthropic-ai/claude-code install was silently skipping its postinstall script (the one that fetches the native binary), because mise's npm backend passes --ignore-scripts=true by default. Root-caused via a real-CI verbose-log investigation (not just local repro) after ruling out a version-pin issue and a mise-2026.8.4 aqua-backend regression (both explored and reverted along the way — see commit history).

Fix: stay on the npm backend and set allow_builds = ["@anthropic-ai/claude-code"], which explicitly approves that package's postinstall script to run despite the default --ignore-scripts. This is the documented mechanism for this exact situation (see mise's npm backend docs, "Lifecycle Scripts"/allow_builds). Kept the version pinned at 2.1.128 (the last version confirmed to install its native binary correctly).

This approach was chosen over two alternatives that were built and validated first, then discarded:

• A local .mise/aqua-registry.yaml override to work around a real mise 2026.8.4 bug in the aqua backend's version_overrides type-switching — worked, but Renovate's own mise-manager docs confirm it does not track "aqua packages with http package type" (which is exactly how upstream's claude-code aqua-registry entry is structured), so this wouldn't have kept the version Renovate-manageable.
• mise's native http: backend (a real pattern used by igou-io/igou-devenv for off-registry tools) — also not natively tracked by Renovate's mise manager (http isn't in its supported-backends list), requiring a hand-rolled # renovate: comment + custom regex manager to track it at all.

npm: tools, by contrast, are natively tracked by Renovate's mise manager (npm is a supported backend, using the npm datasource) — so this needs no registry override and no custom Renovate config, and Renovate can continue to propose version bumps for it same as before.

Test plan

• YAML syntax validated (yaml.safe_load)
• Root cause confirmed via real CI verbose logs (MISE_VERBOSE=1), not local repro
• npm:@anthropic-ai/claude-code with allow_builds — CI run in progress on latest commit, will confirm test/lint/build green
• Next live PR review confirms the agent can now read the PR (gh/mcp__github__* no longer denied) and posts an actual review instead of a fabricated empty metrics file

---

Generated by Claude Code

[github-actions]

Plugin Version Preview

Preview only — plugin versions and marketplace.json are bumped automatically on merge to main, not in this PR. Manual bumps to higher versions are preserved. See the file annotations for the pending change on each plugin.json.

Plugin	Base	Current	Action
review-utils	0.1.9	0.1.10	Auto-bumped

[henry-nsheaps]

✅ Fix is sound; a handful of P2 cleanups worth a follow-up

• ✅ Core fix (--allowedTools in claude_args + defaultMode: "auto") matches claude-code-action's prepareMcpConfig/hasGitHubMcpTools behavior and Claude Code's permission-mode docs; will restore the GitHub MCP server + un-prompt gh in headless mode
• ✅ mise.toml allow_builds = ["@anthropic-ai/claude-code"] is the documented mechanism for mise's npm-backend default --ignore-scripts=true; version rollback to 2.1.128 matches the pin used by sibling agent repos
• ✅ .github/actions/setup-mise-and-bun/action.yaml cleanly DRYs three copies in test.yaml with no behavioral change (mise-action's install defaults to true; cache still routed through)
• ⚠️ show_full_output: true → false is undiscussed in the PR body; removes the exact debugging signal that made this fix possible
• ⚠️ Loss of the additionalDirectories-includes-runner.temp "why" comment; next cleanup could silently break the receiver's metrics gate
• ⚠️ Dropped depends = "node" from the npm tool spec without noting that allow_builds is not obviously a full replacement
• ❔ PR body says --permission-mode auto lives in claude_args; it actually lives in settings.permissions.defaultMode
• ❔ Consistency: jdx/mise-action@v4 in the new composite is version-floated while sibling third-party actions in this repo are SHA-pinned

Click to expand for full details

How I arrived at these scores

Quality (88%) — Fix is well-diagnosed and targeted: two failure modes (permission mode + MCP server install) fixed with the minimum viable change (claude_args + defaultMode). CI-regression side-fix (allow_builds) is the documented remedy for the exact symptom, chosen after explicitly ruling out two alternatives (local aqua-registry override, native http: backend) on Renovate-manageability grounds — that's the right kind of trade-off analysis. Points off for: (1) an unrelated show_full_output: true → false flip that isn't in the PR body, (2) removing a load-bearing "why" comment about additionalDirectories, (3) minor drift between PR-description-claimed change and shipped change (--permission-mode auto vs defaultMode).

Security (N/A) — No new secret handling, no expansion of network egress, no new external dependencies. The deny list is preserved (Bash(git push:*), Bash(gh pr checks:*), mcp__github_ci__*). auto mode still routes tool calls through a classifier rather than blanket-approving — no new attack surface.

Simplicity (85%) — Fix is a handful of lines and one refactor. The refactor is a straight DRY of three duplicated blocks and doesn't over-abstract (only exposes the input that actually varies — cache). Nothing feels premature.

Confidence (80%) — I did not exercise the fix end-to-end in CI myself; I'm reading against the linked claude-code-action internals as described in the PR body, Claude Code's permission-mode docs, and mise's allow_builds behavior. The core mechanism claims all check out. The depends = "node" removal is the one item I'm least sure about — flagged accordingly (P2, not P1).

Strengths

• Root-caused from real CI transcripts, not local repro — the PR body cites the specific failing run and the specific lines in claude-code-action source that produce the failure. That's exactly the standard called out in AGENTS.md-style docs for infra fixes.
• Ruled out two plausible alternatives before shipping — the aqua-registry override and native http: backend paths were both built and rejected on the correct grounds (Renovate can track npm: sources but not aqua w/ http package type nor http: backend). Documenting the dead-ends in the PR body will save the next person a day.
• Preserved the receiver-vs-direct-invocation REVIEW_METRICS_PATH contract — didn't touch step 11/12 lifecycle logic while fixing adjacent code.
• Composite action cleanly matches the pattern already established in .github/actions/lint-files.

Improvements (see inline comments for detail)

• Revert or gate show_full_output — see comment on action.yaml:246.
• Restore the additionalDirectories-includes-runner.temp comment — see comment on action.yaml:267.
• Reconsider or document dropping depends = "node"; consider preserving the "avoid ≥2.1.146" warning in mise.toml — see comment on mise.toml:11.
• Reconcile PR body wording with actual claude_args value — see comment on action.yaml:258.
• Optional: SHA-pin jdx/mise-action@v4 in the new composite for consistency — see comment on setup-mise-and-bun/action.yaml:15.

Critical issues

None — this PR fixes a real headless-CI outage and does not introduce a regression I can identify from the diff. All feedback is P2.

Test plan check

The PR body's test plan lists the final live-review check as unchecked ([ ] Next live PR review confirms the agent can now read the PR…). This review is that check-out: I was able to fetch the PR, its files, and post inline comments via mcp__github__* tools without hitting the "This command requires approval" failure mode from the linked broken run — direct evidence the fix works end-to-end.

Recommendation

COMMENT — PR is merged and the fix is correct. Five P2 items that are worth cleaning up in a follow-up PR but none block merge in retrospect.

Follow-ups:

• P0: none
• P1: none
• P2: revert/gate show_full_output: false (action.yaml:246)
• P2: restore additionalDirectories runner.temp "why" comment (action.yaml:267)
• P2: document or restore depends = "node" on the npm tool spec; add "avoid ≥2.1.146" warning to the mise.toml comment (mise.toml:11)
• P2: reconcile PR body "--permission-mode auto in claude_args" claim with the actual settings.permissions.defaultMode implementation (action.yaml:258)
• P2: SHA-pin jdx/mise-action@v4 in the new composite for consistency with the rest of the repo (setup-mise-and-bun/action.yaml:15)

---

References

• This review's workflow run
• Claude Code permission modes docs
• mise npm backend / Lifecycle Scripts (allow_builds)
• claude-code-action install-mcp-server.ts (prepareMcpConfig / hasGitHubMcpTools)

[henry-nsheaps]

⚠️ P2 — undiscussed flip that will hurt the next root-cause investigation

show_full_output: true → false isn't mentioned in the PR body, and the whole reason this PR exists is that the previous outage was root-caused by reading the agent's own transcript (the linked failing run's tool-call log). Turning full output off in the same PR that fixes the last silent failure removes the exact signal that made that root-cause possible.

If the intent was to trim log noise now that the agent works, consider:

• keeping true for at least a few real-world runs to confirm the fix behaves as expected end-to-end, then flipping later, or
• exposing it as an action input (defaulting to false) so debugging future regressions doesn't require editing this file and re-releasing the composite action.

If it was accidental (i.e. bundled from a local experiment), revert it — the diff isn't otherwise related to output verbosity.

[henry-nsheaps]

❔ P2 — PR body / code discrepancy on where permission-mode auto lives

The PR body's "Fix" section says:

Add claude_args to the Run Claude Code step:

--permission-mode auto --allowedTools "mcp__github__*,Bash(gh:*),Bash(git:*)"

…but the actual claude_args value below only carries --allowedTools. auto is set via settings.permissions.defaultMode: "auto" (line 267) instead. Both are functionally valid, but this is a real divergence between "the fix I'm shipping" and "what's on disk," which matters if anyone later tries to reproduce the fix in another consumer of run-agent or claude-code-action from the PR description alone.

Two nits while here:

1. The inline comment above claude_args explains that --allowedTools must be a claude_args flag (because prepareMcpConfig/hasGitHubMcpTools reads it to decide whether to install the GitHub MCP server) — that's a great note. It would be worth adding one sentence about why defaultMode moved to settings.json rather than --permission-mode in claude_args, so a future reader doesn't try to "consolidate" them.
2. Optional consistency check: the deny list still uses settings.permissions.deny while defaultMode is now also there and the allow list is via claude_args. If there's a reason (there is — the comment above covers --allowedTools), the same reason arguably applies asymmetrically: deny rules apply in every mode per the permission-modes docs, so settings.json is fine for those. Worth a one-liner sibling comment near the deny list confirming the intent so it doesn't drift.

[henry-nsheaps]

⚠️ P2 — lost institutional knowledge on additionalDirectories

The comment that used to sit above settings: | is gone:

# additionalDirectories includes runner.temp because REVIEW_METRICS_PATH
# (set above) points at $RUNNER_TEMP/review-metrics.yaml, outside the
# workspace — without this the skill's mandatory metrics write is
# sandbox-denied and the receiver's metrics gate always fails.

The "${{ runner.temp }}" entry inside additionalDirectories is still there (line 272), but with no explanation, and the next person who "cleans up" that list has no way to know that dropping the entry silently breaks the receiver's metrics gate. This is exactly the kind of non-obvious "why" comment that should stay.

Suggest re-adding it just above "additionalDirectories": [ (or inline next to the "${{ runner.temp }}" entry). The PR body doesn't call out removing this comment, so I suspect it got lost in the reshuffle around adding claude_args.

[henry-nsheaps]

⚠️ P2 — dropped depends = "node" without acknowledging it

Old line:

"npm:@anthropic-ai/claude-code" = { version = "2.1.205", depends = "node" }

New line:

"npm:@anthropic-ai/claude-code" = { version = "2.1.128", allow_builds = ["@anthropic-ai/claude-code"] }

The removed comment said:

depends = "node" ensures node installs first, else the binary is missing ("claude native binary not installed", which fails claude plugin validate).

The PR body reframes the "native binary not installed" symptom as being caused by mise's default --ignore-scripts=true (which allow_builds now overrides), and treats the previous fix as based on a wrong root-cause theory. That's plausible — mise's npm: backend does require a resolvable node, so an implicit ordering is usually there — but the two mechanisms aren't mutually exclusive, and depends = "node" was cheap defense-in-depth against tool-installation race conditions.

Two concrete asks:

1. Note in the PR body (or a follow-up commit) why depends = "node" is no longer needed alongside allow_builds, so the reasoning survives if this regresses again. The current comment only explains allow_builds.
2. If there's any chance mise's npm-backend install order isn't deterministic across versions, put depends = "node" back — it's one field, doesn't conflict with allow_builds, and it costs nothing.

Also, the version downgrade 2.1.205 → 2.1.128 isn't loudly explained in the code. The PR body does say "the last version confirmed to install its native binary correctly" and "Avoid 2.1.146+ which ships a broken native-binary install regardless" — worth preserving that "avoid ≥2.1.146" warning in the mise.toml comment so Renovate reviewers don't just click "merge" on the next auto-bump PR.

Suggested comment update:

Suggested change

	"npm:@anthropic-ai/claude-code" = { version = "2.1.128", allow_builds = ["@anthropic-ai/claude-code"] }
	# npm's postinstall script fetches the native binary; mise's npm backend
	# ignores lifecycle scripts by default, so allow_builds explicitly approves
	# this package's postinstall to let that download run.
	# Pinned to 2.1.128: 2.1.146+ ships a broken native-binary install
	# regardless of allow_builds; do not bump past 2.1.145 without re-verifying
	# `claude plugin validate` succeeds in CI.
	"npm:@anthropic-ai/claude-code" = { version = "2.1.128", allow_builds = ["@anthropic-ai/claude-code"] }

[henry-nsheaps]

✅ Nice DRY-up — three copies of the same "mise + bun install" pair collapse into one composite. The pre-refactor lint job passed install: true to jdx/mise-action@v4; that input defaults to true upstream, so dropping it here is a no-op. No behavior change intended.

Two minor consistency nits (P2):

1. SHA-pin jdx/mise-action. Elsewhere in this repo third-party actions are pinned by SHA + tag comment (e.g. LouisBrunner/checks-action@937cbbcde3259005b50746dc91cde29098aac2ff # v3.1.0 in run-agent/action.yaml, qoomon/actions--context@e12e182be797e3476990faf801dd1b87f107f91c # v5, etc.). This composite action uses a floating @v4. The pre-refactor test.yaml was already floating so this doesn't regress, but centralizing the mise setup here is the right moment to also lock it — otherwise upstream v4 can silently move under CI. Not blocking.
2. bun install --frozen-lockfile is hardcoded. If any consumer of this composite ever needs --no-frozen-lockfile (e.g. a lockfile-regeneration workflow), they'd have to fork. YAGNI-fine for today; leaving a note in case that comes up.