Correct the RustyNES core license (MIT/Apache -> GPL-3.0-or-later) by doublegate · Pull Request #1180 · libretro/docs

doublegate · 2026-08-16T04:14:59Z

RustyNES relicensed from MIT/Apache-2.0 to GPL-3.0-or-later in its v2.2.9 release (2026-08-04). This page predates that change and still lists the old terms.

Single-line change to the Author/License section:

 The RustyNES core is licensed under

-- MIT OR Apache-2.0
+- GPL-3.0-or-later

The matching dist/info correction is libretro-super#2069.

SPDX form used here deliberately. This page reads as prose, and the neighbouring library pages use full license names rather than the short .info tokens, so GPL-3.0-or-later fits the context. The info file uses libretro's own dialect (GPLv3+) for the same license.

I checked the rest of the page against the current core rather than only fixing the line I came for, and nothing else needs changing:

Extensions (.nes, .fds) match the core's retro_get_system_info declaration.
Databases and the disksys.rom BIOS entry (md5 ca30b50f880eb660a320674ed365ef7a) are unchanged and correct.
Features table is accurate as written. Worth noting Restart ✔ is newly true rather than newly documented — retro_reset was unimplemented and inherited the library's no-op default until v2.3.5, so the button did nothing before then. Core Options ✕ remains correct; core options are still unexposed.

RustyNES relicensed in its v2.2.9 release (2026-08-04). This page predates that change and still lists the old terms.

Copilot

Pull request overview

Updates the RustyNES library documentation to reflect the core’s relicensing, keeping the docs aligned with current upstream licensing.

Changes:

Update RustyNES core license in the Author/License section from MIT OR Apache-2.0 to GPL-3.0-or-later.

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

The cut. Workspace version 2.3.5 -> 2.3.6, the CHANGELOG [2.3.6] section, the maintainer-authored release body, STATUS.md, VERSION-PLAN.md, the README badge, AGENTS.md and CLAUDE.local.md. The version bump immediately tripped `libretro_info_audit`, which pins the local `.info`'s `display_version` against the workspace manifest. That is the guard working: it exists because v2.3.5 shipped a `.info` whose version had drifted from the crate for eleven days, and it caught this in one run. A gap it CANNOT close remains — RetroArch reads `libretro-super`'s copy, which will still say v2.3.5 after this release, so an upstream PR is a release-time step, on the same trigger the v2.2.9 licence drift set. Two claims in AGENTS.md were true when written and are now false; both are corrected. `libretro-super#2069` merged, so RetroArch reads `GPLv3+`, and `RetroArch#19416` merged, so `rustynes` is in `appstore_cores` — verified against `master` rather than the PR state. Being in the build list is not being installable: it arrives with the next App Store RetroArch build. Only `libretro/docs#1180` is still open. Six standing rules are added to AGENTS.md, each earned by a defect in this release rather than invented: a fix touching one call site of a shared path may not fix the bug and will report that it did; a test asserting "not empty" passes while a second defect remains; prose asserting an intent is how a defect survives releases; `ab_check.sh` benches the reference straight after a ~45-second fat-LTO compile, so read the order-bias control first; "inert on almost every cycle" predicts a win only if the work is actually executed; and the bot ceremony has a third hiding place in plain issue comments. A seventh covers panel state outliving the `Nes` it describes, now funnelled through one `clear_rom_bound_analysis` hook. The CHANGELOG and release notes also record the three defects the RAM Atlas review caught after the feature was written — the dead audio observable, the missing locked-session gate, and the `Inert` verdict for an address that was never perturbed. Verified: workspace clippy, 124 test binaries, version consistent across the manifest, the README badge and the libretro `.info`, and — though this commit touches no core code — AccuracyCoin 141/141 on the authoritative RAM decoder with nestest 0-diff.

…EADME Both files carried a "not fixed by this release" note from the v2.3.5 cut saying RetroArch shows the wrong licence and RustyNES is absent on iOS/iPadOS/tvOS. Both were true when written and both merged upstream on 2026-08-16 — `libretro-super#2069` and `RetroArch#19416`. The v2.3.6 blocks at the top of each file already said so, which made the files internally contradictory: a reader reaching the older passage would take the stale claim as current. Caught in review, and it is the same class as the AGENTS.md corrections in this cut — a release note is written in the present tense and then silently becomes history. Rewritten to say what is true now, including the distinction that matters: being in `appstore_cores` is not being installable. It arrives with the next App Store RetroArch build, on libretro's cadence. `libretro/docs#1180` is the one item still open.

* release: v2.3.6 "Sounding" — measuring, and what a measurement may claim The cut. Workspace version 2.3.5 -> 2.3.6, the CHANGELOG [2.3.6] section, the maintainer-authored release body, STATUS.md, VERSION-PLAN.md, the README badge, AGENTS.md and CLAUDE.local.md. The version bump immediately tripped `libretro_info_audit`, which pins the local `.info`'s `display_version` against the workspace manifest. That is the guard working: it exists because v2.3.5 shipped a `.info` whose version had drifted from the crate for eleven days, and it caught this in one run. A gap it CANNOT close remains — RetroArch reads `libretro-super`'s copy, which will still say v2.3.5 after this release, so an upstream PR is a release-time step, on the same trigger the v2.2.9 licence drift set. Two claims in AGENTS.md were true when written and are now false; both are corrected. `libretro-super#2069` merged, so RetroArch reads `GPLv3+`, and `RetroArch#19416` merged, so `rustynes` is in `appstore_cores` — verified against `master` rather than the PR state. Being in the build list is not being installable: it arrives with the next App Store RetroArch build. Only `libretro/docs#1180` is still open. Six standing rules are added to AGENTS.md, each earned by a defect in this release rather than invented: a fix touching one call site of a shared path may not fix the bug and will report that it did; a test asserting "not empty" passes while a second defect remains; prose asserting an intent is how a defect survives releases; `ab_check.sh` benches the reference straight after a ~45-second fat-LTO compile, so read the order-bias control first; "inert on almost every cycle" predicts a win only if the work is actually executed; and the bot ceremony has a third hiding place in plain issue comments. A seventh covers panel state outliving the `Nes` it describes, now funnelled through one `clear_rom_bound_analysis` hook. The CHANGELOG and release notes also record the three defects the RAM Atlas review caught after the feature was written — the dead audio observable, the missing locked-session gate, and the `Inert` verdict for an address that was never perturbed. Verified: workspace clippy, 124 test binaries, version consistent across the manifest, the README badge and the libretro `.info`, and — though this commit touches no core code — AccuracyCoin 141/141 on the authoritative RAM decoder with nestest 0-diff. * docs: correct two stale upstream claims further down STATUS and the README Both files carried a "not fixed by this release" note from the v2.3.5 cut saying RetroArch shows the wrong licence and RustyNES is absent on iOS/iPadOS/tvOS. Both were true when written and both merged upstream on 2026-08-16 — `libretro-super#2069` and `RetroArch#19416`. The v2.3.6 blocks at the top of each file already said so, which made the files internally contradictory: a reader reaching the older passage would take the stale claim as current. Caught in review, and it is the same class as the AGENTS.md corrections in this cut — a release note is written in the present tense and then silently becomes history. Rewritten to say what is true now, including the distinction that matters: being in `appstore_cores` is not being installable. It arrives with the next App Store RetroArch build, on libretro's cadence. `libretro/docs#1180` is the one item still open. * docs(libretro): batch upstream .info syncs to MINOR releases Maintainer decision at the v2.3.6 cut: the next upstream sync is v2.4.0. Recorded in three places because the alternative is that it reads as an oversight — which is exactly what the v2.2.9 incident was. RetroArch reads `dist/info/rustynes_libretro.info` from `libretro/libretro-super`, a separate copy nothing syncs automatically and nothing compares. Through the v2.3.6-v2.3.9 line it will therefore read `display_version = "v2.3.5"`. The standing `libretro_info_audit` pins the LOCAL file against the workspace manifest and the core's own `retro_get_system_info`, so the sync stays a copy rather than a re-derivation whenever it does happen; the audit cannot see upstream, so the divergence is invisible to CI by construction and has to be a written decision instead. The distinction that makes deferring safe is the one UPSTREAM_SYNC.md exists for. A stale `display_version` misreports a NUMBER. A stale `license` misreports the terms under which the software is distributed, which is what actually went wrong in v2.2.9 — RetroArch advertised a GPL-3.0-or-later emulator as MIT/Apache-2.0 for eleven days. So the existing rule stands unchanged: licence, supported extensions, and declared capabilities sync immediately regardless of where the version line sits. Only the version-bump-alone case is batched.

