From ecba9d6d45280bc3b7e1a4d42f36b50710589e71 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 22 Jul 2026 00:01:41 -0400 Subject: [PATCH 1/6] ci: add Antigravity PR reviewer (self-hosted, Ultra) --- .github/agy-review.md | 35 +++++ .github/workflows/antigravity-review.yml | 49 +++++++ scripts/_agy_print.sh | 9 ++ scripts/agy-review.sh | 162 +++++++++++++++++++++++ 4 files changed, 255 insertions(+) create mode 100644 .github/agy-review.md create mode 100644 .github/workflows/antigravity-review.yml create mode 100755 scripts/_agy_print.sh create mode 100755 scripts/agy-review.sh diff --git a/.github/agy-review.md b/.github/agy-review.md new file mode 100644 index 00000000..51c9bc8f --- /dev/null +++ b/.github/agy-review.md @@ -0,0 +1,35 @@ +# Review Style Guide (example) + +Copy this to **`.github/agy-review.md`** in any repo that uses the Antigravity PR +reviewer (a dedicated filename, so it never collides with an existing `GEMINI.md` +or `AGENTS.md`). Everything here is fed to the reviewer as project-specific rules +to enforce. Delete what does not apply and add your own. Override the path with +the `STYLE_GUIDE` env var in the workflow if you prefer a different location. + +## Priorities (in order) + +1. Correctness and data integrity. +2. Security: validate all external input at boundaries; no secrets in code, logs, + or error messages; prefer allowlists. +3. Clear error handling: typed results over panics/`unwrap` on untrusted input. +4. Tests accompany behavior changes. + +## Conventions + +- Conventional Commits (`feat|fix|docs|refactor|test|chore|perf|build|ci`). +- Match surrounding code style; smallest correct change; reuse existing utilities. +- No emojis in code, comments, commits, or docs. +- Public APIs and non-obvious decisions are documented in the same change. + +## What to flag as BLOCKING + +- Unvalidated external input reaching a sink (SQL, shell, filesystem, network). +- Hardcoded credentials or tokens. +- Breaking changes to a public API or on-disk/wire format without a version bump. +- Silent failure paths (swallowed errors, ignored return values). + +## What to keep as SUGGESTION / NITPICK + +- Naming, structure, and readability. +- Missing tests for non-critical paths. +- Performance ideas without a measurement (note: profile before optimizing). diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml new file mode 100644 index 00000000..6aab629f --- /dev/null +++ b/.github/workflows/antigravity-review.yml @@ -0,0 +1,49 @@ +# Antigravity PR Review -- copy this file (and the scripts/ dir) into any repo. +# Runs a Gemini review on a self-hosted runner using your `agy` OAuth session, +# so it draws on your Google AI Ultra limits instead of a paid API key. +# +# Requires a self-hosted runner registered with the label `agy` on a machine +# where `agy` is logged in. See ../../README.md for the full setup. +name: Antigravity PR Review + +on: + pull_request: + types: [opened, reopened] + issue_comment: + types: [created] + +# One review at a time per PR; cancel a superseded run. +concurrency: + group: agy-review-${{ github.event.pull_request.number || github.event.issue.number }} + cancel-in-progress: true + +permissions: + contents: read + pull-requests: write + issues: write + +jobs: + review: + # PR opened/reopened, OR an `/agy-review` comment on a PR. + if: >- + github.event_name == 'pull_request' || + (github.event.issue.pull_request != null && + startsWith(github.event.comment.body, '/agy-review')) + runs-on: [self-hosted, agy] + steps: + - name: Check out repo (for the style guide + scripts) + uses: actions/checkout@v4 + with: + fetch-depth: 1 + + - name: Run Antigravity review + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # --- optional overrides (uncomment to change) --- + # AGY_MODEL: gemini-3-pro # default: agy's configured model + AGY_EFFORT: high # low|medium|high + MAX_DIFF_BYTES: "200000" # truncate huge diffs (~200 KB) + # STYLE_GUIDE: .github/agy-review.md # style guide, loaded if present + run: | + chmod +x scripts/agy-review.sh scripts/_agy_print.sh + scripts/agy-review.sh diff --git a/scripts/_agy_print.sh b/scripts/_agy_print.sh new file mode 100755 index 00000000..1f1184f4 --- /dev/null +++ b/scripts/_agy_print.sh @@ -0,0 +1,9 @@ +#!/usr/bin/env bash +# +# _agy_print.sh -- inner helper for the script(1) PTY fallback in agy-review.sh. +# Only used when `unbuffer` (from the `expect` package) is unavailable. +# +# Usage: _agy_print.sh [agy flags...] +set -euo pipefail +prompt_file="$1"; shift +exec "${AGY_BIN:-agy}" "$@" --print "$(cat "$prompt_file")" diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh new file mode 100755 index 00000000..1de4db18 --- /dev/null +++ b/scripts/agy-review.sh @@ -0,0 +1,162 @@ +#!/usr/bin/env bash +# +# agy-review.sh -- headless GitHub PR reviewer driven by Antigravity CLI (`agy`). +# +# Runs on a SELF-HOSTED GitHub Actions runner that lives on a machine where `agy` +# is already logged in via Google OAuth. Because it uses the CLI's cached OAuth +# session (not an API key), every review is billed against your Google AI Ultra +# rate limits -- i.e. free under the subscription, no metered API spend. +# +# Flow: resolve PR -> `gh pr diff` -> build an adversarial-reviewer prompt +# (+ repo style guide) -> `agy --print` under a PTY -> post via `gh pr comment`. +# +# See ../README.md for setup, the issue #76 PTY workaround, and the ToS caveat. +set -euo pipefail + +# --- configuration (all env-overridable from the workflow) --------------------- +AGY_BIN="${AGY_BIN:-agy}" +command -v "$AGY_BIN" >/dev/null 2>&1 || AGY_BIN="$HOME/.local/bin/agy" +AGY_MODEL="${AGY_MODEL:-}" # empty = agy's configured default (Gemini 3.x Pro) +AGY_EFFORT="${AGY_EFFORT:-high}" # low|medium|high +AGY_PRINT_TIMEOUT="${AGY_PRINT_TIMEOUT:-5m}" +MAX_DIFF_BYTES="${MAX_DIFF_BYTES:-200000}" # truncate very large diffs (~200 KB) +STYLE_GUIDE="${STYLE_GUIDE:-.github/agy-review.md}" # repo-relative; loaded if present + # (dedicated name -- avoids colliding with GEMINI.md/AGENTS.md) +CONV_DIR="${CONV_DIR:-$HOME/.gemini/antigravity-cli/conversations}" +LOG="${RUNNER_TEMP:-/tmp}/agy-review.log" +MARKER="" + +log() { printf '[agy-review] %s\n' "$*" >&2; } +have_text() { [ -s "$1" ] && grep -q '[^[:space:]]' "$1"; } + +REPO="${GITHUB_REPOSITORY:?GITHUB_REPOSITORY not set}" + +# --- resolve the PR number from the triggering event -------------------------- +case "${GITHUB_EVENT_NAME:-}" in + pull_request|pull_request_target) + PR="$(jq -r '.pull_request.number' "$GITHUB_EVENT_PATH")" + ;; + issue_comment) + is_pr="$(jq -r '.issue.pull_request // empty' "$GITHUB_EVENT_PATH")" + body="$(jq -r '.comment.body // ""' "$GITHUB_EVENT_PATH")" + [ -n "$is_pr" ] || { log "comment is not on a PR; skipping"; exit 0; } + case "$body" in + /agy-review*) : ;; + *) log "comment is not an /agy-review command; skipping"; exit 0 ;; + esac + PR="$(jq -r '.issue.number' "$GITHUB_EVENT_PATH")" + ;; + *) + PR="${1:-}" + [ -n "$PR" ] || { log "unknown event; pass a PR number as \$1"; exit 1; } + ;; +esac +log "reviewing ${REPO}#${PR}" + +# --- fetch the diff + metadata ------------------------------------------------- +diff_file="$(mktemp)"; meta_file="$(mktemp)" +gh pr diff "$PR" --repo "$REPO" > "$diff_file" || { log "gh pr diff failed"; exit 1; } +gh pr view "$PR" --repo "$REPO" --json title > "$meta_file" 2>/dev/null || echo '{}' > "$meta_file" + +if ! have_text "$diff_file"; then log "empty diff; nothing to review"; exit 0; fi + +truncated="" +if [ "$(wc -c < "$diff_file")" -gt "$MAX_DIFF_BYTES" ]; then + head -c "$MAX_DIFF_BYTES" "$diff_file" > "$diff_file.cut" && mv "$diff_file.cut" "$diff_file" + truncated=$'\n\n> Note: the diff was truncated to '"${MAX_DIFF_BYTES}"$' bytes for this review.' + log "diff truncated to ${MAX_DIFF_BYTES} bytes" +fi + +# --- build the prompt ---------------------------------------------------------- +title="$(jq -r '.title // ""' "$meta_file")" +style=""; [ -f "$STYLE_GUIDE" ] && style="$(cat "$STYLE_GUIDE")" + +prompt_file="$(mktemp)" +{ + cat < "$prompt_file" + +# --- run agy headless, under a PTY (works around agy issue #76: -p drops -------- +# stdout when stdout is not a TTY, e.g. piped/redirected/subprocess) --------- +flags=( --print-timeout "$AGY_PRINT_TIMEOUT" --sandbox --dangerously-skip-permissions ) +[ -n "$AGY_MODEL" ] && flags+=( --model "$AGY_MODEL" ) +[ -n "$AGY_EFFORT" ] && flags+=( --effort "$AGY_EFFORT" ) + +out_file="$(mktemp)" +here="$(cd "$(dirname "$0")" && pwd)" +: > "$LOG" + +if command -v unbuffer >/dev/null 2>&1; then + log "running agy via unbuffer (allocates a PTY)" + unbuffer "$AGY_BIN" "${flags[@]}" --print "$(cat "$prompt_file")" > "$out_file" 2>>"$LOG" || true +else + log "unbuffer not found; falling back to script(1)" + raw="$(mktemp)" + AGY_BIN="$AGY_BIN" script -qfec "$here/_agy_print.sh '$prompt_file' ${flags[*]}" "$raw" >/dev/null 2>>"$LOG" || true + col -b < "$raw" > "$out_file" +fi + +# normalize CRs without sed -i (avoid in-place edit footguns) +tr -d '\r' < "$out_file" > "$out_file.clean" && mv "$out_file.clean" "$out_file" + +# --- fallback: recover the answer from agy's conversation SQLite store ---------- +# (belt-and-suspenders for issue #76 on hosts where the PTY trick still +# yields nothing). The schema is NOT officially documented and can change +# between agy versions -- inspect with `sqlite3 .schema` and adjust. +if ! have_text "$out_file"; then + log "print output empty; trying SQLite conversation fallback" + if command -v sqlite3 >/dev/null 2>&1 && [ -d "$CONV_DIR" ]; then + db="$(ls -t "$CONV_DIR"/*.db 2>/dev/null | head -1 || true)" + if [ -n "${db:-}" ]; then + for q in \ + "SELECT text FROM messages WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;" \ + "SELECT content FROM messages WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;" \ + "SELECT body FROM message WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;"; do + sqlite3 "$db" "$q" > "$out_file" 2>/dev/null && have_text "$out_file" && break + done + fi + fi +fi + +if ! have_text "$out_file"; then + log "no review output produced. Check $LOG and confirm 'agy -p \"hi\"' works for this user." + exit 1 +fi + +# --- assemble the comment body ------------------------------------------------- +body_file="$(mktemp)" +{ + printf '%s\n' "$MARKER" + printf '## Antigravity review (Gemini via Ultra)\n\n' + cat "$out_file" + printf '%s' "$truncated" + 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 ------------------------- +gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ + --jq ".[] | select(.body | contains(\"${MARKER}\")) | .id" 2>/dev/null \ + | while read -r cid; do + [ -n "$cid" ] && gh api -X DELETE "repos/${REPO}/issues/comments/${cid}" >/dev/null 2>&1 || true + done + +gh pr comment "$PR" --repo "$REPO" --body-file "$body_file" +log "posted review to ${REPO}#${PR}" From 0237135d526cf28d0e45843345a8646012bea0c6 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 22 Jul 2026 00:11:24 -0400 Subject: [PATCH 2/6] =?UTF-8?q?ci(review):=20harden=20reviewer=20=E2=80=94?= =?UTF-8?q?=20gate=20triggers,=20argv-safe=20PTY,=20drop=20racy=20sqlite?= =?UTF-8?q?=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/workflows/antigravity-review.yml | 19 ++-- scripts/_agy_print.sh | 9 -- scripts/_agy_pty.py | 22 +++++ scripts/agy-review.sh | 116 ++++++++++------------- 4 files changed, 86 insertions(+), 80 deletions(-) delete mode 100755 scripts/_agy_print.sh create mode 100755 scripts/_agy_pty.py diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml index 6aab629f..de236097 100644 --- a/.github/workflows/antigravity-review.yml +++ b/.github/workflows/antigravity-review.yml @@ -24,15 +24,22 @@ permissions: jobs: review: - # PR opened/reopened, OR an `/agy-review` comment on a PR. + # SECURITY GATE (self-hosted runner on a public repo): + # - pull_request: only from a branch in THIS repo, never a fork (a fork PR + # must not execute on the self-hosted host). + # - issue_comment: only `/agy-review` from someone with write access + # (OWNER/MEMBER/COLLABORATOR), so a stranger can't trigger execution. if: >- - github.event_name == 'pull_request' || - (github.event.issue.pull_request != null && - startsWith(github.event.comment.body, '/agy-review')) + (github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository) || + (github.event_name == 'issue_comment' && + github.event.issue.pull_request != null && + startsWith(github.event.comment.body, '/agy-review') && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) runs-on: [self-hosted, agy] steps: - name: Check out repo (for the style guide + scripts) - uses: actions/checkout@v4 + uses: actions/checkout@v5 with: fetch-depth: 1 @@ -45,5 +52,5 @@ jobs: MAX_DIFF_BYTES: "200000" # truncate huge diffs (~200 KB) # STYLE_GUIDE: .github/agy-review.md # style guide, loaded if present run: | - chmod +x scripts/agy-review.sh scripts/_agy_print.sh + chmod +x scripts/agy-review.sh scripts/agy-review.sh diff --git a/scripts/_agy_print.sh b/scripts/_agy_print.sh deleted file mode 100755 index 1f1184f4..00000000 --- a/scripts/_agy_print.sh +++ /dev/null @@ -1,9 +0,0 @@ -#!/usr/bin/env bash -# -# _agy_print.sh -- inner helper for the script(1) PTY fallback in agy-review.sh. -# Only used when `unbuffer` (from the `expect` package) is unavailable. -# -# Usage: _agy_print.sh [agy flags...] -set -euo pipefail -prompt_file="$1"; shift -exec "${AGY_BIN:-agy}" "$@" --print "$(cat "$prompt_file")" diff --git a/scripts/_agy_pty.py b/scripts/_agy_pty.py new file mode 100755 index 00000000..517537f4 --- /dev/null +++ b/scripts/_agy_pty.py @@ -0,0 +1,22 @@ +#!/usr/bin/env python3 +# _agy_pty.py -- argv-safe PTY wrapper used by agy-review.sh as a fallback when +# `unbuffer` (from the `expect` package) is unavailable. +# +# Runs argv[1:] under a pseudo-terminal so a program that only flushes stdout +# when attached to a TTY (agy --print, upstream issue #76) still emits its output +# when its stdout is captured. No shell is involved -- the command is passed as an +# argv list -- so caller-provided flags cannot inject a command or be word-split. +# +# Usage: _agy_pty.py [args...] (child's terminal output goes to stdout) +import os +import pty +import sys + +if len(sys.argv) < 2: + sys.stderr.write("usage: _agy_pty.py [args...]\n") + sys.exit(2) + +# pty.spawn copies the child's PTY output onto our stdout (fd 1), which the +# caller redirects to a file. Returns the child's raw wait status. +status = pty.spawn(sys.argv[1:]) +sys.exit(os.waitstatus_to_exitcode(status)) diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 1de4db18..67e6da50 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -2,36 +2,45 @@ # # agy-review.sh -- headless GitHub PR reviewer driven by Antigravity CLI (`agy`). # -# Runs on a SELF-HOSTED GitHub Actions runner that lives on a machine where `agy` -# is already logged in via Google OAuth. Because it uses the CLI's cached OAuth -# session (not an API key), every review is billed against your Google AI Ultra -# rate limits -- i.e. free under the subscription, no metered API spend. +# Runs on a SELF-HOSTED GitHub Actions runner where `agy` is logged in via Google +# OAuth, so reviews draw on your Google AI Ultra rate limits (no metered API key). # -# Flow: resolve PR -> `gh pr diff` -> build an adversarial-reviewer prompt -# (+ repo style guide) -> `agy --print` under a PTY -> post via `gh pr comment`. +# Flow: resolve PR -> `gh pr diff` -> adversarial-reviewer prompt (+ style guide) +# -> `agy --print` under a PTY (argv-safe) -> post via `gh pr comment`. # -# See ../README.md for setup, the issue #76 PTY workaround, and the ToS caveat. +# Security: the workflow gates who can trigger this (same-repo PRs only; comments +# only from OWNER/MEMBER/COLLABORATOR). This script re-checks the comment gate +# defensively. See ../README.md. set -euo pipefail -# --- configuration (all env-overridable from the workflow) --------------------- +# --- configuration (env-overridable from the workflow) ------------------------ AGY_BIN="${AGY_BIN:-agy}" command -v "$AGY_BIN" >/dev/null 2>&1 || AGY_BIN="$HOME/.local/bin/agy" -AGY_MODEL="${AGY_MODEL:-}" # empty = agy's configured default (Gemini 3.x Pro) +AGY_MODEL="${AGY_MODEL:-}" # empty = agy's configured default AGY_EFFORT="${AGY_EFFORT:-high}" # low|medium|high AGY_PRINT_TIMEOUT="${AGY_PRINT_TIMEOUT:-5m}" -MAX_DIFF_BYTES="${MAX_DIFF_BYTES:-200000}" # truncate very large diffs (~200 KB) -STYLE_GUIDE="${STYLE_GUIDE:-.github/agy-review.md}" # repo-relative; loaded if present - # (dedicated name -- avoids colliding with GEMINI.md/AGENTS.md) -CONV_DIR="${CONV_DIR:-$HOME/.gemini/antigravity-cli/conversations}" -LOG="${RUNNER_TEMP:-/tmp}/agy-review.log" +MAX_DIFF_BYTES="${MAX_DIFF_BYTES:-200000}" # truncate very large diffs (bounds ARG_MAX too) +STYLE_GUIDE="${STYLE_GUIDE:-.github/agy-review.md}" MARKER="" +# --- temp files + cleanup trap (no leftover scratch on the runner) ------------ +diff_file= meta_file= prompt_file= out_file= body_file= LOG= +cleanup() { + local f + for f in "$diff_file" "$meta_file" "$prompt_file" "$out_file" "$body_file" "$LOG"; do + [ -n "$f" ] && rm -f "$f" + done +} +trap cleanup EXIT +LOG="$(mktemp)" + log() { printf '[agy-review] %s\n' "$*" >&2; } have_text() { [ -s "$1" ] && grep -q '[^[:space:]]' "$1"; } +die() { log "$*"; [ -s "$LOG" ] && { log "--- agy stderr ---"; cat "$LOG" >&2; }; exit 1; } REPO="${GITHUB_REPOSITORY:?GITHUB_REPOSITORY not set}" -# --- resolve the PR number from the triggering event -------------------------- +# --- resolve the PR number; re-verify the comment gate defensively ------------ case "${GITHUB_EVENT_NAME:-}" in pull_request|pull_request_target) PR="$(jq -r '.pull_request.number' "$GITHUB_EVENT_PATH")" @@ -39,26 +48,27 @@ case "${GITHUB_EVENT_NAME:-}" in issue_comment) is_pr="$(jq -r '.issue.pull_request // empty' "$GITHUB_EVENT_PATH")" body="$(jq -r '.comment.body // ""' "$GITHUB_EVENT_PATH")" - [ -n "$is_pr" ] || { log "comment is not on a PR; skipping"; exit 0; } - case "$body" in - /agy-review*) : ;; - *) log "comment is not an /agy-review command; skipping"; exit 0 ;; + assoc="$(jq -r '.comment.author_association // ""' "$GITHUB_EVENT_PATH")" + [ -n "$is_pr" ] || { log "comment not on a PR; skipping"; exit 0; } + case "$body" in /agy-review*) : ;; *) log "not an /agy-review command; skipping"; exit 0 ;; esac + case "$assoc" in + OWNER|MEMBER|COLLABORATOR) : ;; + *) log "comment author association '$assoc' lacks write access; skipping"; exit 0 ;; esac PR="$(jq -r '.issue.number' "$GITHUB_EVENT_PATH")" ;; *) PR="${1:-}" - [ -n "$PR" ] || { log "unknown event; pass a PR number as \$1"; exit 1; } + [ -n "$PR" ] || die "unknown event; pass a PR number as \$1" ;; esac log "reviewing ${REPO}#${PR}" -# --- fetch the diff + metadata ------------------------------------------------- +# --- fetch the diff + metadata ------------------------------------------------ diff_file="$(mktemp)"; meta_file="$(mktemp)" -gh pr diff "$PR" --repo "$REPO" > "$diff_file" || { log "gh pr diff failed"; exit 1; } +gh pr diff "$PR" --repo "$REPO" > "$diff_file" || die "gh pr diff failed" gh pr view "$PR" --repo "$REPO" --json title > "$meta_file" 2>/dev/null || echo '{}' > "$meta_file" - -if ! have_text "$diff_file"; then log "empty diff; nothing to review"; exit 0; fi +have_text "$diff_file" || { log "empty diff; nothing to review"; exit 0; } truncated="" if [ "$(wc -c < "$diff_file")" -gt "$MAX_DIFF_BYTES" ]; then @@ -67,7 +77,7 @@ if [ "$(wc -c < "$diff_file")" -gt "$MAX_DIFF_BYTES" ]; then log "diff truncated to ${MAX_DIFF_BYTES} bytes" fi -# --- build the prompt ---------------------------------------------------------- +# --- build the prompt --------------------------------------------------------- title="$(jq -r '.title // ""' "$meta_file")" style=""; [ -f "$STYLE_GUIDE" ] && style="$(cat "$STYLE_GUIDE")" @@ -87,61 +97,35 @@ Do not praise. Focus on what could be wrong. If the change is trivial, say so br PR title: ${title} EOF - if [ -n "$style" ]; then - printf '\n--- PROJECT STYLE GUIDE (enforce these) ---\n%s\n' "$style" - fi + [ -n "$style" ] && printf '\n--- PROJECT STYLE GUIDE (enforce these) ---\n%s\n' "$style" printf '\n--- UNIFIED DIFF ---\n' cat "$diff_file" } > "$prompt_file" -# --- run agy headless, under a PTY (works around agy issue #76: -p drops -------- -# stdout when stdout is not a TTY, e.g. piped/redirected/subprocess) --------- +# --- run agy headless under a PTY (works around agy issue #76: -p drops -------- +# stdout on a non-TTY). Both paths pass an argv ARRAY -- no shell string -- +# so env-provided flags cannot inject a command or word-split. ------------- flags=( --print-timeout "$AGY_PRINT_TIMEOUT" --sandbox --dangerously-skip-permissions ) [ -n "$AGY_MODEL" ] && flags+=( --model "$AGY_MODEL" ) [ -n "$AGY_EFFORT" ] && flags+=( --effort "$AGY_EFFORT" ) out_file="$(mktemp)" here="$(cd "$(dirname "$0")" && pwd)" -: > "$LOG" - +prompt="$(cat "$prompt_file")" +rc=0 if command -v unbuffer >/dev/null 2>&1; then - log "running agy via unbuffer (allocates a PTY)" - unbuffer "$AGY_BIN" "${flags[@]}" --print "$(cat "$prompt_file")" > "$out_file" 2>>"$LOG" || true + unbuffer "$AGY_BIN" "${flags[@]}" --print "$prompt" > "$out_file" 2>>"$LOG" || rc=$? +elif command -v python3 >/dev/null 2>&1; then + python3 "$here/_agy_pty.py" "$AGY_BIN" "${flags[@]}" --print "$prompt" > "$out_file" 2>>"$LOG" || rc=$? else - log "unbuffer not found; falling back to script(1)" - raw="$(mktemp)" - AGY_BIN="$AGY_BIN" script -qfec "$here/_agy_print.sh '$prompt_file' ${flags[*]}" "$raw" >/dev/null 2>>"$LOG" || true - col -b < "$raw" > "$out_file" + die "need 'unbuffer' (from the 'expect' package) or python3 for a PTY; neither found" fi - -# normalize CRs without sed -i (avoid in-place edit footguns) +[ "$rc" -eq 0 ] || log "agy exited non-zero ($rc); checking output anyway" tr -d '\r' < "$out_file" > "$out_file.clean" && mv "$out_file.clean" "$out_file" -# --- fallback: recover the answer from agy's conversation SQLite store ---------- -# (belt-and-suspenders for issue #76 on hosts where the PTY trick still -# yields nothing). The schema is NOT officially documented and can change -# between agy versions -- inspect with `sqlite3 .schema` and adjust. -if ! have_text "$out_file"; then - log "print output empty; trying SQLite conversation fallback" - if command -v sqlite3 >/dev/null 2>&1 && [ -d "$CONV_DIR" ]; then - db="$(ls -t "$CONV_DIR"/*.db 2>/dev/null | head -1 || true)" - if [ -n "${db:-}" ]; then - for q in \ - "SELECT text FROM messages WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;" \ - "SELECT content FROM messages WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;" \ - "SELECT body FROM message WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;"; do - sqlite3 "$db" "$q" > "$out_file" 2>/dev/null && have_text "$out_file" && break - done - fi - fi -fi - -if ! have_text "$out_file"; then - log "no review output produced. Check $LOG and confirm 'agy -p \"hi\"' works for this user." - exit 1 -fi +have_text "$out_file" || die "no review output from agy (rc=$rc). Confirm 'agy -p \"hi\"' works for this user." -# --- assemble the comment body ------------------------------------------------- +# --- assemble the comment body ------------------------------------------------ body_file="$(mktemp)" { printf '%s\n' "$MARKER" @@ -151,7 +135,9 @@ 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 ------------------------- +# --- replace any prior review comment, then post fresh ------------------------ +# Comment deletion is best-effort by design (a delete failure must not block the +# new review), so its errors are intentionally not fatal. gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ --jq ".[] | select(.body | contains(\"${MARKER}\")) | .id" 2>/dev/null \ | while read -r cid; do From 9875dabbe572cd748b4a020f44965de5ba7c2052 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 22 Jul 2026 00:28:50 -0400 Subject: [PATCH 3/6] =?UTF-8?q?ci(review):=20re-sync=20reviewer=20to=20tem?= =?UTF-8?q?plate=20=E2=80=94=20job-level=20concurrency,=20agy=20flock=20+?= =?UTF-8?q?=20retry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/workflows/antigravity-review.yml | 34 ++--- scripts/_agy_print.sh | 11 ++ scripts/_agy_pty.py | 22 --- scripts/agy-review.sh | 166 +++++++++++++++-------- 4 files changed, 140 insertions(+), 93 deletions(-) create mode 100755 scripts/_agy_print.sh delete mode 100755 scripts/_agy_pty.py diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml index de236097..252bd2e4 100644 --- a/.github/workflows/antigravity-review.yml +++ b/.github/workflows/antigravity-review.yml @@ -12,11 +12,6 @@ on: issue_comment: types: [created] -# One review at a time per PR; cancel a superseded run. -concurrency: - group: agy-review-${{ github.event.pull_request.number || github.event.issue.number }} - cancel-in-progress: true - permissions: contents: read pull-requests: write @@ -24,22 +19,29 @@ permissions: jobs: review: - # SECURITY GATE (self-hosted runner on a public repo): - # - pull_request: only from a branch in THIS repo, never a fork (a fork PR - # must not execute on the self-hosted host). - # - issue_comment: only `/agy-review` from someone with write access - # (OWNER/MEMBER/COLLABORATOR), so a stranger can't trigger execution. + # PR opened/reopened, OR an `/agy-review` comment on a PR from a trusted author. + # The comment path is gated on author_association so an untrusted outside user cannot run the + # self-hosted runner by commenting `/agy-review` (belt-and-suspenders with the repo's + # Settings -> Actions -> "Fork pull request workflows -> require approval for all outside + # collaborators"). The `pull_request` trigger is already `opened`/`reopened` only. if: >- - (github.event_name == 'pull_request' && - github.event.pull_request.head.repo.full_name == github.repository) || - (github.event_name == 'issue_comment' && - github.event.issue.pull_request != null && + github.event_name == 'pull_request' || + (github.event.issue.pull_request != null && startsWith(github.event.comment.body, '/agy-review') && contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) + # Concurrency at the JOB level (NOT workflow level): a job skipped by the `if:` + # above never enters the group, so an unrelated `issue_comment` on the PR (e.g. + # another review bot commenting) can't cancel an in-progress review. And + # cancel-in-progress:false means a second *real* review queues behind the first + # instead of killing it. Fixes the race where any comment on the PR aborted the + # running review mid-`agy`. + concurrency: + group: agy-review-${{ github.event.pull_request.number || github.event.issue.number }} + cancel-in-progress: false runs-on: [self-hosted, agy] steps: - name: Check out repo (for the style guide + scripts) - uses: actions/checkout@v5 + uses: actions/checkout@v4 with: fetch-depth: 1 @@ -52,5 +54,5 @@ jobs: MAX_DIFF_BYTES: "200000" # truncate huge diffs (~200 KB) # STYLE_GUIDE: .github/agy-review.md # style guide, loaded if present run: | - chmod +x scripts/agy-review.sh + chmod +x scripts/agy-review.sh scripts/_agy_print.sh scripts/agy-review.sh diff --git a/scripts/_agy_print.sh b/scripts/_agy_print.sh new file mode 100755 index 00000000..df4cbab3 --- /dev/null +++ b/scripts/_agy_print.sh @@ -0,0 +1,11 @@ +#!/usr/bin/env bash +# +# _agy_print.sh -- inner helper for the script(1) PTY fallback in agy-review.sh. +# Only used when `unbuffer` (from the `expect` package) is unavailable. +# +# Invoked as: _agy_print.sh [agy flags...] +# (agy-review.sh passes it through `script -qfec` so agy runs attached to a PTY, +# working around agy issue #76 where `-p` drops stdout on a non-TTY.) +set -euo pipefail +prompt_file="$1"; shift +exec "${AGY_BIN:-agy}" "$@" --print "$(cat "$prompt_file")" diff --git a/scripts/_agy_pty.py b/scripts/_agy_pty.py deleted file mode 100755 index 517537f4..00000000 --- a/scripts/_agy_pty.py +++ /dev/null @@ -1,22 +0,0 @@ -#!/usr/bin/env python3 -# _agy_pty.py -- argv-safe PTY wrapper used by agy-review.sh as a fallback when -# `unbuffer` (from the `expect` package) is unavailable. -# -# Runs argv[1:] under a pseudo-terminal so a program that only flushes stdout -# when attached to a TTY (agy --print, upstream issue #76) still emits its output -# when its stdout is captured. No shell is involved -- the command is passed as an -# argv list -- so caller-provided flags cannot inject a command or be word-split. -# -# Usage: _agy_pty.py [args...] (child's terminal output goes to stdout) -import os -import pty -import sys - -if len(sys.argv) < 2: - sys.stderr.write("usage: _agy_pty.py [args...]\n") - sys.exit(2) - -# pty.spawn copies the child's PTY output onto our stdout (fd 1), which the -# caller redirects to a file. Returns the child's raw wait status. -status = pty.spawn(sys.argv[1:]) -sys.exit(os.waitstatus_to_exitcode(status)) diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 67e6da50..4ac539f0 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -2,45 +2,40 @@ # # agy-review.sh -- headless GitHub PR reviewer driven by Antigravity CLI (`agy`). # -# Runs on a SELF-HOSTED GitHub Actions runner where `agy` is logged in via Google -# OAuth, so reviews draw on your Google AI Ultra rate limits (no metered API key). +# Runs on a SELF-HOSTED GitHub Actions runner that lives on a machine where `agy` +# is already logged in via Google OAuth. Because it uses the CLI's cached OAuth +# session (not an API key), every review is billed against your Google AI Ultra +# rate limits -- i.e. free under the subscription, no metered API spend. # -# Flow: resolve PR -> `gh pr diff` -> adversarial-reviewer prompt (+ style guide) -# -> `agy --print` under a PTY (argv-safe) -> post via `gh pr comment`. +# Flow: resolve PR -> `gh pr diff` -> build an adversarial-reviewer prompt +# (+ repo style guide) -> `agy --print` under a PTY -> post via `gh pr comment`. # -# Security: the workflow gates who can trigger this (same-repo PRs only; comments -# only from OWNER/MEMBER/COLLABORATOR). This script re-checks the comment gate -# defensively. See ../README.md. +# See ../README.md for setup, the issue #76 PTY workaround, and the ToS caveat. set -euo pipefail -# --- configuration (env-overridable from the workflow) ------------------------ +# --- configuration (all env-overridable from the workflow) --------------------- AGY_BIN="${AGY_BIN:-agy}" command -v "$AGY_BIN" >/dev/null 2>&1 || AGY_BIN="$HOME/.local/bin/agy" -AGY_MODEL="${AGY_MODEL:-}" # empty = agy's configured default +AGY_MODEL="${AGY_MODEL:-}" # empty = agy's configured default (Gemini 3.x Pro) AGY_EFFORT="${AGY_EFFORT:-high}" # low|medium|high AGY_PRINT_TIMEOUT="${AGY_PRINT_TIMEOUT:-5m}" -MAX_DIFF_BYTES="${MAX_DIFF_BYTES:-200000}" # truncate very large diffs (bounds ARG_MAX too) -STYLE_GUIDE="${STYLE_GUIDE:-.github/agy-review.md}" +MAX_DIFF_BYTES="${MAX_DIFF_BYTES:-200000}" # truncate very large diffs (~200 KB) +STYLE_GUIDE="${STYLE_GUIDE:-.github/agy-review.md}" # repo-relative; loaded if present + # (dedicated name -- avoids colliding with GEMINI.md/AGENTS.md) +CONV_DIR="${CONV_DIR:-$HOME/.gemini/antigravity-cli/conversations}" +LOG="${RUNNER_TEMP:-/tmp}/agy-review.log" +AGY_LOCK="${AGY_LOCK:-$HOME/.gemini/antigravity-cli/.agy-review.lock}" +AGY_LOCK_WAIT="${AGY_LOCK_WAIT:-600}" # seconds to wait for the agy lock before proceeding +AGY_RETRIES="${AGY_RETRIES:-3}" # attempts to get a usable agy response +AGY_RETRY_DELAY="${AGY_RETRY_DELAY:-15}" # base backoff seconds between retries (grows per attempt) MARKER="" -# --- temp files + cleanup trap (no leftover scratch on the runner) ------------ -diff_file= meta_file= prompt_file= out_file= body_file= LOG= -cleanup() { - local f - for f in "$diff_file" "$meta_file" "$prompt_file" "$out_file" "$body_file" "$LOG"; do - [ -n "$f" ] && rm -f "$f" - done -} -trap cleanup EXIT -LOG="$(mktemp)" - log() { printf '[agy-review] %s\n' "$*" >&2; } have_text() { [ -s "$1" ] && grep -q '[^[:space:]]' "$1"; } -die() { log "$*"; [ -s "$LOG" ] && { log "--- agy stderr ---"; cat "$LOG" >&2; }; exit 1; } REPO="${GITHUB_REPOSITORY:?GITHUB_REPOSITORY not set}" -# --- resolve the PR number; re-verify the comment gate defensively ------------ +# --- resolve the PR number from the triggering event -------------------------- case "${GITHUB_EVENT_NAME:-}" in pull_request|pull_request_target) PR="$(jq -r '.pull_request.number' "$GITHUB_EVENT_PATH")" @@ -48,27 +43,31 @@ case "${GITHUB_EVENT_NAME:-}" in issue_comment) is_pr="$(jq -r '.issue.pull_request // empty' "$GITHUB_EVENT_PATH")" body="$(jq -r '.comment.body // ""' "$GITHUB_EVENT_PATH")" - assoc="$(jq -r '.comment.author_association // ""' "$GITHUB_EVENT_PATH")" - [ -n "$is_pr" ] || { log "comment not on a PR; skipping"; exit 0; } - case "$body" in /agy-review*) : ;; *) log "not an /agy-review command; skipping"; exit 0 ;; esac - case "$assoc" in - OWNER|MEMBER|COLLABORATOR) : ;; - *) log "comment author association '$assoc' lacks write access; skipping"; exit 0 ;; + [ -n "$is_pr" ] || { log "comment is not on a PR; skipping"; exit 0; } + case "$body" in + /agy-review*) : ;; + *) log "comment is not an /agy-review command; skipping"; exit 0 ;; esac PR="$(jq -r '.issue.number' "$GITHUB_EVENT_PATH")" ;; *) PR="${1:-}" - [ -n "$PR" ] || die "unknown event; pass a PR number as \$1" + [ -n "$PR" ] || { log "unknown event; pass a PR number as \$1"; exit 1; } ;; esac log "reviewing ${REPO}#${PR}" -# --- fetch the diff + metadata ------------------------------------------------ +# Remove every temp file on exit. Pre-declared so the trap is safe under `set -u` even if the +# script exits before a given file is created. +diff_file= meta_file= prompt_file= out_file= raw= body_file= +trap 'rm -f "$diff_file" "$meta_file" "$prompt_file" "$out_file" "$raw" "$body_file"' EXIT + +# --- fetch the diff + metadata ------------------------------------------------- diff_file="$(mktemp)"; meta_file="$(mktemp)" -gh pr diff "$PR" --repo "$REPO" > "$diff_file" || die "gh pr diff failed" +gh pr diff "$PR" --repo "$REPO" > "$diff_file" || { log "gh pr diff failed"; exit 1; } gh pr view "$PR" --repo "$REPO" --json title > "$meta_file" 2>/dev/null || echo '{}' > "$meta_file" -have_text "$diff_file" || { log "empty diff; nothing to review"; exit 0; } + +if ! have_text "$diff_file"; then log "empty diff; nothing to review"; exit 0; fi truncated="" if [ "$(wc -c < "$diff_file")" -gt "$MAX_DIFF_BYTES" ]; then @@ -77,7 +76,7 @@ if [ "$(wc -c < "$diff_file")" -gt "$MAX_DIFF_BYTES" ]; then log "diff truncated to ${MAX_DIFF_BYTES} bytes" fi -# --- build the prompt --------------------------------------------------------- +# --- build the prompt ---------------------------------------------------------- title="$(jq -r '.title // ""' "$meta_file")" style=""; [ -f "$STYLE_GUIDE" ] && style="$(cat "$STYLE_GUIDE")" @@ -97,35 +96,89 @@ Do not praise. Focus on what could be wrong. If the change is trivial, say so br PR title: ${title} EOF - [ -n "$style" ] && printf '\n--- PROJECT STYLE GUIDE (enforce these) ---\n%s\n' "$style" + if [ -n "$style" ]; then + printf '\n--- PROJECT STYLE GUIDE (enforce these) ---\n%s\n' "$style" + fi printf '\n--- UNIFIED DIFF ---\n' cat "$diff_file" } > "$prompt_file" -# --- run agy headless under a PTY (works around agy issue #76: -p drops -------- -# stdout on a non-TTY). Both paths pass an argv ARRAY -- no shell string -- -# so env-provided flags cannot inject a command or word-split. ------------- +# --- run agy headless, under a PTY (works around agy issue #76: -p drops -------- +# stdout when stdout is not a TTY, e.g. piped/redirected/subprocess) --------- flags=( --print-timeout "$AGY_PRINT_TIMEOUT" --sandbox --dangerously-skip-permissions ) [ -n "$AGY_MODEL" ] && flags+=( --model "$AGY_MODEL" ) [ -n "$AGY_EFFORT" ] && flags+=( --effort "$AGY_EFFORT" ) out_file="$(mktemp)" here="$(cd "$(dirname "$0")" && pwd)" -prompt="$(cat "$prompt_file")" -rc=0 -if command -v unbuffer >/dev/null 2>&1; then - unbuffer "$AGY_BIN" "${flags[@]}" --print "$prompt" > "$out_file" 2>>"$LOG" || rc=$? -elif command -v python3 >/dev/null 2>&1; then - python3 "$here/_agy_pty.py" "$AGY_BIN" "${flags[@]}" --print "$prompt" > "$out_file" 2>>"$LOG" || rc=$? -else - die "need 'unbuffer' (from the 'expect' package) or python3 for a PTY; neither found" +: > "$LOG" + +# 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 + 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" fi -[ "$rc" -eq 0 ] || log "agy exited non-zero ($rc); checking output anyway" -tr -d '\r' < "$out_file" > "$out_file.clean" && mv "$out_file.clean" "$out_file" -have_text "$out_file" || die "no review output from agy (rc=$rc). Confirm 'agy -p \"hi\"' works for this user." +# Retry the whole agy attempt on empty/failed output: transient "agy is down" +# (backend rate-limit / local-server contention) usually clears within seconds. +# The flock (above) is held across all attempts, released after the loop. +for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do + : > "$out_file" # clear any partial output from a prior attempt + + if command -v unbuffer >/dev/null 2>&1; then + log "running agy via unbuffer (allocates a PTY) [attempt ${attempt}/${AGY_RETRIES}]" + unbuffer "$AGY_BIN" "${flags[@]}" --print "$(cat "$prompt_file")" > "$out_file" 2>>"$LOG" || true + else + log "unbuffer not found; falling back to script(1) [attempt ${attempt}/${AGY_RETRIES}]" + raw="$(mktemp)" + # `script -c` runs its command through `sh -c`, so every path in the command string is quoted for + # that inner shell: `'$here'` and `'$prompt_file'` are wrapped in single quotes (the outer double + # quotes still expand them) so a repo path containing spaces survives the word-split. + AGY_BIN="$AGY_BIN" script -qfec "'$here'/_agy_print.sh '$prompt_file' ${flags[*]}" "$raw" >/dev/null 2>>"$LOG" || true + col -b < "$raw" > "$out_file" + fi + + # normalize CRs without sed -i (avoid in-place edit footguns) + tr -d '\r' < "$out_file" > "$out_file.clean" && mv "$out_file.clean" "$out_file" + + # --- fallback: recover the answer from agy's conversation SQLite store -------- + # (belt-and-suspenders for issue #76 on hosts where the PTY trick still + # yields nothing). The schema is NOT officially documented and can change + # between agy versions -- inspect with `sqlite3 .schema` and adjust. + if ! have_text "$out_file"; then + log "print output empty; trying SQLite conversation fallback" + if command -v sqlite3 >/dev/null 2>&1 && [ -d "$CONV_DIR" ]; then + db="$(ls -t "$CONV_DIR"/*.db 2>/dev/null | head -1 || true)" + if [ -n "${db:-}" ]; then + for q in \ + "SELECT text FROM messages WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;" \ + "SELECT content FROM messages WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;" \ + "SELECT body FROM message WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;"; do + sqlite3 "$db" "$q" > "$out_file" 2>/dev/null && have_text "$out_file" && break + done + fi + fi + fi + + have_text "$out_file" && break + if [ "$attempt" -lt "$AGY_RETRIES" ]; then + delay=$(( AGY_RETRY_DELAY * attempt )) + log "no usable output (attempt ${attempt}/${AGY_RETRIES}); retrying in ${delay}s" + sleep "$delay" + fi +done +exec 9>&- 2>/dev/null || true # release the agy lock so the next queued job proceeds + +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." + exit 1 +fi -# --- assemble the comment body ------------------------------------------------ +# --- assemble the comment body ------------------------------------------------- body_file="$(mktemp)" { printf '%s\n' "$MARKER" @@ -135,13 +188,16 @@ 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 ------------------------ -# Comment deletion is best-effort by design (a delete failure must not block the -# new review), so its errors are intentionally not fatal. +# --- 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. gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ --jq ".[] | select(.body | contains(\"${MARKER}\")) | .id" 2>/dev/null \ | while read -r cid; do - [ -n "$cid" ] && gh api -X DELETE "repos/${REPO}/issues/comments/${cid}" >/dev/null 2>&1 || true + [ -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 gh pr comment "$PR" --repo "$REPO" --body-file "$body_file" From e5ccbc9ab0f67f1fed61bedb9da8cf221699f9fd Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 22 Jul 2026 00:43:30 -0400 Subject: [PATCH 4/6] ci(review): re-harden the Antigravity reviewer after the template re-sync The template re-sync in 9875dabb pulled the upstream reviewer over the hardened version from 0237135d and silently reverted five of its fixes -- three of which CodeRabbit had already marked "Addressed in commit 0237135" on this PR, so the resolved threads no longer described the tree. This restores the hardening on top of the re-sync's genuine improvements (job-level concurrency, the agy flock, the retry loop), and closes the remaining review findings. Trigger and execution gating (workflow) The `pull_request` fork check was dropped, so any fork PR could schedule work on the self-hosted runner. GitHub's "require approval for outside collaborators" is not a substitute: its default covers only FIRST-TIME contributors, so a returning outside contributor gets a runner on the maintainer's own hardware. The `head.repo.full_name == github.repository` condition is restored, alongside the `issue_comment` author_association gate. The checkout now takes the DEFAULT BRANCH rather than the PR head. The job executes the checked-out `scripts/agy-review.sh` with a token in scope, so checking out the PR let the reviewed change rewrite its own reviewer -- code execution on the runner from an untrusted diff. Review content is unaffected: the diff comes from the API via `gh pr diff`, not the working tree. The consequence is worth stating plainly: a PR that edits the reviewer or the style guide is reviewed by the version already on main. `persist-credentials: false` joins it per the repo-wide rule from PR #319, and the action moves to `actions/checkout@v7`, matching the other 19 checkout sites (this was the only `@v4` in the repo). Token exposure at the agy boundary `--dangerously-skip-permissions` stays: it is required for headless operation -- without it agy blocks on an interactive approval no one is there to answer and the run times out. Removing the flag, as suggested, would break the feature rather than secure it. What the flag actually costs is the approval gate, so the agent could act on instructions embedded in the attacker-controlled diff it is reviewing; the exposure that matters is the GitHub token in the job environment. agy is therefore launched through `env -u GH_TOKEN -u GITHUB_TOKEN`. `gh` runs in this script, before and after, and agy never needs the token. `--sandbox` still confines filesystem and network access. Command injection in the script(1) fallback The re-sync replaced the argv-safe `_agy_pty.py` wrapper with `script -qfec "... ${flags[*]}"`. `script -c` takes a command STRING and runs it through `sh -c`, so interpolating the flags array raw makes any space or metacharacter in AGY_MODEL / AGY_EFFORT / AGY_PRINT_TIMEOUT -- all env-settable -- into shell syntax. The command string is now assembled with `printf '%q '`, which emits a shell-safe rendering of each element. Verified: a flag value of `5m; touch /tmp/AGY_PWNED` now arrives as one literal argv element and the injected command does not run. Single-argument size ceiling (a live E2BIG, not a hypothetical) The prompt reaches agy as ONE argv string, and Linux caps a single argument at MAX_ARG_STRLEN = 32 * PAGE_SIZE = 128 KiB -- a separate and far lower ceiling than ARG_MAX (4 MiB here). The workflow shipped `MAX_DIFF_BYTES: "200000"`, which exceeds it outright, and capping the diff alone would not be sufficient anyway since the instruction boilerplate and the style guide ride in the same string. Measured locally on this kernel: 131000 bytes execs, 131073 fails, 200000 fails. The diff budget drops to 90 KB and a new MAX_PROMPT_BYTES (120 KB) caps the ASSEMBLED prompt. Conversation-database fallback could publish an unrelated session The `ls -t | head -1` fallback takes the globally newest conversation DB, so when agy fails BEFORE creating a conversation it posts the last assistant message from whatever else ran on that host -- another repo's review, or the owner's interactive chat -- into a public PR comment. The search is now scoped with `find -newermt` to a DB this attempt actually touched; with none, the fallback yields nothing and the retry/failure path takes over. This also retires the `ls`-parsing shellcheck flagged as SC2012. Prior-comment cleanup deleted by marker alone The HTML marker is plain text in a public comment and the filter ignored the author, so anyone who pasted the marker into a comment had it deleted on the next run -- and the job holds `issues: write`, so the delete succeeded. Deletion is now restricted to comments authored by `github-actions[bot]`. Also: the log path gains a run-ID suffix (a fixed name collides between concurrent jobs whenever RUNNER_TEMP is unset and /tmp is shared), the script-side author_association re-check is restored as defense in depth against a future workflow edit losing the gate, and the bare `|| true` around the agy invocation now records the exit code -- it had erased the difference between "agy failed" and "agy produced nothing". CHANGELOG gains an [Unreleased] Added + Security entry. CI-only: no crate, no shipped artifact, and nothing under crates/rustynes-{cpu,ppu,apu,mappers,core} is touched, so the deterministic chip stack and every golden vector are unchanged by construction. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/antigravity-review.yml | 40 +++++++++--- CHANGELOG.md | 23 +++++++ scripts/agy-review.sh | 81 +++++++++++++++++++++--- 3 files changed, 126 insertions(+), 18 deletions(-) diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml index 252bd2e4..22157c87 100644 --- a/.github/workflows/antigravity-review.yml +++ b/.github/workflows/antigravity-review.yml @@ -19,14 +19,22 @@ permissions: jobs: review: - # PR opened/reopened, OR an `/agy-review` comment on a PR from a trusted author. - # The comment path is gated on author_association so an untrusted outside user cannot run the - # self-hosted runner by commenting `/agy-review` (belt-and-suspenders with the repo's - # Settings -> Actions -> "Fork pull request workflows -> require approval for all outside - # collaborators"). The `pull_request` trigger is already `opened`/`reopened` only. + # SECURITY GATE (a self-hosted runner on a PUBLIC repo executes on real hardware + # and spends the owner's Google AI Ultra quota, so "who can start a run" is the + # primary control): + # - pull_request: only from a branch in THIS repo. A FORK PR must never schedule + # work on the self-hosted host. GitHub's "require approval for outside + # collaborators" setting is belt-and-suspenders, not the gate — its default only + # covers *first-time* contributors, so a returning outside contributor would + # otherwise get a runner. + # - issue_comment: only `/agy-review` from someone with write access + # (OWNER/MEMBER/COLLABORATOR), so a stranger cannot trigger execution by + # commenting. `scripts/agy-review.sh` re-checks this defensively. if: >- - github.event_name == 'pull_request' || - (github.event.issue.pull_request != null && + (github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository) || + (github.event_name == 'issue_comment' && + github.event.issue.pull_request != null && startsWith(github.event.comment.body, '/agy-review') && contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) # Concurrency at the JOB level (NOT workflow level): a job skipped by the `if:` @@ -40,10 +48,20 @@ jobs: cancel-in-progress: false runs-on: [self-hosted, agy] 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, + # so taking those scripts from the PR would let the reviewed change rewrite its own + # reviewer. The review content is unaffected: the diff is fetched from the API by + # `gh pr diff`, not from the working tree. Consequence worth knowing: a PR that + # edits the reviewer or the style guide is reviewed by the version already on main. - name: Check out repo (for the style guide + scripts) - uses: actions/checkout@v4 + uses: actions/checkout@v7 with: + ref: ${{ github.event.repository.default_branch }} fetch-depth: 1 + # Repo-wide rule (PR #319): no checkout persists credentials. Nothing here + # performs an authenticated git network operation — `gh` uses GH_TOKEN. + persist-credentials: false - name: Run Antigravity review env: @@ -51,7 +69,11 @@ jobs: # --- optional overrides (uncomment to change) --- # AGY_MODEL: gemini-3-pro # default: agy's configured model AGY_EFFORT: high # low|medium|high - MAX_DIFF_BYTES: "200000" # truncate huge diffs (~200 KB) + # Diff budget. The prompt is passed to agy as a SINGLE argv string, and Linux + # caps one argument at MAX_ARG_STRLEN = 32 * PAGE_SIZE = 128 KiB (this is a + # separate, much lower ceiling than ARG_MAX). 90 KB leaves room for the + # instruction boilerplate + style guide; the script enforces the total cap. + MAX_DIFF_BYTES: "90000" # STYLE_GUIDE: .github/agy-review.md # style guide, loaded if present run: | chmod +x scripts/agy-review.sh scripts/_agy_print.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 71b1508e..5942c5f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,29 @@ cycle-accurate core later replaced. ## [Unreleased] +### Added + +- Antigravity PR reviewer (`.github/workflows/antigravity-review.yml` + + `scripts/agy-review.sh`): an automated first-pass code review on a self-hosted + runner, driven by the `agy` CLI's OAuth session (Google AI Ultra, no metered + API key). Runs on PR open/reopen and on an `/agy-review` comment from a + contributor with write access, and replaces its own prior comment each run. + Review priorities live in `.github/agy-review.md`. CI-only — no crate, no + shipped artifact, and no emulation-core change. + +### Security + +- The reviewer executes on the maintainer's own hardware with a token in scope, + so its trigger and execution paths are gated accordingly: fork PRs cannot + schedule the job, the automation scripts are checked out from the default + branch rather than the PR head (a PR cannot rewrite its own reviewer), `agy` + is launched with `GH_TOKEN`/`GITHUB_TOKEN` removed from its environment, the + `script(1)` fallback quotes its argv with `printf %q` instead of interpolating + env-settable flags into a shell string, the conversation-database fallback is + scoped to the current invocation (it can no longer publish an unrelated + session's output), and prior-comment cleanup is restricted to comments + authored by the workflow's own bot. + ## [2.2.2] - 2026-07-21 - "Conduit" (libretro buildbot 10/10 + CI supply-chain hardening + single-source toolchain) A **build, distribution, and CI-integrity patch**. It carries RustyNES onto diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 4ac539f0..8b0cc9c1 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -19,11 +19,18 @@ command -v "$AGY_BIN" >/dev/null 2>&1 || AGY_BIN="$HOME/.local/bin/agy" AGY_MODEL="${AGY_MODEL:-}" # empty = agy's configured default (Gemini 3.x Pro) AGY_EFFORT="${AGY_EFFORT:-high}" # low|medium|high AGY_PRINT_TIMEOUT="${AGY_PRINT_TIMEOUT:-5m}" -MAX_DIFF_BYTES="${MAX_DIFF_BYTES:-200000}" # truncate very large diffs (~200 KB) +MAX_DIFF_BYTES="${MAX_DIFF_BYTES:-90000}" # truncate very large diffs (~90 KB) +# Hard ceiling on the ASSEMBLED prompt. The prompt reaches agy as one argv string, and +# Linux caps a single argument at MAX_ARG_STRLEN = 32 * PAGE_SIZE = 128 KiB regardless of +# ARG_MAX; exceeding it fails the exec with E2BIG. Capping only the diff is not enough -- +# the boilerplate and the style guide ride in the same string. +MAX_PROMPT_BYTES="${MAX_PROMPT_BYTES:-120000}" STYLE_GUIDE="${STYLE_GUIDE:-.github/agy-review.md}" # repo-relative; loaded if present # (dedicated name -- avoids colliding with GEMINI.md/AGENTS.md) CONV_DIR="${CONV_DIR:-$HOME/.gemini/antigravity-cli/conversations}" -LOG="${RUNNER_TEMP:-/tmp}/agy-review.log" +# Per-run log path. A fixed name would collide between concurrent jobs whenever +# RUNNER_TEMP is unset (local runs fall back to /tmp, which is shared). +LOG="${RUNNER_TEMP:-/tmp}/agy-review-${GITHUB_RUN_ID:-$$}.log" AGY_LOCK="${AGY_LOCK:-$HOME/.gemini/antigravity-cli/.agy-review.lock}" AGY_LOCK_WAIT="${AGY_LOCK_WAIT:-600}" # seconds to wait for the agy lock before proceeding AGY_RETRIES="${AGY_RETRIES:-3}" # attempts to get a usable agy response @@ -43,11 +50,19 @@ case "${GITHUB_EVENT_NAME:-}" in issue_comment) is_pr="$(jq -r '.issue.pull_request // empty' "$GITHUB_EVENT_PATH")" body="$(jq -r '.comment.body // ""' "$GITHUB_EVENT_PATH")" + assoc="$(jq -r '.comment.author_association // ""' "$GITHUB_EVENT_PATH")" [ -n "$is_pr" ] || { log "comment is not on a PR; skipping"; exit 0; } case "$body" in /agy-review*) : ;; *) log "comment is not an /agy-review command; skipping"; exit 0 ;; esac + # Defense in depth: the workflow `if:` already gates on author_association, but this + # script is also runnable by hand and by any future caller. Re-check here so the gate + # cannot be lost by an edit to the workflow alone. + case "$assoc" in + OWNER|MEMBER|COLLABORATOR) : ;; + *) log "comment author association '$assoc' lacks write access; skipping"; exit 0 ;; + esac PR="$(jq -r '.issue.number' "$GITHUB_EVENT_PATH")" ;; *) @@ -103,12 +118,30 @@ EOF cat "$diff_file" } > "$prompt_file" +# Enforce the single-argument ceiling on the WHOLE prompt (see MAX_PROMPT_BYTES above). +# MAX_DIFF_BYTES alone cannot guarantee this: a long style guide can push the assembled +# prompt past 128 KiB even with a modest diff, and the exec then fails with E2BIG. +if [ "$(wc -c < "$prompt_file")" -gt "$MAX_PROMPT_BYTES" ]; then + head -c "$MAX_PROMPT_BYTES" "$prompt_file" > "$prompt_file.cut" && mv "$prompt_file.cut" "$prompt_file" + truncated=$'\n\n> Note: the review prompt was truncated to '"${MAX_PROMPT_BYTES}"$' bytes (single-argument limit).' + log "prompt truncated to ${MAX_PROMPT_BYTES} bytes" +fi + # --- run agy headless, under a PTY (works around agy issue #76: -p drops -------- # stdout when stdout is not a TTY, e.g. piped/redirected/subprocess) --------- flags=( --print-timeout "$AGY_PRINT_TIMEOUT" --sandbox --dangerously-skip-permissions ) [ -n "$AGY_MODEL" ] && flags+=( --model "$AGY_MODEL" ) [ -n "$AGY_EFFORT" ] && flags+=( --effort "$AGY_EFFORT" ) +# `--dangerously-skip-permissions` is REQUIRED for headless operation -- without it agy +# blocks on an interactive approval prompt that no one is there to answer, and the run +# just times out. What it removes is the approval gate, so the agent could act on +# instructions embedded in the (attacker-controlled) diff it is reviewing. The exposure +# that actually matters is the GitHub token in this job's environment, so agy is launched +# WITHOUT it: `gh` calls happen in this script, before and after, and agy never needs it. +# `--sandbox` still confines filesystem and network access. +agy_env=( env -u GH_TOKEN -u GITHUB_TOKEN ) + out_file="$(mktemp)" here="$(cd "$(dirname "$0")" && pwd)" : > "$LOG" @@ -128,19 +161,31 @@ fi # The flock (above) is held across all attempts, released after the loop. for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do : > "$out_file" # clear any partial output from a prior attempt + # Reference instant for the SQLite fallback below: only a conversation DB modified at + # or after this point can plausibly belong to this attempt. + attempt_start="$(date +%s)" + rc=0 if command -v unbuffer >/dev/null 2>&1; then log "running agy via unbuffer (allocates a PTY) [attempt ${attempt}/${AGY_RETRIES}]" - unbuffer "$AGY_BIN" "${flags[@]}" --print "$(cat "$prompt_file")" > "$out_file" 2>>"$LOG" || true + "${agy_env[@]}" unbuffer "$AGY_BIN" "${flags[@]}" --print "$(cat "$prompt_file")" \ + > "$out_file" 2>>"$LOG" || rc=$? else log "unbuffer not found; falling back to script(1) [attempt ${attempt}/${AGY_RETRIES}]" raw="$(mktemp)" - # `script -c` runs its command through `sh -c`, so every path in the command string is quoted for - # that inner shell: `'$here'` and `'$prompt_file'` are wrapped in single quotes (the outer double - # quotes still expand them) so a repo path containing spaces survives the word-split. - AGY_BIN="$AGY_BIN" script -qfec "'$here'/_agy_print.sh '$prompt_file' ${flags[*]}" "$raw" >/dev/null 2>>"$LOG" || true + # `script -c` takes a COMMAND STRING and runs it through `sh -c`, so every word must be + # quoted for that inner shell. Interpolating `${flags[*]}` raw (as the upstream template + # does) is a command-injection surface: any space or metacharacter in AGY_MODEL / + # AGY_EFFORT / AGY_PRINT_TIMEOUT -- all env-settable -- becomes shell syntax. `printf %q` + # emits a shell-safe rendering of each element, so the string is exactly the argv we mean. + cmd="$(printf '%q ' "$here/_agy_print.sh" "$prompt_file" "${flags[@]}")" + "${agy_env[@]}" AGY_BIN="$AGY_BIN" script -qfec "$cmd" "$raw" >/dev/null 2>>"$LOG" || rc=$? col -b < "$raw" > "$out_file" fi + # Not fatal on its own -- agy can exit non-zero and still have printed a usable review, + # and the retry loop below is the real gate. But record it: a silent `|| true` hid the + # difference between "agy failed" and "agy produced nothing". + [ "$rc" -eq 0 ] || log "agy exited non-zero (${rc}); checking output anyway (see $LOG)" # normalize CRs without sed -i (avoid in-place edit footguns) tr -d '\r' < "$out_file" > "$out_file.clean" && mv "$out_file.clean" "$out_file" @@ -149,10 +194,23 @@ for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do # (belt-and-suspenders for issue #76 on hosts where the PTY trick still # yields nothing). The schema is NOT officially documented and can change # between agy versions -- inspect with `sqlite3 .schema` and adjust. + # + # SCOPED TO THIS ATTEMPT ON PURPOSE. Taking the globally newest DB (the upstream + # template's `ls -t | head -1`) means that when agy fails BEFORE creating a + # conversation, this posts the last assistant message from whatever unrelated + # session ran on this host -- another repo's review, or the owner's interactive + # chat -- straight into a public PR comment. `find -newermt` restricts the search + # to a DB this attempt actually touched; if there is none, the fallback yields + # nothing and the retry/failure path takes over. (`find` also avoids the `ls` + # parsing that shellcheck SC2012 flags.) if ! have_text "$out_file"; then log "print output empty; trying SQLite conversation fallback" if command -v sqlite3 >/dev/null 2>&1 && [ -d "$CONV_DIR" ]; then - db="$(ls -t "$CONV_DIR"/*.db 2>/dev/null | head -1 || true)" + db="$(find "$CONV_DIR" -maxdepth 1 -name '*.db' -newermt "@$(( attempt_start - 1 ))" \ + -printf '%T@ %p\n' 2>/dev/null | sort -rn | head -1 | cut -d' ' -f2- || true)" + if [ -z "${db:-}" ]; then + log "no conversation DB from this attempt; skipping fallback (refusing to read an unrelated session)" + fi if [ -n "${db:-}" ]; then for q in \ "SELECT text FROM messages WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;" \ @@ -191,8 +249,13 @@ body_file="$(mktemp)" # --- 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 a correctness requirement, not a nicety: the marker is plain text in a +# public comment, so matching on the marker ALONE lets anyone who pastes it into a comment have +# this workflow delete that comment on the next run -- the job holds `issues: write`, so the +# delete succeeds. Restricting to the bot that posts these comments keeps deletion to our own. gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ - --jq ".[] | select(.body | contains(\"${MARKER}\")) | .id" 2>/dev/null \ + --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 From 675fa5240ea1a7b0cb0b3c1a2eabfdcdd75bef3f Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 22 Jul 2026 00:58:11 -0400 Subject: [PATCH 5/6] ci(review): drop the shared conversation-store fallback; state the trust model Two Major findings from the re-review of e5ccbc9, both adopted. Remove the SQLite conversation-store fallback entirely The previous round scoped it with `find -newermt` against a per-attempt timestamp, which was an improvement but not the fix. mtime bounds a time WINDOW, not ownership: `$HOME/.gemini/antigravity-cli/conversations` is shared across every agy session on the host, and the flock in this script serializes review JOBS only -- it does nothing about the owner's own interactive agy usage on the same machine. Any session that touches a DB during the window could still be selected and posted into a PUBLIC pull-request comment. The sound version of this fallback needs a conversation ID or a private store tied to this invocation, and agy exposes neither. Relocating the store per run means relocating $HOME, which is where the OAuth session lives -- the thing that makes these reviews free under Ultra. Since ownership cannot be established, the fallback fails closed instead of guessing: no usable stdout means retry, then fail the job. Losing a review is recoverable (`/agy-review` re-runs it); publishing someone else's conversation is not. This deletes the fallback block along with the now-unused CONV_DIR and attempt_start. The PTY path (unbuffer, else script(1)) was always the actual mechanism; the store read was belt-and-suspenders for agy issue #76. Stop presenting `--sandbox` as the isolation boundary The prior comment claimed "--sandbox still confines filesystem and network access." That overclaims. Upstream antigravity-cli#36 reports that `--dangerously-skip-permissions` can auto-approve the very prompts required to escape the sandbox, and there is a published prompt-injection -> RCE/sandbox- escape writeup against the CLI. The flag stays (defense in depth, no cost), but the comment no longer rests any argument on it. The invocation site now documents where the boundary actually is: the workflow's `if:`. A fork PR cannot schedule this job, and `/agy-review` requires OWNER/MEMBER/COLLABORATOR, so a same-repo branch means the diff was pushed by someone who already holds write access -- and has far more direct means available than prompt injection. No external party's diff reaches agy. That gate is the entire defense, and the comment says so in the imperative: weaken it (add `pull_request_target`, drop the fork check) and this becomes remote code execution on the maintainer's machine. The reviewer's recommended remedy -- an ephemeral account or VM -- is recorded as the prerequisite for ever opening this to untrusted diffs, rather than silently declined. It is incompatible with the $HOME-resident OAuth session today, which is a constraint worth writing down rather than rediscovering. CHANGELOG's [Unreleased] Security entry is updated to match on both points. CI-only; no crate and no emulation-core file is touched. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 9 +++-- scripts/agy-review.sh | 86 ++++++++++++++++++++++--------------------- 2 files changed, 51 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5942c5f6..b084a85f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,9 +33,12 @@ cycle-accurate core later replaced. is launched with `GH_TOKEN`/`GITHUB_TOKEN` removed from its environment, the `script(1)` fallback quotes its argv with `printf %q` instead of interpolating env-settable flags into a shell string, the conversation-database fallback is - scoped to the current invocation (it can no longer publish an unrelated - session's output), and prior-comment cleanup is restricted to comments - authored by the workflow's own bot. + removed outright (agy's store is shared per-user, so it could copy an unrelated + session into a public comment, and agy exposes no per-invocation store to scope + it to), and prior-comment cleanup is restricted to comments authored by the + workflow's own bot. The trigger gate — not `agy --sandbox`, which upstream + reports can be auto-approved away — is the trust boundary, and is documented as + such at the invocation site. ## [2.2.2] - 2026-07-21 - "Conduit" (libretro buildbot 10/10 + CI supply-chain hardening + single-source toolchain) diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 8b0cc9c1..1126d55f 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -27,7 +27,6 @@ MAX_DIFF_BYTES="${MAX_DIFF_BYTES:-90000}" # truncate very large diffs (~90 KB) MAX_PROMPT_BYTES="${MAX_PROMPT_BYTES:-120000}" STYLE_GUIDE="${STYLE_GUIDE:-.github/agy-review.md}" # repo-relative; loaded if present # (dedicated name -- avoids colliding with GEMINI.md/AGENTS.md) -CONV_DIR="${CONV_DIR:-$HOME/.gemini/antigravity-cli/conversations}" # Per-run log path. A fixed name would collide between concurrent jobs whenever # RUNNER_TEMP is unset (local runs fall back to /tmp, which is shared). LOG="${RUNNER_TEMP:-/tmp}/agy-review-${GITHUB_RUN_ID:-$$}.log" @@ -133,13 +132,34 @@ flags=( --print-timeout "$AGY_PRINT_TIMEOUT" --sandbox --dangerously-skip-permis [ -n "$AGY_MODEL" ] && flags+=( --model "$AGY_MODEL" ) [ -n "$AGY_EFFORT" ] && flags+=( --effort "$AGY_EFFORT" ) -# `--dangerously-skip-permissions` is REQUIRED for headless operation -- without it agy -# blocks on an interactive approval prompt that no one is there to answer, and the run -# just times out. What it removes is the approval gate, so the agent could act on -# instructions embedded in the (attacker-controlled) diff it is reviewing. The exposure -# that actually matters is the GitHub token in this job's environment, so agy is launched -# WITHOUT it: `gh` calls happen in this script, before and after, and agy never needs it. -# `--sandbox` still confines filesystem and network access. +# TRUST MODEL -- read this before changing the trigger conditions. +# +# `--dangerously-skip-permissions` is REQUIRED for headless operation: without it agy +# blocks on an interactive approval prompt that no one is present to answer, and the run +# burns --print-timeout and exits empty. What it removes is the approval gate, so agy +# could act on instructions embedded in the diff it is reviewing (prompt injection). +# +# `--sandbox` is NOT a security boundary and must not be treated as one. Upstream +# antigravity-cli#36 reports that --dangerously-skip-permissions can auto-approve the +# very prompts needed to escape the sandbox, and there is a published prompt-injection +# -> RCE/sandbox-escape writeup against the CLI. It is kept for defense in depth only. +# +# The ACTUAL boundary is upstream of this script, in the workflow's `if:`: a fork PR +# cannot schedule this job, and `/agy-review` requires OWNER/MEMBER/COLLABORATOR. A +# same-repo branch means the diff was pushed by someone who already has write access to +# this repository -- and therefore has far more direct means than prompt injection. +# There is no path by which an external party's diff reaches agy. That gate is the whole +# defense: weaken it (e.g. add `pull_request_target`, or drop the fork check) and this +# becomes remote code execution on the maintainer's machine. +# +# Within that model, two cheap reductions are still worth having: +# * agy runs with GH_TOKEN/GITHUB_TOKEN removed from its environment -- `gh` runs in +# this script, before and after, and agy has no use for the token. +# * the conversation-store fallback is gone (see the loop below), so nothing from the +# shared per-user agy state can be copied into a public comment. +# Neither isolates the host. Doing that properly needs an ephemeral account or VM, which +# is incompatible with the OAuth session in $HOME that makes these reviews free under +# Ultra; if this is ever opened to untrusted diffs, that isolation becomes mandatory. agy_env=( env -u GH_TOKEN -u GITHUB_TOKEN ) out_file="$(mktemp)" @@ -161,9 +181,6 @@ fi # The flock (above) is held across all attempts, released after the loop. for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do : > "$out_file" # clear any partial output from a prior attempt - # Reference instant for the SQLite fallback below: only a conversation DB modified at - # or after this point can plausibly belong to this attempt. - attempt_start="$(date +%s)" rc=0 if command -v unbuffer >/dev/null 2>&1; then @@ -190,37 +207,24 @@ for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do # normalize CRs without sed -i (avoid in-place edit footguns) tr -d '\r' < "$out_file" > "$out_file.clean" && mv "$out_file.clean" "$out_file" - # --- fallback: recover the answer from agy's conversation SQLite store -------- - # (belt-and-suspenders for issue #76 on hosts where the PTY trick still - # yields nothing). The schema is NOT officially documented and can change - # between agy versions -- inspect with `sqlite3 .schema` and adjust. + # --- NO conversation-store fallback, deliberately ----------------------------- + # The upstream template recovers the answer from agy's SQLite conversation store + # when the PTY trick yields nothing (belt-and-suspenders for agy issue #76). That + # store is SHARED per-user: `$HOME/.gemini/antigravity-cli/conversations` holds + # every session on the host, including the owner's interactive chats and reviews + # for other repos. Reading it and posting the result to a PUBLIC pull request + # comment risks publishing an unrelated conversation. # - # SCOPED TO THIS ATTEMPT ON PURPOSE. Taking the globally newest DB (the upstream - # template's `ls -t | head -1`) means that when agy fails BEFORE creating a - # conversation, this posts the last assistant message from whatever unrelated - # session ran on this host -- another repo's review, or the owner's interactive - # chat -- straight into a public PR comment. `find -newermt` restricts the search - # to a DB this attempt actually touched; if there is none, the fallback yields - # nothing and the retry/failure path takes over. (`find` also avoids the `ls` - # parsing that shellcheck SC2012 flags.) - if ! have_text "$out_file"; then - log "print output empty; trying SQLite conversation fallback" - if command -v sqlite3 >/dev/null 2>&1 && [ -d "$CONV_DIR" ]; then - db="$(find "$CONV_DIR" -maxdepth 1 -name '*.db' -newermt "@$(( attempt_start - 1 ))" \ - -printf '%T@ %p\n' 2>/dev/null | sort -rn | head -1 | cut -d' ' -f2- || true)" - if [ -z "${db:-}" ]; then - log "no conversation DB from this attempt; skipping fallback (refusing to read an unrelated session)" - fi - if [ -n "${db:-}" ]; then - for q in \ - "SELECT text FROM messages WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;" \ - "SELECT content FROM messages WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;" \ - "SELECT body FROM message WHERE role='assistant' ORDER BY rowid DESC LIMIT 1;"; do - sqlite3 "$db" "$q" > "$out_file" 2>/dev/null && have_text "$out_file" && break - done - fi - fi - fi + # Narrowing by mtime is not sufficient -- it bounds a time window, not ownership, + # and the flock above serializes review JOBS, not the owner's own agy usage. The + # only sound version of this fallback needs a conversation ID (or a private store) + # tied to THIS invocation, and agy exposes neither: the OAuth session lives in + # $HOME, so it cannot be relocated per run without losing the login that makes + # these reviews free under Ultra. + # + # So it fails closed instead: no usable stdout means retry, then fail the job. + # Losing a review is recoverable (`/agy-review` re-runs it); publishing someone + # else's session into a public comment is not. have_text "$out_file" && break if [ "$attempt" -lt "$AGY_RETRIES" ]; then From 98d0162f597e9262875587fb752d303e7a77888f Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 22 Jul 2026 01:13:24 -0400 Subject: [PATCH 6/6] ci(review): close the fork-PR hole on the /agy-review comment path The trust-model comment added in 675fa52 claimed "there is no path by which an external party's diff reaches agy." That claim was false, and the review caught it: the `issue_comment` branch authorizes the COMMENTER, never the DIFF. A collaborator typing `/agy-review` on a fork PR scheduled this job against that fork's diff, which `gh pr diff` fetches by number regardless of where the head branch lives. So an external diff did reach agy -- under --dangerously-skip-permissions, on the maintainer's own machine -- through the single most natural use of the feature: a maintainer asking for a review of a contributor's pull request. The workflow `if:` cannot fix this. An `issue_comment` payload carries no head-repo information at all -- only `.issue.pull_request` as a marker that the comment is on a PR -- so the same-repo condition that guards the `pull_request` trigger has nothing to evaluate. The check has to happen after the PR number is resolved, which is here. `gh pr view` now requests `isCrossRepository` alongside the title, and a fork PR exits 0 with a message pointing at the alternatives (review by hand, or push the branch into this repo). The metadata fetch is also now FAIL-CLOSED: it used to fall back to `{}` on error, which was harmless when the only field read was the title and is not harmless when the same document decides whether an untrusted diff reaches agy. A lookup failure and "same-repo" must not be the same outcome. Note the `jq` filter is `.isCrossRepository`, deliberately NOT `.isCrossRepository // empty`: jq's `//` alternative fires on `false` as well as on null, so the `// empty` form collapses the same-repo case into the unknown case and refuses every legitimate review. Caught by table-testing the gate against all four input shapes (true / false / `{}` / non-JSON) before pushing; the first version had exactly that bug. The trust model at the invocation site is rewritten to describe what is actually enforced: the boundary is "agy only ever sees a same-repo diff", and it takes TWO checks because neither trigger is covered by one -- the workflow for `pull_request`, this script for `issue_comment`. Both are named as load-bearing, with the failure mode spelled out, so a future edit cannot remove either without reading why it exists. The CHANGELOG entry is corrected to match rather than left restating the overstated version. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 11 ++++++--- scripts/agy-review.sh | 56 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b084a85f..180a9a76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,9 +36,14 @@ cycle-accurate core later replaced. removed outright (agy's store is shared per-user, so it could copy an unrelated session into a public comment, and agy exposes no per-invocation store to scope it to), and prior-comment cleanup is restricted to comments authored by the - workflow's own bot. The trigger gate — not `agy --sandbox`, which upstream - reports can be auto-approved away — is the trust boundary, and is documented as - such at the invocation site. + workflow's own bot. The trust boundary is "`agy` only ever sees a same-repo + diff", enforced by two checks because neither trigger is covered by one: the + workflow rejects fork PRs on `pull_request`, and the script rejects them again + on the `issue_comment` path, where the payload carries no head-repo field and + `/agy-review` on a fork PR would otherwise feed in an external diff. + Authorizing the commenter is not the same as trusting the diff. `agy --sandbox` + is explicitly *not* part of that boundary — upstream reports it can be + auto-approved away — and the invocation site says so. ## [2.2.2] - 2026-07-21 - "Conduit" (libretro buildbot 10/10 + CI supply-chain hardening + single-source toolchain) diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 1126d55f..ddbeebcd 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -76,10 +76,39 @@ log "reviewing ${REPO}#${PR}" diff_file= meta_file= prompt_file= out_file= raw= body_file= trap 'rm -f "$diff_file" "$meta_file" "$prompt_file" "$out_file" "$raw" "$body_file"' EXIT -# --- fetch the diff + metadata ------------------------------------------------- -diff_file="$(mktemp)"; meta_file="$(mktemp)" +# --- metadata first, because the fork gate depends on it ----------------------- +# FAIL-CLOSED. The old form fell back to `{}` when `gh pr view` failed, which was +# harmless when the only field read was the title -- it is not harmless now that +# isCrossRepository gates whether an untrusted diff reaches agy. A lookup failure must +# never be indistinguishable from "same-repo". +meta_file="$(mktemp)" +gh pr view "$PR" --repo "$REPO" --json title,isCrossRepository > "$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:` +# blocks fork PRs on the `pull_request` trigger, but it CANNOT do so on `issue_comment`: +# that payload carries no head-repo information at all, so a collaborator commenting +# `/agy-review` on a fork PR would otherwise schedule this job against an external diff. +# A trusted person typing the command does not make the DIFF trusted -- and the diff is +# what agy ingests, under --dangerously-skip-permissions, on the maintainer's machine. +# Enforced here because this is the first point where the answer is knowable. +# NOT `.isCrossRepository // empty` -- jq's `//` treats `false` as absent, so the +# alternative fires on exactly the same-repo case this gate is meant to admit, and every +# legitimate review would be refused. Read the raw value and match all three shapes. +is_fork="$(jq -r '.isCrossRepository' "$meta_file")" +case "$is_fork" in + true) + log "PR #${PR} is from a fork; refusing to run agy on an external diff" + log "(review it by hand, or push the branch into this repo first)" + exit 0 + ;; + false) : ;; + *) log "could not determine whether PR #${PR} is cross-repository; refusing"; exit 1 ;; +esac + +# --- fetch the diff ------------------------------------------------------------ +diff_file="$(mktemp)" gh pr diff "$PR" --repo "$REPO" > "$diff_file" || { log "gh pr diff failed"; exit 1; } -gh pr view "$PR" --repo "$REPO" --json title > "$meta_file" 2>/dev/null || echo '{}' > "$meta_file" if ! have_text "$diff_file"; then log "empty diff; nothing to review"; exit 0; fi @@ -144,13 +173,20 @@ flags=( --print-timeout "$AGY_PRINT_TIMEOUT" --sandbox --dangerously-skip-permis # very prompts needed to escape the sandbox, and there is a published prompt-injection # -> RCE/sandbox-escape writeup against the CLI. It is kept for defense in depth only. # -# The ACTUAL boundary is upstream of this script, in the workflow's `if:`: a fork PR -# cannot schedule this job, and `/agy-review` requires OWNER/MEMBER/COLLABORATOR. A -# same-repo branch means the diff was pushed by someone who already has write access to -# this repository -- and therefore has far more direct means than prompt injection. -# There is no path by which an external party's diff reaches agy. That gate is the whole -# defense: weaken it (e.g. add `pull_request_target`, or drop the fork check) and this -# becomes remote code execution on the maintainer's machine. +# The ACTUAL boundary is "agy only ever sees a same-repo diff", and it takes TWO checks +# because no single one covers both triggers: +# * the workflow `if:` rejects fork PRs on `pull_request`, and requires +# OWNER/MEMBER/COLLABORATOR on `issue_comment`; +# * the isCrossRepository check above rejects fork PRs on the `issue_comment` path, +# which the workflow cannot do -- that payload carries no head-repo field, so +# `/agy-review` on a fork PR would otherwise feed an external diff straight in. +# Authorizing the COMMENTER is not the same as trusting the DIFF, and the diff is +# what agy ingests. +# With both in place, a diff reaching agy was pushed to a branch of this repository by +# someone who already holds write access -- and therefore has far more direct means +# available than prompt injection. Those two checks are the whole defense: weaken either +# (add `pull_request_target`, drop the fork check, let the metadata lookup fail open) and +# this becomes remote code execution on the maintainer's machine. # # Within that model, two cheap reductions are still worth having: # * agy runs with GH_TOKEN/GITHUB_TOKEN removed from its environment -- `gh` runs in