Merge pull request #27 from hotdata-dev/feat/workflow-smoke-test

test(ci): smoke-run the review workflow in CI

Author: zfarrell

.github/workflows/claude-pr-review.yml

@@ -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:
@@ -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:

.github/workflows/tests.yml

@@ -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
+    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

tests/workflow-lint-test.sh

@@ -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' '{')"
@@ -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)
+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
@@ -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.