Skip to content

[OMNIML-6273] Let the Claude review bot submit its review again - #3156

Merged
shengliangxu merged 1 commit into
mainfrom
shengliangx/claude-review-allowlist-fix
Oct 31, 2026
Merged

shengliangxu merged 1 commit into
mainfrom
shengliangx/claude-review-allowlist-fix

Conversation

@shengliangxu

@shengliangxu shengliangxu commented Oct 31, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Type of change: Bug fix (CI workflow)

Since #3149 merged, /claude review still runs and reaches a verdict, but it can't publish it. The CLI refuses every review and approval the bot submits with "This command requires approval", and the run still ends green. Only inline comments get through.

Root cause. #3149 replaced Bash(gh pr review:*) with Bash(gh api repos/*/pulls/*/reviews:*) in --allowedTools. A Bash rule ending in :* is a literal prefix, in which an inner * matches only itself. So the rule never matched the command the prompt prescribes, gh api repos/NVIDIA/Model-Optimizer/pulls/<N>/reviews --method POST .... With gh pr review gone, the bot had no other way to post. These runs show it (search the log for permission_denied):

Fix.

  • The rule names this PR's reviews endpoint, built from the workflow's REPO and PR_NUMBER: Bash(gh api repos/${{ env.REPO }}/pulls/${{ env.PR_NUMBER }}/reviews:*). Actions expands the expressions before the action reads claude_args, so the CLI gets a literal prefix. The bot can review only the PR it was asked to review, which is narrower than both gh pr review:* and the wildcard ci: establish a shared review baseline and adopt it in Claude reviews #3149 intended.
  • The prompt's two publish commands show the same expanded endpoint. One added sentence tells the bot to run the command as written, because other forms are refused.
  • The rest of ci: establish a shared review baseline and adopt it in Claude reviews #3149 is unchanged: the commit_id pin, the head re-read before publishing, and the publishing policy.

Usage

No change: comment /claude review on a PR.

Testing

issue_comment workflows run the default branch's workflow file, so this change can run live only after it merges. After that, a /claude review on any open PR should land a review or an approval.

Checked locally:

  • claude-code-action e4a6faad497d (v1.0.255), the pinned version, installs Claude Code 2.1.305: claudeCodeVersion is set in src/entrypoints/run.ts, and the run logs print Installing Claude Code v2.1.305.
  • I ran Claude Code 2.1.305 in print mode, isolated from any user settings, under each rule. Instead of gh it ran ./ghx, a stand-in script that only prints its arguments. echo can't be used here because the CLI auto-allows it as a read-only command.
--allowedTools rule ./ghx api repos/NVIDIA/Model-Optimizer/pulls/2779/reviews --method POST -f event=COMMENT ./ghx api -X POST repos/NVIDIA/Model-Optimizer/pulls/2779/reviews -f event=COMMENT ./ghx hello (control)
Bash(./ghx api repos/*/pulls/*/reviews:*) (as on main) refused refused refused
Bash(./ghx api repos/NVIDIA/Model-Optimizer/pulls/2779/reviews:*) (this PR, expanded) runs refused refused
  • Claude Code 2.1.314 gives the same results.
  • The expanded rule, inside this PR's full --allowedTools list, also lets realistic bodies through on 2.1.305: multi-line single-quoted bodies with backticks, '"'"' apostrophes and </>, and double-quoted bodies with escaped backticks.
  • The CLI's own check still refuses a body line that starts with # ("Newline followed by # inside a quoted argument"). That check predates ci: establish a shared review baseline and adopt it in Claude reviews #3149, and the shared baseline's summary format allows no headings.
  • Pre-commit passes.

