Skip to content

ci: quote the review bot's allowed-tools value - #164

Merged
fullstackjam merged 1 commit into
mainfrom
claude/fix-review-bot-allowlist
Aug 2, 2026
Merged

ci: quote the review bot's allowed-tools value#164
fullstackjam merged 1 commit into
mainfrom
claude/fix-review-bot-allowlist

Conversation

@fullstackjam

@fullstackjam fullstackjam commented Aug 2, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Double-quotes the --allowed-tools value in claude-code-review.yml so the review bot can actually post its review.

Why?

The claude-review job has never posted anything. It runs, burns a few minutes and ~$1.50 of tokens, exits green, and drops the review on the floor. On #163 it did this three times in a row (7, then 10 permission denials, then a third silent run). Every PR in this repo has been merging with the review gate silently absent — the job's green tick made it look closed.

claude_args is parsed with a shell-style splitter — the action ships base-action/test/parse-shell-args.test.ts, which has cases for both single- and double-quoted values containing spaces. Unquoted, Bash(gh pr diff:*) splits on its own spaces. The run log from #163 shows what the CLI actually received:

"allowedTools": [
  "mcp__github_inline_comment__create_inline_comment",
  "Bash(gh",
  "pr",
  "diff:*)",
  "view:*)",
  "comment:*)"
]

The MCP tool survived (no spaces), which is why the workflow's existing comment about pull-requests: write was a correct diagnosis of a different earlier bug. But Bash(gh, pr, diff:*) match no real permission, so every gh pr diff / view / comment call hit the gate. With nothing readable and nothing postable, the run buffered a review and discarded it.

Wrapping the value in double quotes makes it one token. The comment above the line now records why the quotes are load-bearing, so nobody helpfully removes them.

Testing

  • go vet ./... passes — n/a, no Go code changed

  • Relevant tests added or updated — n/a; this is CI config with no test hook. Verified by parsing the edited YAML and running the value through shlex.split, the same splitting model the action uses:

    '--allowed-tools'
    'mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr comment:*)'
    

    Two tokens, allowlist intact — versus the six mangled fragments above.

  • Tested locally — and here is where the plan failed. I wrote this PR intending it to test itself. It cannot. claude-code-action refuses to run on any PR that modifies its own workflow file:

    Workflow validation failed. The workflow file must exist and have identical content to the version on the repository's default branch. [...] your workflow will begin working once you merge your PR.

    That is a deliberate security guard (otherwise a PR could rewrite the reviewer that vets it). The claude-review job here finished in 10s having never started Claude — versus 2–3 minutes on docs: realign branch-protection sources of truth with live protection #163. So this fix is verified by reasoning, not end to end, and it cannot be verified before merging. Proof will be the first non-workflow PR after this merges: if the bot posts a review there, the fix worked.

Cross-repo checklist

  • Does this need a docs/content update in openboot.dev? — No.
  • Does this change the CLI ↔ server API contract? — No.

Notes for reviewer

Merge criterion. Green CI proves nothing here — a green claude-review job is exactly what both the broken state and the workflow-validation skip produce. What to review is the reasoning: the mangled allowedTools array quoted above is from #163's actual run log, and the shlex.split output above is what the quoted form produces. If you accept those two, the fix follows.

This touches .github/workflows/, which the repo's own ship-pr skill flags as wanting a human glance rather than an agent merge.

Follow-ups from #163, still open and not addressed here: old-cli compat fails open (its release lookup ends in || true and every later step is gated on a non-empty version, so a green tick is not evidence it tested anything), and CONTRIBUTING.md:46 / auto-release.yml:179,184 instruct the release operator to verify L4 is green on main, which vm-e2e-spike.yml's trigger makes impossible.

The claude-review job has never been able to post. On PR #163 it ran
three times, each ending in permission denials and "No buffered inline
comments" — 7, then 10, then silence — so no PR in this repo has had a
bot review despite the workflow being wired up.

claude_args is parsed with a shell-style splitter (the action ships
base-action/test/parse-shell-args.test.ts, which covers both quote
styles). Unquoted, `Bash(gh pr diff:*)` splits on its spaces. PR #163's
run log shows the resulting allowlist:

  "mcp__github_inline_comment__create_inline_comment",
  "Bash(gh", "pr", "diff:*)", "view:*)", "comment:*)"

None of those match a real permission, so every `gh pr diff/view/comment`
call hit the gate and the buffered review was dropped.

Double-quoting the value keeps it a single token. Verified with
shlex.split, which is the same splitting model:

  '--allowed-tools'
  'mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),...'

This PR is its own test: if the fix works, the bot posts a review here.
@github-actions github-actions Bot added the ci CI/CD changes label Aug 2, 2026
@fullstackjam
fullstackjam merged commit 635cf84 into main Aug 2, 2026
14 checks passed
@fullstackjam
fullstackjam deleted the claude/fix-review-bot-allowlist branch August 2, 2026 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci CI/CD changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant