Skip to content

Commit d7f8a87

Browse files
zfarrellzfarrell
committed
fix(review): close the remaining exclusion gaps
1 parent e3e6c08 commit d7f8a87

7 files changed

Lines changed: 200 additions & 30 deletions

‎README.md‎

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,21 +82,38 @@ what the other reads as settled prior feedback, not to be re-raised. It also pro
8282
this reviewer already competes for: comment threads may take half of it, and a Pullfrog review
8383
body is a PR summary plus its findings.
8484

85-
The login lives in one constant, `OTHER_REVIEW_BOT`, and reaches three programs as a jq `--arg`.
86-
The third is the review-cycle drift predicate, which asks whether a bot review exists that the
87-
`claude[bot]` filter did not count — true on the first review of every PR in a trial repo, so
88-
without the exclusion it would report a reviewer-identity change that has not happened, on every
89-
pull request, which is how the warning goes unread on the one where it is real.
85+
The login lives in one constant, `OTHER_REVIEW_BOT`, and reaches five programs as a jq `--arg`.
86+
One of them reads nothing out of the prompt at all: the review-cycle drift predicate asks whether
87+
a bot review exists that the `claude[bot]` filter did not count, which is true on the first review
88+
of every PR in a trial repo — so without the exclusion it would report a reviewer-identity change
89+
that has not happened, on every pull request, which is how the warning goes unread on the one
90+
where it is real. The predicate still has to fire when a drifted login sits *beside* the excluded
91+
one, and after the trial that is the only shape a real drift takes, so it is the case the fixtures
92+
pin.
93+
94+
The CI block is the third channel and the one with teeth, so `CHECKS_JQ` and `FAILING_JOBS_JQ`
95+
drop the same reviewer's checks by name — matched against the app slug, since a GitHub App's login
96+
is its slug plus `[bot]`, so `$skip` stays the single constant. Pullfrog's verdict is a check,
97+
`pullfrog-approval`, failing when it requested changes, and this prompt tells the reviewer that a
98+
failing check is a blocking issue to name and cite. Unfiltered, the rollup does not merely leak
99+
the other arm's conclusion; it converts it into a request-changes this reviewer cannot
100+
substantiate from the diff. Its run-status check is worse per byte: the check links to Pullfrog's
101+
own Actions job, and `FAILING_JOBS_JQ` would fetch that job's log into a `### Failing job` excerpt
102+
— the other reviewer's transcript, verbatim, at up to an eighth of the context. That the check
103+
gates no merge is true of merge and silent about the prompt.
90104

91105
Where the exclusion empties a block it says so and how much it withheld, because
92106
"No prior review comments." on a PR that has some is the same false claim as an empty CI block
93-
reading as a green one. Where other comments remain it stays silent: the block claims nothing
94-
about being every comment on the PR.
95-
96-
Pullfrog's own review verdict is a check, `pullfrog-approval`, reporting whether it *would*
97-
approve. It is not required in the org ruleset and Pullfrog cannot submit an approving review, so
98-
this reviewer remains the only automated approval in the org for the duration of the trial. The
99-
constant, and this section, come out when the trial ends.
107+
reading as a green one. Where other entries remain it stays silent: neither block claims to be
108+
every comment or every check on the PR. One rendering changes with it. A human reply to an
109+
excluded comment is kept — real feedback, and dropping it would hide more than the exclusion
110+
protects — but its parent is gone, and an id pointing at nothing in the block turns "fixed in the
111+
next push" into a settled finding whose subject the reviewer never sees. So an orphaned reply is
112+
relabelled rather than renumbered or dropped.
113+
114+
Pullfrog cannot submit an approving review and `pullfrog-approval` is not required in the org
115+
ruleset, so this reviewer remains the only automated approval in the org for the duration of the
116+
trial. The constant, the four exclusions, and this section come out when the trial ends.
100117

101118
### Tool usage artifact
102119