Before your PR is "Ready for review"

  • Is this change backward compatible?: N/A (CI workflow only)
  • If you copied code from any other sources or added a new PIP dependency, did you follow guidance in CONTRIBUTING.md: N/A
  • Did you write any new necessary tests?: N/A (an allow-list rule, checked against the CLI as above)
  • Did you update Changelog?: N/A (CI only)
  • Did you get Claude approval on this PR?: ❌ (/claude review can't publish until this fix is on main)

Additional Information

Summary by CodeRabbit

  • Chores
    • Automated review submissions now use a destination tied to the current repository and pull request for both approval and comment reviews. Submission commands must follow the supported format, and review actions are scoped to the pull request being reviewed rather than arbitrary repository or pull-request endpoints. The same scope applies consistently across both review types.

#3149 replaced Bash(gh pr review:*) with Bash(gh api repos/*/pulls/*/reviews:*) in the bot's
allowed tools. A Bash rule ending in ":*" is a literal prefix, in which an inner "*" matches only
itself, so the rule never matched "gh api repos/NVIDIA/Model-Optimizer/pulls/<N>/reviews ...".
Since then the CLI has refused every review and approval with "This command requires approval".

Spell the rule as this PR's reviews endpoint, from the workflow's REPO and PR_NUMBER, and expand
the same endpoint in the prompt's two publish commands, so the command the bot is told to run is
the one the rule admits. Tell the bot to run it as written: other spellings, such as
"gh api -X POST <endpoint>" or "gh pr review", are refused.

Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
@shengliangxu
shengliangxu requested a review from a team as a code owner October 31, 2026 01:20
@shengliangxu

Copy link
Copy Markdown
Collaborator Author

/ok to test 732fb1a

@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between d464d30 and 732fb1a.

Files selected for processing: 1
  • .github/workflows/claude_review.yml

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

The Claude review workflow now restricts review-submission API access to the current repository and pull request. Its approval and comment instructions use that endpoint and disallow alternate command forms.

Changes: Review submission endpoint

Layer / File(s) Summary
Restrict review submission commands
.github/workflows/claude_review.yml
The gh api allow-list and publishing instructions specify the current repository and pull request endpoint. The instructions require the endpoint to immediately follow gh api and disallow alternate command forms.

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Possibly related PRs

Suggested reviewers: chenhanyu

Merge Risk: Minimal · up to 732fb

This change fixes the review bot's allow-list so it can publish reviews on the PR it was asked to review. The reported over-permissioning concern was disproved. Live validation is only possible after merge, because issue_comment workflows use the default branch's workflow file. That is normal uncertainty and not a merge blocker.

Pre-merge checks: Passed 6
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly summarizes the main change: enabling the Claude review bot to submit reviews again.
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Security Anti-Patterns Passed The pull request changes only .github/workflows/claude_review.yml. It adds no Python changes under the ModelOpt package or examples, no dependency changes, and none of the listed security anti-patte…
Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)

Full details: Security Anti-Patterns

Explanation

The pull request changes only .github/workflows/claude_review.yml. It adds no Python changes under the ModelOpt package or examples, no dependency changes, and none of the listed security anti-patterns are introduced.

Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.34%. Comparing base (d464d30) to head (732fb1a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3156   +/-   ##
=======================================
  Coverage   80.34%   80.34%           
=======================================
  Files         675      675           
  Lines       77221    77221           
=======================================
  Hits        62042    62042           
  Misses      15179    15179           
Flag Coverage Δ
unit 62.62% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@meenchen meenchen 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.

Bot review (bedrock-claude-opus-5-5) — DM the bot to share feedback.

Approve: spelling out this PR's literal reviews endpoint in --allowedTools and in the prompt fixes the broken publish step. It also narrows what the bot can post to, and nothing that works today can break.

No action needed.

No action needed:

  • REPO and PR_NUMBER are job-level env (github.repository, github.event.issue.number). The same with: block already expands ${{ env.REPO }} in the prompt header, so the claude_args expansion works. Neither value is attacker-controlled.
  • The prompt's publish commands now match the allow-list rule character for character. Before, the prompt wrote $REPO/$PR_NUMBER, and the CLI checks the command text without expanding variables, so that form would not have matched.
  • The live check only happens after merge, because issue_comment runs the workflow file from the default branch. Run /claude review on an open PR to confirm.

@ChenhanYu ChenhanYu 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.

Automatic approval delegated by chenhany (mode: approve).

Pensieve approve review using chenhany's profile (revision 12).

Approve: /reviews prefixes match exactly; steps.with env-expansion, commit_id, head re-read, and policy are unchanged; follow-ups: unlisted pulls/<N>/comments re-review replies and draft #2977 rebasing (baseline d464d30b120254151309c217a8410adde6ee1c45; CLI unexecuted; history limited).

No action needed.

Review baseline: https://github.com/NVIDIA/Model-Optimizer/blob/d464d30b120254151309c217a8410adde6ee1c45/.agents/review-guidelines/code-review.md

Review run: 8bf09a86686f4f069ef45e705a154fe9

@shengliangxu
shengliangxu merged commit e352cac into main Oct 31, 2026
36 checks passed
@shengliangxu
shengliangxu deleted the shengliangx/claude-review-allowlist-fix branch October 31, 2026 04:16
@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-31 04:17 UTC

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants