diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml new file mode 100644 index 00000000..06b9ec70 --- /dev/null +++ b/.github/actionlint.yaml @@ -0,0 +1,25 @@ +# actionlint configuration. +# +# actionlint validates `runs-on:` against GitHub's list of hosted-runner labels +# and reports anything else as unknown, because a typo'd label is a job that +# queues forever rather than failing. Self-hosted labels therefore have to be +# declared here — that is the mechanism actionlint's own diagnostic points at. +# +# Without this file, `actionlint` reports an error on EVERY run in a repo that +# has the reviewer installed. That is not a harmless false positive: a linter +# that always reports something is a linter people stop reading, and the real +# findings go with it. +# +# Run it over the whole tree with: +# +# actionlint +# +# (no arguments — it discovers `.github/workflows/*.yml` and reads this file.) + +self-hosted-runner: + labels: + # The maintainer's self-hosted runner for the Antigravity PR reviewer + # (`.github/workflows/antigravity-review.yml`, `runs-on: [self-hosted, agy]`). + # It is a personal machine holding the `agy` CLI's Google AI Ultra OAuth + # session, which is why that workflow cannot run on a hosted runner. + - agy diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml index fa3d9d0a..4a5abbbc 100644 --- a/.github/workflows/antigravity-review.yml +++ b/.github/workflows/antigravity-review.yml @@ -53,6 +53,10 @@ jobs: group: agy-review-${{ github.event.pull_request.number || github.event.issue.number }} cancel-in-progress: false runs-on: [self-hosted, agy] + # Bounded harder than the hosted jobs, not softer: this runs on the + # maintainer's own hardware, so a hung run holds a real machine rather than + # a disposable VM. Observed runtime is 1-3 minutes. + timeout-minutes: 30 steps: # Check out the DEFAULT BRANCH, never the PR head. This job runs the checked-out # `scripts/agy-review.sh` on a self-hosted runner with a token in the environment, @@ -63,7 +67,13 @@ jobs: # branch throughout. Consequence worth knowing: a PR that edits the reviewer or the # style guide is reviewed by the version already on the default branch until it merges. - name: Check out repo (for the style guide + scripts) - uses: actions/checkout@v7 + # SHA-pinned, not `@v7`: this job runs on a SELF-HOSTED runner -- the + # maintainer's own machine -- so a compromised tag would execute there + # rather than in a disposable VM. Verified to be exactly what `v7` + # resolves to (v7.0.1, 2026-07-17). The trailing `# v7` is the form + # Dependabot's github-actions ecosystem reads to keep the pin current. + # Adopted FROM RustySNES and SLAC, which had it while the template did not. + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: ref: ${{ github.event.repository.default_branch }} # Not `fetch-depth: 0`: the large-diff fallback fetches exactly the two refs it @@ -91,5 +101,21 @@ jobs: # MAX_PROMPT_BYTES: "125000" # inline/file threshold + hard backstop on the argv prompt # STYLE_GUIDE: .github/agy-review.md # style guide, loaded if present run: | + # The workflow and the scripts come from DIFFERENT REFS: for a `pull_request` + # event GitHub runs this YAML from the PR branch, while the checkout step + # above deliberately fetches the DEFAULT branch to get the scripts. So a + # change that adds a script file breaks its own PR -- the new workflow + # chmods a file the default branch does not have yet. Observed exactly + # once, on the PR that introduced `_agy_comment_body.sh`. + # + # The two required files are chmod'd unconditionally; anything added later + # is chmod'd only if present, so the workflow stays compatible with both + # the old and the new script set. A genuinely missing required file still + # fails loudly -- `agy-review.sh` sources it and dies -- rather than being + # papered over here. chmod +x scripts/agy-review.sh scripts/_agy_print.sh + for opt in scripts/_agy_comment_body.sh; do + [ -f "$opt" ] && chmod +x "$opt" + done + true scripts/agy-review.sh diff --git a/scripts/_agy_comment_body.sh b/scripts/_agy_comment_body.sh new file mode 100755 index 00000000..5668f9ce --- /dev/null +++ b/scripts/_agy_comment_body.sh @@ -0,0 +1,75 @@ +#!/usr/bin/env bash +# +# _agy_comment_body.sh -- the review comment's body format, as sourceable functions. +# +# Sourced by `agy-review.sh` (which uses them) and by `agy-review-selftest.sh` (which tests +# them). It exists as a separate file for one reason: `agy-review.sh` does its work at TOP +# LEVEL, so it cannot be sourced without running a review, and a test that cannot call the +# real implementation ends up reimplementing it. That failure is not hypothetical here -- the +# first version of the archive test inlined its own copy of the `awk` pipeline, so a mutation +# deleting the marker strip from the script came back NOT CAUGHT. A test that reimplements its +# subject agrees with itself forever. +# +# Defines no state and runs nothing on source. + +# The sentinels delimiting the archive of earlier review rounds inside the comment body. +# +# The reviewer used to POST a fresh comment each round and DELETE the previous one. That kept +# the PR tidy and destroyed the record: a round nobody read before the next push was gone, with +# nothing on the PR indicating it had existed -- and unlike a CodeRabbit or Copilot thread, an +# unaddressed finding left no trace. Observed on a real PR where two consecutive rounds each +# raised a blocking issue and only the second survived. +# +# Now there is ONE comment per PR, edited in place: newest round on top, earlier rounds folded +# into a collapsed `
` below. Same tidiness, no destruction. +AGY_ARCHIVE_START='' +AGY_ARCHIVE_END='' + +# The newest round of a comment body: everything after the marker, before the archive. +# +# `awk` matching WHOLE LINES rather than `sed` with a pattern, because a review body +# legitimately contains regex metacharacters, backslashes and HTML, and the sentinels must not +# match a line that merely mentions one. +agy_body_head() { + local marker="$1" + awk -v s="$AGY_ARCHIVE_START" '$0 == s { exit } { print }' \ + | grep -v -F -x "$marker" || true +} + +# The archived rounds of a comment body: everything strictly between the sentinels. +# +# The `$0 == e` test comes FIRST so a body whose archive is empty yields nothing rather than +# emitting its own end sentinel. +agy_body_archive() { + awk -v s="$AGY_ARCHIVE_START" -v e="$AGY_ARCHIVE_END" ' + $0 == e { inside = 0 } + inside { print } + $0 == s { inside = 1 }' || true +} + +# Delimits one archived round. Emitted by the caller ahead of each round's `
`. +# +# A dedicated sentinel, NOT the `
` tag itself. Matching `/^
$/` looked +# equivalent and was a data-corruption bug: a review body legitimately contains `
` +# blocks -- folded logs, collapsed code, another bot's summary, and the archived rounds are +# themselves nested `
` -- so the cut could land INSIDE a round and leave torn HTML +# plus half a review. The sentinel is an HTML comment, so it is invisible when rendered and +# cannot occur by accident in prose the way a tag can. Found in review. +AGY_ROUND_MARK='' + +# Drop the OLDEST archived round -- everything from the last round marker onward. +# +# Exits non-zero when there is no marker to cut at, so a caller trimming to a size limit +# terminates rather than spinning. That is also the fail-safe direction for an archive written +# by an older version of this script: with no markers present, nothing is dropped and the edit +# simply fails on size, rather than the archive being silently mangled. +agy_drop_oldest_round() { + awk -v m="$AGY_ROUND_MARK" ' + $0 == m { starts[++n] = NR } + { line[NR] = $0 } + END { + cut = (n > 0) ? starts[n] : 0 + if (cut == 0) exit 1 + for (i = 1; i < cut; i++) print line[i] + }' +} diff --git a/scripts/agy-review-selftest.sh b/scripts/agy-review-selftest.sh old mode 100644 new mode 100755 index db684954..491f8a5f --- a/scripts/agy-review-selftest.sh +++ b/scripts/agy-review-selftest.sh @@ -30,16 +30,56 @@ SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" extract_marker() { sed -n 's/^MARKER="\(.*\)"$/\1/p' "$SCRIPT_DIR/agy-review.sh" | head -n 1 } +# Extraction is delimited by explicit `SELFTEST-EXTRACT` markers rather than by matching the +# declaration's own syntax. Adopted FROM SLAC, and it is the better mechanism: the `sed` range +# it replaced ended at the first line closing with a quote, so a filter whose body ever ended a +# line that way would be silently TRUNCATED -- and a truncated jq program can still compile and +# still return ids, which is exactly the silent-wrong-answer this file exists to prevent. +# Markers also let a guard be several statements rather than one assignment, which is what makes +# the OAuth and service-error guards testable at all. +extract_block() { + sed -n "/^# >>> SELFTEST-EXTRACT: $1\$/,/^# <<< SELFTEST-EXTRACT\$/p" "$SCRIPT_DIR/agy-review.sh" +} extract_filter() { - sed -n "/^SELECT_STALE_JQ='/,/'\$/p" "$SCRIPT_DIR/agy-review.sh" \ - | sed "1s/^SELECT_STALE_JQ='//; \$s/'\$//" + extract_block "ours-comment filter" \ + | sed -n "/^SELECT_OURS_JQ='/,/'\$/p" \ + | sed "1s/^SELECT_OURS_JQ='//; \$s/'\$//" } +# Every marked block must exist and be sourceable. A renamed or unbalanced marker would +# otherwise extract EMPTY, and an empty guard sources fine and asserts nothing -- the same +# absence-reads-as-agreement failure the markers were adopted to prevent. +for guard in "service-error guard" "oauth guard" "ours-comment filter"; do + blk="$(extract_block "$guard")" + [ -n "$blk" ] || { echo "FAIL: SELFTEST-EXTRACT block '$guard' is missing or empty" >&2; exit 1; } + printf '%s\n' "$blk" | bash -n - 2>/dev/null \ + || { echo "FAIL: extracted block '$guard' is not valid shell (unbalanced markers?)" >&2; exit 1; } + eval "$blk" +done + 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; } +[ -n "$FILTER" ] || { echo "FAIL: could not extract SELECT_OURS_JQ from agy-review.sh" >&2; exit 1; } + +# The REAL body-format implementation, sourced rather than reimplemented. The first version of +# these checks inlined its own copy of the `awk` pipeline, and a mutation deleting the marker +# strip from the script came back NOT CAUGHT -- a test that reimplements its subject agrees with +# itself forever. `agy-review.sh` cannot be sourced (it works at top level), which is exactly +# why the format lives in its own file. +# shellcheck source=scripts/_agy_comment_body.sh +. "$SCRIPT_DIR/_agy_comment_body.sh" +[ -n "${AGY_ARCHIVE_START:-}" ] || { echo "FAIL: _agy_comment_body.sh defined no AGY_ARCHIVE_START" >&2; exit 1; } +[ -n "${AGY_ARCHIVE_END:-}" ] || { echo "FAIL: _agy_comment_body.sh defined no AGY_ARCHIVE_END" >&2; exit 1; } + +# The script must actually USE the shared helper, or these checks test a file nothing runs. +grep -q '_agy_comment_body.sh' "$SCRIPT_DIR/agy-review.sh" \ + || { echo "FAIL: agy-review.sh does not source _agy_comment_body.sh" >&2; exit 1; } +for fn in agy_body_head agy_body_archive agy_drop_oldest_round; do + grep -q "$fn" "$SCRIPT_DIR/agy-review.sh" \ + || { echo "FAIL: agy-review.sh does not call $fn" >&2; exit 1; } +done # 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 @@ -51,14 +91,14 @@ FILTER="$(extract_filter)" # 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 +if ! printf '[]' | jq --arg marker x "$FILTER" >/dev/null 2>&1; then + echo "FAIL: extracted SELECT_OURS_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 + *'|.id//empty') : ;; + *) echo "FAIL: extracted SELECT_OURS_JQ does not end in '| .id // empty'; extraction truncated" >&2 printf '%s\n' "$FILTER" >&2 exit 1 ;; esac @@ -66,20 +106,19 @@ esac fixture() { cat <\nOLDER ROUND\n
')" "$archive_part" + +# A body with NO archive yet (the first round) must split to a head and an empty archive, not to +# an empty head -- the first re-review is exactly when this path runs for the first time. +first_body="$(printf '%s\nFIRST ROUND\n' "$MARKER")" +check "a body with no archive still yields its head" "FIRST ROUND" \ + "$(printf '%s\n' "$first_body" | agy_body_head "$MARKER")" +check "a body with no archive yields an empty archive" "" \ + "$(printf '%s\n' "$first_body" | agy_body_archive)" + +# The head must NOT carry the marker forward: an archived round that still contains it would +# make every future run's `contains($marker)` match inside the archive, and the split would +# then cut at the wrong place. A mutation removing the strip must fail here. +check "the head never carries the marker into the archive" "" \ + "$(printf '%s\nX\n' "$MARKER" | agy_body_head "$MARKER" | grep -F -x "$MARKER" || true)" + +# Trimming must terminate: with no `
` left, dropping fails rather than looping. +check "dropping from an empty archive fails rather than spinning" "1" \ + "$(printf 'no rounds here\n' | agy_drop_oldest_round >/dev/null 2>&1; echo $?)" + +# An archive written by an OLDER version has no round markers. Nothing must be dropped: +# the edit then fails on size, which is recoverable, rather than the archive being mangled. +check "an unmarked (legacy) archive drops nothing" "1" \ + "$(printf '
\nNEW\n
\n
\nOLD\n
\n' \ + | agy_drop_oldest_round >/dev/null 2>&1; echo $?)" + +marked_archive="$(printf '%s\n
\nNEW\n
\n%s\n
\nOLD\n
\n' \ + "$AGY_ROUND_MARK" "$AGY_ROUND_MARK")" +check "dropping removes the OLDEST round, keeping the newest" \ + "$(printf '%s\n
\nNEW\n
' "$AGY_ROUND_MARK")" \ + "$(printf '%s\n' "$marked_archive" | agy_drop_oldest_round)" -# Regression #1, pinned: an unknown id must not select everything. The caller now refuses to run -# the delete at all in this case, but the filter itself is checked so the two guards are -# independent rather than one relying on the other. -check "an id of 0 still excludes nothing real" "111 222 999" "$(select_ids 0)" +# THE BUG THIS SENTINEL EXISTS FOR. A review body legitimately contains `
` blocks +# -- folded logs, collapsed code, another bot's summary -- and matching the tag itself cut +# INSIDE a round, leaving torn HTML and half a review. The newest round below carries its own +# `
`; dropping the oldest must not touch it. +nested="$(printf '%s\n
\nRound\n
\nfolded log\n
\nfinding\n
\n%s\n
\nOLD\n
\n' \ + "$AGY_ROUND_MARK" "$AGY_ROUND_MARK")" +check "a
INSIDE a round is not mistaken for a round boundary" \ + "$(printf '%s\n
\nRound\n
\nfolded log\n
\nfinding\n
' "$AGY_ROUND_MARK")" \ + "$(printf '%s\n' "$nested" | agy_drop_oldest_round)" -# A different id in the set behaves the same way, so the exclusion is genuinely by value. -check "excludes whichever id it is given" "222 999" "$(select_ids 111)" +# The writer must actually EMIT the sentinel, or every archive is legacy-shaped and the trim +# silently never fires -- the comment would then grow until the edit fails. +if grep -q 'AGY_ROUND_MARK' "$SCRIPT_DIR/agy-review.sh"; then + echo " ok agy-review.sh emits the round sentinel" +else + echo " FAIL agy-review.sh never emits AGY_ROUND_MARK; the archive would never trim" + fails=$((fails + 1)) +fi + +# The script must not delete comments any more. A reintroduced DELETE is the regression that +# would silently restore the destructive behavior this design replaced. +if grep -qE 'gh api +-X +DELETE' "$SCRIPT_DIR/agy-review.sh"; then + echo " FAIL agy-review.sh deletes comments again; the archive design forbids it" + fails=$((fails + 1)) +else + echo " ok agy-review.sh never deletes a comment" +fi # Regression #2, pinned: `--arg`/`--argjson` belong to jq. If they are ever moved onto `gh api` # again, that command exits non-zero — assert the flags are not passed to `gh api` in the script. @@ -128,6 +242,54 @@ else echo " ok --arg/--argjson are not passed to \`gh api\`" fi +# --- the service-error guard --------------------------------------------------- +# When agy's upstream is down it prints an error, not a review. That text is non-empty, so +# `have_text` alone treats it as a valid review and POSTs it -- a green check for a review that +# never ran. Observed twice on SLAC PR #14, where the check passed in 7 seconds with the error +# string as its entire body. A control that cannot fail is worse than no control. +se() { printf '%s' "$1" > "$TMPD/cap"; service_error_present "$TMPD/cap" && echo MATCH || echo NOMATCH; } +TMPD="$(mktemp -d)" +trap 'rm -rf "$TMPD"' EXIT + +check "a bare backend 503 is caught" "MATCH" \ + "$(se 'Error: Eligibility check failed: UNAVAILABLE (code 503): The service is currently unavailable.')" +check "RESOURCE_EXHAUSTED is caught" "MATCH" "$(se 'Error: RESOURCE_EXHAUSTED')" + +# The false positives the anchoring exists to avoid. A genuine review may quote a 503 while +# reviewing retry logic, and aborting on that would be the very failure the OAuth guard's design +# notes warn about. +check "a review DISCUSSING a 503 is not caught" "NOMATCH" \ + "$(se "$(printf '## Review\n\nThe retry path should handle Error: UNAVAILABLE (code 503) here.\n')")" +check "an error on line 3 is not caught (only line 1 counts)" "NOMATCH" \ + "$(se "$(printf '## Review\n\nError: UNAVAILABLE (code 503)\n')")" +check "an empty capture is not caught" "NOMATCH" "$(se '')" + +# The ANCHOR, tested where it is the only thing that matters: the error text is on LINE ONE but +# not at its start. `head -n 1` cannot exclude this; only `^` can. Without this fixture the +# anchor could be deleted and every other check still passed. +check "a heading MENTIONING an error mid-line is not caught" "NOMATCH" \ + "$(se '## Review of the Error: UNAVAILABLE (code 503) retry path')" + +# ...and the anchored form still matches with leading whitespace, which the regex allows. +check "a leading-whitespace error is still caught" "MATCH" \ + "$(se ' Error: UNAVAILABLE (code 503)')" + +# The size cap is what separates "the error IS the whole capture" from "a review mentions one". +long="Error: UNAVAILABLE (code 503) $(head -c 3000 /dev/zero | tr '\0' 'x')" +check "a long capture opening with an error is not caught" "NOMATCH" "$(se "$long")" + +# The script must FAIL rather than post when the backend errored and nothing was produced. +# Called, not grepped for: `if false && [ "${service_errors:-0}" -gt 0 ]` still contains what a +# structural grep looks for, and that mutation came back NOT CAUGHT. +: > "$TMPD/empty" +printf 'a real review\n' > "$TMPD/review" +check "outage + no review -> fail the job" "0" \ + "$(backend_outage_should_fail 2 "$TMPD/empty"; echo $?)" +check "outage + a review -> do not fail" "1" \ + "$(backend_outage_should_fail 2 "$TMPD/review"; echo $?)" +check "no outage + no review -> not THIS guard's job" "1" \ + "$(backend_outage_should_fail 0 "$TMPD/empty"; echo $?)" + if [ "$fails" -ne 0 ]; then echo "$fails check(s) failed" >&2 exit 1 diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 9b273dea..656e760a 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -17,7 +17,60 @@ set -euo pipefail # clamp, so log() must already exist. (Defined later, a clamp that fires would die with # `log: command not found` under set -e instead of warning — a latent misconfiguration trap.) log() { printf '[agy-review] %s\n' "$*" >&2; } + +# A capture that is only a BACKEND ERROR, not a review. When agy's upstream is down it prints +# something like: +# +# Error: Eligibility check failed: UNAVAILABLE (code 503): The service is currently unavailable. +# +# That text is non-empty, so have_text() alone treats it as a valid review, POSTs it as the +# review comment, and the job exits 0 -- a green check for a review that never happened. Observed +# on SLAC PR #14: the `review` check passed in 7 seconds with that string as its entire body, +# twice. A control that cannot fail is worse than no control. +# +# The match is deliberately ANCHORED to the start of the capture rather than being a substring +# search. A genuine review may legitimately quote a 503, an "UNAVAILABLE" constant, or an +# eligibility check while reviewing retry logic -- and aborting on that would be the +# false-positive the OAuth guard's design notes warn about. agy's backend failures occupy the +# WHOLE capture and begin with `Error:`, so requiring the error at the top, in a capture short +# enough to contain nothing else, separates the two without a content heuristic. +# +# Adopted FROM SLAC, which had it while the template and the other installs did not. +# >>> SELFTEST-EXTRACT: service-error guard +# `have_text` lives INSIDE this block, not above it: `backend_outage_should_fail` calls it, so +# an extracted block without it is not self-contained and the selftest cannot run it. It is a +# general helper used throughout the script -- the marker is a comment, so its position +# changes nothing about where the function is defined. have_text() { [ -s "$1" ] && grep -q '[^[:space:]]' "$1"; } +AGY_ERROR_RE='^[[:space:]]*Error:.*(Eligibility check failed|UNAVAILABLE|unavailable|RESOURCE_EXHAUSTED|INTERNAL|DEADLINE_EXCEEDED|code [45][0-9]{2})' +# Captures longer than this are assumed to be real reviews even if they open with an error line: +# a genuine review is thousands of bytes, a bare backend error is a couple of hundred. +AGY_ERROR_MAX_BYTES="${AGY_ERROR_MAX_BYTES:-2000}" +# `head -n 1`, not `head -n 5`. grep matches line-by-line, so scanning five lines +# means "any of the first five lines is an error line" -- which would discard a short, +# genuine review whose heading is on line 1 and which happens to discuss `Error: ... +# UNAVAILABLE` on line 3. That is the false positive the anchoring was supposed to +# prevent, reintroduced by the very check meant to enforce it. The invariant is that a +# backend failure IS the whole capture, so only the first line can carry it. +service_error_present() { + [ -s "$1" ] || return 1 + [ "$(wc -c < "$1")" -le "$AGY_ERROR_MAX_BYTES" ] || return 1 + head -n 1 "$1" | grep -qE "$AGY_ERROR_RE" +} + +# True when the run must FAIL rather than post: the backend errored at least once and no review +# survived. A separate named function rather than an inline condition, because a structural grep +# for the condition is too weak to notice it being disabled -- `if false && [ ... ]` still +# contains the text a grep looks for, and that mutation came back NOT CAUGHT. +# +# The `have_text` half is redundant today (the retry loop truncates the capture whenever it +# counts a backend error) and is kept deliberately: it makes the "post nothing" guarantee +# independent of that truncation surviving a future edit. +backend_outage_should_fail() { + [ "${1:-0}" -gt 0 ] || return 1 + ! have_text "$2" +} +# <<< SELFTEST-EXTRACT # Guarding against a leak of agy's interactive Google OAuth login flow. When agy's cached # session lapses on the runner, `--print` emits the login prompt (a live OAuth URL + "paste the # authorization code here") instead of a review; that text is non-empty, so have_text() alone @@ -44,6 +97,7 @@ have_text() { [ -s "$1" ] && grep -q '[^[:space:]]' "$1"; } # would miss a URL emitted as `.../o/oauth2?client_id=…` or bare `.../o/oauth2` (a leak). The # scheme is what keeps a review's bare-pattern quote from matching, so dropping the trailing # slash loses no safety. +# >>> SELFTEST-EXTRACT: oauth guard OAUTH_URL_RE='https?://accounts\.google\.com/o/oauth2' # The single guard, used at BOTH the retry-loop capture (Layer 1) and the assembled body before @@ -54,6 +108,7 @@ OAUTH_URL_RE='https?://accounts\.google\.com/o/oauth2' # URL is always present in a real login flow, so it alone is both necessary and sufficient; a # lapse is detected AND a leak is blocked by the same unconditional check. oauth_url_present() { [ -s "$1" ] && grep -qiE "$OAUTH_URL_RE" "$1"; } +# <<< SELFTEST-EXTRACT # --- configuration (all env-overridable from the workflow) --------------------- AGY_BIN="${AGY_BIN:-agy}" @@ -100,21 +155,33 @@ 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 comment body's format -- sentinels plus the split/trim helpers -- lives in a sourceable +# file so `agy-review-selftest.sh` can test the REAL implementation rather than a copy of it. +# This script does its work at top level and so cannot itself be sourced. +# shellcheck source=scripts/_agy_comment_body.sh +. "$(dirname -- "${BASH_SOURCE[0]}")/_agy_comment_body.sh" + +MAX_BODY_BYTES="${MAX_BODY_BYTES:-60000}" + +# The jq program that finds THIS bot's existing review comment on the PR, so it can be edited +# rather than replaced. Named, and exercised directly by `scripts/agy-review-selftest.sh`, +# because its predecessor (which selected comments to DELETE) was wrong twice in ways nothing +# observed: first the just-posted comment was not excluded, so a run deleted its own review; +# then jq's `--arg` was handed to `gh api`, which has no such flag, so the step died silently. +# 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='.[] +# The AUTHOR filter is load-bearing, not cosmetic: without it, any user could put the marker +# (an HTML comment, invisible when rendered) in a PR comment and have this bot edit it. Only +# ever touch our own bot's comments. `first` picks the OLDEST match, so if duplicates exist +# from an older version of this script, the canonical thread is the one that keeps growing. +# >>> SELFTEST-EXTRACT: ours-comment filter +SELECT_OURS_JQ='[ .[] | select(.user.type == "Bot" and .user.login == "github-actions[bot]") - | select(.body | contains($marker)) - | select(.id != $new_id) - | .id' -readonly SELECT_STALE_JQ + | select(.body | contains($marker)) ] + | first + | .id // empty' +readonly SELECT_OURS_JQ +# <<< SELFTEST-EXTRACT REPO="${GITHUB_REPOSITORY:?GITHUB_REPOSITORY not set}" @@ -155,6 +222,9 @@ diff_file= diff_err= meta_file= prompt_file= out_file= raw= body_file= agy_diff_ # Set to 1 if agy printed its interactive OAuth login flow instead of a review (lapsed session); # gates the no-post abort below. Pre-declared so `${auth_failed:-0}` is set-u-safe on every path. auth_failed=0 +# Counts backend-error captures across attempts; see `service_error_present`. Pre-declared so +# `${service_errors:-0}` is set-u-safe even on paths that never enter the retry loop. +service_errors=0 # Set when the large-diff fallback below creates refs/agy/* so the trap can remove them. agy_refs_created= cleanup() { @@ -175,7 +245,7 @@ trap cleanup EXIT # but is NOT harmless now that isCrossRepository gates whether an untrusted diff # reaches agy: a lookup failure must never be indistinguishable from "same-repo". diff_file="$(mktemp)"; meta_file="$(mktemp)"; diff_err="$(mktemp)" -gh pr view "$PR" --repo "$REPO" --json title,isCrossRepository,baseRefName > "$meta_file" \ +gh pr view "$PR" --repo "$REPO" --json title,isCrossRepository,baseRefName,headRefOid > "$meta_file" \ || { log "gh pr view failed; refusing to review without knowing the PR's head repo"; exit 1; } # THE FORK GATE (see the trust model at the agy invocation below). The workflow `if:` @@ -221,13 +291,28 @@ esac # ($diff_err was allocated alongside $diff_file / $meta_file above, so the cleanup # trap never references it before it exists.) if ! gh pr diff "$PR" --repo "$REPO" > "$diff_file" 2>"$diff_err"; then - if grep -qi 'diff exceeded the maximum number of lines' "$diff_err"; then + # GitHub refuses an oversized diff TWO ways, with different wording: over + # 20,000 lines, and over 300 FILES. Both are HTTP 406 and both mean the same + # thing here -- the PR is too big for the API, not that anything went wrong -- + # so both must reach the local fallback. Matching only the `lines` variant made + # a wide-but-shallow PR -- hundreds of files, well under the line limit, as a + # bulk regeneration of test baselines produces -- fail the review outright + # instead of falling back. + if grep -qiE 'diff exceeded the maximum number of (lines|files)' "$diff_err"; then base_ref="$(jq -r '.baseRefName // empty' "$meta_file")" if [ -z "$base_ref" ] || [ "$base_ref" = "null" ]; then log "diff exceeds the API limit and the base branch is unknown; cannot fall back" exit 1 fi - log "diff exceeds GitHub's 20,000-line API limit; falling back to a local git diff" + # Name the limit that actually fired. Reporting "20,000-line" for a + # file-count refusal is the same class of misleading triage signal that + # made this bug look like a runner auth failure in the first place. + if grep -qi 'maximum number of files' "$diff_err"; then + hit="300-file" + else + hit="20,000-line" + fi + log "diff exceeds GitHub's ${hit} API limit; falling back to a local git diff" pr_ref="refs/agy/pr-${PR}" base_local="refs/agy/base-${PR}" agy_refs_created=1 @@ -245,8 +330,39 @@ if ! gh pr diff "$PR" --repo "$REPO" > "$diff_file" 2>"$diff_err"; then log "could not fetch PR #${PR} refs for the local diff fallback" exit 1 fi - merge_base="$(git merge-base "$base_local" "$pr_ref")" || { - log "could not compute the merge base for PR #${PR}"; exit 1; } + # The workflow clones with `fetch-depth: 1`, so the two refs above arrive as + # DISCONNECTED shallow histories -- there is no common ancestor for + # `git merge-base` to find, and it fails even though both refs fetched fine. + # (A full local clone hides this completely, which is how it got missed.) + # + # Ask the API for the merge base and fetch that one commit, rather than + # unshallowing: a repo with a large history would pay a full clone on a path + # that only exists because the PR is already unusually big. Diffing two + # commits needs both trees, not the history between them, so a shallow fetch + # of the merge base is enough. + merge_base="$(git merge-base "$base_local" "$pr_ref" 2>/dev/null || true)" + if [ -z "$merge_base" ]; then + head_sha="$(jq -r '.headRefOid // empty' "$meta_file")" + # Percent-encode the branch name for the URL path. NOT for the `/` in a + # `/` branch -- GitHub's compare endpoint accepts those + # raw, verified against a real slashed branch, returning the same SHA + # either way. It is for `%` and `#`, which git permits in a ref name and + # which a URL does not survive: `%` starts an escape and `#` truncates the + # path at the fragment. Both would fail silently into the `|| true`. + base_enc="$(jq -rn --arg v "$base_ref" '$v|@uri')" + api_base="$(gh api "repos/${REPO}/compare/${base_enc}...${head_sha}" \ + --jq '.merge_base_commit.sha' 2>/dev/null || true)" + if [ -n "$api_base" ] && [ "$api_base" != "null" ]; then + if git fetch --no-tags --quiet origin "$api_base" 2>/dev/null \ + || git fetch --no-tags --quiet --deepen=250 origin "${fetch_refspecs[@]}" 2>/dev/null; then + merge_base="$(git merge-base "$base_local" "$pr_ref" 2>/dev/null || echo "$api_base")" + log "shallow clone: merge base ${merge_base} resolved via the compare API" + fi + fi + fi + if [ -z "$merge_base" ]; then + log "could not compute the merge base for PR #${PR}"; exit 1 + fi git diff "$merge_base" "$pr_ref" > "$diff_file" || { log "local git diff failed for PR #${PR}"; exit 1; } log "local diff: $(wc -l < "$diff_file") lines, $(wc -c < "$diff_file") bytes" @@ -498,6 +614,21 @@ for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do # reading it would post that session's output into a public PR comment (data # leak). The PTY path above plus the retry loop cover agy issue #76 without it. + # A backend error is TRANSIENT, so it retries like empty output rather than aborting the way a + # lapsed session does -- but it must never be mistaken for a review. Blank the capture so no + # later path can post it, and let the loop try again. + # + # A COUNTER, not a per-attempt flag. A boolean reset each attempt reflects only the last one, + # so a 503 on attempt 1 followed by empty output on attempt 3 would report the generic "no + # output" cause, and the reverse would claim the backend was down on every attempt when it was + # down on one. Both exit non-zero, so nothing unsafe -- but the log line is the only thing + # telling a human which outage they are looking at. + if service_error_present "$out_file"; then + log "agy returned a backend error rather than a review (attempt ${attempt}/${AGY_RETRIES}): $(head -n 1 "$out_file")" + : > "$out_file" + service_errors=$(( ${service_errors:-0} + 1 )) + fi + have_text "$out_file" && break if [ "$attempt" -lt "$AGY_RETRIES" ]; then delay=$(( AGY_RETRY_DELAY * attempt )) @@ -515,6 +646,17 @@ if [ "${auth_failed:-0}" = "1" ]; then exit 1 fi +# A persistent backend failure is reported as its own cause, and it FAILS THE JOB. Posting the +# error as a review comment (the previous behavior) produced a passing check for a review that +# never ran; posting nothing and exiting non-zero makes the outage visible where it matters. +# The `! have_text` is redundant today -- the loop truncates $out_file whenever it counts a +# backend error -- and is kept deliberately: it makes the "post nothing" guarantee independent +# of that truncation surviving a future edit. +if backend_outage_should_fail "${service_errors:-0}" "$out_file"; then + log "agy's backend returned an error on ${service_errors} of ${AGY_RETRIES} attempt(s) and no review was produced. Failing the check rather than posting the error as a review. Re-run with '/agy-review' once the service recovers." + exit 1 +fi + if ! have_text "$out_file"; then log "no review output after ${AGY_RETRIES} attempt(s). Check $LOG and confirm 'agy -p \"hi\"' works for this user." # Surface agy's stderr into the job log. RUNNER_TEMP is wiped between jobs, so a bare @@ -554,65 +696,102 @@ if oauth_url_present "$body_file"; then exit 1 fi -# --- 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. +# --- edit our existing comment, appending the previous round to its archive ------ +# One comment per PR, edited in place: newest round on top, earlier rounds folded into a +# collapsed `
` below. NOTHING IS DELETED. The previous design posted fresh and +# deleted the prior comment, which kept the PR tidy at the cost of destroying any round +# nobody had read yet — and left no evidence a round had happened at all. +# +# Fail-closed in the direction that matters: every step below falls back to a plain POST of +# the new review. A duplicate comment is noise; failing to publish a review, or losing one, is +# not. The lookup happens BEFORE the post so a PATCH is possible at all, but a failed lookup +# costs only the archive, never the review. +prior_id="" +prior_body_file="$(mktemp)" +if prior_json="$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate 2>/dev/null)"; then + prior_id="$(printf '%s' "$prior_json" | jq -r --arg marker "$MARKER" "$SELECT_OURS_JQ" 2>/dev/null || true)" + if [ -n "$prior_id" ] && [ "$prior_id" != "null" ]; then + printf '%s' "$prior_json" \ + | jq -r --argjson id "$prior_id" '.[] | select(.id == $id) | .body' > "$prior_body_file" 2>/dev/null \ + || : > "$prior_body_file" + else + prior_id="" + fi +else + log "warning: could not list PR comments; posting a fresh review without the archive" +fi + +if [ -n "$prior_id" ] && [ -s "$prior_body_file" ]; then + # Split the prior body into its newest round (everything after the marker, before the + # archive) and the archive's existing inner rounds. `awk` rather than `sed`, because the + # sentinels must match whole lines and a review body legitimately contains regex + # metacharacters, backslashes and HTML. + prior_head="$(agy_body_head "$MARKER" < "$prior_body_file")" + prior_archive="$(agy_body_archive < "$prior_body_file")" + + archived_file="$(mktemp)" + { + printf '%s\n' "$AGY_ROUND_MARK" + printf '
\nRound reviewed at %s\n\n' \ + "$(date -u +'%Y-%m-%d %H:%M UTC')" + printf '%s\n' "$prior_head" + printf '\n
\n' + printf '%s\n' "$prior_archive" + } > "$archived_file" + + # Drop the oldest rounds until the whole comment fits, and SAY SO. A silent truncation + # here would look identical to "there were never any earlier rounds", which is the exact + # confusion this whole change exists to remove. + dropped=0 + while :; do + combined_size=$(( $(wc -c < "$body_file") + $(wc -c < "$archived_file") + 200 )) + [ "$combined_size" -le "$MAX_BODY_BYTES" ] && break + # Remove the LAST `
` block (the oldest round) from the archive. + trimmed="$(mktemp)" + agy_drop_oldest_round < "$archived_file" > "$trimmed" || { rm -f "$trimmed"; break; } + mv "$trimmed" "$archived_file" + dropped=$(( dropped + 1 )) + done + + { + printf '\n%s\n' "$AGY_ARCHIVE_START" + if [ "$dropped" -gt 0 ]; then + printf '%d earlier round(s) dropped to stay under GitHub'"'"'s comment size limit.\n\n' "$dropped" + fi + printf '
\nEarlier review rounds (newest first)\n\n' + cat "$archived_file" + printf '\n
\n' + printf '%s\n' "$AGY_ARCHIVE_END" + } >> "$body_file" + rm -f "$archived_file" + + # Re-run the OAuth guard on the ASSEMBLED body. The archive is text this script published + # earlier and so has already passed the guard once, but the body is what gets published now + # and the guard's contract is that it runs on exactly that. + if oauth_url_present "$body_file"; then + log "refusing to post: the assembled comment body contains a live OAuth authorization URL." + exit 1 + fi + + # The body goes through STDIN as JSON, never through argv. At `MAX_BODY_BYTES` + # the comment can approach 60 KB, and a single execve argument is capped at + # `MAX_ARG_STRLEN` (128 KB on Linux) -- close enough that a future raise of that + # bound would start failing with E2BIG, and the failure would look like a + # GitHub error rather than a local limit. `--rawfile` also makes the value a + # JSON string by construction, so no shell quoting or `-F` type-coercion can + # reinterpret a body that happens to look like a number or a boolean. + if jq -n --rawfile b "$body_file" '{body: $b}' \ + | gh api -X PATCH "repos/${REPO}/issues/comments/${prior_id}" --input - >/dev/null 2>&1; then + log "updated review comment ${prior_id} on ${REPO}#${PR} (earlier rounds archived in place)" + rm -f "$prior_body_file" + exit 0 + fi + log "warning: could not edit comment ${prior_id}; posting a fresh review instead" +fi +rm -f "$prior_body_file" + 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