ci: add Antigravity PR reviewer (self-hosted, Ultra)#324
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an Antigravity-based automated pull request reviewer with GitHub Actions triggers, authorization gates, bounded prompts, PTY-compatible execution, retries, and replacement PR comments. Review rules and security behavior are documented. ChangesAntigravity Review Automation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant GitHubActions
participant agyReview
participant gh
participant agy
participant PullRequest
GitHub->>GitHubActions: emit pull request event or authorized command comment
GitHubActions->>agyReview: execute trusted review script
agyReview->>gh: fetch pull request title and unified diff
gh-->>agyReview: return review inputs
agyReview->>agy: submit bounded review prompt
agy-->>agyReview: return review text
agyReview->>gh: replace marked bot comment
gh-->>PullRequest: publish Antigravity review
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Antigravity review (Gemini via Ultra)This pull request adds a GitHub Actions workflow and shell scripts to automate pull request code reviews using the Antigravity CLI on a self-hosted runner. Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/antigravity-review.yml:
- Around line 28-31: Restrict manual /agy-review execution to trusted
contributors by adding an author_association check for OWNER, MEMBER, or
COLLABORATOR in the workflow condition in
.github/workflows/antigravity-review.yml lines 28-31. Repeat the same validation
in scripts/agy-review.sh lines 39-46 before invoking agy, while preserving
automatic pull-request review behavior.
- Around line 34-49: Update the checkout step used by the Antigravity review to
fetch trusted automation assets from the default branch rather than the pull
request ref, and set persist-credentials to false. Keep the existing checkout
action and shallow fetch behavior, ensuring scripts/agy-review.sh and
scripts/_agy_print.sh come only from the trusted ref before execution.
In `@scripts/agy-review.sh`:
- Around line 124-136: Remove the global newest-database fallback in the output
recovery block of scripts/agy-review.sh. Tie SQLite recovery to the current agy
invocation’s conversation ID/database, and if that invocation-specific
identifier cannot be recovered, fail without reading or publishing any
pre-existing conversation data.
- Line 22: Cap the complete assembled prompt, not just the diff: reduce
MAX_DIFF_BYTES by the boilerplate and style-guide overhead, or otherwise enforce
a safe total size before invoking the review command. Apply the same safe
transport handling to the prompt arguments at scripts/agy-review.sh lines 22-22
and 109-109 and scripts/_agy_print.sh lines 9-9, covering the unbuffer and
script(1) paths while preserving the existing prompt content within Linux’s
per-argument limit.
- Line 99: Remove the --dangerously-skip-permissions flag from the flags array
in scripts/agy-review.sh, preserving the existing --print-timeout and --sandbox
options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 683ceac5-8ccb-4d2c-917e-b93129e83649
📒 Files selected for processing (4)
.github/agy-review.md.github/workflows/antigravity-review.ymlscripts/_agy_print.shscripts/agy-review.sh
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/agy-review.sh (1)
141-145: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorization Bypass (CWE-863): Incorrect Authorization
Reachability: External
Delete only this workflow’s prior comment.
The marker is public and the filter ignores comment author, so it selects any matching human comment for deletion. The delete endpoint accepts the write permissions granted by this workflow. Restrict deletion to the authenticated bot’s comment (and the marker), or persist its exact comment ID. (docs.github.com)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/agy-review.sh` around lines 141 - 145, Update the comment cleanup pipeline around the gh api query to require both the existing MARKER and the authenticated workflow bot as the comment author before deleting IDs. Preserve pagination and avoid deleting matching comments authored by humans.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/_agy_pty.py`:
- Around line 21-22: Enforce a Python 3.9+ requirement in the wrapper before
invoking pty.spawn or os.waitstatus_to_exitcode, using the script’s existing
entry-point structure. Add a clear version check that exits with an actionable
message on older interpreters, while preserving the current status propagation
for supported Python versions.
---
Outside diff comments:
In `@scripts/agy-review.sh`:
- Around line 141-145: Update the comment cleanup pipeline around the gh api
query to require both the existing MARKER and the authenticated workflow bot as
the comment author before deleting IDs. Preserve pagination and avoid deleting
matching comments authored by humans.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77140ba4-12ed-49b2-82b0-8ffbf70a49e2
📒 Files selected for processing (3)
.github/workflows/antigravity-review.ymlscripts/_agy_pty.pyscripts/agy-review.sh
…sync The template re-sync in 9875dab pulled the upstream reviewer over the hardened version from 0237135 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 <noreply@anthropic.com>
Review adjudication — every finding answered, fixes in
|
Reverted by 9875dabb |
Consequence |
|---|---|
pull_request fork gate |
any fork PR could schedule work on the self-hosted runner |
script-side author_association re-check |
gate lost if the workflow alone is edited |
argv-safe PTY wrapper → script -qfec "…${flags[*]}" |
command injection via env-settable flags |
| assembled-prompt size cap | live E2BIG (see below) |
| invocation-scoped SQLite fallback | could publish an unrelated agy session into a public comment |
Three of those carried a CodeRabbit "✅ Addressed in commit 0237135" note that had stopped being true. I've replied on each of those resolved threads to correct the record rather than leave them reading as settled.
CodeRabbit — outside-diff: prior-comment cleanup deletes by marker alone
Adopted. The marker is plain text in a public comment and the filter ignored the author, so anyone who pasted <!-- antigravity-pr-review --> into a comment had it deleted on the next run — and the job holds issues: write, so the delete succeeds. Now restricted to the workflow's own bot:
.[] | select(.user.type == "Bot" and .user.login == "github-actions[bot]")
| select(.body | contains("<!-- antigravity-pr-review -->")) | .idPagination is unchanged (--paginate), and a failed delete still logs a warning rather than being swallowed.
Antigravity's self-review
Blocking 1 — command injection via ${flags[*]}. Adopted. script -c takes a command string and runs it through sh -c, so raw interpolation turns any metacharacter in AGY_MODEL / AGY_EFFORT / AGY_PRINT_TIMEOUT into shell syntax. Now assembled with printf '%q '. Verified: a flag value of 5m; touch /tmp/AGY_PWNED arrives as one literal argv element and the injected command does not run.
Blocking 2 — SQLite fallback data leakage. Adopted; scoped to the current invocation with find -newermt against a per-attempt timestamp. With no match, the fallback yields nothing and the retry/failure path takes over.
Blocking 3 — silent failure suppression. Adopted in substance, not in form. The || true is replaced by || rc=$? and the non-zero status is logged with a pointer to the stderr log. It stays non-fatal on purpose: agy can exit non-zero having already printed a usable review, and the retry loop plus the final have_text check are the real gate. What was wrong was erasing the difference between "agy failed" and "agy produced nothing", and that is now recorded.
Suggestion 1 — temp-file cleanup trap. Already present since 0237135 (trap 'rm -f …' EXIT, with the variables pre-declared so it is safe under set -u before they are assigned). No change.
Suggestion 2 — shared log file. Adopted: agy-review-${GITHUB_RUN_ID:-$$}.log. RUNNER_TEMP is already per-job on a GitHub runner, so the collision only existed on the /tmp fallback path — but that path is what local and non-Actions runs take.
Suggestion 3 — non-portable script(1) syntax. Rejected, with reason. The runner is a Linux self-hosted host with util-linux script; there is no BSD/macOS path to support. unbuffer is the preferred branch and script(1) is the fallback for hosts without expect. (A python3 PTY wrapper did exist and was removed by the re-sync; the %q fix closes the injection concern that motivated the suggestion, so reinstating a third transport is not worth the surface.)
Nitpick 1 — replace $(cat "$prompt_file") with < "$prompt_file". Rejected: agy --print takes the prompt as an argv value, not on stdin, so redirection is not an equivalent. This is precisely why the 128 KiB single-argument ceiling applies at all — see the size cap below.
Nitpick 2 — swallowed gh api errors on delete. Already fixed in 0237135: a failed delete logs warning: could not delete prior review comment … instead of being discarded, because silently ignoring it leaves the old comment and posts a new one.
One finding neither bot could have measured: a live E2BIG
The workflow was shipping MAX_DIFF_BYTES: "200000", and the prompt reaches agy as a single argv string. ARG_MAX is 4 MiB here, which is why this hides — the binding limit is MAX_ARG_STRLEN = 32 * PAGE_SIZE = 128 KiB per argument. Measured on this kernel: 131000 bytes execs, 131073 fails, 200000 fails. Any PR with a large diff would have failed the exec before agy ever started. Diff budget is now 90 KB, with a new MAX_PROMPT_BYTES (120 KB) capping the assembled prompt — capping the diff alone never bounded the total, since the boilerplate and style guide ride in the same string.
Also in this commit
Checkout moves to the default branch with persist-credentials: false and actions/checkout@v7 (this was the repo's only remaining @v4, against 19 sites on @v7); agy launches through env -u GH_TOKEN -u GITHUB_TOKEN; and CHANGELOG.md gains the [Unreleased] Added + Security entry that the pre-merge check asked for.
CI-only. Nothing under crates/rustynes-{cpu,ppu,apu,mappers,core} is touched, so the deterministic #![no_std] chip stack and every golden vector are unchanged by construction.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/agy-review.sh`:
- Around line 193-220: Update the SQLite fallback in the empty-output path to
read only from a conversation store uniquely owned by the current invocation,
using a private per-run directory or the conversation ID produced by the current
agy process. Do not rely on the mtime filter in the existing db discovery; if
ownership cannot be established, leave the output empty and continue through the
existing retry/failure path without querying or posting any database content.
- Around line 132-143: Update the reviewer execution around the agy_env and agy
invocation to use an ephemeral account or VM with no persistent runner state,
and enforce policy-level denial of reads outside the workspace, network access,
and MCP tools. Do not rely on --sandbox or unsetting GH_TOKEN/GITHUB_TOKEN as
the isolation boundary; preserve the existing headless invocation while ensuring
attacker-controlled diffs cannot access host files or process state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c0487e8a-66f3-43df-ab29-a212c1e9a880
📒 Files selected for processing (4)
.github/workflows/antigravity-review.ymlCHANGELOG.mdscripts/_agy_print.shscripts/agy-review.sh
…ust 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 36-41: The changelog overstates the workflow trigger as a complete
trust boundary because issue-comment triggers do not verify the PR originates
from the same repository. Update the changelog entry to remove or qualify that
claim, and align it with the corrected trust checks in scripts/agy-review.sh and
the workflow invocation behavior.
In `@scripts/agy-review.sh`:
- Around line 135-162: Update the trust-model documentation around the fork-PR
boundary to accurately account for the issue_comment-triggered /agy-review path:
either add and enforce a same-repository check before scheduling the job, or
revise the text to explicitly exclude fork PRs from this path and avoid claiming
external diffs cannot reach agy. Anchor the change to the workflow trigger
condition referenced by the comments and preserve the existing token-removal and
conversation-store safeguards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72a88c3c-cb08-4350-8ccc-770002120864
📒 Files selected for processing (2)
CHANGELOG.mdscripts/agy-review.sh
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 <noreply@anthropic.com>
|
@coderabbitai review Requesting an explicit pass on |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== target commit ==='
git show --no-ext-diff --format=fuller --stat 98d0162
printf '%s\n' '=== target diff ==='
git diff --no-ext-diff --unified=80 98d0162^ 98d0162 -- scripts/agy-review.sh .github/workflows/antigravity-review.yml
printf '%s\n' '=== script outline ==='
ast-grep outline scripts/agy-review.sh --items all || true
printf '%s\n' '=== workflow ==='
cat -n .github/workflows/antigravity-review.yml
printf '%s\n' '=== event, PR-resolution, and cross-repository checks ==='
rg -n -C 12 -e 'issue_comment|pull_request|isCrossRepository|author_association|gh api|PR_NUMBER|GITHUB_EVENT' scripts/agy-review.shLength of output: 33247
The
I found no bypass in the reviewed trigger paths. |
Adds the self-hosted Antigravity PR reviewer (runs on the
agyrunner, free under Google AI Ultra). Auto-reviews on PR open and via/agy-reviewcomments once merged to main. Template: Local_Only-Projects/antigravity-pr-review.Summary by CodeRabbit
New Features
/agy-review.Bug Fixes
Security
Documentation