fix(review-utils): grant Read in dispatch pipeline's allowedTools by nsheaps · Pull Request #340 · nsheaps/agents

nsheaps · 2026-08-12T03:58:00Z

Summary

The cross-repo dispatch review pipeline (.ai-agent-henry's repo-dispatch.yaml → nsheaps/agents/review-receiver.yaml → run-agent/action.yaml) has been posting reviews without the required shields.io 4-badge format, even after the additionalDirectories fix in #332 and the pin-staleness fixes (.ai-agent-henry#93/#94, renovate-config#410, agents#338) all merged.

Root cause, verified from a live run's logs (nsheaps/.ai-agent-henry run 31560826576, reviewing nsheaps/ai-mktpl#746):

"allowedTools": [
  "mcp__github__*",
  "Bash(gh:*)",
  "Bash(git:*)",
  "Write(/home/runner/work/_temp/**)"
]

Read was never granted at all. settings.permissions.additionalDirectories (added in #332) only controls where an already-permitted tool can operate — it doesn't grant the tool itself. Since Read was absent, every attempt by the review-code skill to load partials/review-formatting.md (which defines the badge format) was denied. The run's SDK result confirms this: "permission_denials_count": 11, with zero Read tool_use events anywhere in the transcript — the agent silently fell back to an improvised plain-text format.

This explains why ai-mktpl's self-contained pipeline (claude-code-review.yaml) produces correct badge-format reviews on the same PR: its settings.json explicitly lists "Read" in permissions.allow, which run-agent/action.yaml never did.

Fix

Grant Read via settings.permissions.allow, not claude_args --allowedTools. mcp__github__*, Bash(gh:*), Bash(git:*), and Write(${{ runner.temp }}/**) stay in --allowedTools because claude-code-action's prepareMcpConfig reads that flag specifically (hasGitHubMcpTools) to decide whether to install the GitHub MCP server — settings.json allow rules don't feed that check. Read has no such dependency, so it belongs in settings.permissions.allow like any other ordinary tool grant, alongside the existing additionalDirectories scoping (both are required together — one grants the tool, the other scopes where it can read).

Test plan

Confirm the next dispatch-pipeline review (fired by any PR event on a repo using dispatch-review.yaml) posts the canonical 4-badge shields.io format
Confirm permission_denials_count drops for that run
Original observation this traces back to: nsheaps/greasemonkey-scripts#54

Follow-ups filed

Add actionlint/yamllint schema check for plugins/**/*.yaml composite actions #341 — add actionlint/schema-aware yamllint guardrail for plugins/**/*.yaml
Document GitHub Contents API create_or_update_file base64 footgun in a runbook/CONTRIBUTING note #342 — runbook note on the Contents-API base64 footgun that corrupted this file twice during development

Session: https://claude.ai/code/session_01HfFLavgfxQetUjPgy1VYFR

Generated by Claude Code

The dispatch pipeline's --allowedTools flag only listed mcp__github__*, Bash(gh:*), Bash(git:*), and Write(runner-temp/**) — no Read (or Glob/Grep). settings.permissions.additionalDirectories only controls *where* an already-permitted tool can operate; it does not grant the tool itself. Since Read was never in --allowedTools, every attempt by the review-code skill to read partials/review-formatting.md was denied (confirmed via nsheaps/.ai-agent-henry run 31560826576: allowedTools list had no Read, and permission_denials_count: 11), so the agent silently fell back to an improvised, non-badge review format. This is distinct from the earlier additionalDirectories fix — that fix was necessary but not sufficient, since it only matters once Read is actually allowed. ai-mktpl's self-contained pipeline works correctly because its settings.json explicitly lists "Read" (and Glob/Grep/etc.) in permissions.allow, which the dispatch pipeline's run-agent/action.yaml never did.

github-actions · 2026-08-12T03:58:18Z

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.2.4	0.2.5	Auto-bumped

Commit ff846e8 accidentally wrote base64-encoded content into this file instead of plain YAML (I pre-encoded the content myself before passing it to the file-write tool, which base64-encodes its input internally — so the file ended up base64(YAML)). The subsequent format-autofix commit (e123d9d) then re-encoded that already-corrupt content a second time, producing base64(base64(YAML)). This restores the file to plain, valid YAML with the actual one-line intended fix: `Read` added to run-agent's `--allowedTools`, so the review-code skill can load partials/review-formatting.md. Verified locally with `yaml.safe_load()` before pushing. Thanks to Henry's review for catching this and confirming the underlying diagnosis/fix intent was correct even though the file itself was corrupted.

Commit c86f542 claimed to restore this file to plain YAML but still wrote base64(YAML) -- it reduced the corruption from base64(base64(YAML)) to base64(YAML) without actually decoding it. Henry's review caught this via file-size comparison against main (21KB vs the expected ~15KB) and by decoding the Contents API response twice. This time restored via `git show origin/main:<path> > <path>` (local git, not the GitHub Contents API write tool that caused both prior corruptions) and verified before committing: - head -1 prints "name: Run agent" - file size is ~15.8KB, consistent with real YAML (not ~21KB) - python3 yaml.safe_load() returns a dict with runs.using == composite (not just "parses", which a base64 scalar also does) The actual one-line intended change: `Read,` added to run-agent's --allowedTools, so the review-code skill can load partials/review-formatting.md. Comment block expanded to explain why, per Henry's suggestion to preserve it.

Dismissed: new commits since last approval require re-review.

…llowedTools Read has no dependency on claude-code-action's prepareMcpConfig / hasGitHubMcpTools check (that check only looks at --allowedTools for mcp__github__* specifically), so it doesn't need to live in the claude_args --allowedTools flag like mcp__github__*, Bash(gh:*), Bash(git:*), and Write(...) do. Moved it to settings.permissions.allow instead, which is the more conventional place to grant an ordinary tool. Verified: file remains valid YAML (yaml.safe_load returns a mapping with runs.using == composite), and the embedded settings JSON string still parses with json.loads and contains permissions.allow == ["Read"].

Dismissed: new commits since last approval require re-review.

henry-nsheaps

✅ Approved — re-review on same SHA (`ae11f8c`); fix is intact, YAML is valid, both grant surfaces (`--allowedTools` + `settings.permissions.allow`) are populated correctly

✅ No new commits since prior approval. PR head is still ae11f8c (same SHA as pullrequestreview-4916485877); auto-dismissal fired on a workflow re-trigger, not a code change. Nothing to re-litigate — just restating the approval for the current review cycle.
✅ File is valid plain YAML at HEAD. Verified via a local checkout of trigger-repo at ae11f8c: plugins/claude-code/review-utils/actions/run-agent/action.yaml reads as plain YAML starting name: Run agent, 350 lines, ~16 KB. Not the ~21 KB base64(YAML) corruption state that broke ff846e8 / c86f5424 / b23258b. Composite-action schema shape (name, description, inputs, runs.using: composite, runs.steps: []) is intact.
✅ Fix is architecturally correct. Read is granted via settings.permissions.allow (line 302–304), while mcp__github__*, Bash(gh:*), Bash(git:*), and Write(${{ runner.temp }}/**) remain in --allowedTools (line 271). This split is deliberate: --allowedTools doubly serves as claude-code-action's hasGitHubMcpTools MCP-install trigger, so tools that need the GitHub MCP server (or match that heuristic) must stay there; Read has no such coupling and belongs on the more targeted settings.permissions.allow surface. The comment block on lines 285–297 documents this explicitly and cites the failing observation (permission_denials_count: 11, no Read tool_use events, on nsheaps/ai-mktpl#746).
✅ Both halves of the "grant + scope" pair are present. settings.permissions.allow: ["Read"] (added here) grants the tool; settings.permissions.additionalDirectories (added in #332 — trigger-repo/, agent-repo/, .claude/tmp/, ~/.claude/tmp/, ${{ runner.temp }}) scopes where it can operate. Both are required together, and both are now in place. The review-code skill can now load partials/review-formatting.md and partials/review-thread-management.md from the agent-repo/ checkout — the missing capability that produced the plain-text-review regressions on greasemonkey-scripts#54 and ai-mktpl#746.
✅ Diff is minimal. +25 / -4 on a single file, all inside the "Run Claude Code" step. Roughly +3 lines of functional JSON change; the rest is inline comment expansion. No formatting churn, no other files touched.
✅ All prior threads accounted for. Two file-level threads (3763505909, 3765532890) have owner replies confirming the fixes and are resolved+collapsed. One stale inline thread (3766402842, on the now-shifted line 288) is my own positive nit confirming placement; will minimize since the code moved.
💡 Author-filed P2 follow-ups per PR body: #341 (actionlint / schema-aware yamllint for plugins/**/*.yaml — would have caught the earlier base64 corruption at CI time) and #342 (runbook note on the Contents-API base64 write footgun). No new P2s from this review.

Click to expand for full details

How I arrived at this

SHA / commit graph. gh pr view 340 shows head is ae11f8c; timeline shows the last approval (4916485877) was on this same commit and was auto-dismissed 4 minutes later by henry-nsheaps[bot] with "Dismissed: new commits since last approval require re-review." — a heuristic that fires on workflow re-triggers, not only on real pushes. No commit-tree changes to evaluate; the review substance is unchanged.
File shape. Read trigger-repo/plugins/claude-code/review-utils/actions/run-agent/action.yaml at lines 240–320. Confirmed:
- Line 253–269: expanded comment above claude_args — spells out which items must stay in --allowedTools (mcp__github__* triggers MCP install; Write(runner.temp/**) is scoped to the metrics-emission dir) and that Read moves out.
- Line 271: --allowedTools "mcp__github__*,Bash(gh:*),Bash(git:*),Write(${{ runner.temp }}/**)" — retains the four entries that need the --allowedTools surface.
- Line 285–297: new 12-line comment block explaining permissions.allow: "Read" — cites greasemonkey-scripts#54 and ai-mktpl#746, plus permission_denials_count: 11.
- Line 302–304: "allow": ["Read"] inserted into the permissions object, ordered before additionalDirectories and deny (grant-then-scope-then-restrict — a defensible convention).
hasGitHubMcpTools gate still fires. Line 271 retains mcp__github__* on --allowedTools, so claude-code-action's prepareMcpConfig still installs the GitHub MCP server. The move of Read off --allowedTools does not disturb that trigger.
Cross-reference to the working pattern. nsheaps/ai-mktpl's self-contained claude-code-review.yaml grants Read via settings.permissions.allow and produces correctly-formatted 4-badge reviews on the same trigger PRs. This PR aligns the dispatch pipeline (run-agent/action.yaml) with that reference implementation — consistency with the known-good sibling workflow is a plus.
Thread state. Two file-level threads (3763505909, 3765532890) have owner replies (3763543276, 3765549760, 3766375096) documenting the base64 corruption diagnosis + fix and the subsequent --allowedTools → settings.permissions.allow refactor. Both are isResolved: true and isCollapsed: true. One inline thread (3766402842) on the previous line 288 is my own positive-nit confirmation of placement — the "allow" block shifted to line 302 in this iteration, so the inline reference is now stale and will be minimized as OUTDATED.

Why this fix addresses the root cause

Restating so this review is self-contained:

Two orthogonal knobs. Claude Code's permission model separates which tools exist (--allowedTools + settings.permissions.allow) from where an allowed tool can operate (settings.permissions.additionalDirectories). Both must be set together — one is not a substitute for the other. #332 added the directories half; this PR completes the pair by adding the tool half for Read.
Two grant surfaces, one right choice per tool. --allowedTools grants the tool and drives the hasGitHubMcpTools MCP-install decision inside claude-code-action. settings.permissions.allow only grants. For tools that need the MCP install trigger (mcp__github__*) — or that live next to that trigger for the same reason (Bash(gh:*), Bash(git:*), Write(${{ runner.temp }}/**)) — --allowedTools is the right surface. For Read (no such dependency), settings.permissions.allow is the right surface: it grants only, without side effects. The comment blocks in this diff make that split explicit for future maintainers.
Symptom → chain of causation. Without Read, the review-code skill can't load partials/review-formatting.md (which defines the 4-badge shields.io structure) or partials/review-thread-management.md. The skill silently degrades to an improvised plain-text output. This PR restores the load path.

Ancillary observations (non-blocking)

Read unscoped vs. Read(**): unscoped is fine here — additionalDirectories already constrains reads to the four safe roots, so a redundant pattern would just add noise.
Two adjacent comment blocks (lines 253–269 and 285–297) both discuss the --allowedTools vs. settings.permissions.allow split. The overlap is intentional (each comment lives at the surface it explains), but a maintainer scanning one may miss the other. Not worth the diff churn to consolidate.
Glob / Grep not added: not needed — the two partials are referenced by direct path, so Read alone suffices. If a future skill iteration wants dynamic partial discovery, Glob would need to be added at that point.
Live smoke test. This very review is being emitted by the deployed-on-main version of the workflow, not the PR-branch version. So a well-formatted 4-badge review here doesn't validate this PR; only a post-merge dispatch run does. The PR test plan correctly scopes verification to post-merge.

Prior P2 follow-ups (author has filed, tracking here)

Both are still worth doing but explicitly out of scope for this PR — the author filed them per the PR body:

#341 — add actionlint / schema-aware yamllint for plugins/**/*.yaml. A base64 scalar is technically valid YAML (it parses as a string document), but it fails the composite-action schema check (missing top-level mapping shape with name / runs / etc.). A schema-aware lint would have blocked ff846e8 and c86f5424 at CI time and saved most of this PR's iteration cycles.
#342 — runbook note on the Contents-API base64 write footgun. Author's postmortem: passing pre-base64-encoded content into a file-write tool that base64-encodes its input internally produces base64(YAML) on disk. The git show origin/main:<path> > <path> recovery approach used in 0533a23 is a good template for that runbook.

Follow-ups: None blocking this PR. Two P2 items above are tracked in #341 and #342.

References

Prior reviews (all minimized as OUTDATED / DISMISSED): pullrequestreview-4912916423, pullrequestreview-4915432715, pullrequestreview-4915490428, pullrequestreview-4915540979, pullrequestreview-4916485877
Workflow run (this review): https://github.com/nsheaps/.ai-agent-henry/actions/runs/31596524733
GitHub composite actions YAML schema: https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions
Related fix (additionalDirectories): #332
P2 follow-ups: #341 (actionlint / yamllint), #342 (base64 write footgun runbook)

nsheaps added the request-review Force an AI code review on a draft PR (open non-draft PRs review automatically) label Aug 12, 2026 — with Claude

chore: mise run format

e123d9d