From 963fbdbdd38d0ff716bad1fc9288628d1cc5e8d3 Mon Sep 17 00:00:00 2001 From: Ivan Date: Tue, 10 Nov 2026 23:42:58 -0800 Subject: [PATCH 1/6] ci(claude-review): the test drives the step that sets the required check The step that turns the review's summary into the required `claude-review / claude-review` check had no test; claude-review-test.sh covered the scope and diff steps only. An edit that drops `.commit_id == env.HEAD_SHA`, or reorders the results so `tail -n 1` no longer picks the newest, would let an older head's `Blocking: 0` pass a new head whose own review crashed before its summary, and ci.yml would stay green. The test now extracts the step's run and VERDICT the way the scope cases extract theirs and runs it over the review() fixtures at a head. Only a `Blocking: 0` at the head passes, also after an inline comment's empty review and when a re-review on page 2 clears an earlier `Blocking: 1`. No review, `Blocking: 1`, `Blocking: incomplete`, an inline-only review, another user's `Blocking: 0`, a `Blocking: 0` at an older head, and a re-review that blocks after a clean one all fail. A fail counts only with the step's own `::error::`, since a crash fails the check too. Mutation sweep, each applied alone to the step (test exit, first red case): - commit filter dropped: 1, check-older-head - `tail -n 1` -> `head -n 1`: 1, check-rereview-clean (the step crashes) - `| sort` before `tail -n 1`: 1, check-rereview-clean - `--paginate` dropped: 1, check-rereview-clean - `.body != ""` dropped: 1, check-inline-after - user filter dropped: 1, check-other-user - `split("\n")[0]` -> `[-1]`: 1, check-clean - fail only on `Blocking: [1-9]*`: 1, check-none Unmutated: 0. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FH67SxLACTMXjF9NCk3D5q --- .github/scripts/claude-review-test.sh | 63 +++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/.github/scripts/claude-review-test.sh b/.github/scripts/claude-review-test.sh index 8539555..9cbc368 100644 --- a/.github/scripts/claude-review-test.sh +++ b/.github/scripts/claude-review-test.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# claude-review.yml's scope and diff steps against fixtures, and the JSON its -# claude_args carries; run from the repo root. +# claude-review.yml's scope, diff and verdict steps against fixtures, and the +# JSON its claude_args carries; run from the repo root. set -euo pipefail fail() { @@ -35,7 +35,7 @@ gh() { shift done case "$path" in - "repos/$GITHUB_REPOSITORY/pulls/$PR/reviews?per_page=100") kind=reviews ;; + "repos/$GITHUB_REPOSITORY/pulls/$PR/reviews?per_page=100" | "repos/$GITHUB_REPOSITORY/pulls/$PR/reviews") kind=reviews ;; "repos/$GITHUB_REPOSITORY/pulls/$PR/comments?per_page=100") kind=comments ;; *) echo "gh stub: unexpected path $path" >&2; return 1 ;; esac @@ -336,3 +336,60 @@ for agent in ("correctness-reviewer", "design-reviewer", "verifier"): # shellcheck disable=SC2016 # the backticks are the prompt's own, not expansions [[ "$prompt" == *'an incomplete one ends with a `Read:` line, then the `- :` line of each scope unit'* ]] || fail "prompt: step 3 no longer asks for the \`Read:\` format VERDICTS parses" + +# The step that sets the required check, run over a case's reviews at head 9. +# A crash fails the check too, so a fail counts only with the step's ::error::. +check_step='.jobs.claude-review.steps[] | select(.env.VERDICT != null)' +check_script="$(yq -e -r "$check_step | .run" "$WORKFLOW_FILE")" +check_jq="$(yq -e -r "$check_step | .env.VERDICT" "$WORKFLOW_FILE")" +assert_check() { + local name="$1" want="$2" got=pass + PAGES="$PAGES" GITHUB_REPOSITORY=org/repo PR=7 HEAD_SHA="$(sha 9)" VERDICT="$check_jq" \ + bash --noprofile --norc -eo pipefail -c "$check_script" >"$work/$name.out" 2>"$work/$name.err" || got=fail + [[ "$got" == pass ]] || grep -q '^::error::' "$work/$name.out" || fail "check $name: the step crashed: $(cat "$work/$name.err")" + [[ "$got" == "$want" ]] || fail "check $name: the check would $got, want $want" +} +clean=$'Blocking: 0\n\n**Verdict:** no findings' + +new_case check-clean +review "$bot" 9 "$clean" | page reviews 1 +assert_check check-clean pass + +new_case check-none +assert_check check-none fail + +new_case check-blocking +review "$bot" 9 'Blocking: 1' | page reviews 1 +assert_check check-blocking fail + +new_case check-incomplete +review "$bot" 9 'Blocking: incomplete' | page reviews 1 +assert_check check-incomplete fail + +# An inline comment is a review with an empty body: alone it is no verdict, and +# after the summary it must not hide it. +new_case check-inline-only +review "$bot" 9 '' | page reviews 1 +assert_check check-inline-only fail +new_case check-inline-after +{ review "$bot" 9 "$clean"; review "$bot" 9 '' 10; } | page reviews 1 +assert_check check-inline-after pass + +new_case check-other-user +review someone 9 "$clean" | page reviews 1 +assert_check check-other-user fail + +# A head whose own review crashed before its summary must not pass on the last +# head's. +new_case check-older-head +review "$bot" 8 "$clean" | page reviews 1 +assert_check check-older-head fail + +# A re-review at the head decides, its page past the first included. +new_case check-rereview-clean +review "$bot" 9 'Blocking: 1' | page reviews 1 +review "$bot" 9 "$clean" 10 | page reviews 2 +assert_check check-rereview-clean pass +new_case check-rereview-blocks +{ review "$bot" 9 "$clean"; review "$bot" 9 'Blocking: 1' 10; } | page reviews 1 +assert_check check-rereview-blocks fail From c4ee0fbaaa1163a7915bd2737f7309cd3199ac77 Mon Sep 17 00:00:00 2001 From: Ivan Date: Tue, 10 Nov 2026 23:52:34 -0800 Subject: [PATCH 2/6] ci(claude-review): both steps read reviews at one path, and the test pins no fail-open From the review on #37: - The test's gh stub had widened its reviews arm to serve the verdict step's path with no per_page, so dropping `?per_page=100` from the scope step no longer failed the test. The verdict step now reads `reviews?per_page=100` like the scope step, and the stub keeps its one strict arm. Live on pixtuoid/pixtuoid#2213: 30 reviews on a default page, 100 with per_page=100, 128 paginated either way, so the step makes fewer calls for the same data. - check-inline-after pinned `.body != ""` as a pass. That filter changes the result only when an empty review is the newest at the head: a re-run that posts inline findings and stops before its summary then passes on the head's earlier `Blocking: 0`. It goes, so the test pins no fail-open, and the PR body's live-order claim holds for the one empty-body case left. Dropping the filter only makes the gate stricter, so leaving it unpinned lets no non-clean head pass. Mutation sweep, each applied alone (test exit, first red case): - verdict step's `?per_page=100` dropped: 1, check-clean (stub refuses) - scope step's `?per_page=100` dropped: 1, first (stub refuses) - `.body != ""` dropped: 0 (stricter only) The earlier rows still hold. Unmutated: 0. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/claude-review-test.sh | 12 ++++-------- .github/workflows/claude-review.yml | 2 +- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/.github/scripts/claude-review-test.sh b/.github/scripts/claude-review-test.sh index 9cbc368..bca28ee 100644 --- a/.github/scripts/claude-review-test.sh +++ b/.github/scripts/claude-review-test.sh @@ -19,8 +19,8 @@ work="$(mktemp -d)" trap 'rm -rf "$work"' EXIT # Serves $PAGES/..json in page order, each through --jq as gh does, -# and only page 1 without --paginate; anything else is a call the step must -# not make. +# and only page 1 without --paginate; anything else is a call no step may +# make. gh() { local paginate=false path="" expr="" kind [[ "${1:-}" == api ]] || { echo "gh stub: unexpected: $*" >&2; return 1; } @@ -35,7 +35,7 @@ gh() { shift done case "$path" in - "repos/$GITHUB_REPOSITORY/pulls/$PR/reviews?per_page=100" | "repos/$GITHUB_REPOSITORY/pulls/$PR/reviews") kind=reviews ;; + "repos/$GITHUB_REPOSITORY/pulls/$PR/reviews?per_page=100") kind=reviews ;; "repos/$GITHUB_REPOSITORY/pulls/$PR/comments?per_page=100") kind=comments ;; *) echo "gh stub: unexpected path $path" >&2; return 1 ;; esac @@ -366,14 +366,10 @@ new_case check-incomplete review "$bot" 9 'Blocking: incomplete' | page reviews 1 assert_check check-incomplete fail -# An inline comment is a review with an empty body: alone it is no verdict, and -# after the summary it must not hide it. +# An inline comment is a review with an empty body, so alone it is no verdict. new_case check-inline-only review "$bot" 9 '' | page reviews 1 assert_check check-inline-only fail -new_case check-inline-after -{ review "$bot" 9 "$clean"; review "$bot" 9 '' 10; } | page reviews 1 -assert_check check-inline-after pass new_case check-other-user review someone 9 "$clean" | page reviews 1 diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index f6809d5..d566ded 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -254,7 +254,7 @@ jobs: | .body | split("\n")[0] run: | set -euo pipefail - verdict="$(gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR/reviews" --jq "$VERDICT" | tail -n 1)" + verdict="$(gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR/reviews?per_page=100" --jq "$VERDICT" | tail -n 1)" if [[ "$verdict" != "Blocking: 0" ]]; then echo "::error::the review at ${HEAD_SHA::7} reports a blocking finding or no verdict: fix it, or disposition and resolve its thread, then re-run" exit 1 From f253005213f15a2e5e7d5b19994bbbb51e2c0eff Mon Sep 17 00:00:00 2001 From: Ivan Date: Wed, 11 Nov 2026 00:02:06 -0800 Subject: [PATCH 3/6] ci(claude-review): an inline comment newer than the head's summary fails the check From the re-review on #37 (blocking): VERDICT dropped empty-body reviews before `tail -n 1`, so a re-run at a head that posted an inline finding and stopped before its summary passed on the head's earlier `Blocking: 0`. The filter goes, and check-inline-after now asserts that this fails. With classify_inline_comments false, the action posts inline comments immediately, so a run's empty reviews precede its summary (anthropics/claude-code-action@d535f51, action.yml:116-117). Only the default buffering posts them after the session ends, which the comment on that input now says. Mutation sweep: re-adding `.body != ""` fails the test at check-inline-after (exit 1). The earlier rows still fail (exit 1), and `head -n 1` now first fails at check-inline-after. Unmutated: 0. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/claude-review-test.sh | 6 +++++- .github/workflows/claude-review.yml | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/scripts/claude-review-test.sh b/.github/scripts/claude-review-test.sh index bca28ee..1f2724e 100644 --- a/.github/scripts/claude-review-test.sh +++ b/.github/scripts/claude-review-test.sh @@ -366,10 +366,14 @@ new_case check-incomplete review "$bot" 9 'Blocking: incomplete' | page reviews 1 assert_check check-incomplete fail -# An inline comment is a review with an empty body, so alone it is no verdict. +# An inline comment is a review with an empty body, so alone it is no verdict, +# and after a summary it is a finding of a run that stopped before its own. new_case check-inline-only review "$bot" 9 '' | page reviews 1 assert_check check-inline-only fail +new_case check-inline-after +{ review "$bot" 9 "$clean"; review "$bot" 9 '' 10; } | page reviews 1 +assert_check check-inline-after fail new_case check-other-user review someone 9 "$clean" | page reviews 1 diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index d566ded..f93ddf9 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -194,6 +194,8 @@ jobs: # The job's token, so neither the Claude GitHub App nor id-token. github_token: ${{ github.token }} # Buffering screens out probe comments, and only this prompt posts. + # It also posts them after the session ends (the action's action.yml), + # past the summary the verdict step needs as the newest review. classify_inline_comments: false prompt: | Review pull request #${{ github.event.pull_request.number }} of ${{ github.repository }}: base ${{ github.event.pull_request.base.sha }}, head ${{ github.event.pull_request.head.sha }}, checked out here. @@ -250,7 +252,7 @@ jobs: HEAD_SHA: ${{ github.event.pull_request.head.sha }} PR: ${{ github.event.pull_request.number }} VERDICT: >- - .[] | select(.user.login == "github-actions[bot]" and .commit_id == env.HEAD_SHA and .body != "") + .[] | select(.user.login == "github-actions[bot]" and .commit_id == env.HEAD_SHA) | .body | split("\n")[0] run: | set -euo pipefail From cf88a65e997bc47ff2e36c3a3306c3b9cad9ba7c Mon Sep 17 00:00:00 2001 From: Ivan Date: Wed, 11 Nov 2026 00:14:32 -0800 Subject: [PATCH 4/6] ci(claude-review): name what reaches the verdict step and what buffering delays From the re-review on #37 (non-blocking, Words): - check-older-head's comment said "crashed". A crash fails the action step, and the verdict step has no `if: always()`, so the case covers a review that ended without its summary. - The classify_inline_comments comment's "them" read as "probe comments". Buffering delays every inline comment. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/claude-review-test.sh | 4 ++-- .github/workflows/claude-review.yml | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/scripts/claude-review-test.sh b/.github/scripts/claude-review-test.sh index 1f2724e..7cb2411 100644 --- a/.github/scripts/claude-review-test.sh +++ b/.github/scripts/claude-review-test.sh @@ -379,8 +379,8 @@ new_case check-other-user review someone 9 "$clean" | page reviews 1 assert_check check-other-user fail -# A head whose own review crashed before its summary must not pass on the last -# head's. +# A head whose own review ended without its summary must not pass on the +# previous head's. new_case check-older-head review "$bot" 8 "$clean" | page reviews 1 assert_check check-older-head fail diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index f93ddf9..f27ab37 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -194,8 +194,9 @@ jobs: # The job's token, so neither the Claude GitHub App nor id-token. github_token: ${{ github.token }} # Buffering screens out probe comments, and only this prompt posts. - # It also posts them after the session ends (the action's action.yml), - # past the summary the verdict step needs as the newest review. + # It also posts every inline comment after the session ends (the + # action's action.yml), past the summary the verdict step needs as + # the newest review. classify_inline_comments: false prompt: | Review pull request #${{ github.event.pull_request.number }} of ${{ github.repository }}: base ${{ github.event.pull_request.base.sha }}, head ${{ github.event.pull_request.head.sha }}, checked out here. From a328a2913efd92cfe16aa1cba2a96b8fcd71d17f Mon Sep 17 00:00:00 2001 From: Ivan Date: Wed, 11 Nov 2026 00:22:09 -0800 Subject: [PATCH 5/6] ci(claude-review): buffering holds only an inline comment not sent with confirmed=true From the re-review on #37 (non-blocking, Words): the action buffers a comment only when classification is on and the call does not set confirmed=true (anthropics/claude-code-action@d535f51, action.yml:117), so "every inline comment" overstated what buffering delays. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/claude-review.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index f27ab37..7d332b7 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -194,9 +194,9 @@ jobs: # The job's token, so neither the Claude GitHub App nor id-token. github_token: ${{ github.token }} # Buffering screens out probe comments, and only this prompt posts. - # It also posts every inline comment after the session ends (the - # action's action.yml), past the summary the verdict step needs as - # the newest review. + # It also holds each inline comment not sent with confirmed=true until + # the session ends (the action's action.yml), past the summary the + # verdict step needs as the newest review. classify_inline_comments: false prompt: | Review pull request #${{ github.event.pull_request.number }} of ${{ github.repository }}: base ${{ github.event.pull_request.base.sha }}, head ${{ github.event.pull_request.head.sha }}, checked out here. From 9429cc380b32ab65b02c7b0b5fb4a04a44c7e783 Mon Sep 17 00:00:00 2001 From: Ivan Date: Wed, 11 Nov 2026 00:32:17 -0800 Subject: [PATCH 6/6] ci(claude-review): the test pins classify_inline_comments false, which VERDICT now needs From the re-review on #37 (non-blocking, Proportion): with `.body != ""` gone, the newest bot review at the head decides, so the setting that posts inline comments before the summary is load-bearing. The action defaults it to true (anthropics/claude-code-action@d535f51, action.yml), and deleting or flipping it would fail every review with an inline finding, org-wide, since review.yml runs this workflow from main. Mutation check: classify_inline_comments set to true, exit 1; the line deleted, exit 1. Unmutated: 0. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/claude-review-test.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/scripts/claude-review-test.sh b/.github/scripts/claude-review-test.sh index 7cb2411..deb7026 100644 --- a/.github/scripts/claude-review-test.sh +++ b/.github/scripts/claude-review-test.sh @@ -336,6 +336,8 @@ for agent in ("correctness-reviewer", "design-reviewer", "verifier"): # shellcheck disable=SC2016 # the backticks are the prompt's own, not expansions [[ "$prompt" == *'an incomplete one ends with a `Read:` line, then the `- :` line of each scope unit'* ]] || fail "prompt: step 3 no longer asks for the \`Read:\` format VERDICTS parses" +[[ "$(yq -r "$action | .with.classify_inline_comments" "$WORKFLOW_FILE")" == false ]] || + fail "classify_inline_comments: buffering would post inline reviews after the summary VERDICT needs as the newest" # The step that sets the required check, run over a case's reviews at head 9. # A crash fails the check too, so a fail counts only with the step's ::error::.