Repository navigation
chore: add the Claude review workflow - #139
Conversation
- Reviews every pull request against the conventions of the repository - Guards the job to the branches of this repository, as a fork receives no secrets and its code may not run on the self-hosted runners
|
Caution Review failedThe pull request is closed.
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. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @joamag's task in 4m 59s —— View job Review of PR #139: the Claude review workflowI found four problems in High: security
Medium: bug
Low
Nit
Nothing else to report:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| github.event.pull_request.draft == false && | ||
| github.event.pull_request.user.type != 'Bot' && | ||
| github.event.pull_request.head.repo.full_name == github.repository | ||
| runs-on: [self-hosted, ubuntu, latest] |
There was a problem hiding this comment.
Security, high confidence. The head.repo.full_name guard doesn't keep fork code off these runners, so the PR description is wrong to say it does. For pull_request events, GitHub takes the workflow files from the PR's merge commit (refs/pull/N/merge), not from master. A fork can therefore delete the guard from this file, or add its own on: pull_request workflow with runs-on: [self-hosted, ubuntu, latest] and a run: step, and that will run on the runner.
- This job is running on
github-runner-resolute-11right now, so the runner group already accepts this public repository. - The runner is persistent (
/_work/github-runner-resolute-11/..., which is why the cleanup step exists). A fork job can leave something behind for a later job that holdsCLAUDE_CODE_OAUTH_TOKEN, or holds the secrets of another repository on the same runner group. - What actually decides is the setting for approving fork pull request workflows (Settings → Actions → General).
The guard is still worth keeping because it avoids the red cross. To protect the runners, though, run on a GitHub-hosted runner like main.yml and deploy.yml do. Those minutes are free for public repositories, and this job needs nothing from the machine:
| runs-on: [self-hosted, ubuntu, latest] | |
| runs-on: ubuntu-latest |
Then remove the Cleanup the hosted runner step (lines 34-35). If you keep the self-hosted runner instead, set fork approval to Require approval for all external contributors.
| types: | ||
| - opened | ||
| - synchronize | ||
| - ready_for_review | ||
| - reopened |
There was a problem hiding this comment.
Bug, high confidence (this run shows it). Nothing stops a review when the PR closes. When the action finds the PR already merged or closed, it checks out the default branch instead (claude-code-action src/github/operations/branch.ts:173: PR is MERGED, creating new branch from source..., then git checkout master and checkout -b claude/pr-N-…).
That's what happened to this run:
- It was triggered by
ready_for_review. - The PR merged at 11:22:12, before the job reached its setup.
- The reviewer started on
claude/pr-139-20261106-1125, withgit diff origin/master...HEADempty.
A less careful run would spend up to 30 minutes of opus/max reviewing master, and might report "no findings" for a change it never saw.
The fix is to listen for closed. The closing run joins the review-<number> group, and cancel-in-progress cancels the queued or running review:
| types: | |
| - opened | |
| - synchronize | |
| - ready_for_review | |
| - reopened | |
| types: | |
| - opened | |
| - synchronize | |
| - ready_for_review | |
| - reopened | |
| - closed |
Then skip the job for that event by adding github.event.action != 'closed' && as the first line of the if (line 23).
| - name: Checkout code | ||
| uses: actions/checkout@v7 | ||
| with: | ||
| fetch-depth: 1 |
There was a problem hiding this comment.
Review quality, fairly sure. After this depth-1 checkout, the action fetches the PR head at depth max(commits, 20) (branch.ts:189-241) and the base branch at depth 1 (restore-config.ts:310-318). Once master has moved past the point where the PR branched, origin/master is a shallow tip with no parents. Two things then break:
git diff origin/master...HEAD, the command the action's prompt tells the reviewer to run, fails withno merge base.git log origin/master..HEADlists up to 20 commits, including ones already merged.
The action's own fetch-depth.ts:7-17 describes this, and it leaves the history whole when the checkout isn't shallow. The reviewer can fall back to gh pr diff, but it can also review already-merged commits as if they were part of the PR. Netius has a small history, so a full fetch is cheap:
| fetch-depth: 1 | |
| fetch-depth: 0 |
| claude_args: | | ||
| --model ${{ env.CLAUDE_MODEL }} | ||
| --effort ${{ env.CLAUDE_EFFORT }} | ||
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*)" |
There was a problem hiding this comment.
Low, medium confidence. With track_progress: true the action runs in tag mode, where the summary goes into the tracking comment, and that comment is what use_sticky_comment reuses. Bash(gh pr comment:*) is left over from the omni setup and lets the model post a separate top-level comment:
GH_TOKENis exported to the model (run.ts:191), so the command works.- Lines 102-103 ("use a top-level comment for the summary") invite the model to use it.
A run that does this stacks a new comment on every push, which defeats the sticky summary the PR describes. Tag mode tells the model to update its own comment, so this may rarely happen, but nothing here needs gh pr comment:
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*)" | |
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*)" |
Also change line 103 to say "put the summary in the tracking comment".
Closes #138.
Ports the review workflow from
omni, adapted to what this repository is.What it does
On a pull request being opened, updated, reopened or marked ready, the branch is checked out and handed to Claude together with the conventions of the repository, which answers with the findings as inline comments and a summary at the top. The comment is sticky, so a new push replaces the previous summary instead of stacking another one.
The model and the effort come from the
CLAUDE_MODELandCLAUDE_EFFORTrepository variables, falling back toopusandmax, so they can be changed without touching the file. Runs are serialised per pull request and the one in flight is cancelled when a new push arrives.What was adapted, and why
The fork guard. This repository is public, unlike
omni. A pull request opened from a fork receives no secrets, so the job could never authenticate and would put a red cross on every external contribution. It is therefore guarded withgithub.event.pull_request.head.repo.full_name == github.repository, and a fork pull request shows the job as skipped instead. The guard carries a second benefit on self-hosted runners: the code of a fork never runs on them.The findings. The
omniprompt looks for the things that go wrong in an ERP, documents and stock left inconsistent, the rounding of money, the templates that exist in two locales, the migrations. None of that applies here, so the prompt asks about the things that go wrong in a network library instead: connections and sockets left open, blocking calls reached from the event loop, buffers and parsers facing a partial or malformed message, the limits that guard them against a hostile peer, the paths a file service may serve.The public interface. A section of its own, because netius is published to PyPI and its callers live outside this repository. A signature that gains an argument before the existing ones, a reordered parameter or a changed default breaks them silently. This is not a hypothetical: #137 inserted an argument in the middle of
build_dataandbuild_iframe, and a positional call that worked in 1.65.0 started raisingAttributeError. The agents are extended from the outside too, so the protected methods are named as part of that surface.The conventions. Pointed at what
AGENTS.mdanddoc/typing.mdactually require here: the Python 2.7 to 3.14 range, the annotations living in the.pyistub beside the module rather than in the.py, the stub mirroring the module in source order with defaults written as..., the CRLF line endings, the module docstring opening with the dotted name,not item in list,item == None, and the version ofsetup.pyleft alone.The mention workflow that
omnigained in the same commit is deliberately not ported, sinceomniremoved it again in hivesolutions/omni#203.Before it can work
CLAUDE_CODE_OAUTH_TOKENis not among the repository secrets ofomni, so it is an organisation secret. If its visibility is set to private repositories, this public repository will not see it and the step fails to authenticate. Worth confirming before merging; I could not check it myself, as listing organisation secrets needs an owner.Verification
The file parses as YAML, with the triggers, the runner, the permissions, the three steps and the guard all reading as intended, and it was written with the CRLF line endings the other workflows in this repository use.