Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
46 changes: 43 additions & 3 deletions .github/workflows/claude-pr-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,38 @@ name: Claude PR Review
on:
pull_request:
types: [opened, synchronize, ready_for_review, reopened]

# Callable so this repository's own CI can run this file and prove it starts. That is not a
# theoretical worry: an org ruleset injects this workflow into every repo resolved from
# `main`, so a pull request here is reviewed by main's copy and never by the copy it is
# changing. Twice in one day a file that Actions refuses to parse merged green -- an empty
# expression delimiter in a shell comment, then a `run:` block past the 21,000-character
# expression limit -- because nothing in CI had ever executed the version under review.
#
# tests.yml calls this with `uses: ./...`, which resolves from the calling commit, so the
# smoke run is the pull request's copy. A copy Actions cannot load fails the caller too, which
# is the whole point: "does it start" is exactly what both outages got wrong.
workflow_call:
inputs:
dry_run:
description: >-
Everything except the review itself. Set by tests.yml, never on the production path,
where `inputs` is empty and this reads as false.
type: boolean
default: false

# Distinct groups for the smoke run and the live review, because a called workflow inherits the
# caller's `github` context: both would otherwise compute pr-review-<same number> on the same
# pull request, and cancel-in-progress would have each cancelling the other. A smoke test that
# kills real reviews is worse than no smoke test.
#
# The run_id fallback is for the other event. tests.yml also calls this on `push: branches:
# [main]`, where there is no pull request and the number expands to nothing, so without it every
# main-push run shares one constant group -- and cancel-in-progress on a *called* workflow does
# not just drop the smoke job, it takes the caller's whole Tests run with it. Two merges close
# together would leave the earlier commit with no test signal on main. run_id is the caller's,
# and unique per run.
concurrency:
group: pr-review-${{ github.event.pull_request.number }}
group: pr-review-${{ inputs.dry_run && 'smoke' || 'live' }}-${{ github.event.pull_request.number || github.run_id }}
cancel-in-progress: true

jobs:
Expand Down Expand Up @@ -509,8 +538,19 @@ jobs:
PR_TITLE: ${{ github.event.pull_request.title }}
PR_BODY: ${{ github.event.pull_request.body }}

# The only step a dry run skips, and the only one it needs to: every step after this one
# is already gated on this step's outcome or its outputs, so skipping it silences the whole
# write side of the workflow without a second condition anywhere.
#
# Reduce execution log -- needs steps.review.outputs.execution_file, empty when skipped
# Upload tool usage -- needs steps.tool-usage.outcome == 'success', which is 'skipped'
# Notify on failure -- needs outcome 'failure' or 'cancelled', and this is 'skipped'
#
# So a smoke run posts no review, no comment and no artifact. Everything before this step
# still runs against the live API: the app token, the cross-repo prompt checkout, and the
# nine context reads with the job's real permissions.
- uses: anthropics/claude-code-action@v1
if: github.event.pull_request.user.login != 'dependabot[bot]'
if: github.event.pull_request.user.login != 'dependabot[bot]' && !inputs.dry_run
id: review
continue-on-error: true
with:
Expand Down
38 changes: 38 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,41 @@ jobs:

- name: Context step end to end
run: tests/context-step-test.sh

# Runs claude-pr-review.yml itself, from this commit, with the review step skipped. The checks
# above all read the workflow as text; this one hands it to Actions and asks whether it starts.
#
# That is the gap the last two outages went through. An org ruleset injects the review workflow
# into every repo resolved from `main`, so the review on a pull request here comes from main's
# copy -- never from the copy the pull request is changing. Both times, a file Actions refuses
# to parse merged with this suite green, and the org lost reviews until someone noticed: an
# empty expression delimiter inside a shell comment, then a `run:` block over the
# 21,000-character expression limit. Neither is visible to yaml.safe_load, to bash, or to
# actionlint, and enumerating the next limit ahead of time is a game with no end. Executing the
# file has no such gap -- if Actions will not load it, this job cannot start, and CI is red.
#
# `uses: ./` resolves from the calling commit rather than from the default branch, which is what
# makes this the pull request's copy and not main's.
smoke:
name: Review workflow starts
# The same set the called job declares. A caller cannot grant a reusable workflow more than
# it holds, and the point of this job is to exercise the real thing: the nine context reads
# each need their permission, and a missing one degrades silently into a "could not read"
# sentence rather than failing. pull-requests: write is unused on this path -- the only two
# steps that write are skipped with the review step -- but it is what production runs with,
# and a smoke test that runs with a different token is testing a different workflow.
permissions:
contents: read
pull-requests: write
id-token: write
actions: read
issues: read
checks: read
statuses: read
Comment thread
zfarrell marked this conversation as resolved.
uses: ./.github/workflows/claude-pr-review.yml
with:
dry_run: true
# The GitHub App private key, for the token step and the cross-repo prompt checkout. Both run
# in a dry run, so a broken sparse-checkout or an expired key surfaces here rather than in the
# org.
secrets: inherit
55 changes: 55 additions & 0 deletions tests/workflow-lint-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ cd "$(dirname "$0")/.."

failures=0
WORKFLOW_FILE=.github/workflows/claude-pr-review.yml
TESTS_FILE=.github/workflows/tests.yml

# The delimiter, assembled rather than written, so this file does not trip its own scan.
OPEN="\${$(printf '%s' '{')"
Expand Down Expand Up @@ -97,6 +98,30 @@ for wf in "${WORKFLOWS[@]}"; do
fi
done

# cancel-in-progress cancels whatever else is in the group, so the group needs a key that is
# never empty on any event the workflow accepts. It is keyed on the pull request number, and
# tests.yml calls this workflow on `push: branches: [main]` as well, where there is no pull
# request and that key expands to nothing -- collapsing every main-push run into one constant
# group. Two merges landing close together would then cancel each other, and because the
# cancellation lands on a *called* workflow it takes the caller's whole Tests run with it, so a
# commit on main silently loses its test signal.
#
# Checked as a property of the expression rather than by evaluating it: the PR number must be
# followed by a `||` fallback, so the group stays unique when there is no pull request.
group_line=$(grep -n '^ group:' "$WORKFLOW_FILE" | head -1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: under set -euo pipefail this assignment makes the guard below unreachable. (not blocking)

If ^ group: is gone from the workflow, grep exits 1, pipefail propagates it through head, and the exit status of an assignment is the status of its command substitution — so set -e terminates the script right here. The FAIL ... this check proves nothing branch on line 113 never runs.

The result is still a red CI, so this isn't a silent pass. But it exits after ok .../*.yml expressions are well formed with no further output, which reads like the harness died rather than like the assertion it is — and this file's own habit is that a check which can no longer prove anything says so. The other extraction of this kind (line 144) is fine because awk exits 0 on no match.

Suggested change
group_line=$(grep -n '^ group:' "$WORKFLOW_FILE" | head -1)
group_line=$(grep -n '^ group:' "$WORKFLOW_FILE" | head -1 || true)

if [ -z "$group_line" ]; then
echo "FAIL $WORKFLOW_FILE has no workflow-level concurrency group; this check proves nothing"
failures=$((failures + 1))
elif ! printf '%s\n' "$group_line" | grep -q 'pull_request\.number[[:space:]]*||'; then
echo "FAIL the concurrency group keys on the pull request number with no fallback, so on a"
echo " push to main it collapses to a constant and concurrent merges cancel each other's"
echo " Tests run:"
printf '%s\n' "$group_line" | sed 's/^/ /'
failures=$((failures + 1))
else
echo "ok the concurrency group stays unique when there is no pull request"
fi

# Every read the context step makes needs a permission declared on the job, because the job
# declares `permissions:` explicitly and anything unlisted is `none`. That failure is silent
# by design -- each block degrades to its "could not read" sentence -- so a missing line here
Expand Down Expand Up @@ -142,6 +167,36 @@ check_permission "statusCheckRollup" statuses "the StatusContext half of the CI
check_permission "/compare/" contents "the since-last-review comparison"
check_permission "/pulls/" pull-requests "the PR reads"

# The table above forces a new API call in the context step to declare its permission on the
# review job. That does nothing for the smoke job in tests.yml, which calls the review workflow
# and has to grant the same set by hand: a caller cannot give a reusable workflow more than it
# holds, so a permission added on one side and not the other fails the smoke job with Actions'
# "is requesting 'x: read', but is only allowed 'x: none'" -- loud, but on a file that looks
# unrelated to the change that caused it. Asserting the two match keeps the claim in tests.yml's
# comment true by construction instead of by review.
#
# Name and value both, so pull-requests: write degrading to read is caught too.
perm_pairs() {
awk '/^ permissions:$/ { p = 1; next }
p && /^ [a-z-]+:[[:space:]]/ { print $1, $2 }
p && /^ [a-z]/ { exit }' "$1" | sort
}
review_perms=$(perm_pairs "$WORKFLOW_FILE")
smoke_perms=$(perm_pairs "$TESTS_FILE")
if [ -z "$review_perms" ] || [ -z "$smoke_perms" ]; then
echo "FAIL a permissions block came back empty (review: $(printf '%s' "$review_perms" | wc -l)," \
"smoke: $(printf '%s' "$smoke_perms" | wc -l)); the parity check proves nothing"
failures=$((failures + 1))
elif [ "$review_perms" != "$smoke_perms" ]; then
echo "FAIL the smoke job in $TESTS_FILE does not grant what the review job declares."
echo " A caller cannot grant a reusable workflow more than it holds, so the smoke job"
echo " fails until both sides agree. Difference (< review job, > smoke job):"
diff <(printf '%s\n' "$review_perms") <(printf '%s\n' "$smoke_perms") | sed 's/^/ /'
failures=$((failures + 1))
else
echo "ok the smoke job grants exactly what the review job declares"
fi

# The scan above is a backstop for one class. actionlint checks the schema, the expression
# grammar, and the shell; run it when it is on PATH. shellcheck findings are excluded because
# the run blocks here intentionally use unquoted word splitting for job ids.
Expand Down