From 380616ece335b2e3d73d159c9dd93d8e951167a5 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Wed, 29 Jul 2026 12:11:14 -0700 Subject: [PATCH 1/6] test(review): cover review cycle counter with captured API fixtures --- .github/workflows/tests.yml | 17 + tests/fixtures/reviews-approve-with-nits.json | 352 ++++++++++++++++++ tests/fixtures/reviews-first-review.json | 1 + tests/fixtures/reviews-mixed-bots.json | 44 +++ tests/review-cycle-test.sh | 58 +++ 5 files changed, 472 insertions(+) create mode 100644 .github/workflows/tests.yml create mode 100644 tests/fixtures/reviews-approve-with-nits.json create mode 100644 tests/fixtures/reviews-first-review.json create mode 100644 tests/fixtures/reviews-mixed-bots.json create mode 100755 tests/review-cycle-test.sh diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 0000000..934bcae --- /dev/null +++ b/.github/workflows/tests.yml @@ -0,0 +1,17 @@ +name: Tests + +on: + pull_request: + push: + branches: [main] + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6.0.2 + with: + fetch-depth: 1 + + - name: Review cycle counter + run: tests/review-cycle-test.sh diff --git a/tests/fixtures/reviews-approve-with-nits.json b/tests/fixtures/reviews-approve-with-nits.json new file mode 100644 index 0000000..48beb35 --- /dev/null +++ b/tests/fixtures/reviews-approve-with-nits.json @@ -0,0 +1,352 @@ +[ + { + "id": 4803070490, + "commit_id": "28027b1414cfd97666d91523815287373f2b7392", + "state": "COMMENTED", + "submitted_at": "2026-07-29T00:55:42Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803070659, + "commit_id": "28027b1414cfd97666d91523815287373f2b7392", + "state": "COMMENTED", + "submitted_at": "2026-07-29T00:55:45Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803071124, + "commit_id": "28027b1414cfd97666d91523815287373f2b7392", + "state": "COMMENTED", + "submitted_at": "2026-07-29T00:55:54Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803071286, + "commit_id": "28027b1414cfd97666d91523815287373f2b7392", + "state": "COMMENTED", + "submitted_at": "2026-07-29T00:55:57Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803071631, + "commit_id": "28027b1414cfd97666d91523815287373f2b7392", + "state": "COMMENTED", + "submitted_at": "2026-07-29T00:56:03Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803071837, + "commit_id": "28027b1414cfd97666d91523815287373f2b7392", + "state": "DISMISSED", + "submitted_at": "2026-07-29T00:56:07Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803172821, + "commit_id": "172678856137ab53d98d6f4f3ec64d3b487c51a5", + "state": "COMMENTED", + "submitted_at": "2026-07-29T01:21:16Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803173638, + "commit_id": "172678856137ab53d98d6f4f3ec64d3b487c51a5", + "state": "COMMENTED", + "submitted_at": "2026-07-29T01:21:23Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803174365, + "commit_id": "172678856137ab53d98d6f4f3ec64d3b487c51a5", + "state": "COMMENTED", + "submitted_at": "2026-07-29T01:21:31Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803174637, + "commit_id": "172678856137ab53d98d6f4f3ec64d3b487c51a5", + "state": "DISMISSED", + "submitted_at": "2026-07-29T01:21:35Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803287072, + "commit_id": "2a9f29992bc41238cc515fe3ded706e659ca63b0", + "state": "COMMENTED", + "submitted_at": "2026-07-29T01:46:43Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803287878, + "commit_id": "2a9f29992bc41238cc515fe3ded706e659ca63b0", + "state": "COMMENTED", + "submitted_at": "2026-07-29T01:46:53Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803288508, + "commit_id": "2a9f29992bc41238cc515fe3ded706e659ca63b0", + "state": "COMMENTED", + "submitted_at": "2026-07-29T01:47:02Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803289241, + "commit_id": "2a9f29992bc41238cc515fe3ded706e659ca63b0", + "state": "COMMENTED", + "submitted_at": "2026-07-29T01:47:11Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803289574, + "commit_id": "2a9f29992bc41238cc515fe3ded706e659ca63b0", + "state": "DISMISSED", + "submitted_at": "2026-07-29T01:47:15Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803335452, + "commit_id": "67dad03d0ee86f6d7c1cc06845b3b005166252d9", + "state": "COMMENTED", + "submitted_at": "2026-07-29T01:59:06Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803335626, + "commit_id": "67dad03d0ee86f6d7c1cc06845b3b005166252d9", + "state": "COMMENTED", + "submitted_at": "2026-07-29T01:59:09Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803335837, + "commit_id": "67dad03d0ee86f6d7c1cc06845b3b005166252d9", + "state": "DISMISSED", + "submitted_at": "2026-07-29T01:59:13Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803472086, + "commit_id": "c66e3592047e43f65db110b3c5cf959145eb928f", + "state": "COMMENTED", + "submitted_at": "2026-07-29T02:28:39Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803472879, + "commit_id": "c66e3592047e43f65db110b3c5cf959145eb928f", + "state": "COMMENTED", + "submitted_at": "2026-07-29T02:28:48Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803473129, + "commit_id": "c66e3592047e43f65db110b3c5cf959145eb928f", + "state": "COMMENTED", + "submitted_at": "2026-07-29T02:28:52Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4803473496, + "commit_id": "c66e3592047e43f65db110b3c5cf959145eb928f", + "state": "DISMISSED", + "submitted_at": "2026-07-29T02:28:58Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4810541861, + "commit_id": "ee53bdd8320e59270eba423d31d513dfae80a7c8", + "state": "COMMENTED", + "submitted_at": "2026-07-29T16:18:19Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4810543705, + "commit_id": "ee53bdd8320e59270eba423d31d513dfae80a7c8", + "state": "COMMENTED", + "submitted_at": "2026-07-29T16:18:30Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4810545119, + "commit_id": "ee53bdd8320e59270eba423d31d513dfae80a7c8", + "state": "DISMISSED", + "submitted_at": "2026-07-29T16:18:38Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4810648343, + "commit_id": "239056bc5f535a6c55dfe494d54102a5c5bb3b90", + "state": "COMMENTED", + "submitted_at": "2026-07-29T16:29:54Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4810650459, + "commit_id": "239056bc5f535a6c55dfe494d54102a5c5bb3b90", + "state": "COMMENTED", + "submitted_at": "2026-07-29T16:30:06Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4810651071, + "commit_id": "239056bc5f535a6c55dfe494d54102a5c5bb3b90", + "state": "DISMISSED", + "submitted_at": "2026-07-29T16:30:10Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4811705495, + "commit_id": "3f3ba8d1962bb2b650e0394525be88a676101748", + "state": "COMMENTED", + "submitted_at": "2026-07-29T18:32:04Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4811707279, + "commit_id": "3f3ba8d1962bb2b650e0394525be88a676101748", + "state": "COMMENTED", + "submitted_at": "2026-07-29T18:32:16Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4811708296, + "commit_id": "3f3ba8d1962bb2b650e0394525be88a676101748", + "state": "DISMISSED", + "submitted_at": "2026-07-29T18:32:23Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4811789242, + "commit_id": "fab04bd24df816278c5ac3aa38935f47ecac9b03", + "state": "COMMENTED", + "submitted_at": "2026-07-29T18:41:37Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4811790302, + "commit_id": "fab04bd24df816278c5ac3aa38935f47ecac9b03", + "state": "COMMENTED", + "submitted_at": "2026-07-29T18:41:45Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4811791546, + "commit_id": "fab04bd24df816278c5ac3aa38935f47ecac9b03", + "state": "COMMENTED", + "submitted_at": "2026-07-29T18:41:56Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4811792773, + "commit_id": "fab04bd24df816278c5ac3aa38935f47ecac9b03", + "state": "APPROVED", + "submitted_at": "2026-07-29T18:42:06Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + } +] diff --git a/tests/fixtures/reviews-first-review.json b/tests/fixtures/reviews-first-review.json new file mode 100644 index 0000000..fe51488 --- /dev/null +++ b/tests/fixtures/reviews-first-review.json @@ -0,0 +1 @@ +[] diff --git a/tests/fixtures/reviews-mixed-bots.json b/tests/fixtures/reviews-mixed-bots.json new file mode 100644 index 0000000..980f83b --- /dev/null +++ b/tests/fixtures/reviews-mixed-bots.json @@ -0,0 +1,44 @@ +[ + { + "id": 1, + "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "state": "COMMENTED", + "submitted_at": "2026-07-01T00:00:00Z", + "user": { "login": "claude[bot]", "type": "Bot" } + }, + { + "id": 2, + "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "state": "CHANGES_REQUESTED", + "submitted_at": "2026-07-01T00:00:05Z", + "user": { "login": "claude[bot]", "type": "Bot" } + }, + { + "id": 3, + "commit_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "state": "DISMISSED", + "submitted_at": "2026-07-01T01:00:00Z", + "user": { "login": "claude[bot]", "type": "Bot" } + }, + { + "id": 4, + "commit_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "state": "APPROVED", + "submitted_at": "2026-07-01T01:05:00Z", + "user": { "login": "hotdata-automation[bot]", "type": "Bot" } + }, + { + "id": 5, + "commit_id": "cccccccccccccccccccccccccccccccccccccccc", + "state": "APPROVED", + "submitted_at": "2026-07-01T02:00:00Z", + "user": { "login": "aikido-autofix[bot]", "type": "Bot" } + }, + { + "id": 6, + "commit_id": "dddddddddddddddddddddddddddddddddddddddd", + "state": "APPROVED", + "submitted_at": "2026-07-01T03:00:00Z", + "user": { "login": "zfarrell", "type": "User" } + } +] diff --git a/tests/review-cycle-test.sh b/tests/review-cycle-test.sh new file mode 100755 index 0000000..356b97b --- /dev/null +++ b/tests/review-cycle-test.sh @@ -0,0 +1,58 @@ +#!/usr/bin/env bash +# +# Guards the REVIEW CYCLE counter in claude-pr-review.yml against fixtures captured from +# the real reviews API. The jq program is extracted from the workflow rather than copied, +# so the test exercises the shipped expression. +# +# Regression: a counter that filtered on review state read 1 on every round, because +# dismiss_stale_reviews_on_push flips each prior APPROVED to DISMISSED. That silently +# disabled the prompt's cycle-awareness ladder on every approve-with-nits PR. + +set -euo pipefail + +cd "$(dirname "$0")/.." + +WORKFLOW=.github/workflows/claude-pr-review.yml + +JQ_PROG=$(sed -n "s/.*| jq -s '\(.*\)')\$/\1/p" "$WORKFLOW") +if [ -z "$JQ_PROG" ]; then + echo "FAIL: could not extract the review-cycle jq program from $WORKFLOW" + exit 1 +fi +if [ "$(printf '%s\n' "$JQ_PROG" | wc -l)" -ne 1 ]; then + echo "FAIL: extracted more than one jq program from $WORKFLOW:" + printf '%s\n' "$JQ_PROG" + exit 1 +fi + +failures=0 + +# expect +expect() { + local fixture=$1 want=$2 desc=$3 + local count actual + count=$(jq -s "$JQ_PROG" "tests/fixtures/$fixture") + actual=$((count + 1)) + if [ "$actual" -eq "$want" ]; then + echo "ok $desc (cycle=$actual)" + else + echo "FAIL $desc: expected cycle $want, got $actual" + failures=$((failures + 1)) + fi +} + +# No prior reviews. +expect reviews-first-review.json 1 "opened PR is cycle 1" + +# monopoly#1560: nine review rounds, every one an approve-with-nits that the next push +# dismissed. The tenth review must know it is the tenth. +expect reviews-approve-with-nits.json 10 "nine dismissed approve-with-nits rounds count" + +# Rounds counted regardless of state; approvals by other bots and by humans do not count. +expect reviews-mixed-bots.json 3 "only claude[bot] rounds count, one per commit" + +if [ "$failures" -ne 0 ]; then + echo "$failures test(s) failed" + exit 1 +fi +echo "all tests passed" From b7d93e8bb2d12071e178eaa06d4f2cbe7dc177d0 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Wed, 29 Jul 2026 12:11:14 -0700 Subject: [PATCH 2/6] fix(review): count review rounds by commit, not review state --- .github/workflows/claude-pr-review.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 11c4de7..4ab7844 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -65,8 +65,12 @@ jobs: PR_NUMBER=${{ github.event.pull_request.number }} REPO=${{ github.repository }} + # Count distinct commits already reviewed, never review state: the org ruleset + # sets dismiss_stale_reviews_on_push, so a push flips every prior APPROVED to + # DISMISSED. Inline comments each create their own COMMENTED review sharing the + # round's commit_id, so unique commit_id == round count. CYCLE=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews" \ - --paginate | jq -s '[.[][] | select(.user.type == "Bot" and (.state == "CHANGES_REQUESTED" or .state == "APPROVED"))] | length') + --paginate | jq -s '[.[][] | select(.user.login == "claude[bot]") | .commit_id] | unique | length') echo "review_cycle=$((CYCLE + 1))" >> $GITHUB_OUTPUT THREADS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ From 55c52d244c1c50127cfe912acf7f772dc7515ce3 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Wed, 29 Jul 2026 12:32:02 -0700 Subject: [PATCH 3/6] fix(review): keep cycle counter from failing the review step --- .github/workflows/claude-pr-review.yml | 24 +- .github/workflows/tests.yml | 3 + tests/fixtures/reviews-foreign-reviewer.json | 62 ++++++ tests/fixtures/reviews-paginated.json | 2 + tests/fixtures/reviews-straddled-round.json | 222 +++++++++++++++++++ tests/review-cycle-test.sh | 30 ++- 6 files changed, 332 insertions(+), 11 deletions(-) create mode 100644 tests/fixtures/reviews-foreign-reviewer.json create mode 100644 tests/fixtures/reviews-paginated.json create mode 100644 tests/fixtures/reviews-straddled-round.json diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 4ab7844..7194d60 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -66,11 +66,25 @@ jobs: REPO=${{ github.repository }} # Count distinct commits already reviewed, never review state: the org ruleset - # sets dismiss_stale_reviews_on_push, so a push flips every prior APPROVED to - # DISMISSED. Inline comments each create their own COMMENTED review sharing the - # round's commit_id, so unique commit_id == round count. - CYCLE=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews" \ - --paginate | jq -s '[.[][] | select(.user.login == "claude[bot]") | .commit_id] | unique | length') + # sets dismiss_stale_reviews_on_push, so a push flips a prior APPROVED to + # DISMISSED and a state filter stops matching it. Inline comments each create + # their own COMMENTED review sharing the round's commit_id, so unique commit_id + # == round count, +/-1 when a push lands mid-round and splits it across two SHAs. + # Coupled to the reviewer's login: if that ever changes the count silently drops + # to 0 and every round looks like the first, hence the warning below. + CYCLE_JQ='[.[][] | select(.user.login == "claude[bot]") | .commit_id] | unique | length' + # Never fail the review over the cycle number; degrade to 1, but say so. gh + # writes its error body to stdout, so an unguarded pipe into jq aborts the step + # under `bash -e` and skips the failure-notification step below. + if ! REVIEWS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews" --paginate); then + echo "::warning::Could not read prior reviews; treating this as review cycle 1." + REVIEWS='' + fi + CYCLE=$(printf '%s' "$REVIEWS" | jq -s "$CYCLE_JQ" 2>/dev/null) || CYCLE=0 + if [ "$CYCLE" -eq 0 ] && printf '%s' "$REVIEWS" \ + | jq -e -s 'any(.[][]; .user.type == "Bot")' >/dev/null 2>&1; then + echo "::warning::Bot reviews exist but none matched the reviewer login; the review cycle counter is stale." + fi echo "review_cycle=$((CYCLE + 1))" >> $GITHUB_OUTPUT THREADS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 934bcae..c77e53d 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -5,6 +5,9 @@ on: push: branches: [main] +permissions: + contents: read + jobs: test: runs-on: ubuntu-latest diff --git a/tests/fixtures/reviews-foreign-reviewer.json b/tests/fixtures/reviews-foreign-reviewer.json new file mode 100644 index 0000000..2e7b381 --- /dev/null +++ b/tests/fixtures/reviews-foreign-reviewer.json @@ -0,0 +1,62 @@ +[ + { + "id": 1, + "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "state": "COMMENTED", + "submitted_at": "2026-07-01T00:00:00Z", + "user": { + "login": "hotdata-automation[bot]", + "type": "Bot" + } + }, + { + "id": 2, + "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "state": "CHANGES_REQUESTED", + "submitted_at": "2026-07-01T00:00:05Z", + "user": { + "login": "hotdata-automation[bot]", + "type": "Bot" + } + }, + { + "id": 3, + "commit_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "state": "DISMISSED", + "submitted_at": "2026-07-01T01:00:00Z", + "user": { + "login": "hotdata-automation[bot]", + "type": "Bot" + } + }, + { + "id": 4, + "commit_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "state": "APPROVED", + "submitted_at": "2026-07-01T01:05:00Z", + "user": { + "login": "hotdata-automation[bot]", + "type": "Bot" + } + }, + { + "id": 5, + "commit_id": "cccccccccccccccccccccccccccccccccccccccc", + "state": "APPROVED", + "submitted_at": "2026-07-01T02:00:00Z", + "user": { + "login": "hotdata-automation[bot]", + "type": "Bot" + } + }, + { + "id": 6, + "commit_id": "dddddddddddddddddddddddddddddddddddddddd", + "state": "APPROVED", + "submitted_at": "2026-07-01T03:00:00Z", + "user": { + "login": "hotdata-automation[bot]", + "type": "Bot" + } + } +] diff --git a/tests/fixtures/reviews-paginated.json b/tests/fixtures/reviews-paginated.json new file mode 100644 index 0000000..f7a482a --- /dev/null +++ b/tests/fixtures/reviews-paginated.json @@ -0,0 +1,2 @@ +[{"id":1,"commit_id":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","state":"COMMENTED","submitted_at":"2026-07-01T00:00:00Z","user":{"login":"claude[bot]","type":"Bot"}},{"id":2,"commit_id":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","state":"CHANGES_REQUESTED","submitted_at":"2026-07-01T00:00:05Z","user":{"login":"claude[bot]","type":"Bot"}},{"id":3,"commit_id":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb","state":"DISMISSED","submitted_at":"2026-07-01T01:00:00Z","user":{"login":"claude[bot]","type":"Bot"}}] +[{"id":4,"commit_id":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb","state":"APPROVED","submitted_at":"2026-07-01T01:05:00Z","user":{"login":"hotdata-automation[bot]","type":"Bot"}},{"id":5,"commit_id":"cccccccccccccccccccccccccccccccccccccccc","state":"APPROVED","submitted_at":"2026-07-01T02:00:00Z","user":{"login":"aikido-autofix[bot]","type":"Bot"}},{"id":6,"commit_id":"dddddddddddddddddddddddddddddddddddddddd","state":"APPROVED","submitted_at":"2026-07-01T03:00:00Z","user":{"login":"zfarrell","type":"User"}}] diff --git a/tests/fixtures/reviews-straddled-round.json b/tests/fixtures/reviews-straddled-round.json new file mode 100644 index 0000000..fc75dcc --- /dev/null +++ b/tests/fixtures/reviews-straddled-round.json @@ -0,0 +1,222 @@ +[ + { + "id": 4790491756, + "commit_id": "a89f19e81690a9091b320594f0f5a2dd1a79ed4b", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:00:03Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790492307, + "commit_id": "a89f19e81690a9091b320594f0f5a2dd1a79ed4b", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:00:08Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790493522, + "commit_id": "a89f19e81690a9091b320594f0f5a2dd1a79ed4b", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:00:17Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790494429, + "commit_id": "a89f19e81690a9091b320594f0f5a2dd1a79ed4b", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:00:25Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790495081, + "commit_id": "a89f19e81690a9091b320594f0f5a2dd1a79ed4b", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:00:30Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790495676, + "commit_id": "951c8afaddf5579bf8496df9d0c6ebd5e186d640", + "state": "APPROVED", + "submitted_at": "2026-07-27T19:00:35Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790560226, + "commit_id": "951c8afaddf5579bf8496df9d0c6ebd5e186d640", + "state": "DISMISSED", + "submitted_at": "2026-07-27T19:07:06Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790604991, + "commit_id": "d0f6dcab75199784b136cf89be2680980777efd7", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:11:49Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790606647, + "commit_id": "d0f6dcab75199784b136cf89be2680980777efd7", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:11:59Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790607676, + "commit_id": "d0f6dcab75199784b136cf89be2680980777efd7", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:12:05Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790608309, + "commit_id": "d0f6dcab75199784b136cf89be2680980777efd7", + "state": "DISMISSED", + "submitted_at": "2026-07-27T19:12:08Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790725357, + "commit_id": "8099fef1793b2ac7531643cdeffc16b9e18d6eb1", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:24:14Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790726786, + "commit_id": "8099fef1793b2ac7531643cdeffc16b9e18d6eb1", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:24:25Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790767497, + "commit_id": "8099fef1793b2ac7531643cdeffc16b9e18d6eb1", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:29:36Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790768432, + "commit_id": "8099fef1793b2ac7531643cdeffc16b9e18d6eb1", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:29:44Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790769530, + "commit_id": "8099fef1793b2ac7531643cdeffc16b9e18d6eb1", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:29:53Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790770077, + "commit_id": "8099fef1793b2ac7531643cdeffc16b9e18d6eb1", + "state": "DISMISSED", + "submitted_at": "2026-07-27T19:29:58Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790978887, + "commit_id": "703e6f0f6139631dbab17fee47034fe05318cd35", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:56:17Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790980555, + "commit_id": "703e6f0f6139631dbab17fee47034fe05318cd35", + "state": "COMMENTED", + "submitted_at": "2026-07-27T19:56:31Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4790981470, + "commit_id": "703e6f0f6139631dbab17fee47034fe05318cd35", + "state": "DISMISSED", + "submitted_at": "2026-07-27T19:56:38Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4791181568, + "commit_id": "ffe5a7d50cccac18957cb592f4330d27473d10fb", + "state": "COMMENTED", + "submitted_at": "2026-07-27T20:22:28Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + }, + { + "id": 4791181903, + "commit_id": "ffe5a7d50cccac18957cb592f4330d27473d10fb", + "state": "APPROVED", + "submitted_at": "2026-07-27T20:22:32Z", + "user": { + "login": "claude[bot]", + "type": "Bot" + } + } +] diff --git a/tests/review-cycle-test.sh b/tests/review-cycle-test.sh index 356b97b..f626719 100755 --- a/tests/review-cycle-test.sh +++ b/tests/review-cycle-test.sh @@ -4,9 +4,11 @@ # the real reviews API. The jq program is extracted from the workflow rather than copied, # so the test exercises the shipped expression. # -# Regression: a counter that filtered on review state read 1 on every round, because -# dismiss_stale_reviews_on_push flips each prior APPROVED to DISMISSED. That silently -# disabled the prompt's cycle-awareness ladder on every approve-with-nits PR. +# Regression: the counter used to filter on review state (CHANGES_REQUESTED/APPROVED). On +# a PR whose every round ends in approve-with-nits, dismiss_stale_reviews_on_push flips +# each of those approvals to DISMISSED, so the filter matched nothing and the counter read +# 1 forever, silently disabling the prompt's cycle-awareness ladder. CHANGES_REQUESTED +# survives a push, so request-changes PRs counted correctly and the bug stayed hidden. set -euo pipefail @@ -14,13 +16,13 @@ cd "$(dirname "$0")/.." WORKFLOW=.github/workflows/claude-pr-review.yml -JQ_PROG=$(sed -n "s/.*| jq -s '\(.*\)')\$/\1/p" "$WORKFLOW") +JQ_PROG=$(sed -n "s/^ *CYCLE_JQ='\(.*\)'\$/\1/p" "$WORKFLOW") if [ -z "$JQ_PROG" ]; then - echo "FAIL: could not extract the review-cycle jq program from $WORKFLOW" + echo "FAIL: no CYCLE_JQ='...' assignment found in $WORKFLOW" exit 1 fi if [ "$(printf '%s\n' "$JQ_PROG" | wc -l)" -ne 1 ]; then - echo "FAIL: extracted more than one jq program from $WORKFLOW:" + echo "FAIL: more than one CYCLE_JQ assignment in $WORKFLOW:" printf '%s\n' "$JQ_PROG" exit 1 fi @@ -51,6 +53,22 @@ expect reviews-approve-with-nits.json 10 "nine dismissed approve-with-nits round # Rounds counted regardless of state; approvals by other bots and by humans do not count. expect reviews-mixed-bots.json 3 "only claude[bot] rounds count, one per commit" +# The counter is keyed on the reviewer's login. If claude-code-action ever posts under a +# different identity the count collapses to 0 and every round reads as cycle 1 again -- +# the original bug. Asserted so the coupling is explicit; no fixture can catch that in CI, +# so the workflow also emits a warning annotation when it sees this shape at runtime. +expect reviews-foreign-reviewer.json 1 "unknown reviewer login yields no rounds" + +# gh 2.93 merges --paginate pages into one array; older versions concatenate one array per +# page. `jq -s '.[][]'` must handle both, so keep a concatenated fixture. +expect reviews-paginated.json 3 "concatenated --paginate pages count once each" + +# Known +/-1: monopoly#1534 round 1 posted 5 inline comments against a89f19e8, then its +# approve landed 5s later against 951c8afa because a push arrived mid-review, so 5 real +# rounds count as 6. Harmless against a 5-step ladder; asserted so a future change to the +# counter has to acknowledge this case rather than shift it silently. +expect reviews-straddled-round.json 7 "push landing mid-round splits it in two" + if [ "$failures" -ne 0 ]; then echo "$failures test(s) failed" exit 1 From 02b2ac24bcc03d341585618eac75e104c0ff353b Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Wed, 29 Jul 2026 12:37:29 -0700 Subject: [PATCH 4/6] fix(review): guard prior-comment fetch and scope the drift warning --- .github/workflows/claude-pr-review.yml | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 7194d60..a13ab3e 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -81,15 +81,25 @@ jobs: REVIEWS='' fi CYCLE=$(printf '%s' "$REVIEWS" | jq -s "$CYCLE_JQ" 2>/dev/null) || CYCLE=0 + # Zero rounds plus an *unrecognized* bot reviewer is the login-drift shape. The + # bots below legitimately review ahead of the first Claude round, so counting + # them here would warn on every ordinary cycle 1. if [ "$CYCLE" -eq 0 ] && printf '%s' "$REVIEWS" \ - | jq -e -s 'any(.[][]; .user.type == "Bot")' >/dev/null 2>&1; then - echo "::warning::Bot reviews exist but none matched the reviewer login; the review cycle counter is stale." + | jq -e -s --argjson known '["hotdata-automation[bot]", "aikido-autofix[bot]", "dependabot[bot]"]' \ + 'any(.[][]; .user.type == "Bot" and (.user.login | IN($known[]) | not))' >/dev/null 2>&1; then + echo "::warning::Unrecognized bot reviewer and no counted rounds; the review cycle counter may be stale." fi echo "review_cycle=$((CYCLE + 1))" >> $GITHUB_OUTPUT - THREADS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" \ - --paginate | jq -s -r ' - add | sort_by(.created_at) | + # Same guard as the counter above: unguarded `gh api | jq` aborts the step, and a + # failure here *skips* the review step, so the notify step's failure check never + # fires and the PR gets no review and no explanation. + if ! COMMENTS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" --paginate); then + echo "::warning::Could not read prior review comments; reviewing without them." + COMMENTS='' + fi + THREADS=$(printf '%s' "$COMMENTS" | jq -s -r ' + (add // []) | sort_by(.created_at) | if length == 0 then "No prior review comments." else .[] | "---", @@ -100,7 +110,7 @@ jobs: "", .body end - ') + ') || THREADS='No prior review comments.' DELIMITER="REVIEW_CONTEXT_$(openssl rand -hex 16)" { From 791e9a6d63633ce18552f365e5345231246e8837 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Wed, 29 Jul 2026 12:44:04 -0700 Subject: [PATCH 5/6] refactor(review): simplify drift warning; warn on unparsable reviews --- .github/workflows/claude-pr-review.yml | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index a13ab3e..5fe395e 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -80,14 +80,16 @@ jobs: echo "::warning::Could not read prior reviews; treating this as review cycle 1." REVIEWS='' fi - CYCLE=$(printf '%s' "$REVIEWS" | jq -s "$CYCLE_JQ" 2>/dev/null) || CYCLE=0 - # Zero rounds plus an *unrecognized* bot reviewer is the login-drift shape. The - # bots below legitimately review ahead of the first Claude round, so counting - # them here would warn on every ordinary cycle 1. - if [ "$CYCLE" -eq 0 ] && printf '%s' "$REVIEWS" \ - | jq -e -s --argjson known '["hotdata-automation[bot]", "aikido-autofix[bot]", "dependabot[bot]"]' \ - 'any(.[][]; .user.type == "Bot" and (.user.login | IN($known[]) | not))' >/dev/null 2>&1; then - echo "::warning::Unrecognized bot reviewer and no counted rounds; the review cycle counter may be stale." + CYCLE=$(printf '%s' "$REVIEWS" | jq -s "$CYCLE_JQ" 2>/dev/null) || CYCLE='' + if [ -z "$CYCLE" ]; then + echo "::warning::Could not parse prior reviews; treating this as review cycle 1." + CYCLE=0 + elif [ "$CYCLE" -eq 0 ] && printf '%s' "$REVIEWS" \ + | jq -e -s 'any(.[][]; .user.type == "Bot")' >/dev/null 2>&1; then + # claude[bot] is the only bot that submits reviews across the org (598 of 598 + # sampled), so bot reviews that the login filter did not count mean the + # reviewer's identity moved and the counter has silently pinned at 1. + echo "::warning::Bot reviews exist but none matched the reviewer login; the review cycle counter is stale." fi echo "review_cycle=$((CYCLE + 1))" >> $GITHUB_OUTPUT From a0fc644cdeed2e93fe40a60958632d62377ca9f1 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Wed, 29 Jul 2026 13:07:06 -0700 Subject: [PATCH 6/6] test(review): assert the drift-warning predicate --- .github/workflows/claude-pr-review.yml | 5 ++- tests/review-cycle-test.sh | 51 +++++++++++++++++++------- 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 5fe395e..001fdc9 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -73,6 +73,9 @@ jobs: # Coupled to the reviewer's login: if that ever changes the count silently drops # to 0 and every round looks like the first, hence the warning below. CYCLE_JQ='[.[][] | select(.user.login == "claude[bot]") | .commit_id] | unique | length' + # Only consulted when CYCLE is 0; see the warning below. Kept in its own variable + # so tests/review-cycle-test.sh can assert it against the fixtures. + DRIFT_JQ='any(.[][]; .user.type == "Bot")' # Never fail the review over the cycle number; degrade to 1, but say so. gh # writes its error body to stdout, so an unguarded pipe into jq aborts the step # under `bash -e` and skips the failure-notification step below. @@ -85,7 +88,7 @@ jobs: echo "::warning::Could not parse prior reviews; treating this as review cycle 1." CYCLE=0 elif [ "$CYCLE" -eq 0 ] && printf '%s' "$REVIEWS" \ - | jq -e -s 'any(.[][]; .user.type == "Bot")' >/dev/null 2>&1; then + | jq -e -s "$DRIFT_JQ" >/dev/null 2>&1; then # claude[bot] is the only bot that submits reviews across the org (598 of 598 # sampled), so bot reviews that the login filter did not count mean the # reviewer's identity moved and the counter has silently pinned at 1. diff --git a/tests/review-cycle-test.sh b/tests/review-cycle-test.sh index f626719..5985362 100755 --- a/tests/review-cycle-test.sh +++ b/tests/review-cycle-test.sh @@ -16,16 +16,24 @@ cd "$(dirname "$0")/.." WORKFLOW=.github/workflows/claude-pr-review.yml -JQ_PROG=$(sed -n "s/^ *CYCLE_JQ='\(.*\)'\$/\1/p" "$WORKFLOW") -if [ -z "$JQ_PROG" ]; then - echo "FAIL: no CYCLE_JQ='...' assignment found in $WORKFLOW" - exit 1 -fi -if [ "$(printf '%s\n' "$JQ_PROG" | wc -l)" -ne 1 ]; then - echo "FAIL: more than one CYCLE_JQ assignment in $WORKFLOW:" - printf '%s\n' "$JQ_PROG" - exit 1 -fi +# extract_jq -- pull a single-quoted jq program out of the workflow +extract_jq() { + local name=$1 prog + prog=$(sed -n "s/^ *$name='\(.*\)'\$/\1/p" "$WORKFLOW") + if [ -z "$prog" ]; then + echo "FAIL: no $name='...' assignment found in $WORKFLOW" >&2 + exit 1 + fi + if [ "$(printf '%s\n' "$prog" | wc -l)" -ne 1 ]; then + echo "FAIL: more than one $name assignment in $WORKFLOW:" >&2 + printf '%s\n' "$prog" >&2 + exit 1 + fi + printf '%s' "$prog" +} + +CYCLE_JQ=$(extract_jq CYCLE_JQ) +DRIFT_JQ=$(extract_jq DRIFT_JQ) failures=0 @@ -33,7 +41,7 @@ failures=0 expect() { local fixture=$1 want=$2 desc=$3 local count actual - count=$(jq -s "$JQ_PROG" "tests/fixtures/$fixture") + count=$(jq -s "$CYCLE_JQ" "tests/fixtures/$fixture") actual=$((count + 1)) if [ "$actual" -eq "$want" ]; then echo "ok $desc (cycle=$actual)" @@ -43,6 +51,20 @@ expect() { fi } +# expect_drift +expect_drift() { + local fixture=$1 want=$2 desc=$3 actual=silent + if jq -e -s "$DRIFT_JQ" "tests/fixtures/$fixture" >/dev/null 2>&1; then + actual=fires + fi + if [ "$actual" = "$want" ]; then + echo "ok $desc ($actual)" + else + echo "FAIL $desc: expected $want, got $actual" + failures=$((failures + 1)) + fi +} + # No prior reviews. expect reviews-first-review.json 1 "opened PR is cycle 1" @@ -55,9 +77,12 @@ expect reviews-mixed-bots.json 3 "only claude[bot] rounds count, one per commit" # The counter is keyed on the reviewer's login. If claude-code-action ever posts under a # different identity the count collapses to 0 and every round reads as cycle 1 again -- -# the original bug. Asserted so the coupling is explicit; no fixture can catch that in CI, -# so the workflow also emits a warning annotation when it sees this shape at runtime. +# the original bug. The drift predicate is the runtime backstop for that; it is only +# consulted when the count is 0, so it has to separate "the login moved" from "genuine +# first review". expect reviews-foreign-reviewer.json 1 "unknown reviewer login yields no rounds" +expect_drift reviews-foreign-reviewer.json fires "drift warning fires when the login moved" +expect_drift reviews-first-review.json silent "drift warning silent on a genuine cycle 1" # gh 2.93 merges --paginate pages into one array; older versions concatenate one array per # page. `jq -s '.[][]'` must handle both, so keep a concatenated fixture.