feat(v2.4.0): "Concordance" — atomic writes, the timeline counter, and the owed upstream sync by doublegate · Pull Request #428 · doublegate/RustyNES

doublegate · 2026-08-20T13:35:11Z

Delivers v2.4.0 "Concordance" items A–D. Every one traces to a recorded deferral; nothing here is invented for the release.

Item C — one atomic, durable file write (marquee)

The seven-property sequence v2.3.9 built for Config::save_to is extracted into crate::atomic_write and adopted everywhere. The plan named three call sites; there were four.

save_state.rs matters most and the plan named it last: a truncated save state is a user's game progress, a worse loss than a truncated config, and it was still using the bare fs::write the config path had already been fixed for.

per_game.rs was not in the plan at all, because it looks correct — sibling temp file, rename — so a sweep for fs::write-onto-a-target clears it. It held two of seven properties. No fsync, so the rename could commit a directory entry pointing at bytes that never reached the medium; and a fixed scratch name shared across every process and concurrent call, which is the exact failure the mechanism exists to prevent, reintroduced by the mechanism itself. A partially-correct implementation is harder to spot than an absent one.

The config path gains something it never had: a bounded retry past a transient Windows sharing violation, where MoveFileEx fails if another process has the target open. POSIX has no such constraint — which is why it went unnoticed, and why it would have surfaced as a Windows user reporting a save that failed for no visible reason.

Two mutations forced design changes rather than confirming the design:

The retry loop's predicate had to become a parameter. Hard-wired, the exhaustion branch is unreachable on Unix, and a mutation making it return Ok(()) — silently reporting a save that never happened — went uncaught.
The mode test asserted less than its name claimed; opts.mode(0o600) at creation already yields 0600 under any ordinary umask, so only a mode the umask masks distinguishes the two mechanisms.

Two properties are not observable in-process and the module says so rather than letting a green suite imply coverage: fsync (needs a power loss) and creation-mode (a race-window narrowing; a test can only see the end state).

Item B — the timeline generation counter

v2.3.9 recorded that stale debug telemetry could not be cleared on a save-state load: of four ways the emulator jumps timeline, only one is reachable from a patchable frontend call site — wasm load-state restores inside a spawn_local task, and rewind happens entirely inside the core.

restore_inner's existing clear_rewind parameter already draws the needed distinction, so the counter reuses it. This departs from the plan's enumeration deliberately: the plan listed netplay rollback as a bump site and stated the mechanism forbidding it — a same-timeline restore must not bump. Netplay rollback and run-ahead both go through restore_quiet precisely because they are same-timeline; bumping there would clear telemetry sixty times a second, worse than the defect being fixed.

The counter is not serialized, and the plan asked for a snapshot_schema_audit.rs entry — but that file audits the four chips, not Nes. The property is pinned by an executable assertion instead: snapshot at generation N, advance past N, restore, assert it did not come back to N. Simulating serialization makes it fail with exactly that diagnostic.

TimelineWatch is extracted because DebuggerOverlay::new needs a window and a wgpu device, so nothing living only inside it is unit-testable — the same reasoning as item C's injectable predicate.

Item D — two false byte-identity claims, now true

graphics.hd_packs and graphics.shader_presets both documented a pre-feature config as "byte-identical". Both were, only until the first save. v2.3.9 corrected the prose and left the behaviour; this is that decision.

Both directions tested, because the over-eager direction is the dangerous one: an is_empty returning true unconditionally would silently discard saved presets on every save — data loss wearing the shape of a tidiness fix. A third test asserts the property for the field that does not exist yet, since the defect is precisely "a field was added and the save-side property was not considered".

Item A — the upstream sync, filed

libretro-super#2074 — one line, display_version v2.3.5 → v2.3.9. Everything else was already correct upstream including license = "GPLv3+". Verified before pushing that the branch file is now byte-identical to this repo's copy, which is what libretro_info_audit.rs exists to make possible.

libretro/docs#1180 needed nothing — it is a pull request open since 2026-08-16, MERGEABLE/CLEAN, unreviewed. I had called it an issue; the reusable lesson is that gh api repos/OWNER/REPO/issues/N returns pull requests, because GitHub's issues endpoint serves both.

AGENTS.md gains the cadence rule: upstream PRs only on MINOR/MAJOR releases; next is v2.5.0; a licence change overrides and syncs immediately.

Accuracy

rustynes-core changes (item B), so the contract is verified, not asserted:

AccuracyCoin (RAM): pass rate = 100.00% over 141 assigned tests
nestest: test result: ok. 1 passed

Gates

fmt --check / clippy --workspace --all-targets -D warnings ... clean
clippy wasm32: default + wasm-canvas ......................... clean
rustdoc -D warnings / no_std thumbv7em ....................... clean
rustynes-core 188 passed · rustynes-frontend 564 passed
release_anchor_audit 8 passed (AGENTS.md edits re-verified post-rebase)

Mutations, each confirmed to have actually run its named test: 6 for item B (including a simulated serialized counter), 4 for item D in both directions, 6 of 7 properties for item C with the two uncoverable ones documented.

…rsists user data v2.4.0 item C. Extracts the seven-property write sequence from `Config::save_to` into `crate::atomic_write` and adopts it at every remaining call site. WHY A MODULE AND NOT THREE COPIES v2.3.9 made the config path atomic after `fs::write` was found capable of leaving a user holding a truncated `config.toml`. It took seven properties to get right, and FIVE of them came from review rather than from the first draft. A property that five separate reviews had to find once will not be independently rediscovered three more times, which is the entire argument for one implementation. WHAT EACH PATH WAS ACTUALLY DOING config.rs::save_to .......... the full v2.3.9 sequence ....... 7 of 7 save_state.rs::save_to_slot . fs::write .................... 0 of 7 cheats.rs::save_for_rom ..... fs::write .................... 0 of 7 per_game.rs::save_overlay ... tmp + rename ................. 2 of 7 `save_state.rs` is the one that matters most and the plan named it last: A TRUNCATED SAVE STATE IS A USER'S GAME PROGRESS, a worse loss than a truncated config, and it was still using the bare call the config path had already been fixed for. It is also the path most likely to be written under load -- rewind capture, run-ahead and netplay rollback all produce save states, and a user pressing F1 during a busy frame is the ordinary case rather than an edge one. `per_game.rs` was NOT IN THE PLAN and is the instructive one. It writes a sibling temp file and renames, so it *looks* correct and a sweep for `fs::write` straight onto a target clears it. It held two of seven. The two that mattered: no `fsync`, so the rename could commit a directory entry pointing at bytes that never reached the medium; and a FIXED scratch name, `path.with_extension("json.tmp")`, shared across every process and every concurrent call -- the exact failure the mechanism exists to prevent, reintroduced by the mechanism itself. A partially-correct implementation is harder to spot than an absent one, which is an argument for the shared helper that the plan did not have when it was written. THE WINDOWS TAIL, WHICH THE CONFIG PATH NEVER HAD `std::fs::rename` maps to `MoveFileEx` with `MOVEFILE_REPLACE_EXISTING`, so replace-existing holds on both platforms -- with a caveat POSIX does not have: on Windows the rename FAILS if another process has the target open, and an antivirus scanner or a search indexer reading `config.toml` is enough. That is why the config path never needed it, and why it would have gone unnoticed until a Windows user reported a save that failed for no visible reason. A bounded retry now covers it, and when the attempts are exhausted the error PROPAGATES: a save that fails silently after N attempts is worse than one that fails on the first, because the user gets no signal at all. THE RETRY LOOP IS PORTABLE SO THAT IT CAN BE TESTED The obvious shape is a `#[cfg(windows)]` block. That is deliberately not used for the loop, because CI runs the suite on Linux and a cfg-walled retry is code no test on the primary platform can execute -- an untested mechanism guarding a failure nobody can reproduce locally. Instead the loop is portable and the PREDICATE is platform-scoped, and the loop takes both the operation and the predicate as parameters. That second parameter is not tidiness; it was forced by a mutation. The first version called `is_transient_rename_error` directly, which reads as testable and is not: that predicate is unconditionally false on Unix, so the exhaustion branch is UNREACHABLE on the platform CI runs. A mutation making exhaustion return `Ok(())` -- silently reporting a save that never happened, the worst outcome this module has -- was NOT CAUGHT by the test written for it. Injecting the predicate makes the branch reachable everywhere, and a separate test pins the Unix single-attempt guarantee with the real predicate. MUTATION RESULTS, INCLUDING THE TWO THAT ARE NOT COVERED Seven properties deleted in turn. Five caught: symlink resolution removed ............... CAUGHT broken-symlink fallback removed .......... CAUGHT exact mode after creation removed ........ CAUGHT (needed a new test, below) occupied-scratch retry removed ........... CAUGHT exhaustion reports success ............... CAUGHT (needed the predicate param) unix predicate forced true ............... CAUGHT Two are NOT observable from inside the process, and the module says so rather than leaving a green suite to imply coverage it does not have: * `fsync` before the rename. Deleting it changes nothing an in-process assertion can see -- the page cache serves the read back identically. Only a power loss or a fault injector distinguishes them. * Mode applied AT CREATION. Deleting `opts.mode(...)` still ends at the right mode, because the explicit `set_permissions` after it corrects the result. Creation-mode is a RACE-WINDOW NARROWING, not an end-state property: it removes an interval in which the file sits at the umask default, and a test can only observe the end state. Neither should be removed on the evidence that no test fails. An untested property is not an unnecessary one. The mode test itself was rewritten because the mutation pass caught it asserting less than its name claimed: `opts.mode(0o600)` at creation already yields 0600 under any ordinary umask, so deleting the exact set afterwards left it green. `open(2)` applies `mode & ~umask`, so only a mode carrying bits the umask clears (0666 under the usual 022 gives 0644) distinguishes the two mechanisms. The new test OBSERVES the umask rather than assuming 022, and returns early when the umask masks nothing, because the two are then genuinely indistinguishable. THE WASM32 GATE CAUGHT WHAT NATIVE CLIPPY DID NOT `sync_parent_dir` compiles to an empty body off Unix, which trips clippy's `missing_const_for_fn` -- visible only on a non-Unix target, so native clippy passed while the wasm32 gate failed. Split into two cfg'd definitions with the non-Unix one `const`, which also states the truth: on Windows `MoveFileEx` already orders the metadata write, and on wasm there is no directory to sync. Recorded alongside it: the retry `sleep` is unreachable off Windows, which matters on wasm specifically because `std::thread::sleep` cannot block on `wasm32-unknown-unknown`. NET `config.rs` loses 273 lines, of which the great majority is the rationale that now lives once in the module rather than being duplicated at four call sites. Behaviour on Unix is unchanged; the config path GAINS the Windows retry it never had. GATES cargo fmt --all --check ......................... clean clippy: default / scripting / scripting,hd-pack / retroachievements / full ................ clean clippy wasm32: default / wasm-canvas ............ clean RUSTDOCFLAGS=-D warnings cargo doc .............. clean rustynes-frontend lib tests ..................... 557 passed atomic_write module tests ....................... 12 passed Frontend-only: no emulation source changes, so the AccuracyCoin 141/141 and nestest 0-diff results verified for v2.3.9 are unaffected.

…ntouched config v2.4.0 item D. `graphics.hd_packs` (v1.5.0) and `graphics.shader_presets` (v1.2.0) both documented a pre-feature config as "byte-identical". Both were byte-identical only until the first save. `#[serde(default)]` is a LOAD guarantee. It says nothing about what SAVE writes, and the TOML serializer emits an empty table for an empty collection -- so a user who had never opened the HD-pack manager or saved a shader preset found their config rewritten with a bare `[graphics.hd_packs]` and `[graphics.shader_presets]` on the first save after upgrading. Not data loss, but a claim the file itself contradicted, and one that made a genuine diff harder to read. v2.3.9 corrected the PROSE and deliberately left the behaviour, on the reasoning that changing what two shipped features write is a separate decision from fixing a false claim. This is that decision, and the plan put it here for that reason. `hd_packs` is a bare `BTreeMap`, so `skip_serializing_if` names `BTreeMap::is_empty` directly, matching `input.latency_reports` which got this treatment in v2.3.9. `shader_presets` is a `ShaderPresetBank` struct wrapping a map, so it needed an `is_empty` on the type before the attribute had anything to name -- which is a fair part of why it was the one left behind when `latency_reports` was fixed. BOTH DIRECTIONS, BECAUSE ONE DIRECTION PROVES NOTHING The plan is explicit that a one-directional test passes just as happily against a field that never persists anything at all. So each field gets: the empty case is OMITTED, a populated one SURVIVES, and both round-trip back through `from_str` -- because the string checks verify the KEY and say nothing about the VALUE. Mutation-tested in both directions, each confirmed to have actually run its named test rather than matching zero and exiting 0: removed hd_packs skip ................... CAUGHT removed hd_packs skip (property test) ... CAUGHT removed shader_presets skip ............. CAUGHT over-eager is_empty (always true) ....... CAUGHT That last one is the direction that matters most: an `is_empty` returning `true` unconditionally would silently DISCARD a user's saved presets on every save, which is a data-loss bug wearing the shape of a tidiness fix. It is caught. A THIRD TEST, FOR THE FIELD THAT DOES NOT EXIST YET `a_default_config_writes_no_empty_opt_in_tables` asserts the property once rather than per field: a default config must carry no empty table for any of the three opt-in collections. The per-field tests would each still pass if a FOURTH such field were added tomorrow without the attribute; this is the one that would start failing. The defect being fixed here is precisely "a field was added and the save-side property was not considered", so the regression net should be shaped around the field that has not been written yet. GATES cargo fmt --all --check ......................... clean clippy: default / full .......................... clean clippy wasm32: default / wasm-canvas ............ clean RUSTDOCFLAGS=-D warnings cargo doc .............. clean rustynes-frontend lib tests ..................... 560 passed Frontend-only. Config files written by an older build still load unchanged -- this only removes keys that carried no information.

…s that read it v2.4.0 item B. `Nes` gains a session-local `timeline_generation` that changes whenever the emulator jumps to a different point on its timeline, and the debug telemetry that describes a run now clears itself when it does. WHY A COUNTER RATHER THAN MORE CALL SITES v2.3.9 item E cleared the call stack and access counters on a ROM change and recorded, honestly, that the same telemetry is NOT cleared on a save-state load. A two-call-site patch was declined as insufficient, with the reason measured: native load-state ....... reachable from a frontend call site wasm load-state ......... NOT -- restores inside a `spawn_local` task holding only a cloned `EmuHandle` rewind .................. NOT -- happens entirely inside the core netplay rollback ........ NOT One of four, and patching it would have presented a quarter of the fix as the whole of it. The plan originally proposed each consumer remembering the last `Nes::cycle()` it saw and noticing a non-monotonic step. Review on #415 proposed better, and the difference is a case the heuristic provably cannot cover: a restore to a LATER state advances `cycle()`, so it is indistinguishable from execution. A core-side counter sees it, and needs no cooperation from any call site. LOUD VERSUS QUIET, WHICH THE CODEBASE ALREADY DISTINGUISHED `restore_inner` already takes `clear_rewind`, which is exactly the distinction: `true` for a user-driven load that invalidates rewind history, `false` for a same-timeline machine-driven restore (run-ahead's per-frame rollback, netplay's rollback-resimulate) where the history stays valid. The counter reuses it rather than inventing a parallel notion. This DEPARTS FROM THE PLAN'S ENUMERATION, deliberately. The plan listed netplay rollback as a bump site, and also stated the mechanism -- "a same-timeline restore is exactly one that must NOT bump the counter". The two cannot both hold; netplay rollback goes through `restore_quiet` precisely because it is same-timeline. The mechanism wins: bumping there would clear a user's telemetry sixty times a second under run-ahead, which is a worse defect than the stale telemetry this fixes. Both directions are pinned by tests. `reset` and `power_cycle` bump too. They are discontinuities by any reading, and a reconstructed call stack describes a run that no longer exists after either. The bump happens BEFORE the restore can fail. A partially-applied restore is a discontinuity whether or not it completed, and a consumer that keeps stale telemetry because the jump errored is the bug in its most confusing form. THE COUNTER MUST NOT BE SERIALIZED Its only job is to be DIFFERENT after a discontinuity. Serializing it would put an OLD value back on restore, so loading a state saved earlier in the same session could hand a consumer a generation it has already seen -- and the consumer would conclude nothing jumped at the exact moment something did. The plan asked for an entry in `snapshot_schema_audit.rs`. That file audits `Ppu`, `Cpu`, `Apu` and `Opll`; `Nes` is not among them, and retrofitting it means classifying every field of `Nes`, which is a larger change than this item. So the property is pinned by an EXECUTABLE assertion instead, which is stronger than a list entry would have been: snapshot at generation N, advance past N, restore, and assert the generation did not come back to N. Simulating serialization makes it fail with exactly the diagnostic it should: the generation went BACKWARDS to 1 (a consumer had already seen 3), so the counter is being carried in the save state -- which defeats its only purpose THE CONSUMER SIDE, LANDED WITH IT Checked once per frame in `DebuggerOverlay::pump_watchpoints`, which already runs under the emu lock with `&mut Nes` -- rather than at each site that could cause a jump, since two of the four are not reachable from one. The decision is extracted into a `TimelineWatch` value rather than an `Option<u64>` field, for a reason that has now come up twice in this release: `DebuggerOverlay::new` needs a window and a wgpu device, so anything living only inside it cannot be unit-tested. The same argument produced the injectable predicate in `atomic_write`. The FIRST observation adopts rather than reporting a jump, because a fresh `Nes` starts its counter at zero and "never observed" must stay distinguishable from "observed a zero" -- otherwise loading a ROM and immediately loading a save state compares 0 against 0 and misses it. `clear_rom_bound_analysis` calls `forget()` for the same reason: a generation from the previous cartridge is not comparable with the new core's. Only telemetry RECONSTRUCTED FROM A RUN is cleared. Watch lists and breakpoints are user-authored and survive, under the rule v2.3.9 settled for ROM transitions. A timeline jump is a weaker event than a cartridge change, so it can only ever clear a subset of what that hook does -- never more. ACCURACY -- VERIFIED, NOT ASSERTED `rustynes-core` changes, so the contract was re-run rather than reasoned about: AccuracyCoin (RAM): pass rate = 100.00% over 141 assigned tests nestest: test result: ok. 1 passed (The framebuffer decoder also reports 100.00% over 120 cells; the RAM decoder is the authoritative one.) MUTATIONS -- six, each confirmed to have actually RUN its named test loud restore stops bumping .............. CAUGHT quiet restore ALSO bumps ................ CAUGHT reset stops bumping ..................... CAUGHT first observation reports a jump ........ CAUGHT forget() does nothing ................... CAUGHT counter behaves as if serialized ........ CAUGHT GATES cargo fmt --all --check ......................... clean cargo clippy --workspace --all-targets -D warn .. clean clippy wasm32: default / wasm-canvas ............ clean RUSTDOCFLAGS=-D warnings cargo doc --workspace .. clean no_std thumbv7em-none-eabihf .................... clean rustynes-core lib ............................... 188 passed rustynes-frontend lib ........................... 564 passed

…cription v2.4.0 item A, local half. Both upstream surfaces were fetched read-only and compared against this tree, and the exact change is written down so the human step is a COPY rather than a re-derivation -- which is the same reasoning behind `libretro_info_audit.rs`. The v2.3.5 incident happened precisely because a re-derivation was asked of a human and not performed. The result is smaller than the plan assumed, in one direction and larger in the other. libretro-super's `dist/info/rustynes_libretro.info` needs ONE LINE: `display_version` v2.3.5 -> v2.3.9. Everything else is already in sync, including `license = "GPLv3+"` (landed upstream 2026-08-16 via libretro-super#2069) and the description's 174-mapper-family figure. Verified by diffing the fetched upstream file against this repo's copy: two changed lines, which is that field and its counterpart. So the specific failure v2.3.5 found -- `.info` advertising MIT/Apache-2.0 eleven days after the relicense -- is closed, and what remains is ordinary four-release version drift. libretro/docs' `docs/library/rustynes.md` is the one still wrong, and it is the LICENCE again: The RustyNES core is licensed under - MIT OR Apache-2.0 RustyNES has been GPL-3.0-or-later since v2.2.9 (ADR 0036), as a derivative work of GPL emulators. `libretro/docs#1180` is open against exactly this, was filed at the time, and has not been actioned upstream. That page is what a user reads BEFORE the `.info`, so of the two surfaces the stale one is the more visible. Neither PR is opened here. Both are outward-facing changes to third-party repositories this project does not own, so they are prepared and left for a maintainer. It is also why the local audit deliberately cannot see upstream: a test that could would be a test that silently disagreed with a repository nobody here controls. Documentation only; markdownlint passes.

…hat misled me v2.4.0 item A, closing the local half. Both surfaces are now proposed upstream. libretro-super#2074 is opened: one line, display_version v2.3.5 -> v2.3.9. Verified before pushing rather than after -- the branch's dist/info/rustynes_libretro.info is now BYTE-IDENTICAL to this repository's copy. That is precisely the property libretro_info_audit.rs exists to make possible: the sync is a copy, not a re-derivation performed by hand. libretro/docs#1180 needed nothing. It has been open since 2026-08-16, and it is a PULL REQUEST rather than an issue -- re-verified today as OPEN, MERGEABLE/CLEAN, +1/-1, with zero comments. Correct, still applicable, simply unreviewed. A second PR would have been a duplicate. The misreading that nearly produced that duplicate is recorded, because it is reusable: "gh api repos/OWNER/REPO/issues/1180" RETURNS THE PULL REQUEST, since GitHub's issues endpoint serves PRs too. An earlier pass here ran exactly that, saw "#1180 open -- Correct the RustyNES core license", and concluded the docs fix still needed filing. "gh pr view" is the query when the question is whether a change is already proposed; the issues endpoint cannot answer it. Also cross-referenced #2074 on #1180, so a reviewer picking up either one can see that the docs page is now the last surface still showing the pre-relicense terms. Documentation only; markdownlint passes. Note on this commit: its first version was written with `git commit -m` and lost three backtick-quoted commands to shell substitution -- zsh evaluated them, and `<owner>` was read as an input redirect. Amended from a file. Commit bodies in this project carry command examples routinely, so -m is the wrong tool for them.

….4.0 work THE CADENCE RULE (maintainer decision, 2026-08-20) 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 sync: **v2.5.0**. Attached to the existing bullet rather than added beside it, because that bullet already carries the one OVERRIDE and the two must be read together: a **licence change syncs immediately**, regardless of version. That is what the v2.3.5 incident was about -- RetroArch advertised MIT/Apache-2.0 for eleven days after the GPL relicense -- and it stays on the same footing as a release. EIGHT OPERATING NOTES, ALL FROM THINGS THAT ACTUALLY HAPPENED * `gh api repos/OWNER/REPO/issues/N` RETURNS PULL REQUESTS. This nearly opened a duplicate upstream PR: a pass ran exactly that against libretro/docs#1180, saw an "open issue", and concluded the docs licence fix still needed filing -- into a plan, a commit body and a user-facing summary. #1180 is a PR, open since 2026-08-16, MERGEABLE/CLEAN. Use `gh pr view` for "is this already proposed". * Never write a commit body with `git commit -m` here. zsh treats backticks as command substitution and `<word>` as an input redirect; a message documenting three `gh` invocations lost all three and emitted `no such file or directory: owner`. This project's house style puts command examples in commit bodies routinely, so `-m` is structurally wrong for them -- use `-F` and then grep the result for each phrase that was supposed to survive. * A test that reimplements its subject is testing itself. Found in a test written FOR a review finding: it declared a local `strip` helper and asserted against that, so deleting the production code came back NOT CAUGHT. Only the mutation pass could see it. The fix -- extract the decision into a named item both sides call -- was needed THREE times this release (the atomic-write predicate, `TimelineWatch`, and this), and in all three the code READ as testable beforehand. * Never byte-slice in a panic or format path. `&text[at..at+24]` panics inside a multi-byte character, and these docs are full of em-dashes -- so the audit crashed while formatting its own diagnostic. A diagnostic that can crash the diagnosis is worse than none. * Verify a reviewer's claim before writing the fix, especially when their other findings were right. A claim that `starts_with("[workspace.package]")` matches sub-tables is false (the literal ends with `]`), and the fix plus a commit body describing "the regression I introduced" were written before it was tested. * The let-chains claim is false and has been raised SEVEN times. Stable in edition 2024; identical construct on `main` since v2.3.5; compiled green at five SHAs. Refute on the CURRENT SHA rather than by reference to earlier ones. * The workspace cannot carry a SemVer pre-release version -- cargo rejects it before any test runs, because a caret requirement does not match a pre-release. * `release_anchor_audit.rs` pins 15 anchors across 10 documents and fails closed; rewording an anchor means updating `ANCHORS` in the same change. Documentation only. markdownlint passes (one MD038 fixed: a code span may not begin with a space).

Folds the four v2.4.0 items into the existing [Unreleased] sections rather than adding parallel ones -- #427 had already opened Added/Fixed/Changed there, and markdownlint's MD024 caught the duplicate headings before they landed. Covers: the shared atomic-write helper and the fourth call site the plan did not name; the timeline generation counter and why it deliberately does not bump on a same-timeline restore; the two skip_serializing_if fields and why both directions are tested; and the upstream sync, which turned out to be one line.

coderabbitai · 2026-08-20T13:35:19Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c8026b29-e21e-4afa-8f30-8aca1dbe1fb0

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Pull request overview

This PR delivers the v2.4.0 “Concordance” release items by (1) introducing a shared atomic/durable write helper and migrating user-data persistence paths to it, (2) adding a session-local “timeline generation” counter in rustynes-core plus frontend tracking to clear derived debug telemetry on timeline discontinuities, and (3) updating release/process docs (including the owed libretro upstream sync cadence and release notes).

Changes:

Add rustynes-frontend::atomic_write and migrate persistence paths (config, save states, cheats, per-game overlays) to use atomic + durable writes.
Introduce Nes::timeline_generation and frontend TimelineWatch to detect timeline jumps and reset reconstructed telemetry.
Update v2.4.0 release documentation/automation artifacts and upstream-sync guidance.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
CHANGELOG.md	Adds/updates the v2.4.0 release entry to match the shipped changes.
crates/rustynes-core/src/nes.rs	Adds `timeline_generation` and bumps it on timeline discontinuities; includes pinning tests.
crates/rustynes-frontend/src/atomic_write.rs	Implements the shared atomic + durable write helper used by persistence paths.
crates/rustynes-frontend/src/lib.rs	Exposes the new `atomic_write` module from the frontend crate.
crates/rustynes-frontend/src/config.rs	Migrates config persistence to the shared atomic/durable write helper.
crates/rustynes-frontend/src/save_state.rs	Migrates save-state persistence to the shared atomic/durable write helper.
crates/rustynes-frontend/src/cheats.rs	Migrates cheat persistence to the shared atomic/durable write helper.
crates/rustynes-frontend/src/per_game.rs	Migrates per-game overlay persistence to the shared atomic/durable write helper.
crates/rustynes-frontend/src/shader_pass.rs	Adjusts shader preset persistence/byte-identity behavior per item D.
crates/rustynes-frontend/src/debugger/mod.rs	Adds `TimelineWatch` and clears derived telemetry when `timeline_generation` changes.
crates/rustynes-test-harness/tests/release_anchor_audit.rs	Updates release-anchor checks to account for v2.4.0 doc/version changes.
crates/rustynes-test-harness/src/bin/coverage_smoke.rs	Adjusts coverage smoke tooling/docs to align with v2.4.0 changes.
crates/rustynes-mappers/src/m021_vrc4.rs	Minor v2.4.0-related update (likely documentation/notes alignment).
crates/rustynes-mappers/src/m022_vrc2.rs	Minor v2.4.0-related update (likely documentation/notes alignment).
docs/STATUS.md	Updates project status/version anchors for v2.4.0.
docs/release-notes/README.md	Updates release-notes guidance to reflect current release process/versioning.
docs/release-notes/v2.4.0.md	Adds the v2.4.0 release notes content.
docs/release-notes/v2.4.1.md	Adds/updates v2.4.1 notes as part of the release-notes flow.
docs/libretro/UPSTREAM_SYNC.md	Updates upstream sync guidance (cadence/requirements) referenced by item A.
.github/release-notes/v2.3.6.md	Updates older GH release notes file as part of release-notes maintenance.
scripts/release-automation/release_v240.sh	Adds/updates automation script supporting the v2.4.0 release process.
scripts/release-automation/release_v241.sh	Adds/updates automation script supporting the v2.4.1 release process.
AGENTS.md	Records updated upstream-sync cadence rule and related operational guidance.
to-dos/ROADMAP.md	Updates roadmap to reflect v2.4.0 deliverables/cadence.
to-dos/plans/v2.4.0-concordance-plan.md	Adds/updates the v2.4.0 plan document (items A–D).
to-dos/plans/v2.3.9-crucible-plan.md	Updates prior plan doc (context/cross-references for v2.4.0 work).
to-dos/plans/v2.3.7-overtone-plan.md	Updates prior plan doc (context/cross-references for v2.4.0 work).
to-dos/plans/v2.3.8-parallax-plan.md	Updates prior plan doc (context/cross-references for v2.4.0 work).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

doublegate · 2026-08-20T14:14:23Z

Response to the Antigravity review

Blocking issue — accepted, and it was right in a way worth spelling out.

Both swallowed errors are fixed and now propagate.

set_permissions mattered in one specific direction: the mode being applied is
the mode the target already had, so a swallowed failure replaces a 0600
file with one at the umask default — wider than what it replaced — and reports
success. That is a security property silently inverted, not merely a durability
one skipped.

The parent-directory sync_all had both halves discarded (the File::open as
well as the sync_all), so the entire barrier could be a no-op while the
module's own platform table claimed "yes" for Unix. It now propagates, with one
deliberate exception: EINVAL, and EBADF on some network mounts, mean this
filesystem does not offer a directory fsync rather than the write failed, and
failing a save outright on those mounts would be a worse answer than proceeding.
EIO — the exact condition the sync exists to detect — no longer passes as
success. The classification is a named function, directory_fsync_is_unsupported,
with a test asserting EIO/ENOSPC/EACCES all propagate.

Both decisions are extracted (apply_mode_using, directory_fsync_is_unsupported)
rather than left inline, because on Unix neither failure can be arranged against a
file this process just created and owns — so hard-wired call sites would have left
both propagation paths permanently unexercised, which is how the swallowed
versions survived review in the first place. Three mutations confirm the new
assertions fail when the old behaviour is restored.

Suggestion 2 (chained symlinks) — accepted as valid, deferred with a reason.
resolve_write_target follows one level, so link1 -> link2 -> missing replaces
link2 rather than preserving the chain. Real, and narrow enough that it does not
justify expanding this PR further; iterative resolution needs a cycle bound and
its own tests. Tracked for the follow-up rather than bundled here.

Suggestion 1 (cheats.rs swallows to stderr) — agreed in principle, out of
scope here. This is the same class as the swallowed latency-config save fixed in
#411, and it deserves the same treatment: a Result signature plus status-bar
plumbing at the single call site (persist_cheats, reached from egui paint code
that currently has nowhere to put an error). That is a UI change, not a
persistence one, and belongs in its own change rather than riding a durability PR.

Nitpick (leaked temp file) — declined, deliberately. The remove_file calls
in the error paths ignore their results on purpose: the primary error is the one
worth returning, and a cleanup that fails when the disk is full should not
displace it. A log line would be the only addition, and this module is reached
from paths with no logger at hand.

Response to the Copilot thread (scratch-name retry)

Correct, and fixed. The single retry was justified by "advancing the counter
cannot repeat a name within a process" — true, and beside the point, because the
collision comes from a previous process. A run that crashed mid-session orphans
one scratch file per save it made, and pid reuse restarts the counter at zero, so
two orphans defeat one retry and the save fails for a reason the user cannot act
on. It is now a loop bounded at SCRATCH_ATTEMPTS = 8 — bounded rather than bare,
so a directory rejecting creation for a persistent reason errors instead of
hanging.

Writing the test for it turned up a second thing: reaching the exhaustion branch
through write_atomic means predicting the process-global SCRATCH_SEQ and
planting a decoy at every name the call will pick, and that prediction races,
because every parallel test calling write_atomic consumes sequence values.
Measured rather than assumed — a serialising mutex over the three tests that
peek at the counter still failed 2 runs in 5, since the tests doing the
consuming are precisely the ones that never look at it. The pre-existing
single-decoy test had been latently flaky since it was written and had simply
never lost the race. The loop is therefore driven directly through
open_fresh_scratch; 8 consecutive runs of the module are stable at 19 passed.

… flaky test that hid one Review of this PR found three places `write_atomic` reported success it had not earned. All three share a shape worth naming: an error discarded at a call site, under a comment that explains the rest of the operation and reads as though it covered the discard too. set_permissions was swallowed with `let _ =`. The direction is what makes this more than a nitpick: the mode being applied is the mode the target ALREADY had, so a failure replaces a file at 0600 with one at the umask default -- wider than what it replaced -- and tells the caller nothing. A security property silently inverted, not a durability step merely skipped. It now propagates, after removing the scratch file so the original is left intact. The parent-directory sync_all was swallowed along with the File::open that fed it, so the entire durability barrier could be a no-op while the module's own platform table claimed "yes" for Unix. It now propagates, with one deliberate exception: EINVAL, and EBADF on some network mounts, mean this filesystem does not offer a directory fsync rather than the write failed, and failing a save outright on those mounts is a worse answer than proceeding. EIO -- the exact condition the sync exists to detect -- no longer passes as success. The occupied-scratch retry was a single attempt, justified by "advancing the counter cannot repeat a name within a process". True, and beside the point: the collision comes from a PREVIOUS process. A run that crashed mid-session orphans one scratch file per save it made, and pid reuse restarts the counter at zero, so two orphans defeat one retry and the save fails for a reason the user cannot act on. Now a loop bounded at SCRATCH_ATTEMPTS = 8 -- bounded rather than bare, because a directory rejecting creation for a persistent reason would otherwise hang, and a hang is a worse answer than an error. THE TEST THAT WAS ALREADY FLAKY Getting the last one under test surfaced something the suite was not reporting. Reaching the exhaustion branch through write_atomic means predicting the process-global SCRATCH_SEQ and planting a decoy at every name the call will pick -- and that prediction races, because cargo test runs in parallel and every sibling test calling write_atomic consumes sequence values. Measured rather than theorised. A serialising mutex over the three tests that PEEK at the counter still failed 2 runs in 5, because the tests doing the consuming are precisely the ones that never look at it. Which means the pre-existing single-decoy test had been latently flaky since it was written and had simply never lost the race -- it needs one value where the new test needs eight, so it was forgiving enough to hide the defect rather than immune to it. All three decisions are therefore extracted into named functions -- apply_mode_using, directory_fsync_is_unsupported, open_fresh_scratch -- and driven directly. On Unix none of the three failures can be arranged against a file this process just created and owns, so hard-wired call sites would have left every propagation path permanently unexercised. That is how the swallowed versions survived review to begin with, and it is the fourth time this release that "extract it so a test can reach it" was the actual fix rather than a stylistic preference. Eight consecutive module runs are stable at 19 passed. THE WASM32 GATE, AGAIN Once the Unix arm started propagating, the non-Unix sync_parent_dir had to match its signature -- and an always-Ok return is exactly what clippy's unnecessary_wraps objects to, on non-Unix targets only. Native clippy passed; the wasm32 gate did not. Clippy's suggested fix (return unit) would break the parity the shared call site depends on, since write_atomic ends in sync_parent_dir(&target) as its tail expression, so the lint is allowed locally with that reason recorded. This is the second cfg-specific lint this one function has needed, and both were visible only off Unix. DECLINED AND DEFERRED, WITH REASONS Iterative symlink resolution (link1 -> link2 -> missing currently replaces link2 rather than preserving the chain) is real but needs a cycle bound and its own tests; tracked for follow-up rather than bundled here. Pushing the cheats.rs save error to its caller is agreed in principle and is the same class as the swallowed latency-config save, but it is a UI change -- a Result signature plus status-bar plumbing at a call site reached from egui paint code -- and belongs in its own change. Logging the leaked temp file is declined: the cleanup calls ignore their results deliberately, because a cleanup that fails when the disk is full should not displace the primary error. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build and rustdoc with warnings as errors. Three mutations confirm the new assertions fail when the old behaviour is restored; a first mutation pass reporting "caught" was rejected on inspection because the mutants had not compiled, which is not evidence of anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

Second review round on this PR, and the finding is in the fix from the first one. When the scratch-name loop exhausts, every name it tried was occupied -- that is what exhaustion means. The cleanup below then removed the last one. But that file already existed and belongs to somebody else: an orphan from a crashed run, or a scratch file a colliding instance is actively writing. A save that failed took another process's in-progress data with it. The defect predates the bounded loop -- a single retry could reach it too -- but the loop widened it from one chance to eight, so it arrived with the change that made it likelier. The scratch path is now `Option<PathBuf>`, assigned only after a successful exclusive create, and the cleanup runs only when there is something of ours to clean up. `None` means nothing was created, so anything sitting at those names is not ours to delete. THE FIRST TEST FOR THIS DID NOT TEST IT Worth recording, because the test looked right and passed. It forced a failure by calling `write_atomic` on a directory. That does fail -- but at the *rename*, not at the scratch create, and the rename branch is one where the scratch file genuinely is ours. So the test exercised a path the fix does not touch and passed identically against the defect and against the fix. Two mutations reported NOT CAUGHT, which is the only reason this was noticed. The first restored the unconditional delete and the second restored the exact reported shape -- assigning the scratch path before the create rather than after -- and neither moved the suite. Reaching the real branch means every candidate name colliding, and doing that through the real `scratch_name` means predicting the process-global `SCRATCH_SEQ` and planting a decoy at each name it will pick -- the same race documented on `open_fresh_scratch`. So the name source is now injectable: `write_atomic_with` takes the generator, `write_atomic` passes `scratch_name`, and the test passes a closure returning one fixed occupied name. Exhaustion is then deterministic, and the mutation restoring the reported defect is now caught. That is the fifth time this release that the fix was "extract it so a test can reach it", and the first time the lesson arrived through a test that had already been written and believed. ALSO FIXED, FROM THE SAME REVIEW `RENAME_ATTEMPTS`' doc claimed a 310 ms worst case. The loop returns on the fifth failure rather than backing off after it, so there are four sleeps, not five: 10 + 20 + 40 + 80 = 150 ms. 310 would be the figure if a fifth sleep of 160 ms happened, and it does not. Unchanged from the previous round, with reasons already given on the PR: iterative symlink resolution is deferred (needs a cycle bound and its own tests), pushing the cheats.rs save error to its caller is agreed but is UI plumbing that belongs in its own change, and the ignored `remove_file` results in the error paths are deliberate -- a cleanup that fails when the disk is full should not displace the primary error. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 20 module tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

doublegate · 2026-08-20T14:28:17Z

Round 2 — the blocking issue was in the round-1 fix, and it was right

Accepted and fixed. On exhaustion every candidate name was occupied — that is
what exhaustion means — and the cleanup removed the last one. That file already
existed and belongs to somebody else: an orphan from a crashed run, or a scratch
file a colliding instance is actively writing. A failed save took another
process's in-progress data with it.

The defect predates the bounded loop (a single retry could reach it too), but the
loop widened it from one chance to eight, so it arrived alongside the change that
made it likelier. The scratch path is now Option<PathBuf>, assigned only after a
successful exclusive create; the cleanup runs only when there is something of ours
to clean up. None means nothing was created, so anything at those names is not
ours to delete.

The first test for this did not test it

Worth recording, because it looked right and passed.

It forced a failure by calling write_atomic on a directory. That does fail — but
at the rename, not at the scratch create, and the rename branch is one where the
scratch file genuinely is ours. So it exercised a path the fix does not touch,
and passed identically against the defect and the fix.

Two mutations reported NOT CAUGHT — one restoring the unconditional delete, one
restoring the exact shape you described (assigning the scratch path before the
create rather than after) — and that is the only reason it was noticed.

Reaching the real branch needs every candidate name to collide, and doing that
through the real scratch_name means predicting the process-global SCRATCH_SEQ,
which races every parallel test that calls write_atomic. So the name source is
now injectable: write_atomic_with takes the generator, write_atomic passes
scratch_name, and the test passes a closure returning one fixed occupied name.
Exhaustion is deterministic, and the mutation restoring the reported defect is now
caught.

Nitpick — accepted, it was a factual error

RENAME_ATTEMPTS' doc claimed a 310 ms worst case. You are right: the loop returns
on the fifth failure rather than backing off after it, so there are four sleeps —
10 + 20 + 40 + 80 = 150 ms. 310 would be the figure if a fifth sleep of 160 ms
happened, and it does not. Corrected, with the arithmetic spelled out so the next
reader can check it.

Repeated suggestions — unchanged, reasons unchanged

cheats.rs: agreed in principle, and it is the same class as the swallowed
latency-config save fixed in #411. The fix is UI plumbing — a Result signature
plus a status-bar path from egui paint code, where the single call site
(persist_cheats) currently has nowhere to put an error — so it belongs in its own
change rather than riding a durability PR.

Symlink chains: resolve_write_target resolves one level, so
link1 -> link2 -> missing replaces link2. Real; iterative resolution needs a
cycle bound and its own tests. Tracked for follow-up.

Gates re-run: fmt, workspace clippy, the four frontend feature combos, both wasm32
combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and
572 frontend tests (20 in this module).

… a mutation found Third review round: no blocking issues, two suggestions worth taking, and one finding that came out of checking the second rather than out of the review. ENOTSUP / EOPNOTSUPP joins the excused set for directory fsync. The set is a list of ways a filesystem says "there is no barrier available here", and some network filesystems answer io::ErrorKind::Unsupported where others answer EINVAL. Leaving one out fails a save on that mount for a path that otherwise fully succeeded -- the same reasoning that put EINVAL and EBADF there. Parent-directory resolution loses an allocation and reads better: a filter plus unwrap_or_else over a borrowed Path, rather than a map_or_else building a PathBuf on both arms. THE MUTATION FOUND MORE THAN THE REVIEW DID Mutating that second change -- deleting the filter that maps an empty parent to "." -- came back NOT CAUGHT. Nothing in the suite covered it. Path::new("f.txt").parent() is Some(""), not None, and File::open("") fails with ENOENT. The fallback has been documented as load-bearing since it was written, and was never tested. It also matters more now than it did then. While the sync was best-effort, losing the fallback meant a durability step quietly skipped. Now that the sync propagates, losing it means write_atomic FAILS OUTRIGHT for any relative target -- a working call site turned into an error. The property tightened underneath a test that never existed. Tested by calling sync_parent_dir directly rather than through write_atomic, because reaching it that way needs a relative target and therefore a set_current_dir, which is process-global and races the parallel suite. Same trap open_fresh_scratch documents; third time in this PR that the deterministic route was to drive the function rather than the caller. DEFERRED, WITH THE REASONING scratch_name appends about twenty bytes, so a target already near the 255-byte filesystem limit fails with ENAMETOOLONG where a naive fs::write would have succeeded. Real, and a genuine regression in principle. Deferred because no current call site can reach it -- save states, per-game config and cheats all name their files from a SHA-256 hex digest or a ROM-derived stem, none of which approaches the limit -- and because truncating the base to make room introduces a collision risk that needs its own design rather than a one-line guard. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 21 module tests. Both changes mutation-checked; the second one twice, since the first attempt is what exposed the gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

doublegate · 2026-08-20T14:37:18Z

Round 3 — no blocking issues; both suggestions taken, and one of them found more

ENOTSUP / EOPNOTSUPP — accepted. Right for the same reason EINVAL and
EBADF are already there: the set is a list of ways a filesystem says there is
no barrier available here, and leaving one out fails a save on that mount for a
path that otherwise fully succeeded. Now matched via io::ErrorKind::Unsupported,
with the test extended.

The map_or_else simplification — accepted, and it drops an allocation from a
save path rather than only reading better.

Mutating that second change found something the review did not

Deleting the filter that maps an empty parent to . came back NOT CAUGHT.
Nothing in the suite covered it.

Path::new("f.txt").parent() is Some(""), not None, and File::open("")
fails with ENOENT. That fallback has been documented as load-bearing since it
was written and was never tested — and it matters more now than it did then.
While the sync was best-effort, losing it meant a durability step quietly skipped;
now that the sync propagates, losing it makes write_atomic fail outright for
any relative target. The property tightened underneath a test that never existed.

Tested by calling sync_parent_dir directly, because reaching it through
write_atomic needs a relative target and therefore a set_current_dir — which
is process-global and races the parallel suite. Third time in this PR the
deterministic route was to drive the function rather than the caller.

`ENAMETOOLONG` — valid, deferred, with the reasoning

You are right that scratch_name appends ~20 bytes, so a target already near the
255-byte limit fails where a naive fs::write would have succeeded. That is a
genuine regression in principle.

Deferred for two reasons. No current call site can reach it: save states, per-game
config and cheats all name their files from a SHA-256 hex digest or a ROM-derived
stem, none of which approaches the limit. And truncating the base to make room
introduces a collision risk — two long names sharing a truncated prefix would map
to the same scratch name, which is precisely what the pid-plus-counter scheme
exists to prevent. That needs its own design, not a one-line guard.

Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32
combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and
573 frontend tests (21 in this module). Both changes mutation-checked; the second
one twice, since the first attempt is what exposed the gap.

…n followed one level Fourth review round on this PR. The blocking finding is a consequence of the first round's fix, which is the honest way to describe it. Round 1 made the parent-directory sync propagate instead of being swallowed. That sync is the LAST step, and it necessarily runs after the rename it exists to make durable -- so its failure returns Err from a call in which the target was successfully replaced. The doc said "on failure the existing file is left untouched", which was true when the sync was best-effort and stopped being true the moment it propagated. A caller reading that error would conclude the old file survived; it did not. Rolling the rename back would mean writing the old contents again, turning a durability warning into a second full write that can itself fail. Swallowing it again is the defect round 1 fixed. So the contract is corrected instead: the doc now states which failures happen before the rename and which one happens after, and the post-rename error is wrapped so its message says the data WAS written and names what is actually uncertain -- whether the directory entry survives a power loss. The kind is preserved, so callers matching on io::ErrorKind still see the real cause. Broken symlink chains now resolve to their end. canonicalize cannot help here -- it fails outright when the final target does not exist -- so the chain is walked by hand. Following one level was enough for the dotfiles case that motivated it and wrong in general: link1 -> link2 -> missing replaced link2 with a regular file rather than writing through. Bounded at SYMLINK_DEPTH, because a chain can be a cycle and read_link succeeds forever on one; on exhaustion the last resolved path is returned rather than an error, since picking a write target is this function's whole job and a pathological chain should not fail a save. Raised in three consecutive rounds before being fixed, which is long enough. The redundant create_dir_all in save_state is removed. write_atomic creates the parent itself, and doing it twice meant a failure surfaced with one function's path context or the other's depending on which won the race. A TEST THAT TESTED THE HELPER, NOT THE CALLER The first test for the post-rename wrapper called post_rename_sync_error directly. That asserts the helper behaves and says nothing about whether the call site uses it -- a mutation deleting the map_err came back NOT CAUGHT. The same shape as the scratch-cleanup test earlier in this PR, arriving from a different direction: testing a helper is not testing the code that was supposed to call it. Fixed by injecting the parent sync alongside the scratch-name generator that was already injected, so a test can force a post-rename failure and assert the whole contract at once -- Err returned, message says written, and the target holds the NEW bytes. The mutation is now caught. A MUTATION HARNESS THAT POISONED ITS OWN BASELINE Recorded because the result was confident and wrong. An earlier harness run was killed by a foreground timeout mid-mutation. Python buffers stdout when not a tty, so every line it had printed was lost and it looked as though it had done nothing. It had: the third mutant was still on disk. The next run read that file as its baseline, so the bounded symlink loop was silently `loop {` in the BASELINE, stayed there after the run asserted "RESTORED clean" -- true, and restored to the mutation -- and a cycle test then spun forever against it, with the timeout attributed to the FIRST mutation. A specific, plausible, wrong "caught (hang)". Only a grep for the constant afterwards showed the bound was gone. The harness now writes a guard file before mutating and refuses to start if one exists, flushes every print, and warms the build against the real baseline first so a cold compile cannot be misread as a hang. DEFERRED, WITH REASONS The ENAMETOOLONG case stands from the previous round: scratch_name appends about twenty bytes, so a target near the 255-byte limit fails where a naive fs::write would succeed. No current call site can reach it -- save states, per-game config and cheats all name files from a SHA-256 digest or a ROM-derived stem -- and truncating the base to make room introduces a collision risk that needs its own design rather than a one-line guard. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 25 module tests. Three mutations, all caught, against a verified-clean baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

