Skip to content

Fix code review workflow trigger and hardening#7

Open
xdanger wants to merge 1 commit intomainfrom
xdanger-codex/review-codereview-workflow
Open

Fix code review workflow trigger and hardening#7
xdanger wants to merge 1 commit intomainfrom
xdanger-codex/review-codereview-workflow

Conversation

@xdanger
Copy link
Copy Markdown
Member

@xdanger xdanger commented Mar 30, 2026

Summary

  • add explicit workflow_call inputs so the review job works for both direct PR runs and reusable callers
  • make the Copilot reviewer request best-effort instead of blocking the main review job
  • tighten workflow permissions and Claude's allowed tool surface to reduce unnecessary write access
  • deepen checkout history, add review guidance, cap turns, and pin third-party actions to SHAs

Testing

  • Not run (not requested)

@github-actions github-actions bot requested a review from Copilot March 30, 2026 20:43
@xdanger xdanger review requested due to automatic review settings March 30, 2026 20:43
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 30, 2026

Greptile Summary

This PR hardens the reusable code-review.yml workflow by adding explicit workflow_call inputs for PR context, removing excess permissions (id-token: write, issues: write), pinning third-party actions to SHAs, simplifying the Claude prompt, and making the Copilot reviewer request non-blocking. The changes are well-reasoned and move in a clear positive direction, but one issue needs attention before merge.

Key changes:

  • workflow_call inputs (pr_number, head_repo_full_name) added so reusable callers can pass PR context explicitly
  • Concurrency group now scoped to github.repository to prevent cross-repo key collisions
  • id-token: write and issues: write permissions removed — only pull-requests: write is retained
  • Third-party actions pinned to full commit SHAs (actions/checkout, anthropics/claude-code-action)
  • --max-turns 30 cap added; WebSearch/WebFetch and the trailing comma removed from --allowedTools
  • Copilot reviewer step marked continue-on-error: true
  • ANTHROPIC_BASE_URL env var dropped (aligns with commit f6a9c53 which removed the corresponding secret input)

Issues found:

  • P1 — claude_args comment line will be passed literally to the CLI (line 83): The # Keep the tool surface narrow… line sits inside a YAML literal block scalar (|) and is therefore part of the string value, not a YAML comment. If claude-code-action expands args without shell eval, this unrecognised token will likely cause the step to fail on every run.
  • P2 — No draft-PR guard on the workflow_call path: Unlike the pull_request branch, the workflow_call condition does not check for draft status, leaving callers to enforce this themselves.
  • P2 — fetch-depth: 0 clones full history: Switching from a shallow clone to an unbounded full history adds unnecessary time and bandwidth for large repos; a shallow depth of 10–50 would satisfy git log/git blame needs.

Confidence Score: 3/5

Not safe to merge as-is — the literal comment line in claude_args will likely break the review step on every run.

The overall direction and structural changes are solid (pinned SHAs, tighter permissions, correct workflow_call wiring). However, the # comment line embedded inside the claude_args YAML block scalar is a runtime defect that would silently or noisily break the primary review step. Fixing that one line is a trivial change that would bring this to a clean 4 or 5.

.github/workflows/code-review.yml — specifically line 83 (comment in claude_args block scalar)

Important Files Changed

Filename Overview
.github/workflows/code-review.yml Adds workflow_call inputs, tightens permissions, pins action SHAs, simplifies the Claude prompt, and caps turns — but a literal # comment line inside the claude_args block scalar will likely break the step at runtime.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([Trigger]) --> B{Event type?}
    B -- pull_request --> C{Is draft?}
    B -- workflow_call --> D{pr_number set?}
    C -- Yes --> Z([Skip — draft PR])
    C -- No --> E{Same-repo fork?}
    D -- No --> Z2([Skip — no PR context])
    D -- Yes --> F{head_repo_full_name match or unset?}
    E -- No --> Z3([Skip — fork])
    E -- Yes --> G[review job]
    F -- No --> Z4([Skip — fork])
    F -- Yes --> G
    G --> H[Request Copilot review
continue-on-error: true]
    H --> I[Checkout @ SHA
fetch-depth: 0]
    I --> J[claude-code-action @ SHA
--model opus
--max-turns 30]
    J --> K([Post inline review comments])
Loading

Reviews (1): Last reviewed commit: "🐛 fix: harden reusable code review work..." | Re-trigger Greptile

--model opus
--allowedTools "Read,Glob,Grep,WebSearch,WebFetch,mcp__github_inline_comment__create_inline_comment,Bash(gh api:*),Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr checks:*),Bash(git log:*),Bash(git blame:*),Bash(git diff:*),"
--max-turns 30
# Keep the tool surface narrow: inline comments plus read-only PR inspection.
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 Shell comment inside claude_args block scalar will be passed literally to the CLI

The line # Keep the tool surface narrow: inline comments plus read-only PR inspection. is inside a YAML literal block scalar (|), so it is not a YAML comment — it is a literal string that becomes part of the claude_args value passed to claude-code-action.

If the action expands claude_args into CLI arguments without shell evaluation (e.g., by splitting on newlines and passing each line as a separate arg), then # would be handed to the Claude CLI as an unrecognised argument and likely cause the step to fail. Only if the action uses eval "claude $claude_args" would the line be silently ignored as a shell comment.

Looking at typical claude-code-action implementations, arguments are split by whitespace/newlines and passed directly — not via eval — so this comment line is likely to break the run.

Suggested fix — remove the comment from the block scalar and move it to a YAML key-level comment above claude_args:

Suggested change
# Keep the tool surface narrow: inline comments plus read-only PR inspection.
--max-turns 30

Comment on lines +41 to +48
) || (
github.event_name == 'workflow_call' &&
inputs.pr_number &&
(
!inputs.head_repo_full_name ||
inputs.head_repo_full_name == github.repository
)
)
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 workflow_call path has no draft-PR guard

The pull_request branch of the if condition correctly checks !github.event.pull_request.draft, but the workflow_call branch has no equivalent. A caller that triggers this workflow for a draft PR will not be blocked.

This is likely intentional (delegating the draft check to callers), but it's an implicit contract that is easy to miss and could lead to unnecessary review runs on in-progress work. Consider either documenting this assumption clearly in the workflow comment, or adding an optional is_draft input so callers can pass the flag and this workflow can enforce it uniformly.

uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
with:
fetch-depth: 1
fetch-depth: 0
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 fetch-depth: 0 clones the full repository history

Changing from fetch-depth: 1 to fetch-depth: 0 means a complete, unbounded history clone on every run. For large or long-lived repositories this can add significant time (and egress cost) to each review job without a proportional benefit — Claude typically only needs the diff plus a few commits of context.

fetch-depth: 10 or fetch-depth: 50 would satisfy git log/git blame use-cases in the prompt while keeping checkout fast:

Suggested change
fetch-depth: 0
fetch-depth: 10

Copy link
Copy Markdown

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

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: 8bd03579fd

ℹ️ 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".

--model opus
--allowedTools "Read,Glob,Grep,WebSearch,WebFetch,mcp__github_inline_comment__create_inline_comment,Bash(gh api:*),Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr checks:*),Bash(git log:*),Bash(git blame:*),Bash(git diff:*),"
--max-turns 30
# Keep the tool surface narrow: inline comments plus read-only PR inspection.
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 Remove inline comment from claude_args block

The line starting with # Keep the tool surface narrow... is inside the claude_args payload, not a YAML comment for the workflow file, so it will be passed to anthropics/claude-code-action as part of the CLI arguments. In runs where the action parses claude_args into flags, this can introduce an unexpected token (for example #) and cause argument parsing to fail before any review is posted.

Useful? React with 👍 / 👎.

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.

1 participant