diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 90ca503..e6022b5 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.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* -- 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' diff --git a/scripts/gather-review-context.sh b/scripts/gather-review-context.sh index 2404ff5..721d6ab 100755 --- a/scripts/gather-review-context.sh +++ b/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 diff --git a/tests/context-step-test.sh b/tests/context-step-test.sh index 7dbd2b1..f23a677 100755 --- a/tests/context-step-test.sh +++ b/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 diff --git a/tests/workflow-lint-test.sh b/tests/workflow-lint-test.sh index 88f13ed..f377be8 100755 --- a/tests/workflow-lint-test.sh +++ b/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 ` 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