From e3e6c08ecdbd2f7c6ad22d1c94da46fc06311e8b Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 28 Aug 2026 09:32:13 -0700 Subject: [PATCH 1/3] feat(review): exclude the parallel reviewer from the prompt --- README.md | 27 ++++++++ scripts/gather-review-context.sh | 65 +++++++++++++------ .../fixtures/pull-comments-mixed-authors.json | 35 ++++++++++ tests/fixtures/reviews-other-review-bot.json | 22 +++++++ tests/lib.sh | 9 +++ tests/pr-context-test.sh | 62 +++++++++++++++++- tests/review-cycle-test.sh | 14 +++- 7 files changed, 211 insertions(+), 23 deletions(-) create mode 100644 tests/fixtures/pull-comments-mixed-authors.json create mode 100644 tests/fixtures/reviews-other-review-bot.json diff --git a/README.md b/README.md index 7df58e8..a995213 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,33 @@ block delimiters are neutralised by shape rather than by exact string: `` and `< / pr_context foo="1">` all read as the same delimiter to a model, and any of them would otherwise end the data block early and land the rest where it reads as instructions. +### A second reviewer in parallel + +A comparison trial runs [Pullfrog](https://pullfrog.com) on OpenAI beside this reviewer, so its +output is excluded from the two blocks that would otherwise carry it: `/pulls/{n}/comments` +becomes `` and `/issues/{n}/comments` becomes the PR conversation, and +neither read filters by author. Both reviewers fire on `opened`, so the exclusion is what keeps +the two arms of the comparison independent — whichever reviewer posts first would otherwise set +what the other reads as settled prior feedback, not to be re-raised. It also protects the budget +this reviewer already competes for: comment threads may take half of it, and a Pullfrog review +body is a PR summary plus its findings. + +The login lives in one constant, `OTHER_REVIEW_BOT`, and reaches three programs as a jq `--arg`. +The third is the review-cycle drift predicate, which asks whether a bot review exists that the +`claude[bot]` filter did not count — true on the first review of every PR in a trial repo, so +without the exclusion it would report a reviewer-identity change that has not happened, on every +pull request, which is how the warning goes unread on the one where it is real. + +Where the exclusion empties a block it says so and how much it withheld, because +"No prior review comments." on a PR that has some is the same false claim as an empty CI block +reading as a green one. Where other comments remain it stays silent: the block claims nothing +about being every comment on the PR. + +Pullfrog's own review verdict is a check, `pullfrog-approval`, reporting whether it *would* +approve. It is not required in the org ruleset and Pullfrog cannot submit an approving review, so +this reviewer remains the only automated approval in the org for the duration of the trial. The +constant, and this section, come out when the trial ends. + ### Tool usage artifact Each run attaches a `claude-tool-usage-pr-` artifact (14-day retention): tool call counts, diff --git a/scripts/gather-review-context.sh b/scripts/gather-review-context.sh index 513ac42..f2b8659 100755 --- a/scripts/gather-review-context.sh +++ b/scripts/gather-review-context.sh @@ -298,9 +298,31 @@ fetch_raw() { # 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' +# The login of the second automated reviewer running beside this one, and the one value +# three programs below have to agree on. A comparison trial has Pullfrog reviewing the same +# pull requests, and both reviewers fire on `opened`, so its output reaches this reviewer +# through two reads that filter by nothing: `/pulls/{n}/comments` becomes +# and `/issues/{n}/comments` becomes the PR conversation. Left in, +# it costs the trial its independence -- whichever reviewer posts first sets what the other +# reads as settled prior feedback -- and it costs the diff blocks the bytes, since comment +# threads may take half the escaped-byte budget and a cut context measured 0.92 findings a +# run against 1.91 uncut. So the exclusion is not tidiness; it is what keeps the two arms +# of the comparison, and the reviewer's own budget, intact. +# +# Handed to jq with --arg at each site rather than written into each program, because a +# second copy of a constant is a copy free to drift from the first. tests/lib.sh reads this +# assignment for the same reason it extracts the jq programs. +# +# It comes out when the trial ends. Nothing else in the org posts reviews. +OTHER_REVIEW_BOT='pullfrog[bot]' # 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")' +# +# $skip is excluded from the predicate, not merely from the count: the trial makes a bot +# review on a genuine cycle 1 the ordinary case, so without it every first review in a +# trial repo would report a reviewer-identity drift that has not happened -- and an alarm +# that fires on every PR is an alarm nobody reads on the PR where it is real. +DRIFT_JQ='any(.[][]; .user.type == "Bot" and .user.login != $skip)' # 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. @@ -326,9 +348,10 @@ 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 "$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 + | jq --arg skip "$OTHER_REVIEW_BOT" -e -s "$DRIFT_JQ" >/dev/null 2>&1; then + # claude[bot] was the only bot submitting reviews across the org when this was + # written (598 of 598 sampled), and $OTHER_REVIEW_BOT is the stated exception, + # so any *other* bot review that the login filter did not count means 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 @@ -352,22 +375,22 @@ fi # "No prior review comments." is only true when the fetch worked and returned # none. Saying it after a failed fetch is the same false claim as an empty CI block # reading as a green one, and it is the claim the cycle ladder acts on. +# +# $skip's comments are dropped here -- see OTHER_REVIEW_BOT above for why. The drop is +# announced only when it empties the block, and that asymmetry is the point: with other +# comments still present the block claims nothing about being every comment on the PR, but +# "No prior review comments." on a PR that has some is the same false claim as an empty CI +# block reading as a green one. So the empty case names what was withheld and how much. +# +# Single-line and single-quoted so tests/lib.sh can extract it; it was inline, and inline +# meant the one program in this script that shapes the prompt's other output could not be +# asserted against a fixture at all. +THREADS_JQ='(add // []) as $all | ($all | map(select(.user.login != $skip))) as $kept | (($all | length) - ($kept | length)) as $dropped | if ($kept | length) == 0 then (if $dropped > 0 then "No prior review comments. (\($dropped) comment(s) from \($skip) are excluded from this block.)" else "No prior review comments." end) else ($kept | sort_by(.created_at) | .[] | "---", "Author: \(.user.login)", "File: \(.path)", (if .line then "Line: \(.line)" else empty end), (if .in_reply_to_id then "Reply to #\(.in_reply_to_id)" else "Thread #\(.id)" end), "", ((.body // "")[0:3000])) end' if [ "$COMMENTS_OK" -eq 0 ]; then THREADS='Unavailable: the prior inline review comments could not be read. This block is empty because the fetch failed, not because there were none.' else -THREADS=$(printf '%s' "$COMMENTS" | jq -s -r ' - (add // []) | sort_by(.created_at) | - if length == 0 then "No prior review comments." - else .[] | - "---", - "Author: \(.user.login)", - "File: \(.path)", - (if .line then "Line: \(.line)" else empty end), - (if .in_reply_to_id then "Reply to #\(.in_reply_to_id)" else "Thread #\(.id)" end), - "", - ((.body // "")[0:3000]) - end -') || THREADS='Unavailable: the prior inline review comments could not be parsed.' + THREADS=$(printf '%s' "$COMMENTS" | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$THREADS_JQ") \ + || THREADS='Unavailable: the prior inline review comments could not be parsed.' fi # The prompt wraps both blocks below in and @@ -706,9 +729,13 @@ if [ "$FULL_DIFF_MAX" -gt "$DIFF_MAX" ]; then FULL_DIFF_MAX=$DIFF_MAX; fi # prompt on www.hotdata.dev#332. Stripped before the cap for the reason strip_block_tags # always runs first -- the substitution grows the text, so a cap on the unstripped file # bounds a smaller string than the one emitted. -ISSUE_COMMENTS_JQ='[.[][]] | if length == 0 then "No PR conversation comments." else sort_by(.created_at) | map("--- \(.user.login) at \(.created_at)\n\((.body // "")[0:3000])") | join("\n") end' +# +# $skip is excluded here as well, and for the same reason: the review body Pullfrog posts +# is a PR summary plus its findings, and this endpoint is where it lands. The empty case +# names the withholding on the same principle as the threads block above. +ISSUE_COMMENTS_JQ='[.[][]] as $all | ($all | map(select(.user.login != $skip))) as $kept | (($all | length) - ($kept | length)) as $dropped | if ($kept | length) == 0 then (if $dropped > 0 then "No PR conversation comments. (\($dropped) comment(s) from \($skip) are excluded from this block.)" else "No PR conversation comments." end) else ($kept | sort_by(.created_at) | map("--- \(.user.login) at \(.created_at)\n\((.body // "")[0:3000])") | join("\n")) end' if CONVO_JSON=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate); then - CONVO=$(printf '%s' "$CONVO_JSON" | jq -s -r "$ISSUE_COMMENTS_JQ" 2>/dev/null) \ + CONVO=$(printf '%s' "$CONVO_JSON" | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$ISSUE_COMMENTS_JQ" 2>/dev/null) \ || CONVO="Could not parse PR conversation comments." else echo "::warning::Could not read PR conversation comments." diff --git a/tests/fixtures/pull-comments-mixed-authors.json b/tests/fixtures/pull-comments-mixed-authors.json new file mode 100644 index 0000000..47f1723 --- /dev/null +++ b/tests/fixtures/pull-comments-mixed-authors.json @@ -0,0 +1,35 @@ +[ + { + "id": 2101, + "user": { "login": "claude[bot]" }, + "path": "scripts/gather-review-context.sh", + "line": 412, + "created_at": "2026-08-27T09:00:00Z", + "body": "The cap is measured on the unstripped file, so the emitted block can exceed it." + }, + { + "id": 2102, + "user": { "login": "zfarrell" }, + "path": "scripts/gather-review-context.sh", + "line": 412, + "in_reply_to_id": 2101, + "created_at": "2026-08-27T09:31:00Z", + "body": "Fixed in the next push -- strip_block_tags runs first now." + }, + { + "id": 2103, + "user": { "login": "pullfrog[bot]" }, + "path": "scripts/gather-review-context.sh", + "line": 88, + "created_at": "2026-08-27T09:04:00Z", + "body": "## Summary\n\nThis PR reworks the context budget. One finding: the byte cap and the line cap disagree." + }, + { + "id": 2104, + "user": { "login": "claude[bot]" }, + "path": "tests/lib.sh", + "line": 22, + "created_at": "2026-08-27T09:05:00Z", + "body": null + } +] diff --git a/tests/fixtures/reviews-other-review-bot.json b/tests/fixtures/reviews-other-review-bot.json new file mode 100644 index 0000000..b6a479c --- /dev/null +++ b/tests/fixtures/reviews-other-review-bot.json @@ -0,0 +1,22 @@ +[ + { + "id": 1, + "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "state": "COMMENTED", + "submitted_at": "2026-08-28T00:00:00Z", + "user": { + "login": "pullfrog[bot]", + "type": "Bot" + } + }, + { + "id": 2, + "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "state": "CHANGES_REQUESTED", + "submitted_at": "2026-08-28T00:00:05Z", + "user": { + "login": "pullfrog[bot]", + "type": "Bot" + } + } +] diff --git a/tests/lib.sh b/tests/lib.sh index 6ac134f..054ea93 100755 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -39,3 +39,12 @@ extract_jq() { fi printf '%s' "$prog" } + +# extract_const -- the same extraction for a value that is not a jq +# program, so a test can pass the shipped constant to a program that takes it as --arg +# instead of writing a second copy of it here. OTHER_REVIEW_BOT is the case: three shipped +# programs read it, and a test hardcoding the login would keep passing after the constant +# changed -- which is precisely the drift the extraction of the programs exists to prevent. +extract_const() { + extract_jq "$1" +} diff --git a/tests/pr-context-test.sh b/tests/pr-context-test.sh index 22e09f3..fff112c 100755 --- a/tests/pr-context-test.sh +++ b/tests/pr-context-test.sh @@ -26,6 +26,11 @@ FAILING_JOBS_JQ=$(extract_jq FAILING_JOBS_JQ) LAST_REVIEW_JQ=$(extract_jq LAST_REVIEW_JQ) COMPARE_STATUS_JQ=$(extract_jq COMPARE_STATUS_JQ) ISSUE_COMMENTS_JQ=$(extract_jq ISSUE_COMMENTS_JQ) +THREADS_JQ=$(extract_jq THREADS_JQ) +# Both comment programs take the second reviewer's login as --arg. Extracted, not written +# here: a test carrying its own copy of the login would keep passing after the shipped +# constant changed, which is the drift extracting the programs exists to prevent. +OTHER_REVIEW_BOT=$(extract_const OTHER_REVIEW_BOT) failures=0 @@ -43,11 +48,15 @@ expect() { # The paginated endpoints go through `jq -s` in the workflow, because gh 2.93 merges # --paginate pages into one array while older versions concatenate one array per page. +# +# --arg skip on every call, whether the program reads it or not: jq only objects to an +# undefined $skip, never to an unused one, and one helper is easier to keep right than a +# second helper for the two programs that take it. slurped() { - jq -s -r "$2" "tests/fixtures/$1" + jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$2" "tests/fixtures/$1" } plain() { - jq -r "$2" "tests/fixtures/$1" + jq --arg skip "$OTHER_REVIEW_BOT" -r "$2" "tests/fixtures/$1" } # --- Commits ----------------------------------------------------------------------------- @@ -178,6 +187,38 @@ expect "$(printf '{"status":"diverged"}' | jq -r "$COMPARE_STATUS_JQ")" "diverge expect "$(printf '{}' | jq -r "$COMPARE_STATUS_JQ")" "unknown" \ "comparison with no status reports unknown, never ahead" +# --- Prior review threads ---------------------------------------------------------------- + +# This block is the prompt's other output, interpolated into the same string as pr_context +# and wrapped in . It was an inline jq program until the exclusion +# below needed asserting, so these are its first fixture-level assertions. + +# Every kept comment is labelled, and the reply carries its parent rather than a thread id: +# the reviewer decides whether prior feedback was answered by reading who said what to whom. +expect "$(slurped pull-comments-mixed-authors.json "$THREADS_JQ" | grep -c '^Author: ')" "3" \ + "every kept thread comment is labelled with its author" +expect "$(slurped pull-comments-mixed-authors.json "$THREADS_JQ" | grep -c '^Reply to #2101$')" "1" \ + "a reply carries its parent comment id, not a thread id" + +# The exclusion the trial turns on. A second reviewer's findings arriving here would reach +# this reviewer as settled prior feedback -- not to be re-raised -- which is how a parallel +# comparison stops comparing two independent reviews. +expect "$(slurped pull-comments-mixed-authors.json "$THREADS_JQ" | grep -c 'pullfrog')" "0" \ + "the other review bot's inline comments do not reach the prompt" +expect "$(slurped pull-comments-mixed-authors.json "$THREADS_JQ" | grep -c '^null$')" "0" \ + "null comment body does not render as the word null" + +# Withholding the only comments on the PR leaves a sentence that would otherwise be false, +# and this block's sentences are what the cycle ladder acts on. So the empty case says how +# many comments were withheld and whose, while the genuinely empty case stays plain. +expect "$(printf '[{"id":1,"user":{"login":"%s"},"path":"a.py","line":3,"created_at":"2026-08-28T00:00:00Z","body":"finding"}]' \ + "$OTHER_REVIEW_BOT" | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$THREADS_JQ")" \ + "No prior review comments. (1 comment(s) from $OTHER_REVIEW_BOT are excluded from this block.)" \ + "a block emptied by the exclusion says so, and how much it withheld" +expect "$(printf '[]' | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$THREADS_JQ")" \ + "No prior review comments." \ + "a genuinely empty block claims nothing about an exclusion" + # --- PR conversation --------------------------------------------------------------------- # Chronological, and every author labelled: the reviewer's own prior summary comments are in @@ -194,9 +235,24 @@ expect "$(slurped issue-comments.json "$ISSUE_COMMENTS_JQ" | head -1)" \ expect "$(slurped issue-comments.json "$ISSUE_COMMENTS_JQ" | grep -c '^null$')" "0" \ "null comment body does not render as the word null" -expect "$(printf '[]' | jq -s -r "$ISSUE_COMMENTS_JQ")" "No PR conversation comments." \ +expect "$(printf '[]' | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$ISSUE_COMMENTS_JQ")" \ + "No PR conversation comments." \ "no conversation comments says so" +# The other half of the exclusion. The second reviewer posts its review body here -- a PR +# summary plus its findings -- so this endpoint carries the bulk of what it would contribute +# to this reviewer's prompt, against a block that may take half the escaped-byte budget. +expect "$(printf '[{"id":1,"user":{"login":"zfarrell"},"created_at":"2026-08-28T00:00:00Z","body":"rebased"},{"id":2,"user":{"login":"%s"},"created_at":"2026-08-28T00:01:00Z","body":"## Summary"}]' \ + "$OTHER_REVIEW_BOT" | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$ISSUE_COMMENTS_JQ")" \ + "--- zfarrell at 2026-08-28T00:00:00Z +rebased" \ + "the other review bot's conversation comments do not reach the prompt" + +expect "$(printf '[{"id":2,"user":{"login":"%s"},"created_at":"2026-08-28T00:01:00Z","body":"## Summary"}]' \ + "$OTHER_REVIEW_BOT" | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$ISSUE_COMMENTS_JQ")" \ + "No PR conversation comments. (1 comment(s) from $OTHER_REVIEW_BOT are excluded from this block.)" \ + "a conversation emptied by the exclusion says so too" + if [ "$failures" -ne 0 ]; then echo "$failures test(s) failed" exit 1 diff --git a/tests/review-cycle-test.sh b/tests/review-cycle-test.sh index c1962c3..bf3ae0d 100755 --- a/tests/review-cycle-test.sh +++ b/tests/review-cycle-test.sh @@ -19,6 +19,9 @@ cd "$(dirname "$0")/.." CYCLE_JQ=$(extract_jq CYCLE_JQ) DRIFT_JQ=$(extract_jq DRIFT_JQ) +# The drift predicate takes the second reviewer's login as --arg, so the test has to supply +# it -- extracted, never written here, so it cannot outlive a change to the shipped value. +OTHER_REVIEW_BOT=$(extract_const OTHER_REVIEW_BOT) failures=0 @@ -39,7 +42,7 @@ expect() { # 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 + if jq --arg skip "$OTHER_REVIEW_BOT" -e -s "$DRIFT_JQ" "tests/fixtures/$fixture" >/dev/null 2>&1; then actual=fires fi if [ "$actual" = "$want" ]; then @@ -69,6 +72,15 @@ 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" +# The second reviewer in the comparison trial is a Bot and reviews the same pull requests, +# and both reviewers fire on `opened` -- so its review landing before this one's is the +# ordinary cycle 1, not evidence that this reviewer's identity moved. Without the exclusion +# in the predicate the warning would fire on the first review of every PR in a trial repo, +# which is how a real drift goes unread. +expect reviews-other-review-bot.json 1 "the other reviewer's rounds do not count as ours" +expect_drift reviews-other-review-bot.json silent \ + "drift warning silent when only the other review bot has reviewed" + # 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" From d7f8a879ab9bef7f80fc1e2a27ba398cf2258a1e Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 28 Aug 2026 09:44:28 -0700 Subject: [PATCH 2/3] fix(review): close the remaining exclusion gaps --- README.md | 41 +++++++---- scripts/gather-review-context.sh | 29 ++++++-- .../fixtures/pull-comments-mixed-authors.json | 9 +++ .../reviews-drift-with-other-bot.json | 32 +++++++++ tests/fixtures/rollup-with-review-bot.json | 39 +++++++++++ tests/pr-context-test.sh | 70 +++++++++++++++---- tests/review-cycle-test.sh | 10 +++ 7 files changed, 200 insertions(+), 30 deletions(-) create mode 100644 tests/fixtures/reviews-drift-with-other-bot.json create mode 100644 tests/fixtures/rollup-with-review-bot.json diff --git a/README.md b/README.md index a995213..f4933d5 100644 --- a/README.md +++ b/README.md @@ -82,21 +82,38 @@ what the other reads as settled prior feedback, not to be re-raised. It also pro this reviewer already competes for: comment threads may take half of it, and a Pullfrog review body is a PR summary plus its findings. -The login lives in one constant, `OTHER_REVIEW_BOT`, and reaches three programs as a jq `--arg`. -The third is the review-cycle drift predicate, which asks whether a bot review exists that the -`claude[bot]` filter did not count — true on the first review of every PR in a trial repo, so -without the exclusion it would report a reviewer-identity change that has not happened, on every -pull request, which is how the warning goes unread on the one where it is real. +The login lives in one constant, `OTHER_REVIEW_BOT`, and reaches five programs as a jq `--arg`. +One of them reads nothing out of the prompt at all: the review-cycle drift predicate asks whether +a bot review exists that the `claude[bot]` filter did not count, which is true on the first review +of every PR in a trial repo — so without the exclusion it would report a reviewer-identity change +that has not happened, on every pull request, which is how the warning goes unread on the one +where it is real. The predicate still has to fire when a drifted login sits *beside* the excluded +one, and after the trial that is the only shape a real drift takes, so it is the case the fixtures +pin. + +The CI block is the third channel and the one with teeth, so `CHECKS_JQ` and `FAILING_JOBS_JQ` +drop the same reviewer's checks by name — matched against the app slug, since a GitHub App's login +is its slug plus `[bot]`, so `$skip` stays the single constant. Pullfrog's verdict is a check, +`pullfrog-approval`, failing when it requested changes, and this prompt tells the reviewer that a +failing check is a blocking issue to name and cite. Unfiltered, the rollup does not merely leak +the other arm's conclusion; it converts it into a request-changes this reviewer cannot +substantiate from the diff. Its run-status check is worse per byte: the check links to Pullfrog's +own Actions job, and `FAILING_JOBS_JQ` would fetch that job's log into a `### Failing job` excerpt +— the other reviewer's transcript, verbatim, at up to an eighth of the context. That the check +gates no merge is true of merge and silent about the prompt. Where the exclusion empties a block it says so and how much it withheld, because "No prior review comments." on a PR that has some is the same false claim as an empty CI block -reading as a green one. Where other comments remain it stays silent: the block claims nothing -about being every comment on the PR. - -Pullfrog's own review verdict is a check, `pullfrog-approval`, reporting whether it *would* -approve. It is not required in the org ruleset and Pullfrog cannot submit an approving review, so -this reviewer remains the only automated approval in the org for the duration of the trial. The -constant, and this section, come out when the trial ends. +reading as a green one. Where other entries remain it stays silent: neither block claims to be +every comment or every check on the PR. One rendering changes with it. A human reply to an +excluded comment is kept — real feedback, and dropping it would hide more than the exclusion +protects — but its parent is gone, and an id pointing at nothing in the block turns "fixed in the +next push" into a settled finding whose subject the reviewer never sees. So an orphaned reply is +relabelled rather than renumbered or dropped. + +Pullfrog cannot submit an approving review and `pullfrog-approval` is not required in the org +ruleset, so this reviewer remains the only automated approval in the org for the duration of the +trial. The constant, the four exclusions, and this section come out when the trial ends. ### Tool usage artifact diff --git a/scripts/gather-review-context.sh b/scripts/gather-review-context.sh index f2b8659..aeb3dc7 100755 --- a/scripts/gather-review-context.sh +++ b/scripts/gather-review-context.sh @@ -385,7 +385,7 @@ fi # Single-line and single-quoted so tests/lib.sh can extract it; it was inline, and inline # meant the one program in this script that shapes the prompt's other output could not be # asserted against a fixture at all. -THREADS_JQ='(add // []) as $all | ($all | map(select(.user.login != $skip))) as $kept | (($all | length) - ($kept | length)) as $dropped | if ($kept | length) == 0 then (if $dropped > 0 then "No prior review comments. (\($dropped) comment(s) from \($skip) are excluded from this block.)" else "No prior review comments." end) else ($kept | sort_by(.created_at) | .[] | "---", "Author: \(.user.login)", "File: \(.path)", (if .line then "Line: \(.line)" else empty end), (if .in_reply_to_id then "Reply to #\(.in_reply_to_id)" else "Thread #\(.id)" end), "", ((.body // "")[0:3000])) end' +THREADS_JQ='(add // []) as $all | ($all | map(select(.user.login != $skip))) as $kept | ($kept | map(.id)) as $ids | (($all | length) - ($kept | length)) as $dropped | if ($kept | length) == 0 then (if $dropped > 0 then "No prior review comments. (\($dropped) comment(s) from \($skip) are excluded from this block.)" else "No prior review comments." end) else ($kept | sort_by(.created_at) | .[] | "---", "Author: \(.user.login)", "File: \(.path)", (if .line then "Line: \(.line)" else empty end), (if .in_reply_to_id then (if (.in_reply_to_id | IN($ids[])) then "Reply to #\(.in_reply_to_id)" else "Reply to a comment excluded from this block" end) else "Thread #\(.id)" end), "", ((.body // "")[0:3000])) end' if [ "$COMMENTS_OK" -eq 0 ]; then THREADS='Unavailable: the prior inline review comments could not be read. This block is empty because the fetch failed, not because there were none.' else @@ -549,14 +549,33 @@ cap_file_escaped "$FILES_FILE" $((CTX_MAX_BYTES / 8)) \ # The reviewer cannot run tests -- no dependencies are installed and the allowlist # would refuse anyway -- but CI already ran them. Whether they passed is the one # fact it was asserting without evidence. -CHECKS_JQ='(.statusCheckRollup // []) | if length == 0 then "No checks reported." else map(if .__typename == "CheckRun" then "\(.conclusion // .status // "UNKNOWN") \(.workflowName // "") / \(.name // "(unnamed check)")" else "\(.state // "UNKNOWN") \(.context // "status")" end) | sort | join("\n") end' +# +# This block is the third channel $skip reaches, and the one where it does the most damage. +# Pullfrog posts its verdict as a check -- `pullfrog-approval`, failing when it requested +# changes -- and the prompt tells this reviewer that a failing check is a blocking issue to +# name and cite. So an unfiltered rollup does not merely leak the other arm's conclusion; it +# converts it into a request-changes this reviewer cannot substantiate from the diff. +# `pullfrog` (the run-status check) rides along for the same reason, and its detailsUrl would +# otherwise feed FAILING_JOBS_JQ below the *other reviewer's own job log* as a failing-job +# excerpt, at up to an eighth of the context. +# +# Matched by name against the app slug rather than by a second constant: a GitHub App's bot +# login is its slug plus "[bot]", and its checks are the slug and slug-prefixed names, so +# $skip still carries the one value all five programs agree on. Both rollup shapes are +# matched -- a CheckRun by .name, a StatusContext by .context -- because which of the two an +# app posts is the app's choice, not ours. +# +# Silent while other checks remain, and stated when the exclusion empties the block: a list +# claims nothing about being every check, but "No checks reported." on a PR that has some is +# a false claim, and this is the block whose emptiness the README warns gets read as green. +CHECKS_JQ='def slug: $skip | sub("\\[bot\\]$"; ""); def theirs: ((if .__typename == "CheckRun" then (.name // "") else (.context // "") end) | . == slug or startswith(slug + "-")); def render: if .__typename == "CheckRun" then "\(.conclusion // .status // "UNKNOWN") \(.workflowName // "") / \(.name // "(unnamed check)")" else "\(.state // "UNKNOWN") \(.context // "status")" end; (.statusCheckRollup // []) as $all | ($all | map(select(theirs | not))) as $kept | (($all | length) - ($kept | length)) as $dropped | if ($kept | length) == 0 then (if $dropped > 0 then "No checks reported. (\($dropped) check(s) from \(slug) are excluded from this block; they carry a verdict from another reviewer, not a CI result.)" else "No checks reported." end) else ($kept | map(render) | sort | join("\n")) end' # Actions check runs carry the job id in detailsUrl; scan rather than capture so a # non-Actions check with no job id drops out instead of erroring. -FAILING_JOBS_JQ='[(.statusCheckRollup // [])[] | select(.__typename == "CheckRun") | select((.conclusion // "") | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) | (.detailsUrl // "") | [scan("/job/([0-9]+)")] | flatten | .[0] // empty] | unique | .[0:3] | join(" ")' +FAILING_JOBS_JQ='def slug: $skip | sub("\\[bot\\]$"; ""); [(.statusCheckRollup // [])[] | select(.__typename == "CheckRun") | select(((.name // "") | . == slug or startswith(slug + "-")) | not) | select((.conclusion // "") | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) | (.detailsUrl // "") | [scan("/job/([0-9]+)")] | flatten | .[0] // empty] | unique | .[0:3] | join(" ")' if ROLLUP=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json statusCheckRollup); then - CHECKS=$(printf '%s' "$ROLLUP" | jq -r "$CHECKS_JQ" 2>/dev/null) \ + CHECKS=$(printf '%s' "$ROLLUP" | jq --arg skip "$OTHER_REVIEW_BOT" -r "$CHECKS_JQ" 2>/dev/null) \ || CHECKS="Could not parse checks." - JOB_IDS=$(printf '%s' "$ROLLUP" | jq -r "$FAILING_JOBS_JQ" 2>/dev/null) || JOB_IDS='' + JOB_IDS=$(printf '%s' "$ROLLUP" | jq --arg skip "$OTHER_REVIEW_BOT" -r "$FAILING_JOBS_JQ" 2>/dev/null) || JOB_IDS='' else echo "::warning::Could not read check status." CHECKS="Could not read check status." diff --git a/tests/fixtures/pull-comments-mixed-authors.json b/tests/fixtures/pull-comments-mixed-authors.json index 47f1723..122bf4e 100644 --- a/tests/fixtures/pull-comments-mixed-authors.json +++ b/tests/fixtures/pull-comments-mixed-authors.json @@ -24,6 +24,15 @@ "created_at": "2026-08-27T09:04:00Z", "body": "## Summary\n\nThis PR reworks the context budget. One finding: the byte cap and the line cap disagree." }, + { + "id": 2105, + "user": { "login": "zfarrell" }, + "path": "scripts/gather-review-context.sh", + "line": 88, + "in_reply_to_id": 2103, + "created_at": "2026-08-27T09:40:00Z", + "body": "Good catch -- fixed in the next push." + }, { "id": 2104, "user": { "login": "claude[bot]" }, diff --git a/tests/fixtures/reviews-drift-with-other-bot.json b/tests/fixtures/reviews-drift-with-other-bot.json new file mode 100644 index 0000000..393e3f1 --- /dev/null +++ b/tests/fixtures/reviews-drift-with-other-bot.json @@ -0,0 +1,32 @@ +[ + { + "id": 1, + "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "state": "COMMENTED", + "submitted_at": "2026-08-28T00:00:00Z", + "user": { + "login": "pullfrog[bot]", + "type": "Bot" + } + }, + { + "id": 2, + "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "state": "CHANGES_REQUESTED", + "submitted_at": "2026-08-28T00:01:00Z", + "user": { + "login": "claude-review[bot]", + "type": "Bot" + } + }, + { + "id": 3, + "commit_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "state": "APPROVED", + "submitted_at": "2026-08-28T01:00:00Z", + "user": { + "login": "claude-review[bot]", + "type": "Bot" + } + } +] diff --git a/tests/fixtures/rollup-with-review-bot.json b/tests/fixtures/rollup-with-review-bot.json new file mode 100644 index 0000000..7e8b33c --- /dev/null +++ b/tests/fixtures/rollup-with-review-bot.json @@ -0,0 +1,39 @@ +{ + "statusCheckRollup": [ + { + "__typename": "CheckRun", + "name": "test", + "workflowName": "Tests", + "status": "COMPLETED", + "conclusion": "FAILURE", + "detailsUrl": "https://github.com/hotdata-dev/github-workflows/actions/runs/33190449262/job/98914277093", + "startedAt": "2026-08-28T17:47:38Z", + "completedAt": "2026-08-28T17:51:24Z" + }, + { + "__typename": "CheckRun", + "name": "pullfrog", + "workflowName": "Pullfrog", + "status": "IN_PROGRESS", + "conclusion": null, + "detailsUrl": "https://github.com/hotdata-dev/github-workflows/actions/runs/33190500001/job/98914300001", + "startedAt": "2026-08-28T17:48:02Z", + "completedAt": null + }, + { + "__typename": "CheckRun", + "name": "pullfrog-approval", + "workflowName": "Pullfrog", + "status": "COMPLETED", + "conclusion": "FAILURE", + "detailsUrl": "https://github.com/hotdata-dev/github-workflows/actions/runs/33190500001/job/98914300002", + "startedAt": "2026-08-28T17:48:02Z", + "completedAt": "2026-08-28T17:52:11Z" + }, + { + "__typename": "StatusContext", + "context": "aikido/code-scan", + "state": "SUCCESS" + } + ] +} diff --git a/tests/pr-context-test.sh b/tests/pr-context-test.sh index fff112c..f7b1ded 100755 --- a/tests/pr-context-test.sh +++ b/tests/pr-context-test.sh @@ -69,7 +69,7 @@ expect "$(slurped pull-commits.json "$COMMITS_JQ")" \ 77f4a9a8 feat(ingest): POST /jobs/sync + continuous flag" \ "commits reduce to short SHA and subject" -expect "$(printf '[]' | jq -s -r "$COMMITS_JQ")" "No commits reported." \ +expect "$(printf '[]' | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$COMMITS_JQ")" "No commits reported." \ "no commits says so rather than emitting nothing" # --- Changed files ----------------------------------------------------------------------- @@ -84,24 +84,24 @@ renamed +219/-3 connectors/files/__init__.py added +12/-0 migrations/004_sync_state.sql" \ "changed files carry status and per-file counts under a total" -expect "$(printf '[]' | jq -s -r "$FILES_JQ")" "No changed files reported." \ +expect "$(printf '[]' | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$FILES_JQ")" "No changed files reported." \ "no changed files says so" # A field the API omitted must not render as a number. "+null/-null" is a claim about the # change, and the reviewer quotes these counts back in its comments; "unknown" and 0 are # visibly not measurements. Same reason the CheckRun name is defaulted below. -expect "$(printf '[{"filename":"api/app.py"},{"status":"modified","additions":4,"deletions":1,"filename":"b.py"}]' | jq -s -r "$FILES_JQ")" \ +expect "$(printf '[{"filename":"api/app.py"},{"status":"modified","additions":4,"deletions":1,"filename":"b.py"}]' | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$FILES_JQ")" \ "2 files, +4 -1 unknown +0/-0 api/app.py modified +4/-1 b.py" \ "file entry missing its counts renders as unknown, not null" -expect "$(printf '[{"status":"added","additions":1,"deletions":0}]' | jq -s -r "$FILES_JQ")" \ +expect "$(printf '[{"status":"added","additions":1,"deletions":0}]' | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$FILES_JQ")" \ "1 files, +1 -0 added +1/-0 (unnamed file)" \ "file entry missing its name says so rather than naming null" -expect "$(printf '{"statusCheckRollup":[{"__typename":"CheckRun","conclusion":"FAILURE"}]}' | jq -r "$CHECKS_JQ")" \ +expect "$(printf '{"statusCheckRollup":[{"__typename":"CheckRun","conclusion":"FAILURE"}]}' | jq --arg skip "$OTHER_REVIEW_BOT" -r "$CHECKS_JQ")" \ "FAILURE / (unnamed check)" \ "check missing its name does not become a check called null" @@ -126,7 +126,7 @@ expect "$(plain rollup-mixed.json "$CHECKS_JQ" | grep -c 'IN_PROGRESS\|PENDING') expect "$(plain rollup-empty.json "$CHECKS_JQ")" "No checks reported." \ "empty rollup says so rather than claiming success" -expect "$(printf '{}' | jq -r "$CHECKS_JQ")" "No checks reported." \ +expect "$(printf '{}' | jq --arg skip "$OTHER_REVIEW_BOT" -r "$CHECKS_JQ")" "No checks reported." \ "missing rollup key does not error" # --- Failing job ids --------------------------------------------------------------------- @@ -142,9 +142,43 @@ expect "$(plain rollup-empty.json "$FAILING_JOBS_JQ")" "" \ # A check whose detailsUrl is not an Actions job URL must drop out silently: the log fetch # is keyed on a numeric job id and there is nothing to fetch here. -expect "$(printf '{"statusCheckRollup":[{"__typename":"CheckRun","conclusion":"FAILURE","detailsUrl":"https://app.aikido.dev/scan/1"}]}' | jq -r "$FAILING_JOBS_JQ")" \ +expect "$(printf '{"statusCheckRollup":[{"__typename":"CheckRun","conclusion":"FAILURE","detailsUrl":"https://app.aikido.dev/scan/1"}]}' | jq --arg skip "$OTHER_REVIEW_BOT" -r "$FAILING_JOBS_JQ")" \ "" "failing check with no job id in detailsUrl drops out" +# --- The other reviewer's checks ---------------------------------------------------------- + +# The third channel the exclusion has to close, and the one with teeth. The other reviewer +# posts its verdict as a check -- failing when it requested changes -- and this prompt tells +# the reviewer that a failing check is a blocking issue to name and cite. Left in, the rollup +# does not merely leak the other arm's conclusion; it converts it into a request-changes this +# reviewer cannot substantiate from the diff. +expect "$(plain rollup-with-review-bot.json "$CHECKS_JQ")" \ + "FAILURE Tests / test +SUCCESS aikido/code-scan" \ + "the other reviewer's checks are not listed among the CI checks" + +# Worse than the listing: its check links to its own Actions job, so an unfiltered scan feeds +# the other reviewer's agent log into the prompt as a failing-job excerpt, at up to an eighth +# of the context. +expect "$(plain rollup-with-review-bot.json "$FAILING_JOBS_JQ")" "98914277093" \ + "the other reviewer's failing check contributes no job log" + +# Both rollup shapes, because which one an app posts is the app's choice. The slug comes from +# $skip -- a GitHub App's login is its slug plus "[bot]" -- so there is no second constant to +# keep in step. +expect "$(printf '{"statusCheckRollup":[{"__typename":"StatusContext","context":"pullfrog-approval","state":"FAILURE"},{"__typename":"StatusContext","context":"terraform","state":"SUCCESS"}]}' \ + | jq --arg skip "$OTHER_REVIEW_BOT" -r "$CHECKS_JQ")" \ + "SUCCESS terraform" \ + "the verdict is excluded as a commit status too, not only as a check run" + +# Same asymmetry as the comment blocks: silent while other checks remain, stated when the +# exclusion empties the block. This is the block whose emptiness gets read as green, so +# "No checks reported." on a PR that has checks is the false claim to avoid. +expect "$(printf '{"statusCheckRollup":[{"__typename":"CheckRun","name":"pullfrog-approval","conclusion":"FAILURE"}]}' \ + | jq --arg skip "$OTHER_REVIEW_BOT" -r "$CHECKS_JQ")" \ + "No checks reported. (1 check(s) from pullfrog are excluded from this block; they carry a verdict from another reviewer, not a CI result.)" \ + "a check list emptied by the exclusion says so rather than reading as no checks" + # --- Base SHA for the since-last-review diff --------------------------------------------- # The blast radius if this picks the wrong SHA: the reviewer is handed a diff labelled @@ -167,12 +201,12 @@ expect "$(slurped reviews-first-review.json "$LAST_REVIEW_JQ")" "" \ expect "$(slurped reviews-foreign-reviewer.json "$LAST_REVIEW_JQ")" "" \ "reviews by another login do not supply the base SHA" -expect "$(printf '[]' | jq -s -r "$LAST_REVIEW_JQ")" "" \ +expect "$(printf '[]' | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$LAST_REVIEW_JQ")" "" \ "no reviews at all yields an empty base SHA" # A PENDING review has no submitted_at. Sorting on null would put it anywhere, and it has no # commit the author can have responded to yet. -expect "$(printf '[{"user":{"login":"claude[bot]"},"commit_id":"aaa","submitted_at":null}]' | jq -s -r "$LAST_REVIEW_JQ")" \ +expect "$(printf '[{"user":{"login":"claude[bot]"},"commit_id":"aaa","submitted_at":null}]' | jq --arg skip "$OTHER_REVIEW_BOT" -s -r "$LAST_REVIEW_JQ")" \ "" "unsubmitted review is not treated as the last review" # --- Comparison status -------------------------------------------------------------------- @@ -180,11 +214,11 @@ expect "$(printf '[{"user":{"login":"claude[bot]"},"commit_id":"aaa","submitted_ # Only "ahead" means the reviewed SHA fast-forwards to the head, which is the one case where # a three-dot compare really is "everything since my last review". Anything else -- and # anything unreadable -- has to be distinguishable from it by the caller. -expect "$(printf '{"status":"ahead","ahead_by":2}' | jq -r "$COMPARE_STATUS_JQ")" "ahead" \ +expect "$(printf '{"status":"ahead","ahead_by":2}' | jq --arg skip "$OTHER_REVIEW_BOT" -r "$COMPARE_STATUS_JQ")" "ahead" \ "fast-forward comparison reports ahead" -expect "$(printf '{"status":"diverged"}' | jq -r "$COMPARE_STATUS_JQ")" "diverged" \ +expect "$(printf '{"status":"diverged"}' | jq --arg skip "$OTHER_REVIEW_BOT" -r "$COMPARE_STATUS_JQ")" "diverged" \ "rebased comparison reports diverged" -expect "$(printf '{}' | jq -r "$COMPARE_STATUS_JQ")" "unknown" \ +expect "$(printf '{}' | jq --arg skip "$OTHER_REVIEW_BOT" -r "$COMPARE_STATUS_JQ")" "unknown" \ "comparison with no status reports unknown, never ahead" # --- Prior review threads ---------------------------------------------------------------- @@ -195,11 +229,21 @@ expect "$(printf '{}' | jq -r "$COMPARE_STATUS_JQ")" "unknown" \ # Every kept comment is labelled, and the reply carries its parent rather than a thread id: # the reviewer decides whether prior feedback was answered by reading who said what to whom. -expect "$(slurped pull-comments-mixed-authors.json "$THREADS_JQ" | grep -c '^Author: ')" "3" \ +expect "$(slurped pull-comments-mixed-authors.json "$THREADS_JQ" | grep -c '^Author: ')" "4" \ "every kept thread comment is labelled with its author" expect "$(slurped pull-comments-mixed-authors.json "$THREADS_JQ" | grep -c '^Reply to #2101$')" "1" \ "a reply carries its parent comment id, not a thread id" +# A human answering one of the excluded comments is kept -- it is real feedback, and dropping +# it would hide more than the exclusion protects. But its parent is gone, so an id pointing at +# nothing in the block turns "fixed in the next push" into a settled finding whose subject this +# reviewer never sees, which is the not-to-be-re-raised reading the block is written to produce. +# So an orphaned reply is relabelled rather than renumbered or dropped. +expect "$(slurped pull-comments-mixed-authors.json "$THREADS_JQ" | grep -c '^Reply to #2103$')" "0" \ + "a reply to an excluded comment does not point at an id absent from the block" +expect "$(slurped pull-comments-mixed-authors.json "$THREADS_JQ" | grep -c '^Reply to a comment excluded from this block$')" "1" \ + "an orphaned reply says its parent is missing, and is still shown" + # The exclusion the trial turns on. A second reviewer's findings arriving here would reach # this reviewer as settled prior feedback -- not to be re-raised -- which is how a parallel # comparison stops comparing two independent reviews. diff --git a/tests/review-cycle-test.sh b/tests/review-cycle-test.sh index bf3ae0d..c84e17a 100755 --- a/tests/review-cycle-test.sh +++ b/tests/review-cycle-test.sh @@ -81,6 +81,16 @@ expect reviews-other-review-bot.json 1 "the other reviewer's rounds do not count expect_drift reviews-other-review-bot.json silent \ "drift warning silent when only the other review bot has reviewed" +# And the state the exclusion makes ordinary is the one the predicate still has to catch: a +# drifted login sitting *beside* the excluded one. Every PR in a trial repo carries a +# pullfrog[bot] review, so this -- not the foreign bot alone -- is what a real identity change +# looks like from now on. A predicate that excluded $skip in a way that also swallowed its +# neighbours (`all` in place of `any`, or a filter applied to the whole array before the type +# test) passes both cases above and goes silent here, which is the only case left that matters. +expect reviews-drift-with-other-bot.json 1 "a drifted login beside the excluded one yields no rounds" +expect_drift reviews-drift-with-other-bot.json fires \ + "drift warning still fires when a drifted login sits beside the excluded one" + # 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" From e5093df520bcd7973c6691fc72fdf9042decc304 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 28 Aug 2026 09:52:44 -0700 Subject: [PATCH 3/3] fix(review): match the other reviewer by workflow too --- README.md | 14 ++++++++++--- scripts/gather-review-context.sh | 24 ++++++++++++++++++---- tests/fixtures/rollup-with-review-bot.json | 15 ++++++++++++++ tests/pr-context-test.sh | 19 +++++++++++++++-- 4 files changed, 63 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index f4933d5..65887dc 100644 --- a/README.md +++ b/README.md @@ -92,8 +92,12 @@ one, and after the trial that is the only shape a real drift takes, so it is the pin. The CI block is the third channel and the one with teeth, so `CHECKS_JQ` and `FAILING_JOBS_JQ` -drop the same reviewer's checks by name — matched against the app slug, since a GitHub App's login -is its slug plus `[bot]`, so `$skip` stays the single constant. Pullfrog's verdict is a check, +drop the same reviewer's checks through one shared owner test, `CHECK_OWNER_JQ`, composed into both +the way the workflow composes `CMD_JQ` into `TOOL_USAGE_JQ` — two copies of that rule were free to +disagree about what "theirs" means, and did. It matches the app slug against both the entry's own +name and its `workflowName`, downcased, since a GitHub App's login is its slug plus `[bot]` (so +`$skip` stays the single constant) while an Actions check run carries the *job* name in `.name` — +matching that alone would make the exclusion depend on a job key in another repository. Pullfrog's verdict is a check, `pullfrog-approval`, failing when it requested changes, and this prompt tells the reviewer that a failing check is a blocking issue to name and cite. Unfiltered, the rollup does not merely leak the other arm's conclusion; it converts it into a request-changes this reviewer cannot @@ -113,7 +117,11 @@ relabelled rather than renumbered or dropped. Pullfrog cannot submit an approving review and `pullfrog-approval` is not required in the org ruleset, so this reviewer remains the only automated approval in the org for the duration of the -trial. The constant, the four exclusions, and this section come out when the trial ends. +trial. The constant, the four exclusions, the drift predicate's exception and this section come out +when the trial ends — the drift predicate by name, because it is the one place an incomplete +removal is silent. Delete `OTHER_REVIEW_BOT` and leave `$skip` in `DRIFT_JQ` and its `jq` call +fails to compile, which the `elif` guarding it reads as "no drift": the backstop against a +reviewer-login change would be gone with no warning in either direction. ### Tool usage artifact diff --git a/scripts/gather-review-context.sh b/scripts/gather-review-context.sh index aeb3dc7..c2b25d0 100755 --- a/scripts/gather-review-context.sh +++ b/scripts/gather-review-context.sh @@ -568,14 +568,30 @@ cap_file_escaped "$FILES_FILE" $((CTX_MAX_BYTES / 8)) \ # Silent while other checks remain, and stated when the exclusion empties the block: a list # claims nothing about being every check, but "No checks reported." on a PR that has some is # a false claim, and this is the block whose emptiness the README warns gets read as green. -CHECKS_JQ='def slug: $skip | sub("\\[bot\\]$"; ""); def theirs: ((if .__typename == "CheckRun" then (.name // "") else (.context // "") end) | . == slug or startswith(slug + "-")); def render: if .__typename == "CheckRun" then "\(.conclusion // .status // "UNKNOWN") \(.workflowName // "") / \(.name // "(unnamed check)")" else "\(.state // "UNKNOWN") \(.context // "status")" end; (.statusCheckRollup // []) as $all | ($all | map(select(theirs | not))) as $kept | (($all | length) - ($kept | length)) as $dropped | if ($kept | length) == 0 then (if $dropped > 0 then "No checks reported. (\($dropped) check(s) from \(slug) are excluded from this block; they carry a verdict from another reviewer, not a CI result.)" else "No checks reported." end) else ($kept | map(render) | sort | join("\n")) end' +# One definition of "belongs to the other reviewer", composed into both programs below rather +# than written into each: they are two programs, and a second copy of the rule is a copy free to +# disagree with the first about what it matches -- which it already did, the failing-job scan +# testing only .name while the list tested .context as well. Composed the way the workflow +# composes CMD_JQ into TOOL_USAGE_JQ. +# +# It reads both names on the entry, not one. For a check the app posts itself, .name is the check +# ("pullfrog", "pullfrog-approval") and there is no workflow behind it; for one that reached the +# rollup from an Actions run, .name is the *job* name and .workflowName is the workflow's `name:`. +# So a name-only test makes this exclusion depend on a job key in another repository staying +# `pullfrog`: `name: Pullfrog` over a job called `review` arrives as +# {name: "review", workflowName: "Pullfrog"} and slips through both programs whole. Dropping a +# check belonging to a workflow named for the other reviewer is the intent in either shape. +# Compared downcased for the same reason -- the slug is lowercase by construction, and a job name +# is whatever someone typed. +CHECK_OWNER_JQ='def slug: $skip | sub("\\[bot\\]$"; "") | ascii_downcase; def theirs: [(if .__typename == "CheckRun" then (.name // "") else (.context // "") end), (.workflowName // "")] | any(. != "" and (ascii_downcase | . == slug or startswith(slug + "-")));' +CHECKS_JQ='def render: if .__typename == "CheckRun" then "\(.conclusion // .status // "UNKNOWN") \(.workflowName // "") / \(.name // "(unnamed check)")" else "\(.state // "UNKNOWN") \(.context // "status")" end; (.statusCheckRollup // []) as $all | ($all | map(select(theirs | not))) as $kept | (($all | length) - ($kept | length)) as $dropped | if ($kept | length) == 0 then (if $dropped > 0 then "No checks reported. (\($dropped) check(s) from \(slug) are excluded from this block; they carry a verdict from another reviewer, not a CI result.)" else "No checks reported." end) else ($kept | map(render) | sort | join("\n")) end' # Actions check runs carry the job id in detailsUrl; scan rather than capture so a # non-Actions check with no job id drops out instead of erroring. -FAILING_JOBS_JQ='def slug: $skip | sub("\\[bot\\]$"; ""); [(.statusCheckRollup // [])[] | select(.__typename == "CheckRun") | select(((.name // "") | . == slug or startswith(slug + "-")) | not) | select((.conclusion // "") | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) | (.detailsUrl // "") | [scan("/job/([0-9]+)")] | flatten | .[0] // empty] | unique | .[0:3] | join(" ")' +FAILING_JOBS_JQ='[(.statusCheckRollup // [])[] | select(.__typename == "CheckRun") | select(theirs | not) | select((.conclusion // "") | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) | (.detailsUrl // "") | [scan("/job/([0-9]+)")] | flatten | .[0] // empty] | unique | .[0:3] | join(" ")' if ROLLUP=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json statusCheckRollup); then - CHECKS=$(printf '%s' "$ROLLUP" | jq --arg skip "$OTHER_REVIEW_BOT" -r "$CHECKS_JQ" 2>/dev/null) \ + CHECKS=$(printf '%s' "$ROLLUP" | jq --arg skip "$OTHER_REVIEW_BOT" -r "$CHECK_OWNER_JQ $CHECKS_JQ" 2>/dev/null) \ || CHECKS="Could not parse checks." - JOB_IDS=$(printf '%s' "$ROLLUP" | jq --arg skip "$OTHER_REVIEW_BOT" -r "$FAILING_JOBS_JQ" 2>/dev/null) || JOB_IDS='' + JOB_IDS=$(printf '%s' "$ROLLUP" | jq --arg skip "$OTHER_REVIEW_BOT" -r "$CHECK_OWNER_JQ $FAILING_JOBS_JQ" 2>/dev/null) || JOB_IDS='' else echo "::warning::Could not read check status." CHECKS="Could not read check status." diff --git a/tests/fixtures/rollup-with-review-bot.json b/tests/fixtures/rollup-with-review-bot.json index 7e8b33c..39e5778 100644 --- a/tests/fixtures/rollup-with-review-bot.json +++ b/tests/fixtures/rollup-with-review-bot.json @@ -30,6 +30,21 @@ "startedAt": "2026-08-28T17:48:02Z", "completedAt": "2026-08-28T17:52:11Z" }, + { + "__typename": "CheckRun", + "name": "review", + "workflowName": "Pullfrog", + "status": "COMPLETED", + "conclusion": "FAILURE", + "detailsUrl": "https://github.com/hotdata-dev/github-workflows/actions/runs/33190500002/job/98914300003", + "startedAt": "2026-08-28T17:48:02Z", + "completedAt": "2026-08-28T17:53:40Z" + }, + { + "__typename": "StatusContext", + "context": "Pullfrog-Approval", + "state": "FAILURE" + }, { "__typename": "StatusContext", "context": "aikido/code-scan", diff --git a/tests/pr-context-test.sh b/tests/pr-context-test.sh index f7b1ded..c0d0177 100755 --- a/tests/pr-context-test.sh +++ b/tests/pr-context-test.sh @@ -21,8 +21,12 @@ cd "$(dirname "$0")/.." COMMITS_JQ=$(extract_jq COMMITS_JQ) FILES_JQ=$(extract_jq FILES_JQ) -CHECKS_JQ=$(extract_jq CHECKS_JQ) -FAILING_JOBS_JQ=$(extract_jq FAILING_JOBS_JQ) +# Both check programs are shipped composed with the shared owner test, the way the workflow +# ships CMD_JQ composed into TOOL_USAGE_JQ -- so they are exercised composed, and a change that +# only holds in one of the two halves has nowhere to hide. +CHECK_OWNER_JQ=$(extract_jq CHECK_OWNER_JQ) +CHECKS_JQ="$CHECK_OWNER_JQ $(extract_jq CHECKS_JQ)" +FAILING_JOBS_JQ="$CHECK_OWNER_JQ $(extract_jq FAILING_JOBS_JQ)" LAST_REVIEW_JQ=$(extract_jq LAST_REVIEW_JQ) COMPARE_STATUS_JQ=$(extract_jq COMPARE_STATUS_JQ) ISSUE_COMMENTS_JQ=$(extract_jq ISSUE_COMMENTS_JQ) @@ -163,6 +167,17 @@ SUCCESS aikido/code-scan" \ expect "$(plain rollup-with-review-bot.json "$FAILING_JOBS_JQ")" "98914277093" \ "the other reviewer's failing check contributes no job log" +# The entry name is not the only place the other reviewer's identity shows up, and on an Actions +# check run it is the *job* name -- so an exclusion testing only that depends on a job key in +# another repository staying `pullfrog`, which nothing here pins. `{name: "review", workflowName: +# "Pullfrog"}` is the ordinary shape of `name: Pullfrog` with a job called review, and the fixture +# carries it alongside a `Pullfrog-Approval` status: both leaks come back whole under a +# name-only, case-sensitive match. +expect "$(plain rollup-with-review-bot.json "$CHECKS_JQ" | grep -ci pullfrog)" "0" \ + "a workflow named for the other reviewer is excluded whatever its job is called" +expect "$(plain rollup-with-review-bot.json "$FAILING_JOBS_JQ" | grep -c '98914300003')" "0" \ + "that job's log is not fetched either" + # Both rollup shapes, because which one an app posts is the app's choice. The slug comes from # $skip -- a GitHub App's login is its slug plus "[bot]" -- so there is no second constant to # keep in step.