claude-review.yml: workflow_dispatch tool grant is unquoted, so a dispatched re-review can never post

[kyle-sexton]

Summary

In the reusable claude-review.yml, the tool grant composed for the workflow_dispatch
re-review path is unquoted. The shell passes it through as one word, the action's argument
parser then whitespace-tokenizes it, and the resulting allowedTools array is garbage. On
a dispatched run the model therefore cannot call gh pr review or gh pr comment, so it
posts nothing. The lane still reports success.

Evidence

Consumer run: melodic-software/claude-code-plugins
run 34159099502,
workflow_dispatch with pr-number=3939
(PR #3939).
Run conclusion: success.

The run log shows the composed argument string reaching the action intact:

CLAUDE_ARGS: --model claude-sonnet-5 --max-turns 75 --allowedTools "Bash(gh pr diff:*)" --allowedTools Bash(gh pr comment:*),Bash(gh pr review:*),Bash(gh pr diff:*)

and then the parsed result:

SDK options: {
  "model": "claude-sonnet-5",
  "maxTurns": 75,
  "allowedTools": [
    "Bash(gh pr diff:*)",
    "Bash(gh",
    "pr",
    "comment:*)",
    "review:*)",
    "diff:*)"
  ],

The quoted base grant from claude-args survives as one entry. The unquoted dispatch grant
shatters into five fragments, none of which matches any real tool. The session then ran to
completion and posted nothing:

  "subtype": "success",
  "is_error": false,
  "duration_ms": 216274,
  "num_turns": 39,
  "permission_denials_count": 22,

39 turns, 22 permission denials, review_ran=true, review_failed=false, no review and no
comment on the PR.

Where it is in the workflow

.github/workflows/claude-review.yml, Compose Claude CLI arguments step, lines 695-700
as of the current default branch:

          # Repeated --allowedTools flags accumulate and dedupe.
          if [ "$EVENT_NAME" = "pull_request" ]; then
            args="$args --allowedTools mcp__github_inline_comment__create_inline_comment"
          else
            args="$args --allowedTools Bash(gh pr comment:*),Bash(gh pr review:*),Bash(gh pr diff:*)"
          fi

Line 697 is safe by accident: the MCP tool name is a single token with no whitespace, so
tokenizing it is a no-op. Line 699 is the only branch whose value contains spaces, and it is
the only one without quotes. The claude-args input default at line 209 already quotes its
grant for exactly this reason:

        default: >-
          --model claude-sonnet-5 --max-turns 75
          --allowedTools "Bash(gh pr diff:*)"

The dispatch branch also grants no file-reading tools at all, so even with the quoting fixed
a dispatched reviewer cannot Read, Grep, or Glob the checkout. That accounts for a
large share of the 22 denials.

Why the lane still reports green

The reusable's success test does not look for a posted review. claude-lane-outcome
(.github/actions/claude-lane-outcome/action.yml, around lines 91-94) sets:

          if (outcome === "success") {
            if (executionFile !== "" && fs.existsSync(executionFile)) {
              core.setOutput("review_failed", "false");
              core.setOutput("review_ran", "true");

An execution file existing is the whole test. A dispatched run that produced 39 turns of
nothing therefore reports review_ran=true and passes.

For context, the one check in the fleet that does read a posted review body is
consumer-owned, not part of this reusable:
melodic-software/claude-code-plugins/.github/workflows/claude-review.yml, the
review-skill-evidence job at line 85, whose own if is github.event_name == 'pull_request' and whose comment says it is "already pull_request-only". So on the dispatch
path nothing anywhere checks that a review was posted. The consumer can widen its own job,
but the reusable should not depend on a consumer opting in.

Proposed fix

1. Quote the dispatch grant and add the file-reading tools the branch never granted. Inside
   the existing double-quoted assignment the inner quotes need escaping:

   args="$args --allowedTools \"Bash(gh pr comment:*),Bash(gh pr review:*),Bash(gh pr diff:*),Read,Grep,Glob,LS\""

2. Give the reusable its own dispatch-mode evidence check, so a dispatched run that posts
   nothing cannot report success. The natural home is claude-lane-outcome or a step beside
   Report review outcome: on workflow_dispatch, confirm a review or comment landed on the
   target PR before calling the lane green.

A regression test asserting the parsed allowedTools array on the dispatch path would catch
this class directly. The bug is invisible in a diff of the compose step and visible only in
the action's parsed options.

Secondary observation: the pull_request timeout budget

The first pull_request run on the same PR,
run 34157222224
(346 changed files), spent roughly 11 minutes inside the Claude step (19:52:16 to 20:03:55)
and was cut off by the consumer's timeout-minutes: 15, posting an infra-status comment
instead of a review. That run also concluded success.

Consumers with large PRs may need a documented way to raise that budget, or at least
documented guidance on what the reusable expects, so the ceiling is a deliberate choice
rather than a surprise on the first big PR.