Skip to content

Commit 71bb2c4

Browse files
committed
docs(review): correct comments the context gate inverted
1 parent 0d9c0f7 commit 71bb2c4

3 files changed

Lines changed: 36 additions & 23 deletions

File tree

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

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,13 @@ jobs:
120120
#
121121
# It guards a future break of the sparse-checkout, not a present one: a renamed path, a
122122
# pattern that stops matching in non-cone mode, a file moved without updating the checkout.
123+
#
124+
# It also fires on a rename done *correctly*, and that is not a bug to fix here. The pattern
125+
# and the path come from the pull request; the tree they are matched against is `main`. So a
126+
# PR that renames the script consistently still fails its own smoke job, because main has
127+
# only the old path to hand the checkout. Landing a rename is two merges: add the file at the
128+
# new path, then switch the pattern and the `run:` over. The prompt document has always
129+
# carried the same constraint through `cat`.
123130
- name: Verify the context step's preconditions
124131
if: github.event.pull_request.user.login != 'dependabot[bot]'
125132
run: |
@@ -144,18 +151,23 @@ jobs:
144151
- name: Gather review context
145152
if: github.event.pull_request.user.login != 'dependabot[bot]'
146153
id: context
147-
# Nine API reads feed the prompt now. A failure in this step *skips* the review
148-
# step, and with it the notify step's failure check, so the PR would get no review
149-
# and no explanation. Every command below is guarded individually; this is the
150-
# backstop that keeps a bug in one block from costing the PR its review.
154+
# Nine API reads feed the prompt. Every command in the script is guarded individually,
155+
# so one failing read costs its own block and nothing else -- that is what keeps a bug
156+
# in one block from costing the PR its review.
157+
#
158+
# continue-on-error is no longer a way to salvage the review, though, and the two are
159+
# easy to confuse. A non-zero exit from the *step* now skips the review outright: the
160+
# review step requires steps.context.outcome == 'success'. It stays continue-on-error so
161+
# the job reaches the notify step, which announces the skip; without that the failure
162+
# would be a green run with no review and no comment. So the trade is a stated absence
163+
# instead of a review built on nothing.
151164
#
152-
# What it costs when it fires: pr_context is appended to $GITHUB_OUTPUT once, at the
153-
# end, so an abort anywhere before that leaves the output unset and the prompt gets
154-
# an *empty* <pr_context> -- not a partial one. review_cycle and threads are written
155-
# earlier and survive. Each degraded block carries a sentence saying what is
156-
# missing, but a degraded step carries nothing, so the prompt tells the reviewer to
157-
# fetch what it needs itself when the block is empty. Without that line the prompt
158-
# would be telling it not to re-fetch context it never received.
165+
# Why not let it through: pr_context is appended to $GITHUB_OUTPUT once, at the end, so
166+
# an abort anywhere before that leaves it unset entirely rather than partial, while
167+
# review_cycle and threads were written earlier and survive. A degraded *block* carries
168+
# a sentence saying what is missing; a degraded *step* carries nothing, and the surviving
169+
# outputs then assert a clean cycle 1 that never happened. The prompt's "fetch it
170+
# yourself if the block is empty" line still covers the per-block case.
159171
continue-on-error: true
160172
# The work is a script rather than an inline block because Actions parses a `run:` block
161173
# as one template expression and refuses any over 21,000 characters. Inline, this one was
@@ -178,9 +190,10 @@ jobs:
178190
PR_TITLE: ${{ github.event.pull_request.title }}
179191
PR_BODY: ${{ github.event.pull_request.body }}
180192

181-
# The only step a dry run skips, and the only one it needs to: every step after this one
182-
# is already gated on this step's outcome or its outputs, so skipping it silences the whole
183-
# write side of the workflow without a second condition anywhere.
193+
# The only step a dry run skips by design. Two of the three steps after it are already gated
194+
# on its outcome or its outputs, so skipping it silences them for free; the notify step needs
195+
# its own condition, because it can also fire on a context failure and would otherwise
196+
# comment during a smoke run.
184197
#
185198
# Reduce execution log -- needs steps.review.outputs.execution_file, empty when skipped
186199
# Upload tool usage -- needs steps.tool-usage.outcome == 'success', which is 'skipped'

‎scripts/gather-review-context.sh‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -191,14 +191,9 @@ DELIMITER="REVIEW_CONTEXT_$(openssl rand -hex 16)"
191191

192192
# Title and body reach the shell through env, never an Actions expression
193193
# interpolation: both are attacker-controlled text and would otherwise be spliced
194-
# into this script.
194+
# into this script. (The header explains why no interpolation can reach this file at
195+
# all now; tests/context-step-test.sh still rejects the delimiter anywhere in it.)
195196
#
196-
# That expression syntax cannot be written out inside this run block, not even in a
197-
# comment. Actions parses those delimiters in the block's *string value*, comments
198-
# included, and an empty pair is a syntax error that makes the whole workflow
199-
# unparseable -- no jobs, no required check, every PR in the org blocked behind
200-
# "Please close and reopen the PR to trigger this workflow". A YAML comment outside
201-
# a block scalar is safe, because the YAML parser strips it before Actions looks.
202197
# First in the file on purpose: the byte cap keeps the head, so anything the
203198
# reviewer must not miss has to be above the blocks that can grow.
204199
if [ -s "$WARN_FILE" ]; then

‎tests/context-step-test.sh‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -562,14 +562,19 @@ ENDPOINTS
562562
# written `${PR_NUMBER:?}`, which fires on empty as well as unset, so the whole script exited on
563563
# line 26 of every main-push smoke run -- the run stayed green because the step is
564564
# continue-on-error, and the smoke test silently stopped covering anything past that line.
565-
# The reads are expected to degrade here; what matters is that the script gets to them.
565+
# What this proves is narrow and worth stating exactly: the guard does not fire and the script
566+
# runs to the end. The reads do not degrade under the stub -- an empty number still produces
567+
# `repos/.../pulls//reviews` and `gh pr diff ""`, which match the stub's patterns as happily as a
568+
# real number does, so the context comes out fully populated. Against the real gh an empty
569+
# selector resolves the PR from the current branch, and on a push to main there is no such PR, so
570+
# the reads degrade into their guarded sentences there. Either way the script reaches them.
566571
PR_NUMBER='' run_step "no pull request number" > "$WORK/code.txt"
567572
expect "$(cat "$WORK/code.txt")" "0" "step exits 0 when there is no pull request number"
568573
if grep -q "must set PR_NUMBER" "$WORK/step.out"; then
569574
echo "FAIL an empty PR_NUMBER aborts the script; a main-push smoke run covers nothing"
570575
failures=$((failures + 1))
571576
else
572-
echo "ok an empty PR_NUMBER degrades the reads instead of aborting the script"
577+
echo "ok an empty PR_NUMBER runs the script to the end instead of tripping the guard"
573578
fi
574579

575580
if [ "$failures" -ne 0 ]; then

0 commit comments

Comments
 (0)