…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.

doublegate · 2026-08-20T12:50:46Z

Still applicable, and re-verified against master today: the page continues to read MIT OR Apache-2.0, and this is a one-line correction to GPL-3.0-or-later.

Cross-referencing for whoever picks this up — the same relicense has now been carried on the metadata side:

libretro/libretro-super#2069 (merged 2026-08-16) corrected license in dist/info/rustynes_libretro.info to GPLv3+.
libretro/libretro-super#2074 (opened today) brings that file's display_version up to v2.3.9.

So this docs page is the last surface still showing the pre-relicense terms. For context on why it matters rather than being cosmetic: RustyNES became GPL-3.0-or-later in v2.2.9 as a derivative work of GPL-licensed emulators, so the old text understates the obligations that attach to the core.

No rush intended — just noting it is unblocked and unchanged if it is easier to batch with other doc updates.

…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: RETURNS THE PULL REQUEST, since GitHub's issues endpoint serves PRs too. An earlier pass here ran exactly that, saw , and concluded the docs fix still needed filing. 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.

…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).

…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).

…d the owed upstream sync (#428) * feat(frontend): one atomic, durable file write for every path that persists 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. * fix(config): stop two shipped features writing empty tables into an untouched 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. * feat(core): a timeline generation counter, and the telemetry consumers 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 * docs(libretro): record the pending upstream sync as a diff, not a description 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. * docs(libretro): record both upstream PRs as filed, and the endpoint that 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. * docs(agents): the upstream-PR cadence, and eight findings from the v2.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). * docs(changelog): record v2.4.0 items A-D under [Unreleased] 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. * fix(frontend): three silent successes in the atomic-write path, and a 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 * fix(frontend): a failed write deleted a file it did not create 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 * fix(frontend): a third excused fsync errno, and the untested fallback 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 * fix(frontend): a contract that stopped being true, and a symlink chain 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 * ci(agy): the reviewer appends its rounds instead of destroying them 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 * ci(agy): SHA-pin the reviewer's checkout, adopted from the sibling repos 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 * ci(agy): adopt SLAC's backend-error guard and marker-based test extraction 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 * fix(ci): two CI failures I caused, and the gate claim that should have 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 * fix(frontend): a const fn that only fails on Windows, and the cfg that 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 * fix(frontend): the cheat save reports its failure instead of printing it 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 * docs(core): the timeline counter is session-local, and a test that says 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 * fix(ci): a <details> in a review body could tear the round it lives in 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…

Correct the RustyNES core license (MIT/Apache -> GPL-3.0-or-later)

2893846

RustyNES relicensed in its v2.2.9 release (2026-08-04). This page predates that change and still lists the old terms.

Copilot AI lite review requested due to automatic review settings August 16, 2026 04:14

Copilot started reviewing on behalf of doublegate August 16, 2026 04:15 View session

doublegate mentioned this pull request Aug 16, 2026

apple: add rustynes core to App Store builds libretro/RetroArch#19416

Merged

Copilot AI reviewed Aug 16, 2026

View reviewed changes

Navigation Menu

Uh oh!

Correct the RustyNES core license (MIT/Apache -> GPL-3.0-or-later) - #1180

Correct the RustyNES core license (MIT/Apache -> GPL-3.0-or-later)#1180
doublegate wants to merge 1 commit into
libretro:masterfrom
doublegate:rustynes-license-gplv3

doublegate commented Aug 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

doublegate commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

doublegate commented Aug 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

doublegate commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants