fix(review): fail loudly when the context script is missing

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

@@ -111,9 +111,32 @@ jobs:
           echo "$PROMPT" >> $GITHUB_OUTPUT
           echo "EOF" >> $GITHUB_OUTPUT
 
-      - name: Verify jq is available
+      # Both preconditions for the context step, checked here because this step has no
+      # continue-on-error and that step does. A missing script is otherwise the quietest failure
+      # this workflow has: exit 127 swallowed into a green run, an empty context, and -- before
+      # the review step was gated on the context outcome -- a review that read an absent history
+      # as a clean first cycle. The prompt document has always had this protection, because
+      # `cat` on a missing file fails the job; the script needs it spelled out.
+      #
+      # It guards a future break of the sparse-checkout, not a present one: a renamed path, a
+      # pattern that stops matching in non-cone mode, a file moved without updating the checkout.
+      #
+      # It also fires on a rename done *correctly*, and that is not a bug to fix here. The pattern
+      # and the path come from the pull request; the tree they are matched against is `main`. So a
+      # PR that renames the script consistently still fails its own smoke job, because main has
+      # only the old path to hand the checkout. Landing a rename is two merges: add the file at the
+      # new path, then switch the pattern and the `run:` over. The prompt document has always
+      # carried the same constraint through `cat`.
+      - name: Verify the context step's preconditions
         if: github.event.pull_request.user.login != 'dependabot[bot]'
-        run: jq --version
+        run: |
+          jq --version
+          script=.github-workflows/scripts/gather-review-context.sh
+          if [ ! -f "$script" ]; then
+            echo "$script is missing from the cross-repo checkout." >&2
+            echo "Check the sparse-checkout patterns against the paths in hotdata-dev/github-workflows@main." >&2
+            exit 1
+          fi
 
       # Frontloads what a week of tool-usage artifacts showed the reviewer fetching for
       # itself, one denied command at a time. Over 109 runs it averaged 19.5 Bash calls and
@@ -128,18 +151,23 @@ jobs:
       - name: Gather review context
         if: github.event.pull_request.user.login != 'dependabot[bot]'
         id: context
-        # Nine API reads feed the prompt now. A failure in this step *skips* the review
-        # step, and with it the notify step's failure check, so the PR would get no review
-        # and no explanation. Every command below is guarded individually; this is the
-        # backstop that keeps a bug in one block from costing the PR its review.
+        # Nine API reads feed the prompt. Every command in the script is guarded individually,
+        # so one failing read costs its own block and nothing else -- that is what keeps a bug
+        # in one block from costing the PR its review.
+        #
+        # continue-on-error is no longer a way to salvage the review, though, and the two are
+        # easy to confuse. A non-zero exit from the *step* now skips the review outright: the
+        # review step requires steps.context.outcome == 'success'. It stays continue-on-error so
+        # the job reaches the notify step, which announces the skip; without that the failure
+        # would be a green run with no review and no comment. So the trade is a stated absence
+        # instead of a review built on nothing.
         #
-        # What it costs when it fires: pr_context is appended to $GITHUB_OUTPUT once, at the
-        # end, so an abort anywhere before that leaves the output unset and the prompt gets
-        # an *empty* <pr_context> -- not a partial one. review_cycle and threads are written
-        # earlier and survive. Each degraded block carries a sentence saying what is
-        # missing, but a degraded step carries nothing, so the prompt tells the reviewer to
-        # fetch what it needs itself when the block is empty. Without that line the prompt
-        # would be telling it not to re-fetch context it never received.
+        # Why not let it through: pr_context is appended to $GITHUB_OUTPUT once, at the end, so
+        # an abort anywhere before that leaves it unset entirely rather than partial, while
+        # review_cycle and threads were written earlier and survive. A degraded *block* carries
+        # a sentence saying what is missing; a degraded *step* carries nothing, and the surviving
+        # outputs then assert a clean cycle 1 that never happened. The prompt's "fetch it
+        # yourself if the block is empty" line still covers the per-block case.
         continue-on-error: true
         # The work is a script rather than an inline block because Actions parses a `run:` block
         # as one template expression and refuses any over 21,000 characters. Inline, this one was
@@ -162,9 +190,10 @@ 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.
+      # The only step a dry run skips by design. Two of the three steps after it are already gated
+      # on its outcome or its outputs, so skipping it silences them for free; the notify step needs
+      # its own condition, because it can also fire on a context failure and would otherwise
+      # comment during a smoke run.
       #
       #   Reduce execution log  -- needs steps.review.outputs.execution_file, empty when skipped
       #   Upload tool usage     -- needs steps.tool-usage.outcome == 'success', which is 'skipped'

scripts/gather-review-context.sh

@@ -23,7 +23,13 @@
 # assume it.
 set -eo pipefail
 
-: "${PR_NUMBER:?the calling step must set PR_NUMBER}"
+# `?` and not `:?` for PR_NUMBER, so unset is an error but empty is not. tests.yml calls the
+# workflow on `push: branches: [main]` too, where there is no pull request and the number is
+# legitimately empty; the reads below then degrade into their guarded "could not read" sentences,
+# which is what the inline version did and what keeps the main-push smoke run exercising the
+# script rather than stopping on its first line. REPO comes from github.repository and is never
+# legitimately empty, so it keeps `:?`.
+: "${PR_NUMBER?the calling step must set PR_NUMBER}"
 : "${REPO:?the calling step must set REPO}"
 
 # Caps. The median PR reviewed across the org is 161 changed lines and the largest
@@ -185,14 +191,9 @@ DELIMITER="REVIEW_CONTEXT_$(openssl rand -hex 16)"
 
 # Title and body reach the shell through env, never an Actions expression
 # interpolation: both are attacker-controlled text and would otherwise be spliced
-# into this script.
+# into this script. (The header explains why no interpolation can reach this file at
+# all now; tests/context-step-test.sh still rejects the delimiter anywhere in it.)
 #
-# That expression syntax cannot be written out inside this run block, not even in a
-# comment. Actions parses those delimiters in the block's *string value*, comments
-# included, and an empty pair is a syntax error that makes the whole workflow
-# unparseable -- no jobs, no required check, every PR in the org blocked behind
-# "Please close and reopen the PR to trigger this workflow". A YAML comment outside
-# a block scalar is safe, because the YAML parser strips it before Actions looks.
 # First in the file on purpose: the byte cap keeps the head, so anything the
 # reviewer must not miss has to be above the blocks that can grow.
 if [ -s "$WARN_FILE" ]; then

tests/context-step-test.sh

@@ -196,7 +196,7 @@ run_step() {
     RUNNER_TEMP="$WORK/rt" \
     GITHUB_OUTPUT="$WORK/out.txt" \
     STUB_FIXTURES="$PWD/tests/fixtures" \
-    PR_NUMBER=172 \
+    PR_NUMBER="${PR_NUMBER-172}" \
     REPO=hotdata-dev/dlthubworker \
     STUB_JOB_LOG="${STUB_JOB_LOG:-job-log-django.txt}" \
     GH_VERSION="${GH_VERSION:-2.96}" \
@@ -557,6 +557,26 @@ reviews|prior reviews could not be read
 comments|prior inline review comments could not be read
 ENDPOINTS
 
+# An empty PR_NUMBER is a real case, not a caller bug: tests.yml calls the workflow on
+# `push: branches: [main]`, where there is no pull request. The first guard in the script was
+# written `${PR_NUMBER:?}`, which fires on empty as well as unset, so the whole script exited on
+# line 26 of every main-push smoke run -- the run stayed green because the step is
+# continue-on-error, and the smoke test silently stopped covering anything past that line.
+# What this proves is narrow and worth stating exactly: the guard does not fire and the script
+# runs to the end. The reads do not degrade under the stub -- an empty number still produces
+# `repos/.../pulls//reviews` and `gh pr diff ""`, which match the stub's patterns as happily as a
+# real number does, so the context comes out fully populated. Against the real gh an empty
+# selector resolves the PR from the current branch, and on a push to main there is no such PR, so
+# the reads degrade into their guarded sentences there. Either way the script reaches them.
+PR_NUMBER='' run_step "no pull request number" > "$WORK/code.txt"
+expect "$(cat "$WORK/code.txt")" "0" "step exits 0 when there is no pull request number"
+if grep -q "must set PR_NUMBER" "$WORK/step.out"; then
+  echo "FAIL an empty PR_NUMBER aborts the script; a main-push smoke run covers nothing"
+  failures=$((failures + 1))
+else
+  echo "ok   an empty PR_NUMBER runs the script to the end instead of tripping the guard"
+fi
+
 if [ "$failures" -ne 0 ]; then
   echo "$failures test(s) failed"
   exit 1

tests/workflow-lint-test.sh

@@ -162,6 +162,35 @@ 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"
 
+# A missing context script has to fail loudly somewhere, and it cannot be the context step: that
+# one is continue-on-error, so `bash <missing file>` exits 127 into a green run. The prompt
+# document gets this for free -- `cat` on a missing file fails its step -- and the script needs an
+# explicit check to match.
+#
+# Scoped to the step, not grepped over the file, because where the check sits is the whole property
+# being asserted: the same `-f` test moved into the context step would satisfy a file-wide grep and
+# prove nothing. So pull the one step and require both halves -- that it tests for the script, and
+# that it is not continue-on-error.
+precondition_step=$(awk '/^      - name: Verify the context step.s preconditions$/ { found = 1; next }
+                         found && /^      - / { exit }
+                         found { print }' "$WORKFLOW_FILE")
+if [ -z "$precondition_step" ]; then
+  echo "FAIL no 'Verify the context step's preconditions' step in $WORKFLOW_FILE; a missing"
+  echo "     context script would exit 127 inside a continue-on-error step and leave the run"
+  echo "     green with an empty context"
+  failures=$((failures + 1))
+elif ! printf '%s\n' "$precondition_step" | grep -q -- '-f .*gather-review-context\.sh\|script=.*gather-review-context\.sh'; then
+  echo "FAIL the precondition step does not test for the context script:"
+  printf '%s\n' "$precondition_step" | sed 's/^/       /'
+  failures=$((failures + 1))
+elif printf '%s\n' "$precondition_step" | grep -q 'continue-on-error'; then
+  echo "FAIL the precondition step is continue-on-error, so the check it makes cannot fail the"
+  echo "     job and proves nothing"
+  failures=$((failures + 1))
+else
+  echo "ok   a missing context script fails the job rather than emptying the context"
+fi
+
 # The context step is continue-on-error, so everything it can fail at -- a checkout that does not
 # deliver the script, a bad path, a rename that stops matching the sparse pattern -- leaves the run
 # green with pr_context, threads and review_cycle all unset. Ungated, the review step then runs on