‎scripts/gather-review-context.sh‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,7 @@ fi
385385
# Single-line and single-quoted so tests/lib.sh can extract it; it was inline, and inline
386386
# meant the one program in this script that shapes the prompt's other output could not be
387387
# asserted against a fixture at all.
388-
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'
388+
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'
389389
if [ "$COMMENTS_OK" -eq 0 ]; then
390390
THREADS='Unavailable: the prior inline review comments could not be read. This block is empty because the fetch failed, not because there were none.'
391391
else
@@ -549,14 +549,33 @@ cap_file_escaped "$FILES_FILE" $((CTX_MAX_BYTES / 8)) \
549549
# The reviewer cannot run tests -- no dependencies are installed and the allowlist
550550
# would refuse anyway -- but CI already ran them. Whether they passed is the one
551551
# fact it was asserting without evidence.
552-
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'
552+
#
553+
# This block is the third channel $skip reaches, and the one where it does the most damage.
554+
# Pullfrog posts its verdict as a check -- `pullfrog-approval`, failing when it requested
555+
# changes -- and the prompt tells this reviewer that a failing check is a blocking issue to
556+
# name and cite. So an unfiltered rollup does not merely leak the other arm's conclusion; it
557+
# converts it into a request-changes this reviewer cannot substantiate from the diff.
558+
# `pullfrog` (the run-status check) rides along for the same reason, and its detailsUrl would
559+
# otherwise feed FAILING_JOBS_JQ below the *other reviewer's own job log* as a failing-job
560+
# excerpt, at up to an eighth of the context.
561+
#
562+
# Matched by name against the app slug rather than by a second constant: a GitHub App's bot
563+
# login is its slug plus "[bot]", and its checks are the slug and slug-prefixed names, so
564+
# $skip still carries the one value all five programs agree on. Both rollup shapes are
565+
# matched -- a CheckRun by .name, a StatusContext by .context -- because which of the two an
566+
# app posts is the app's choice, not ours.
567+
#
568+
# Silent while other checks remain, and stated when the exclusion empties the block: a list
569+
# claims nothing about being every check, but "No checks reported." on a PR that has some is
570+
# a false claim, and this is the block whose emptiness the README warns gets read as green.
571+
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'
553572
# Actions check runs carry the job id in detailsUrl; scan rather than capture so a
554573
# non-Actions check with no job id drops out instead of erroring.
555-
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(" ")'
574+
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(" ")'
556575
if ROLLUP=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json statusCheckRollup); then
557-
CHECKS=$(printf '%s' "$ROLLUP" | jq -r "$CHECKS_JQ" 2>/dev/null) \
576+
CHECKS=$(printf '%s' "$ROLLUP" | jq --arg skip "$OTHER_REVIEW_BOT" -r "$CHECKS_JQ" 2>/dev/null) \
558577
|| CHECKS="Could not parse checks."
559-
JOB_IDS=$(printf '%s' "$ROLLUP" | jq -r "$FAILING_JOBS_JQ" 2>/dev/null) || JOB_IDS=''
578+
JOB_IDS=$(printf '%s' "$ROLLUP" | jq --arg skip "$OTHER_REVIEW_BOT" -r "$FAILING_JOBS_JQ" 2>/dev/null) || JOB_IDS=''
560579
else
561580
echo "::warning::Could not read check status."
562581
CHECKS="Could not read check status."

‎tests/fixtures/pull-comments-mixed-authors.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,15 @@
2424
"created_at": "2026-08-27T09:04:00Z",
2525
"body": "## Summary\n\nThis PR reworks the context budget. One finding: the byte cap and the line cap disagree."
2626
},
27+
{
28+
"id": 2105,
29+
"user": { "login": "zfarrell" },
30+
"path": "scripts/gather-review-context.sh",
31+
"line": 88,
32+
"in_reply_to_id": 2103,
33+
"created_at": "2026-08-27T09:40:00Z",
34+
"body": "Good catch -- fixed in the next push."
35+
},
2736
{
2837
"id": 2104,
2938
"user": { "login": "claude[bot]" },

‎tests/fixtures/reviews-drift-with-other-bot.json‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
[
2+
{
3+
"id": 1,
4+
"commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
5+
"state": "COMMENTED",
6+
"submitted_at": "2026-08-28T00:00:00Z",
7+
"user": {
8+
"login": "pullfrog[bot]",
9+
"type": "Bot"
10+
}
11+
},
12+
{
13+
"id": 2,
14+
"commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
15+
"state": "CHANGES_REQUESTED",
16+
"submitted_at": "2026-08-28T00:01:00Z",
17+
"user": {
18+
"login": "claude-review[bot]",
19+
"type": "Bot"
20+
}
21+
},
22+
{
23+
"id": 3,
24+
"commit_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
25+
"state": "APPROVED",
26+
"submitted_at": "2026-08-28T01:00:00Z",
27+
"user": {
28+
"login": "claude-review[bot]",
29+
"type": "Bot"
30+
}
31+
}
32+
]

‎tests/fixtures/rollup-with-review-bot.json‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
{
2+
"statusCheckRollup": [
3+
{
4+
"__typename": "CheckRun",
5+
"name": "test",
6+
"workflowName": "Tests",
7+
"status": "COMPLETED",
8+
"conclusion": "FAILURE",
9+
"detailsUrl": "https://github.com/hotdata-dev/github-workflows/actions/runs/33190449262/job/98914277093",
10+
"startedAt": "2026-08-28T17:47:38Z",
11+
"completedAt": "2026-08-28T17:51:24Z"
12+
},
13+
{
14+
"__typename": "CheckRun",
15+
"name": "pullfrog",
16+
"workflowName": "Pullfrog",
17+
"status": "IN_PROGRESS",
18+
"conclusion": null,
19+
"detailsUrl": "https://github.com/hotdata-dev/github-workflows/actions/runs/33190500001/job/98914300001",
20+
"startedAt": "2026-08-28T17:48:02Z",
21+
"completedAt": null
22+
},
23+
{
24+
"__typename": "CheckRun",
25+
"name": "pullfrog-approval",
26+
"workflowName": "Pullfrog",
27+
"status": "COMPLETED",
28+
"conclusion": "FAILURE",
29+
"detailsUrl": "https://github.com/hotdata-dev/github-workflows/actions/runs/33190500001/job/98914300002",
30+
"startedAt": "2026-08-28T17:48:02Z",
31+
"completedAt": "2026-08-28T17:52:11Z"
32+
},
33+
{
34+
"__typename": "StatusContext",
35+
"context": "aikido/code-scan",
36+
"state": "SUCCESS"
37+
}
38+
]
39+
}

0 commit comments

Comments
 (0)