feat(v2.4.0): "Concordance" — atomic writes, the timeline counter, and the owed upstream sync

.github/workflows/antigravity-review.yml

@@ -67,7 +67,13 @@ jobs:
       # branch throughout. Consequence worth knowing: a PR that edits the reviewer or the
       # style guide is reviewed by the version already on the default branch until it merges.
       - name: Check out repo (for the style guide + scripts)
-        uses: actions/checkout@v7
+        # SHA-pinned, not `@v7`: this job runs on a SELF-HOSTED runner -- the
+        # maintainer's own machine -- so a compromised tag would execute there
+        # rather than in a disposable VM. Verified to be exactly what `v7`
+        # resolves to (v7.0.1, 2026-07-17). The trailing `# v7` is the form
+        # Dependabot's github-actions ecosystem reads to keep the pin current.
+        # Adopted FROM RustySNES and SLAC, which had it while the template did not.
+        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
         with:
           ref: ${{ github.event.repository.default_branch }}
           # Not `fetch-depth: 0`: the large-diff fallback fetches exactly the two refs it
@@ -95,5 +101,21 @@ jobs:
           # MAX_PROMPT_BYTES: "125000"     # inline/file threshold + hard backstop on the argv prompt
           # STYLE_GUIDE: .github/agy-review.md   # style guide, loaded if present
         run: |
+          # The workflow and the scripts come from DIFFERENT REFS: for a `pull_request`
+          # event GitHub runs this YAML from the PR branch, while the checkout step
+          # above deliberately fetches the DEFAULT branch to get the scripts. So a
+          # change that adds a script file breaks its own PR -- the new workflow
+          # chmods a file the default branch does not have yet. Observed exactly
+          # once, on the PR that introduced `_agy_comment_body.sh`.
+          #
+          # The two required files are chmod'd unconditionally; anything added later
+          # is chmod'd only if present, so the workflow stays compatible with both
+          # the old and the new script set. A genuinely missing required file still
+          # fails loudly -- `agy-review.sh` sources it and dies -- rather than being
+          # papered over here.
           chmod +x scripts/agy-review.sh scripts/_agy_print.sh
+          for opt in scripts/_agy_comment_body.sh; do
+            [ -f "$opt" ] && chmod +x "$opt"
+          done
+          true
           scripts/agy-review.sh

AGENTS.md

@@ -226,7 +226,7 @@ These cross-cutting decisions span multiple files. Reading individual chip docs
 - **The bot-comment ceremony must read the review BODIES, not just the resolvable threads.** CodeRabbit posts "Outside diff range" and other suppressed findings **inside the review body**, where they are invisible to a resolve-every-thread sweep — and Copilot does the same. This has now cost the project three times: issue #360 (an untested attestation path) reached `main` unaddressed; two findings of the same class on #357 were genuine defects, **one critical** (two threads producing frames during fast-forward under threaded display-sync, fixed in #358); and a **use-after-free** in the v2.3.5 libretro controller tables was caught only because the review body was read. A green "all threads resolved" is not evidence the review was addressed. Fetch the bodies explicitly — `gh pr view <n> --json reviews --jq '.reviews[].body'` — and triage every finding in them before merging.
 - **lz4_flex 0.14+ requires the crate's own `alloc` feature explicitly** for `compress_prepend_size`/`decompress_size_prepended` (used by `rewind.rs`/`zwinder.rs`) — it split real no_std support into an `alloc`-vs-`std` distinction that didn't exist in 0.13. A `cargo build --workspace` will NOT catch a missing `alloc` feature here because `rustynes-core`'s own default-on `std` feature implies it via cargo's feature unification; only a standalone `cargo build -p rustynes-core --target thumbv7em-none-eabihf --no-default-features` (the exact CI `no_std build` job) will. Run that command locally before pushing any bump that touches this dependency.
 
-- **The libretro `.info` RetroArch reads is a DIFFERENT FILE from this repo's, and it went stale for eleven days.** RetroArch downloads `dist/info/rustynes_libretro.info` from `libretro/libretro-super`; `crates/rustynes-libretro/rustynes_libretro.info` is an unrelated copy that nothing syncs and nothing compared. So the v2.2.9 GPL relicense reached `Cargo.toml`, `NOTICE`, `deny.toml`, the SPDX headers and the local `.info` — and **not** the file users actually see, which went on advertising "MIT OR Apache-2.0" at `display_version = v2.2.1`. Both upstream PRs had merged *exactly two weeks before* the relicense, so no sync could have carried it. **A license change is now a mandatory upstream-sync trigger**, on the same footing as a release. `crates/rustynes-test-harness/tests/libretro_info_audit.rs` pins the local file against the workspace manifest so the sync is a *copy*, never a re-derivation; it cannot see upstream, so the sync itself stays a human step. libretro `.info` uses short license tokens, not SPDX, and marks "or later" with a trailing `+` (tallied across all 316 upstream cores: `GPLv2` x100, `GPLv3` x64, `GPLv2+` x19, `GPLv3+` x5) — RustyNES is **`GPLv3+`**; a bare `GPLv3` understates it as GPL-3.0-only. Full detail + the surface table: `docs/libretro/UPSTREAM_SYNC.md`.
+- **The libretro `.info` RetroArch reads is a DIFFERENT FILE from this repo's, and it went stale for eleven days.** RetroArch downloads `dist/info/rustynes_libretro.info` from `libretro/libretro-super`; `crates/rustynes-libretro/rustynes_libretro.info` is an unrelated copy that nothing syncs and nothing compared. So the v2.2.9 GPL relicense reached `Cargo.toml`, `NOTICE`, `deny.toml`, the SPDX headers and the local `.info` — and **not** the file users actually see, which went on advertising "MIT OR Apache-2.0" at `display_version = v2.2.1`. Both upstream PRs had merged *exactly two weeks before* the relicense, so no sync could have carried it. **Upstream PRs are opened only on MINOR or MAJOR releases** — a `vX.Y.0` where `X` or `Y` changed. Patch releases do NOT trigger an upstream sync; the `.info` `display_version` is allowed to lag through a `v2.4.1`..`v2.4.9` run and is brought current at the next `vX.Y.0`. **Next scheduled upstream sync: v2.5.0** (maintainer decision, 2026-08-20). The one override is a **licence change, which syncs immediately** regardless of version — that is what this bullet's incident was about, and it stays on the same footing as a release. `crates/rustynes-test-harness/tests/libretro_info_audit.rs` pins the local file against the workspace manifest so the sync is a *copy*, never a re-derivation; it cannot see upstream, so the sync itself stays a human step. libretro `.info` uses short license tokens, not SPDX, and marks "or later" with a trailing `+` (tallied across all 316 upstream cores: `GPLv2` x100, `GPLv3` x64, `GPLv2+` x19, `GPLv3+` x5) — RustyNES is **`GPLv3+`**; a bare `GPLv3` understates it as GPL-3.0-only. Full detail + the surface table: `docs/libretro/UPSTREAM_SYNC.md`.
 - **iOS/iPadOS/tvOS availability is a THIRD repo and a HARDCODED list — being on the buildbot buys nothing there.** **RESOLVED 2026-08-16** by `libretro/RetroArch#19416` (merged `76f60626984a`; verified against `master`, not the PR state — `rustynes` sits at line 268 between `reminiscence` and `sameboy`). The mechanism below is retained because it recurs for any other core and for the sibling forges, and because "in the build list" is not "installable": it ships with the next App Store RetroArch build. iOS cannot download cores (Apple bans fetching executable code), so the App Store build bundles a fixed set chosen by `pkg/apple/update-cores.sh` in `libretro/RetroArch`. That script has two lists: `allcores`, fetched *dynamically* from the buildbot directory (RustyNES is in it automatically), and `appstore_cores`, a hardcoded array (RustyNES is **absent**). The iOS/tvOS build phases run `rm -f ${SRCROOT}/<platform>/modules/*.dylib` then `./update-cores.sh appstore`, so only the hardcoded list survives. One entry covers iOS + tvOS + macOS App Store. **Alphabetical order is mandatory** — `rustynes` sorts between `reminiscence` and `sameboy`; re-check the neighbours at submission time rather than trusting a line number.
 - **RetroArch retains SOME environment-callback pointers and copies others — the asymmetry is not documented in `libretro.h`, so check `runloop.c`.** `SET_CONTROLLER_INFO` shallow-`memcpy`s the outer `retro_controller_info` array but **retains** each entry's `types` pointer and dereferences it later when the Controls menu is built: the description arrays MUST be `'static` (a stack local compiles cleanly and hands the frontend a use-after-free). `SET_INPUT_DESCRIPTORS` is different — RetroArch walks it during the call and retains only the `description` string pointers — so a stack array is fine there. Never generalize from one to the other; read the handler.
 - **A fix that touches ONE call site of a shared code path may not fix the bug — and will report that it did.** v2.3.6 hit this squarely. Review reported that a latency measurement destroyed the user's rewind history; the fix changed `measure_in_place`'s FINAL restore to `restore_quiet` and stopped. Every *trial* still went through `Probe::run_uncounted`'s loud `nes.restore(..)`, and a measurement runs up to **21 trials** against the live emulator, so the ring was still being cleared twenty-one times over behind a fix that closed the thread. Before declaring a fix complete, grep for every caller of the mechanism, not just the one the report named. The corollary is about tests: my test for that fix would have asserted "ring not empty", which **passes while a second defect remains** — the ring in fact GREW, polluted with replayed frames that never happened on the user's timeline. Assert the state comes back EXACTLY as it was; a weaker assertion is how an incomplete fix clears review.
@@ -250,5 +250,13 @@ These cross-cutting decisions span multiple files. Reading individual chip docs
 - **Bound every workflow job, and every network fetch inside one.** PR #400 bounded `ci.yml` and nothing else; v2.3.9 found **six** more unbounded workflows including `release-auto.yml` itself, after `Clippy Security Lints` hung **two hours** in a setup step and blocked the v2.3.7 release PR. Separately, apt provisioning hung **four times across two PRs in one day**, always in a setup/provisioning step and never in a compile or test step. A job timeout bounds the damage but cannot *notice*: a stalled fetch inside a 25-minute budget is indistinguishable from a slow job, and the run reports as `cancelled`, which reads as noise. `.github/scripts/apt-install-retry.sh` adds a per-command `timeout` plus three attempts, and warns on every attempt including ones that succeed — a run needing three and one needing one are identical in the conclusion, and that difference is the early warning.
 - **Summing two percentiles is as invalid as differencing them.** `docs/performance.md` records the subtraction case (a published table whose `work p95` sat below its `work p50`). The addition case bit the v2.3.9 Latency Oracle design: an end-to-end figure needs `render_work + render_lock` (+ `render_wait`), and `PerfView` exposes those as three **separate** series, so the design was not implementable from existing data — found by trying to write it. The one valid case is adding a **constant**: internal lag is `frames * frame_ms`, so `lag + render_work.p95` genuinely is a p95. That rescues exactly one series, which is why `PerfPanelState::render_work` deliberately exposes only that one. A true wall-clock figure needs a new single per-redraw series on `RenderPerf`.
 - **`grep -i` on a short token matches more than you mean.** Reading the AccuracyCoin result with `grep -iE "RAM.*pass rate"` matched the **framebuffer** line, because `-i` makes `RAM` match "f-ram-ebuffer" — and the framebuffer decoder is the known-buggy one reporting 120. The authoritative line is `AccuracyCoin (RAM): pass rate = 100.00% over 141 assigned tests`; match it case-sensitively, e.g. `grep -E "AccuracyCoin \((RAM|framebuffer)\)"` and read both. Same session, the same class of mistake produced two false negatives from patterns that could not match (`full \*\*2x2` against `**full 2x2`). **A pattern that cannot match looks exactly like content that is not there.**
+- **`gh api repos/OWNER/REPO/issues/N` RETURNS PULL REQUESTS**, because GitHub's issues endpoint serves both. v2.4.0 item A nearly opened a duplicate upstream PR because of it: a pass ran exactly that against `libretro/docs#1180`, saw `#1180 open — Correct the RustyNES core license`, concluded "an open *issue*, so the fix still needs filing", and wrote that into a plan, a commit body and a user-facing summary. #1180 was a **pull request** filed 2026-08-16, `MERGEABLE / CLEAN`, `+1/-1`, awaiting review. When the question is *"is this change already proposed?"*, the query is `gh pr view N --repo O/R` or `gh pr list --author <you> --state all`; the issues endpoint cannot distinguish them and its `pull_request` field is easy to miss.
+- **Never write a commit body with `git commit -m` in this harness.** The shell is zsh, so **backticks are command substitution** and `<word>` is an input redirect. A v2.4.0 message documenting three `gh` invocations lost all three to substitution and emitted `no such file or directory: owner` from a literal `repos/<owner>/<repo>/...` — the commit succeeded with a mangled body reading "recorded, because it is reusable:  RETURNS THE PULL REQUEST". This project's house style puts command examples in commit bodies routinely, so `-m` is structurally the wrong tool: write the message to a file and use `git commit -F`. After amending, grep the message for each phrase that was supposed to survive.
+- **A test that reimplements its subject is testing itself, and it will agree forever.** v2.4.0 hit this in a test written *for a review finding*: it declared a local `fn strip(t) { t.trim_start_matches([' ', '*']) }` and asserted against that, so deleting the production stripping came back **NOT CAUGHT**. Only the mutation pass could see it — the test passed, read correctly, and covered nothing. The fix is the one this release needed three times over: **extract the decision into a named item both the code and the test call.** The other two were `atomic_write`'s injectable rename predicate (with it hard-wired, the exhaustion branch is unreachable on Unix and a mutation returning `Ok(())` for a save that never happened went uncaught) and `TimelineWatch` (`DebuggerOverlay::new` needs a window and a wgpu device, so nothing living only inside it is unit-testable). In all three the code **read** as testable beforehand.
+- **Never byte-slice in a panic or format path.** `&text[at..(at + 24).min(text.len())]` panics when the offset lands inside a multi-byte character, and these documents are full of em-dashes and arrows — so the audit would crash *while formatting the diagnostic*, replacing the message explaining the real failure with a char-boundary error about the reporting code. **A diagnostic that can crash the diagnosis is worse than none**, because the failure it exists to explain becomes harder to read than if the excerpt were omitted. Use `s.chars().take(n).collect::<String>()`.
+- **Verify a reviewer's claim before writing the fix, especially when their other findings were right.** On #427 a reviewer stated `starts_with("[workspace.package]")` also matches `[workspace.package.metadata]`. Plausible, a real class of bug, and the fix plus a commit body describing "the regression I introduced" were written before it was tested. It is **false**: the literal ends with `]` and the sub-table has `.` there, so the match is `false`; injecting such a sub-table and running the audit reads `2.3.9` under both forms. The same reviewer's two other findings that pass were both correct — which is exactly what makes the third easy to wave through. Adopt the change if it is better anyway (it was), but write down what is *true*, not a fix for a bug that never existed.
+- **The libretro "let-chains are unstable" review claim is FALSE and has now been raised seven times.** `if let Some(x) = e && cond` is stable in **edition 2024**, which this workspace uses on a pinned stable 1.96.0; the identical construct has been in `libretro_info_audit.rs` on `main` since v2.3.5; and CI's `fmt + clippy + rustdoc` job has compiled it green at five distinct SHAs. Do not "fix" it. Refute with the edition, the existing site, and the green lint job **on the current SHA** rather than by reference to earlier ones.
+- **The workspace cannot carry a SemVer pre-release version.** Setting `[workspace.package] version = "2.3.9-rc.1"` fails before any test runs: `failed to select a version for the requirement rustynes-apu = "^2.0.0" / candidate versions found which didn't match: 2.3.9-rc.1`. A caret requirement does not match a pre-release, so every intra-workspace dependency would have to be rewritten first. Relevant when reasoning about version parsing — `release_anchor_audit.rs` guards the case anyway, and its `version_core` is tested directly because the manifest route is unreachable.
+- **`release_anchor_audit.rs` pins 15 release anchors across 10 documents** against `[workspace.package] version` (README badge + Current Release, `docs/STATUS.md`, both `AGENTS.md` anchors + the never-claim-later guard, `VERSION-PLAN.md` header and its `(current)` table row, `to-dos/ROADMAP.md`, `SUPPORT.md`, `SECURITY.md`, root `ROADMAP.md`, `OVERVIEW.md`, `ARCHITECTURE.md`). It **fails closed** — a marker matching nothing panics rather than silently checking nothing — and it also asserts the CHANGELOG header parses the way `release-auto.yml` parses it (the `- YYYY-MM-DD -` prefix, a quoted codename, then a parenthesised theme), since a malformed header degrades the published release title. **Rewording an anchor means updating `ANCHORS` in the same change**; the test will tell you, by name.
 
 <<< MC-PROJECT-END >>>