Skip to content

Improve Claude review workflow - #7

Merged
daviddiviny merged 3 commits into
mainfrom
codex/improve-claude-review-ci
Apr 7, 2026
Merged

daviddiviny merged 3 commits into
mainfrom
codex/improve-claude-review-ci

Conversation

@daviddiviny

Copy link
Copy Markdown
Owner

Summary

  • modernize the Claude PR review workflow to use the current anthropics/claude-code-action@v1 review pattern
  • skip draft and docs-only pull requests, cancel stale in-progress review runs, and add a timeout
  • pin the review model to claude-opus-4-6 and give Claude tighter repo-specific review instructions

Testing

  • ruby -e 'require "yaml"; YAML.load_file(".github/workflows/claude-code-review.yml"); puts "YAML OK"'
  • actionlint .github/workflows/claude-code-review.yml
  • pnpm lint
  • pnpm typecheck
  • pnpm test

Risks

  • the workflow still depends on the existing CLAUDE_CODE_OAUTH_TOKEN secret being configured in GitHub
  • docs-only pull requests will no longer trigger automatic Claude review, which is intentional to reduce noise

@daviddiviny

Copy link
Copy Markdown
Owner Author

This updates the automatic Claude review workflow so it uses the current action pattern, runs on Opus 4.6, skips draft and docs-only pull requests, cancels stale runs, and avoids self-failing when the PR edits the workflow itself.

Local checks run:

  • YAML parse
  • actionlint
  • pnpm lint
  • pnpm typecheck
  • pnpm test

@claude is your feedback properly addressed?

@claude

claude Bot commented Apr 7, 2026 •

Copy link
Copy Markdown

Claude finished @daviddiviny's task in 1m 14s —— View job


Claude's Review

  • Read the changed workflow file
  • Compare against origin/main
  • Assess if previous feedback was addressed
  • Post review findings

The PR is well-structured and addresses the key goals (modern action pattern, draft/docs-only skip, stale-run cancellation, timeout, self-review guard). Here's my assessment:

What looks good

  • Concurrency + cancellation — group: claude-code-review-${{ github.event.pull_request.number }} with cancel-in-progress: true correctly cancels stale runs per PR.
  • Draft skip — if: ${{ !github.event.pull_request.draft }} is the right place to put this (job-level, not step-level).
  • Timeout — timeout-minutes: 20 is a sensible ceiling for an LLM review job.
  • Self-review guard — The github-script step correctly detects when the workflow file itself changed and short-circuits cleanly. The gating with if: steps.workflow-changes.outputs.self_changed != 'true' on the review step means the job still exits green rather than failing.
  • Permission tightening — Dropping issues: read and upgrading pull-requests from read → write is correct; the action needs write to post inline comments.
  • Prompt quality — Repo-specific instructions referencing AGENTS.md and clear signal/noise guidance are a meaningful improvement over the old plugin pattern.

Issues to address

1. actions/checkout@v6 may not exist — As of the latest known releases, actions/checkout is at v4. If v6 hasn't shipped, every run of this workflow will fail with a resolution error. Verify the tag exists before merging, or pin to actions/checkout@v4 (the current stable).

Fix this →

2. --allowedTools prevents Claude from reading source files — The current list only allows:

mcp__github_inline_comment__create_inline_comment
Bash(gh pr comment:*)
Bash(gh pr diff:*)
Bash(gh pr view:*)

Claude can see the diff via gh pr diff, but can't Read, Grep, or Glob any files. For correctness checks (e.g. tracing a function call, verifying a type signature), this is a meaningful handicap. Consider adding Read,Glob,Grep or at minimum Bash(cat:*) to the allowed list so Claude can inspect the surrounding code when the diff alone isn't enough.

Fix this →

3. paths-ignore applies to the trigger, not to the job body — This is working as intended and the PR description correctly describes it as "docs-only PRs". Just worth documenting: a PR that touches both .md files and source files will still trigger a review. No code change needed, but a comment in the YAML would clarify intent for future maintainers.

Minor nit

The run step under "Skip self-review for workflow updates" (echo "Skipping...") will be skipped in the common case (non-self-modifying PRs) and has no effect on workflow outcome — it's just a log line. That's fine, but consider naming it clearly: name: Inform skipped review or similar so the Actions UI is readable at a glance.


Summary: The two substantive items are the checkout@v6 version (likely a breakage) and the restricted --allowedTools (a quality trade-off). Everything else is solid. The self-review guard and the new prompt are the strongest improvements in this PR.

@daviddiviny

Copy link
Copy Markdown
Owner Author

I addressed the actionable follow-up:

  • widened the review tool access so Claude can read nearby files during review, not just the diff
  • added a short note clarifying that the docs ignore only skips docs-only pull requests
  • renamed the skip step so the Actions view reads more clearly

I did not change actions/checkout@v6, because the workflow has now completed successfully with that version on this PR.

Checks rerun after the update:

  • Claude Code Review workflow passed
  • YAML parse
  • actionlint
  • pnpm lint
  • pnpm typecheck
  • pnpm test

@claude is your feedback properly addressed?

@claude

claude Bot commented Apr 7, 2026 •

Copy link
Copy Markdown

Claude finished @daviddiviny's task in 34s —— View job


  • Read the current workflow file
  • Verify each addressed item
  • Post review findings

Yes, all actionable feedback has been properly addressed. Here's the item-by-item rundown:

1. --allowedTools widened ✅
Line 89 now includes Read,Glob,Grep alongside the original gh pr commands:

--allowedTools "Read,Glob,Grep,mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*)"

Claude can now read source files during review, not just the diff.

2. paths-ignore clarification ✅
Line 10 now has:

# Skip docs-only pull requests. PRs that also change code still run the review.

That's exactly the intent documented inline.

3. Skip step renamed ✅
The step is now Log skipped self-review (line 52) — clear and readable in the Actions UI.

4. actions/checkout@v6 — accepted on empirical evidence
My prior concern was that v6 might not exist. You've confirmed the workflow ran successfully on this branch, which means the tag resolves at runtime. I'll take that as sufficient evidence it shipped between my knowledge cutoff and now. No change needed.

This PR is good to merge.


@daviddiviny
daviddiviny merged commit d38b746 into main Apr 7, 2026
1 check passed
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