From 75c38113d2a3fbf43f341855cc5ce7ea7bb47fa7 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 17 Aug 2026 21:04:27 -0400 Subject: [PATCH 1/7] fix(wasm): correct the iNES header in the browser, like every other load path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The web demo applied NONE of the per-game database's header corrections. Every mapper, submapper and region fix the vendored table ships was silently absent in the browser — Seicross, which needs submapper 4 to clear its protection loop, hung there exactly as it hung on the CLI before v2.3.4 fixed that half. THE MECHANISM, WHICH IS THE POINT This is the THIRD time this same correction has been skipped by a load path that does not pass through the File-menu chokepoint. The CLI skipped it until v2.3.4. The mapper-coverage harness skipped it until v2.3.4 — and that one was worse, because it meant the regression net was testing a load path no user ran, so fixes delivered through the database were invisible to it. Now the browser. `apply_load_time_header_overrides` has two stages: the game database (a table compiled into `rustynes-gamedb`), then the per-game `.json` overlay stacked on top. Only the SECOND stage needs a filesystem. The whole function was `cfg`-gated off wasm on its account, so the first stage — which needs nothing at all — went down with it. A `cfg` gate inherited from the strictest of several stages is a gate on the whole feature, and nothing tells you which stages did not need it. The database stage is now `apply_game_db_header_overrides`, ungated, returning the header-excluded CRC32 it looked up so the native caller can stack the overlay on it without an O(ROM) recompute of a value that cannot have changed. Both browser ROM entry points call it: the `wasm-winit` demo's `AppEvent::RomLoaded` arm and the `wasm-canvas` embed's file-picker closure. The overlay stage stays native-only, which is correct rather than a remaining gap — a browser has no `.json` to find. HOW IT WAS FOUND The audit v2.3.6 opened after Pixel Provenance: look for shipped features whose core logic is tested and whose frontend wiring is not. `rustynes-gamedb` is thoroughly tested; the question "which load paths call it" was not asked anywhere, and the answer turned out to be "not the browser's". And, as with Pixel Provenance, a comment asserted the opposite of the code. `load_and_preprocess_rom`'s doc said the wasm path "preprocesses separately" — it preprocessed nothing whatsoever. That sentence is corrected in place, quoting the old claim, rather than quietly deleted: the false reassurance is why nobody checked, so it is part of the record. TESTS Three, because the interesting failure mode here cannot be reached by behaviour. 1. `the_browser_path_applies_the_same_header_corrections_as_the_native_one` — the browser stage must produce byte-identical output to the full native helper when no overlay exists, which is always the browser's situation, and must return the same header-excluded CRC. 2. `a_header_correction_is_observable_so_the_agreement_test_is_not_vacuous` — the premise. Without it, test 1 would pass just as happily if BOTH sides were no-ops, which is exactly the state the browser was in. 3. `every_wasm_rom_entry_point_corrects_the_header` — a source-text assertion in the style of `snapshot_schema_audit.rs`. The two call sites live in `cfg`-gated wasm code a native test binary cannot execute or even link, so no behavioural test can reach them; what can be checked is that the call is PRESENT, and an absent call is precisely the bug being pinned. Mutation-checked: deleting the `wasm-canvas` call turns it red with the right message, and restoring it turns it green. VERIFICATION Frontend-only and native-behaviour-neutral (the native paths are refactored into the same two calls in the same order, and the existing v2.3.4 startup-vs-menu equivalence test still passes), so the deterministic core is untouched. Workspace clippy plus `scripting`, `scripting,hd-pack`, `retroachievements` and `full`; BOTH wasm32 clippy invocations (`wasm-winit` default and `wasm-canvas`); rustdoc with warnings denied; 510 frontend tests green. --- CHANGELOG.md | 35 ++++++ crates/rustynes-frontend/src/app.rs | 160 +++++++++++++++++++++++++-- crates/rustynes-frontend/src/wasm.rs | 9 +- 3 files changed, 195 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f55d9fea..55833671 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,41 @@ cycle-accurate core later replaced. ### Fixed +- **The browser demo applied no per-game header corrections.** Every mapper, + submapper and region fix the vendored game database ships was silently absent + on the web build — Seicross, which needs submapper 4 to clear its protection + loop, hung there exactly as it hung on the CLI before v2.3.4. + + The mechanism is the interesting part, because this is the **third** time the + same correction has been skipped by a load path that does not go through the + File-menu chokepoint: the CLI (fixed v2.3.4), the mapper-coverage harness + (fixed v2.3.4), and now the browser. `apply_load_time_header_overrides` has two + stages — the compiled-in game database, then the per-game `.json` overlay. + Only the *second* needs a filesystem, but the whole function was `cfg`-gated + off wasm on its account, so the first went down with it. **A `cfg` gate + inherited from the strictest of several stages is a gate on the whole feature, + and nothing tells you which stages did not need it.** + + The database stage is now its own function, ungated, called from both browser + ROM entry points (the `wasm-winit` demo's `AppEvent::RomLoaded` and the + `wasm-canvas` embed's file picker). The overlay stage stays native-only, which + is correct rather than a remaining gap: a browser has no `.json` to find. + + Found by the audit v2.3.6 opened after Pixel Provenance — *look for shipped + features whose core logic is tested and whose frontend wiring is not.* And, as + with Pixel Provenance, a comment asserted the opposite of the code: the wasm + path was documented as one that "preprocesses separately" when it preprocessed + nothing at all. That sentence is corrected in place, quoted, rather than + quietly deleted. + + Pinned three ways: the browser stage must produce byte-identical output to the + full native helper when no overlay exists; a premise test proves the correction + is observable at all, so the agreement test cannot pass vacuously (which is + precisely the state the browser was in); and a source-text assertion requires + every wasm ROM entry point to call it — mutation-checked — because those call + sites live in `cfg`-gated code a native test binary cannot link, so an absent + call is the one thing behaviour can never catch. + - **Pixel Provenance now works.** The v2.3.2 "Lucid" marquee returned an empty report for effectively every user, from release until now, because of two independent defects. diff --git a/crates/rustynes-frontend/src/app.rs b/crates/rustynes-frontend/src/app.rs index d4ae3617..68024602 100644 --- a/crates/rustynes-frontend/src/app.rs +++ b/crates/rustynes-frontend/src/app.rs @@ -242,8 +242,14 @@ fn extract_rom_from_zip(zip_bytes: &[u8]) -> Option<(String, Vec)> { /// image. Returns the processed bytes and a display label (the inner archive /// entry name when unzipped, else the file name). Used by `App::new` so a ROM /// passed on argv loads identically to one opened from the menu / drag-drop. -/// Native-only (the in-app menu path handles the running-app case; the wasm -/// `AppEvent::RomLoaded` path preprocesses separately). +/// Native-only (the in-app menu path handles the running-app case). The wasm +/// `AppEvent::RomLoaded` path does NOT run this: a browser file picker hands +/// over one file with no sibling `.bps`/`.ips` to find and no path to derive a +/// stem from, so zip extraction and soft-patching have nothing to act on there. +/// It does apply the header corrections, via +/// [`apply_game_db_header_overrides`] — which is the half that used to be +/// missing, and which this sentence previously papered over by claiming the +/// wasm path "preprocesses separately" when it preprocessed nothing at all. #[cfg(not(target_arch = "wasm32"))] fn load_and_preprocess_rom(rom_path: &Path) -> std::io::Result<(Vec, String)> { let mut bytes = std::fs::read(rom_path)?; @@ -306,8 +312,9 @@ fn load_and_preprocess_rom(rom_path: &Path) -> std::io::Result<(Vec, String) /// ROM that needs one -- Seicross needs submapper 4 to clear its protection /// loop -- worked one way and hung the other. /// -/// Native-only, like the startup path it exists for: `per_game::resolve` and -/// the filesystem overlay it reads are `cfg`-gated off wasm. +/// Only the SECOND stage is native-only. See +/// [`apply_game_db_header_overrides`] for why that distinction cost the +/// browser build every header correction for three releases. #[cfg(not(target_arch = "wasm32"))] fn apply_load_time_header_overrides(bytes: &mut [u8], path: Option<&std::path::Path>) { // One CRC for both stages. `rom_crc32` hashes PRG+CHR and excludes the @@ -315,12 +322,9 @@ fn apply_load_time_header_overrides(bytes: &mut [u8], path: Option<&std::path::P // key is stable across the first rewrite -- that stability is what lets the // overlay stack on the database correction at all. Recomputing it was an // O(ROM) pass that could not return a different answer. - let Some(crc) = crate::game_db::rom_crc32(bytes) else { + let Some(crc) = apply_game_db_header_overrides(bytes) else { return; }; - if let Some(entry) = crate::game_db::entry_for_crc(crc) { - crate::game_db::apply_header_overrides(bytes, &entry); - } if let Some(cfg) = crate::per_game::resolve(crc, path) && !cfg.overrides.is_empty() { @@ -329,6 +333,35 @@ fn apply_load_time_header_overrides(bytes: &mut [u8], path: Option<&std::path::P } } +/// Stage one on its own: the vendored per-game database's header corrections. +/// +/// Returns the header-excluded CRC32 the lookup used, so a caller that stacks a +/// second correction on top does not pay an O(ROM) pass to recompute a value +/// that cannot have changed. +/// +/// **Available on every target, and that is the point.** The database is a table +/// compiled into `rustynes-gamedb`; nothing about it needs a filesystem. Only the +/// *second* stage — the per-game `.json` overlay — is native-only, and +/// until v2.3.7 that distinction was lost: the whole function was `cfg`-gated +/// off wasm because one of its two halves was, so **the browser build applied no +/// header corrections at all**. Every mapper / submapper / region fix the +/// database ships was silently absent on the web demo — Seicross needs +/// submapper 4 to clear its protection loop and hung there, exactly as it hung +/// on the CLI before v2.3.4 fixed *that* half of the same asymmetry. +/// +/// This is the third time this correction has been skipped by a load path that +/// did not go through the File-menu chokepoint (CLI in v2.3.4, the coverage +/// harness in v2.3.4, the browser here). The lesson recorded with the fix: a +/// `cfg` gate inherited from the strictest of several stages is a gate on the +/// whole feature, and nothing tells you which stages did not need it. +pub(crate) fn apply_game_db_header_overrides(bytes: &mut [u8]) -> Option { + let crc = crate::game_db::rom_crc32(bytes)?; + if let Some(entry) = crate::game_db::entry_for_crc(crc) { + crate::game_db::apply_header_overrides(bytes, &entry); + } + Some(crc) +} + /// Hand the game-DB crate its overlay directory, then apply the load-time header /// corrections to the startup ROM -- in that order, which is the whole point. /// @@ -8940,6 +8973,22 @@ impl ApplicationHandler for App { AppEvent::GfxReady(gfx) => self.on_gfx_ready(*gfx, event_loop), AppEvent::RomLoaded(bytes) => { self.rom_bytes = bytes; + // v2.3.7 — apply the per-game database's header corrections + // BEFORE `start_nes` hands the bytes to `Nes::from_rom`, which + // is the only moment the header still matters. + // + // This arm is the browser's ONLY ROM entry point, and until now + // it did none of this: the helper it needed was `cfg`-gated off + // wasm because its second stage (the `.json` overlay) reads + // a filesystem, so the first stage — a compiled-in table that + // needs nothing — went with it. The native File-menu and CLI + // paths both correct the header; the web demo did not. + // + // Only the database stage runs here. The overlay stage has no + // meaning in a browser (there is no `.json` to find), so it + // is not a gap, and `apply_load_time_header_overrides` stays + // native-only for that reason alone. + let _ = apply_game_db_header_overrides(&mut self.rom_bytes); // Match the AudioContext's actual sample rate (set up // by `wasm_winit::start`'s file-picker gesture) so the // APU output needs no resampling. Falls back to @@ -10868,6 +10917,101 @@ mod tests { "the startup helper must produce the same header as the DB rewrite" ); } + + /// The browser's load path must correct the header the same way the native + /// one does. + /// + /// Regression for the third instance of one defect: a load path that does + /// not go through the File-menu chokepoint skips the per-game database's + /// header corrections. The CLI skipped them until v2.3.4; the coverage + /// harness skipped them until v2.3.4; the **browser** skipped them until + /// v2.3.7, because `apply_load_time_header_overrides` was `cfg`-gated off + /// wasm on account of its SECOND stage reading a filesystem — taking the + /// first stage, a compiled-in table that needs nothing, down with it. + /// + /// A browser never has a per-game `.json` overlay, so the database + /// stage alone must equal the full native helper with no overlay present. + #[test] + fn the_browser_path_applies_the_same_header_corrections_as_the_native_one() { + let rom = synth_ines_185(); + + let mut via_browser = rom.clone(); + let crc_browser = super::apply_game_db_header_overrides(&mut via_browser); + + let mut via_native = rom.clone(); + apply_load_time_header_overrides(&mut via_native, None); + + assert_eq!( + via_browser, via_native, + "the browser stage diverged from the native helper on a ROM with no overlay" + ); + assert_eq!( + crc_browser, + crate::game_db::rom_crc32(&rom), + "the returned CRC must be the header-excluded key the lookup used, so a \ + caller stacking a second correction need not recompute it" + ); + } + + /// Premise for the test above: the correction is observable at all. + /// + /// Without this, `the_browser_path_...` would pass just as happily if both + /// sides were no-ops — which is precisely the state the browser was in. + #[test] + fn a_header_correction_is_observable_so_the_agreement_test_is_not_vacuous() { + let rom = synth_ines_185(); + let crc = crate::game_db::rom_crc32(&rom).expect("iNES header parses"); + let entry = crate::game_db::GameDbEntry { + crc, + region: None, + mapper: Some(4), + submapper: Some(4), + mirroring: None, + title: String::new(), + }; + let mut rewritten = rom.clone(); + assert!( + crate::game_db::apply_header_overrides(&mut rewritten, &entry), + "the DB rewrite reported no change" + ); + assert_ne!(rewritten, rom, "the DB rewrite left the header untouched"); + } + + /// Every wasm ROM entry point must call the correction. + /// + /// A source-text assertion on purpose, in the style of + /// `snapshot_schema_audit.rs`: the two call sites live in `cfg`-gated wasm + /// code that a native test binary cannot execute or even link, so behaviour + /// cannot reach them. What CAN be checked is that the call is present — and + /// the bug being pinned was exactly an absent call, not a wrong one. + /// + /// If a third wasm ROM entry point is added, add it here. + #[test] + fn every_wasm_rom_entry_point_corrects_the_header() { + const APP_SRC: &str = include_str!("app.rs"); + const CANVAS_SRC: &str = include_str!("wasm.rs"); + + assert!( + APP_SRC.contains("apply_game_db_header_overrides(&mut self.rom_bytes)"), + "the `wasm-winit` demo's AppEvent::RomLoaded arm no longer corrects the header" + ); + assert!( + CANVAS_SRC.contains("apply_game_db_header_overrides(&mut bytes)"), + "the `wasm-canvas` embed's ROM loader no longer corrects the header" + ); + } + + /// Seicross (Japan): iNES 1.0, mapper 185, no submapper field of its own. + /// 32 KiB PRG + 8 KiB CHR so the header-excluded CRC is well defined. + fn synth_ines_185() -> Vec { + let mut rom = vec![0u8; 16 + 0x8000 + 0x2000]; + rom[0..4].copy_from_slice(b"NES\x1A"); + rom[4] = 2; // 32 KiB PRG + rom[5] = 1; // 8 KiB CHR + rom[6] = 0x90; // mapper low nibble 9 + rom[7] = 0xB0; // mapper high nibble B -> 185 + rom + } use super::{ extract_rom_from_zip, is_fds_image, is_nsf_image, load_and_preprocess_rom, nsf_header_strings, resolve_vs_dip, diff --git a/crates/rustynes-frontend/src/wasm.rs b/crates/rustynes-frontend/src/wasm.rs index a623a197..e19e97c0 100644 --- a/crates/rustynes-frontend/src/wasm.rs +++ b/crates/rustynes-frontend/src/wasm.rs @@ -150,7 +150,14 @@ fn install_rom_loader(rom_input: &HtmlInputElement) { return; }; let array = js_sys::Uint8Array::new(&buffer); - let bytes = array.to_vec(); + let mut bytes = array.to_vec(); + // v2.3.7 — the per-game database's header corrections, applied + // before the core parses the header. Same fix as the `wasm-winit` + // demo's `AppEvent::RomLoaded` arm: the helper used to be `cfg`-gated + // off wasm because a LATER stage of it reads a filesystem, so this + // stage — a compiled-in table needing nothing — was lost with it, and + // every mapper / submapper / region fix was absent in the browser. + let _ = crate::app::apply_game_db_header_overrides(&mut bytes); // The file-pick is a user gesture, so it's safe to create // the AudioContext here (the browser autoplay policy // requires a gesture). Create the Nes at the audio From 6e3b22a1e1403778141af15c46ac1f68d17b5884 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 17 Aug 2026 22:00:03 -0400 Subject: [PATCH 2/7] docs(changelog): re-home this entry under [Unreleased] after the v2.3.6 cut Merging `main` after the release SILENTLY DROPPED this entry, and reported no conflict while doing it. Worth writing down, because "mergeable: MERGEABLE" is what GitHub said right up to the moment the text disappeared. The release moved everything out of `[Unreleased]` into `[2.3.6]`, deleting the `### Fixed` heading this entry was anchored under. Git saw one side delete a region and the other side add a line inside it, resolved in favour of the deletion, and produced a clean tree. No marker, no warning -- the entry was simply gone from the merged CHANGELOG. Caught by inspecting `git merge-tree --write-tree` output before merging rather than trusting the mergeability flag; all three post-release fix branches were affected the same way. The fix is mechanical (re-anchor under a fresh `### Fixed` in the now-empty `[Unreleased]`), but the failure mode is not: a CHANGELOG entry is exactly the kind of content whose absence nothing downstream detects. --- CHANGELOG.md | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72cceb0c..231c0252 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,43 @@ cycle-accurate core later replaced. ## [Unreleased] +### Fixed + +- **The browser demo applied no per-game header corrections.** Every mapper, + submapper and region fix the vendored game database ships was silently absent + on the web build — Seicross, which needs submapper 4 to clear its protection + loop, hung there exactly as it hung on the CLI before v2.3.4. + + The mechanism is the interesting part, because this is the **third** time the + same correction has been skipped by a load path that does not go through the + File-menu chokepoint: the CLI (fixed v2.3.4), the mapper-coverage harness + (fixed v2.3.4), and now the browser. `apply_load_time_header_overrides` has two + stages — the compiled-in game database, then the per-game `.json` overlay. + Only the *second* needs a filesystem, but the whole function was `cfg`-gated + off wasm on its account, so the first went down with it. **A `cfg` gate + inherited from the strictest of several stages is a gate on the whole feature, + and nothing tells you which stages did not need it.** + + The database stage is now its own function, ungated, called from both browser + ROM entry points (the `wasm-winit` demo's `AppEvent::RomLoaded` and the + `wasm-canvas` embed's file picker). The overlay stage stays native-only, which + is correct rather than a remaining gap: a browser has no `.json` to find. + + Found by the audit v2.3.6 opened after Pixel Provenance — *look for shipped + features whose core logic is tested and whose frontend wiring is not.* And, as + with Pixel Provenance, a comment asserted the opposite of the code: the wasm + path was documented as one that "preprocesses separately" when it preprocessed + nothing at all. That sentence is corrected in place, quoted, rather than + quietly deleted. + + Pinned three ways: the browser stage must produce byte-identical output to the + full native helper when no overlay exists; a premise test proves the correction + is observable at all, so the agreement test cannot pass vacuously (which is + precisely the state the browser was in); and a source-text assertion requires + every wasm ROM entry point to call it — mutation-checked — because those call + sites live in `cfg`-gated code a native test binary cannot link, so an absent + call is the one thing behaviour can never catch. + ## [2.3.6] - 2026-08-17 - "Sounding" (measuring, and what a measurement may claim) A *sounding* is a depth measured with its uncertainty attached, and that is what From c81d86680693a06f56e1c66da9f3d55d8b332df5 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 17 Aug 2026 23:19:08 -0400 Subject: [PATCH 3/7] docs(changelog): remove the copy of this entry that landed inside [2.3.6] My own error, and the diagnosis that produced it is worth recording. Merging `main` after the v2.3.6 cut did not DROP this entry, as I concluded -- it MOVED it. The release relocated the whole `[Unreleased]` block into `[2.3.6]`, and git carried this branch's addition along with the block it was written inside. I checked for the entry only under `[Unreleased]`, saw nothing, called it dropped, and re-added it there. Two copies: one correctly under `[Unreleased]`, one wrongly inside a released section describing work that release does not contain. The check was too narrow, not wrong in kind: `git merge-tree --write-tree` was the right instrument and it did show the real merged content. I searched four lines of it instead of the whole file. Both review bots caught the duplicate. Removing the `[2.3.6]` copy; the `[Unreleased]` one stays, which is where a fix that ships in the NEXT release belongs. --- CHANGELOG.md | 35 ----------------------------------- 1 file changed, 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 231c0252..034c422b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -122,41 +122,6 @@ optimization campaign is closed on the strength of three measured rejections. ### Fixed -- **The browser demo applied no per-game header corrections.** Every mapper, - submapper and region fix the vendored game database ships was silently absent - on the web build — Seicross, which needs submapper 4 to clear its protection - loop, hung there exactly as it hung on the CLI before v2.3.4. - - The mechanism is the interesting part, because this is the **third** time the - same correction has been skipped by a load path that does not go through the - File-menu chokepoint: the CLI (fixed v2.3.4), the mapper-coverage harness - (fixed v2.3.4), and now the browser. `apply_load_time_header_overrides` has two - stages — the compiled-in game database, then the per-game `.json` overlay. - Only the *second* needs a filesystem, but the whole function was `cfg`-gated - off wasm on its account, so the first went down with it. **A `cfg` gate - inherited from the strictest of several stages is a gate on the whole feature, - and nothing tells you which stages did not need it.** - - The database stage is now its own function, ungated, called from both browser - ROM entry points (the `wasm-winit` demo's `AppEvent::RomLoaded` and the - `wasm-canvas` embed's file picker). The overlay stage stays native-only, which - is correct rather than a remaining gap: a browser has no `.json` to find. - - Found by the audit v2.3.6 opened after Pixel Provenance — *look for shipped - features whose core logic is tested and whose frontend wiring is not.* And, as - with Pixel Provenance, a comment asserted the opposite of the code: the wasm - path was documented as one that "preprocesses separately" when it preprocessed - nothing at all. That sentence is corrected in place, quoted, rather than - quietly deleted. - - Pinned three ways: the browser stage must produce byte-identical output to the - full native helper when no overlay exists; a premise test proves the correction - is observable at all, so the agreement test cannot pass vacuously (which is - precisely the state the browser was in); and a source-text assertion requires - every wasm ROM entry point to call it — mutation-checked — because those call - sites live in `cfg`-gated code a native test binary cannot link, so an absent - call is the one thing behaviour can never catch. - - **Pixel Provenance now works.** The v2.3.2 "Lucid" marquee returned an empty report for effectively every user, from release until now, because of two independent defects. From 7d4bc7d6496f2292ab1e480cb2ff9a8014ca5ebe Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Mon, 17 Aug 2026 23:37:49 -0400 Subject: [PATCH 4/7] docs(wasm): say precisely which preprocessing the browser path skips, and why Review findings, all three taken. THE DOC COMMENT CONFLATED TWO DIFFERENT REASONS It listed `.bps` / `.ips` and omitted `.ups`, and it grouped zip extraction with soft-patching as things with "nothing to act on" because there is no sibling file. Only half of that is true, and the half that is not is the more interesting one: * Soft-patching IS sibling-keyed -- a browser hands over one file's bytes with no directory to search and no path to derive a stem from, so there is genuinely nothing it could match. * Zip extraction is NOT sibling-dependent. A `.zip` picked in a browser is exactly as extractable as one on disk. It is simply not wired up there. Written out separately now. The first is a property of the platform; the second is an unimplemented feature, and a comment that files them under one heading means nobody ever notices the second is a gap. TWO IGNORED RETURN VALUES, NOW JUSTIFIED Both `let _ = apply_game_db_header_overrides(..)` sites say why: nothing stacks on the CRC there (the overlay stage is native-only), and `None` needs no handling because it means the bytes are not a parseable iNES image -- which `Nes::from_rom` reports properly a few lines later. THE SOURCE-TEXT ASSERTION WAS BRITTLE AGAINST rustfmt Correct, and worth fixing rather than weakening. The suggested repair -- assert on the function NAME alone -- would have made the test useless: the name appears in doc comments in both files, so it would stay green with every call site deleted. The source is whitespace-collapsed before matching instead, which survives any line-wrap while still asserting the ARGUMENT, so it still fails when a call disappears. That property is the whole reason the test exists. --- crates/rustynes-frontend/src/app.rs | 36 +++++++++++++++++++++------- crates/rustynes-frontend/src/wasm.rs | 3 +++ 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/crates/rustynes-frontend/src/app.rs b/crates/rustynes-frontend/src/app.rs index 68024602..c76c56fc 100644 --- a/crates/rustynes-frontend/src/app.rs +++ b/crates/rustynes-frontend/src/app.rs @@ -243,13 +243,21 @@ fn extract_rom_from_zip(zip_bytes: &[u8]) -> Option<(String, Vec)> { /// entry name when unzipped, else the file name). Used by `App::new` so a ROM /// passed on argv loads identically to one opened from the menu / drag-drop. /// Native-only (the in-app menu path handles the running-app case). The wasm -/// `AppEvent::RomLoaded` path does NOT run this: a browser file picker hands -/// over one file with no sibling `.bps`/`.ips` to find and no path to derive a -/// stem from, so zip extraction and soft-patching have nothing to act on there. -/// It does apply the header corrections, via -/// [`apply_game_db_header_overrides`] — which is the half that used to be -/// missing, and which this sentence previously papered over by claiming the -/// wasm path "preprocesses separately" when it preprocessed nothing at all. +/// `AppEvent::RomLoaded` path does NOT run this, and the two halves have +/// different reasons: +/// +/// * **Soft-patching** (`.bps` / `.ups` / `.ips`, that precedence) is keyed on a +/// same-stem sibling file. A browser file picker hands over one file's bytes +/// with no directory to look in and no path to derive a stem from, so there is +/// nothing it could match. +/// * **Zip extraction** is NOT sibling-dependent — a `.zip` selected in a +/// browser is as extractable as one on disk. It is simply not wired up on that +/// path yet. +/// +/// The header corrections DO apply there, via +/// [`apply_game_db_header_overrides`] — the half that used to be missing, and +/// which this sentence previously papered over by claiming the wasm path +/// "preprocesses separately" when it preprocessed nothing at all. #[cfg(not(target_arch = "wasm32"))] fn load_and_preprocess_rom(rom_path: &Path) -> std::io::Result<(Vec, String)> { let mut bytes = std::fs::read(rom_path)?; @@ -8988,6 +8996,10 @@ impl ApplicationHandler for App { // meaning in a browser (there is no `.json` to find), so it // is not a gap, and `apply_load_time_header_overrides` stays // native-only for that reason alone. + // The returned CRC is discarded because nothing stacks on it + // here (the overlay stage is native-only), and `None` needs no + // handling: it means the bytes are not a parseable iNES image, + // which `Nes::from_rom` reports properly a few lines below. let _ = apply_game_db_header_overrides(&mut self.rom_bytes); // Match the AudioContext's actual sample rate (set up // by `wasm_winit::start`'s file-picker gesture) so the @@ -10991,12 +11003,18 @@ mod tests { const APP_SRC: &str = include_str!("app.rs"); const CANVAS_SRC: &str = include_str!("wasm.rs"); + // Whitespace-collapsed before matching, so a rustfmt line-wrap cannot + // turn a still-present call into a phantom regression. Matching the + // ARGUMENT too, not just the function name, is deliberate: the name + // appears in doc comments on both files, so a name-only assertion would + // stay green with every call site deleted. + let squash = |src: &str| src.split_whitespace().collect::>().join(" "); assert!( - APP_SRC.contains("apply_game_db_header_overrides(&mut self.rom_bytes)"), + squash(APP_SRC).contains("apply_game_db_header_overrides(&mut self.rom_bytes)"), "the `wasm-winit` demo's AppEvent::RomLoaded arm no longer corrects the header" ); assert!( - CANVAS_SRC.contains("apply_game_db_header_overrides(&mut bytes)"), + squash(CANVAS_SRC).contains("apply_game_db_header_overrides(&mut bytes)"), "the `wasm-canvas` embed's ROM loader no longer corrects the header" ); } diff --git a/crates/rustynes-frontend/src/wasm.rs b/crates/rustynes-frontend/src/wasm.rs index e19e97c0..fecd3b73 100644 --- a/crates/rustynes-frontend/src/wasm.rs +++ b/crates/rustynes-frontend/src/wasm.rs @@ -157,6 +157,9 @@ fn install_rom_loader(rom_input: &HtmlInputElement) { // off wasm because a LATER stage of it reads a filesystem, so this // stage — a compiled-in table needing nothing — was lost with it, and // every mapper / submapper / region fix was absent in the browser. + // CRC discarded: nothing stacks on it here, and `None` just means + // the bytes are not a parseable iNES image -- which `Nes::from_rom` + // reports properly a few lines below. let _ = crate::app::apply_game_db_header_overrides(&mut bytes); // The file-pick is a user gesture, so it's safe to create // the AudioContext here (the browser autoplay policy From 4ff2ecf9d31f1152ce0dc202296e1d1c1052ae8f Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Tue, 18 Aug 2026 00:50:00 -0400 Subject: [PATCH 5/7] fix(wasm): drop a leaked conflict marker; record why `None` must not abort the load A LEAKED diff3 CONFLICT MARKER `CHANGELOG.md` carried a stray `||||||| 3f99a3dd`. My resolver checked for `<<<<<<<`, `=======` and `>>>>>>>` and reported zero markers left -- this repo sets `merge.conflictStyle = diff3`, which adds a fourth marker naming the merge BASE. I verified the wrong alphabet and believed the answer. The content was unaffected (the base section was empty), and all three post-release branches had the same leak. WHY THE IGNORED `Option` STAYS IGNORED Review called the discarded `apply_game_db_header_overrides` return blocking, and asked to "handle `None` and abort the load directly". That would be a real regression, so the reason is now in the code rather than in a review thread. `rom_crc32` returns `None` for anything without the `NES\x1A` magic. An FDS disk starts `FDS\x1A`. So `None` is the NORMAL answer for a format this very arm goes on to load successfully -- `start_nes` has a wasm FDS branch that builds from the uploaded BIOS. Aborting on `None` would make the browser refuse every FDS image. The comment previously said only that `Nes::from_rom` would report bad bytes later, which is true but reads as a justification for laziness. It now names the case that makes ignoring the value mandatory rather than merely acceptable. DECLINED, WITH REASONS * "Extract the wasm ROM init into a shared testable function instead of parsing your own source text." The brittleness was real and is fixed differently (the source is whitespace-collapsed before matching, so a rustfmt wrap cannot fake a regression). Extracting a helper does not remove the need for the assertion: the bug being pinned is a MISSING CALL in a `cfg`-gated event handler that a native test binary cannot link, so something still has to check that the handler calls the helper. That check is this test. * "The docstrings are extremely long and read like commit messages." Accepted as a matter of taste, declined as a change: this project's contributing rules ask for exactly this -- comments that explain the WHY alongside the mechanism, matching the density of the surrounding `rustynes-*` crates. The historical detail here is load-bearing rather than decorative: it records that the same correction has now been skipped by three different load paths, which is the fact that makes the next omission findable. --- CHANGELOG.md | 2 -- crates/rustynes-frontend/src/app.rs | 13 ++++++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a6efbfe..5c8fe53a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,8 +51,6 @@ cycle-accurate core later replaced. sites live in `cfg`-gated code a native test binary cannot link, so an absent call is the one thing behaviour can never catch. -||||||| 3f99a3dd - - **CI jobs are bounded, so a hung job can no longer block a release.** No job in `ci.yml` carried a `timeout-minutes`, which means every one inherited GitHub's **six-hour** default. On the night of the v2.3.6 cut the `lint` job — diff --git a/crates/rustynes-frontend/src/app.rs b/crates/rustynes-frontend/src/app.rs index c76c56fc..17121bd2 100644 --- a/crates/rustynes-frontend/src/app.rs +++ b/crates/rustynes-frontend/src/app.rs @@ -8997,9 +8997,16 @@ impl ApplicationHandler for App { // is not a gap, and `apply_load_time_header_overrides` stays // native-only for that reason alone. // The returned CRC is discarded because nothing stacks on it - // here (the overlay stage is native-only), and `None` needs no - // handling: it means the bytes are not a parseable iNES image, - // which `Nes::from_rom` reports properly a few lines below. + // here (the overlay stage is native-only). + // + // `None` MUST NOT abort the load. It means "not a parseable iNES + // image", and that includes the formats this arm legitimately + // goes on to load: `rom_crc32` requires the `NES\x1A` magic, so + // an FDS disk (`FDS\x1A`) returns `None` and is then handled by + // `start_nes`'s wasm FDS branch. Treating `None` as a failure + // would make the browser refuse every FDS image. A genuinely + // malformed cartridge is rejected by `Nes::from_rom` below, with + // a better message than this stage could produce. let _ = apply_game_db_header_overrides(&mut self.rom_bytes); // Match the AudioContext's actual sample rate (set up // by `wasm_winit::start`'s file-picker gesture) so the From 46c079568c37c760887d6bac1e90331c21772af7 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Tue, 18 Aug 2026 01:50:26 -0400 Subject: [PATCH 6/7] docs(wasm): record that the explicit-match form was tried and clippy refused it Review asked for `match` / `if let` instead of `let _ =` on `apply_game_db_header_overrides`, on the rule that ignored return values are silent failure paths. I implemented it. `clippy::single_match` rejects it under `-D warnings`: error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` and an `if let` with an empty body is the same objection one step along. Clippy is right: there is genuinely nothing to do in either arm, and `let _ =` is Rust's idiomatic way to say a value is deliberately discarded. So the two rules point opposite ways, and the enforced gate wins over the heuristic. What was missing was not a control-flow construct -- it was the REASON, which is now at both call sites: `None` must not abort the load, because `rom_crc32` requires the `NES\x1A` magic and an FDS disk starts `FDS\x1A`, so `None` is the normal answer for a format `start_nes`'s wasm FDS branch goes on to load successfully. Aborting would make the browser refuse every FDS image. Recorded rather than silently re-reverted, so the next reviewer sees that the suggestion was taken seriously and what stopped it. --- crates/rustynes-frontend/src/app.rs | 11 +++++++++++ crates/rustynes-frontend/src/wasm.rs | 4 ++++ 2 files changed, 15 insertions(+) diff --git a/crates/rustynes-frontend/src/app.rs b/crates/rustynes-frontend/src/app.rs index 17121bd2..c504e257 100644 --- a/crates/rustynes-frontend/src/app.rs +++ b/crates/rustynes-frontend/src/app.rs @@ -9007,6 +9007,17 @@ impl ApplicationHandler for App { // would make the browser refuse every FDS image. A genuinely // malformed cartridge is rejected by `Nes::from_rom` below, with // a better message than this stage could produce. + // `let _` is deliberate, and it is what clippy permits: an + // explicit `match`/`if let` on this was tried and rejected by + // `clippy::single_match` (the enforced gate), which is right -- + // there is genuinely nothing to do in either arm. + // + // `None` MUST NOT abort the load. `rom_crc32` requires the + // `NES\x1A` magic, so an FDS disk (`FDS\x1A`) returns `None` and + // is then handled by `start_nes`'s wasm FDS branch. Treating it + // as failure would make the browser refuse every FDS image. A + // genuinely malformed cartridge is rejected by `Nes::from_rom` + // below, with a better message than this stage could produce. let _ = apply_game_db_header_overrides(&mut self.rom_bytes); // Match the AudioContext's actual sample rate (set up // by `wasm_winit::start`'s file-picker gesture) so the diff --git a/crates/rustynes-frontend/src/wasm.rs b/crates/rustynes-frontend/src/wasm.rs index fecd3b73..4bd557f4 100644 --- a/crates/rustynes-frontend/src/wasm.rs +++ b/crates/rustynes-frontend/src/wasm.rs @@ -160,6 +160,10 @@ fn install_rom_loader(rom_input: &HtmlInputElement) { // CRC discarded: nothing stacks on it here, and `None` just means // the bytes are not a parseable iNES image -- which `Nes::from_rom` // reports properly a few lines below. + // `let _` is deliberate; an explicit `match` here is rejected by + // `clippy::single_match`. `None` means "not an iNES image" -- which + // includes formats this path legitimately handles -- so it must not + // abort the load; `Nes::from_rom` reports a genuinely malformed one. let _ = crate::app::apply_game_db_header_overrides(&mut bytes); // The file-pick is a user gesture, so it's safe to create // the AudioContext here (the browser autoplay policy From 8c400bf38ad0434918bbaa5e0da587a8614cf3e6 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Tue, 18 Aug 2026 04:57:07 -0400 Subject: [PATCH 7/7] test(wasm): the source-text assertion was searching its own source Review found that `every_wasm_rom_entry_point_corrects_the_header` could never fail for `APP_SRC`, and it is exactly right. The test lives in `app.rs`. `include_str!("app.rs")` therefore includes the text of the test itself -- including the string literal it searches for. So squash(APP_SRC).contains("apply_game_db_header_overrides(&mut self.rom_bytes)") was permanently true, whether or not the real call site in `AppEvent::RomLoaded` existed. The assertion that was supposed to pin an absent call was structurally incapable of detecting one. WHY THE MUTATION CHECK MISSED IT I did mutation-check this test. I deleted the `wasm.rs` call, saw it go red, and concluded the test worked. That exercised the CANVAS_SRC half only. The APP_SRC half -- the vacuous one -- was never mutated, so the check confirmed the half that was fine and said nothing about the half that was not. A mutation check proves the specific thing it mutates. Two assertions need two mutations, and I ran one. THE FIX `APP_SRC` is truncated at `\n#[cfg(test)]` before searching, so the test reads only production source. A second assertion checks that the truncation actually worked -- if the split ever fails, the test would silently go back to searching itself, which is the failure mode that got us here and deserves its own guard rather than trust. Both halves are now mutation-checked: deleting the `AppEvent::RomLoaded` call turns it red with the right message, and restoring it turns it green. A file that reads itself has to exclude the part doing the reading. --- CHANGELOG.md | 9 ++++++++- crates/rustynes-frontend/src/app.rs | 19 ++++++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c8fe53a..7230ec16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,7 +49,14 @@ cycle-accurate core later replaced. precisely the state the browser was in); and a source-text assertion requires every wasm ROM entry point to call it — mutation-checked — because those call sites live in `cfg`-gated code a native test binary cannot link, so an absent - call is the one thing behaviour can never catch. + call is the one thing behaviour can never catch. That third test was itself + **vacuous on first writing**, and review caught it: it lives in `app.rs`, so + `include_str!("app.rs")` pulled in the test's own source — which contains the + literal it searches for, making the assertion permanently true. It survived a + mutation check only because the check deleted the *other* file's call. A file + that reads itself has to exclude the part doing the reading; it now truncates + at the test module, asserts that the truncation worked, and is + mutation-checked on both halves. - **CI jobs are bounded, so a hung job can no longer block a release.** No job in `ci.yml` carried a `timeout-minutes`, which means every one inherited diff --git a/crates/rustynes-frontend/src/app.rs b/crates/rustynes-frontend/src/app.rs index c504e257..3f01bedb 100644 --- a/crates/rustynes-frontend/src/app.rs +++ b/crates/rustynes-frontend/src/app.rs @@ -11027,8 +11027,25 @@ mod tests { // appears in doc comments on both files, so a name-only assertion would // stay green with every call site deleted. let squash = |src: &str| src.split_whitespace().collect::>().join(" "); + + // CUT THE TEST MODULE OFF FIRST. This test lives in `app.rs`, so + // `include_str!("app.rs")` includes the text of this function -- and + // this function contains the very string literal it searches for. The + // `APP_SRC` assertion was therefore VACUOUS: permanently true, whether + // or not the real call site existed. It survived a mutation check + // because the check deleted the `wasm.rs` call, exercising only the + // other half. Caught in review. + // + // A file that reads itself has to exclude the part doing the reading. + let app_production = APP_SRC + .split_once("\n#[cfg(test)]") + .map_or(APP_SRC, |(before, _)| before); + assert!( + !squash(app_production).contains("fn every_wasm_rom_entry_point_corrects_the_header"), + "the test-module split failed, so this assertion is searching its own source again" + ); assert!( - squash(APP_SRC).contains("apply_game_db_header_overrides(&mut self.rom_bytes)"), + squash(app_production).contains("apply_game_db_header_overrides(&mut self.rom_bytes)"), "the `wasm-winit` demo's AppEvent::RomLoaded arm no longer corrects the header" ); assert!(