Skip to content

Commit a0fc644

Browse files
committed
test(review): assert the drift-warning predicate
1 parent 791e9a6 commit a0fc644

2 files changed

Lines changed: 42 additions & 14 deletions

File tree

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,9 @@ jobs:
7373
# Coupled to the reviewer's login: if that ever changes the count silently drops
7474
# to 0 and every round looks like the first, hence the warning below.
7575
CYCLE_JQ='[.[][] | select(.user.login == "claude[bot]") | .commit_id] | unique | length'
76+
# Only consulted when CYCLE is 0; see the warning below. Kept in its own variable
77+
# so tests/review-cycle-test.sh can assert it against the fixtures.
78+
DRIFT_JQ='any(.[][]; .user.type == "Bot")'
7679
# Never fail the review over the cycle number; degrade to 1, but say so. gh
7780
# writes its error body to stdout, so an unguarded pipe into jq aborts the step
7881
# under `bash -e` and skips the failure-notification step below.
@@ -85,7 +88,7 @@ jobs:
8588
echo "::warning::Could not parse prior reviews; treating this as review cycle 1."
8689
CYCLE=0
8790
elif [ "$CYCLE" -eq 0 ] && printf '%s' "$REVIEWS" \
88-
| jq -e -s 'any(.[][]; .user.type == "Bot")' >/dev/null 2>&1; then
91+
| jq -e -s "$DRIFT_JQ" >/dev/null 2>&1; then
8992
# claude[bot] is the only bot that submits reviews across the org (598 of 598
9093
# sampled), so bot reviews that the login filter did not count mean the
9194
# reviewer's identity moved and the counter has silently pinned at 1.

tests/review-cycle-test.sh

Lines changed: 38 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,24 +16,32 @@ cd "$(dirname "$0")/.."
1616

1717
WORKFLOW=.github/workflows/claude-pr-review.yml
1818

19-
JQ_PROG=$(sed -n "s/^ *CYCLE_JQ='\(.*\)'\$/\1/p" "$WORKFLOW")
20-
if [ -z "$JQ_PROG" ]; then
21-
echo "FAIL: no CYCLE_JQ='...' assignment found in $WORKFLOW"
22-
exit 1
23-
fi
24-
if [ "$(printf '%s\n' "$JQ_PROG" | wc -l)" -ne 1 ]; then
25-
echo "FAIL: more than one CYCLE_JQ assignment in $WORKFLOW:"
26-
printf '%s\n' "$JQ_PROG"
27-
exit 1
28-
fi
19+
# extract_jq <shell variable name> -- pull a single-quoted jq program out of the workflow
20+
extract_jq() {
21+
local name=$1 prog
22+
prog=$(sed -n "s/^ *$name='\(.*\)'\$/\1/p" "$WORKFLOW")
23+
if [ -z "$prog" ]; then
24+
echo "FAIL: no $name='...' assignment found in $WORKFLOW" >&2
25+
exit 1
26+
fi
27+
if [ "$(printf '%s\n' "$prog" | wc -l)" -ne 1 ]; then
28+
echo "FAIL: more than one $name assignment in $WORKFLOW:" >&2
29+
printf '%s\n' "$prog" >&2
30+
exit 1
31+
fi
32+
printf '%s' "$prog"
33+
}
34+
35+
CYCLE_JQ=$(extract_jq CYCLE_JQ)
36+
DRIFT_JQ=$(extract_jq DRIFT_JQ)
2937

3038
failures=0
3139

3240
# expect <fixture> <expected cycle> <description>
3341
expect() {
3442
local fixture=$1 want=$2 desc=$3
3543
local count actual
36-
count=$(jq -s "$JQ_PROG" "tests/fixtures/$fixture")
44+
count=$(jq -s "$CYCLE_JQ" "tests/fixtures/$fixture")
3745
actual=$((count + 1))
3846
if [ "$actual" -eq "$want" ]; then
3947
echo "ok $desc (cycle=$actual)"
@@ -43,6 +51,20 @@ expect() {
4351
fi
4452
}
4553

54+
# expect_drift <fixture> <fires|silent> <description>
55+
expect_drift() {
56+
local fixture=$1 want=$2 desc=$3 actual=silent
57+
if jq -e -s "$DRIFT_JQ" "tests/fixtures/$fixture" >/dev/null 2>&1; then
58+
actual=fires
59+
fi
60+
if [ "$actual" = "$want" ]; then
61+
echo "ok $desc ($actual)"
62+
else
63+
echo "FAIL $desc: expected $want, got $actual"
64+
failures=$((failures + 1))
65+
fi
66+
}
67+
4668
# No prior reviews.
4769
expect reviews-first-review.json 1 "opened PR is cycle 1"
4870

@@ -55,9 +77,12 @@ expect reviews-mixed-bots.json 3 "only claude[bot] rounds count, one per commit"
5577

5678
# The counter is keyed on the reviewer's login. If claude-code-action ever posts under a
5779
# different identity the count collapses to 0 and every round reads as cycle 1 again --
58-
# the original bug. Asserted so the coupling is explicit; no fixture can catch that in CI,
59-
# so the workflow also emits a warning annotation when it sees this shape at runtime.
80+
# the original bug. The drift predicate is the runtime backstop for that; it is only
81+
# consulted when the count is 0, so it has to separate "the login moved" from "genuine
82+
# first review".
6083
expect reviews-foreign-reviewer.json 1 "unknown reviewer login yields no rounds"
84+
expect_drift reviews-foreign-reviewer.json fires "drift warning fires when the login moved"
85+
expect_drift reviews-first-review.json silent "drift warning silent on a genuine cycle 1"
6186

6287
# gh 2.93 merges --paginate pages into one array; older versions concatenate one array per
6388
# page. `jq -s '.[][]'` must handle both, so keep a concatenated fixture.

0 commit comments

Comments
 (0)