The Antigravity reviewer posted a fresh comment each round and DELETED the previous one. That kept the PR tidy and destroyed the record: a round nobody had read before the next push was gone, with nothing on the PR indicating it had ever existed. Unlike a CodeRabbit or Copilot review thread, an unaddressed finding left no trace at all -- so a clean comment list was not evidence that nothing had been raised. Observed on this very PR. Round 1 posted at 13:37:57Z and round 2 at 14:21:35Z; afterwards the issue-comments endpoint returned exactly ONE bot comment, with created_at equal to updated_at equal to 14:21:35Z. The first was gone -- not edited, since the timestamps would differ, and not appended to. Both of those rounds raised a blocking issue and both were correct, one of them a data-loss defect, so the cost of losing a round is not hypothetical. There is now ONE comment per PR, edited in place: the newest round on top, every earlier round folded into a collapsed <details> block beneath it. Same tidiness, nothing destroyed. The script issues no DELETE at all any more, and the selftest asserts the absence of one so the behaviour cannot return unnoticed. The archive is bounded by MAX_BODY_BYTES (60000, under GitHub's 65536 hard limit) because a PR with many pushes would otherwise grow it until an EDIT starts failing -- stranding the comment at whatever round last fit, which is the worst possible failure since the newest review is the one that cannot be posted. Oldest rounds drop first, and the drop is ANNOUNCED in the body: a silent truncation would look exactly like a PR that had only ever been reviewed once, which is the confusion this whole change exists to remove. Every failure path falls back to a plain post of the new review. A duplicate comment is noise; failing to publish a review is not. THE FORMAT LIVES IN ITS OWN FILE, AND THAT IS THE POINT scripts/_agy_comment_body.sh holds the sentinels and the split/trim helpers, and is sourced by both the reviewer and the selftest. agy-review.sh does its work at top level and so cannot be sourced, which is exactly how a test ends up reimplementing its subject -- and that happened here. The first version of these checks inlined its own copy of the awk pipeline, so a mutation deleting the marker strip from the script came back NOT CAUGHT. A test that reimplements what it tests agrees with itself forever. The fixture changed for the same reason. It had our own bot's comment first, so `first` selected it whether or not the author filter was present -- the security control that stops any user from putting the marker in a comment and having the bot edit it was untestable. A User comment carrying the marker now sorts ahead of ours, and deleting the filter fails. Eight mutations, all caught: the author filter, empty-versus-null, oldest-versus- newest selection, a reintroduced DELETE, the marker strip, the archive split's sentinel ordering, dropping the newest round instead of the oldest, and a drop that succeeds on an empty archive (which would spin the trim loop). Verified end to end by simulating four rounds through the real functions: all four findings present in the final body, newest first, marker appearing exactly once. INSTALLER AND WORKFLOW _agy_comment_body.sh is REQUIRED, not optional -- agy-review.sh sources it at startup, so an install without it fails at runtime rather than degrading. install-into-repo.sh now copies it and the selftest, the workflow chmods it, and both temporaries the archive path creates are pre-declared so the cleanup trap frees them on every exit including the early one after a successful edit. THIS DOES NOT TAKE EFFECT UNTIL IT MERGES The workflow checks out the DEFAULT BRANCH to run the scripts, so a change to agy-review.sh has no effect on any PR -- not even the PR that makes it. The README's default-branch rule covered the workflow and comment triggers; it now covers the scripts too, since that is the surprising half. Synced byte-identical to the canonical template at Local_Only-Projects/antigravity-pr-review/, including the timeout-minutes bound this repo had added locally. The four sibling repos keep the old behaviour until install-into-repo.sh is re-run against them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

The Antigravity reviewer's checkout used `actions/checkout@v7`, a tag that moves. RustySNES and SLAC already pinned it to a SHA; the template and this repo did not, so hardening was flowing the wrong way between copies of the same file. It matters more here than on a hosted job. This workflow runs on a SELF-HOSTED runner -- the maintainer's own machine, holding the agy CLI's Google AI Ultra OAuth session -- so a compromised tag executes there rather than in a disposable VM. The same reasoning already applied to `dtolnay/rust-toolchain` in this repo's CI, and this was the remaining unpinned action on the highest-trust runner. The SHA was verified rather than copied: `actions/checkout` tag v7 resolves to 3d3c42e5aac5ba805825da76410c181273ba90b1, the "prep v7.0.1 release" commit of 2026-07-17. The trailing `# v7` is the form Dependabot's github-actions ecosystem reads to keep the pin current, so it is not decoration. Synced to the canonical template, which now carries the pin for every future install. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

…ction Two mechanisms existed in exactly one of the five installs. Both belong in all of them, and the sweep that unified the comment-archive behaviour is the right moment to say so. A BACKEND OUTAGE POSTED AS A PASSING REVIEW When agy's upstream is down it prints an error rather than a review: Error: Eligibility check failed: UNAVAILABLE (code 503): The service is currently unavailable. That text is non-empty, so have_text() treated it as a valid review, POSTed it as the review comment, and the job exited 0 -- a green check for a review that never ran. Observed twice on SLAC PR #14, where the check passed in seven seconds with that string as its entire body. A control that cannot fail is worse than no control. The match is deliberately ANCHORED to the start of the capture rather than being a substring search, and it is bounded by size. A genuine review may quote a 503 or an UNAVAILABLE constant while reviewing retry logic, and aborting on that would be the false positive the OAuth guard's design notes warn about. A backend failure IS the whole capture and begins with `Error:`, so requiring the error on line one, in a capture short enough to contain nothing else, separates the two without a content heuristic. A backend error is transient, so it retries like empty output rather than aborting the way a lapsed session does -- but the capture is blanked so no later path can post it. The tally is a COUNTER, not a per-attempt flag: a boolean reset each attempt reflects only the last one, so a 503 on attempt 1 followed by empty output on attempt 3 would report the wrong cause. Both exit non-zero, so nothing unsafe -- but the log line is the only thing telling a human which outage they are looking at. MARKER-BASED EXTRACTION, AND WHY IT IS BETTER The selftest lifted the jq filter out of the reviewer by matching the declaration's own syntax: a sed range ending at the first line closing with a quote. A filter whose body ever ended a line that way would be SILENTLY TRUNCATED, and a truncated jq program can still compile and still return ids -- the exact silent-wrong-answer that file exists to prevent. Explicit `SELFTEST-EXTRACT` markers replace it. They also let a guard be several statements rather than one assignment, which is what makes the OAuth and service-error guards testable at all. Every marked block is now asserted to exist, to be valid shell, and to be sourceable, because a renamed marker would extract EMPTY -- and an empty guard sources fine and asserts nothing. WHAT THE MUTATIONS CHANGED Three of six came back NOT CAUGHT on the first pass, and two were real. The anchor could be deleted with every check still passing, because the fixture for "a review discussing a 503" put the error on line 3, where `head -n 1` already excluded it. A fixture whose FIRST line contains the error text mid-line -- which only `^` can reject -- now covers it. The persistent-outage abort was checked by grepping the script for its condition, which `if false && [ ... ]` still satisfies. That decision is now a named function, `backend_outage_should_fail`, called by the test rather than grepped for; three mutations of it are caught where the grep caught none. `have_text` moved inside the marked block so the block is self-contained -- the marker is a comment, so nothing about where the function is defined changed. The third, removing the `[ -s ]` empty-file check, is an EQUIVALENT mutant and is recorded as such rather than papered over with a test: an empty capture yields no grep match either way, so the check is defensive and its removal is unobservable. Superset verified rather than assumed: every non-comment line SLAC had before this sweep is either present in the template or is old delete machinery this design removes, plus a large-diff fallback the template supersedes -- SLAC's copy handled GitHub's 20,000-line limit only, the template's handles the 300-FILE limit too. All five installs now run one implementation. Selftest passes and actionlint is clean in each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

…e caught one Both were on the PR's own run, and both are mine. A RUSTDOC LINK I CLAIMED TO HAVE CHECKED `write_atomic` is public and `post_rename_sync_error` is private, so an intra-doc link between them fails `rustdoc::private-intra-doc-links` under `-D warnings`. Now a plain code span, which is the rule this repository already applies to feature-gated dependency names. The commit that introduced it listed "rustdoc with warnings as errors" among its gates. That claim was false: the full gate run predated the last edits, and I did not re-run it before committing. The lint is exactly the sort a green earlier run cannot vouch for, which is the whole reason the gate is meant to be re-run rather than remembered. Recorded plainly because the failure was the claim, not the link. THE WORKFLOW AND THE SCRIPTS COME FROM DIFFERENT REFS The reviewer workflow checks out the DEFAULT BRANCH to get its scripts -- that is deliberate, and documented, so a fork's code never executes on the self-hosted runner. But for a `pull_request` event GitHub runs the workflow YAML itself from the PR BRANCH. So the two halves come from different refs, and a change spanning both breaks its own PR. The previous commit added `scripts/_agy_comment_body.sh` and added it to the workflow's chmod; the job then died with chmod: cannot access 'scripts/_agy_comment_body.sh': No such file or directory because the checkout was of `main`, which does not have the file yet. The surprising half is the inversion: the default-branch rule is documented for the scripts -- a change to agy-review.sh has no effect until it merges -- and the corollary is that the workflow moves IMMEDIATELY while the scripts do not. That runs against the usual intuition that everything in a PR is consistent with itself. The workflow half now tolerates both script sets: the two required files are chmod'd unconditionally, anything added later only if present, with a trailing `true` so a false `[ -f ]` cannot fail the step under `bash -e`. A genuinely missing required file still fails loudly, because agy-review.sh sources it and dies -- the tolerance is in the workflow, not in the contract. Swept to the canonical template and to all four sibling installs, whose open PRs would have hit the identical failure on their next run. Gates, re-run in full this time: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, 573 frontend tests, the reviewer selftest, and actionlint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

…t hid it `is_transient_rename_error` was a `const fn` whose body called `io::Error::kind`. That method is not `const`, so the call is E0015 -- but the call sat behind `#[cfg(windows)]`, so the body was never compiled on Linux and every local gate and every PR check passed. PR runs here are Linux-only; the full matrix runs on `main`. So this would have turned `main` RED AFTER MERGE, on the branch where a red build blocks a release, rather than failing the PR that introduced it. Caught in review, not by a gate. Verified before being believed. A plausible reviewer claim had already proved false once this release, so the language question was settled against a minimal two-line crate: `const fn f(e: &io::Error) -> bool { matches!(e.kind(), ...) }` gives `error[E0015]: cannot call non-const method std::io::Error::kind in constant functions`. It was also checked against `main` -- the function is new in this PR, so nothing shipped broken. THE FIX IS NOT JUST DROPPING `const` Dropping it would fix this instance and leave the mechanism intact: any Windows- only code behind a `#[cfg]` is invisible to a Linux PR build, so the next error in it would land the same way. The Windows predicate now lives in an always-compiled function, reached through `cfg!(windows) && is_windows_sharing_violation(e)` rather than a `#[cfg]` block. `cfg!` is a compile-time boolean inside an ordinary expression, so the predicate is parsed, type-checked and borrow-checked on every platform, while `&&` short-circuits it away on non-Windows and the optimizer drops the branch. Runtime behaviour is identical; what changes is that a Linux `cargo check` now compiles the Windows logic. The proof is that restoring the `const` NOW FAILS ON LINUX, with `error[E0015]: cannot call non-const function is_windows_sharing_violation in constant functions`. The defect class moved from "invisible until another platform builds it" to "fails the PR". Two tests come with it: the sharing-violation predicate is exercised on whatever platform the suite runs, and a Unix-only test pins that the public predicate stays unconditionally false, so `cfg!` did not quietly change the single-attempt guarantee. Three mutations caught -- the predicate inverted, the `cfg!` guard dropped so Unix would retry, and the symlink bound reduced -- and a fourth, restoring `const`, is now a compile error rather than a silent pass. SYMLINK_DEPTH MATCHES THE KERNEL Raised from 8 to 40, Linux's own MAXSYMLINKS. The old value was justified as "far past any real dotfiles arrangement", which is true and beside the point: where the kernel would resolve a chain and this function gives up, the two disagree about where the file IS, and the write lands somewhere the user did not mean. It costs one `read_link` per level, on a chain already known to be broken. DECLINED, WITH REASONS A `Drop` guard removing the scratch file if `write_all` panics: the bounded scratch loop exists precisely so an orphan is survivable, and it now advances past up to eight of them. Adding an unwinding path to buy what the retry already handles is not worth the surface. SCRATCH_ATTEMPTS staying 8 rather than a rounder 5 or 10: the number is bounded by "how many scratch files one crashed run can plausibly orphan", and 8 is already generous for that. Rounding it changes nothing measurable. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 579 frontend tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

doublegate · 2026-08-20T16:00:27Z

Round 5 — the blocking finding was right, and no local gate could have caught it

Correct, and it would have broken main rather than this PR.
is_transient_rename_error was a const fn calling io::Error::kind, which is
not const — E0015. The call sat behind #[cfg(windows)], so the body was
never compiled on Linux and every local gate and every PR check passed. PR runs
here are Linux-only; the full matrix runs on main, so this would have turned
main red after merge.

Verified before believing it, since a plausible reviewer claim proved false once
earlier in this release. A two-line crate settles the language question:
const fn f(e: &io::Error) -> bool { matches!(e.kind(), ...) } →
error[E0015]: cannot call non-const method. Also checked against main: the
function is new in this PR, so nothing shipped broken.

Dropping `const` would have been the smaller half of the fix

That fixes the instance and leaves the mechanism: any Windows-only code behind a
#[cfg] is invisible to a Linux PR build, so the next error in it lands the same
way.

The predicate now lives in an always-compiled function reached through
cfg!(windows) && is_windows_sharing_violation(e) rather than a #[cfg] block.
cfg! is a compile-time boolean in an ordinary expression, so it is parsed,
type-checked and borrow-checked everywhere, while && short-circuits it away and
the optimizer drops the branch. Runtime behaviour is identical.

The proof is that restoring the const now fails on Linux:
error[E0015]: cannot call non-const function is_windows_sharing_violation in constant functions. The defect class moved from "invisible until another platform
builds it" to "fails the PR".

Two tests came with it — the sharing-violation predicate exercised on whatever
platform the suite runs, and a Unix-only test pinning that the public predicate
stays unconditionally false so cfg! did not quietly change the single-attempt
guarantee. Three mutations caught; a fourth (restoring const) is now a compile
error.

`SYMLINK_DEPTH` → 40 — accepted

You're right that the kernel's MAXSYMLINKS is the number that matters. My own
comment cited 40 and then chose 8 as "far past any real dotfiles arrangement" —
true, and beside the point: where the kernel resolves a chain and this function
gives up, the two disagree about where the file is, and the write lands
somewhere the user did not mean. Now 40.

Declined, with reasons

A Drop guard for a panic during write_all. The bounded scratch loop exists
precisely so an orphan is survivable, and it now advances past up to eight of
them. Adding an unwinding path to buy what the retry already handles is not worth
the surface — and a panic mid-save has a bigger problem than a stray .tmp.

SCRATCH_ATTEMPTS 8 → 5 or 10. The bound is "how many scratch files one
crashed run can plausibly orphan before its pid is reused", and 8 is already
generous for that. A rounder number changes nothing measurable; the comment states
the constraint so the next reader can re-derive it.

Gates re-run in full: fmt, workspace clippy, the four frontend feature combos,
both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as
errors, and 579 frontend tests.

Raised as blocking in three consecutive review rounds before I stopped deferring it, and the deferral was wrong on its facts. `cheats::save` wrote its error to `stderr`. On a windowed build nobody reads `stderr`, so a save that failed looked exactly like one that worked and the user lost their cheat list for that ROM with no signal at all. Same defect class as the swallowed latency-config save fixed in #411, in a PR whose entire subject is not swallowing save errors. WHY I DEFERRED IT, AND WHY THAT WAS WRONG The stated reason was that the fix needed UI plumbing: a `Result` signature plus a status-bar path from egui paint code with nowhere to put an error. The second half was false. The panel already carries `error`, `raw_error` and `enc_error`, each rendered with the same `colored_label` idiom and each cleared on a ROM change. The place to put it already existed; I asserted otherwise without looking. WHAT IT LOOKS LIKE NOW `save` returns `io::Result<()>`, with the `create_dir_all` failure propagating rather than printing-and-returning, and a serialization failure mapped to `InvalidData` -- the caller has one error channel and one thing to tell the user, so a bespoke error type for a case these types cannot produce would be ceremony. `persist_cheats` takes `&mut CheatPanelState` and records the outcome, clearing the field on success so a fixed problem stops being reported. It is cleared on a ROM change too, because a save error names the PREVIOUS ROM's cheat file and carrying it across would report a failure against a game it never touched -- the stale-panel-state seam this project has hit three times. The panel renders it ABOVE the lists rather than beside the add-fields, because the message is not about any single edit: it says the whole list on screen is not on disk. A test forces the failure by pointing the data dir at a regular file, so `create_dir_all` cannot create the parent, and asserts an error reaches the caller at all. The three existing round-trip tests now assert the save succeeded rather than discarding its result. ALSO: THE REVIEW COMMENT NO LONGER GOES THROUGH ARGV `gh api -f body="$(cat "$body_file")"` passed the whole comment as a single execve argument. At `MAX_BODY_BYTES` that approaches 60 KB against a `MAX_ARG_STRLEN` of 128 KB on Linux -- close enough that raising the bound later would start failing with E2BIG, and the failure would read as a GitHub error rather than a local limit. It is now `jq -n --rawfile b "$body_file" '{body: $b}' | gh api ... --input -`. Nothing traverses argv, and `--rawfile` makes the value a JSON string by construction, so neither shell quoting nor `-F` type-coercion can reinterpret a body that happens to look like a number or a boolean. Swept to the template and the four sibling installs. Declined, with the reason unchanged: logging a failed `remove_file` in the cleanup paths. The result is discarded so the PRIMARY error survives, and a cleanup that fails when the disk is full should not displace the error that matters. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, 580 frontend tests, and the reviewer selftest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

doublegate · 2026-08-20T16:13:03Z

Round 6 — the blocking finding was right, and my three deferrals were wrong on their facts

cheats.rs — accepted and fixed. I deferred this three rounds running on the
grounds that it "needs UI plumbing: a Result signature plus a status-bar path
from egui paint code, where the call site has nowhere to put an error."

The second half was false. The panel already carries error, raw_error and
enc_error, each rendered with the same colored_label idiom and each cleared on
a ROM change. The place to put it already existed; I asserted otherwise without
looking.

cheats::save returns io::Result<()> — create_dir_all propagates instead of
printing-and-returning, and a serialization failure maps to InvalidData (the
caller has one error channel and one thing to tell the user).
persist_cheats records the outcome in a new save_error, cleared on success
and on ROM change — a save error names the previous ROM's cheat file, and
carrying it across would report a failure against a game it never touched.
The panel renders it above the lists, not beside the add-fields: the message
isn't about any single edit, it says the whole list on screen is not on disk.
A test forces the failure (data dir pointed at a regular file, so
create_dir_all cannot create the parent) and asserts an error reaches the
caller. The three existing round-trip tests now assert success rather than
discarding the result.

`ARG_MAX` — good catch, taken, slightly differently

You're right that -f body="$(cat …)" puts the whole comment through argv. At
MAX_BODY_BYTES that approaches 60 KB against Linux's 128 KB MAX_ARG_STRLEN —
close enough that raising the bound later starts failing with E2BIG, and it
would read as a GitHub error rather than a local limit.

I used jq -n --rawfile b "$body_file" '{body: $b}' | gh api … --input - rather
than -F body=@file. Same avoidance of argv, plus --rawfile makes the value a
JSON string by construction — -F is a typed field, so a body that happened
to look like a number or boolean could be coerced. Swept to the template and all
four sibling installs.

Declined, unchanged

Logging a failed remove_file in the cleanup paths. The result is discarded
so the primary error survives — a cleanup that fails because the disk is full
must not displace the error that actually matters. The module says so at each
site.

EBADF = 9 vs pulling in libc/rustix. Agreed in principle and explicitly
conditional in the doc comment: if either dependency is ever added to this crate
for another reason, this should use it. Adding one for a single integer that is
fixed across every Unix ABI this builds for is the worse trade today.

Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32
combinations, the no_std thumbv7em build, rustdoc with warnings as errors, 580
frontend tests, and the reviewer selftest.

…ys so Round seven found no blocking issues. Its useful finding was a nitpick: the timeline counter's documentation explained the whole design and never answered whether a save state carries it. It does not, and both ways of getting that wrong are silent. The counter is not written by `snapshot`, not read by `restore`, and a loaded state does not carry its own value across. A restore instead ADVANCES the live counter, which is the correct reading of the event -- the timeline you were on has been replaced -- and is true regardless of which state was loaded. Serializing it would break two things without any visible symptom. Loading the same slot twice would restore the same generation twice, so a consumer comparing against its last-seen value would miss the second load entirely. And a value from another session says nothing about this one: the counter is only meaningful against the previous value THIS process observed, which is why the accessor already documents that comparing it across two `Nes` instances is meaningless. Because it lives outside the snapshot, `snapshot_schema_audit` cannot see it -- the very property that makes the design correct also means nothing mechanical would notice the reasoning being invalidated. So the behaviour is pinned by a test instead: a restore advances it, a SECOND restore of the SAME slot advances it again (the assertion that fails if it were ever serialized), and a fresh `Nes` restored from that state counts its own restores rather than inheriting a stored value. THREE SUGGESTIONS WERE ALREADY IMPLEMENTED Recorded because re-raising them is cheap and re-verifying them is not. The symlink resolver already has a hard cap. It is 40, matching Linux's MAXSYMLINKS, and `a_symlink_cycle_terminates` covers `a -> b -> a`. Round five of this same review asked for the kernel's number where I had 8; round seven suggests 8 or 16. Holding at kernel parity, for the reason round five gave: where the kernel resolves a chain and this function gives up, the two disagree about where the file IS. The comment archive is already bounded. MAX_BODY_BYTES is 60000 against GitHub's 65536, oldest rounds drop first, and the count of dropped rounds is printed in the body rather than truncating silently. The scratch file is already cleaned up on an early return via `?`. The `?` returns from the CLOSURE, not from `write_atomic`, so `write_result` is `Err` and its cleanup runs. `a_failed_write_leaves_the_original_intact` now asserts no `.tmp` survives -- and its comment states precisely which branch that covers, because a mutation showed it reaches the RENAME-failure cleanup rather than the write-failure one. Removing the branch it does reach is caught; removing the other is not, and the test says so rather than being read as covering both. The two `skip_serializing_if` fields were checked rather than assumed: both carry `#[serde(default)]`, so a config written before them still loads. `rustynes-core` changes, so the accuracy gates are VERIFIED rather than asserted: AccuracyCoin 141/141 (100.00%, RAM decoder; the framebuffer decoder reports 120 and is known-buggy) and nestest 0-diff. Full clippy matrix, both wasm32 combinations, no_std thumbv7em, rustdoc with warnings as errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

doublegate · 2026-08-20T16:25:54Z

Round 7 — no blocking issues. The nitpick was the useful finding; the three suggestions were already implemented.

`timeline_generation` and save states — real gap, closed

The docs explained the whole design and never answered your question. It is not
part of the save state: not written by snapshot, not read by restore, and a
loaded state does not carry its own value across. A restore advances the live
counter, which is the correct reading of the event — the timeline you were on has
been replaced — and is true regardless of which state was loaded.

Serializing it would break two things silently: loading the same slot twice would
restore the same generation twice, so a consumer comparing against its last-seen
value would miss the second load; and a value from another session says nothing
about this one.

Because it lives outside the snapshot, snapshot_schema_audit cannot see it —
the property that makes the design right also means nothing mechanical would notice
the reasoning being invalidated. So it is pinned by a test: a restore advances it, a
second restore of the same slot advances it again (the assertion that fails
if it were ever serialized), and a fresh Nes restored from that state counts its
own restores rather than inheriting a stored value.

The three suggestions are already in the branch

Symlink cycles are already bounded. SYMLINK_DEPTH = 40 with
a_symlink_cycle_terminates covering a -> b -> a. Worth flagging a contradiction:
round 5 of this review asked for the kernel's number where I had 8, and round 7
suggests 8 or 16. Holding at 40, for round 5's reason — where the kernel resolves a
chain and this gives up, the two disagree about where the file is.

The archive is already bounded. MAX_BODY_BYTES = 60000 against GitHub's 65536,
oldest rounds dropped first, and the number dropped is printed in the body rather
than truncating silently.

The ? path already cleans up. The ? returns from the closure, not from
write_atomic, so write_result is Err and its cleanup runs.
a_failed_write_leaves_the_original_intact now asserts no scratch file survives.

One correction to my own work there, found by mutating: that test reaches the
rename-failure cleanup, not the write-failure one. Removing the branch it does
reach is caught; removing the other is not — and the test's comment now says so
rather than being read as covering both. Only a panic can orphan a scratch file,
and the bounded scratch loop exists so that is survivable.

The `serde` nitpick — checked, not assumed

Both skip_serializing_if fields carry #[serde(default)], so a config written
before them still loads. Verified in the source rather than trusted.

`EBADF = 9` — agreed, and already conditional

The doc comment says exactly this: if libc or rustix is ever added to this crate
for another reason, it should use it. Adding a dependency for one integer that is
fixed across every Unix ABI this builds for is the worse trade today.

rustynes-core changed, so the accuracy gates are verified, not asserted:
AccuracyCoin 141/141 (100.00%, RAM decoder) and nestest 0-diff. Plus the full
clippy matrix, both wasm32 combinations, no_std thumbv7em, and rustdoc with
warnings as errors.

Round eight, and the blocking finding is a data-corruption bug in code from round two of this same PR. `agy_drop_oldest_round` located the oldest archived round by matching `/^<details>$/` -- the tag itself. A review body legitimately contains `<details>` blocks: folded logs, collapsed code, another bot's summary, and the archived rounds are themselves nested `<details>`. So the cut could land INSIDE a round, leaving torn HTML and half a review in a comment nobody would think to check. It is now delimited by `AGY_ROUND_MARK`, an HTML comment the writer emits ahead of each round. Invisible when rendered, and it cannot occur by accident in prose the way a tag can. This is the same mechanism the archive boundaries already used, and the same mechanism this PR adopted from SLAC one commit earlier for exactly this reason -- that a `sed` range matching a declaration's own syntax truncates silently. I used markers for the outer boundaries and a naive regex for the inner ones in the same file, which is the kind of half-application that reads as consistent until someone tries it with real content. FAIL-SAFE FOR ARCHIVES ALREADY IN THE WILD An archive written by the previous version has no round markers. With none present the function exits non-zero and NOTHING is dropped, so the edit fails on size -- recoverable, visible, and repaired by the next round -- rather than the archive being silently mangled. Pinned by its own check. Three checks and two mutations. The nested case is the one that matters: a round carrying its own `<details>` must survive the oldest being dropped, and restoring the tag-matching form fails it. Also asserted: the writer actually EMITS the sentinel, because if it did not, every archive would look legacy-shaped, the trim would silently never fire, and the comment would grow until the edit failed. ALSO FROM THIS ROUND `cheats::save` no longer calls `create_dir_all` before `write_atomic`, which creates the parent itself. Doing it twice meant a failure surfaced with one function's path context or the other's depending on which won -- the same redundancy already removed from `save_state.rs`. Swept to the canonical template and the four sibling installs; selftest passes in all five. Full clippy matrix, both wasm32 combinations, no_std thumbv7em, rustdoc with warnings as errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

doublegate · 2026-08-20T16:33:26Z

Round 8 — the blocking finding was right, and it was my bug from round 2

Accepted and fixed. agy_drop_oldest_round located the oldest archived round
by matching /^<details>$/ — the tag itself. A review body legitimately contains
<details> blocks (folded logs, collapsed code, another bot's summary), and the
archived rounds are themselves nested <details>. So the cut could land inside
a round, leaving torn HTML and half a review in a comment nobody would think to
check.

Now delimited by AGY_ROUND_MARK, an HTML comment the writer emits ahead of each
round: invisible when rendered, and it cannot occur by accident in prose the way a
tag can.

What makes this worth recording is that it's the same mechanism this PR adopted
one commit earlier — the SELFTEST-EXTRACT markers, taken from SLAC precisely
because a sed range matching a declaration's own syntax truncates silently. I
used markers for the outer archive boundaries and a naive regex for the inner round
boundaries in the same file. That reads as consistent right up until someone
tries it with real content.

Fail-safe for archives already written

An archive from the previous version has no round markers. With none present the
function exits non-zero and nothing is dropped, so the edit fails on size —
recoverable, visible, repaired by the next round — rather than the archive being
silently mangled. Pinned by its own check.

Three checks and two mutations. The nested case is the one that matters: a round
carrying its own <details> must survive the oldest being dropped, and restoring
the tag-matching form fails it. Also asserted that the writer actually emits
the sentinel — without that, every archive looks legacy-shaped, the trim silently
never fires, and the comment grows until the edit fails.

`cheats.rs` redundant `create_dir_all` — taken

Right, and it matches the cleanup already done in save_state.rs. Doing it twice
meant a failure surfaced with one function's path context or the other's depending
on which won.

Swept to the template and all four sibling installs; selftest passes in all five.
Full clippy matrix, both wasm32 combinations, no_std thumbv7em, rustdoc with
warnings as errors.

…ertion that pins it Round nine found no blocking issues. Two of its suggestions were worth acting on, and getting the first one under test exposed two holes -- one of which I had just created. THE RETRY BUDGET Review reports that Windows Defender can hold a lock on a newly written file for longer than the 150 ms the retry loop waited, so the retry would exhaust before the lock cleared and the save would fail for a reason that resolves itself a moment later. Eight attempts now give 10+20+40+80+160+320+640 = 1270 ms. THAT TIMING CLAIM IS NOT ONE THIS PROJECT CAN VERIFY. There is no Windows runner in the PR matrix and no measurement behind it here. It was adopted anyway because the trade is one-sided: the cost is a longer stall on a save that is ALREADY FAILING, and the benefit is not losing a save the user believes happened. The comment says so, and names itself as the place a real number belongs if anyone measures one. TWO HOLES, BOTH FOUND BY MUTATION Raising the count made the exhaustion test really sleep 1270 ms -- measurably, 0.15 s to 1.27 s for the module. The base backoff is now a parameter and tests pass 0, which changes nothing they assert: the loop's contract is the attempt COUNT and the propagated error, not wall time. That injection created a hole. A mutation zeroing the PRODUCTION backoff came back NOT CAUGHT, and it is not a weak test: on Unix `is_transient_rename_error` is always false, so the loop never sleeps and there is nothing to observe on the platform CI runs. Only a Windows run could see it. Declared in the module's existing "what the tests do NOT cover" section rather than papered over. The second hole was already there. `assert_eq!(calls, RENAME_ATTEMPTS)` pins NOTHING -- both sides move together, so reducing the constant satisfies it while silently halving the budget, and that mutation came back NOT CAUGHT. The property is now asserted through a derived `total_backoff_ms()`: at least 500 ms in total, which is the requirement the count was chosen to meet rather than a restatement of the count. The mutation is caught. `total_backoff_ms` is `#[cfg(test)]`: nothing in the shipped path needs the sum, the loop sleeps per attempt. A derivation of two production constants that exists so a test can assert what they were chosen for is a fine reason for a test-only item and a poor reason to keep dead code in the binary. THE PANEL ERROR IS WRAPPED, NOT TRUNCATED An OS-level I/O error can be long, and an unwrapped label widens the window to fit it -- the panel's other error fields carry short messages this crate authored, so they never showed the problem. Wrapped rather than truncated because the tail of an I/O error is usually the part naming the actual cause. THREE SUGGESTIONS DECLINED, WITH REASONS The archive cap is already implemented -- MAX_BODY_BYTES against GitHub's 65536, oldest dropped first, count announced. Third round it has been raised. Documenting the `timeline_generation` serde trap: already done in the previous commit, and the premise is wrong. There is no `#[serde(skip)]`; `rustynes-core`'s `nes.rs` contains zero `serde` at all -- the snapshot is a hand-written binary format. The field is excluded by not being written, which is what the new documentation and its test describe. A startup sweep of orphaned scratch files: declined on a hazard, not on effort. Two RustyNES instances can run at once, and the sweeper cannot tell a dead run's orphan from a live run's in-flight scratch file without checking whether the pid is alive -- which is racy and platform-specific. Deleting a live instance's scratch mid-write is a worse failure than the clutter it cleans. The bounded scratch loop already makes orphans survivable. Logging the excused directory-fsync errnos: this crate has no logger. Adding a logging dependency for one debug line is not the trade today, and the errnos are named in the code with the reason each is excused. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 580 frontend tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

doublegate · 2026-08-20T16:43:47Z

Round 9 — no blocking issues. Two suggestions taken; getting one under test exposed two holes.

The retry budget — taken, on an unverified premise, stated as such

You report that Defender can hold a lock longer than the 150 ms the loop waited.
Eight attempts now give 10+20+40+80+160+320+640 = 1270 ms.

I can't verify that timing claim — there's no Windows runner in the PR matrix
and no measurement behind it here. Adopted anyway because the trade is one-sided:
the cost is a longer stall on a save that is already failing; the benefit is not
losing a save the user believes happened. The comment says exactly that, and names
itself as where a real number belongs if anyone measures one.

Two holes, both found by mutating

Raising the count made the exhaustion test really sleep — 0.15 s → 1.27 s for the
module. The base backoff is now a parameter and tests pass 0, which changes
nothing they assert (the contract is the attempt count and the propagated
error, not wall time).

That injection created a hole. A mutation zeroing the production backoff came
back NOT CAUGHT — and not because the test is weak: on Unix
is_transient_rename_error is always false, so the loop never sleeps and there
is nothing to observe on the platform CI runs. Only a Windows run could see it.
Declared in the module's existing "what the tests do NOT cover" section rather than
papered over.

The second hole was already there. assert_eq!(calls, RENAME_ATTEMPTS) pins
nothing — both sides move together, so reducing the constant satisfies it while
silently halving the budget. That mutation came back NOT CAUGHT. The property is
now asserted through a derived total_backoff_ms(): at least 500 ms in total,
which is the requirement the count was chosen to meet rather than a restatement of
the count. Now caught.

The panel error — wrapped, not truncated

Right that a long OS error would blow out the layout. Wrapped rather than
truncated: the tail of an I/O error is usually the part naming the actual cause.

Declined, with reasons

The archive cap is already implemented — MAX_BODY_BYTES against GitHub's
65536, oldest dropped first, count announced in the body. Third round this has been
raised.

The timeline_generation serde trap — already documented in the previous
commit, and the premise is off: there is no #[serde(skip)]. rustynes-core's
nes.rs contains zero serde; the snapshot is a hand-written binary format,
and the field is excluded by not being written. That is what the new docs and test
describe.

A startup sweep of orphaned scratch files — declined on a hazard, not on
effort. Two RustyNES instances can run at once, and a sweeper cannot distinguish a
dead run's orphan from a live run's in-flight scratch file without checking
whether the pid is alive, which is racy and platform-specific. Deleting a live
instance's scratch mid-write is a worse failure than the clutter it cleans, and the
bounded scratch loop already makes orphans survivable.

Logging the excused fsync errnos — this crate has no logger. Adding a logging
dependency for one debug line isn't the trade today; the errnos are named in code
with the reason each is excused.

Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32
combinations, no_std thumbv7em, rustdoc with warnings as errors, 580 frontend
tests.

github-actions · 2026-08-20T16:47:57Z

Antigravity review (Gemini via Ultra)

This PR extracts atomic file writing into a robust, platform-aware shared utility to prevent data loss across configs, save states, and cheats, introduces a timeline generation counter to safely reset debug telemetry upon timeline jumps, avoids serializing empty TOML tables for opt-in config maps, and updates the libretro upstream version string.

Blocking issues

None found.

Suggestions

crates/rustynes-frontend/src/atomic_write.rs: Consider typing base_backoff_ms in rename_with_retry_using as a std::time::Duration rather than a u64 representing milliseconds. This leverages Rust's type system for time intervals and avoids potential unit confusion.

Nitpicks

crates/rustynes-frontend/src/atomic_write.rs: In post_rename_sync_error, wrapping the error with io::Error::new() preserves the ErrorKind but causes .raw_os_error() to return None. Since callers only surface this error to the user rather than matching on OS errnos, this is completely acceptable, but worth noting for future maintenance.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

…GA device-under-test (#429) * feat(cosim): RustyNES as a co-simulation oracle for an FPGA device-under-test Opens the v2.4.1 - v2.5.0 "Fabric" line (ADR 0037): a new NES core written in SystemVerilog from public hardware documentation, in a sibling RustyNES_MiSTer repository, verified against this emulator. This commit lands the half that lives here -- the boundary between the two -- plus the decision record, the execution plan, and the research archive behind them. RustyNES is not being ported to FPGA and cannot be. A MiSTer core is SystemVerilog compiled by Quartus 17.0.2 into a Cyclone V bitstream; Rust does not become a bitstream, and high-level synthesis of a cycle-accurate emulator's control flow does not produce usable hardware. What is buildable is a new implementation with RustyNES as its verification oracle -- the one role it is uniquely equipped for, and the reason to attempt this here rather than elsewhere. The reference firewall extends to HDL accordingly: NES_MiSTer and fpganes rtl/ are strict black boxes, instantiable as opaque modules to compare outputs, never readable as source. WHAT LANDS crates/rustynes-cosim is a pure wrapper. It adds no core API, changes no core behaviour, sits outside workspace default-members (verified through cargo metadata, not assumed), and exposes two surfaces: a narrow C ABI a Verilator testbench links, and the safe Rust API a golden-export binary uses. The nes_golden_export CLI emits five formats -- the CpuBootTrace binary, the per-cycle IRQ/bus CSV, a pre-palette u16 index framebuffer, the 2 KiB work RAM the AccuracyCoin RAM decoder reads, and a provenance manifest. Two design choices are load-bearing and recorded so they are not re-litigated. Replay rather than lockstep, because Nes exposes run_frame() and step_instruction() and nothing finer -- cycle-lockstep would mean new core API on the hot path, and it would gain nothing, since the determinism contract already makes a pre-recorded trace exactly the trace a lockstep run produces. And the framebuffer is exported pre-palette, so a palette difference cannot masquerade as a rendering difference: the failure mode v2.3.8 "Parallax" was built to prevent. TWO FINDINGS THE CRATE WAS NOT LOOKING FOR No CI invocation had ever enabled cpu-boot-trace or irq-timing-trace for clippy, so those two rustynes-core modules had never passed the lint gate. Turning them on surfaced six pre-existing errors -- four missing_const_for_fn, one manual_is_multiple_of, one collapsible_if -- all fixed here. The workspace gate reads as exhaustive and is not: --workspace --all-targets covers each crate's DEFAULT feature set, so a cfg-gated module is invisible to it. All six sites are behind default-off features, so the shipped core is untouched by construction; the collapsed if keeps its is_some() guard as the first arm of a let-chain, because take() clears trace_a12_latest and short-circuiting is what keeps that from running when tracing is off. This crate therefore enables both features UNCONDITIONALLY rather than re-exposing them as its own optional features. A build without them would compile, link, run, and export empty goldens -- an absence of signal that reads exactly like agreement. Mandatory turns that into a compile error. And the first run_frame() after power-on advances zero cycles. The PPU is constructed at dot 340 of the pre-render line, so the 7-cycle reset sequence ticks past the frame wrap and leaves frame_complete latched; the first call consumes the latch and returns having stepped nothing. Measured, not inferred: frame 0 advances the cycle counter by 0, frames 1..3 by ~29,780 each. Every other caller in the workspace runs thousands of frames, so one lost frame is invisible to them -- but a bare loop would have emitted an (n-1)-frame golden under a manifest claiming n, a provenance record wrong in the one direction that matters, since a DUT compared against it would be off by a frame for reasons nothing in the record explains. Oracle::advance_frames gates on the frame counter instead, with a jam bail-out, and the manifest records requested and actual separately because they can legitimately differ. VERIFICATION Three new assertions, each proven by mutation before being trusted: counting calls instead of frames is caught, an off-by-one target is caught, and removing the jam bail-out is caught as a HANG rather than a failure -- which is why the mutation harness needed a timeout to register it at all. rustynes-core changed, so the accuracy gates are verified rather than asserted: AccuracyCoin 141/141 (100.00%, RAM decoder -- the framebuffer decoder reports 120 and is known-buggy) and nestest 0-diff. Workspace 2226 passed across 128 suites, 0 failed. fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build and rustdoc with warnings as errors all pass. SCOPE, STATED RATHER THAN DISCOVERED v2.5.0 is scoped to "the 6502 rung closes" -- the harness plus a cycle-exact 6502, gated on nestest 0-diff and per-cycle bus equality. A from-scratch cycle-accurate NES core is 7-13 months FTE against a two-to-four-week window at demonstrated cadence, so PPU, APU and MiSTer integration become the v2.6-v2.9 programme. Two risks are accepted in writing: NES_MiSTer already scores 121/125 on AccuracyCoin where real Famicom AV hardware also scores ~121/125, so there is no published accuracy headroom and the core may be declined as a duplicate; and the oracle can be wrong, since 141/141 is not "matches silicon", so every rung is labelled by whether it has an independent oracle. Docs: ADR 0037, docs/mister.md, to-dos/plans/v2.5.0-fabric-plan.md, and the research archive at to-dos/plans/research/. AGENTS.md gains the HDL firewall extension and three operating notes. Addresses the "Fabric" programme opening; no issue is finished by this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj * test(cosim): the rung-0 gate, and a demonstration that it can fail The "Fabric" ladder's rung 0 is stated as: feed RustyNES's own golden back in as if it were the device-under-test and get zero divergences. Without it, every later red result is ambiguous between "the RTL is wrong" and "my writer packs a field wrong" -- and early on the second is far likelier. Three tests, and the second and third exist because the first alone proves less than it appears to. The self-diff asserts two independent exports of the same ROM and seed are byte-identical across the boot trace, the index framebuffer, work RAM, the cycle count and the call count. This is the determinism contract observed at the boundary this crate exposes: if it fails, a pre-generated golden is not the trace a lockstep run would have produced, and replay-as-oracle is unsound rather than merely inconvenient. The corruption test exists because a comparator that always reports agreement passes a self-diff trivially. One flipped bit in the middle of the trace is the smallest divergence an RTL bug could plausibly produce, and it must not compare equal. The frame-count test checks the count WITHOUT trusting the counter that produced it: five frames must be five NTSC frames' worth of CPU cycles, and it must take six run_frame() calls to get there, because the first after power-on is swallowed by the frame_complete latch the reset sequence leaves set. Done in integer half-cycles rather than floating point -- the whole job is an arithmetic check, and doing it in f64 would have meant two lossy casts and a lint suppression to verify five frames. VERIFIED BY HAND FIRST, THEN BY MUTATION Exported against the committed mmc1_a12 ROM and checked three ways, because a manifest that agrees with itself is not evidence. The recorded rom_sha256 matches an independent sha256sum. run_frame_calls = 6 for frames_actual = 5 makes the power-on latch visible in the record rather than hidden by it. And 148905 cycles divided by 29780.5 is exactly 5.000 frames, where a bare loop gives 4.0. The real cpu_boot_trace_diff CLI reports "All 5464 aligned records match" on the self-diff and exits 0; the same CLI against a one-bit corruption reports the divergence at cycle 561, PC $C419, naming the field and both values. So the zero is a signal from a tool that can tell, not from one that cannot. Then mutated: advance_frames reverted to the bare `while calls < frames` loop that motivated it. Caught, with the magnitude quantified -- 119123 cycles against an expected 148902, short by 59559 half-cycles, which is exactly one frame. Gates: fmt and clippy clean for the crate; 8 unit plus 3 integration tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj * docs(mister): record the null-DUT gate that now backs rung 0 The spec described rung 0's acceptance criterion as something to build. It is built for the half that lives in this repository, so the document should say what exists rather than what is planned -- including that the zero was checked against the real cpu_boot_trace_diff CLI and shown to be able to report non-zero. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj * docs(agents): the Antigravity reviewer replaces its comment, it does not append The ceremony rules already record that this reviewer posts as a plain issue comment, invisible to both reviewThreads and reviews[].body. There is a sharper hazard on top of that, observed on PR #428. Round 1 posted at 13:37:57Z and round 2 at 14:21:35Z. Afterwards the issue-comments endpoint returned exactly ONE bot comment, created_at equal to updated_at equal to 14:21:35Z. The first was gone -- not edited, since the timestamps would differ, and not appended to. Deleted and replaced. CodeRabbit and Copilot threads persist and can be resolved, so an unread finding stays visible. An Antigravity finding does not, and nothing on the PR indicates a round ever existed. A clean comment list is therefore not evidence that nothing was raised. Both rounds on that PR were blocking and correct, one of them a data-loss defect, so the cost of missing a round is not hypothetical. The rule that follows: read the comment before every push rather than after, and quote its findings into the reply, since the reply persists and the original does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj * docs(agents): the reviewer's destructive replacement is fixed, and where it is not The note written when the behaviour was discovered described it as current. v2.4.0 fixed it, so the note now records what happened, what replaced it, and the two places the fix does not yet apply. The mechanism is worth keeping in full rather than trimming to "fixed": it is the reason the bot ceremony has to be run before a push rather than after, and that rule still binds on the four sibling repos until their own PRs land. Two details added that the original note could not have had. Rounds are delimited by an HTML-comment sentinel rather than the `<details>` tag, because a review body legitimately contains `<details>` blocks and matching the tag cut inside a round -- found in review one commit after adopting markers elsewhere in the same file for exactly that reason. And the default-branch rule has a corollary that bit once: the workflow YAML comes from the PR branch while its checkout fetches the default branch, so a change adding a script file breaks its own PR unless the workflow half tolerates both script sets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj * fix(cosim): the rung-0 gate cannot be an integration test in this crate CI's `cargo test --workspace --release --features test-roms` failed to compile the `null_dut_self_diff` target with nineteen errors of the form: error: the crate `sha2` requires panic strategy `abort` which is incompatible with this crate's strategy of `unwind` This crate declares `crate-type = ["rlib", "staticlib", "cdylib"]` because a Verilator testbench links the C ABI. Under `--release` the workspace profile sets `panic = "abort"`, so the dependency graph is built with that strategy -- and an INTEGRATION test in `tests/` is a separate binary that links the rlib and needs `unwind`. The two cannot share those dependencies. The three checks now live in the lib's `#[cfg(test)]` module beside the eight that were already there. Nothing is lost: they drive the crate's public API exactly as an external consumer would, which is the property that made them integration tests in the first place. The reason is recorded at the site, because the next reader will want to move them back. `rustynes-libretro` is the same shape and has never hit this, and comparing the two is what identified the mechanism rather than the symptom: it declares `cdylib` and `staticlib` with NO `rlib`, and keeps its tests `#[cfg(test)]` in `src/`. That is the working configuration for a crate in this workspace that exports a C ABI. WHY NO LOCAL GATE CAUGHT IT It compiles clean in debug. My gate script runs `cargo test` without `--release`, and `cargo test --release -p rustynes-cosim` alone also passes -- the failure needs the workspace-wide release build with the harness features, which is precisely what the CI job runs and what I had not run locally. That is the same shape as the two other misses in this line: a gate that exists but does not reach the configuration in question. The wasm32 combinations were one, `#[cfg(windows)]` code never being type-checked on Linux was another, and this is a third -- release-profile linking of a multi-crate-type crate. 11 tests pass in the crate. Verified where it counts rather than by inference: under the exact CI invocation (`cargo test --workspace --release --features test-roms`), `rustynes-cosim` now compiles with ZERO panic-strategy errors -- the crate that previously produced nineteen. The rest of that build is the test harness's fat-LTO link and is left to CI, which is the authoritative gate for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj * fix(cosim): a dot in a ROM name ate the golden's filename, and two swallowed errors Three of the four blocking findings from review, all confirmed before acting and one of them a corrupted-output bug rather than a style point. A DOT IN THE ROM NAME TRUNCATED EVERY GOLDEN `Path::with_extension` replaces everything after the LAST dot. A ROM named `Super Mario Bros. 3.nes` has a stem of `Super Mario Bros. 3`, so `with_extension("ram.bin")` produced `Super Mario Bros.ram.bin` -- the frame number silently eaten, and every one of the five goldens landing under a name that is not the one the manifest describes. Verified rather than reasoned about: a two-line program prints exactly that. Dots are common in NES filenames, so this would have corrupted real golden sets rather than being a theoretical edge. `suffixed()` appends to the `OsString` instead. The test asserts the correct name AND asserts that it differs from `with_extension`'s, so a future "simplify this" is caught rather than silently reintroducing it. TWO SWALLOWED ERRORS `sha256_hex` discarded a `fmt::Result` through `let _ = write!`. It cannot fail into a `String`, but the project's rule is about the shape, not the odds, and the hex is now built by hand with no fallible call to discard. Review also suggested `format!("{:x}", Sha256::digest(data))`. That does not compile: sha2 0.11 returns `hybrid_array::Array<u8, _>`, which does not implement `LowerHex` -- `the trait bound Array<u8, ...>: LowerHex is not satisfied`, checked against a scratch crate rather than assumed. The digests are now pinned against the known SHA-256 of `""` and `"abc"`, which is an independent oracle rather than our own output. The C ABI returned a flat `-3` for every write failure. A full disk, a read-only directory and a permission denial were indistinguishable to the testbench -- and the testbench is the only thing that sees this, so discarding the cause at the boundary is the one place it cannot be recovered later. Codes at or below `-100` are now `-(100 + errno)`, with `-1`..`-9` still meaning "the call was malformed" and `-3` reserved for an error carrying no `raw_os_error`. STILL OPEN, DELIBERATELY NOT FIXED HERE The fourth finding -- workspace feature unification -- is REAL and confirmed. A `cargo build --workspace` builds `rustynes-core` ONCE with the union of features, and that union now includes `cpu-boot-trace` and `irq-timing-trace`: ['cpu-boot-trace', 'debug-hooks', 'default', 'hd-pack', 'irq-timing-trace', 'std'] measured through `--message-format=json` rather than inferred. It does not affect the shipped binary (`-p rustynes-frontend` never selects this crate) and it does not affect behaviour (the features are output-only; AccuracyCoin is 141/141 in a workspace run), but "the emulation core is untouched" is imprecise as written for workspace-wide builds. Every fix for it trades away something real -- excluding the crate from the workspace costs `--workspace` test coverage of its 14 tests, and making the trace features non-default costs the compile-time guarantee that a misconfigured build cannot export empty goldens. That is a maintainer's call about which guarantee matters more, not one to make silently inside a review round, so it is carried rather than guessed at. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 14 tests in the crate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj * fix(cosim): exclude the crate from the workspace so the accuracy battery tests the shipped scheduler `rustynes-cosim` enables `cpu-boot-trace` and `irq-timing-trace` on `rustynes-core`, and cargo unifies features across a workspace build. As a member, it made `cargo build --workspace` compile the core ONCE with the union. Measured through `--message-format=json` rather than inferred: before: ['cpu-boot-trace', 'debug-hooks', 'default', 'hd-pack', 'irq-timing-trace', 'std'] after: ['debug-hooks', 'default', 'hd-pack', 'std'] THE PERCENTAGE IS NOT THE REASON The instrumented build costs +1.24% / +1.39% / +1.89% across the three `full_frame` benches -- BELOW this project's own 3% adoption bar. Measured and recorded precisely because it shows performance was never the argument, and a reviewer's "severely degrading emulator performance" overstated it. The reason is that `irq-timing-trace` selects a DIFFERENT `for sub_dot in 0..3` loop in `Bus::tick_one_cpu_cycle` -- there are two, under opposite `cfg`s. So CI's `cargo test --workspace --release --features test-roms`, the accuracy battery, was validating a scheduler no user runs. That is the v2.3.4 defect repeating, where the coverage harness tested a load path no user runs, and it is wrong at any percentage. It does not affect the shipped binary: `cargo build --release -p rustynes-frontend` never selects this crate. Nor the perf gate, which is `cargo bench -p rustynes-core`. Exactly one gate was compromised, and it was the one that matters most. WHAT EXCLUSION COSTS, AND WHY NEITHER COST IS LEFT ON TRUST An excluded package cannot use `field.workspace = true`, so version, edition, rust-version, license, repository and both lint tables are duplicated. Duplication nothing checks is duplication that drifts -- and a crate quietly holding itself to weaker lints than the rest of the project is the kind of erosion nobody notices until it matters. `cosim_manifest_audit.rs` asserts every duplicated value still equals the workspace's, AND that the crate is still in `exclude` -- so re-adding it to `members` fails a test rather than silently restoring the unified build. Four mutations, all caught: re-added to members, a version drift, a weakened lint, an edition drift. `cargo fmt --all`, `clippy --workspace` and `test --workspace` no longer reach it, so CI gains three explicit steps naming its manifest. Without them the crate would simply stop being checked and nothing would say so. THE CLIPPY STEP EARNED ITS PLACE ON ITS FIRST RUN It reported a `must_use_candidate` on `Oracle::nes()` that `cargo clippy --workspace` had never surfaced. I do not have a confident explanation for why the standalone invocation sees it and the workspace one did not, and I would rather record that than invent one -- the useful fact is that the explicit step is not ceremony, it covers a real gap. VERIFIED The full gate matrix (fmt, workspace clippy, four frontend feature combos, both wasm32 combinations, no_std thumbv7em, rustdoc with warnings as errors), plus the excluded crate's own fmt / clippy / 14 tests, plus AccuracyCoin 141/141 (100.00%, RAM decoder) and nestest 0-diff on the SHIPPED path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

…#430) Promotes the accumulated CHANGELOG entries into a dated section, bumps the workspace version, and moves all 15 release anchors. Nothing else changes: no source file outside the two manifests is touched, so the shipped emulation core is the one that merged in #429. The release carries two versions, and that is not a naming convenience. v2.4.0 "Concordance" merged to main in #428 and was never tagged, so its entries were still sitting under [Unreleased] alongside v2.4.1's. Splitting them now would mean tagging v2.4.0 at a commit that main has already moved past, and publishing two releases minutes apart whose binaries are identical except for a version string. Instead the section states plainly that it carries both, and every entry belonging to the earlier one is marked (v2.4.0 item) so the attribution survives. Two entries belong to neither -- the release-anchor audit and the deferred-backlog sweep landed in #427, between the two -- and the preamble says so rather than letting "unmarked" silently mean v2.4.1. The version bump is deliberately part of this commit rather than an earlier one. Cargo.toml's own comment records why: release-auto.yml reads "workspace version has no matching tag" as "this is ready to release", so a bump that arrives early reaches main while the release does not exist and the workflow then fails closed on every push for want of release notes. Bumping here, with the CHANGELOG section and the release-notes override in the same change, is the only ordering that does not leave main red for a development window. Three mechanical consequences, each verified rather than assumed: - rustynes_libretro.info's display_version moves to v2.4.1, because libretro_info_audit.rs pins the LOCAL file against the workspace manifest. The cadence rule that lets display_version lag through a patch run applies to the UPSTREAM copy in libretro-super, which nothing here touches; the next upstream sync is still v2.5.0. - crates/rustynes-cosim/Cargo.toml carries its own version literal, because an excluded package cannot use `field.workspace = true`. cosim_manifest_audit.rs is what makes that duplication safe, and it is what would have caught this line being missed. - The CHANGELOG header is load-bearing, not decoration. release-auto.yml parses it for both the release-body fallback and the title codename, so the ` - <date> - "<Codename>" (<theme>)` shape is asserted by release_anchor_audit.rs rather than left to care. .github/release-notes/v2.4.1.md is a maintainer-authored override, so the published body is the narrative rather than the changelog. It discloses something the previous release's notes got wrong: v2.3.9's published body describes release_anchor_audit.rs, and `git cat-file -e v2.3.9:crates/rustynes-test-harness/tests/release_anchor_audit.rs` fails -- that work landed in #427, after the tag was cut. The audit exists because eight documents had reached six different answers about the current version; a release note describing work its own tag does not contain is the same failure one level up, so it is recorded here rather than quietly corrected. Anchors moved (15 across 10 documents, all named by the audit when it fails): README.md badge + Current Release, docs/STATUS.md, AGENTS.md x3 (the "What this is" block, the operating-notes bullet, and the never-claim-a-later-version guard), VERSION-PLAN.md header + its (current) table row, to-dos/ROADMAP.md, SUPPORT.md, SECURITY.md, ROADMAP.md x2, OVERVIEW.md x2, ARCHITECTURE.md. The VERSION-PLAN table gains a row for each of v2.4.0 and v2.4.1; the audit asserts exactly one row is marked (current), so a forgotten row fails loudly. Gates run on this tree, not inherited from the merge: fmt (workspace + the excluded crate), clippy --workspace --all-targets, the excluded crate's clippy, the four frontend feature combinations (scripting; scripting,hd-pack; retroachievements; full), BOTH wasm32 invocations, the no_std thumbv7em build, rustdoc with warnings as errors, cargo deny (advisories/bans/licenses/sources ok), actionlint, and markdownlint over every changed document. Tests: 126 workspace suites / 2222 passed / 0 failed, plus the excluded crate's 3 suites / 14 passed, which --workspace cannot reach. Accuracy re-run rather than reasoned about, since rustynes-core changed in both halves of what this tags: AccuracyCoin (RAM): pass rate = 100.00% over 141 assigned tests nestest_pc_c000_matches_golden_log ... ok The framebuffer decoder reports 120 and stays known-buggy; the RAM decoder is the authoritative one.

doublegate added 7 commits August 20, 2026 09:33

Copilot AI lite review requested due to automatic review settings August 20, 2026 13:35

Copilot started reviewing on behalf of doublegate August 20, 2026 13:35 View session

Copilot AI reviewed Aug 20, 2026

View reviewed changes

Comment thread crates/rustynes-frontend/src/atomic_write.rs Outdated

doublegate and others added 2 commits August 20, 2026 10:16

doublegate and others added 6 commits August 20, 2026 11:11

doublegate merged commit b67c4f9 into main Aug 20, 2026
29 checks passed

doublegate deleted the feat/v2.4.0-timeline-generation branch August 20, 2026 17:20

doublegate mentioned this pull request Aug 20, 2026

chore(release): cut v2.4.1 "Fabric", carrying the never-tagged v2.4.0 #430

Merged

Navigation Menu

Uh oh!

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

feat(v2.4.0): "Concordance" — atomic writes, the timeline counter, and the owed upstream sync#428
doublegate merged 20 commits into
mainfrom
feat/v2.4.0-timeline-generation

doublegate commented Aug 20, 2026

Uh oh!

coderabbitai Bot commented Aug 20, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

doublegate commented Aug 20, 2026

Uh oh!

doublegate commented Aug 20, 2026

Uh oh!

doublegate commented Aug 20, 2026

Uh oh!

doublegate commented Aug 20, 2026

Uh oh!

doublegate commented Aug 20, 2026

Uh oh!

doublegate commented Aug 20, 2026

Uh oh!

doublegate commented Aug 20, 2026

Uh oh!

doublegate commented Aug 20, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

doublegate commented Aug 20, 2026

Item C — one atomic, durable file write (marquee)

Item B — the timeline generation counter

Item D — two false byte-identity claims, now true

Item A — the upstream sync, filed

Accuracy

Gates

Uh oh!

coderabbitai Bot commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

doublegate commented Aug 20, 2026

Response to the Antigravity review

Response to the Copilot thread (scratch-name retry)

Uh oh!

doublegate commented Aug 20, 2026

Round 2 — the blocking issue was in the round-1 fix, and it was right

The first test for this did not test it

Nitpick — accepted, it was a factual error

Repeated suggestions — unchanged, reasons unchanged

Uh oh!

doublegate commented Aug 20, 2026

Round 3 — no blocking issues; both suggestions taken, and one of them found more

Mutating that second change found something the review did not

ENAMETOOLONG — valid, deferred, with the reasoning

Uh oh!

doublegate commented Aug 20, 2026

Round 5 — the blocking finding was right, and no local gate could have caught it

Dropping const would have been the smaller half of the fix

SYMLINK_DEPTH → 40 — accepted

Declined, with reasons

Uh oh!

doublegate commented Aug 20, 2026

Round 6 — the blocking finding was right, and my three deferrals were wrong on their facts

ARG_MAX — good catch, taken, slightly differently

Declined, unchanged

Uh oh!

doublegate commented Aug 20, 2026

Round 7 — no blocking issues. The nitpick was the useful finding; the three suggestions were already implemented.

timeline_generation and save states — real gap, closed

The three suggestions are already in the branch

The serde nitpick — checked, not assumed

EBADF = 9 — agreed, and already conditional

Uh oh!

doublegate commented Aug 20, 2026

Round 8 — the blocking finding was right, and it was my bug from round 2

Fail-safe for archives already written

cheats.rs redundant create_dir_all — taken

Uh oh!

doublegate commented Aug 20, 2026

Round 9 — no blocking issues. Two suggestions taken; getting one under test exposed two holes.

The retry budget — taken, on an unverified premise, stated as such

Two holes, both found by mutating

The panel error — wrapped, not truncated

Declined, with reasons

Uh oh!

github-actions Bot commented Aug 20, 2026

Antigravity review (Gemini via Ultra)

Blocking issues

Suggestions

Nitpicks

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Aug 20, 2026 •

edited

Loading

`ENAMETOOLONG` — valid, deferred, with the reasoning

Dropping `const` would have been the smaller half of the fix

`SYMLINK_DEPTH` → 40 — accepted

`ARG_MAX` — good catch, taken, slightly differently

`timeline_generation` and save states — real gap, closed

The `serde` nitpick — checked, not assumed

`EBADF = 9` — agreed, and already conditional

`cheats.rs` redundant `create_dir_all` — taken