From 1a2e791a4eaf7b0b75c073ec7dd13c2831ccabee Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 23 Jul 2026 18:06:13 -0400 Subject: [PATCH 1/8] ci(review): converge the Antigravity reviewer onto the shared template Replaces RustyNES's bespoke `agy-review.sh` + workflow with the canonical `Local_Only-Projects/antigravity-pr-review` template, so the three repos that run this reviewer (RustyNES, RustySNES, RustyN64) stop diverging and a future improvement to the template applies to all of them at once instead of being re-invented per repo. After this change the two files are byte-identical to the template. WHY THIS WAS NEEDED. RustyNES's copy and the template had drifted into two different solutions to the same problem. The template independently grew an `AGY_DIFF_MODE=auto|inline|file` mechanism that hands a large diff to agy as a single file under `.git/` (which git never lists in `status`, so it can't show up as working-tree pollution) rather than truncating it. RustyNES, without knowing the template had moved on, solved the same large-diff case its own way -- splitting the diff into 150 KB parts under a gitignored work dir. The template's approach is cleaner (one file, one read, no coverage bookkeeping), so RustyNES adopts it. THE ONE GENUINE GAP, NOW UPSTREAM. RustyNES's copy had exactly one thing the template lacked and every installed repo needed: a fallback for GitHub's 20,000-line diff API limit. `gh pr diff` returns HTTP 406 for any diff over that, and the template's `gh pr diff ... || exit 1` meant the review failed outright on a large PR -- which is precisely the PR most worth reviewing (the v2.2.3 mapper-rename PR was 67k lines). That fallback was ported into the template and is part of this converged version: on a 406 (distinguished from a genuine auth/network/bad-PR error, which stays fatal) it fetches the PR's objects and the base branch into private `refs/agy/*`, takes `git merge-base`, and computes the diff locally -- fetching objects but NEVER checking them out, so the working tree stays on the default branch and a PR cannot rewrite its own reviewer. The refs are removed on exit. WHAT THE CONVERGED VERSION IS (union of the best of both): * the template's `AGY_DIFF_MODE` file/inline diff handling; * the 20,000-line API-limit local-git-diff fallback (from here); * RustyNES's security hardening, folded UPSTREAM so all three inherit it: the `isCrossRepository` fork gate in the script (a trusted commenter is not a trusted diff, and the diff is what agy ingests under --dangerously-skip-permissions), fail-closed metadata (a `gh pr view` failure must never be indistinguishable from "same-repo"), the `AGY_DRY_RUN` escape hatch for testing the acquisition + prompt-assembly path without spending an agy run, the default-branch checkout (the reviewer never runs PR-supplied code on the self-hosted runner), `persist-credentials: false`, and `actions/checkout@v7`; * the template's `synchronize` trigger (auto-re-review on every push), which is safe under the default-branch checkout since the PR supplies only the diff (data), never the reviewer code. ONE RECONCILIATION TRAP FIXED IN THE STANDARD WORKFLOW. RustyNES's old workflow set `MAX_DIFF_BYTES: "90000"` -- correct for the parts mechanism, where that value was the inline budget. But the template script truncates the diff to `MAX_DIFF_BYTES` BEFORE the inline/file decision, so carrying that env onto the template script would have truncated every diff to 90 KB and defeated file mode entirely. The standard workflow drops the override (5 MB sanity-cap default) and documents why lowering it is wrong. VERIFICATION. `bash -n` clean; `shellcheck -S warning` reports nothing beyond the pre-existing SC1007/SC2218 in the template's config block; `actionlint` clean against `.github/actionlint.yaml` (the `agy` self-hosted label is declared there); the repo pre-commit hook passes on both files. The full path -- fork gate -> 406 detection -> local `git diff` -> file-mode handoff -- was exercised end-to-end with `AGY_DRY_RUN=1` against this repo's own PR #325 (67,645 lines / 2.69 MB fetched via the fallback, handed off as a `.git/agy-review-diff.*.patch` with a 2.6 KB prompt), and the `refs/agy/*` and temp files were confirmed removed on exit. No emulation-core, test, or build change -- reviewer tooling only. --- .github/workflows/antigravity-review.yml | 35 +- scripts/agy-review.sh | 408 ++++++++++------------- 2 files changed, 200 insertions(+), 243 deletions(-) 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..8569730b 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -18,35 +18,32 @@ 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. +if ! [ "$MAX_PROMPT_BYTES" -le "$ARG_SIZE_CEILING" ] 2>/dev/null; then + log "MAX_PROMPT_BYTES='${MAX_PROMPT_BYTES}' invalid 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). -LOG="${RUNNER_TEMP:-/tmp}/agy-review-${GITHUB_RUN_ID:-$$}.log" +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 @@ -66,19 +63,11 @@ 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")" ;; *) @@ -90,16 +79,11 @@ 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= +# 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" 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 +91,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)" 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 +120,29 @@ 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)" +# 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="$(mktemp)" -if ! gh pr diff "$PR" --repo "$REPO" > "$diff_file" 2> "$diff_err"; then +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 +151,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 +178,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 +if [ "$use_file" = "1" ]; then + # agy's CWD is the repo checkout, so a file written under it is readable by its file tool. Prefer + # `.git/` -- git never lists it in `git status`, so the transient diff can't show up as working-tree + # pollution if agy inspects repo state -- and fall back to the repo root when `.git` is not a real + # directory (a worktree/submodule gitfile). Either way the path is CWD-relative for the prompt. + if [ -d "$PWD/.git" ]; then + diff_name=".git/agy-review-diff.$$.patch" else - printf '\n--- UNIFIED DIFF ---\n' - cat "$diff_file" + diff_name=".agy-review-diff.$$.patch" fi -} > "$prompt_file" + 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 `%s` in your current working directory\n' "$diff_name" + 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 ${diff_name}" +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 when iconv is available (glibc + macOS ship it); a no-op when clean. +if command -v iconv >/dev/null 2>&1; then + # Explicit branches, not `&& mv || rm`: that idiom masks an mv failure and would then feed agy the + # original (possibly split) bytes. A successful iconv must replace the file; a failed mv is fatal + # (set -e), a failed iconv 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). + if iconv -c -f UTF-8 -t UTF-8 "$prompt_file" > "$prompt_file.utf8" 2>/dev/null; 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 @@ -324,43 +310,6 @@ flags=( --print-timeout "$AGY_PRINT_TIMEOUT" --sandbox --dangerously-skip-permis [ -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 +319,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 +334,41 @@ 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=$? + 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. - 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=$? + # `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" + 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. + # --- 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 @@ -436,6 +381,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,13 +409,8 @@ 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(.user.type == \"Bot\" and .user.login == \"github-actions[bot]\") | select(.body | contains(\"${MARKER}\")) | .id" 2>/dev/null \ + --jq ".[] | 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 ed9ce58709767b463b04161786d6f8dd47444a2b Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 23 Jul 2026 19:27:27 -0400 Subject: [PATCH 2/8] =?UTF-8?q?release:=20v2.2.4=20"Cartridge"=20=E2=80=94?= =?UTF-8?q?=20libretro=20core=20builds/installs=20for=20RetroArch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A libretro / RetroArch distribution cut. Its purpose is that the RustyNES core builds and installs cleanly through the Libretro buildbot (git.libretro.com/libretro/RustyNES) so RetroArch users can pull it from the in-app Online Updater -> Core Downloader. ZERO EMULATION-CORE CHANGES. The deterministic #![no_std] chip stack, the save-state / TAS / netplay formats, and every golden vector are byte-identical to v2.2.3, so AccuracyCoin holds 141/141 (100.00%) and nestest is 0-diff by construction. This is a metadata + version + docs cut, not a code cut. LIBRETRO AUDIT (the substantive work). crates/rustynes-libretro is a thin wrapper over rustynes-core, so it inherits every recent change automatically and needed no code change to carry them: * the fast PPU dot path is the core default now, so the wrapper gets the ~11% rendering-heavy win for free; * the PPU_SNAPSHOT_VERSION 8 / APU v4 save-state schema is transparent to the wrapper, because get_serialize_size / on_serialize size and emit the CURRENT snapshot via Nes::snapshot_core_into / VsDualSystem::snapshot, not a hardcoded layout -- so RetroArch save states, run-ahead, and rollback carry the v8 sprite-eval FSM + OAM data-bus state with no wrapper change (and since libretro save states are session-local, the ADR-0028 epoch never applies); * the Mapper::mix_audio i32 widening, the Zapper model, and the mNNN_ mapper rename are all below the crate's public dependency surface. Both buildbot cross-ABIs the GitHub early-warning gate models build clean, verified with the exact CI command: cargo check --release -p rustynes-libretro --target x86_64-pc-windows-gnu cargo check --release -p rustynes-libretro --target aarch64-linux-android rustynes_libretro.info METADATA CORRECTED (the file RetroArch's core downloader reads to learn the core's capabilities): * disk_control "false" -> "true" -- THE REAL FIX. The FDS multi-side Disk Control interface (enable_disk_control_interface() + the on_set_eject_state / on_get_image_index / on_get_num_images / on_replace_image_index callback trampolines) has been wired since the buildbot recipe landed, but the .info advertised it as absent, so RetroArch's Quick Menu -> Disk Control never surfaced multi-disk FDS swapping. A multi-side FDS game is now swappable from the RetroArch UI as intended. * display_version "v1.0.0" -> "v2.2.4" (stale since the v1.0.0 era; the runtime library_version the core reports has always tracked CARGO_PKG_VERSION -- this is the static metadata string RetroArch shows). * description mapper count 168 -> 172, plus a note that FDS multi-disk swapping runs through the Disk Control interface. Libretro core options (region / overscan / palette / accuracy toggles) remain unexposed: core_options = "false" is accurate, not stale -- the CoreOptions impl is deliberately empty. Documented as a future enhancement rather than a v2.2.4 gap, since it is a new capability deserving its own focused work + determinism/save-state testing rather than a rushed add in a distribution cut. TOOLING. The Antigravity PR reviewer standardization onto the shared template (scripts/agy-review.sh + .github/workflows/antigravity-review.yml, the prior commit on this branch) rides along in this release -- the same canonical version now installed across RustyNES / RustySNES / RustyN64. VERSION + DOCS. Workspace 2.2.3 -> 2.2.4 (all 18 crates inherit); README badge + Current Release; CHANGELOG [2.2.4]; docs/STATUS.md current-release block; AGENTS.md MC-PROJECT block + the never-claim-later guard (-> v2.2.4); to-dos/ROADMAP.md re-synced from its stale v2.1.0 "current release" line to v2.2.4 with a compact release-line bridge (full per-release detail stays in CHANGELOG.md + docs/STATUS.md, the single source of truth). .gitignore needs no change -- the libretro build artifacts (.so/.dll/.dylib/.a) are already covered. VERIFICATION. cargo fmt --all --check; clippy --workspace --all-targets -D warnings (+ the retroachievements feature combo); no_std build (thumbv7em-none-eabihf); markdownlint (pinned v0.39.0) on every touched document + the release notes; the codename parses to "Cartridge" for the release-auto title. AccuracyCoin 141/141 is inherited unchanged from v2.2.3. Mobile versions stay frozen at their v2.0.x host-only release points; the joint store launch remains v2.3.0. --- .github/release-notes/v2.2.4.md | 63 ++++++++++++++++ AGENTS.md | 6 +- CHANGELOG.md | 50 +++++++++++++ Cargo.lock | 36 +++++----- Cargo.toml | 2 +- README.md | 71 +++++++++++-------- .../rustynes-libretro/rustynes_libretro.info | 6 +- docs/STATUS.md | 28 +++++++- to-dos/ROADMAP.md | 3 +- 9 files changed, 208 insertions(+), 57 deletions(-) create mode 100644 .github/release-notes/v2.2.4.md diff --git a/.github/release-notes/v2.2.4.md b/.github/release-notes/v2.2.4.md new file mode 100644 index 00000000..29ad7ead --- /dev/null +++ b/.github/release-notes/v2.2.4.md @@ -0,0 +1,63 @@ +# RustyNES v2.2.4 — "Cartridge" (libretro core builds/installs for RetroArch) + +A **libretro / RetroArch distribution** cut. Its single purpose is that the RustyNES core **builds and installs cleanly through the Libretro buildbot** ([git.libretro.com/libretro/RustyNES](https://git.libretro.com/libretro/RustyNES)), so RetroArch users can pull it straight from the in-app **Online Updater → Core Downloader**. + +A *cartridge* is the thing you slot in to play. That is what this release is about: getting the RustyNES core packaged and advertised correctly so it drops into RetroArch and runs. + +**Zero emulation-core changes.** The deterministic `#![no_std]` chip stack, the save-state / TAS / netplay formats, and every golden vector are **byte-identical to v2.2.3**, so **AccuracyCoin holds 141/141 (100.00%)** and nestest is 0-diff *by construction*. + +--- + +## The libretro core is complete, current, and builds for the buildbot + +`crates/rustynes-libretro` is a thin wrapper over `rustynes-core`, so it inherits every recent change automatically — it needed no code change to carry the v2.2.3 work: + +- **The fast PPU dot path** is the core default now, so the libretro core gets the ~11% rendering-heavy win for free (it constructs `Nes` with the current defaults). +- **The `PPU_SNAPSHOT_VERSION` 8 / APU v4 save-state schema** is transparent to the wrapper: `get_serialize_size` and `on_serialize` size and emit the *current* snapshot via `Nes::snapshot_core_into` / `VsDualSystem::snapshot`, not a hardcoded layout. RetroArch save states, run-ahead, and rollback netplay therefore carry the v8 sprite-evaluation FSM + OAM data-bus state with no wrapper change — and because libretro save states are session-local, the ADR-0028 epoch never enters the picture. +- **The `Mapper::mix_audio` i32 widening**, the Zapper model, and the `mNNN_` mapper rename are all below the crate's public dependency surface. + +Both buildbot cross-ABIs the GitHub early-warning gate models build clean, verified with the exact CI command: + +```text +cargo check --release -p rustynes-libretro --target x86_64-pc-windows-gnu Finished +cargo check --release -p rustynes-libretro --target aarch64-linux-android Finished +``` + +(The full ten-job buildbot recipe went green in v2.2.2; the Apple ABIs are excluded from the Linux-runner gate because bindgen needs a real per-target Apple sysroot.) + +## `rustynes_libretro.info` metadata corrected + +`rustynes_libretro.info` is the file RetroArch's core downloader reads to learn a core's capabilities. Three entries were stale or wrong: + +- **`disk_control` `false` → `true` — the real fix.** The FDS multi-side **Disk Control interface** (`enable_disk_control_interface()` plus the `on_set_eject_state` / `on_get_image_index` / `on_get_num_images` / `on_replace_image_index` callback trampolines) has been wired since the buildbot recipe landed — but the `.info` advertised it as absent, so RetroArch's **Quick Menu → Disk Control** never surfaced multi-disk FDS swapping. Now a Famicom Disk System game with more than one disk side is swappable from the RetroArch UI as intended. +- **`display_version` `v1.0.0` → `v2.2.4`** — stale since the v1.0.0 era (the runtime `library_version` the core reports has always tracked `CARGO_PKG_VERSION`; this is the static metadata string RetroArch shows). +- **Description mapper count `168` → `172`**, with a note that FDS multi-disk swapping runs through the Disk Control interface. + +## Documented follow-up: libretro core options + +Libretro **core options** (region NTSC/PAL/Dendy, overscan crop, palette selection, the opt-in accuracy toggles) remain unexposed. `core_options = "false"` in the `.info` is **accurate, not stale** — the `CoreOptions` impl is deliberately empty. This is a genuine future enhancement, not a v2.2.4 gap: it is a new capability that deserves its own focused work and testing (each option wired to the core config API and verified against the determinism / save-state contract), rather than being rushed into a distribution cut. + +## Tooling: the Antigravity reviewer standardized onto the shared template + +`scripts/agy-review.sh` and `.github/workflows/antigravity-review.yml` are now byte-identical to the canonical `antigravity-pr-review` template installed across RustyNES / RustySNES / RustyN64, so future reviewer improvements apply uniformly. It carries the large-diff-to-file handling, the 20,000-line `gh pr diff` API-limit local-`git diff` fallback (fetches PR objects but never checks them out), the `isCrossRepository` fork gate, fail-closed metadata, a default-branch checkout with `persist-credentials: false`, and the `synchronize` auto-re-review trigger. + +--- + +## Upgrade notes + +- **Nothing breaks.** There are no core, save-state, movie, or netplay changes. `.rns` save states and `.rnm` movies from v2.2.3 load and play identically. +- **RetroArch users:** once the buildbot mirrors this tag, the RustyNES core appears in **Online Updater → Core Downloader**; FDS multi-disk games gain working Disk Control. +- No API changes; `Mapper::mix_audio` stays `i32` (unchanged from v2.2.3). + +## Verification + +```text +cargo check --release -p rustynes-libretro clean +cargo check --release -p rustynes-libretro --target x86_64-pc-windows-gnu clean +cargo check --release -p rustynes-libretro --target aarch64-linux-android clean +cargo test --workspace --features test-roms AccuracyCoin 141/141, nestest 0-diff +cargo fmt --all --check · clippy --workspace --all-targets -D warnings · retroachievements combo clean +no_std build (thumbv7em-none-eabihf) · markdownlint · actionlint clean +``` + +Mobile versions stay frozen at their v2.0.x host-only release points; the joint store launch remains v2.3.0. diff --git a/AGENTS.md b/AGENTS.md index 9d8392a0..cb2066ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,7 +27,7 @@ RustyNES is a cycle-accurate Nintendo Entertainment System emulator written in pure Rust. The accuracy bar is Mesen2 / higan / ares: tight lockstep scheduling at PPU-dot resolution on a master-clock-precise timebase, sub-instruction PPU events visible to subsequent CPU code, and a lookup-table non-linear audio mixer with band-limited synthesis. The frontend is pure Rust (`winit` + `wgpu` + `cpal` + `egui`). -**Current release: v2.2.3 "Datum"** (2026-07-23) — a **performance and accuracy-closure patch**, the product of a measure-first appraisal that profiled the emulator and acted on what the profile showed rather than on intuition. **Performance:** the specialized PPU fast dot path is promoted to the **default** and exposed to users for the first time — `Nes::set_fast_dotloop` had **no caller outside the core**, so a **−11.3%** frame-time win (fresh clean-host Criterion, reproducing v2.1.8's +12.3% by a different method; differential-tested bit-identical every frame since v2.1.8) shipped switched off and unreachable; release builds now ship **PGO-optimized** Linux binaries when the existing >3%-and-byte-identical gate passes; and CI gained a same-runner **relative** frame-time regression gate, closing a hole where a 2.5x slowdown passed the deliberately-loose absolute ceiling. **Two optimizations were measured and REJECTED** and are documented with their numbers per `docs/performance.md`'s convention — P3 (`emit_pixel` bounds-check elision) made the shipped default *slower* (+4.32% / +3.35% on the `_fast` workloads, p ≤ 0.02), and P4 (`cpu_clock`) found both textbook optimizations already implemented with the one remaining lever capped at **≤1.9%**. **Accuracy:** the **last two Holy Mapperel residuals are closed**, so all 17 ROMs report `detail=0000` (was 15/17) — MMC1's two software WRAM write-protect layers (`$E000` bit 4 + SNROM's CHR-register layer, gated on `chr_is_ram`) and FME-7's open bus on the RAM-selected-but-disabled window, both routed through the trait's existing `cpu_read_unmapped` contract. MMC1 is the change Holy Mapperel's README calls a game-compatibility hazard (FCEUX / PowerPak omit it), so it was validated before landing: **60/60** commercial ROMs including seven battery-backed MMC1 saves, plus **138/138** extended. The **Sunsoft 5B absolute level** is calibrated against Mesen2, which required widening `Mapper::mix_audio` to `i32` (the correct full-scale 5B tone `1882 * 18.471 = 34,761` does not fit `i16`). A **save-state schema gap** is fixed — `PPU_SNAPSHOT_VERSION` **8** carrying the sprite-eval FSM + OAM data-bus state, plus an APU **v4** tail — which is what made AccuracyCoin report **141/141 through run-ahead** as well as without it; a new standing field-vs-schema audit found it and the two APU gaps mechanically. A **Zapper beam-relative light model** lands opt-in / default-off (no pass-fail light-gun ROM exists to adjudicate it). **AccuracyCoin holds 141/141 (100.00%)**, nestest 0-diff. Also: the eleven `sprintN.rs` mapper modules (27,631 lines, ~110 boards) are renamed for the boards they emulate with `mNNN_` mapper-number prefixes, proven content-preserving by a byte-for-byte item comparison (930 items, 0 altered) and an identical 172-ID dispatch table. +**Current release: v2.2.4 "Cartridge"** (2026-07-24) — a **libretro / RetroArch distribution** cut whose purpose is that the RustyNES core **builds and installs cleanly through the Libretro buildbot** () for in-RetroArch use. **Zero emulation-core changes** — the deterministic `#![no_std]` chip stack, save-state / TAS / netplay formats, and every golden vector are byte-identical to v2.2.3, so **AccuracyCoin holds 141/141 (100.00%)** and nestest is 0-diff by construction. `crates/rustynes-libretro` wraps `rustynes-core`, so it inherits every v2.2.3 change automatically (the fast-dot-path default; the `PPU_SNAPSHOT_VERSION` 8 / APU v4 save-state schema, transparent because `get_serialize_size` / `on_serialize` size and emit the *current* snapshot via `Nes::snapshot_core_into` rather than a fixed layout; the `Mapper::mix_audio` i32 widening; the Zapper model; the `mNNN_` mapper rename), and both buildbot cross-ABIs the CI early-warning gate models — `x86_64-pc-windows-gnu` and `aarch64-linux-android` — `cargo check --release -p rustynes-libretro` clean. The concrete change is a **`rustynes_libretro.info` metadata correction**: **`disk_control` `false` → `true`** (the real fix — the FDS multi-side Disk Control interface has been wired since the buildbot recipe landed but was advertised as absent, hiding multi-disk FDS swapping from RetroArch's Quick Menu), `display_version` `v1.0.0` → `v2.2.4`, and the description mapper count `168` → `172`. Libretro **core options** (region / overscan / palette / accuracy toggles) remain unexposed — `core_options = "false"` is accurate, a documented future enhancement rather than a v2.2.4 gap. The Antigravity PR reviewer standardization onto the shared template rides along. On top of **v2.2.3 "Datum"** (2026-07-23) — a **performance and accuracy-closure patch**, the product of a measure-first appraisal that profiled the emulator and acted on what the profile showed rather than on intuition. **Performance:** the specialized PPU fast dot path is promoted to the **default** and exposed to users for the first time — `Nes::set_fast_dotloop` had **no caller outside the core**, so a **−11.3%** frame-time win (fresh clean-host Criterion, reproducing v2.1.8's +12.3% by a different method; differential-tested bit-identical every frame since v2.1.8) shipped switched off and unreachable; release builds now ship **PGO-optimized** Linux binaries when the existing >3%-and-byte-identical gate passes; and CI gained a same-runner **relative** frame-time regression gate, closing a hole where a 2.5x slowdown passed the deliberately-loose absolute ceiling. **Two optimizations were measured and REJECTED** and are documented with their numbers per `docs/performance.md`'s convention — P3 (`emit_pixel` bounds-check elision) made the shipped default *slower* (+4.32% / +3.35% on the `_fast` workloads, p ≤ 0.02), and P4 (`cpu_clock`) found both textbook optimizations already implemented with the one remaining lever capped at **≤1.9%**. **Accuracy:** the **last two Holy Mapperel residuals are closed**, so all 17 ROMs report `detail=0000` (was 15/17) — MMC1's two software WRAM write-protect layers (`$E000` bit 4 + SNROM's CHR-register layer, gated on `chr_is_ram`) and FME-7's open bus on the RAM-selected-but-disabled window, both routed through the trait's existing `cpu_read_unmapped` contract. MMC1 is the change Holy Mapperel's README calls a game-compatibility hazard (FCEUX / PowerPak omit it), so it was validated before landing: **60/60** commercial ROMs including seven battery-backed MMC1 saves, plus **138/138** extended. The **Sunsoft 5B absolute level** is calibrated against Mesen2, which required widening `Mapper::mix_audio` to `i32` (the correct full-scale 5B tone `1882 * 18.471 = 34,761` does not fit `i16`). A **save-state schema gap** is fixed — `PPU_SNAPSHOT_VERSION` **8** carrying the sprite-eval FSM + OAM data-bus state, plus an APU **v4** tail — which is what made AccuracyCoin report **141/141 through run-ahead** as well as without it; a new standing field-vs-schema audit found it and the two APU gaps mechanically. A **Zapper beam-relative light model** lands opt-in / default-off (no pass-fail light-gun ROM exists to adjudicate it). **AccuracyCoin holds 141/141 (100.00%)**, nestest 0-diff. Also: the eleven `sprintN.rs` mapper modules (27,631 lines, ~110 boards) are renamed for the boards they emulate with `mNNN_` mapper-number prefixes, proven content-preserving by a byte-for-byte item comparison (930 items, 0 altered) and an identical 172-ID dispatch table. The prior release, **v2.2.2 "Conduit"** (2026-07-21), was a **build, distribution, and CI-integrity patch**: the **libretro buildbot recipe from 1 of 10 jobs green to all ten building** (the last step before RustyNES lands in RetroArch's built-in core downloader), a **GitHub Actions supply-chain hardening** pass (`persist-credentials: false` on all 19 checkouts, a fail-closed release-tag check via `git/matching-refs`, `dtolnay/rust-toolchain` SHA-pinned off `@master`), and the toolchain **collapsed to one pinned source of truth** — no toolchain version literal anywhere under `.github/` and **no `nightly` on any build path**. **Zero emulation-core changes**, so AccuracyCoin held 141/141 by construction. Its one behavioural improvement in a shipped artifact: the libretro **tvOS** core built with `panic = "abort"` like every other platform. @@ -43,7 +43,7 @@ The prior release, **v2.2.0 "Capstone"** (2026-07-12), was the **milestone cut** - **Mapper breadth → 172 families** (up from 168 at the v1.7.x tag), Core / Curated / BestEffort behind the CI accuracy-honesty gate. - **Release automation** — `.github/workflows/release-auto.yml`: when a new version goes final-green on `main`, it auto-tags + publishes the GitHub Release (body from a maintainer-authored `.github/release-notes/vX.Y.Z.md` override, else the CHANGELOG `[X.Y.Z]` section; title codename parsed from the CHANGELOG header) and builds + attaches the desktop binaries by invoking `release.yml` via `workflow_call` (a tag pushed by `GITHUB_TOKEN` can't trigger `on: push: tags`, hence the direct call). The v1.8.0–v1.9.9 GitHub Releases are all published with comprehensive notes + Linux / macOS-aarch64 / Windows binaries. -Platform additions through v1.10.0 were **host-only and additive**: the deterministic `#![no_std]` chip stack was untouched and byte-identical on ARM. **v2.0.0 "Timebase" is different by design** — it rewrites the scheduler substrate itself (still `#![no_std]`-clean, AccuracyCoin now back at a full **141/141 (100%)** from v2.0.3 — see above, but the save-state / movie format epochs deliberately bump per ADR 0028, so cross-version `.rns`/`.rnm` round-trip is a v1.x-only guarantee, not a v1.x⇄v2.x one). Forward path: the **v2.0.x "Harbor" mobile-finalization re-port train** onto the v2.0.0 core has fully shipped — v2.0.1 (first Android re-port + AccuracyCoin oracle re-sync), v2.0.2–v2.0.3 (the 2-cycle-ALE accuracy closure to 141/141), v2.0.4 (Android release candidate), v2.0.5–v2.0.8 (iOS finalization), and v2.0.9 (both-apps readiness) — followed by the **v2.1.x "Fathom" accuracy line** (v2.1.0 → v2.1.10) capped by the **v2.2.0 "Capstone"** milestone cut, then the v2.2.1 housekeeping patch, **v2.2.2 "Conduit"**, and **v2.2.3 "Datum"**, the current release; see the "Current release" paragraph above. The **v2.1.5 → v2.2.0** line is a **"deepen the existing project"** run (accuracy / performance / features / quality); **v2.1.5 "Vernier"** opened it (the tepples Holy Mapperel mapper bank-reachability / IRQ regression net, the first PAL-region APU oracle at `pal_apu_tests` 10/10, the MMC3 R1/R2 F5.0 A12-phase study, a measured fat-LTO A/B, and a real TURN NAT-traversal retransmit production fix — all NTSC-byte-identical), **v2.1.6 "Timbre"** continued it (the expansion-audio decibel oracle, the hardware/Mesen2 channel-level calibration incl. the Namco 163 ~12 dB fix, VRC7 patch-set verification vs Nuke.YKT, and a frontend Audio Mixer panel — base 2A03 NTSC output byte-identical), **v2.1.7 "Stepping"** added opt-in PPU / 2A03 die-revisions + power-on RAM/palette hardware models (the DMA "unexpected read" frontier proven a documented no-op on every oracle, ADR 0033 — honest, not faked), **v2.1.8 "Tempo"** the default-OFF specialized fast PPU dot path (~12% rendering-heavy, differential-tested bit-identical) + a SIMD-validated software blitter + a wasm size pass, **v2.1.9 "Aperture"** the marquee CRT shader stack + a raw NTSC composite signal-decode path + GIF/WAV capture + a palette editor, **v2.1.10 "Loom"** the TAStudio greenzone + Lua API breadth + the browser-RA auth-proxy deploy stack + Vs. `DualSystem` libretro presentation, and **v2.2.0 "Capstone"** the milestone cut closing the run (the netplay matchmaking / lobby stack + the FDS medium model + a peripherals & quality/security pass — fuzz targets 3 → 8, a `Movie::deserialize` OOM-DoS fix, a read-only Tools → ROM Info browser) — all NTSC-byte-identical, AccuracyCoin 141/141 throughout; the v2.1.5 → v2.2.0 run is now closed. The **joint Google Play + Apple App Store + AltStore PAL + F-Droid launch** — with it the `rustynes-monetization` activation — is the future **v2.3.0** (moved from the earlier v2.1.0 / v2.2.0 targets). +Platform additions through v1.10.0 were **host-only and additive**: the deterministic `#![no_std]` chip stack was untouched and byte-identical on ARM. **v2.0.0 "Timebase" is different by design** — it rewrites the scheduler substrate itself (still `#![no_std]`-clean, AccuracyCoin now back at a full **141/141 (100%)** from v2.0.3 — see above, but the save-state / movie format epochs deliberately bump per ADR 0028, so cross-version `.rns`/`.rnm` round-trip is a v1.x-only guarantee, not a v1.x⇄v2.x one). Forward path: the **v2.0.x "Harbor" mobile-finalization re-port train** onto the v2.0.0 core has fully shipped — v2.0.1 (first Android re-port + AccuracyCoin oracle re-sync), v2.0.2–v2.0.3 (the 2-cycle-ALE accuracy closure to 141/141), v2.0.4 (Android release candidate), v2.0.5–v2.0.8 (iOS finalization), and v2.0.9 (both-apps readiness) — followed by the **v2.1.x "Fathom" accuracy line** (v2.1.0 → v2.1.10) capped by the **v2.2.0 "Capstone"** milestone cut, then the v2.2.1 housekeeping patch, **v2.2.2 "Conduit"**, **v2.2.3 "Datum"**, and **v2.2.4 "Cartridge"** (the libretro/RetroArch distribution cut), the current release; see the "Current release" paragraph above. The **v2.1.5 → v2.2.0** line is a **"deepen the existing project"** run (accuracy / performance / features / quality); **v2.1.5 "Vernier"** opened it (the tepples Holy Mapperel mapper bank-reachability / IRQ regression net, the first PAL-region APU oracle at `pal_apu_tests` 10/10, the MMC3 R1/R2 F5.0 A12-phase study, a measured fat-LTO A/B, and a real TURN NAT-traversal retransmit production fix — all NTSC-byte-identical), **v2.1.6 "Timbre"** continued it (the expansion-audio decibel oracle, the hardware/Mesen2 channel-level calibration incl. the Namco 163 ~12 dB fix, VRC7 patch-set verification vs Nuke.YKT, and a frontend Audio Mixer panel — base 2A03 NTSC output byte-identical), **v2.1.7 "Stepping"** added opt-in PPU / 2A03 die-revisions + power-on RAM/palette hardware models (the DMA "unexpected read" frontier proven a documented no-op on every oracle, ADR 0033 — honest, not faked), **v2.1.8 "Tempo"** the default-OFF specialized fast PPU dot path (~12% rendering-heavy, differential-tested bit-identical) + a SIMD-validated software blitter + a wasm size pass, **v2.1.9 "Aperture"** the marquee CRT shader stack + a raw NTSC composite signal-decode path + GIF/WAV capture + a palette editor, **v2.1.10 "Loom"** the TAStudio greenzone + Lua API breadth + the browser-RA auth-proxy deploy stack + Vs. `DualSystem` libretro presentation, and **v2.2.0 "Capstone"** the milestone cut closing the run (the netplay matchmaking / lobby stack + the FDS medium model + a peripherals & quality/security pass — fuzz targets 3 → 8, a `Movie::deserialize` OOM-DoS fix, a read-only Tools → ROM Info browser) — all NTSC-byte-identical, AccuracyCoin 141/141 throughout; the v2.1.5 → v2.2.0 run is now closed. The **joint Google Play + Apple App Store + AltStore PAL + F-Droid launch** — with it the `rustynes-monetization` activation — is the future **v2.3.0** (moved from the earlier v2.1.0 / v2.2.0 targets). --- @@ -185,7 +185,7 @@ These cross-cutting decisions span multiple files. Reading individual chip docs - `ref-docs/` is immutable. Research updates go in dated supplemental files. - ADRs go in `docs/adr/` (Michael Nygard format). - `rustynes-core` re-exports the public types from the chip crates; downstream consumers (`rustynes-frontend`, `rustynes-test-harness`) should depend on `rustynes-core` rather than the chip crates directly. -- When relabeling old engine "v2.x" narrative for users, present it as upstream lineage/history — **never as a current RustyNES release version.** The current release is **v2.2.3 "Datum"** (2026-07-23, a performance and accuracy-closure patch — the fast PPU dot path promoted to default and exposed, PGO binaries shipped on the release path, a same-runner relative frame-time CI gate, the last two Holy Mapperel residuals closed [MMC1 WRAM write-protect + FME-7 open bus, all 17 ROMs now `detail=0000`], the Sunsoft 5B level calibrated with `Mapper::mix_audio` widened to i32, a save-state schema gap fixed at `PPU_SNAPSHOT_VERSION` 8 + an APU v4 tail, an opt-in Zapper beam-relative light model, and the eleven `sprintN.rs` mapper modules renamed to `mNNN_.rs`; two optimizations measured and REJECTED and documented as such; AccuracyCoin 141/141 — on top of **v2.2.2 "Conduit"** [2026-07-21, a build/distribution/CI-integrity patch — the libretro buildbot recipe taken from 1 of 10 jobs green to all ten building, a GitHub Actions supply-chain hardening pass, and the toolchain collapsed to one pinned source of truth with no `nightly` on any build path; zero emulation-core changes], itself on **v2.2.1** [2026-07-15, a housekeeping patch: dev-tooling archival, a zero-source-change dependency consolidation, and a gitignored FDS test-corpus addition], itself on **v2.2.0 "Capstone"** [2026-07-12], the milestone cut that closes the v2.1.5 → v2.2.0 "deepen the existing project" run — its two remaining marquees the netplay matchmaking / lobby stack and the FDS medium model, atop a peripherals + quality/security pass (Famicom `$4016`-bit-2 microphone + 3×3-aperture Zapper; cargo-fuzz targets 3 → 8 finding + fixing two `Movie::deserialize` OOM-DoS paths; a read-only Tools → ROM Info browser); every change additive or default-off, AccuracyCoin 141/141) on the v2.0.0 "Timebase" one-clock / every-cycle-bus-access scheduler rewrite + Vs. `DualSystem` dual-console support. The v2.0.x "Harbor" mobile-finalization train (v2.0.1→v2.0.9) and the entire v2.1.x "Fathom" line (v2.1.0→v2.1.10) plus the v2.2.0 "Capstone" milestone have all shipped — the run's steps being v2.1.5 "Vernier" (regression-net & residual) → v2.1.6 "Timbre" (expansion-audio fidelity) → v2.1.7 "Stepping" (opt-in PPU/2A03 die-revisions + power-on RAM/palette models; the DMA "unexpected read" frontier a documented no-op on every oracle, ADR 0033) → v2.1.8 "Tempo" (a default-OFF fast PPU dot path + SIMD blitter + wasm size pass) → v2.1.9 "Aperture" (a marquee CRT shader stack + raw NTSC composite signal-decode + GIF/WAV capture + palette editor) → v2.1.10 "Loom" (TAStudio greenzone + Lua API breadth + browser-RA auth-proxy deploy stack + Vs. `DualSystem` libretro presentation) → v2.2.0 "Capstone" (the milestone cut closing the run) → v2.2.1 (housekeeping) → **v2.2.2 "Conduit"** the build/distribution/CI-integrity patch (the prior release) — preceded by v1.10.0 "Arcade" the native Libretro / RetroArch core, the v1.9.0→v1.9.9 iOS TestFlight train, the v1.8.0→v1.8.9 "Android" train, and the desktop-feature lineage v1.1.0→v1.7.1, all on the v1.0.0 production core (see the top "Current release" block + `docs/STATUS.md`). **Never claim any version *later* than v2.2.3 is released** — in particular the joint mobile app-store launch (Google Play + Apple App Store + AltStore PAL + F-Droid) is the future **v2.3.0** (NOT v2.1.0 or v2.2.0 — the entire v2.1.x line and the v2.2.0 "Capstone" milestone have all already shipped, closing the "deepen the existing project" run; the store launch moved out to v2.3.0 — see `to-dos/ROADMAP.md`). Two distinct "v2.0"s exist and must not be conflated, **both now shipped, at different times, for different reasons**: the **engine-lineage v2.0** master-clock work shipped as the **v1.0.0** production core (2026-06-13) — it was the *only* scheduler through v1.10.0. RustyNES's own **v2.0.0 "Timebase"** release (2026-07-03) is a *different* milestone that *replaces* that same dot-lockstep scheduler outright: the **one-clock + every-cycle-bus-access collapse** (a single canonical cycle counter + a split-around-the-access `start_cycle`/`end_cycle` PPU catch-up, mirroring Mesen2's structure), full Vs. `DualSystem` dual-console emulation (core-and-harness-only; frontend wiring deferred), and the breaking save-state / cross-version changes it entailed (ADR 0002 / ADR 0028 / ADR 0029) — the one release that broke byte-identity / save-state compatibility, by design. The R1/R2 hard-tier MMC3 IRQ-timing residual was investigated under a bounded-effort campaign and is by-design-deferred beyond v2.0.0, not closed — see ADR 0002's decision-update section for the mechanism-level finding. +- When relabeling old engine "v2.x" narrative for users, present it as upstream lineage/history — **never as a current RustyNES release version.** The current release is **v2.2.4 "Cartridge"** (2026-07-24, a libretro/RetroArch distribution cut — zero emulation-core changes so AccuracyCoin holds 141/141 by construction; the libretro core is confirmed up-to-date with all recent changes and builds for the buildbot ABIs [`x86_64-pc-windows-gnu`, `aarch64-linux-android`], and `rustynes_libretro.info` is corrected: `disk_control` false→true [the FDS Disk Control interface was wired but advertised absent], `display_version` v1.0.0→v2.2.4, mapper count 168→172; core options remain a documented future enhancement; the Antigravity reviewer standardization rides along), on top of **v2.2.3 "Datum"** (2026-07-23, a performance and accuracy-closure patch — the fast PPU dot path promoted to default and exposed, PGO binaries shipped on the release path, a same-runner relative frame-time CI gate, the last two Holy Mapperel residuals closed [MMC1 WRAM write-protect + FME-7 open bus, all 17 ROMs now `detail=0000`], the Sunsoft 5B level calibrated with `Mapper::mix_audio` widened to i32, a save-state schema gap fixed at `PPU_SNAPSHOT_VERSION` 8 + an APU v4 tail, an opt-in Zapper beam-relative light model, and the eleven `sprintN.rs` mapper modules renamed to `mNNN_.rs`; two optimizations measured and REJECTED and documented as such; AccuracyCoin 141/141 — on top of **v2.2.2 "Conduit"** [2026-07-21, a build/distribution/CI-integrity patch — the libretro buildbot recipe taken from 1 of 10 jobs green to all ten building, a GitHub Actions supply-chain hardening pass, and the toolchain collapsed to one pinned source of truth with no `nightly` on any build path; zero emulation-core changes], itself on **v2.2.1** [2026-07-15, a housekeeping patch: dev-tooling archival, a zero-source-change dependency consolidation, and a gitignored FDS test-corpus addition], itself on **v2.2.0 "Capstone"** [2026-07-12], the milestone cut that closes the v2.1.5 → v2.2.0 "deepen the existing project" run — its two remaining marquees the netplay matchmaking / lobby stack and the FDS medium model, atop a peripherals + quality/security pass (Famicom `$4016`-bit-2 microphone + 3×3-aperture Zapper; cargo-fuzz targets 3 → 8 finding + fixing two `Movie::deserialize` OOM-DoS paths; a read-only Tools → ROM Info browser); every change additive or default-off, AccuracyCoin 141/141) on the v2.0.0 "Timebase" one-clock / every-cycle-bus-access scheduler rewrite + Vs. `DualSystem` dual-console support. The v2.0.x "Harbor" mobile-finalization train (v2.0.1→v2.0.9) and the entire v2.1.x "Fathom" line (v2.1.0→v2.1.10) plus the v2.2.0 "Capstone" milestone have all shipped — the run's steps being v2.1.5 "Vernier" (regression-net & residual) → v2.1.6 "Timbre" (expansion-audio fidelity) → v2.1.7 "Stepping" (opt-in PPU/2A03 die-revisions + power-on RAM/palette models; the DMA "unexpected read" frontier a documented no-op on every oracle, ADR 0033) → v2.1.8 "Tempo" (a default-OFF fast PPU dot path + SIMD blitter + wasm size pass) → v2.1.9 "Aperture" (a marquee CRT shader stack + raw NTSC composite signal-decode + GIF/WAV capture + palette editor) → v2.1.10 "Loom" (TAStudio greenzone + Lua API breadth + browser-RA auth-proxy deploy stack + Vs. `DualSystem` libretro presentation) → v2.2.0 "Capstone" (the milestone cut closing the run) → v2.2.1 (housekeeping) → **v2.2.2 "Conduit"** the build/distribution/CI-integrity patch (the prior release) — preceded by v1.10.0 "Arcade" the native Libretro / RetroArch core, the v1.9.0→v1.9.9 iOS TestFlight train, the v1.8.0→v1.8.9 "Android" train, and the desktop-feature lineage v1.1.0→v1.7.1, all on the v1.0.0 production core (see the top "Current release" block + `docs/STATUS.md`). **Never claim any version *later* than v2.2.4 is released** — in particular the joint mobile app-store launch (Google Play + Apple App Store + AltStore PAL + F-Droid) is the future **v2.3.0** (NOT v2.1.0 or v2.2.0 — the entire v2.1.x line and the v2.2.0 "Capstone" milestone have all already shipped, closing the "deepen the existing project" run; the store launch moved out to v2.3.0 — see `to-dos/ROADMAP.md`). Two distinct "v2.0"s exist and must not be conflated, **both now shipped, at different times, for different reasons**: the **engine-lineage v2.0** master-clock work shipped as the **v1.0.0** production core (2026-06-13) — it was the *only* scheduler through v1.10.0. RustyNES's own **v2.0.0 "Timebase"** release (2026-07-03) is a *different* milestone that *replaces* that same dot-lockstep scheduler outright: the **one-clock + every-cycle-bus-access collapse** (a single canonical cycle counter + a split-around-the-access `start_cycle`/`end_cycle` PPU catch-up, mirroring Mesen2's structure), full Vs. `DualSystem` dual-console emulation (core-and-harness-only; frontend wiring deferred), and the breaking save-state / cross-version changes it entailed (ADR 0002 / ADR 0028 / ADR 0029) — the one release that broke byte-identity / save-state compatibility, by design. The R1/R2 hard-tier MMC3 IRQ-timing residual was investigated under a bounded-effort campaign and is by-design-deferred beyond v2.0.0, not closed — see ADR 0002's decision-update section for the mechanism-level finding. - **Forward plans + roadmap live in `to-dos/`.** `to-dos/ROADMAP.md` (updated in #129) is the planning entry point and frames the release line + "the path to v2.0.0 and beyond"; `to-dos/plans/` holds the per-release plan docs (through `v1.7.0-forge-plan.md` on `main`, plus the staged-forward `v1.8.0-android-plan.md` / `v1.9.0-ios-plan.md` / `v2.0.0-master-clock-plan.md`) + the `to-dos/plans/engine-lineage/` history archive + a `to-dos/plans/research/` reference-mining archive. - The v1.0.0 release + GitHub Pages/CI + post-release record is in `docs/v1.0.0-synthesis-handoff-2026-06-13.md` — read it before touching CI, Pages, or release tooling. Full per-release history is in `CHANGELOG.md`. - **Markdownlint is a CI gate** (pre-commit, pinned `markdownlint-cli v0.39.0`). The local `markdownlint` binary is a newer version that reports rules v0.39.0 lacks (e.g. MD060) — those are NOT gated; verify with `pre-commit run markdownlint --all-files`, not the bare binary. `.markdownlint.json` keeps `MD013`/`MD033`/`MD041` disabled by design (long technical tables, the README HTML banner/``, the HTML-led README). `.markdownlintignore` exempts `ref-docs/`, `ref-proj/`, the vendored `tricnes/` + upstream READMEs, and the frozen `docs/archive/` + `to-dos/archive/` trees — don't lint or reformat those. diff --git a/CHANGELOG.md b/CHANGELOG.md index b4b258a9..b5bb0952 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,56 @@ cycle-accurate core later replaced. ## [Unreleased] +## [2.2.4] - 2026-07-24 - "Cartridge" (libretro core builds/installs for RetroArch) + +A **libretro / RetroArch distribution** cut. Its purpose is that the RustyNES +core builds and installs cleanly through the Libretro buildbot +() so RetroArch users can pull it +from the in-app core downloader. **Zero emulation-core changes**, so +AccuracyCoin holds **141/141 (100.00%)**, nestest is 0-diff, and the `#![no_std]` +chip stack, save-state / TAS / netplay formats, and every golden vector are +byte-identical to v2.2.3 by construction. + +### Libretro / distribution + +- **The libretro core is confirmed complete and up-to-date with every recent + change, and builds for the buildbot ABIs.** `crates/rustynes-libretro` wraps + `rustynes-core`, so it inherits the v2.2.3 work automatically and required no + code change to carry it: the fast PPU dot path (now the core default) is + active; the `PPU_SNAPSHOT_VERSION` 8 + APU v4 save-state schema is transparent + because `get_serialize_size` / `on_serialize` size and emit the *current* + snapshot via `Nes::snapshot_core_into` rather than a hardcoded layout; the + `Mapper::mix_audio` i32 widening, the Zapper model, and the `mNNN_` mapper + rename are all below the crate's public dependency surface. Both buildbot + cross-ABIs the GitHub early-warning gate models — `x86_64-pc-windows-gnu` and + `aarch64-linux-android` — `cargo check --release -p rustynes-libretro` + clean. +- **`rustynes_libretro.info` metadata corrected** (the file RetroArch's core + downloader reads to learn the core's capabilities): + - **`disk_control` `false` → `true`** — the real fix. The FDS multi-side Disk + Control interface (`enable_disk_control_interface()` + the + `on_set_eject_state` / `on_get_image_index` / … callback trampolines) has + been wired since the buildbot recipe landed, but the `.info` advertised it + as absent, so RetroArch's Quick Menu → Disk Control never surfaced multi-disk + FDS swapping. + - `display_version` `v1.0.0` → `v2.2.4` (stale since the v1.0.0 era). + - Description mapper count `168` → `172`, and a note that FDS multi-disk + swapping runs through the Disk Control interface. +- Documented follow-up: libretro **core options** (region / overscan / palette / + accuracy toggles) remain unexposed. `core_options = "false"` is accurate, not + stale — a deliberate future enhancement, not a v2.2.4 gap. + +### Tooling + +- **The Antigravity PR reviewer is standardized onto the shared template** + (`scripts/agy-review.sh` + `.github/workflows/antigravity-review.yml`), the + same canonical version now installed across RustyNES / RustySNES / RustyN64. + It carries the large-diff handling (a diff too big to inline goes to `agy` as + a file), the 20,000-line `gh pr diff` API-limit local-`git diff` fallback, the + `isCrossRepository` fork gate, fail-closed metadata, default-branch checkout + with `persist-credentials: false`, and the `synchronize` auto-re-review + trigger. + ## [2.2.3] - 2026-07-23 - "Datum" (fast dot path promoted + PGO shipped + the last two mapper residuals closed) ### Performance diff --git a/Cargo.lock b/Cargo.lock index 5b00f659..991986d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4290,7 +4290,7 @@ dependencies = [ [[package]] name = "rustynes-android" -version = "2.2.3" +version = "2.2.4" dependencies = [ "android-activity", "android_logger", @@ -4308,7 +4308,7 @@ dependencies = [ [[package]] name = "rustynes-apu" -version = "2.2.3" +version = "2.2.4" dependencies = [ "bitflags 2.13.1", "criterion", @@ -4321,7 +4321,7 @@ dependencies = [ [[package]] name = "rustynes-cheevos" -version = "2.2.3" +version = "2.2.4" dependencies = [ "cc", "ureq", @@ -4329,7 +4329,7 @@ dependencies = [ [[package]] name = "rustynes-core" -version = "2.2.3" +version = "2.2.4" dependencies = [ "bitflags 2.13.1", "criterion", @@ -4346,7 +4346,7 @@ dependencies = [ [[package]] name = "rustynes-cpu" -version = "2.2.3" +version = "2.2.4" dependencies = [ "bitflags 2.13.1", "criterion", @@ -4357,7 +4357,7 @@ dependencies = [ [[package]] name = "rustynes-frontend" -version = "2.2.3" +version = "2.2.4" dependencies = [ "anstyle", "arboard", @@ -4411,11 +4411,11 @@ dependencies = [ [[package]] name = "rustynes-gfx-shaders" -version = "2.2.3" +version = "2.2.4" [[package]] name = "rustynes-hdpack" -version = "2.2.3" +version = "2.2.4" dependencies = [ "lewton", "png", @@ -4426,7 +4426,7 @@ dependencies = [ [[package]] name = "rustynes-ios" -version = "2.2.3" +version = "2.2.4" dependencies = [ "bytemuck", "cpal", @@ -4440,7 +4440,7 @@ dependencies = [ [[package]] name = "rustynes-libretro" -version = "2.2.3" +version = "2.2.4" dependencies = [ "libc", "rust-libretro", @@ -4449,7 +4449,7 @@ dependencies = [ [[package]] name = "rustynes-mappers" -version = "2.2.3" +version = "2.2.4" dependencies = [ "bitflags 2.13.1", "criterion", @@ -4461,7 +4461,7 @@ dependencies = [ [[package]] name = "rustynes-mobile" -version = "2.2.3" +version = "2.2.4" dependencies = [ "rustynes-core", "rustynes-hdpack", @@ -4476,14 +4476,14 @@ dependencies = [ [[package]] name = "rustynes-monetization" -version = "2.2.3" +version = "2.2.4" dependencies = [ "uniffi", ] [[package]] name = "rustynes-netplay" -version = "2.2.3" +version = "2.2.4" dependencies = [ "futures-util", "js-sys", @@ -4499,7 +4499,7 @@ dependencies = [ [[package]] name = "rustynes-ppu" -version = "2.2.3" +version = "2.2.4" dependencies = [ "bitflags 2.13.1", "criterion", @@ -4511,14 +4511,14 @@ dependencies = [ [[package]] name = "rustynes-ra" -version = "2.2.3" +version = "2.2.4" dependencies = [ "rustynes-cheevos", ] [[package]] name = "rustynes-script" -version = "2.2.3" +version = "2.2.4" dependencies = [ "mlua", "piccolo", @@ -4529,7 +4529,7 @@ dependencies = [ [[package]] name = "rustynes-test-harness" -version = "2.2.3" +version = "2.2.4" dependencies = [ "insta", "png", diff --git a/Cargo.toml b/Cargo.toml index 7a5fc85a..d3d85516 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ members = [ default-members = ["crates/rustynes-libretro"] [workspace.package] -version = "2.2.3" +version = "2.2.4" edition = "2024" rust-version = "1.96" license = "MIT OR Apache-2.0" diff --git a/README.md b/README.md index c21714ab..e1e627e1 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@

- Build Status License: MIT OR Apache-2.0 Version Rust: 1.96
+ Build Status License: MIT OR Apache-2.0 Version Rust: 1.96
AccuracyCoin nestest Try in browser
Platform

@@ -775,35 +775,46 @@ and the Material-for-MkDocs documentation handbook at ## Current Release -RustyNES's current release is **v2.2.3 "Datum"**, a **performance and -accuracy-closure patch**. A measure-first appraisal profiled the emulator and -acted on what it found rather than on intuition. - -**Performance.** The specialized PPU fast dot path — measured at **−11.3%** -frame time on rendering-heavy content and differential-tested bit-identical -every frame since v2.1.8 — is now the **default**, and reachable from the -frontend for the first time (`Nes::set_fast_dotloop` previously had no caller -outside the core, so the win shipped switched off and unreachable). Release -builds now ship **PGO-optimized** Linux binaries when the >3%-and-byte-identical -gate passes, and CI gained a same-runner relative frame-time regression gate to -close a hole where a 2.5x slowdown could pass the old absolute ceiling. - -**Accuracy.** The **last two Holy Mapperel residuals are closed** — all 17 ROMs -now report `detail=0000`. MMC1's two software WRAM write-protect layers and -FME-7's open-bus-on-disabled-RAM window are both modelled, the former validated -against **60/60** commercial ROMs (including seven battery-backed MMC1 saves, -precisely the titles that corrupt if the RAM enable is wrong) plus **138/138** -extended. The Sunsoft 5B's absolute level is calibrated against Mesen2, which -required widening `Mapper::mix_audio` to `i32` — the correct full-scale 5B tone -does not fit `i16`. A save-state schema gap found by a new standing audit is -fixed (`PPU_SNAPSHOT_VERSION` 8, plus an APU v4 tail), which is what made -AccuracyCoin report 141/141 through run-ahead as well as without it. - -**Two optimizations were measured and rejected**, and are documented with their -numbers — the project records what did not clear the bar as well as what did. -**AccuracyCoin holds 141/141 (100.00%)**, nestest 0-diff. - -It follows **v2.2.2 "Conduit"**, a build, distribution, and CI-integrity patch +RustyNES's current release is **v2.2.4 "Cartridge"**, a **libretro / RetroArch +distribution** cut. Its purpose is that the RustyNES core builds and installs +cleanly through the Libretro buildbot +([git.libretro.com/libretro/RustyNES](https://git.libretro.com/libretro/RustyNES)) +so RetroArch users can pull it from the in-app core downloader. + +**Zero emulation-core changes**, so **AccuracyCoin holds 141/141 (100.00%)**, +nestest is 0-diff, and the `#![no_std]` chip stack, save-state / TAS / netplay +formats, and every golden vector are byte-identical to v2.2.3 by construction. +`crates/rustynes-libretro` wraps `rustynes-core`, so it inherits every v2.2.3 +change automatically (the fast dot path default; the `PPU_SNAPSHOT_VERSION` 8 / +APU v4 save-state schema, handled transparently because the serialize path sizes +and emits the *current* snapshot via `Nes::snapshot_core_into`; the +`Mapper::mix_audio` i32 widening; the Zapper model; the `mNNN_` mapper rename), +and both buildbot cross-ABIs the CI gate models — `x86_64-pc-windows-gnu` and +`aarch64-linux-android` — build clean. + +The concrete work is a **`rustynes_libretro.info` metadata correction** (the file +RetroArch's core downloader reads): **`disk_control` `false` → `true`** — the +real fix, since the FDS multi-side Disk Control interface has been wired since +the buildbot recipe landed but was advertised as absent, hiding multi-disk FDS +swapping from RetroArch's Quick Menu; plus `display_version` `v1.0.0` → `v2.2.4` +and the mapper count `168` → `172`. Libretro **core options** (region / overscan +/ palette / accuracy toggles) remain a documented future enhancement +(`core_options = "false"` is accurate, not stale). The Antigravity PR reviewer +standardization onto the shared template rides along. + +It follows **v2.2.3 "Datum"**, a performance and accuracy-closure patch: the +specialized PPU fast dot path (**−11.3%** on rendering-heavy content, +differential-tested bit-identical since v2.1.8) promoted to the **default** and +exposed to users for the first time; PGO-optimized Linux release binaries when +the >3%-and-byte-identical gate passes; a same-runner relative frame-time +regression gate; the **last two Holy Mapperel residuals closed** (all 17 ROMs +`detail=0000` — MMC1's two WRAM write-protect layers validated against **60/60** +commercial ROMs, FME-7's open-bus window); the Sunsoft 5B level calibrated to +Mesen2 (widening `Mapper::mix_audio` to `i32`); and a save-state schema gap fixed +(`PPU_SNAPSHOT_VERSION` 8 + APU v4 tail). Two optimizations were measured and +rejected, documented with their numbers. + +Earlier still: **v2.2.2 "Conduit"**, a build, distribution, and CI-integrity patch that took the libretro buildbot recipe from 1 of 10 jobs green to **all ten building**, hardened the GitHub Actions supply chain, and collapsed the toolchain to a single pinned source of truth with no `nightly` on any build diff --git a/crates/rustynes-libretro/rustynes_libretro.info b/crates/rustynes-libretro/rustynes_libretro.info index c3768bfc..e7fb90ab 100644 --- a/crates/rustynes-libretro/rustynes_libretro.info +++ b/crates/rustynes-libretro/rustynes_libretro.info @@ -5,7 +5,7 @@ supported_extensions = "nes|fds" corename = "RustyNES" license = "MIT OR Apache-2.0" permissions = "" -display_version = "v1.0.0" +display_version = "v2.2.4" categories = "Emulator" # Hardware Information @@ -25,7 +25,7 @@ core_options = "false" load_subsystem = "false" hw_render = "false" needs_fullpath = "false" -disk_control = "false" +disk_control = "true" database = "Nintendo - Nintendo Entertainment System|Nintendo - Family Computer Disk System" # Firmware / BIOS @@ -35,4 +35,4 @@ firmware0_path = "disksys.rom" firmware0_opt = "true" notes = "(!) disksys.rom (md5): ca30b50f880eb660a320674ed365ef7a|RustyNES requires this BIOS in the system directory to boot Famicom Disk System games." -description = "RustyNES is a cycle-accurate Nintendo Entertainment System emulator written entirely in safe Rust. Targeting the higan and Mesen2 accuracy tier, it relies on a strict, lockstep master-clock scheduler at PPU-dot resolution rather than per-game hacks. The core guarantees absolute determinism, making it an exceptional choice for advanced libretro features like run-ahead latency reduction, rollback netplay, and RetroAchievements. With high compatibility spanning over 168 mapper families, full Famicom Disk System support, and true RGB Vs. System arcade boards, RustyNES is ideal for users who seek reference-grade accuracy paired with modern stability." +description = "RustyNES is a cycle-accurate Nintendo Entertainment System emulator written entirely in safe Rust. Targeting the higan and Mesen2 accuracy tier, it relies on a strict, lockstep master-clock scheduler at PPU-dot resolution rather than per-game hacks. The core guarantees absolute determinism, making it an exceptional choice for advanced libretro features like run-ahead latency reduction, rollback netplay, and RetroAchievements. With high compatibility spanning over 172 mapper families, full Famicom Disk System support (multi-disk swapping via the Disk Control interface), and true RGB Vs. System arcade boards, RustyNES is ideal for users who seek reference-grade accuracy paired with modern stability." diff --git a/docs/STATUS.md b/docs/STATUS.md index 87df66ad..9ed61da9 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -1,6 +1,32 @@ # RustyNES — Project Status Matrix -> **Current release: v2.2.3** (2026-07-23) — **"Datum"**, a **performance and +> **Current release: v2.2.4** (2026-07-24) — **"Cartridge"**, a **libretro / +> RetroArch distribution** cut whose purpose is that the RustyNES core builds and +> installs cleanly through the Libretro buildbot +> () for in-RetroArch use. **Zero +> emulation-core changes** — the deterministic `#![no_std]` chip stack, +> save-state / TAS / netplay formats, and every golden vector are byte-identical +> to v2.2.3, so **AccuracyCoin holds 141/141 (100.00%)** and nestest is 0-diff by +> construction. `crates/rustynes-libretro` wraps `rustynes-core` and so inherits +> every v2.2.3 change automatically (the fast-dot-path default; the +> `PPU_SNAPSHOT_VERSION` 8 / APU v4 save-state schema, handled transparently +> because `get_serialize_size` / `on_serialize` size and emit the *current* +> snapshot via `Nes::snapshot_core_into`, not a fixed layout; the +> `Mapper::mix_audio` i32 widening; the Zapper model; the `mNNN_` mapper rename), +> and both buildbot cross-ABIs the CI early-warning gate models — +> `x86_64-pc-windows-gnu` and `aarch64-linux-android` — `cargo check --release -p +> rustynes-libretro` clean. The concrete change is a **`rustynes_libretro.info` +> metadata correction**: **`disk_control` `false` → `true`** (the real fix — the +> FDS multi-side Disk Control interface has been wired since the buildbot recipe +> landed, but was advertised as absent, hiding multi-disk FDS swapping from +> RetroArch's Quick Menu), `display_version` `v1.0.0` → `v2.2.4`, and the mapper +> count `168` → `172`. Libretro **core options** (region / overscan / palette / +> accuracy toggles) remain unexposed — `core_options = "false"` is accurate, a +> documented future enhancement rather than a v2.2.4 gap. The Antigravity PR +> reviewer standardization onto the shared template rides along. On top of +> **v2.2.3** (below): +> +> > **v2.2.3** (2026-07-23) — **"Datum"**, a **performance and > accuracy-closure patch** on top of v2.2.2 (below), produced by a measure-first > appraisal that profiled the emulator and acted on what it found. > diff --git a/to-dos/ROADMAP.md b/to-dos/ROADMAP.md index 84d95725..4dfeb46d 100644 --- a/to-dos/ROADMAP.md +++ b/to-dos/ROADMAP.md @@ -46,7 +46,8 @@ v2.8.0 → v0.9.7; the synthesis itself = **v1.0.0**. ## Status -- **Current release:** **RustyNES v2.1.0 "Fathom"** (2026-07-09) — the **accuracy-remediation** release and the first of the new "Fathom" line, a **core / desktop** cut landing **ahead of** the joint mobile store launch (**moved from v2.1.0 to v2.2.0**, so the Android + iOS apps re-release on this improved core). The deterministic core is unchanged but for one **display-only** PPU fix, so **AccuracyCoin holds 141/141 (100.00%)**, nestest 0-diff, `#![no_std]` untouched, no save-state bump. Lands: **(F1.1)** the **PPU palette backdrop-override** (rendering-disabled + `v` in `$3F00-$3FFF` → `palette[v & 0x1F]`, **byte-exact with TriCNES**; 9 snapshots re-blessed — 2 palette demos + 7 commercial games — all converging with the oracle, `external_real_games` 60/60 byte-identical); **(F1.2/F1.3)** OAM + open-bus audits regression-locked; **(F3)** the **mapper completion** — **86** families promoted BestEffort → Curated with commercial-ROM oracle evidence (57 staged + 29 GoodNES v3.23b), so the tier is **51 Core + 95 Curated + 26 BestEffort** and oracle-gated coverage rises **60 → 146** of 172 (the 26 left have no cleanly-booting dump — 16 NES 2.0 high-id + 8 no-cart + 2 jam-at-boot); **(F5)** the **MMC3 R1/R2 scanline-IRQ residual CLOSED** by-design-permanent (ADR 0002 F5.0 — differential 1-dot deficit, structurally unreachable, zero game impact), with all **20** `#[ignore]`'d tests catalogued in the new `docs/accuracy-ledger.md`; and **(F0)** doc reconciliation (MMC5 + DualSystem stale docs). Version bump (workspace `2.0.8 → 2.1.0`; mobile `MARKETING_VERSION`s unchanged — apps re-release at v2.2.0). See `docs/STATUS.md` (single source of truth) + `CHANGELOG.md` `[2.1.0]` + `docs/accuracy-ledger.md` + `to-dos/plans/v2.1.0-fathom-accuracy-remediation-plan.md`. +- **Current release:** **RustyNES v2.2.4** (2026-07-24) — a **libretro / RetroArch distribution** cut whose purpose is that the RustyNES core **builds and installs cleanly through the Libretro buildbot** () for in-RetroArch use. **Zero emulation-core changes** — the deterministic `#![no_std]` chip stack, save-state / TAS / netplay formats, and every golden vector are byte-identical to v2.2.3, so **AccuracyCoin holds 141/141 (100.00%)**, nestest 0-diff, by construction. The work is a libretro-completeness audit + metadata correction: the core is confirmed to inherit every v2.2.3 change automatically (the fast-dot-path default, the `PPU_SNAPSHOT_VERSION` 8 / APU v4 save-state schema handled transparently by the dynamic `snapshot_core_into` sizing, the `Mapper::mix_audio` i32 widening, the Zapper model, and the `mNNN_` mapper rename), and both buildbot cross-ABIs the GitHub gate models — `x86_64-pc-windows-gnu` and `aarch64-linux-android` — build clean. `rustynes_libretro.info` (the metadata RetroArch's core downloader reads) is corrected: **`disk_control` `false` → `true`** (the FDS multi-side Disk Control interface has been wired since the buildbot recipe landed, but was advertised as absent — the real fix), `display_version` `v1.0.0` → `v2.2.4`, and the mapper count `168` → `172`. Also: the reviewer-tooling standardization onto the shared Antigravity template rides along (`scripts/agy-review.sh` + workflow). Documented libretro follow-up: **core options** (region / overscan / palette / accuracy toggles) remain unexposed (`core_options = "false"` is accurate, not stale) — a deliberate future enhancement, not a v2.2.4 gap. See `docs/STATUS.md` (single source of truth) + `CHANGELOG.md` `[2.2.4]` + `docs/libretro/`. +- **Release line since v2.1.0:** the v2.1.x **"Fathom"** accuracy line (v2.1.0 → v2.1.10) → **v2.2.0 "Capstone"** (the milestone cut closing the "deepen the existing project" run) → **v2.2.1** (housekeeping) → **v2.2.2 "Conduit"** (build / distribution / CI-integrity) → **v2.2.3 "Datum"** (performance appraisal + the last two Holy Mapperel residuals closed) → **v2.2.4** (this libretro/RetroArch cut). Each is byte-identical NTSC (AccuracyCoin 141/141 throughout). **Full per-release detail is in `CHANGELOG.md` and `docs/STATUS.md` (the single source of truth)** — the entries below (v2.1.0 "Fathom" was the prior anchor here; v2.0.8 → v2.0.1) are the older historical trail, retained rather than duplicated. - **Preceding release:** **RustyNES v2.0.8 "Harbor"** (2026-07-09) — the eighth release of the **v2.0.x mobile-finalization train** and the **iOS release candidate** ("Harborlight"), the final release of the iOS finalization window (**v2.0.5 → v2.0.8**). A **host / iOS-only** cut: the cycle-accurate core is **unchanged and byte-identical to v2.0.7** (AccuracyCoin still **141/141, 100.00%**; nestest 0-diff; `#![no_std]` chip stack untouched). It stages the App Store scaffolding for v2.1.0: version-controlled **App Store Connect listing metadata** (`fastlane/metadata/ios/{en-US,es-ES}/`, mirroring the Android tree, files-only), a **dormant App Store `release` lane** in `fastlane/Fastfile` that stages the build + listing but **does not submit** (`submit_for_review: false`) and is **not** CI-wired (the interim channel stays **TestFlight**), and an **App-Review §4.7 self-audit** (no bundled/downloadable ROMs, ownership notice, searchable library, 4+ rating) in `docs/ios-v2.0.8-readiness.md`. Version bump (workspace `2.0.7 → 2.0.8`; iOS `MARKETING_VERSION → 2.0.8`). **No store submission** (that is v2.1.0); screenshots, real signing, the listing upload, and the App-Review submission are the **maintainer / v2.0.9 / v2.1.0** closeout. See `docs/STATUS.md` (single source of truth) + `CHANGELOG.md` `[2.0.8]` + `docs/ios-v2.0.8-readiness.md` + `to-dos/plans/v2.0.5-v2.0.8-ios-finalization-plan.md`. - **Earlier in the train:** **RustyNES v2.0.7 "Harbor"** (2026-07-09) — the seventh release of the **v2.0.x mobile-finalization train** and the **third iOS finalization release** ("Trim"), continuing the iOS window (**v2.0.5 → v2.0.8**). A **host / iOS-only** cut: the cycle-accurate core is **unchanged and byte-identical to v2.0.6** (AccuracyCoin still **141/141, 100.00%**; nestest 0-diff; `#![no_std]` chip stack untouched). It wires the **App Store submission floor** (Apple mandates the **iOS 26 SDK / Xcode 26** for every App Store Connect upload from **2026-04-28**, so the tag-gated iOS CI now selects the newest Xcode 26.x on the runner — a build-SDK pin, non-breaking fallback on older images), **reconciles the deployment target `iOS 15.0 → 17.0`** to match the code's real API floor (`NavigationStack` iOS 16 + `.topBarTrailing` iOS 17, unguarded at 12+ sites — the prior 15.0 was never buildable), and **re-audits `PrivacyInfo.xcprivacy`** against the v2.0.6 crash reporter (no new data type / required-reason API — local-only, backup-excluded, off by default). Version bump (workspace `2.0.6 → 2.0.7`; iOS `MARKETING_VERSION → 2.0.7`). **TestFlight-only** (App Store + AltStore PAL deferred to v2.1.0); on-device profiling + the Xcode-26 archive are a **maintainer / v2.0.9** step. See `docs/STATUS.md` (single source of truth) + `CHANGELOG.md` `[2.0.7]` + `docs/ios-v2.0.7-readiness.md` + `to-dos/plans/v2.0.5-v2.0.8-ios-finalization-plan.md`. - **Earlier in the train:** **RustyNES v2.0.6 "Harbor"** (2026-07-09) — the sixth release of the **v2.0.x mobile-finalization train** and the **second iOS finalization release** ("Parity"), continuing the iOS window (**v2.0.5 → v2.0.8**). A **host / iOS-only** cut: the cycle-accurate core is **unchanged and byte-identical to v2.0.5** (AccuracyCoin still **141/141, 100.00%**; nestest 0-diff; `#![no_std]` chip stack untouched), so no accuracy / save-state / determinism number moves. It adds a **new opt-in, privacy-first crash-reporting surface** (off by default — the iOS analogue of the Android v1.8.8 `CrashReporter`, closing the v1.9.9 iOS-applicable deferral): **Settings → Diagnostics** installs an uncaught-`NSException` handler that writes **local** crash logs the user can view + copy in-app — **nothing is uploaded**, so the "Data Not Collected" privacy label is unchanged (EN + ES); the handler re-checks the live opt-in at crash time so opting out stops new logs immediately. It also records the **feature-parity re-verification** of the v1.9.x host features (Game Center, CloudKit save sync, MFi controllers, capture / PiP, accessibility) against the unchanged v2.0.0 bridge surface. Version bump (workspace `2.0.5 → 2.0.6`; iOS `MARKETING_VERSION → 2.0.6`). **TestFlight-only** (App Store + AltStore PAL deferred to v2.1.0); on-device crash-capture verification is a **maintainer / v2.0.9** step. See `docs/STATUS.md` (single source of truth) + `CHANGELOG.md` `[2.0.6]` + `docs/ios-v2.0.6-readiness.md` + `to-dos/plans/v2.0.5-v2.0.8-ios-finalization-plan.md`. From 7ce7fc7fbbc2fb7f379d7865c01a7f697f73491b Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 23 Jul 2026 19:37:28 -0400 Subject: [PATCH 3/8] fix(security): restore five agy-review hardenings the reviewer flagged on this PR The Antigravity reviewer ran against this PR and flagged five security regressions in `scripts/agy-review.sh` -- each a case where standardizing onto the shared template quietly WEAKENED what RustyNES's prior (main) reviewer already did. The reviewer catching a regression in its own standardization is the system working as intended; all five are fixed, and the fixes are also committed upstream in the canonical template so re-installs carry them. 1. Command injection in the `script(1)` PTY fallback (BLOCKING). The command handed to `script -qfec` (run via `sh -c`) interpolated `${flags[*]}` raw; the flag values come from env vars (AGY_MODEL / AGY_EFFORT / AGY_PRINT_TIMEOUT), so a shell metacharacter in any would be evaluated by the inner shell. Rebuilt with `printf '%q '` (main's proven form). 2. Arbitrary comment deletion (BLOCKING). The prior-comment cleanup selected every comment CONTAINING the marker with no author filter, so any user could put the marker (an HTML comment) in a comment and have the bot delete arbitrary comments. Now scoped to `.user.type == "Bot" and .user.login == "github-actions[bot]"`. 3. SQLite conversation-store data leak (BLOCKING). The empty-output fallback read the most-recent `*.db` under `$CONV_DIR` by mtime with no session scoping; on a shared runner that db can belong to an unrelated concurrent local `agy` session, leaking its output into a public PR comment. Removed entirely -- the PTY path + retry loop cover agy issue #76, and main never had it. Dead `$CONV_DIR` dropped with it. 4. agy inherited the repo tokens (suggestion). agy runs under --dangerously-skip-permissions and ingests an untrusted diff yet inherited GH_TOKEN / GITHUB_TOKEN, which it never uses. Both invocations now run under `env -u GH_TOKEN -u GITHUB_TOKEN`. 5. Missing issue_comment author re-check (suggestion). The workflow `if:` gates `/agy-review` on OWNER/MEMBER/COLLABORATOR, but the script -- also hand-runnable -- did not re-check it. The `author_association` gate is restored in the script's `issue_comment` branch as defense in depth. This is the same reconciliation error that produced the standardized template: the earlier pass ported RustyNES's LARGE-DIFF features into the template but not its SECURITY surface, and wrongly concluded RustyNES had nothing to add. main's version was harder in these five places; the template (and the just-merged RustySNES / RustyN64 standardizations) inherited the weaker forms. RustySNES and RustyN64 need a follow-up re-sync from the now-hardened template. Verified `bash -n` clean and `shellcheck -S warning` clean beyond the pre-existing SC1007/SC2218 config-block findings; the CHANGELOG Tooling entry records the hardening. --- CHANGELOG.md | 9 +++++++ scripts/agy-review.sh | 57 ++++++++++++++++++++++++------------------- 2 files changed, 41 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5bb0952..b5e9d407 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,15 @@ byte-identical to v2.2.3 by construction. `isCrossRepository` fork gate, fail-closed metadata, default-branch checkout with `persist-credentials: false`, and the `synchronize` auto-re-review trigger. +- **Reviewer security hardening (found by the reviewer itself).** The + Antigravity reviewer, run against this PR, flagged five security regressions + the standardized template had relative to RustyNES's prior version — all + fixed: `printf '%q '`-escaped `script(1)` fallback (was a raw `${flags[*]}` in + `sh -c` — command injection), an author-scoped comment-deletion filter (was + marker-only — arbitrary comment deletion), removal of the unscoped SQLite + conversation-store fallback (a shared-runner data-leak vector), stripping + `GH_TOKEN` / `GITHUB_TOKEN` from `agy`'s environment (`env -u`), and the + `issue_comment` author-association re-check restored in the script. ## [2.2.3] - 2026-07-23 - "Datum" (fast dot path promoted + PGO shipped + the last two mapper residuals closed) diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 8569730b..6c79d91a 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -42,7 +42,6 @@ if ! [ "$MAX_PROMPT_BYTES" -le "$ARG_SIZE_CEILING" ] 2>/dev/null; then fi 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 @@ -63,11 +62,20 @@ 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, 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 ;; + esac PR="$(jq -r '.issue.number' "$GITHUB_EVENT_PATH")" ;; *) @@ -307,6 +315,12 @@ 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 ) + +# 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" ) @@ -336,14 +350,17 @@ for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do 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" || 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 + # `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" || true col -b < "$raw" > "$out_file" rm -f "$raw" # each retry makes a fresh $raw; the EXIT trap only holds the last one fi @@ -351,24 +368,11 @@ 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. - 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 + # 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 @@ -409,8 +413,11 @@ 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 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(.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 b2b613bf9af1dfacbde4f2b1f598ed9a402b7247 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 23 Jul 2026 19:47:16 -0400 Subject: [PATCH 4/8] =?UTF-8?q?fix(security,robustness):=20address=202nd?= =?UTF-8?q?=20agy-review=20pass=20=E2=80=94=20log=20path,=20diff=5Ferr,=20?= =?UTF-8?q?UTF-8=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The re-review of the hardened reviewer (from the first fix commit) flagged three more items; all addressed, and mirrored upstream in the canonical template: 1. BLOCKING -- fixed /tmp log path. Standardizing dropped the run-id/PID suffix (a 6th regression from main), so a fixed /tmp/agy-review.log collides between concurrent jobs when RUNNER_TEMP is unset and is a predictable symlink/tamper target. Restored LOG=...agy-review-${GITHUB_RUN_ID:-$$}.log with the collision-avoidance comment (main's proven form). 2. diff_err is now allocated alongside diff_file / meta_file up front, so the cleanup trap cannot reference it before it exists if the early metadata fetch exits (it was pre-declared empty, so this was hardening, not a bug). 3. UTF-8 sanitize path gained a python3 fallback for hosts without iconv (a template feature main never had, so not a regression -- agy's point is a valid edge-case hardening): a no-iconv host could otherwise pass a truncation-split multibyte sequence to agy, and Rust std::env::args() panics on non-UTF-8. Both iconv and python3 absent is vanishingly rare on a CI runner. bash -n + shellcheck clean beyond the pre-existing SC1007/SC2218. --- scripts/agy-review.sh | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 6c79d91a..4d252136 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -42,7 +42,11 @@ if ! [ "$MAX_PROMPT_BYTES" -le "$ARG_SIZE_CEILING" ] 2>/dev/null; then fi STYLE_GUIDE="${STYLE_GUIDE:-.github/agy-review.md}" # repo-relative; loaded if present # (dedicated name -- avoids colliding with GEMINI.md/AGENTS.md) -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) -- 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 AGY_RETRIES="${AGY_RETRIES:-3}" # attempts to get a usable agy response @@ -103,7 +107,7 @@ trap cleanup EXIT # 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_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; } @@ -147,7 +151,8 @@ esac # 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="$(mktemp)" +# ($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 base_ref="$(jq -r '.baseRefName // empty' "$meta_file")" @@ -285,17 +290,25 @@ 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 when iconv is available (glibc + macOS ship it); a no-op when clean. +# 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). if command -v iconv >/dev/null 2>&1; then - # Explicit branches, not `&& mv || rm`: that idiom masks an mv failure and would then feed agy the - # original (possibly split) bytes. A successful iconv must replace the file; a failed mv is fatal - # (set -e), a failed iconv 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). if iconv -c -f UTF-8 -t UTF-8 "$prompt_file" > "$prompt_file.utf8" 2>/dev/null; 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; 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 From 8299aa37362bb45020d269c596d16016f12b4f9a Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 23 Jul 2026 20:00:20 -0400 Subject: [PATCH 5/8] docs(review): reconcile stale current-state narratives + exact mapper count Addresses the CodeRabbit and agy review findings on PR #327. `rustynes_libretro.info`: the RetroArch core-downloader description said "over 172 mapper families", but 172 is the exact family count in the honesty-gated mapper matrix (`docs/STATUS.md`), not a lower bound. "over 172" reads as ">172", overstating coverage. Corrected to the precise "172 mapper families". `to-dos/ROADMAP.md`: four current-state narratives predated the v2.0.0 "Timebase" tag and still asserted v1.10.0 "Arcade" as the latest tagged release with v2.0.0 "code-complete on `main`, tag pending". v2.0.0 shipped 2026-07-03 and the release line has since advanced to v2.2.4 "Cartridge"; those statements now contradict the Status block at the top of the file and the release chain in AGENTS.md/CHANGELOG. Reconciled: - L40-41 blockquote: "code-complete on `main`, tag pending" -> "shipped as v2.0.0 on 2026-07-03", and the forward pointer relabelled to the "historical landing snapshot" section title. - L59 bullet: "Preceding additive/platform release" / "This is the latest in an unbroken ... chain" -> "Historical anchor - the last v1.x release" / "It closed an unbroken ... chain" (past tense; v1.10.0 is no longer latest). - L88 "Current state": v1.10.0-latest / v2.0.0-tag-pending -> "RustyNES v2.2.4 'Cartridge' is the latest tagged release ... v2.0.0 shipped 2026-07-03", with the retained v2.0.0 paragraph explicitly flagged as a historical snapshot rather than a current-state claim. - L90 section header: "code-complete, tag pending" -> "historical landing snapshot (shipped 2026-07-03; this section was written as it landed)". No behavioural change; documentation-only reconciliation so agents and readers receive one consistent version policy. Co-Authored-By: Claude Opus 4.8 --- crates/rustynes-libretro/rustynes_libretro.info | 2 +- to-dos/ROADMAP.md | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/rustynes-libretro/rustynes_libretro.info b/crates/rustynes-libretro/rustynes_libretro.info index e7fb90ab..58bc8f5b 100644 --- a/crates/rustynes-libretro/rustynes_libretro.info +++ b/crates/rustynes-libretro/rustynes_libretro.info @@ -35,4 +35,4 @@ firmware0_path = "disksys.rom" firmware0_opt = "true" notes = "(!) disksys.rom (md5): ca30b50f880eb660a320674ed365ef7a|RustyNES requires this BIOS in the system directory to boot Famicom Disk System games." -description = "RustyNES is a cycle-accurate Nintendo Entertainment System emulator written entirely in safe Rust. Targeting the higan and Mesen2 accuracy tier, it relies on a strict, lockstep master-clock scheduler at PPU-dot resolution rather than per-game hacks. The core guarantees absolute determinism, making it an exceptional choice for advanced libretro features like run-ahead latency reduction, rollback netplay, and RetroAchievements. With high compatibility spanning over 172 mapper families, full Famicom Disk System support (multi-disk swapping via the Disk Control interface), and true RGB Vs. System arcade boards, RustyNES is ideal for users who seek reference-grade accuracy paired with modern stability." +description = "RustyNES is a cycle-accurate Nintendo Entertainment System emulator written entirely in safe Rust. Targeting the higan and Mesen2 accuracy tier, it relies on a strict, lockstep master-clock scheduler at PPU-dot resolution rather than per-game hacks. The core guarantees absolute determinism, making it an exceptional choice for advanced libretro features like run-ahead latency reduction, rollback netplay, and RetroAchievements. With high compatibility spanning 172 mapper families, full Famicom Disk System support (multi-disk swapping via the Disk Control interface), and true RGB Vs. System arcade boards, RustyNES is ideal for users who seek reference-grade accuracy paired with modern stability." diff --git a/to-dos/ROADMAP.md b/to-dos/ROADMAP.md index 4dfeb46d..156be199 100644 --- a/to-dos/ROADMAP.md +++ b/to-dos/ROADMAP.md @@ -37,9 +37,9 @@ v2.8.0 → v0.9.7; the synthesis itself = **v1.0.0**. > AccuracyCoin to **100.00%**) is *upstream engine history* and shipped as the > **v1.0.0 production core**. The forward **RustyNES v2.0.0 "Timebase"** > (ADR 0002/0029) was a *different* milestone — the one-clock/every-cycle-bus- -> access scheduler collapse — that is now **code-complete on `main`, tag -> pending** (see "v2.0.0 'Timebase' — code-complete, tag pending" below for -> what actually shipped, including the one known gap: the MMC3 R1/R2 +> access scheduler collapse — which **shipped as v2.0.0 on 2026-07-03** (see +> the "v2.0.0 'Timebase' — historical landing snapshot" section below for +> what shipped, including the one known gap: the MMC3 R1/R2 > IRQ-timing residual, by-design-deferred rather than closed). The engine's > own `v1.x`/`v2.x` markers in the bullets and "Phases" sections remain > historical anchors, **never** RustyNES release numbers. @@ -56,7 +56,7 @@ v2.8.0 → v0.9.7; the synthesis itself = **v1.0.0**. - **Earlier in the train:** **RustyNES v2.0.3 "Harbor" ("Keel")** (2026-07-08) — the third release of the **v2.0.x mobile-finalization train** and the one that makes the octal-latch accuracy work real at the shipped default. The **2-cycle-ALE PPU fetch model is promoted from the experimental `mc-ppu-2cycle-ale` flag to the unconditional, only PPU fetch path** (ADR 0030), so the shipped default now scores **AccuracyCoin 141/141 (100.00%, RAM-authoritative)** — both **"ALE + Read"** (`$0491`) and **"Hybrid Addresses"** (`$0492`) pass out of the box (previously an honest 139/141). This is the genuine two-dot fetch (even-dot ALE-drive + `octal_latch` load; odd-dot `(address & 0x3F00) | octal_latch` splice + read) where the latch *naturally* carries the stale byte (`copy_v_delay = 4` → NT splice `$2F19` for Hybrid; `$2007`-ALE overlap freeze → `$0FFF` for ALE+Read), replacing v2.0.2's whole-dot `+1 coarse-X` stand-in. **Both experiment flags retired** (`mc-ppu-2cycle-ale` + `mc-ppu-bus-addr-hybrid`); stand-in code deleted; `octal_trace` survives behind the new default-off `ppu-octal-trace`. Verified: **60-ROM oracle 60/60** with two documented re-blesses (SMB3, Uchuu Keibitai SDF — single-tile `$2006`-during-render shifts, more TriCNES-faithful, audio/cycle byte-identical), nestest 0-diff, mmc3 18/18, `ppu_sprites` 19/19; ~10% headless frame-cost rise (~4.15 ms/frame). **Save-state:** additive **`PPU_SNAPSHOT_VERSION` 4 → 5** tail (netplay-rollback determinism; pre-v5 `.rns` still load; forward-incompatible with ≤v2.0.2 but not an ADR-0028 epoch break). Also: the **Harbor Android foss/play monetization glue** (step 5 — AppLovin MAX + RevenueCat 8.10.0 `MonetizationGate`, gating/paywall/session/progress; no-op `foss` twin; both flavors assemble, dormant pending v2.0.9 on-device verify) + a **host-localizable mobile bridge-warning** API (`HostWarning` enum + `drain_warning_codes()`). See `docs/STATUS.md` (single source of truth) + `CHANGELOG.md` `[2.0.3]` + `to-dos/plans/v2.0.3-2cycle-ale-plan.md`. - **Earlier in the train:** **RustyNES v2.0.2 "Harbor" ("Soundings")** (2026-07-08) — the second release of the **v2.0.x mobile-finalization train** and Harbor's **headline accuracy release**: the two new upstream AccuracyCoin PPU tests v2.0.1 documented as honest gaps — **"ALE + Read"** (`$0491`) and **"Hybrid Addresses"** (`$0492`) — are now **solved flag-on** by a whole-dot port of TriCNES's **octal-latch multiplexed-bus PPU model** (ADR 0030, commit `27c103c`), behind the pre-existing default-off `mc-ppu-bus-addr-hybrid` flag. **Shipped default stays honest 139/141 (98.58%), byte-identical to v2.0.1; flag-on the same build is verified 141/141 (100.00%)** (framebuffer 100%, nestest 0-diff, mmc3 A12 + IRQ all pass, `ppu_sprites` 19/19). The campaign corrected two ADR 0030 premises — **Mesen2 does NOT pass these tests** (both bytes `0x0A`; the correct oracle is TriCNES, the AccuracyCoin author's own MIT emulator, `ref-proj/TriCNES` commit `9199870`), and **a whole-dot port suffices** (the full 2-cycle-ALE refactor was not required). Per the maintainer's **refine-then-promote** decision (ADR 0030), the flag ships **default-off** in v2.0.2 and is **promoted to default (shipped 141/141) in v2.0.3** — after the Hybrid `+1 coarse-X` approximation is reworked to a first-principles latch-carry model and gated on the 60-ROM commercial byte-identity oracle. No snapshot-format bump (`PPU_SNAPSHOT_VERSION` stays 4). **This release does not claim the shipped build is 141/141, nor that the flag is promoted.** See `docs/STATUS.md` (single source of truth) + `CHANGELOG.md` `[2.0.2]` + `to-dos/plans/v2.0.2-harbor-plan.md`. - **Earlier in the train:** **RustyNES v2.0.1 "Harbor" ("Mooring")** (2026-07-08) — the first release of the **v2.0.x mobile-finalization train** on the v2.0.0 "Timebase" core: the Android core re-port + `foss`/`play` flavor-split scaffolding (ADR 0025), the **AccuracyCoin oracle re-sync** (catalog 144→146 rows / 139→141 assigned; measured honestly at **139/141, 98.58%** — the two new upstream PPU tests "ALE + Read" / "Hybrid Addresses" documented as gaps, then solved flag-on in v2.0.2 per ADR 0030), the **CI cost optimization** (heavy suite gated to `release/*` + a weekly cron), the **dependency sweep** (uniffi 0.32 / mlua 0.12 / wgpu-naga 29.0.4 / cc 1.2.66; wgpu 30 deferred on the egui 0.35 pin), and the **`mc-r1-dmc-abort-probe` housekeeping removal**. Every core change is behaviour-neutral, so the deterministic core is byte-identical to v2.0.0: the **139 passing** AccuracyCoin tests and nestest 0-diff are unchanged — only the *denominator* grew (139→141) as the oracle re-sync added the two new upstream PPU tests. See `docs/STATUS.md` (single source of truth) + `CHANGELOG.md` `[2.0.1]` + `to-dos/plans/v2.0.1-harbor-plan.md`. -- **Preceding additive/platform release:** **RustyNES v1.10.0 "Arcade"** (2026-07-01) — the native **Libretro core** (`crates/rustynes-libretro` builds `rustynes_libretro` for RetroArch: allocation-free video, batched-audio dynamic-rate sync, WRAM/SRAM RetroAchievements maps, deterministic rollback-ready save-states) plus the egui 0.34.3 → 0.35.0 dependency-tier refresh. This is the latest in an unbroken additive/off-by-default chain running all the way back to v1.0.0: the **v1.1.0 → v1.7.1 "Forge"** desktop-feature line, the **v1.8.0 → v1.8.9 "Atlas"** Android platform train, and the **v1.9.0 → v1.9.9 "Workshop"** iOS TestFlight train (see the sub-bullets below for each). AccuracyCoin has held **100.00% (139/139)** and nestest **0-diff** through every one of these releases; mapper coverage is **172 families** (Core / Curated / BestEffort, CI honesty-gated). RustyNES ships as: a native desktop app (Linux/macOS/Windows), a WebAssembly build (browser demo), a native Android app (GitHub-sideload; Google Play deferred to v2.1.0), a native iOS/iPadOS app (TestFlight; App Store deferred to v2.1.0), and a native Libretro/RetroArch core. See `docs/STATUS.md` (single source of truth) + `CHANGELOG.md` `[1.10.0]`…`[1.0.0]`. +- **Historical anchor — the last v1.x release:** **RustyNES v1.10.0 "Arcade"** (2026-07-01) — the native **Libretro core** (`crates/rustynes-libretro` builds `rustynes_libretro` for RetroArch: allocation-free video, batched-audio dynamic-rate sync, WRAM/SRAM RetroAchievements maps, deterministic rollback-ready save-states) plus the egui 0.34.3 → 0.35.0 dependency-tier refresh. It closed an unbroken additive/off-by-default chain running all the way back to v1.0.0: the **v1.1.0 → v1.7.1 "Forge"** desktop-feature line, the **v1.8.0 → v1.8.9 "Atlas"** Android platform train, and the **v1.9.0 → v1.9.9 "Workshop"** iOS TestFlight train (see the sub-bullets below for each). AccuracyCoin has held **100.00% (139/139)** and nestest **0-diff** through every one of these releases; mapper coverage is **172 families** (Core / Curated / BestEffort, CI honesty-gated). RustyNES ships as: a native desktop app (Linux/macOS/Windows), a WebAssembly build (browser demo), a native Android app (GitHub-sideload; Google Play deferred to v2.1.0), a native iOS/iPadOS app (TestFlight; App Store deferred to v2.1.0), and a native Libretro/RetroArch core. See `docs/STATUS.md` (single source of truth) + `CHANGELOG.md` `[1.10.0]`…`[1.0.0]`. - **v2.0.0 "Timebase" — released 2026-07-03.** The forward architectural milestone this Status block used to describe as a distant, high-risk future refactor (see "The path to v2.0.0" below, now updated) has landed and shipped: the one-clock/every-cycle-bus-access scheduler promote (beta.1→beta.4, PRs #217-220), full Vs. `DualSystem` dual-console support with a real commercial-title boot (beta.5, PR #221), and the save-state/movie format break + the two capstone ADRs (rc.1, PR #222 — ADR 0028 save-state v3 + ADR 0029 the timebase architecture) are all merged to `main`. AccuracyCoin held 100% (139/139) at every gate across all five betas + rc.1. The MMC3 R1/R2 IRQ-timing residual was investigated exhaustively (21+ documented attempts total, including two dedicated 2026-07-02 campaigns) and is by-design-deferred beyond v2.0.0 with a mechanism-level explanation (ADR 0002's decision-update section) rather than closed — this is the one known gap in an otherwise complete cut. The tag + release-ceremony + binary publish are done, and the **v2.0.1 "Harbor" ("Mooring")** train now builds on it. - **RustyNES feature/platform-release history (on the v1.0.0 core; all additive / off-by-default; AccuracyCoin held 100% (139/139) throughout):** - **v1.1.0 "Scriptable"** (2026-06-15) — full NES_NTSC composite + CRT/scanline shaders + `.pal` palette filters; NES Power Pad + turbo/autofire + an input-display overlay + a per-game nametable-mirroring override DB; debugger breakpoints + a cycle trace logger + an event viewer (behind `debug-hooks`); an NSF/NSFe player + a 5-band graphic EQ; and the flagship **Lua scripting engine** (`rustynes-script`, ADR 0010). See `CHANGELOG.md` `[1.1.0]`. @@ -85,9 +85,9 @@ v2.8.0 → v0.9.7; the synthesis itself = **v1.0.0**. - **Engine-lineage phase:** Phase 8 — **engine v1.2.0 (2026-05-24).** DMC DMA scheduler refactor landed under default-off cargo feature `dmc-get-put-scheduler` introducing Mesen2's canonical get/put cycle alternation model alongside the v1.1.0 phase-agnostic scheduler via the parallel-implementation pattern (ADR 0007). AccuracyCoin DMA cluster under flag-on: **6/10 match baseline** (closing 4 → 0 deferred to v1.2.x patches or v2.0 master-clock absorption). Default build bit-identical to v1.1.0. - **Engine-lineage — earlier work:** **engine v1.1.0 (2026-05-25)** — VRC7 OPLL FM audio via clean-room pure-Rust port of `emu2413 v1.5.9` (MIT); ADR 0006 supersedes ADR 0004; *Lagrange Point* plays with audio. (engine v1.1.0 was an engine v2.0.0-release-plan milestone slotted between Phase 6 and Phase 8, **not** the ROADMAP's Phase 7 — see the numbering note below.) Phase 6 — **engine v1.0.0 (2026-05-23)**: AccuracyCoin gate CLEARED at 90.65% (126/139); T-60-001 C1 IRQ-timing residuals (3 `cpu_interrupts_v2` sub-ROMs + `mmc3_test_2/4` #3) deferred to the master-clock-precise scheduling refactor (Session-29 empirically falsified Option A global PPU-position shift; 17 documented rollbacks). [That engine-lineage master-clock work subsequently landed in the RustyNES v1.0.0 core, taking AccuracyCoin to 100%.] - **Phase-numbering note:** the shipped releases v1.1.0 → v1.4.0 were sequenced from the v2.0.0 release plan and back-labelled in the detailed sections as v1.1.0 (VRC7) → Phase 8 (v1.2.0 DMC) → Phase 9 (v1.3.0 wasm) → Phase 10 (v1.4.0 TAS). **Phase 7 — Nesdev Accuracy Hardening (below) was authored but never executed**; it is now being executed as **v1.5.0**. See `docs/audit/phase-7-assessment-2026-05-24.md` for the full intent-vs-accomplished-vs-completable disposition. -- **Current state:** **RustyNES v1.10.0 "Arcade" — the latest tagged release; v2.0.0 "Timebase" is code-complete on `main` with the tag pending.** Every accuracy, compatibility, platform, netplay, RetroAchievements, FDS, Vs/PC10, and performance milestone in the engine-lineage history above is folded into the v1.0.0 core; the v1.1.0 → v1.7.x feature releases then layered (in order) the Lua scripting engine + visual filters/peripherals/devtools/NSF, the library/compatibility/reach pass, the toolchain modernization + Memory-Compare + Vs.-DualSystem detection, the accuracy-and-finish pass, the insight/scriptability/creator-tooling/polish pass, the studio/TAS-tooling/debugger-depth pass, and the writable/programmable-tooling "Forge" pass; the v1.8.x train ported the whole core to Android, the v1.9.x train ported it to iOS/iPadOS, and v1.10.0 added the native Libretro core. Mapper coverage rose **51 → 172 families** across these releases, all additive / off-by-default, with AccuracyCoin holding **100% (139/139)** the entire time. v2.0.0 then landed the one-clock/every-cycle timebase promote + full Vs. `DualSystem` support + the save-state/movie format break — the first genuinely BREAKING release since v1.0.0, by design (ADR 0028/0029). The engine-lineage version markers (v0.9.x → v2.x) in the bullets above and the phase bodies are upstream history, not RustyNES releases. +- **Current state:** **RustyNES v2.2.4 "Cartridge" is the latest tagged release** (see the Status section at the top for the current-release detail and the v2.1.0 → v2.2.4 line). **v2.0.0 "Timebase" shipped 2026-07-03** — the paragraph below describing it as "code-complete, tag pending" is retained as a historical snapshot of that release's landing, not a current-state claim. Every accuracy, compatibility, platform, netplay, RetroAchievements, FDS, Vs/PC10, and performance milestone in the engine-lineage history above is folded into the v1.0.0 core; the v1.1.0 → v1.7.x feature releases then layered (in order) the Lua scripting engine + visual filters/peripherals/devtools/NSF, the library/compatibility/reach pass, the toolchain modernization + Memory-Compare + Vs.-DualSystem detection, the accuracy-and-finish pass, the insight/scriptability/creator-tooling/polish pass, the studio/TAS-tooling/debugger-depth pass, and the writable/programmable-tooling "Forge" pass; the v1.8.x train ported the whole core to Android, the v1.9.x train ported it to iOS/iPadOS, and v1.10.0 added the native Libretro core. Mapper coverage rose **51 → 172 families** across these releases, all additive / off-by-default, with AccuracyCoin holding **100% (139/139)** the entire time. v2.0.0 then landed the one-clock/every-cycle timebase promote + full Vs. `DualSystem` support + the save-state/movie format break — the first genuinely BREAKING release since v1.0.0, by design (ADR 0028/0029). The engine-lineage version markers (v0.9.x → v2.x) in the bullets above and the phase bodies are upstream history, not RustyNES releases. -**v2.0.0 "Timebase" — code-complete, tag pending. What actually shipped (2026-07-01 → 2026-07-03):** +**v2.0.0 "Timebase" — historical landing snapshot (shipped 2026-07-03; this section was written as it landed). What shipped (2026-07-01 → 2026-07-03):** The forward architectural milestone this section used to describe as a distant, XL/HIGH-risk future refactor has landed, across beta.1 → beta.5 → rc.1 (PRs #217-222). What was originally scoped as workstreams A-F in `to-dos/plans/v2.0.0-master-clock-plan.md`: From 5e6b07216d5459c8d3afe41eacfdc6289cea1a11 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 23 Jul 2026 20:06:29 -0400 Subject: [PATCH 6/8] fix(agy): hand large diffs off via gitignored .agy-review-work/, not .git/ Adopts the agy first-pass suggestion on this PR (scripts/agy-review.sh:888 in the diff): the on-disk large-diff handoff was written inside the hidden `.git/` directory (`.git/agy-review-diff.$$.patch`), which risks a file-read failure if the reviewer subagent or sandbox restricts tool access to `.git`. The `.git`-is-a- worktree-gitfile fallback wrote `.agy-review-diff.$$.patch` at the repo ROOT, where an untracked `.patch` pollutes `git status` if agy inspects working-tree state -- defeating the reason `.git/` was chosen in the first place. Write the diff into the dedicated, gitignored working-tree scratch dir instead, `.agy-review-work/agy-review-diff.$$.patch` -- which this repo's `.gitignore` has documented as the intended handoff location all along (lines 285-289), a comment the script never actually honoured until now. This: - removes the `.git/` tool-access dependency agy flagged; - collapses the two-branch `.git`-vs-root fork into one gitignored path, so the transient .patch is never working-tree pollution regardless of whether `.git` is a real dir or a worktree gitfile; - keeps per-run `$$` isolation, and the EXIT-trap cleanup now `rmdir`s the dir only when empty (concurrency-safe: a concurrent run's different-PID file is never clobbered; a SIGKILL leftover lands in a gitignored dir, not at root -- addressing the reviewer's SIGKILL nitpick). `.gitignore` already carries `/.agy-review-work/`, so no ignore change is needed here. This mirrors the canonical template (Local_Only-Projects/antigravity-pr-review) byte-for-byte in the diff-handoff block so the standardized reviewer does not diverge across projects. No change to diff content, prompt assembly, or the file-vs-inline decision. Co-Authored-By: Claude Opus 4.8 --- scripts/agy-review.sh | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 4d252136..eda79e32 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -91,11 +91,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= agy_diff_file= +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= cleanup() { 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 @@ -252,15 +256,15 @@ case "$AGY_DIFF_MODE" in esac if [ "$use_file" = "1" ]; then - # agy's CWD is the repo checkout, so a file written under it is readable by its file tool. Prefer - # `.git/` -- git never lists it in `git status`, so the transient diff can't show up as working-tree - # pollution if agy inspects repo state -- and fall back to the repo root when `.git` is not a real - # directory (a worktree/submodule gitfile). Either way the path is CWD-relative for the prompt. - if [ -d "$PWD/.git" ]; then - diff_name=".git/agy-review-diff.$$.patch" - else - diff_name=".agy-review-diff.$$.patch" - fi + # 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 path + # is CWD-relative for the prompt. + 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" { From 6e946b88743263efa7152c0417a88a1bdff05729 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 23 Jul 2026 20:34:31 -0400 Subject: [PATCH 7/8] =?UTF-8?q?fix(agy):=20make=20the=20large-diff=20hando?= =?UTF-8?q?ff=20readable=20=E2=80=94=20absolute=20path=20+=20--add-dir?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The on-disk large-diff handoff (the `.agy-review-work/` file mode this PR introduced) never actually worked: for a diff over the inline budget the reviewer writes it to disk and tells agy to read it, but agy came back "the file does not exist on the filesystem" and produced an empty review — which is exactly what happened when this PR's own diff crossed the ~90 KB inline budget (below it the diff is inlined and the file path is never exercised, which is why every earlier head of #327 reviewed fine). Root cause, proven on the live `agy` runner (this host is the self-hosted runner) with a three-way probe under the exact review flags (`--sandbox --dangerously-skip-permissions`): 1) relative path `.agy-review-work/...`, no --add-dir -> FAIL ("does not exist") 2) same relative path + `--add-dir "$PWD"` -> PASS 3) absolute path, no --add-dir -> PASS agy's sandboxed file tool does NOT resolve a relative path against the shell's CWD — it resolves against agy's own workspace root, and under --sandbox the checkout is not in that workspace — so a CWD-relative handoff path is never found. Fix, both halves (each independently sufficient per the probe; a confirming run of the exact combined production-shaped invocation, an absolute path + `--add-dir "$PWD" --sandbox --dangerously-skip-permissions`, also passed reading a realistic ~400-line diff-shaped file with a token buried mid-file): - The prompt now hands agy the diff by ABSOLUTE path (`$agy_diff_file`) instead of the CWD-relative `$diff_name`. - `--add-dir "$PWD"` is appended to the flags, but ONLY in file-handoff mode (`use_file=1`). An inline review reads nothing from disk and keeps zero filesystem access, so the common path's prompt-injection surface is unchanged; only the large-diff path (which must read a file) gains workspace access to the checkout. Mirrors the canonical template (Local_Only-Projects/antigravity-pr-review), byte-identical in the handoff + flags regions, so the standardized reviewer stays in sync across all three repos. NOTE: the reviewer that runs on a PR is always the DEFAULT-BRANCH version (the workflow checks out `main`, never the PR head), so this fix does not change the review of #327 itself — it takes effect for every PR reviewed once #327 merges to main. Co-Authored-By: Claude Opus 4.8 --- scripts/agy-review.sh | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index eda79e32..4c599c46 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -260,8 +260,14 @@ if [ "$use_file" = "1" ]; then # 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 path - # is CWD-relative for the prompt. + # 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" @@ -269,11 +275,11 @@ if [ "$use_file" = "1" ]; then 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 `%s` in your current working directory\n' "$diff_name" + 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 ${diff_name}" + 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. @@ -332,6 +338,12 @@ 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 From c60551eaeb8f5baab80746c0fa5ce3d9b4bc96a8 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 23 Jul 2026 20:36:24 -0400 Subject: [PATCH 8/8] docs(changelog): record the agy large-diff handoff fix under v2.2.4 Tooling Documents the reviewer handoff fix landed on this PR (absolute-path + --add-dir, proven on the live runner) in the CHANGELOG's [2.2.4] Tooling section, keeping the single source of truth for user-visible change in sync with the code (docs-as-spec). No behavioural change. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5e9d407..3c25205e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,20 @@ byte-identical to v2.2.3 by construction. conversation-store fallback (a shared-runner data-leak vector), stripping `GH_TOKEN` / `GITHUB_TOKEN` from `agy`'s environment (`env -u`), and the `issue_comment` author-association re-check restored in the script. +- **Large-diff handoff made readable (found by the reviewer, fixed on the + runner).** The reviewer files a diff too large to inline into a gitignored + working-tree scratch dir (`.agy-review-work/`, relocated from `.git/` after the + reviewer flagged the hidden-dir read risk) and tells `agy` to read it — but + that on-disk handoff never actually worked: `agy`'s sandboxed file tool + resolves relative paths against its own workspace root, not the shell CWD, so + the file came back "does not exist" and the review was empty. Latent until a + diff first crossed the ~90 KB inline budget (below it the diff is inlined and + the file path is never exercised). Root cause and fix proven on the live `agy` + runner with a three-way probe under the exact review flags: the prompt now + hands `agy` an **absolute** path, and `--add-dir "$PWD"` adds the checkout to + `agy`'s sandbox workspace — but only in file-handoff mode, so an inline review + keeps zero filesystem access. All changes ride the canonical template so the + three consuming repos stay in sync. ## [2.2.3] - 2026-07-23 - "Datum" (fast dot path promoted + PGO shipped + the last two mapper residuals closed)