Skip to content

ci(review): gate the claude pairing and share the branch probe #7076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
otavio merged 1 commit into from
Sep 10, 2026
+152 −85
Merged

ci(review): gate the claude pairing and share the branch probe #7076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions .github/actions/paired-branch/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
name: Paired branch
description: >-
Resolve the branch of another repository that pairs with this pull request — one named the same
as the PR head branch — falling back to a default when there is none.

inputs:
repository:
description: Repository to look the branch up in, as owner/name.
required: true
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: ''
Comment on lines +10 to +15

@github-actions github-actions Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pr-repository:
description: >-
Repository holding the pull request, as owner/name. Used to look the head branch up when
head-ref is empty because the event payload carries no head — an issue_comment event.
required: false
default: ''
pr-number:
description: Pull request number, read together with pr-repository.
required: false
default: ''
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: ''
Comment on lines +26 to +33

@github-actions github-actions Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

gh-token:
description: Token for the gh CLI, used only when looking up the head branch name.
required: false
default: ''

outputs:
ref:
description: The paired branch, or the default branch.
value: ${{ steps.resolve.outputs.ref }}

runs:
using: composite
steps:
- id: resolve
shell: bash
env:
REPOSITORY: ${{ inputs.repository }}
HEAD_REF: ${{ inputs.head-ref }}
PR_REPOSITORY: ${{ inputs.pr-repository }}
PR_NUMBER: ${{ inputs.pr-number }}
DEFAULT_BRANCH: ${{ inputs.default-branch }}
TOKEN: ${{ inputs.token }}
GH_TOKEN: ${{ inputs.gh-token }}
run: |
BRANCH="$HEAD_REF"

if [[ -z "$BRANCH" && -n "$PR_REPOSITORY" && -n "$PR_NUMBER" ]]; then
BRANCH=$(gh pr view "$PR_NUMBER" --repo "$PR_REPOSITORY" --json headRefName --jq '.headRefName') || {
echo "::warning::Could not read the head branch of $PR_REPOSITORY#$PR_NUMBER; using $DEFAULT_BRANCH"
BRANCH=""
}
fi

if [[ -z "$BRANCH" ]]; then
echo "ref=$DEFAULT_BRANCH" >> "$GITHUB_OUTPUT"
exit 0
fi

AUTH=()
if [[ -n "$TOKEN" ]]; then
AUTH=(-H "Authorization: Bearer $TOKEN")
fi

STATUS=$(curl -s -o /dev/null -w '%{http_code}' "${AUTH[@]}" \
"https://api.github.com/repos/$REPOSITORY/branches/$BRANCH")
Comment on lines +77 to +78

@github-actions github-actions Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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.


case "$STATUS" in
200)
echo "ref=$BRANCH" >> "$GITHUB_OUTPUT"
;;
404)
echo "ref=$DEFAULT_BRANCH" >> "$GITHUB_OUTPUT"
;;
*)
echo "::warning::Probing $REPOSITORY for branch $BRANCH returned HTTP $STATUS; using $DEFAULT_BRANCH"
echo "ref=$DEFAULT_BRANCH" >> "$GITHUB_OUTPUT"
;;
esac
108 changes: 45 additions & 63 deletions .github/workflows/claude-code-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,31 +147,26 @@ jobs:
gh api "repos/$REPO/issues/comments/$id" -X DELETE > /dev/null || true
done

- name: Checkout trusted actions
if: steps.gate.outputs.proceed == 'true'
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
with:
ref: master
sparse-checkout: .github/actions
sparse-checkout-cone-mode: false
path: .actions

@github-actions github-actions Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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: Determine cloud branch
id: cloud-branch
if: steps.gate.outputs.proceed == 'true'
env:
HEAD_REF: ${{ github.head_ref || github.event.pull_request.head.ref }}
PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }}
APP_TOKEN: ${{ steps.app-token.outputs.token }}
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
REPO: ${{ github.repository }}
run: |
if [[ -n "$HEAD_REF" ]]; then
BRANCH="$HEAD_REF"
else
BRANCH=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json headRefName --jq '.headRefName') || {
echo "::warning::Failed to resolve PR head ref name, falling back to master"
true
}
BRANCH="${BRANCH:-master}"
fi
if curl -sf -H "Authorization: Bearer $APP_TOKEN" \
"https://api.github.com/repos/shellhub-io/cloud/branches/$BRANCH" > /dev/null 2>&1; then
echo "ref=$BRANCH" >> "$GITHUB_OUTPUT"
else
echo "ref=master" >> "$GITHUB_OUTPUT"
fi
uses: ./.actions/.github/actions/paired-branch
with:
repository: shellhub-io/cloud
head-ref: ${{ github.head_ref || github.event.pull_request.head.ref }}
pr-repository: ${{ github.repository }}
pr-number: ${{ github.event.pull_request.number || github.event.issue.number }}
token: ${{ steps.app-token.outputs.token }}
gh-token: ${{ secrets.GITHUB_TOKEN }}

- name: Checkout cloud (context)
if: steps.gate.outputs.proceed == 'true'
Expand All @@ -183,47 +178,6 @@ jobs:
fetch-depth: 1
path: cloud

- name: Determine claude branch
id: claude-branch
if: steps.gate.outputs.proceed == 'true'
env:
HEAD_REF: ${{ github.head_ref || github.event.pull_request.head.ref }}
PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }}
APP_TOKEN: ${{ steps.app-token.outputs.token }}
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
REPO: ${{ github.repository }}
run: |
if [[ -n "$HEAD_REF" ]]; then
BRANCH="$HEAD_REF"
else
BRANCH=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json headRefName --jq '.headRefName') || {
echo "::warning::Failed to resolve PR head ref name, falling back to master"
true
}
BRANCH="${BRANCH:-master}"
fi
if curl -sf -H "Authorization: Bearer $APP_TOKEN" \
"https://api.github.com/repos/shellhub-io/claude/branches/$BRANCH" > /dev/null 2>&1; then
echo "ref=$BRANCH" >> "$GITHUB_OUTPUT"
else
echo "ref=master" >> "$GITHUB_OUTPUT"
fi

- name: Checkout claude config
if: steps.gate.outputs.proceed == 'true'
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
with:
repository: shellhub-io/claude
token: ${{ steps.app-token.outputs.token }}
ref: ${{ steps.claude-branch.outputs.ref }}
fetch-depth: 1
path: claude

- name: Setup workspace context
if: steps.gate.outputs.proceed == 'true'
run: |
"$GITHUB_WORKSPACE/claude/workspace.sh" sync -w "$GITHUB_WORKSPACE" --project shellhub

- name: Check PR author team membership
id: author-check
if: steps.gate.outputs.proceed == 'true'
Expand All @@ -246,6 +200,34 @@ jobs:
echo "is_admin=false" >> "$GITHUB_OUTPUT"
fi

- name: Determine claude branch
id: claude-branch
if: steps.gate.outputs.proceed == 'true'
uses: ./.actions/.github/actions/paired-branch
with:
repository: shellhub-io/claude
head-ref: ${{ steps.author-check.outputs.is_admin == 'true' && (github.head_ref || github.event.pull_request.head.ref) || '' }}
pr-repository: ${{ steps.author-check.outputs.is_admin == 'true' && github.repository || '' }}
pr-number: ${{ github.event.pull_request.number || github.event.issue.number }}
token: ${{ steps.app-token.outputs.token }}
gh-token: ${{ secrets.GITHUB_TOKEN }}

- name: Checkout claude config
if: steps.gate.outputs.proceed == 'true'
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
with:
repository: shellhub-io/claude
token: ${{ steps.app-token.outputs.token }}
persist-credentials: false
ref: ${{ steps.claude-branch.outputs.ref }}
fetch-depth: 1
path: claude

- name: Setup workspace context
if: steps.gate.outputs.proceed == 'true'
run: |
"$GITHUB_WORKSPACE/claude/workspace.sh" sync -w "$GITHUB_WORKSPACE" --project shellhub

- name: Load review procedure
id: review-procedure
if: steps.gate.outputs.proceed == 'true'
Expand Down
38 changes: 16 additions & 22 deletions .github/workflows/claude.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,31 +64,25 @@ jobs:
with:
ref: ${{ steps.pr-ref.outputs.sha || '' }}

- name: Checkout trusted actions
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
with:
ref: master
sparse-checkout: .github/actions
sparse-checkout-cone-mode: false
path: .actions
Comment on lines +67 to +73

@github-actions github-actions Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


- name: Determine cloud branch
id: cloud-branch
if: github.event_name == 'pull_request_review_comment' || (github.event_name == 'issue_comment' && github.event.issue.pull_request)
env:
HEAD_REF: ${{ github.head_ref || github.event.pull_request.head.ref }}
PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }}
APP_TOKEN: ${{ steps.app-token.outputs.token }}
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
REPO: ${{ github.repository }}
run: |
if [[ -n "$HEAD_REF" ]]; then
BRANCH="$HEAD_REF"
else
BRANCH=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json headRefName --jq '.headRefName') || {
echo "::warning::Failed to resolve PR head ref name, falling back to master"
true
}
BRANCH="${BRANCH:-master}"
fi
if curl -sf -H "Authorization: Bearer $APP_TOKEN" \
"https://api.github.com/repos/shellhub-io/cloud/branches/$BRANCH" > /dev/null 2>&1; then
echo "ref=$BRANCH" >> "$GITHUB_OUTPUT"
else
echo "ref=master" >> "$GITHUB_OUTPUT"
fi
uses: ./.actions/.github/actions/paired-branch
with:
repository: shellhub-io/cloud
head-ref: ${{ github.head_ref || github.event.pull_request.head.ref }}
pr-repository: ${{ github.repository }}
pr-number: ${{ github.event.pull_request.number || github.event.issue.number }}
token: ${{ steps.app-token.outputs.token }}
gh-token: ${{ secrets.GITHUB_TOKEN }}

- name: Checkout cloud (context)
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
Expand Down
Loading