fix(wasm): correct the iNES header in the browser, like every other load path - #399
Conversation
…oad path
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 `<rom>.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
`<rom>.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.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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. Comment |
There was a problem hiding this comment.
Pull request overview
Ensures the wasm/browser ROM load paths apply the same per-game database iNES header corrections as the native load paths by factoring the “game DB” stage out of the native-only overlay stage, wiring it into both wasm entry points, and pinning the behavior with targeted tests.
Changes:
- Split the load-time header override logic into an always-available “game DB” stage (
apply_game_db_header_overrides) and a native-only overlay stage, then call the DB stage from both wasm ROM entry points. - Update documentation/comments to correctly describe wasm vs native preprocessing responsibilities and why they differ.
- Add regression tests to ensure the browser path’s header corrections match the native helper (when no overlay exists) and to source-assert that all wasm entry points perform the correction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/rustynes-frontend/src/wasm.rs | Applies game DB header corrections to the wasm-canvas file-picker bytes before Nes::from_rom. |
| crates/rustynes-frontend/src/app.rs | Introduces apply_game_db_header_overrides, refactors native helper to use it, wires it into AppEvent::RomLoaded, and adds regression tests. |
| CHANGELOG.md | Documents the wasm header-correction gap, mechanism, and test coverage added by this change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
….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.
…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.
… 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.
|
Antigravity review addressed. Blocking — CHANGELOG entry duplicated under Suggestion — comment the ignored Suggestion — the source-text assertion is brittle against rustfmt. The |
…ides # Conflicts: # CHANGELOG.md
…abort the load A LEAKED diff3 CONFLICT MARKER `CHANGELOG.md` carried a stray `||||||| 3f99a3d`. 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.
|
Antigravity review addressed. Blocking — leaked Blocking — "handle
You were right that the justification was too weak, though: the comment said only Suggestion — extract the wasm ROM init instead of parsing my own source. The Nitpick — docstrings read like commit messages. Accepted as taste, declined |
…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.
|
Blocking — and an What was actually missing was not a control-flow construct but the reason, Recorded in the commit rather than silently re-reverted, so the next reviewer can |
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.
|
Exactly right, and this is the best finding of the review. Fixed in the The test lives in And I had mutation-checked it. I deleted the The fix truncates Both halves are now mutation-checked: deleting the A file that reads itself has to exclude the part doing the reading. |
…ides # Conflicts: # CHANGELOG.md
Antigravity review (Gemini via Ultra)This PR extracts the ROM header override logic into a target-independent function so that WASM browser builds correctly apply the per-game database corrections. Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
Summary
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 #366 fixed
that half.
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.
rustynes-gamedbis thoroughly tested; the question "which load paths call it"had never been asked, and the answer was "not the browser's".
The mechanism
This is the third time this same correction has been skipped by a load path
that does not pass through the File-menu chokepoint:
wasm-winit+wasm-canvas)apply_load_time_header_overrideshas two stages — the game database (a tablecompiled into
rustynes-gamedb) and then the per-game<rom>.jsonoverlaystacked on top. Only the second needs a filesystem. The whole function was
cfg-gated off wasm on its account, so the first stage, which needs nothing atall, went down with it.
The database stage is now
apply_game_db_header_overrides, ungated, returningthe header-excluded CRC32 it looked up so the native caller stacks the overlay on
it without recomputing a value that cannot have changed. Both browser ROM entry
points call it. The overlay stage stays native-only — correct, not a remaining
gap, since a browser has no
<rom>.jsonto find.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" whenit preprocessed nothing whatsoever. Corrected in place, quoting the old claim,
rather than quietly deleted — the false reassurance is why nobody checked.
Tests
Three, because the interesting failure mode here cannot be reached by behaviour.
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 (always the browser's situation), and return the same
CRC.
a_header_correction_is_observable_so_the_agreement_test_is_not_vacuous—the premise. Without it, test 1 passes just as happily if both sides are
no-ops — precisely the state the browser was in.
every_wasm_rom_entry_point_corrects_the_header— a source-text assertionin the style of
snapshot_schema_audit.rs. Those call sites live incfg-gated wasm code a native test binary cannot execute or even link, so nobehavioural test can reach them; an absent call is exactly the bug being
pinned. Mutation-checked: deleting the
wasm-canvascall turns it redwith the right message.
Verification
Frontend-only and native-behaviour-neutral — the native paths are refactored into
the same two calls in the same order, and v2.3.4's startup-vs-menu equivalence
test still passes — so the deterministic core is untouched.
cargo clippy --workspace --all-targets -- -D warningsscripting/scripting,hd-pack/retroachievements/fullwasm-winit(default)wasm-canvasRUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-depscargo test -p rustynes-frontend