docs(cart): the copier-strip rule is % 32768, not the % 1024 the dossier quotes - #318
Conversation
…ier quotes Checked while looking at whether G1.18 was coverable. RustySNES tests `rom.len() % 0x8000 == 0x200`; the dossier (and the folk rule, and nocash) say `filesize % 1024 == 512`. Those are not the same predicate. Measured against the references rather than assumed: ares/bsnes (mia/medium/super-famicom.cpp:222) test `(rom.size() & 0x7fff) == 512`, which is exactly what RustySNES does. snes9x takes a third, looser route (`size - calc_size == 512` against a rounded size). The two rules agree for every real cartridge image, because a stripped image is a multiple of 32 KiB. They differ only for an odd-sized dump, where the stricter rule refuses to strip rather than shifting an image that was never prefixed. So this is a documentation finding, not a defect -- and the comment says DO NOT "fix" it to match the quoted rule, because that would diverge from both bsnes-lineage references to match prose no reference implements. The dossier row now carries the same correction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughThe change documents the copier-prefix detection modulus rule in ChangesCopier-prefix rule documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
Antigravity review (Gemini via Ultra)This PR updates code comments in Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Pull request overview
Clarifies the “copier header strip” predicate used by RustySNES (and reference emulators) so contributors don’t “fix” it to the commonly-quoted but different % 1024 == 512 folklore rule. This is a documentation/comment correction intended to prevent future behavior regressions in ROM header detection.
Changes:
- Updates the AccuracySNES research dossier entry for
G1.18to document that references implement the stricter% 32768 == 512predicate (equivalent tosize % 0x8000 == 0x200). - Adds an in-code comment in
Header::detectexplaining the rationale and reference provenance for the existing predicate, explicitly warning against changing it to match the folklore rule.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/accuracysnes-research-dossier.md | Corrects/extends G1.18 documentation to distinguish the folklore rule from the stricter reference-implemented predicate. |
| crates/rustysnes-cart/src/header.rs | Adds a detailed comment documenting why copier-prefix stripping uses len % 0x8000 == 0x200, with reference provenance and a “don’t change this” warning. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rustysnes-cart/src/header.rs`:
- Around line 182-185: Replace “odd-sized dump” with “non-32 KiB-aligned dump”
in crates/rustysnes-cart/src/header.rs lines 182-185 and update the
corresponding G1.18 terminology in docs/accuracysnes-research-dossier.md line
1402; keep the code and documentation descriptions consistent.
In `@docs/accuracysnes-research-dossier.md`:
- Line 1402: Update the G1.18 documentation to describe the predicates
accurately: replace “odd-sized dump” with “non-32 KiB-aligned dump,” or
explicitly state both size conditions and their divergence. Keep the documented
behavior aligned with rustysnes-cart::header::detect, including even sizes such
as 0x8600.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 57d4249a-cb5d-402e-b640-81e24c99ce82
📒 Files selected for processing (2)
crates/rustysnes-cart/src/header.rsdocs/accuracysnes-research-dossier.md
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: accuracysnes
- GitHub Check: lint
- GitHub Check: test-light
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (12)
docs/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before changing a subsystem, consult
docs/architecture.md,docs/STATUS.md,CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.New subsystems must add documentation under
docs/.
Files:
docs/accuracysnes-research-dossier.md
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.A chip change must update both the chip implementation and its corresponding
docs/<chip>.mddocumentation in the same change.
Files:
docs/accuracysnes-research-dossier.mdcrates/rustysnes-cart/src/header.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
docs/accuracysnes-research-dossier.mdcrates/rustysnes-cart/src/header.rs
docs/**/*
📄 CodeRabbit inference engine (docs/testing-strategy.md)
Chip crates should exceed 90% unit-test coverage, and each chip should be fuzzable in isolation.
Files:
docs/accuracysnes-research-dossier.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/accuracysnes-research-dossier.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
docs/accuracysnes-research-dossier.md
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Busowns mutable machine state, and the CPU borrows&mut Bus.
Preserve determinism: seed, ROM, and input must produce bit-identical output.
Treat test ROMs as the behavioral specification; when documentation disagrees with passing ROM behavior, update the documentation.
Keepunsafeconfined to existing allowed areas, namely frontend and FFI code, and document everyunsafeblock with a// SAFETY:comment.
Files:
crates/rustysnes-cart/src/header.rs
crates/rustysnes-{cpu,ppu,apu,cart,core}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep core chip implementation changes localized to the owning chip crate and preserve the workspace crate boundaries.
Files:
crates/rustysnes-cart/src/header.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use Rust edition 2024 and the toolchain pinned inrust-toolchain.toml(Rust 1.96).
Runcargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy withcargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc becausemissing_docsis a workspace lint.
Do not runcargo clippy --all-features;scriptingandscript-wasmare mutually exclusive. Use explicit per-feature jobs instead.
**/*.rs: Do not introduce.unwrap(),.expect(), orpanic!()on untrusted external input—such as ROM/save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths—outside#[cfg(test)]code. Use typed errors at those boundaries; locally constructed values or values immediately protected by a checked invariant are allowed.
Every newunsafe { ... }block orunsafe fnmust have an adjacent// SAFETY:comment naming the relied-on invariant and its guarantor. Unsafe code outside the frontend and FFI shims should additionally be questioned becauseunsafe_codeis a workspace lint.
**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspacepedantic,nursery,missing_docs, andunsafe_codewarnings because CI runs with-D warnings. Document every public item.
Keepunsafecode restricted to the frontend and FFI, and include a// SAFETY:justification for each use.
Keep hot paths allocation-free.
Treatrustysnes_core::Busas the owner of mutable emulator state; the CPU borrows&mut Bus.
Use the master clock at 21477270 Hz as the timing master; advance the scheduler in lockstep and run other chips on their divisors.
Maintain determinism: seed, ROM, and input must produce bit-identical audio/video; frontend rate control must not alter emulation results.
When implementing hardware behavior, pin and run the failing test ROM first; treat test ROMs as the specification.
Files:
crates/rustysnes-cart/src/header.rs
crates/rustysnes-*/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, any observable behavior change under
crates/rustysnes-<chip>/must be accompanied by an edit to the matchingdocs/<chip>.md; a crate change passes without documentation only when it does not alter observable behavior, with the non-behavioral change stated explicitly.
Files:
crates/rustysnes-cart/src/header.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Additive features must be default-off so shipped/native,no_std, and wasm builds remain byte-identical.
Never use or configure--all-features; validate opt-in feature combinations individually as required by the project recipe.
Files:
crates/rustysnes-cart/src/header.rs
crates/**
⚙️ CodeRabbit configuration file
crates/**: Emulator core. Hot paths are allocation-free;unsaferequires a// SAFETY:comment
naming the invariant. Any change to save-stated fields needs aFORMAT_VERSIONbump and a
docs/adr/0006bump-log entry. Behavior changes must update the matchingdocs/<chip>.md
in the same change.
Files:
crates/rustysnes-cart/src/header.rs
🧠 Learnings (2)
📚 Learning: 2026-07-21T01:34:22.909Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 189
File: docs/accuracysnes-plan.md:0-0
Timestamp: 2026-07-21T01:34:22.909Z
Learning: When reviewing the AccuracySNES documentation in docs/accuracysnes-*.md (notably docs/accuracysnes-plan.md vs the generated docs/accuracysnes-coverage.md), treat the reported metrics as intentionally non-equivalent: the battery test count and dossier assertion coverage are not interchangeable. Do not infer one count/coverage from the other during review (e.g., one test may contain multiple assertions, and multiple tests may contribute to a single assertion/row such as E6.02).
Applied to files:
docs/accuracysnes-research-dossier.md
📚 Learning: 2026-07-21T05:22:58.848Z
Learnt from: doublegate
Repo: doublegate/RustySNES PR: 197
File: docs/accuracysnes-plan.md:598-600
Timestamp: 2026-07-21T05:22:58.848Z
Learning: In the AccuracySNES documentation under `docs/`, when an assertion exists in the research dossier but cannot be measured/verified by the current cartridge timing test, distinguish the dossier assertion from test measurability: keep the original hardware assertion (and any contribution to the coverage denominator) intact, withdraw/stop using the specific test coverage only if the sources cannot decompose the required CPU-cycle timing into bus vs internal components, and mark the row as not measurable using the `[NOT CART-MEASURABLE ...]` annotation with links to the corresponding plan section (e.g., `docs/accuracysnes-plan.md` §A5.20) and the related roadmap/ticket (e.g., `to-dos/ROADMAP.md` ticket `T-06-A`). Ensure the documentation/coverage reporting treats the row as uncovered rather than removing or redefining the assertion.
Applied to files:
docs/accuracysnes-research-dossier.md
🔇 Additional comments (1)
crates/rustysnes-cart/src/header.rs (1)
178-181: LGTM!Also applies to: 186-188
| // this. The two rules agree for every real cartridge image, because a stripped image is a | ||
| // multiple of 32 KiB; they differ only for an odd-sized dump, where the stricter rule | ||
| // refuses to strip rather than shifting an image that was never prefixed. snes9x takes a | ||
| // third, looser route (`size - calc_size == 512` against a rounded size). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe the divergence as non-32 KiB alignment.
The two predicates differ when filesize % 1024 == 512 but filesize % 32768 != 512. For example, 0x8600 bytes is an even length that triggers this divergence.
crates/rustysnes-cart/src/header.rs#L182-L185: replace “odd-sized dump” with “non-32 KiB-aligned dump”.docs/accuracysnes-research-dossier.md#L1402-L1402: use the same terminology in G1.18.
As per path instructions, docs are the spec and must not drift from the code they describe.
📍 Affects 2 files
crates/rustysnes-cart/src/header.rs#L182-L185(this comment)docs/accuracysnes-research-dossier.md#L1402-L1402
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/rustysnes-cart/src/header.rs` around lines 182 - 185, Replace
“odd-sized dump” with “non-32 KiB-aligned dump” in
crates/rustysnes-cart/src/header.rs lines 182-185 and update the corresponding
G1.18 terminology in docs/accuracysnes-research-dossier.md line 1402; keep the
code and documentation descriptions consistent.
Source: Path instructions
| | G1.16 | ExHiROM: A23 inverted into cart A22 | | ||
| | G1.17 | SRAM mapping (Thracia 776 / Ys III) | | ||
| | G1.18 | Copier header when `filesize % 1024 == 512` | | ||
| | G1.18 | Copier header when `filesize % 1024 == 512` — **but the references implement the stricter `% 32768 == 512`** (ares/bsnes `(rom.size() & 0x7fff) == 512`); the two agree for every real cartridge image and differ only for an odd-sized dump. Measured 2026-08-01; see `rustysnes-cart/src/header.rs::detect` | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the exact size condition.
Replace “odd-sized dump” with “non-32 KiB-aligned dump” or state the two predicates explicitly. An input such as 0x8600 bytes is even but produces the documented divergence.
As per path instructions, docs are the spec and must not drift from the code they describe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/accuracysnes-research-dossier.md` at line 1402, Update the G1.18
documentation to describe the predicates accurately: replace “odd-sized dump”
with “non-32 KiB-aligned dump,” or explicitly state both size conditions and
their divergence. Keep the documented behavior aligned with
rustysnes-cart::header::detect, including even sizes such as 0x8600.
Source: Path instructions
Checked while assessing whether
G1.18was coverable. RustySNES testsrom.len() % 0x8000 == 0x200; the dossier — and the folk rule, and nocash — sayfilesize % 1024 == 512. Those are not the same predicate, and the discrepancy was undocumented on both sides.Measured against the references rather than assumed:
mia/medium/super-famicom.cpp:222)(rom.size() & 0x7fff) == 512rom.len() % 0x8000 == 0x200— the same predicatememmap.cpp:1193)size - calc_size == 512against a rounded size — looser againThe two rules agree for every real cartridge image, because a stripped image is a multiple of 32 KiB. They differ only for an odd-sized dump, where the stricter rule refuses to strip rather than shifting an image that was never prefixed.
So this is a documentation finding, not a defect. The code comment now says explicitly not to "fix" it to match the quoted rule — doing so would diverge from both bsnes-lineage references in order to match prose that no reference implements. The dossier row carries the same correction with its provenance.
Docs and one comment only; no behaviour change.
cargo clippy -p rustysnes-cart --all-targets -- -D warningsclean,copier_prefix_strippedstill passes.🤖 Generated with Claude Code
RustySNES retains the
rom.len() % 0x8000 == 0x200copier-prefix rule used by ares/bsnes. The change claims that this rule agrees withfilesize % 1024 == 512for real cartridge images but differs for odd-sized dumps. The claim is false if reference implementations use another rule or if valid cartridge images produce different results.The G1.18 dossier assertion now identifies the stricter
filesize % 32768 == 512reference rule and its relationship to the documented rule. The coverage denominator did not move.No emulator behavior changes. Odd-sized dumps remain the observable case where the existing RustySNES rule can differ from the dossier rule.