Skip to content

ci: fix /claude-review prompt — inject PR number for issue_comment trigger#1991

Closed
chongchonghe wants to merge 1 commit into
developmentfrom
chong/github/claude-review-fix-2
Closed

ci: fix /claude-review prompt — inject PR number for issue_comment trigger#1991
chongchonghe wants to merge 1 commit into
developmentfrom
chong/github/claude-review-fix-2

Conversation

@chongchonghe

Copy link
Copy Markdown
Contributor

Description

The `/claude-review` workflow was still failing after #1987 because the prompt only included the PR number for `workflow_dispatch` events, not for `issue_comment` events. Claude received an empty context string and spent all 30 turns trying to discover which PR to review via `env`, `printenv`, and shell variable expansion (all blocked by the permission system).

Root cause: The expression `${{ github.event_name == 'workflow_dispatch' && format(...) || '' }}` evaluates to `''` for `issue_comment` triggers, so Claude gets no PR context.

Fix:

  1. Extend the prompt expression to inject `Review PR #N in repo.` for all three trigger types (`issue_comment`, `pull_request`, `workflow_dispatch`).
  2. Expand `.claude/settings.json` allowlist with the commands Claude actually needs: `git fetch`, `gh api`, `cat`/`head`/`tail`/`echo`, `env`, `printenv`.

Related issues

Follows #1987 (permission denials fix).

Checklist

  • I have added a description (see above).
  • I have added a link to any related issues (if applicable; see above).
  • I have read the Contributing Guide.
  • I have added tests for any new physics that this PR adds to the code.
  • (For quokka-astro org members) I have manually triggered the GPU tests with the magic comment `/azp run`.

The issue_comment trigger was passing an empty prompt because the PR-number
expression only handled workflow_dispatch. Claude spent all 30 turns trying
to discover the PR via env/printenv/shell-expansion (all denied).

Fix: extend the prompt expression to inject 'Review PR #N in repo' for all
three trigger types (workflow_dispatch, issue_comment, pull_request).

Also expand settings.json allowlist with the commands Claude actually needs:
git fetch, gh api, cat/head/tail/echo, env, printenv.
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. CI github_actions Pull requests that update GitHub Actions code labels Jun 23, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request expands the list of allowed Bash commands in .claude/settings.json to include commands such as git fetch, gh pr list, gh api, and environment inspection utilities like env and printenv. The review identifies significant security vulnerabilities introduced by these changes: allowing env and printenv risks exposing sensitive repository secrets (like GITHUB_TOKEN) to prompt injection attacks, while allowing arbitrary gh api * commands could lead to privilege escalation. It is highly recommended to remove these commands from the allowlist to maintain a secure environment.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread .claude/settings.json
Comment on lines +25 to +28
"Bash(echo *)",
"Bash(env)",
"Bash(printenv)",
"Bash(printenv *)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

Allowing the execution of env and printenv commands poses a significant security risk. In GitHub Actions, the environment contains sensitive tokens (such as GITHUB_TOKEN) and potentially other repository secrets. Since this workflow is triggered by issue_comment events, an external contributor could use prompt injection to force Claude to run these commands and exfiltrate secrets.\n\nSince the PR number is now successfully injected into the prompt, Claude no longer needs to inspect the environment to discover the context. These commands should be removed from the allowlist.

      "Bash(echo *)"

Comment thread .claude/settings.json
"Bash(gh pr diff *)",
"Bash(gh pr list *)",
"Bash(gh issue view *)",
"Bash(gh api *)",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

Allowing arbitrary gh api * commands enables the execution of any GitHub API request using the runner's GITHUB_TOKEN. If the token has write permissions, a prompt injection attack could exploit this to modify repository resources, create releases, or comment on issues maliciously.\n\nConsider removing gh api * or restricting it to specific safe subcommands if absolutely necessary.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f0fb518a36

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .claude/settings.json
Comment on lines +26 to +28
"Bash(env)",
"Bash(printenv)",
"Bash(printenv *)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep env dumping commands out of Claude's allowlist

This review workflow grants id-token: write, passes ANTHROPIC_API_KEY, and has show_full_output: true; allowing env/printenv gives a prompt-injected review run, such as malicious PR text processed after /claude-review, a direct way to read runner credentials like OIDC request variables and API tokens and then expose them in logs or comments. The PR number is now passed explicitly, so environment inspection should stay disallowed or be replaced with a narrowly scoped, non-secret diagnostic.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good point.

Comment thread .claude/settings.json
"Bash(gh pr diff *)",
"Bash(gh pr list *)",
"Bash(gh issue view *)",
"Bash(gh api *)",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restrict gh api to read-only endpoints

The GitHub CLI manual for gh api says request parameters switch calls to POST and --method can override the HTTP method, so Bash(gh api *) grants more than read-only PR discovery. In this workflow the token can write issues and pull requests, and Claude processes PR/comment content; a prompt-injected review can now run mutating API calls such as PATCH/DELETE against PR or issue state. Please keep using gh pr view/gh pr diff or allow only the specific GET endpoints needed for review context.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good point.

@BenWibking BenWibking left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the AI comments. I think it should be scoped to avoid prompt-injection attacks.

@chongchonghe

Copy link
Copy Markdown
Contributor Author

This is a wrong approach. Replaced by #2003

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI github_actions Pull requests that update GitHub Actions code size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants