fix(frontend): report a failed latency-config save instead of swallowing it by doublegate · Pull Request #411 · doublegate/RustyNES

doublegate · 2026-08-19T18:30:51Z

Follow-up to the blocking finding on #410, which I merged before acting on. That was my error — the ceremony check and the merge were in the same command, so the finding and the merge landed together.

The finding was right, and sharper than the general rule

The style guide names swallowed errors as blocking. What was being hidden here is specifically the measurement silently not being persisted — a quieter version of the bug that block was written to fix.

Reported on two channels, because they reach different people:

stderr, matching the FDS BIOS path's existing convention for a config write that matters;
the panel's status line, for the user who is looking at the panel and nowhere else. They opened it to ask about this game; "your measurement was not saved" belongs beside the answer.

A second defect, found by looking at the failure path

take_unpersisted marks the value written before the caller's save runs — so a failed save was already being recorded as a success. That is now explicit rather than accidental: the failure is reported and the value stays marked, deliberately.

Un-marking would look more diligent and be worse. It retries on the very next frame, so a full disk or a read-only config becomes a per-frame write attempt and a per-frame log line — one failure amplified into a flood. The user has been told, the measurement is still on screen, and re-measuring is an explicit retry. Pinned by a test, because the tempting change is the one that silently degrades.

A comment that broke my own verification

The first version quoted the anti-pattern verbatim, and my verification grep then matched the comment rather than the code, reporting a swallowed save that no longer existed. Same lexical trap AGENTS.md records for closing keywords, where quoting the pattern reproduces it. The comment now describes the form instead of spelling it.

Verification

Two tests, two mutations — one for the failure being surfaced at all, one for it carrying the reason. Gates: fmt, workspace clippy, debug-hooks and full combos, both wasm32 invocations, rustdoc, frontend suite at 531.

No emulation-core file, no accuracy path — test-roms should be skipped.

coderabbitai · 2026-08-19T18:31:01Z

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: 49589aaa-e4ee-4fd2-a28a-6815bf9d2238

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 improves Latency Oracle persistence in the frontend by surfacing config-save failures (instead of silently ignoring them), ensuring users are explicitly informed when a measured result was not written to disk.

Changes:

Replace an ignored config.save() result with explicit error handling (stderr + panel status line).
Add LatencyPanel::note_persist_failure to display save failures in-panel without scheduling per-frame retries.
Add unit tests to pin the “failure is visible” and “no retry flood” behavior.

Reviewed changes

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

File	Description
crates/rustynes-frontend/src/debugger/mod.rs	Handles `config.save()` failures when persisting latency measurements and reports them via stderr + panel status.
crates/rustynes-frontend/src/debugger/latency_panel.rs	Adds a panel-visible persistence failure message and tests covering visibility + non-retry behavior.

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

doublegate · 2026-08-19T19:50:27Z

Antigravity review addressed — one accepted, one declined with reasoning.

Accepted (nitpick): impl core::fmt::Display over &dyn. Done in 3e8537bb; the two test call sites drop their &. The dynamic form bought nothing here — one production call site on a cold path, so there is no code size to trade away.

Declined (suggestion): removing the review-provenance notes. This is a deliberate and long-standing convention in this repo, not incidental meta-commentary. Examples already on main:

atlas_panel.rs: "A bare 2 in the budget with a stale number in the tooltip is the drift this prevents. (PR feat: the RAM Atlas — classify work RAM, then verify by perturbation #392 review.)"
.github/scripts/apt-install-retry.sh: "Review on ci(v2.3.9): open "Crucible" — what the gates actually cover #408 caught the ordering and it is not cosmetic" — followed by the mechanism (SIGTERM to the elevation helper orphaning apt-get on the dpkg lock).
Same file: "(Review on feat(perf): measure the two-acquisition race, and land the apt fix #408 missed #409; both reviewers raised it independently.)"

What these record is not that a review happened; it is why a non-obvious constraint exists and where the evidence for it is. The sudo env line looks like a pointless indirection until you know it is about SETENV in sudoers; the citation is what lets a future author check the claim instead of re-deriving it — and, in this project's recent history, what stops a "simplification" that quietly reintroduces the defect.

The general rule you are applying is a good one where the note carries no information. Here the note is the information.

Also for the record, since it affects the check list: CodeRabbit does not auto-review this repository (its comment says so and offers a manual trigger), so its check sits pending indefinitely rather than being an outstanding review.

doublegate · 2026-08-19T19:51:24Z

@coderabbitai review

coderabbitai · 2026-08-19T19:51:39Z

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…k never resolves Found while working #411. CodeRabbit's own comment gives the reason: this repository does not receive automatic reviews because it has fewer than 10 stars. The configuration from #316 is loaded and correct -- it reports the .coderabbit.yaml path, the ASSERTIVE profile and the Pro Plus plan -- so only the automatic trigger is missing. Two consequences, both of which have been operated wrongly since #316. The bot ceremony silently covers two bots rather than three. AGENTS.md credits CodeRabbit with catching a critical fast-forward defect (#358) and a use-after-free in the v2.3.5 libretro controller tables. None of that arrives unless a review is explicitly requested, so a PR reviewed by "all three bots" has in fact been reviewed by Copilot and Antigravity. `@coderabbitai review` requests one. And waiting for zero pending checks never terminates. The CodeRabbit context sits with a null status and a null conclusion indefinitely, so a healthy PR here reads as roughly 27 checks with exactly one permanently pending. The merge criterion is `CI success` = SUCCESS with every OTHER check complete -- never "nothing pending", and never a bare check count either, since a CONFLICTING PR also shows a short list because CI cannot run on it at all.

…ing it Review flagged the ignored `save()` result as blocking — the style guide names swallowed errors explicitly — and it was right for a reason sharper than the general rule: the thing being hidden is the measurement silently not being persisted, which is a quieter version of the bug that block was written to fix. I merged before acting on the finding, so this is the follow-up. Two channels, because they reach different people. Stderr, matching the FDS BIOS path's existing convention for a config write that matters, and the panel's own status line for the user who is looking at the panel and nowhere else — they opened it to ask about this game, so "your measurement was not saved" belongs beside the answer. Looking at the failure path turned up a second defect of my own in the same block. `take_unpersisted` marks the value written BEFORE the caller's save runs, so a failed save was already recorded as a success. That is now explicit rather than accidental: the failure is reported and the value STAYS marked, deliberately. Un-marking would look more diligent and be worse. It would retry on the very next frame, so a full disk or a read-only config becomes a per-frame write attempt and a per-frame log line — one failure amplified into a flood. The user has been told, the measurement is still on screen, and re-measuring is an explicit retry. Both halves are pinned by tests, including the one asserting no retry, because the tempting change is the one that silently degrades. Also reworded a comment. It quoted the anti-pattern verbatim, and my own verification grep then matched the COMMENT rather than the code and reported a swallowed save that no longer existed. The same lexical trap `AGENTS.md` records for closing keywords, where quoting the pattern reproduces it — so the comment now describes the form instead of spelling it. Verified: fmt, workspace clippy, `debug-hooks` and `full` combos, both wasm32 invocations, rustdoc, frontend suite at 531. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…ing the anti-pattern Two review findings, both accepted. Copilot: the test's doc comment still contained the literal discard expression, which is the exact string this PR's own description records as having matched the COMMENT rather than any code during verification — reporting a defect that was already fixed. Removed, with the reason written down in its place so the next author does not helpfully restore it for clarity. Antigravity: `impl core::fmt::Display` is the more idiomatic signature than `&dyn core::fmt::Display` here, and it lets the two test call sites drop a `&` around a string literal. The dynamic form bought nothing -- there is one production call site on a cold path, so there is no code size to trade away.

…k never resolves Found while working #411. CodeRabbit's own comment gives the reason: this repository does not receive automatic reviews because it has fewer than 10 stars. The configuration from #316 is loaded and correct -- it reports the .coderabbit.yaml path, the ASSERTIVE profile and the Pro Plus plan -- so only the automatic trigger is missing. Two consequences, both of which have been operated wrongly since #316. The bot ceremony silently covers two bots rather than three. AGENTS.md credits CodeRabbit with catching a critical fast-forward defect (#358) and a use-after-free in the v2.3.5 libretro controller tables. None of that arrives unless a review is explicitly requested, so a PR reviewed by "all three bots" has in fact been reviewed by Copilot and Antigravity. `@coderabbitai review` requests one. And waiting for zero pending checks never terminates. The CodeRabbit context sits with a null status and a null conclusion indefinitely, so a healthy PR here reads as roughly 27 checks with exactly one permanently pending. The merge criterion is `CI success` = SUCCESS with every OTHER check complete -- never "nothing pending", and never a bare check count either, since a CONFLICTING PR also shows a short list because CI cannot run on it at all.

github-actions · 2026-08-19T21:34:45Z

Antigravity review (Gemini via Ultra)

This PR fixes a silent failure path by surfacing latency config save errors to both stderr and the latency panel UI, while explicitly preventing infinite retry loops on failure.

Blocking issues

None found.

Suggestions

The reasoning for not un-marking the value as persisted upon failure is sound and prevents a potential log/I/O flood on every frame. The tests adequately cover both the visibility of the error and the prevention of the retry loop. No further changes needed.

Nitpicks

None.

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

…k never resolves Found while working #411. CodeRabbit's own comment gives the reason: this repository does not receive automatic reviews because it has fewer than 10 stars. The configuration from #316 is loaded and correct -- it reports the .coderabbit.yaml path, the ASSERTIVE profile and the Pro Plus plan -- so only the automatic trigger is missing. Two consequences, both of which have been operated wrongly since #316. The bot ceremony silently covers two bots rather than three. AGENTS.md credits CodeRabbit with catching a critical fast-forward defect (#358) and a use-after-free in the v2.3.5 libretro controller tables. None of that arrives unless a review is explicitly requested, so a PR reviewed by "all three bots" has in fact been reviewed by Copilot and Antigravity. `@coderabbitai review` requests one. And waiting for zero pending checks never terminates. The CodeRabbit context sits with a null status and a null conclusion indefinitely, so a healthy PR here reads as roughly 27 checks with exactly one permanently pending. The merge criterion is `CI success` = SUCCESS with every OTHER check complete -- never "nothing pending", and never a bare check count either, since a CONFLICTING PR also shows a short list because CI cannot run on it at all.

…k never resolves (#416) * docs(agents): CodeRabbit does not auto-review this repo, and its check never resolves Found while working #411. CodeRabbit's own comment gives the reason: this repository does not receive automatic reviews because it has fewer than 10 stars. The configuration from #316 is loaded and correct -- it reports the .coderabbit.yaml path, the ASSERTIVE profile and the Pro Plus plan -- so only the automatic trigger is missing. Two consequences, both of which have been operated wrongly since #316. The bot ceremony silently covers two bots rather than three. AGENTS.md credits CodeRabbit with catching a critical fast-forward defect (#358) and a use-after-free in the v2.3.5 libretro controller tables. None of that arrives unless a review is explicitly requested, so a PR reviewed by "all three bots" has in fact been reviewed by Copilot and Antigravity. `@coderabbitai review` requests one. And waiting for zero pending checks never terminates. The CodeRabbit context sits with a null status and a null conclusion indefinitely, so a healthy PR here reads as roughly 27 checks with exactly one permanently pending. The merge criterion is `CI success` = SUCCESS with every OTHER check complete -- never "nothing pending", and never a bare check count either, since a CONFLICTING PR also shows a short list because CI cannot run on it at all. * docs(agents): state the conflicting-PR trap inline, and why removing the check is not the fix Two review findings, both correct in what they point at. Copilot: the parenthetical pointed at a "memory index" that does not exist in this repository -- it referenced a private note a contributor cannot see, which makes the sentence unactionable exactly where it is meant to be actionable. The trap is now stated inline: a CONFLICTING PR shows a short check list because GitHub cannot build a merge ref so CI never runs, and that is indistinguishable from a healthy PR whose jobs are still queuing. The merge criterion is spelled out as three conditions rather than as a cross-reference. Antigravity proposed removing the CodeRabbit check from branch protection instead of documenting it. Checked before answering: `main`'s ruleset requires exactly ONE context, `CI success` (`gh api repos/OWNER/REPO/rules/branches/main`), so the pending CodeRabbit context is not required and blocks nothing -- there is nothing to remove. Suppressing its status reporting would also lose the check on the PRs where a review IS triggered, which is the one occasion it carries information. Both facts are now in the entry so the next reader does not re-propose it. Also records what this session found the hard way: CodeRabbit rate-limits manual triggers per developer, so a batch of `@coderabbitai review` comments can produce no reviews at all while looking like it worked. * docs(agents): correct the older bullet instead of contradicting it two lines later Review caught something this PR did to itself. The bullet directly above the new one has said, since #316, that CodeRabbit is a "3rd AUTOMATED PR review bot" whose ceremony "applies before any merge". The new bullet says it does not auto-review and that the ceremony covers two bots. Both were left standing, adjacent, so a reader arriving at the first one gets the false claim and the correction only if they keep going. That is the same defect class this release keeps finding -- prose asserting a behaviour the system does not have -- reproduced while documenting an instance of it. Corrected at the source: the #316 bullet now says CodeRabbit is CONFIGURED as a third bot, that the ceremony applies to its threads, and that it does not review automatically, pointing at the bullet with the detail. It also records what the old wording cost: "automated" plus "applies before any merge" read as a promise that its findings were arriving, and they were not.

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

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

Copilot AI lite review requested due to automatic review settings August 19, 2026 18:30

Copilot started reviewing on behalf of doublegate August 19, 2026 18:31 View session

Copilot AI reviewed Aug 19, 2026

View reviewed changes

Comment thread crates/rustynes-frontend/src/debugger/latency_panel.rs Outdated

doublegate mentioned this pull request Aug 19, 2026

docs(agents): CodeRabbit does not auto-review this repo, and its check never resolves #416

Merged

doublegate force-pushed the fix/persist-save-error branch from 3e8537b to d9d0d7d Compare August 19, 2026 21:06

doublegate and others added 2 commits August 19, 2026 17:33

doublegate force-pushed the fix/persist-save-error branch from d9d0d7d to a3cb2d8 Compare August 19, 2026 21:33

doublegate merged commit 7902a21 into main Aug 19, 2026
25 checks passed

doublegate deleted the fix/persist-save-error branch August 19, 2026 21:55

doublegate mentioned this pull request Aug 20, 2026

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

Merged

Navigation Menu

Uh oh!

fix(frontend): report a failed latency-config save instead of swallowing it - #411

fix(frontend): report a failed latency-config save instead of swallowing it#411
doublegate merged 2 commits into
mainfrom
fix/persist-save-error

doublegate commented Aug 19, 2026

Uh oh!

coderabbitai Bot commented Aug 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

doublegate commented Aug 19, 2026

Uh oh!

doublegate commented Aug 19, 2026

Uh oh!

coderabbitai Bot commented Aug 19, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

doublegate commented Aug 19, 2026

The finding was right, and sharper than the general rule

A second defect, found by looking at the failure path

A comment that broke my own verification

Verification

Uh oh!

coderabbitai Bot commented Aug 19, 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 19, 2026

Uh oh!

doublegate commented Aug 19, 2026

Uh oh!

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

Uh oh!

Uh oh!

github-actions Bot commented Aug 19, 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 19, 2026 •

edited

Loading

coderabbitai Bot commented Aug 19, 2026 •

edited

Loading