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