ci(review): gate the claude pairing and share the branch probe - #7076
Conversation
The claude checkout is executed, not merely read: workspace.sh runs from it, the .claude/ it syncs supplies hooks and settings that run outside the review's tool allowlist, and the procedure it carries becomes the agent's prompt. Selecting its ref by the PR head branch name therefore lowered the bar from landing a reviewed commit on claude@master to pushing any branch to claude, and a fork PR could name its branch after one. Only an admin author pairs now; for everyone else the checkout takes master, as before. That checkout also kept the app token in its working tree, and the token reaches two repos. The probe read every failure as "branch absent": an expired token or an API outage silently became master. It now warns on anything but 200 and 404. The resolution itself existed three times in this repo. The composite action is checked out from master rather than from the pull request, so a fork PR cannot rewrite the code the workflow runs before the review starts.
|
Claude finished @otavio's task in 11m 25s —— View job Code Review CompleteReviewed 3 files. 5 inline issues posted — Standards: 0 violations, 4 judgement calls. Spec: 0. Correctness: 1. StandardsNo documented rule is broken. The worst judgement call is Correctness
SpecThe Spec axis did not run: the PR references no issue, and its commits carry no Additional notesNo findings were dropped by a cap. One asymmetry the diff leaves in place, flagged for confirmation rather than as a finding: the cloud pairing ( To request another review round, comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| STATUS=$(curl -s -o /dev/null -w '%{http_code}' "${AUTH[@]}" \ | ||
| "https://api.github.com/repos/$REPOSITORY/branches/$BRANCH") |
There was a problem hiding this comment.
Correctness: a transport failure here aborts the step instead of falling back.
A composite action's shell: bash runs as bash --noprofile --norc -e -o pipefail {0}. A bare VAR=$(cmd) assignment is not one of -e's exemptions (&&/|| lists, if/while/until conditions, !), so its exit status is curl's, and the step dies before case is reached.
Concrete path: DNS failure, connection refused, connect timeout or a TLS reset reaching api.github.com (curl exits 6/7/28/35 — without -f, an HTTP 404 or 500 still exits 0). The code this replaced put curl in an if condition, which is -e-exempt, so the same failure fell through to master and the job carried on. Now Determine cloud branch fails the whole review job on a transient network blip.
It also makes the *) arm unreachable for exactly the case its wording anticipates: curl prints 000 on a connect error, but that value never reaches $STATUS. And default-branch's own description (line 27) promises it is used when the branch "does not exist or cannot be probed" — the second half is currently only true for HTTP-level failures.
| STATUS=$(curl -s -o /dev/null -w '%{http_code}' "${AUTH[@]}" \ | |
| "https://api.github.com/repos/$REPOSITORY/branches/$BRANCH") | |
| STATUS=$(curl -s -o /dev/null -w '%{http_code}' "${AUTH[@]}" \ | |
| "https://api.github.com/repos/$REPOSITORY/branches/$BRANCH") || STATUS="000" |
000 falls into the existing *) arm, which warns and returns the default. There is no pipeline here, so pipefail is not involved. Separately, consider --max-time: with no timeout, a hung API stalls the step until the 30-minute job timeout.
| head-ref: | ||
| description: >- | ||
| The PR head branch name. Pass an empty string to skip pairing and take the default; a caller | ||
| that gates pairing on authorization does that. | ||
| required: false | ||
| default: '' |
There was a problem hiding this comment.
Standards (judgement call) — codebase-design: "Interface — everything a caller must know to use the module correctly: the type signature, but also invariants, ordering constraints, error modes […]".
The invariant this PR exists to establish — pairing happens only for an admin author — is not in the action's interface. It lives in the caller's expression, and enforcing it takes two inputs blanked in concert. head-ref: '' alone does not skip pairing: lines 60-65 re-derive the branch via gh pr view whenever pr-repository and pr-number are set. The description here says "Pass an empty string to skip pairing and take the default" without saying that pr-repository must go too.
The call site at claude-code-review.yml:209-211 is correct today — pr-repository is gated, so the fallback never fires, and the ungated pr-number on line 211 is inert. But the gate can be half-applied and nothing notices: a future caller that reads this description and blanks only head-ref silently resumes pairing through the gh pr view path, reopening the hole the commit message describes. head-ref is also carrying two jobs — a branch name and an on/off flag — which is what forces the is_admin == 'true' && … || '' ternary at both gated inputs.
Worth moving the decision behind the seam: an explicit paired:/enabled: input that owns "not paired ⇒ default branch", with the PR coordinates passed unconditionally. Then the gate is one expression, and it cannot be applied to only part of the input set.
| ref: master | ||
| sparse-checkout: .github/actions | ||
| sparse-checkout-cone-mode: false | ||
| path: .actions |
There was a problem hiding this comment.
Standards (judgement call): this new checkout keeps the token that the same commit removed from its sibling.
actions/checkout defaults persist-credentials to true, so secrets.GITHUB_TOKEN lands as an http.extraheader in .actions/.git/config and stays there for the rest of the job — inside the workspace the review agent has Read/Grep over. This step only reads action.yml off disk; nothing later runs a git operation in .actions/, so the credential has no reason to persist.
The commit message states the rule it should be following ("That checkout also kept the app token in its working tree, and the token reaches two repos"), and applies it to the claude checkout at line 221 — but not to the checkout this commit introduces.
| path: .actions | |
| path: .actions | |
| persist-credentials: false |
Same at .github/workflows/claude.yml:67-73, where it matters more: that job holds permissions: contents: write, so the persisted GITHUB_TOKEN is write-scoped.
| - name: Checkout trusted actions | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 | ||
| with: | ||
| ref: master | ||
| sparse-checkout: .github/actions | ||
| sparse-checkout-cone-mode: false | ||
| path: .actions |
There was a problem hiding this comment.
Standards (judgement call): this checkout is unguarded while its only consumer is guarded, and the two workflows now diverge on it for no stated reason.
Determine cloud branch (line 77) carries if: github.event_name == 'pull_request_review_comment' || (github.event_name == 'issue_comment' && github.event.issue.pull_request), and it is the sole reference to .actions in this file. The job-level if admits an @claude mention on a plain (non-PR) issue, so on that path this step clones for a step that never runs. The counterpart in claude-code-review.yml:151 is guarded (steps.gate.outputs.proceed == 'true').
Copying the consumer's condition onto the checkout would restore the symmetry. (I'm not offering a suggestion block — the condition is long enough that it wants to be read against line 77 rather than pasted.)
Minor, and not something this branch introduces: .actions/ joins cloud/ and claude/ as an untracked directory at the repo root that .gitignore does not cover, in the workflow where the agent holds contents: write. git-workflow.md's "Never git add -A or git add ." is the only thing keeping it out of a commit.
| default-branch: | ||
| description: Branch to use when the paired branch does not exist or cannot be probed. | ||
| required: false | ||
| default: master | ||
| token: | ||
| description: Token authorising the branch probe. Required when the repository is private. | ||
| required: false | ||
| default: '' |
There was a problem hiding this comment.
Standards (judgement call): two inputs on this new interface are worth a second look — Speculative Generality on one, a silent failure mode on the other.
default-branch is a knob no caller turns. All three call sites (claude.yml:80, claude-code-review.yml:164 and :208) take the master default.
token matters more. It is required: false with an empty default, and both probed repos are private — GitHub answers a private-repo endpoint with 404, not 401/403, precisely so it does not leak existence. So a caller that forgets token gets a 404, which line 84 maps to "branch absent", and the job proceeds on master with nothing in the log. That is the exact failure mode this commit set out to eliminate ("The probe read every failure as 'branch absent': an expired token or an API outage silently became master"); a missing token still slips through the one arm that stayed silent. The rate-limit case is fine — an unauthenticated request over the limit returns 403, which the new *) arm catches and warns on.
No caller omits token today, so this is about the interface rather than a live bug. required: true would make the misconfiguration loud.
1 participant
What
Addresses the review feedback on the branch-pairing change (shellhub-io/cloud#2545): the
claudecheckout only pairs for an admin author, no longer keeps the app token, and the branch probe distinguishes "absent" from "could not ask". The resolution moved into a composite action used by every call site.Why
The
claudecheckout is executed, not read:workspace.shruns from it, the.claude/it syncs supplies hooks andsettings.jsonthat run outside the review's--allowedTools, andpr-review.mdbecomes the agent prompt. Making its ref selectable by PR head branch name dropped the bar from "land a reviewed commit onclaude@master" to "push any branch toclaude" — and onpull_request_targeta fork PR can name its branch after an existing one.Changes
is_admin, so theCheck PR author team membershipstep moved above the resolution it now feeds. A non-admin author (every fork author) getsmaster.persist-credentials: falseon theclaudecheckout — the app token reaches two repos and had no reason to stay in a tree that gets executed.curl -sfcollapsed 401/403/5xx into "branch absent". The action now reads the status code and warns on anything but 200 and 404.claude.yml). It is checked out frommasterinto.actions/, not taken from the PR head, so a fork cannot rewrite what the workflow executes before the review starts.claude.yml: uses the same action for its sibling-repo resolution. Itsclaudecheckout stays unpaired — it has no admin gate, and pairing without one is the escalation described above.Testing
The probe paths were exercised locally: an existing branch resolves to itself, a missing one falls back silently, and a bad token now emits
::warning::... returned HTTP 401; using masterinstead of passing as absent.