diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml index 6da5f7e6..fa3d9d0a 100644 --- a/.github/workflows/antigravity-review.yml +++ b/.github/workflows/antigravity-review.yml @@ -8,7 +8,11 @@ name: Antigravity PR Review on: pull_request: - types: [opened, reopened] + # `synchronize` auto-re-reviews on every push to a same-repo PR. It is safe + # under the default-branch checkout below: the reviewer always runs from the + # default branch, and the PR only supplies the diff (data fed to agy), never + # the reviewer code. + types: [opened, reopened, synchronize] issue_comment: types: [created] @@ -29,7 +33,9 @@ jobs: # 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. + # commenting. `scripts/agy-review.sh` re-checks the fork status defensively, + # because the `issue_comment` payload carries no head-repo field for the `if:` + # to gate on — a trusted commenter is not the same as a trusted diff. if: >- (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) || @@ -55,7 +61,7 @@ jobs: # for a diff over GitHub's 20,000-line API limit, from a local `git diff` of fetched # objects) — never from the checked-out working tree, which stays on the default # branch throughout. Consequence worth knowing: a PR that edits the reviewer or the - # style guide is reviewed by the version already on main. + # 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 with: @@ -64,11 +70,10 @@ jobs: # needs on demand, so a full history clone would be paid on every run for a path # most runs never take. fetch-depth: 1 - # Repo-wide rule (PR #319): no checkout persists credentials. The large-diff - # fallback does perform an authenticated fetch, but supplies the token per - # command via `http.extraheader` rather than persisting it into .git/config — - # which is the point of the rule, since agy executes with the repo as its - # workspace. + # No checkout should persist credentials: agy executes with the repo as its + # workspace, so a token left in .git/config would be reachable by anything the + # review runs. The large-diff fallback still performs an authenticated fetch, but + # supplies the token per command via `http.extraheader` rather than persisting it. persist-credentials: false - name: Run Antigravity review @@ -77,13 +82,13 @@ jobs: # --- optional overrides (uncomment to change) --- # AGY_MODEL: gemini-3-pro # default: agy's configured model AGY_EFFORT: high # low|medium|high - # INLINE diff budget — not a cap on what gets reviewed. The prompt reaches agy - # as a SINGLE argv string, and Linux caps one argument at - # MAX_ARG_STRLEN = 32 * PAGE_SIZE = 128 KiB (a separate, much lower ceiling than - # ARG_MAX). 90 KB leaves room for the instruction boilerplate + style guide. - # A diff over this is NOT truncated: it is written into agy's workspace in parts - # and read with agy's file tools, so PR size no longer bounds review coverage. - MAX_DIFF_BYTES: "90000" + # AGY_DIFF_MODE: auto # auto|inline|file — how the diff reaches agy. `auto` + # # (default) inlines a small diff and hands a large one to + # # agy as a file, so PR size never truncates the review. + # MAX_DIFF_BYTES: "5000000" # sanity cap on a pathological diff (5 MB). Do NOT lower + # # this to bound the review: it truncates the diff BEFORE the + # # file handoff, so a low value defeats the large-PR path. + # 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: | chmod +x scripts/agy-review.sh scripts/_agy_print.sh diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 32cab7da..4a5b86cd 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -13,39 +13,50 @@ # See ../README.md for setup, the issue #76 PTY workaround, and the ToS caveat. set -euo pipefail +# Helpers are defined FIRST: the configuration block below calls log() from the MAX_PROMPT_BYTES +# 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; } +have_text() { [ -s "$1" ] && grep -q '[^[:space:]]' "$1"; } + # --- 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 -# Recorded BEFORE defaulting so the large-diff path can raise the timeout without ever -# overriding a value the caller asked for explicitly. -agy_print_timeout_explicit="${AGY_PRINT_TIMEOUT:+set}" AGY_PRINT_TIMEOUT="${AGY_PRINT_TIMEOUT:-5m}" -# Separate budget for the on-disk handoff below. An inlined diff arrives with the prompt -# and 5m is ample; a handed-off diff has to be READ first, one tool call per part, before -# reasoning even starts. Timing that out would reproduce the failure this replaces -- an -# empty or partial review of a large PR -- so large PRs get proportionally longer. -AGY_PRINT_TIMEOUT_LARGE="${AGY_PRINT_TIMEOUT_LARGE:-25m}" -MAX_DIFF_BYTES="${MAX_DIFF_BYTES:-90000}" # inline-embedding budget for the diff (~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}" -# Above MAX_DIFF_BYTES the diff is HANDED OFF ON DISK instead of being truncated (see -# "hand the diff off on disk" below). agy is an agent with file-reading tools, so the -# argv ceiling bounds only what can be *inlined* -- it is not a bound on what can be -# reviewed. Parts are sized so each is a comfortable single read for the agent. -AGY_DIFF_PART_BYTES="${AGY_DIFF_PART_BYTES:-150000}" -# Work dir for the on-disk handoff. Kept INSIDE the checkout deliberately: it is then -# part of agy's own workspace, so the read needs no --add-dir and no sandbox exception. -# Untracked, gitignored (`.agy-review-work/`), and removed on exit. -AGY_WORK_DIR="${AGY_WORK_DIR:-.agy-review-work}" +AGY_DIFF_MODE="${AGY_DIFF_MODE:-auto}" # auto|inline|file. A diff is passed to agy either inlined + # in the --print prompt, or written to a FILE agy reads with + # its own tools. `auto` inlines a diff that fits under the + # arg-size budget and files anything larger (so large PRs are + # never truncated); `inline`/`file` force one path. +MAX_DIFF_BYTES="${MAX_DIFF_BYTES:-5000000}" # sanity cap on a pathological diff (5 MB). No longer the + # arg-size limit -- a large diff goes to agy as a file, not + # as an argv value -- just a guard against a runaway diff. +MAX_PROMPT_BYTES="${MAX_PROMPT_BYTES:-125000}" # hard ceiling on the INLINED prompt: agy takes it as a + # --print VALUE (not stdin), and a single execve argument + # cannot exceed MAX_ARG_STRLEN (PAGE_SIZE*32 = 128 KiB on + # Linux). Over it, execve fails with E2BIG before agy even + # starts. In `auto` mode this is the inline/file threshold; + # it also backstops the assembled prompt in every mode. +ARG_SIZE_CEILING=128000 # hard cap: a configured MAX_PROMPT_BYTES above the + # MAX_ARG_STRLEN-derived safe bound would defeat the guard + # and re-expose E2BIG, so clamp any override down to it. +# Require a POSITIVE integer at or below the ceiling. The `-gt 0` half is load-bearing, not +# cosmetic: a negative override (e.g. MAX_PROMPT_BYTES=-1) satisfies `-le "$ARG_SIZE_CEILING"`, +# so without it the clamp is skipped and the `head -c "$MAX_PROMPT_BYTES"` prompt cap below runs +# as GNU `head -c -1` (print all-but-last-byte) — silently defeating the E2BIG backstop. A +# non-numeric value trips the `2>/dev/null` and is clamped too. +if ! { [ "$MAX_PROMPT_BYTES" -gt 0 ] && [ "$MAX_PROMPT_BYTES" -le "$ARG_SIZE_CEILING" ]; } 2>/dev/null; then + log "MAX_PROMPT_BYTES='${MAX_PROMPT_BYTES}' invalid, non-positive, or above the ${ARG_SIZE_CEILING}-byte ceiling; clamping" + MAX_PROMPT_BYTES="$ARG_SIZE_CEILING" +fi STYLE_GUIDE="${STYLE_GUIDE:-.github/agy-review.md}" # repo-relative; loaded if present # (dedicated name -- avoids colliding with GEMINI.md/AGENTS.md) -# 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). +# 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) -- and a +# predictable /tmp path is a symlink/file-tampering target. The run-id / PID +# suffix keeps it unique per run. 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 @@ -53,9 +64,6 @@ 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="" -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 -------------------------- @@ -73,8 +81,9 @@ case "${GITHUB_EVENT_NAME:-}" in *) 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. + # script is also runnable by hand and by any future caller, so re-check here that the + # commenter has write access. A stranger's `/agy-review` must never schedule an agy + # run on the self-hosted host — losing this to a workflow-only gate would be silent. case "$assoc" in OWNER|MEMBER|COLLABORATOR) : ;; *) log "comment author association '$assoc' lacks write access; skipping"; exit 0 ;; @@ -90,16 +99,15 @@ log "reviewing ${REPO}#${PR}" # 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= diff_err= meta_file= prompt_file= out_file= raw= body_file= -# Set when the large-diff fallback creates refs/agy/* so the trap can remove them. +diff_file= diff_err= meta_file= prompt_file= out_file= raw= body_file= agy_diff_file= agy_work_dir= +# Set when the large-diff fallback below creates refs/agy/* so the trap can remove them. agy_refs_created= -# Set when the on-disk diff handoff creates $AGY_WORK_DIR inside the checkout. -agy_work_created= cleanup() { - rm -f "$diff_file" "$diff_err" "$meta_file" "$prompt_file" "$out_file" "$raw" "$body_file" - # Guarded on the flag, not on directory existence: this must never remove a path - # that was already there when the script started. - [ -n "$agy_work_created" ] && rm -rf "$AGY_WORK_DIR" || true + rm -f "$diff_file" "$diff_err" "$meta_file" "$prompt_file" "$out_file" "$raw" "$body_file" "$agy_diff_file" + # Remove the gitignored diff-handoff scratch dir once its file is gone. `rmdir` only unlinks an + # empty dir, so a concurrent run's file (a different $$) is never clobbered; a non-empty dir is + # gitignored and harmless if left behind. + [ -n "$agy_work_dir" ] && rmdir "$agy_work_dir" 2>/dev/null || true if [ -n "$agy_refs_created" ] && [ -n "${PR:-}" ]; then git update-ref -d "refs/agy/pr-${PR}" 2>/dev/null || true git update-ref -d "refs/agy/base-${PR}" 2>/dev/null || true @@ -107,12 +115,11 @@ cleanup() { } trap cleanup EXIT -# --- 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)" +# --- fetch metadata first, because the fork gate + the large-diff fallback depend on it --- +# FAIL-CLOSED. A `{}` fallback was harmless when the only field read was the title, +# 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" \ || { log "gh pr view failed; refusing to review without knowing the PR's head repo"; exit 1; } @@ -137,38 +144,30 @@ case "$is_fork" in *) log "could not determine whether PR #${PR} is cross-repository; refusing"; exit 1 ;; esac -# --- fetch the diff ------------------------------------------------------------ +# --- fetch the diff, with a fallback for a diff over GitHub's API line limit ----- # `gh pr diff` can fail for two very different reasons and they must not be # conflated. A genuine error (auth, network, bad PR) is fatal. But GitHub's API -# also refuses any diff over **20,000 lines** with HTTP 406, and that is not an -# error condition -- it just means this PR is too large to review through the -# API. A reviewer that cannot see the diff must not claim a verdict, but it also -# must not fail the build: a red check that means "the PR was big" is noise that -# trains people to ignore the check. Skip cleanly instead. +# also refuses any diff over 20,000 lines with HTTP 406, and that is not an error +# -- it just means the PR is too large to fetch through the API. A large PR is +# exactly the one worth reviewing, so fall back to computing the diff locally. # -# Note this is upstream of the MAX_DIFF_BYTES truncation below, which can only -# shrink a diff we already have. -diff_file="$(mktemp)" -diff_err="$(mktemp)" -if ! gh pr diff "$PR" --repo "$REPO" > "$diff_file" 2> "$diff_err"; then +# SECURITY: this fetches the PR's objects but NEVER checks them out. The working +# tree is untouched, and the PR content is treated exactly as the API diff was -- +# read-only bytes that become prompt text and are never executed. So the fallback +# does not widen the trust boundary the workflow already sets: whatever governs +# whether a given PR's diff is allowed to reach agy at all (the workflow `if:` +# author-association gate; see the trust model at the agy invocation below) is +# unchanged, and this only changes HOW an already-permitted diff is obtained when +# it is too large for the API. `refs/agy/*` are private namespaces (cannot clobber +# a real branch) and are removed on exit. Auth goes through `http.extraheader` for +# the one fetch rather than a persisted credential, so a `persist-credentials: +# false` checkout stays intact; a hand-run without GH_TOKEN falls through to git's +# ambient credential helper. +# ($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's API refuses any diff over 20,000 lines with HTTP 406. That is a - # limit of the *transport*, not a reason to skip the review -- a large PR is - # precisely the one worth reviewing. Fall back to computing the diff locally, - # which has no such ceiling; MAX_DIFF_BYTES below then trims it to the prompt - # budget exactly as it does for any other large diff. - # - # SECURITY: this fetches the PR's objects but NEVER checks them out. The - # working tree stays on the default branch, so the reviewer scripts and style - # guide still come from `main` and a PR cannot rewrite its own reviewer. The - # PR's content is treated exactly as the API diff was: read-only bytes that - # become prompt text and are never executed. `refs/agy/*` are private - # namespaces so this cannot clobber a real branch, and are deleted on exit. - # - # Auth goes through `http.extraheader` for this one command rather than a - # persisted credential, keeping the repo-wide `persist-credentials: false` - # rule intact. - base_ref="$(jq -r '.baseRefName' "$meta_file")" + 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 @@ -177,14 +176,10 @@ if ! gh pr diff "$PR" --repo "$REPO" > "$diff_file" 2> "$diff_err"; then pr_ref="refs/agy/pr-${PR}" base_local="refs/agy/base-${PR}" agy_refs_created=1 - # - # Two auth shapes, tried in order, because the same script runs in two places. - # `AUTHORIZATION: bearer` is the form Actions' GITHUB_TOKEN accepts and is tried - # first, since that is the path that matters in CI. It does NOT work for a personal - # token from `gh auth token` ("remote: invalid credentials"), so a hand-run on a - # developer box falls through to a plain fetch using whatever credential helper git - # is already configured with. Neither path persists anything into .git/config. fetch_refspecs=( "+refs/pull/${PR}/head:${pr_ref}" "+refs/heads/${base_ref}:${base_local}" ) + # `bearer` is what Actions' GITHUB_TOKEN accepts; a personal token from + # `gh auth token` is rejected ("remote: invalid credentials"), so a hand-run + # falls through to git's ambient credentials. Neither path persists anything. if [ -n "${GH_TOKEN:-}" ] \ && git -c "http.extraheader=AUTHORIZATION: bearer ${GH_TOKEN}" fetch --no-tags --quiet \ origin "${fetch_refspecs[@]}" 2>/dev/null; then @@ -208,48 +203,31 @@ fi if ! have_text "$diff_file"; then log "empty diff; nothing to review"; exit 0; fi -# --- decide how the diff reaches agy: inlined, or handed off on disk ------------ -# A diff larger than the argv budget used to be TRUNCATED, which silently produced a -# review of the first ~90 KB while reading as a review of the whole PR. That is worse -# than no review: it is a confident verdict over an arbitrary prefix. The argv ceiling -# is a limit on what can be *inlined*, not on what agy can *read* -- it has file tools -# -- so large diffs are written into agy's own workspace and the prompt points at them. -# Truncation now happens only if the on-disk handoff itself cannot be set up. truncated="" -diff_bytes="$(wc -c < "$diff_file")" -diff_parts=() -if [ "$diff_bytes" -le "$MAX_DIFF_BYTES" ]; then - log "diff is ${diff_bytes} bytes; inlining it in the prompt" -else - log "diff is ${diff_bytes} bytes; handing it off on disk (over the ${MAX_DIFF_BYTES}-byte inline budget)" - rm -rf "$AGY_WORK_DIR" - mkdir -p "$AGY_WORK_DIR" - agy_work_created=1 - # Split rather than pointing at one 2+ MB file: a single read of that size is at the - # mercy of whatever per-read cap the agent applies, and a silent cap is exactly the - # failure this replaces. Fixed-size parts make coverage explicit -- the prompt lists - # every part with its line count, so an incomplete read is visible in the output. - split -b "$AGY_DIFF_PART_BYTES" -d -a 3 \ - --additional-suffix=.diff "$diff_file" "$AGY_WORK_DIR/pr-${PR}.part-" - while IFS= read -r part; do diff_parts+=("$part"); done \ - < <(find "$AGY_WORK_DIR" -maxdepth 1 -name "pr-${PR}.part-*.diff" | sort) - if [ "${#diff_parts[@]}" -eq 0 ]; then - log "on-disk handoff produced no parts; falling back to truncating the diff" - 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.' - else - log "wrote ${#diff_parts[@]} diff part(s) to ${AGY_WORK_DIR}/" - if [ -z "$agy_print_timeout_explicit" ]; then - AGY_PRINT_TIMEOUT="$AGY_PRINT_TIMEOUT_LARGE" - log "print timeout raised to ${AGY_PRINT_TIMEOUT} for the on-disk handoff" - fi - fi +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 exceeded '"${MAX_DIFF_BYTES}"$' bytes and was truncated for this review.' + log "diff truncated to the ${MAX_DIFF_BYTES}-byte sanity cap" fi # --- build the prompt ---------------------------------------------------------- title="$(jq -r '.title // ""' "$meta_file")" -style=""; [ -f "$STYLE_GUIDE" ] && style="$(cat "$STYLE_GUIDE")" +# Bound the style guide so it can never fill the whole arg budget and crowd out the diff -- or, in +# file mode, the file pointer that the MAX_PROMPT_BYTES guard would otherwise truncate away, leaving +# agy with no diff at all. Reserve headroom for the instructions, the diff header, and the pointer. +# head -c is byte-accurate (a shell substring is by character, which is wrong for multi-byte UTF-8). +style="" +if [ -f "$STYLE_GUIDE" ]; then + style_cap=$(( MAX_PROMPT_BYTES - 8192 )); [ "$style_cap" -lt 0 ] && style_cap=0 + style="$(head -c "$style_cap" "$STYLE_GUIDE")" + [ "$(wc -c < "$STYLE_GUIDE")" -gt "$style_cap" ] && log "STYLE_GUIDE capped to ${style_cap} bytes so the diff / file pointer always fits under the arg budget" +fi +# The instruction HEAD (everything except the diff body). agy takes the whole prompt as one --print +# argv value, capped at MAX_ARG_STRLEN (128 KiB), so a diff that would push the prompt over the budget +# is handed to agy as a FILE it reads with its own tools instead of being inlined or truncated. Small +# diffs still inline (the proven path); only a large PR takes the file path -- and a large PR used to +# fail outright with E2BIG, so the file path can only improve on that. prompt_file="$(mktemp)" { cat < "$prompt_file" -Do not review from filenames, paths, or part sizes. Do not extrapolate from a sample. If -you could not read some part, say so explicitly at the top of your review and scope your -verdict to what you did read -- an honest partial review is useful, a confident review of -an unread diff is not. +# Decide inline vs file. Budget: keep the whole argv prompt under MAX_PROMPT_BYTES (itself clamped +# below the 128 KiB ceiling), reserving a small margin for the diff header. `file` mode writes the +# diff into agy's working directory (the repo checkout) and points the prompt at it by name. +head_bytes="$(wc -c < "$prompt_file")" +diff_bytes="$(wc -c < "$diff_file")" +inline_budget=$(( MAX_PROMPT_BYTES - head_bytes - 512 )) # margin covers the diff header + file notice +[ "$inline_budget" -lt 0 ] && inline_budget=0 # a huge STYLE_GUIDE can exceed it: file the diff +use_file=0 +case "$AGY_DIFF_MODE" in + file) use_file=1 ;; + inline) use_file=0 ;; + *) [ "$diff_bytes" -gt "$inline_budget" ] && use_file=1 ;; +esac -Begin your output with a line of the form: - -EOF - else - printf '\n--- UNIFIED DIFF ---\n' - cat "$diff_file" - fi -} > "$prompt_file" +if [ "$use_file" = "1" ]; then + # agy's CWD is the repo checkout, so a file written under it is readable by its file tool. Write it + # into a dedicated, gitignored scratch dir (`.agy-review-work/`, listed in .gitignore) rather than + # `.git/` (a sandbox may deny tool access to the hidden `.git` dir) or the repo root (an untracked + # `.patch` there pollutes `git status` if agy inspects repo state). Being gitignored, the dir never + # shows as working-tree pollution; the EXIT trap removes the file and the (now-empty) dir. + # + # The prompt hands agy an ABSOLUTE path, and the run below adds "$PWD" to agy's workspace via + # --add-dir. Both are load-bearing and were proven on the live runner: agy's sandboxed file tool + # does NOT resolve a relative path against the shell CWD (it uses its own workspace root), so a + # relative `.agy-review-work/...` comes back "does not exist on the filesystem" — the exact + # large-diff review failure this fixes. An absolute path resolves on its own, and --add-dir makes + # the checkout part of the sandbox workspace; using both is belt-and-suspenders. + agy_work_dir="$PWD/.agy-review-work" + mkdir -p "$agy_work_dir" + diff_name=".agy-review-work/agy-review-diff.$$.patch" + agy_diff_file="$PWD/$diff_name" + cp "$diff_file" "$agy_diff_file" + { + printf '\n--- UNIFIED DIFF (in a file) ---\n' + printf 'The full unified diff for this PR is in the file at the absolute path `%s`\n' "$agy_diff_file" + printf '(it is too large to inline). Read that file IN FULL with your file-reading tool first, then\n' + printf 'produce the review above from its actual contents. Do not review from the PR title alone.\n' + } >> "$prompt_file" + log "diff is ${diff_bytes} bytes (> ${inline_budget}-byte inline budget); handing it to agy as ${agy_diff_file}" +else + # Forced inline (AGY_DIFF_MODE=inline) on an over-budget diff: the MAX_PROMPT_BYTES guard below + # still prevents E2BIG by truncating, but warn since `auto` would have filed it in full instead. + [ "$diff_bytes" -gt "$inline_budget" ] && log "warning: forced inline with a ${diff_bytes}-byte diff over the ${inline_budget}-byte budget; the prompt will be truncated -- use AGY_DIFF_MODE=auto to file it in full" + { printf '\n--- UNIFIED DIFF ---\n'; cat "$diff_file"; } >> "$prompt_file" + log "diff is ${diff_bytes} bytes; inlined into the prompt" +fi -# 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. +# --- guard the argv size (E2BIG) ----------------------------------------------- +# agy takes the prompt as a --print VALUE, so the whole prompt is one execve argument +# and must stay under MAX_ARG_STRLEN (128 KiB). MAX_DIFF_BYTES bounds the diff, but the +# boilerplate + style guide ride on top, so cap the assembled prompt as a hard backstop. 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" + truncated+=$'\n\n> Note: the review prompt was capped to '"${MAX_PROMPT_BYTES}"$' bytes (execve arg-size limit).' + log "prompt capped to ${MAX_PROMPT_BYTES} bytes (execve arg-size ceiling)" +fi +# Byte truncation (here or in the MAX_DIFF_BYTES cap above) can slice a multi-byte UTF-8 sequence. +# agy is a Rust binary and std::env::args() PANICS on a non-UTF-8 argument, which would reintroduce +# an instant startup failure -- exactly the class of bug this guard exists to prevent. Drop any +# invalid/partial sequences with iconv (glibc + macOS ship it), and fall back to python3 (present on +# every CI runner) if iconv is absent, so a no-iconv host can't leave a split sequence in the prompt. +# Explicit branches, not `&& mv || rm`: that idiom masks an mv failure and would then feed agy the +# original (possibly split) bytes. A successful sanitize must replace the file; a failed mv is fatal +# (set -e); a failed/absent sanitizer leaves the original and we proceed (it may already be clean, and +# any residual invalid byte now surfaces via the captured stderr rather than a silent instant crash). +# The `[ -s ... ]` guard is deliberate: the prompt is always non-empty here (boilerplate at +# minimum), so a sanitizer that exits 0 but emits ZERO bytes is a malfunction, and mv-ing that +# empty file over $prompt_file would wipe the prompt and hand agy nothing. Replace only on a +# non-empty result; otherwise discard it and proceed with the original (already likely clean). +if command -v iconv >/dev/null 2>&1; then + if iconv -c -f UTF-8 -t UTF-8 "$prompt_file" > "$prompt_file.utf8" 2>/dev/null && [ -s "$prompt_file.utf8" ]; then + mv "$prompt_file.utf8" "$prompt_file" + else + rm -f "$prompt_file.utf8" + fi +elif command -v python3 >/dev/null 2>&1; then + if python3 -c 'import sys; sys.stdout.buffer.write(open(sys.argv[1],"rb").read().decode("utf-8","ignore").encode("utf-8"))' \ + "$prompt_file" > "$prompt_file.utf8" 2>/dev/null && [ -s "$prompt_file.utf8" ]; then + mv "$prompt_file.utf8" "$prompt_file" + else + rm -f "$prompt_file.utf8" + fi fi -# Escape hatch for verifying the diff-acquisition and prompt-assembly path (including the -# large-PR fallbacks) without spending an agy run or posting to the PR. Prints the -# assembled prompt to stdout and stops before the agy invocation. +# Escape hatch for verifying the diff-acquisition and prompt-assembly path (including +# the large-PR API-limit fallback and the inline/file decision) without spending an agy +# run or posting to the PR. Prints the assembled prompt to stdout and stops before agy. if [ -n "${AGY_DRY_RUN:-}" ]; then log "AGY_DRY_RUN set: printing the assembled prompt and exiting before agy runs" - log "prompt: $(wc -c < "$prompt_file") bytes; diff parts: ${#diff_parts[@]}" + if [ "${use_file:-0}" = "1" ]; then + log "prompt: $(wc -c < "$prompt_file") bytes (diff handed off on disk as ${agy_diff_file:-?}, $(wc -c < "$diff_file") bytes)" + else + log "prompt: $(wc -c < "$prompt_file") bytes (diff inlined, $(wc -c < "$diff_file") bytes)" + fi cat "$prompt_file" exit 0 fi @@ -321,46 +350,21 @@ 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 ) +# In file-handoff mode agy must read the on-disk diff, and --sandbox otherwise confines its file +# tool to its own workspace root (NOT the shell CWD) — so add the checkout to the workspace. Only in +# file mode: an inline review reads nothing from disk, so it keeps zero filesystem access (narrower +# prompt-injection surface for the common path). Proven necessary on the live runner: without this +# (and the absolute path above) the large-diff handoff file reads back as "does not exist". +[ "${use_file:-0}" = "1" ] && flags+=( --add-dir "$PWD" ) + +# Strip the repo tokens from agy's own environment. agy runs under +# --dangerously-skip-permissions and ingests an untrusted diff (prompt-injection +# surface); `gh` and the large-diff fetch run in THIS script, before and after, and +# agy has no use for GH_TOKEN / GITHUB_TOKEN — so it should not inherit them. +agy_env=( env -u GH_TOKEN -u GITHUB_TOKEN ) [ -n "$AGY_MODEL" ] && flags+=( --model "$AGY_MODEL" ) [ -n "$AGY_EFFORT" ] && flags+=( --effort "$AGY_EFFORT" ) -# 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 "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 -# 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)" here="$(cd "$(dirname "$0")" && pwd)" : > "$LOG" @@ -370,7 +374,11 @@ here="$(cd "$(dirname "$0")" && pwd)" # 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 \ + # Create the lock dir first: a failed `exec 9>` redirection is a FATAL shell error (it aborts + # before the `|| log` fallback can run), so ensure the parent exists on a fresh runner. `>>` opens + # for append rather than truncating the lockfile — flock uses the fd, not the contents. + mkdir -p "$(dirname "$AGY_LOCK")" 2>/dev/null || true + exec 9>>"$AGY_LOCK" 2>/dev/null \ && flock -w "$AGY_LOCK_WAIT" 9 \ || log "agy lock unavailable or timed out (${AGY_LOCK_WAIT}s); proceeding unserialized" fi @@ -381,49 +389,31 @@ fi for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do : > "$out_file" # clear any partial output from a prior attempt - rc=0 if command -v unbuffer >/dev/null 2>&1; then log "running agy via unbuffer (allocates a PTY) [attempt ${attempt}/${AGY_RETRIES}]" - "${agy_env[@]}" unbuffer "$AGY_BIN" "${flags[@]}" --print "$(cat "$prompt_file")" \ - > "$out_file" 2>>"$LOG" || rc=$? + "${agy_env[@]}" 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` 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. + # `script -c` runs its command through `sh -c`. Build that command with `printf '%q '` + # so every argument -- including the flag values, which come from env vars + # (AGY_MODEL / AGY_EFFORT / AGY_PRINT_TIMEOUT) -- is shell-escaped for the inner shell. + # A raw `${flags[*]}` here would let a shell metacharacter in any of those be evaluated + # by `sh -c` (command injection); `%q` quotes each token exactly. 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=$? + "${agy_env[@]}" AGY_BIN="$AGY_BIN" script -qfec "$cmd" "$raw" >/dev/null 2>>"$LOG" || true col -b < "$raw" > "$out_file" + rm -f "$raw" # each retry makes a fresh $raw; the EXIT trap only holds the last one 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" - # --- 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. - # - # 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. + # NOTE: there is deliberately no SQLite-conversation-store fallback here. agy's + # store is keyed by mtime, not session, and on a shared/multi-user runner the + # most-recent `.db` can belong to an UNRELATED concurrent local `agy` session -- + # 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. have_text "$out_file" && break if [ "$attempt" -lt "$AGY_RETRIES" ]; then @@ -436,6 +426,18 @@ exec 9>&- 2>/dev/null || true # release the agy lock so the next queued job p 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 + # `exit 1` otherwise leaves the real cause invisible in CI (E2BIG, auth, backend, ...). + if [ -s "$LOG" ]; then + # Bound the dump to the last lines: GitHub Actions auto-masks registered secrets (incl. + # GITHUB_TOKEN) in logs, and this is agy's own diagnostic stream, but a bounded tail avoids + # publishing an unbounded volume of stderr (which could echo prompt/diff content) into CI. + log "----- captured agy stderr ($LOG), last ${AGY_LOG_TAIL_LINES:-60} lines (secrets auto-masked) -----" + tail -n "${AGY_LOG_TAIL_LINES:-60}" "$LOG" | sed 's/^/[agy] /' >&2 || true + log "----- end agy stderr -----" + else + log "(agy stderr log is empty -- agy likely failed before writing, e.g. execve E2BIG on an oversized prompt)" + fi exit 1 fi @@ -452,11 +454,9 @@ 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. +# The author filter is load-bearing, not cosmetic: without it, ANY user could put the +# marker (an HTML comment) in a PR comment and have this bot delete arbitrary comments on +# the next run. Only ever delete OUR OWN bot's prior review comments. gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ --jq ".[] | select(.user.type == \"Bot\" and .user.login == \"github-actions[bot]\") | select(.body | contains(\"${MARKER}\")) | .id" 2>/dev/null \ | while read -r cid; do