ci: gate Claude review on CI success and cancel on quota limits - #216
Conversation
Run the review only after the CI workflow has gone green for the same commit, and stop a usage-limit hit from red-flagging a healthy PR. Trigger is now `workflow_run` on CI, gated to same-repo pull_request runs that concluded success and were not triggered by a bot. CI also runs on push to main, so the event filter keeps this PR-only. The prompt states that clippy, the test suite and fixture validation already passed, so the reviewer spends its budget on logic, ANTLR parity and coverage instead of re-verifying mechanics. This required moving from claude-code-action to claude-code-base-action. The wrapper derives PR context from the webhook payload and classifies `workflow_run` as an automation event with no PR entity: `track_progress` throws outright, `use_sticky_comment` is gated behind `isPullRequestEvent` and silently no-ops, and no tracking comment is wired up, which removes the `update_claude_comment` tool. Overriding the event shape is not possible either -- GITHUB_EVENT_NAME is a reserved default and the assignment is ignored. The base action has no event-shape validation, so the sticky progress comment is reimplemented with `gh api`: a hidden first-line marker keys lookup, matching the convention the other bots on this repo already use, and the review is delivered by PATCHing that same comment so re-runs update one comment instead of piling up new ones. The marker match requires first-line position and a Bot author. Matching it anywhere would let a human who merely quotes the marker have their comment overwritten. The empty-string default sits after the line index because an empty or null comment body splits to `[]`, making `[0]` null, which makes `startswith` raise and abort the step. On failure the execution log is parsed for the result message's `api_error_status`. A 429 cancels the run rather than failing it: the code under review is fine, and a cancelled run reads as "did not happen" instead of leaving a red check and a failed required-check to re-run. Any other error still fails loudly, including an unparseable log. Fork PRs are skipped. `workflow_run` grants the base repository's secrets, so a fork head would be untrusted code running with those secrets. Gating to same-repo branches means the author already has write access, which is what keeps this to a single ordinary checkout of the PR head. base-action is pinned to a commit rather than a tag: the newest tag predates the claude_args, plugins and plugin_marketplaces inputs, and GitHub ignores unknown inputs silently, so a tag pin would drop the model flags and the review plugin with no error.
📝 WalkthroughWalkthroughThe workflow now runs after successful CI for eligible pull requests, manages progress comments through the GitHub API, executes Claude with expanded tooling, uploads execution logs, and handles rate-limit and other review failures separately. ChangesClaude review workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant ReviewWorkflow
participant GitHubAPI
participant Claude
participant ArtifactStorage
CI->>ReviewWorkflow: successful CI workflow_run
ReviewWorkflow->>GitHubAPI: resolve eligible pull request
ReviewWorkflow->>GitHubAPI: create or reuse progress comment
ReviewWorkflow->>Claude: execute review on checked-out PR head
Claude->>GitHubAPI: patch progress comment with review result
ReviewWorkflow->>ArtifactStorage: upload execution log
ReviewWorkflow->>ReviewWorkflow: classify result and API error status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Source Code Metrics (this PR vs
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f08404f5c4
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/claude-code-review.yml:
- Around line 182-187: Pin the actions/checkout invocation in the “Checkout PR
head” step to an immutable commit SHA instead of the mutable v7 tag, while
preserving the existing ref and fetch-depth settings.
- Around line 220-222: Update the “install cargo binstall” step to fetch
install-from-binstall-release.sh from a specific tagged cargo-binstall release
rather than the mutable main branch, preserving the existing curl security flags
and execution flow.
- Around line 199-206: Pin the upstream ANTLR checkout step using the existing
actions/checkout symbol to an immutable commit SHA instead of the floating
actions/checkout@v7 tag, while preserving the repository, ref, path,
fetch-depth, and credential settings.
- Around line 329-337: Pin the actions/upload-artifact step to a full commit SHA
instead of the mutable v7 tag, while preserving the existing artifact name,
path, retention, and conditional execution settings.
- Around line 230-244: Pin the actions/setup-java step and the
astral-sh/setup-uv step to immutable commit SHAs instead of version tags, while
preserving their existing versions and configuration. Update only the uses
references in these two steps.
- Around line 251-274: Restrict the credential exposed through GH_TOKEN in the
claude-review step to a short-lived or fine-grained token limited to updating
the targeted PR comment, rather than github.token with repository-wide issue,
pull-request, and actions write access. Preserve the existing Claude review
invocation and comment-editing behavior while ensuring the
dangerously-skip-permissions environment cannot use broad repository privileges.
- Around line 27-31: Add a concurrency configuration to the workflow triggered
by workflow_run, grouping runs by the associated pull request and head SHA, with
cancellation enabled for superseded runs. Ensure overlapping claude-review
executions for the same PR revision are cancelled while distinct PRs or commits
remain independent.
- Around line 267-271: Update the model argument in the claude_args
configuration to use the canonical "claude-opus-5" identifier instead of
"claude-opus-5[1m]", while preserving the remaining Claude CLI arguments
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c3f8657a-b579-4b36-ae5e-eed8de6e85ad
📒 Files selected for processing (1)
.github/workflows/claude-code-review.yml
| - name: Checkout PR head | ||
| if: steps.pr.outputs.found == 'true' | ||
| uses: actions/checkout@v7 | ||
| with: | ||
| ref: ${{ github.event.workflow_run.head_sha }} | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Unpinned actions/checkout@v7 (tag, not SHA).
zizmor flags this as unpinned. Given this job runs with elevated permissions (issues: write, pull-requests: write, actions: write) on a workflow_run trigger, and the PR already went to the trouble of SHA-pinning the Claude action for exactly this reason, the same rationale applies here — a tag can be moved to point at different content.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 182-197: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 184-184: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/claude-code-review.yml around lines 182 - 187, Pin the
actions/checkout invocation in the “Checkout PR head” step to an immutable
commit SHA instead of the mutable v7 tag, while preserving the existing ref and
fetch-depth settings.
Source: Linters/SAST tools
| if: steps.pr.outputs.found == 'true' | ||
| uses: actions/checkout@v7 | ||
| with: | ||
| repository: antlr/antlr4 | ||
| ref: "4.13.2" | ||
| path: antlr-upstream | ||
| fetch-depth: 1 | ||
| persist-credentials: false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Unpinned actions/checkout@v7 (tag, not SHA) for the upstream ANTLR checkout.
Same supply-chain concern as the primary checkout step — pinned to a tag (4.13.2) and to actions/checkout@v7, neither of which is a commit SHA.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 200-200: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/claude-code-review.yml around lines 199 - 206, Pin the
upstream ANTLR checkout step using the existing actions/checkout symbol to an
immutable commit SHA instead of the floating actions/checkout@v7 tag, while
preserving the repository, ref, path, fetch-depth, and credential settings.
Source: Linters/SAST tools
| - name: install cargo binstall | ||
| if: steps.pr.outputs.found == 'true' | ||
| run: curl -L --proto '=https' --tlsv1.2 -sSf https://raw.githubusercontent.com/cargo-bins/cargo-binstall/main/install-from-binstall-release.sh | bash |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
cargo-binstall installer is pinned to main, not a release.
curl ... https://raw.githubusercontent.com/cargo-bins/cargo-binstall/main/install-from-binstall-release.sh | bash fetches and executes a script from a mutable branch ref. Anyone who can push to cargo-bins/cargo-binstall's main branch (or compromise it) changes what this workflow executes on every run, with no diff in this repo to review. Pin to a tagged release URL instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/claude-code-review.yml around lines 220 - 222, Update the
“install cargo binstall” step to fetch install-from-binstall-release.sh from a
specific tagged cargo-binstall release rather than the mutable main branch,
preserving the existing curl security flags and execution flow.
| - uses: actions/setup-java@v5 | ||
| if: steps.pr.outputs.found == 'true' | ||
| with: | ||
| distribution: "temurin" | ||
| java-version: "25" | ||
|
|
||
| - name: Install uv with | ||
| if: steps.pr.outputs.found == 'true' | ||
| uses: astral-sh/setup-uv@v9.0.0 | ||
| with: | ||
| python-version: "3.12" | ||
| github-token: ${{ github.token }} | ||
| ignore-nothing-to-cache: "true" | ||
| activate-environment: "true" | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Unpinned actions/setup-java@v5 and astral-sh/setup-uv@v9.0.0 (tags, not SHAs).
Same unpinned-action pattern as the checkout steps above.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 230-230: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 238-238: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/claude-code-review.yml around lines 230 - 244, Pin the
actions/setup-java step and the astral-sh/setup-uv step to immutable commit SHAs
instead of version tags, while preserving their existing versions and
configuration. Update only the uses references in these two steps.
Source: Linters/SAST tools
| - name: Run Claude Code Review | ||
| id: claude-review | ||
| uses: anthropics/claude-code-action@v1 | ||
| if: steps.pr.outputs.found == 'true' | ||
| # Do not fail the job here. A quota/weekly-limit 429 is not a code | ||
| # problem and must not red-flag the PR; the classify step below decides | ||
| # between cancelling (limit) and failing (genuine error). | ||
| continue-on-error: true | ||
| uses: anthropics/claude-code-base-action@e257d767763882223c40b615cbbe0d26ca75a981 | ||
| env: | ||
| CLAUDE_BASH_MAINTAIN_PROJECT_WORKING_DIR: "1" | ||
| # Available to the reviewer's `gh` calls for editing its own comment. | ||
| GH_TOKEN: ${{ github.token }} | ||
| with: | ||
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} | ||
| plugin_marketplaces: "https://github.com/anthropics/claude-code.git" | ||
| # When track_progress is enabled: | ||
| # - Creates a tracking comment with progress checkboxes | ||
| # - Includes all PR context (comments, attachments, images) | ||
| # - Updates progress as the review proceeds | ||
| # - Marks as completed when done | ||
| track_progress: true | ||
| # Deliver the review through a single reused comment instead of a new one | ||
| # each run. | ||
| use_sticky_comment: true | ||
| plugins: "code-review@claude-code-plugins" | ||
| claude_args: | | ||
| --effort max | ||
| --model "claude-opus-5[1m]" | ||
| --no-chrome | ||
| --dangerously-skip-permissions | ||
| plugins: "code-review@claude-code-plugins" | ||
|
|
||
| # The trailing directive fixes a CI-only stall: the review skill fans out to | ||
| # parallel sub-agents, and on its final turn the model would emit "I'll wait | ||
| # for the agents to report…" and end the turn (stop_reason: end_turn) — but in | ||
| # a non-interactive batch there is no async resume, so the consolidated review | ||
| # was never posted even though every sub-agent had already returned. Forbid the | ||
| # yield: sub-agent results are available synchronously in-session. | ||
| prompt: | | ||
| /code-review:code-review ${{ github.repository }}/pull/${{ github.event.pull_request.number }} | ||
| /code-review:code-review ${{ github.repository }}/pull/${{ steps.pr.outputs.number }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Broad GH_TOKEN scope handed to an unrestricted, permission-skipping agent.
GH_TOKEN: ${{ github.token }} is injected into the Claude Bash environment, and claude_args includes --dangerously-skip-permissions with no allowed_tools/disallowed_tools restriction — the agent gets unrestricted Bash plus a token scoped to issues: write, pull-requests: write, and actions: write across the whole repo, not just "edit comment ${{ steps.comment.outputs.id }}".
The file's extensive comments justify the fork/bot trust boundary (pwn-request risk) but don't address a different vector: this token's privileges are available to whatever the agent decides to run while reading PR content (code comments, commit messages, file contents) it doesn't fully control the provenance of. A prompt injected into reviewed content could, in principle, get the agent to use this token for something beyond "PATCH one comment" — e.g., editing other issues/PRs or cancelling unrelated runs. Consider minting a narrower-scoped token (fine-grained PAT or short-lived App installation token limited to the single comment's issue) for the agent's GH_TOKEN, rather than reusing the full job token.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/claude-code-review.yml around lines 251 - 274, Restrict
the credential exposed through GH_TOKEN in the claude-review step to a
short-lived or fine-grained token limited to updating the targeted PR comment,
rather than github.token with repository-wide issue, pull-request, and actions
write access. Preserve the existing Claude review invocation and comment-editing
behavior while ensuring the dangerously-skip-permissions environment cannot use
broad repository privileges.
| claude_args: | | ||
| --effort max | ||
| --model "claude-opus-5[1m]" | ||
| --no-chrome | ||
| --dangerously-skip-permissions |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the Anthropic API or Claude Code CLI reject or normalize a "claude-opus-5[1m]" model string, given Opus 5's 1M context is already the default?
💡 Result:
The Anthropic Claude Code CLI does not reject the model string claude-opus-5[1m], though appending the [1m] suffix to claude-opus-5 is unnecessary because 1M token context is the default for Opus 5 [1][2]. Under normal operation, the Claude Code CLI handles model strings by either using defined aliases (like opus) or by allowing explicit model IDs [3][4]. Historically, the [1m] suffix was used as an internal CLI convention to explicitly toggle the 1M-token context window for models where it was an optional feature [5][6]. However, because Claude Opus 5 supports a 1M-token context window as its standard, native configuration, this suffix is redundant [1][2]. If a user provides the literal string claude-opus-5[1m], the CLI's behavior can depend on its version and whether it is communicating directly with the Anthropic API or through a proxy [5]. In some configurations, the CLI may strip the suffix before sending the request to the API, while in others, it might pass the literal string through [5]. If an invalid or unexpected model ID is passed to the Anthropic API (such as one with an unsupported suffix), the API may return a 404 error, potentially causing the CLI to fall back to a different model or context window [5]. For Claude Opus 5, it is recommended to use the clean model ID claude-opus-5 to ensure compatibility and avoid potential issues with model name resolution [3][4].
Citations:
- 1: https://platform.claude.com/docs/en/about-claude/models/whats-new-opus-5
- 2: https://platform.claude.com/docs/en/build-with-claude/context-windows.md
- 3: https://code.claude.com/docs/en/model-config
- 4: https://code.claude.com/docs/en/model-config.md
- 5: [BUG] CLI 2.1.145 sends literal "claude-opus-4-7[1m]" as model name on session resume, hits 404, silently falls back to 200K anthropics/claude-code#60913
- 6: Add Claude Opus 5 model pingdotgg/t3code#4472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow actionlint availability =="
if command -v actionlint >/dev/null 2>&1; then
actionlint -version 2>&1 || true
else
echo "actionlint not installed"
fi
echo "== target workflow excerpt =="
sed -n '230,290p' .github/workflows/claude-code-review.yml
echo "== repo workflow files =="
git ls-files '.github/workflows/*.yml'
echo "== actionlint on target workflow =="
if command -v actionlint >/dev/null 2>&1; then
actionlint .github/workflows/claude-code-code-review.yml 2>&1 || true
else
echo "skipping actionlint"
fiRepository: ophi-dev/antlr-rust-runtime
Length of output: 4126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== run actionlint on the existing workflow =="
actionlint .github/workflows/claude-code-review.yml
echo "== inspect Claude Code model config docs in repo if present =="
git ls-files | rg -i 'claude|model|context|model-config|anthropic' || trueRepository: ophi-dev/antlr-rust-runtime
Length of output: 6660
Use the canonical claude-opus-5 model ID.
claude-opus-5[1m] is redundant for Opus 5 because 1M context is already default; use --model "claude-opus-5" to avoid version-dependent model string stripping or API resolution surprises.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/claude-code-review.yml around lines 267 - 271, Update the
model argument in the claude_args configuration to use the canonical
"claude-opus-5" identifier instead of "claude-opus-5[1m]", while preserving the
remaining Claude CLI arguments unchanged.
| - name: Upload Claude execution log | ||
| if: always() | ||
| if: always() && steps.pr.outputs.found == 'true' | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: claude-review-log-${{ github.event.pull_request.number }}-${{ github.run_attempt }} | ||
| name: claude-review-log-${{ steps.pr.outputs.number }}-${{ github.run_attempt }} | ||
| path: ${{ runner.temp }}/claude-execution-output.json | ||
| if-no-files-found: warn | ||
| retention-days: 14 | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Unpinned actions/upload-artifact@v7 (tag, not SHA).
Same unpinned-action pattern flagged above.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 331-331: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/claude-code-review.yml around lines 329 - 337, Pin the
actions/upload-artifact step to a full commit SHA instead of the mutable v7 tag,
while preserving the existing artifact name, path, retention, and conditional
execution settings.
Source: Linters/SAST tools
"Create a PR" here carries repo-specific conventions that were tribal knowledge: branch from origin/main, use gh with --body-file to avoid a shell-escaping fight, never open a draft (the reviewers only engage on ready PRs), and keep the description current as scope moves. Merges are squash-only with squash_merge_commit_message=COMMIT_MESSAGES, so the commit message -- not the PR description -- is what lands in permanent history. Verified against #215's squashed commit. Document all three AI reviewers and how they differ, because "one of them is happy" is not the finish line: CodeRabbit's out-of-diff comments are the ones worth the most and the easiest to skip; Codex is per-push and signals via eyes/+1 reactions, so re-triggering while eyes are present just wastes a pass; and Claude Code Review posts no status at all, so a green workflow is not approval -- the verdict is in the comment text. State the exit condition explicitly: loop until Codex reacts +1 and Claude Code Review reports no blockers.
Addresses four findings from the Codex review of #216. Restore PR-scoped concurrency, which this branch had dropped when it replaced the pull_request trigger. Without it two overlapping successful CI completions for the same PR both resolve the same PR, reuse the same sticky comment, and PATCH it -- so the run that finishes last wins, which is not necessarily the newest commit. Keyed on head_branch rather than PR number because workflow_run carries no PR object and the group has to resolve before any step runs; one branch maps to at most one open PR here. Skip runs whose SHA is no longer the PR head. workflow_run completions can arrive out of commit order, and the payload supplies a PR number with no freshness guarantee, so an older green run could review an obsolete commit and overwrite a newer verdict. Concurrency does not cover this on its own because the two runs need not overlap. Drop the `|| true` on the fallback PR lookup. The endpoint already exits 0 with an empty array when no PR matches (verified), so a nonzero exit means a real failure -- auth, rate limit, transient 5xx. Swallowing it reported found=false and let the job go green as though the PR did not exist. Add an always() finalizer so the comment never stays stuck on "Claude Code is reviewing...". An auth error, API 5xx, malformed result, crashed runner or cancelled run all previously left the placeholder in place, which misrepresents a finished run. Detection uses a second sentinel marker carried only by the placeholder rather than matching its prose; the sentinel sits on line 2 so the first-line sticky lookup is unaffected. The step is continue-on-error because a cosmetic comment update must not fail an otherwise good job.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7299dad97e
ℹ️ 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".
| current_head="$(gh api "repos/$GH_REPO/pulls/$number" --jq '.head.sha')" | ||
| if [[ "$current_head" != "$HEAD_SHA" ]]; then |
There was a problem hiding this comment.
Fresh evidence beyond the earlier stale-SHA finding is the remaining time-of-check/time-of-use window: this lookup runs before dependency setup and the 10–20-minute review, so a push can move the PR from A to B after the check while Claude still publishes A's verdict. Concurrency cannot cancel A until B's successful CI completion starts another review; if B's CI fails or is cancelled, that never happens and the sticky comment permanently presents A as the latest review of an unreviewed B. Revalidate the head immediately before or after publication and replace/discard the verdict when it no longer matches.
AGENTS.md reference: AGENTS.md:L254-L256
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — this is a genuinely different window from the earlier finding, and your reasoning about why concurrency does not close it is the key part: cancellation only happens once the newer commit's successful CI starts another review, so a failed or cancelled CI on B leaves A's verdict standing as the latest word on an unreviewed B.
Fixed in df3c37a with a publish-time re-check (Discard the verdict if the PR head moved during review), which runs after the review and before the quota classifier:
current_head="$(gh api "repos/$GH_REPO/pulls/$PR_NUMBER" --jq ".head.sha")"
if [[ "$current_head" == "$REVIEWED_SHA" ]]; then ... verdict stands ... fi
# otherwise: replace the comment with a "superseded" pointer
I chose to retract rather than keep the text: findings against the old tree can be actively misleading on the new one, so the comment points at the job run and the full review stays in the uploaded execution log. It keeps STICKY_MARKER on line 1 and omits the in-progress sentinel, so the always() finalizer leaves it alone and the next run still reuses the same comment.
The window is now bounded by one API call rather than 10–20 minutes. It is not zero — a push landing between the re-check and the PATCH would still slip through — but closing that entirely needs optimistic concurrency the Issues API does not offer.
| if: >- | ||
| github.event.workflow_run.event == 'pull_request' && | ||
| github.event.workflow_run.conclusion == 'success' && | ||
| github.event.workflow_run.head_repository.full_name == github.repository && | ||
| github.event.workflow_run.actor.type != 'Bot' |
There was a problem hiding this comment.
When a PR is initially opened as a draft, the bare pull_request: trigger in .github/workflows/ci.yml still runs CI for the opened activity, but this downstream gate never checks the PR's draft flag, so the raw base action spends a full review and quota on work in progress. This also defeats the previous explicit ready_for_review flow and the documented ready-only reviewer behavior; resolve the PR state and defer the review until it is ready.
AGENTS.md reference: AGENTS.md:L222-L223
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, fixed in df3c37a. I verified the premise: ci.yml uses a bare pull_request: trigger, which does include drafts, and the draft check is now part of the same lookup described in the reply above.
You also identified the cause precisely — the old pull_request trigger got this for free from types: [opened, synchronize, ready_for_review, reopened], and workflow_run has no equivalent filter, so the behaviour had to be re-established explicitly. That is a regression this branch introduced, not a pre-existing gap.
One behavioural note for the record: marking a draft ready does not itself trigger CI, so the review arrives on the next push rather than at the moment of readiness. That is a deliberate consequence of gating on CI completion, and it matches the intent of only reviewing commits CI has already vetted.
| claude_args: | | ||
| --effort max | ||
| --model "claude-opus-5[1m]" | ||
| --no-chrome | ||
| --dangerously-skip-permissions |
There was a problem hiding this comment.
For a same-repository PR authored by a collaborator who has branch write access but not Actions-secret access, this runs Claude with permission checks disabled after checking out the collaborator-controlled tree and explicitly asks it to obey the PR's CLAUDE.md. A malicious project instruction can therefore make the Bash-capable reviewer execute commands while GH_TOKEN has actions: write and CLAUDE_CODE_OAUTH_TOKEN is available to the action, leaking the shared OAuth credential or abusing the privileged workflow token. Treat same-repo PR contents as untrusted too: run analysis without write/OAuth credentials and perform the final comment update in a separate trusted step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one, but the mechanism you describe is accurate and worth recording rather than hand-waving.
Verified parts: claude-code-base-action passes claude_code_oauth_token through as CLAUDE_CODE_OAUTH_TOKEN in the step env, it has no subprocess env scrubbing (that lives only in the wrapper action, and only activates when allowed_non_write_users is set), and with --dangerously-skip-permissions a Bash tool call can read both it and GH_TOKEN. So if a hostile CLAUDE.md reached this reviewer, the exposure would be as you describe.
Where it does not apply is the threat model. The finding is scoped to "a collaborator who has branch write access but not Actions-secret access" — in this repo that person does not exist:
$ gh api repos/ophi-dev/antlr-rust-runtime/collaborators
tinovyatkin push=true admin=true
One collaborator, with admin. Anyone who can push a branch here can already read the secrets, edit the workflows, and open a terminal on a runner directly. Isolating the reviewer would not remove a capability that person lacks — it would only add moving parts (a credential-free analysis pass plus a separate trusted publish step) to guard against someone who cannot exist without an org-membership change. Forks, which are the case where an untrusted author genuinely can propose code, are already excluded at the job gate for exactly this reason.
This is a deliberate trade rather than an oversight: the split-privilege design was in an earlier revision of this PR and was removed as unnecessary complexity once forks were excluded. Worth revisiting if the repo ever gains write-only collaborators or enables allowed_bots — at that point the premise becomes true and the isolation earns its cost. Recording it here so the reasoning is discoverable if that day comes.
| run: | | ||
| set -euo pipefail | ||
|
|
||
| number="$(jq -r 'if type == "array" and length > 0 then .[0].number else empty end' <<<"$PAYLOAD_PR")" |
There was a problem hiding this comment.
When the PR is closed or merged while CI is running but remains associated with the completed workflow run, pull_requests still supplies its number and this fast path bypasses the fallback's state == "open" filter. The later pull request lookup extracts only .head.sha, which remains equal after closing, so the job performs an expensive review and posts its sticky verdict on an already-ended PR despite the explicit intent to treat closure as “nothing to review.” Fetch and validate the current PR state before setting found=true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in df3c37a. You are right that the fast path bypassed the filter: pull_requests[] supplies only a number, and I verified head.sha is unchanged by closing — #215 is state: closed, merged: true with its original head SHA — so the SHA check alone would have let a merged PR through.
The lookup that already ran for head.sha now returns all three fields in one call, checked open -> draft -> SHA:
read -r current_head current_state current_draft < <(
gh api "repos/$GH_REPO/pulls/$number" --jq "\"\(.head.sha) \(.state) \(.draft)\""
)
No extra API cost, and it makes the "closure means nothing to review" intent actually hold on both paths rather than only the fallback.
Addresses the second Codex round on #216. Fold state and draft checks into the PR lookup that already ran for head.sha, so one API call answers three questions. The payload's pull_requests[] fast path carries only a number and skips the fallback's state filter, and head.sha is unchanged by closing or merging -- so checking the SHA alone would review a PR that ended while CI ran. Drafts need checking here too: CI triggers on a bare `pull_request:`, which includes them, and the old trigger's `types: [..., ready_for_review]` filter has no workflow_run equivalent, so a work-in-progress PR would burn a full review and quota. Re-check the head after the review and retract the verdict when it moved. The resolve-time check leaves a time-of-check/time-of-use window across dependency setup and a 10-20 minute review. Concurrency does not close it: it can only cancel this run once the newer commit's *successful* CI starts another review, so if that CI fails or is cancelled the stale verdict stands as the latest word on an unreviewed commit. The retraction points at the job run rather than showing findings that may no longer apply; the full text stays in the uploaded execution log. It keeps the sticky marker on line 1 and drops the in-progress sentinel, so the finalizer leaves it alone and the next run still reuses the comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df3c37a80d
ℹ️ 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".
| if: >- | ||
| steps.pr.outputs.found == 'true' && | ||
| steps.claude-review.outputs.conclusion == 'success' | ||
| continue-on-error: true |
There was a problem hiding this comment.
Fresh evidence beyond the earlier stale-SHA finding is that the new safeguard is explicitly continue-on-error: if the PR moves during review and either the final gh api head lookup or comment PATCH fails transiently, the job continues successfully while the old commit's verdict remains in the sticky comment. The finalizer will not correct it because Claude's verdict has already removed the progress sentinel, so the comment can still masquerade as the latest review; let this safety step fail or otherwise replace the verdict reliably before allowing success.
AGENTS.md reference: AGENTS.md:L254-L256
Useful? React with 👍 / 👎.
| github.event.workflow_run.event == 'pull_request' && | ||
| github.event.workflow_run.conclusion == 'success' && | ||
| github.event.workflow_run.head_repository.full_name == github.repository && | ||
| github.event.workflow_run.actor.type != 'Bot' |
There was a problem hiding this comment.
workflow_run.actor identifies who initiated the CI run, not necessarily who authored the pull request, so this gate changes the previous author-based behavior: automation pushing a commit to a human-authored PR suppresses its Claude review, while a maintainer pushing to a bot-authored branch can trigger the review and consume quota. Fetch .user.type with the PR metadata in the resolver and gate on that value so eligible human PRs are not silently missed.
AGENTS.md reference: AGENTS.md:L234-L237
Useful? React with 👍 / 👎.
Switching the review workflow to `claude-code-base-action` (#216) was necessary for `workflow_run` — the wrapper rejects it as an automation event with no PR entity — but it silently cost the review its GitHub identity. Comments now come from `github-actions[bot]` instead of `claude[bot]`. The identity was never an input we could set. `claude-code-action` trades the job's OIDC token for a Claude GitHub App installation token in `src/github/token.ts`, before it runs anything; the base action has no such code, so nothing in this workflow could re-enable it. Confirmed by comparing comment authors: PR #192 has a `claude[bot]` comment, the #216-era runs do not. Re-implement the exchange as a step, mirroring the wrapper: mint an OIDC token with audience `claude-code-github-action` (Anthropic validates the `aud` claim, so the default audience is rejected), POST it to `api.anthropic.com/api/github/github-app-token-exchange`, and use the returned installation token for every comment write. Revoke it in a final step rather than letting it idle out its remaining ~40 minutes. Identity is kept strictly cosmetic. `continue-on-error` plus a `|| github.token` fallback on each consumer means an uninstalled app, a changed endpoint, or a network blip downgrades authorship instead of losing a review. `gh run cancel` on the quota path keeps using the workflow token: the app token's grants are contents/pull-requests/issues, with no `actions: write`. Three details found by testing the step against a local stub rather than by reading the upstream source: * `ACTIONS_ID_TOKEN_REQUEST_{URL,TOKEN}` must be read as shell variables. The `env` expression context only exposes workflow/job/step-declared variables, so mapping these runner-injected ones in yields empty strings and the exchange is never attempted — it would have failed open, always, and looked like the app was uninstalled. * `-w '%{http_code}'` is unusable as a success signal here: on a connection-teardown retry curl emits the template once per attempt, so the capture reads "000200" and matches no status test. A recovered transient fault would have cost the identity. Judge by curl's exit code with `--fail-with-body`, which still saves the error JSON. * Bodies go to a file, never piped to jq. Under `-sS --retry` curl writes "Transient problem … will retry" to stderr, which a `2>&1` capture splices into the JSON. Verified with a stub covering the happy path through a forced retry, a permanent 401, an unreachable endpoint, and a missing OIDC context: all four fall back cleanly and exit 0. actionlint clean. Note this cannot take effect until it lands on the default branch — the exchange refuses workflows absent from it, and `workflow_run` reads the default-branch copy regardless.
1 participant
Summary
Two robustness changes to the Claude Code review workflow:
Run only after CI passes. Trigger is now
workflow_runon the CI workflow, gated to same-repopull_requestruns that concludedsuccess. CI also runs on push to main, so the event filter keeps the review PR-only. The prompt now states that clippy, the test suite and fixture validation already passed for that exact commit, so the reviewer spends its budget on logic, ANTLR parity, panic paths and coverage instead of re-verifying mechanics.Graceful quota handling. On failure the execution log is parsed for the result message's
api_error_status. A429cancels the run instead of failing it — the code under review is fine, and a cancelled run reads as "did not happen" rather than leaving a red ✗ and a failed required-check to re-run. Any other error still fails loudly.The wrapper action can't do this, so this moves to
claude-code-base-actionclaude-code-actionderives PR context from the webhook payload and classifiesworkflow_runas an automation event with no PR entity:track_progressthrows —validateTrackProgressEventrejects any event outsidepull_request/issues/comment events.use_sticky_commentis gated behindisPullRequestEventand silently no-ops.claudeCommentId: undefined), which removes theupdate_claude_commenttool.Forcing the event shape isn't an option either:
GITHUB_EVENT_NAMEis a reserved default, and per GitHub's docs "if you attempt to override the value of one of these default variables, the assignment is ignored."claude-code-base-actionhas no event-shape validation — it installs Claude Code, runs a prompt, and reports aconclusionoutput — so the sticky/progress comment is reimplemented here in ~20 lines ofgh api. It posts a progress comment up front, keyed by a hidden first-line marker, and the review is delivered by PATCHing that same comment, so re-runs update one comment instead of piling up new ones. This matches the convention the other bots on this repo already use (<!-- mehen-metrics -->,<!-- cpd-report -->).Gating on base-action's real
conclusionoutput also makes the quota check more reliable than the wrapper's throw-based failure.Fork PRs are skipped
workflow_rungrants the base repository's secrets, so a fork head would be untrusted code running with those secrets. Gating to same-repo branches means the PR author already has write access — which is what keeps this to a single ordinary checkout of the PR head, with no split-checkout dance and no confusing "the workspace is the base branch" instructions in the prompt. An external contribution gets no automated review (the job reportsskipped, neutral) rather than a subtly weakened one. Zero of the last 100 PRs came from a fork.Testing
actionlintclean (it shellchecksrun:blocks too). The two script-heavy steps were extracted from the workflow and executed against synthetic payloads:Quota classification — 5/5
api_error_status: 429(the reported payload)api_error_status: 500::error::Verified via a logging stub that the 429 path really does both PATCH the comment and call
gh run cancel. Confirmed the log is a JSON array of SDK messages, so the verdict is selected with.[] | select(.type=="result")— readingapi_error_statusfrom the root would always be null.Sticky comment lookup — 9/9, including two bugs this caught:
split("\n")[0]on an empty or null body returnsnull, andstartswiththen raisesrequires string inputs, whichset -eturns into an aborted step. GitHub does returnbody: nullfor some comments. The default has to sit after the line index.Remaining cases: empty list, no marker, one match, duplicates (picks newest), quoted-by-human, human-impersonating, null body, empty body, other bots' markers.
Gate logic — 7/7: same-repo/green/human runs; fork, push-to-main, CI-failed, CI-cancelled, bot-authored PR, and fork-from-bot all skip.
Notes for the reviewer
actions: writeis load-bearing. This repo's default workflow token is read-only (default_workflow_permissions: "read"), sogh run cancelwould 403 without it.v0.0.63) predates theclaude_args,pluginsandplugin_marketplacesinputs, and GitHub ignores unknown inputs silently — a tag pin would have dropped the model flags and the review plugin with no error. Pinned to the currentmainsync (e257d76).workflow_runreads the workflow file from the default branch, so this change cannot exercise itself on this PR. It takes effect only after merge; worth watching the first PR that lands afterward.restoreConfigFromBase, which only matters for untrusted heads.Summary by CodeRabbit
Also: documented the PR workflow (
CLAUDE.md/AGENTS.md)A second commit records conventions that were tribal knowledge, since this PR is
itself the first thing the new review pipeline will meet:
origin/main, useghwith--body-file(inlining a body is a shell-escaping fight), never open a draftbecause the AI reviewers only engage on ready PRs, and keep the description
current as scope moves.
PR description —
squash_merge_commit_messageisCOMMIT_MESSAGES. Verifiedagainst fix(frontend): accept a UTF-8 BOM in grammar sources #215's squashed commit body.
not the finish line: CodeRabbit's out-of-diff comments are the highest-value
and easiest to skip; Codex is per-push and signals via 👀/+1, so re-triggering
while 👀 is present just wastes a pass; Claude Code Review posts no status, so a
green workflow is not approval — the verdict is in the comment text.
Code Review reports no blockers.
Both files carry the section byte-identically (verified by diff); AGENTS.md now
also back-references CLAUDE.md, so the sync note points both directions.
Codex review round 1 — all four findings fixed (
7299dad)P1 · stale CI runs.
workflow_runcompletions can arrive out of commit order, and the payload supplies a PR number with no freshness guarantee, so an older green run could review an obsolete commit and overwrite a newer verdict. The resolve step now compares the PR's currenthead.shaagainst the run's SHA and exits before posting the progress comment. Genuinely distinct from the concurrency fix — out-of-order completions need not overlap.P1 · PR-scoped concurrency. This was a regression I introduced, not an omission:
origin/mainhadconcurrency: code-review-${{ github.event.pull_request.number }}withcancel-in-progress: true, and the trigger rewrite dropped it. Restored, keyed onhead_branch—workflow_runhas no PR object and the group must resolve before any step runs, so it cannot use the number the first step looks up. One branch maps to at most one open PR here.P2 ·
|| trueswallowed API failures. Verified the premise before changing it: the endpoint exits0with an empty array for "no open PR" and1for a real error, so the guard was destroying a usable signal and letting a transient auth/rate-limit failure go green as if no PR existed. Removed; real failures now abort, genuine no-match still reaches the cleanfound=falsepath.P2 · stranded progress comment. Added the suggested
always()finalizer, which beats patching each exit path since the list is longer than the non-429 branch alone (auth error, API 5xx, malformed result, crashed runner, cancelled run). To avoid clobbering a verdict already written — and to avoid brittle prose matching — the placeholder carries a second sentinel marker that both the review and the quota rewrite drop; the finalizer acts only while it is present. Sentinel is on line 2, so the first-line sticky lookup is unaffected (verified).continue-on-error, since a cosmetic comment update must not fail a good job.Codex review round 2 — three fixed, one declined (
df3c37a)P1 · TOCTOU on the head SHA. The resolve-time check left a window across dependency setup and a 10–20 minute review. Codex's key insight: concurrency cannot close it, because cancellation only happens once the newer commit's CI succeeds — so a failed or cancelled CI on B leaves A's verdict standing as the latest word on an unreviewed B. Added a publish-time re-check that retracts the verdict when the head moved, pointing at the job run rather than showing findings that may no longer apply (full text stays in the execution log artifact).
P2 · payload fast path skipped the state filter.
pull_requests[]supplies only a number, andhead.shais unchanged by closing or merging — verified on #215 (state: closed, merged: true, original head SHA) — so the SHA check alone would have reviewed an already-merged PR.P2 · draft PRs.
ci.ymluses a barepull_request:trigger, which includes drafts. Another regression from the trigger swap: the oldtypes: [..., ready_for_review]filter has noworkflow_runequivalent. Both checks now come from the single PR lookup that already ran forhead.sha, gated open → draft → SHA, at no extra API cost.P1 · isolate PR-controlled instructions from privileged Claude — declined. The mechanism is accurate and I verified it: base-action passes the OAuth token through in the step env, has no subprocess scrubbing (that is wrapper-only, and only with
allowed_non_write_users), and--dangerously-skip-permissionslets a Bash call read it. But the finding is scoped to "a collaborator with branch write access but not Actions-secret access," and this repo has exactly one collaborator, with admin — that person can already read secrets and edit workflows directly, so isolation would add moving parts without removing a capability. Forks, the case where an untrusted author genuinely can propose code, are already excluded at the job gate. Full reasoning in the review thread; worth revisiting if the repo ever gains write-only collaborators or enablesallowed_bots.