ci(claude): allow fork-PR authors to trigger Claude review

[kovtcharov]

Summary

The claude-code-action gates execution on the actor's repo permission. Fork-PR authors only have read, so the action exits with "Actor does not have write permissions to the repository" before ever calling Anthropic — which is why PR #924 from @theonlychant (and any future fork PR) gets a 5-second red ❌ on the Claude AI Assistant check. Setting allowed_non_write_users: "*" on pr-review and issue-handler bypasses that gate so external contributors get the same auto-review maintainers do, with no username allowlist to maintain.

Threads

• pr-review + issue-handler get allowed_non_write_users: "*" — these are the two jobs that take input from non-maintainers (fork-PR diffs, @claude mentions on issues / PR conversations).
• pr-comment and release-notes left alone — pr-comment is already filtered to non-fork PRs (head.repo.full_name == github.repository), and release-notes only fires from a maintainer-owned tag push via workflow_run. The actor on those is always a write user, so the gate is a no-op there.
• Header comment documents the residual risk — fork-PR diff feeds into Claude, which has Bash in --allowedTools and access to ANTHROPIC_API_KEY + GITHUB_TOKEN. Prompt injection in the diff could try to coerce Claude into running an exfiltration command. The action's built-in mitigations (subprocess secret scrubbing via CLAUDE_CODE_SUBPROCESS_ENV_SCRUB=1 auto-set when this input is non-empty, pinned bun binary, hardened PATH) reduce — but don't eliminate — the surface. Future-us reading the header knows what to tighten if a real attempt lands (swap * for a literal username list — verified upstream in src/github/validation/permissions.ts that allowed_non_write_users only takes literal usernames or *, not author_association values like CONTRIBUTOR).

Test plan

pull_request_target uses the workflow from main, not the PR head, so this PR can't validate itself. The verification path is post-merge:

• Merge to main
• Re-trigger PR feat(agents): split ChatAgent into Chat, FileIO, and DocumentQA agent… #924 (push a commit to it, or close/reopen) and confirm the Claude AI Assistant check now runs to completion instead of failing in 5s with the permission error
• Confirm the run log shows ⚠️ SECURITY WARNING: Bypassing write permission check for theonlychant due to allowed_non_write_users configuration (the action emits this when the bypass takes effect)
• Sanity-check that pr-comment (non-fork-only) still works on a maintainer's @claude review-comment reply

[github-actions]

Summary

Surgical 3-hunk change adding allowed_non_write_users: "*" to the two jobs that take input from non-maintainers (pr-review via pull_request_target, issue-handler via issues / issue_comment), plus a 12-line header block documenting the residual prompt-injection risk and the escape hatch (swap * for a literal username list). The scoping is correct: pr-comment is already filtered to same-repo PRs (head.repo.full_name == github.repository, line 334) and release-notes runs only on maintainer-owned tag pushes via workflow_run — adding the bypass to either would be a no-op. The single most important thing for the author/reviewer to know: this does widen the attack surface in exchange for fork-PR ergonomics, and the mitigation story leans entirely on upstream (claude-code-action) defense-in-depth rather than anything in this repo.

Issues Found

🟡 Important — claude_args for pr-review / issue-handler includes Edit,Write against repo files (claude.yml:322, 438)

Both fork-reachable jobs ship the same toolset:

--allowedTools Edit,Read,Write,Grep,Glob,Bash

The prompts only ever ask Claude to write /tmp/review.md / /tmp/reply.md and post via gh, so Edit against repo files isn't load-bearing. With allowed_non_write_users: "*" now letting an external author trigger these, a successful prompt injection has Edit + Write + Bash against the checked-out base-branch tree. The header already names secret exfiltration as the headline risk; arbitrary file writes (e.g. dropping a payload into a workflow file the next maintainer-triggered run picks up) is a quieter variant worth closing.

This isn't blocking — the PR is correctly scoped and pull_request_target itself doesn't push the worktree anywhere — but if you want the bypass to be defensible long-term, the cheapest follow-up is to either:

• Drop Edit from these two jobs' claude_args (review/reply doesn't need it), or
• Add a --disallowedTools line restricting Bash patterns (git push, curl, wget, nc, etc.) for the fork-reachable surface.

Worth a follow-up issue rather than blocking this PR, since the trade-off is documented and reverting is one config flip.

🟢 Minor — Header reference to src/github/validation/permissions.ts is ambiguous (claude.yml:39)

// no `author_association` values supported — checked
// upstream `src/github/validation/permissions.ts`, only literal usernames or `*`

Reads as a path in this repo on first scan. A future maintainer trying to verify the claim will grep src/github/... and come up empty. Suggest naming the source repo + file explicitly:

# this to a literal username list (no `author_association` values supported — verified
# in upstream anthropics/claude-code-action `src/github/validation/permissions.ts`,
# which only accepts literal usernames or `*`).

🟢 Minor — Test plan correctly notes self-test isn't possible, but the verification log line is worth pinning

The PR description references the upstream warning string ⚠️ SECURITY WARNING: Bypassing write permission check for <user> due to allowed_non_write_users configuration. That string is upstream-controlled and could change between SHAs. Once verified post-merge, consider adding it to the file header as the canonical signal so future-us doesn't re-derive it. Optional, not blocking.

Strengths

• Header comment is genuinely useful, not just performative. Names the threat (prompt-injection in fork diff → Bash exfiltration via ANTHROPIC_API_KEY / GITHUB_TOKEN), the mitigations actually in play (CLAUDE_CODE_SUBPROCESS_ENV_SCRUB=1, pinned bun, hardened PATH), and the explicit escape hatch (literal username list). This is the rare "we accepted residual risk" comment that future-us can actually act on.
• Scoping is right. Touching only pr-review and issue-handler — and explicitly not pr-comment (head.repo.full_name == github.repository at line 334 makes the bypass moot) or release-notes (workflow_run from maintainer tag push, actor is always a write user) — shows the author traced each job's trigger surface rather than carpet-bombing the file.
• Test plan honestly acknowledges the pull_request_target self-test gap. The post-merge verification path (re-trigger PR feat(agents): split ChatAgent into Chat, FileIO, and DocumentQA agent… #924, confirm bypass log line) is concrete and falsifiable.

Verdict

Approve with suggestions — change is minimal, scoped correctly, and the security trade-off is documented in the right place (the workflow header, not a buried PR comment). The 🟡 on claude_args toolset is a hardening follow-up, not a blocker; the 🟢 items are textual. Safe to merge once the upstream-path clarification on line 39 is folded in.