fix(review): match the other reviewer by workflow too

Author: zfarrell

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
 

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."

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",

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.