From 908d3fc6967895a656847ae914b0fa4dd116f76a Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 3 Aug 2026 16:31:33 -0400 Subject: [PATCH] ci: sync Antigravity reviewer to the fixed comment-selection version Bring the agy PR reviewer up to the version already on RustySNES main (via its #270 template-hardening sync + #273 comment-selection fix), superseding the stale sync PR, which carried an earlier version with a real self-deletion bug that agy flagged. What this version adds over the prior sync: - The just-posted review comment's id now comes from the POST itself (`gh pr comment` prints the new comment's URL; its trailing `#issuecomment-` is authoritative), NOT from re-querying the comment list. The re-query raced GitHub's read replication -- right after posting, the list could still omit the new comment, so the "delete all but the newest" exclusion matched nothing and the run deleted the review it had just published (publish-before-delete turning into publish-then-destroy). - SELECT_STALE_JQ: the delete-selection jq filter is now a named, readonly constant (author + id-exclusion selects), exercised directly by a new offline self-test. - scripts/agy-review-selftest.sh: a network-free, gh-free test of that filter against fixtures (the filter has been wrong twice, both times invisibly -- the review still posted, so nothing observed it). Six checks, all passing. - Fail-closed flock (exit rather than run two agy processes unserialized) plus an empty-AGY_LOCK guard. - Keeps the URL-only OAuth-leak guard (oauth_url_present) unchanged. Byte-identical with RustySNES main and the shared reviewer template. bash -n clean; the self-test passes all six checks. Co-Authored-By: Claude Opus 4.8 --- scripts/agy-review-selftest.sh | 135 ++++++++++++++++++++++++++++++++ scripts/agy-review.sh | 138 +++++++++++++++++++++++++-------- 2 files changed, 242 insertions(+), 31 deletions(-) create mode 100644 scripts/agy-review-selftest.sh diff --git a/scripts/agy-review-selftest.sh b/scripts/agy-review-selftest.sh new file mode 100644 index 00000000..db684954 --- /dev/null +++ b/scripts/agy-review-selftest.sh @@ -0,0 +1,135 @@ +#!/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; } + +# 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 <&2 + exit 1 +fi +echo "all checks passed" diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index d2285e73..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 -------------------------- @@ -411,16 +427,30 @@ here="$(cd "$(dirname "$0")" && pwd)" # Serialize agy across concurrent review jobs on this host. agy runs a SINGLETON # local language-server + conversation store per user, so two `--print` calls at # once collide (one reports the backend "unavailable"). flock makes jobs queue -# instead of failing. Best-effort: if the lock can't be taken, proceed anyway. -if command -v flock >/dev/null 2>&1; then - # Create the lock dir first: a failed `exec 9>` redirection is a FATAL shell error (it aborts - # before the `|| log` fallback can run), so ensure the parent exists on a fresh runner. `>>` opens - # for append rather than truncating the lockfile — flock uses the fd, not the contents. - mkdir -p "$(dirname "$AGY_LOCK")" 2>/dev/null || true - exec 9>>"$AGY_LOCK" 2>/dev/null \ - && flock -w "$AGY_LOCK_WAIT" 9 \ - || log "agy lock unavailable or timed out (${AGY_LOCK_WAIT}s); proceeding unserialized" +# instead of failing. FAIL CLOSED: if flock is missing, or the lock can't be +# taken/times out, exit rather than let two agy processes race each other -- +# a fail-open here made the exact collision this lock exists to prevent still +# reachable (one run can burn the whole ${AGY_RETRIES}x${AGY_LOCK_WAIT}s wait). +command -v flock >/dev/null 2>&1 || { + log "flock is required to serialize agy; refusing to run unserialized" + exit 1 +} +# Create the lock dir first: a failed `exec 9>` redirection is a FATAL shell error (it aborts +# before the `|| log` fallback can run), so ensure the parent exists on a fresh runner. `>>` opens +# for append rather than truncating the lockfile — flock uses the fd, not the contents. +# Validated before use: an empty `AGY_LOCK` (an env override set to "") would make `dirname` +# yield "." and the redirection below fail with an obscure shell error, at the one point where a +# clear message matters -- this is the guard that keeps two agy runs off each other. +if [ -z "$AGY_LOCK" ]; then + log "AGY_LOCK is empty; refusing to run unserialized" + exit 1 fi +mkdir -p "$(dirname "$AGY_LOCK")" +exec 9>>"$AGY_LOCK" +flock -w "$AGY_LOCK_WAIT" 9 || { + log "agy lock timed out after ${AGY_LOCK_WAIT}s" + exit 1 +} # Retry the whole agy attempt on empty/failed output: transient "agy is down" # (backend rate-limit / local-server contention) usually clears within seconds. @@ -512,31 +542,77 @@ body_file="$(mktemp)" printf '\n\nAutomated first-pass review by `agy` on a self-hosted runner -- not a human review.\n' } > "$body_file" -# --- replace any prior review comment, then post fresh ------------------------- -# A failed delete is logged, not swallowed: silently ignoring it would let a transient API/perms -# error leave the old comment in place AND post a new one, so runs accumulate duplicates. -# The author filter is load-bearing, not cosmetic: without it, ANY user could put the -# marker (an HTML comment) in a PR comment and have this bot delete arbitrary comments on -# the next run. Only ever delete OUR OWN bot's prior review comments. -gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ - --jq ".[] | select(.user.type == \"Bot\" and .user.login == \"github-actions[bot]\") | select(.body | contains(\"${MARKER}\")) | .id" 2>/dev/null \ - | while read -r cid; do - [ -n "$cid" ] || continue - if ! gh api -X DELETE "repos/${REPO}/issues/comments/${cid}" >/dev/null 2>&1; then - log "warning: could not delete prior review comment ${cid}; a duplicate may result" - fi - done - -# Final hard guard — the last line of defense, and UNCONDITIONAL. Layer 1 (the retry loop) -# already rejects a lapsed-session capture, but a public PR comment must NEVER carry a live -# OAuth authorization URL, whatever any upstream change does to the body — and with no "looks -# like a review" exemption that a header alongside a URL could disarm. A genuine review that -# merely discusses auth or quotes this script's bare regex has no live URL and posts normally; -# only an actual authorization URL blocks the post. +# Final hard guard — the last line of defense, and UNCONDITIONAL, run BEFORE anything is +# deleted or posted. Layer 1 (the retry loop) already rejects a lapsed-session capture, but +# a public PR comment must NEVER carry a live OAuth authorization URL, whatever any upstream +# change does to the body — and with no "looks like a review" exemption that a header +# alongside a URL could disarm. A genuine review that merely discusses auth or quotes this +# script's bare regex has no live URL and posts normally; only an actual authorization URL +# blocks the post. if oauth_url_present "$body_file"; then log "refusing to post: the assembled comment body contains a live OAuth authorization URL. Re-authenticate agy on the runner host." exit 1 fi -gh pr comment "$PR" --repo "$REPO" --body-file "$body_file" +# --- post fresh, THEN replace any prior review comment -------------------------- +# Publish-before-delete, deliberately: if this ordering were reversed and posting failed +# afterward (a transient gh/API error), the PR would be left with NO review comment at all +# instead of the still-valid prior one. Posting first means a failure here can only ever +# leave a harmless duplicate, never a silent loss of the last review. +# The posted comment's id comes from the POST itself, not from a read-back. `gh pr comment` +# prints the new comment's URL, whose trailing `#issuecomment-` is authoritative the instant +# it returns. Re-querying the comment list to find "the newest one with our marker" raced with +# GitHub's own read replication: right after posting, the list can still omit it, and then the +# exclusion below matched nothing and the script deleted the comment it had just published -- +# turning publish-before-delete into publish-then-destroy, the exact failure the ordering exists +# to prevent. +if ! post_output="$(gh pr comment "$PR" --repo "$REPO" --body-file "$body_file" 2>&1)"; then + # Nothing is deleted when the post fails: the prior review comment is still the best + # information the PR has, and removing it would leave no review at all. + log "failed to post review to ${REPO}#${PR}: ${post_output}" + exit 1 +fi log "posted review to ${REPO}#${PR}" +new_comment_id="$(printf '%s\n' "$post_output" | sed -n 's/.*#issuecomment-\([0-9][0-9]*\).*/\1/p' | tail -n 1)" + +# A failed delete is logged, not swallowed: silently ignoring it would let a transient API/perms +# error leave the old comment in place alongside the new one, so runs accumulate duplicates. +# The author filter is load-bearing, not cosmetic: without it, ANY user could put the +# marker (an HTML comment) in a PR comment and have this bot delete arbitrary comments on +# the next run. Only ever delete OUR OWN bot's prior review comments -- and only ones from +# BEFORE this run (the just-posted comment's own id is excluded so it can never delete itself). +if [ -z "$new_comment_id" ]; then + # FAIL CLOSED. Without a known id there is no way to tell the new comment from the old ones, + # and the safe direction is unambiguous: a leftover duplicate is noise, deleting the review + # that was just posted is data loss. + log "warning: could not determine the posted comment id; leaving prior review comments in place" +else + # `--arg`/`--argjson` rather than shell interpolation into the filter: the marker is an HTML + # comment today, but a quote or a backslash in it would otherwise break the jq program itself + # rather than simply not matching. + # + # Those are JQ flags, so the JSON is fetched raw and piped into a real `jq` — `gh api` has no + # `--arg`/`--argjson` of its own and rejects them. Handing them to `gh api --jq` made it exit + # non-zero on every run; with the old `2>/dev/null` swallowing the message and `set -o pipefail` + # in force, the script then died *after* posting, so the stale comments were never deleted and + # the job went red for a reason nothing printed. stderr is kept this time for exactly that + # reason. (`--paginate` without `--jq` emits one JSON array per page; `jq` reads that stream + # 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_STALE_JQ" + )" || { + log "warning: could not list prior review comments; leaving them in place" + stale_ids="" + } + while read -r cid; do + [ -n "$cid" ] || continue + if ! gh api -X DELETE "repos/${REPO}/issues/comments/${cid}" >/dev/null 2>&1; then + log "warning: could not delete prior review comment ${cid}; a duplicate may result" + fi + done <<< "$stale_ids" +fi + +# The delete loop above is the last real work; end on a defined status so a stray non-zero from +# it can never be mistaken for "the review failed" once the comment is already published. +exit 0