From 4c23da1d0192228488c530e2170d1d5af5d4a075 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 30 Jul 2026 19:31:19 -0400 Subject: [PATCH 1/2] test(reviewer): guard the comment-selection filter that has broken twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `agy-review.sh`'s filter decides which PR comments the bot DELETES. It has been wrong twice, and neither time was observable from the outside — the review posted fine on both occasions: 1. The just-posted comment was not reliably excluded. `new_comment_id` came from re-querying the comment list, which races GitHub's read replication; on a miss the exclusion degenerated to `select(.id != null)` — true for every id — and the run deleted the review it had just published. 2. jq's `--arg`/`--argjson` were passed to `gh api`, which has no such flags. It exited non-zero, `2>/dev/null` hid the message, and `set -o pipefail` plus `set -e` killed the script AFTER posting. Stale comments silently accumulated (PR #272 reached four) and the job went red with nothing in the log to say why. Both bugs lived in the same ~10 lines, and the second was mine. - The filter moves into a named `SELECT_STALE_JQ` so a test can exercise the real thing rather than a copy that can drift. - `scripts/agy-review-selftest.sh` lifts `MARKER` and that filter out of the script by pattern and runs them against fixtures — offline, no `gh`, no network, no self-hosted runner. Lifting by pattern is deliberate: rename or reshape either declaration and the test fails loudly instead of quietly checking something stale. - Six checks: the just-posted comment is never selected; other users and other bots are ignored (the author filter is a security control — without it anyone could paste the marker into a comment and have the bot delete comments on the next run); bot comments without the marker are ignored; an id of 0 excludes nothing real; the exclusion is genuinely by value; and `--arg`/`--argjson` are not passed to `gh api`. - Wired into the `lint` job, so it runs on every PR rather than only where `agy` is installed. Verified by injecting each historical regression and confirming the suite fails at the matching check and exits 1: dropping the id exclusion fails "excludes whichever id it is given"; moving `--arg` back onto `gh api` fails the flag check. Healthy tree exits 0. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/ci.yml | 5 ++ scripts/agy-review-selftest.sh | 107 +++++++++++++++++++++++++++++++++ scripts/agy-review.sh | 19 +++++- 3 files changed, 129 insertions(+), 2 deletions(-) create mode 100755 scripts/agy-review-selftest.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8df9f9c2..0ce9fab2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -98,6 +98,11 @@ jobs: components: rustfmt, clippy linux-frontend-deps: "true" - run: cargo fmt --all --check + # The reviewer's comment-selection filter decides which PR comments the bot DELETES, and + # it has been wrong twice in ways nothing observed (it posted its review fine both times). + # This check is offline — fixtures and `jq`, no network, no `gh`, no self-hosted runner — + # so it runs here on every PR rather than only where `agy` is installed. + - run: bash scripts/agy-review-selftest.sh # `--exclude rustysnes-android` (`v1.15.0 "Sideload"`): that crate's `ndk-sys` dependency # hard-fails (`compile_error!`) on every non-Android target, unconditionally -- it's not # feature-gated, so no `--features`/`--no-default-features` combination avoids it. Its own diff --git a/scripts/agy-review-selftest.sh b/scripts/agy-review-selftest.sh new file mode 100755 index 00000000..42ba89c9 --- /dev/null +++ b/scripts/agy-review-selftest.sh @@ -0,0 +1,107 @@ +#!/usr/bin/env bash +# +# agy-review-selftest.sh -- guards the comment-selection logic in `agy-review.sh`. +# +# Why this exists: that filter decides which PR comments the bot DELETES, and it has been wrong +# twice, both times invisibly. +# +# 1. The just-posted comment was not reliably excluded. `new_comment_id` came from re-querying +# the comment list, which races GitHub's read replication; on a miss the exclusion became +# `select(.id != null)`, true for every id, and the run deleted the review it had just +# published. +# 2. jq's `--arg`/`--argjson` were handed to `gh api`, which has no such flags. It exited +# non-zero, `2>/dev/null` hid the message, and `set -o pipefail` + `set -e` killed the script +# AFTER posting — so stale comments silently accumulated and the job went red with nothing in +# the log explaining why. +# +# Neither was catchable by looking at the review the bot posted: both times it posted fine. So the +# filter is tested here directly, offline, against fixtures — no network, no `gh`, no runner. +# +# Run: bash scripts/agy-review-selftest.sh + +set -euo pipefail + +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" + +# Source the constants out of the reviewer without running it. `agy-review.sh` does its work at +# top level, so it cannot simply be sourced; the two values under test are lifted by pattern +# instead. That coupling is deliberate: if either declaration is renamed or reshaped, this test +# fails loudly rather than silently checking a stale copy of the filter. +extract_marker() { + sed -n 's/^MARKER="\(.*\)"$/\1/p' "$SCRIPT_DIR/agy-review.sh" | head -n 1 +} +extract_filter() { + sed -n "/^SELECT_STALE_JQ='/,/'\$/p" "$SCRIPT_DIR/agy-review.sh" \ + | sed "1s/^SELECT_STALE_JQ='//; \$s/'\$//" +} + +MARKER="$(extract_marker)" +FILTER="$(extract_filter)" + +[ -n "$MARKER" ] || { echo "FAIL: could not extract MARKER from agy-review.sh" >&2; exit 1; } +[ -n "$FILTER" ] || { echo "FAIL: could not extract SELECT_STALE_JQ from agy-review.sh" >&2; exit 1; } + +fixture() { + cat </dev/null 2>&1; then + echo " FAIL --arg/--argjson passed to \`gh api\` (jq flags; gh api rejects them)" + fails=$((fails + 1)) +else + echo " ok --arg/--argjson are not passed to \`gh api\`" +fi + +if [ "$fails" -ne 0 ]; then + echo "$fails check(s) failed" >&2 + exit 1 +fi +echo "all checks passed" diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 4a300629..9b273dea 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -100,6 +100,22 @@ AGY_RETRIES="${AGY_RETRIES:-3}" # attempts to get a usable agy respon AGY_RETRY_DELAY="${AGY_RETRY_DELAY:-15}" # base backoff seconds between retries (grows per attempt) MARKER="" +# The jq program that picks which prior review comments to delete. Named, and exercised directly +# by `scripts/agy-review-selftest.sh`, because this filter has now been wrong TWICE in ways +# nothing observed: first the just-posted comment was not excluded (so it deleted itself), then +# jq's `--arg` was handed to `gh api`, which has no such flag (so the whole step died silently and +# stale comments accumulated). Both were invisible from the outside — the review still posted. +# +# The two `select`s that matter: the AUTHOR filter (without it, any user could put the marker in a +# comment and have this bot delete arbitrary comments) and the ID exclusion (without it, the run +# deletes the comment it just published). +SELECT_STALE_JQ='.[] + | select(.user.type == "Bot" and .user.login == "github-actions[bot]") + | select(.body | contains($marker)) + | select(.id != $new_id) + | .id' +readonly SELECT_STALE_JQ + REPO="${GITHUB_REPOSITORY:?GITHUB_REPOSITORY not set}" # --- resolve the PR number from the triggering event -------------------------- @@ -584,8 +600,7 @@ else # fine, applying `.[]` to each.) stale_ids="$( gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ - | jq -r --arg marker "$MARKER" --argjson new_id "$new_comment_id" \ - '.[] | select(.user.type == "Bot" and .user.login == "github-actions[bot]") | select(.body | contains($marker)) | select(.id != $new_id) | .id' + | jq -r --arg marker "$MARKER" --argjson new_id "$new_comment_id" "$SELECT_STALE_JQ" )" || { log "warning: could not list prior review comments; leaving them in place" stale_ids="" From ce99537997cc2a5a1c19ee34be87f1e64b0205b6 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 30 Jul 2026 19:35:13 -0400 Subject: [PATCH 2/2] test(reviewer): make the self-test's own extraction fail loudly, not silently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for #273. All three findings describe silent-failure modes in a test whose entire premise is that silent failure is what went wrong twice — so all three are taken. - The `sed` range that lifts `SELECT_STALE_JQ` ends at the first line closing with a quote, so a filter body that ever did so would be TRUNCATED — and a truncated jq program can still compile and still return ids, i.e. the test would keep passing while checking something that is not the shipped filter. Two independent guards now: the extracted program must compile, and it must end with the `| .id` projection that makes it a complete pipeline rather than a prefix of one. The compile check passes `--arg marker` / `--argjson new_id` itself: the filter references both, and jq rejects an undefined variable at compile time, so omitting them fails a perfectly good program. Caught by running it. - The `gh api ... --arg` scan was line-based, so moving the flag onto a continuation line would have produced a false pass on precisely the mistake the check exists to catch. Continuations are folded before matching. - `tr '\n' ' '` left a trailing space that every expected value had to mirror. `paste -sd' '` instead, and the expectations now read as what they are. Verified by injecting each new failure mode: dropping the final `| .id` trips the truncation guard; putting `--arg` on a continuation line trips the flag check. Both exit 1. Healthy tree passes all six checks and exits 0. shellcheck clean. Co-Authored-By: Claude Opus 4.8 --- scripts/agy-review-selftest.sh | 42 ++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/scripts/agy-review-selftest.sh b/scripts/agy-review-selftest.sh index 42ba89c9..db684954 100755 --- a/scripts/agy-review-selftest.sh +++ b/scripts/agy-review-selftest.sh @@ -41,6 +41,28 @@ FILTER="$(extract_filter)" [ -n "$MARKER" ] || { echo "FAIL: could not extract MARKER from agy-review.sh" >&2; exit 1; } [ -n "$FILTER" ] || { echo "FAIL: could not extract SELECT_STALE_JQ from agy-review.sh" >&2; exit 1; } +# A non-empty extraction is not the same as a COMPLETE one. The `sed` range above ends at the +# first line closing with a quote, so a filter whose body ever ends a line that way would be +# truncated — and a truncated jq program can still be valid and still return ids, which is the +# silent-wrong-answer this whole file exists to prevent. Two independent guards: +# +# 1. it must compile (a truncated program is usually, though not always, a syntax error); +# 2. it must END with the projection, which is what makes it a complete pipeline rather than a +# prefix of one. +# The named args must be supplied here too: the filter references `$marker`/`$new_id`, and jq +# rejects an undefined variable at COMPILE time — so omitting them fails a perfectly good program. +if ! printf '[]' | jq --arg marker x --argjson new_id 0 "$FILTER" >/dev/null 2>&1; then + echo "FAIL: extracted SELECT_STALE_JQ is not a valid jq program (truncated?):" >&2 + printf '%s\n' "$FILTER" >&2 + exit 1 +fi +case "$(printf '%s' "$FILTER" | tr -d '[:space:]')" in + *'|.id') : ;; + *) echo "FAIL: extracted SELECT_STALE_JQ does not end in '| .id'; extraction truncated" >&2 + printf '%s\n' "$FILTER" >&2 + exit 1 ;; +esac + fixture() { cat </dev/null 2>&1; then +# Line continuations are folded first: `--arg` moved onto a continuation line would otherwise sit +# on a different physical line from `gh api`, and a line-by-line grep would report a false pass on +# exactly the mistake this check exists to catch. +if sed -e ':a' -e '/\\$/{N;s/\\\n//;ba' -e '}' "$SCRIPT_DIR/agy-review.sh" \ + | grep -qE 'gh api[^|]*--(arg|argjson)'; then echo " FAIL --arg/--argjson passed to \`gh api\` (jq flags; gh api rejects them)" fails=$((fails + 1)) else